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

[Cédric Le Goater]

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.