[PATCH linux dev-4.10] ARM: dts: aspeed: Add ARM system BMC device tree
Joel Stanley
joel at jms.id.au
Wed Jan 17 11:17:35 AEDT 2018
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