qemu-ppc
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 5/6] hw/ppc: SPI controller wiring to P10 chip and create

From: Chalapathi V
Subject: Re: [PATCH v2 5/6] hw/ppc: SPI controller wiring to P10 chip and create seeprom device
Date: Wed, 24 Apr 2024 22:42:44 +0530
User-agent: Mozilla Thunderbird 
Hello Cedric,

Thank You for reviewing v2 patches.

Regards,

Chalapathi

On 22-04-2024 20:33, Cédric Le Goater wrote:

On 4/9/24 19:56, Chalapathi V wrote:

In this commit
Creates SPI controller on p10 chip.
Create the keystore seeprom of type "seeprom-25csm04"
Connect the cs of seeprom to PIB_SPIC[2] cs irq.

The QOM tree of spi controller and seeprom are.
/machine (powernv10-machine)
   /chip[0] (power10_v2.0-pnv-chip)
     /pib_spic[2] (pnv-spi-controller)
       /bus (pnv-spi-bus)
         /pnv-spi-bus.2 (SSI)
       /xscom-spi-controller-regs[0] (memory-region)

/machine (powernv10-machine)
   /unattached (container)
     /device[7] (seeprom-25csm04)
       /ssi-gpio-cs[0] (irq)

(qemu) qom-get /machine/unattached/device[7] "parent_bus"
"/machine/chip[0]/pib_spic[2]/bus/pnv-spi-bus.2"

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
  include/hw/ppc/pnv_chip.h |  3 +++
  hw/ppc/pnv.c              | 36 +++++++++++++++++++++++++++++++++++-
  2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
index 8589f3291e..3edf13e8f9 100644
--- a/include/hw/ppc/pnv_chip.h
+++ b/include/hw/ppc/pnv_chip.h
@@ -6,6 +6,7 @@
  #include "hw/ppc/pnv_core.h"
  #include "hw/ppc/pnv_homer.h"
  #include "hw/ppc/pnv_n1_chiplet.h"
+#include "hw/ppc/pnv_spi_controller.h"
  #include "hw/ppc/pnv_lpc.h"
  #include "hw/ppc/pnv_occ.h"
  #include "hw/ppc/pnv_psi.h"
@@ -118,6 +119,8 @@ struct Pnv10Chip {
      PnvSBE       sbe;
      PnvHomer     homer;
      PnvN1Chiplet     n1_chiplet;
+#define PNV10_CHIP_MAX_PIB_SPIC 6
+    PnvSpiController pib_spic[PNV10_CHIP_MAX_PIB_SPIC];
        uint32_t     nr_quads;
      PnvQuad      *quads;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 6e3a5ccdec..eeb2d650bd 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -46,6 +46,7 @@
  #include "hw/pci-host/pnv_phb.h"
  #include "hw/pci-host/pnv_phb3.h"
  #include "hw/pci-host/pnv_phb4.h"
+#include "hw/ssi/ssi.h"
    #include "hw/ppc/xics.h"
  #include "hw/qdev-properties.h"
@@ -1829,6 +1830,11 @@ static void pnv_chip_power10_instance_init(Object *obj) 
      for (i = 0; i < pcc->i2c_num_engines; i++) {
          object_initialize_child(obj, "i2c[*]", &chip10->i2c[i], TYPE_PNV_I2C); 
      }
+
+    for (i = 0; i < PNV10_CHIP_MAX_PIB_SPIC ; i++) {
+        object_initialize_child(obj, "pib_spic[*]", &chip10->pib_spic[i], 
+                                TYPE_PNV_SPI_CONTROLLER);
+    }
  }
    static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp) @@ -2043,7 +2049,35 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp) 
qdev_get_gpio_in(DEVICE(&chip10->psi),
PSIHB9_IRQ_SBE_I2C));
      }
-
+    /* PIB SPI Controller */
+    for (i = 0; i < PNV10_CHIP_MAX_PIB_SPIC; i++) {
+ object_property_set_int(OBJECT(&chip10->pib_spic[i]), "spic_num",
+                                i , &error_fatal);
+        /*
+         * The TPM attached SPIC needs to reverse the bit order in each byte 
+         * it sends to the TPM.
+         */
+        if (i == 4) {
+ object_property_set_bool(OBJECT(&chip10->pib_spic[i]),
+                    "reverse_bits", true, &error_fatal);
+        }


or

object_property_set_bool(OBJECT(&chip10->pib_spic[i]),
                    "reverse_bits", (i == 4) , &error_fatal);


That said. This setting looks weird to me.

Why do we need to reverse the bits ? is it an endian issue ?

Are there other SPI devices on the buses ?

There are no other SPI devices attached to this bus.
Checking about reversing the bits that sent to TPM.



+        if (!qdev_realize(DEVICE(&chip10->pib_spic[i]), NULL, errp)) {
+            return;
+        }
+        pnv_xscom_add_subregion(chip, PNV10_XSCOM_PIB_SPIC_BASE +
+                                i * PNV10_XSCOM_PIB_SPIC_SIZE,
+ &chip10->pib_spic[i].xscom_spic_regs);
+    }
The devices below belong to the rainer machine it seems. We should introduce a per-machine handler to create them like it was done for the I2C devices. 
For this purpose, the PnvMachineClass::i2c_init) handler could be changed
to create all machine specific devices.

ACK, Thank You.



+    /* Primary MEAS/MVPD/Keystore SEEPROM connected to pib_spic[2] */
+    DeviceState *seeprom = qdev_new("seeprom-25csm04");
+    qdev_prop_set_string(seeprom, "filename",
+                         "sbe_measurement_seeprom.bin.ecc");


This should be done differently. Here is a command line example :

$ qemu-system-arm -M ast2600-evb \
      -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \
      -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
      -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \
      -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
      -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
      -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
      ...
Please try to rework "seeprom-25csm04" on top of "m25p80". It should help. 
Sure, Will check and do the updates.
+    ssi_realize_and_unref(seeprom, ((&chip10->pib_spic[2])->bus).ssi_bus, 
+                          &error_fatal);
+    qemu_irq seeprom_cs = qdev_get_gpio_in_named(seeprom, SSI_GPIO_CS, 0); 
+    Object *bus = OBJECT(&(&chip10->pib_spic[2])->bus);
+    sysbus_connect_irq(SYS_BUS_DEVICE(bus), 0, seeprom_cs);


Could you please slightly change the models to connect the IRQ line using
qdev_connect_gpio_out instead ?  See pnv_rainier_i2c_init.

Thanks,

C.

Sure, Will check and update. Thank You.



  }
    static void pnv_rainier_i2c_init(PnvMachineState *pnv)
reply via email to
[Prev in Thread] Current Thread [Next in Thread]