net: phy: Misc improvements for Generic 10G PHY
Subsystems (3)
cover net: phy: Misc improvements for Generic 10G PHY
Set of improvements for Generic 10G PHY. All of them tested using stmmac with XGMAC2 IP working at 10G Link with a C45 PHY.
Jose Abreu (4):
net: phy: Use C45 Helpers when forcing PHY
net: phy-c45: Populate autoneg_done callback
net: phy-c45: Implement reset/suspend/resume callbacks
net: phy-c45: Populate missing features
drivers/net/phy/phy-c45.c | 59 +++++++++++++++++++++++++++++++++++++++++++++-- drivers/net/phy/phy.c | 2 +- include/linux/phy.h | 8 +++++++ 3 files changed, 66 insertions(+), 3 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/4 net: phy: Use C45 Helpers when forcing PHY
If PHY is in force state and we have a C45 phy we need to use the standard C45 helpers and not the C22 ones.
--- drivers/net/phy/phy.c | 2 +- include/linux/phy.h | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
drivers/net/phy/phy.c +1 -1
@@ -995,7 +995,7 @@ void phy_state_machine(struct work_struct *work) }break ;case PHY_FORCING : - err =genphy_update_link (phydev); + err =phy_update_link (phydev);if (err)break ;
include/linux/phy.h +8 -0
@@ -1044,6 +1044,14 @@ static inline int phy_read_status(struct phy_device *phydev)return genphy_read_status (phydev); } +static inline int phy_update_link (struct phy_device *phydev) +{ +if (phydev->is_c45) +return gen10g_read_status (phydev); +else +return genphy_update_link (phydev); +} +void phy_driver_unregister (struct phy_driver *drv);void phy_drivers_unregister (struct phy_driver *drv,int n);int phy_driver_register (struct phy_driver *new_driver,struct module *owner);
Signature · 1 line
2.7.4
Re: [PATCH net-next 1/4] net: phy: Use C45 Helpers when forcing PHY
Quoted 31 lines · Jose Abreu
If PHY is in force state and we have a C45 phy we need to use the standard C45 helpers and not the C22 ones.
Signed-off-by Jose Abreu <[email protected]>Cc Andrew Lunn <[email protected]>Cc Florian Fainelli <[email protected]>Cc "David S. Miller" <[email protected]>Cc Joao Pinto <[email protected]>--- drivers/net/phy/phy.c | 2 +- include/linux/phy.h | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)drivers/net/phy/phy.c +1
-1@@ -995,7 +995,7 @@ void phy_state_machine(struct work_struct *work) }break ;case PHY_FORCING : - err =genphy_update_link (phydev); + err =phy_update_link (phydev);if (err)break ;include/linux/phy.h +0
-0
include/linux/phy.h static inline int phy_read_status(struct phy_device *phydev) · line 1050
Quoted 7 lines
include/linux/phy.h +4
-0@@ -1044,6 +1044,14 @@ static inline int phy_read_status(struct phy_device *phydev)return genphy_read_status (phydev); } +static inline int phy_update_link (struct phy_device *phydev) +{ +if (phydev->is_c45) +return gen10g_read_status (phydev);
Should not this be genphy_c45_read_link() for symmetry with genphy_update_link() which only updates phydev->link?
Signature · 1 line
Florian
Re: [PATCH net-next 1/4] net: phy: Use C45 Helpers when forcing PHY
Quoted 35 lines · Florian Fainelli
Quoted 34 lines · Jose Abreu
If PHY is in force state and we have a C45 phy we need to use the standard C45 helpers and not the C22 ones.
Signed-off-by Jose Abreu <[email protected]>Cc Andrew Lunn <[email protected]>Cc Florian Fainelli <[email protected]>Cc "David S. Miller" <[email protected]>Cc Joao Pinto <[email protected]>--- drivers/net/phy/phy.c | 2 +- include/linux/phy.h | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)drivers/net/phy/phy.c +1
-1@@ -995,7 +995,7 @@ void phy_state_machine(struct work_struct *work) }break ;case PHY_FORCING : - err =genphy_update_link (phydev); + err =phy_update_link (phydev);if (err)break ;include/linux/phy.h +0
-0@@ -1044,6 +1044,14 @@ static inline int phy_read_status(struct phy_device *phydev)return genphy_read_status (phydev); }
Quoted 6 lines
include/linux/phy.h static inline int phy_read_status(struct phy_device *phydev) · line 1050
Quoted 4 lines
include/linux/phy.h +4
-0+static inline int phy_update_link (struct phy_device *phydev) +{ +if (phydev->is_c45) +return gen10g_read_status (phydev);Should not this be
genphy_c45_read_link()for symmetry withgenphy_update_link()which only updatesphydev->link?
Hmmm, genphy_c45_read_link() does not update phydev->link ... I can create a new gen10g_update_link() that wraps around genphy_c45_read_link() and updates link ...
Thanks and Best Regards,
Jose Miguel Abreu
PATCH 2/4 net: phy-c45: Populate autoneg_done callback
In reply to Jose AbreuWe already have this callback implemented. Use it in driver structure.
--- drivers/net/phy/phy-c45.c | 1 + 1 file changed, 1 insertion(+)
drivers/net/phy/phy-c45.c +1 -0
@@ -330,6 +330,7 @@ struct phy_driver genphy_10g_driver = { .soft_reset = gen10g_no_soft_reset, .config_init = gen10g_config_init, .features = 0, + .aneg_done = genphy_c45_aneg_done, .config_aneg = gen10g_config_aneg, .read_status = gen10g_read_status, .suspend = gen10g_suspend,
Signature · 1 line
2.7.4
Andrew Lunn
maintainerRe: [PATCH net-next 2/4] net: phy-c45: Populate autoneg_done callback
Quoted 16 lines · Jose Abreu
We already have this callback implemented. Use it in driver structure.
Signed-off-by Jose Abreu <[email protected]>Cc Andrew Lunn <[email protected]>Cc Florian Fainelli <[email protected]>Cc "David S. Miller" <[email protected]>Cc Joao Pinto <[email protected]>--- drivers/net/phy/phy-c45.c | 1 + 1 file changed, 1 insertion(+)drivers/net/phy/phy-c45.c +0
-0
drivers/net/phy/phy-c45.c struct phy_driver genphy_10g_driver = { · line 336
Quoted 7 lines
drivers/net/phy/phy-c45.c +1
-0@@ -330,6 +330,7 @@ struct phy_driver genphy_10g_driver = { .soft_reset = gen10g_no_soft_reset, .config_init = gen10g_config_init, .features = 0, + .aneg_done = genphy_c45_aneg_done, .config_aneg = gen10g_config_aneg, .read_status = gen10g_read_status, .suspend = gen10g_suspend,
We should probably do something about the naming. I need to look at the history to understand why we have gen10g_ and genphy_c45_.
Andrew
PATCH 3/4 net: phy-c45: Implement reset/suspend/resume callbacks
In reply to Jose AbreuImplement the missing callbacks for Generic 10G PHY. Tested using XGMAC with a C45 PHY working at 10G Link.
--- drivers/net/phy/phy-c45.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)
drivers/net/phy/phy-c45.c +55 -1
@@ -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,
Signature · 1 line
2.7.4
Andrew Lunn
maintainerRe: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
drivers/net/phy/phy-c45.c struct phy_driver genphy_10g_driver = { · line 387
Quoted 10 lines
EXPORT_SYMBOL_GPL(gen10g_resume);drivers/net/phy/phy-c45.c +1
-1@@ -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
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Hello,
Quoted 12 lines · Andrew Lunn
drivers/net/phy/phy-c45.c struct phy_driver genphy_10g_driver = { · line 387
Quoted 10 lines
EXPORT_SYMBOL_GPL(gen10g_resume);drivers/net/phy/phy-c45.c +1
-1@@ -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
Quoted 5 lines
You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an emptygen10g_soft_reset. Some PHYs break when you do a reset. So adding agen10g_soft_resetis 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 ...
Quoted 1 line
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
Quoted 2 lines
Andrew
Russell King - ARM Linux
reviewerRe: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Quoted 21 lines · Jose Abreu
Hello,
Quoted 12 lines · Andrew Lunn
drivers/net/phy/phy-c45.c struct phy_driver genphy_10g_driver = { · line 387
Quoted 10 lines
EXPORT_SYMBOL_GPL(gen10g_resume);drivers/net/phy/phy-c45.c +1
-1@@ -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
Quoted 5 lines
You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an emptygen10g_soft_reset. Some PHYs break when you do a reset. So adding agen10g_soft_resetis fine, but don't change this here, without first understanding the history, and talking to Russell King.
Quoted 7 lines
Hmm, the reset function only interacts with standard PCS registers, which should always be available ...
Quoted 1 line
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.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Quoted 45 lines · Russell King - ARM Linux
Quoted 21 lines · Jose Abreu
Hello,
Quoted 12 lines · Andrew Lunn
drivers/net/phy/phy-c45.c struct phy_driver genphy_10g_driver = { · line 387
Quoted 10 lines
EXPORT_SYMBOL_GPL(gen10g_resume);drivers/net/phy/phy-c45.c +1
-1@@ -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
Quoted 5 lines
You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an emptygen10g_soft_reset. Some PHYs break when you do a reset. So adding agen10g_soft_resetis fine, but don't change this here, without first understanding the history, and talking to Russell King.Quoted 7 lines
Hmm, the reset function only interacts with standard PCS registers, which should always be available ...
Quoted 1 line
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.
Quoted 3 lines
I would recommend not touching the generic 10G driver, but instead implement your own driver for your PHY to avoid causing regressions.
Agreed.
Signature · 1 line
Florian
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Quoted 44 lines · Florian Fainelli
Quoted 43 lines · Russell King - ARM Linux
Quoted 20 lines · Jose Abreu
Hello,
Quoted 12 lines · Andrew Lunn
drivers/net/phy/phy-c45.c struct phy_driver genphy_10g_driver = { · line 387
Quoted 10 lines
EXPORT_SYMBOL_GPL(gen10g_resume);drivers/net/phy/phy-c45.c +1
-1@@ -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
Quoted 5 lines
You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an emptygen10g_soft_reset. Some PHYs break when you do a reset. So adding agen10g_soft_resetis fine, but don't change this here, without first understanding the history, and talking to Russell King.Quoted 7 lines
Hmm, the reset function only interacts with standard PCS registers, which should always be available ...
Quoted 1 line
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.
Quoted 4 lines
Quoted 3 lines
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
Russell King - ARM Linux
reviewerRe: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Quoted 45 lines · Jose Abreu
Quoted 44 lines · Florian Fainelli
Quoted 43 lines · Russell King - ARM Linux
Quoted 20 lines · Jose Abreu
Hello,
Quoted 12 lines · Andrew Lunn
drivers/net/phy/phy-c45.c struct phy_driver genphy_10g_driver = { · line 387
Quoted 10 lines
EXPORT_SYMBOL_GPL(gen10g_resume);drivers/net/phy/phy-c45.c +1
-1@@ -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
Quoted 5 lines
You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an emptygen10g_soft_reset. Some PHYs break when you do a reset. So adding agen10g_soft_resetis fine, but don't change this here, without first understanding the history, and talking to Russell King.Quoted 7 lines
Hmm, the reset function only interacts with standard PCS registers, which should always be available ...
Quoted 1 line
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.
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
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.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Quoted 46 lines · Russell King - ARM Linux
Quoted 45 lines · Jose Abreu
Quoted 44 lines · Florian Fainelli
Quoted 43 lines · Russell King - ARM Linux
Quoted 20 lines · Jose Abreu
Hello,
Quoted 12 lines · Andrew Lunn
Review struct phy_driver genphy_10g_driver = { · line 387
Quoted 10 lines
EXPORT_SYMBOL_GPL(gen10g_resume);(quoted diff) +1
-1@@ -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
Quoted 5 lines
You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an emptygen10g_soft_reset. Some PHYs break when you do a reset. So adding agen10g_soft_resetis fine, but don't change this here, without first understanding the history, and talking to Russell King.Quoted 7 lines
Hmm, the reset function only interacts with standard PCS registers, which should always be available ...
Quoted 1 line
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.
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
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
Russell King - ARM Linux
reviewerRe: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Quoted 13 lines · Jose Abreu
Quoted 2 lines · Russell King - ARM Linux
I have no idea what you're proposing there - your patches weren't copied to me.
They just set / unset
MDIO_CTRL1_LPOWERbit 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 ...
Quoted 4 lines
BTW, I just found out currently Generic 10G Driver is broken without patch 4/4 of this series [1]
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.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Quoted 15 lines · Russell King - ARM Linux
Quoted 14 lines · Jose Abreu
Quoted 2 lines · Russell King - ARM Linux
I have no idea what you're proposing there - your patches weren't copied to me.
They just set / unset
MDIO_CTRL1_LPOWERbit 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]
Quoted 8 lines
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 inphy_attach_direct(). This calls the probe function, which isphy_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
Quoted 24 lines
However,
phy_attach_direct()goes on to callphy_init_hw(), which calls theconfig_init()method. Theconfig_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.
Andrew Lunn
maintainerRe: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
Quoted 5 lines
Since
719655a14971("net: phy: Replace phy driver features u32 withlink_modebitmap"),phy_probe()callsethtool_convert_link_mode_to_legacy_u32()withphydrv->featuresas argument. Since features are NULL, we will get NULL pointer dereference.
Hi Jose
Thanks for pointing that out. I will fix it ASAP.
Andrew
Andrew Lunn
maintainerRe: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
In reply to Jose AbreuQuoted 3 lines
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
PATCH 4/4 net: phy-c45: Populate missing features
In reply to Jose AbreuPopulate the missing features field of Generic 10G PHY Driver. This will be overwritten in .config_init callback so we can just set basic 10G funcionalities in the field.
--- drivers/net/phy/phy-c45.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/phy/phy-c45.c +1 -1
@@ -383,7 +383,7 @@ struct phy_driver genphy_10g_driver = { .name = "Generic 10G PHY", .soft_reset = gen10g_soft_reset, .config_init = gen10g_config_init, - .features = 0, + .features = PHY_10GBIT_FEATURES, .aneg_done = genphy_c45_aneg_done, .config_aneg = gen10g_config_aneg, .read_status = gen10g_read_status,
Signature · 1 line
2.7.4