Re: [[PATCH linux dev-5.8] ARM: dts: Aspeed: Add Compal's Liwu2 BMC machine] ARM: dts: aspeed: Add device tree for Compal's Liwu2 BMC

[Andrew Jeffery]

On Fri, 19 Feb 2021, at 17:41, Willie Thai wrote:
> The Liwu2 is a server platform with an ASPEED AST2500 based BMC.
> 
> Reviewed-by: Andrew Jeffery <andrew at aj.id.au>

I don't recall providing my r-b tag.

I have comments below.

> Reviewed-by: Joel Stanley <joel at jms.id.au>
> Signed-off-by: willie_thai at compal.com
> ---
>  arch/arm/boot/dts/Makefile                    |   1 +
>  arch/arm/boot/dts/aspeed-bmc-compal-liwu2.dts | 325 ++++++++++++++++++++++++++
>  2 files changed, 326 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-compal-liwu2.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 6320124..a67576d 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1354,6 +1354,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-bmc-arm-centriq2400-rep.dtb \
>  	aspeed-bmc-arm-stardragon4800-rep2.dtb \
>  	aspeed-bmc-bytedance-g220a.dtb \
> +	aspeed-bmc-compal-liwu2.dtb \
>  	aspeed-bmc-facebook-cmm.dtb \
>  	aspeed-bmc-facebook-minipack.dtb \
>  	aspeed-bmc-facebook-tiogapass.dtb \
> diff --git a/arch/arm/boot/dts/aspeed-bmc-compal-liwu2.dts 
> b/arch/arm/boot/dts/aspeed-bmc-compal-liwu2.dts
> new file mode 100644
> index 0000000..68faf3d
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-compal-liwu2.dts
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +#include "aspeed-g5.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> +	model = "AST2500 liwu2";
> +	compatible = "aspeed,ast2500";
> +
> +	aliases {
> +		serial4 = &uart5;
> +	};
> +
> +	chosen {
> +		stdout-path = &uart5;
> +		bootargs = "console=tty0 console=ttyS4,115200 earlyprintk";
> +	};
> +
> +	memory at 80000000 {
> +		reg = <0x80000000 0x20000000>;
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		gfx_memory: framebuffer {
> +			size = <0x01000000>;
> +			alignment = <0x01000000>;
> +			compatible = "shared-dma-pool";
> +			reusable;
> +		};
> +	};
> +
> +	iio-hwmon {
> +		compatible = "iio-hwmon";
> +		io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> +				<&adc 4>, <&adc 5>, <&adc 6>, <&adc 7>,
> +				<&adc 8>, <&adc 9>, <&adc 10>, <&adc 11>,
> +				<&adc 12>, <&adc 13>, <&adc 14>, <&adc 15>;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		led_fan0_fault {
> +			label = "LED_FAN0_FAULT";
> +			gpios = <&gpio ASPEED_GPIO(F, 4) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		led_fan1_fault {
> +			label = "LED_FAN1_FAULT";
> +			gpios = <&gpio ASPEED_GPIO(F, 5) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		led_fan2_fault {
> +			label = "LED_FAN2_FAULT";
> +			gpios = <&gpio ASPEED_GPIO(H, 2) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		led_fan3_fault {
> +			label = "LED_FAN3_FAULT";
> +			gpios = <&gpio ASPEED_GPIO(H, 5) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		led_fan4_fault {
> +			label = "LED_FAN4_FAULT";
> +			gpios = <&gpio ASPEED_GPIO(H, 6) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		led_fan5_fault {
> +			label = "LED_FAN5_FAULT";
> +			gpios = <&gpio ASPEED_GPIO(H, 7) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		fp_led_status_amber_n {
> +			label = "FP_LED_STATUS_AMBER_N";
> +			gpios = <&gpio ASPEED_GPIO(S, 5) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		rear_id_led_n {
> +			label = "REAR_ID_LED_N";
> +			gpios = <&gpio ASPEED_GPIO(S, 6) GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +};
> +
> +&fmc {
> +	status = "okay";
> +	flash at 0 {
> +		status = "okay";
> +		m25p,fast-read;
> +		label = "bmc";
> +		spi-max-frequency = <50000000>;
> +#include "openbmc-flash-layout.dtsi"
> +	};
> +};
> +
> +&spi1 {
> +	status = "okay";

Should have at least:

pinctrl-names = "default";
pinctrl-0 = <&pinctrl_spi1_default>;

And then if you need additional chip selects, you should add them too.

> +	flash at 0 {
> +		status = "okay";
> +		m25p,fast-read;
> +		label = "pnor";
> +		spi-max-frequency = <100000000>;
> +	};
> +};
> +
> +&spi2 {
> +	status = "okay";

Is this necessary given no flash device is defined?

If it is, you also need pinctrl properties.

> +};
> +
> +&uart5 {
> +	status = "okay";
> +};
> +
> +&mac0 {
> +	status = "okay";
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
> +};
> +
> +&mac1 {
> +	status = "okay";
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_rgmii2_default &pinctrl_mdio2_default>;
> +};
> +
> +&adc {
> +	status = "okay";

Given you're using the ADC lines in the iio-hwmon bridge, you need to 
request the ADC pins from pinmux here. ADC is the default mux 
configuration for the pins, so this will happen to work as is, but 
pinmux will not prevent something else from requesting the pins out 
from underneath the ADC.

Andrew