| Message ID | 20221223110930.1337536-1-danishanwar@ti.com |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
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. > + > +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. > + > + 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... > + 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... > + > + 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? > + > + interrupts: > + minItems: 2 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:. > + - dmas > + - dma-names > + - ethernet-ports > + - interrupts > + - interrupt-names > + > +unevaluatedProperties: false > + > +examples: > + - | > + No need for 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 > + 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. Best regards, Krzysztof
On 23/12/2022 12:09, MD Danish Anwar wrote: > From: Roger Quadros <rogerq@ti.com> > > This is the Ethernet driver for TI AM654 Silicon rev. 2 > with the ICSSG PRU Sub-system running dual-EMAC firmware. > (...) > + > +/* Memory Usage of : DMEM1 > + * > + */ ??? What's this? > + > +/* Memory Usage of : PA_STAT > + * > + */ Same question. > + > +/*Start of 32 bits PA_STAT counters*/ That's not a Linux coding style. Add spaces and make it readable. Best regards, Krzysztof
> + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + pruss2_emac0: port@0 { > + reg = <0>; > + phy-handle = <&pruss2_eth0_phy>; > + phy-mode = "rgmii-rxid"; That is unusual. Where are the TX delays coming from? Andrew
Hi, On 23/12/2022 13:32, Krzysztof Kozlowski wrote: > On 23/12/2022 12:09, MD Danish Anwar wrote: >> From: Roger Quadros <rogerq@ti.com> >> >> This is the Ethernet driver for TI AM654 Silicon rev. 2 >> with the ICSSG PRU Sub-system running dual-EMAC firmware. >> > > > (...) > >> + >> +/* Memory Usage of : DMEM1 >> + * >> + */ > > ??? What's this? > >> + >> +/* Memory Usage of : PA_STAT >> + * >> + */ > > Same question. > >> + >> +/*Start of 32 bits PA_STAT counters*/ > > That's not a Linux coding style. Add spaces and make it readable. The contents of this file were copied from f/w. This file needs to be reduced to only the elements that we really use in the Linux driver and coding style fixed. cheers, -roger
On 23/12/2022 16:28, Andrew Lunn wrote: >> + ethernet-ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + pruss2_emac0: port@0 { >> + reg = <0>; >> + phy-handle = <&pruss2_eth0_phy>; >> + phy-mode = "rgmii-rxid"; > > That is unusual. Where are the TX delays coming from?
On Mon, Jan 02, 2023 at 03:04:19PM +0200, Roger Quadros wrote: > > > On 23/12/2022 16:28, Andrew Lunn wrote: > >> + ethernet-ports { > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + pruss2_emac0: port@0 { > >> + reg = <0>; > >> + phy-handle = <&pruss2_eth0_phy>; > >> + phy-mode = "rgmii-rxid"; > > > > That is unusual. Where are the TX delays coming from? > > >From the below property > > + ti,syscon-rgmii-delay = <&scm_conf 0x4120>; > > The TX delay can be enabled/disabled from within the ICSSG block. > > If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID > nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled. > > This logic is in prueth_config_rgmiidelay() function in the introduced driver. What nearly every other MAC driver does is pass the phy-mode to the PHY and lets the PHY add the delays. I would recommend you do that, rather than be special and different. Andrew
On 02/01/2023 20:34, Andrew Lunn wrote: > On Mon, Jan 02, 2023 at 03:04:19PM +0200, Roger Quadros wrote: >> >> >> On 23/12/2022 16:28, Andrew Lunn wrote: >>>> + ethernet-ports { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + pruss2_emac0: port@0 { >>>> + reg = <0>; >>>> + phy-handle = <&pruss2_eth0_phy>; >>>> + phy-mode = "rgmii-rxid"; >>> >>> That is unusual. Where are the TX delays coming from? >> >> >From the below property >> >> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>; >> >> The TX delay can be enabled/disabled from within the ICSSG block. >> >> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID >> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled. >> >> This logic is in prueth_config_rgmiidelay() function in the introduced driver. > > What nearly every other MAC driver does is pass the phy-mode to the > PHY and lets the PHY add the delays. I would recommend you do that, > rather than be special and different. If I remember right we couldn't disable MAC TX delay on some earlier silicon so had to take this route. I don't remember why we couldn't disable it though. In more recent Silicon Manuals I do see that MAC TX delay can be enabled/disabled. If this really is the case then we should change to phy-mode = "rgmii-id"; And let PHY handle the TX+RX delays. Danish, could you please make the change and test if it works on current silicon? cheers, -roger
> >> On 23/12/2022 16:28, Andrew Lunn wrote: > >>>> + ethernet-ports { > >>>> + #address-cells = <1>; > >>>> + #size-cells = <0>; > >>>> + pruss2_emac0: port@0 { > >>>> + reg = <0>; > >>>> + phy-handle = <&pruss2_eth0_phy>; > >>>> + phy-mode = "rgmii-rxid"; > >>> > >>> That is unusual. Where are the TX delays coming from? > >> > >> >From the below property > >> > >> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>; > >> > >> The TX delay can be enabled/disabled from within the ICSSG block. > >> > >> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID > >> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled. > >> > >> This logic is in prueth_config_rgmiidelay() function in the introduced driver. > > > > What nearly every other MAC driver does is pass the phy-mode to the > > PHY and lets the PHY add the delays. I would recommend you do that, > > rather than be special and different. > > > If I remember right we couldn't disable MAC TX delay on some earlier silicon > so had to take this route. I don't remember why we couldn't disable it though. > > In more recent Silicon Manuals I do see that MAC TX delay can be enabled/disabled. > If this really is the case then we should change to > > phy-mode = "rgmii-id"; > > And let PHY handle the TX+RX delays. DT describes the board. PHY mode indicates what delays the board requires, because the board itself is not performing the delays by using extra long lines. So typically, phy-mode is rgmii-id, indicating delays need to be added somewhere in both directions. Who adds the delays is then between the MAC and the PHY. In most cases, the MAC does nothing, and passes phy-mode to the PHY and the PHY does it. But it is also possible for the MAC to do the delay. So if you cannot actually disable the TX delay in the MAC, that is O.K. But you need to modify phy-mode you pass to the PHY to indicate the MAC is doing the delay, otherwise the PHY will additionally do the delay. So your DT will contain rgmii-id, because that is what the board requires, but the MAC will pass rmgii-rxid to the PHY, since that is what the PHY needs to add. Andrew
On 05/01/2023 19:13, Andrew Lunn wrote: >>>> On 23/12/2022 16:28, Andrew Lunn wrote: >>>>>> + ethernet-ports { >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <0>; >>>>>> + pruss2_emac0: port@0 { >>>>>> + reg = <0>; >>>>>> + phy-handle = <&pruss2_eth0_phy>; >>>>>> + phy-mode = "rgmii-rxid"; >>>>> >>>>> That is unusual. Where are the TX delays coming from? >>>> >>>> >From the below property >>>> >>>> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>; >>>> >>>> The TX delay can be enabled/disabled from within the ICSSG block. >>>> >>>> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID >>>> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled. >>>> >>>> This logic is in prueth_config_rgmiidelay() function in the introduced driver. >>> >>> What nearly every other MAC driver does is pass the phy-mode to the >>> PHY and lets the PHY add the delays. I would recommend you do that, >>> rather than be special and different. >> >> >> If I remember right we couldn't disable MAC TX delay on some earlier silicon >> so had to take this route. I don't remember why we couldn't disable it though. >> >> In more recent Silicon Manuals I do see that MAC TX delay can be enabled/disabled. >> If this really is the case then we should change to >> >> phy-mode = "rgmii-id"; >> >> And let PHY handle the TX+RX delays. > > DT describes the board. PHY mode indicates what delays the board > requires, because the board itself is not performing the delays by > using extra long lines. So typically, phy-mode is rgmii-id, indicating > delays need to be added somewhere in both directions. > > Who adds the delays is then between the MAC and the PHY. In most > cases, the MAC does nothing, and passes phy-mode to the PHY and the > PHY does it. > > But it is also possible for the MAC to do the delay. So if you cannot > actually disable the TX delay in the MAC, that is O.K. But you need to > modify phy-mode you pass to the PHY to indicate the MAC is doing the > delay, otherwise the PHY will additionally do the delay. So your DT > will contain rgmii-id, because that is what the board requires, but > the MAC will pass rmgii-rxid to the PHY, since that is what the PHY > needs to add. Thanks for the explanation. :) cheers, -roger
Hi Roger, On 05/01/23 17:03, Roger Quadros wrote: > On 02/01/2023 20:34, Andrew Lunn wrote: >> On Mon, Jan 02, 2023 at 03:04:19PM +0200, Roger Quadros wrote: >>> >>> >>> On 23/12/2022 16:28, Andrew Lunn wrote: >>>>> + ethernet-ports { >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + pruss2_emac0: port@0 { >>>>> + reg = <0>; >>>>> + phy-handle = <&pruss2_eth0_phy>; >>>>> + phy-mode = "rgmii-rxid"; >>>> >>>> That is unusual. Where are the TX delays coming from? >>> >>> >From the below property >>> >>> + ti,syscon-rgmii-delay = <&scm_conf 0x4120>; >>> >>> The TX delay can be enabled/disabled from within the ICSSG block. >>> >>> If this property exists and PHY mode is neither PHY_INTERFACE_MODE_RGMII_ID >>> nor PHY_INTERFACE_MODE_RGMII_TXID then the internal delay is enabled. >>> >>> This logic is in prueth_config_rgmiidelay() function in the introduced driver. >> >> What nearly every other MAC driver does is pass the phy-mode to the >> PHY and lets the PHY add the delays. I would recommend you do that, >> rather than be special and different. > > > If I remember right we couldn't disable MAC TX delay on some earlier silicon > so had to take this route. I don't remember why we couldn't disable it though. > > In more recent Silicon Manuals I do see that MAC TX delay can be enabled/disabled. > If this really is the case then we should change to > > phy-mode = "rgmii-id"; > > And let PHY handle the TX+RX delays. > > Danish, > could you please make the change and test if it works on current silicon? > I changed the phy-mode to "rgmii-id" instead of "rgmii-rxid". I did the testing on current silicon (AM654x SR 2.0) and it is working fine. > cheers, > -roger Thanks and Regards, Danish.
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 >