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 <so...@cmu.edu>
>
> Weird.
> So does this mean have_autoneg is always false then?
> How does it work?
phy_reg[PHY_CTRL] is initialized (along with the rest of phy_reg)
by a memmove from phy_reg_init, during e1000_reset().
So the question would be rather "does it make sense to look at
anything besides (s->compat_flags & E1000_FLAG_AUTONEG), since
we never allow phy_reg[PHY_CTRL] to change after initialization,
so MII_CR_AUTO_NEG_EN and MII_CR_RESTART_AUTO_NEG bits will always
be set?"
> Wrt to patch itself, let's do it properly pls:
> - bits 0:5 are RO on some cards
> - bit 9 is SC
Will do, once we've sorted out the phy_reg[] mystery :)
Thanks,
--Gabriel
>
> > ---
> >
> > I'm not 100% sure whether "val" in set_phy_ctrl() is not written
> > to phy_reg[PHY_CTRL] by mistake, or whether there's an ulterior
> > motive (in which case we should add a comment as to why that is).
> >
> > Assuming it's a mistake, and we *should* have been writing it (as
> > per my patch below), we can then use have_autoneg() to also simplify
> > the code significantly :)
> >
> > Thanks,
> > Gabriel
> >
> > hw/net/e1000.c | 27 ++++++++++++++-------------
> > 1 file changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index 0fc29a0..2376910 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -186,6 +186,14 @@ e1000_link_up(E1000State *s)
> > s->phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
> > }
> >
> > +static bool
> > +have_autoneg(E1000State *s)
> > +{
> > + return (s->compat_flags & E1000_FLAG_AUTONEG) &&
> > + (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
> > + (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
> > +}
> > +
> > static void
> > set_phy_ctrl(E1000State *s, int index, uint16_t val)
> > {
> > @@ -194,13 +202,14 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
> > * migrate during auto negotiation, after migration the link will be
> > * down.
> > */
> > - if (!(s->compat_flags & E1000_FLAG_AUTONEG)) {
> > - return;
> > - }
> > - if ((val & MII_CR_AUTO_NEG_EN) && (val & MII_CR_RESTART_AUTO_NEG)) {
> > +
> > + s->phy_reg[PHY_CTRL] = val;
> > +
> > + if (have_autoneg(s)) {
> > e1000_link_down(s);
> > DBGOUT(PHY, "Start link auto negotiation\n");
> > - timer_mod(s->autoneg_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL)
> > + 500);
> > + timer_mod(s->autoneg_timer,
> > + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> > }
> > }
> >
> > @@ -848,14 +857,6 @@ receive_filter(E1000State *s, const uint8_t *buf, int
> > size)
> > return 0;
> > }
> >
> > -static bool
> > -have_autoneg(E1000State *s)
> > -{
> > - return (s->compat_flags & E1000_FLAG_AUTONEG) &&
> > - (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
> > - (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
> > -}
> > -
> > static void
> > e1000_set_link_status(NetClientState *nc)
> > {
> > --
> > 1.9.3
