| Message ID | E1jqDIk-0004m5-0L@rmk-PC.armlinux.org.uk |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,net-next] net: mtk_eth_soc: use resolved link config for PCS PHY | expand |
On Tue, Jun 30, 2020 at 11:15:42AM +0100, Russell King wrote: > The SGMII PCS PHY needs to be updated with the link configuration in > the mac_link_up() call rather than in mac_config(). However, > mtk_sgmii_setup_mode_force() programs the SGMII block during > mac_config() when using 802.3z interface modes with the link > configuration. > > Split that functionality from mtk_sgmii_setup_mode_force(), moving it > to a new mtk_sgmii_link_up() function, and call it from mac_link_up(). > > This does not look correct to me: 802.3z modes operate at a fixed > speed. The contents of mtk_sgmii_link_up() look more appropriate for > SGMII mode, but the original code definitely did not call > mtk_sgmii_setup_mode_force() for SGMII mode but only 802.3z mode. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > René, can you assist with this patch please - I really think there are > problems with the existing code. You call mtk_sgmii_setup_mode_force() > in a block which is conditionalised as: > > if (state->interface == PHY_INTERFACE_MODE_SGMII || > phy_interface_mode_is_8023z(state->interface)) { > ... > if (state->interface != PHY_INTERFACE_MODE_SGMII) > err = mtk_sgmii_setup_mode_force(eth->sgmii, sid, > state); > > Hence, mtk_sgmii_setup_mode_force() is only called for 1000BASE-X and > 2500BASE-X, which do not support anything but their native speeds. > Yet, mtk_sgmii_setup_mode_force() tries to program the SGMII for 10M > and 100M. > > Note that this patch is more about moving uses of state->{speed,duplex} > into mac_link_up(), rather than fixing this problem, but I don't think > the addition in mtk_mac_link_up(), nor mtk_sgmii_link_up() is of any > use. My Coccinelle script just found this use of state->{speed,duplex} still remaining: if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_TRGMII_MT7621_CLK)) { ... } else { if (state->interface != PHY_INTERFACE_MODE_TRGMII) mtk_gmac0_rgmii_adjust(mac->hw, state->speed); which also needs to be eliminated. Can that also be moved to mtk_mac_link_up()? Thanks. > > Thanks. > > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 9 ++++- > drivers/net/ethernet/mediatek/mtk_eth_soc.h | 3 +- > drivers/net/ethernet/mediatek/mtk_sgmii.c | 37 +++++++++++++++------ > 3 files changed, 36 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > index 20db302d31ce..ef9ec3b6a5c8 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > @@ -326,7 +326,7 @@ static void mtk_mac_config(struct phylink_config *config, unsigned int mode, > /* Setup SGMIISYS with the determined property */ > if (state->interface != PHY_INTERFACE_MODE_SGMII) > err = mtk_sgmii_setup_mode_force(eth->sgmii, sid, > - state); > + state->interface); > else if (phylink_autoneg_inband(mode)) > err = mtk_sgmii_setup_mode_an(eth->sgmii, sid); > > @@ -423,6 +423,13 @@ static void mtk_mac_link_up(struct phylink_config *config, > phylink_config); > u32 mcr = mtk_r32(mac->hw, MTK_MAC_MCR(mac->id)); > > + if (phy_interface_mode_is_8023z(interface)) { > + /* Decide how GMAC and SGMIISYS be mapped */ > + int sid = (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_SGMII)) ? > + 0 : mac->id; > + mtk_sgmii_link_up(eth->sgmii, sid, speed, duplex); > + } > + > mcr &= ~(MAC_MCR_SPEED_100 | MAC_MCR_SPEED_1000 | > MAC_MCR_FORCE_DPX | MAC_MCR_FORCE_TX_FC | > MAC_MCR_FORCE_RX_FC); > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > index 454cfcd465fd..6f4b99bb7bfb 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > @@ -932,7 +932,8 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *np, > u32 ana_rgc3); > int mtk_sgmii_setup_mode_an(struct mtk_sgmii *ss, int id); > int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, > - const struct phylink_link_state *state); > + phy_interface_t interface); > +void mtk_sgmii_link_up(struct mtk_sgmii *ss, int id, int speed, int duplex); > void mtk_sgmii_restart_an(struct mtk_eth *eth, int mac_id); > > int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id); > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c > index 32d83421226a..372c85c830b5 100644 > --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c > +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c > @@ -60,7 +60,7 @@ int mtk_sgmii_setup_mode_an(struct mtk_sgmii *ss, int id) > } > > int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, > - const struct phylink_link_state *state) > + phy_interface_t interface) > { > unsigned int val; > > @@ -69,7 +69,7 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, > > regmap_read(ss->regmap[id], ss->ana_rgc3, &val); > val &= ~RG_PHY_SPEED_MASK; > - if (state->interface == PHY_INTERFACE_MODE_2500BASEX) > + if (interface == PHY_INTERFACE_MODE_2500BASEX) > val |= RG_PHY_SPEED_3_125G; > regmap_write(ss->regmap[id], ss->ana_rgc3, val); > > @@ -78,11 +78,33 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, > val &= ~SGMII_AN_ENABLE; > regmap_write(ss->regmap[id], SGMSYS_PCS_CONTROL_1, val); > > + if (interface == PHY_INTERFACE_MODE_1000BASEX || > + interface == PHY_INTERFACE_MODE_2500BASEX) { > + /* SGMII force mode setting */ > + regmap_read(ss->regmap[id], SGMSYS_SGMII_MODE, &val); > + val &= ~SGMII_IF_MODE_MASK; > + val |= SGMII_SPEED_1000; > + val |= SGMII_DUPLEX_FULL; > + regmap_write(ss->regmap[id], SGMSYS_SGMII_MODE, val); > + } > + > + /* Release PHYA power down state */ > + regmap_read(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, &val); > + val &= ~SGMII_PHYA_PWD; > + regmap_write(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, val); > + > + return 0; > +} > + > +void mtk_sgmii_link_up(struct mtk_sgmii *ss, int id, int speed, int duplex) > +{ > + unsigned int val; > + > /* SGMII force mode setting */ > regmap_read(ss->regmap[id], SGMSYS_SGMII_MODE, &val); > val &= ~SGMII_IF_MODE_MASK; > > - switch (state->speed) { > + switch (speed) { > case SPEED_10: > val |= SGMII_SPEED_10; > break; > @@ -95,17 +117,10 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, > break; > } > > - if (state->duplex == DUPLEX_FULL) > + if (duplex == DUPLEX_FULL) > val |= SGMII_DUPLEX_FULL; > > regmap_write(ss->regmap[id], SGMSYS_SGMII_MODE, val); > - > - /* Release PHYA power down state */ > - regmap_read(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, &val); > - val &= ~SGMII_PHYA_PWD; > - regmap_write(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, val); > - > - return 0; > } > > void mtk_sgmii_restart_an(struct mtk_eth *eth, int mac_id) > -- > 2.20.1 > >
Hi Russel and Sean, Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>: > On Tue, Jun 30, 2020 at 11:15:42AM +0100, Russell King wrote: >> The SGMII PCS PHY needs to be updated with the link configuration in >> the mac_link_up() call rather than in mac_config(). However, >> mtk_sgmii_setup_mode_force() programs the SGMII block during >> mac_config() when using 802.3z interface modes with the link >> configuration. >> >> Split that functionality from mtk_sgmii_setup_mode_force(), moving it >> to a new mtk_sgmii_link_up() function, and call it from mac_link_up(). >> >> This does not look correct to me: 802.3z modes operate at a fixed >> speed. The contents of mtk_sgmii_link_up() look more appropriate for >> SGMII mode, but the original code definitely did not call >> mtk_sgmii_setup_mode_force() for SGMII mode but only 802.3z mode. >> >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >> --- >> René, can you assist with this patch please - I really think there are >> problems with the existing code. You call mtk_sgmii_setup_mode_force() >> in a block which is conditionalised as: >> >> if (state->interface == PHY_INTERFACE_MODE_SGMII || >> phy_interface_mode_is_8023z(state->interface)) { >> ... >> if (state->interface != PHY_INTERFACE_MODE_SGMII) >> err = mtk_sgmii_setup_mode_force(eth->sgmii, sid, >> state); >> >> Hence, mtk_sgmii_setup_mode_force() is only called for 1000BASE-X and >> 2500BASE-X, which do not support anything but their native speeds. >> Yet, mtk_sgmii_setup_mode_force() tries to program the SGMII for 10M >> and 100M. >> >> Note that this patch is more about moving uses of state->{speed,duplex} >> into mac_link_up(), rather than fixing this problem, but I don't think >> the addition in mtk_mac_link_up(), nor mtk_sgmii_link_up() is of any >> use. > > My Coccinelle script just found this use of state->{speed,duplex} still > remaining: > > if (MTK_HAS_CAPS(mac->hw->soc->caps, > MTK_TRGMII_MT7621_CLK)) { > ... > } else { > if (state->interface != > PHY_INTERFACE_MODE_TRGMII) > mtk_gmac0_rgmii_adjust(mac->hw, > state->speed); > > which also needs to be eliminated. Can that also be moved to > mtk_mac_link_up()? I know, you have pointed that out before. But I don't know how to fix mtk_gmac0_rgmii_adjust(). This function changes the PLL of the MAC. But without documentation I am not sure what all the bits are used for. Begin April I had a conversation with Sean about this. I also explained what the issue was. AFAIK he was going to take care of this issue. Sean did you had time to resolve this issue? Greats, René > > Thanks. > >> >> Thanks. >> >> drivers/net/ethernet/mediatek/mtk_eth_soc.c | 9 ++++- >> drivers/net/ethernet/mediatek/mtk_eth_soc.h | 3 +- >> drivers/net/ethernet/mediatek/mtk_sgmii.c | 37 +++++++++++++++------ >> 3 files changed, 36 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c >> b/drivers/net/ethernet/mediatek/mtk_eth_soc.c >> index 20db302d31ce..ef9ec3b6a5c8 100644 >> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c >> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c >> @@ -326,7 +326,7 @@ static void mtk_mac_config(struct >> phylink_config *config, unsigned int mode, >> /* Setup SGMIISYS with the determined property */ >> if (state->interface != PHY_INTERFACE_MODE_SGMII) >> err = mtk_sgmii_setup_mode_force(eth->sgmii, sid, >> - state); >> + state->interface); >> else if (phylink_autoneg_inband(mode)) >> err = mtk_sgmii_setup_mode_an(eth->sgmii, sid); >> >> @@ -423,6 +423,13 @@ static void mtk_mac_link_up(struct >> phylink_config *config, >> phylink_config); >> u32 mcr = mtk_r32(mac->hw, MTK_MAC_MCR(mac->id)); >> >> + if (phy_interface_mode_is_8023z(interface)) { >> + /* Decide how GMAC and SGMIISYS be mapped */ >> + int sid = (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_SGMII)) ? >> + 0 : mac->id; >> + mtk_sgmii_link_up(eth->sgmii, sid, speed, duplex); >> + } >> + >> mcr &= ~(MAC_MCR_SPEED_100 | MAC_MCR_SPEED_1000 | >> MAC_MCR_FORCE_DPX | MAC_MCR_FORCE_TX_FC | >> MAC_MCR_FORCE_RX_FC); >> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h >> b/drivers/net/ethernet/mediatek/mtk_eth_soc.h >> index 454cfcd465fd..6f4b99bb7bfb 100644 >> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h >> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h >> @@ -932,7 +932,8 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct >> device_node *np, >> u32 ana_rgc3); >> int mtk_sgmii_setup_mode_an(struct mtk_sgmii *ss, int id); >> int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, >> - const struct phylink_link_state *state); >> + phy_interface_t interface); >> +void mtk_sgmii_link_up(struct mtk_sgmii *ss, int id, int speed, >> int duplex); >> void mtk_sgmii_restart_an(struct mtk_eth *eth, int mac_id); >> >> int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id); >> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c >> b/drivers/net/ethernet/mediatek/mtk_sgmii.c >> index 32d83421226a..372c85c830b5 100644 >> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c >> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c >> @@ -60,7 +60,7 @@ int mtk_sgmii_setup_mode_an(struct mtk_sgmii *ss, int id) >> } >> >> int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, >> - const struct phylink_link_state *state) >> + phy_interface_t interface) >> { >> unsigned int val; >> >> @@ -69,7 +69,7 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii >> *ss, int id, >> >> regmap_read(ss->regmap[id], ss->ana_rgc3, &val); >> val &= ~RG_PHY_SPEED_MASK; >> - if (state->interface == PHY_INTERFACE_MODE_2500BASEX) >> + if (interface == PHY_INTERFACE_MODE_2500BASEX) >> val |= RG_PHY_SPEED_3_125G; >> regmap_write(ss->regmap[id], ss->ana_rgc3, val); >> >> @@ -78,11 +78,33 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii >> *ss, int id, >> val &= ~SGMII_AN_ENABLE; >> regmap_write(ss->regmap[id], SGMSYS_PCS_CONTROL_1, val); >> >> + if (interface == PHY_INTERFACE_MODE_1000BASEX || >> + interface == PHY_INTERFACE_MODE_2500BASEX) { >> + /* SGMII force mode setting */ >> + regmap_read(ss->regmap[id], SGMSYS_SGMII_MODE, &val); >> + val &= ~SGMII_IF_MODE_MASK; >> + val |= SGMII_SPEED_1000; >> + val |= SGMII_DUPLEX_FULL; >> + regmap_write(ss->regmap[id], SGMSYS_SGMII_MODE, val); >> + } >> + >> + /* Release PHYA power down state */ >> + regmap_read(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, &val); >> + val &= ~SGMII_PHYA_PWD; >> + regmap_write(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, val); >> + >> + return 0; >> +} >> + >> +void mtk_sgmii_link_up(struct mtk_sgmii *ss, int id, int speed, int duplex) >> +{ >> + unsigned int val; >> + >> /* SGMII force mode setting */ >> regmap_read(ss->regmap[id], SGMSYS_SGMII_MODE, &val); >> val &= ~SGMII_IF_MODE_MASK; >> >> - switch (state->speed) { >> + switch (speed) { >> case SPEED_10: >> val |= SGMII_SPEED_10; >> break; >> @@ -95,17 +117,10 @@ int mtk_sgmii_setup_mode_force(struct >> mtk_sgmii *ss, int id, >> break; >> } >> >> - if (state->duplex == DUPLEX_FULL) >> + if (duplex == DUPLEX_FULL) >> val |= SGMII_DUPLEX_FULL; >> >> regmap_write(ss->regmap[id], SGMSYS_SGMII_MODE, val); >> - >> - /* Release PHYA power down state */ >> - regmap_read(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, &val); >> - val &= ~SGMII_PHYA_PWD; >> - regmap_write(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, val); >> - >> - return 0; >> } >> >> void mtk_sgmii_restart_an(struct mtk_eth *eth, int mac_id) >> -- >> 2.20.1 >> >> > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Jun 30, 2020 at 10:13:08PM +0000, René van Dorst wrote: > Hi Russel and Sean, > > Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>: > > > On Tue, Jun 30, 2020 at 11:15:42AM +0100, Russell King wrote: > > > The SGMII PCS PHY needs to be updated with the link configuration in > > > the mac_link_up() call rather than in mac_config(). However, > > > mtk_sgmii_setup_mode_force() programs the SGMII block during > > > mac_config() when using 802.3z interface modes with the link > > > configuration. > > > > > > Split that functionality from mtk_sgmii_setup_mode_force(), moving it > > > to a new mtk_sgmii_link_up() function, and call it from mac_link_up(). > > > > > > This does not look correct to me: 802.3z modes operate at a fixed > > > speed. The contents of mtk_sgmii_link_up() look more appropriate for > > > SGMII mode, but the original code definitely did not call > > > mtk_sgmii_setup_mode_force() for SGMII mode but only 802.3z mode. > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > --- > > > René, can you assist with this patch please - I really think there are > > > problems with the existing code. You call mtk_sgmii_setup_mode_force() > > > in a block which is conditionalised as: > > > > > > if (state->interface == PHY_INTERFACE_MODE_SGMII || > > > phy_interface_mode_is_8023z(state->interface)) { > > > ... > > > if (state->interface != PHY_INTERFACE_MODE_SGMII) > > > err = mtk_sgmii_setup_mode_force(eth->sgmii, sid, > > > state); > > > > > > Hence, mtk_sgmii_setup_mode_force() is only called for 1000BASE-X and > > > 2500BASE-X, which do not support anything but their native speeds. > > > Yet, mtk_sgmii_setup_mode_force() tries to program the SGMII for 10M > > > and 100M. > > > > > > Note that this patch is more about moving uses of state->{speed,duplex} > > > into mac_link_up(), rather than fixing this problem, but I don't think > > > the addition in mtk_mac_link_up(), nor mtk_sgmii_link_up() is of any > > > use. > > > > My Coccinelle script just found this use of state->{speed,duplex} still > > remaining: > > > > if (MTK_HAS_CAPS(mac->hw->soc->caps, > > MTK_TRGMII_MT7621_CLK)) { > > ... > > } else { > > if (state->interface != > > PHY_INTERFACE_MODE_TRGMII) > > mtk_gmac0_rgmii_adjust(mac->hw, > > state->speed); > > > > which also needs to be eliminated. Can that also be moved to > > mtk_mac_link_up()? > > I know, you have pointed that out before. But I don't know how to fix > mtk_gmac0_rgmii_adjust(). This function changes the PLL of the MAC. But > without documentation I am not sure what all the bits are used for. I'd forgotten... > Begin April I had a conversation with Sean about this. I also explained what > the issue was. AFAIK he was going to take care of this issue. > > Sean did you had time to resolve this issue? Well, I think the code as it stands is quite broken. If we start a bit earlier in mtk_mac_config(), we have this: if (!MTK_HAS_CAPS(eth->soc->caps, MTK_SOC_MT7628) && mac->interface != state->interface) { which prevents us entering this block unless the interface mode has changed and we are not MT7628. This block of code includes the two calls to mtk_gmac0_rgmii_adjust(), which are dependent on state->speed. Since mac->interface starts off as PHY_INTERFACE_MODE_NA, the first time we head into mtk_mac_config(), the interface mode will be different, and we will enter this block of code, maybe calling down into mtk_gmac0_rgmii_adjust() if appropriate. The first call will be via phylink_start(), which will call it with the initial configuration - the link will be down, and state->speed will be SPEED_UNKNOWN. So, the various tests inside mtk_gmac0_rgmii_adjust() for speed == SPEED_1000 will all be false, meaning it'll program it as if for 10M or 100M speeds. When the link comes up, yes, mtk_mac_config() will be called again with the link parameters, but state->interface will now match mac->interface - so the block of code containing the call to mtk_gmac0_rgmii_adjust() will not be entered, and so none of that code gets executed when the link comes up/down. Now, if I dig out object 8ddbb8dcf032 from the git repository, which was the state of the file immedately prior to the phylink conversion, I find: static void mtk_phy_link_adjust(struct net_device *dev) { This is the function that phylib would call when the link comes up or down. It tests for MTK_RESETTING, starts preparing a value for mcr, and then: if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_GMAC1_TRGMII) && !mac->id) { if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_TRGMII_MT7621_CLK)) { if (mt7621_gmac0_rgmii_adjust(mac->hw, dev->phydev->interface)) return; } else { if (!mac->trgmii) mtk_gmac0_rgmii_adjust(mac->hw, dev->phydev->speed); } } It then finishes creating a value for mcr, before writing it to the register, and printing the link status to the kernel log. Hence, mtk_gmac0_rgmii_adjust() would've been called every time there's a change of link state, and is expected to be passed the current speed. There seems to be a difference in behaviour between the pre-phylink and post-phylink drivers, and I think moving mtk_gmac0_rgmii_adjust() into mtk_mac_link_up() would be a definite improvement, possibly even a regression fix. However, it would be reasonable to assume that there should be reports that mtk_eth_soc doesn't work if this were the case. So... odd.
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index 20db302d31ce..ef9ec3b6a5c8 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -326,7 +326,7 @@ static void mtk_mac_config(struct phylink_config *config, unsigned int mode, /* Setup SGMIISYS with the determined property */ if (state->interface != PHY_INTERFACE_MODE_SGMII) err = mtk_sgmii_setup_mode_force(eth->sgmii, sid, - state); + state->interface); else if (phylink_autoneg_inband(mode)) err = mtk_sgmii_setup_mode_an(eth->sgmii, sid); @@ -423,6 +423,13 @@ static void mtk_mac_link_up(struct phylink_config *config, phylink_config); u32 mcr = mtk_r32(mac->hw, MTK_MAC_MCR(mac->id)); + if (phy_interface_mode_is_8023z(interface)) { + /* Decide how GMAC and SGMIISYS be mapped */ + int sid = (MTK_HAS_CAPS(eth->soc->caps, MTK_SHARED_SGMII)) ? + 0 : mac->id; + mtk_sgmii_link_up(eth->sgmii, sid, speed, duplex); + } + mcr &= ~(MAC_MCR_SPEED_100 | MAC_MCR_SPEED_1000 | MAC_MCR_FORCE_DPX | MAC_MCR_FORCE_TX_FC | MAC_MCR_FORCE_RX_FC); diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h index 454cfcd465fd..6f4b99bb7bfb 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h @@ -932,7 +932,8 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *np, u32 ana_rgc3); int mtk_sgmii_setup_mode_an(struct mtk_sgmii *ss, int id); int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, - const struct phylink_link_state *state); + phy_interface_t interface); +void mtk_sgmii_link_up(struct mtk_sgmii *ss, int id, int speed, int duplex); void mtk_sgmii_restart_an(struct mtk_eth *eth, int mac_id); int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id); diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c index 32d83421226a..372c85c830b5 100644 --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -60,7 +60,7 @@ int mtk_sgmii_setup_mode_an(struct mtk_sgmii *ss, int id) } int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, - const struct phylink_link_state *state) + phy_interface_t interface) { unsigned int val; @@ -69,7 +69,7 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, regmap_read(ss->regmap[id], ss->ana_rgc3, &val); val &= ~RG_PHY_SPEED_MASK; - if (state->interface == PHY_INTERFACE_MODE_2500BASEX) + if (interface == PHY_INTERFACE_MODE_2500BASEX) val |= RG_PHY_SPEED_3_125G; regmap_write(ss->regmap[id], ss->ana_rgc3, val); @@ -78,11 +78,33 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, val &= ~SGMII_AN_ENABLE; regmap_write(ss->regmap[id], SGMSYS_PCS_CONTROL_1, val); + if (interface == PHY_INTERFACE_MODE_1000BASEX || + interface == PHY_INTERFACE_MODE_2500BASEX) { + /* SGMII force mode setting */ + regmap_read(ss->regmap[id], SGMSYS_SGMII_MODE, &val); + val &= ~SGMII_IF_MODE_MASK; + val |= SGMII_SPEED_1000; + val |= SGMII_DUPLEX_FULL; + regmap_write(ss->regmap[id], SGMSYS_SGMII_MODE, val); + } + + /* Release PHYA power down state */ + regmap_read(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, &val); + val &= ~SGMII_PHYA_PWD; + regmap_write(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, val); + + return 0; +} + +void mtk_sgmii_link_up(struct mtk_sgmii *ss, int id, int speed, int duplex) +{ + unsigned int val; + /* SGMII force mode setting */ regmap_read(ss->regmap[id], SGMSYS_SGMII_MODE, &val); val &= ~SGMII_IF_MODE_MASK; - switch (state->speed) { + switch (speed) { case SPEED_10: val |= SGMII_SPEED_10; break; @@ -95,17 +117,10 @@ int mtk_sgmii_setup_mode_force(struct mtk_sgmii *ss, int id, break; } - if (state->duplex == DUPLEX_FULL) + if (duplex == DUPLEX_FULL) val |= SGMII_DUPLEX_FULL; regmap_write(ss->regmap[id], SGMSYS_SGMII_MODE, val); - - /* Release PHYA power down state */ - regmap_read(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, &val); - val &= ~SGMII_PHYA_PWD; - regmap_write(ss->regmap[id], SGMSYS_QPHY_PWR_STATE_CTRL, val); - - return 0; } void mtk_sgmii_restart_an(struct mtk_eth *eth, int mac_id)
The SGMII PCS PHY needs to be updated with the link configuration in the mac_link_up() call rather than in mac_config(). However, mtk_sgmii_setup_mode_force() programs the SGMII block during mac_config() when using 802.3z interface modes with the link configuration. Split that functionality from mtk_sgmii_setup_mode_force(), moving it to a new mtk_sgmii_link_up() function, and call it from mac_link_up(). This does not look correct to me: 802.3z modes operate at a fixed speed. The contents of mtk_sgmii_link_up() look more appropriate for SGMII mode, but the original code definitely did not call mtk_sgmii_setup_mode_force() for SGMII mode but only 802.3z mode. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- René, can you assist with this patch please - I really think there are problems with the existing code. You call mtk_sgmii_setup_mode_force() in a block which is conditionalised as: if (state->interface == PHY_INTERFACE_MODE_SGMII || phy_interface_mode_is_8023z(state->interface)) { ... if (state->interface != PHY_INTERFACE_MODE_SGMII) err = mtk_sgmii_setup_mode_force(eth->sgmii, sid, state); Hence, mtk_sgmii_setup_mode_force() is only called for 1000BASE-X and 2500BASE-X, which do not support anything but their native speeds. Yet, mtk_sgmii_setup_mode_force() tries to program the SGMII for 10M and 100M. Note that this patch is more about moving uses of state->{speed,duplex} into mac_link_up(), rather than fixing this problem, but I don't think the addition in mtk_mac_link_up(), nor mtk_sgmii_link_up() is of any use. Thanks. drivers/net/ethernet/mediatek/mtk_eth_soc.c | 9 ++++- drivers/net/ethernet/mediatek/mtk_eth_soc.h | 3 +- drivers/net/ethernet/mediatek/mtk_sgmii.c | 37 +++++++++++++++------ 3 files changed, 36 insertions(+), 13 deletions(-)