| Message ID | 20190826122109.20660-7-marek.behun@nic.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Peridot/Topaz SERDES changes | expand |
> +static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, > + phy_interface_t mode, bool allow_over_2500, > + bool make_cmode_writable) I don't like these two parameters. The caller of this function can do the check for allow_over_2500 and error out before calling this. Is make_cmode_writable something that could be done once at probe and then forgotten about? Or is it needed before every write? At least move it into the specific port_set_cmode() that requires it. Thanks Andrew
On Mon, 26 Aug 2019 17:38:30 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > +static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, > > + phy_interface_t mode, bool allow_over_2500, > > + bool make_cmode_writable) > > I don't like these two parameters. The caller of this function can do > the check for allow_over_2500 and error out before calling this. > > Is make_cmode_writable something that could be done once at probe and > then forgotten about? Or is it needed before every write? At least > move it into the specific port_set_cmode() that requires it. It can be done once at probe. At first I thought about doing this in setup_errata, but this is not an erratum. So shall I create a new method for this in chip operations structure? Something like port_additional_setup() ? Marek
On Mon, 26 Aug 2019 17:38:30 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > +static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, > > + phy_interface_t mode, bool allow_over_2500, > > + bool make_cmode_writable) > > I don't like these two parameters. The caller of this function can do > the check for allow_over_2500 and error out before calling this. > > Is make_cmode_writable something that could be done once at probe and > then forgotten about? Or is it needed before every write? At least > move it into the specific port_set_cmode() that requires it. > > Thanks > Andrew Btw those two additional parameters were modeled as in static int mv88e6xxx_port_set_speed(struct mv88e6xxx_chip *chip, int port, int speed, bool alt_bit, bool force_bit);
On Mon, 26 Aug 2019 19:27:17 +0200, Marek Behun <marek.behun@nic.cz> wrote: > On Mon, 26 Aug 2019 17:38:30 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > > > > +static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, > > > + phy_interface_t mode, bool allow_over_2500, > > > + bool make_cmode_writable) > > > > I don't like these two parameters. The caller of this function can do > > the check for allow_over_2500 and error out before calling this. > > > > Is make_cmode_writable something that could be done once at probe and > > then forgotten about? Or is it needed before every write? At least > > move it into the specific port_set_cmode() that requires it. > > It can be done once at probe. At first I thought about doing this in > setup_errata, but this is not an erratum. So shall I create a new > method for this in chip operations structure? Something like > port_additional_setup() ? No. Those "setup" or "config" functions are likely to do everything and become a mess, thus unmaintainable. Operations must be specific.
On Mon, 26 Aug 2019 19:31:25 +0200, Marek Behun <marek.behun@nic.cz> wrote: > On Mon, 26 Aug 2019 17:38:30 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > > > > +static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, > > > + phy_interface_t mode, bool allow_over_2500, > > > + bool make_cmode_writable) > > > > I don't like these two parameters. The caller of this function can do > > the check for allow_over_2500 and error out before calling this. > > > > Is make_cmode_writable something that could be done once at probe and > > then forgotten about? Or is it needed before every write? At least > > move it into the specific port_set_cmode() that requires it. > > > > Thanks > > Andrew > > Btw those two additional parameters were modeled as in > static int mv88e6xxx_port_set_speed(struct mv88e6xxx_chip *chip, > int port, int speed, bool alt_bit, > bool force_bit); "AltSpeed" and "ForceSpeed" are two bits found in the MAC Control Register of certain switch models, which can be set by this private helper. I don't think that is the case for "allow_over_2500" and "make_cmode_writable".
On Mon, 26 Aug 2019 13:44:18 -0400 Vivien Didelot <vivien.didelot@gmail.com> wrote: > > It can be done once at probe. At first I thought about doing this in > > setup_errata, but this is not an erratum. So shall I create a new > > method for this in chip operations structure? Something like > > port_additional_setup() ? > > No. Those "setup" or "config" functions are likely to do everything and > become a mess, thus unmaintainable. Operations must be specific. What about Andrew's complaint, then, abouth the two additional parameters?
From: Andrew Lunn <andrew@lunn.ch> Date: Mon, 26 Aug 2019 17:38:30 +0200 >> +static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, >> + phy_interface_t mode, bool allow_over_2500, >> + bool make_cmode_writable) > > I don't like these two parameters. The caller of this function can do > the check for allow_over_2500 and error out before calling this. > > Is make_cmode_writable something that could be done once at probe and > then forgotten about? Or is it needed before every write? At least > move it into the specific port_set_cmode() that requires it. Please respin this series once these issues are sorted out. Thanks.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 202ccce65b1c..6525075f6bd3 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -2913,6 +2913,7 @@ static const struct mv88e6xxx_ops mv88e6141_ops = { .port_disable_pri_override = mv88e6xxx_port_disable_pri_override, .port_link_state = mv88e6352_port_link_state, .port_get_cmode = mv88e6352_port_get_cmode, + .port_set_cmode = mv88e6341_port_set_cmode, .port_setup_message_port = mv88e6xxx_setup_message_port, .stats_snapshot = mv88e6390_g1_stats_snapshot, .stats_set_histogram = mv88e6095_g1_stats_set_histogram, @@ -2929,6 +2930,8 @@ static const struct mv88e6xxx_ops mv88e6141_ops = { .vtu_loadpurge = mv88e6352_g1_vtu_loadpurge, .serdes_power = mv88e6390_serdes_power, .serdes_get_lane = mv88e6341_serdes_get_lane, + .serdes_irq_setup = mv88e6390_serdes_irq_setup, + .serdes_irq_free = mv88e6390_serdes_irq_free, .gpio_ops = &mv88e6352_gpio_ops, .phylink_validate = mv88e6341_phylink_validate, }; @@ -3608,6 +3611,7 @@ static const struct mv88e6xxx_ops mv88e6341_ops = { .port_disable_pri_override = mv88e6xxx_port_disable_pri_override, .port_link_state = mv88e6352_port_link_state, .port_get_cmode = mv88e6352_port_get_cmode, + .port_set_cmode = mv88e6341_port_set_cmode, .port_setup_message_port = mv88e6xxx_setup_message_port, .stats_snapshot = mv88e6390_g1_stats_snapshot, .stats_set_histogram = mv88e6095_g1_stats_set_histogram, @@ -3624,6 +3628,8 @@ static const struct mv88e6xxx_ops mv88e6341_ops = { .vtu_loadpurge = mv88e6352_g1_vtu_loadpurge, .serdes_power = mv88e6390_serdes_power, .serdes_get_lane = mv88e6341_serdes_get_lane, + .serdes_irq_setup = mv88e6390_serdes_irq_setup, + .serdes_irq_free = mv88e6390_serdes_irq_free, .gpio_ops = &mv88e6352_gpio_ops, .avb_ops = &mv88e6390_avb_ops, .ptp_ops = &mv88e6352_ptp_ops, diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index 7183c94a92ec..908b95434b4d 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -392,17 +392,37 @@ phy_interface_t mv88e6390x_port_max_speed_mode(int port) return PHY_INTERFACE_MODE_NA; } -int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, - phy_interface_t mode) +static int mv88e6341_port_force_writable_cmode(struct mv88e6xxx_chip *chip, + int port) +{ + int err, addr; + u16 reg, bits; + + addr = chip->info->port_base_addr + port; + + err = mv88e6xxx_port_hidden_read(chip, 0x7, addr, 0, ®); + if (err) + return err; + + bits = MV88E6341_PORT_RESERVED_1A_FORCE_CMODE | + MV88E6341_PORT_RESERVED_1A_SGMII_AN; + + if ((reg & bits) == bits) + return 0; + + reg |= bits; + return mv88e6xxx_port_hidden_write(chip, 0x7, addr, 0, reg); +} + +static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port, + phy_interface_t mode, bool allow_over_2500, + bool make_cmode_writable) { s8 lane; u16 cmode; u16 reg; int err; - if (port != 9 && port != 10) - return -EOPNOTSUPP; - /* Default to a slow mode, so freeing up SERDES interfaces for * other ports which might use them for SFPs. */ @@ -421,9 +441,13 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, break; case PHY_INTERFACE_MODE_XGMII: case PHY_INTERFACE_MODE_XAUI: + if (!allow_over_2500) + return -EINVAL; cmode = MV88E6XXX_PORT_STS_CMODE_XAUI; break; case PHY_INTERFACE_MODE_RXAUI: + if (!allow_over_2500) + return -EINVAL; cmode = MV88E6XXX_PORT_STS_CMODE_RXAUI; break; default: @@ -457,6 +481,12 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, if (err) return err; + if (make_cmode_writable) { + err = mv88e6341_port_force_writable_cmode(chip, port); + if (err) + return err; + } + reg &= ~MV88E6XXX_PORT_STS_CMODE_MASK; reg |= cmode; @@ -484,21 +514,37 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, return 0; } +int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, + phy_interface_t mode) +{ + if (port != 9 && port != 10) + return -EOPNOTSUPP; + + return mv88e6xxx_port_set_cmode(chip, port, mode, true, false); +} + int mv88e6390_port_set_cmode(struct mv88e6xxx_chip *chip, int port, phy_interface_t mode) { - switch (mode) { - case PHY_INTERFACE_MODE_NA: + if (port != 9 && port != 10) + return -EOPNOTSUPP; + + if (mode == PHY_INTERFACE_MODE_NA) + return 0; + + return mv88e6xxx_port_set_cmode(chip, port, mode, false, false); +} + +int mv88e6341_port_set_cmode(struct mv88e6xxx_chip *chip, int port, + phy_interface_t mode) +{ + if (port != 5) + return -EOPNOTSUPP; + + if (mode == PHY_INTERFACE_MODE_NA) return 0; - case PHY_INTERFACE_MODE_XGMII: - case PHY_INTERFACE_MODE_XAUI: - case PHY_INTERFACE_MODE_RXAUI: - return -EINVAL; - default: - break; - } - return mv88e6390x_port_set_cmode(chip, port, mode); + return mv88e6xxx_port_set_cmode(chip, port, mode, false, true); } int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode) diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h index 6d7a067da0f5..d4e9bea6e82f 100644 --- a/drivers/net/dsa/mv88e6xxx/port.h +++ b/drivers/net/dsa/mv88e6xxx/port.h @@ -269,6 +269,8 @@ #define MV88E6XXX_PORT_RESERVED_1A_BLOCK_SHIFT 10 #define MV88E6XXX_PORT_RESERVED_1A_CTRL_PORT 0x04 #define MV88E6XXX_PORT_RESERVED_1A_DATA_PORT 0x05 +#define MV88E6341_PORT_RESERVED_1A_FORCE_CMODE 0x8000 +#define MV88E6341_PORT_RESERVED_1A_SGMII_AN 0x2000 int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip, int port, int reg, u16 *val); @@ -334,6 +336,8 @@ int mv88e6097_port_pause_limit(struct mv88e6xxx_chip *chip, int port, u8 in, u8 out); int mv88e6390_port_pause_limit(struct mv88e6xxx_chip *chip, int port, u8 in, u8 out); +int mv88e6341_port_set_cmode(struct mv88e6xxx_chip *chip, int port, + phy_interface_t mode); int mv88e6390_port_set_cmode(struct mv88e6xxx_chip *chip, int port, phy_interface_t mode); int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
Currently we support SERDES on the Topaz family in a limited way: no IRQs and the cmode is not writable, thus the mode is determined by strapping pins. Marvell's examples though show how to make cmode writable on port 5 and support SGMII autonegotiation. It is done by writing hidden registers, for which we already have code. This patch adds support for making the cmode for the SERDES port writable on the Topaz family, and enables cmode setting and SERDES IRQs. Tested on Turris Mox. Signed-off-by: Marek Behún <marek.behun@nic.cz> --- drivers/net/dsa/mv88e6xxx/chip.c | 6 +++ drivers/net/dsa/mv88e6xxx/port.c | 76 +++++++++++++++++++++++++------- drivers/net/dsa/mv88e6xxx/port.h | 4 ++ 3 files changed, 71 insertions(+), 15 deletions(-)