| Message ID | 20240409175700.27535-6-chalapathi.v@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/ppc: SPI model | expand |
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 ? > + 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. > + /* 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. > + 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. > } > > static void pnv_rainier_i2c_init(PnvMachineState *pnv)
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) >
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); + } + 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); + } + + /* 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"); + 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); } static void pnv_rainier_i2c_init(PnvMachineState *pnv)
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(-)