Re: [PATCH v8 10/20] soc: mediatek: mtk-cmdq: Extend cmdq_pkt_write API for SoCs without subsys ID

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

Il 23/10/25 06:03, Jason-JH Lin (林睿祥) ha scritto:
> On Mon, 2025-10-20 at 12:04 +0200, AngeloGioacchino Del Regno wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Il 17/10/25 08:44, Jason-JH Lin ha scritto:
>>> This patch extends the cmdq_pkt_write API to support SoCs that do
>>> not
>>> have subsys ID mapping by introducing new register write APIs:
>>> - cmdq_pkt_write_pa() and cmdq_pkt_write_subsys() replace
>>>     cmdq_pkt_write()
>>> - cmdq_pkt_write_mask_pa() and cmdq_pkt_write_mask_subsys() replace
>>>     cmdq_pkt_write_mask()
>>>
>>> To ensure consistent function pointer interfaces, both
>>> cmdq_pkt_write_pa() and cmdq_pkt_write_subsys() provide subsys and
>>> pa_base parameters. This unifies how register writes are invoked,
>>> regardless of whether subsys ID is supported by the device.
>>>
>>> All GCEs support writing registers by PA (with mask) without
>>> subsys,
>>> but this requires extra GCE instructions to convert the PA into a
>>> GCE
>>> readable format, reducing performance compared to using subsys
>>> directly.
>>> Therefore, subsys is preferred for register writes when available.
>>>
>>> API documentation and function pointer declarations in
>>> cmdq_client_reg
>>> have been updated. The original write APIs will be removed after
>>> all
>>> CMDQ users transition to the new interfaces.
>>>
>>> Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
>>> ---
>>>    drivers/soc/mediatek/mtk-cmdq-helper.c | 54 +++++++++++++++++
>>>    include/linux/soc/mediatek/mtk-cmdq.h  | 83
>>> ++++++++++++++++++++++++++
>>>    2 files changed, 137 insertions(+)
>>>
>>
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
>>> b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index 154d0511a0ad..f6dc43c036bd 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -57,6 +57,10 @@ struct cmdq_client_reg {
>>>        phys_addr_t pa_base;
>>>        u16 offset;
>>>        u16 size;
>>> +     int (*reg_write)(struct cmdq_pkt *pkt, u8 subsys, u32
>>> pa_base,
>>> +                      u16 offset, u32 value);
>>
>> (*pkt_write)
>>
>>> +     int (*reg_write_mask)(struct cmdq_pkt *pkt, u8 subsys, u32
>>> pa_base,
>>> +                           u16 offset, u32 value, u32 mask);
>>
>> (*pkt_write_mask)
>>
>> those names make a lot more sense.
>>
> Hi Angelo,
> 
> The reason why I use reg_write/reg_write_mask is to imply these APIs
> only provide writing HW register address function, not writing DRAM
> address.
> So we don't need to care about mminfra_offset in these APIs.

Sure I understand that we don't need to care about mminfra_offset - but those
function pointers are effectively replacing the "(xyz)pkt_write" functions.

Changing the name to reg_write will create a lot of confusion.

> 
> I can add comment for this.
> 
> What do you think?
> Or should I change its name to pkt_write/pkt_write_amsk?

Please change the name to pkt_write/pkt_write_mask and if you think it's useful
also add a comment saying that those functions are already accounting for the
mminfra_offset internally.

Cheers,
Angelo

> 
> Regards,
> Jason-JH Lin
> 
>> After applying the requested changes,
>>
>> Reviewed-by: AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@collabora.com>
>>
>>>    };
>