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

[Philippe Mathieu-Daudé]

Hi Finn,

On 1/28/20 11:28 PM, Finn Thain wrote:
> 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.
How the guest can modify it? We have:

589 static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
590                           unsigned int size)
591 {
592     dp8393xState *s = opaque;
593     int reg = addr >> s->it_shift;
594
...
597     switch (reg) {
...
602         /* Prevent write to read-only registers */
...
606         case SONIC_SR:
...
608             DPRINTF("writing to reg %d invalid\n", reg);
609             break;

> 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.