On 12/14/2018 09:23 AM, Andrew Lunn wrote: >> Hi, >> >>> Hopefully the config_aneg callback is not called if you don't list >>> autoneg to the .features. The microchip_t1 driver just uses >>> genphy_config_aneg, but if a NULL works, i would prefer that. >> >> Without the custom config_aneg which sets speed and duplex, I get a >> report claiming the link is at 10/Half , while the link is at 100/Full. >> If I force this in the custom config_aneg, the communication works fine. >> Do you have a hint for me ? > > I can make some guesses, but i've never used a PHY which does not have > auto-neg. > > If the config_aneg function is being called, i wonder if > AUTONEG_DISABLE == phydev->autoneg is true? > > Is the read_status function doing the right thing? Does it set the > speed, duplex and link up? I'm not sure that is needed, it looks like > phy_sanitize_settings() should do that.
The read_status just changes the link, nothing else. But, I dropped all the aneg stuff and that seems to do the right thing, so I now have a fixed 100/Full link. I think I'll just submit a V2, since the patch changed a lot. >> Oh, nice. Shouldn't this be basic_t1 , which is already supported ? > > I'm getting the various T mixed up. Yes, basic_t1. Hmmm, is there a 10/Full variant of the T1 ? >>> We also need to think about what we do with the PHY_BASIC_T1_FEATURES >>> macro. Ideally we want to swap that to also make use of a new >>> ethtool_link_mode_bit_indices, but i've no idea at the moment if that >>> will break something. >> >> Do you mind if I skip this part for now , until I get the driver into >> better shape ? > > For the moment, we can postpone that. Lets get the basics working. > What i'm slightly worried about is this could be an ABI change, and > break older systems. If things work correctly such that T1 is selected > without userspace being involved, no need to set the link parameters, > then we are probably safe. We can also ask Microchip to test there T1 > driver to make sure it still works. Fine -- Best regards, Marek Vasut
