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?
Wrt to patch itself, let's do it properly pls:
- bits 0:5 are RO on some cards
- bit 9 is SC
> ---
>
> 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
