Messages in this thread | | | Date | Fri, 10 Mar 2023 14:19:28 +0100 | From | Lukasz Majewski <> | Subject | Re: [PATCH 6/7] dsa: marvell: Correct value of max_frame_size variable after validation |
| |
Hi Russell,
> On Fri, Mar 10, 2023 at 12:53:46PM +0100, Lukasz Majewski wrote: > > Hi Andrew, > > > > > > > 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? > > > > > > 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); } >
Yes, correct.
> 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(). >
I do have the impression that major refactoring of the mv6xxx driver would be welcome...
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 [unhandled content-type:application/pgp-signature] | |