| Message ID | 20231120160740.3532848-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 20/11/2023 17:07, Niklas Söderlund wrote: > Add bindings for Renesas R-Car Ethernet TSN End-station IP. The RTSN > device provides Ethernet network. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC (and consider --no-git-fallback argument). It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Why do you decide to skip some maintainers? > --- > .../bindings/net/renesas,ethertsn.yaml | 133 ++++++++++++++++++ > 1 file changed, 133 insertions(+) A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings. > create mode 100644 Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > > diff --git a/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > new file mode 100644 > index 000000000000..255c8f3a5a3b > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > @@ -0,0 +1,133 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/renesas,ethertsn.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Renesas Ethernet TSN End-station > + > +maintainers: > + - Niklas Söderlund <niklas.soderlund@ragnatech.se> > + > +description: > + The RTSN device provides Ethernet network using a 10 Mbps, 100 Mbps, or 1 > + Gbps full-duplex link via MII/GMII/RMII/RGMII. Depending on the connected PHY. > + > +properties: > + compatible: > + oneOf: > + - items: Drop items. I assume you have oneOf above because you predict this will grow with entries with fallbacks? If not, drop. > + - enum: > + - renesas,ethertsn-r8a779g0 # R-Car V4H > + > + reg: > + items: > + - description: TSN End Station target > + - description: generalized Precision Time Protocol target > + > + reg-names: > + items: > + - const: tsnes > + - const: gptp > + > + interrupts: > + items: > + - description: TX data interrupt > + - description: RX data interrupt > + > + interrupt-names: > + items: > + - const: tx_data tx > + - const: rx_data rx > + > + clocks: > + maxItems: 1 > + > + power-domains: > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > + phy-mode: > + contains: > + enum: > + - mii > + - rgmii > + > + phy-handle: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Specifies a reference to a node representing a PHY device. You miss top-level $ref to ethernet controller > + > + renesas,rx-internal-delay: > + type: boolean > + description: > + Enable internal Rx clock delay, typically 1.8ns. Why this is bool, not delay in ns? Why this is property of a board (not SoC)? > + > + renesas,tx-internal-delay: > + type: boolean > + description: > + Enable internal Tx clock delay, typically 2.0ns. Same questions. > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > +patternProperties: > + "^ethernet-phy@[0-9a-f]$": > + type: object > + $ref: ethernet-phy.yaml# Missing unevaluatedProperties. Open existing bindings and look how it is done there. Don't create something different. Best regards, Krzysztof
On Tue, Nov 21, 2023 at 8:45 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 20/11/2023 17:07, Niklas Söderlund wrote: > > Add bindings for Renesas R-Car Ethernet TSN End-station IP. The RTSN > > device provides Ethernet network. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > > @@ -0,0 +1,133 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/net/renesas,ethertsn.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Renesas Ethernet TSN End-station > > + > > +maintainers: > > + - Niklas Söderlund <niklas.soderlund@ragnatech.se> > > + > > +description: > > + The RTSN device provides Ethernet network using a 10 Mbps, 100 Mbps, or 1 > > + Gbps full-duplex link via MII/GMII/RMII/RGMII. Depending on the connected PHY. > > + > > +properties: > > + compatible: > > + oneOf: > > + - items: > > Drop items. > > I assume you have oneOf above because you predict this will grow with > entries with fallbacks? If not, drop. > > > + - enum: > > + - renesas,ethertsn-r8a779g0 # R-Car V4H renesas,r8a779g0-ethertsn R-Car S4 also has EtherTSN. Is it identical, so it makes sense to add a renesas,rcar-gen4-ethertsn fallback? > > + renesas,rx-internal-delay: > > + type: boolean > > + description: > > + Enable internal Rx clock delay, typically 1.8ns. > > Why this is bool, not delay in ns? > Why this is property of a board (not SoC)? Standard property is rx-internal-delay-ps. > > + > > + renesas,tx-internal-delay: > > + type: boolean > > + description: > > + Enable internal Tx clock delay, typically 2.0ns. > > Same questions. Standard property is tx-internal-delay-ps. Gr{oetje,eeting}s, Geert
On 21/11/2023 09:00, Geert Uytterhoeven wrote: > On Tue, Nov 21, 2023 at 8:45 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 20/11/2023 17:07, Niklas Söderlund wrote: >>> Add bindings for Renesas R-Car Ethernet TSN End-station IP. The RTSN >>> device provides Ethernet network. >>> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml >>> @@ -0,0 +1,133 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/net/renesas,ethertsn.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Renesas Ethernet TSN End-station >>> + >>> +maintainers: >>> + - Niklas Söderlund <niklas.soderlund@ragnatech.se> >>> + >>> +description: >>> + The RTSN device provides Ethernet network using a 10 Mbps, 100 Mbps, or 1 >>> + Gbps full-duplex link via MII/GMII/RMII/RGMII. Depending on the connected PHY. >>> + >>> +properties: >>> + compatible: >>> + oneOf: >>> + - items: >> >> Drop items. >> >> I assume you have oneOf above because you predict this will grow with >> entries with fallbacks? If not, drop. >> >>> + - enum: >>> + - renesas,ethertsn-r8a779g0 # R-Car V4H > > renesas,r8a779g0-ethertsn You can try to make a schema for this. See for examples: Documentation/devicetree/bindings/arm/qcom-soc.yaml > > R-Car S4 also has EtherTSN. > Is it identical, so it makes sense to add a renesas,rcar-gen4-ethertsn > fallback? > >>> + renesas,rx-internal-delay: >>> + type: boolean >>> + description: >>> + Enable internal Rx clock delay, typically 1.8ns. >> >> Why this is bool, not delay in ns? >> Why this is property of a board (not SoC)? > > Standard property is rx-internal-delay-ps. Thanks. Best regards, Krzysztof
Hi Krzysztof, On Tue, Nov 21, 2023 at 9:40 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 21/11/2023 09:00, Geert Uytterhoeven wrote: > > On Tue, Nov 21, 2023 at 8:45 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> On 20/11/2023 17:07, Niklas Söderlund wrote: > >>> Add bindings for Renesas R-Car Ethernet TSN End-station IP. The RTSN > >>> device provides Ethernet network. > >>> > >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > >>> + - enum: > >>> + - renesas,ethertsn-r8a779g0 # R-Car V4H > > > > renesas,r8a779g0-ethertsn > > You can try to make a schema for this. See for examples: > Documentation/devicetree/bindings/arm/qcom-soc.yaml Thanks for the pointer! Added to (long) TODO list... Gr{oetje,eeting}s, Geert
Hi Krzysztof, Thanks for your review. On 2023-11-21 08:45:29 +0100, Krzysztof Kozlowski wrote: > On 20/11/2023 17:07, Niklas Söderlund wrote: > > Add bindings for Renesas R-Car Ethernet TSN End-station IP. The RTSN > > device provides Ethernet network. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Please use scripts/get_maintainers.pl to get a list of necessary people > and lists to CC (and consider --no-git-fallback argument). It might > happen, that command when run on an older kernel, gives you outdated > entries. Therefore please be sure you base your patches on recent Linux > kernel. > > Why do you decide to skip some maintainers? Sorry will do for v2. > > > --- > > .../bindings/net/renesas,ethertsn.yaml | 133 ++++++++++++++++++ > > 1 file changed, 133 insertions(+) > > A nit, subject: drop second/last, redundant "bindings for". The > "dt-bindings" prefix is already stating that these are bindings. Ack. > > > create mode 100644 Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > > > > diff --git a/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > > new file mode 100644 > > index 000000000000..255c8f3a5a3b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > > @@ -0,0 +1,133 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/net/renesas,ethertsn.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Renesas Ethernet TSN End-station > > + > > +maintainers: > > + - Niklas Söderlund <niklas.soderlund@ragnatech.se> > > + > > +description: > > + The RTSN device provides Ethernet network using a 10 Mbps, 100 Mbps, or 1 > > + Gbps full-duplex link via MII/GMII/RMII/RGMII. Depending on the connected PHY. > > + > > +properties: > > + compatible: > > + oneOf: > > + - items: > > Drop items. Ack. > > I assume you have oneOf above because you predict this will grow with > entries with fallbacks? If not, drop. As Geert points out I will add a fallback here and somewhen add support for R-Car S4 could be added. > > > + - enum: > > + - renesas,ethertsn-r8a779g0 # R-Car V4H > > + > > + reg: > > + items: > > + - description: TSN End Station target > > + - description: generalized Precision Time Protocol target > > + > > + reg-names: > > + items: > > + - const: tsnes > > + - const: gptp > > + > > + interrupts: > > + items: > > + - description: TX data interrupt > > + - description: RX data interrupt > > + > > + interrupt-names: > > + items: > > + - const: tx_data > > tx > > > + - const: rx_data > > rx Ack. > > > + > > + clocks: > > + maxItems: 1 > > + > > + power-domains: > > + maxItems: 1 > > + > > + resets: > > + maxItems: 1 > > + > > + phy-mode: > > + contains: > > + enum: > > + - mii > > + - rgmii > > + > > + phy-handle: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + Specifies a reference to a node representing a PHY device. > > You miss top-level $ref to ethernet controller Sorry I don't fully understand what you are asking here, there are a few things about bindings I still need to learn. Looking at other bindings some have a maintainers: .. allOf: - $ref: ethernet-controller.yaml# properties: .. Is it this all ollOff node I'm missing? > > > + > > + renesas,rx-internal-delay: > > + type: boolean > > + description: > > + Enable internal Rx clock delay, typically 1.8ns. > > Why this is bool, not delay in ns? The TSN is only capable of enabling or disable internal delays, not set how long the delay is. The documentation states that the delay depends on the electronic characteristics of the particular board, but states that they typically are 1.8ns for Rx and 2.0ns for Tx. I looked at the generic properties {rx,tx}-internal-delay-ps but they are of int type. So I opted for a vendor specific bool property. Do you think a better route is to use the generic property and force the value to be either 0 or the typical delay? > Why this is property of a board (not SoC)? I'm sorry I don't understand this question. > > > + > > + renesas,tx-internal-delay: > > + type: boolean > > + description: > > + Enable internal Tx clock delay, typically 2.0ns. > > Same questions. > > > + > > + '#address-cells': > > + const: 1 > > + > > + '#size-cells': > > + const: 0 > > + > > +patternProperties: > > + "^ethernet-phy@[0-9a-f]$": > > + type: object > > + $ref: ethernet-phy.yaml# > > Missing unevaluatedProperties. Open existing bindings and look how it is > done there. Don't create something different. Ack. > > > > Best regards, > Krzysztof >
Hi Geert, Thanks for your review. On 2023-11-21 09:00:41 +0100, Geert Uytterhoeven wrote: > On Tue, Nov 21, 2023 at 8:45 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > On 20/11/2023 17:07, Niklas Söderlund wrote: > > > Add bindings for Renesas R-Car Ethernet TSN End-station IP. The RTSN > > > device provides Ethernet network. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml > > > @@ -0,0 +1,133 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/net/renesas,ethertsn.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Renesas Ethernet TSN End-station > > > + > > > +maintainers: > > > + - Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > + > > > +description: > > > + The RTSN device provides Ethernet network using a 10 Mbps, 100 Mbps, or 1 > > > + Gbps full-duplex link via MII/GMII/RMII/RGMII. Depending on the connected PHY. > > > + > > > +properties: > > > + compatible: > > > + oneOf: > > > + - items: > > > > Drop items. > > > > I assume you have oneOf above because you predict this will grow with > > entries with fallbacks? If not, drop. > > > > > + - enum: > > > + - renesas,ethertsn-r8a779g0 # R-Car V4H > > renesas,r8a779g0-ethertsn > > R-Car S4 also has EtherTSN. > Is it identical, so it makes sense to add a renesas,rcar-gen4-ethertsn > fallback? Thanks. Will address both suggestions in v2. > > > > + renesas,rx-internal-delay: > > > + type: boolean > > > + description: > > > + Enable internal Rx clock delay, typically 1.8ns. > > > > Why this is bool, not delay in ns? > > Why this is property of a board (not SoC)? > > Standard property is rx-internal-delay-ps. > > > > + > > > + renesas,tx-internal-delay: > > > + type: boolean > > > + description: > > > + Enable internal Tx clock delay, typically 2.0ns. > > > > Same questions. > > Standard property is tx-internal-delay-ps. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On 21/11/2023 13:10, Niklas Söderlund wrote: >>> + >>> + phy-handle: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: >>> + Specifies a reference to a node representing a PHY device. >> >> You miss top-level $ref to ethernet controller > > Sorry I don't fully understand what you are asking here, there are a few > things about bindings I still need to learn. Looking at other bindings > some have a > > maintainers: > .. > > allOf: > - $ref: ethernet-controller.yaml# This one. > > properties: > .. > > Is it this all ollOff node I'm missing? > >> >>> + >>> + renesas,rx-internal-delay: >>> + type: boolean >>> + description: >>> + Enable internal Rx clock delay, typically 1.8ns. >> >> Why this is bool, not delay in ns? > > The TSN is only capable of enabling or disable internal delays, not set > how long the delay is. The documentation states that the delay depends > on the electronic characteristics of the particular board, but states > that they typically are 1.8ns for Rx and 2.0ns for Tx. I don't understand that part. If you cannot configure the internal delay, how could it depend on the board characteristics? > > I looked at the generic properties {rx,tx}-internal-delay-ps but they > are of int type. So I opted for a vendor specific bool property. Do you > think a better route is to use the generic property and force the value > to be either 0 or the typical delay? > >> Why this is property of a board (not SoC)? > > I'm sorry I don't understand this question. Why setting internal delay is specific to a board, not to a SoC? Why each board would need to configure it? On which parts of hardware on the board does this depend? Best regards, Krzysztof
Hello Krzysztof, On 2023-11-21 13:20:54 +0100, Krzysztof Kozlowski wrote: > On 21/11/2023 13:10, Niklas Söderlund wrote: > >>> + > >>> + renesas,rx-internal-delay: > >>> + type: boolean > >>> + description: > >>> + Enable internal Rx clock delay, typically 1.8ns. > >> > >> Why this is bool, not delay in ns? > > > > The TSN is only capable of enabling or disable internal delays, not set > > how long the delay is. The documentation states that the delay depends > > on the electronic characteristics of the particular board, but states > > that they typically are 1.8ns for Rx and 2.0ns for Tx. > > I don't understand that part. If you cannot configure the internal > delay, how could it depend on the board characteristics? Each of these two properties reflect a single bit in the device configuration space. If the bit is set the {Rx,Tx} delay mode is active or disabled. The documentation for the bit simply states, Tx clock internal Delay Mode This bit can add internal Tx clock delay typ 2.0ns*. *Refer to Electrical Characteristics for details. Same paragraph for Rx but a typical 1.8ns delay. > > > > > I looked at the generic properties {rx,tx}-internal-delay-ps but they > > are of int type. So I opted for a vendor specific bool property. Do you > > think a better route is to use the generic property and force the value > > to be either 0 or the typical delay? > > > >> Why this is property of a board (not SoC)? > > > > I'm sorry I don't understand this question. > > Why setting internal delay is specific to a board, not to a SoC? Why > each board would need to configure it? On which parts of hardware on the > board does this depend? Ahh, I think I understand. It is per board as I understand the documentation. It depends on the electrical characteristics of the board. Maybe updating the description would help, how about? Enable internal Rx clock delay. Typically 1.8ns but depends on electrical characteristics of the board.
On 21/11/2023 13:44, Niklas Söderlund wrote: >> >>> >>> I looked at the generic properties {rx,tx}-internal-delay-ps but they >>> are of int type. So I opted for a vendor specific bool property. Do you >>> think a better route is to use the generic property and force the value >>> to be either 0 or the typical delay? >>> >>>> Why this is property of a board (not SoC)? >>> >>> I'm sorry I don't understand this question. >> >> Why setting internal delay is specific to a board, not to a SoC? Why >> each board would need to configure it? On which parts of hardware on the >> board does this depend? > > Ahh, I think I understand. It is per board as I understand the > documentation. It depends on the electrical characteristics of the > board. > > Maybe updating the description would help, how about? > > Enable internal Rx clock delay. Typically 1.8ns but depends on > electrical characteristics of the board. Yes, this looks fine. Best regards, Krzysztof
Hi Niklas, On Tue, Nov 21, 2023 at 1:44 PM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > On 2023-11-21 13:20:54 +0100, Krzysztof Kozlowski wrote: > > On 21/11/2023 13:10, Niklas Söderlund wrote: > > >>> + > > >>> + renesas,rx-internal-delay: > > >>> + type: boolean > > >>> + description: > > >>> + Enable internal Rx clock delay, typically 1.8ns. > > >> > > >> Why this is bool, not delay in ns? > > > > > > The TSN is only capable of enabling or disable internal delays, not set > > > how long the delay is. The documentation states that the delay depends > > > on the electronic characteristics of the particular board, but states > > > that they typically are 1.8ns for Rx and 2.0ns for Tx. > > > > I don't understand that part. If you cannot configure the internal > > delay, how could it depend on the board characteristics? > > Each of these two properties reflect a single bit in the device > configuration space. If the bit is set the {Rx,Tx} delay mode is active > or disabled. The documentation for the bit simply states, > > Tx clock internal Delay Mode > > This bit can add internal Tx clock delay typ 2.0ns*. > > *Refer to Electrical Characteristics for details. > > Same paragraph for Rx but a typical 1.8ns delay. > > > > I looked at the generic properties {rx,tx}-internal-delay-ps but they > > > are of int type. So I opted for a vendor specific bool property. Do you > > > think a better route is to use the generic property and force the value > > > to be either 0 or the typical delay? This is not dissimilar from EtherAVB, where the hardware also supports only a single bit, and whose DT bindings have: rx-internal-delay-ps: enum: [0, 1800] tx-internal-delay-ps: enum: [0, 2000] (with additional restrictions depending on the SoC, as on some SoCs the bits cannot be changed). > > >> Why this is property of a board (not SoC)? > > > > > > I'm sorry I don't understand this question. > > > > Why setting internal delay is specific to a board, not to a SoC? Why > > each board would need to configure it? On which parts of hardware on the > > board does this depend? > > Ahh, I think I understand. It is per board as I understand the > documentation. It depends on the electrical characteristics of the > board. Exactly. These bits (and also similar bits in the PHY) are used to adapt signaling to the board trace lengths between MAC (on-SoC) and PHY. Gr{oetje,eeting}s, Geert
Hi Geert, On 2023-11-21 17:11:52 +0100, Geert Uytterhoeven wrote: > Hi Niklas, > > On Tue, Nov 21, 2023 at 1:44 PM Niklas Söderlund > <niklas.soderlund+renesas@ragnatech.se> wrote: > > On 2023-11-21 13:20:54 +0100, Krzysztof Kozlowski wrote: > > > On 21/11/2023 13:10, Niklas Söderlund wrote: > > > >>> + > > > >>> + renesas,rx-internal-delay: > > > >>> + type: boolean > > > >>> + description: > > > >>> + Enable internal Rx clock delay, typically 1.8ns. > > > >> > > > >> Why this is bool, not delay in ns? > > > > > > > > The TSN is only capable of enabling or disable internal delays, not set > > > > how long the delay is. The documentation states that the delay depends > > > > on the electronic characteristics of the particular board, but states > > > > that they typically are 1.8ns for Rx and 2.0ns for Tx. > > > > > > I don't understand that part. If you cannot configure the internal > > > delay, how could it depend on the board characteristics? > > > > Each of these two properties reflect a single bit in the device > > configuration space. If the bit is set the {Rx,Tx} delay mode is active > > or disabled. The documentation for the bit simply states, > > > > Tx clock internal Delay Mode > > > > This bit can add internal Tx clock delay typ 2.0ns*. > > > > *Refer to Electrical Characteristics for details. > > > > Same paragraph for Rx but a typical 1.8ns delay. > > > > > > I looked at the generic properties {rx,tx}-internal-delay-ps but they > > > > are of int type. So I opted for a vendor specific bool property. Do you > > > > think a better route is to use the generic property and force the value > > > > to be either 0 or the typical delay? > > This is not dissimilar from EtherAVB, where the hardware also supports > only a single bit, and whose DT bindings have: > > rx-internal-delay-ps: > enum: [0, 1800] > > tx-internal-delay-ps: > enum: [0, 2000] > > (with additional restrictions depending on the SoC, as on some SoCs > the bits cannot be changed). That is a good point, I will switch to use the standard bindings for delay in v2. > > > > >> Why this is property of a board (not SoC)? > > > > > > > > I'm sorry I don't understand this question. > > > > > > Why setting internal delay is specific to a board, not to a SoC? Why > > > each board would need to configure it? On which parts of hardware on the > > > board does this depend? > > > > Ahh, I think I understand. It is per board as I understand the > > documentation. It depends on the electrical characteristics of the > > board. > > Exactly. These bits (and also similar bits in the PHY) are used to > adapt signaling to the board trace lengths between MAC (on-SoC) and PHY. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
> > > + renesas,rx-internal-delay: > > > + type: boolean > > > + description: > > > + Enable internal Rx clock delay, typically 1.8ns. > > > > Why this is bool, not delay in ns? > > The TSN is only capable of enabling or disable internal delays, not set > how long the delay is. The documentation states that the delay depends > on the electronic characteristics of the particular board, but states > that they typically are 1.8ns for Rx and 2.0ns for Tx. Do you have a board that actually requires this? In general, we try to have the PHY add the delay, not the MAC. So i would start with hard coding the delay to 0ns, and only implement these properties if you have a board where the PHY cannot add the delay. Andrew
Hi Andrew, On Tue, Nov 21, 2023 at 8:54 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > + renesas,rx-internal-delay: > > > > + type: boolean > > > > + description: > > > > + Enable internal Rx clock delay, typically 1.8ns. > > > > > > Why this is bool, not delay in ns? > > > > The TSN is only capable of enabling or disable internal delays, not set > > how long the delay is. The documentation states that the delay depends > > on the electronic characteristics of the particular board, but states > > that they typically are 1.8ns for Rx and 2.0ns for Tx. > > Do you have a board that actually requires this? We have several users of the corresponding properties for EtherAVB: $ git grep -W "&.*avb" -- "*renesas*dts*" | grep internal-delay-ps arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi- rx-internal-delay-ps = <1800>; arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi- tx-internal-delay-ps = <2000>; arch/arm64/boot/dts/renesas/hihope-rzg2-ex.dtsi- tx-internal-delay-ps = <2000>; arch/arm64/boot/dts/renesas/hihope-rzg2-ex.dtsi- rx-internal-delay-ps = <1800>; arch/arm64/boot/dts/renesas/r8a77970-eagle.dts- rx-internal-delay-ps = <1800>; arch/arm64/boot/dts/renesas/r8a77970-eagle.dts- tx-internal-delay-ps = <2000>; arch/arm64/boot/dts/renesas/r8a77970-v3msk.dts- rx-internal-delay-ps = <1800>; arch/arm64/boot/dts/renesas/r8a77970-v3msk.dts- tx-internal-delay-ps = <2000>; arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts- tx-internal-delay-ps = <2000>; arch/arm64/boot/dts/renesas/r8a779g0-white-hawk-cpu.dtsi- tx-internal-delay-ps = <2000>; arch/arm64/boot/dts/renesas/salvator-common.dtsi- tx-internal-delay-ps = <2000>; arch/arm64/boot/dts/renesas/ulcb.dtsi- tx-internal-delay-ps = <2000>; > In general, we try to have the PHY add the delay, not the MAC. So i > would start with hard coding the delay to 0ns, and only implement > these properties if you have a board where the PHY cannot add the > delay. If I understand the KSZ9031 bindings correctly, that PHY is limited to a skew of up to 960 ps, not 1800 or 2000 ps. Gr{oetje,eeting}s, Geert
> > In general, we try to have the PHY add the delay, not the MAC. So i > > would start with hard coding the delay to 0ns, and only implement > > these properties if you have a board where the PHY cannot add the > > delay. > > If I understand the KSZ9031 bindings correctly, that PHY is limited to > a skew of up to 960 ps, not 1800 or 2000 ps. Reading ksz9031_config_rgmii_delay(), it implements the four PHY_INTERFACE_MODE_RGMII* interface modes. So it should be able to provide the 2ns delay. Sometimes the tuning ability is relative to the base delay. So maybe it can do 2n +- 0.960/2 ? Anyway, try interface mode "rgmii_id" with the MAC not providing any delay. Andrew
Hi Andrew, On Wed, Nov 22, 2023 at 2:57 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > In general, we try to have the PHY add the delay, not the MAC. So i > > > would start with hard coding the delay to 0ns, and only implement > > > these properties if you have a board where the PHY cannot add the > > > delay. > > > > If I understand the KSZ9031 bindings correctly, that PHY is limited to > > a skew of up to 960 ps, not 1800 or 2000 ps. > > Reading ksz9031_config_rgmii_delay(), it implements the four > PHY_INTERFACE_MODE_RGMII* interface modes. So it should be able to > provide the 2ns delay. Sometimes the tuning ability is relative to the > base delay. So maybe it can do 2n +- 0.960/2 ? Oh, I do remember the regressions when the KSZ9031 driver started implementing that :-( See commit a6f51f2efa742df0 ("ravb: Add support for explicit internal clock delay configuration") and the commits it links to. > Anyway, try interface mode "rgmii_id" with the MAC not providing any > delay. That's more or less what we had before, cfr. commit 9b81018185965a30 ("arm64: dts: renesas: rcar-gen3: Convert EtherAVB to explicit delay handling")... Gr{oetje,eeting}s, Geert
diff --git a/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml new file mode 100644 index 000000000000..255c8f3a5a3b --- /dev/null +++ b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml @@ -0,0 +1,133 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/renesas,ethertsn.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Renesas Ethernet TSN End-station + +maintainers: + - Niklas Söderlund <niklas.soderlund@ragnatech.se> + +description: + The RTSN device provides Ethernet network using a 10 Mbps, 100 Mbps, or 1 + Gbps full-duplex link via MII/GMII/RMII/RGMII. Depending on the connected PHY. + +properties: + compatible: + oneOf: + - items: + - enum: + - renesas,ethertsn-r8a779g0 # R-Car V4H + + reg: + items: + - description: TSN End Station target + - description: generalized Precision Time Protocol target + + reg-names: + items: + - const: tsnes + - const: gptp + + interrupts: + items: + - description: TX data interrupt + - description: RX data interrupt + + interrupt-names: + items: + - const: tx_data + - const: rx_data + + clocks: + maxItems: 1 + + power-domains: + maxItems: 1 + + resets: + maxItems: 1 + + phy-mode: + contains: + enum: + - mii + - rgmii + + phy-handle: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Specifies a reference to a node representing a PHY device. + + renesas,rx-internal-delay: + type: boolean + description: + Enable internal Rx clock delay, typically 1.8ns. + + renesas,tx-internal-delay: + type: boolean + description: + Enable internal Tx clock delay, typically 2.0ns. + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + +patternProperties: + "^ethernet-phy@[0-9a-f]$": + type: object + $ref: ethernet-phy.yaml# + +required: + - compatible + - reg + - reg-names + - interrupts + - interrupt-names + - clocks + - power-domains + - resets + - phy-mode + - phy-handle + - '#address-cells' + - '#size-cells' + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/r8a779g0-cpg-mssr.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/power/r8a779g0-sysc.h> + #include <dt-bindings/gpio/gpio.h> + + tsn0: ethernet@e6460000 { + compatible = "renesas,ethertsn-r8a779g0"; + reg = <0xe6460000 0x7000>, + <0xe6449000 0x500>; + reg-names = "tsnes", "gptp"; + interrupts = <GIC_SPI 429 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 430 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "tx_data", "rx_data"; + clocks = <&cpg CPG_MOD 2723>; + power-domains = <&sysc R8A779G0_PD_ALWAYS_ON>; + resets = <&cpg 2723>; + + phy-mode = "rgmii"; + renesas,tx-internal-delay; + phy-handle = <&phy3>; + + #address-cells = <1>; + #size-cells = <0>; + + phy3: ethernet-phy@3 { + compatible = "ethernet-phy-ieee802.3-c45"; + reg = <0>; + interrupt-parent = <&gpio4>; + interrupts = <3 IRQ_TYPE_LEVEL_LOW>; + reset-gpios = <&gpio1 23 GPIO_ACTIVE_LOW>; + }; + };
Add bindings for Renesas R-Car Ethernet TSN End-station IP. The RTSN device provides Ethernet network. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- .../bindings/net/renesas,ethertsn.yaml | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/renesas,ethertsn.yaml