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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/09/2025 15:32, Anwar, Md Danish wrote:
> 
> 
> On 9/3/2025 6:24 PM, Krzysztof Kozlowski wrote:
>> On 03/09/2025 09:57, MD Danish Anwar wrote:
>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>> ---
>>>>>  .../devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml     | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>>>> index a492f74a8608..4dbd708ec8ee 100644
>>>>> --- a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml
>>>>> @@ -210,6 +210,12 @@ patternProperties:
>>>>>            should be defined as per the generic bindings in,
>>>>>            Documentation/devicetree/bindings/sram/sram.yaml
>>>>>  
>>>>> +      rpmsg-eth:
>>>>> +        $ref: /schemas/net/ti,rpmsg-eth.yaml
>>>>
>>>> No, not a separate device. Please read slides from my DT for beginners
>>>
>>> I had synced with Andrew and we came to the conclusion that including
>>> rpmsg-eth this way will follow the DT guidelines and should be okay.
>>
>> ... and did you check the guidelines? Instead of repeating something not
>> related to my comment rather bring argument matching the comment.
>>
>>
>> ...
>>
>>> @@ -768,6 +774,7 @@ &main_r5fss0_core0 {
>>>  	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.

Index is fixed, cannot be anything else. Cannot vary.


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

None of these are reasons to add completely new node in DT. You use
arguments of drivers in hardware description. Really, can you read the
slides I asked for already?

> 
> But the driver code won't work for this. We need to have a way to know

Driver code can easily work with this. Multiple choices from using names
up to having driver match data with index.

> which index to look for in existing memory-region which can defer from
> vendor to vendor.
> 
> So to avoid this, I thought of using a new memory region. Which will
> have only 1 entry specifically for this case, and the driver can always
> 
> 	of_parse_phandle(np, "rpmsg-eth-region", 0)
> 
> to get the memory region.

Please don't drag the discussion. Look:

Q: I need a child node for my device to instantiate Linux driver"
A: NO

Q: I need new “vendor,foo-prop” property
A: Please look at existing common properties from common schemas or
devices representing similar class

Or actually let's start with most important:

"What Could You Put into DTS?"
Answers:
1. "Not the Linux Device Driver model"
2. "No Linux driver choices"

And that's exactly what you do and how you argue.

Best regards,
Krzysztof