Re: [PATCH] ARM: dts: aspeed: Adding Inventec Transformers BMC

[Joel Stanley <joel@jms.id.au>]

On Fri, 27 Aug 2021 at 05:45, Lin.TommySC 林世欽 TAO
<Lin.TommySC@inventec.com> wrote:
>
> Initial introduction of Inventec Transformers family equipped with Aspeed 2600 BMC SoC.

I assume this is an x86 server? Mentioning details like this is useful
for review, but is not required.

>
> Signed-off-by: Lin.TommySC <lin.tommysc@inventec.com>

Ensure your author name is set correctly:

git config --global user.anime "Lin Tommy SC" (or whatever spelling
makes sense).

> ---
>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  arch/arm/boot/dts/Makefile                    |   1 +
>  .../dts/aspeed-bmc-inventec-transformers.dts  | 486 ++++++++++++++++++
>  3 files changed, 489 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-inventec-transformers.dts
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 355b81148b85..28c068ed0a75 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -507,6 +507,8 @@ patternProperties:
>      description: Inter Control Group
>    "^invensense,.*":
>      description: InvenSense Inc.
> +  "^inventec,.*":
> +    description: Inventec Corporation

Please send this change in a separate patch.

>    "^inversepath,.*":
>      description: Inverse Path
>    "^iom,.*":
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 48d48c85de9e..930b8ba6c3c5 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1407,6 +1407,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>         aspeed-bmc-intel-s2600wf.dtb \
>         aspeed-bmc-inspur-fp5280g2.dtb \
>         aspeed-bmc-inspur-nf5280m6.dtb \
> +       aspeed-bmc-inventec-transformers.dtb \
>         aspeed-bmc-lenovo-hr630.dtb \
>         aspeed-bmc-lenovo-hr855xg2.dtb \
>         aspeed-bmc-microsoft-olympus.dtb \
> diff --git a/arch/arm/boot/dts/aspeed-bmc-inventec-transformers.dts b/arch/arm/boot/dts/aspeed-bmc-inventec-transformers.dts
> new file mode 100644
> index 000000000000..4ff28d1439cd
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-inventec-transformers.dts
> @@ -0,0 +1,486 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright 2021 Inventec Corp.
> +
> +/dts-v1/;
> +
> +#include "aspeed-g6.dtsi"
> +#include "aspeed-g6-pinctrl.dtsi"
> +#include <dt-bindings/i2c/i2c.h>
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> +       model = "TRANSFORMERS BMC";
> +       compatible = "inventec,transformer-bmc", "aspeed,ast2600";
> +
> +       aliases {
> +               serial4 = &uart5;
> +       };
> +
> +       chosen {
> +               stdout-path = &uart5;
> +               bootargs = "console=tty0 console=ttyS4,115200n8 root=/dev/ram rw init=/linuxrc";

Are you sure this is correct? The console options are ok, but the
others are often not required.

> +       };
> +
> +       memory@80000000 {
> +               device_type = "memory";
> +               reg = <0x80000000 0x80000000>;
> +       };
> +
> +       reserved-memory {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
> +
> +               gfx_memory: framebuffer {

Does your machine run code on the BMC to output to the display without
the host? That is what this is for.

> +                       size = <0x01000000>;
> +                       alignment = <0x01000000>;
> +                       compatible = "shared-dma-pool";
> +                       reusable;
> +               };
> +
> +               video_engine_memory: video {
> +                       size = <0x04000000>;
> +                       alignment = <0x01000000>;
> +                       compatible = "shared-dma-pool";
> +                       reusable;
> +               };
> +
> +               ssp_memory: ssp_memory {

What is ssp, out of interest?

> +                       size = <0x00200000>;
> +                       alignment = <0x00100000>;
> +                       compatible = "shared-dma-pool";
> +                       no-map;
> +               };
> +       };
> +
> +       iio-hwmon {
> +               compatible = "iio-hwmon";

This doesn't make sense without an io-channels property.

> +       };
> +
> +       leds {
> +               compatible = "gpio-leds";
> +
> +               // UID led
> +               uid {
> +                       label = "UID_LED";
> +                       gpios = <&gpio0 ASPEED_GPIO(X, 0) GPIO_ACTIVE_LOW>;
> +               };
> +
> +               // Heart beat led
> +               heartbeat {
> +                       label = "HB_LED";
> +                       gpios = <&gpio0 ASPEED_GPIO(P, 7) GPIO_ACTIVE_LOW>;
> +               };
> +       };
> +

