Re: [PATCH linux dev-5.0 v2 1/1] ARM: dts: aspeed: Add s7106 BMC Machine

[Andrew Jeffery]

Hi Ray,

Thanks for the patch, it would be good to send this upstream!

However, some comments below:

On Tue, 30 Apr 2019, at 20:32, Ray Lue wrote:
> s7106 is a Tyan platform family with an ASPEED AST2500 BMC.
> 
> Signed-off-by: Ray Lue <ray.lue at mic.com.tw>
> ---
>  arch/arm/boot/dts/Makefile                  |   3 +-
>  arch/arm/boot/dts/aspeed-bmc-tyan-s7106.dts | 175 ++++++++++++++++++++++++++++
>  2 files changed, 177 insertions(+), 1 deletion(-)
>  create mode 100755 arch/arm/boot/dts/aspeed-bmc-tyan-s7106.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index bd40148..bc38f77 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1247,4 +1247,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-bmc-opp-witherspoon.dtb \
>  	aspeed-bmc-opp-zaius.dtb \
>  	aspeed-bmc-portwell-neptune.dtb \
> -	aspeed-bmc-quanta-q71l.dtb
> +	aspeed-bmc-quanta-q71l.dtb \
> +	aspeed-bmc-tyan-s7106.dtb
> diff --git a/arch/arm/boot/dts/aspeed-bmc-tyan-s7106.dts 
> b/arch/arm/boot/dts/aspeed-bmc-tyan-s7106.dts
> new file mode 100755
> index 0000000..563558d
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-tyan-s7106.dts
> @@ -0,0 +1,175 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +#include "aspeed-g5.dtsi"
> +
> +/ {
> +	model = "Tyan S7106 BMC";
> +	compatible = "tyan,s7106-bmc", "aspeed,ast2500";
> +
> +	chosen {
> +		stdout-path = &uart5;
> +		bootargs = "console=ttyS4,115200 earlyprintk";
> +	};
> +
> +	memory {
> +		reg = <0x80000000 0x20000000>;
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		vga_memory: framebuffer at 98000000 {
> +			no-map;
> +			reg = <0x98000000 0x00800000>; /* 8MB */
> +		};
> +	};
> +
> +	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>;
> +	};
> +
> +	iio-hwmon-battery {
> +		compatible = "iio-hwmon";
> +		io-channels = <&adc 15>;
> +	};
> +};
> +
> +&fmc {
> +	status = "okay";
> +	flash at 0 {
> +		label = "bmc";
> +		status = "okay";
> +		m25p,fast-read;
> +#include "openbmc-flash-layout.dtsi"
> +	};
> +};
> +
> +// UART1 is used only by the host. While the BMC does not need to 
> access it,
> +// enable it here to make sure the UART's clock gets enabled.
> +&uart1 {
> +	status = "okay";

Needs a pinctrl node as well to ensure the UART IO is routed out of the
BMC.

> +};
> +
> +&uart3 {
> +	status = "okay";

As above.

> +};
> +
> +&uart5 {
> +	status = "okay";

Note that pinctrl isn't necessary here, uart5 functionality isn't behind a mux.

> +};
> +
> +&lpc_ctrl {
> +	status = "okay";

Have you run into issues with not assigning reserved memory? As far
as I'm aware we still need Vijay's patch to be applied.

Are you setting it to "okay" this just to enable the LPC clock?

> +};
> +
> +&lpc_snoop {
> +	status = "okay";
> +	snoop-ports = <0x80>;
> +};
> +
> +&adc {
> +	status = "okay";

Should have explicit pinctrl property here requesting the necessary
ADC lines. Looks like all the other platform DTS files need this fixed
too.

> +};
> +
> +&pwm_tacho {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_pwm3_default &pinctrl_pwm4_default>;
> +
> +	fan at 0 {
> +		reg = <0x03>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x02>;
> +	};
> +
> +	fan at 1 {
> +		reg = <0x04>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x03>;
> +	};
> +
> +	fan at 2 {
> +		reg = <0x03>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x04>;
> +	};
> +
> +	fan at 3 {
> +		reg = <0x04>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x05>;
> +	};
> +
> +	fan at 4 {
> +		reg = <0x03>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x06>;
> +	};
> +
> +	fan at 5 {
> +		reg = <0x04>;
> +		aspeed,fan-tach-ch = /bits/ 8 <0x07>;
> +	};
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +
> +	nct7802 at 28 {
> +		compatible = "nuvoton,nct7802";

This compatible isn't documented anywhere. Do you have out-of-tree
bindings? A driver?

> +		reg = <0x28>;
> +	};
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +};
> +
> +&i2c2 {
> +	status = "okay";
> +
> +	eeprom at 50 {
> +		compatible = "at,24c256";

Compatible also isn't documented anywhere. Same queries as above.

> +		reg = <0x50>;
> +	};
> +};
> +
> +&i2c3 {
> +	status = "okay";
> +};
> +
> +&i2c4 {
> +	status = "okay";
> +};
> +
> +&i2c5 {
> +	status = "okay";
> +};
> +
> +&i2c6 {
> +	status = "okay";
> +};
> +
> +&i2c7 {
> +	status = "okay";
> +};
> +
> +&i2c8 {
> +	status = "okay";
> +};
> +
> +&mac0 {
> +	status = "okay";
> +
> +	use-ncsi;
> +	no-hw-checksum;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_rmii1_default>;
> +};
> +
> +&ibt {
> +	status = "okay";
> +};
> -- 
> 2.7.4
> 
>

Cheers,

Andrew