Re: [PATCH v2 07/12] hw/ssi: Check for duplicate addresses

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> This to avoid address conflicts on the same SSI bus. Adapt machines
> using multiple devices on the same bus to avoid breakage.
>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

One small suggestion below that we could do as a follow up.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/arm/stellaris.c                  |  4 +++-
>  hw/arm/xilinx_zynq.c                |  1 +
>  hw/arm/xlnx-versal-virt.c           |  1 +
>  hw/arm/xlnx-zcu102.c                |  2 ++
>  hw/microblaze/petalogix_ml605_mmu.c |  1 +
>  hw/ssi/ssi.c                        | 21 +++++++++++++++++++++
>  6 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index f7e99baf6236..6744571d55f4 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1242,7 +1242,9 @@ static void stellaris_init(MachineState *ms, 
> stellaris_board_info *board)
>                                     qdev_get_child_bus(sddev, "sd-bus"),
>                                     &error_fatal);
>
> -            ssddev = ssi_create_peripheral(bus, "ssd0323");

Should we update ssi_create_peripheral to make the chip select explicit?

> +            ssddev = qdev_new("ssd0323");
> +            qdev_prop_set_uint8(ssddev, "addr", 1);
> +            qdev_realize_and_unref(ssddev, bus, &error_fatal);
>
>              gpio_d_splitter = qdev_new(TYPE_SPLIT_IRQ);
>              qdev_prop_set_uint32(gpio_d_splitter, "num-lines", 2);
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 3190cc0b8dbc..28e9df684213 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -164,6 +164,7 @@ static inline int zynq_init_spi_flashes(uint32_t 
> base_addr, qemu_irq irq,
>                                          blk_by_legacy_dinfo(dinfo),
>                                          &error_fatal);
>              }
> +            qdev_prop_set_uint8(flash_dev, "addr", j);
>              qdev_realize_and_unref(flash_dev, BUS(spi), &error_fatal);
>
>              cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 668a9d65a437..c90345375090 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -701,6 +701,7 @@ static void versal_virt_init(MachineState *machine)
>              qdev_prop_set_drive_err(flash_dev, "drive",
>                                      blk_by_legacy_dinfo(dinfo), 
> &error_fatal);
>          }
> +        qdev_prop_set_uint8(flash_dev, "addr", i);
>          qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
>
>          cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 4c84bb932aa0..6224b8fc1110 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -201,6 +201,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>              qdev_prop_set_drive_err(flash_dev, "drive",
>                                      blk_by_legacy_dinfo(dinfo), 
> &error_fatal);
>          }
> +        qdev_prop_set_uint8(flash_dev, "addr", i);
>          qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
>
>          cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> @@ -224,6 +225,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>              qdev_prop_set_drive_err(flash_dev, "drive",
>                                      blk_by_legacy_dinfo(dinfo), 
> &error_fatal);
>          }
> +        qdev_prop_set_uint8(flash_dev, "addr", i);
>          qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
>
>          cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
> b/hw/microblaze/petalogix_ml605_mmu.c
> index a24fadddcac0..4c5e4510c333 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -192,6 +192,7 @@ petalogix_ml605_init(MachineState *machine)
>                                          blk_by_legacy_dinfo(dinfo),
>                                          &error_fatal);
>              }
> +            qdev_prop_set_uint8(dev, "addr", i);
>              qdev_realize_and_unref(dev, BUS(spi), &error_fatal);
>
>              cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index 7c71fce0db90..aa0bfa57bb26 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -42,10 +42,31 @@ DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr)
>      return NULL;
>  }
>
> +static bool ssi_bus_check_address(BusState *b, DeviceState *dev, Error 
> **errp)
> +{
> +    SSIPeripheral *s = SSI_PERIPHERAL(dev);
> +
> +    if (ssi_get_cs(SSI_BUS(b), s->addr)) {
> +        error_setg(errp, "addr '0x%x' in use by a %s device", s->addr,
> +                   object_get_typename(OBJECT(dev)));
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static void ssi_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    BusClass *k = BUS_CLASS(klass);
> +
> +    k->check_address = ssi_bus_check_address;
> +}
> +
>  static const TypeInfo ssi_bus_info = {
>      .name = TYPE_SSI_BUS,
>      .parent = TYPE_BUS,
>      .instance_size = sizeof(SSIBus),
> +    .class_init = ssi_bus_class_init,
>  };
>
>  static void ssi_cs_default(void *opaque, int n, int level)
> --
> 2.40.1
>