On Fri, Mar 10, 2023 at 12:53:46PM +0100, Lukasz Majewski wrote:

Hi Andrew,

quoted

quoted

quoted

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.
  

quoted

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?  

The driver takes care with its namespace in order to add per switch
family defines. So you can add MV88E6250_G1_CTL1_MAX_FRAME_2048. It
does not matter if it is the same bit. You can also add a
mv88e6250_g1_set_max_frame_size() and it also does not matter if it is
in effect the same as mv88e6185_g1_set_max_frame_size().

We should always make the driver understandably first, compact and
without redundancy second. We are then less likely to get into
situations like this again where it is not clear what MTU a device
actually supports because the code is cryptic.

Ok, I will add new function.

Thanks for hints.

It may be worth doing:

static int mv88e6xxx_g1_modify(struct mv88e6xxx_chip *chip, int reg,
			       u16 mask, u16 val)
{
	int addr = chip->info->global1_addr;
	int err;
	u16 v;

	err = mv88e6xxx_read(chip, addr, reg, &v);
	if (err < 0)
		return err;

	v = (v & ~mask) | val;

	return mv88e6xxx_write(chip, addr, reg, v);
}

Then, mv88e6185_g1_set_max_frame_size() becomes:

int mv88e6185_g1_set_max_frame_size(struct mv88e6xxx_chip *chip, int mtu)
{
	u16 val = 0;

	if (mtu + ETH_HLEN + ETH_FCS_LEN > 1518)
		val = MV88E6185_G1_CTL1_MAX_FRAME_1632;

	return mv88e6xxx_g1_modify(chip, MV88E6XXX_G1_CTL1,
				   MV88E6185_G1_CTL1_MAX_FRAME_1632, val);
}

The 6250 variant becomes similar.

We can also think about converting all those other read-modify-writes
to use mv88e6xxx_g1_modify().

The strange thing is... we already have mv88e6xxx_g1_ctl2_mask() which
is an implementation of mv88e6xxx_g1_modify() specifically for
MV88E6XXX_G1_CTL2 register, although it uses (val & mask) rather than
just val. That wouldn't be necessary if the bitfield macros (e.g.
FIELD_PREP() were used rather than explicit __bf_shf().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!