Re: [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine
Andrew Jeffery
andrew at aj.id.au
Wed May 29 11:17:57 AEST 2019
Hi Ray,
Thanks for the patch - it looks pretty good overall, but I have a few comments below:
On Tue, 28 May 2019, at 16:11, 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 | 351 ++++++++++++++++++++++++++++
> 2 files changed, 353 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 1276167..72115e0 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1262,4 +1262,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..9f549c6
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-tyan-s7106.dts
> @@ -0,0 +1,351 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Device Tree file for Tyan S7106 platform
> +/dts-v1/;
> +
> +#include "aspeed-g5.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> + model = "Tyan S7106 BMC";
> + compatible = "tyan,s7106-bmc", "aspeed,ast2500";
> +
> + chosen {
> + stdout-path = &uart5;
> + bootargs = "earlyprintk";
> + };
> +
> + memory {
> + reg = <0x80000000 0x20000000>;
> + };
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + flash_memory: region at 98000000 {
> + no-map;
> + reg = <0x98000000 0x04000000>; /* 64M */
> + };
I expect you don't need the functionality this provides. There are some patches
that make the reserved memory property optional for the aspeed-lpc-ctrl node,
but they're not yet upstream.
I'll try to fix that, but in the mean time you could significantly reduce the
amount of memory you're setting aside here.
> +
> + vga_memory: framebuffer at 7f000000 {
> + no-map;
> + reg = <0x7f000000 0x01000000>;
> + };
> +
> + safs_memory: region at 30000000 {
> + no-map;
> + reg = <0x30000000 0x08000000>; /* 128M */
> + };
> +
> + gfx_memory: framebuffer {
> + size = <0x04000000>;
> + alignment = <0x01000000>;
> + compatible = "shared-dma-pool";
> + reusable;
> + };
> + };
> +
> + vga-shared-memory {
> + compatible = "aspeed,ast2500-vga-sharedmem";
> + reg = <0x9ff00000 0x100000>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> +
> + power {
> + gpios = <&gpio ASPEED_GPIO(R, 5) GPIO_ACTIVE_LOW>;
> + };
> +
> + identify {
> + gpios = <&gpio ASPEED_GPIO(A, 2) GPIO_ACTIVE_LOW>;
> + };
> +
> + heartbeat {
> + gpios = <&gpio ASPEED_GPIO(E, 7) GPIO_ACTIVE_LOW>;
> + };
> + };
> +
> + gpio-keys {
> + compatible = "gpio-keys";
> +
> + caterr {
> + label = "caterr";
> + gpios = <&gpio ASPEED_GPIO(AB, 0) GPIO_ACTIVE_LOW>;
> + linux,code = <ASPEED_GPIO(AB, 0)>;
> + };
> +
> + id-button {
> + label = "id-button";
> + gpios = <&gpio ASPEED_GPIO(C, 4) GPIO_ACTIVE_LOW>;
> + linux,code = <ASPEED_GPIO(C, 4)>;
> + };
> + };
> +
> + 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>;
> + };
> +};
> +
> +&fmc {
> + status = "okay";
> + flash at 0 {
> + label = "bmc";
> + status = "okay";
> + m25p,fast-read;
> +#include "openbmc-flash-layout.dtsi"
> + };
> +};
> +
> +&spi1 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_spi1_default>;
> +
> + flash at 0 {
> + status = "okay";
> + m25p,fast-read;
> + label = "pnor";
> + };
> +};
> +
> +// 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";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_txd1_default
> + &pinctrl_rxd1_default
> + &pinctrl_ncts1_default
> + &pinctrl_ndcd1_default
> + &pinctrl_ndsr1_default
> + &pinctrl_nri1_default
> + &pinctrl_ndtr1_default
> + &pinctrl_nrts1_default>;
> +};
> +
> +// Host UART2
> +&uart2 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_txd2_default
> + &pinctrl_rxd2_default
> + &pinctrl_ncts2_default
> + &pinctrl_ndcd2_default
> + &pinctrl_ndsr2_default
> + &pinctrl_nri2_default
> + &pinctrl_ndtr2_default
> + &pinctrl_nrts2_default>;
> +};
> +
> +// SOL,route to UART1 or UART2
> +&uart3 {
> + status = "okay";
> +};
> +
> +&uart5 {
> + status = "okay";
> +};
> +
> +&mac0 {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_rmii1_default>;
> + use-ncsi;
> +};
> +
> +&mac1 {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_rgmii2_default &pinctrl_mdio2_default>;
> +};
> +
> +&i2c0 {
> + status = "okay";
> + // NCT7802 Hardware Monitor @50h
> + nct7802 at 28 {
> + compatible = "nuvoton,nct7802";
> + reg = <0x28>;
> + };
> +};
> +
> +// IPMB, dedicated for AST2500 <-> Intel PCH ME
> +&i2c1 {
> + multi-master =<1>;
The documentation implies that this is a simple boolean property. I don't think
you have to assign it the value '1', the property just needs to be present (e.g. see
'use-ncsi' in &mac0 above).
> + status = "okay";
> + spsfw1 at 10 {
> + compatible = "slave-mqueue";
This compatible isn't documented anywhere. Do you have a bindings document
for it? We'll need that before we can merge the devicetree.
> + reg = <0x10>;
> + message-size = <256>;
> + queue-size = <32>;
> + };
> +};
> +
> +&i2c2 {
> + status = "okay";
> + // FRU 24C256 @50h
> + eeprom at 50 {
> + compatible = "atmel,24c256";
> + reg = <0x50>;
> + pagesize = <32>;
> + };
> +};
> +
> +&i2c3 {
> + status = "okay";
> + power-supply at 58 {
> + compatible = "ibm,cffps1";
Is this a copy/paste error?
> + reg = <0x58>;
> + };
> +};
> +
> +&i2c4 {
> + status = "okay";
> +};
> +
> +&i2c5 {
> + status = "okay";
> +};
> +
> +&i2c6 {
> + status = "okay";
> +};
> +
> +&i2c7 {
> + status = "okay";
> +};
> +
> +&gfx {
> + status = "okay";
> + memory-region = <&gfx_memory>;
> +};
> +
> +&video {
> + status = "okay";
> + memory-region = <&gfx_memory>;
> +};
> +
> +&vhub {
> + 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>;
Need to mux pwm6 as well according to rearfan at 2 below.
> +
> + cpufan at 0 {
> + reg = <0x00>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x00>;
> + };
> +
> + cpufan at 1 {
> + reg = <0x01>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x01>;
> + };
> +
> + frontfan at 1 {
> + reg = <0x02>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x02>;
> + };
> +
> + frontfan at 2 {
> + reg = <0x03>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x03>;
> + };
> +
> + frontfan at 3 {
> + reg = <0x04>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x04>;
> + };
> +
> + rearfan at 1 {
> + reg = <0x04>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x05>;
> + };
> +
> + rearfan at 2 {
> + reg = <0x04>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x06>;
> + };
> +};
> +
> +&lpc_ctrl {
> + status = "okay";
> + memory-region = <&flash_memory>;
> + flash = <&spi1>;
> +};
> +
> +&lpc_snoop {
> + status = "okay";
> + snoop-ports = <0x80>;
> +};
> +
> +&adc {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_adc0_default
> + &pinctrl_adc1_default
> + &pinctrl_adc2_default
> + &pinctrl_adc3_default
> + &pinctrl_adc4_default
> + &pinctrl_adc5_default
> + &pinctrl_adc6_default
> + &pinctrl_adc7_default
> + &pinctrl_adc8_default
> + &pinctrl_adc9_default
> + &pinctrl_adc10_default
> + &pinctrl_adc12_default
> + &pinctrl_adc13_default
> + &pinctrl_adc14_default>;
> +};
> +
> +&lpc_bmc {
> + compatible = "aspeed,ast2500-lpc-bmc", "simple-mfd", "syscon";
>
> + reg = <0x0 0x80>;
> + reg-io-width = <4>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x0 0x80>;
You don't need to specify the compatible line or any of the above properties,
this is done in the node that you're referencing.
Once you've addressed these issues it would be good to send the patch
upstream as well.
Cheers,
Andrew
> +
> + kcs1 at 0 {
> + compatible = "aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
> + interrupts = <8>;
> + kcs_chan = <1>;
> + kcs_addr = <0xca0>;
> + status = "okay";
> + };
> +
> + kcs2 at 0 {
> + compatible = "aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
> + interrupts = <8>;
> + kcs_chan = <2>;
> + kcs_addr = <0xca8>;
> + status = "okay";
> + };
> +
> + kcs3 at 0 {
> + compatible = "aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
> + interrupts = <8>;
> + kcs_chan = <3>;
> + kcs_addr = <0xca2>;
> + status = "okay";
> + };
> +};
> +
> --
> 2.7.4
>
>
More information about the openbmc
mailing list