| Message ID | b65cd0978587224eb1496ad60d224394525b8ba6.1540204183.git.joabreu@synopsys.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: Misc improvements for Generic 10G PHY | expand |
> EXPORT_SYMBOL_GPL(gen10g_resume); > @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = { > .phy_id = 0xffffffff, > .phy_id_mask = 0xffffffff, > .name = "Generic 10G PHY", > - .soft_reset = gen10g_no_soft_reset, > + .soft_reset = gen10g_soft_reset, > .config_init = gen10g_config_init, > .features = 0, > .aneg_done = genphy_c45_aneg_done, Hi Jose You need to be careful here. There is a reason this is called gen10g_no_soft_reset, rather than having an empty gen10g_soft_reset. Some PHYs break when you do a reset. So adding a gen10g_soft_reset is fine, but don't change this here, without first understanding the history, and talking to Russell King. Andrew
Hello, On 22-10-2018 13:28, Andrew Lunn wrote: >> EXPORT_SYMBOL_GPL(gen10g_resume); >> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = { >> .phy_id = 0xffffffff, >> .phy_id_mask = 0xffffffff, >> .name = "Generic 10G PHY", >> - .soft_reset = gen10g_no_soft_reset, >> + .soft_reset = gen10g_soft_reset, >> .config_init = gen10g_config_init, >> .features = 0, >> .aneg_done = genphy_c45_aneg_done, > Hi Jose > > You need to be careful here. There is a reason this is called > gen10g_no_soft_reset, rather than having an empty > gen10g_soft_reset. Some PHYs break when you do a reset. So adding a > gen10g_soft_reset is fine, but don't change this here, without first > understanding the history, and talking to Russell King. Hmm, the reset function only interacts with standard PCS registers, which should always be available ... From my tests I need to do at least 1 reset during power-up so in ultimate case I can add a feature quirk or similar. Russell, can you please comment ? Thanks and Best Regards, Jose Miguel Abreu > > Andrew
On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote: > Hello, > > On 22-10-2018 13:28, Andrew Lunn wrote: > >> EXPORT_SYMBOL_GPL(gen10g_resume); > >> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = { > >> .phy_id = 0xffffffff, > >> .phy_id_mask = 0xffffffff, > >> .name = "Generic 10G PHY", > >> - .soft_reset = gen10g_no_soft_reset, > >> + .soft_reset = gen10g_soft_reset, > >> .config_init = gen10g_config_init, > >> .features = 0, > >> .aneg_done = genphy_c45_aneg_done, > > Hi Jose > > > > You need to be careful here. There is a reason this is called > > gen10g_no_soft_reset, rather than having an empty > > gen10g_soft_reset. Some PHYs break when you do a reset. So adding a > > gen10g_soft_reset is fine, but don't change this here, without first > > understanding the history, and talking to Russell King. > > Hmm, the reset function only interacts with standard PCS > registers, which should always be available ... > > >From my tests I need to do at least 1 reset during power-up so in > ultimate case I can add a feature quirk or similar. > > Russell, can you please comment ? Setting the reset bit on 88x3310 causes the entire device to become completely inaccessible until hardware reset. Therefore, this bit must _never_ be set for these devices. That said, we have a separate driver for these PHYs, but that will only be used for them if it's present in the kernel. If we accidentally fall back to the generic driver, then we'll screw the 88x3310 until a full hardware reset. We also have a bunch of net devices that make use of this crippled "generic" 10G support - we don't know whether resetting the PHY for those systems will cause a regression - maybe board firmware already configured the PHY? I can't say either way on that, except that we've had crippled 10G support in PHYLIB for a number of years now _with_ users, and adding reset support drastically changes the subsystem's behaviour for these users. I would recommend not touching the generic 10G driver, but instead implement your own driver for your PHY to avoid causing regressions.
On 10/22/18 8:48 AM, Russell King - ARM Linux wrote: > On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote: >> Hello, >> >> On 22-10-2018 13:28, Andrew Lunn wrote: >>>> EXPORT_SYMBOL_GPL(gen10g_resume); >>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = { >>>> .phy_id = 0xffffffff, >>>> .phy_id_mask = 0xffffffff, >>>> .name = "Generic 10G PHY", >>>> - .soft_reset = gen10g_no_soft_reset, >>>> + .soft_reset = gen10g_soft_reset, >>>> .config_init = gen10g_config_init, >>>> .features = 0, >>>> .aneg_done = genphy_c45_aneg_done, >>> Hi Jose >>> >>> You need to be careful here. There is a reason this is called >>> gen10g_no_soft_reset, rather than having an empty >>> gen10g_soft_reset. Some PHYs break when you do a reset. So adding a >>> gen10g_soft_reset is fine, but don't change this here, without first >>> understanding the history, and talking to Russell King. >> >> Hmm, the reset function only interacts with standard PCS >> registers, which should always be available ... >> >> >From my tests I need to do at least 1 reset during power-up so in >> ultimate case I can add a feature quirk or similar. >> >> Russell, can you please comment ? > > Setting the reset bit on 88x3310 causes the entire device to become > completely inaccessible until hardware reset. Therefore, this bit > must _never_ be set for these devices. That said, we have a separate > driver for these PHYs, but that will only be used for them if it's > present in the kernel. If we accidentally fall back to the generic > driver, then we'll screw the 88x3310 until a full hardware reset. > > We also have a bunch of net devices that make use of this crippled > "generic" 10G support - we don't know whether resetting the PHY > for those systems will cause a regression - maybe board firmware > already configured the PHY? I can't say either way on that, except > that we've had crippled 10G support in PHYLIB for a number of years > now _with_ users, and adding reset support drastically changes the > subsystem's behaviour for these users. > > I would recommend not touching the generic 10G driver, but instead > implement your own driver for your PHY to avoid causing regressions. > Agreed.
On 22-10-2018 18:13, Florian Fainelli wrote: > On 10/22/18 8:48 AM, Russell King - ARM Linux wrote: >> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote: >>> Hello, >>> >>> On 22-10-2018 13:28, Andrew Lunn wrote: >>>>> EXPORT_SYMBOL_GPL(gen10g_resume); >>>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = { >>>>> .phy_id = 0xffffffff, >>>>> .phy_id_mask = 0xffffffff, >>>>> .name = "Generic 10G PHY", >>>>> - .soft_reset = gen10g_no_soft_reset, >>>>> + .soft_reset = gen10g_soft_reset, >>>>> .config_init = gen10g_config_init, >>>>> .features = 0, >>>>> .aneg_done = genphy_c45_aneg_done, >>>> Hi Jose >>>> >>>> You need to be careful here. There is a reason this is called >>>> gen10g_no_soft_reset, rather than having an empty >>>> gen10g_soft_reset. Some PHYs break when you do a reset. So adding a >>>> gen10g_soft_reset is fine, but don't change this here, without first >>>> understanding the history, and talking to Russell King. >>> Hmm, the reset function only interacts with standard PCS >>> registers, which should always be available ... >>> >>> >From my tests I need to do at least 1 reset during power-up so in >>> ultimate case I can add a feature quirk or similar. >>> >>> Russell, can you please comment ? >> Setting the reset bit on 88x3310 causes the entire device to become >> completely inaccessible until hardware reset. Therefore, this bit >> must _never_ be set for these devices. That said, we have a separate >> driver for these PHYs, but that will only be used for them if it's >> present in the kernel. If we accidentally fall back to the generic >> driver, then we'll screw the 88x3310 until a full hardware reset. >> >> We also have a bunch of net devices that make use of this crippled >> "generic" 10G support - we don't know whether resetting the PHY >> for those systems will cause a regression - maybe board firmware >> already configured the PHY? I can't say either way on that, except >> that we've had crippled 10G support in PHYLIB for a number of years >> now _with_ users, and adding reset support drastically changes the >> subsystem's behaviour for these users. >> >> I would recommend not touching the generic 10G driver, but instead >> implement your own driver for your PHY to avoid causing regressions. >> > Agreed. What about .suspend / .resume ? Thanks and Best Regards, Jose Miguel Abreu
On Tue, Oct 23, 2018 at 11:17:50AM +0100, Jose Abreu wrote: > On 22-10-2018 18:13, Florian Fainelli wrote: > > On 10/22/18 8:48 AM, Russell King - ARM Linux wrote: > >> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote: > >>> Hello, > >>> > >>> On 22-10-2018 13:28, Andrew Lunn wrote: > >>>>> EXPORT_SYMBOL_GPL(gen10g_resume); > >>>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = { > >>>>> .phy_id = 0xffffffff, > >>>>> .phy_id_mask = 0xffffffff, > >>>>> .name = "Generic 10G PHY", > >>>>> - .soft_reset = gen10g_no_soft_reset, > >>>>> + .soft_reset = gen10g_soft_reset, > >>>>> .config_init = gen10g_config_init, > >>>>> .features = 0, > >>>>> .aneg_done = genphy_c45_aneg_done, > >>>> Hi Jose > >>>> > >>>> You need to be careful here. There is a reason this is called > >>>> gen10g_no_soft_reset, rather than having an empty > >>>> gen10g_soft_reset. Some PHYs break when you do a reset. So adding a > >>>> gen10g_soft_reset is fine, but don't change this here, without first > >>>> understanding the history, and talking to Russell King. > >>> Hmm, the reset function only interacts with standard PCS > >>> registers, which should always be available ... > >>> > >>> >From my tests I need to do at least 1 reset during power-up so in > >>> ultimate case I can add a feature quirk or similar. > >>> > >>> Russell, can you please comment ? > >> Setting the reset bit on 88x3310 causes the entire device to become > >> completely inaccessible until hardware reset. Therefore, this bit > >> must _never_ be set for these devices. That said, we have a separate > >> driver for these PHYs, but that will only be used for them if it's > >> present in the kernel. If we accidentally fall back to the generic > >> driver, then we'll screw the 88x3310 until a full hardware reset. > >> > >> We also have a bunch of net devices that make use of this crippled > >> "generic" 10G support - we don't know whether resetting the PHY > >> for those systems will cause a regression - maybe board firmware > >> already configured the PHY? I can't say either way on that, except > >> that we've had crippled 10G support in PHYLIB for a number of years > >> now _with_ users, and adding reset support drastically changes the > >> subsystem's behaviour for these users. > >> > >> I would recommend not touching the generic 10G driver, but instead > >> implement your own driver for your PHY to avoid causing regressions. > >> > > Agreed. > > What about .suspend / .resume ? I have no idea what you're proposing there - your patches weren't copied to me.
On 23-10-2018 11:20, Russell King - ARM Linux wrote: > On Tue, Oct 23, 2018 at 11:17:50AM +0100, Jose Abreu wrote: >> On 22-10-2018 18:13, Florian Fainelli wrote: >>> On 10/22/18 8:48 AM, Russell King - ARM Linux wrote: >>>> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote: >>>>> Hello, >>>>> >>>>> On 22-10-2018 13:28, Andrew Lunn wrote: >>>>>>> EXPORT_SYMBOL_GPL(gen10g_resume); >>>>>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = { >>>>>>> .phy_id = 0xffffffff, >>>>>>> .phy_id_mask = 0xffffffff, >>>>>>> .name = "Generic 10G PHY", >>>>>>> - .soft_reset = gen10g_no_soft_reset, >>>>>>> + .soft_reset = gen10g_soft_reset, >>>>>>> .config_init = gen10g_config_init, >>>>>>> .features = 0, >>>>>>> .aneg_done = genphy_c45_aneg_done, >>>>>> Hi Jose >>>>>> >>>>>> You need to be careful here. There is a reason this is called >>>>>> gen10g_no_soft_reset, rather than having an empty >>>>>> gen10g_soft_reset. Some PHYs break when you do a reset. So adding a >>>>>> gen10g_soft_reset is fine, but don't change this here, without first >>>>>> understanding the history, and talking to Russell King. >>>>> Hmm, the reset function only interacts with standard PCS >>>>> registers, which should always be available ... >>>>> >>>>> >From my tests I need to do at least 1 reset during power-up so in >>>>> ultimate case I can add a feature quirk or similar. >>>>> >>>>> Russell, can you please comment ? >>>> Setting the reset bit on 88x3310 causes the entire device to become >>>> completely inaccessible until hardware reset. Therefore, this bit >>>> must _never_ be set for these devices. That said, we have a separate >>>> driver for these PHYs, but that will only be used for them if it's >>>> present in the kernel. If we accidentally fall back to the generic >>>> driver, then we'll screw the 88x3310 until a full hardware reset. >>>> >>>> We also have a bunch of net devices that make use of this crippled >>>> "generic" 10G support - we don't know whether resetting the PHY >>>> for those systems will cause a regression - maybe board firmware >>>> already configured the PHY? I can't say either way on that, except >>>> that we've had crippled 10G support in PHYLIB for a number of years >>>> now _with_ users, and adding reset support drastically changes the >>>> subsystem's behaviour for these users. >>>> >>>> I would recommend not touching the generic 10G driver, but instead >>>> implement your own driver for your PHY to avoid causing regressions. >>>> >>> Agreed. >> What about .suspend / .resume ? > I have no idea what you're proposing there - your patches weren't copied > to me. > They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that without this remote end doesn't detect link is down ... If it's okay for Generic 10G driver I can submit only this and manually reset PHY in stmmac driver so that I don't need to implement custom PHY driver ... BTW, I just found out currently Generic 10G Driver is broken without patch 4/4 of this series [1] [1] https://patchwork.ozlabs.org/patch/987570/ Thanks and Best Regards, Jose Miguel Abreu
On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote: > On 23-10-2018 11:20, Russell King - ARM Linux wrote: > > I have no idea what you're proposing there - your patches weren't copied > > to me. > > They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that > without this remote end doesn't detect link is down ... > > If it's okay for Generic 10G driver I can submit only this and > manually reset PHY in stmmac driver so that I don't need to > implement custom PHY driver ... > BTW, I just found out currently Generic 10G Driver is broken > without patch 4/4 of this series [1] > > [1] https://patchwork.ozlabs.org/patch/987570/ How is it broken - what are the symptoms? The generic 10G driver is bound not via the normal bus matching and phy_bus_match(), but via a manual bind in phy_attach_direct(). This calls the probe function, which is phy_probe(), which initialises the supported/advertising to the driver's features (which as you note are zero.) However, phy_attach_direct() goes on to call phy_init_hw(), which calls the config_init() method. The config_init() method initialises the supported/advertising masks to 10GbaseT. This is (partly) what I refer to when I say that the generic 10G support is crippled - it only supports this single speed and media. So the supported/advertising masks should be forced to only 10GbaseT at the completion of phy_attach_direct(). The "generic 10G" support doesn't do autonegotiation, configuration or link mode forcing. It only assumes 10GbaseT is supported, and only checks for the "link up" bits. It isn't like the non-10G generic PHY support due to history - it was added in 2014 by Andy Fleming (see 124059fd53af). BTW, your patch 1 is wrong as well (introducing phy_update_link()). You don't take account that a 10G phy may have alternative ways of reading the link (like 88x3310 does, because it has multiple instances of AN/PCS/PHYXS at 1k offsets.) All the gen10g_* functions are legacy functions for the crippled "generic" 10G support.
> If it's okay for Generic 10G driver I can submit only this and > manually reset PHY in stmmac driver so that I don't need to > implement custom PHY driver ... Hi Jose That is a bad idea. What happens when somebody uses a different PHY which uses a different reset sequence? Please keep with the abstraction. Anything touching the PHY needs to be in a PHY driver, or phylib. Andrew
On 23-10-2018 11:58, Russell King - ARM Linux wrote: > On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote: >> On 23-10-2018 11:20, Russell King - ARM Linux wrote: >>> I have no idea what you're proposing there - your patches weren't copied >>> to me. >> They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that >> without this remote end doesn't detect link is down ... >> >> If it's okay for Generic 10G driver I can submit only this and >> manually reset PHY in stmmac driver so that I don't need to >> implement custom PHY driver ... > > >> BTW, I just found out currently Generic 10G Driver is broken >> without patch 4/4 of this series [1] >> >> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_987570_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zYEDSMZPTCHiPc_3B8buyu0kXnlIEYawnHWAxPrsoSU&s=MlF6I2cBSYkGxgEwNV-hXpXJIvXv_gRYXP-CazjkUSw&e= > How is it broken - what are the symptoms? > > The generic 10G driver is bound not via the normal bus matching and > phy_bus_match(), but via a manual bind in phy_attach_direct(). This > calls the probe function, which is phy_probe(), which initialises > the supported/advertising to the driver's features (which as you note > are zero.) Since 719655a14971 ("net: phy: Replace phy driver features u32 with link_mode bitmap"), phy_probe() calls ethtool_convert_link_mode_to_legacy_u32() with phydrv->features as argument. Since features are NULL, we will get NULL pointer dereference. I guess Generic 10G driver was forgotten in the conversion. Thanks and Best Regards, Jose Miguel Abreu > > However, phy_attach_direct() goes on to call phy_init_hw(), which > calls the config_init() method. The config_init() method initialises > the supported/advertising masks to 10GbaseT. This is (partly) what > I refer to when I say that the generic 10G support is crippled - it > only supports this single speed and media. > > So the supported/advertising masks should be forced to only 10GbaseT > at the completion of phy_attach_direct(). > > The "generic 10G" support doesn't do autonegotiation, configuration > or link mode forcing. It only assumes 10GbaseT is supported, and > only checks for the "link up" bits. > > It isn't like the non-10G generic PHY support due to history - it > was added in 2014 by Andy Fleming (see 124059fd53af). > > BTW, your patch 1 is wrong as well (introducing phy_update_link()). > You don't take account that a 10G phy may have alternative ways of > reading the link (like 88x3310 does, because it has multiple > instances of AN/PCS/PHYXS at 1k offsets.) All the gen10g_* > functions are legacy functions for the crippled "generic" 10G > support. >
> Since 719655a14971 ("net: phy: Replace phy driver features u32 > with link_mode bitmap"), phy_probe() calls > ethtool_convert_link_mode_to_legacy_u32() with phydrv->features > as argument. Since features are NULL, we will get NULL pointer > dereference. Hi Jose Thanks for pointing that out. I will fix it ASAP. Andrew
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index c0135217b81f..7e62bd7795a3 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -1,6 +1,7 @@ /* * Clause 45 PHY support */ +#include <linux/delay.h> #include <linux/ethtool.h> #include <linux/export.h> #include <linux/mdio.h> @@ -294,6 +295,35 @@ int gen10g_read_status(struct phy_device *phydev) } EXPORT_SYMBOL_GPL(gen10g_read_status); +static int gen10g_poll_reset(struct phy_device *phydev) +{ + /* Poll until the reset bit clears (50ms per retry == 0.6 sec) */ + unsigned int retries = 12; + int ret; + + do { + msleep(50); + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1); + if (ret < 0) + return ret; + } while (ret & MDIO_CTRL1_RESET && --retries); + if (ret & MDIO_CTRL1_RESET) + return -ETIMEDOUT; + + return 0; +} + +static int gen10g_soft_reset(struct phy_device *phydev) +{ + int val; + + val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, MDIO_CTRL1_RESET); + if (val < 0) + return val; + + return gen10g_poll_reset(phydev); +} + int gen10g_no_soft_reset(struct phy_device *phydev) { /* Do nothing for now */ @@ -313,12 +343,36 @@ EXPORT_SYMBOL_GPL(gen10g_config_init); int gen10g_suspend(struct phy_device *phydev) { + int val; + + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1); + if (val < 0) + return val; + + val |= MDIO_CTRL1_LPOWER; + + val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, val); + if (val < 0) + return val; + return 0; } EXPORT_SYMBOL_GPL(gen10g_suspend); int gen10g_resume(struct phy_device *phydev) { + int val; + + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1); + if (val < 0) + return val; + + val &= ~MDIO_CTRL1_LPOWER; + + val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, val); + if (val < 0) + return val; + return 0; } EXPORT_SYMBOL_GPL(gen10g_resume); @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = { .phy_id = 0xffffffff, .phy_id_mask = 0xffffffff, .name = "Generic 10G PHY", - .soft_reset = gen10g_no_soft_reset, + .soft_reset = gen10g_soft_reset, .config_init = gen10g_config_init, .features = 0, .aneg_done = genphy_c45_aneg_done,
Implement the missing callbacks for Generic 10G PHY. Tested using XGMAC with a C45 PHY working at 10G Link. Signed-off-by: Jose Abreu <joabreu@synopsys.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Joao Pinto <joao.pinto@synopsys.com> --- drivers/net/phy/phy-c45.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)