Re: reset value of MV88E6XXX_G1_IEEE_PRI

[Vivien Didelot <vivien.didelot@gmail.com>]

Hi Rasmus,

On Mon, 27 May 2019 13:02:04 +0000, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> > On Mon, 27 May 2019 09:36:13 +0000, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> >> Looking through the data sheets comparing the mv88e6240 and 6250, I
> >> noticed that they have the exact same description of the G1_IEEE_PRI
> >> register (global1, offset 0x18). However, the current code used by 6240 does
> >>
> >> int mv88e6085_g1_ieee_pri_map(struct mv88e6xxx_chip *chip)
> >> {
> >> 	/* Reset the IEEE Tag priorities to defaults */
> >> 	return mv88e6xxx_g1_write(chip, MV88E6XXX_G1_IEEE_PRI, 0xfa41);
> >> }
> >>
> >> while if my reading of the data sheet is correct, the reset value is
> >> really 0xfa50 (fields 7:6 and 5:4 are RWS to 0x1, field 3:2 and 1:0 are
> >> RWR) - and this is also the value I read from the 6250 on our old BSP
> >> with an out-of-tree driver that doesn't touch that register. This seems
> >> to go way back (at least 3b1588593097). Should this be left alone for
> >> not risking breaking existing setups (just updating the comment), or can
> >> we make the code match the comment? Or am I just reading this all wrong?
> > 
> > If the reset value isn't the same, the bits are certainly differently
> > organized inside this register, so the proper way would be to add a
> > mv88e6240_g1_ieee_pri_map function, used by both 88E6240 and 88E6250.
> > 
> 
> Based on the very systematic description [ieee tags 7 and 6 are mapped
> to 3, 5 and 4 to 2, 3 and 2 to 1, and 1 and 0 to 0], I strongly believe
> that 0xfa50 is also the reset value for the 6085, so this is most likely
> wrong for all the current chips - though I don't have a 6085 data sheet.
> 
> I can certainly add a 6250 variant that does the right thing for the
> 6250, and I probably will - this is more a question about the current code.

Good catch, I double checked 88E6085 and 88E6352 and both describe
a reset value of 0xFA50. Fixing mv88e6085_g1_ieee_pri_map should
be enough.
> 
> > I'm not a big fan of rewriting the default values, but that is the
> > way we chose until we make actually use of these tag priority bits.
> 
> Yes, I was wondering why there's a lot of code which simply serves to
> set default values - but I guess it makes sense to force the switch into
> a known state in case the bootloader did something odd.

That was the idea, yes.


Thank you,
Vivien