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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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