Message-ID: <vgnd5fbapw2hhjbbtzcmxikzprbnzo6m7dr6tqh7n4yepdlmyd@hs7lkieqtshk>
Date: Sat, 28 Sep 2024 10:00:58 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: drew@...7.com, guoren@...nel.org, wefu@...hat.com, 
	jassisinghbrar@...il.com, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu, 
	m.szyprowski@...sung.com, linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH RFC v2 1/3] mailbox: Introduce support for T-head TH1520
 Mailbox driver

On Fri, Sep 27, 2024 at 11:42:05AM +0200, Michal Wilczynski wrote:
> This driver was tested using the drm/imagination GPU driver. It was able
> to successfully power on the GPU, by passing a command through mailbox
> from E910 core to E902 that's responsible for powering up the GPU. The
> GPU driver was able to read the BVC version from control registers,
> which confirms it was successfully powered on.
> 
> [   33.957467] powervr ffef400000.gpu: [drm] loaded firmware
> powervr/rogue_36.52.104.182_v1.fw
> [   33.966008] powervr ffef400000.gpu: [drm] FW version v1.0 (build
> 6621747 OS)
> [   38.978542] powervr ffef400000.gpu: [drm] *ERROR* Firmware failed to
> boot
> 
> Though the driver still fails to boot the firmware, the mailbox driver
> works when used with the not-yet-upstreamed firmware AON driver. There
> is ongoing work to get the BXM-4-64 supported with the drm/imagination
> driver [1], though it's not completed yet.
> 
> This work is based on the driver from the vendor kernel [2].
> 
> Link: https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/2 [1]
> Link: https://github.com/revyos/thead-kernel.git [2]
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
> ---
>  MAINTAINERS                      |   1 +
>  drivers/mailbox/Kconfig          |  10 +
>  drivers/mailbox/Makefile         |   2 +
>  drivers/mailbox/mailbox-th1520.c | 551 +++++++++++++++++++++++++++++++
>  4 files changed, 564 insertions(+)
>  create mode 100644 drivers/mailbox/mailbox-th1520.c

...

