1. Patchew
  2. QEMU
  3. [PATCH v5 0/4] hw/ssi/pnv_spi: Remove PnvXferBuffer and fix CID 1558827
  4. View patch

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

Chalapathi V posted 4 patches 3 months ago
[PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket
Posted by Chalapathi V 3 months ago 
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)

Signed-off-by: Chalapathi V <chalapathi.v@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);
-- 
2.39.5
Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket
Posted by Nicholas Piggin 1 month, 1 week ago 
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?

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

>
> Signed-off-by: Chalapathi V <chalapathi.v@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);
Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket
Posted by Chalapathi V 1 month, 1 week ago 
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
>
> 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<chalapathi.v@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);
Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket
Posted by Cédric Le Goater 1 month, 1 week ago 
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<chalapathi.v@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);
Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket
Posted by Chalapathi V 1 month, 1 week ago 
On 28-02-2025 13:15, Cédric Le Goater wrote:
> 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.
>
Yes, I am attaching seeprom and TPM device from command line.
"-blockdev node-name=pib_spic2,driver=file,filename=%s -device 
25csm04,bus=chip0.pnv-spi-bus.2,cs=0,drive=pib_spic2"

"-chardev 
socket,id=chrtpm,path=/tmp/swtpm.chalap1.250227212039.rOWBH/swtpm-sock 
-tpmdev emulator,id=tpm0,chardev=chrtpm -device 
tpm-tis-spi,tpmdev=tpm0,bus=chip0.pnv-spi-bus.4"

Thank You,

Chalapathi

>
>>> 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<chalapathi.v@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);
>
Re: [PATCH v5 3/4] hw/ssi/pnv_spi: Make bus names distinct for each controllers of a socket
Posted by Cédric Le Goater 1 month, 1 week ago 
On 2/28/25 12:00, Chalapathi V wrote:
> 
> On 28-02-2025 13:15, Cédric Le Goater wrote:
>> 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.
>>
> Yes, I am attaching seeprom and TPM device from command line.
> "-blockdev node-name=pib_spic2,driver=file,filename=%s -device 25csm04,bus=chip0.pnv-spi-bus.2,cs=0,drive=pib_spic2"
> 
> "-chardev socket,id=chrtpm,path=/tmp/swtpm.chalap1.250227212039.rOWBH/swtpm-sock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-tis-spi,tpmdev=tpm0,bus=chip0.pnv-spi-bus.4"
  
"chipX.spi.4" should be enough to identify the bus IMO.

Thanks,

C.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.