[v4] hw/arm/aspeed: Add Fuji machine type

[PATCH v4] hw/arm/aspeed: Add Fuji machine type

Posted by pdel@fb.com 2 years, 7 months ago

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/40cb6373b46/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts

Most of the i2c devices are not there, they're added here:

https://github.com/facebook/openbmc/blob/fb2ed12002fb/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh

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 | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 7a9459340c..95ce4b1670 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,91 @@ 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)
+{
+    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]);
+    /* NOTE: The device tree skips [32, 40) in the alias numbering */
+    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 +1159,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);
+};
+
 static const TypeInfo aspeed_machine_types[] = {
     {
         .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
@@ -1119,6 +1228,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,
-- 
2.30.2

Re: [PATCH v4] hw/arm/aspeed: Add Fuji machine type

Posted by Joel Stanley 2 years, 7 months ago

On Mon, 6 Sept 2021 at 13:31, <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/40cb6373b46/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
>
> Most of the i2c devices are not there, they're added here:
>
> https://github.com/facebook/openbmc/blob/fb2ed12002fb/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh
>
> 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>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> +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]);

Wow, this is interesting. How did you go about testing it? Are you
sure you didn't overwrite any of the pointers?

It might be worth coming up with a better way of describing all of the
i2c buses for future machines.

Cheers,

Joel

> +    get_pca9548_channels(i2c480, 0x70, &i2c[24]);
> +    /* NOTE: The device tree skips [32, 40) in the alias numbering */
> +    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]);
> +    }

Re: [PATCH v4] hw/arm/aspeed: Add Fuji machine type

Posted by Peter Delevoryas 2 years, 7 months ago


> On Sep 7, 2021, at 1:59 AM, Joel Stanley <joel@jms.id.au> wrote:
> 
> On Mon, 6 Sept 2021 at 13:31, <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/40cb6373b46/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
>> 
>> Most of the i2c devices are not there, they're added here:
>> 
>> https://github.com/facebook/openbmc/blob/fb2ed12002fb/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh
>> 
>> 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>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>

Thanks!

> 
>> +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]);
> 
> Wow, this is interesting. How did you go about testing it? Are you
> sure you didn't overwrite any of the pointers?
> 
> It might be worth coming up with a better way of describing all of the
> i2c buses for future machines.
> 
> Cheers,
> 
> Joel

Ah, yes, so, I took the compiled device tree output and printed it out,
and it has a complete list of the i2c aliases from all of the component
device tree’s, like this:

dtc openbmc/build-fuji/tmp/deploy/images/fuji/aspeed-bmc-facebook-fuji.dtb

aliases {
        i2c0 = "/ahb/apb/bus@1e78a000/i2c-bus@80";
        i2c1 = "/ahb/apb/bus@1e78a000/i2c-bus@100";
        i2c2 = "/ahb/apb/bus@1e78a000/i2c-bus@180";
        i2c3 = "/ahb/apb/bus@1e78a000/i2c-bus@200";
        i2c4 = "/ahb/apb/bus@1e78a000/i2c-bus@280";
        i2c5 = "/ahb/apb/bus@1e78a000/i2c-bus@300";
        i2c6 = "/ahb/apb/bus@1e78a000/i2c-bus@380";
        i2c7 = "/ahb/apb/bus@1e78a000/i2c-bus@400";
        i2c8 = "/ahb/apb/bus@1e78a000/i2c-bus@480";
        i2c9 = "/ahb/apb/bus@1e78a000/i2c-bus@500";
        i2c10 = "/ahb/apb/bus@1e78a000/i2c-bus@580";
        i2c11 = "/ahb/apb/bus@1e78a000/i2c-bus@600";
        i2c12 = "/ahb/apb/bus@1e78a000/i2c-bus@680";
        i2c13 = "/ahb/apb/bus@1e78a000/i2c-bus@700";
        i2c14 = "/ahb/apb/bus@1e78a000/i2c-bus@780";
        i2c15 = "/ahb/apb/bus@1e78a000/i2c-bus@800";
        ...
        i2c16 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@0";
        i2c17 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@1";
        i2c18 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@2";
        i2c19 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@3";
        i2c20 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@4";
        i2c21 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@5";
        i2c22 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@6";
        i2c23 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@7";
        i2c24 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@0";
        i2c25 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@1";
        i2c26 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@2";
        i2c27 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@3";
        i2c28 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@4";
        i2c29 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@5";
        i2c30 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@6";
        i2c31 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@7";
        i2c40 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@0";
        i2c41 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@1";
        i2c42 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@2";
        i2c43 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@3";
        i2c44 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@4";
        i2c46 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@6";
        ...
        i2c143 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@7/i2c-switch@76/i2c@7";
};

And setup_i2c.sh’s first parameter is referencing these aliases:

