Message ID | 34caf9dea82cd30dc51b4f8768adb7a90b236bf2.1579474761.git.fthain@telegraphics.com.au |
---|---|
State | New |
Headers | show |
Series | Fixes for DP8393X SONIC device emulation | expand |
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); ---
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. 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.
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. >
On Wed, 29 Jan 2020, Philippe Mathieu-Daudé wrote: > > > > 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; > My mistake. I had completely overlooked that logic. I'll revise this patch in accordance with your suggestion.
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);
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(-)