Re: [PATCH net-next v2 2/8] dt-bindings: remoteproc: k3-r5f: Add rpmsg-eth subnode

[MD Danish Anwar <danishanwar@ti.com>]

On 03/09/25 7:53 pm, Krzysztof Kozlowski wrote:
> On 03/09/2025 16:06, Andrew Lunn wrote:
>>>>>  	mboxes = <&mailbox0_cluster2 &mbox_main_r5fss0_core0>;
>>>>>  	memory-region = <&main_r5fss0_core0_dma_memory_region>,
>>>>>  			<&main_r5fss0_core0_memory_region>;
>>>>> +	rpmsg-eth-region = <&main_r5fss0_core0_memory_region_shm>;
>>>>
>>>> You already have here memory-region, so use that one.
>>>>
>>>
>>> There is a problem with using memory-region. If I add
>>> `main_r5fss0_core0_memory_region_shm` to memory region, to get this
>>> phandle from driver I would have to use
>>> 	
>>> 	of_parse_phandle(np, "memory-region", 2)
>>>
>>> Where 2 is the index for this region. But the problem is how would the
>>> driver know this index. This index can vary for different vendors and
>>> their rproc device.
>>>
>>> If some other vendor tries to use this driver but their memory-region
>>> has 3 existing entries. so this this entry will be the 4th one.
>>
>> Just adding to this, there is nothing really TI specific in this
>> system. We want the design so that any vendor can use it, just by
>> adding the needed nodes to their rpmsg node, indicating there is a
>> compatible implementation on the other end, and an indication of where
>> the shared memory is.
> 
> I don't know your drivers, but I still do not see here a problem with
> 'memory-region'. You just need to tell this common code which
> memory-region phandle by index or name is the one for rpmsg.
> 

I am able to pass this index as driver_data in the `rpmsg_device_id`.

I can work with just adding this reserved memory region to the
'memory-region'. No need to create additional node in dt.

This will be the code,

static const struct rpmsg_eth_data ti_rpmsg_eth_data = {
	.shm_region_index = 2,
};

static struct rpmsg_device_id rpmsg_eth_rpmsg_id_table[] = {
	{ .name = "ti.shm-eth", .driver_data =
(kernel_ulong_t)&ti_rpmsg_eth_data },
	{},
};

Other vendors can add separate entry to rpmsg_eth_rpmsg_id_table with
their index based on their device tree.

I am keeping the data name `ti_rpmsg_eth_data` vendor specific so that
other vendors can create their data filed and add entry to
`rpmsg_eth_rpmsg_id_table` with driver_data pointing to their data
structure.

Andrew, does this sound ok to you. I will send out a v3 once you confirm.

Krzysztof, Andrew thanks for the feedback.

-- 
Thanks and Regards,
Danish