# # i2c-mux 2, channel 2
i2c_device_add 17 0x4c lm75   #SCM temp. sensor
i2c_device_add 17 0x4d lm75   #SCM temp. sensor

# # i2c-mux 2, channel 3
i2c_device_add 19 0x52 24c64   #EEPROM

# # i2c-mux 2, channel 4
i2c_device_add 20 0x50 24c02   #BMC54616S EEPROM

# # i2c-mux 2, channel 6
i2c_device_add 22 0x52 24c02   #BMC54616S EEPROM

My first idea was to make some kind of tree definition
describing the i2c switches and then do a breadth first
search/etc to enumerate all of the buses.

I2CBus *i2c[144] = {}
// Initialize base set of i2c buses.
i2c[0] = …
i2c[1] = …
…
i2c[15] = …
// Initialize switch definitions, includes
// some kind of reference to its parent bus.
struct { … } switches[] = {…}
// Populate i2c buses using switch definitions.
for (int i = 0; i < sizeof(switches)/sizeof(switches[0]); i++) {
    I2CSlave *switch = find_switch(i2c, switches[i]);
                       ^^^^Recursive function or something
    for (int j = 0; j < 8; j++) {
        // Special case because fuji datasheet skips 32..40
        if (n == 32) {
            n = 40;
        }
        i2c[n++] = aspeed_i2c_get_bus(switch, j);
    }
}

I’m realizing, I probably should have done this, but I also realized
there’s not that many switches in the system, so if you just automate
the get_channels() part (x8 buses for each switch) then it was
not unreasonable to just manually write out the locations for each switch.

As far as testing: I didn’t do a lot, I mostly just eyeball’d it
more after writing it out and then I looked at the boot logs, so
I’m still not _absolutely_ sure that I got it right. Let me know
if you guys think I should refactor this!

Thanks,
Peter

> 
>> +    get_pca9548_channels(i2c480, 0x70, &i2c[24]);
>> +    /* NOTE: The device tree skips [32, 40) in the alias numbering */
>> +    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]);
>> +    }

Re: [PATCH v4] hw/arm/aspeed: Add Fuji machine type

Posted by Cédric Le Goater 2 years, 7 months ago

