diff mbox series

[PATCH/RFC,1/5] dt-bindings: net: renesas,ravb: Document internal clock delay properties

Message ID 20200619191554.24942-2-geert+renesas@glider.be
State Superseded
Headers show
Series ravb: Add support for explicit internal clock delay configuration | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Geert Uytterhoeven June 19, 2020, 7:15 p.m. UTC
Some EtherAVB variants support internal clock delay configuration, which
can add larger delays than the delays that are typically supported by
the PHY (using an "rgmii-*id" PHY mode, and/or "[rt]xc-skew-ps"
properties).

Add properties for configuring the internal MAC delays.
These properties are mandatory, even when specified as zero, to
distinguish between old and new DTBs.

Update the example accordingly.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 .../devicetree/bindings/net/renesas,ravb.txt  | 29 ++++++++++---------
 1 file changed, 16 insertions(+), 13 deletions(-)

Comments

Oleksij Rempel June 20, 2020, 5:47 a.m. UTC | #1
Hi Geert,

Am 19.06.20 um 21:15 schrieb Geert Uytterhoeven:
> Some EtherAVB variants support internal clock delay configuration, which
> can add larger delays than the delays that are typically supported by
> the PHY (using an "rgmii-*id" PHY mode, and/or "[rt]xc-skew-ps"
> properties).
>
> Add properties for configuring the internal MAC delays.
> These properties are mandatory, even when specified as zero, to
> distinguish between old and new DTBs.
>
> Update the example accordingly.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  .../devicetree/bindings/net/renesas,ravb.txt  | 29 ++++++++++---------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> index 032b76f14f4fdb38..488ada78b6169b8e 100644
> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> @@ -64,6 +64,18 @@ Optional properties:
>  			 AVB_LINK signal.
>  - renesas,ether-link-active-low: boolean, specify when the AVB_LINK signal is
>  				 active-low instead of normal active-high.
> +- renesas,rxc-delay-ps: Internal RX clock delay.

may be it make sense to add a generic delay property for MACs and PHYs?

> +			This property is mandatory and valid only on R-Car Gen3
> +			and RZ/G2 SoCs.
> +			Valid values are 0 and 1800.
> +			A non-zero value is allowed only if phy-mode = "rgmii".
> +			Zero is not supported on R-Car D3.
> +- renesas,txc-delay-ps: Internal TX clock delay.
> +			This property is mandatory and valid only on R-Car H3,
> +			M3-W, M3-W+, M3-N, V3M, and V3H, and RZ/G2M and RZ/G2N.
> +			Valid values are 0 and 2000.

In the driver i didn't found sanity check for valid values.

