Re: [PATCH linux dev-5.0] ARM: dts: aspeed: Add Microsoft Olympus BMC

["Andrew Jeffery" <andrew@aj.id.au>]

Hello Hongwei,

Thanks for the patch!

I think this is the second or third iteration? If you send an updated patch in the future
please make sure to use the `-v` option as well as`--subject-prefix` to
`git format-patch` - this changes the subject to e.g. `[PATCH linux dev-5.0 v2]` and
allows us to identify why you've sent the patch from reading the subject.

On Tue, 14 May 2019, at 03:48, Hongwei Zhang wrote:
> Olympus is a Microsoft OCP platform equipped with Aspeed 2400 BMC
> Soc.
> 
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
>  arch/arm/boot/dts/Makefile                         |   1 +
>  arch/arm/boot/dts/aspeed-bmc-microsoft-olympus.dts | 204 +++++++++++++++++++++
>  2 files changed, 205 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-microsoft-olympus.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index c01a7b1..072725a 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1242,6 +1242,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-bmc-facebook-tiogapass.dtb \
>  	aspeed-bmc-intel-s2600wf.dtb \
>  	aspeed-bmc-lenovo-hr630.dtb \
> +	aspeed-bmc-microsoft-olympus.dtb \
>  	aspeed-bmc-opp-lanyang.dtb \
>  	aspeed-bmc-opp-palmetto.dtb \
>  	aspeed-bmc-opp-romulus.dtb \
> diff --git a/arch/arm/boot/dts/aspeed-bmc-microsoft-olympus.dts 
> b/arch/arm/boot/dts/aspeed-bmc-microsoft-olympus.dts
> new file mode 100644
> index 0000000..ed25fac
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-microsoft-olympus.dts
> @@ -0,0 +1,204 @@
> +//SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Device Tree file for Microsoft Olympus platform.
> + */
> +
> +/dts-v1/;
> +
> +#include "aspeed-g4.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> +	model = "Olympus BMC";
> +	compatible = "microsoft,olympus-bmc", "aspeed,ast2400";
> +
> +	chosen {
> +		stdout-path = &uart5;
> +		bootargs = "console=ttyS4,115200 earlyprintk";
> +	};
> +
> +	memory@40000000 {
> +		reg = <0x40000000 0x20000000>;
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		vga_memory: framebuffer@5f000000 {
> +			no-map;
> +			reg = <0x5f000000 0x01000000>; /* 16M */
> +		};
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		bmc_heartbeat {
> +			gpios = <&gpio ASPEED_GPIO(B, 0) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		power_green {
> +			gpios = <&gpio ASPEED_GPIO(U, 2) GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		power_amber {
> +			gpios = <&gpio ASPEED_GPIO(U, 3) GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		identify {
> +			gpios = <&gpio ASPEED_GPIO(Q, 5) GPIO_ACTIVE_LOW>;
> +		};
> +
> +		fault {
> +			gpios = <&gpio ASPEED_GPIO(A, 1) GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +
> +
> +	iio-hwmon {
> +		compatible = "iio-hwmon";
> +		io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> +		<&adc 4>, <&adc 5>, <&adc 6>, <&adc 7>;
> +	};
> +};
> +
> +&adc {
> +	status = "okay";

Can you please add the pinmux properties for the ADC lines that you're using?
The iio-hwmon node suggests you're using lines 0-7.

> +};
> +
> +&fmc {
> +	status = "okay";
> +
> +	flash@0 {
> +		status = "okay";
> +		m25p,fast-read;
> +		label = "bmc";
> +#include "openbmc-flash-layout.dtsi"
> +	};
> +};
> +
> +&spi {
> +	status = "okay";
> +	pinctrl-names = "default";

This is missing the pinctrl-0 property.

I think in the last version of the patch you had this set to:

pinctrl-0 = <&pinctrl_spi1debug_default>;

and I queried how the board was strapped - you still need the pinctrl-0
node, but what I should have been clearer about was that you should
probably be using the following instead:

pinctrl-0 = <&pinctrl_spi1_default>;

unless your board is strapped for SPI debug mode. This is
something you'll need to check on the schematics, as pinctrl won't
modify the strapping value to accommodate the devicetree in this case.

Otherwise, the patch looks okay to me!

Cheers,

Andrew

> +
> +	flash@0 {
> +		status = "okay";
> +		m25p,fast-read;
> +		label = "pnor";
> +	};
> +};
> +
> +&uart5 {
> +	status = "okay";
> +};
> +
> +&mac0 {
> +	status = "okay";
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +
> +	tmp421@4c {
> +		compatible = "ti,tmp421";
> +		reg = <0x4c>;
> +	};
> +};
> +
> +&i2c2 {
> +	status = "okay";
> +};
> +
> +&i2c3 {
> +	status = "okay";
> +};
> +
> +&i2c4 {
> +	status = "okay";
> +	clock-frequency = <100000>;
> +};
> +
> +&i2c5 {
> +	status = "okay";
> +};
> +
> +&i2c6 {
> +	status = "okay";
> +
> +	tmp421@4c {
> +		compatible = "ti,tmp421";
> +		reg = <0x4c>;
> +	};
> +};
> +
> +&i2c7 {
> +	status = "okay";
> +};
> +
> +&vuart {
> +	status = "okay";
> +};
> +
> +&wdt2 {
> +	status = "okay";
> +};
> +
> +&lpc_ctrl {
> +	status = "okay";
> +};
> +
> +&mbox {
> +	status = "okay";
> +};
> +
> +&pwm_tacho {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 =    <&pinctrl_pwm0_default
> +			&pinctrl_pwm1_default
> +			&pinctrl_pwm2_default
> +			&pinctrl_pwm3_default
> +			&pinctrl_pwm4_default
> +			&pinctrl_pwm5_default
> +			&pinctrl_pwm6_default>;
> +
> +	fan@0 {
> +		reg = <0x00>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x00>;
> +	};
> +
> +	fan@1 {
> +		reg = <0x01>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x01>;
> +	};
> +
> +	fan@2 {
> +		reg = <0x02>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x02>;
> +	};
> +
> +	fan@3 {
> +		reg = <0x03>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x03>;
> +	};
> +
> +	fan@4 {
> +		reg = <0x04>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x04>;
> +	};
> +
> +	fan@5 {
> +		reg = <0x05>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x05>;
> +	};
> +
> +};
> -- 
> 2.7.4
> 
>