On 9/7/21 4:05 PM, Peter Delevoryas wrote:
> 
> 
>> On Sep 7, 2021, at 1:59 AM, Joel Stanley <joel@jms.id.au> wrote:
>>
>> On Mon, 6 Sept 2021 at 13:31, <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/40cb6373b46/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
>>>
>>> Most of the i2c devices are not there, they're added here:
>>>
>>> https://github.com/facebook/openbmc/blob/fb2ed12002fb/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh
>>>
>>> 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>
>>
>> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> Thanks!
> 
>>
>>> +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]);
>>
>> Wow, this is interesting. How did you go about testing it? Are you
>> sure you didn't overwrite any of the pointers?
>>
>> It might be worth coming up with a better way of describing all of the
>> i2c buses for future machines.
>>
>> Cheers,
>>
>> Joel
> 
> Ah, yes, so, I took the compiled device tree output and printed it out,
> and it has a complete list of the i2c aliases from all of the component
> device tree’s, like this:
> 
> dtc openbmc/build-fuji/tmp/deploy/images/fuji/aspeed-bmc-facebook-fuji.dtb
> 
> aliases {
>         i2c0 = "/ahb/apb/bus@1e78a000/i2c-bus@80";
>         i2c1 = "/ahb/apb/bus@1e78a000/i2c-bus@100";
>         i2c2 = "/ahb/apb/bus@1e78a000/i2c-bus@180";
>         i2c3 = "/ahb/apb/bus@1e78a000/i2c-bus@200";
>         i2c4 = "/ahb/apb/bus@1e78a000/i2c-bus@280";
>         i2c5 = "/ahb/apb/bus@1e78a000/i2c-bus@300";
>         i2c6 = "/ahb/apb/bus@1e78a000/i2c-bus@380";
>         i2c7 = "/ahb/apb/bus@1e78a000/i2c-bus@400";
>         i2c8 = "/ahb/apb/bus@1e78a000/i2c-bus@480";
>         i2c9 = "/ahb/apb/bus@1e78a000/i2c-bus@500";
>         i2c10 = "/ahb/apb/bus@1e78a000/i2c-bus@580";
>         i2c11 = "/ahb/apb/bus@1e78a000/i2c-bus@600";
>         i2c12 = "/ahb/apb/bus@1e78a000/i2c-bus@680";
>         i2c13 = "/ahb/apb/bus@1e78a000/i2c-bus@700";
>         i2c14 = "/ahb/apb/bus@1e78a000/i2c-bus@780";
>         i2c15 = "/ahb/apb/bus@1e78a000/i2c-bus@800";
>         ...
>         i2c16 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@0";
>         i2c17 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@1";
>         i2c18 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@2";
>         i2c19 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@3";
>         i2c20 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@4";
>         i2c21 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@5";
>         i2c22 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@6";
>         i2c23 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@7";
>         i2c24 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@0";
>         i2c25 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@1";
>         i2c26 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@2";
>         i2c27 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@3";
>         i2c28 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@4";
>         i2c29 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@5";
>         i2c30 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@6";
>         i2c31 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@7";
>         i2c40 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@0";
>         i2c41 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@1";
>         i2c42 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@2";
>         i2c43 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@3";
>         i2c44 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@4";
>         i2c46 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@6";
>         ...
>         i2c143 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@7/i2c-switch@76/i2c@7";
> };
> 
> And setup_i2c.sh’s first parameter is referencing these aliases:
> 
> # # i2c-mux 2, channel 2
> i2c_device_add 17 0x4c lm75   #SCM temp. sensor
> i2c_device_add 17 0x4d lm75   #SCM temp. sensor
> 
> # # i2c-mux 2, channel 3
> i2c_device_add 19 0x52 24c64   #EEPROM
> 
> # # i2c-mux 2, channel 4
> i2c_device_add 20 0x50 24c02   #BMC54616S EEPROM
> 
> # # i2c-mux 2, channel 6
> i2c_device_add 22 0x52 24c02   #BMC54616S EEPROM
> 
> My first idea was to make some kind of tree definition
> describing the i2c switches and then do a breadth first
> search/etc to enumerate all of the buses.
> 
> I2CBus *i2c[144] = {}
> // Initialize base set of i2c buses.
> i2c[0] = …
> i2c[1] = …
> …
> i2c[15] = …
> // Initialize switch definitions, includes
> // some kind of reference to its parent bus.
> struct { … } switches[] = {…}
> // Populate i2c buses using switch definitions.
> for (int i = 0; i < sizeof(switches)/sizeof(switches[0]); i++) {
>     I2CSlave *switch = find_switch(i2c, switches[i]);
>                        ^^^^Recursive function or something
>     for (int j = 0; j < 8; j++) {
>         // Special case because fuji datasheet skips 32..40
>         if (n == 32) {
>             n = 40;
>         }
>         i2c[n++] = aspeed_i2c_get_bus(switch, j);
>     }
> }
> 
> I’m realizing, I probably should have done this, but I also realized
> there’s not that many switches in the system, so if you just automate
> the get_channels() part (x8 buses for each switch) then it was
> not unreasonable to just manually write out the locations for each switch.
> 
> As far as testing: I didn’t do a lot, I mostly just eyeball’d it
> more after writing it out and then I looked at the boot logs, so
> I’m still not _absolutely_ sure that I got it right. Let me know
> if you guys think I should refactor this!

It think it's fine. Fixes can come later on if needed.


Phil, 

Are we OK with this ? I would like to include this patch in
the v2 of the aspeed PR.

Thanks,

C.

Re: [PATCH v4] hw/arm/aspeed: Add Fuji machine type

Posted by Philippe Mathieu-Daudé 2 years, 7 months ago

