| Message ID | 20191105001301.27966-4-andrew@lunn.ch |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | mv88e6xxx ATU occupancy as devlink resource | expand |
On Tue, 5 Nov 2019 01:12:59 +0100, Andrew Lunn <andrew@lunn.ch> wrote: > Add helpers to set/get the ATU statistics register. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/dsa/mv88e6xxx/global2.c | 20 ++++++++++++++++++++ > drivers/net/dsa/mv88e6xxx/global2.h | 24 +++++++++++++++++++++++- > 2 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c > index bdbb72fc20ed..14954d92c564 100644 > --- a/drivers/net/dsa/mv88e6xxx/global2.c > +++ b/drivers/net/dsa/mv88e6xxx/global2.c > @@ -280,6 +280,26 @@ int mv88e6xxx_g2_set_switch_mac(struct mv88e6xxx_chip *chip, u8 *addr) > return err; > } > > +/* Offset 0x0E: ATU Statistics */ > + > +int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip, u16 kind, u16 bin) > +{ > + return mv88e6xxx_g2_write(chip, MV88E6XXX_G2_ATU_STATS, > + kind | bin); > +} > + > +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip) > +{ > + int err; > + u16 val; > + > + err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_ATU_STATS, &val); > + if (err) > + return err; > + > + return val & MV88E6XXX_G2_ATU_STATS_MASK; > +} I would use the logic commonly used in the mv88e6xxx driver for functions that may fail, returning only an error code and taking a pointer to the correct type as argument, so that we do as usual: err = mv88e6xxx_g2_atu_stats_get(chip, &val); if (err) return err; > + > /* Offset 0x0F: Priority Override Table */ > > static int mv88e6xxx_g2_pot_write(struct mv88e6xxx_chip *chip, int pointer, > diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h > index 42da4bca73e8..a308ca7a7da6 100644 > --- a/drivers/net/dsa/mv88e6xxx/global2.h > +++ b/drivers/net/dsa/mv88e6xxx/global2.h > @@ -113,7 +113,16 @@ > #define MV88E6XXX_G2_SWITCH_MAC_DATA_MASK 0x00ff > > /* Offset 0x0E: ATU Stats Register */ > -#define MV88E6XXX_G2_ATU_STATS 0x0e > +#define MV88E6XXX_G2_ATU_STATS 0x0e > +#define MV88E6XXX_G2_ATU_STATS_BIN_0 (0x0 << 14) > +#define MV88E6XXX_G2_ATU_STATS_BIN_1 (0x1 << 14) > +#define MV88E6XXX_G2_ATU_STATS_BIN_2 (0x2 << 14) > +#define MV88E6XXX_G2_ATU_STATS_BIN_3 (0x3 << 14) > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL (0x0 << 12) > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL_DYNAMIC (0x1 << 12) > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL (0x2 << 12) > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL_DYNAMIC (0x3 << 12) > +#define MV88E6XXX_G2_ATU_STATS_MASK 0x0fff Please use the 0x1234 format for these 16-bit registers. > > /* Offset 0x0F: Priority Override Table */ > #define MV88E6XXX_G2_PRIO_OVERRIDE 0x0f > @@ -353,6 +362,8 @@ extern const struct mv88e6xxx_gpio_ops mv88e6352_gpio_ops; > > int mv88e6xxx_g2_scratch_gpio_set_smi(struct mv88e6xxx_chip *chip, > bool external); > +int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip, u16 kind, u16 bin); > +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip); > > #else /* !CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */ > > @@ -515,6 +526,17 @@ static inline int mv88e6xxx_g2_device_mapping_write(struct mv88e6xxx_chip *chip, > return -EOPNOTSUPP; > } > > +static inline int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip, > + u16 kind, u16 bin) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip) > +{ > + return -EOPNOTSUPP; > +} > + > #endif /* CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */ > > #endif /* _MV88E6XXX_GLOBAL2_H */ > -- > 2.23.0 > Otherwise this looks good. Thanks, Vivien
Hi Andrew, On Tue, 5 Nov 2019 23:32:41 -0500, Vivien Didelot <vivien.didelot@gmail.com> wrote: > > +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip) > > +{ > > + int err; > > + u16 val; > > + > > + err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_ATU_STATS, &val); > > + if (err) > > + return err; > > + > > + return val & MV88E6XXX_G2_ATU_STATS_MASK; > > +} > > I would use the logic commonly used in the mv88e6xxx driver for > functions that may fail, returning only an error code and taking a > pointer to the correct type as argument, so that we do as usual: > > err = mv88e6xxx_g2_atu_stats_get(chip, &val); > if (err) > return err; > > > + > > /* Offset 0x0F: Priority Override Table */ > > > > static int mv88e6xxx_g2_pot_write(struct mv88e6xxx_chip *chip, int pointer, > > diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h > > index 42da4bca73e8..a308ca7a7da6 100644 > > --- a/drivers/net/dsa/mv88e6xxx/global2.h > > +++ b/drivers/net/dsa/mv88e6xxx/global2.h > > @@ -113,7 +113,16 @@ > > #define MV88E6XXX_G2_SWITCH_MAC_DATA_MASK 0x00ff > > > > /* Offset 0x0E: ATU Stats Register */ > > -#define MV88E6XXX_G2_ATU_STATS 0x0e > > +#define MV88E6XXX_G2_ATU_STATS 0x0e > > +#define MV88E6XXX_G2_ATU_STATS_BIN_0 (0x0 << 14) > > +#define MV88E6XXX_G2_ATU_STATS_BIN_1 (0x1 << 14) > > +#define MV88E6XXX_G2_ATU_STATS_BIN_2 (0x2 << 14) > > +#define MV88E6XXX_G2_ATU_STATS_BIN_3 (0x3 << 14) > > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL (0x0 << 12) > > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL_DYNAMIC (0x1 << 12) > > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL (0x2 << 12) > > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL_DYNAMIC (0x3 << 12) > > +#define MV88E6XXX_G2_ATU_STATS_MASK 0x0fff > > Please use the 0x1234 format for these 16-bit registers. Oops the series has already been applied. Andrew please consider these comments for your future series, they always apply... Thank a lot, Vivien
> > +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip) > > +{ > > + int err; > > + u16 val; > > + > > + err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_ATU_STATS, &val); > > + if (err) > > + return err; > > + > > + return val & MV88E6XXX_G2_ATU_STATS_MASK; > > +} > > I would use the logic commonly used in the mv88e6xxx driver for > functions that may fail, returning only an error code and taking a > pointer to the correct type as argument, so that we do as usual: > > err = mv88e6xxx_g2_atu_stats_get(chip, &val); > if (err) > return err; Well, actually. Take a look at the next patch. This function gets changed there. After you reviewed the first devlink patchset adding control of the ATU algorithm, i went through this patchset and applied the same comments to it. So i change the implementation to how you actually wanted it. But i messed up my git commands and that changed ended up in the next patch :-( > > /* Offset 0x0E: ATU Stats Register */ > > -#define MV88E6XXX_G2_ATU_STATS 0x0e > > +#define MV88E6XXX_G2_ATU_STATS 0x0e > > +#define MV88E6XXX_G2_ATU_STATS_BIN_0 (0x0 << 14) > > +#define MV88E6XXX_G2_ATU_STATS_BIN_1 (0x1 << 14) > > +#define MV88E6XXX_G2_ATU_STATS_BIN_2 (0x2 << 14) > > +#define MV88E6XXX_G2_ATU_STATS_BIN_3 (0x3 << 14) > > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL (0x0 << 12) > > +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL_DYNAMIC (0x1 << 12) > > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL (0x2 << 12) > > +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL_DYNAMIC (0x3 << 12) > > +#define MV88E6XXX_G2_ATU_STATS_MASK 0x0fff > > Please use the 0x1234 format for these 16-bit registers. Ug, no. That is a lot less readable. The datasheet describes there fields in terms of bit offsets in the registers. Bit 14 and 15 for the bin, bits 12 and 13 for the mode. You can clearly see that when using value and shift representation. 0x0200 is much harder to read, and much more error prone. Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c index bdbb72fc20ed..14954d92c564 100644 --- a/drivers/net/dsa/mv88e6xxx/global2.c +++ b/drivers/net/dsa/mv88e6xxx/global2.c @@ -280,6 +280,26 @@ int mv88e6xxx_g2_set_switch_mac(struct mv88e6xxx_chip *chip, u8 *addr) return err; } +/* Offset 0x0E: ATU Statistics */ + +int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip, u16 kind, u16 bin) +{ + return mv88e6xxx_g2_write(chip, MV88E6XXX_G2_ATU_STATS, + kind | bin); +} + +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip) +{ + int err; + u16 val; + + err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_ATU_STATS, &val); + if (err) + return err; + + return val & MV88E6XXX_G2_ATU_STATS_MASK; +} + /* Offset 0x0F: Priority Override Table */ static int mv88e6xxx_g2_pot_write(struct mv88e6xxx_chip *chip, int pointer, diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h index 42da4bca73e8..a308ca7a7da6 100644 --- a/drivers/net/dsa/mv88e6xxx/global2.h +++ b/drivers/net/dsa/mv88e6xxx/global2.h @@ -113,7 +113,16 @@ #define MV88E6XXX_G2_SWITCH_MAC_DATA_MASK 0x00ff /* Offset 0x0E: ATU Stats Register */ -#define MV88E6XXX_G2_ATU_STATS 0x0e +#define MV88E6XXX_G2_ATU_STATS 0x0e +#define MV88E6XXX_G2_ATU_STATS_BIN_0 (0x0 << 14) +#define MV88E6XXX_G2_ATU_STATS_BIN_1 (0x1 << 14) +#define MV88E6XXX_G2_ATU_STATS_BIN_2 (0x2 << 14) +#define MV88E6XXX_G2_ATU_STATS_BIN_3 (0x3 << 14) +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL (0x0 << 12) +#define MV88E6XXX_G2_ATU_STATS_MODE_ALL_DYNAMIC (0x1 << 12) +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL (0x2 << 12) +#define MV88E6XXX_G2_ATU_STATS_MODE_FID_ALL_DYNAMIC (0x3 << 12) +#define MV88E6XXX_G2_ATU_STATS_MASK 0x0fff /* Offset 0x0F: Priority Override Table */ #define MV88E6XXX_G2_PRIO_OVERRIDE 0x0f @@ -353,6 +362,8 @@ extern const struct mv88e6xxx_gpio_ops mv88e6352_gpio_ops; int mv88e6xxx_g2_scratch_gpio_set_smi(struct mv88e6xxx_chip *chip, bool external); +int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip, u16 kind, u16 bin); +int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip); #else /* !CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */ @@ -515,6 +526,17 @@ static inline int mv88e6xxx_g2_device_mapping_write(struct mv88e6xxx_chip *chip, return -EOPNOTSUPP; } +static inline int mv88e6xxx_g2_atu_stats_set(struct mv88e6xxx_chip *chip, + u16 kind, u16 bin) +{ + return -EOPNOTSUPP; +} + +static inline int mv88e6xxx_g2_atu_stats_get(struct mv88e6xxx_chip *chip) +{ + return -EOPNOTSUPP; +} + #endif /* CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */ #endif /* _MV88E6XXX_GLOBAL2_H */
Add helpers to set/get the ATU statistics register. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/dsa/mv88e6xxx/global2.c | 20 ++++++++++++++++++++ drivers/net/dsa/mv88e6xxx/global2.h | 24 +++++++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-)