Re: [PATCH 3/3] ARM: dts: aspeed: Add Balcones system

[Eddie James <eajames@linux.ibm.com>]

On 2/19/25 18:41, Andrew Jeffery wrote:
> Hi Eddie,
>
> I have some minor comments inline:
>
> On Wed, 2025-02-19 at 15:27 -0600, Eddie James wrote:
>> The Balcones system is similar to Bonnell but with a POWER11 processor.
> Can you add some commentary about the P11 dual vs quad changes here?


Yes I will.


>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   arch/arm/boot/dts/aspeed/Makefile             |   1 +
>>   .../dts/aspeed/aspeed-bmc-ibm-balcones.dts    | 594 +++++++++++++
>>   .../arm/boot/dts/aspeed/ibm-power11-dual.dtsi | 779 ++++++++++++++++++
>>   .../arm/boot/dts/aspeed/ibm-power11-quad.dtsi | 769 +----------------
>>   4 files changed, 1376 insertions(+), 767 deletions(-)
>>   create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-balcones.dts
>>   create mode 100644 arch/arm/boot/dts/aspeed/ibm-power11-dual.dtsi
>>
>> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
>> index 2e5f4833a073b..71b2d67fdbac8 100644
>> --- a/arch/arm/boot/dts/aspeed/Makefile
>> +++ b/arch/arm/boot/dts/aspeed/Makefile
>> @@ -34,6 +34,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>          aspeed-bmc-facebook-yamp.dtb \
>>          aspeed-bmc-facebook-yosemitev2.dtb \
>>          aspeed-bmc-facebook-yosemite4.dtb \
>> +       aspeed-bmc-ibm-balcones.dtb \
>>          aspeed-bmc-ibm-blueridge.dtb \
>>          aspeed-bmc-ibm-bonnell.dtb \
>>          aspeed-bmc-ibm-everest.dtb \
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-balcones.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-balcones.dts
>> new file mode 100644
>> index 0000000000000..0fe99a748b63b
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-balcones.dts
>> @@ -0,0 +1,594 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright 2025 IBM Corp.
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/gpio/aspeed-gpio.h>
>> +#include <dt-bindings/i2c/i2c.h>
>> +#include <dt-bindings/leds/leds-pca955x.h>
>> +#include "aspeed-g6.dtsi"
>> +#include "ibm-power11-dual.dtsi"
>> +
>> +/ {
>> +       model = "Balcones";
>> +       compatible = "ibm,balcones-bmc", "aspeed,ast2600";
>> +
>>
> ...
>
>> +};
>> +
>> +&i2c0 {
>> +       status = "okay";
>> +
>> +       eeprom@51 {
>> +               compatible = "atmel,24c64";
>> +               reg = <0x51>;
>> +       };
>> +
>> +       gpio@20 {
> Can you please follow the DTS coding style and order the nodes by unit
> address?
>
> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html#order-of-nodes
>
> Ordering the top-level label references alphabetically would be
> appreciated too. It doesn't seem like they're in an order reflecting
> e.g. the unit address, it feels kind of arbitrary.


Oops yep, I'll fix all these.


>
>> +               compatible = "ti,tca9554";
>> +               reg = <0x20>;
>> +               gpio-controller;
>> +               #gpio-cells = <2>;
>> +
>> +               gpio-line-names =
>> +                       "",
>> +                       "RUSSEL_FW_I2C_ENABLE_N",
>> +                       "RUSSEL_OPPANEL_PRESENCE_N",
>> +                       "BLYTH_OPPANEL_PRESENCE_N",
>> +                       "CPU_TPM_CARD_PRESENT_N",
>> +                       "",
>> +                       "",
>> +                       "DASD_BP_PRESENT_N";
>> +       };
>> +};
>> +
> ...
>
>> +
>> +&i2c5 {
>> +       status = "okay";
>> +
>> +       eeprom@52 {
>> +               compatible = "atmel,24c64";
>> +               reg = <0x52>;
>> +       };
>> +
>> +       temperature-sensor@4e {
> Ordering again here.
>
>> +               compatible = "ti,tmp435";
>> +               reg = <0x4e>;
>> +       };
>> +};
>>
> ...
>
>> +
>> +&kcs3 {
>> +       status = "okay";
>> +       aspeed,lpc-io-reg = <0xca2>;
>> +       aspeed,lpc-interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
>> +};
>> diff --git a/arch/arm/boot/dts/aspeed/ibm-power11-dual.dtsi b/arch/arm/boot/dts/aspeed/ibm-power11-dual.dtsi
>> new file mode 100644
>> index 0000000000000..e5f65258e082f
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed/ibm-power11-dual.dtsi
>> @@ -0,0 +1,779 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright 2025 IBM Corp.
>> +
>> +/ {
>>
> ...
>
>> +
>> +&fsim0 {
>> +       #address-cells = <2>;
>> +       #size-cells = <0>;
>> +       status = "okay";
>> +       bus-frequency = <100000000>;
>> +       cfam-reset-gpios = <&gpio0 ASPEED_GPIO(Q, 0) GPIO_ACTIVE_HIGH>;
>> +
>> +       cfam@0,0 {
>> +               reg = <0 0>;
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               chip-id = <0>;
>> +
>> +
> ...
>
>> +
>> +               sbefifo@2400 {
>> +                       compatible = "ibm,p9-sbefifo";
>> +                       reg = <0x2400 0x400>;
>> +
>> +                       occ {
>> +                               compatible = "ibm,p10-occ";
> I assume this doesn't need a new compatible for P11 (given you're
> moving it out of the quad DTSI).


That is correct. FSI all remains the same for P11.


Thanks for the review!

Eddie


>
> Andrew