| Message ID | 20210710174954.2577195-4-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | dp8393x: fixes and improvements | expand |
On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote: > The SONIC device only allows 16/32-bit accesses. From the machine > view (from the bus), it is only shifted by 1 bit. Another bit is > shifted, but it is an implementation detail of the QEMU model. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/m68k/q800.c | 2 +- > hw/mips/jazz.c | 2 +- > hw/net/dp8393x.c | 2 ++ > 3 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c > index 6817c8b5d1a..d78edfe40e8 100644 > --- a/hw/m68k/q800.c > +++ b/hw/m68k/q800.c > @@ -316,7 +316,7 @@ static void q800_init(MachineState *machine) > > dev = qdev_new("dp8393x"); > qdev_set_nic_properties(dev, &nd_table[0]); > - qdev_prop_set_uint8(dev, "it_shift", 2); > + qdev_prop_set_uint8(dev, "it_shift", 1); > qdev_prop_set_bit(dev, "big_endian", true); > object_property_set_link(OBJECT(dev), "dma_mr", > OBJECT(get_system_memory()), &error_abort); > diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c > index d6183e18821..7f8680a189d 100644 > --- a/hw/mips/jazz.c > +++ b/hw/mips/jazz.c > @@ -295,7 +295,7 @@ static void mips_jazz_init(MachineState *machine, > > dev = qdev_new("dp8393x"); > qdev_set_nic_properties(dev, nd); > - qdev_prop_set_uint8(dev, "it_shift", 2); > + qdev_prop_set_uint8(dev, "it_shift", 1); > qdev_prop_set_bit(dev, "big_endian", big_endian > 0); > object_property_set_link(OBJECT(dev), "dma_mr", > OBJECT(rc4030_dma_mr), &error_abort); > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > index d1e147a82a6..64f3b0fc3ea 100644 > --- a/hw/net/dp8393x.c > +++ b/hw/net/dp8393x.c > @@ -971,6 +971,8 @@ static void dp8393x_realize(DeviceState *dev, Error **errp) > { > dp8393xState *s = DP8393X(dev); > > + s->it_shift += 1; /* Registers are 16-bit wide */ > + > address_space_init(&s->as, s->dma_mr, "dp8393x"); > memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s, > "dp8393x-regs", SONIC_REG_COUNT << s->it_shift); This patch looks wrong to me: by convention it_shift is used to implement the register stride, so by reducing this from 2 to 1 you've immediately broken the POLA regarding the it_shift property. There could be an argument that there is an intermediate bus that could be modelled for 16-bit accesses here, but this is worked around (and clearly commented) in patch 8. ATB, Mark.
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c index 6817c8b5d1a..d78edfe40e8 100644 --- a/hw/m68k/q800.c +++ b/hw/m68k/q800.c @@ -316,7 +316,7 @@ static void q800_init(MachineState *machine) dev = qdev_new("dp8393x"); qdev_set_nic_properties(dev, &nd_table[0]); - qdev_prop_set_uint8(dev, "it_shift", 2); + qdev_prop_set_uint8(dev, "it_shift", 1); qdev_prop_set_bit(dev, "big_endian", true); object_property_set_link(OBJECT(dev), "dma_mr", OBJECT(get_system_memory()), &error_abort); diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c index d6183e18821..7f8680a189d 100644 --- a/hw/mips/jazz.c +++ b/hw/mips/jazz.c @@ -295,7 +295,7 @@ static void mips_jazz_init(MachineState *machine, dev = qdev_new("dp8393x"); qdev_set_nic_properties(dev, nd); - qdev_prop_set_uint8(dev, "it_shift", 2); + qdev_prop_set_uint8(dev, "it_shift", 1); qdev_prop_set_bit(dev, "big_endian", big_endian > 0); object_property_set_link(OBJECT(dev), "dma_mr", OBJECT(rc4030_dma_mr), &error_abort); diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index d1e147a82a6..64f3b0fc3ea 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -971,6 +971,8 @@ static void dp8393x_realize(DeviceState *dev, Error **errp) { dp8393xState *s = DP8393X(dev); + s->it_shift += 1; /* Registers are 16-bit wide */ + address_space_init(&s->as, s->dma_mr, "dp8393x"); memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s, "dp8393x-regs", SONIC_REG_COUNT << s->it_shift);
The SONIC device only allows 16/32-bit accesses. From the machine view (from the bus), it is only shifted by 1 bit. Another bit is shifted, but it is an implementation detail of the QEMU model. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/m68k/q800.c | 2 +- hw/mips/jazz.c | 2 +- hw/net/dp8393x.c | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-)