Re: [Qemu-arm] [PATCH v2 1/6] aspeed: add support for the witherspoon-bmc board

[Cédric Le Goater]

On 10/06/2017 05:10 PM, Peter Maydell wrote:
> On 20 September 2017 at 08:01, Cédric Le Goater <address@hidden> wrote:
>> The Witherspoon boards are OpenPOWER system hosting POWER9 Processors.
>> Let's add support for their BMC including a couple of I2C devices as
>> found on real HW.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/arm/aspeed.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index ab895ad490af..81f522f711ae 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -46,6 +46,7 @@ enum {
>>      PALMETTO_BMC,
>>      AST2500_EVB,
>>      ROMULUS_BMC,
>> +    WITHERSPOON_BMC,
>>  };
>>
>>  /* Palmetto hardware value: 0x120CE416 */
>> @@ -83,8 +84,12 @@ enum {
>>          SCU_AST2500_HW_STRAP_ACPI_ENABLE |                              \
>>          SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_MASTER))
>>
>> +/* Witherspoon hardware value: 0xF10AD216 (but use romulus definition) */
>> +#define WITHERSPOON_BMC_HW_STRAP1 ROMULUS_BMC_HW_STRAP1
>> +
>>  static void palmetto_bmc_i2c_init(AspeedBoardState *bmc);
>>  static void ast2500_evb_i2c_init(AspeedBoardState *bmc);
>> +static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc);
>>
>>  static const AspeedBoardConfig aspeed_boards[] = {
>>      [PALMETTO_BMC] = {
>> @@ -110,6 +115,14 @@ static const AspeedBoardConfig aspeed_boards[] = {
>>          .spi_model = "mx66l1g45g",
>>          .num_cs    = 2,
>>      },
>> +    [WITHERSPOON_BMC]  = {
>> +        .soc_name  = "ast2500-a1",
>> +        .hw_strap1 = WITHERSPOON_BMC_HW_STRAP1,
>> +        .fmc_model = "mx25l25635e",
>> +        .spi_model = "mx66l1g45g",
>> +        .num_cs    = 2,
>> +        .i2c_init  = witherspoon_bmc_i2c_init,
>> +    },
>>  };
>>
>>  #define FIRMWARE_ADDR 0x0
>> @@ -337,11 +350,47 @@ static const TypeInfo romulus_bmc_type = {
>>      .class_init = romulus_bmc_class_init,
>>  };
>>
>> +static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>> +{
>> +    AspeedSoCState *soc = &bmc->soc;
>> +
>> +    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 4), "tmp423", 
>> 0x4c);
>> +    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 5), "tmp423", 
>> 0x4c);
>> +
>> +    i2c_create_slave(aspeed_i2c_get_bus(DEVICE(&soc->i2c), 9), "tmp105", 
>> 0x4a);
>> +}
>> +
>> +static void witherspoon_bmc_init(MachineState *machine)
>> +{
>> +    aspeed_board_init(machine, &aspeed_boards[WITHERSPOON_BMC]);
>> +}
>> +
>> +static void witherspoon_bmc_class_init(ObjectClass *oc, void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +
>> +    mc->desc = "OpenPOWER Witherspoon BMC (ARM1176)";
>> +    mc->init = witherspoon_bmc_init;
>> +    mc->max_cpus = 1;
>> +    mc->no_sdcard = 1;
>> +    mc->no_floppy = 1;
>> +    mc->no_cdrom = 1;
>> +    mc->no_parallel = 1;
>> +    mc->ignore_memory_transaction_failures = true;
> 
> Please don't set this flag for new board models, it is only
> for our legacy existing ones. Instead implement any devices
> that you need for guest code to boot (stub them out with
> create_unimplemented_device() if you like).

I have dug into this a little more and it seems that it is 
required for the Aspeed bootloader (a modified u-boot) which 
uses static variables in early init phases. So legacy firmwares 
won't work in QEMU but will on real HW. 

It's fine with me but what is the goal of the approach ? Force 
SoC providers into fixing their FW when they use QEMU ?

Thanks,

C.