Re: [PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues
On 21/07/2017 16:06, Timur Tabi wrote: > On 7/21/17 8:29 AM, Marc Gonzalez wrote: > >> I don't understand what you're saying. >> >> It is a correct observation that the code enabling >> RGMII RX clock delay is a NOP, since that bit will >> always be set at that point. >> >> The spec for the 8035 (I haven't checked for 8030 and 8031, >> is that what you meant by "other systems"?) states that >> Sel_clk125m_dsp, which is described as: >> "Control bit for rgmii interface rx clock delay" >> is 1 after HW reset, 1 after SW reset. >> >> So my statement "RX clock delay is enabled at reset" >> is universally true. It's not just on some systems. > > Ok, taken out of context, the comment doesn't really explain why the > code is the way it is. I'm not really happy about the word "assumes". If a HW setting defaults to 0 at reset, and some init is called right after reset, then you know the setting's value is 0. If you need that value to be 1, all you need is a function setting it to 1. This is the current situation. Commit 2e5f9f281ee8 assumes 0 at reset, and provides a function setting the value to 1. Reality is different. The HW setting defaults to 1 at reset. So it turns out that the function setting the value to 1 is pointless, because the value is already 1. There is however no way to set the value to 0. Does that explain why I wrote "assume"? Also the commit message: "The current code supports enabling RGMII RX and TX clock delays. The unstated assumption is that these settings are disabled by default at reset, which is not the case." > Maybe you should add a sentence explaining when the code is NOT a no-op. The code is *NEVER* NOT a no-op. I.e. the code enabling RX clock delay is ALWAYS a no-op. I don't understand what you think is unclear in my comment. Regards.
Re: [PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues
On 7/21/17 8:29 AM, Marc Gonzalez wrote: I don't understand what you're saying. It is a correct observation that the code enabling RGMII RX clock delay is a NOP, since that bit will always be set at that point. The spec for the 8035 (I haven't checked for 8030 and 8031, is that what you meant by "other systems"?) states that Sel_clk125m_dsp, which is described as: "Control bit for rgmii interface rx clock delay" is 1 after HW reset, 1 after SW reset. So my statement "RX clock delay is enabled at reset" is universally true. It's not just on some systems. Ok, taken out of context, the comment doesn't really explain why the code is the way it is. I'm not really happy about the word "assumes". Maybe you should add a sentence explaining when the code is NOT a no-op. -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
Re: [PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues
On 21/07/2017 15:20, Timur Tabi wrote: > On 7/21/17 6:25 AM, Marc Gonzalez wrote: > >> + * NB: This code assumes that RGMII RX clock delay is disabled >> + * at reset, but actually, RX clock delay is enabled at reset. > > Could we change this to say, "RX clock delay is enabled at reset on some > systems."? Otherwise, it implies that the code is wrong 100% of the > time and should be fixed, not documented. I don't understand what you're saying. It is a correct observation that the code enabling RGMII RX clock delay is a NOP, since that bit will always be set at that point. The spec for the 8035 (I haven't checked for 8030 and 8031, is that what you meant by "other systems"?) states that Sel_clk125m_dsp, which is described as: "Control bit for rgmii interface rx clock delay" is 1 after HW reset, 1 after SW reset. So my statement "RX clock delay is enabled at reset" is universally true. It's not just on some systems. What am I missing? Regards.
Re: [PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues
On 7/21/17 6:25 AM, Marc Gonzalez wrote: +* NB: This code assumes that RGMII RX clock delay is disabled +* at reset, but actually, RX clock delay is enabled at reset. Could we change this to say, "RX clock delay is enabled at reset on some systems."? Otherwise, it implies that the code is wrong 100% of the time and should be fixed, not documented. -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
[PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues
The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case.
RX clock delay is enabled at reset. And TX clock delay "survives"
across SW resets. Thus, if the bootloader enables TX clock delay,
it will remain enabled at reset in Linux.
Signed-off-by: Marc Gonzalez
---
drivers/net/phy/at803x.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c1e52b9dc58d..7a0954513b91 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -283,6 +283,12 @@ static int at803x_config_init(struct phy_device *phydev)
if (ret < 0)
return ret;
+ /*
+* NB: This code assumes that RGMII RX clock delay is disabled
+* at reset, but actually, RX clock delay is enabled at reset.
+* Disabling the delay if it has not been explicitly requested
+* breaks boards that rely on the enabled-by-default behavior.
+*/
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
ret = at803x_enable_rx_delay(phydev);
@@ -290,6 +296,12 @@ static int at803x_config_init(struct phy_device *phydev)
return ret;
}
+ /*
+* NB: This code assumes that RGMII TX clock delay is disabled
+* at reset, but actually, TX clock delay "survives" across SW
+* resets. If the bootloader enables TX clock delay, Linux is
+* stuck with that setting.
+*/
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
ret = at803x_enable_tx_delay(phydev);
--
2.11.0
