RE: [PATCH linux dev-5.1 v1 1/1] ARM: dts: aspeed: Add s7106 BMC Machine

[<ray.lue@mic.com.tw>]

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