From 76a4cb755cf911ad5acdb82f7228a9e22addf52b Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Sat, 22 Jul 2023 21:32:49 +0100 Subject: [PATCH 1/4] net: ethernet: mtk_eth_soc: remove incorrect PLL configuration MT7623 GMAC0 attempts to configure the system clocking according to the required speed in the .mac_config callback for non-SGMII, non-baseX and non-TRGMII modes. state->speed setting has never been reliable in the .mac_config callback - there are cases where this is not the link speed, particularly via ethtool paths, so this has always been unreliable (as detailed in phylink's documentation.) There is the additional issue that mtk_gmac0_rgmii_adjust() will only be called if state->interface changes, which means it only configures the system clocking on the very first .mac_config call, which will be made when the network device is first brought up before any link is established. Essentially, this code is incredibly buggy, and probably never worked. Moreover, checking the in-kernel DT files, it seems no platform makes use of this code path. Therefore, let's remove it, and disable interface modes for port 0 that are not SGMII, 1000base-X, 2500base-X or TRGMII on the MT7623. Reviewed-by: Daniel Golle Tested-by: Daniel Golle Tested-by: Frank Wunderlich Signed-off-by: Russell King (Oracle) Signed-off-by: Paolo Abeni --- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 59 ++++++--------------- drivers/net/ethernet/mediatek/mtk_eth_soc.h | 1 + 2 files changed, 17 insertions(+), 43 deletions(-) diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index 18a8aca7944d..fe8b7e38decc 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -385,10 +385,8 @@ static int mt7621_gmac0_rgmii_adjust(struct mtk_eth *eth, } static void mtk_gmac0_rgmii_adjust(struct mtk_eth *eth, - phy_interface_t interface, int speed) + phy_interface_t interface) { - unsigned long rate; - u32 tck, rck, intf; int ret; if (interface == PHY_INTERFACE_MODE_TRGMII) { @@ -399,30 +397,7 @@ static void mtk_gmac0_rgmii_adjust(struct mtk_eth *eth, return; } - if (speed == SPEED_1000) { - intf = INTF_MODE_RGMII_1000; - rate = 250000000; - rck = RCK_CTRL_RGMII_1000; - tck = TCK_CTRL_RGMII_1000; - } else { - intf = INTF_MODE_RGMII_10_100; - rate = 500000000; - rck = RCK_CTRL_RGMII_10_100; - tck = TCK_CTRL_RGMII_10_100; - } - - mtk_w32(eth, intf, INTF_MODE); - - regmap_update_bits(eth->ethsys, ETHSYS_CLKCFG0, - ETHSYS_TRGMII_CLK_SEL362_5, - ETHSYS_TRGMII_CLK_SEL362_5); - - ret = clk_set_rate(eth->clks[MTK_CLK_TRGPLL], rate); - if (ret) - dev_err(eth->dev, "Failed to set trgmii pll: %d\n", ret); - - mtk_w32(eth, rck, TRGMII_RCK_CTRL); - mtk_w32(eth, tck, TRGMII_TCK_CTRL); + dev_err(eth->dev, "Missing PLL configuration, ethernet may not work\n"); } static struct phylink_pcs *mtk_mac_select_pcs(struct phylink_config *config, @@ -498,17 +473,8 @@ static void mtk_mac_config(struct phylink_config *config, unsigned int mode, state->interface)) goto err_phy; } else { - /* FIXME: this is incorrect. Not only does it - * use state->speed (which is not guaranteed - * to be correct) but it also makes use of it - * in a code path that will only be reachable - * when the PHY interface mode changes, not - * when the speed changes. Consequently, RGMII - * is probably broken. - */ mtk_gmac0_rgmii_adjust(mac->hw, - state->interface, - state->speed); + state->interface); /* mt7623_pad_clk_setup */ for (i = 0 ; i < NUM_TRGMII_CTRL; i++) @@ -4366,13 +4332,19 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np) mac->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD; - __set_bit(PHY_INTERFACE_MODE_MII, - mac->phylink_config.supported_interfaces); - __set_bit(PHY_INTERFACE_MODE_GMII, - mac->phylink_config.supported_interfaces); + /* MT7623 gmac0 is now missing its speed-specific PLL configuration + * in its .mac_config method (since state->speed is not valid there. + * Disable support for MII, GMII and RGMII. + */ + if (!mac->hw->soc->disable_pll_modes || mac->id != 0) { + __set_bit(PHY_INTERFACE_MODE_MII, + mac->phylink_config.supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_GMII, + mac->phylink_config.supported_interfaces); - if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_RGMII)) - phy_interface_set_rgmii(mac->phylink_config.supported_interfaces); + if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_RGMII)) + phy_interface_set_rgmii(mac->phylink_config.supported_interfaces); + } if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_TRGMII) && !mac->id) __set_bit(PHY_INTERFACE_MODE_TRGMII, @@ -4845,6 +4817,7 @@ static const struct mtk_soc_data mt7623_data = { .offload_version = 1, .hash_offset = 2, .foe_entry_size = MTK_FOE_ENTRY_V1_SIZE, + .disable_pll_modes = true, .txrx = { .txd_size = sizeof(struct mtk_tx_dma), .rxd_size = sizeof(struct mtk_rx_dma), diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h index 707445f6bcb1..28adda0c90c0 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h @@ -1030,6 +1030,7 @@ struct mtk_soc_data { u16 foe_entry_size; netdev_features_t hw_features; bool has_accounting; + bool disable_pll_modes; struct { u32 txd_size; u32 rxd_size; From 28e1737d2544536c23fc71d3a7f24ce8c9f08eb1 Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Sat, 22 Jul 2023 21:32:54 +0100 Subject: [PATCH 2/4] net: ethernet: mtk_eth_soc: remove mac_pcs_get_state and modernise Remove the .mac_pcs_get_state function, since as far as I can tell is never called - no DT appears to specify an in-band-status management nor SFP support for this driver. Removal of this, along with the previous patch to remove the incorrect clocking configuration, means that the driver becomes non-legacy, so we can remove the "legacy_pre_march2020" status from this driver. Reviewed-by: Daniel Golle Tested-by: Daniel Golle Tested-by: Frank Wunderlich Signed-off-by: Russell King (Oracle) Signed-off-by: Paolo Abeni --- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 35 --------------------- 1 file changed, 35 deletions(-) diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index fe8b7e38decc..7490d48000c2 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -568,38 +568,6 @@ static int mtk_mac_finish(struct phylink_config *config, unsigned int mode, return 0; } -static void mtk_mac_pcs_get_state(struct phylink_config *config, - struct phylink_link_state *state) -{ - struct mtk_mac *mac = container_of(config, struct mtk_mac, - phylink_config); - u32 pmsr = mtk_r32(mac->hw, MTK_MAC_MSR(mac->id)); - - state->link = (pmsr & MAC_MSR_LINK); - state->duplex = (pmsr & MAC_MSR_DPX) >> 1; - - switch (pmsr & (MAC_MSR_SPEED_1000 | MAC_MSR_SPEED_100)) { - case 0: - state->speed = SPEED_10; - break; - case MAC_MSR_SPEED_100: - state->speed = SPEED_100; - break; - case MAC_MSR_SPEED_1000: - state->speed = SPEED_1000; - break; - default: - state->speed = SPEED_UNKNOWN; - break; - } - - state->pause &= (MLO_PAUSE_RX | MLO_PAUSE_TX); - if (pmsr & MAC_MSR_RX_FC) - state->pause |= MLO_PAUSE_RX; - if (pmsr & MAC_MSR_TX_FC) - state->pause |= MLO_PAUSE_TX; -} - static void mtk_mac_link_down(struct phylink_config *config, unsigned int mode, phy_interface_t interface) { @@ -722,7 +690,6 @@ static void mtk_mac_link_up(struct phylink_config *config, static const struct phylink_mac_ops mtk_phylink_ops = { .mac_select_pcs = mtk_mac_select_pcs, - .mac_pcs_get_state = mtk_mac_pcs_get_state, .mac_config = mtk_mac_config, .mac_finish = mtk_mac_finish, .mac_link_down = mtk_mac_link_down, @@ -4327,8 +4294,6 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np) mac->phylink_config.dev = ð->netdev[id]->dev; mac->phylink_config.type = PHYLINK_NETDEV; - /* This driver makes use of state->speed in mac_config */ - mac->phylink_config.legacy_pre_march2020 = true; mac->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD; From 4d72c3bb60dd9d5ea180f157bac72b4458112282 Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Sat, 22 Jul 2023 21:32:59 +0100 Subject: [PATCH 3/4] net: phylink: strip out pre-March 2020 legacy code Strip out all the pre-March 2020 legacy code from phylink now that the last user of it is gone. Reviewed-by: Daniel Golle Tested-by: Daniel Golle Tested-by: Frank Wunderlich Signed-off-by: Russell King (Oracle) Signed-off-by: Paolo Abeni --- drivers/net/phy/phylink.c | 25 ++++------------------ include/linux/phylink.h | 45 ++++++--------------------------------- 2 files changed, 10 insertions(+), 60 deletions(-) diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index f07e496319b4..df413fb15088 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1198,13 +1198,6 @@ static int phylink_change_inband_advert(struct phylink *pl) if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) return 0; - if (!pl->pcs && pl->config->legacy_pre_march2020) { - /* Legacy method */ - phylink_mac_config(pl, &pl->link_config); - phylink_pcs_an_restart(pl); - return 0; - } - phylink_dbg(pl, "%s: mode=%s/%s adv=%*pb pause=%02x\n", __func__, phylink_an_mode_str(pl->cur_link_an_mode), phy_modes(pl->link_config.interface), @@ -1257,9 +1250,6 @@ static void phylink_mac_pcs_get_state(struct phylink *pl, if (pl->pcs) pl->pcs->ops->pcs_get_state(pl->pcs, state); - else if (pl->mac_ops->mac_pcs_get_state && - pl->config->legacy_pre_march2020) - pl->mac_ops->mac_pcs_get_state(pl->config, state); else state->link = 0; } @@ -1492,13 +1482,6 @@ static void phylink_resolve(struct work_struct *w) } phylink_major_config(pl, false, &link_state); pl->link_config.interface = link_state.interface; - } else if (!pl->pcs && pl->config->legacy_pre_march2020) { - /* The interface remains unchanged, only the speed, - * duplex or pause settings have changed. Call the - * old mac_config() method to configure the MAC/PCS - * only if we do not have a legacy MAC driver. - */ - phylink_mac_config(pl, &link_state); } } @@ -3513,7 +3496,7 @@ static void phylink_decode_usgmii_word(struct phylink_link_state *state, * * Parse the Clause 37 or Cisco SGMII link partner negotiation word into * the phylink @state structure. This is suitable to be used for implementing - * the mac_pcs_get_state() member of the struct phylink_mac_ops structure if + * the pcs_get_state() member of the struct phylink_pcs_ops structure if * accessing @bmsr and @lpa cannot be done with MDIO directly. */ void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state, @@ -3563,7 +3546,7 @@ EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_decode_state); * Read the MAC PCS state from the MII device configured in @config and * parse the Clause 37 or Cisco SGMII link partner negotiation word into * the phylink @state structure. This is suitable to be directly plugged - * into the mac_pcs_get_state() member of the struct phylink_mac_ops + * into the pcs_get_state() member of the struct phylink_pcs_ops * structure. */ void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs, @@ -3674,8 +3657,8 @@ EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_config); * clause 37 negotiation. * * Restart the clause 37 negotiation with the link partner. This is - * suitable to be directly plugged into the mac_pcs_get_state() member - * of the struct phylink_mac_ops structure. + * suitable to be directly plugged into the pcs_get_state() member + * of the struct phylink_pcs_ops structure. */ void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs) { diff --git a/include/linux/phylink.h b/include/linux/phylink.h index 9e861c8316d0..789c516c6b4a 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -201,8 +201,6 @@ enum phylink_op_type { * struct phylink_config - PHYLINK configuration structure * @dev: a pointer to a struct device associated with the MAC * @type: operation type of PHYLINK instance - * @legacy_pre_march2020: driver has not been updated for March 2020 updates - * (See commit 7cceb599d15d ("net: phylink: avoid mac_config calls") * @poll_fixed_state: if true, starts link_poll, * if MAC link is at %MLO_AN_FIXED mode. * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM. @@ -216,7 +214,6 @@ enum phylink_op_type { struct phylink_config { struct device *dev; enum phylink_op_type type; - bool legacy_pre_march2020; bool poll_fixed_state; bool mac_managed_pm; bool ovr_an_inband; @@ -230,7 +227,6 @@ struct phylink_config { * struct phylink_mac_ops - MAC operations structure. * @validate: Validate and update the link configuration. * @mac_select_pcs: Select a PCS for the interface mode. - * @mac_pcs_get_state: Read the current link state from the hardware. * @mac_prepare: prepare for a major reconfiguration of the interface. * @mac_config: configure the MAC for the selected mode and state. * @mac_finish: finish a major reconfiguration of the interface. @@ -245,8 +241,6 @@ struct phylink_mac_ops { struct phylink_link_state *state); struct phylink_pcs *(*mac_select_pcs)(struct phylink_config *config, phy_interface_t interface); - void (*mac_pcs_get_state)(struct phylink_config *config, - struct phylink_link_state *state); int (*mac_prepare)(struct phylink_config *config, unsigned int mode, phy_interface_t iface); void (*mac_config)(struct phylink_config *config, unsigned int mode, @@ -312,25 +306,6 @@ void validate(struct phylink_config *config, unsigned long *supported, struct phylink_pcs *mac_select_pcs(struct phylink_config *config, phy_interface_t interface); -/** - * mac_pcs_get_state() - Read the current inband link state from the hardware - * @config: a pointer to a &struct phylink_config. - * @state: a pointer to a &struct phylink_link_state. - * - * Read the current inband link state from the MAC PCS, reporting the - * current speed in @state->speed, duplex mode in @state->duplex, pause - * mode in @state->pause using the %MLO_PAUSE_RX and %MLO_PAUSE_TX bits, - * negotiation completion state in @state->an_complete, and link up state - * in @state->link. If possible, @state->lp_advertising should also be - * populated. - * - * Note: This is a legacy method. This function will not be called unless - * legacy_pre_march2020 is set in &struct phylink_config and there is no - * PCS attached. - */ -void mac_pcs_get_state(struct phylink_config *config, - struct phylink_link_state *state); - /** * mac_prepare() - prepare to change the PHY interface mode * @config: a pointer to a &struct phylink_config. @@ -367,17 +342,9 @@ int mac_prepare(struct phylink_config *config, unsigned int mode, * guaranteed to be correct, and so any mac_config() implementation must * never reference these fields. * - * Note: For legacy March 2020 drivers (drivers with legacy_pre_march2020 set - * in their &phylnk_config and which don't have a PCS), this function will be - * called on each link up event, and to also change the in-band advert. For - * non-legacy drivers, it will only be called to reconfigure the MAC for a - * "major" change in e.g. interface mode. It will not be called for changes - * in speed, duplex or pause modes or to change the in-band advertisement. - * In any case, it is strongly preferred that speed, duplex and pause settings - * are handled in the mac_link_up() method and not in this method. - * - * (this requires a rewrite - please refer to mac_link_up() for situations - * where the PCS and MAC are not tightly integrated.) + * This will only be called to reconfigure the MAC for a "major" change in + * e.g. interface mode. It will not be called for changes in speed, duplex + * or pause modes or to change the in-band advertisement. * * In all negotiation modes, as defined by @mode, @state->pause indicates the * pause settings which should be applied as follows. If %MLO_PAUSE_AN is not @@ -409,7 +376,7 @@ int mac_prepare(struct phylink_config *config, unsigned int mode, * 1000base-X or Cisco SGMII mode depending on the @state->interface * mode). In both cases, link state management (whether the link * is up or not) is performed by the MAC, and reported via the - * mac_pcs_get_state() callback. Changes in link state must be made + * pcs_get_state() callback. Changes in link state must be made * by calling phylink_mac_change(). * * Interface mode specific details are mentioned below. @@ -601,8 +568,8 @@ void pcs_disable(struct phylink_pcs *pcs); * in @state->link. If possible, @state->lp_advertising should also be * populated. * - * When present, this overrides mac_pcs_get_state() in &struct - * phylink_mac_ops. + * When present, this overrides pcs_get_state() in &struct + * phylink_pcs_ops. */ void pcs_get_state(struct phylink_pcs *pcs, struct phylink_link_state *state); From c5714f68a76bcad3d2dbaafc5a7e98096f3b4c75 Mon Sep 17 00:00:00 2001 From: "Russell King (Oracle)" Date: Sat, 22 Jul 2023 21:33:05 +0100 Subject: [PATCH 4/4] net: phylink: explicitly invalidate link_state members in mac_config Explicitly invalidate the phylink_link_state structure members in mac_config that do not contain reliable information for this function, thereby preventing their future incorrect use. Reviewed-by: Daniel Golle Tested-by: Daniel Golle Tested-by: Frank Wunderlich Signed-off-by: Russell King (Oracle) Signed-off-by: Paolo Abeni --- drivers/net/phy/phylink.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index df413fb15088..4f1c8bb199e9 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1066,17 +1066,24 @@ static void phylink_pcs_poll_start(struct phylink *pl) static void phylink_mac_config(struct phylink *pl, const struct phylink_link_state *state) { - phylink_dbg(pl, - "%s: mode=%s/%s/%s/%s/%s adv=%*pb pause=%02x link=%u\n", - __func__, phylink_an_mode_str(pl->cur_link_an_mode), - phy_modes(state->interface), - phy_speed_to_str(state->speed), - phy_duplex_to_str(state->duplex), - phy_rate_matching_to_str(state->rate_matching), - __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising, - state->pause, state->link); + struct phylink_link_state st = *state; - pl->mac_ops->mac_config(pl->config, pl->cur_link_an_mode, state); + /* Stop drivers incorrectly using these */ + linkmode_zero(st.lp_advertising); + st.speed = SPEED_UNKNOWN; + st.duplex = DUPLEX_UNKNOWN; + st.an_complete = false; + st.link = false; + + phylink_dbg(pl, + "%s: mode=%s/%s/%s adv=%*pb pause=%02x\n", + __func__, phylink_an_mode_str(pl->cur_link_an_mode), + phy_modes(st.interface), + phy_rate_matching_to_str(st.rate_matching), + __ETHTOOL_LINK_MODE_MASK_NBITS, st.advertising, + st.pause); + + pl->mac_ops->mac_config(pl->config, pl->cur_link_an_mode, &st); } static void phylink_pcs_an_restart(struct phylink *pl)