Re: [PATCH RFC 05/10] mailbox: add MediaTek GPUEB IPI mailbox

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Monday, 8 September 2025 12:06:01 Central European Summer Time AngeloGioacchino Del Regno wrote:
> Il 05/09/25 12:23, Nicolas Frattaroli ha scritto:
> > The MT8196 SoC uses an embedded MCU to control frequencies and power of
> > the GPU. This controller is referred to as "GPUEB".
> > 
> > It communicates to the application processor, among other ways, through
> > a mailbox.
> > 
> > The mailbox exposes one interrupt, which appears to only be fired when a
> > response is received, rather than a transaction is completed. For us,
> > this means we unfortunately need to poll for txdone.
> > 
> > The mailbox also requires the EB clock to be on when touching any of the
> > mailbox registers.
> > 
> > Add a simple driver for it based on the common mailbox framework.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 
> Only a few nits in this, check below.
> 
> [...]
> > +
> > +static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
> > +{
> > +	struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> > +	unsigned int *num = chan->con_priv;
> > +	int i;
> 
> int i, j;
> 
> > +	u32 *values = data;
> > +
> > +	if (*num >= ebm->v->num_channels)
> > +		return -ECHRNG;
> > +
> > +	if (!ebm->v->channels[*num].no_response &&
> > +	    atomic_read(&ebm->rx_status[*num]))
> > +		return -EBUSY;
> > +
> > +	writel(BIT(*num), ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > +
> > +	/*
> > +	 * We don't want any fancy nonsense, just write the 32-bit values in
> > +	 * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
> > +	 */
> > +	for (i = 0; i < ebm->v->channels[*num].tx_len; i += 4) {
> 
> Just use an additional `j` index, so that you can avoid division.

The `/ 4` division here is equivalent to a `>> 2` which comes free with
almost every instruction on arm64, I don't think having two separate
indices makes the code any clearer? Unless I misunderstand how you'd
want me to use j here.

Like this?

  j = 0;
  for (i = 0; i < ebm->v->channels[*num].tx_len; i += 4) {
    writel(values[j++], ebm->mbox_mmio + ebm->v->channels[*num].tx_offset + i);
  }

This makes the relationship between the values index and i less clear. (And
in my rendition, assumes the reader knows how postincrement works, but I
think assuming people know C is fine.)

> [...]
>
> > +
> > +	ebm->clk = devm_clk_get_enabled(ebm->dev, NULL);
> > +	if (IS_ERR(ebm->clk))
> > +		return dev_err_probe(ebm->dev, PTR_ERR(ebm->clk),
> > +				     "Failed to get 'eb' clock\n");
> > +
> > +	ebm->mbox_mmio = devm_platform_ioremap_resource_byname(pdev, "mbox");
> 
> I'd say that "chan" and "ctl" are more descriptive as resource names, but then,
> do we really need to search by name?

In the binding, it was proposed to change "mbox" to something like "data",
which is fine by me, and to drop the "mbox" prefix of "ctl".

> 
> Doing that by index is also an option, as you can write the MMIO names and their
> full description in the bindings instead.

Yeah in the driver I think I'll switch to doing indices until some second
compatible forces us to actually rely on names because it adds a bunch of
other ranges.

> [...]

thanks for the feedback, assume that anything I didn't directly respond
to will be fixed in the next revision.

Kind regards,
Nicolas Frattaroli