| Message ID | 20201021135140.51300-1-alexandru.ardelean@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg | expand |
On Wed, Oct 21, 2020 at 04:51:39PM +0300, Alexandru Ardelean wrote: > The LINKING_EN bit is always cleared during reset. Initially it was set > during the downshift setup, because it's in the same register as the > downshift retry count (PHY_CTRL1). Hi Alexandru For those of us how have not read the datasheet, could you give a brief explanation what LINKING_EN does? > This change moves the handling of LINKING_EN from the downshift handler to > the autonegotiation handler. Also, during autonegotiation setup, the > diagnostics clock is cleared. And what is the diagnostics clock used for? Andrew
On Wed, Oct 21, 2020 at 4:58 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Wed, Oct 21, 2020 at 04:51:39PM +0300, Alexandru Ardelean wrote: > > The LINKING_EN bit is always cleared during reset. Initially it was set > > during the downshift setup, because it's in the same register as the > > downshift retry count (PHY_CTRL1). > > Hi Alexandru > > For those of us how have not read the datasheet, could you give a > brief explanation what LINKING_EN does? So, clearing this bit puts the PHY in a standby-state. The PHY doesn't do any autonegotiation or link handling. > > > This change moves the handling of LINKING_EN from the downshift handler to > > the autonegotiation handler. Also, during autonegotiation setup, the > > diagnostics clock is cleared. > > And what is the diagnostics clock used for? The clock diagnostics is used for 2 things: the diagnostics block [mostly for stuff like cable-diagnostics] and the frame-generator. The frame-generator is an interesting feature of the PHY, that's not useful for the current phylib; the PHY can send packages [like a signal generator], and then these can be looped back, or sent over the wire. Maybe it's being used mostly internally by the group that created the PHY Having said this, I'll include some comments for these in a V2 of this patchset. > > Andrew
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Hi Alexandru Overall, this looks good. > +static int adin_cable_test_report_trans(int result) > +{ > + int mask; > + > + if (result & ADIN1300_CDIAG_RSLT_GOOD) > + return ETHTOOL_A_CABLE_RESULT_CODE_OK; > + if (result & ADIN1300_CDIAG_RSLT_OPEN) > + return ETHTOOL_A_CABLE_RESULT_CODE_OPEN; > + > + /* short with other pairs */ > + mask = ADIN1300_CDIAG_RSLT_XSHRT3 | > + ADIN1300_CDIAG_RSLT_XSHRT2 | > + ADIN1300_CDIAG_RSLT_XSHRT1; > + if (result & mask) > + return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT; The nice thing about the netlink API is that it is extendable without breaking backwards compatibility. You could if you want add another attribute, indicating what pair it is shorted to. Andrew
> The frame-generator is an interesting feature of the PHY, that's not > useful for the current phylib; the PHY can send packages [like a > signal generator], and then these can be looped back, or sent over the > wire. Many PHYs that that. I posted some patches to the list a few years ago adding basic support for the Marvell PHY frame generator. They got NACKed. The netlink API, and some of the infrastructure i added for cable testing would make it possible to fix the issues that caused the NACK. > Having said this, I'll include some comments for these in a V2 of this patchset. Thanks. Andrew P.S. Your mail is broken somehow: Delivery has failed to these recipients or groups: alexaundru.ardelean@analog.com The email address you entered couldn't be found. Please check the recipient's email address and try to resend the message. If the problem continues, please contact your email admin.
On Wed, Oct 21, 2020 at 5:09 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > removed my typo-ed email > Hi Alexandru > > Overall, this looks good. > > > +static int adin_cable_test_report_trans(int result) > > +{ > > + int mask; > > + > > + if (result & ADIN1300_CDIAG_RSLT_GOOD) > > + return ETHTOOL_A_CABLE_RESULT_CODE_OK; > > + if (result & ADIN1300_CDIAG_RSLT_OPEN) > > + return ETHTOOL_A_CABLE_RESULT_CODE_OPEN; > > + > > + /* short with other pairs */ > > + mask = ADIN1300_CDIAG_RSLT_XSHRT3 | > > + ADIN1300_CDIAG_RSLT_XSHRT2 | > > + ADIN1300_CDIAG_RSLT_XSHRT1; > > + if (result & mask) > > + return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT; > > The nice thing about the netlink API is that it is extendable without > breaking backwards compatibility. You could if you want add another > attribute, indicating what pair it is shorted to. That would be an idea. Actually, I'd also be interested [for this PHY], to report a "significance impedance" detection, which is similar to the short-detection that is already done. At first, this report would sound like it could be interesting; but feel free to disagree with me. And there's also some "busy" indicator; as-in "unknown activity during diagnostics"; to-be-honest, I don't know what this is yet. I'd need to check, but odds are that I'd need to also ask about it. So, I don't think I'd implement this. > > Andrew
On Wed, Oct 21, 2020 at 5:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > The frame-generator is an interesting feature of the PHY, that's not > > useful for the current phylib; the PHY can send packages [like a > > signal generator], and then these can be looped back, or sent over the > > wire. > removed my typo-ed [work] email i use gmail as a mirror-email for my work email, because.... reasons and i added my work-email to the --cc list with a typo, because the universe seems to have wanted that [in a manner of saying it] > Many PHYs that that. I posted some patches to the list a few years ago > adding basic support for the Marvell PHY frame generator. They got > NACKed. The netlink API, and some of the infrastructure i added for > cable testing would make it possible to fix the issues that caused the > NACK. i'll think about the frame-generator; i was super-happy when the cable-test support was added; when i first wrote the PHY, i actually wrote this logic for cable-testing, then scrapped it because the code [without any framework around it] just looked bad, and like it was asking to cause trouble; with this minimal framework in place, cable-testing looks like a neat feature [and neatly implemented]; and it took me less than a day to write and test it; so, thank you for this :) > > > Having said this, I'll include some comments for these in a V2 of this patchset. > > Thanks. > > Andrew > > P.S. > > Your mail is broken somehow: > > Delivery has failed to these recipients or groups: > > alexaundru.ardelean@analog.com > The email address you entered couldn't be found. Please check the recipient's > email address and try to resend the message. If the problem continues, please > contact your email admin.
> Actually, I'd also be interested [for this PHY], to report a > "significance impedance" detection, which is similar to the > short-detection that is already done. You can add that as just another element of the enum. > At first, this report would sound like it could be interesting; but > feel free to disagree with me. > > And there's also some "busy" indicator; as-in "unknown activity during > diagnostics"; to-be-honest, I don't know what this is yet. The link partner did not go quiet. You can only do cable tests if the partner is not sending frames or pulses. You will find most PHYs have some sort of error status for this. For the Marvell driver, this is MII_VCT7_RESULTS_INVALID. In that case, the Marvell driver returns ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC. Andrew
On Wed, Oct 21, 2020 at 5:28 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > Actually, I'd also be interested [for this PHY], to report a > > "significance impedance" detection, which is similar to the > > short-detection that is already done. > > You can add that as just another element of the enum. > > > At first, this report would sound like it could be interesting; but > > feel free to disagree with me. > > > > And there's also some "busy" indicator; as-in "unknown activity during > > diagnostics"; to-be-honest, I don't know what this is yet. > > The link partner did not go quiet. You can only do cable tests if the > partner is not sending frames or pulses. You will find most PHYs have > some sort of error status for this. For the Marvell driver, this is > MII_VCT7_RESULTS_INVALID. In that case, the Marvell driver returns > ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC. Good to know. So, then a quick question: would this patchset [well, a V2 of this] be ok in this form, for the initial cable-test support of this PHY? For other enhancements on the PHY's cable-test [that also require some new netlink attributes, I can do other patches, in the form of "new-netlink-attr, then driver change, and then ethtool update". > > Andrew
> i'll think about the frame-generator;
Here were the two main problems i can remember with my first version:
How do you discover what is can actually do? You probably need to
collect up all the open PHY datasheets and get an idea what the
different vendors provide, what is common, what could be shared
extensions etc, and think about how you can describe the
capabilities. Probably a netlink call will be needed to return what
the hardware is capable of doing.
At the time, it was necessary to hold RTNL while performing packet
generation. That is bad, because it means most of the control plane
stops for all devices. We will need to copy some of the ideas from the
cable test to avoid this, adding a state to the state machine, etc.
Andrew
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c index 5bc3926c52f0..619d36685b5d 100644 --- a/drivers/net/phy/adin.c +++ b/drivers/net/phy/adin.c @@ -23,6 +23,7 @@ #define ADIN1300_PHY_CTRL1 0x0012 #define ADIN1300_AUTO_MDI_EN BIT(10) #define ADIN1300_MAN_MDIX_EN BIT(9) +#define ADIN1300_DIAG_CLK_EN BIT(2) #define ADIN1300_RX_ERR_CNT 0x0014 @@ -326,10 +327,9 @@ static int adin_set_downshift(struct phy_device *phydev, u8 cnt) return -E2BIG; val = FIELD_PREP(ADIN1300_DOWNSPEED_RETRIES_MSK, cnt); - val |= ADIN1300_LINKING_EN; rc = phy_modify(phydev, ADIN1300_PHY_CTRL3, - ADIN1300_LINKING_EN | ADIN1300_DOWNSPEED_RETRIES_MSK, + ADIN1300_DOWNSPEED_RETRIES_MSK, val); if (rc < 0) return rc; @@ -560,6 +560,14 @@ static int adin_config_aneg(struct phy_device *phydev) { int ret; + ret = phy_clear_bits(phydev, ADIN1300_PHY_CTRL1, ADIN1300_DIAG_CLK_EN); + if (ret < 0) + return ret; + + ret = phy_set_bits(phydev, ADIN1300_PHY_CTRL3, ADIN1300_LINKING_EN); + if (ret < 0) + return ret; + ret = adin_config_mdix(phydev); if (ret) return ret;
The LINKING_EN bit is always cleared during reset. Initially it was set during the downshift setup, because it's in the same register as the downshift retry count (PHY_CTRL1). This change moves the handling of LINKING_EN from the downshift handler to the autonegotiation handler. Also, during autonegotiation setup, the diagnostics clock is cleared. This is being done as a prequel to the cable-diagnostics patch. When the cable diagnostics finishes, the PHY state machine goes back into the PHY_UP state and the autonegotiation is restarted (or better said, the autonegotiation handler is called). During this call, the diagnostics clock should be disabled, and the LINKING_EN bit set in the PHY_CTRL1 register. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/net/phy/adin.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)