[PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine
ray.lue at mic.com.tw
ray.lue at mic.com.tw
Wed May 29 20:14:48 AEST 2019
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 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.
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 at aj.id.au]
> Sent: Wednesday, May 29, 2019 9:18 AM
> To: Ray Lue <ray.lue at gmail.com>; openbmc at lists.ozlabs.org; Joel Stanley
> <joel at jms.id.au>
> Cc: ray.lue (呂郁銳 - MCT) <ray.lue at 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 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