> +&i2c0 {
> +       status = "okay";
> +
> +       //Set bmc' slave address;
> +       bmc_slave@10 {
> +               compatible = "ipmb-dev";
> +               reg = <(0x10 | I2C_OWN_SLAVE_ADDRESS)>;
> +               i2c-protocol;
> +       };
> +};
> +
> +&i2c2 {
> +       status = "okay";
> +};
> +
> +&i2c3 {
> +       // FRU AT24C512C-SSHM-T
> +       status = "okay";
> +       eeprom@50 {
> +               compatible = "atmel,24c512";
> +               reg = <0x50>;
> +               pagesize = <128>;
> +       };
> +};
> +
> +&i2c5 {
> +       status = "okay";
> +};
> +
> +&i2c6 {
> +       status = "okay";
> +
> +       tmp75@49 {
> +               compatible = "ti,tmp75";
> +               reg = <0x49>;
> +       };
> +
> +       tmp75@4f {
> +               compatible = "ti,tmp75";
> +               reg = <0x4f>;
> +       };
> +
> +       tmp468@48 {
> +               compatible = "ti,tmp468";
> +               reg = <0x48>;
> +       };
> +};
> +
> +&i2c7 {
> +       status = "okay";
> +       adm1278@40 {
> +               compatible = "adi,adm1278";
> +               reg = <0x40>;
> +       };
> +};
> +
> +
> +&i2c8 {
> +       // FRU AT24C512C-SSHM-T
> +       status = "okay";
> +
> +       eeprom@51 {
> +               compatible = "atmel,24c512";
> +               reg = <0x51>;
> +               pagesize = <128>;
> +       };
> +
> +       eeprom@53 {
> +               compatible = "atmel,24c512";
> +               reg = <0x53>;
> +               pagesize = <128>;
> +       };
> +};
> +
> +&i2c9 {
> +       // M.2
> +       status = "okay";
> +};
> +
> +&i2c10 {
> +       // I2C EXPANDER
> +       status = "okay";
> +
> +       i2c-switch@71 {
> +               compatible = "nxp,pca9544";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               reg = <0x71>;
> +       };
> +
> +       i2c-switch@73 {
> +               compatible = "nxp,pca9544";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               reg = <0x73>;
> +       };
> +};
> +
> +&i2c11 {
> +       // I2C EXPANDER
> +       status = "okay";
> +
> +       i2c-switch@70 {
> +               compatible = "nxp,pca9544";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               reg = <0x70>;
> +
> +               pcie_eeprom_riser1: i2c@0 {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       reg = <0>;
> +
> +                       eeprom@55 {
> +                               compatible = "atmel,24c512";
> +                               reg = <0x55>;
> +                               pagesize = <128>;
> +                       };
> +               };
> +
> +               pcie_eeprom_riser2: i2c@1 {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       reg = <1>;
> +
> +                       eeprom@55 {
> +                               compatible = "atmel,24c512";
> +                               reg = <0x55>;
> +                               pagesize = <128>;
> +                       };
> +               };
> +
> +               pcie_eeprom_riser3: i2c@2 {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       reg = <2>;
> +
> +                       eeprom@55 {
> +                               compatible = "atmel,24c512";
> +                               reg = <0x55>;
> +                               pagesize = <128>;
> +                       };
> +               };
> +       };
> +};
> +
> +&i2c12 {
> +       status = "okay";
> +
> +       psu0:psu0@58 {
> +               compatible = "pmbus";
> +               reg = <0x58>;
> +       };
> +};
> +
> +&gpio0 {
> +       status = "okay";
> +       gpio-line-names =
> +       /*A0-A7*/   "","","","","","","","",
> +       /*B0-B7*/   "I2C_HSC_ALERT","BMC_READY","","","","","PSU1_ALERT","",
> +       /*C0-C7*/   "","","","","","","","",
> +       /*D0-D7*/   "","","","","","","","",
> +       /*E0-E7*/   "","","","","","","","",
> +       /*F0-F7*/   "","","","","RST_BMC_SGPIO","","","",
> +       /*G0-G7*/   "","","JTAG_MUX_SEL","","","","","",
> +       /*H0-H7*/   "","","","","RESET_OUT","POWER_OUT","","",
> +       /*I0-I7*/   "","","","","","","NMI_OUT","",
> +       /*J0-J7*/   "","","","","","","","",
> +       /*K0-K7*/   "","","","","","","","",
> +       /*L0-L7*/   "","","","","","","","",
> +       /*M0-M7*/   "","","","","","","","",
> +       /*N0-N7*/   "","","","","","","","",
> +       /*O0-O7*/   "","","","","","","","",
> +       /*P0-P7*/   "","","","TCK_MUX_SEL","BMC_ASD_JTAG_EN","","PREQ_N","",
> +       /*Q0-Q7*/   "","","","","","","","",
> +       /*R0-R7*/   "","","","","","","","",
> +       /*S0-S7*/   "","","","","","","PCH_THERMTRIP","",
> +       /*T0-T7*/   "","","","","","","","",
> +       /*U0-U7*/   "","NMI_BUTTON","","","","","","",
> +       /*V0-V7*/   "","","","","PS_PWROK","","","PRDY_N",
> +       /*W0-W7*/   "","","","","","","","",
> +       /*X0-X7*/   "","","","CPLD_CATERR","","","","",
> +       /*Y0-Y7*/   "","","","","","","","",
> +       /*Z0-Z7*/   "","","","","","","","",
> +       /*AA0-AA7*/ "","","","","","","","",
> +       /*AB0-AB7*/ "","","","","","","","",
> +       /*AC0-AC7*/ "","","","","","","","";

Is this machine going to run openbmc?

If yes, please review this document and ensure your naming follows the
recommendations:

https://github.com/openbmc/docs/blob/master/designs/device-tree-gpio-naming.md


> +};
> +
> +&lpc_snoop {
> +       status = "okay";
> +       snoop-ports = <0x80>;
> +};
> +
> +&emmc_controller {
> +       status = "okay";
> +       timing-phase = <0x700FF>;

This is not the correct style for the upstream driver, please see the bindings:

Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
Documentation/devicetree/bindings/mmc/mmc-controller.yaml

I find them hard to understand, so you may want to compare to other
machines in the tree:

arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts

&emmc {
        status = "okay";
        clk-phase-mmc-hs200 = <180>, <180>;
};


> +};
> +
> +&emmc {
> +       status = "okay";
> +
> +       non-removable;
> +       max-frequency = <52000000>;
> +       sdhci-drive-type = /bits/ 8 <3>;
> +       bus-width = <8>;
> +};
> +
> +&vhub {
> +       status = "okay";
> +       aspeed,vhub-downstream-ports = <7>;
> +       aspeed,vhub-generic-endpoints = <21>;
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_usb2ad_default>;
> +};
> +
> +&rtc {
> +       status = "okay";
> +};
> --
> 2.33.0
>