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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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 <fthain@telegraphics.com.au>
>>> ---
>>>    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.
>