Message ID | 20201021135140.51300-1-alexandru.ardelean@analog.com |
---|---|
State | Deferred |
Delegated to: | David Miller |
Headers | show |
Series | [1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg | expand |
Context | Check | Description |
---|---|---|
jkicinski/cover_letter | success | Link |
jkicinski/fixes_present | success | Link |
jkicinski/patch_count | success | Link |
jkicinski/tree_selection | success | Guessed tree name to be net-next |
jkicinski/subject_prefix | warning | Target tree name not specified in the subject |
jkicinski/source_inline | success | Was 0 now: 0 |
jkicinski/verify_signedoff | success | Link |
jkicinski/module_param | success | Was 0 now: 0 |
jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/verify_fixes | success | Link |
jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/header_inline | success | Link |
jkicinski/stable | success | Stable not CCed |
jkicinski/cover_letter | success | Link |
jkicinski/fixes_present | success | Link |
jkicinski/patch_count | success | Link |
jkicinski/tree_selection | success | Guessed tree name to be net-next |
jkicinski/subject_prefix | warning | Target tree name not specified in the subject |
jkicinski/source_inline | success | Was 0 now: 0 |
jkicinski/verify_signedoff | success | Link |
jkicinski/module_param | success | Was 0 now: 0 |
jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/verify_fixes | success | Link |
jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/header_inline | success | Link |
jkicinski/stable | success | Stable not CCed |
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
> 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: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.
> 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(-)