| Message ID | 20220820224538.59489-4-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, Aug 21, 2022 at 12:45:37AM +0200, Alexander Couzens wrote: > Ensure autonegotiation is enabled. > > Signed-off-by: Alexander Couzens <lynxis@fe80.eu> > --- > drivers/net/ethernet/mediatek/mtk_sgmii.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c > index 782812434367..aa69baf1a42f 100644 > --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c > +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c > @@ -32,12 +32,13 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs) > regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, > SGMII_LINK_TIMER_DEFAULT); > > + /* disable remote fault & enable auto neg */ > regmap_read(mpcs->regmap, SGMSYS_SGMII_MODE, &val); > - val |= SGMII_REMOTE_FAULT_DIS; > + val |= SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN; Does SGMII_SPEED_DUPLEX_AN need to be cleared in mtk_pcs_setup_mode_force(), so mtk_pcs_link_up() can force the duplex setting for base-X protocols?
On Tue, 23 Aug 2022 16:28:47 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Sun, Aug 21, 2022 at 12:45:37AM +0200, Alexander Couzens wrote: > > Ensure autonegotiation is enabled. > > > > Signed-off-by: Alexander Couzens <lynxis@fe80.eu> > > --- > > drivers/net/ethernet/mediatek/mtk_sgmii.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index > > 782812434367..aa69baf1a42f 100644 --- > > a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -32,12 +32,13 @@ > > static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs) > > regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, > > SGMII_LINK_TIMER_DEFAULT); > > + /* disable remote fault & enable auto neg */ > > regmap_read(mpcs->regmap, SGMSYS_SGMII_MODE, &val); > > - val |= SGMII_REMOTE_FAULT_DIS; > > + val |= SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN; > > Does SGMII_SPEED_DUPLEX_AN need to be cleared in > mtk_pcs_setup_mode_force(), so mtk_pcs_link_up() can force the > duplex setting for base-X protocols? > Yes SGMII_SPEED_DUPLEX_AN needs to be cleared to have FORCE_DUPLEX working. But mtk_pcs_setup_mode_force() is clearing it implicit by val &= SGMII_DUPLEX_FULL & ~SGMII_IF_MODE_MASK because it's included in the SGMII_IF_MODE_MASK. I also don't understand why it's forcing it in the mtk_pcs_link_up().
On Fri, Sep 02, 2022 at 05:47:10PM +0200, Alexander 'lynxis' Couzens wrote: > On Tue, 23 Aug 2022 16:28:47 +0100 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > On Sun, Aug 21, 2022 at 12:45:37AM +0200, Alexander Couzens wrote: > > > Ensure autonegotiation is enabled. > > > > > > Signed-off-by: Alexander Couzens <lynxis@fe80.eu> > > > --- > > > drivers/net/ethernet/mediatek/mtk_sgmii.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c > > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index > > > 782812434367..aa69baf1a42f 100644 --- > > > a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ > > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -32,12 +32,13 @@ > > > static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs) > > > regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, > > > SGMII_LINK_TIMER_DEFAULT); > > > + /* disable remote fault & enable auto neg */ > > > regmap_read(mpcs->regmap, SGMSYS_SGMII_MODE, &val); > > > - val |= SGMII_REMOTE_FAULT_DIS; > > > + val |= SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN; > > > > Does SGMII_SPEED_DUPLEX_AN need to be cleared in > > mtk_pcs_setup_mode_force(), so mtk_pcs_link_up() can force the > > duplex setting for base-X protocols? > > > > Yes SGMII_SPEED_DUPLEX_AN needs to be cleared to have FORCE_DUPLEX > working. But mtk_pcs_setup_mode_force() is clearing it implicit by > > val &= SGMII_DUPLEX_FULL & ~SGMII_IF_MODE_MASK It would make the code more reasonable to spell out what is included in SGMII_IF_MODE_MASK. It's: #define SGMII_SPEED_DUPLEX_AN BIT(1) #define SGMII_SPEED_MASK GENMASK(3, 2) #define SGMII_DUPLEX_FULL BIT(4) #define SGMII_IF_MODE_BIT5 BIT(5) I'm guessing no one knows what SGMII_IF_MODE_BIT0 and SGMII_IF_MODE_BIT5 actually do - and as neither seem to be used in the code, the definitions are redundant. > because it's included in the SGMII_IF_MODE_MASK. > I also don't understand why it's forcing it in the > mtk_pcs_link_up(). Please note that I've forgotten the contents of these patches, so these comments may not be entirely accurate... A lot of the code in the driver is quite weird, so when I converted it to phylink_pcs, I tried to keep the decision making the same as in the original code. It would help readability if the decision making was cleaned up - so similar tests in mtk_pcs_link_up() and mtk_pcs_config(). By that, I mean - if the test in mtk_pcs_link_up() is for 802.3z modes, then shouldn't the test in mtk_pcs_config() also be using the same? From what I understand from mtk_eth_soc.c as it originally stood, the PCS driver is only used for SGMII, 1000base-X and 2500base-X. The SGMII duplex setting is changed in mtk_pcs_link_up() only for 802.3z modes - in other words, 1000base-X and 2500base-X. When mtk_pcs_config() is called for 1000base-X and 2500base-X, it calls mtk_pcs_setup_mode_force(). This clears SGMII_AN_ENABLE, disabling autonegotiation, and forcing 1000Mbps. Presumably, this PCS as the code was originally written is only capable of SGMII negotiation. It looks to me like the original code does not support autonegotiation on 802.3z interface modes. Thanks.
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c index 782812434367..aa69baf1a42f 100644 --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -32,12 +32,13 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs) regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, SGMII_LINK_TIMER_DEFAULT); + /* disable remote fault & enable auto neg */ regmap_read(mpcs->regmap, SGMSYS_SGMII_MODE, &val); - val |= SGMII_REMOTE_FAULT_DIS; + val |= SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN; regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val); regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &val); - val |= SGMII_AN_RESTART; + val |= SGMII_AN_RESTART | SGMII_AN_ENABLE; regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val); /* Release PHYA power down state
Ensure autonegotiation is enabled. Signed-off-by: Alexander Couzens <lynxis@fe80.eu> --- drivers/net/ethernet/mediatek/mtk_sgmii.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)