> +static int th1520_mbox_startup(struct mbox_chan *chan)
> +{
> +	struct th1520_mbox_priv *priv = to_th1520_mbox_priv(chan->mbox);
> +	struct th1520_mbox_con_priv *cp = chan->con_priv;
> +	u32 data[8] = {};
> +	int mask_bit;
> +	int ret;
> +
> +	/* clear local and remote generate and info0~info7 */
> +	th1520_mbox_chan_rmw(cp, TH_1520_MBOX_GEN, 0x0, 0xff, true);
> +	th1520_mbox_chan_rmw(cp, TH_1520_MBOX_GEN, 0x0, 0xff, false);
> +	th1520_mbox_chan_wr_ack(cp, &data[7], true);
> +	th1520_mbox_chan_wr_ack(cp, &data[7], false);
> +	th1520_mbox_chan_wr_data(cp, &data[0], true);
> +	th1520_mbox_chan_wr_data(cp, &data[0], false);
> +
> +	/* enable the chan mask */
> +	mask_bit = th1520_mbox_chan_id_to_mapbit(cp);
> +	th1520_mbox_rmw(priv, TH_1520_MBOX_MASK, BIT(mask_bit), 0);
> +
> +	if (cp->type == TH_1520_MBOX_TYPE_DB)
> +		/* tx doorbell doesn't have ACK, rx doorbell requires isr */
> +		tasklet_init(&cp->txdb_tasklet, th1520_mbox_txdb_tasklet,
> +			     (unsigned long)cp);
> +
> +	ret = request_irq(priv->irq, th1520_mbox_isr,
> +			  IRQF_SHARED | IRQF_NO_SUSPEND, cp->irq_desc, chan);

Mixing devm- and non-devm with shared interrupts is error-prone (or even
discouraged). Your code looks here correct, but probably this deserves
a comment that you investigated the path and it is not possible to
trigger interrupt from another device while device is unbound.

> +	if (ret) {
> +		dev_err(priv->dev, "Unable to acquire IRQ %d\n", priv->irq);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void th1520_mbox_shutdown(struct mbox_chan *chan)
> +{
> +	struct th1520_mbox_priv *priv = to_th1520_mbox_priv(chan->mbox);
> +	struct th1520_mbox_con_priv *cp = chan->con_priv;
> +	int mask_bit;
> +
> +	/* clear the chan mask */
> +	mask_bit = th1520_mbox_chan_id_to_mapbit(cp);
> +	th1520_mbox_rmw(priv, TH_1520_MBOX_MASK, 0, BIT(mask_bit));
> +
> +	free_irq(priv->irq, chan);

Odd order. This should be reverse order from startup. Why is it not?

> +}
> +
> +static const struct mbox_chan_ops th1520_mbox_ops = {
> +	.send_data = th1520_mbox_send_data,
> +	.startup = th1520_mbox_startup,
> +	.shutdown = th1520_mbox_shutdown,
> +};
> +
> +static int th1520_mbox_init_generic(struct th1520_mbox_priv *priv)
> +{
> +#ifdef CONFIG_PM_SLEEP
> +	priv->ctx = devm_kzalloc(priv->dev, sizeof(*priv->ctx), GFP_KERNEL);
> +	if (!priv->ctx)
> +		return -ENOMEM;
> +#endif
> +	/* Set default configuration */
> +	th1520_mbox_write(priv, 0xff, TH_1520_MBOX_CLR);
> +	th1520_mbox_write(priv, 0x0, TH_1520_MBOX_MASK);
> +	return 0;
> +}
> +
> +static struct mbox_chan *th1520_mbox_xlate(struct mbox_controller *mbox,
> +					  const struct of_phandle_args *sp)
> +{
> +	struct th1520_mbox_priv *priv = to_th1520_mbox_priv(mbox);
> +	struct th1520_mbox_con_priv *cp;
> +	u32 chan, type;
> +
> +	if (sp->args_count != 2) {
> +		dev_err(mbox->dev, "Invalid argument count %d\n",
> +			sp->args_count);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	chan = sp->args[0]; /* comm remote channel */
> +	type = sp->args[1]; /* comm channel type */
> +
> +	if (chan >= mbox->num_chans) {
> +		dev_err(mbox->dev, "Not supported channel number: %d\n", chan);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (chan == priv->cur_icu_cpu_id) {
> +		dev_err(mbox->dev, "Cannot communicate with yourself\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (type > TH_1520_MBOX_TYPE_DB) {
> +		dev_err(mbox->dev, "Not supported the type for channel[%d]\n",
> +			chan);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	cp = mbox->chans[chan].con_priv;
> +	cp->type = type;
> +
> +	return &mbox->chans[chan];
> +}
> +
> +static int th1520_mbox_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct th1520_mbox_priv *priv;
> +	struct resource *res;
> +	unsigned int remote_idx = 0;
> +	unsigned int i;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	if (of_property_read_u32(np, "thead,icu-cpu-id", &priv->cur_icu_cpu_id)) {
> +		dev_err(dev, "thead,icu-cpu-id is missing\n");
> +		return -EINVAL;
> +	}
> +
> +	if (priv->cur_icu_cpu_id != TH_1520_MBOX_ICU_CPU0 &&
> +	    priv->cur_icu_cpu_id != TH_1520_MBOX_ICU_CPU3) {
> +		dev_err(dev, "thead,icu-cpu-id is invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->dev = dev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "local");
> +	priv->local_icu[TH_1520_MBOX_ICU_CPU0] = devm_ioremap_resource(dev, res);

Use proper wrapper over these two.

> +	if (IS_ERR(priv->local_icu[TH_1520_MBOX_ICU_CPU0]))
> +		return PTR_ERR(priv->local_icu[TH_1520_MBOX_ICU_CPU0]);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "remote-icu0");
> +	priv->remote_icu[0] = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->remote_icu[0]))
> +		return PTR_ERR(priv->remote_icu[0]);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "remote-icu1");
> +	priv->remote_icu[1] = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->remote_icu[1]))
> +		return PTR_ERR(priv->remote_icu[1]);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "remote-icu2");
> +	priv->remote_icu[2] = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->remote_icu[2]))
> +		return PTR_ERR(priv->remote_icu[2]);

Best regards,
Krzysztof

Powered by blists - more mailing lists