[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] hw/arm/aspeed: Add Fuji machine type
|
From: |
Peter Delevoryas |
|
Subject: |
Re: [PATCH v3] hw/arm/aspeed: Add Fuji machine type |
|
Date: |
Mon, 6 Sep 2021 13:16:09 +0000 |
> On Sep 6, 2021, at 2:15 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 9/5/21 8:55 PM, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>>
>> This adds a new machine type "fuji-bmc" based on the following device tree:
>>
>> https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
>
> Sorry for being picky, but 'master' is a branch, not a (fixed) tag.
> Since there is no tag released with this file, please point to the
> commit introducing the file:
>
> https://github.com/torvalds/linux/blob/40cb6373b46/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
Oh, that’s ok, I can resend this patch with these links.
>
>> Most of the i2c devices are not there, they're added here:
>>
>> https://github.com/facebook/openbmc/blob/helium/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh
>
> Similarly:
>
> https://github.com/facebook/openbmc/blob/fb2ed12002fb/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh
>
> (note the nice fb*fb SHA-1 :P)
! What are the odds??
>
>> I tested this by building a Fuji image from Facebook's OpenBMC repo,
>> booting, and ssh'ing from host-to-guest.
>>
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> hw/arm/aspeed.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 112 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 7a9459340c..cc2d721ac7 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -159,6 +159,10 @@ struct AspeedMachineState {
>> #define RAINIER_BMC_HW_STRAP1 0x00000000
>> #define RAINIER_BMC_HW_STRAP2 0x00000000
>>
>> +/* Fuji hardware value */
>> +#define FUJI_BMC_HW_STRAP1 0x00000000
>> +#define FUJI_BMC_HW_STRAP2 0x00000000
>> +
>> /*
>> * The max ram region is for firmwares that scan the address space
>> * with load/store to guess how much RAM the SoC has.
>> @@ -772,6 +776,90 @@ static void rainier_bmc_i2c_init(AspeedMachineState
>> *bmc)
>> aspeed_eeprom_init(aspeed_i2c_get_bus(&soc->i2c, 15), 0x50, 64 * KiB);
>> }
>>
>> +static void get_pca9548_channels(I2CBus *bus, uint8_t mux_addr, I2CBus
>> **channels) {
>
> Per https://wiki.qemu.org/Contribute/SubmitAPatch#Use_the_QEMU_coding_style
Oh yeah, I noticed Cedric mentioned he had to do this, sorry!! I remember
reading through this wiki page, and that there was a checkpatch script,
but then I forgot to actually run it. I’ll make sure to do that in the
future.
>
> static void get_pca9548_channels(I2CBus *bus, uint8_t mux_addr,
> I2CBus **channels)
> {
>
>> + I2CSlave *mux = i2c_slave_create_simple(bus, "pca9548", mux_addr);
>> + for (int i = 0; i < 8; i++) {
>> + channels[i] = pca954x_i2c_get_bus(mux, i);
>> + }
>> +}
>> +
>> +#define TYPE_LM75 TYPE_TMP105
>> +#define TYPE_TMP75 TYPE_TMP105
>> +#define TYPE_TMP422 "tmp422"
>> +
>> +static void fuji_bmc_i2c_init(AspeedMachineState *bmc)
>> +{
>> + AspeedSoCState *soc = &bmc->soc;
>> + I2CBus *i2c[144] = {};
>> +
>> + for (int i = 0; i < 16; i++) {
>> + i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
>> + }
>> + I2CBus *i2c180 = i2c[2];
>> + I2CBus *i2c480 = i2c[8];
>> + I2CBus *i2c600 = i2c[11];
>> +
>> + get_pca9548_channels(i2c180, 0x70, &i2c[16]);
>> + get_pca9548_channels(i2c480, 0x70, &i2c[24]);
>
> QEMU style:
>
> /*
>
>> + // NOTE: The device tree skips [32, 40) in the alias numbering, so we do
>> + // the same here.
>
> */
>
>> + get_pca9548_channels(i2c600, 0x77, &i2c[40]);
>> + get_pca9548_channels(i2c[24], 0x71, &i2c[48]);
>> + get_pca9548_channels(i2c[25], 0x72, &i2c[56]);
>> + get_pca9548_channels(i2c[26], 0x76, &i2c[64]);
>> + get_pca9548_channels(i2c[27], 0x76, &i2c[72]);
>> + for (int i = 0; i < 8; i++) {
>> + get_pca9548_channels(i2c[40 + i], 0x76, &i2c[80 + i * 8]);
>> + }
>> +
>> + i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
>> + i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4d);
>> +
>> + aspeed_eeprom_init(i2c[19], 0x52, 64 * KiB);
>> + aspeed_eeprom_init(i2c[20], 0x50, 2 * KiB);
>> + aspeed_eeprom_init(i2c[22], 0x52, 2 * KiB);
>> +
>> + i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x48);
>> + i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x49);
>> + i2c_slave_create_simple(i2c[3], TYPE_LM75, 0x4a);
>> + i2c_slave_create_simple(i2c[3], TYPE_TMP422, 0x4c);
>> +
>> + aspeed_eeprom_init(i2c[8], 0x51, 64 * KiB);
>> + i2c_slave_create_simple(i2c[8], TYPE_LM75, 0x4a);
>> +
>> + i2c_slave_create_simple(i2c[50], TYPE_LM75, 0x4c);
>> + aspeed_eeprom_init(i2c[50], 0x52, 64 * KiB);
>> + i2c_slave_create_simple(i2c[51], TYPE_TMP75, 0x48);
>> + i2c_slave_create_simple(i2c[52], TYPE_TMP75, 0x49);
>> +
>> + i2c_slave_create_simple(i2c[59], TYPE_TMP75, 0x48);
>> + i2c_slave_create_simple(i2c[60], TYPE_TMP75, 0x49);
>> +
>> + aspeed_eeprom_init(i2c[65], 0x53, 64 * KiB);
>> + i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x49);
>> + i2c_slave_create_simple(i2c[66], TYPE_TMP75, 0x48);
>> + aspeed_eeprom_init(i2c[68], 0x52, 64 * KiB);
>> + aspeed_eeprom_init(i2c[69], 0x52, 64 * KiB);
>> + aspeed_eeprom_init(i2c[70], 0x52, 64 * KiB);
>> + aspeed_eeprom_init(i2c[71], 0x52, 64 * KiB);
>> +
>> + aspeed_eeprom_init(i2c[73], 0x53, 64 * KiB);
>> + i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x49);
>> + i2c_slave_create_simple(i2c[74], TYPE_TMP75, 0x48);
>> + aspeed_eeprom_init(i2c[76], 0x52, 64 * KiB);
>> + aspeed_eeprom_init(i2c[77], 0x52, 64 * KiB);
>> + aspeed_eeprom_init(i2c[78], 0x52, 64 * KiB);
>> + aspeed_eeprom_init(i2c[79], 0x52, 64 * KiB);
>> + aspeed_eeprom_init(i2c[28], 0x50, 2 * KiB);
>> +
>> + for (int i = 0; i < 8; i++) {
>> + aspeed_eeprom_init(i2c[81 + i * 8], 0x56, 64 * KiB);
>> + i2c_slave_create_simple(i2c[82 + i * 8], TYPE_TMP75, 0x48);
>> + i2c_slave_create_simple(i2c[83 + i * 8], TYPE_TMP75, 0x4b);
>> + i2c_slave_create_simple(i2c[84 + i * 8], TYPE_TMP75, 0x4a);
>> + }
>> +}
>> +
>> static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
>> {
>> return ASPEED_MACHINE(obj)->mmio_exec;
>> @@ -1070,6 +1158,26 @@ static void
>> aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
>> aspeed_soc_num_cpus(amc->soc_name);
>> };
>>
>> +static void aspeed_machine_fuji_class_init(ObjectClass *oc, void *data)
>> +{
>> + MachineClass *mc = MACHINE_CLASS(oc);
>> + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>> +
>> + mc->desc = "Facebook Fuji BMC (Cortex-A7)";
>> + amc->soc_name = "ast2600-a3";
>> + amc->hw_strap1 = FUJI_BMC_HW_STRAP1;
>> + amc->hw_strap2 = FUJI_BMC_HW_STRAP2;
>> + amc->fmc_model = "mx66l1g45g";
>> + amc->spi_model = "mx66l1g45g";
>> + amc->num_cs = 2;
>> + amc->macs_mask = ASPEED_MAC3_ON;
>> + amc->i2c_init = fuji_bmc_i2c_init;
>> + amc->uart_default = ASPEED_DEV_UART1;
>> + mc->default_ram_size = 2 * GiB;
>> + mc->default_cpus = mc->min_cpus = mc->max_cpus =
>> + aspeed_soc_num_cpus(amc->soc_name);
>
> Matter of taste:
>
> mc->default_cpus = mc->min_cpus = mc->max_cpus
> = aspeed_soc_num_cpus(amc->soc_name);
I actually like that better too, but I think the rest of the file
does it this way, so I’ll just leave it as-is to be consistent
I suppose.
>
>> +};
>> +
>> static const TypeInfo aspeed_machine_types[] = {
>> {
>> .name = MACHINE_TYPE_NAME("palmetto-bmc"),
>> @@ -1119,6 +1227,10 @@ static const TypeInfo aspeed_machine_types[] = {
>> .name = MACHINE_TYPE_NAME("rainier-bmc"),
>> .parent = TYPE_ASPEED_MACHINE,
>> .class_init = aspeed_machine_rainier_class_init,
>> + }, {
>> + .name = MACHINE_TYPE_NAME("fuji-bmc"),
>> + .parent = TYPE_ASPEED_MACHINE,
>> + .class_init = aspeed_machine_fuji_class_init,
>> }, {
>> .name = TYPE_ASPEED_MACHINE,
>> .parent = TYPE_MACHINE,