On 29.11.2024 07:17, Krzysztof Hałasa wrote: > Andrew, > > thanks for your response. > > Andrew Lunn <andrew@xxxxxxx> writes: > >>> It seems the phy/phylink code assumes the PHY starts with autoneg >>> enabled (if supported). This is simply an incorrect assumption. >> >> This is sounding like a driver bug. When phy_start() is called it >> kicks off the PHY state machine. That should result in >> phy_config_aneg() being called. That function is badly named, since it >> is used both for autoneg and forced setting. The purpose of that call >> is to configure the PHY to the configuration stored in >> phydev->advertise, etc. So if the PHY by hardware defaults has autoneg >> disabled, but the configuration in phydev says it should be enabled, >> calling phy_config_aneg() should actually enabled autoneg. > > But... how would the driver know if autoneg is to be enabled or not? > If autoneg is supported, then phylib defaults to enable it. I don't see anything wrong with this. BaseT modes from 1000Mbps on require autoneg. Your original commit message seems to refer to a use case where a certain operation mode of the PHY doesn't support autoneg. Then the PHY driver should detect this operation mode and clear the autoneg-supported bit. > In the USB ASIX case, the Ethernet driver could dig this info up from > the chip EEPROM. Not sure if I like this way, though. Complicated, and > it's not needed in this case I think. > >> I would say there are two different issues here. >> >> 1) It seems like we are not configuring the hardware to match phydev. >> 2) We are overwriting how the bootloader etc configured the hardware. >> >> 2) is always hard, because how do we know the PHY is not messed up >> from a previous boot/crash cycle etc. In general, a driver should try >> to put the hardware into a well known state. If we have a clear use >> case for this, we can consider how to implement it. > > Well, I think if someone set the PHY previously, and then the machine > rebooted (without actually changing PHY config), then perhaps the > settings are better than any defaults anyway. Though I guess it will be > configured in the init scripts again soon. > > It's not something easily messed up by a crash. But yes, there is a risk > the config was wrong, set by mistake or something. > > BTW USB adapters will almost always reconfig PHY on boot, because they > are powered from USB bus. > > In this case, with ASIX USB adapter (internal PHY ax88796b / > ax88796b_rust), the MAC + PHY will be configured by hardware on USB > power up. So we _know_ the settings are better than any hardcoded > defaults. > > Maybe the specific ASIX PHY code should handle this. > > Nevertheless, the inconsistency between phy/phylink/etc. and the actual > hardware PHY is there. > I guess I will have a look at this again shortly.
