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

[Peter Delevoryas]

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