Message ID | 20241204072427.17778-1-ansuelsmth@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | net: dsa: Add Airoha AN8855 support | expand |
Hello Christian, On Wed, 4 Dec 2024 08:24:10 +0100 Christian Marangi <ansuelsmth@gmail.com> wrote: > Add Airoha AN8855 5-Port Gigabit DSA switch. > > The switch is also a nvmem-provider as it does have EFUSE to calibrate > the internal PHYs. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> [...] My two-cents review below :) > +static void > +an8855_phylink_mac_config(struct phylink_config *config, unsigned int mode, > + const struct phylink_link_state *state) > +{ > + struct dsa_port *dp = dsa_phylink_to_port(config); > + struct dsa_switch *ds = dp->ds; > + struct an8855_priv *priv; > + int port = dp->index; > + > + priv = ds->priv; > + > + if (port != 5) { > + if (port > 5) > + dev_err(ds->dev, "unsupported port: %d", port); > + return; > + } Looks like the above condition can be simplified to : if (port > 5) dev_err(...); > + > + regmap_update_bits(priv->regmap, AN8855_PMCR_P(port), > + AN8855_PMCR_IFG_XMIT | AN8855_PMCR_MAC_MODE | > + AN8855_PMCR_BACKOFF_EN | AN8855_PMCR_BACKPR_EN, > + FIELD_PREP(AN8855_PMCR_IFG_XMIT, 0x1) | > + AN8855_PMCR_MAC_MODE | AN8855_PMCR_BACKOFF_EN | > + AN8855_PMCR_BACKPR_EN); > +} > + > +static void an8855_phylink_get_caps(struct dsa_switch *ds, int port, > + struct phylink_config *config) > +{ > + switch (port) { > + case 0: > + case 1: > + case 2: > + case 3: > + case 4: > + __set_bit(PHY_INTERFACE_MODE_GMII, > + config->supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_INTERNAL, > + config->supported_interfaces); > + break; > + case 5: > + phy_interface_set_rgmii(config->supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_SGMII, > + config->supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_2500BASEX, > + config->supported_interfaces); > + break; > + } > + > + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > + MAC_10 | MAC_100 | MAC_1000FD; For port 5, you may also add the MAC_2500FD capability as it supports 2500BASEX ? > +} > + [...] > + > +static void > +an8855_phylink_mac_link_up(struct phylink_config *config, > + struct phy_device *phydev, unsigned int mode, > + phy_interface_t interface, int speed, int duplex, > + bool tx_pause, bool rx_pause) > +{ > + struct dsa_port *dp = dsa_phylink_to_port(config); > + struct an8855_priv *priv = dp->ds->priv; > + int port = dp->index; > + u32 reg; > + > + reg = regmap_read(priv->regmap, AN8855_PMCR_P(port), ®); > + if (phylink_autoneg_inband(mode)) { > + reg &= ~AN8855_PMCR_FORCE_MODE; > + } else { > + reg |= AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK; > + > + reg &= ~AN8855_PMCR_FORCE_SPEED; > + switch (speed) { > + case SPEED_10: > + reg |= AN8855_PMCR_FORCE_SPEED_10; > + break; > + case SPEED_100: > + reg |= AN8855_PMCR_FORCE_SPEED_100; > + break; > + case SPEED_1000: > + reg |= AN8855_PMCR_FORCE_SPEED_1000; > + break; > + case SPEED_2500: > + reg |= AN8855_PMCR_FORCE_SPEED_2500; > + break; > + case SPEED_5000: > + reg |= AN8855_PMCR_FORCE_SPEED_5000; > + break; There's no mention of any mode that can give support for the 5000Mbps speed, is it a leftover from previous work on the driver ? > + } > + > + reg &= ~AN8855_PMCR_FORCE_FDX; > + if (duplex == DUPLEX_FULL) > + reg |= AN8855_PMCR_FORCE_FDX; > + > + reg &= ~AN8855_PMCR_RX_FC_EN; > + if (rx_pause || dsa_port_is_cpu(dp)) > + reg |= AN8855_PMCR_RX_FC_EN; > + > + reg &= ~AN8855_PMCR_TX_FC_EN; > + if (rx_pause || dsa_port_is_cpu(dp)) > + reg |= AN8855_PMCR_TX_FC_EN; > + > + /* Disable any EEE options */ > + reg &= ~(AN8855_PMCR_FORCE_EEE5G | AN8855_PMCR_FORCE_EEE2P5G | > + AN8855_PMCR_FORCE_EEE1G | AN8855_PMCR_FORCE_EEE100); > + } > + > + reg |= AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN; > + > + regmap_write(priv->regmap, AN8855_PMCR_P(port), reg); > +} > + > +static void an8855_pcs_get_state(struct phylink_pcs *pcs, > + struct phylink_link_state *state) > +{ > + struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs); > + u32 val; > + int ret; > + > + ret = regmap_read(priv->regmap, AN8855_PMSR_P(AN8855_CPU_PORT), &val); > + if (ret < 0) { > + state->link = false; > + return; > + } > + > + state->link = !!(val & AN8855_PMSR_LNK); > + state->an_complete = state->link; > + state->duplex = (val & AN8855_PMSR_DPX) ? DUPLEX_FULL : > + DUPLEX_HALF; > + > + switch (val & AN8855_PMSR_SPEED) { > + case AN8855_PMSR_SPEED_10: > + state->speed = SPEED_10; > + break; > + case AN8855_PMSR_SPEED_100: > + state->speed = SPEED_100; > + break; > + case AN8855_PMSR_SPEED_1000: > + state->speed = SPEED_1000; > + break; > + case AN8855_PMSR_SPEED_2500: > + state->speed = SPEED_2500; > + break; > + case AN8855_PMSR_SPEED_5000: > + state->speed = SPEED_5000; Same here for the SPEED_5000, unless i'm missing something :) Thanks, Maxime
On Wed, Dec 04, 2024 at 10:09:22AM +0100, Maxime Chevallier wrote: > Hello Christian, > > On Wed, 4 Dec 2024 08:24:10 +0100 > Christian Marangi <ansuelsmth@gmail.com> wrote: > > > Add Airoha AN8855 5-Port Gigabit DSA switch. > > > > The switch is also a nvmem-provider as it does have EFUSE to calibrate > > the internal PHYs. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > [...] > > My two-cents review below :) > > > +static void > > +an8855_phylink_mac_config(struct phylink_config *config, unsigned int mode, > > + const struct phylink_link_state *state) > > +{ > > + struct dsa_port *dp = dsa_phylink_to_port(config); > > + struct dsa_switch *ds = dp->ds; > > + struct an8855_priv *priv; > > + int port = dp->index; > > + > > + priv = ds->priv; > > + > > + if (port != 5) { > > + if (port > 5) > > + dev_err(ds->dev, "unsupported port: %d", port); > > + return; > > + } > > Looks like the above condition can be simplified to : > > if (port > 5) > dev_err(...); > > Well.... yep AHHAHA. > > + > > + regmap_update_bits(priv->regmap, AN8855_PMCR_P(port), > > + AN8855_PMCR_IFG_XMIT | AN8855_PMCR_MAC_MODE | > > + AN8855_PMCR_BACKOFF_EN | AN8855_PMCR_BACKPR_EN, > > + FIELD_PREP(AN8855_PMCR_IFG_XMIT, 0x1) | > > + AN8855_PMCR_MAC_MODE | AN8855_PMCR_BACKOFF_EN | > > + AN8855_PMCR_BACKPR_EN); > > +} > > + > > +static void an8855_phylink_get_caps(struct dsa_switch *ds, int port, > > + struct phylink_config *config) > > +{ > > + switch (port) { > > + case 0: > > + case 1: > > + case 2: > > + case 3: > > + case 4: > > + __set_bit(PHY_INTERFACE_MODE_GMII, > > + config->supported_interfaces); > > + __set_bit(PHY_INTERFACE_MODE_INTERNAL, > > + config->supported_interfaces); > > + break; > > + case 5: > > + phy_interface_set_rgmii(config->supported_interfaces); > > + __set_bit(PHY_INTERFACE_MODE_SGMII, > > + config->supported_interfaces); > > + __set_bit(PHY_INTERFACE_MODE_2500BASEX, > > + config->supported_interfaces); > > + break; > > + } > > + > > + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > + MAC_10 | MAC_100 | MAC_1000FD; > > For port 5, you may also add the MAC_2500FD capability as it supports > 2500BASEX ? > I didn't account for the CPU port that runs at 2.5. The LAN port are only 1g. Will add or maybe add the 2500FD only for cpu port? Maybe Russel can help in this? > > +} > > + > > [...] > > > + > > +static void > > +an8855_phylink_mac_link_up(struct phylink_config *config, > > + struct phy_device *phydev, unsigned int mode, > > + phy_interface_t interface, int speed, int duplex, > > + bool tx_pause, bool rx_pause) > > +{ > > + struct dsa_port *dp = dsa_phylink_to_port(config); > > + struct an8855_priv *priv = dp->ds->priv; > > + int port = dp->index; > > + u32 reg; > > + > > + reg = regmap_read(priv->regmap, AN8855_PMCR_P(port), ®); > > + if (phylink_autoneg_inband(mode)) { > > + reg &= ~AN8855_PMCR_FORCE_MODE; > > + } else { > > + reg |= AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK; > > + > > + reg &= ~AN8855_PMCR_FORCE_SPEED; > > + switch (speed) { > > + case SPEED_10: > > + reg |= AN8855_PMCR_FORCE_SPEED_10; > > + break; > > + case SPEED_100: > > + reg |= AN8855_PMCR_FORCE_SPEED_100; > > + break; > > + case SPEED_1000: > > + reg |= AN8855_PMCR_FORCE_SPEED_1000; > > + break; > > + case SPEED_2500: > > + reg |= AN8855_PMCR_FORCE_SPEED_2500; > > + break; > > + case SPEED_5000: > > + reg |= AN8855_PMCR_FORCE_SPEED_5000; > > + break; > > There's no mention of any mode that can give support for the 5000Mbps > speed, is it a leftover from previous work on the driver ? > Added 5000 as this is present in documentation bits but CPU can only go up to 2.5. Should I drop it? Idea was to futureproof it since it really seems they added these bits with the intention of having a newer switch with more advanced ports. > > + } > > + > > + reg &= ~AN8855_PMCR_FORCE_FDX; > > + if (duplex == DUPLEX_FULL) > > + reg |= AN8855_PMCR_FORCE_FDX; > > + > > + reg &= ~AN8855_PMCR_RX_FC_EN; > > + if (rx_pause || dsa_port_is_cpu(dp)) > > + reg |= AN8855_PMCR_RX_FC_EN; > > + > > + reg &= ~AN8855_PMCR_TX_FC_EN; > > + if (rx_pause || dsa_port_is_cpu(dp)) > > + reg |= AN8855_PMCR_TX_FC_EN; > > + > > + /* Disable any EEE options */ > > + reg &= ~(AN8855_PMCR_FORCE_EEE5G | AN8855_PMCR_FORCE_EEE2P5G | > > + AN8855_PMCR_FORCE_EEE1G | AN8855_PMCR_FORCE_EEE100); > > + } > > + > > + reg |= AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN; > > + > > + regmap_write(priv->regmap, AN8855_PMCR_P(port), reg); > > +} > > + > > +static void an8855_pcs_get_state(struct phylink_pcs *pcs, > > + struct phylink_link_state *state) > > +{ > > + struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs); > > + u32 val; > > + int ret; > > + > > + ret = regmap_read(priv->regmap, AN8855_PMSR_P(AN8855_CPU_PORT), &val); > > + if (ret < 0) { > > + state->link = false; > > + return; > > + } > > + > > + state->link = !!(val & AN8855_PMSR_LNK); > > + state->an_complete = state->link; > > + state->duplex = (val & AN8855_PMSR_DPX) ? DUPLEX_FULL : > > + DUPLEX_HALF; > > + > > + switch (val & AN8855_PMSR_SPEED) { > > + case AN8855_PMSR_SPEED_10: > > + state->speed = SPEED_10; > > + break; > > + case AN8855_PMSR_SPEED_100: > > + state->speed = SPEED_100; > > + break; > > + case AN8855_PMSR_SPEED_1000: > > + state->speed = SPEED_1000; > > + break; > > + case AN8855_PMSR_SPEED_2500: > > + state->speed = SPEED_2500; > > + break; > > + case AN8855_PMSR_SPEED_5000: > > + state->speed = SPEED_5000; > > Same here for the SPEED_5000, unless i'm missing something :) > Ditto same explaination above.
On Wed, Dec 04, 2024 at 10:09:22AM +0100, Maxime Chevallier wrote: > Hello Christian, > > On Wed, 4 Dec 2024 08:24:10 +0100 > Christian Marangi <ansuelsmth@gmail.com> wrote: > > > +static void > > +an8855_phylink_mac_config(struct phylink_config *config, unsigned int mode, > > + const struct phylink_link_state *state) > > +{ > > + struct dsa_port *dp = dsa_phylink_to_port(config); > > + struct dsa_switch *ds = dp->ds; > > + struct an8855_priv *priv; > > + int port = dp->index; > > + > > + priv = ds->priv; > > + > > + if (port != 5) { > > + if (port > 5) > > + dev_err(ds->dev, "unsupported port: %d", port); > > + return; > > + } > > Looks like the above condition can be simplified to : > > if (port > 5) > dev_err(...); Not quite, since if port is 0..4, the function returns at this point. If >5, then it prints an error and returns. So: if (port > 5) dev_err(...); if (port != 5) return; However, net/dsa/dsa.c::dsa_switch_parse_ports_of() already validates that the port number will not exceed ds->num_ports, so if that is set appropriately, the phylink functions will never be called with a port number larger than this, and checking it is redundant. So I think this should just be: if (port != 5) return; > > + case 5: > > + phy_interface_set_rgmii(config->supported_interfaces); > > + __set_bit(PHY_INTERFACE_MODE_SGMII, > > + config->supported_interfaces); > > + __set_bit(PHY_INTERFACE_MODE_2500BASEX, > > + config->supported_interfaces); > > + break; > > + } > > + > > + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > + MAC_10 | MAC_100 | MAC_1000FD; > > For port 5, you may also add the MAC_2500FD capability as it supports > 2500BASEX ? Agreed, that's needed. > > + case SPEED_5000: > > + reg |= AN8855_PMCR_FORCE_SPEED_5000; > > + break; > > There's no mention of any mode that can give support for the 5000Mbps > speed, is it a leftover from previous work on the driver ? Or maybe it's because 5GBASE-R wasn't a phy interface type when the driver was originally developed (iirc it's a relatively recent addition.)
On Wed, Dec 04, 2024 at 10:14:31AM +0100, Christian Marangi wrote: > On Wed, Dec 04, 2024 at 10:09:22AM +0100, Maxime Chevallier wrote: > > > + case 5: > > > + phy_interface_set_rgmii(config->supported_interfaces); > > > + __set_bit(PHY_INTERFACE_MODE_SGMII, > > > + config->supported_interfaces); > > > + __set_bit(PHY_INTERFACE_MODE_2500BASEX, > > > + config->supported_interfaces); > > > + break; > > > + } > > > + > > > + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > > + MAC_10 | MAC_100 | MAC_1000FD; > > > > For port 5, you may also add the MAC_2500FD capability as it supports > > 2500BASEX ? > > > > I didn't account for the CPU port that runs at 2.5. The LAN port are > only 1g. Will add or maybe add the 2500FD only for cpu port? > > Maybe Russel can help in this? *ll* please. Well, 2500BASE-X runs at 2.5G, so if MAC_2500FD isn't set in the mask, validation will fail for 2500BASE-X. > > > + case SPEED_5000: > > > + reg |= AN8855_PMCR_FORCE_SPEED_5000; > > > + break; > > > > There's no mention of any mode that can give support for the 5000Mbps > > speed, is it a leftover from previous work on the driver ? > > > > Added 5000 as this is present in documentation bits but CPU can only go > up to 2.5. Should I drop it? Idea was to futureproof it since it really > seems they added these bits with the intention of having a newer switch > with more advanced ports. Is there any mention of supporting interfaces faster than 2500BASE-X ?
On Wed, Dec 04, 2024 at 09:24:45AM +0000, Russell King (Oracle) wrote: > On Wed, Dec 04, 2024 at 10:14:31AM +0100, Christian Marangi wrote: > > On Wed, Dec 04, 2024 at 10:09:22AM +0100, Maxime Chevallier wrote: > > > > + case 5: > > > > + phy_interface_set_rgmii(config->supported_interfaces); > > > > + __set_bit(PHY_INTERFACE_MODE_SGMII, > > > > + config->supported_interfaces); > > > > + __set_bit(PHY_INTERFACE_MODE_2500BASEX, > > > > + config->supported_interfaces); > > > > + break; > > > > + } > > > > + > > > > + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > > > + MAC_10 | MAC_100 | MAC_1000FD; > > > > > > For port 5, you may also add the MAC_2500FD capability as it supports > > > 2500BASEX ? > > > > > > > I didn't account for the CPU port that runs at 2.5. The LAN port are > > only 1g. Will add or maybe add the 2500FD only for cpu port? > > > > Maybe Russel can help in this? > > *ll* please. emabrassing... sorry. > > Well, 2500BASE-X runs at 2.5G, so if MAC_2500FD isn't set in the mask, > validation will fail for 2500BASE-X. > > > > > + case SPEED_5000: > > > > + reg |= AN8855_PMCR_FORCE_SPEED_5000; > > > > + break; > > > > > > There's no mention of any mode that can give support for the 5000Mbps > > > speed, is it a leftover from previous work on the driver ? > > > > > > > Added 5000 as this is present in documentation bits but CPU can only go > > up to 2.5. Should I drop it? Idea was to futureproof it since it really > > seems they added these bits with the intention of having a newer switch > > with more advanced ports. > > Is there any mention of supporting interfaces faster than 2500BASE-X ? > In MAC Layer function description, they say: - Support 10/100/1000/2500/5000 Mbps bit rates So in theory it can support up to that speed.
On Wed, Dec 04, 2024 at 10:33:33AM +0100, Christian Marangi wrote: > On Wed, Dec 04, 2024 at 09:24:45AM +0000, Russell King (Oracle) wrote: > > > Added 5000 as this is present in documentation bits but CPU can only go > > > up to 2.5. Should I drop it? Idea was to futureproof it since it really > > > seems they added these bits with the intention of having a newer switch > > > with more advanced ports. > > > > Is there any mention of supporting interfaces faster than 2500BASE-X ? > > > > In MAC Layer function description, they say: > - Support 10/100/1000/2500/5000 Mbps bit rates > > So in theory it can support up to that speed. Maybe the internal IP supports this but the SoC doesn't? However, I was asking about interfaces rather than speeds - so RGMII, SGMII, 2500BASE-X... is there a mention of anything else?
On Wed, Dec 04, 2024 at 10:00:41AM +0000, Russell King (Oracle) wrote: > On Wed, Dec 04, 2024 at 10:33:33AM +0100, Christian Marangi wrote: > > On Wed, Dec 04, 2024 at 09:24:45AM +0000, Russell King (Oracle) wrote: > > > > Added 5000 as this is present in documentation bits but CPU can only go > > > > up to 2.5. Should I drop it? Idea was to futureproof it since it really > > > > seems they added these bits with the intention of having a newer switch > > > > with more advanced ports. > > > > > > Is there any mention of supporting interfaces faster than 2500BASE-X ? > > > > > > > In MAC Layer function description, they say: > > - Support 10/100/1000/2500/5000 Mbps bit rates > > > > So in theory it can support up to that speed. > > Maybe the internal IP supports this but the SoC doesn't? > > However, I was asking about interfaces rather than speeds - so RGMII, > SGMII, 2500BASE-X... is there a mention of anything else? > I can see mention of 5g Base-R so I assume supported but in the sdk there isn't any code to configure it/I don't have any hardware. (and documentation is empty for anything PCS related for register, just block diagram) I can ask it that is expected to work tho.
> Added 5000 as this is present in documentation bits but CPU can only go > up to 2.5. Should I drop it? Idea was to futureproof it since it really > seems they added these bits with the intention of having a newer switch > with more advanced ports. The other way to future proof this is to have a dev_err() and -EINVAL. Testing will then quickly find any missing places in the code. Andrew