Starting with commit 1a136ca2e089 ("net: mdio: scan bus based on bus
capabilities for C22 and C45"), 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.
Two things were suggested:
(1) to move the expensive call of mv88e6xxx_mdios_register() from
mv88e6xxx_probe() to mv88e6xxx_setup().
(2) to mask apparently non-existing phys during probing.
Before that:
Patch #1 prepares the driver to handle the movement of
mv88e6xxx_mdios_register() to mv88e6xxx_setup() for cross-chip DSA trees.
Patch #2 is preparatory code movement, without functional change.
With those changes, boot time on the Turris Omnia is back to normal.
Link: https://lore.kernel.org/lkml/[email protected]/
v2:
Add cover letter
Extend the cleanup in mv88e6xxx_setup() to remove the mdio bus on failure
Add patch "mask apparently non-existing phys during probing"
v3:
Add patch "don't dispose of Global2 IRQ mappings from mdiobus code"
v4:
Resubmit, this time hopefully with all subject prefixes correct
Klaus Kudielka (3):
net: dsa: mv88e6xxx: re-order functions
net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()
net: dsa: mv88e6xxx: mask apparently non-existing phys during probing
Vladimir Oltean (1):
net: dsa: mv88e6xxx: don't dispose of Global2 IRQ mappings from
mdiobus code
drivers/net/dsa/mv88e6xxx/chip.c | 381 ++++++++++++++--------------
drivers/net/dsa/mv88e6xxx/global2.c | 20 +-
2 files changed, 196 insertions(+), 205 deletions(-)
--
2.39.2
Call the rather expensive mv88e6xxx_mdios_register() at the beginning of
mv88e6xxx_setup(). This avoids the double call via mv88e6xxx_probe()
during boot.
For symmetry, call mv88e6xxx_mdios_unregister() at the end of
mv88e6xxx_teardown().
Link: https://lore.kernel.org/lkml/[email protected]/
Suggested-by: Andrew Lunn <[email protected]>
Signed-off-by: Klaus Kudielka <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
Notes:
v2: Extend the cleanup in mv88e6xxx_setup() to remove the mdio bus on failure
drivers/net/dsa/mv88e6xxx/chip.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 496015baac..29b0f3bb1c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -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);
@@ -4015,7 +4022,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
mv88e6xxx_reg_unlock(chip);
if (err)
- return err;
+ goto out_mdios;
/* Have to be called without holding the register lock, since
* they take the devlink lock, and we later take the locks in
@@ -4024,7 +4031,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
*/
err = mv88e6xxx_setup_devlink_resources(ds);
if (err)
- return err;
+ goto out_mdios;
err = mv88e6xxx_setup_devlink_params(ds);
if (err)
@@ -4040,6 +4047,8 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
mv88e6xxx_teardown_devlink_params(ds);
out_resources:
dsa_devlink_resources_unregister(ds);
+out_mdios:
+ mv88e6xxx_mdios_unregister(chip);
return err;
}
@@ -7220,18 +7229,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 +7271,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);
--
2.39.2
Move mv88e6xxx_setup() below mv88e6xxx_mdios_register(), so that we are
able to call the latter one from here. Do the same thing for the
inverse functions.
Signed-off-by: Klaus Kudielka <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/net/dsa/mv88e6xxx/chip.c | 358 +++++++++++++++----------------
1 file changed, 179 insertions(+), 179 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 0a5d6c7bb1..496015baac 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3672,185 +3672,6 @@ static int mv88e6390_setup_errata(struct mv88e6xxx_chip *chip)
return mv88e6xxx_software_reset(chip);
}
-static void mv88e6xxx_teardown(struct dsa_switch *ds)
-{
- mv88e6xxx_teardown_devlink_params(ds);
- dsa_devlink_resources_unregister(ds);
- mv88e6xxx_teardown_devlink_regions_global(ds);
-}
-
-static int mv88e6xxx_setup(struct dsa_switch *ds)
-{
- struct mv88e6xxx_chip *chip = ds->priv;
- u8 cmode;
- int err;
- int i;
-
- chip->ds = ds;
- ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
-
- /* Since virtual bridges are mapped in the PVT, the number we support
- * depends on the physical switch topology. We need to let DSA figure
- * that out and therefore we cannot set this at dsa_register_switch()
- * time.
- */
- if (mv88e6xxx_has_pvt(chip))
- ds->max_num_bridges = MV88E6XXX_MAX_PVT_SWITCHES -
- ds->dst->last_switch - 1;
-
- mv88e6xxx_reg_lock(chip);
-
- if (chip->info->ops->setup_errata) {
- err = chip->info->ops->setup_errata(chip);
- if (err)
- goto unlock;
- }
-
- /* Cache the cmode of each port. */
- for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
- if (chip->info->ops->port_get_cmode) {
- err = chip->info->ops->port_get_cmode(chip, i, &cmode);
- if (err)
- goto unlock;
-
- chip->ports[i].cmode = cmode;
- }
- }
-
- err = mv88e6xxx_vtu_setup(chip);
- if (err)
- goto unlock;
-
- /* Must be called after mv88e6xxx_vtu_setup (which flushes the
- * VTU, thereby also flushing the STU).
- */
- err = mv88e6xxx_stu_setup(chip);
- if (err)
- goto unlock;
-
- /* Setup Switch Port Registers */
- for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
- if (dsa_is_unused_port(ds, i))
- continue;
-
- /* Prevent the use of an invalid port. */
- if (mv88e6xxx_is_invalid_port(chip, i)) {
- dev_err(chip->dev, "port %d is invalid\n", i);
- err = -EINVAL;
- goto unlock;
- }
-
- err = mv88e6xxx_setup_port(chip, i);
- if (err)
- goto unlock;
- }
-
- err = mv88e6xxx_irl_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_mac_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_phy_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_pvt_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_atu_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_broadcast_setup(chip, 0);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_pot_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_rmu_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_rsvd2cpu_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_trunk_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_devmap_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_pri_setup(chip);
- if (err)
- goto unlock;
-
- /* Setup PTP Hardware Clock and timestamping */
- if (chip->info->ptp_support) {
- err = mv88e6xxx_ptp_setup(chip);
- if (err)
- goto unlock;
-
- err = mv88e6xxx_hwtstamp_setup(chip);
- if (err)
- goto unlock;
- }
-
- err = mv88e6xxx_stats_setup(chip);
- if (err)
- goto unlock;
-
-unlock:
- mv88e6xxx_reg_unlock(chip);
-
- if (err)
- return err;
-
- /* Have to be called without holding the register lock, since
- * they take the devlink lock, and we later take the locks in
- * the reverse order when getting/setting parameters or
- * resource occupancy.
- */
- err = mv88e6xxx_setup_devlink_resources(ds);
- if (err)
- return err;
-
- err = mv88e6xxx_setup_devlink_params(ds);
- if (err)
- goto out_resources;
-
- err = mv88e6xxx_setup_devlink_regions_global(ds);
- if (err)
- goto out_params;
-
- return 0;
-
-out_params:
- mv88e6xxx_teardown_devlink_params(ds);
-out_resources:
- dsa_devlink_resources_unregister(ds);
-
- return err;
-}
-
-static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
-{
- return mv88e6xxx_setup_devlink_regions_port(ds, port);
-}
-
-static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
-{
- mv88e6xxx_teardown_devlink_regions_port(ds, port);
-}
-
/* prod_id for switch families which do not have a PHY model number */
static const u16 family_prod_id_table[] = {
[MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
@@ -4054,6 +3875,185 @@ static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
return 0;
}
+static void mv88e6xxx_teardown(struct dsa_switch *ds)
+{
+ mv88e6xxx_teardown_devlink_params(ds);
+ dsa_devlink_resources_unregister(ds);
+ mv88e6xxx_teardown_devlink_regions_global(ds);
+}
+
+static int mv88e6xxx_setup(struct dsa_switch *ds)
+{
+ struct mv88e6xxx_chip *chip = ds->priv;
+ u8 cmode;
+ int err;
+ int i;
+
+ chip->ds = ds;
+ ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
+
+ /* Since virtual bridges are mapped in the PVT, the number we support
+ * depends on the physical switch topology. We need to let DSA figure
+ * that out and therefore we cannot set this at dsa_register_switch()
+ * time.
+ */
+ if (mv88e6xxx_has_pvt(chip))
+ ds->max_num_bridges = MV88E6XXX_MAX_PVT_SWITCHES -
+ ds->dst->last_switch - 1;
+
+ mv88e6xxx_reg_lock(chip);
+
+ if (chip->info->ops->setup_errata) {
+ err = chip->info->ops->setup_errata(chip);
+ if (err)
+ goto unlock;
+ }
+
+ /* Cache the cmode of each port. */
+ for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
+ if (chip->info->ops->port_get_cmode) {
+ err = chip->info->ops->port_get_cmode(chip, i, &cmode);
+ if (err)
+ goto unlock;
+
+ chip->ports[i].cmode = cmode;
+ }
+ }
+
+ err = mv88e6xxx_vtu_setup(chip);
+ if (err)
+ goto unlock;
+
+ /* Must be called after mv88e6xxx_vtu_setup (which flushes the
+ * VTU, thereby also flushing the STU).
+ */
+ err = mv88e6xxx_stu_setup(chip);
+ if (err)
+ goto unlock;
+
+ /* Setup Switch Port Registers */
+ for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
+ if (dsa_is_unused_port(ds, i))
+ continue;
+
+ /* Prevent the use of an invalid port. */
+ if (mv88e6xxx_is_invalid_port(chip, i)) {
+ dev_err(chip->dev, "port %d is invalid\n", i);
+ err = -EINVAL;
+ goto unlock;
+ }
+
+ err = mv88e6xxx_setup_port(chip, i);
+ if (err)
+ goto unlock;
+ }
+
+ err = mv88e6xxx_irl_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_mac_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_phy_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_pvt_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_atu_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_broadcast_setup(chip, 0);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_pot_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_rmu_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_rsvd2cpu_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_trunk_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_devmap_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_pri_setup(chip);
+ if (err)
+ goto unlock;
+
+ /* Setup PTP Hardware Clock and timestamping */
+ if (chip->info->ptp_support) {
+ err = mv88e6xxx_ptp_setup(chip);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_hwtstamp_setup(chip);
+ if (err)
+ goto unlock;
+ }
+
+ err = mv88e6xxx_stats_setup(chip);
+ if (err)
+ goto unlock;
+
+unlock:
+ mv88e6xxx_reg_unlock(chip);
+
+ if (err)
+ return err;
+
+ /* Have to be called without holding the register lock, since
+ * they take the devlink lock, and we later take the locks in
+ * the reverse order when getting/setting parameters or
+ * resource occupancy.
+ */
+ err = mv88e6xxx_setup_devlink_resources(ds);
+ if (err)
+ return err;
+
+ err = mv88e6xxx_setup_devlink_params(ds);
+ if (err)
+ goto out_resources;
+
+ err = mv88e6xxx_setup_devlink_regions_global(ds);
+ if (err)
+ goto out_params;
+
+ return 0;
+
+out_params:
+ mv88e6xxx_teardown_devlink_params(ds);
+out_resources:
+ dsa_devlink_resources_unregister(ds);
+
+ return err;
+}
+
+static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
+{
+ return mv88e6xxx_setup_devlink_regions_port(ds, port);
+}
+
+static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
+{
+ mv88e6xxx_teardown_devlink_regions_port(ds, port);
+}
+
static int mv88e6xxx_get_eeprom_len(struct dsa_switch *ds)
{
struct mv88e6xxx_chip *chip = ds->priv;
--
2.39.2
To avoid excessive mdio bus transactions during probing, mask all phy
addresses that do not exist (there is a 1:1 mapping between switch port
number and phy address).
Suggested-by: Andrew Lunn <[email protected]>
Signed-off-by: Klaus Kudielka <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
Notes:
v2: Patch is new
drivers/net/dsa/mv88e6xxx/chip.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 29b0f3bb1c..c52798d9ce 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3797,6 +3797,7 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip,
bus->read_c45 = mv88e6xxx_mdio_read_c45;
bus->write_c45 = mv88e6xxx_mdio_write_c45;
bus->parent = chip->dev;
+ bus->phy_mask = GENMASK(31, mv88e6xxx_num_ports(chip));
if (!external) {
err = mv88e6xxx_g2_irq_mdio_setup(chip, bus);
--
2.39.2
From: Vladimir Oltean <[email protected]>
irq_find_mapping() does not need irq_dispose_mapping(), only
irq_create_mapping() does.
Calling irq_dispose_mapping() from mv88e6xxx_g2_irq_mdio_free() and from
the error path of mv88e6xxx_g2_irq_mdio_setup() effectively means that
the mdiobus logic (for internal PHY interrupts) is disposing of a
hwirq->virq mapping which it is not responsible of (but instead, the
function pair mv88e6xxx_g2_irq_setup() + mv88e6xxx_g2_irq_free() is).
With the current code structure, this isn't such a huge problem, because
mv88e6xxx_g2_irq_mdio_free() is called relatively close to the real
owner of the IRQ mappings:
mv88e6xxx_remove()
-> mv88e6xxx_unregister_switch()
-> mv88e6xxx_mdios_unregister()
-> mv88e6xxx_g2_irq_mdio_free()
-> mv88e6xxx_g2_irq_free()
and the switch isn't 'live' in any way such that it would be able of
generating interrupts at this point (mv88e6xxx_unregister_switch() has
been called).
However, there is a desire to split mv88e6xxx_mdios_unregister() and
mv88e6xxx_g2_irq_free() such that mv88e6xxx_mdios_unregister() only gets
called from mv88e6xxx_teardown(). This is much more problematic, as can
be seen below.
In a cross-chip scenario (say 3 switches d0032004.mdio-mii:10,
d0032004.mdio-mii:11 and d0032004.mdio-mii:12 which form a single DSA
tree), it is possible to unbind the device driver from a single switch
(say d0032004.mdio-mii:10).
When that happens, mv88e6xxx_remove() will be called for just that one
switch, and this will call mv88e6xxx_unregister_switch() which will tear
down the entire tree (calling mv88e6xxx_teardown() for all 3 switches).
Assuming mv88e6xxx_mdios_unregister() was moved to mv88e6xxx_teardown(),
at this stage, all 3 switches will have called irq_dispose_mapping() on
their mdiobus virqs.
When we bind again the device driver to d0032004.mdio-mii:10,
mv88e6xxx_probe() is called for it, which calls dsa_register_switch().
The DSA tree is now complete again, and mv88e6xxx_setup() is called for
all 3 switches.
Also assuming that mv88e6xxx_mdios_register() is moved to
mv88e6xxx_setup() (the 2 assumptions go together), at this point,
d0032004.mdio-mii:11 and d0032004.mdio-mii:12 don't have an IRQ mapping
for the internal PHYs anymore, as they've disposed of it in
mv88e6xxx_teardown(). Whereas switch d0032004.mdio-mii:10 has re-created
it, because its code path comes from mv88e6xxx_probe().
Simply put, this change prepares the driver to handle the movement of
mv88e6xxx_mdios_register() to mv88e6xxx_setup() for cross-chip DSA trees.
Also, the code being deleted was partially wrong anyway (in a way which
may have hidden this other issue). mv88e6xxx_g2_irq_mdio_setup()
populates bus->irq[] starting with offset chip->info->phy_base_addr, but
the teardown path doesn't apply that offset too. So it disposes of virq
0 for phy = [ 0, phy_base_addr ).
All switch families have phy_base_addr = 0, except for MV88E6141 and
MV88E6341 which have it as 0x10. I guess those families would have
happened to work by mistake in cross-chip scenarios too.
I'm deleting the body of mv88e6xxx_g2_irq_mdio_free() but leaving its
call sites and prototype in place. This is because, if we ever need to
add back some teardown procedure in the future, it will be perhaps
error-prone to deduce the proper call sites again. Whereas like this,
no extra code should get generated, it shouldn't bother anybody.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Klaus Kudielka <[email protected]>
---
Notes:
v3: Patch is new
drivers/net/dsa/mv88e6xxx/global2.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
index ed3b2f88e7..a26546d3d7 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -1176,31 +1176,19 @@ int mv88e6xxx_g2_irq_setup(struct mv88e6xxx_chip *chip)
int mv88e6xxx_g2_irq_mdio_setup(struct mv88e6xxx_chip *chip,
struct mii_bus *bus)
{
- int phy, irq, err, err_phy;
+ int phy, irq;
for (phy = 0; phy < chip->info->num_internal_phys; phy++) {
irq = irq_find_mapping(chip->g2_irq.domain, phy);
- if (irq < 0) {
- err = irq;
- goto out;
- }
+ if (irq < 0)
+ return irq;
+
bus->irq[chip->info->phy_base_addr + phy] = irq;
}
return 0;
-out:
- err_phy = phy;
-
- for (phy = 0; phy < err_phy; phy++)
- irq_dispose_mapping(bus->irq[phy]);
-
- return err;
}
void mv88e6xxx_g2_irq_mdio_free(struct mv88e6xxx_chip *chip,
struct mii_bus *bus)
{
- int phy;
-
- for (phy = 0; phy < chip->info->num_internal_phys; phy++)
- irq_dispose_mapping(bus->irq[phy]);
}
--
2.39.2
On Wed, Mar 15, 2023 at 05:38:43PM +0100, Klaus Kudielka wrote:
> From: Vladimir Oltean <[email protected]>
>
> Simply put, this change prepares the driver to handle the movement of
> mv88e6xxx_mdios_register() to mv88e6xxx_setup() for cross-chip DSA trees.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Klaus Kudielka <[email protected]>
> ---
Tested-by: Vladimir Oltean <[email protected]>
No point in adding my Reviewed-by tag, since I wrote this patch...
On Wed, Mar 15, 2023 at 05:38:44PM +0100, Klaus Kudielka wrote:
> Move mv88e6xxx_setup() below mv88e6xxx_mdios_register(), so that we are
> able to call the latter one from here. Do the same thing for the
> inverse functions.
>
> Signed-off-by: Klaus Kudielka <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Wed, Mar 15, 2023 at 05:38:45PM +0100, Klaus Kudielka wrote:
> Call the rather expensive mv88e6xxx_mdios_register() at the beginning of
> mv88e6xxx_setup(). This avoids the double call via mv88e6xxx_probe()
> during boot.
>
> For symmetry, call mv88e6xxx_mdios_unregister() at the end of
> mv88e6xxx_teardown().
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Suggested-by: Andrew Lunn <[email protected]>
> Signed-off-by: Klaus Kudielka <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
Tested-by: Vladimir Oltean <[email protected]>
On Wed, Mar 15, 2023 at 05:38:46PM +0100, Klaus Kudielka wrote:
> To avoid excessive mdio bus transactions during probing, mask all phy
> addresses that do not exist (there is a 1:1 mapping between switch port
> number and phy address).
>
> Suggested-by: Andrew Lunn <[email protected]>
> Signed-off-by: Klaus Kudielka <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
Note that on Turris MOX, the PHYs are described in the device tree, so
the MDIO bus is not auto-scanned and this patch has no effect, therefore
I won't add my Tested-by tag here.
On 3/15/23 09:38, Klaus Kudielka wrote:
> From: Vladimir Oltean <[email protected]>
>
> irq_find_mapping() does not need irq_dispose_mapping(), only
> irq_create_mapping() does.
>
> Calling irq_dispose_mapping() from mv88e6xxx_g2_irq_mdio_free() and from
> the error path of mv88e6xxx_g2_irq_mdio_setup() effectively means that
> the mdiobus logic (for internal PHY interrupts) is disposing of a
> hwirq->virq mapping which it is not responsible of (but instead, the
> function pair mv88e6xxx_g2_irq_setup() + mv88e6xxx_g2_irq_free() is).
>
> With the current code structure, this isn't such a huge problem, because
> mv88e6xxx_g2_irq_mdio_free() is called relatively close to the real
> owner of the IRQ mappings:
>
> mv88e6xxx_remove()
> -> mv88e6xxx_unregister_switch()
> -> mv88e6xxx_mdios_unregister()
> -> mv88e6xxx_g2_irq_mdio_free()
> -> mv88e6xxx_g2_irq_free()
>
> and the switch isn't 'live' in any way such that it would be able of
> generating interrupts at this point (mv88e6xxx_unregister_switch() has
> been called).
>
> However, there is a desire to split mv88e6xxx_mdios_unregister() and
> mv88e6xxx_g2_irq_free() such that mv88e6xxx_mdios_unregister() only gets
> called from mv88e6xxx_teardown(). This is much more problematic, as can
> be seen below.
>
> In a cross-chip scenario (say 3 switches d0032004.mdio-mii:10,
> d0032004.mdio-mii:11 and d0032004.mdio-mii:12 which form a single DSA
> tree), it is possible to unbind the device driver from a single switch
> (say d0032004.mdio-mii:10).
>
> When that happens, mv88e6xxx_remove() will be called for just that one
> switch, and this will call mv88e6xxx_unregister_switch() which will tear
> down the entire tree (calling mv88e6xxx_teardown() for all 3 switches).
>
> Assuming mv88e6xxx_mdios_unregister() was moved to mv88e6xxx_teardown(),
> at this stage, all 3 switches will have called irq_dispose_mapping() on
> their mdiobus virqs.
>
> When we bind again the device driver to d0032004.mdio-mii:10,
> mv88e6xxx_probe() is called for it, which calls dsa_register_switch().
> The DSA tree is now complete again, and mv88e6xxx_setup() is called for
> all 3 switches.
>
> Also assuming that mv88e6xxx_mdios_register() is moved to
> mv88e6xxx_setup() (the 2 assumptions go together), at this point,
> d0032004.mdio-mii:11 and d0032004.mdio-mii:12 don't have an IRQ mapping
> for the internal PHYs anymore, as they've disposed of it in
> mv88e6xxx_teardown(). Whereas switch d0032004.mdio-mii:10 has re-created
> it, because its code path comes from mv88e6xxx_probe().
>
> Simply put, this change prepares the driver to handle the movement of
> mv88e6xxx_mdios_register() to mv88e6xxx_setup() for cross-chip DSA trees.
>
> Also, the code being deleted was partially wrong anyway (in a way which
> may have hidden this other issue). mv88e6xxx_g2_irq_mdio_setup()
> populates bus->irq[] starting with offset chip->info->phy_base_addr, but
> the teardown path doesn't apply that offset too. So it disposes of virq
> 0 for phy = [ 0, phy_base_addr ).
>
> All switch families have phy_base_addr = 0, except for MV88E6141 and
> MV88E6341 which have it as 0x10. I guess those families would have
> happened to work by mistake in cross-chip scenarios too.
>
> I'm deleting the body of mv88e6xxx_g2_irq_mdio_free() but leaving its
> call sites and prototype in place. This is because, if we ever need to
> add back some teardown procedure in the future, it will be perhaps
> error-prone to deduce the proper call sites again. Whereas like this,
> no extra code should get generated, it shouldn't bother anybody.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Klaus Kudielka <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/15/23 09:38, Klaus Kudielka wrote:
> Move mv88e6xxx_setup() below mv88e6xxx_mdios_register(), so that we are
> able to call the latter one from here. Do the same thing for the
> inverse functions.
>
> Signed-off-by: Klaus Kudielka <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/15/23 09:38, Klaus Kudielka wrote:
> Call the rather expensive mv88e6xxx_mdios_register() at the beginning of
> mv88e6xxx_setup(). This avoids the double call via mv88e6xxx_probe()
> during boot.
>
> For symmetry, call mv88e6xxx_mdios_unregister() at the end of
> mv88e6xxx_teardown().
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Suggested-by: Andrew Lunn <[email protected]>
> Signed-off-by: Klaus Kudielka <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 3/15/23 09:38, Klaus Kudielka wrote:
> To avoid excessive mdio bus transactions during probing, mask all phy
> addresses that do not exist (there is a 1:1 mapping between switch port
> number and phy address).
>
> Suggested-by: Andrew Lunn <[email protected]>
> Signed-off-by: Klaus Kudielka <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Wed, 15 Mar 2023 17:38:42 +0100 you wrote:
> Starting with commit 1a136ca2e089 ("net: mdio: scan bus based on bus
> capabilities for C22 and C45"), 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.
>
> [...]
Here is the summary with links:
- [net-next,v4,1/4] net: dsa: mv88e6xxx: don't dispose of Global2 IRQ mappings from mdiobus code
https://git.kernel.org/netdev/net-next/c/b1a2de9ccfe6
- [net-next,v4,2/4] net: dsa: mv88e6xxx: re-order functions
https://git.kernel.org/netdev/net-next/c/f1bee740fa82
- [net-next,v4,3/4] net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()
https://git.kernel.org/netdev/net-next/c/2cb0658d4f88
- [net-next,v4,4/4] net: dsa: mv88e6xxx: mask apparently non-existing phys during probing
https://git.kernel.org/netdev/net-next/c/2c7e46edbd03
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Wed, 15 Mar 2023 17:38:46 +0100
Klaus Kudielka <[email protected]> wrote:
> To avoid excessive mdio bus transactions during probing, mask all phy
> addresses that do not exist (there is a 1:1 mapping between switch port
> number and phy address).
>
> Suggested-by: Andrew Lunn <[email protected]>
> Signed-off-by: Klaus Kudielka <[email protected]>
> Reviewed-by: Andrew Lunn <[email protected]>
> ---
>
> Notes:
> v2: Patch is new
>
> drivers/net/dsa/mv88e6xxx/chip.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 29b0f3bb1c..c52798d9ce 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -3797,6 +3797,7 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip,
> bus->read_c45 = mv88e6xxx_mdio_read_c45;
> bus->write_c45 = mv88e6xxx_mdio_write_c45;
> bus->parent = chip->dev;
> + bus->phy_mask = GENMASK(31, mv88e6xxx_num_ports(chip));
shouldnt this be
GENMASK(31, mv88e6xxx_num_ports(chip) + chip->info->phy_base_addr) |
GENMASK(chip->info->phy_base_addr, 0)
?
Or alternatively
~GENMASK(chip->info->phy_base_addr + mv88e6xxx_num_ports(chip),
chip->info->phy_base_addr)
Marek
On Sun, Mar 19, 2023 at 11:06:06AM +0100, Marek Beh?n wrote:
> > + bus->phy_mask = GENMASK(31, mv88e6xxx_num_ports(chip));
>
> shouldnt this be
> GENMASK(31, mv88e6xxx_num_ports(chip) + chip->info->phy_base_addr) |
> GENMASK(chip->info->phy_base_addr, 0)
> ?
> Or alternatively
> ~GENMASK(chip->info->phy_base_addr + mv88e6xxx_num_ports(chip),
> chip->info->phy_base_addr)
Good point. Would you mind sending a patch? I prefer the second variant BTW.
On Sun, Mar 19, 2023 at 11:06:06AM +0100, Marek Beh?n wrote:
> ~GENMASK(chip->info->phy_base_addr + mv88e6xxx_num_ports(chip),
> chip->info->phy_base_addr)
But it needs to be ~GENMASK(base + num - 1, base), no?