Messages in this thread | | | From | Ante Knezic <> | Subject | Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X | Date | Wed, 26 Jul 2023 11:50:40 +0200 |
| |
On Tue, 25 Jul 2023 20:23:43 +0300 Vladimir Oltean wrote: > I'm not sure which way is preferred by PHY maintainers, but it seems to > be a useless complication to simulate that you have a struct mdio_device > for the other lanes when you don't. It appears more appropriate to just > use mdiobus_c45_write(mpcs->mdio.bus, lanes[i]). >
Agreed.
> There's also the locking question (with the big caveat that we don't > know what the register writes do!). There's locking at the bus level, > but the MDIO device isn't locked. So phylink on those other PCSes can > still do stuff, even in-between the first and the second write to > undocumented register 0xf054. > > I can speculate that writing 0x400c -> 0x4000 is something like: set > RX_RESET | TX_RESET followed by clear RX_RESET | TX_RESET. Is it ok if > stuff happens in between these writes - will it stick, or does this > logically interact with anything else in any other way? I guess we won't > know. I might be a bit closer to being okay with it if you could confirm > that some other (unrelated) register write to the PCS does make it > through (and can be read back) in between the 2 erratum writes.
I was able to confirm this by successfully reading and writing to the SGMII_BMCR register between erratum writes. This did not affect the issue that erratum fixes. Unfortunatelly, there is no info about what the actuall writing to magic registers does.
>> static int mv88e639x_sgmii_pcs_post_config(struct phylink_pcs *pcs, >> phy_interface_t interface) >> { >> struct mv88e639x_pcs *mpcs = sgmii_pcs_to_mv88e639x_pcs(pcs); >> + struct mv88e6xxx_chip *chip = mpcs->chip; >> >> mv88e639x_sgmii_pcs_control_pwr(mpcs, true); >> >> + if (chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6190X || >> + chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6390X) >> + mv88e6390_erratum_3_14(mpcs); > >You could at least print an error if a write failure occurred, so that >it doesn't go completely unnoticed.
Ok, I was simply following the above notion (we don't check or print errors when powering on the serdes lane) but I agree with your point and will adapt the patch for the next version.
| |