Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Ante Knezic]

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.