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.