[PATCH linux dev-4.10] ARM: dts: aspeed: Add ARM system BMC device tree
Chen.KenYY 陳永營 TAO
Chen.KenYY at inventec.com
Mon Jan 22 17:02:40 AEDT 2018
Hi Joel,
Thanks you.
Before I update new patch, I have some question.
Should I remove the definition which the driver didn't update now?
The related driver added or modified will be update in future.
The watchdog is check for BMC and avoid BMC boot error, if we didn't stop this, BMC chip will boot second BMC SPI ROM.
Does the driver support this? Could you give me an example or advice?
The multi-function is enable SPI. It might modify driver.
What is the mean of "Can you use a GPIO hog instead?" ?
Hi Mohit and Harold,
Do you have any advice for the company name?
Thanks.
Ken
-----Original Message-----
From: joel.stan at gmail.com [mailto:joel.stan at gmail.com] On Behalf Of Joel Stanley
Sent: Wednesday, January 17, 2018 8:18 AM
To: Chen.KenYY 陳永營 TAO; Brad Bishop; Andrew Jeffery
Cc: OpenBMC Maillist
Subject: Re: [PATCH linux dev-4.10] ARM: dts: aspeed: Add ARM system BMC device tree
Hello Ken,
Thanks for the patch. Some comments are below. Feel free to ask questions if you need more information.
On Sun, Jan 7, 2018 at 9:00 PM, Ken Chen <chen.kenyy at inventec.com> wrote:
> Add 2400-rep dts and modify platform initial in aspeed.c
What is 2400-rep stand for?
I can see it being confused with the ast2400. Do you have other suggestions for names for this machine?
>
> Signed-off-by: Ken Chen <chen.kenyy at inventec.com>
> ---
> arch/arm/boot/dts/aspeed-bmc-arm-2400-rep.dts | 440 ++++++++++++++++++++++++++
> arch/arm/mach-aspeed/aspeed.c | 22 ++
> 2 files changed, 462 insertions(+)
> create mode 100644 arch/arm/boot/dts/aspeed-bmc-arm-2400-rep.dts
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-2400-rep.dts
> b/arch/arm/boot/dts/aspeed-bmc-arm-2400-rep.dts
> new file mode 100644
> index 0000000..cf389f5
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-arm-2400-rep.dts
> @@ -0,0 +1,440 @@
Add copyright information to the first line. Something like this:
// SPDX-License-Identifier: GPL-2.0
// Copyright 2018 My Company, Inc
eg:
https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git/tree/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts?h=for-next
> +/dts-v1/;
> +
> +#include "aspeed-g5.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> + model = "Qualcomm Centriq 2400 REP AST2520";
> + compatible = "qualcomm,centriq2400-rep-bmc", "aspeed,ast2500";
> +
> + chosen {
> + stdout-path = &uart5;
> + bootargs = "console=ttyS4,115200 earlyprintk";
> + };
> +
> + memory {
> + reg = <0x80000000 0x40000000>;
> + };
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + flash_memory: region at 98000000 {
> + no-map;
> + reg = <0x98000000 0x04000000>; /* 64M */
We use this for an OpenPower specific feature that allows faster loading of the host firmware. Are you planning on doing the same with ARM?
> + };
> + };
> +
> + iio-hwmon {
> + compatible = "iio-hwmon";
> + io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> + <&adc 4>, <&adc 5>, <&adc 6>, <&adc 8>;
> + };
> +
> + iio-hwmon-battery {
> + compatible = "iio-hwmon";
> + io-channels = <&adc 7>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> +
> + UID_LED {
lowercase for the node please:
uid_led {
}
> + label = "UID_LED";
I think we have a naming convention for openbmc led labels. Brad, can you help out here?
> + gpios = <&gpio ASPEED_GPIO(Q, 5) GPIO_ACTIVE_LOW>;
> + };
> +
> + RAS_ERROR_LED {
> + label = "RAS_ERROR_LED";
> + gpios = <&gpio ASPEED_GPIO(F, 6) GPIO_ACTIVE_LOW>;
> + };
> +
> + System_Fault {
> + label = "System_fault";
> + gpios = <&gpio ASPEED_GPIO(A, 1) GPIO_ACTIVE_LOW>;
> + };
> + };
> +};
> +
> +&fmc {
> + status = "okay";
> + flash at 0 {
> + status = "okay";
> + m25p,fast-read;
add:
label = "bmc";
> +#include "openbmc-flash-layout.dtsi"
> + };
> +};
> +
> +&spi1 {
> + reg = <0x1e630000 0xc4 0x30000000 0x08000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "aspeed,ast2500-spi";
> + status = "okay";
> + clocks = <&clk_ahb>;
> + clock-names = "ahb";
> + flash at 0 {
> + reg = < 0 >; /* chip select number */
> + compatible = "jedec,spi-nor";
> + status = "okay";
> + };
This is all defined in the aspeed-g5.dtsi. You just need this to enable the controller and the first chip select:
&spi1 {
status = "okay";
flash at 0 {
status = "okay";
m25p,fast-read;
label = "pnor";
};
};
> +};
> +
> +&spi2 {
> + compatible = "aspeed,ast2500-spi-generic";
> + reg = <0x1e631000 0xc4 0x38000000 0x02000000>;
> + interrupts = <59>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "okay";
> + spi2 at 0 {
> + reg = < 0 >;
> + compatible = "centriq,centriq2400-spi-hwmon";
That's kind of strange. I assume you have a driver for this?
Either way, the device tree is incorrect.
> + spi-max-frequency = <16000000>;
> + status = "okay";
> + };
> +};
> +
> +&uart3 {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_txd3_default
> + &pinctrl_rxd3_default>;
> + current-speed = <115200>;
> +};
> +
> +&lpc_ctrl {
> + status = "okay";
> + memory-region = <&flash_memory>;
> + flash = <&spi1>;
> +};
> +
> +&uart5 {
> + status = "okay";
> +};
> +
> +&mac0 {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
> +};
> +
> +&i2c0 {
> + status = "okay";
> +
> + pca9542 at 70 {
> + compatible = "pca9542";
> + reg = <0x70>;
> + i2c at 0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + pca9546 at 77 {
> + compatible = "pca9546";
> + reg = <0x77>;
> + i2c at 0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + eeprom at 52 {
> + compatible = "atmel,24c02";
> + reg = <0x52>;
> + };
> + };
> + i2c at 2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> + eeprom at 57 {
> + compatible = "atmel,24c02";
> + reg = <0x57>;
> + };
> + };
> + };
> + };
> + i2c at 1 {
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> + pca9546 at 77 {
> + compatible = "pca9546";
> + reg = <0x77>;
> + i2c at 2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> + eeprom at 57 {
> + compatible = "atmel,24c02";
> + reg = <0x57>;
> + };
> + };
> + };
> + };
> +
> + };
> +};
> +
> +&i2c1 {
> + status = "okay";
> +
> + tmp421 at 1e {
> + compatible = "ti,tmp421";
> + reg = <0x1e>;
> + };
> + tmp421 at 2a {
> + compatible = "ti,tmp421";
> + reg = <0x2a>;
> + };
> + tmp421 at 4e {
> + compatible = "ti,tmp421";
> + reg = <0x4e>;
> + };
> + tmp421 at 1c {
> + compatible = "ti,tmp421";
> + reg = <0x1c>;
> + };
> +};
> +
> +&i2c2 {
> + status = "okay";
> + ssif {
> + compatible = "ssif-smbus-slave";
There are no bindings that use this string. Do you have a driver for this?
> + reg = <0x42>;
> + };
> +
> +};
> +
> +&i2c3 {
> + status = "okay";
> +};
> +
> +&i2c4 {
> + status = "okay";
> +};
> +
> +&i2c5 {
> + status = "okay";
> +
> + ir38163 at 42 {
> + compatible = "ir38163";
There are no bindings that use this string. Do you have a driver for this?
> + reg = <0x42>;
> + };
> + ir38163 at 44 {
> + compatible = "ir38163";
> + reg = <0x44>;
> + };
> + ir38163 at 46 {
> + compatible = "ir38163";
> + reg = <0x46>;
> + };
> + ir38163 at 48 {
> + compatible = "ir38163";
> + reg = <0x48>;
> + };
> + pxm1310 at 02 {
> + compatible = "pxm1310";
Again, there are no bindings that use this string. Do you have a driver for this?
> + reg = <0x02>;
> + };
> + pxm1310 at 04 {
> + compatible = "pxm1310";
> + reg = <0x04>;
> + };
> +};
> +
> +&i2c6 {
> + status = "okay";
> +
> + tmp421 at 1d {
> + compatible = "ti,tmp421";
> + reg = <0x1d>;
> + };
> + tmp421 at 1f {
> + compatible = "ti,tmp421";
> + reg = <0x1f>;
> + };
> + tmp421 at 4d {
> + compatible = "ti,tmp421";
> + reg = <0x4d>;
> + };
> + tmp421 at 4f {
> + compatible = "ti,tmp421";
> + reg = <0x4f>;
> + };
> +
> + nvt210 at 4c {
> + compatible = "nvt210";
> + reg = <0x4c>;
> + };
> +
> + eeprom at 50 {
> + compatible = "atmel,24c128";
> + reg = <0x50>;
> + pagesize = <128>;
Whitespace is bad here.
> + };
> +};
> +
> +&i2c7 {
> + status = "okay";
> +
> + adm1278 at 10 {
> + compatible = "adi,adm1278";
> + reg = <0x10>;
> + Rsense = <250>;
> + };
> + adm1278 at 11 {
> + compatible = "adi,adm1278";
> + reg = <0x11>;
> + Rsense = <250>;
This looks wrong. Is this a custom modification to the driver to support this property?
> + };
> +};
> +
> +&i2c8 {
> + status = "okay";
> +
> + pca9641 at 70 {
> + compatible = "nxp,pca9641";
> + reg = <0x70>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + i2c-arb{
> + tmp421 at 1d {
> + compatible = "tmp421";
> + reg = <0x1d>;
> + };
> + adm1278 at 12 {
> + compatible = "adi,adm1278";
> + reg = <0x12>;
> + Rsense = <500>;
> + };
> + max31790 at 20 {
> + compatible = "max31790";
> + reg = <0x20>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + fan-mode = "pwm";
> + fanconfig1 = <0x08>;
> + fanconfig2 = <0x09>;
> + fanconfig3 = <0x08>;
> + fanconfig4 = <0x08>;
> + fanconfig5 = <0x09>;
> + fanconfig6 = <0x08>;
> + };
Andrew, can you please review the max parts?
> + max31790 at 23 {
> + compatible = "max31790";
> + reg = <0x23>;
> + #address-cells = <2>;
> + #size-cells = <0>;
> + fan-mode = "pwm";
> + fanconfig1 = <0x08>;
> + fanconfig2 = <0x09>;
> + fanconfig3 = <0x08>;
> + fanconfig4 = <0x08>;
> + fanconfig5 = <0x09>;
> + fanconfig6 = <0x08>;
> + };
> + eeprom at 50 {
> + compatible = "atmel,24c02";
> + reg = <0x50>;
> + };
> + ds1100 at 58 {
> + compatible = "ds1100";
> + reg = <0x58>;
> + };
> + };
> + };
> +};
> +
> +&i2c9 {
> + status = "okay";
> +};
> +
> +&i2c10 {
> + status = "disabled";
> +};
> +
> +&i2c11 {
> + status = "disabled";
> +};
> +
> +&i2c12 {
> + status = "disabled";
> +};
> +
> +&i2c13 {
> + status = "disabled";
> +};
Delete the above; they are disabled by default so you don't need to specify this.
> +&vuart {
> + status = "okay";
> +};
> +
> +&gfx {
> + status = "okay";
> +};
> +
> +&pinctrl {
> + aspeed,external-nodes = <&gfx &lhc>; }; &gpio {
> + pin_gpio_a3 {
Do you want to add labels that contain the net names for each gpio? I recommend doing this so it is understood what each line is doing.
> + gpios = <ASPEED_GPIO(A, 3) GPIO_ACTIVE_HIGH>;
> + output-low;
> + };
> + pin_gpio_c7 {
> + gpios = <ASPEED_GPIO(C, 7) GPIO_ACTIVE_HIGH>;
> + output-low;
> + };
> + pin_gpio_d2 {
> + gpios = <ASPEED_GPIO(D, 2) GPIO_ACTIVE_HIGH>;
> + output-high;
> + };
> + pin_gpio_d3 {
> + gpios = <ASPEED_GPIO(D, 3) GPIO_ACTIVE_LOW>;
> + output-high;
> + };
> + pin_gpio_f6 {
> + gpios = <ASPEED_GPIO(F, 6) GPIO_ACTIVE_LOW>;
> + output-high;
> + };
> + pin_gpio_g4 {
> + gpios = <ASPEED_GPIO(G, 4) GPIO_ACTIVE_LOW>;
> + output-low;
> + };
> + pin_gpio_g6 {
> + gpios = <ASPEED_GPIO(G, 6) GPIO_ACTIVE_HIGH>;
> + output-low;
> + };
> + pin_gpio_l4 {
> + gpios = <ASPEED_GPIO(L, 4) GPIO_ACTIVE_LOW>;
> + output-high;
> + };
> + pin_gpio_o6 {
> + gpios = <ASPEED_GPIO(O, 6) GPIO_ACTIVE_HIGH>;
> + output-low;
> + };
> + pin_gpio_p0 {
> + gpios = <ASPEED_GPIO(P, 0) GPIO_ACTIVE_HIGH>;
> + output-high;
> + };
> + pin_gpio_p2 {
> + gpios = <ASPEED_GPIO(P, 2) GPIO_ACTIVE_HIGH>;
> + output-low;
> + };
> + pin_gpio_q4 {
> + gpios = <ASPEED_GPIO(Q, 4) GPIO_ACTIVE_HIGH>;
> + output-low;
> + };
> + pin_gpio_r1 {
> + gpios = <ASPEED_GPIO(R, 1) GPIO_ACTIVE_LOW>;
> + output-high;
> + };
> + pin_gpio_y2 {
> + gpios = <ASPEED_GPIO(Y, 2) GPIO_ACTIVE_HIGH>;
> + output-high;
> + };
> +};
> diff --git a/arch/arm/mach-aspeed/aspeed.c
> b/arch/arm/mach-aspeed/aspeed.c index 5965bbf..4f6a5e9 100644
> --- a/arch/arm/mach-aspeed/aspeed.c
> +++ b/arch/arm/mach-aspeed/aspeed.c
Device tree changes should go in a separate to code changes.
> @@ -29,6 +29,7 @@
> #define AST_BASE_MAC0 0X1E660000 /* MAC 1 */
> #define AST_BASE_SCU 0x1E6E2000 /* System Control Unit (SCU) */
> #define AST_BASE_GPIO 0x1E780000 /* GPIO Controller */
> +#define AST_BASE_WDT 0x1E785000 /* Watchdog Timer */
>
> static struct map_desc aspeed_io_desc[] __initdata __maybe_unused = {
> {
> @@ -232,6 +233,25 @@ static void __init do_mellanox_setup(void)
> writel(reg, AST_IO(AST_BASE_SCU | 0x48)); }
>
> +
> +static void __init do_centriq2400rep_setup(void) {
> + u32 reg;
> +
> + do_common_setup();
> + writel(0x00000010, AST_IO(AST_BASE_WDT | 0x2c));
We have a watchdog driver for modifying the watchdog. Please use that instead of this line.
> +
> + writel(0xff000000, AST_IO(AST_BASE_SCU | 0x88));
This appears to be modifying the multi function pin control. What are you trying to do?
We should be able to use the pinctrl driver to modify these settings.
> +
> + /* Set GPIOC7 to output high to init host spi driver */
> + reg = readl(AST_IO(AST_BASE_GPIO | 0x04)) | BIT(23);
> + writel(reg, AST_IO(AST_BASE_GPIO | 0x04));
> + reg = readl(AST_IO(AST_BASE_GPIO)) | BIT(23);
> + writel(reg, AST_IO(AST_BASE_GPIO));
Can you use a GPIO hog instead?
> +}
> +
> +
> #define SCU_PASSWORD 0x1688A8A8
>
> static void __init aspeed_init_early(void) @@ -284,6 +304,8 @@ static
> void __init aspeed_init_early(void)
> do_lanyang_setup();
> if (of_machine_is_compatible("mellanox,msn-bmc"))
> do_mellanox_setup();
> + if (of_machine_is_compatible("qualcomm,centriq2400-rep-bmc"))
> + do_centriq2400rep_setup();
> }
>
> static void __init aspeed_map_io(void)
> --
> 2.9.3
>
More information about the openbmc
mailing list