Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket

[Cédric Le Goater]

On 2/28/25 04:03, Chalapathi V wrote:

On 27-02-2025 07:24, Nicholas Piggin wrote:

On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:

Create a spi buses with distict names on each socket so that responders
are attached to correct SPI controllers.

QOM tree on a 2 socket machine:
(qemu) info qom-tree
/machine (powernv10-machine)
   /chip[0] (power10_v2.0-pnv-chip)
     /pib_spic[0] (pnv-spi)
       /chip0.pnv.spi.bus.0 (SSI)
       /xscom-spi[0] (memory-region)
   /chip[1] (power10_v2.0-pnv-chip)
     /pib_spic[0] (pnv-spi)
       /chip1.pnv.spi.bus.0 (SSI)
       /xscom-spi[0] (memory-region)

Mechanics of the patch looks fine. I don't know about the name
though.

I think "pnv-spi-bus" is the right name for the bus. Using dots as
with chip0. makes it seem like each element is part of a topology.

Would chip0.pnv-spi-bus be better?

Will rename the bus name to chip0.pnv-spi-bus . Thank You

Yep. I don't think the bus suffix is useful (minor).

Will you be attaching flash devices from the command line ? Can you provide
an example if so ?

Thanks,

C.

I don't suppose there is a good way to create an alias so existing
cmdline works and refers to the bus on chip0? Maybe the chip0 bus
could just not have the chip0. prefix?

Thanks,
Nick

Would it be best to keep the chip0 prefix to have uniformity?

Signed-off-by: Chalapathi V<chalapath...@linux.ibm.com>
---
  include/hw/ssi/pnv_spi.h           | 3 ++-
  hw/ppc/pnv.c                       | 2 ++
  hw/ssi/pnv_spi.c                   | 5 +++--
  tests/qtest/pnv-spi-seeprom-test.c | 2 +-
  4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
index 9878d9a25f..7fc5da1f84 100644
--- a/include/hw/ssi/pnv_spi.h
+++ b/include/hw/ssi/pnv_spi.h
@@ -31,7 +31,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PnvSpi, PNV_SPI)
  #define PNV_SPI_REG_SIZE 8
  #define PNV_SPI_REGS 7

-#define TYPE_PNV_SPI_BUS "pnv-spi-bus"

+#define TYPE_PNV_SPI_BUS "pnv.spi.bus"
  typedef struct PnvSpi {
      SysBusDevice parent_obj;

@@ -42,6 +42,7 @@ typedef struct PnvSpi {

      Fifo8 rx_fifo;
      /* SPI object number */
      uint32_t        spic_num;
+    uint32_t        chip_id;
      uint8_t         transfer_len;
      uint8_t         responder_select;
      /* To verify if shift_n1 happens prior to shift_n2 */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 11fd477b71..ce23892fdf 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -2226,6 +2226,8 @@ static void pnv_chip_power10_realize(DeviceState *dev, 
Error **errp)
          /* pib_spic[2] connected to 25csm04 which implements 1 byte transfer 
*/
          object_property_set_int(OBJECT(&chip10->pib_spic[i]), "transfer_len",
                                  (i == 2) ? 1 : 4, &error_fatal);
+        object_property_set_int(OBJECT(&chip10->pib_spic[i]), "chip-id",
+                                chip->chip_id, &error_fatal);
          if (!sysbus_realize(SYS_BUS_DEVICE(OBJECT
                                          (&chip10->pib_spic[i])), errp)) {
              return;
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 87eac666bb..41beb559c6 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -1116,14 +1116,15 @@ static const MemoryRegionOps pnv_spi_xscom_ops = {

static const Property pnv_spi_properties[] = {

      DEFINE_PROP_UINT32("spic_num", PnvSpi, spic_num, 0),
+    DEFINE_PROP_UINT32("chip-id", PnvSpi, chip_id, 0),
      DEFINE_PROP_UINT8("transfer_len", PnvSpi, transfer_len, 4),
  };

static void pnv_spi_realize(DeviceState *dev, Error **errp)

  {
      PnvSpi *s = PNV_SPI(dev);
-    g_autofree char *name = g_strdup_printf(TYPE_PNV_SPI_BUS ".%d",
-                    s->spic_num);
+    g_autofree char *name = g_strdup_printf("chip%d." TYPE_PNV_SPI_BUS ".%d",
+                    s->chip_id, s->spic_num);
      s->ssi_bus = ssi_create_bus(dev, name);
      s->cs_line = g_new0(qemu_irq, 1);
      qdev_init_gpio_out_named(DEVICE(s), s->cs_line, "cs", 1);
diff --git a/tests/qtest/pnv-spi-seeprom-test.c 
b/tests/qtest/pnv-spi-seeprom-test.c
index 57f20af76e..ef1005a926 100644
--- a/tests/qtest/pnv-spi-seeprom-test.c
+++ b/tests/qtest/pnv-spi-seeprom-test.c
@@ -92,7 +92,7 @@ static void test_spi_seeprom(const void *data)
      qts = qtest_initf("-machine powernv10 -smp 2,cores=2,"
                        "threads=1 -accel tcg,thread=single -nographic "
                        "-blockdev node-name=pib_spic2,driver=file,"
-                      "filename=%s -device 25csm04,bus=pnv-spi-bus.2,cs=0,"
+                      "filename=%s -device 
25csm04,bus=chip0.pnv.spi.bus.2,cs=0,"
                        "drive=pib_spic2", tmp_path);
      spi_seeprom_transaction(qts, chip);
      qtest_quit(qts);