reset value of MV88E6XXX_G1_IEEE_PRI

reset value of MV88E6XXX_G1_IEEE_PRI

[Rasmus Villemoes]

Hi,

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?

Rasmus

Re: reset value of MV88E6XXX_G1_IEEE_PRI

[Vivien Didelot]

Hi Rasmus,

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.

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.


Thanks,
Vivien

Re: reset value of MV88E6XXX_G1_IEEE_PRI

[Rasmus Villemoes]

On 27/05/2019 14.32, Vivien Didelot wrote:
> Hi Rasmus,
> 
> 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.

> 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.

Rasmus

Re: reset value of MV88E6XXX_G1_IEEE_PRI

[Vivien Didelot]

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

Re: reset value of MV88E6XXX_G1_IEEE_PRI

[Rasmus Villemoes]

On 27/05/2019 16.20, Vivien Didelot wrote:
> Hi Rasmus,
> 
>>
>> 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.

Urgh, yes, but now that I got access to other data sheets I also checked
88e6095, and that actually does describe a reset value of 0xfa41. So
that value is not taken out of thin air, though it does not apply to the
chip variant that the current mv88e6085_g1_ieee_pri_map helper is named
after :(

So I think I'll add a mv88e6250_g1_ieee_pri_map for the chip I'm working
on, then other chips that have 0xfa50 as the reset value can be switched
over to use that one by one, double-checking the data sheet (and ideally
also the actual hardware...) for each one.

Rasmus