[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