On 9/10/21 9:49 AM, Cédric Le Goater wrote:
> On 9/7/21 4:05 PM, Peter Delevoryas wrote:
>>
>>> On Sep 7, 2021, at 1:59 AM, Joel Stanley <joel@jms.id.au> wrote:
>>>
>>> On Mon, 6 Sept 2021 at 13:31, <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/40cb6373b46/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
>>>>
>>>> Most of the i2c devices are not there, they're added here:
>>>>
>>>> https://github.com/facebook/openbmc/blob/fb2ed12002fb/meta-facebook/meta-fuji/recipes-utils/openbmc-utils/files/setup_i2c.sh
>>>>
>>>> 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>
>>>
>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>
>> Thanks!
>>
>>>
>>>> +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]);
>>>
>>> Wow, this is interesting. How did you go about testing it? Are you
>>> sure you didn't overwrite any of the pointers?
>>>
>>> It might be worth coming up with a better way of describing all of the
>>> i2c buses for future machines.
>>>
>>> Cheers,
>>>
>>> Joel
>>
>> Ah, yes, so, I took the compiled device tree output and printed it out,
>> and it has a complete list of the i2c aliases from all of the component
>> device tree’s, like this:
>>
>> dtc openbmc/build-fuji/tmp/deploy/images/fuji/aspeed-bmc-facebook-fuji.dtb
>>
>> aliases {
>>         i2c0 = "/ahb/apb/bus@1e78a000/i2c-bus@80";
>>         i2c1 = "/ahb/apb/bus@1e78a000/i2c-bus@100";
>>         i2c2 = "/ahb/apb/bus@1e78a000/i2c-bus@180";
>>         i2c3 = "/ahb/apb/bus@1e78a000/i2c-bus@200";
>>         i2c4 = "/ahb/apb/bus@1e78a000/i2c-bus@280";
>>         i2c5 = "/ahb/apb/bus@1e78a000/i2c-bus@300";
>>         i2c6 = "/ahb/apb/bus@1e78a000/i2c-bus@380";
>>         i2c7 = "/ahb/apb/bus@1e78a000/i2c-bus@400";
>>         i2c8 = "/ahb/apb/bus@1e78a000/i2c-bus@480";
>>         i2c9 = "/ahb/apb/bus@1e78a000/i2c-bus@500";
>>         i2c10 = "/ahb/apb/bus@1e78a000/i2c-bus@580";
>>         i2c11 = "/ahb/apb/bus@1e78a000/i2c-bus@600";
>>         i2c12 = "/ahb/apb/bus@1e78a000/i2c-bus@680";
>>         i2c13 = "/ahb/apb/bus@1e78a000/i2c-bus@700";
>>         i2c14 = "/ahb/apb/bus@1e78a000/i2c-bus@780";
>>         i2c15 = "/ahb/apb/bus@1e78a000/i2c-bus@800";
>>         ...
>>         i2c16 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@0";
>>         i2c17 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@1";
>>         i2c18 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@2";
>>         i2c19 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@3";
>>         i2c20 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@4";
>>         i2c21 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@5";
>>         i2c22 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@6";
>>         i2c23 = "/ahb/apb/bus@1e78a000/i2c-bus@180/i2c-switch@70/i2c@7";
>>         i2c24 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@0";
>>         i2c25 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@1";
>>         i2c26 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@2";
>>         i2c27 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@3";
>>         i2c28 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@4";
>>         i2c29 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@5";
>>         i2c30 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@6";
>>         i2c31 = "/ahb/apb/bus@1e78a000/i2c-bus@480/i2c-switch@70/i2c@7";
>>         i2c40 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@0";
>>         i2c41 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@1";
>>         i2c42 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@2";
>>         i2c43 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@3";
>>         i2c44 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@4";
>>         i2c46 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@6";
>>         ...
>>         i2c143 = "/ahb/apb/bus@1e78a000/i2c-bus@600/i2c-switch@77/i2c@7/i2c-switch@76/i2c@7";
>> };
>>
>> And setup_i2c.sh’s first parameter is referencing these aliases:
>>
>> # # i2c-mux 2, channel 2
>> i2c_device_add 17 0x4c lm75   #SCM temp. sensor
>> i2c_device_add 17 0x4d lm75   #SCM temp. sensor
>>
>> # # i2c-mux 2, channel 3
>> i2c_device_add 19 0x52 24c64   #EEPROM
>>
>> # # i2c-mux 2, channel 4
>> i2c_device_add 20 0x50 24c02   #BMC54616S EEPROM
>>
>> # # i2c-mux 2, channel 6
>> i2c_device_add 22 0x52 24c02   #BMC54616S EEPROM
>>
>> My first idea was to make some kind of tree definition
>> describing the i2c switches and then do a breadth first
>> search/etc to enumerate all of the buses.
>>
>> I2CBus *i2c[144] = {}
>> // Initialize base set of i2c buses.
>> i2c[0] = …
>> i2c[1] = …
>> …
>> i2c[15] = …
>> // Initialize switch definitions, includes
>> // some kind of reference to its parent bus.
>> struct { … } switches[] = {…}
>> // Populate i2c buses using switch definitions.
>> for (int i = 0; i < sizeof(switches)/sizeof(switches[0]); i++) {
>>     I2CSlave *switch = find_switch(i2c, switches[i]);
>>                        ^^^^Recursive function or something
>>     for (int j = 0; j < 8; j++) {
>>         // Special case because fuji datasheet skips 32..40
>>         if (n == 32) {
>>             n = 40;
>>         }
>>         i2c[n++] = aspeed_i2c_get_bus(switch, j);
>>     }
>> }
>>
>> I’m realizing, I probably should have done this, but I also realized
>> there’s not that many switches in the system, so if you just automate
>> the get_channels() part (x8 buses for each switch) then it was
>> not unreasonable to just manually write out the locations for each switch.
>>
>> As far as testing: I didn’t do a lot, I mostly just eyeball’d it
>> more after writing it out and then I looked at the boot logs, so
>> I’m still not _absolutely_ sure that I got it right. Let me know
>> if you guys think I should refactor this!
> 
> It think it's fine. Fixes can come later on if needed.
> 
> 
> Phil, 
> 
> Are we OK with this ? I would like to include this patch in
> the v2 of the aspeed PR.

My previous comment was only about having referenced URL valid
long term. I'm not sure if Joel is asking to include the dtc
output too. I haven't looked at the patch content but am happier
with the patch description, thanks both :)

Regards,

Phil.