| Message ID | 20201114200104.4148283-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dwmac-meson8b: picosecond precision RX delay support | expand |
On Sat, Nov 14, 2020 at 09:01:01PM +0100, Martin Blumenstingl wrote: > Amlogic Meson G12A, G12B and SM1 SoCs have a more advanced RGMII RX > delay register which allows picoseconds precision. Deprecate the old > "amlogic,rx-delay-ns" in favour of a new "amlogic,rgmii-rx-delay-ps" > property. > > For older SoCs the only known supported values were 0ns and 2ns. The new > SoCs have 200ps precision and support RGMII RX delays between 0ps and > 3000ps. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > .../bindings/net/amlogic,meson-dwmac.yaml | 52 ++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml b/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml > index 6b057b117aa0..bafde1194193 100644 > --- a/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml > @@ -74,18 +74,68 @@ allOf: > Any configuration is ignored when the phy-mode is set to "rmii". > > amlogic,rx-delay-ns: > + deprecated: true > enum: > - 0 > - 2 > default: 0 > + description: > + The internal RGMII RX clock delay in nanoseconds. Deprecated, use > + amlogic,rgmii-rx-delay-ps instead. > + > + amlogic,rgmii-rx-delay-ps: > + default: 0 > description: > The internal RGMII RX clock delay (provided by this IP block) in > - nanoseconds. When phy-mode is set to "rgmii" then the RX delay > + picoseconds. When phy-mode is set to "rgmii" then the RX delay > should be explicitly configured. When the phy-mode is set to > either "rgmii-id" or "rgmii-rxid" the RX clock delay is already > provided by the PHY. Any configuration is ignored when the > phy-mode is set to "rmii". Hi Martin I don't think the wording matches what the driver is actually doing: if (dwmac->rx_delay_ns == 2) rx_dly_config = PRG_ETH0_ADJ_ENABLE | PRG_ETH0_ADJ_SETUP; else rx_dly_config = 0; switch (dwmac->phy_mode) { case PHY_INTERFACE_MODE_RGMII: delay_config = tx_dly_config | rx_dly_config; break; case PHY_INTERFACE_MODE_RGMII_RXID: delay_config = tx_dly_config; break; case PHY_INTERFACE_MODE_RGMII_TXID: delay_config = rx_dly_config; break; case PHY_INTERFACE_MODE_RGMII_ID: case PHY_INTERFACE_MODE_RMII: delay_config = 0; break; So rx_delay is used for both rgmii and rgmii-txid. The binding says nothing about rgmii-txid. And while i'm looking at the code, i wonder about this: if (rx_dly_config & PRG_ETH0_ADJ_ENABLE) { if (!dwmac->timing_adj_clk) { dev_err(dwmac->dev, "The timing-adjustment clock is mandatory for the RX delay re-timing\n"); return -EINVAL; } /* The timing adjustment logic is driven by a separate clock */ ret = meson8b_devm_clk_prepare_enable(dwmac, dwmac->timing_adj_clk); if (ret) { dev_err(dwmac->dev, "Failed to enable the timing-adjustment clock\n"); return ret; } } It looks like it can be that rx_dly_config & PRG_ETH0_ADJ_ENABLE is true, so the clock is enabled, but delay_config does not contain rx_delay_config, so it is pointless turning it on. Andrew
Hi Andrew, On Sat, Nov 14, 2020 at 11:32 PM Andrew Lunn <andrew@lunn.ch> wrote: [...] > > + amlogic,rgmii-rx-delay-ps: > > + default: 0 > > description: > > The internal RGMII RX clock delay (provided by this IP block) in > > - nanoseconds. When phy-mode is set to "rgmii" then the RX delay > > + picoseconds. When phy-mode is set to "rgmii" then the RX delay > > should be explicitly configured. When the phy-mode is set to > > either "rgmii-id" or "rgmii-rxid" the RX clock delay is already > > provided by the PHY. Any configuration is ignored when the > > phy-mode is set to "rmii". > > Hi Martin > > I don't think the wording matches what the driver is actually doing: > > if (dwmac->rx_delay_ns == 2) > rx_dly_config = PRG_ETH0_ADJ_ENABLE | PRG_ETH0_ADJ_SETUP; > else > rx_dly_config = 0; > > switch (dwmac->phy_mode) { > case PHY_INTERFACE_MODE_RGMII: > delay_config = tx_dly_config | rx_dly_config; > break; > case PHY_INTERFACE_MODE_RGMII_RXID: > delay_config = tx_dly_config; > break; > case PHY_INTERFACE_MODE_RGMII_TXID: > delay_config = rx_dly_config; > break; > case PHY_INTERFACE_MODE_RGMII_ID: > case PHY_INTERFACE_MODE_RMII: > delay_config = 0; > break; > > So rx_delay is used for both rgmii and rgmii-txid. The binding says > nothing about rgmii-txid. interesting point here. it's been like this before this patch. still I would like to understand what the proper way to fix it is so I can also include a fix for it: 1. should rgmii-txid not add any RX delay on the MAC side? that would mean for my board I will switch to phy-mode rgmii so the MAC applies both the RX and TX delays 2. update the documentation to clarify that rgmii-txid would also add the RX delay on the MAC side > And while i'm looking at the code, i wonder about this: > > if (rx_dly_config & PRG_ETH0_ADJ_ENABLE) { > if (!dwmac->timing_adj_clk) { > dev_err(dwmac->dev, > "The timing-adjustment clock is mandatory for the RX delay re-timing\n"); > return -EINVAL; > } > > /* The timing adjustment logic is driven by a separate clock */ > ret = meson8b_devm_clk_prepare_enable(dwmac, > dwmac->timing_adj_clk); > if (ret) { > dev_err(dwmac->dev, > "Failed to enable the timing-adjustment clock\n"); > return ret; > } > } > > It looks like it can be that rx_dly_config & PRG_ETH0_ADJ_ENABLE is > true, so the clock is enabled, but delay_config does not contain > rx_delay_config, so it is pointless turning it on. that is a good point and also a bug with one of the previous patches I'll include a patch fixing this in v2 Best regards, Martin
On Sun, Nov 15, 2020 at 10:22:06AM +0100, Martin Blumenstingl wrote: > Hi Andrew, > > On Sat, Nov 14, 2020 at 11:32 PM Andrew Lunn <andrew@lunn.ch> wrote: > [...] > > > + amlogic,rgmii-rx-delay-ps: > > > + default: 0 > > > description: > > > The internal RGMII RX clock delay (provided by this IP block) in > > > - nanoseconds. When phy-mode is set to "rgmii" then the RX delay > > > + picoseconds. When phy-mode is set to "rgmii" then the RX delay > > > should be explicitly configured. When the phy-mode is set to > > > either "rgmii-id" or "rgmii-rxid" the RX clock delay is already > > > provided by the PHY. Any configuration is ignored when the > > > phy-mode is set to "rmii". > > > > Hi Martin > > > > I don't think the wording matches what the driver is actually doing: > > > > if (dwmac->rx_delay_ns == 2) > > rx_dly_config = PRG_ETH0_ADJ_ENABLE | PRG_ETH0_ADJ_SETUP; > > else > > rx_dly_config = 0; > > > > switch (dwmac->phy_mode) { > > case PHY_INTERFACE_MODE_RGMII: > > delay_config = tx_dly_config | rx_dly_config; > > break; > > case PHY_INTERFACE_MODE_RGMII_RXID: > > delay_config = tx_dly_config; > > break; > > case PHY_INTERFACE_MODE_RGMII_TXID: > > delay_config = rx_dly_config; > > break; > > case PHY_INTERFACE_MODE_RGMII_ID: > > case PHY_INTERFACE_MODE_RMII: > > delay_config = 0; > > break; > > > > So rx_delay is used for both rgmii and rgmii-txid. The binding says > > nothing about rgmii-txid. > interesting point here. it's been like this before this patch. still I > would like to understand what the proper way to fix it is so I can > also include a fix for it: > 1. should rgmii-txid not add any RX delay on the MAC side? that would > mean for my board I will switch to phy-mode rgmii so the MAC applies > both the RX and TX delays > 2. update the documentation to clarify that rgmii-txid would also add > the RX delay on the MAC side Hi Martin I would fix the documentation. > that is a good point and also a bug with one of the previous patches > I'll include a patch fixing this in v2 Thanks for looking at these. Andrew
diff --git a/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml b/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml index 6b057b117aa0..bafde1194193 100644 --- a/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml +++ b/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml @@ -74,18 +74,68 @@ allOf: Any configuration is ignored when the phy-mode is set to "rmii". amlogic,rx-delay-ns: + deprecated: true enum: - 0 - 2 default: 0 + description: + The internal RGMII RX clock delay in nanoseconds. Deprecated, use + amlogic,rgmii-rx-delay-ps instead. + + amlogic,rgmii-rx-delay-ps: + default: 0 description: The internal RGMII RX clock delay (provided by this IP block) in - nanoseconds. When phy-mode is set to "rgmii" then the RX delay + picoseconds. When phy-mode is set to "rgmii" then the RX delay should be explicitly configured. When the phy-mode is set to either "rgmii-id" or "rgmii-rxid" the RX clock delay is already provided by the PHY. Any configuration is ignored when the phy-mode is set to "rmii". + - if: + properties: + compatible: + contains: + enum: + - amlogic,meson8b-dwmac + - amlogic,meson8m2-dwmac + - amlogic,meson-gxbb-dwmac + - amlogic,meson-axg-dwmac + then: + properties: + amlogic,rgmii-rx-delay-ps: + enum: + - 0 + - 2000 + + - if: + properties: + compatible: + contains: + enum: + - amlogic,meson-g12a-dwmac + then: + properties: + amlogic,rgmii-rx-delay-ps: + enum: + - 0 + - 200 + - 400 + - 600 + - 800 + - 1000 + - 1200 + - 1400 + - 1600 + - 1800 + - 2000 + - 2200 + - 2400 + - 2600 + - 2800 + - 3000 + properties: compatible: additionalItems: true
Amlogic Meson G12A, G12B and SM1 SoCs have a more advanced RGMII RX delay register which allows picoseconds precision. Deprecate the old "amlogic,rx-delay-ns" in favour of a new "amlogic,rgmii-rx-delay-ps" property. For older SoCs the only known supported values were 0ns and 2ns. The new SoCs have 200ps precision and support RGMII RX delays between 0ps and 3000ps. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- .../bindings/net/amlogic,meson-dwmac.yaml | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-)