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

ray.lue at mic.com.tw ray.lue at mic.com.tw
Mon May 6 20:53:33 AEST 2019


Hi Andrew,

Thanks for your review, I will update the patch accordingly.
Detailed response below.

> -----Original Message-----
> From: Andrew Jeffery [mailto:andrew at aj.id.au]
> Sent: Wednesday, May 1, 2019 1:43 PM
> To: Ray Lue <ray.lue at gmail.com>; openbmc at lists.ozlabs.org; Joel Stanley
> <joel at jms.id.au>; Vijay Khemka <vijaykhemka at fb.com>
> Cc: ray.lue (呂郁銳 - MCT) <ray.lue at mic.com.tw>
> Subject: Re: [PATCH linux dev-5.0 v2 1/1] ARM: dts: aspeed: Add s7106 BMC
> Machine
> 
> 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.

Will add in V3 patch

> 
> > +};
> > +
> > +&uart3 {
> > +	status = "okay";
> 
> As above.

Will add in V3 patch

> 
> > +};
> > +
> > +&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.

Yes, I need to add below description or the LCLK will be disabled.

	memory-region = <&flash_memory>;
	flash = <&spi1>;

We can't relate this description to LCLK, so we think it's a bug and created an
another patch to add these two lines to update s7016 device tree. 
Sorry for the confusion made, I will add this into V3 patch directly.   

> 
> 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.
> 

Got it, will update the device pinctrl for all ADC lines.

> > +};
> > +
> > +&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?
> 

The driver is in /drivers/hwmon/nct7802.c, I don't know why it's not documented in 
/devicetree/bindings/hwmon. 

> > +		reg = <0x28>;
> > +	};
> > +};
> > +
> > +&i2c1 {
> > +	status = "okay";
> > +};
> > +
> > +&i2c2 {
> > +	status = "okay";
> > +
> > +	eeprom at 50 {
> > +		compatible = "at,24c256";
> 
> Compatible also isn't documented anywhere. Same queries as above.
> 

Sorry for using the deprecated term, will update to "atmel".


> > +		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


More information about the openbmc mailing list