Re: [RFC v2 PATCH 01/10] dt-bindings: net: ti: Adds DUAL-EMAC mode support on PRU-ICSS2 for AM57xx SOCs

[Basharath Hussain Khaja <basharath@couthit.com>]

> On Wed, Jan 29, 2025 at 10:46:52AM +0530, Basharath Hussain Khaja wrote:
>> > On Fri, Jan 24, 2025 at 05:53:44PM +0530, Basharath Hussain Khaja wrote:
>> >> From: Parvathi Pudi <parvathi@couthit.com>
>> >> 
>> >> Documentation update for the newly added "pruss2_eth" device tree
>> >> node and its dependencies along with compatibility for PRU-ICSS
>> >> Industrial Ethernet Peripheral (IEP), PRU-ICSS Enhanced Capture
>> >> (eCAP) peripheral and using YAML binding document for AM57xx SoCs.
>> >> 
>> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> >> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> >> Signed-off-by: Parvathi Pudi <parvathi@couthit.com>
>> >> Signed-off-by: Basharath Hussain Khaja <basharath@couthit.com>
>> > 
>> > I find this hard to believe. If all these people handled the patch, the
>> > signoff from Parvathi would be first, no? Should some of these people be
>> > co-developers?
>> > 
>> 
>> Changes are about multiple modules. We have added our sign-off followed by
>> original module authors.
> 
> I think what you're trying to say is that these people are
> co-developers? Anyone that contributed to the content of this patch
> needs to get a co-developed-by. If they're not co-developers, and you
> just want to put them in the maintainers section, they don't get
> sign-offs.
> 

Yes you may be right. We thought it would be good to include module
owners in signed-off-by, though it is a documentation file which was
not available earlier we have newly added. Due to that the ownership 
is with us. 

As you suggested we will clean this in the next version as below.

Signed-off-by: Parvathi Pudi <parvathi@couthit.com>
Signed-off-by: Basharath Hussain Khaja <basharath@couthit.com>

>> >> ---
>> >>  .../devicetree/bindings/net/ti,icss-iep.yaml  |   5 +
>> >>  .../bindings/net/ti,icssm-prueth.yaml         | 147 ++++++++++++++++++
>> >>  .../bindings/net/ti,pruss-ecap.yaml           |  32 ++++
>> >>  .../devicetree/bindings/soc/ti/ti,pruss.yaml  |   9 ++
>> >>  4 files changed, 193 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/net/ti,icssm-prueth.yaml
>> >>  create mode 100644 Documentation/devicetree/bindings/net/ti,pruss-ecap.yaml
>> >> 
>> >> diff --git a/Documentation/devicetree/bindings/net/ti,icss-iep.yaml
>> >> b/Documentation/devicetree/bindings/net/ti,icss-iep.yaml
>> >> index e36e3a622904..aad7d37fb47e 100644
>> >> --- a/Documentation/devicetree/bindings/net/ti,icss-iep.yaml
>> >> +++ b/Documentation/devicetree/bindings/net/ti,icss-iep.yaml
>> >> @@ -8,6 +8,8 @@ title: Texas Instruments ICSS Industrial Ethernet Peripheral
>> >> (IEP) module
>> >>  
>> >>  maintainers:
>> >>    - Md Danish Anwar <danishanwar@ti.com>
>> >> +  - Parvathi Pudi <parvathi@couthit.com>
>> >> +  - Basharath Hussain Khaja <basharath@couthit.com>
>> >>  
>> >>  properties:
>> >>    compatible:
>> >> @@ -20,6 +22,9 @@ properties:
>> >>  
>> >>        - const: ti,am654-icss-iep
>> >>  
>> >> +      - items:
>> >> +          - enum:
>> >> +              - ti,am5728-icss-iep
>> > 
>> > "items: - enum: <one item>" is the same as const.
>> > 
>> 
>> Sure, we will modify as below.
>> 
>>       - const: ti,am5728-icss-iep
>> 
>> >>  
>> >>    reg:
>> >>      maxItems: 1
>> >> diff --git a/Documentation/devicetree/bindings/net/ti,icssm-prueth.yaml
>> >> b/Documentation/devicetree/bindings/net/ti,icssm-prueth.yaml
>> >> new file mode 100644
>> >> index 000000000000..51e99beb5f5f
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/net/ti,icssm-prueth.yaml
>> >> @@ -0,0 +1,147 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/net/ti,icssm-prueth.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: Texas Instruments ICSSM PRUSS Ethernet
>> >> +
>> >> +maintainers:
>> >> +  - Roger Quadros <rogerq@ti.com>
>> >> +  - Andrew F. Davis <afd@ti.com>
>> >> +  - Parvathi Pudi <parvathi@couthit.com>
>> >> +  - Basharath Hussain Khaja <basharath@couthit.com>
>> >> +
>> >> +description:
>> >> +  Ethernet based on the Programmable Real-Time Unit and Industrial
>> >> +  Communication Subsystem.
>> >> +
>> >> +properties:
>> >> +  compatible:
>> >> +    enum:
>> >> +      - ti,am57-prueth     # for AM57x SoC family
>> >> +
>> >> +  sram:
>> >> +    $ref: /schemas/types.yaml#/definitions/phandle
>> >> +    description:
>> >> +      phandle to OCMC SRAM node
>> >> +
>> >> +  ti,mii-rt:
>> >> +    $ref: /schemas/types.yaml#/definitions/phandle
>> >> +    description:
>> >> +      phandle to MII_RT module's syscon regmap
>> >> +
>> >> +  ti,iep:
>> >> +    $ref: /schemas/types.yaml#/definitions/phandle
>> >> +    description:
>> >> +      phandle to IEP (Industrial Ethernet Peripheral) for ICSS
>> >> +
>> >> +  ecap:
>> > 
>> > Why's this one not got a ti prefix?
>> > 
>> 
>> We will add "ti" prefix to ecap as "ti,ecap" in the next version.
>> 
>> >> +    $ref: /schemas/types.yaml#/definitions/phandle
>> >> +    description:
>> >> +      phandle to Enhanced Capture (eCAP) event for ICSS
>> > 
>> > Why do you need phandles for these things, can they not be looked up by
>> > compatible? (e.g. multiple devices on one SoC).
>> > 
>> 
>> ecap is another peripheral similar to IEP in ICSSM/ICSSG. We have created a
>> separate driver for possible reuse with ICSSG in future.
> 
> That's not an answer to my question.
> 

We can use compatible if we have only one instance of a peripheral in the SOC. 
On the AM57x SOC we have two identical ICSS instances(ICSS1 and ICSS2). So we 
use phandles to differentiate between the two instances. Currently this patch 
series adds support for ICSS2 instance on the AM57x SOC. Support for ICSS1 instance 
will be added in subsequent patches.

>> 
>> >> +
>> >> +  interrupts:
>> >> +    items:
>> >> +      - description: High priority Rx Interrupt specifier.
>> > > +      - description: Low priority Rx Interrupt specifier.
>> 
>> 

Thanks & Best Regards,
Basharath