[2/2] net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()
Commit Message
From commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities
for C22 and C45") onwards, mdiobus_scan_bus_c45() is being called on buses
with MDIOBUS_NO_CAP. On a Turris Omnia (Armada 385, 88E6176 switch), this
causes a significant increase of boot time, from 1.6 seconds, to 6.3
seconds. The boot time stated here is until start of /init.
Further testing revealed that the C45 scan is indeed expensive (around
2.7 seconds, due to a huge number of bus transactions), and called twice.
It was suggested, to call mv88e6xxx_mdios_register() at the beginning of
mv88e6xxx_setup(), and mv88e6xxx_mdios_unregister() at the end of
mv88e6xxx_teardown(). This is accomplished by this patch.
Testing on the Turris Omnia revealed, that this improves the situation.
Now mdiobus_scan_bus_c45() is called only once, ending up in a boot time
of 4.3 seconds.
Link: https://lore.kernel.org/lkml/449bde236c08d5ab5e54abd73b645d8b29955894.camel@gmail.com/
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Klaus Kudielka <klaus.kudielka@gmail.com>
Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
Comments
On Sat, Mar 11, 2023 at 10:41:41AM +0100, Klaus Kudielka wrote:
> >From commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities
> for C22 and C45") onwards, mdiobus_scan_bus_c45() is being called on buses
> with MDIOBUS_NO_CAP. On a Turris Omnia (Armada 385, 88E6176 switch), this
> causes a significant increase of boot time, from 1.6 seconds, to 6.3
> seconds. The boot time stated here is until start of /init.
>
> Further testing revealed that the C45 scan is indeed expensive (around
> 2.7 seconds, due to a huge number of bus transactions), and called twice.
>
> It was suggested, to call mv88e6xxx_mdios_register() at the beginning of
> mv88e6xxx_setup(), and mv88e6xxx_mdios_unregister() at the end of
> mv88e6xxx_teardown(). This is accomplished by this patch.
>
> Testing on the Turris Omnia revealed, that this improves the situation.
> Now mdiobus_scan_bus_c45() is called only once, ending up in a boot time
> of 4.3 seconds.
For those who are interested, here is a bit of background on why this
change reduces the number of bus scans.
The MAC driver probes, which i think in this case is mvneta. Part way
through its probe, it registers its MDIO bus. That triggers a scan of
its bus, and the switch is found. The mv88e6xxx driver is then loaded
and its probe function called. towards the end of the mv88e6xxxx probe
function, it registers its MDIO bus. That causes a scan of the
switches MDIO bus. Which is slow. After the scan completes, the
mv88e6xxx probe continues, and registers the switch with DSA core. The
core then parses the DT binding for the switch and looks for the
master ethernet interface. That is the interface which mvneta
provides. But mvneta is still only part way through its probe. It has
not yet registered its interface with the netdev core. So the DSA core
fails to find it and return EPROBE_DEFER. This causes the mv88e6xxx
driver to unwind its probe. The mvneat then gets a chance to finish
its probe and register its netdev. Some timer later, the driver core
runs the probes again for those drivers which returned EPROBE_DEFER,
mv88e6xxx registers its MDIO bus again, another scan is performed, the
switch is registered with the code, and this time the master device is
available, so things continue. The DSA core then calls the drivers
.setup() callback to get the switch into a usable state.
I think what remains in the probe function is cheap, so it can
probable stay there and be done twice. But it might be worth putting
in a few printks to get some time stamps and see if anything is
expensive.
> static int mv88e6xxx_setup(struct dsa_switch *ds)
> @@ -3889,6 +3892,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
> int err;
> int i;
>
> + err = mv88e6xxx_mdios_register(chip);
> + if (err)
> + return err;
> +
> chip->ds = ds;
> ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
Other calls in mv88e6xxx_setup() can fail, so you need to extend the
cleanup to remove the mdio bus on failure.
Andrew
On Sat, Mar 11, 2023 at 10:41:41AM +0100, Klaus Kudielka wrote:
> From commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities
> for C22 and C45") onwards, mdiobus_scan_bus_c45() is being called on buses
> with MDIOBUS_NO_CAP. On a Turris Omnia (Armada 385, 88E6176 switch), this
> causes a significant increase of boot time, from 1.6 seconds, to 6.3
> seconds. The boot time stated here is until start of /init.
>
> Further testing revealed that the C45 scan is indeed expensive (around
> 2.7 seconds, due to a huge number of bus transactions), and called twice.
>
> It was suggested, to call mv88e6xxx_mdios_register() at the beginning of
> mv88e6xxx_setup(), and mv88e6xxx_mdios_unregister() at the end of
> mv88e6xxx_teardown(). This is accomplished by this patch.
>
> Testing on the Turris Omnia revealed, that this improves the situation.
> Now mdiobus_scan_bus_c45() is called only once, ending up in a boot time
> of 4.3 seconds.
>
> Link: https://lore.kernel.org/lkml/449bde236c08d5ab5e54abd73b645d8b29955894.camel@gmail.com/
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Klaus Kudielka <klaus.kudielka@gmail.com>
> Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com>
> ---
No objection to the change. However you might want to bundle it up with
another patch for the phy_mask restriction, and resend the series using
Andrew's indications.
On Sat, Mar 11, 2023 at 04:19:43PM +0100, Andrew Lunn wrote:
> > static int mv88e6xxx_setup(struct dsa_switch *ds)
> > @@ -3889,6 +3892,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
> > int err;
> > int i;
> >
> > + err = mv88e6xxx_mdios_register(chip);
> > + if (err)
> > + return err;
> > +
> > chip->ds = ds;
> > ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
>
> Other calls in mv88e6xxx_setup() can fail, so you need to extend the
> cleanup to remove the mdio bus on failure.
FWIW, here is a snippet of how mv88e6xxx_setup() and mv88e6xxx_teardown()
should look like, with error handling taken into consideration (but I
was lazy and just added forward declarations, something which Klaus
handled better with the movement preparatory patch):
https://lore.kernel.org/lkml/20230210210804.vdyfrog5nq6hrxi5@skbuf/
@@ -3840,9 +3840,9 @@ static void mv88e6xxx_mdios_unregister(struct mv88e6xxx_chip *chip)
}
}
-static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
- struct device_node *np)
+static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip)
{
+ struct device_node *np = chip->dev->of_node;
struct device_node *child;
int err;
@@ -3877,9 +3877,12 @@ static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
static void mv88e6xxx_teardown(struct dsa_switch *ds)
{
+ struct mv88e6xxx_chip *chip = ds->priv;
+
mv88e6xxx_teardown_devlink_params(ds);
dsa_devlink_resources_unregister(ds);
mv88e6xxx_teardown_devlink_regions_global(ds);
+ mv88e6xxx_mdios_unregister(chip);
}
static int mv88e6xxx_setup(struct dsa_switch *ds)
@@ -3889,6 +3892,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
int err;
int i;
+ err = mv88e6xxx_mdios_register(chip);
+ if (err)
+ return err;
+
chip->ds = ds;
ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
@@ -7220,18 +7227,12 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
if (err)
goto out_g1_atu_prob_irq;
- err = mv88e6xxx_mdios_register(chip, np);
- if (err)
- goto out_g1_vtu_prob_irq;
-
err = mv88e6xxx_register_switch(chip);
if (err)
- goto out_mdio;
+ goto out_g1_vtu_prob_irq;
return 0;
-out_mdio:
- mv88e6xxx_mdios_unregister(chip);
out_g1_vtu_prob_irq:
mv88e6xxx_g1_vtu_prob_irq_free(chip);
out_g1_atu_prob_irq:
@@ -7268,7 +7269,6 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
mv88e6xxx_phy_destroy(chip);
mv88e6xxx_unregister_switch(chip);
- mv88e6xxx_mdios_unregister(chip);
mv88e6xxx_g1_vtu_prob_irq_free(chip);
mv88e6xxx_g1_atu_prob_irq_free(chip);