Re: [Qemu-devel] [RFC PATCH v1 1/2] e1000: clean up set_phy_ctrl function

[Gabriel L. Somlo]

On Mon, Jun 30, 2014 at 02:12:51PM -0400, Gabriel L. Somlo wrote:
> On Mon, Jun 30, 2014 at 09:04:12PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jun 30, 2014 at 12:55:49PM -0400, Gabriel L. Somlo wrote:
> > > set_phy_ctrl() runs the same autonegotiation availability checks
> > > that were factored out into have_autoneg() in an earlier commit
> > > (d7a4155265416a1c8f3067b59e68bf5fda1d6215), except it runs them
> > > on the value about to be written into the phy_ctrl register, and
> > > then does not actually write the register.
> > > 
> > > This patch moves the have_autoneg() function in front of
> > > set_phy_ctrl(), then updates the latter to actually write
> > > the register with the given value, and use the factored-out
> > > check to determine whether the link should be bounced.
> > > 
> > > Also, fix indentation/line width when setting up the timer.
> > > 
> > > Signed-off-by: Gabriel Somlo <address@hidden>
> > 
> > Weird.
> > So does this mean have_autoneg is always false then?
> > How does it work?

Oh, I think I know what's going on now:

In set_mdic() we have:

...
        } else {
            if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
                phyreg_writeops[addr](s, index, data);
            }
            s->phy_reg[addr] = data;
        }
...

Basically, phyreg_writeops[PHY_CTRL] == set_phy_ctrl() is the only
one that applies, but it's like a "pre-write" writeop, the write
happens *after* the write_op completes :) :) Fun stuff...

Leaving it as-is is probably OK, although it does indeed not handle
SC bits properly.

I could rewrite have_autoneg() to accept the *value* of the phy control
register, then we can use it from set_phy_ctrl() before the register
is actually written. Not sure about whether that would be more or less
ugly than the current form :)

Thanks,
--Gabriel