[PATCH linux dev-4.13 v4] ARM: dts: aspeed: Add ARM system BMC device tree
ChenKenYY 陳永營 TAO
chen.kenyy at inventec.com
Mon Feb 12 22:48:57 AEDT 2018
Hi Joel,
2018-02-12 15:53 GMT+08:00 Joel Stanley <joel at jms.id.au>:
> Hi Ken,
>
> This is looking much better. Just a few small things to tweak before I
> can merge this.
>
> On Mon, Feb 12, 2018 at 1:25 PM, Ken Chen <chen.kenyy at inventec.com> wrote:
>> Add centriq2400-rep dts and modify platform initial
>
> This commit message needs some work.
>
> Take a look at the commit messages from the introduction of other machines:
>
>> commit 8f9bafbb92c0308cf8d33536803c822e14bed4d7
>> Author: Joel Stanley <joel at jms.id.au>
>> Date: Wed Jan 4 16:31:28 2017
>>
>> ARM: dts: aspeed: Add Romulus BMC platform
>>
>> Romulus is an OpenPower machine with an ast2500 BMC. It has NCSI
>> networking and 512MB of RAM.
>
> or
>
>> commit f88bc8e15f1c1154495107ed378cce271309349d
>> Author: Rick Altherr <raltherr at google.com>
>> Date: Tue Nov 28 23:11:05 2017
>>
>> ARM: dts: aspeed: Add Qanta Q71L BMC machine
>>
>> The Qanta Q71L BMC is an ASPEED ast2400 based BMC that is part of a
>> Qanta x86 server.
>
>
>>
>> Signed-off-by: Ken Chen <chen.kenyy at inventec.com>
>>
>> ---
>> v3->v4
>> - Modify Makefile for centriq2400-rep dts
>> - Fix typo label
>> - Removed unnecessary define of spi2 and centriq2400-spi-hwmon device
>> - Removed lpc device
>> - Fix coding style
>> - Removed ssif device
>> - Removed max31790 device
>> - Removed register control of GPIO and Watchdog
>
> Great changelog. This is very useful to me as a reviewer.
>
>> ---
>> arch/arm/boot/dts/Makefile | 1 +
>> .../boot/dts/aspeed-bmc-arm-centriq2400-rep.dts | 276 +++++++++++++++++++++
>> arch/arm/mach-aspeed/aspeed.c | 11 +
>> 3 files changed, 288 insertions(+)
>> create mode 100644 arch/arm/boot/dts/aspeed-bmc-arm-centriq2400-rep.dts
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 7c54fc8..77aef19 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -993,6 +993,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += aspeed-bmc-opp-palmetto.dtb \
>> aspeed-bmc-mellanox-msn.dtb \
>> aspeed-bmc-quanta-q71l.dtb \
>> aspeed-bmc-intel-s2600wf.dtb \
>> + aspeed-bmc-arm-centriq2400-rep.dtb \
>> aspeed-ast2500-evb.dtb
>> endif
>>
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-arm-centriq2400-rep.dts b/arch/arm/boot/dts/aspeed-bmc-arm-centriq2400-rep.dts
>> new file mode 100644
>> index 0000000..6c7784f
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed-bmc-arm-centriq2400-rep.dts
>> @@ -0,0 +1,276 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +#include "aspeed-g5.dtsi"
>> +#include <dt-bindings/gpio/aspeed-gpio.h>
>> +
>> +/ {
>> + model = "Qualcomm Centriq 2400 REP AST2520";
>
> May I ask, what does REP mean?
>
REP is reference evaluation platform.
>> + compatible = "qualcomm,centriq2400-rep-bmc", "aspeed,ast2500";
>> +
>> + chosen {
>> + stdout-path = &uart5;
>> + bootargs = "console=ttyS4,115200 earlyprintk";
>> + };
>> +
>> + memory {
>> + reg = <0x80000000 0x40000000>;
>> + };
>> +
>> + 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 {
>> + label = "UID_LED";
>> + 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;
>> + label = "bmc";
>
> Still some strange whitespace here. Make sure your editor is using tabs.
>
>> +#include "openbmc-flash-layout.dtsi"
>> + };
>> +};
>> +
>> +&spi1 {
>> + status = "okay";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_spi1_default>;
>> + flash at 0 {
>> + status = "okay";
>> + };
>> +};
>> +
>> +&spi2 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_spi2ck_default &pinctrl_spi2miso_default &pinctrl_spi2mosi_default &pinctrl_spi2cs0_default>;
>
> Andrew, can you please review the pinctrl bits?
>
>> +};
>> +
>> +&uart3 {
>> + status = "okay";
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_txd3_default &pinctrl_rxd3_default>;
>> + current-speed = <115200>;
>> +};
>> +
>
>> diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c
>> index 5965bbf..8fce9bc 100644
>> --- a/arch/arm/mach-aspeed/aspeed.c
>> +++ b/arch/arm/mach-aspeed/aspeed.c
>
> We do not have an aspeed.c in the dev-4.13 branch.
>
> I do not plan on merging one, as at this stage all of the
> functionality that was present in aspeed.c can be performed in other
> drivers. I suggest submitting the device tree without any change to
> aspeed.c.
>
OK, I will remove this.
> To review what the changes are:
>
>> static void __init do_common_setup(void)
>> {
>> /* Enable LPC FWH cycles, Enable LPC to AHB bridge */
>> writel(0x00000500, AST_IO(AST_BASE_LPC | 0x80));
>
> This is now done in drivers/misc/aspeed-lpc-ctrl.c
>
>> /* Set UART routing */
>> writel(0x00000000, AST_IO(AST_BASE_LPC | 0x9c));
>
> This is the default, so we do not need have this line.
>
>>
>> /* Setup scratch registers */
>> writel(0x00000042, AST_IO(AST_BASE_LPC | 0x170));
>> writel(0x00008000, AST_IO(AST_BASE_LPC | 0x174));
>
> These are specific to the operation of Hostboot on OpenPower
> platforms. I suspect you do not require these lines.
>
> Cheers,
>
> Joel
>
>
>
>> @@ -232,6 +232,15 @@ 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();
>> +}
>> +
>> +
>> #define SCU_PASSWORD 0x1688A8A8
>>
>> static void __init aspeed_init_early(void)
>> @@ -284,6 +293,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