| Message ID | 20230122212153.295387-2-bjorn@mork.no (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fixes for mtk_eth_soc | expand |
On Sun, Jan 22, 2023 at 10:21:51PM +0100, Bjørn Mork wrote: > From: Alexander Couzens <lynxis@fe80.eu> > > 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. > > Only power down when reconfiguring to avoid bouncing the link when there's > no reason to - based on code from Russell King. > > There are cases when the SGMII_PHYA_PWD register contains 0x9 which > prevents SGMII from working. The SGMII still shows link but no traffic > can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was > taken from a good working state of the SGMII interface. > > Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk> > Signed-off-by: Alexander Couzens <lynxis@fe80.eu> > [ bmork: rebased and squashed into one patch ] > Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 ++ > drivers/net/ethernet/mediatek/mtk_sgmii.c | 39 +++++++++++++++------ > 2 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > index dff0e3ad2de6..70e729468a95 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > @@ -1067,11 +1067,13 @@ struct mtk_soc_data { > * SGMII modes > * @ana_rgc3: The offset refers to register ANA_RGC3 related to regmap > * @pcs: Phylink PCS structure > + * @interface: Currently configured interface mode > */ > struct mtk_pcs { > struct regmap *regmap; > u32 ana_rgc3; nit: on x86_64 (at least) there is a 4 byte hole here. It could be filled by the interface field. > struct phylink_pcs pcs; > + phy_interface_t interface; > }; ...
On Sun, 2023-01-22 at 22:21 +0100, Bjørn Mork wrote: > From: Alexander Couzens <lynxis@fe80.eu> > > 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. > > Only power down when reconfiguring to avoid bouncing the link when there's > no reason to - based on code from Russell King. > > There are cases when the SGMII_PHYA_PWD register contains 0x9 which > prevents SGMII from working. The SGMII still shows link but no traffic > can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was > taken from a good working state of the SGMII interface. This looks like a legitimate fix for -net, but we need a suitable Fixes tag pointing to the culprit commit. Please repost including such tag. While at that you could also consider including Simon's suggestion. The following 2 patches looks like new features/refactor that would be more suitable for net-next, and included here due to the code dependency. If so, please repost the later 2 separately for net-next after the fix will land there. Thanks, Paolo
Paolo Abeni <pabeni@redhat.com> writes: > On Sun, 2023-01-22 at 22:21 +0100, Bjørn Mork wrote: >> From: Alexander Couzens <lynxis@fe80.eu> >> >> 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. >> >> Only power down when reconfiguring to avoid bouncing the link when there's >> no reason to - based on code from Russell King. >> >> There are cases when the SGMII_PHYA_PWD register contains 0x9 which >> prevents SGMII from working. The SGMII still shows link but no traffic >> can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was >> taken from a good working state of the SGMII interface. > > This looks like a legitimate fix for -net, but we need a suitable Fixes > tag pointing to the culprit commit. > > Please repost including such tag. While at that you could also consider > including Simon's suggestion. Thanks. Will do. Taking advantage of the hole is a good idea. > The following 2 patches looks like new features/refactor that would be > more suitable for net-next, and included here due to the code > dependency. > > If so, please repost the later 2 separately for net-next after the fix > will land there. I believe all these 3 patches are fixes and therefore appropriate for net. I will verify and add Fixes tags for all of them. Bjørn
On Tue, Jan 24, 2023 at 01:19:15PM +0100, Paolo Abeni wrote: > On Sun, 2023-01-22 at 22:21 +0100, Bjørn Mork wrote: > > From: Alexander Couzens <lynxis@fe80.eu> > > > > 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. > > > > Only power down when reconfiguring to avoid bouncing the link when there's > > no reason to - based on code from Russell King. > > > > There are cases when the SGMII_PHYA_PWD register contains 0x9 which > > prevents SGMII from working. The SGMII still shows link but no traffic > > can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was > > taken from a good working state of the SGMII interface. > > This looks like a legitimate fix for -net, but we need a suitable Fixes > tag pointing to the culprit commit. > > Please repost including such tag. While at that you could also consider > including Simon's suggestion. > > The following 2 patches looks like new features/refactor that would be > more suitable for net-next, and included here due to the code > dependency. I'm not sure why you think that, especially for patch 2. Patch 2 corrects the sense of the duplex bit - the code originally set this for full duplex, but in actual fact, the bit needs to be set for half duplex. I can't see how one could regard that as a feature or a refactor. I'm also not sure how you could regard patch 3 as a refactor. It could be argued that it is a new feature, but it is actually a bug fix for the patch converting the driver to phylink_pcs which omitted setting this, making the pcs_get_state() method rather useless. So I regard all three patches as fixes, not features or refactoring.
On Tue, 2023-01-24 at 12:45 +0000, Russell King (Oracle) wrote: > On Tue, Jan 24, 2023 at 01:19:15PM +0100, Paolo Abeni wrote: > > On Sun, 2023-01-22 at 22:21 +0100, Bjørn Mork wrote: > > > From: Alexander Couzens <lynxis@fe80.eu> > > > > > > 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. > > > > > > Only power down when reconfiguring to avoid bouncing the link when there's > > > no reason to - based on code from Russell King. > > > > > > There are cases when the SGMII_PHYA_PWD register contains 0x9 which > > > prevents SGMII from working. The SGMII still shows link but no traffic > > > can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was > > > taken from a good working state of the SGMII interface. > > > > This looks like a legitimate fix for -net, but we need a suitable Fixes > > tag pointing to the culprit commit. > > > > Please repost including such tag. While at that you could also consider > > including Simon's suggestion. > > > > The following 2 patches looks like new features/refactor that would be > > more suitable for net-next, and included here due to the code > > dependency. > > I'm not sure why you think that, especially for patch 2. Because I misread the patch contents as a simple macro rename (I missed the s/==/!=/ part). > > Patch 2 corrects the sense of the duplex bit - the code originally > set this for full duplex, but in actual fact, the bit needs to be set > for half duplex. I can't see how one could regard that as a feature > or a refactor. > > I'm also not sure how you could regard patch 3 as a refactor. It > could be argued that it is a new feature, but it is actually a bug > fix for the patch converting the driver to phylink_pcs which > omitted setting this, making the pcs_get_state() method rather > useless. Well, it looked more a new feature than a fix to me. The above explanation cleared the matter. A more descriptive commit could have avoid confusion on my side :) > So I regard all three patches as fixes, not features or refactoring. No objections on my side, but please add suitable fixes tag on every patch then. Thanks! Paolo
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h index dff0e3ad2de6..70e729468a95 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h @@ -1067,11 +1067,13 @@ struct mtk_soc_data { * SGMII modes * @ana_rgc3: The offset refers to register ANA_RGC3 related to regmap * @pcs: Phylink PCS structure + * @interface: Currently configured interface mode */ struct mtk_pcs { struct regmap *regmap; u32 ana_rgc3; struct phylink_pcs pcs; + phy_interface_t interface; }; /* struct mtk_sgmii - This is the structure holding sgmii regmap and its diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c index 5c286f2c9418..0a06995099cf 100644 --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -43,11 +43,6 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, int advertise, link_timer; bool changed, use_an; - if (interface == PHY_INTERFACE_MODE_2500BASEX) - rgc3 = RG_PHY_SPEED_3_125G; - else - rgc3 = 0; - advertise = phylink_mii_c22_pcs_encode_advertisement(interface, advertising); if (advertise < 0) @@ -88,9 +83,22 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, bmcr = 0; } - /* Configure the underlying interface speed */ - regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3, - RG_PHY_SPEED_3_125G, rgc3); + if (mpcs->interface != interface) { + /* PHYA power down */ + regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, + SGMII_PHYA_PWD, SGMII_PHYA_PWD); + + if (interface == PHY_INTERFACE_MODE_2500BASEX) + rgc3 = RG_PHY_SPEED_3_125G; + else + rgc3 = 0; + + /* Configure the underlying interface speed */ + regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3, + RG_PHY_SPEED_3_125G, rgc3); + + mpcs->interface = interface; + } /* Update the advertisement, noting whether it has changed */ regmap_update_bits_check(mpcs->regmap, SGMSYS_PCS_ADVERTISE, @@ -108,9 +116,17 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1, SGMII_AN_RESTART | SGMII_AN_ENABLE, bmcr); - /* Release PHYA power down state */ - regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, - SGMII_PHYA_PWD, 0); + /* Release PHYA power down state + * Only removing bit SGMII_PHYA_PWD isn't enough. + * There are cases when the SGMII_PHYA_PWD register contains 0x9 which + * prevents SGMII from working. The SGMII still shows link but no traffic + * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was + * taken from a good working state of the SGMII interface. + * Unknown how much the QPHY needs but it is racy without a sleep. + * Tested on mt7622 & mt7986. + */ + usleep_range(50, 100); + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0); return changed; } @@ -171,6 +187,7 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3) return PTR_ERR(ss->pcs[i].regmap); ss->pcs[i].pcs.ops = &mtk_pcs_ops; + ss->pcs[i].interface = PHY_INTERFACE_MODE_NA; } return 0;