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

AngeloGioacchino Del Regno
2025-09-08T12:34:04Z

Il 08/09/25 14:05, Nicolas Frattaroli ha scritto:
> 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.)

I guess assuming people know that compilers will auto-optimize that is okay then.

>
>> [...]
>>
>>> +
>>> + 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".
>

Heh, didn't see that comment. data and ctl are also fine for me, go with those :-)

>>
>> 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.

Perfect.

Cheers,
Angelo