| Message ID | 20190816150834.26939-4-marek.behun@nic.cz |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | mv88e6xxx: setting 2500base-x mode for CPU/DSA port in dts | expand |
On Fri, 16 Aug 2019 17:08:34 +0200, Marek Behún <marek.behun@nic.cz> wrote: > @@ -2151,16 +2151,6 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) > if (err) > return err; > > - /* Enable the SERDES interface for DSA and CPU ports. Normal > - * ports SERDES are enabled when the port is enabled, thus > - * saving a bit of power. > - */ > - if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) { > - err = mv88e6xxx_serdes_power(chip, port, true); > - if (err) > - return err; > - } > - > /* Port Control 2: don't force a good FCS, set the maximum frame size to > * 10240 bytes, disable 802.1q tags checking, don't discard tagged or > * untagged frames on this port, do a destination address lookup on all > @@ -2557,6 +2547,48 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) > return err; > } > > +static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port) > +{ > + struct mv88e6xxx_chip *chip = ds->priv; > + int err; > + > + /* Enable the SERDES interface for DSA and CPU ports. Normal > + * ports SERDES are enabled when the port is enabled, thus > + * saving a bit of power. > + */ > + if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) { > + mv88e6xxx_reg_lock(chip); > + > + err = mv88e6xxx_serdes_power(chip, port, true); > + > + if (!err && chip->info->ops->serdes_irq_setup) > + err = chip->info->ops->serdes_irq_setup(chip, port); > + > + mv88e6xxx_reg_unlock(chip); > + > + return err; > + } > + > + return 0; > +} > + > +static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port) > +{ > + struct mv88e6xxx_chip *chip = ds->priv; > + > + if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) { > + mv88e6xxx_reg_lock(chip); > + > + if (chip->info->ops->serdes_irq_free) > + chip->info->ops->serdes_irq_free(chip, port); > + > + if (mv88e6xxx_serdes_power(chip, port, false)) > + dev_err(chip->dev, "failed to power off SERDES\n"); > + > + mv88e6xxx_reg_unlock(chip); > + } > +} So now we have mv88e6xxx_setup_port() and mv88e6xxx_port_setup(), which both setup a port, differently, at different time. This is definitely error prone.
On Fri, 16 Aug 2019 12:25:52 -0400 Vivien Didelot <vivien.didelot@gmail.com> wrote: > So now we have mv88e6xxx_setup_port() and mv88e6xxx_port_setup(), which both > setup a port, differently, at different time. This is definitely error prone. Hmm. I don't know how much of mv88e6xxx_setup_port() could be moved to this new port_setup(), since there are other setup functions called in mv88e6xxx_setup() that can possibly depend on what was done by mv88e6xxx_setup_port(). Maybe the new DSA operations should be called .after_setup() and .before_teardown(), and be called just once for the whole switch, not for each port?
Hi Marek, On Fri, 16 Aug 2019 19:05:20 +0200, Marek Behun <marek.behun@nic.cz> wrote: > On Fri, 16 Aug 2019 12:25:52 -0400 > Vivien Didelot <vivien.didelot@gmail.com> wrote: > > > So now we have mv88e6xxx_setup_port() and mv88e6xxx_port_setup(), which both > > setup a port, differently, at different time. This is definitely error prone. > > Hmm. I don't know how much of mv88e6xxx_setup_port() could be moved to > this new port_setup(), since there are other setup functions called in > mv88e6xxx_setup() that can possibly depend on what was done by > mv88e6xxx_setup_port(). > > Maybe the new DSA operations should be called .after_setup() > and .before_teardown(), and be called just once for the whole switch, > not for each port? I think the DSA switch port_setup/port_teardown operations are fine, but the idea would be that the drivers must no longer setup their ports directly in their .setup function. So for mv88e6xxx precisely, we should rename mv88e6xxx_setup_port to mv88e6xxx_port_setup, and move all the port-related code from mv88e6xxx_setup into mv88e6xxx_port_setup. Also, the DSA stack must call ds->ops->port_setup() for all ports, regardless their type, i.e. even if their are unused. As a reminder, *setup/*teardown are more like typical probe/remove callbacks found in drivers, while enable/disable are a runtime thing, switching a port on and off (think ifconfig up/down). Thanks, Vivien
Hi Vivien, On Fri, 16 Aug 2019 19:05:37 -0400 Vivien Didelot <vivien.didelot@gmail.com> wrote: > I think the DSA switch port_setup/port_teardown operations are fine, but the > idea would be that the drivers must no longer setup their ports directly > in their .setup function. So for mv88e6xxx precisely, we should rename > mv88e6xxx_setup_port to mv88e6xxx_port_setup, and move all the port-related > code from mv88e6xxx_setup into mv88e6xxx_port_setup. I looked into the driver, and found out that mv88e6xxx_setup calls many other setup functions after the calls to mv88e6xxx_setup_port for each port: 1. setup errata 2. cache cmode 3. for each port setup_port 4. irl setup 5. mac setup 6. phy setup 7. vtu setup 8. pvt setup 9. atu setup 10. broadcast setuo 11. pot setup 12. rmu setup 13. rsvd2cpu setup 14. trunk setup 15. devmap setup 16. pri setup 17. ptp setup 18. hwtstamp setup 19. stats setup The problem is that some of these steps (after step 3) may depend on some of the work done by step 3. Some of these functions iterate again over the port array (mv88e6xxx_hwtstamp_setup, for example). We cannot simply move step 3 to be called from DSA after mv88e6xxx_setup. I now do not know exactly what to do about the error prone naming of setup_port vs port_setup. One way would be to rename the mv88e6xxx_setup_port function to mv88e6xxx_setup_port_regs, or mv88e6xxx_port_pre_setup, or something like that. Would the names mv88e6xxx_port_setup and mv88e6xxx_setup_port_regs still be very confusing and error prone? I think maybe yes... Other solution would be to, instead of the .port_setup() and .port_teardown() DSA ops, create the .after_setup() and .before_teardown() ops I mentioned in the previous mail. And yet another (in my opinion very improper) solution could be that the .setup() method could call dsa_port_setup() from within itself, to ensure that the needed structres exist. Please let me know what you think about this. The first solution to me currently seems as the easiest. Marek
On Sat, 17 Aug 2019 20:03:42 +0200 Marek Behun <marek.behun@nic.cz> wrote: > One way would be to rename the mv88e6xxx_setup_port function to > mv88e6xxx_setup_port_regs, or mv88e6xxx_port_pre_setup, or something > like that. Would the names mv88e6xxx_port_setup and > mv88e6xxx_setup_port_regs still be very confusing and error prone? > I think maybe yes... > > Other solution would be to, instead of the .port_setup() > and .port_teardown() DSA ops, create the .after_setup() > and .before_teardown() ops I mentioned in the previous mail. > > And yet another (in my opinion very improper) solution could be that > the .setup() method could call dsa_port_setup() from within itself, to > ensure that the needed structres exist. I thought of another solution, one that does not need new DSA operations. What if dsa_port_enable was called for CPU/DSA ports after in dsa_port_setup_switches, after all ports are setup, and dsa_port_disable called for CPU/DSA ports in dsa_port_teardown_switches? This seems to me as cleaner solution. Marek
Hi Marek, On Sat, 17 Aug 2019 20:15:52 +0200, Marek Behun <marek.behun@nic.cz> wrote: > On Sat, 17 Aug 2019 20:03:42 +0200 > Marek Behun <marek.behun@nic.cz> wrote: > > > One way would be to rename the mv88e6xxx_setup_port function to > > mv88e6xxx_setup_port_regs, or mv88e6xxx_port_pre_setup, or something > > like that. Would the names mv88e6xxx_port_setup and > > mv88e6xxx_setup_port_regs still be very confusing and error prone? > > I think maybe yes... > > > > Other solution would be to, instead of the .port_setup() > > and .port_teardown() DSA ops, create the .after_setup() > > and .before_teardown() ops I mentioned in the previous mail. > > > > And yet another (in my opinion very improper) solution could be that > > the .setup() method could call dsa_port_setup() from within itself, to > > ensure that the needed structres exist. > > I thought of another solution, one that does not need new DSA > operations. What if dsa_port_enable was called for CPU/DSA ports after > in dsa_port_setup_switches, after all ports are setup, and > dsa_port_disable called for CPU/DSA ports in dsa_port_teardown_switches? > > This seems to me as cleaner solution. > > Marek Yes DSA needs to initialize a switch before its ports, but the driver may need the opposite. Splitting the switch and ports setup and moving it up to the DSA stack is a separate topic in fact that I'll handle soon. I guess you meant dsa_tree_setup_switches instead of dsa_port_setup_switches. Let's call dsa_port_enable/dsa_port_disable for the CPU and DSA ports as well. Thanks, Vivien
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 9b3ad22a5b98..23d3e39d2b9c 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -2151,16 +2151,6 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) if (err) return err; - /* Enable the SERDES interface for DSA and CPU ports. Normal - * ports SERDES are enabled when the port is enabled, thus - * saving a bit of power. - */ - if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) { - err = mv88e6xxx_serdes_power(chip, port, true); - if (err) - return err; - } - /* Port Control 2: don't force a good FCS, set the maximum frame size to * 10240 bytes, disable 802.1q tags checking, don't discard tagged or * untagged frames on this port, do a destination address lookup on all @@ -2557,6 +2547,48 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) return err; } +static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port) +{ + struct mv88e6xxx_chip *chip = ds->priv; + int err; + + /* Enable the SERDES interface for DSA and CPU ports. Normal + * ports SERDES are enabled when the port is enabled, thus + * saving a bit of power. + */ + if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) { + mv88e6xxx_reg_lock(chip); + + err = mv88e6xxx_serdes_power(chip, port, true); + + if (!err && chip->info->ops->serdes_irq_setup) + err = chip->info->ops->serdes_irq_setup(chip, port); + + mv88e6xxx_reg_unlock(chip); + + return err; + } + + return 0; +} + +static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port) +{ + struct mv88e6xxx_chip *chip = ds->priv; + + if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) { + mv88e6xxx_reg_lock(chip); + + if (chip->info->ops->serdes_irq_free) + chip->info->ops->serdes_irq_free(chip, port); + + if (mv88e6xxx_serdes_power(chip, port, false)) + dev_err(chip->dev, "failed to power off SERDES\n"); + + mv88e6xxx_reg_unlock(chip); + } +} + static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg) { struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv; @@ -4692,6 +4724,8 @@ static int mv88e6xxx_port_egress_floods(struct dsa_switch *ds, int port, static const struct dsa_switch_ops mv88e6xxx_switch_ops = { .get_tag_protocol = mv88e6xxx_get_tag_protocol, .setup = mv88e6xxx_setup, + .port_setup = mv88e6xxx_port_setup, + .port_teardown = mv88e6xxx_port_teardown, .phylink_validate = mv88e6xxx_validate, .phylink_mac_link_state = mv88e6xxx_link_state, .phylink_mac_config = mv88e6xxx_mac_config,
When CPU/DSA port is put into for example into 2500base-x mode, the SERDES irq has to be enabled so that port's MAC is configured properly after autonegotiation. When SERDES irq is being enabled, the port's phylink structure already has to exist. Otherwise if the IRQ fires immediately, the IRQ routine's access to the nonexistent phylink structure results in an exception. We therefore enable SERDES irqs for CPU/DSA ports in the .port_setup() method, which is called by DSA from dsa_setup_port after the port is registered and phylink structures exist. We also move SERDES powering on for CPU/DSA ports to this method. We also free the IRQ and power off SERDESes for these ports in the .port_teardown() method. Signed-off-by: Marek Behún <marek.behun@nic.cz> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Vladimir Oltean <olteanv@gmail.com> Cc: Vivien Didelot <vivien.didelot@gmail.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 54 ++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 10 deletions(-)