> +			A non-zero value is allowed only if phy-mode = "rgmii".
> +			Zero is not supported on R-Car V3H.>  Example:
>
> @@ -105,8 +117,10 @@ Example:
>  				  "ch24";
>  		clocks = <&cpg CPG_MOD 812>;
>  		power-domains = <&cpg>;
> -		phy-mode = "rgmii-id";
> +		phy-mode = "rgmii";
>  		phy-handle = <&phy0>;
> +		renesas,rxc-delay-ps = <0>;
> +		renesas,txc-delay-ps = <2000>;
>
>  		pinctrl-0 = <&ether_pins>;
>  		pinctrl-names = "default";
> @@ -115,18 +129,7 @@ Example:
>  		#size-cells = <0>;
>
>  		phy0: ethernet-phy@0 {
> -			rxc-skew-ps = <900>;
> -			rxdv-skew-ps = <0>;
> -			rxd0-skew-ps = <0>;
> -			rxd1-skew-ps = <0>;
> -			rxd2-skew-ps = <0>;
> -			rxd3-skew-ps = <0>;
> -			txc-skew-ps = <900>;
> -			txen-skew-ps = <0>;
> -			txd0-skew-ps = <0>;
> -			txd1-skew-ps = <0>;
> -			txd2-skew-ps = <0>;
> -			txd3-skew-ps = <0>;
> +			rxc-skew-ps = <1500>;


I'm curios, how this numbers ware taken?
Old configurations was:
TX delay:
(txd*-skew-ps = 0) == -420ns
(txc-skew-ps = 900) == 0ns
resulting delays 0.420ns

RX delay:
(rxd*-skew-ps = 0) == -420ns
(rxc-skew-ps = 900) == 0ns
internal delay 1.2 + 420ns will be 1.62ns

I was not able to find actual delay values provided by the MAC. But from your patches it looks like 2ns.

So, end result will be:
TXID = 2.4ns
RXID = 3.62ns

Both values seems to be a bit out of spec. New values are:
TXID = PHY 0ns + MAC 2.0ns
RXID =
(rxd*-skew-ps = no change) == 0ns
(rxc-skew-ps = 1500) == +600ns
internal delay 1.2 + 600ns = 1.8ns

From the PHY point of view, it is RGMII_RXID mode. Are you sure, RGMII_ID is not working for you?
Till now the numbers was not looking as a fine tuning.
I assume, it is better to use proper phy-mode instead of skew-ps values. I feel like no one had time
to understand real configured values in this PHY.

>  			reg = <0>;
>  			interrupt-parent = <&gpio2>;
>  			interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
>


--
Regards,
Oleksij
Andrew Lunn June 20, 2020, 2:43 p.m. UTC | #2
On Sat, Jun 20, 2020 at 07:47:16AM +0200, Oleksij Rempel wrote:
> Hi Geert,
> 
> Am 19.06.20 um 21:15 schrieb Geert Uytterhoeven:
> > Some EtherAVB variants support internal clock delay configuration, which
> > can add larger delays than the delays that are typically supported by
> > the PHY (using an "rgmii-*id" PHY mode, and/or "[rt]xc-skew-ps"
> > properties).
> >
> > Add properties for configuring the internal MAC delays.
> > These properties are mandatory, even when specified as zero, to
> > distinguish between old and new DTBs.
> >
> > Update the example accordingly.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  .../devicetree/bindings/net/renesas,ravb.txt  | 29 ++++++++++---------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > index 032b76f14f4fdb38..488ada78b6169b8e 100644
> > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > @@ -64,6 +64,18 @@ Optional properties:
> >  			 AVB_LINK signal.
> >  - renesas,ether-link-active-low: boolean, specify when the AVB_LINK signal is
> >  				 active-low instead of normal active-high.
> > +- renesas,rxc-delay-ps: Internal RX clock delay.
> 
> may be it make sense to add a generic delay property for MACs and PHYs?

See Dan Murphys "RGMII Internal delay common property" patchset. That
patchset is addressing the PHY side. Maybe we can build on that to
address the MAC side?

	Andrew
Sergei Shtylyov June 20, 2020, 6:15 p.m. UTC | #3
Hello!

On 06/19/2020 10:15 PM, Geert Uytterhoeven wrote:

> Some EtherAVB variants support internal clock delay configuration, which
> can add larger delays than the delays that are typically supported by
> the PHY (using an "rgmii-*id" PHY mode, and/or "[rt]xc-skew-ps"
> properties).
> 
> Add properties for configuring the internal MAC delays.
> These properties are mandatory, even when specified as zero, to
> distinguish between old and new DTBs.
> 
> Update the example accordingly.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  .../devicetree/bindings/net/renesas,ravb.txt  | 29 ++++++++++---------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> index 032b76f14f4fdb38..488ada78b6169b8e 100644
> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> @@ -64,6 +64,18 @@ Optional properties:
>  			 AVB_LINK signal.
>  - renesas,ether-link-active-low: boolean, specify when the AVB_LINK signal is
>  				 active-low instead of normal active-high.
> +- renesas,rxc-delay-ps: Internal RX clock delay.
> +			This property is mandatory and valid only on R-Car Gen3
> +			and RZ/G2 SoCs.
> +			Valid values are 0 and 1800.
> +			A non-zero value is allowed only if phy-mode = "rgmii".
> +			Zero is not supported on R-Car D3.

   Hm, where did you see about the D3 limitation?

> +- renesas,txc-delay-ps: Internal TX clock delay.
> +			This property is mandatory and valid only on R-Car H3,
> +			M3-W, M3-W+, M3-N, V3M, and V3H, and RZ/G2M and RZ/G2N.
> +			Valid values are 0 and 2000.
> +			A non-zero value is allowed only if phy-mode = "rgmii".
> +			Zero is not supported on R-Car V3H.

  Same question about V3H here...

[...]
> @@ -105,8 +117,10 @@ Example:
>  				  "ch24";
>  		clocks = <&cpg CPG_MOD 812>;
>  		power-domains = <&cpg>;
> -		phy-mode = "rgmii-id";
> +		phy-mode = "rgmii";
>  		phy-handle = <&phy0>;
> +		renesas,rxc-delay-ps = <0>;

   Mhm, zero RX delay in RGMII-ID mode?

> +		renesas,txc-delay-ps = <2000>;
>  
>  		pinctrl-0 = <&ether_pins>;
>  		pinctrl-names = "default";
> @@ -115,18 +129,7 @@ Example:
>  		#size-cells = <0>;
>  
>  		phy0: ethernet-phy@0 {
> -			rxc-skew-ps = <900>;
> -			rxdv-skew-ps = <0>;
> -			rxd0-skew-ps = <0>;
> -			rxd1-skew-ps = <0>;
> -			rxd2-skew-ps = <0>;
> -			rxd3-skew-ps = <0>;
> -			txc-skew-ps = <900>;
> -			txen-skew-ps = <0>;
> -			txd0-skew-ps = <0>;
> -			txd1-skew-ps = <0>;
> -			txd2-skew-ps = <0>;
> -			txd3-skew-ps = <0>;
> +			rxc-skew-ps = <1500>;

   Ah, you're relying on a PHY?

[...]

MBR, Sergei
Geert Uytterhoeven June 21, 2020, 8:23 a.m. UTC | #4
Hi Oleksij,

On Sat, Jun 20, 2020 at 7:47 AM Oleksij Rempel <linux@rempel-privat.de> wrote:
> Am 19.06.20 um 21:15 schrieb Geert Uytterhoeven:
> > Some EtherAVB variants support internal clock delay configuration, which
> > can add larger delays than the delays that are typically supported by
> > the PHY (using an "rgmii-*id" PHY mode, and/or "[rt]xc-skew-ps"
> > properties).
> >
> > Add properties for configuring the internal MAC delays.
> > These properties are mandatory, even when specified as zero, to
> > distinguish between old and new DTBs.
> >
> > Update the example accordingly.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt

> > +                     This property is mandatory and valid only on R-Car Gen3
> > +                     and RZ/G2 SoCs.
> > +                     Valid values are 0 and 1800.
> > +                     A non-zero value is allowed only if phy-mode = "rgmii".
> > +                     Zero is not supported on R-Car D3.
> > +- renesas,txc-delay-ps: Internal TX clock delay.
> > +                     This property is mandatory and valid only on R-Car H3,
> > +                     M3-W, M3-W+, M3-N, V3M, and V3H, and RZ/G2M and RZ/G2N.
> > +                     Valid values are 0 and 2000.
>
> In the driver i didn't found sanity check for valid values.

As EtherAVB supports only zero and a single non-zero value, I didn't
bother validating the actual non-zero value in the driver.

However, I did implement full validation in the json-schema version of
the DT bindings, cfr. "[PATCH/RFC] dt-bindings: net: renesas,etheravb:
Convert to json-schema"
(https://lore.kernel.org/r/20200621081710.10245-1-geert+renesas@glider.be)
(In hindsight, I should not have postponed posting that patch)

> > @@ -105,8 +117,10 @@ Example:
> >                                 "ch24";
> >               clocks = <&cpg CPG_MOD 812>;
> >               power-domains = <&cpg>;
> > -             phy-mode = "rgmii-id";
> > +             phy-mode = "rgmii";
> >               phy-handle = <&phy0>;
> > +             renesas,rxc-delay-ps = <0>;
> > +             renesas,txc-delay-ps = <2000>;
> >
> >               pinctrl-0 = <&ether_pins>;
> >               pinctrl-names = "default";
> > @@ -115,18 +129,7 @@ Example:
> >               #size-cells = <0>;
> >
> >               phy0: ethernet-phy@0 {
> > -                     rxc-skew-ps = <900>;
> > -                     rxdv-skew-ps = <0>;
> > -                     rxd0-skew-ps = <0>;
> > -                     rxd1-skew-ps = <0>;
> > -                     rxd2-skew-ps = <0>;
> > -                     rxd3-skew-ps = <0>;
> > -                     txc-skew-ps = <900>;
> > -                     txen-skew-ps = <0>;
> > -                     txd0-skew-ps = <0>;
> > -                     txd1-skew-ps = <0>;
> > -                     txd2-skew-ps = <0>;
> > -                     txd3-skew-ps = <0>;
> > +                     rxc-skew-ps = <1500>;
>
>
> I'm curios, how this numbers ware taken?
> Old configurations was:
> TX delay:
> (txd*-skew-ps = 0) == -420ns
> (txc-skew-ps = 900) == 0ns
> resulting delays 0.420ns

Please ignore the actual contents of the old example.  It was based on a
very old DTS, which has received several fixes in the mean time.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven June 21, 2020, 8:26 a.m. UTC | #5
Hi Sergei,

On Sat, Jun 20, 2020 at 8:16 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 06/19/2020 10:15 PM, Geert Uytterhoeven wrote:
> > Some EtherAVB variants support internal clock delay configuration, which
> > can add larger delays than the delays that are typically supported by
> > the PHY (using an "rgmii-*id" PHY mode, and/or "[rt]xc-skew-ps"
> > properties).
> >
> > Add properties for configuring the internal MAC delays.
> > These properties are mandatory, even when specified as zero, to
> > distinguish between old and new DTBs.
> >
> > Update the example accordingly.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
> > @@ -64,6 +64,18 @@ Optional properties:
> >                        AVB_LINK signal.
> >  - renesas,ether-link-active-low: boolean, specify when the AVB_LINK signal is
> >                                active-low instead of normal active-high.
> > +- renesas,rxc-delay-ps: Internal RX clock delay.
> > +                     This property is mandatory and valid only on R-Car Gen3
> > +                     and RZ/G2 SoCs.
> > +                     Valid values are 0 and 1800.
> > +                     A non-zero value is allowed only if phy-mode = "rgmii".
> > +                     Zero is not supported on R-Car D3.
>
>    Hm, where did you see about the D3 limitation?

R-Car Gen3 Hardware User's Manual, Section 50.2.7 ("AVB-DMAC Product
Specific Register (APSR)"), "RDM" bit:

    For R-Car D3, delayed mode is only available

> > +- renesas,txc-delay-ps: Internal TX clock delay.
> > +                     This property is mandatory and valid only on R-Car H3,
> > +                     M3-W, M3-W+, M3-N, V3M, and V3H, and RZ/G2M and RZ/G2N.
> > +                     Valid values are 0 and 2000.
> > +                     A non-zero value is allowed only if phy-mode = "rgmii".
> > +                     Zero is not supported on R-Car V3H.
>
>   Same question about V3H here...

Same section, "TDM" bit:

    For R-Car V3H, delayed mode is only available.

> > @@ -105,8 +117,10 @@ Example:
> >                                 "ch24";
> >               clocks = <&cpg CPG_MOD 812>;
> >               power-domains = <&cpg>;
> > -             phy-mode = "rgmii-id";
> > +             phy-mode = "rgmii";
> >               phy-handle = <&phy0>;
> > +             renesas,rxc-delay-ps = <0>;
>
>    Mhm, zero RX delay in RGMII-ID mode?

Please ignore the actual contents of the old example.  It was based on a
very old DTS, which has received several fixes in the mean time.

Should have written:

    Update the (bogus) example accordingly.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt
index 032b76f14f4fdb38..488ada78b6169b8e 100644
--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt
+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt
@@ -64,6 +64,18 @@  Optional properties:
 			 AVB_LINK signal.
 - renesas,ether-link-active-low: boolean, specify when the AVB_LINK signal is
 				 active-low instead of normal active-high.
+- renesas,rxc-delay-ps: Internal RX clock delay.
+			This property is mandatory and valid only on R-Car Gen3
+			and RZ/G2 SoCs.
+			Valid values are 0 and 1800.
+			A non-zero value is allowed only if phy-mode = "rgmii".
+			Zero is not supported on R-Car D3.
+- renesas,txc-delay-ps: Internal TX clock delay.
+			This property is mandatory and valid only on R-Car H3,
+			M3-W, M3-W+, M3-N, V3M, and V3H, and RZ/G2M and RZ/G2N.
+			Valid values are 0 and 2000.
+			A non-zero value is allowed only if phy-mode = "rgmii".
+			Zero is not supported on R-Car V3H.
 
 Example:
 
@@ -105,8 +117,10 @@  Example:
 				  "ch24";
 		clocks = <&cpg CPG_MOD 812>;
 		power-domains = <&cpg>;
-		phy-mode = "rgmii-id";
+		phy-mode = "rgmii";
 		phy-handle = <&phy0>;
+		renesas,rxc-delay-ps = <0>;
+		renesas,txc-delay-ps = <2000>;
 
 		pinctrl-0 = <&ether_pins>;
 		pinctrl-names = "default";
@@ -115,18 +129,7 @@  Example:
 		#size-cells = <0>;
 
 		phy0: ethernet-phy@0 {
-			rxc-skew-ps = <900>;
-			rxdv-skew-ps = <0>;
-			rxd0-skew-ps = <0>;
-			rxd1-skew-ps = <0>;
-			rxd2-skew-ps = <0>;
-			rxd3-skew-ps = <0>;
-			txc-skew-ps = <900>;
-			txen-skew-ps = <0>;
-			txd0-skew-ps = <0>;
-			txd1-skew-ps = <0>;
-			txd2-skew-ps = <0>;
-			txd3-skew-ps = <0>;
+			rxc-skew-ps = <1500>;
 			reg = <0>;
 			interrupt-parent = <&gpio2>;
 			interrupts = <11 IRQ_TYPE_LEVEL_LOW>;