| Message ID | 20200619191554.24942-2-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | ravb: Add support for explicit internal clock delay configuration | expand |
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 = <ðer_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
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
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 = <ðer_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
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 = <ðer_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
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 --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 = <ðer_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>;
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(-)