| Message ID | 20220818102924.287719-4-mattias.forsblad@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Add RMU support | expand |
> @@ -1310,16 +1323,22 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port, > struct mv88e6xxx_chip *chip = ds->priv; > int ret; > > - mv88e6xxx_reg_lock(chip); > + if (chip->rmu.ops && chip->rmu.ops->get_rmon) { > + ret = chip->rmu.ops->get_rmon(chip, port, data); > + if (ret == -ETIMEDOUT) > + return; > + } else { > > - ret = mv88e6xxx_stats_snapshot(chip, port); > - mv88e6xxx_reg_unlock(chip); > + mv88e6xxx_reg_lock(chip); > > - if (ret < 0) > - return; > + ret = mv88e6xxx_stats_snapshot(chip, port); > + mv88e6xxx_reg_unlock(chip); > > - mv88e6xxx_get_stats(chip, port, data); > + if (ret < 0) > + return; > > + mv88e6xxx_get_stats(chip, port, data); > + } > } I don't particularly like the way this is all mixed together. Could you try to split it, so there is an MDIO set of functions and an RMU set of functions. Maybe you have some helpers which are used by both. I would also suggest you try to think about ATU dump and VTU dump. You ideally want a code structure which is very similar for all these dump operations. Take a look at how qca8k-8xxx.c does things. Is it documented in the datasheet that when RMU is used a snapshot is not required? Andrew
On 2022-08-18 17:36, Andrew Lunn wrote: >> @@ -1310,16 +1323,22 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port, >> struct mv88e6xxx_chip *chip = ds->priv; >> int ret; >> >> - mv88e6xxx_reg_lock(chip); >> + if (chip->rmu.ops && chip->rmu.ops->get_rmon) { >> + ret = chip->rmu.ops->get_rmon(chip, port, data); >> + if (ret == -ETIMEDOUT) >> + return; >> + } else { >> >> - ret = mv88e6xxx_stats_snapshot(chip, port); >> - mv88e6xxx_reg_unlock(chip); >> + mv88e6xxx_reg_lock(chip); >> >> - if (ret < 0) >> - return; >> + ret = mv88e6xxx_stats_snapshot(chip, port); >> + mv88e6xxx_reg_unlock(chip); >> >> - mv88e6xxx_get_stats(chip, port, data); >> + if (ret < 0) >> + return; >> >> + mv88e6xxx_get_stats(chip, port, data); >> + } >> } > > I don't particularly like the way this is all mixed together. > > Could you try to split it, so there is an MDIO set of functions and an > RMU set of functions. Maybe you have some helpers which are used by > both. > I think that a great idea. Will split in separate functions. > I would also suggest you try to think about ATU dump and VTU dump. You > ideally want a code structure which is very similar for all these dump > operations. Take a look at how qca8k-8xxx.c does things. > I think that the ATU dump is the logical next step to implement. I'll have a look in qca8k to see how they do things. AFAIK there is no RMU operation for VTU dump. That needs to be done with register reads and writes. > Is it documented in the datasheet that when RMU is used a snapshot is > not required? > > Andrew In the documentation they only mention captured counters with regards to MDIO accesses. I cannot see that that is necessary with RMU. FYI on our board the time delta between request and reply is ~100us (which is nice)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 888c6e47dd16..344d6633ad6d 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1226,16 +1226,29 @@ static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port, u16 bank1_select, u16 histogram) { struct mv88e6xxx_hw_stat *stat; + int offset = 0; + u64 high; int i, j; for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) { stat = &mv88e6xxx_hw_stats[i]; if (stat->type & types) { - mv88e6xxx_reg_lock(chip); - data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port, - bank1_select, - histogram); - mv88e6xxx_reg_unlock(chip); + if (chip->rmu.ops->get_rmon && !(stat->type & STATS_TYPE_PORT)) { + if (stat->type & STATS_TYPE_BANK1) + offset = 32; + + data[j] = chip->ports[port].rmu_raw_stats[stat->reg + offset]; + if (stat->size == 8) { + high = chip->ports[port].rmu_raw_stats[stat->reg + offset + + 1]; + data[j] += (high << 32); + } + } else { + mv88e6xxx_reg_lock(chip); + data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port, + bank1_select, histogram); + mv88e6xxx_reg_unlock(chip); + } j++; } @@ -1310,16 +1323,22 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port, struct mv88e6xxx_chip *chip = ds->priv; int ret; - mv88e6xxx_reg_lock(chip); + if (chip->rmu.ops && chip->rmu.ops->get_rmon) { + ret = chip->rmu.ops->get_rmon(chip, port, data); + if (ret == -ETIMEDOUT) + return; + } else { - ret = mv88e6xxx_stats_snapshot(chip, port); - mv88e6xxx_reg_unlock(chip); + mv88e6xxx_reg_lock(chip); - if (ret < 0) - return; + ret = mv88e6xxx_stats_snapshot(chip, port); + mv88e6xxx_reg_unlock(chip); - mv88e6xxx_get_stats(chip, port, data); + if (ret < 0) + return; + mv88e6xxx_get_stats(chip, port, data); + } } static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
If RMU is supported, use that interface to collect rmon data. Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 41 +++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 11 deletions(-)