Hi Russell,

On Thu, Mar 09, 2023 at 01:54:20PM +0100, Lukasz Majewski wrote:

quoted

Running of the mv88e6xxx_validate_frame_size() function provided
following results:

[    1.585565] BUG: Marvell 88E6020 has differing max_frame_size:
1632 != 2048 [    1.592540] BUG: Marvell 88E6071 has differing
max_frame_size: 1632 != 2048 ^------ Correct -> mv88e6250 family
max frame size = 2048B

[    1.599507] BUG: Marvell 88E6085 has differing max_frame_size:
1632 != 1522 [    1.606476] BUG: Marvell 88E6165 has differing
max_frame_size: 1522 != 1632 [    1.613445] BUG: Marvell 88E6190X
has differing max_frame_size: 10240 != 1522 [    1.620590] BUG:
Marvell 88E6191X has differing max_frame_size: 10240 != 1522 [
1.627730] BUG: Marvell 88E6193X has differing max_frame_size: 10240
!= 1522 ^------ Needs to be fixed!!!

[    1.634871] BUG: Marvell 88E6220 has differing max_frame_size:
1632 != 2048 [    1.641842] BUG: Marvell 88E6250 has differing
max_frame_size: 1632 != 2048 ^------ Correct -> mv88e6250 family
max frame size = 2048B  

If I understand this correctly, in patch 4, you add a call to the 6250
family to call mv88e6185_g1_set_max_frame_size(), which sets a bit
called MV88E6185_G1_CTL1_MAX_FRAME_1632 if the frame size is larger
than 1518.

Yes, correct.

However, you're saying that 6250 has a frame size of 2048. That's
fine, but it makes MV88E6185_G1_CTL1_MAX_FRAME_1632 rather misleading
as a definition. While the bit may increase the frame size, I think
if we're going to do this, then this definition ought to be renamed.

I thought about rename, but then I've double checked; register offset
and exact bit definition is the same as for 6185, so to avoid
unnecessary code duplication - I've reused the existing function.

Maybe comment would be just enough?

That said, I would like Andrew and Vladimir's thoughts on this too.

Ok.

Finally, I would expect, if this series was done the way I suggested,
that patch 1 should set the max frame size according to how the
existing code works, which means patch 2, being the validation patch,
should be completely silent if patch 1 is correct - and that's the
entire point of validating. It's to make sure that patch 1 is
correct.

Ok.

If it isn't correct, then patch 1 is wrong and should be updated.

Please correct my understanding - I do see two approaches here:


A. In patch 1 I do set the max_frame_size values (deduced). Then I add
validation function (patch 2). This function shows "BUG:...." only when
we do have a mismatch. In patch 3 I do correct the max_frame_size
values (according to validation function) and remove the validation
function. This is how it is done in v5 and is going to be done in v6.


B. Having showed the v5 in public, the validation function is known.
Then I do prepare v6 with only patch 1 having correct values (from the
outset) and provide in the commit message the code for validation
function. Then patch 2 and 3 (validation function and the corrected
values of max_frame_size) can be omitted in v6.

For me it would be better to choose approach B.

Essentially, this patch should only exist if the values we are using
today are actually incorrect.

To put this another way, the conversion from our existing way of
determining the max mtu to using the .max_frame_size method should be
an entire no-op from the driver operation point of view. Then any
errors in those values should be fixed and explained in a separate
commit. Then the new support added.

At least that's how I see it. Andrew and Vladimir may disagree.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de