[PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Niklas Söderlund]

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

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>;
+        };
+    };
-- 
2.42.1

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Geert Uytterhoeven]

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

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Geert Uytterhoeven]

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

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Niklas Söderlund]

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

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Niklas Söderlund]

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
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Niklas Söderlund]

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.

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Krzysztof Kozlowski]

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Geert Uytterhoeven]

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

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Niklas Söderlund]

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

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Andrew Lunn]

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Geert Uytterhoeven]

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

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Andrew Lunn]

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

Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for Ethernet TSN

[Geert Uytterhoeven]

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

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