| Message ID | 20220820224538.59489-3-lynxis@fe80.eu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mediatek: sgmii: add support to change interface parameter while running | expand |
On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote: > The code expect the PHY to be in power down which is only true after reset. > Allow changes of the SGMII parameters more than once. > > Signed-off-by: Alexander Couzens <lynxis@fe80.eu> > --- > drivers/net/ethernet/mediatek/mtk_sgmii.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c > index a01bb20ea957..782812434367 100644 > --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c > +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c > @@ -7,6 +7,7 @@ > * > */ > > +#include <linux/delay.h> > #include <linux/mfd/syscon.h> > #include <linux/of.h> > #include <linux/phylink.h> > @@ -24,6 +25,9 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs) > { > unsigned int val; > > + /* PHYA power down */ > + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite the full register contents? > + > /* Setup the link timer and QPHY power up inside SGMIISYS */ > regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, > SGMII_LINK_TIMER_DEFAULT); > @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs) > val |= SGMII_AN_RESTART; > regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val); > > + /* Release PHYA power down state > + * unknown how much the QPHY needs but it is racy without a sleep > + */ > + usleep_range(50, 100); Ouch, this looks fragile, without any related H/W specification. > regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0); > > return 0; > @@ -50,6 +58,9 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs, > { > unsigned int val; > > + /* PHYA power down */ > + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); > + > regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); > val &= ~RG_PHY_SPEED_MASK; > if (interface == PHY_INTERFACE_MODE_2500BASEX) > @@ -67,7 +78,10 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs, > val |= SGMII_SPEED_1000; > regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val); > > - /* Release PHYA power down state */ > + /* Release PHYA power down state > + * unknown how much the QPHY needs but it is racy without a sleep > + */ > + usleep_range(50, 100); Same here. Thanks! Paolo
On Tue, 23 Aug 2022 15:18:31 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote: > > The code expect the PHY to be in power down which is only true > > after reset. Allow changes of the SGMII parameters more than once. > > > > Signed-off-by: Alexander Couzens <lynxis@fe80.eu> > > --- > > drivers/net/ethernet/mediatek/mtk_sgmii.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index > > a01bb20ea957..782812434367 100644 --- > > a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -7,6 +7,7 @@ > > * > > */ > > > > +#include <linux/delay.h> > > #include <linux/mfd/syscon.h> > > #include <linux/of.h> > > #include <linux/phylink.h> > > @@ -24,6 +25,9 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs > > *mpcs) { > > unsigned int val; > > > > + /* PHYA power down */ > > + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, > > SGMII_PHYA_PWD); > > in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code > carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite > the full register contents? I've read out the register without my patch and it's 0x0. The old driver worked as long the engine came out of reset. When writing the single bit SGMII_PHYA_PWD (0x10), the register might end up containing 0x19 and as long 0x9 is in the register the link doesn't work. I've tested the driver with a mt7622 and Daniel Golle tested it with a mt7986. > > > + > > /* Setup the link timer and QPHY power up inside SGMIISYS > > */ regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, > > SGMII_LINK_TIMER_DEFAULT); > > @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs > > *mpcs) val |= SGMII_AN_RESTART; > > regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val); > > > > + /* Release PHYA power down state > > + * unknown how much the QPHY needs but it is racy without > > a sleep > > + */ > > + usleep_range(50, 100); > > Ouch, this looks fragile, without any related H/W specification. The datasheet [1] doesn't say anything about it. I'ven't found a mediatek SDK which adds a usleep(). It seems they always expect the SGMII came out of reset and don't change after initial configured. But without it, it's racy. [1] MT7622 Reference Manual, v1.0, 2018-12-19, 1972 pages > > > regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0); > > > > return 0; > > @@ -50,6 +58,9 @@ static int mtk_pcs_setup_mode_force(struct > > mtk_pcs *mpcs, { > > unsigned int val; > > > > + /* PHYA power down */ > > + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, > > SGMII_PHYA_PWD); + > > regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); > > val &= ~RG_PHY_SPEED_MASK; > > if (interface == PHY_INTERFACE_MODE_2500BASEX) > > @@ -67,7 +78,10 @@ static int mtk_pcs_setup_mode_force(struct > > mtk_pcs *mpcs, val |= SGMII_SPEED_1000; > > regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val); > > > > - /* Release PHYA power down state */ > > + /* Release PHYA power down state > > + * unknown how much the QPHY needs but it is racy without > > a sleep > > + */ > > + usleep_range(50, 100); > > Same here. Best, lynxis
On Tue, 2022-08-23 at 16:17 +0200, Alexander 'lynxis' Couzens wrote: > On Tue, 23 Aug 2022 15:18:31 +0200 > Paolo Abeni <pabeni@redhat.com> wrote: > > > On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote: > > > The code expect the PHY to be in power down which is only true > > > after reset. Allow changes of the SGMII parameters more than once. > > > > > > Signed-off-by: Alexander Couzens <lynxis@fe80.eu> > > > --- > > > drivers/net/ethernet/mediatek/mtk_sgmii.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c > > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index > > > a01bb20ea957..782812434367 100644 --- > > > a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ > > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -7,6 +7,7 @@ > > > * > > > */ > > > > > > +#include <linux/delay.h> > > > #include <linux/mfd/syscon.h> > > > #include <linux/of.h> > > > #include <linux/phylink.h> > > > @@ -24,6 +25,9 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs > > > *mpcs) { > > > unsigned int val; > > > > > > + /* PHYA power down */ > > > + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, > > > SGMII_PHYA_PWD); > > > > in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code > > carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite > > the full register contents? > > I've read out the register without my patch and it's 0x0. The old driver > worked as long the engine came out of reset. > When writing the single bit SGMII_PHYA_PWD (0x10), the register might > end up containing 0x19 and as long 0x9 is in the register the link > doesn't work. > > I've tested the driver with a mt7622 and Daniel Golle tested it with a > mt7986. > > > > > > > + > > > /* Setup the link timer and QPHY power up inside SGMIISYS > > > */ regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, > > > SGMII_LINK_TIMER_DEFAULT); > > > @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs > > > *mpcs) val |= SGMII_AN_RESTART; > > > regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val); > > > > > > + /* Release PHYA power down state > > > + * unknown how much the QPHY needs but it is racy without > > > a sleep > > > + */ > > > + usleep_range(50, 100); > > > > Ouch, this looks fragile, without any related H/W specification. > > The datasheet [1] doesn't say anything about it. I'ven't found a > mediatek SDK which adds a usleep(). It seems they always expect the > SGMII came out of reset and don't change after initial configured. > But without it, it's racy. > > [1] MT7622 Reference Manual, v1.0, 2018-12-19, 1972 pages I see. Since it looks like a new revision of this patchset will be needed - as per Russell comments - I suggest to extend/replace the comments with something more alike this longer description, it will make the future mainteinance simpler. Thanks, Paolo
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c index a01bb20ea957..782812434367 100644 --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -7,6 +7,7 @@ * */ +#include <linux/delay.h> #include <linux/mfd/syscon.h> #include <linux/of.h> #include <linux/phylink.h> @@ -24,6 +25,9 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs) { unsigned int val; + /* PHYA power down */ + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); + /* Setup the link timer and QPHY power up inside SGMIISYS */ regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, SGMII_LINK_TIMER_DEFAULT); @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs) val |= SGMII_AN_RESTART; regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val); + /* Release PHYA power down state + * unknown how much the QPHY needs but it is racy without a sleep + */ + usleep_range(50, 100); regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0); return 0; @@ -50,6 +58,9 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs, { unsigned int val; + /* PHYA power down */ + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); + regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); val &= ~RG_PHY_SPEED_MASK; if (interface == PHY_INTERFACE_MODE_2500BASEX) @@ -67,7 +78,10 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs, val |= SGMII_SPEED_1000; regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val); - /* Release PHYA power down state */ + /* Release PHYA power down state + * unknown how much the QPHY needs but it is racy without a sleep + */ + usleep_range(50, 100); regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0); return 0;
The code expect the PHY to be in power down which is only true after reset. Allow changes of the SGMII parameters more than once. Signed-off-by: Alexander Couzens <lynxis@fe80.eu> --- drivers/net/ethernet/mediatek/mtk_sgmii.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)