Re: [Qemu-devel] [PATCH] e1000: Fix wrong microwire EEPROM state initialization

[Aurelien Jarno]

On Sat, Jul 10, 2010 at 11:03:45PM +0900, Izumi Tsutsui wrote:
> This change fixes initialization of e1000's microwire EEPROM internal
> state values so that qemu's e1000 emulation works on NetBSD,
> which doesn't use Intel's em driver but has its own wm driver
> for the Intel i8254x Gigabit Ethernet.
> 
> Previously set_eecd() function in e1000.c clears EEPROM internal state
> values on SK rising edge during CS==L, but according to FM93C06 EEPROM
> (which is MicroWire compatible) data sheet, EEPROM internal status
> should be cleared on CS rise edge regardless of SK input:
>  "... a rising edge on this (CS) signal is required to reset the internal
>   state-machine to accept a new cycle .."
> and nothing should be changed during CS (chip select) is inactive.
> 
> Intel's em driver seems to explicitly raise SK output after CS is negated
> in em_standby_eeprom() so many other OSes that use Intel's driver
> don't have this problem even on the previous e1000.c implementation,
> but I can't find any articles that say the MICROWIRE or EEPROM spec
> requires such sequence, and actually hardware works fine without it
> (i.e. real i82540EM has been working on NetBSD).
> 
> This fix also changes initialization to clear each state value in
> struct eecd_state individually rather than using memset() against
> the whole structre. The old_eecd member stores the last SK and CS
> signal levels and it should be preserved even after reset of internal
> EEPROM state to detect next signal edges for proper EEPROM emulation.
> 
> Signed-off-by: Izumi Tsutsui <address@hidden>
> ---
>  hw/e1000.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)

Thanks, applied.

> diff --git a/hw/e1000.c b/hw/e1000.c
> index 0da65f9..db9143d 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -262,21 +262,20 @@ set_eecd(E1000State *s, int index, uint32_t val)
>  
>      s->eecd_state.old_eecd = val & (E1000_EECD_SK | E1000_EECD_CS |
>              E1000_EECD_DI|E1000_EECD_FWE_MASK|E1000_EECD_REQ);
> +    if (!(E1000_EECD_CS & val))                      // CS inactive; nothing 
> to do
> +     return;
> +    if (E1000_EECD_CS & (val ^ oldval)) {    // CS rise edge; reset state
> +     s->eecd_state.val_in = 0;
> +     s->eecd_state.bitnum_in = 0;
> +     s->eecd_state.bitnum_out = 0;
> +     s->eecd_state.reading = 0;
> +    }
>      if (!(E1000_EECD_SK & (val ^ oldval)))   // no clock edge
>          return;
>      if (!(E1000_EECD_SK & val)) {            // falling edge
>          s->eecd_state.bitnum_out++;
>          return;
>      }
> -    if (!(val & E1000_EECD_CS)) {            // rising, no CS (EEPROM reset)
> -        memset(&s->eecd_state, 0, sizeof s->eecd_state);
> -        /*
> -         * restore old_eecd's E1000_EECD_SK (known to be on)
> -         * to avoid false detection of a clock edge
> -         */
> -        s->eecd_state.old_eecd = E1000_EECD_SK;
> -        return;
> -    }
>      s->eecd_state.val_in <<= 1;
>      if (val & E1000_EECD_DI)
>          s->eecd_state.val_in |= 1;
> -- 
> 1.6.6.2
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net