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

[Chalapathi V <chalapathi.v@linux.ibm.com>]

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)
>