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

[Philippe Mathieu-Daudé]

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.