| Message ID | 20190624145251.4849-2-opensource@vdorst.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: MT7530: Convert to PHYLINK and add support for port 5 | expand |
Hi, On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote: > Convert mt7530 to PHYLINK API > > Signed-off-by: René van Dorst <opensource@vdorst.com> > --- > drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++---------- > drivers/net/dsa/mt7530.h | 9 ++ > 2 files changed, 187 insertions(+), 59 deletions(-) > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 3181e95586d6..9c5e4dd00826 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -13,7 +13,7 @@ > #include <linux/of_mdio.h> > #include <linux/of_net.h> > #include <linux/of_platform.h> > -#include <linux/phy.h> > +#include <linux/phylink.h> > #include <linux/regmap.h> > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset) > return ARRAY_SIZE(mt7530_mib); > } > > -static void mt7530_adjust_link(struct dsa_switch *ds, int port, > - struct phy_device *phydev) > -{ > - struct mt7530_priv *priv = ds->priv; > - > - if (phy_is_pseudo_fixed_link(phydev)) { > - dev_dbg(priv->dev, "phy-mode for master device = %x\n", > - phydev->interface); > - > - /* Setup TX circuit incluing relevant PAD and driving */ > - mt7530_pad_clk_setup(ds, phydev->interface); > - > - if (priv->id == ID_MT7530) { > - /* Setup RX circuit, relevant PAD and driving on the > - * host which must be placed after the setup on the > - * device side is all finished. > - */ > - mt7623_pad_clk_setup(ds); > - } > - } else { > - u16 lcl_adv = 0, rmt_adv = 0; > - u8 flowctrl; > - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE; > - > - switch (phydev->speed) { > - case SPEED_1000: > - mcr |= PMCR_FORCE_SPEED_1000; > - break; > - case SPEED_100: > - mcr |= PMCR_FORCE_SPEED_100; > - break; > - } > - > - if (phydev->link) > - mcr |= PMCR_FORCE_LNK; > - > - if (phydev->duplex) { > - mcr |= PMCR_FORCE_FDX; > - > - if (phydev->pause) > - rmt_adv = LPA_PAUSE_CAP; > - if (phydev->asym_pause) > - rmt_adv |= LPA_PAUSE_ASYM; > - > - lcl_adv = linkmode_adv_to_lcl_adv_t( > - phydev->advertising); > - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv); > - > - if (flowctrl & FLOW_CTRL_TX) > - mcr |= PMCR_TX_FC_EN; > - if (flowctrl & FLOW_CTRL_RX) > - mcr |= PMCR_RX_FC_EN; > - } > - mt7530_write(priv, MT7530_PMCR_P(port), mcr); > - } > -} > - > static int > mt7530_cpu_port_enable(struct mt7530_priv *priv, > int port) > @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds) > return 0; > } > > +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port, > + unsigned int mode, > + const struct phylink_link_state *state) > +{ > + struct mt7530_priv *priv = ds->priv; > + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | > + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN; > + > + switch (port) { > + case 0: /* Internal phy */ > + case 1: > + case 2: > + case 3: > + case 4: > + if (state->interface != PHY_INTERFACE_MODE_GMII) > + goto unsupported; > + break; > + /* case 5: Port 5 is not supported! */ > + case 6: /* 1st cpu port */ > + if (state->interface != PHY_INTERFACE_MODE_RGMII && > + state->interface != PHY_INTERFACE_MODE_TRGMII) > + goto unsupported; > + > + /* Setup TX circuit incluing relevant PAD and driving */ > + mt7530_pad_clk_setup(ds, state->interface); > + > + if (priv->id == ID_MT7530) { > + /* Setup RX circuit, relevant PAD and driving on the > + * host which must be placed after the setup on the > + * device side is all finished. > + */ > + mt7623_pad_clk_setup(ds); > + } > + break; > + default: > + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); > + return; > + } > + > + if (!state->an_enabled || mode == MLO_AN_FIXED) { > + mcr |= PMCR_FORCE_MODE; > + > + if (state->speed == SPEED_1000) > + mcr |= PMCR_FORCE_SPEED_1000; > + if (state->speed == SPEED_100) > + mcr |= PMCR_FORCE_SPEED_100; > + if (state->duplex == DUPLEX_FULL) > + mcr |= PMCR_FORCE_FDX; > + if (state->link || mode == MLO_AN_FIXED) > + mcr |= PMCR_FORCE_LNK; This should be removed - state->link is not for use in mac_config. Even in fixed mode, the link can be brought up/down by means of a gpio, and this should be dealt with via the mac_link_* functions. > + if (state->pause || phylink_test(state->advertising, Pause)) > + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN; > + if (state->pause & MLO_PAUSE_TX) > + mcr |= PMCR_TX_FC_EN; > + if (state->pause & MLO_PAUSE_RX) > + mcr |= PMCR_RX_FC_EN; This is clearly wrong - if any bit in state->pause is set, then we end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause Pause set in the advertising mask, then both are set. This doesn't seem right - are these bits setting the advertisement, or are they telling the MAC to use flow control? > + } > + > + mt7530_write(priv, MT7530_PMCR_P(port), mcr); > + > + return; > + > +unsupported: > + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n", > + __func__, port, state->interface, phy_modes(state->interface)); > +} > + > +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port, > + unsigned int mode, > + phy_interface_t interface) > +{ > + /* Do nothing */ > +} > + > +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port, > + unsigned int mode, > + phy_interface_t interface, > + struct phy_device *phydev) > +{ > + /* Do nothing */ > +} These two are where you should be forcing the link up or down if required (basically, inband modes should let the link come up/down irrespective of these functions, otherwise it should be forced.) > + > +static void mt7530_phylink_validate(struct dsa_switch *ds, int port, > + unsigned long *supported, > + struct phylink_link_state *state) > +{ > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > + > + switch (port) { > + case 0: /* Internal phy */ > + case 1: > + case 2: > + case 3: > + case 4: > + if (state->interface != PHY_INTERFACE_MODE_NA && > + state->interface != PHY_INTERFACE_MODE_GMII) > + goto unsupported; > + break; > + /* case 5: Port 5 not supported! */ > + case 6: /* 1st cpu port */ > + if (state->interface != PHY_INTERFACE_MODE_RGMII && > + state->interface != PHY_INTERFACE_MODE_TRGMII) PHY_INTERFACE_MODE_NA ? > + goto unsupported; > + break; > + default: > + linkmode_zero(supported); > + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); > + return; > + } > + > + phylink_set(mask, Autoneg); > + phylink_set(mask, Pause); > + phylink_set(mask, Asym_Pause); > + phylink_set(mask, MII); > + > + phylink_set(mask, 10baseT_Half); > + phylink_set(mask, 10baseT_Full); > + phylink_set(mask, 100baseT_Half); > + phylink_set(mask, 100baseT_Full); > + phylink_set(mask, 1000baseT_Full); > + phylink_set(mask, 1000baseT_Half); You seem to be missing phylink_set_port_modes() here. > + > + linkmode_and(supported, supported, mask); > + linkmode_and(state->advertising, state->advertising, mask); > + return; > + > +unsupported: > + linkmode_zero(supported); > + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n", > + __func__, state->interface, phy_modes(state->interface)); Not a good idea to print this at error level; sometimes we just probe for support. Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY interface mode that the MAC can't support - we detect that by the validation failing, and printing a more meaningful message in phylink itself. > +} > + > +static int > +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port, > + struct phylink_link_state *state) > +{ > + struct mt7530_priv *priv = ds->priv; > + u32 pmsr; > + > + if (port < 0 || port >= MT7530_NUM_PORTS) > + return -EINVAL; > + > + pmsr = mt7530_read(priv, MT7530_PMSR_P(port)); > + > + state->link = (pmsr & PMSR_LINK); > + state->an_complete = state->link; > + state->duplex = (pmsr & PMSR_DPX) >> 1; > + > + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) { > + case 0: > + state->speed = SPEED_10; > + break; > + case PMSR_SPEED_100: > + state->speed = SPEED_100; > + break; > + case PMSR_SPEED_1000: > + state->speed = SPEED_1000; > + break; > + default: > + state->speed = SPEED_UNKNOWN; > + break; > + } > + > + state->pause = 0; > + if (pmsr & PMSR_RX_FC) > + state->pause |= MLO_PAUSE_RX; > + if (pmsr & PMSR_TX_FC) > + state->pause |= MLO_PAUSE_TX; > + > + return 1; > +} > + > static const struct dsa_switch_ops mt7530_switch_ops = { > .get_tag_protocol = mtk_get_tag_protocol, > .setup = mt7530_setup, > @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = { > .phy_write = mt7530_phy_write, > .get_ethtool_stats = mt7530_get_ethtool_stats, > .get_sset_count = mt7530_get_sset_count, > - .adjust_link = mt7530_adjust_link, > .port_enable = mt7530_port_enable, > .port_disable = mt7530_port_disable, > .port_stp_state_set = mt7530_stp_state_set, > @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = { > .port_vlan_prepare = mt7530_port_vlan_prepare, > .port_vlan_add = mt7530_port_vlan_add, > .port_vlan_del = mt7530_port_vlan_del, > + .phylink_validate = mt7530_phylink_validate, > + .phylink_mac_link_state = mt7530_phylink_mac_link_state, > + .phylink_mac_config = mt7530_phylink_mac_config, > + .phylink_mac_link_down = mt7530_phylink_mac_link_down, > + .phylink_mac_link_up = mt7530_phylink_mac_link_up, > }; > > static const struct of_device_id mt7530_of_match[] = { > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h > index bfac90f48102..41d9a132ac70 100644 > --- a/drivers/net/dsa/mt7530.h > +++ b/drivers/net/dsa/mt7530.h > @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr { > #define PMCR_FORCE_SPEED_100 BIT(2) > #define PMCR_FORCE_FDX BIT(1) > #define PMCR_FORCE_LNK BIT(0) > +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE > #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ > PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ > PMCR_TX_EN | PMCR_RX_EN | \ > @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr { > PMCR_TX_FC_EN | PMCR_RX_FC_EN) > > #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100) > +#define PMSR_EEE1G BIT(7) > +#define PMSR_EEE100M BIT(6) > +#define PMSR_RX_FC BIT(5) > +#define PMSR_TX_FC BIT(4) > +#define PMSR_SPEED_1000 BIT(3) > +#define PMSR_SPEED_100 BIT(2) > +#define PMSR_DPX BIT(1) > +#define PMSR_LINK BIT(0) > > /* Register for MIB */ > #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100) > -- > 2.20.1 > >
On 6/24/19 9:52 AM, René van Dorst wrote: > Convert mt7530 to PHYLINK API > > Signed-off-by: René van Dorst <opensource@vdorst.com> > --- > drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++---------- > drivers/net/dsa/mt7530.h | 9 ++ > 2 files changed, 187 insertions(+), 59 deletions(-) > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 3181e95586d6..9c5e4dd00826 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -13,7 +13,7 @@ > #include <linux/of_mdio.h> > #include <linux/of_net.h> > #include <linux/of_platform.h> > -#include <linux/phy.h> > +#include <linux/phylink.h> > #include <linux/regmap.h> > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset) > return ARRAY_SIZE(mt7530_mib); > } > > -static void mt7530_adjust_link(struct dsa_switch *ds, int port, > - struct phy_device *phydev) > -{ > - struct mt7530_priv *priv = ds->priv; > - > - if (phy_is_pseudo_fixed_link(phydev)) { > - dev_dbg(priv->dev, "phy-mode for master device = %x\n", > - phydev->interface); > - > - /* Setup TX circuit incluing relevant PAD and driving */ > - mt7530_pad_clk_setup(ds, phydev->interface); > - > - if (priv->id == ID_MT7530) { > - /* Setup RX circuit, relevant PAD and driving on the > - * host which must be placed after the setup on the > - * device side is all finished. > - */ > - mt7623_pad_clk_setup(ds); > - } > - } else { > - u16 lcl_adv = 0, rmt_adv = 0; > - u8 flowctrl; > - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE; > - > - switch (phydev->speed) { > - case SPEED_1000: > - mcr |= PMCR_FORCE_SPEED_1000; > - break; > - case SPEED_100: > - mcr |= PMCR_FORCE_SPEED_100; > - break; > - } > - > - if (phydev->link) > - mcr |= PMCR_FORCE_LNK; > - > - if (phydev->duplex) { > - mcr |= PMCR_FORCE_FDX; > - > - if (phydev->pause) > - rmt_adv = LPA_PAUSE_CAP; > - if (phydev->asym_pause) > - rmt_adv |= LPA_PAUSE_ASYM; > - > - lcl_adv = linkmode_adv_to_lcl_adv_t( > - phydev->advertising); > - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv); > - > - if (flowctrl & FLOW_CTRL_TX) > - mcr |= PMCR_TX_FC_EN; > - if (flowctrl & FLOW_CTRL_RX) > - mcr |= PMCR_RX_FC_EN; > - } > - mt7530_write(priv, MT7530_PMCR_P(port), mcr); > - } > -} > - > static int > mt7530_cpu_port_enable(struct mt7530_priv *priv, > int port) > @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds) > return 0; > } > > +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port, > + unsigned int mode, > + const struct phylink_link_state *state) > +{ > + struct mt7530_priv *priv = ds->priv; > + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | > + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN; > + > + switch (port) { > + case 0: /* Internal phy */ > + case 1: > + case 2: > + case 3: > + case 4: > + if (state->interface != PHY_INTERFACE_MODE_GMII) > + goto unsupported; > + break; > + /* case 5: Port 5 is not supported! */ > + case 6: /* 1st cpu port */ > + if (state->interface != PHY_INTERFACE_MODE_RGMII && > + state->interface != PHY_INTERFACE_MODE_TRGMII) > + goto unsupported; > + > + /* Setup TX circuit incluing relevant PAD and driving */ > + mt7530_pad_clk_setup(ds, state->interface); > + > + if (priv->id == ID_MT7530) { > + /* Setup RX circuit, relevant PAD and driving on the > + * host which must be placed after the setup on the > + * device side is all finished. > + */ > + mt7623_pad_clk_setup(ds); > + } > + break; > + default: > + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); > + return; > + } > + > + if (!state->an_enabled || mode == MLO_AN_FIXED) { > + mcr |= PMCR_FORCE_MODE; > + > + if (state->speed == SPEED_1000) > + mcr |= PMCR_FORCE_SPEED_1000; > + if (state->speed == SPEED_100) > + mcr |= PMCR_FORCE_SPEED_100; I would suggest using the defines #define PMCR_FORCE_SPEED 0x0000000c /* or PMCR_FORCE_SPEED_MASK */ #define PMCR_FORCE_SPEED_10 0x00000000 #define PMCR_FORCE_SPEED_100 0x00000004 #define PMCR_FORCE_SPEED_1000 0x00000008 and a switch statement such as switch (state->speed) { case SPEED_10: mcr |= PMCR_FORCE_SPEED_10; break; case SPEED_100: mcr |= PMCR_FORCE_SPEED_100; break; case SPEED_1000: mcr |= PMCR_FORCE_SPEED_1000; break; } This will compile the same (i.e, the mcr |= 0 will optimize away, etc.), while alleviating the need to intimately know the hardware in order to easily understand what the code is doing at a glance. It's also better form as we're treating the two bits as a composite value, rather than two separate bits. > + if (state->duplex == DUPLEX_FULL) > + mcr |= PMCR_FORCE_FDX; > + if (state->link || mode == MLO_AN_FIXED) > + mcr |= PMCR_FORCE_LNK; > + if (state->pause || phylink_test(state->advertising, Pause)) > + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN; > + if (state->pause & MLO_PAUSE_TX) > + mcr |= PMCR_TX_FC_EN; > + if (state->pause & MLO_PAUSE_RX) > + mcr |= PMCR_RX_FC_EN; > + } > + > + mt7530_write(priv, MT7530_PMCR_P(port), mcr); > + > + return; > + > +unsupported: > + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n", > + __func__, port, state->interface, phy_modes(state->interface)); > +} > + > +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port, > + unsigned int mode, > + phy_interface_t interface) > +{ > + /* Do nothing */ > +} > + > +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port, > + unsigned int mode, > + phy_interface_t interface, > + struct phy_device *phydev) > +{ > + /* Do nothing */ > +} > + > +static void mt7530_phylink_validate(struct dsa_switch *ds, int port, > + unsigned long *supported, > + struct phylink_link_state *state) > +{ > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > + > + switch (port) { > + case 0: /* Internal phy */ > + case 1: > + case 2: > + case 3: > + case 4: > + if (state->interface != PHY_INTERFACE_MODE_NA && > + state->interface != PHY_INTERFACE_MODE_GMII) > + goto unsupported; > + break; > + /* case 5: Port 5 not supported! */ > + case 6: /* 1st cpu port */ > + if (state->interface != PHY_INTERFACE_MODE_RGMII && > + state->interface != PHY_INTERFACE_MODE_TRGMII) > + goto unsupported; > + break; > + default: > + linkmode_zero(supported); > + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); > + return; > + } > + > + phylink_set(mask, Autoneg); > + phylink_set(mask, Pause); > + phylink_set(mask, Asym_Pause); > + phylink_set(mask, MII); > + > + phylink_set(mask, 10baseT_Half); > + phylink_set(mask, 10baseT_Full); > + phylink_set(mask, 100baseT_Half); > + phylink_set(mask, 100baseT_Full); > + phylink_set(mask, 1000baseT_Full); > + phylink_set(mask, 1000baseT_Half); > + > + linkmode_and(supported, supported, mask); > + linkmode_and(state->advertising, state->advertising, mask); > + return; > + > +unsupported: > + linkmode_zero(supported); > + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n", > + __func__, state->interface, phy_modes(state->interface)); > +} > + > +static int > +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port, > + struct phylink_link_state *state) > +{ > + struct mt7530_priv *priv = ds->priv; > + u32 pmsr; > + > + if (port < 0 || port >= MT7530_NUM_PORTS) > + return -EINVAL; > + > + pmsr = mt7530_read(priv, MT7530_PMSR_P(port)); > + > + state->link = (pmsr & PMSR_LINK); > + state->an_complete = state->link; > + state->duplex = (pmsr & PMSR_DPX) >> 1; > + > + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) { > + case 0: > + state->speed = SPEED_10; > + break; > + case PMSR_SPEED_100: > + state->speed = SPEED_100; > + break; > + case PMSR_SPEED_1000: > + state->speed = SPEED_1000; > + break; > + default: > + state->speed = SPEED_UNKNOWN; > + break; > + } > + Same as above: add PMSR_SPEED_10, and and with PMSR_SPEED (or PMSR_SPEED_MASK if you prefer). > + state->pause = 0; > + if (pmsr & PMSR_RX_FC) > + state->pause |= MLO_PAUSE_RX; > + if (pmsr & PMSR_TX_FC) > + state->pause |= MLO_PAUSE_TX; > + > + return 1; > +} > + > static const struct dsa_switch_ops mt7530_switch_ops = { > .get_tag_protocol = mtk_get_tag_protocol, > .setup = mt7530_setup, > @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = { > .phy_write = mt7530_phy_write, > .get_ethtool_stats = mt7530_get_ethtool_stats, > .get_sset_count = mt7530_get_sset_count, > - .adjust_link = mt7530_adjust_link, > .port_enable = mt7530_port_enable, > .port_disable = mt7530_port_disable, > .port_stp_state_set = mt7530_stp_state_set, > @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = { > .port_vlan_prepare = mt7530_port_vlan_prepare, > .port_vlan_add = mt7530_port_vlan_add, > .port_vlan_del = mt7530_port_vlan_del, > + .phylink_validate = mt7530_phylink_validate, > + .phylink_mac_link_state = mt7530_phylink_mac_link_state, > + .phylink_mac_config = mt7530_phylink_mac_config, > + .phylink_mac_link_down = mt7530_phylink_mac_link_down, > + .phylink_mac_link_up = mt7530_phylink_mac_link_up, > }; > > static const struct of_device_id mt7530_of_match[] = { > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h > index bfac90f48102..41d9a132ac70 100644 > --- a/drivers/net/dsa/mt7530.h > +++ b/drivers/net/dsa/mt7530.h > @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr { > #define PMCR_FORCE_SPEED_100 BIT(2) > #define PMCR_FORCE_FDX BIT(1) > #define PMCR_FORCE_LNK BIT(0) > +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE > #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ > PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ > PMCR_TX_EN | PMCR_RX_EN | \ > @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr { > PMCR_TX_FC_EN | PMCR_RX_FC_EN) > > #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100) > +#define PMSR_EEE1G BIT(7) > +#define PMSR_EEE100M BIT(6) > +#define PMSR_RX_FC BIT(5) > +#define PMSR_TX_FC BIT(4) > +#define PMSR_SPEED_1000 BIT(3) > +#define PMSR_SPEED_100 BIT(2) > +#define PMSR_DPX BIT(1) > +#define PMSR_LINK BIT(0) > > /* Register for MIB */ > #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100) Cheers, Daniel
Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>: Hi Russel, Thanks for your review, see also my comments and questions below. > Hi, > > On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote: >> Convert mt7530 to PHYLINK API >> >> Signed-off-by: René van Dorst <opensource@vdorst.com> >> --- >> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++---------- >> drivers/net/dsa/mt7530.h | 9 ++ >> 2 files changed, 187 insertions(+), 59 deletions(-) >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 3181e95586d6..9c5e4dd00826 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -13,7 +13,7 @@ >> #include <linux/of_mdio.h> >> #include <linux/of_net.h> >> #include <linux/of_platform.h> >> -#include <linux/phy.h> >> +#include <linux/phylink.h> >> #include <linux/regmap.h> >> #include <linux/regulator/consumer.h> >> #include <linux/reset.h> >> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, >> int port, int sset) >> return ARRAY_SIZE(mt7530_mib); >> } >> >> -static void mt7530_adjust_link(struct dsa_switch *ds, int port, >> - struct phy_device *phydev) >> -{ >> - struct mt7530_priv *priv = ds->priv; >> - >> - if (phy_is_pseudo_fixed_link(phydev)) { >> - dev_dbg(priv->dev, "phy-mode for master device = %x\n", >> - phydev->interface); >> - >> - /* Setup TX circuit incluing relevant PAD and driving */ >> - mt7530_pad_clk_setup(ds, phydev->interface); >> - >> - if (priv->id == ID_MT7530) { >> - /* Setup RX circuit, relevant PAD and driving on the >> - * host which must be placed after the setup on the >> - * device side is all finished. >> - */ >> - mt7623_pad_clk_setup(ds); >> - } >> - } else { >> - u16 lcl_adv = 0, rmt_adv = 0; >> - u8 flowctrl; >> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE; >> - >> - switch (phydev->speed) { >> - case SPEED_1000: >> - mcr |= PMCR_FORCE_SPEED_1000; >> - break; >> - case SPEED_100: >> - mcr |= PMCR_FORCE_SPEED_100; >> - break; >> - } >> - >> - if (phydev->link) >> - mcr |= PMCR_FORCE_LNK; >> - >> - if (phydev->duplex) { >> - mcr |= PMCR_FORCE_FDX; >> - >> - if (phydev->pause) >> - rmt_adv = LPA_PAUSE_CAP; >> - if (phydev->asym_pause) >> - rmt_adv |= LPA_PAUSE_ASYM; >> - >> - lcl_adv = linkmode_adv_to_lcl_adv_t( >> - phydev->advertising); >> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv); >> - >> - if (flowctrl & FLOW_CTRL_TX) >> - mcr |= PMCR_TX_FC_EN; >> - if (flowctrl & FLOW_CTRL_RX) >> - mcr |= PMCR_RX_FC_EN; >> - } >> - mt7530_write(priv, MT7530_PMCR_P(port), mcr); >> - } >> -} >> - >> static int >> mt7530_cpu_port_enable(struct mt7530_priv *priv, >> int port) >> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds) >> return 0; >> } >> >> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + const struct phylink_link_state *state) >> +{ >> + struct mt7530_priv *priv = ds->priv; >> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | >> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN; >> + >> + switch (port) { >> + case 0: /* Internal phy */ >> + case 1: >> + case 2: >> + case 3: >> + case 4: >> + if (state->interface != PHY_INTERFACE_MODE_GMII) >> + goto unsupported; >> + break; >> + /* case 5: Port 5 is not supported! */ >> + case 6: /* 1st cpu port */ >> + if (state->interface != PHY_INTERFACE_MODE_RGMII && >> + state->interface != PHY_INTERFACE_MODE_TRGMII) >> + goto unsupported; >> + >> + /* Setup TX circuit incluing relevant PAD and driving */ >> + mt7530_pad_clk_setup(ds, state->interface); >> + >> + if (priv->id == ID_MT7530) { >> + /* Setup RX circuit, relevant PAD and driving on the >> + * host which must be placed after the setup on the >> + * device side is all finished. >> + */ >> + mt7623_pad_clk_setup(ds); >> + } >> + break; >> + default: >> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); >> + return; >> + } >> + >> + if (!state->an_enabled || mode == MLO_AN_FIXED) { >> + mcr |= PMCR_FORCE_MODE; >> + >> + if (state->speed == SPEED_1000) >> + mcr |= PMCR_FORCE_SPEED_1000; >> + if (state->speed == SPEED_100) >> + mcr |= PMCR_FORCE_SPEED_100; >> + if (state->duplex == DUPLEX_FULL) >> + mcr |= PMCR_FORCE_FDX; >> + if (state->link || mode == MLO_AN_FIXED) >> + mcr |= PMCR_FORCE_LNK; > > This should be removed - state->link is not for use in mac_config. > Even in fixed mode, the link can be brought up/down by means of a > gpio, and this should be dealt with via the mac_link_* functions. Maybe I understand it wrong, but is it the intention that in phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always forces into a certain speed/mode/interface. So it never auto-negotiate because phylink select the best configuration for you? Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here and do nothing phylink_link_up()? Other question: Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}? Or only mac_config() and port_disable? > >> + if (state->pause || phylink_test(state->advertising, Pause)) >> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN; >> + if (state->pause & MLO_PAUSE_TX) >> + mcr |= PMCR_TX_FC_EN; >> + if (state->pause & MLO_PAUSE_RX) >> + mcr |= PMCR_RX_FC_EN; > > This is clearly wrong - if any bit in state->pause is set, then we > end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause > Pause set in the advertising mask, then both are set. This doesn't > seem right - are these bits setting the advertisement, or are they > telling the MAC to use flow control? Last one, tell the MAC to use flow control. Qoute of the datasheet: If you want to disable TX and RX flow control disable bit 4 and 5 MAC Control Register: BIT(4): Force TX FC: 0: Disabled 1: Let the MAC transmit a pause frame when operating in full-duplex mode with low internal free memory page count. BIT(5): Force RX FC: 0: Disabled 1: Let the MA accept a pause frame when operating in full-duplex mode. On the current driver both bits are set in a forced-link situation. If we always forces the MAC mode I think I always set these bits and don't anything with the Pause modes? Is that the right way to do it? > >> + } >> + >> + mt7530_write(priv, MT7530_PMCR_P(port), mcr); >> + >> + return; >> + >> +unsupported: >> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n", >> + __func__, port, state->interface, phy_modes(state->interface)); >> +} >> + >> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + phy_interface_t interface) >> +{ >> + /* Do nothing */ >> +} >> + >> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + phy_interface_t interface, >> + struct phy_device *phydev) >> +{ >> + /* Do nothing */ >> +} > > These two are where you should be forcing the link up or down if > required (basically, inband modes should let the link come up/down > irrespective of these functions, otherwise it should be forced.) > OK >> + >> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port, >> + unsigned long *supported, >> + struct phylink_link_state *state) >> +{ >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; >> + >> + switch (port) { >> + case 0: /* Internal phy */ >> + case 1: >> + case 2: >> + case 3: >> + case 4: >> + if (state->interface != PHY_INTERFACE_MODE_NA && >> + state->interface != PHY_INTERFACE_MODE_GMII) >> + goto unsupported; >> + break; >> + /* case 5: Port 5 not supported! */ >> + case 6: /* 1st cpu port */ >> + if (state->interface != PHY_INTERFACE_MODE_RGMII && >> + state->interface != PHY_INTERFACE_MODE_TRGMII) > > PHY_INTERFACE_MODE_NA ? You mean PHY_INTERFACE_MODE_NA is missing? PHY_INTERFACE_MODE_TRGMII is a valid mode for this port. > >> + goto unsupported; >> + break; >> + default: >> + linkmode_zero(supported); >> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); >> + return; >> + } >> + >> + phylink_set(mask, Autoneg); >> + phylink_set(mask, Pause); >> + phylink_set(mask, Asym_Pause); >> + phylink_set(mask, MII); >> + >> + phylink_set(mask, 10baseT_Half); >> + phylink_set(mask, 10baseT_Full); >> + phylink_set(mask, 100baseT_Half); >> + phylink_set(mask, 100baseT_Full); >> + phylink_set(mask, 1000baseT_Full); >> + phylink_set(mask, 1000baseT_Half); > > You seem to be missing phylink_set_port_modes() here. OK > >> + >> + linkmode_and(supported, supported, mask); >> + linkmode_and(state->advertising, state->advertising, mask); >> + return; >> + >> +unsupported: >> + linkmode_zero(supported); >> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n", >> + __func__, state->interface, phy_modes(state->interface)); > > Not a good idea to print this at error level; sometimes we just probe > for support. > > Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY > interface mode that the MAC can't support - we detect that by the > validation failing, and printing a more meaningful message in phylink > itself. OK, It will be removed. > >> +} >> + >> +static int >> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port, >> + struct phylink_link_state *state) >> +{ >> + struct mt7530_priv *priv = ds->priv; >> + u32 pmsr; >> + >> + if (port < 0 || port >= MT7530_NUM_PORTS) >> + return -EINVAL; >> + >> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port)); >> + >> + state->link = (pmsr & PMSR_LINK); >> + state->an_complete = state->link; >> + state->duplex = (pmsr & PMSR_DPX) >> 1; >> + >> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) { >> + case 0: >> + state->speed = SPEED_10; >> + break; >> + case PMSR_SPEED_100: >> + state->speed = SPEED_100; >> + break; >> + case PMSR_SPEED_1000: >> + state->speed = SPEED_1000; >> + break; >> + default: >> + state->speed = SPEED_UNKNOWN; >> + break; >> + } >> + >> + state->pause = 0; >> + if (pmsr & PMSR_RX_FC) >> + state->pause |= MLO_PAUSE_RX; >> + if (pmsr & PMSR_TX_FC) >> + state->pause |= MLO_PAUSE_TX; >> + >> + return 1; >> +} >> + >> static const struct dsa_switch_ops mt7530_switch_ops = { >> .get_tag_protocol = mtk_get_tag_protocol, >> .setup = mt7530_setup, >> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops >> mt7530_switch_ops = { >> .phy_write = mt7530_phy_write, >> .get_ethtool_stats = mt7530_get_ethtool_stats, >> .get_sset_count = mt7530_get_sset_count, >> - .adjust_link = mt7530_adjust_link, >> .port_enable = mt7530_port_enable, >> .port_disable = mt7530_port_disable, >> .port_stp_state_set = mt7530_stp_state_set, >> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops >> mt7530_switch_ops = { >> .port_vlan_prepare = mt7530_port_vlan_prepare, >> .port_vlan_add = mt7530_port_vlan_add, >> .port_vlan_del = mt7530_port_vlan_del, >> + .phylink_validate = mt7530_phylink_validate, >> + .phylink_mac_link_state = mt7530_phylink_mac_link_state, >> + .phylink_mac_config = mt7530_phylink_mac_config, >> + .phylink_mac_link_down = mt7530_phylink_mac_link_down, >> + .phylink_mac_link_up = mt7530_phylink_mac_link_up, >> }; >> >> static const struct of_device_id mt7530_of_match[] = { >> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h >> index bfac90f48102..41d9a132ac70 100644 >> --- a/drivers/net/dsa/mt7530.h >> +++ b/drivers/net/dsa/mt7530.h >> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr { >> #define PMCR_FORCE_SPEED_100 BIT(2) >> #define PMCR_FORCE_FDX BIT(1) >> #define PMCR_FORCE_LNK BIT(0) >> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE >> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ >> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ >> PMCR_TX_EN | PMCR_RX_EN | \ >> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr { >> PMCR_TX_FC_EN | PMCR_RX_FC_EN) >> >> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100) >> +#define PMSR_EEE1G BIT(7) >> +#define PMSR_EEE100M BIT(6) >> +#define PMSR_RX_FC BIT(5) >> +#define PMSR_TX_FC BIT(4) >> +#define PMSR_SPEED_1000 BIT(3) >> +#define PMSR_SPEED_100 BIT(2) >> +#define PMSR_DPX BIT(1) >> +#define PMSR_LINK BIT(0) >> >> /* Register for MIB */ >> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100) >> -- >> 2.20.1 >> >> > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up Greats, René
Quoting Daniel Santos <daniel.santos@pobox.com>: Hi Daniel, > On 6/24/19 9:52 AM, René van Dorst wrote: >> Convert mt7530 to PHYLINK API >> >> Signed-off-by: René van Dorst <opensource@vdorst.com> >> --- >> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++---------- >> drivers/net/dsa/mt7530.h | 9 ++ >> 2 files changed, 187 insertions(+), 59 deletions(-) >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 3181e95586d6..9c5e4dd00826 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -13,7 +13,7 @@ >> #include <linux/of_mdio.h> >> #include <linux/of_net.h> >> #include <linux/of_platform.h> >> -#include <linux/phy.h> >> +#include <linux/phylink.h> >> #include <linux/regmap.h> >> #include <linux/regulator/consumer.h> >> #include <linux/reset.h> >> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, >> int port, int sset) >> return ARRAY_SIZE(mt7530_mib); >> } >> >> -static void mt7530_adjust_link(struct dsa_switch *ds, int port, >> - struct phy_device *phydev) >> -{ >> - struct mt7530_priv *priv = ds->priv; >> - >> - if (phy_is_pseudo_fixed_link(phydev)) { >> - dev_dbg(priv->dev, "phy-mode for master device = %x\n", >> - phydev->interface); >> - >> - /* Setup TX circuit incluing relevant PAD and driving */ >> - mt7530_pad_clk_setup(ds, phydev->interface); >> - >> - if (priv->id == ID_MT7530) { >> - /* Setup RX circuit, relevant PAD and driving on the >> - * host which must be placed after the setup on the >> - * device side is all finished. >> - */ >> - mt7623_pad_clk_setup(ds); >> - } >> - } else { >> - u16 lcl_adv = 0, rmt_adv = 0; >> - u8 flowctrl; >> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE; >> - >> - switch (phydev->speed) { >> - case SPEED_1000: >> - mcr |= PMCR_FORCE_SPEED_1000; >> - break; >> - case SPEED_100: >> - mcr |= PMCR_FORCE_SPEED_100; >> - break; >> - } >> - >> - if (phydev->link) >> - mcr |= PMCR_FORCE_LNK; >> - >> - if (phydev->duplex) { >> - mcr |= PMCR_FORCE_FDX; >> - >> - if (phydev->pause) >> - rmt_adv = LPA_PAUSE_CAP; >> - if (phydev->asym_pause) >> - rmt_adv |= LPA_PAUSE_ASYM; >> - >> - lcl_adv = linkmode_adv_to_lcl_adv_t( >> - phydev->advertising); >> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv); >> - >> - if (flowctrl & FLOW_CTRL_TX) >> - mcr |= PMCR_TX_FC_EN; >> - if (flowctrl & FLOW_CTRL_RX) >> - mcr |= PMCR_RX_FC_EN; >> - } >> - mt7530_write(priv, MT7530_PMCR_P(port), mcr); >> - } >> -} >> - >> static int >> mt7530_cpu_port_enable(struct mt7530_priv *priv, >> int port) >> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds) >> return 0; >> } >> >> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + const struct phylink_link_state *state) >> +{ >> + struct mt7530_priv *priv = ds->priv; >> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | >> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN; >> + >> + switch (port) { >> + case 0: /* Internal phy */ >> + case 1: >> + case 2: >> + case 3: >> + case 4: >> + if (state->interface != PHY_INTERFACE_MODE_GMII) >> + goto unsupported; >> + break; >> + /* case 5: Port 5 is not supported! */ >> + case 6: /* 1st cpu port */ >> + if (state->interface != PHY_INTERFACE_MODE_RGMII && >> + state->interface != PHY_INTERFACE_MODE_TRGMII) >> + goto unsupported; >> + >> + /* Setup TX circuit incluing relevant PAD and driving */ >> + mt7530_pad_clk_setup(ds, state->interface); >> + >> + if (priv->id == ID_MT7530) { >> + /* Setup RX circuit, relevant PAD and driving on the >> + * host which must be placed after the setup on the >> + * device side is all finished. >> + */ >> + mt7623_pad_clk_setup(ds); >> + } >> + break; >> + default: >> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); >> + return; >> + } >> + >> + if (!state->an_enabled || mode == MLO_AN_FIXED) { >> + mcr |= PMCR_FORCE_MODE; >> + >> + if (state->speed == SPEED_1000) >> + mcr |= PMCR_FORCE_SPEED_1000; >> + if (state->speed == SPEED_100) >> + mcr |= PMCR_FORCE_SPEED_100; > > I would suggest using the defines > > #define PMCR_FORCE_SPEED 0x0000000c /* or PMCR_FORCE_SPEED_MASK */ > #define PMCR_FORCE_SPEED_10 0x00000000 > #define PMCR_FORCE_SPEED_100 0x00000004 > #define PMCR_FORCE_SPEED_1000 0x00000008 > > and a switch statement such as > > switch (state->speed) { > case SPEED_10: > mcr |= PMCR_FORCE_SPEED_10; > break; > case SPEED_100: > mcr |= PMCR_FORCE_SPEED_100; > break; > case SPEED_1000: > mcr |= PMCR_FORCE_SPEED_1000; > break; > } > > This will compile the same (i.e, the mcr |= 0 will optimize away, etc.), > while alleviating the need to intimately know the hardware in order to > easily understand what the code is doing at a glance. It's also better > form as we're treating the two bits as a composite value, rather than > two separate bits. I will change it based on your surgestion. > > >> + if (state->duplex == DUPLEX_FULL) >> + mcr |= PMCR_FORCE_FDX; >> + if (state->link || mode == MLO_AN_FIXED) >> + mcr |= PMCR_FORCE_LNK; >> + if (state->pause || phylink_test(state->advertising, Pause)) >> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN; >> + if (state->pause & MLO_PAUSE_TX) >> + mcr |= PMCR_TX_FC_EN; >> + if (state->pause & MLO_PAUSE_RX) >> + mcr |= PMCR_RX_FC_EN; >> + } >> + >> + mt7530_write(priv, MT7530_PMCR_P(port), mcr); >> + >> + return; >> + >> +unsupported: >> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n", >> + __func__, port, state->interface, phy_modes(state->interface)); >> +} >> + >> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + phy_interface_t interface) >> +{ >> + /* Do nothing */ >> +} >> + >> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + phy_interface_t interface, >> + struct phy_device *phydev) >> +{ >> + /* Do nothing */ >> +} >> + >> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port, >> + unsigned long *supported, >> + struct phylink_link_state *state) >> +{ >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; >> + >> + switch (port) { >> + case 0: /* Internal phy */ >> + case 1: >> + case 2: >> + case 3: >> + case 4: >> + if (state->interface != PHY_INTERFACE_MODE_NA && >> + state->interface != PHY_INTERFACE_MODE_GMII) >> + goto unsupported; >> + break; >> + /* case 5: Port 5 not supported! */ >> + case 6: /* 1st cpu port */ >> + if (state->interface != PHY_INTERFACE_MODE_RGMII && >> + state->interface != PHY_INTERFACE_MODE_TRGMII) >> + goto unsupported; >> + break; >> + default: >> + linkmode_zero(supported); >> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); >> + return; >> + } >> + >> + phylink_set(mask, Autoneg); >> + phylink_set(mask, Pause); >> + phylink_set(mask, Asym_Pause); >> + phylink_set(mask, MII); >> + >> + phylink_set(mask, 10baseT_Half); >> + phylink_set(mask, 10baseT_Full); >> + phylink_set(mask, 100baseT_Half); >> + phylink_set(mask, 100baseT_Full); >> + phylink_set(mask, 1000baseT_Full); >> + phylink_set(mask, 1000baseT_Half); >> + >> + linkmode_and(supported, supported, mask); >> + linkmode_and(state->advertising, state->advertising, mask); >> + return; >> + >> +unsupported: >> + linkmode_zero(supported); >> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n", >> + __func__, state->interface, phy_modes(state->interface)); >> +} >> + >> +static int >> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port, >> + struct phylink_link_state *state) >> +{ >> + struct mt7530_priv *priv = ds->priv; >> + u32 pmsr; >> + >> + if (port < 0 || port >= MT7530_NUM_PORTS) >> + return -EINVAL; >> + >> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port)); >> + >> + state->link = (pmsr & PMSR_LINK); >> + state->an_complete = state->link; >> + state->duplex = (pmsr & PMSR_DPX) >> 1; >> + >> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) { >> + case 0: >> + state->speed = SPEED_10; >> + break; >> + case PMSR_SPEED_100: >> + state->speed = SPEED_100; >> + break; >> + case PMSR_SPEED_1000: >> + state->speed = SPEED_1000; >> + break; >> + default: >> + state->speed = SPEED_UNKNOWN; >> + break; >> + } >> + > > Same as above: add PMSR_SPEED_10, and and with PMSR_SPEED (or > PMSR_SPEED_MASK if you prefer). Same as above. > >> + state->pause = 0; >> + if (pmsr & PMSR_RX_FC) >> + state->pause |= MLO_PAUSE_RX; >> + if (pmsr & PMSR_TX_FC) >> + state->pause |= MLO_PAUSE_TX; >> + >> + return 1; >> +} >> + >> static const struct dsa_switch_ops mt7530_switch_ops = { >> .get_tag_protocol = mtk_get_tag_protocol, >> .setup = mt7530_setup, >> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops >> mt7530_switch_ops = { >> .phy_write = mt7530_phy_write, >> .get_ethtool_stats = mt7530_get_ethtool_stats, >> .get_sset_count = mt7530_get_sset_count, >> - .adjust_link = mt7530_adjust_link, >> .port_enable = mt7530_port_enable, >> .port_disable = mt7530_port_disable, >> .port_stp_state_set = mt7530_stp_state_set, >> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops >> mt7530_switch_ops = { >> .port_vlan_prepare = mt7530_port_vlan_prepare, >> .port_vlan_add = mt7530_port_vlan_add, >> .port_vlan_del = mt7530_port_vlan_del, >> + .phylink_validate = mt7530_phylink_validate, >> + .phylink_mac_link_state = mt7530_phylink_mac_link_state, >> + .phylink_mac_config = mt7530_phylink_mac_config, >> + .phylink_mac_link_down = mt7530_phylink_mac_link_down, >> + .phylink_mac_link_up = mt7530_phylink_mac_link_up, >> }; >> >> static const struct of_device_id mt7530_of_match[] = { >> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h >> index bfac90f48102..41d9a132ac70 100644 >> --- a/drivers/net/dsa/mt7530.h >> +++ b/drivers/net/dsa/mt7530.h >> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr { >> #define PMCR_FORCE_SPEED_100 BIT(2) >> #define PMCR_FORCE_FDX BIT(1) >> #define PMCR_FORCE_LNK BIT(0) >> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE >> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ >> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ >> PMCR_TX_EN | PMCR_RX_EN | \ >> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr { >> PMCR_TX_FC_EN | PMCR_RX_FC_EN) >> >> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100) >> +#define PMSR_EEE1G BIT(7) >> +#define PMSR_EEE100M BIT(6) >> +#define PMSR_RX_FC BIT(5) >> +#define PMSR_TX_FC BIT(4) >> +#define PMSR_SPEED_1000 BIT(3) >> +#define PMSR_SPEED_100 BIT(2) >> +#define PMSR_DPX BIT(1) >> +#define PMSR_LINK BIT(0) >> >> /* Register for MIB */ >> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100) > > Cheers, > Daniel Greats, René
On Tue, Jun 25, 2019 at 11:31:58AM +0000, René van Dorst wrote: > > > + if (state->link || mode == MLO_AN_FIXED) > > > + mcr |= PMCR_FORCE_LNK; > > > > This should be removed - state->link is not for use in mac_config. > > Even in fixed mode, the link can be brought up/down by means of a > > gpio, and this should be dealt with via the mac_link_* functions. > > Maybe I understand it wrong, but is it the intention that in > phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always > forces into a certain speed/mode/interface. So it never auto-negotiate because > phylink select the best configuration for you? You are not the only one who has recently tried to make use of state->link in mac_config(), so I'm now preparing a set of patches to split the current mac_config() method into two separate methods: void (*mac_config_fixed)(struct net_device *ndev, phy_interface_t iface, int speed, int duplex, int pause); void (*mac_config_inband)(struct net_device *ndev, phy_interface_t iface, bool an_enabled, unsigned long *advertising, int pause); so that it's not possible to use members that should not be accessed in various modes. > Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here > and do nothing phylink_link_up()? When the link comes up/down, mac_link_up() and mac_link_down() will be called appropriately. In PHY mode, this is equivalent to phylink doing this: if (link_changed) { if (phydev->link) mac_link_up(); else mac_link_down(); } So the actions you would've done depending on phydev->link should be in the mac_link_*() methods. > Other question: > Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}? > Or only mac_config() and port_disable? mac_link_*(). > > > + if (state->pause || phylink_test(state->advertising, Pause)) > > > + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN; > > > + if (state->pause & MLO_PAUSE_TX) > > > + mcr |= PMCR_TX_FC_EN; > > > + if (state->pause & MLO_PAUSE_RX) > > > + mcr |= PMCR_RX_FC_EN; > > > > This is clearly wrong - if any bit in state->pause is set, then we > > end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause > > Pause set in the advertising mask, then both are set. This doesn't > > seem right - are these bits setting the advertisement, or are they > > telling the MAC to use flow control? > > Last one, tell the MAC to use flow control. So the first if() statement is incorrect, and should be removed entirely. You only want to enable the MAC to use flow control as a result of the negotiation results. > On the current driver both bits are set in a forced-link situation. > > If we always forces the MAC mode I think I always set these bits and don't > anything with the Pause modes? Is that the right way to do it? So what happens if your link partner (e.g. switch) does not support flow control? What if your link partner floods such frames to all ports? You end up transmitting flow control frames, which could be sent to all stations on the network... seems not a good idea. Implementing stuff properly and not taking short-cuts is always a good idea for inter-operability. > > > + > > > +static void mt7530_phylink_validate(struct dsa_switch *ds, int port, > > > + unsigned long *supported, > > > + struct phylink_link_state *state) > > > +{ > > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > > > + > > > + switch (port) { > > > + case 0: /* Internal phy */ > > > + case 1: > > > + case 2: > > > + case 3: > > > + case 4: > > > + if (state->interface != PHY_INTERFACE_MODE_NA && > > > + state->interface != PHY_INTERFACE_MODE_GMII) > > > + goto unsupported; > > > + break; > > > + /* case 5: Port 5 not supported! */ > > > + case 6: /* 1st cpu port */ > > > + if (state->interface != PHY_INTERFACE_MODE_RGMII && > > > + state->interface != PHY_INTERFACE_MODE_TRGMII) > > > > PHY_INTERFACE_MODE_NA ? > > You mean PHY_INTERFACE_MODE_NA is missing? Yes. Please see the updated documentation in the patch I sent this morning "net: phylink: further documentation clarifications".
Hello, Although I'm new to the entire Ethernet / *MII subsystem and I haven't touched DSA yet, I've recently had to add some of this functionality to the older OpenWRT drivers for swconfig control over the ports. René, do you have an actual datasheet or programming guide for the mt7530? I only have one for the mt7620. On 6/25/19 7:10 AM, Russell King - ARM Linux admin wrote: > mac_link_*(). > >>>> + if (state->pause || phylink_test(state->advertising, Pause)) >>>> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN; >>>> + if (state->pause & MLO_PAUSE_TX) >>>> + mcr |= PMCR_TX_FC_EN; >>>> + if (state->pause & MLO_PAUSE_RX) >>>> + mcr |= PMCR_RX_FC_EN; >>> This is clearly wrong - if any bit in state->pause is set, then we >>> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause >>> Pause set in the advertising mask, then both are set. This doesn't >>> seem right - are these bits setting the advertisement, or are they >>> telling the MAC to use flow control? >> Last one, tell the MAC to use flow control. > So the first if() statement is incorrect, and should be removed > entirely. You only want to enable the MAC to use flow control as a > result of the negotiation results. René, iiuc, this is what's documented in table 28B-3 of the 802.3 spec on page 598. pdf of section 2 here: http://www.ismlab.usf.edu/dcom/Ch3_802.3-2005_section2.pdf >> On the current driver both bits are set in a forced-link situation. >> >> If we always forces the MAC mode I think I always set these bits and don't >> anything with the Pause modes? Is that the right way to do it? > So what happens if your link partner (e.g. switch) does not support > flow control? What if your link partner floods such frames to all > ports? You end up transmitting flow control frames, which could be > sent to all stations on the network... seems not a good idea. > > Implementing stuff properly and not taking short-cuts is always a > good idea for inter-operability. But will there still be a mechanism to ignore link partner's advertising and force these parameters? I've run into what appears to be quirks on two separate NICs or their drivers, a tp-link tg-3468 (r8169) and an Aquantia AQC107 802.3bz (atlantic) where these link partners aren't auto-negotiating correctly after I switch the mt7530 out of auto-negotiation mode. Of course, it could be a mistake I've made (and should thus be discussed elsewhere), but iirc, I had to force enable flow control and then also disable auto-negotiation on the link partner and force the mode I wanted. Cheers, Daniel
> But will there still be a mechanism to ignore link partner's advertising > and force these parameters? From man 1 ethtool: -a --show-pause Queries the specified Ethernet device for pause parameter information. -A --pause Changes the pause parameters of the specified Ethernet device. autoneg on|off Specifies whether pause autonegotiation should be enabled. rx on|off Specifies whether RX pause should be enabled. tx on|off Specifies whether TX pause should be enabled. You need to check the driver to see if it actually implements this ethtool call, but that is how it should be configured. Andrew
On 6/25/19 2:02 PM, Andrew Lunn wrote: >> But will there still be a mechanism to ignore link partner's advertising >> and force these parameters? > >From man 1 ethtool: > > -a --show-pause > Queries the specified Ethernet device for pause parameter information. > > -A --pause > Changes the pause parameters of the specified Ethernet device. > > autoneg on|off > Specifies whether pause autonegotiation should be enabled. > > rx on|off > Specifies whether RX pause should be enabled. > > tx on|off > Specifies whether TX pause should be enabled. > > You need to check the driver to see if it actually implements this > ethtool call, but that is how it should be configured. > > Andrew > Thank you Andrew, So in this context, my question is the difference between "enabling" and "forcing". Here's that register for the mt7620 (which has an mt7530 on its die): https://imgur.com/a/pTk0668 I believe this is also what René is seeking clarity on? Thanks, Daniel
Hi Russell, On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > Hi, > > On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote: >> Convert mt7530 to PHYLINK API >> >> Signed-off-by: René van Dorst <opensource@vdorst.com> >> --- >> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++---------- >> drivers/net/dsa/mt7530.h | 9 ++ >> 2 files changed, 187 insertions(+), 59 deletions(-) >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 3181e95586d6..9c5e4dd00826 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -13,7 +13,7 @@ >> #include <linux/of_mdio.h> >> #include <linux/of_net.h> >> #include <linux/of_platform.h> >> -#include <linux/phy.h> >> +#include <linux/phylink.h> >> #include <linux/regmap.h> >> #include <linux/regulator/consumer.h> >> #include <linux/reset.h> >> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset) >> return ARRAY_SIZE(mt7530_mib); >> } >> >> -static void mt7530_adjust_link(struct dsa_switch *ds, int port, >> - struct phy_device *phydev) >> -{ >> - struct mt7530_priv *priv = ds->priv; >> - >> - if (phy_is_pseudo_fixed_link(phydev)) { >> - dev_dbg(priv->dev, "phy-mode for master device = %x\n", >> - phydev->interface); >> - >> - /* Setup TX circuit incluing relevant PAD and driving */ >> - mt7530_pad_clk_setup(ds, phydev->interface); >> - >> - if (priv->id == ID_MT7530) { >> - /* Setup RX circuit, relevant PAD and driving on the >> - * host which must be placed after the setup on the >> - * device side is all finished. >> - */ >> - mt7623_pad_clk_setup(ds); >> - } >> - } else { >> - u16 lcl_adv = 0, rmt_adv = 0; >> - u8 flowctrl; >> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE; >> - >> - switch (phydev->speed) { >> - case SPEED_1000: >> - mcr |= PMCR_FORCE_SPEED_1000; >> - break; >> - case SPEED_100: >> - mcr |= PMCR_FORCE_SPEED_100; >> - break; >> - } >> - >> - if (phydev->link) >> - mcr |= PMCR_FORCE_LNK; >> - >> - if (phydev->duplex) { >> - mcr |= PMCR_FORCE_FDX; >> - >> - if (phydev->pause) >> - rmt_adv = LPA_PAUSE_CAP; >> - if (phydev->asym_pause) >> - rmt_adv |= LPA_PAUSE_ASYM; >> - >> - lcl_adv = linkmode_adv_to_lcl_adv_t( >> - phydev->advertising); >> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv); >> - >> - if (flowctrl & FLOW_CTRL_TX) >> - mcr |= PMCR_TX_FC_EN; >> - if (flowctrl & FLOW_CTRL_RX) >> - mcr |= PMCR_RX_FC_EN; >> - } >> - mt7530_write(priv, MT7530_PMCR_P(port), mcr); >> - } >> -} >> - >> static int >> mt7530_cpu_port_enable(struct mt7530_priv *priv, >> int port) >> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds) >> return 0; >> } >> >> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + const struct phylink_link_state *state) >> +{ >> + struct mt7530_priv *priv = ds->priv; >> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | >> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN; >> + >> + switch (port) { >> + case 0: /* Internal phy */ >> + case 1: >> + case 2: >> + case 3: >> + case 4: >> + if (state->interface != PHY_INTERFACE_MODE_GMII) >> + goto unsupported; >> + break; >> + /* case 5: Port 5 is not supported! */ >> + case 6: /* 1st cpu port */ >> + if (state->interface != PHY_INTERFACE_MODE_RGMII && >> + state->interface != PHY_INTERFACE_MODE_TRGMII) >> + goto unsupported; >> + >> + /* Setup TX circuit incluing relevant PAD and driving */ >> + mt7530_pad_clk_setup(ds, state->interface); >> + >> + if (priv->id == ID_MT7530) { >> + /* Setup RX circuit, relevant PAD and driving on the >> + * host which must be placed after the setup on the >> + * device side is all finished. >> + */ >> + mt7623_pad_clk_setup(ds); >> + } >> + break; >> + default: >> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); >> + return; >> + } >> + >> + if (!state->an_enabled || mode == MLO_AN_FIXED) { >> + mcr |= PMCR_FORCE_MODE; >> + >> + if (state->speed == SPEED_1000) >> + mcr |= PMCR_FORCE_SPEED_1000; >> + if (state->speed == SPEED_100) >> + mcr |= PMCR_FORCE_SPEED_100; >> + if (state->duplex == DUPLEX_FULL) >> + mcr |= PMCR_FORCE_FDX; >> + if (state->link || mode == MLO_AN_FIXED) >> + mcr |= PMCR_FORCE_LNK; > > This should be removed - state->link is not for use in mac_config. > Even in fixed mode, the link can be brought up/down by means of a > gpio, and this should be dealt with via the mac_link_* functions. > What do you mean exactly that state->link is not for use, is that true in general? In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for ports that are BR_STATE_DISABLED. Is that normal? >> + if (state->pause || phylink_test(state->advertising, Pause)) >> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN; >> + if (state->pause & MLO_PAUSE_TX) >> + mcr |= PMCR_TX_FC_EN; >> + if (state->pause & MLO_PAUSE_RX) >> + mcr |= PMCR_RX_FC_EN; > > This is clearly wrong - if any bit in state->pause is set, then we > end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause > Pause set in the advertising mask, then both are set. This doesn't > seem right - are these bits setting the advertisement, or are they > telling the MAC to use flow control? > >> + } >> + >> + mt7530_write(priv, MT7530_PMCR_P(port), mcr); >> + >> + return; >> + >> +unsupported: >> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n", >> + __func__, port, state->interface, phy_modes(state->interface)); >> +} >> + >> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + phy_interface_t interface) >> +{ >> + /* Do nothing */ >> +} >> + >> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port, >> + unsigned int mode, >> + phy_interface_t interface, >> + struct phy_device *phydev) >> +{ >> + /* Do nothing */ >> +} > > These two are where you should be forcing the link up or down if > required (basically, inband modes should let the link come up/down > irrespective of these functions, otherwise it should be forced.) > >> + >> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port, >> + unsigned long *supported, >> + struct phylink_link_state *state) >> +{ >> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; >> + >> + switch (port) { >> + case 0: /* Internal phy */ >> + case 1: >> + case 2: >> + case 3: >> + case 4: >> + if (state->interface != PHY_INTERFACE_MODE_NA && >> + state->interface != PHY_INTERFACE_MODE_GMII) >> + goto unsupported; >> + break; >> + /* case 5: Port 5 not supported! */ >> + case 6: /* 1st cpu port */ >> + if (state->interface != PHY_INTERFACE_MODE_RGMII && >> + state->interface != PHY_INTERFACE_MODE_TRGMII) > > PHY_INTERFACE_MODE_NA ? > >> + goto unsupported; >> + break; >> + default: >> + linkmode_zero(supported); >> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); >> + return; >> + } >> + >> + phylink_set(mask, Autoneg); >> + phylink_set(mask, Pause); >> + phylink_set(mask, Asym_Pause); >> + phylink_set(mask, MII); >> + >> + phylink_set(mask, 10baseT_Half); >> + phylink_set(mask, 10baseT_Full); >> + phylink_set(mask, 100baseT_Half); >> + phylink_set(mask, 100baseT_Full); >> + phylink_set(mask, 1000baseT_Full); >> + phylink_set(mask, 1000baseT_Half); > > You seem to be missing phylink_set_port_modes() here. > >> + >> + linkmode_and(supported, supported, mask); >> + linkmode_and(state->advertising, state->advertising, mask); >> + return; >> + >> +unsupported: >> + linkmode_zero(supported); >> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n", >> + __func__, state->interface, phy_modes(state->interface)); > > Not a good idea to print this at error level; sometimes we just probe > for support. > > Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY > interface mode that the MAC can't support - we detect that by the > validation failing, and printing a more meaningful message in phylink > itself. > >> +} >> + >> +static int >> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port, >> + struct phylink_link_state *state) >> +{ >> + struct mt7530_priv *priv = ds->priv; >> + u32 pmsr; >> + >> + if (port < 0 || port >= MT7530_NUM_PORTS) >> + return -EINVAL; >> + >> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port)); >> + >> + state->link = (pmsr & PMSR_LINK); >> + state->an_complete = state->link; >> + state->duplex = (pmsr & PMSR_DPX) >> 1; >> + >> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) { >> + case 0: >> + state->speed = SPEED_10; >> + break; >> + case PMSR_SPEED_100: >> + state->speed = SPEED_100; >> + break; >> + case PMSR_SPEED_1000: >> + state->speed = SPEED_1000; >> + break; >> + default: >> + state->speed = SPEED_UNKNOWN; >> + break; >> + } >> + >> + state->pause = 0; >> + if (pmsr & PMSR_RX_FC) >> + state->pause |= MLO_PAUSE_RX; >> + if (pmsr & PMSR_TX_FC) >> + state->pause |= MLO_PAUSE_TX; >> + >> + return 1; >> +} >> + >> static const struct dsa_switch_ops mt7530_switch_ops = { >> .get_tag_protocol = mtk_get_tag_protocol, >> .setup = mt7530_setup, >> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = { >> .phy_write = mt7530_phy_write, >> .get_ethtool_stats = mt7530_get_ethtool_stats, >> .get_sset_count = mt7530_get_sset_count, >> - .adjust_link = mt7530_adjust_link, >> .port_enable = mt7530_port_enable, >> .port_disable = mt7530_port_disable, >> .port_stp_state_set = mt7530_stp_state_set, >> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = { >> .port_vlan_prepare = mt7530_port_vlan_prepare, >> .port_vlan_add = mt7530_port_vlan_add, >> .port_vlan_del = mt7530_port_vlan_del, >> + .phylink_validate = mt7530_phylink_validate, >> + .phylink_mac_link_state = mt7530_phylink_mac_link_state, >> + .phylink_mac_config = mt7530_phylink_mac_config, >> + .phylink_mac_link_down = mt7530_phylink_mac_link_down, >> + .phylink_mac_link_up = mt7530_phylink_mac_link_up, >> }; >> >> static const struct of_device_id mt7530_of_match[] = { >> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h >> index bfac90f48102..41d9a132ac70 100644 >> --- a/drivers/net/dsa/mt7530.h >> +++ b/drivers/net/dsa/mt7530.h >> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr { >> #define PMCR_FORCE_SPEED_100 BIT(2) >> #define PMCR_FORCE_FDX BIT(1) >> #define PMCR_FORCE_LNK BIT(0) >> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE >> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ >> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ >> PMCR_TX_EN | PMCR_RX_EN | \ >> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr { >> PMCR_TX_FC_EN | PMCR_RX_FC_EN) >> >> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100) >> +#define PMSR_EEE1G BIT(7) >> +#define PMSR_EEE100M BIT(6) >> +#define PMSR_RX_FC BIT(5) >> +#define PMSR_TX_FC BIT(4) >> +#define PMSR_SPEED_1000 BIT(3) >> +#define PMSR_SPEED_100 BIT(2) >> +#define PMSR_DPX BIT(1) >> +#define PMSR_LINK BIT(0) >> >> /* Register for MIB */ >> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100) >> -- >> 2.20.1 >> >> > Regards, -Vladimir
On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote: > On 6/25/19 2:02 PM, Andrew Lunn wrote: > >> But will there still be a mechanism to ignore link partner's advertising > >> and force these parameters? > > >From man 1 ethtool: > > > > -a --show-pause > > Queries the specified Ethernet device for pause parameter information. > > > > -A --pause > > Changes the pause parameters of the specified Ethernet device. > > > > autoneg on|off > > Specifies whether pause autonegotiation should be enabled. > > > > rx on|off > > Specifies whether RX pause should be enabled. > > > > tx on|off > > Specifies whether TX pause should be enabled. > > > > You need to check the driver to see if it actually implements this > > ethtool call, but that is how it should be configured. > > > > Andrew > > > Thank you Andrew, > > So in this context, my question is the difference between "enabling" and > "forcing". Here's that register for the mt7620 (which has an mt7530 on > its die): https://imgur.com/a/pTk0668 I believe this is also what René > is seeking clarity on? Lets start with normal operation. If the MAC supports pause or asym pause, it calls phy_support_sym_pause() or phy_support_asym_pause(). phylib will then configure the PHY to advertise pause as appropriate. Once auto-neg has completed, the results of the negotiation are set in phydev. So phdev->pause and phydev->asym_pause. The MAC callback is then used to tell the MAC about the autoneg results. The MAC should be programmed using the values in phdev->pause and phydev->asym_pause. For ethtool, the MAC driver needs to implement .get_pauseparam and .set_pauseparam. The set_pauseparam needs to validate the settings, using phy_validate_pause(). If valid, phy_set_asym_pause() is used to tell the PHY about the new configuration. This will trigger a new auto-neg if auto-neg is enabled, and the results will be passed back in the usual way. If auto-neg is disabled, or pause auto-neg is disabled, the MAC should configure pause directly based on the settings passed. Looking at the data sheet page, you want FORCE_MODE_Pn set. You never want the MAC directly talking to the PHY. Bad things will happen. Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and phydev->asym_pause. The same idea applies when using phylink. Andrew
Quoting Andrew Lunn <andrew@lunn.ch>: Hi Andrew, > On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote: >> On 6/25/19 2:02 PM, Andrew Lunn wrote: >> >> But will there still be a mechanism to ignore link partner's advertising >> >> and force these parameters? >> > >From man 1 ethtool: >> > >> > -a --show-pause >> > Queries the specified Ethernet device for pause >> parameter information. >> > >> > -A --pause >> > Changes the pause parameters of the specified >> Ethernet device. >> > >> > autoneg on|off >> > Specifies whether pause autonegotiation should >> be enabled. >> > >> > rx on|off >> > Specifies whether RX pause should be enabled. >> > >> > tx on|off >> > Specifies whether TX pause should be enabled. >> > >> > You need to check the driver to see if it actually implements this >> > ethtool call, but that is how it should be configured. >> > >> > Andrew >> > >> Thank you Andrew, >> >> So in this context, my question is the difference between "enabling" and >> "forcing". Here's that register for the mt7620 (which has an mt7530 on >> its die): https://imgur.com/a/pTk0668 I believe this is also what René >> is seeking clarity on? > > Lets start with normal operation. If the MAC supports pause or asym > pause, it calls phy_support_sym_pause() or phy_support_asym_pause(). > phylib will then configure the PHY to advertise pause as appropriate. > Once auto-neg has completed, the results of the negotiation are set in > phydev. So phdev->pause and phydev->asym_pause. The MAC callback is > then used to tell the MAC about the autoneg results. The MAC should be > programmed using the values in phdev->pause and phydev->asym_pause. > > For ethtool, the MAC driver needs to implement .get_pauseparam and > .set_pauseparam. The set_pauseparam needs to validate the settings, > using phy_validate_pause(). If valid, phy_set_asym_pause() is used to > tell the PHY about the new configuration. This will trigger a new > auto-neg if auto-neg is enabled, and the results will be passed back > in the usual way. If auto-neg is disabled, or pause auto-neg is > disabled, the MAC should configure pause directly based on the > settings passed. > > Looking at the data sheet page, you want FORCE_MODE_Pn set. You never > want the MAC directly talking to the PHY. Bad things will happen. > Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and > phydev->asym_pause. > > The same idea applies when using phylink. Thanks for this information. I updated mt7530_phylink_mac_config(), I think I done it right this time with the pause bits. Now mt7530_phylink_mac_config() looks like this: static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, const struct phylink_link_state *state) { struct mt7530_priv *priv = ds->priv; u32 mcr_cur, mcr_new = 0; switch (port) { case 0: /* Internal phy */ case 1: case 2: case 3: case 4: if (state->interface != PHY_INTERFACE_MODE_GMII) return; break; case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */ if (!phy_interface_mode_is_rgmii(state->interface) && state->interface != PHY_INTERFACE_MODE_MII) return; if (priv->p5_intf_sel != P5_INTF_SEL_GMAC5) { priv->p5_intf_sel = P5_INTF_SEL_GMAC5; mt7530_setup_port5(ds, state->interface); } /* We are connected to external phy */ if (dsa_is_user_port(ds, 5)) mcr_new |= PMCR_EXT_PHY; break; case 6: /* 1st cpu port */ if (state->interface != PHY_INTERFACE_MODE_RGMII && state->interface != PHY_INTERFACE_MODE_TRGMII) return; if (priv->p6_interface == state->interface) break; /* Setup TX circuit incluing relevant PAD and driving */ mt7530_pad_clk_setup(ds, state->interface); if (priv->id == ID_MT7530) { /* Setup RX circuit, relevant PAD and driving on the * host which must be placed after the setup on the * device side is all finished. */ mt7623_pad_clk_setup(ds); } priv->p6_interface = state->interface; break; default: dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); return; } mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port)); mcr_new |= mcr_cur; mcr_new &= ~(PMCR_FORCE_SPEED_1000 | PMCR_FORCE_SPEED_100 | PMCR_FORCE_FDX | PMCR_TX_FC_EN | PMCR_RX_FC_EN); mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | PMCR_BACKPR_EN | PMCR_FORCE_MODE | PMCR_FORCE_LNK; switch (state->speed) { case SPEED_1000: mcr_new |= PMCR_FORCE_SPEED_1000; break; case SPEED_100: mcr_new |= PMCR_FORCE_SPEED_100; break; } if (state->duplex == DUPLEX_FULL) { mcr_new |= PMCR_FORCE_FDX; if (state->pause & MLO_PAUSE_TX) mcr_new |= PMCR_TX_FC_EN; if (state->pause & MLO_PAUSE_RX) mcr_new |= PMCR_RX_FC_EN; } if (mcr_new != mcr_cur) mt7530_write(priv, MT7530_PMCR_P(port), mcr_new); } Greats, René
On Tue, Jun 25, 2019 at 09:02:46PM +0200, Andrew Lunn wrote: > > But will there still be a mechanism to ignore link partner's advertising > > and force these parameters? > > From man 1 ethtool: > > -a --show-pause > Queries the specified Ethernet device for pause parameter information. > > -A --pause > Changes the pause parameters of the specified Ethernet device. > > autoneg on|off > Specifies whether pause autonegotiation should be enabled. > > rx on|off > Specifies whether RX pause should be enabled. > > tx on|off > Specifies whether TX pause should be enabled. > > You need to check the driver to see if it actually implements this > ethtool call, but that is how it should be configured. Note that phylink provides this call, and provided mac_config() is correctly implemented, will result in the pause mode parameters in the MAC being correctly set.
On Tue, Jun 25, 2019 at 10:41:48PM +0200, Andrew Lunn wrote: > On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote: > > On 6/25/19 2:02 PM, Andrew Lunn wrote: > > >> But will there still be a mechanism to ignore link partner's advertising > > >> and force these parameters? > > > >From man 1 ethtool: > > > > > > -a --show-pause > > > Queries the specified Ethernet device for pause parameter information. > > > > > > -A --pause > > > Changes the pause parameters of the specified Ethernet device. > > > > > > autoneg on|off > > > Specifies whether pause autonegotiation should be enabled. > > > > > > rx on|off > > > Specifies whether RX pause should be enabled. > > > > > > tx on|off > > > Specifies whether TX pause should be enabled. > > > > > > You need to check the driver to see if it actually implements this > > > ethtool call, but that is how it should be configured. > > > > > > Andrew > > > > > Thank you Andrew, > > > > So in this context, my question is the difference between "enabling" and > > "forcing". Here's that register for the mt7620 (which has an mt7530 on > > its die): https://imgur.com/a/pTk0668 I believe this is also what René > > is seeking clarity on? > > Lets start with normal operation. If the MAC supports pause or asym > pause, it calls phy_support_sym_pause() or phy_support_asym_pause(). > phylib will then configure the PHY to advertise pause as appropriate. > Once auto-neg has completed, the results of the negotiation are set in > phydev. So phdev->pause and phydev->asym_pause. The MAC callback is > then used to tell the MAC about the autoneg results. The MAC should be > programmed using the values in phdev->pause and phydev->asym_pause. > > For ethtool, the MAC driver needs to implement .get_pauseparam and > .set_pauseparam. The set_pauseparam needs to validate the settings, > using phy_validate_pause(). If valid, phy_set_asym_pause() is used to > tell the PHY about the new configuration. This will trigger a new > auto-neg if auto-neg is enabled, and the results will be passed back > in the usual way. If auto-neg is disabled, or pause auto-neg is > disabled, the MAC should configure pause directly based on the > settings passed. > > Looking at the data sheet page, you want FORCE_MODE_Pn set. You never > want the MAC directly talking to the PHY. Bad things will happen. > Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and > phydev->asym_pause. > > The same idea applies when using phylink. Except when using phylink, use pause & MLO_PAUSE_RX to determine whether FORCE_RX_FC_Pn should be set, and use pause & MLO_PAUSE_TX to determine whether FORCE_TX_Pn should be set. phylink will take care of the results of negotiation with the link partner and tell you what should be set if pause autoneg is enabled. If the user has decided to force it via ethtool, and the MAC driver passes the calls on to phylink, phylink will instead set MLO_PAUSE_RX and MLO_PAUSE_TX according to the users settings. So, with phylink, it's quite literally "if MLO_PAUSE_RX is set in mac_config, enable receiption of pause frames. if MLO_PAUSE_TX is set, enable transmission of pause frames." The above applies for phylink's PHY, FIXED, and SGMII in-band modes. For 802.3z in-band modes, pause is negotiated between the two link parters (which could be the PCS built into the MACs at either end) and in some cases its possible to set the MAC to automatically adjust to the results of the built-in PCS negotiation.
On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > Hi Russell, > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > This should be removed - state->link is not for use in mac_config. > > Even in fixed mode, the link can be brought up/down by means of a > > gpio, and this should be dealt with via the mac_link_* functions. > > > > What do you mean exactly that state->link is not for use, is that true in > general? Yes. mac_config() should not touch it; it is not always in a defined state. For example, if you set modes via ethtool (the ethtool_ksettings_set API) then state->link will probably contain zero irrespective of the true link state. It exists in this structure because it was convenient to just use one structure to store all the link information in various parts of the code, and when requesting the negotiated in-band MAC configuration. I've come to the conclusion that that decision was a mistake, based on patches such as the above mistakenly thinking that everything in the state structure is fair game. I've since updated the docs to explicitly spell it out, but I'm also looking at the feasibility of changing the mac_config() interface entirely - splitting it into two (mac_config_fixed() and mac_config_inband()) and passing only the appropriate parameters to each. However, having looked at that, I think such a change will make some MAC drivers quite a bit more complicated - having all the config steps in one method appears to make the configuration of MAC drivers easier (eg, mvneta, mvpp2.) > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for > ports that are BR_STATE_DISABLED. Is that normal? This looks like another driver which has been converted to phylink without my review; I certainly wasn't aware of it. It gets a few things wrong, such as: 1) not checking state->interface in the validate callback - so it is basically saying that it can support any PHY interface mode that the kernel happens to support. 2) if phylink is configured to use in-band, then state->speed is undefined; this driver will fail. (If folk don't want to support that, we ought to have a way to tell phylink to reject anything that attempts to set it to in-band mode!) 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't support SGMII or 802.3z phy interface modes (see 1).
On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > > Hi Russell, > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > > This should be removed - state->link is not for use in mac_config. > > > Even in fixed mode, the link can be brought up/down by means of a > > > gpio, and this should be dealt with via the mac_link_* functions. > > > > > > > What do you mean exactly that state->link is not for use, is that true in > > general? > > Yes. mac_config() should not touch it; it is not always in a defined > state. For example, if you set modes via ethtool (the > ethtool_ksettings_set API) then state->link will probably contain > zero irrespective of the true link state. > Experimentally, state->link is zero at the same time as state->speed is -1, so just ignoring !state->link made sense. This is not in-band AN. What is your suggestion? Should I proceed to try and configure the MAC for SPEED_UNKNOWN? > It exists in this structure because it was convenient to just use one > structure to store all the link information in various parts of the > code, and when requesting the negotiated in-band MAC configuration. > > I've come to the conclusion that that decision was a mistake, based > on patches such as the above mistakenly thinking that everything in > the state structure is fair game. I've since updated the docs to > explicitly spell it out, but I'm also looking at the feasibility of > changing the mac_config() interface entirely - splitting it into two > (mac_config_fixed() and mac_config_inband()) and passing only the > appropriate parameters to each. > > However, having looked at that, I think such a change will make some > MAC drivers quite a bit more complicated - having all the config > steps in one method appears to make the configuration of MAC drivers > easier (eg, mvneta, mvpp2.) > > > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if > > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for > > ports that are BR_STATE_DISABLED. Is that normal? > > This looks like another driver which has been converted to phylink > without my review; I certainly wasn't aware of it. It gets a few > things wrong, such as: > > 1) not checking state->interface in the validate callback - so it > is basically saying that it can support any PHY interface mode > that the kernel happens to support. > Partially true. It does check the DT bindings for supported MII modes in sja1105_init_mii_settings (for fundamental reasons... the switch expects an 'all-in-one' configuration buffer with the operating modes of all MACs - don't ask me to delay the uploading of this static config until all ports collected their interface_mode from phylink via the mac_config callback - it's a deadlock). It is a gigabit switch with MII/RMII/RGMII MACs - I have never seen any PHY wired for these modes that can change system interface type. > 2) if phylink is configured to use in-band, then state->speed is > undefined; this driver will fail. (If folk don't want to support > that, we ought to have a way to tell phylink to reject anything > that attempts to set it to in-band mode!) > Ok. > 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't > support SGMII or 802.3z phy interface modes (see 1). > No, it doesn't. Some odd switch in this device family supports SGMII on 1 of its ports, however I haven't put my hands on it. When I do I'll add checks for strange scenarios, like connecting it to an Aquantia PHY that can switch between SGMII and USXGMII (although why would anyone pair a 10G capable PHY to a 1G capable MAC...) > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up Thanks, -Vladimir
On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote: > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > > > Hi Russell, > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > > > This should be removed - state->link is not for use in mac_config. > > > > Even in fixed mode, the link can be brought up/down by means of a > > > > gpio, and this should be dealt with via the mac_link_* functions. > > > > > > > > > > What do you mean exactly that state->link is not for use, is that true in > > > general? > > > > Yes. mac_config() should not touch it; it is not always in a defined > > state. For example, if you set modes via ethtool (the > > ethtool_ksettings_set API) then state->link will probably contain > > zero irrespective of the true link state. > > > > Experimentally, state->link is zero at the same time as state->speed > is -1, so just ignoring !state->link made sense. This is not in-band > AN. What is your suggestion? Should I proceed to try and configure the > MAC for SPEED_UNKNOWN? What would you have done with a PHY when the link is down, what speed would you have configured in the phylib adjust_link callback? phylib also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down. What we do in Marvell drivers is set to the lowest speed (10M) in such cases, which is fine as the MAC supports 10M. It wouldn't be appropriate for phylink to force something on MAC drivers, it's easier if the MAC just defaults SPEED_UNKNOWN to something itself. > > > It exists in this structure because it was convenient to just use one > > structure to store all the link information in various parts of the > > code, and when requesting the negotiated in-band MAC configuration. > > > > I've come to the conclusion that that decision was a mistake, based > > on patches such as the above mistakenly thinking that everything in > > the state structure is fair game. I've since updated the docs to > > explicitly spell it out, but I'm also looking at the feasibility of > > changing the mac_config() interface entirely - splitting it into two > > (mac_config_fixed() and mac_config_inband()) and passing only the > > appropriate parameters to each. > > > > However, having looked at that, I think such a change will make some > > MAC drivers quite a bit more complicated - having all the config > > steps in one method appears to make the configuration of MAC drivers > > easier (eg, mvneta, mvpp2.) > > > > > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if > > > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for > > > ports that are BR_STATE_DISABLED. Is that normal? > > > > This looks like another driver which has been converted to phylink > > without my review; I certainly wasn't aware of it. It gets a few > > things wrong, such as: > > > > 1) not checking state->interface in the validate callback - so it > > is basically saying that it can support any PHY interface mode > > that the kernel happens to support. > > > > Partially true. It does check the DT bindings for supported MII modes > in sja1105_init_mii_settings (for fundamental reasons... the switch > expects an 'all-in-one' configuration buffer with the operating modes > of all MACs - don't ask me to delay the uploading of this static > config until all ports collected their interface_mode from phylink via > the mac_config callback - it's a deadlock). Ok, so you need to reject interface modes that are not compatible with the currently configured mode in the validate() callback, but please keep PHY_INTERFACE_MODE_NA reporting back the capabilities. (this is now documented.) > It is a gigabit switch with MII/RMII/RGMII MACs - I have never seen > any PHY wired for these modes that can change system interface type. It is unlikely that MII/RMII/RGMII will switch modes, but in terms of correct implementation, sticking to the way the function is expected to behave means that I don't get surprises when changing phylink layer in the future. > > > 2) if phylink is configured to use in-band, then state->speed is > > undefined; this driver will fail. (If folk don't want to support > > that, we ought to have a way to tell phylink to reject anything > > that attempts to set it to in-band mode!) > > > > Ok. > > > 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't > > support SGMII or 802.3z phy interface modes (see 1). > > > > No, it doesn't. > Some odd switch in this device family supports SGMII on 1 of its > ports, however I haven't put my hands on it. > When I do I'll add checks for strange scenarios, like connecting it to > an Aquantia PHY that can switch between SGMII and USXGMII (although > why would anyone pair a 10G capable PHY to a 1G capable MAC...) It's unlikely that it would switch between SGMII and USXGMII dynamically, as USXGMII supports speeds from 10G down to 10M. Where interface mode switching tends to be used is with modes such as 10GBASE-R, which doesn't support anything except 10G. In order for the PHY to operate at slower speeds, it has a few options: 1) perform rate adaption. 2) dynamically switch interface type to an interface type that supports the desired speed. 3) just not support slower speeds.
On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote: > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin > > <linux@armlinux.org.uk> wrote: > > > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > > > > Hi Russell, > > > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > > > > This should be removed - state->link is not for use in mac_config. > > > > > Even in fixed mode, the link can be brought up/down by means of a > > > > > gpio, and this should be dealt with via the mac_link_* functions. > > > > > > > > > > > > > What do you mean exactly that state->link is not for use, is that true in > > > > general? > > > > > > Yes. mac_config() should not touch it; it is not always in a defined > > > state. For example, if you set modes via ethtool (the > > > ethtool_ksettings_set API) then state->link will probably contain > > > zero irrespective of the true link state. > > > > > > > Experimentally, state->link is zero at the same time as state->speed > > is -1, so just ignoring !state->link made sense. This is not in-band > > AN. What is your suggestion? Should I proceed to try and configure the > > MAC for SPEED_UNKNOWN? > > What would you have done with a PHY when the link is down, what speed > would you have configured in the phylib adjust_link callback? phylib > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down. > With phylib, I'd make the driver ignore the speed and do nothing. With phylink, I'd make the core not call mac_config. But what happened is I saw phylink call mac_config anyway, said 'weird' and proceeded to ignore it as I would have for phylib. I'm just not understanding your position - it seems like you're implying there's a bug in phylink and the function call with MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken place, which is what I wanted to confirm. > What we do in Marvell drivers is set to the lowest speed (10M) in such > cases, which is fine as the MAC supports 10M. > > It wouldn't be appropriate for phylink to force something on MAC > drivers, it's easier if the MAC just defaults SPEED_UNKNOWN to something > itself. > > > > > > It exists in this structure because it was convenient to just use one > > > structure to store all the link information in various parts of the > > > code, and when requesting the negotiated in-band MAC configuration. > > > > > > I've come to the conclusion that that decision was a mistake, based > > > on patches such as the above mistakenly thinking that everything in > > > the state structure is fair game. I've since updated the docs to > > > explicitly spell it out, but I'm also looking at the feasibility of > > > changing the mac_config() interface entirely - splitting it into two > > > (mac_config_fixed() and mac_config_inband()) and passing only the > > > appropriate parameters to each. > > > > > > However, having looked at that, I think such a change will make some > > > MAC drivers quite a bit more complicated - having all the config > > > steps in one method appears to make the configuration of MAC drivers > > > easier (eg, mvneta, mvpp2.) > > > > > > > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if > > > > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for > > > > ports that are BR_STATE_DISABLED. Is that normal? > > > > > > This looks like another driver which has been converted to phylink > > > without my review; I certainly wasn't aware of it. It gets a few > > > things wrong, such as: > > > > > > 1) not checking state->interface in the validate callback - so it > > > is basically saying that it can support any PHY interface mode > > > that the kernel happens to support. > > > > > > > Partially true. It does check the DT bindings for supported MII modes > > in sja1105_init_mii_settings (for fundamental reasons... the switch > > expects an 'all-in-one' configuration buffer with the operating modes > > of all MACs - don't ask me to delay the uploading of this static > > config until all ports collected their interface_mode from phylink via > > the mac_config callback - it's a deadlock). > > Ok, so you need to reject interface modes that are not compatible > with the currently configured mode in the validate() callback, but > please keep PHY_INTERFACE_MODE_NA reporting back the capabilities. > (this is now documented.) > Ok. > > It is a gigabit switch with MII/RMII/RGMII MACs - I have never seen > > any PHY wired for these modes that can change system interface type. > > It is unlikely that MII/RMII/RGMII will switch modes, but in terms of > correct implementation, sticking to the way the function is expected > to behave means that I don't get surprises when changing phylink layer > in the future. > > > > > > 2) if phylink is configured to use in-band, then state->speed is > > > undefined; this driver will fail. (If folk don't want to support > > > that, we ought to have a way to tell phylink to reject anything > > > that attempts to set it to in-band mode!) > > > > > > > Ok. > > > > > 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't > > > support SGMII or 802.3z phy interface modes (see 1). > > > > > > > No, it doesn't. > > Some odd switch in this device family supports SGMII on 1 of its > > ports, however I haven't put my hands on it. > > When I do I'll add checks for strange scenarios, like connecting it to > > an Aquantia PHY that can switch between SGMII and USXGMII (although > > why would anyone pair a 10G capable PHY to a 1G capable MAC...) > > It's unlikely that it would switch between SGMII and USXGMII > dynamically, as USXGMII supports speeds from 10G down to 10M. > > Where interface mode switching tends to be used is with modes such > as 10GBASE-R, which doesn't support anything except 10G. In order > for the PHY to operate at slower speeds, it has a few options: > > 1) perform rate adaption. > 2) dynamically switch interface type to an interface type that > supports the desired speed. > 3) just not support slower speeds. > So am I reading this correctly - it kind of makes sense for gigabit MAC drivers to not check for the MII interface changing protocol? > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up Thanks, -Vladimir
On Wed, 26 Jun 2019 at 02:10, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote: > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin > > > <linux@armlinux.org.uk> wrote: > > > > > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > > > > > Hi Russell, > > > > > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > > > > > This should be removed - state->link is not for use in mac_config. > > > > > > Even in fixed mode, the link can be brought up/down by means of a > > > > > > gpio, and this should be dealt with via the mac_link_* functions. > > > > > > > > > > > > > > > > What do you mean exactly that state->link is not for use, is that true in > > > > > general? > > > > > > > > Yes. mac_config() should not touch it; it is not always in a defined > > > > state. For example, if you set modes via ethtool (the > > > > ethtool_ksettings_set API) then state->link will probably contain > > > > zero irrespective of the true link state. > > > > > > > > > > Experimentally, state->link is zero at the same time as state->speed > > > is -1, so just ignoring !state->link made sense. This is not in-band > > > AN. What is your suggestion? Should I proceed to try and configure the > > > MAC for SPEED_UNKNOWN? > > > > What would you have done with a PHY when the link is down, what speed > > would you have configured in the phylib adjust_link callback? phylib > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down. > > > > With phylib, I'd make the driver ignore the speed and do nothing. > With phylink, I'd make the core not call mac_config. > But what happened is I saw phylink call mac_config anyway, said > 'weird' and proceeded to ignore it as I would have for phylib. > I'm just not understanding your position - it seems like you're > implying there's a bug in phylink and the function call with > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken I meant MLO_AN_PHY, sorry. > place, which is what I wanted to confirm. > > > What we do in Marvell drivers is set to the lowest speed (10M) in such > > cases, which is fine as the MAC supports 10M. > > > > It wouldn't be appropriate for phylink to force something on MAC > > drivers, it's easier if the MAC just defaults SPEED_UNKNOWN to something > > itself. > > > > > > > > > It exists in this structure because it was convenient to just use one > > > > structure to store all the link information in various parts of the > > > > code, and when requesting the negotiated in-band MAC configuration. > > > > > > > > I've come to the conclusion that that decision was a mistake, based > > > > on patches such as the above mistakenly thinking that everything in > > > > the state structure is fair game. I've since updated the docs to > > > > explicitly spell it out, but I'm also looking at the feasibility of > > > > changing the mac_config() interface entirely - splitting it into two > > > > (mac_config_fixed() and mac_config_inband()) and passing only the > > > > appropriate parameters to each. > > > > > > > > However, having looked at that, I think such a change will make some > > > > MAC drivers quite a bit more complicated - having all the config > > > > steps in one method appears to make the configuration of MAC drivers > > > > easier (eg, mvneta, mvpp2.) > > > > > > > > > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if > > > > > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for > > > > > ports that are BR_STATE_DISABLED. Is that normal? > > > > > > > > This looks like another driver which has been converted to phylink > > > > without my review; I certainly wasn't aware of it. It gets a few > > > > things wrong, such as: > > > > > > > > 1) not checking state->interface in the validate callback - so it > > > > is basically saying that it can support any PHY interface mode > > > > that the kernel happens to support. > > > > > > > > > > Partially true. It does check the DT bindings for supported MII modes > > > in sja1105_init_mii_settings (for fundamental reasons... the switch > > > expects an 'all-in-one' configuration buffer with the operating modes > > > of all MACs - don't ask me to delay the uploading of this static > > > config until all ports collected their interface_mode from phylink via > > > the mac_config callback - it's a deadlock). > > > > Ok, so you need to reject interface modes that are not compatible > > with the currently configured mode in the validate() callback, but > > please keep PHY_INTERFACE_MODE_NA reporting back the capabilities. > > (this is now documented.) > > > > Ok. > > > > It is a gigabit switch with MII/RMII/RGMII MACs - I have never seen > > > any PHY wired for these modes that can change system interface type. > > > > It is unlikely that MII/RMII/RGMII will switch modes, but in terms of > > correct implementation, sticking to the way the function is expected > > to behave means that I don't get surprises when changing phylink layer > > in the future. > > > > > > > > > 2) if phylink is configured to use in-band, then state->speed is > > > > undefined; this driver will fail. (If folk don't want to support > > > > that, we ought to have a way to tell phylink to reject anything > > > > that attempts to set it to in-band mode!) > > > > > > > > > > Ok. > > > > > > > 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't > > > > support SGMII or 802.3z phy interface modes (see 1). > > > > > > > > > > No, it doesn't. > > > Some odd switch in this device family supports SGMII on 1 of its > > > ports, however I haven't put my hands on it. > > > When I do I'll add checks for strange scenarios, like connecting it to > > > an Aquantia PHY that can switch between SGMII and USXGMII (although > > > why would anyone pair a 10G capable PHY to a 1G capable MAC...) > > > > It's unlikely that it would switch between SGMII and USXGMII > > dynamically, as USXGMII supports speeds from 10G down to 10M. > > > > Where interface mode switching tends to be used is with modes such > > as 10GBASE-R, which doesn't support anything except 10G. In order > > for the PHY to operate at slower speeds, it has a few options: > > > > 1) perform rate adaption. > > 2) dynamically switch interface type to an interface type that > > supports the desired speed. > > 3) just not support slower speeds. > > > > So am I reading this correctly - it kind of makes sense for gigabit > MAC drivers to not check for the MII interface changing protocol? > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > > According to speedtest.net: 11.9Mbps down 500kbps up > > Thanks, > -Vladimir
On Wed, Jun 26, 2019 at 02:13:25AM +0300, Vladimir Oltean wrote: > On Wed, 26 Jun 2019 at 02:10, Vladimir Oltean <olteanv@gmail.com> wrote: > > > > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin > > <linux@armlinux.org.uk> wrote: > > > > > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote: > > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin > > > > <linux@armlinux.org.uk> wrote: > > > > > > > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > > > > > > Hi Russell, > > > > > > > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > > > > > > This should be removed - state->link is not for use in mac_config. > > > > > > > Even in fixed mode, the link can be brought up/down by means of a > > > > > > > gpio, and this should be dealt with via the mac_link_* functions. > > > > > > > > > > > > > > > > > > > What do you mean exactly that state->link is not for use, is that true in > > > > > > general? > > > > > > > > > > Yes. mac_config() should not touch it; it is not always in a defined > > > > > state. For example, if you set modes via ethtool (the > > > > > ethtool_ksettings_set API) then state->link will probably contain > > > > > zero irrespective of the true link state. > > > > > > > > > > > > > Experimentally, state->link is zero at the same time as state->speed > > > > is -1, so just ignoring !state->link made sense. This is not in-band > > > > AN. What is your suggestion? Should I proceed to try and configure the > > > > MAC for SPEED_UNKNOWN? > > > > > > What would you have done with a PHY when the link is down, what speed > > > would you have configured in the phylib adjust_link callback? phylib > > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down. > > > > > > > With phylib, I'd make the driver ignore the speed and do nothing. > > With phylink, I'd make the core not call mac_config. > > But what happened is I saw phylink call mac_config anyway, said > > 'weird' and proceeded to ignore it as I would have for phylib. > > I'm just not understanding your position - it seems like you're > > implying there's a bug in phylink and the function call with > > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken > > I meant MLO_AN_PHY, sorry. The MAC could go into a low power mode. Andrew
On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote: > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote: > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin > > > <linux@armlinux.org.uk> wrote: > > > > > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > > > > > Hi Russell, > > > > > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > > > > > This should be removed - state->link is not for use in mac_config. > > > > > > Even in fixed mode, the link can be brought up/down by means of a > > > > > > gpio, and this should be dealt with via the mac_link_* functions. > > > > > > > > > > > > > > > > What do you mean exactly that state->link is not for use, is that true in > > > > > general? > > > > > > > > Yes. mac_config() should not touch it; it is not always in a defined > > > > state. For example, if you set modes via ethtool (the > > > > ethtool_ksettings_set API) then state->link will probably contain > > > > zero irrespective of the true link state. > > > > > > > > > > Experimentally, state->link is zero at the same time as state->speed > > > is -1, so just ignoring !state->link made sense. This is not in-band > > > AN. What is your suggestion? Should I proceed to try and configure the > > > MAC for SPEED_UNKNOWN? > > > > What would you have done with a PHY when the link is down, what speed > > would you have configured in the phylib adjust_link callback? phylib > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down. > > > > With phylib, I'd make the driver ignore the speed and do nothing. > With phylink, I'd make the core not call mac_config. > But what happened is I saw phylink call mac_config anyway, said > 'weird' and proceeded to ignore it as I would have for phylib. > I'm just not understanding your position - it seems like you're > implying there's a bug in phylink and the function call with > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken > place, which is what I wanted to confirm. It is not a bug. It is a request to configure the MAC, and what it's saying is "we don't know what speed and/or duplex". Take for instance when the network adapter is brought up initially. The link is most likely down, but we should configure the initial MAC operating parameters (such as the PHY interface). Phylink makes a mac_config() call with the speed and duplex set to UNKNOWN. Using your theory, we shouldn't be making that call. In which case, MAC drivers aren't going to initially configure their interface settings. _That_ would be a bug. > > It's unlikely that it would switch between SGMII and USXGMII > > dynamically, as USXGMII supports speeds from 10G down to 10M. > > > > Where interface mode switching tends to be used is with modes such > > as 10GBASE-R, which doesn't support anything except 10G. In order > > for the PHY to operate at slower speeds, it has a few options: > > > > 1) perform rate adaption. > > 2) dynamically switch interface type to an interface type that > > supports the desired speed. > > 3) just not support slower speeds. > > > > So am I reading this correctly - it kind of makes sense for gigabit > MAC drivers to not check for the MII interface changing protocol? Again, that's incorrect in the general case. Gigabit includes SGMII and 802.3z PHY protocols which need to be switched between for SFPs.
On Wed, 26 Jun 2019 at 10:42, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote: > > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin > > <linux@armlinux.org.uk> wrote: > > > > > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote: > > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin > > > > <linux@armlinux.org.uk> wrote: > > > > > > > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > > > > > > Hi Russell, > > > > > > > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > > > > > > This should be removed - state->link is not for use in mac_config. > > > > > > > Even in fixed mode, the link can be brought up/down by means of a > > > > > > > gpio, and this should be dealt with via the mac_link_* functions. > > > > > > > > > > > > > > > > > > > What do you mean exactly that state->link is not for use, is that true in > > > > > > general? > > > > > > > > > > Yes. mac_config() should not touch it; it is not always in a defined > > > > > state. For example, if you set modes via ethtool (the > > > > > ethtool_ksettings_set API) then state->link will probably contain > > > > > zero irrespective of the true link state. > > > > > > > > > > > > > Experimentally, state->link is zero at the same time as state->speed > > > > is -1, so just ignoring !state->link made sense. This is not in-band > > > > AN. What is your suggestion? Should I proceed to try and configure the > > > > MAC for SPEED_UNKNOWN? > > > > > > What would you have done with a PHY when the link is down, what speed > > > would you have configured in the phylib adjust_link callback? phylib > > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down. > > > > > > > With phylib, I'd make the driver ignore the speed and do nothing. > > With phylink, I'd make the core not call mac_config. > > But what happened is I saw phylink call mac_config anyway, said > > 'weird' and proceeded to ignore it as I would have for phylib. > > I'm just not understanding your position - it seems like you're > > implying there's a bug in phylink and the function call with > > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken > > place, which is what I wanted to confirm. > > It is not a bug. It is a request to configure the MAC, and what it's > saying is "we don't know what speed and/or duplex". > > Take for instance when the network adapter is brought up initially. > The link is most likely down, but we should configure the initial MAC > operating parameters (such as the PHY interface). Phylink makes a > mac_config() call with the speed and duplex set to UNKNOWN. > > Using your theory, we shouldn't be making that call. In which case, > MAC drivers aren't going to initially configure their interface > settings. > > _That_ would be a bug. > So you're saying that: - state->link should not be checked, because it is not guaranteed to be valid - state->speed, state->duplex, state->pause *should* be checked, because it is not guaranteed to be valid Is state->interface always valid? I don't think I follow the pattern here. Or shouldn't I check speed, duplex and pause either, and try to pass the MAC UNKNOWN values, inevitably failing at some point? Do Marvell MACs have an UNKNOWN setting? > > It's unlikely that it would switch between SGMII and USXGMII > > > dynamically, as USXGMII supports speeds from 10G down to 10M. > > > > > > Where interface mode switching tends to be used is with modes such > > > as 10GBASE-R, which doesn't support anything except 10G. In order > > > for the PHY to operate at slower speeds, it has a few options: > > > > > > 1) perform rate adaption. > > > 2) dynamically switch interface type to an interface type that > > > supports the desired speed. > > > 3) just not support slower speeds. > > > > > > > So am I reading this correctly - it kind of makes sense for gigabit > > MAC drivers to not check for the MII interface changing protocol? > > Again, that's incorrect in the general case. Gigabit includes SGMII > and 802.3z PHY protocols which need to be switched between for SFPs. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up Thanks, -Vladimir
On Wed, Jun 26, 2019 at 11:46:15AM +0300, Vladimir Oltean wrote: > On Wed, 26 Jun 2019 at 10:42, Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote: > > > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin > > > <linux@armlinux.org.uk> wrote: > > > > > > > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote: > > > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin > > > > > <linux@armlinux.org.uk> wrote: > > > > > > > > > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote: > > > > > > > Hi Russell, > > > > > > > > > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote: > > > > > > > > This should be removed - state->link is not for use in mac_config. > > > > > > > > Even in fixed mode, the link can be brought up/down by means of a > > > > > > > > gpio, and this should be dealt with via the mac_link_* functions. > > > > > > > > > > > > > > > > > > > > > > What do you mean exactly that state->link is not for use, is that true in > > > > > > > general? > > > > > > > > > > > > Yes. mac_config() should not touch it; it is not always in a defined > > > > > > state. For example, if you set modes via ethtool (the > > > > > > ethtool_ksettings_set API) then state->link will probably contain > > > > > > zero irrespective of the true link state. > > > > > > > > > > > > > > > > Experimentally, state->link is zero at the same time as state->speed > > > > > is -1, so just ignoring !state->link made sense. This is not in-band > > > > > AN. What is your suggestion? Should I proceed to try and configure the > > > > > MAC for SPEED_UNKNOWN? > > > > > > > > What would you have done with a PHY when the link is down, what speed > > > > would you have configured in the phylib adjust_link callback? phylib > > > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down. > > > > > > > > > > With phylib, I'd make the driver ignore the speed and do nothing. > > > With phylink, I'd make the core not call mac_config. > > > But what happened is I saw phylink call mac_config anyway, said > > > 'weird' and proceeded to ignore it as I would have for phylib. > > > I'm just not understanding your position - it seems like you're > > > implying there's a bug in phylink and the function call with > > > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken > > > place, which is what I wanted to confirm. > > > > It is not a bug. It is a request to configure the MAC, and what it's > > saying is "we don't know what speed and/or duplex". > > > > Take for instance when the network adapter is brought up initially. > > The link is most likely down, but we should configure the initial MAC > > operating parameters (such as the PHY interface). Phylink makes a > > mac_config() call with the speed and duplex set to UNKNOWN. > > > > Using your theory, we shouldn't be making that call. In which case, > > MAC drivers aren't going to initially configure their interface > > settings. > > > > _That_ would be a bug. > > > > So you're saying that: > - state->link should not be checked, because it is not guaranteed to be valid state->link is undefined. > - state->speed, state->duplex, state->pause *should* be checked, These will always be valid for FIXED and PHY modes, but _may_ be UNKNOWN, meaning phylink does not have any information about what the speed should be. speed and duplex are not defined for inband modes, since the purpose of inband modes is to communicate this information through... inband information, which the MAC driver already has access to. pause is a different matter because it is present in some inband modes but not others. Which fields may be examined are now documented in the phylink documentation in mainline kernels. > Is state->interface always valid? Yes. > I don't think I follow the pattern here. Or shouldn't I check speed, > duplex and pause either, and try to pass the MAC UNKNOWN values, > inevitably failing at some point? Do Marvell MACs have an UNKNOWN > setting? Why do you think that just because state->speed is SPEED_UNKNOWN you have to dream up some weird "unknown" value for the MAC? Default it to something sensible, just like you would do if phylib reports SPEED_UNKNOWN during link-down. I really don't get what the problem is here.
On 6/25/19 3:41 PM, Andrew Lunn wrote: > On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote: >> On 6/25/19 2:02 PM, Andrew Lunn wrote: >>>> But will there still be a mechanism to ignore link partner's advertising >>>> and force these parameters? >>> >From man 1 ethtool: >>> >>> -a --show-pause >>> Queries the specified Ethernet device for pause parameter information. >>> >>> -A --pause >>> Changes the pause parameters of the specified Ethernet device. >>> >>> autoneg on|off >>> Specifies whether pause autonegotiation should be enabled. >>> >>> rx on|off >>> Specifies whether RX pause should be enabled. >>> >>> tx on|off >>> Specifies whether TX pause should be enabled. >>> >>> You need to check the driver to see if it actually implements this >>> ethtool call, but that is how it should be configured. >>> >>> Andrew >>> >> Thank you Andrew, >> >> So in this context, my question is the difference between "enabling" and >> "forcing". Here's that register for the mt7620 (which has an mt7530 on >> its die): https://imgur.com/a/pTk0668 I believe this is also what René >> is seeking clarity on? > Lets start with normal operation. If the MAC supports pause or asym > pause, it calls phy_support_sym_pause() or phy_support_asym_pause(). > phylib will then configure the PHY to advertise pause as appropriate. > Once auto-neg has completed, the results of the negotiation are set in > phydev. So phdev->pause and phydev->asym_pause. The MAC callback is > then used to tell the MAC about the autoneg results. The MAC should be > programmed using the values in phdev->pause and phydev->asym_pause. > > For ethtool, the MAC driver needs to implement .get_pauseparam and > .set_pauseparam. The set_pauseparam needs to validate the settings, > using phy_validate_pause(). If valid, phy_set_asym_pause() is used to > tell the PHY about the new configuration. This will trigger a new > auto-neg if auto-neg is enabled, and the results will be passed back > in the usual way. If auto-neg is disabled, or pause auto-neg is > disabled, the MAC should configure pause directly based on the > settings passed. > > Looking at the data sheet page, you want FORCE_MODE_Pn set. You never > want the MAC directly talking to the PHY. Bad things will happen. So what exactly do you mean by the MAC directly talking to the PHY? Do you mean setting speed, duplex, etc. via the MAC registers instead of via MDIO to the MII registers of the PHY? > Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and > phydev->asym_pause. > > The same idea applies when using phylink. > > Andrew You're help is greatly appreciated here. Admittedly, I'm also trying to get this working in the now deprecated swconfig for a 3.18 kernel that's in production. In my code, I had just set the appropriate bits in both the MAC and mii registers -- did I just shoot myself in the foot or only toe or two? :) I should probably start a separate thread for this. (And probably attempt to wrestle an mt7530 programmer's guide out of MediaTek!) Thanks, Daniel PS: I found a rather humorous quote from the mt7621 datasheet regarding the MAC registers (at 0x3000 for port 0, 0x3100 for port 1, etc.): 2.4 Link Status You can find MAC control register put at 0x3500 for MAC 5, and 0x3600 for MAC 6. You can change MAC ability at this register. We would suggest don’t use the register 0x3000 to 0x3400. It may not work. I'm not sure if this only applies to something in between the mt7621 and it's internal mt7530 or not.
> > Looking at the data sheet page, you want FORCE_MODE_Pn set. You never > > want the MAC directly talking to the PHY. Bad things will happen. > > So what exactly do you mean by the MAC directly talking to the PHY? Do > you mean setting speed, duplex, etc. via the MAC registers instead of > via MDIO to the MII registers of the PHY? The data sheet implies the MAC hardware performs reads on the PHY to get the status, and then uses that to configure the MAC. This is often called PHY polling. The MAC hardware however has no idea what the PHY driver is doing. The MAC does not take the PHY mutex. So the PHY driver might be doing something at the same time the MAC hardware polls the PHY, and it gets odd results. Some PHYs have multiple pages, and for example reading the temperature sensor means swapping pages. If the MAC hardware was to poll while the sensor is being read, it would not get the link status, it would read some random temperature register. So you want the PHY driver to read the results of auto-neg and it then tell the MAC the results, so the MAC can be configured correctly. > > Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and > > phydev->asym_pause. > > > > The same idea applies when using phylink. > > > > Andrew > > You're help is greatly appreciated here. Admittedly, I'm also trying to > get this working in the now deprecated swconfig for a 3.18 kernel that's > in production. I'm not very familiar with swconfig. Is there software driving the PHY? If not, it is then safe for the MAC hardware to poll the PHY. Andrew
Hello Andrew, On 6/27/19 2:28 PM, Andrew Lunn wrote: >>> Looking at the data sheet page, you want FORCE_MODE_Pn set. You never >>> want the MAC directly talking to the PHY. Bad things will happen. >> So what exactly do you mean by the MAC directly talking to the PHY? Do >> you mean setting speed, duplex, etc. via the MAC registers instead of >> via MDIO to the MII registers of the PHY? > The data sheet implies the MAC hardware performs reads on the PHY to > get the status, and then uses that to configure the MAC. This is often > called PHY polling. The MAC hardware however has no idea what the PHY > driver is doing. The MAC does not take the PHY mutex. So the PHY > driver might be doing something at the same time the MAC hardware > polls the PHY, and it gets odd results. Some PHYs have multiple pages, > and for example reading the temperature sensor means swapping > pages. If the MAC hardware was to poll while the sensor is being read, > it would not get the link status, it would read some random > temperature register. > > So you want the PHY driver to read the results of auto-neg and it then > tell the MAC the results, so the MAC can be configured correctly. Thank you, this is very helpful! I finally understand why these settings are in two different places. :) Unfortunately this driver (in OpenWRT) does a lot of "magic" during init to registers that I don't have documentation for, but I see where auto-polling can be disabled now. >>> Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and >>> phydev->asym_pause. >>> >>> The same idea applies when using phylink. >>> >>> Andrew >> You're help is greatly appreciated here. Admittedly, I'm also trying to >> get this working in the now deprecated swconfig for a 3.18 kernel that's >> in production. > I'm not very familiar with swconfig. Is there software driving the > PHY? If not, it is then safe for the MAC hardware to poll the PHY. > > Andrew swconfig is an netlink-based interface the OpenWRT team developed for configuring switches before DSA was converted into a vendor-neutral interface. Now that DSA does what swconfig was designed for it has been deprecated, although (to my knowledge) we don't yet have DSA for all devices that OpenWRT supports. Daniel
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 3181e95586d6..9c5e4dd00826 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -13,7 +13,7 @@ #include <linux/of_mdio.h> #include <linux/of_net.h> #include <linux/of_platform.h> -#include <linux/phy.h> +#include <linux/phylink.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/reset.h> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset) return ARRAY_SIZE(mt7530_mib); } -static void mt7530_adjust_link(struct dsa_switch *ds, int port, - struct phy_device *phydev) -{ - struct mt7530_priv *priv = ds->priv; - - if (phy_is_pseudo_fixed_link(phydev)) { - dev_dbg(priv->dev, "phy-mode for master device = %x\n", - phydev->interface); - - /* Setup TX circuit incluing relevant PAD and driving */ - mt7530_pad_clk_setup(ds, phydev->interface); - - if (priv->id == ID_MT7530) { - /* Setup RX circuit, relevant PAD and driving on the - * host which must be placed after the setup on the - * device side is all finished. - */ - mt7623_pad_clk_setup(ds); - } - } else { - u16 lcl_adv = 0, rmt_adv = 0; - u8 flowctrl; - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE; - - switch (phydev->speed) { - case SPEED_1000: - mcr |= PMCR_FORCE_SPEED_1000; - break; - case SPEED_100: - mcr |= PMCR_FORCE_SPEED_100; - break; - } - - if (phydev->link) - mcr |= PMCR_FORCE_LNK; - - if (phydev->duplex) { - mcr |= PMCR_FORCE_FDX; - - if (phydev->pause) - rmt_adv = LPA_PAUSE_CAP; - if (phydev->asym_pause) - rmt_adv |= LPA_PAUSE_ASYM; - - lcl_adv = linkmode_adv_to_lcl_adv_t( - phydev->advertising); - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv); - - if (flowctrl & FLOW_CTRL_TX) - mcr |= PMCR_TX_FC_EN; - if (flowctrl & FLOW_CTRL_RX) - mcr |= PMCR_RX_FC_EN; - } - mt7530_write(priv, MT7530_PMCR_P(port), mcr); - } -} - static int mt7530_cpu_port_enable(struct mt7530_priv *priv, int port) @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds) return 0; } +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port, + unsigned int mode, + const struct phylink_link_state *state) +{ + struct mt7530_priv *priv = ds->priv; + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN | + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN; + + switch (port) { + case 0: /* Internal phy */ + case 1: + case 2: + case 3: + case 4: + if (state->interface != PHY_INTERFACE_MODE_GMII) + goto unsupported; + break; + /* case 5: Port 5 is not supported! */ + case 6: /* 1st cpu port */ + if (state->interface != PHY_INTERFACE_MODE_RGMII && + state->interface != PHY_INTERFACE_MODE_TRGMII) + goto unsupported; + + /* Setup TX circuit incluing relevant PAD and driving */ + mt7530_pad_clk_setup(ds, state->interface); + + if (priv->id == ID_MT7530) { + /* Setup RX circuit, relevant PAD and driving on the + * host which must be placed after the setup on the + * device side is all finished. + */ + mt7623_pad_clk_setup(ds); + } + break; + default: + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); + return; + } + + if (!state->an_enabled || mode == MLO_AN_FIXED) { + mcr |= PMCR_FORCE_MODE; + + if (state->speed == SPEED_1000) + mcr |= PMCR_FORCE_SPEED_1000; + if (state->speed == SPEED_100) + mcr |= PMCR_FORCE_SPEED_100; + if (state->duplex == DUPLEX_FULL) + mcr |= PMCR_FORCE_FDX; + if (state->link || mode == MLO_AN_FIXED) + mcr |= PMCR_FORCE_LNK; + if (state->pause || phylink_test(state->advertising, Pause)) + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN; + if (state->pause & MLO_PAUSE_TX) + mcr |= PMCR_TX_FC_EN; + if (state->pause & MLO_PAUSE_RX) + mcr |= PMCR_RX_FC_EN; + } + + mt7530_write(priv, MT7530_PMCR_P(port), mcr); + + return; + +unsupported: + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n", + __func__, port, state->interface, phy_modes(state->interface)); +} + +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port, + unsigned int mode, + phy_interface_t interface) +{ + /* Do nothing */ +} + +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port, + unsigned int mode, + phy_interface_t interface, + struct phy_device *phydev) +{ + /* Do nothing */ +} + +static void mt7530_phylink_validate(struct dsa_switch *ds, int port, + unsigned long *supported, + struct phylink_link_state *state) +{ + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; + + switch (port) { + case 0: /* Internal phy */ + case 1: + case 2: + case 3: + case 4: + if (state->interface != PHY_INTERFACE_MODE_NA && + state->interface != PHY_INTERFACE_MODE_GMII) + goto unsupported; + break; + /* case 5: Port 5 not supported! */ + case 6: /* 1st cpu port */ + if (state->interface != PHY_INTERFACE_MODE_RGMII && + state->interface != PHY_INTERFACE_MODE_TRGMII) + goto unsupported; + break; + default: + linkmode_zero(supported); + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port); + return; + } + + phylink_set(mask, Autoneg); + phylink_set(mask, Pause); + phylink_set(mask, Asym_Pause); + phylink_set(mask, MII); + + phylink_set(mask, 10baseT_Half); + phylink_set(mask, 10baseT_Full); + phylink_set(mask, 100baseT_Half); + phylink_set(mask, 100baseT_Full); + phylink_set(mask, 1000baseT_Full); + phylink_set(mask, 1000baseT_Half); + + linkmode_and(supported, supported, mask); + linkmode_and(state->advertising, state->advertising, mask); + return; + +unsupported: + linkmode_zero(supported); + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n", + __func__, state->interface, phy_modes(state->interface)); +} + +static int +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port, + struct phylink_link_state *state) +{ + struct mt7530_priv *priv = ds->priv; + u32 pmsr; + + if (port < 0 || port >= MT7530_NUM_PORTS) + return -EINVAL; + + pmsr = mt7530_read(priv, MT7530_PMSR_P(port)); + + state->link = (pmsr & PMSR_LINK); + state->an_complete = state->link; + state->duplex = (pmsr & PMSR_DPX) >> 1; + + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) { + case 0: + state->speed = SPEED_10; + break; + case PMSR_SPEED_100: + state->speed = SPEED_100; + break; + case PMSR_SPEED_1000: + state->speed = SPEED_1000; + break; + default: + state->speed = SPEED_UNKNOWN; + break; + } + + state->pause = 0; + if (pmsr & PMSR_RX_FC) + state->pause |= MLO_PAUSE_RX; + if (pmsr & PMSR_TX_FC) + state->pause |= MLO_PAUSE_TX; + + return 1; +} + static const struct dsa_switch_ops mt7530_switch_ops = { .get_tag_protocol = mtk_get_tag_protocol, .setup = mt7530_setup, @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = { .phy_write = mt7530_phy_write, .get_ethtool_stats = mt7530_get_ethtool_stats, .get_sset_count = mt7530_get_sset_count, - .adjust_link = mt7530_adjust_link, .port_enable = mt7530_port_enable, .port_disable = mt7530_port_disable, .port_stp_state_set = mt7530_stp_state_set, @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = { .port_vlan_prepare = mt7530_port_vlan_prepare, .port_vlan_add = mt7530_port_vlan_add, .port_vlan_del = mt7530_port_vlan_del, + .phylink_validate = mt7530_phylink_validate, + .phylink_mac_link_state = mt7530_phylink_mac_link_state, + .phylink_mac_config = mt7530_phylink_mac_config, + .phylink_mac_link_down = mt7530_phylink_mac_link_down, + .phylink_mac_link_up = mt7530_phylink_mac_link_up, }; static const struct of_device_id mt7530_of_match[] = { diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h index bfac90f48102..41d9a132ac70 100644 --- a/drivers/net/dsa/mt7530.h +++ b/drivers/net/dsa/mt7530.h @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr { #define PMCR_FORCE_SPEED_100 BIT(2) #define PMCR_FORCE_FDX BIT(1) #define PMCR_FORCE_LNK BIT(0) +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \ PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \ PMCR_TX_EN | PMCR_RX_EN | \ @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr { PMCR_TX_FC_EN | PMCR_RX_FC_EN) #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100) +#define PMSR_EEE1G BIT(7) +#define PMSR_EEE100M BIT(6) +#define PMSR_RX_FC BIT(5) +#define PMSR_TX_FC BIT(4) +#define PMSR_SPEED_1000 BIT(3) +#define PMSR_SPEED_100 BIT(2) +#define PMSR_DPX BIT(1) +#define PMSR_LINK BIT(0) /* Register for MIB */ #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
Convert mt7530 to PHYLINK API Signed-off-by: René van Dorst <opensource@vdorst.com> --- drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++---------- drivers/net/dsa/mt7530.h | 9 ++ 2 files changed, 187 insertions(+), 59 deletions(-)