On Wed, Jul 02, 2014 at 10:21:34AM -0400, Gabriel L. Somlo wrote: > On Wed, Jul 02, 2014 at 12:33:16PM +0300, Michael S. Tsirkin wrote: > > Just poking around the spec I find more things > > we don't implement correctly wrt to auto-negotiation. > > For example, MII_SR_AUTONEG_CAPS isn't set, is it? > > Maybe that's why your guest doesn't work: > > it doesn't expect to get autonegotation at all? > > > > So I have a question: does your patch actually help any guests? > > If not, maybe we should defer it to after release, > > and try to clean up autonegotiation more thouroughly for 2.2? > > I'll re-submit after 2.1 is officially out. But, since we're talking > about MII_SR_AUTONEG_CAPS: PHY_STATUS is initialized to 0x794d, which > includes setting the MII_SR_AUTONEG_CAPS bit (|= 0x8). > Did you mean: we should check for MII_SR_AUTONEG_CAPS in have_autoneg() ? > (i.e., on the chance it gets turned off by a guest-side write to > PHY_STATUS) ?
PHY_STATUS isn't writeable is it? No I just got confused with the binary math. We really should use symbolic constants there, we already have them defined. > > Thx, > --G > > PS. Maybe also spell out the individual bits in phy_reg_init[] ? Like, > instead of: > > [PHY_STATUS] = 0x794d, > > do this: > > [PHY_STATUS] = MII_SR_EXTENDED_CAPS | > MII_SR_LINK_STATUS | > MII_SR_AUTONEG_CAPS | > MII_SR_PREAMBLE_SUPPRESS | > MII_SR_EXTENDED_STATUS | > MII_SR_10T_HD_CAPS | > MII_SR_10T_FD_CAPS | > MII_SR_100X_HD_CAPS | > MII_SR_100X_FD_CAPS, > > ... for all registers ? Much more verbose, but IMHO that'd be a good > thing :) > >
