Re: [PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register

[Finn Thain]

On Tue, 28 Jan 2020, Philippe Mathieu-Daud? wrote:

> On 1/19/20 11:59 PM, Finn Thain wrote:
> > The jazzsonic driver in Linux uses the Silicon Revision register value
> > to probe the chip. The driver fails unless the SR register contains 4.
> > Unfortunately, reading this register in QEMU usually returns 0 because
> > the s->regs[] array gets wiped after a software reset.
> > 
> > Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
> > Signed-off-by: Finn Thain <address@hidden>
> > ---
> >   hw/net/dp8393x.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index 1b73a8703b..71af0fad51 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
> > unsigned int size)
> >                   val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 -
> > reg)];
> >               }
> >               break;
> > +        /* Read-only */
> > +        case SONIC_SR:
> > +            val = 4; /* only revision recognized by Linux/mips */
> > +            break;
> >           /* All other registers have no special contrainst */
> >           default:
> >               val = s->regs[reg];
> > @@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error
> > **errp)
> >       qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> >         s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> > -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> >         memory_region_init_ram(&s->prom, OBJECT(dev),
> >                              "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> > 
> 
> Please fix in dp8393x_reset() instead:
> 
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index cdc2631c0c..65eb9c23a7 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -862,6 +862,7 @@ static void dp8393x_reset(DeviceState *dev)
>      timer_del(s->watchdog);
> 
>      memset(s->regs, 0, sizeof(s->regs));
> +    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>      s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
>      s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
>      s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | SONIC_RCR_BRD |
> SONIC_RCR_RNT);
> @@ -914,7 +915,6 @@ static void dp8393x_realize(DeviceState *dev, Error
> **errp)
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> 
>      s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> 
>      memory_region_init_ram(&s->prom, OBJECT(dev),
>                             "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> ---
> 

This would allow the host to change the value of the Silicon Revision 
register. However, the datasheet says,

    4.3.13 Silicon Revision Register
    This is a 16-bit read only register. It contains information on the 
    current revision of the SONIC. The value of the DP83932CVF revision 
    register is 6h.

I haven't actually tried storing a different value in this register on 
National Semiconductor hardware, but I'm willing to do that test if you 
wish.