| Message ID | cover.1605830552.git.pavana.sharma@digi.com |
|---|---|
| Headers | show |
| Series | Add support for mv88e6393x family of Marvell | expand |
On Fri, Nov 20, 2020 at 10:25:33AM +1000, Pavana Sharma wrote: > Add 5GBASE-R phy interface mode > > Signed-off-by: Pavana Sharma <pavana.sharma@digi.com> > --- > include/linux/phy.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index eb3cb1a98b45..71e280059ec5 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -106,6 +106,7 @@ extern const int phy_10gbit_features_array[1]; > * @PHY_INTERFACE_MODE_TRGMII: Turbo RGMII > * @PHY_INTERFACE_MODE_1000BASEX: 1000 BaseX > * @PHY_INTERFACE_MODE_2500BASEX: 2500 BaseX > + * @PHY_INTERFACE_MODE_5GBASER: 5G BaseR > * @PHY_INTERFACE_MODE_RXAUI: Reduced XAUI > * @PHY_INTERFACE_MODE_XAUI: 10 Gigabit Attachment Unit Interface > * @PHY_INTERFACE_MODE_10GBASER: 10G BaseR > @@ -137,6 +138,8 @@ typedef enum { > PHY_INTERFACE_MODE_TRGMII, > PHY_INTERFACE_MODE_1000BASEX, > PHY_INTERFACE_MODE_2500BASEX, > + /* 5GBASE-R mode */ > + PHY_INTERFACE_MODE_5GBASER, Again, what is the value of the comment? 10GBASE-R has a comment because it is different from the rest, XFI and SFI caused a of discussion, and it was used wrong. But there does not seem to be anything special for 5GBASE-R. Andrew
> @@ -459,7 +459,7 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, > chip->ports[port].cmode = cmode; > > lane = mv88e6xxx_serdes_get_lane(chip, port); > - if (!lane) > + if (lane < 0) > return -ENODEV; return lane since lane is an errno. Other than that: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> @@ -222,8 +231,8 @@ static int mv88e6xxx_port_set_speed_duplex(struct mv88e6xxx_chip *chip, > return err; > > reg &= ~(MV88E6XXX_PORT_MAC_CTL_SPEED_MASK | > - MV88E6XXX_PORT_MAC_CTL_FORCE_DUPLEX | > - MV88E6XXX_PORT_MAC_CTL_DUPLEX_FULL); > + MV88E6XXX_PORT_MAC_CTL_FORCE_DUPLEX | > + MV88E6XXX_PORT_MAC_CTL_DUPLEX_FULL); This looks like a white space change? > if (alt_bit) > reg &= ~MV88E6390_PORT_MAC_CTL_ALTSPEED; > @@ -390,6 +399,84 @@ phy_interface_t mv88e6390x_port_max_speed_mode(int port) > return PHY_INTERFACE_MODE_NA; > } > > +/* Support 10, 100, 200, 1000, 2500, 5000, 10000 Mbps (e.g. 88E6393X) */ > +int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port, > + int speed, int duplex) > +{ > + u16 reg, ctrl; > + int err; > + > + if (speed == SPEED_MAX) > + speed = (port > 0 && port < 9) ? 1000 : 10000; > + > + if (speed == 200 && port != 0) > + return -EOPNOTSUPP; > + > + if (speed >= 2500 && port > 0 && port < 9) > + return -EOPNOTSUPP; Maybe i'm missing something, but it looks like at this point you can call return mv88e6xxx_port_set_speed_duplex(chip, port, speed, true, true, duplex); > +/* Offset 0x0E: Policy & MGMT Control Register for FAMILY 6191X 6193X 6393X */ > + > +static int mv88e6393x_port_policy_write(struct mv88e6xxx_chip *chip, u16 pointer, > + u8 data) > +{ > + > + int err = 0; > + int port; > + u16 reg; > + > + /* Setup per Port policy register */ > + for (port = 0; port < mv88e6xxx_num_ports(chip); port++) { > + if (dsa_is_unused_port(chip->ds, port)) > + continue; > + > + /* Prevent the use of an invalid port. */ > + if (mv88e6xxx_is_invalid_port(chip, port)) { > + dev_err(chip->dev, "port %d is invalid\n", port); > + return -EINVAL; > + } /* Mark certain ports as invalid. This is required for example for the * MV88E6220 (which is in general a MV88E6250 with 7 ports) but the * ports 2-4 are not routed to pins. */ unsigned int invalid_port_mask; You have not set this in the info structure of the 6393x devices, so you can skip this check. > +/* Only Ports 0, 9 and 10 have SERDES lanes. Return the SERDES lane address > + * a port is using else Returns -ENODEV. > + */ > +int mv88e6393x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port) > +{ > + u8 cmode = chip->ports[port].cmode; > + int lane = -ENODEV; > + > + if (port == 0 || port == 9 || port == 10) { Maybe if (port != 0 && port != 9 && port == 10) return -ENODEV > + if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX || > + cmode == MV88E6XXX_PORT_STS_CMODE_SGMII || > + cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX || > + cmode == MV88E6XXX_PORT_STS_CMODE_5GBASER || > + cmode == MV88E6XXX_PORT_STS_CMODE_10GBASER || > + cmode == MV88E6XXX_PORT_STS_CMODE_USXGMII) Indentation is messed up. > + lane = port; return port; Andrew
Hi Andrew, On Fri, 20 Nov 2020 02:29:06 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > + if (speed >= 2500 && port > 0 && port < 9) > > + return -EOPNOTSUPP; > > Maybe i'm missing something, but it looks like at this point you can > call > > return mv88e6xxx_port_set_speed_duplex(chip, port, speed, true, true, duplex); He can't. That function does not support speed 5000. You can't simply add it, because it clashes with register value for speed 2500 on previous switches (Peridot, Topaz). Amethyst reg val Peridot + Topaz reg val 2500 SPD_1000 | ALT_BIT SPD_10000 | ALT_BIT 5000 SPD_10000 | ALT_BIT not supported 10000 SPD_UNFORCED SPD_UNFORCED When I sent my proposal for Amethyst I somehow did it, and you commented [1]: > This is getting more and more complex. Maybe it is time to refactor it? And I agree :) Marek [1] https://www.spinics.net/lists/netdev/msg678090.html
On Fri, Nov 20, 2020 at 02:43:11AM +0100, Marek Behun wrote: > Hi Andrew, > > On Fri, 20 Nov 2020 02:29:06 +0100 > Andrew Lunn <andrew@lunn.ch> wrote: > > > > + if (speed >= 2500 && port > 0 && port < 9) > > > + return -EOPNOTSUPP; > > > > Maybe i'm missing something, but it looks like at this point you can > > call > > > > return mv88e6xxx_port_set_speed_duplex(chip, port, speed, true, true, duplex); > > He can't. That function does not support speed 5000. You can't simply > add it, because it clashes with register value for speed 2500 on > previous switches (Peridot, Topaz). > > Amethyst reg val Peridot + Topaz reg val > 2500 SPD_1000 | ALT_BIT SPD_10000 | ALT_BIT > 5000 SPD_10000 | ALT_BIT not supported > 10000 SPD_UNFORCED SPD_UNFORCED Hi Marek O.K, as i said, i might be missing something :-) I think a comment would be nice, pointing this out. Otherwise somebody might try refactoring it, and make the same mistake! Andrew
Updated patchset after incorporating feedback.
Pavana Sharma (4):
dt-bindings: net: Add 5GBASER phy interface mode
net: phy: Add 5GBASER interface mode
net: dsa: mv88e6xxx: Change serdes lane parameter type from u8 type
to int
net: dsa: mv88e6xxx: Add support for mv88e6393x family of Marvell
.../bindings/net/ethernet-controller.yaml | 2 +
drivers/net/dsa/mv88e6xxx/chip.c | 164 +++++++++-
drivers/net/dsa/mv88e6xxx/chip.h | 20 +-
drivers/net/dsa/mv88e6xxx/global1.h | 2 +
drivers/net/dsa/mv88e6xxx/global2.h | 8 +
drivers/net/dsa/mv88e6xxx/port.c | 238 +++++++++++++-
drivers/net/dsa/mv88e6xxx/port.h | 43 ++-
drivers/net/dsa/mv88e6xxx/serdes.c | 299 +++++++++++++++---
drivers/net/dsa/mv88e6xxx/serdes.h | 93 ++++--
include/linux/phy.h | 5 +
10 files changed, 783 insertions(+), 91 deletions(-)
On Wed, 9 Dec 2020 15:02:54 +1000 Pavana Sharma wrote:
> Updated patchset after incorporating feedback.
This set does not apply to net-next. Please rebase.
On Wed, Dec 09, 2020 at 03:04:23PM +1000, Pavana Sharma wrote: > Add 5GBASE-R phy interface mode > > Signed-off-by: Pavana Sharma <pavana.sharma@digi.com> > --- > include/linux/phy.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 56563e5e0dc7..8151e6ecf1b9 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -106,6 +106,7 @@ extern const int phy_10gbit_features_array[1]; > * @PHY_INTERFACE_MODE_TRGMII: Turbo RGMII > * @PHY_INTERFACE_MODE_1000BASEX: 1000 BaseX > * @PHY_INTERFACE_MODE_2500BASEX: 2500 BaseX > + * @PHY_INTERFACE_MODE_5GBASER: 5G BaseR > * @PHY_INTERFACE_MODE_RXAUI: Reduced XAUI > * @PHY_INTERFACE_MODE_XAUI: 10 Gigabit Attachment Unit Interface > * @PHY_INTERFACE_MODE_10GBASER: 10G BaseR > @@ -137,6 +138,8 @@ typedef enum { > PHY_INTERFACE_MODE_TRGMII, > PHY_INTERFACE_MODE_1000BASEX, > PHY_INTERFACE_MODE_2500BASEX, > + /* 5GBASE-R mode */ > + PHY_INTERFACE_MODE_5GBASER, > PHY_INTERFACE_MODE_RXAUI, > PHY_INTERFACE_MODE_XAUI, For v10 i said: > Again, what is the value of the comment? 10GBASE-R has a comment > because it is different from the rest, XFI and SFI caused a of > discussion, and it was used wrong. But there does not seem to be > anything special for 5GBASE-R. Please don't ignore comments. If you don't understand, please ask. If you think the comments are wrong, please explain why, so we can discuss it. Andrew
>On Wed, Dec 09, 2020 at 03:05:17PM +1000, Pavana Sharma wrote: > Returning 0 is no more an error case with MV88E6393 family > which has serdes lane numbers 0, 9 or 10. > So with this change .serdes_get_lane will return lane number > or -errno (-ENODEV or -EOPNOTSUPP). > > Signed-off-by: Pavana Sharma <pavana.sharma@digi.com> I see here you did actually act on my comment. Thanks. But i also said: > Other than that: > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Please add such tags to new versions of the patches. It then makes it easier for everybody to know the review state of the patches, which have been reviewed and deemed O.K, and which need more review. Thanks Andrew
> +/* Support 10, 100, 200, 1000, 2500, 5000, 10000 Mbps (e.g. 88E6393X) > + * This function adds new speed 5000 supported by Amethyst family. > + * Function mv88e6xxx_port_set_speed_duplex() can't be used as the register > + * values for speeds 2500 & 5000 conflict. > + */ Thanks, that should stop my or somebody else trying to wrong combine them. > +/* Offset 0x10 & 0x11: EPC */ > + > +static int mv88e6393x_epc_wait_ready(struct mv88e6xxx_chip *chip, int port) > +{ > + int bit = __bf_shf(MV88E6393X_PORT_EPC_CMD_BUSY); > + > + return mv88e6xxx_port_wait_bit(chip, port, MV88E6393X_PORT_EPC_CMD, bit, 0); > +} To follow the naming convention, this should really be called mv88e6393x_port_epc_wait_ready > +int mv88e6393x_serdes_irq_enable(struct mv88e6xxx_chip *chip, int port, > + int lane, bool enable) It can be hard to tell in a diff, but the indentation looks wrong here. 'int lane' should line up with 'struct'. > +{ > + u8 cmode = chip->ports[port].cmode; > + int err = 0; > + > + switch (cmode) { > + case MV88E6XXX_PORT_STS_CMODE_SGMII: > + case MV88E6XXX_PORT_STS_CMODE_1000BASEX: > + case MV88E6XXX_PORT_STS_CMODE_2500BASEX: > + case MV88E6XXX_PORT_STS_CMODE_5GBASER: > + case MV88E6XXX_PORT_STS_CMODE_10GBASER: > + err = mv88e6390_serdes_irq_enable_sgmii(chip, lane, enable); > + } > + > + return err; > +} > + > +irqreturn_t mv88e6393x_serdes_irq_status(struct mv88e6xxx_chip *chip, int port, > + int lane) Maybe here as well? > +int mv88e6393x_setup_errata(struct mv88e6xxx_chip *chip) It should have _serdes_ in the name to follow the naming convention. Andrew