* [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine
@ 2019-05-28 6:39 Ray Lue
2019-05-29 1:17 ` Andrew Jeffery
0 siblings, 1 reply; 4+ messages in thread
From: Ray Lue @ 2019-05-28 6:39 UTC (permalink / raw)
To: openbmc, joel; +Cc: Ray Lue
s7106 is a Tyan platform family with an ASPEED AST2500 BMC
Signed-off-by: Ray Lue <ray.lue@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@98000000 {
+ no-map;
+ reg = <0x98000000 0x04000000>; /* 64M */
+ };
+
+ vga_memory: framebuffer@7f000000 {
+ no-map;
+ reg = <0x7f000000 0x01000000>;
+ };
+
+ safs_memory: region@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@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@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@28 {
+ compatible = "nuvoton,nct7802";
+ reg = <0x28>;
+ };
+};
+
+// IPMB, dedicated for AST2500 <-> Intel PCH ME
+&i2c1 {
+ multi-master =<1>;
+ status = "okay";
+ spsfw1@10 {
+ compatible = "slave-mqueue";
+ reg = <0x10>;
+ message-size = <256>;
+ queue-size = <32>;
+ };
+};
+
+&i2c2 {
+ status = "okay";
+ // FRU 24C256 @50h
+ eeprom@50 {
+ compatible = "atmel,24c256";
+ reg = <0x50>;
+ pagesize = <32>;
+ };
+};
+
+&i2c3 {
+ status = "okay";
+ power-supply@58 {
+ compatible = "ibm,cffps1";
+ 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>;
+
+ cpufan@0 {
+ reg = <0x00>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x00>;
+ };
+
+ cpufan@1 {
+ reg = <0x01>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x01>;
+ };
+
+ frontfan@1 {
+ reg = <0x02>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x02>;
+ };
+
+ frontfan@2 {
+ reg = <0x03>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x03>;
+ };
+
+ frontfan@3 {
+ reg = <0x04>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x04>;
+ };
+
+ rearfan@1 {
+ reg = <0x04>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x05>;
+ };
+
+ rearfan@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>;
+
+ kcs1@0 {
+ compatible = "aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <1>;
+ kcs_addr = <0xca0>;
+ status = "okay";
+ };
+
+ kcs2@0 {
+ compatible = "aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <2>;
+ kcs_addr = <0xca8>;
+ status = "okay";
+ };
+
+ kcs3@0 {
+ compatible = "aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <3>;
+ kcs_addr = <0xca2>;
+ status = "okay";
+ };
+};
+
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine
2019-05-28 6:39 [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine Ray Lue
@ 2019-05-29 1:17 ` Andrew Jeffery
2019-05-29 10:14 ` ray.lue
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Jeffery @ 2019-05-29 1:17 UTC (permalink / raw)
To: Ray Lue, openbmc, Joel Stanley; +Cc: Ray Lue
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@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@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@7f000000 {
> + no-map;
> + reg = <0x7f000000 0x01000000>;
> + };
> +
> + safs_memory: region@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@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@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@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@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@50 {
> + compatible = "atmel,24c256";
> + reg = <0x50>;
> + pagesize = <32>;
> + };
> +};
> +
> +&i2c3 {
> + status = "okay";
> + power-supply@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@2 below.
> +
> + cpufan@0 {
> + reg = <0x00>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x00>;
> + };
> +
> + cpufan@1 {
> + reg = <0x01>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x01>;
> + };
> +
> + frontfan@1 {
> + reg = <0x02>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x02>;
> + };
> +
> + frontfan@2 {
> + reg = <0x03>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x03>;
> + };
> +
> + frontfan@3 {
> + reg = <0x04>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x04>;
> + };
> +
> + rearfan@1 {
> + reg = <0x04>;
> + aspeed,fan-tach-ch = /bits/ 8 <0x05>;
> + };
> +
> + rearfan@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@0 {
> + compatible = "aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
> + interrupts = <8>;
> + kcs_chan = <1>;
> + kcs_addr = <0xca0>;
> + status = "okay";
> + };
> +
> + kcs2@0 {
> + compatible = "aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
> + interrupts = <8>;
> + kcs_chan = <2>;
> + kcs_addr = <0xca8>;
> + status = "okay";
> + };
> +
> + kcs3@0 {
> + compatible = "aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
> + interrupts = <8>;
> + kcs_chan = <3>;
> + kcs_addr = <0xca2>;
> + status = "okay";
> + };
> +};
> +
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine
2019-05-29 1:17 ` Andrew Jeffery
@ 2019-05-29 10:14 ` ray.lue
2019-05-30 5:09 ` Andrew Jeffery
0 siblings, 1 reply; 4+ messages in thread
From: ray.lue @ 2019-05-29 10:14 UTC (permalink / raw)
To: andrew, ray.lue, openbmc, joel
Hi Andrew,
Thank you for reviewing this dts, I will test and update it based on your recommendation.
Regarding the "slave-mqueue"
>
> > + status = "okay";
> > + spsfw1@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.
it's an Intel developed driver for IPMB, but yet upstream to kernel.
https://lore.kernel.org/patchwork/patch/934565/
https://github.com/Intel-BMC/openbmc/blob/a7715486507e75e4a7cee843a48067b15595defa/meta-openbmc-mods/meta-common/recipes-kernel/linux/linux-aspeed/0019-Add-I2C-IPMB-support.patch
However, it's a necessary driver for OpenBMC ipmbbridge service like below.
https://github.com/openbmc/ipmbbridge/blob/master/ipmb-channels.json
{
"channels": [
{
"type": "me",
"master-path": "/dev/i2c-5",
"slave-path": "/sys/bus/i2c/devices/5-1010/slave-mqueue",
"bmc-addr": 32,
"remote-addr": 44
},
{
"type": "ipmb",
"master-path": "/dev/i2c-0",
"slave-path": "/sys/bus/i2c/devices/0-1010/slave-mqueue",
"bmc-addr": 32,
"remote-addr": 88
}
]
}
The IPMB to ME is a very important channel for Intel based platform to retrieve critical sensors like CPU, DIMM, IO ...etc. What's your recommendation? should I remove it from dts until this driver approved by upstream?
Thanks,
Ray
> -----Original Message-----
> From: Andrew Jeffery [mailto:andrew@aj.id.au]
> Sent: Wednesday, May 29, 2019 9:18 AM
> To: Ray Lue <ray.lue@gmail.com>; openbmc@lists.ozlabs.org; Joel Stanley
> <joel@jms.id.au>
> Cc: ray.lue (呂郁銳 - MCT) <ray.lue@mic.com.tw>
> Subject: Re: [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC
> Machine
>
> 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@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@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@7f000000 {
> > + no-map;
> > + reg = <0x7f000000 0x01000000>;
> > + };
> > +
> > + safs_memory: region@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@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@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@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@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@50 {
> > + compatible = "atmel,24c256";
> > + reg = <0x50>;
> > + pagesize = <32>;
> > + };
> > +};
> > +
> > +&i2c3 {
> > + status = "okay";
> > + power-supply@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@2 below.
>
> > +
> > + cpufan@0 {
> > + reg = <0x00>;
> > + aspeed,fan-tach-ch = /bits/ 8 <0x00>;
> > + };
> > +
> > + cpufan@1 {
> > + reg = <0x01>;
> > + aspeed,fan-tach-ch = /bits/ 8 <0x01>;
> > + };
> > +
> > + frontfan@1 {
> > + reg = <0x02>;
> > + aspeed,fan-tach-ch = /bits/ 8 <0x02>;
> > + };
> > +
> > + frontfan@2 {
> > + reg = <0x03>;
> > + aspeed,fan-tach-ch = /bits/ 8 <0x03>;
> > + };
> > +
> > + frontfan@3 {
> > + reg = <0x04>;
> > + aspeed,fan-tach-ch = /bits/ 8 <0x04>;
> > + };
> > +
> > + rearfan@1 {
> > + reg = <0x04>;
> > + aspeed,fan-tach-ch = /bits/ 8 <0x05>;
> > + };
> > +
> > + rearfan@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@0 {
> > + compatible = "aspeed,ast2500-kcs-bmc";
> > + reg = <0x0 0x80>;
> > + interrupts = <8>;
> > + kcs_chan = <1>;
> > + kcs_addr = <0xca0>;
> > + status = "okay";
> > + };
> > +
> > + kcs2@0 {
> > + compatible = "aspeed,ast2500-kcs-bmc";
> > + reg = <0x0 0x80>;
> > + interrupts = <8>;
> > + kcs_chan = <2>;
> > + kcs_addr = <0xca8>;
> > + status = "okay";
> > + };
> > +
> > + kcs3@0 {
> > + compatible = "aspeed,ast2500-kcs-bmc";
> > + reg = <0x0 0x80>;
> > + interrupts = <8>;
> > + kcs_chan = <3>;
> > + kcs_addr = <0xca2>;
> > + status = "okay";
> > + };
> > +};
> > +
> > --
> > 2.7.4
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine
2019-05-29 10:14 ` ray.lue
@ 2019-05-30 5:09 ` Andrew Jeffery
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Jeffery @ 2019-05-30 5:09 UTC (permalink / raw)
To: Ray Lue, Ray Lue, openbmc, Joel Stanley
Hi Ray,
On Wed, 29 May 2019, at 19:45, ray.lue@mic.com.tw wrote:
> Hi Andrew,
>
> Thank you for reviewing this dts, I will test and update it based on
> your recommendation.
Great!
>
> Regarding the "slave-mqueue"
>
> >
> > > + status = "okay";
> > > + spsfw1@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.
>
> it's an Intel developed driver for IPMB, but yet upstream to kernel.
>
> https://lore.kernel.org/patchwork/patch/934565/
> https://github.com/Intel-BMC/openbmc/blob/a7715486507e75e4a7cee843a48067b15595defa/meta-openbmc-mods/meta-common/recipes-kernel/linux/linux-aspeed/0019-Add-I2C-IPMB-support.patch
>
> However, it's a necessary driver for OpenBMC ipmbbridge service like below.
>
> https://github.com/openbmc/ipmbbridge/blob/master/ipmb-channels.json
> {
> "channels": [
> {
> "type": "me",
> "master-path": "/dev/i2c-5",
> "slave-path": "/sys/bus/i2c/devices/5-1010/slave-mqueue",
> "bmc-addr": 32,
> "remote-addr": 44
> },
> {
> "type": "ipmb",
> "master-path": "/dev/i2c-0",
> "slave-path": "/sys/bus/i2c/devices/0-1010/slave-mqueue",
> "bmc-addr": 32,
> "remote-addr": 88
> }
> ]
> }
>
> The IPMB to ME is a very important channel for Intel based platform to
> retrieve critical sensors like CPU, DIMM, IO ...etc. What's your
> recommendation? should I remove it from dts until this driver approved
> by upstream?
My recommendation here is to split the patch. In the first patch, add everything
you have here except for the IPMB node. Then in the following patch, add the
IPMB node. This way you can send the first patch upstream while you harass
Intel to upstream the IPMB bindings and driver.
Cheers,
Andrew
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-30 5:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-28 6:39 [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine Ray Lue
2019-05-29 1:17 ` Andrew Jeffery
2019-05-29 10:14 ` ray.lue
2019-05-30 5:09 ` Andrew Jeffery
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.