Messages in this thread | | | Subject | Re: [PATCH 2/4] net: mediatek: sgmii: ensure the SGMII PHY is powered down on configuration | From | Paolo Abeni <> | Date | Tue, 23 Aug 2022 18:38:00 +0200 |
| |
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
| |