| Message ID | 20190603144112.27713-2-rasmus.villemoes@prevas.dk |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: mv88e6xxx: support for mv88e6250 | expand |
On Mon, Jun 03, 2019 at 02:42:12PM +0000, Rasmus Villemoes wrote: > diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h > index bb92a130cbef..2bcbe7c8b279 100644 > --- a/drivers/net/dsa/mv88e6xxx/global1.h > +++ b/drivers/net/dsa/mv88e6xxx/global1.h > @@ -279,6 +279,8 @@ int mv88e6390_g1_mgmt_rsvd2cpu(struct mv88e6xxx_chip *chip); > int mv88e6085_g1_ip_pri_map(struct mv88e6xxx_chip *chip); > int mv88e6085_g1_ieee_pri_map(struct mv88e6xxx_chip *chip); > > +int mv88e6250_g1_ieee_pri_map(struct mv88e6xxx_chip *chip); > + Hi Rasmus We are not 100% consistent, but we try to put all variants of a method together, and then put a blank line afterwards. So could you make this: int mv88e6085_g1_ip_pri_map(struct mv88e6xxx_chip *chip); int mv88e6085_g1_ieee_pri_map(struct mv88e6xxx_chip *chip); int mv88e6250_g1_ieee_pri_map(struct mv88e6xxx_chip *chip); int mv88e6185_g1_set_cascade_port(struct mv88e6xxx_chip *chip, int port); Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Rasmus, On Mon, 3 Jun 2019 14:42:12 +0000, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > Quite a few of the existing supported chips that use > mv88e6085_g1_ieee_pri_map as ->ieee_pri_map (including, incidentally, > mv88e6085 itself) actually have a reset value of 0xfa50 in the > G1_IEEE_PRI register. > > The data sheet for the mv88e6095, however, does describe a reset value > of 0xfa41. > > So rather than changing the value in the existing callback, introduce > a new variant with the 0xfa50 value. That will be used by the upcoming > mv88e6250, and existing chips can be switched over one by one, > preferably double-checking both the data sheet and actual hardware in > each case - if anybody actually feels this is important enough to > care. Given your previous thread on this topic, I'd prefer that you include a first patch which implements mv88e6095_g1_ieee_pri_map() using 0xfa41 and update mv88e{6092,6095}_ops to use it, then a second one which fixes mv88e6085_g1_ieee_pri_map to use 0xfa50. Then mv88e6250_ops can use it. Thanks, Vivien
On 03/06/2019 17.37, Vivien Didelot wrote: > Hi Rasmus, > > On Mon, 3 Jun 2019 14:42:12 +0000, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: >> Quite a few of the existing supported chips that use >> mv88e6085_g1_ieee_pri_map as ->ieee_pri_map (including, incidentally, >> mv88e6085 itself) actually have a reset value of 0xfa50 in the >> G1_IEEE_PRI register. >> >> The data sheet for the mv88e6095, however, does describe a reset value >> of 0xfa41. >> >> So rather than changing the value in the existing callback, introduce >> a new variant with the 0xfa50 value. That will be used by the upcoming >> mv88e6250, and existing chips can be switched over one by one, >> preferably double-checking both the data sheet and actual hardware in >> each case - if anybody actually feels this is important enough to >> care. > > Given your previous thread on this topic, I'd prefer that you include > a first patch which implements mv88e6095_g1_ieee_pri_map() using 0xfa41 > and update mv88e{6092,6095}_ops to use it, then a second one which fixes > mv88e6085_g1_ieee_pri_map to use 0xfa50. Then mv88e6250_ops can use it. Well, the thing is, that would of course fix the value for 6240 and 6085, keeping the right value for 6092 and 6095, but I'd also be changing the value for a whole lot of other chips for which I don't know which one would be the right one. Originally I thought that 0xfa50 was the right value for all chips (simply because x -> floor(x/2) is a sane default mapping), but since I now know that 0xfa41 (i.e., 3 and 0 are mapped to 1, 2 and 1 are mapped to 0) is the reset value for at least some chips, I'd rather not make a blanket change that might fix some and "break" other chips. Rasmus
diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c index 770c03406033..c851b7b532a4 100644 --- a/drivers/net/dsa/mv88e6xxx/global1.c +++ b/drivers/net/dsa/mv88e6xxx/global1.c @@ -299,6 +299,12 @@ int mv88e6085_g1_ieee_pri_map(struct mv88e6xxx_chip *chip) return mv88e6xxx_g1_write(chip, MV88E6XXX_G1_IEEE_PRI, 0xfa41); } +int mv88e6250_g1_ieee_pri_map(struct mv88e6xxx_chip *chip) +{ + /* Reset the IEEE Tag priorities to defaults */ + return mv88e6xxx_g1_write(chip, MV88E6XXX_G1_IEEE_PRI, 0xfa50); +} + /* Offset 0x1a: Monitor Control */ /* Offset 0x1a: Monitor & MGMT Control on some devices */ diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h index bb92a130cbef..2bcbe7c8b279 100644 --- a/drivers/net/dsa/mv88e6xxx/global1.h +++ b/drivers/net/dsa/mv88e6xxx/global1.h @@ -279,6 +279,8 @@ int mv88e6390_g1_mgmt_rsvd2cpu(struct mv88e6xxx_chip *chip); int mv88e6085_g1_ip_pri_map(struct mv88e6xxx_chip *chip); int mv88e6085_g1_ieee_pri_map(struct mv88e6xxx_chip *chip); +int mv88e6250_g1_ieee_pri_map(struct mv88e6xxx_chip *chip); + int mv88e6185_g1_set_cascade_port(struct mv88e6xxx_chip *chip, int port); int mv88e6085_g1_rmu_disable(struct mv88e6xxx_chip *chip);
Quite a few of the existing supported chips that use mv88e6085_g1_ieee_pri_map as ->ieee_pri_map (including, incidentally, mv88e6085 itself) actually have a reset value of 0xfa50 in the G1_IEEE_PRI register. The data sheet for the mv88e6095, however, does describe a reset value of 0xfa41. So rather than changing the value in the existing callback, introduce a new variant with the 0xfa50 value. That will be used by the upcoming mv88e6250, and existing chips can be switched over one by one, preferably double-checking both the data sheet and actual hardware in each case - if anybody actually feels this is important enough to care. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/net/dsa/mv88e6xxx/global1.c | 6 ++++++ drivers/net/dsa/mv88e6xxx/global1.h | 2 ++ 2 files changed, 8 insertions(+)