From: Md Danish Anwar <a0501179@ti.com> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, MD Danish Anwar <danishanwar@ti.com>, "Andrew F. Davis" <afd@ti.com>, Tero Kristo <t-kristo@ti.com>, Suman Anna <s-anna@ti.com>, Roger Quadros <rogerq@kernel.org>, YueHaibing <yuehaibing@huawei.com>, Vignesh Raghavendra <vigneshr@ti.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Rob Herring <robh+dt@kernel.org>, Paolo Abeni <pabeni@redhat.com>, "Jakub Kicinski" <kuba@kernel.org>, Eric Dumazet <edumazet@google.com>, "David S. Miller" <davem@davemloft.net>, <andrew@lunn.ch> Cc: <nm@ti.com>, <ssantosh@kernel.org>, <srk@ti.com>, <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>, <netdev@vger.kernel.org>, <linux-omap@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org> Subject: Re: [EXTERNAL] Re: [PATCH v3 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings Date: Wed, 1 Feb 2023 16:40:17 +0530 [thread overview] Message-ID: <703df816-e7bf-dfef-4a55-6fd784f5854c@ti.com> (raw) In-Reply-To: <b17e9a32-cfdb-223c-f500-c897061753f6@linaro.org> Hi Krzysztof, On 23/12/22 17:01, Krzysztof Kozlowski wrote: > On 23/12/2022 12:09, MD Danish Anwar wrote: >> From: Puranjay Mohan <p-mohan@ti.com> >> >> Add a YAML binding document for the ICSSG Programmable real time unit >> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs >> to interface the PRUs and load/run the firmware for supporting ethernet >> functionality. >> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com> >> Signed-off-by: Md Danish Anwar <danishanwar@ti.com> >> --- >> .../bindings/net/ti,icssg-prueth.yaml | 174 ++++++++++++++++++ >> 1 file changed, 174 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> new file mode 100644 >> index 000000000000..7659f5fd3132 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> @@ -0,0 +1,174 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/ti,icssg-prueth.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: >> + Texas Instruments ICSSG PRUSS Ethernet > > Keep it in one line. > Sure, I'll do it. >> + >> +maintainers: >> + - Puranjay Mohan <p-mohan@ti.com> >> + - Md Danish Anwar <danishanwar@ti.com> >> + >> +description: >> + Ethernet based on the Programmable Real-Time >> + Unit and Industrial Communication Subsystem. >> + >> +allOf: >> + - $ref: /schemas/remoteproc/ti,pru-consumer.yaml# >> + >> +properties: >> + compatible: >> + enum: >> + - ti,am654-icssg-prueth # for AM65x SoC family >> + >> + sram: >> + description: >> + phandle to MSMC SRAM node > > Where does the definition of this come from? If from nowhere, usually > you need vendor prefix and type/ref. > It is phandle to sram node in SoC DT file. I'll change it to ti,sram as it is TI specific. I'll also add $ref: /schemas/types.yaml#/definitions/phandle >> + >> + dmas: >> + maxItems: 10 >> + >> + dma-names: >> + items: >> + - const: tx0-0 >> + - const: tx0-1 >> + - const: tx0-2 >> + - const: tx0-3 >> + - const: tx1-0 >> + - const: tx1-1 >> + - const: tx1-2 >> + - const: tx1-3 >> + - const: rx0 >> + - const: rx1 >> + >> + ethernet-ports: >> + type: object >> + properties: >> + '#address-cells': >> + const: 1 >> + '#size-cells': >> + const: 0 >> + >> + patternProperties: >> + ^port@[0-1]$: >> + type: object >> + description: ICSSG PRUETH external ports >> + >> + $ref: ethernet-controller.yaml# >> + >> + unevaluatedProperties: false >> + additionalProperties: false > > You cannot have both. Did you test the binding? I doubt it... > Sorry, must have missed it in testing. I will remove additionalProperties and just keep unevaluatedProperties. >> + properties: >> + reg: >> + items: >> + - enum: [0, 1] >> + description: ICSSG PRUETH port number >> + >> + ti,syscon-rgmii-delay: >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: >> + phandle to system controller node and register offset >> + to ICSSG control register for RGMII transmit delay > > You need to describe the items. And in your own bindings from TI you > will even find example... > Sure. I'll describe the items for this. >> + >> + required: >> + - reg >> + >> + ti,mii-g-rt: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + phandle to MII_G_RT module's syscon regmap. >> + >> + ti,mii-rt: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + phandle to MII_RT module's syscon regmap > > Why do you have so many phandles? Aren't you missing some phys? > That is because control bits were sprinkled around System Control register so we need to use syscon regmap to access them. >> + >> + interrupts: >> + minItems: 2 > > Drop minItems > Sure. I'll drop minItems. >> + maxItems: 2 >> + description: | >> + Interrupt specifiers to TX timestamp IRQ. >> + >> + interrupt-names: >> + items: >> + - const: tx_ts0 >> + - const: tx_ts1 >> + >> +required: >> + - compatible >> + - sram >> + - ti,mii-g-rt > > Keep same order as in properties:. > Sure, I will make sure it has same order as in properties. >> + - dmas >> + - dma-names >> + - ethernet-ports >> + - interrupts >> + - interrupt-names >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + > > No need for blank line. > I'll remove this blank line. >> + /* Example k3-am654 base board SR2.0, dual-emac */ >> + pruss2_eth: pruss2_eth { > > No underscores in node names. > > Node names should be generic. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > I will change the node name 'pruss2_eth' to generic for example 'ethernet'. >> + compatible = "ti,am654-icssg-prueth"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&icssg2_rgmii_pins_default>; >> + sram = <&msmc_ram>; >> + >> + ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>, >> + <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>; >> + firmware-name = "ti-pruss/am65x-pru0-prueth-fw.elf", >> + "ti-pruss/am65x-rtu0-prueth-fw.elf", >> + "ti-pruss/am65x-txpru0-prueth-fw.elf", >> + "ti-pruss/am65x-pru1-prueth-fw.elf", >> + "ti-pruss/am65x-rtu1-prueth-fw.elf", >> + "ti-pruss/am65x-txpru1-prueth-fw.elf"; > > I really doubt it was tested... and maybe there will be no testing from > Rob's bot due to dependency. > > Please post dt_binding_check testing results. > I had run dt_binding check before posting, must have missed these errors. I will be careful to check for this errors before posting patches. I will post new revision after doing the above mentioned changes. I will make sure to do testing properly. > Best regards, > Krzysztof > -- Thanks and Regards, Danish.
WARNING: multiple messages have this Message-ID (diff)
From: Md Danish Anwar <a0501179@ti.com> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, MD Danish Anwar <danishanwar@ti.com>, "Andrew F. Davis" <afd@ti.com>, Tero Kristo <t-kristo@ti.com>, Suman Anna <s-anna@ti.com>, Roger Quadros <rogerq@kernel.org>, YueHaibing <yuehaibing@huawei.com>, Vignesh Raghavendra <vigneshr@ti.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Rob Herring <robh+dt@kernel.org>, Paolo Abeni <pabeni@redhat.com>, "Jakub Kicinski" <kuba@kernel.org>, Eric Dumazet <edumazet@google.com>, "David S. Miller" <davem@davemloft.net>, <andrew@lunn.ch> Cc: <nm@ti.com>, <ssantosh@kernel.org>, <srk@ti.com>, <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>, <netdev@vger.kernel.org>, <linux-omap@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org> Subject: Re: [EXTERNAL] Re: [PATCH v3 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings Date: Wed, 1 Feb 2023 16:40:17 +0530 [thread overview] Message-ID: <703df816-e7bf-dfef-4a55-6fd784f5854c@ti.com> (raw) In-Reply-To: <b17e9a32-cfdb-223c-f500-c897061753f6@linaro.org> Hi Krzysztof, On 23/12/22 17:01, Krzysztof Kozlowski wrote: > On 23/12/2022 12:09, MD Danish Anwar wrote: >> From: Puranjay Mohan <p-mohan@ti.com> >> >> Add a YAML binding document for the ICSSG Programmable real time unit >> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs >> to interface the PRUs and load/run the firmware for supporting ethernet >> functionality. >> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com> >> Signed-off-by: Md Danish Anwar <danishanwar@ti.com> >> --- >> .../bindings/net/ti,icssg-prueth.yaml | 174 ++++++++++++++++++ >> 1 file changed, 174 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> new file mode 100644 >> index 000000000000..7659f5fd3132 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml >> @@ -0,0 +1,174 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/ti,icssg-prueth.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: >> + Texas Instruments ICSSG PRUSS Ethernet > > Keep it in one line. > Sure, I'll do it. >> + >> +maintainers: >> + - Puranjay Mohan <p-mohan@ti.com> >> + - Md Danish Anwar <danishanwar@ti.com> >> + >> +description: >> + Ethernet based on the Programmable Real-Time >> + Unit and Industrial Communication Subsystem. >> + >> +allOf: >> + - $ref: /schemas/remoteproc/ti,pru-consumer.yaml# >> + >> +properties: >> + compatible: >> + enum: >> + - ti,am654-icssg-prueth # for AM65x SoC family >> + >> + sram: >> + description: >> + phandle to MSMC SRAM node > > Where does the definition of this come from? If from nowhere, usually > you need vendor prefix and type/ref. > It is phandle to sram node in SoC DT file. I'll change it to ti,sram as it is TI specific. I'll also add $ref: /schemas/types.yaml#/definitions/phandle >> + >> + dmas: >> + maxItems: 10 >> + >> + dma-names: >> + items: >> + - const: tx0-0 >> + - const: tx0-1 >> + - const: tx0-2 >> + - const: tx0-3 >> + - const: tx1-0 >> + - const: tx1-1 >> + - const: tx1-2 >> + - const: tx1-3 >> + - const: rx0 >> + - const: rx1 >> + >> + ethernet-ports: >> + type: object >> + properties: >> + '#address-cells': >> + const: 1 >> + '#size-cells': >> + const: 0 >> + >> + patternProperties: >> + ^port@[0-1]$: >> + type: object >> + description: ICSSG PRUETH external ports >> + >> + $ref: ethernet-controller.yaml# >> + >> + unevaluatedProperties: false >> + additionalProperties: false > > You cannot have both. Did you test the binding? I doubt it... > Sorry, must have missed it in testing. I will remove additionalProperties and just keep unevaluatedProperties. >> + properties: >> + reg: >> + items: >> + - enum: [0, 1] >> + description: ICSSG PRUETH port number >> + >> + ti,syscon-rgmii-delay: >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: >> + phandle to system controller node and register offset >> + to ICSSG control register for RGMII transmit delay > > You need to describe the items. And in your own bindings from TI you > will even find example... > Sure. I'll describe the items for this. >> + >> + required: >> + - reg >> + >> + ti,mii-g-rt: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + phandle to MII_G_RT module's syscon regmap. >> + >> + ti,mii-rt: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + phandle to MII_RT module's syscon regmap > > Why do you have so many phandles? Aren't you missing some phys? > That is because control bits were sprinkled around System Control register so we need to use syscon regmap to access them. >> + >> + interrupts: >> + minItems: 2 > > Drop minItems > Sure. I'll drop minItems. >> + maxItems: 2 >> + description: | >> + Interrupt specifiers to TX timestamp IRQ. >> + >> + interrupt-names: >> + items: >> + - const: tx_ts0 >> + - const: tx_ts1 >> + >> +required: >> + - compatible >> + - sram >> + - ti,mii-g-rt > > Keep same order as in properties:. > Sure, I will make sure it has same order as in properties. >> + - dmas >> + - dma-names >> + - ethernet-ports >> + - interrupts >> + - interrupt-names >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + > > No need for blank line. > I'll remove this blank line. >> + /* Example k3-am654 base board SR2.0, dual-emac */ >> + pruss2_eth: pruss2_eth { > > No underscores in node names. > > Node names should be generic. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > I will change the node name 'pruss2_eth' to generic for example 'ethernet'. >> + compatible = "ti,am654-icssg-prueth"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&icssg2_rgmii_pins_default>; >> + sram = <&msmc_ram>; >> + >> + ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>, >> + <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>; >> + firmware-name = "ti-pruss/am65x-pru0-prueth-fw.elf", >> + "ti-pruss/am65x-rtu0-prueth-fw.elf", >> + "ti-pruss/am65x-txpru0-prueth-fw.elf", >> + "ti-pruss/am65x-pru1-prueth-fw.elf", >> + "ti-pruss/am65x-rtu1-prueth-fw.elf", >> + "ti-pruss/am65x-txpru1-prueth-fw.elf"; > > I really doubt it was tested... and maybe there will be no testing from > Rob's bot due to dependency. > > Please post dt_binding_check testing results. > I had run dt_binding check before posting, must have missed these errors. I will be careful to check for this errors before posting patches. I will post new revision after doing the above mentioned changes. I will make sure to do testing properly. > Best regards, > Krzysztof > -- Thanks and Regards, Danish. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-02-01 11:11 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-23 11:09 [PATCH v3 0/2] Introduce ICSSG based ethernet Driver MD Danish Anwar 2022-12-23 11:09 ` MD Danish Anwar 2022-12-23 11:09 ` [PATCH v3 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings MD Danish Anwar 2022-12-23 11:09 ` MD Danish Anwar 2022-12-23 11:31 ` Krzysztof Kozlowski 2022-12-23 11:31 ` Krzysztof Kozlowski 2023-02-01 11:10 ` Md Danish Anwar [this message] 2023-02-01 11:10 ` [EXTERNAL] " Md Danish Anwar 2022-12-23 14:28 ` Andrew Lunn 2022-12-23 14:28 ` Andrew Lunn 2023-01-02 13:04 ` Roger Quadros 2023-01-02 13:04 ` Roger Quadros 2023-01-02 18:34 ` Andrew Lunn 2023-01-02 18:34 ` Andrew Lunn 2023-01-05 11:33 ` Roger Quadros 2023-01-05 11:33 ` Roger Quadros 2023-01-05 17:13 ` Andrew Lunn 2023-01-05 17:13 ` Andrew Lunn 2023-01-06 11:15 ` Roger Quadros 2023-01-06 11:15 ` Roger Quadros 2023-01-30 7:02 ` [EXTERNAL] " Md Danish Anwar 2023-01-30 7:02 ` Md Danish Anwar 2022-12-23 11:09 ` [PATCH v3 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver MD Danish Anwar 2022-12-23 11:32 ` Krzysztof Kozlowski 2022-12-23 11:32 ` Krzysztof Kozlowski 2023-01-02 12:55 ` Roger Quadros 2023-01-02 12:55 ` Roger Quadros
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=703df816-e7bf-dfef-4a55-6fd784f5854c@ti.com \ --to=a0501179@ti.com \ --cc=afd@ti.com \ --cc=andrew@lunn.ch \ --cc=danishanwar@ti.com \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=edumazet@google.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=krzysztof.kozlowski@linaro.org \ --cc=kuba@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=nm@ti.com \ --cc=pabeni@redhat.com \ --cc=robh+dt@kernel.org \ --cc=rogerq@kernel.org \ --cc=s-anna@ti.com \ --cc=srk@ti.com \ --cc=ssantosh@kernel.org \ --cc=t-kristo@ti.com \ --cc=vigneshr@ti.com \ --cc=yuehaibing@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.