| Message ID | 20240110033543.799919-1-supreeth.venkatesh@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] ARM:dts:aspeed: Initial device tree for AMD Onyx Platform | expand |
On Thu, 2024-02-08 at 08:57 -0600, Supreeth Venkatesh wrote: > This patch is pending for a month now. > > Can DT maintainers please help review this and provide feedback? Joel's on leave and I'm having to manage some personal concerns as well as other professional priorities. I'm trying to find time to address the BMC patch backlog but it really is a best-effort thing at the moment. Andrew
On 08/02/2024 15:57, Supreeth Venkatesh wrote: > This patch is pending for a month now. > > Can DT maintainers please help review this and provide feedback? I don't have the original patch in my inbox anymore, so probably you should resend. Anyway quick look points to obvious issues (comments below). > > On 1/9/24 21:35, Supreeth Venkatesh wrote: >> This patch adds initial device tree and makefile updates for >> AMD Onyx platform. >> >> AMD Onyx platform is an AMD customer reference board with an Aspeed >> ast2600 BMC manufactured by AMD. >> It describes I2c devices, Fans, Kcs devices, Uarts, Mac, LEDs, etc. >> present on AMD Onyx platform. >> >> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@amd.com> >> --- >> arch/arm/boot/dts/aspeed/Makefile | 1 + >> .../boot/dts/aspeed/aspeed-bmc-amd-onyx.dts | 98 +++++++++++++++++++ >> 2 files changed, 99 insertions(+) >> create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts >> >> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile >> index fb9cc95f1b60..2b27d377aae2 100644 >> --- a/arch/arm/boot/dts/aspeed/Makefile >> +++ b/arch/arm/boot/dts/aspeed/Makefile >> @@ -5,6 +5,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \ >> aspeed-ast2600-evb.dtb \ >> aspeed-bmc-amd-daytonax.dtb \ >> aspeed-bmc-amd-ethanolx.dtb \ >> + aspeed-bmc-amd-onyx.dtb \ >> aspeed-bmc-ampere-mtjade.dtb \ >> aspeed-bmc-ampere-mtmitchell.dtb \ >> aspeed-bmc-arm-stardragon4800-rep2.dtb \ >> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts >> new file mode 100644 >> index 000000000000..a7056cd29553 >> --- /dev/null >> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts >> @@ -0,0 +1,98 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +// Copyright (c) 2021 - 2024 AMD Inc. >> +// Author: Supreeth Venkatesh <supreeth.venkatesh@amd.com> >> + >> +/dts-v1/; >> + >> +#include "aspeed-g6.dtsi" >> +#include <dt-bindings/gpio/aspeed-gpio.h> >> + >> +/ { >> + model = "AMD Onyx BMC"; >> + compatible = "amd,onyx-bmc", "aspeed,ast2600"; Undocumented compatibles. Please run scripts/checkpatch.pl and fix reported warnings. Some warnings can be ignored, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. >> + >> + aliases { >> + serial0 = &uart1; >> + serial4 = &uart5; >> + }; >> + >> + chosen { >> + stdout-path = &uart5; >> + bootargs = "console=ttyS4,115200 earlyprintk vmalloc=512MB"; earlyprintk is debugging, not for mainline, so drop. Console should be encoded in stdout-path. vmalloc looks like OS tuning, so also not suitable for mainline DTS. >> + }; >> + >> + memory@80000000 { >> + device_type = "memory"; >> + reg = <0x80000000 0x80000000>; >> + }; >> + Drop redundant blank lines. >> +}; >> + >> +&mdio0 { >> + status = "okay"; >> + >> + ethphy0: ethernet-phy@0 { >> + compatible = "ethernet-phy-ieee802.3-c22"; >> + reg = <0>; >> + }; >> +}; >> + >> +&mac3 { >> + status = "okay"; >> + phy-mode = "rgmii"; >> + phy-handle = <ðphy0>; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_rgmii4_default>; >> +}; >> + >> +>> + Drop redundant blank lines. You can open existing, recent DTS from maintained platforms like Qcom or TI and look there at coding style. Best regards, Krzysztof
On Tue, 2024-01-09 at 21:35 -0600, Supreeth Venkatesh wrote: > This patch adds initial device tree and makefile updates for > AMD Onyx platform. > > AMD Onyx platform is an AMD customer reference board with an Aspeed > ast2600 BMC manufactured by AMD. > It describes I2c devices, Fans, Kcs devices, Uarts, Mac, LEDs, etc. > present on AMD Onyx platform. > > Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@amd.com> > --- > arch/arm/boot/dts/aspeed/Makefile | 1 + > .../boot/dts/aspeed/aspeed-bmc-amd-onyx.dts | 98 +++++++++++++++++++ > 2 files changed, 99 insertions(+) > create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts > > diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile > index fb9cc95f1b60..2b27d377aae2 100644 > --- a/arch/arm/boot/dts/aspeed/Makefile > +++ b/arch/arm/boot/dts/aspeed/Makefile > @@ -5,6 +5,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \ > aspeed-ast2600-evb.dtb \ > aspeed-bmc-amd-daytonax.dtb \ > aspeed-bmc-amd-ethanolx.dtb \ > + aspeed-bmc-amd-onyx.dtb \ > aspeed-bmc-ampere-mtjade.dtb \ > aspeed-bmc-ampere-mtmitchell.dtb \ > aspeed-bmc-arm-stardragon4800-rep2.dtb \ > diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts > new file mode 100644 > index 000000000000..a7056cd29553 > --- /dev/null > +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts > @@ -0,0 +1,98 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +// Copyright (c) 2021 - 2024 AMD Inc. > +// Author: Supreeth Venkatesh <supreeth.venkatesh@amd.com> > + > +/dts-v1/; > + > +#include "aspeed-g6.dtsi" > +#include <dt-bindings/gpio/aspeed-gpio.h> > + > +/ { > + model = "AMD Onyx BMC"; > + compatible = "amd,onyx-bmc", "aspeed,ast2600"; > + > + aliases { > + serial0 = &uart1; > + serial4 = &uart5; > + }; > + > + chosen { > + stdout-path = &uart5; > + bootargs = "console=ttyS4,115200 earlyprintk vmalloc=512MB"; Why `vmalloc=512MB`? Can you add a comment explaining the need for that? That said, setting bootargs in the devicetree is generally discouraged. > + }; > + > + memory@80000000 { > + device_type = "memory"; > + reg = <0x80000000 0x80000000>; > + }; > + > +}; > + > +&mdio0 { > + status = "okay"; > + > + ethphy0: ethernet-phy@0 { > + compatible = "ethernet-phy-ieee802.3-c22"; > + reg = <0>; > + }; > +}; > + > +&mac3 { > + status = "okay"; > + phy-mode = "rgmii"; > + phy-handle = <ðphy0>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_rgmii4_default>; > +}; > + > + > + > +&fmc { > + status = "okay"; > + flash@0 { > + compatible = "jedec,spi-nor"; > + status = "okay"; > + #include "openbmc-flash-layout-128.dtsi" > + }; > +}; > + > +//Host Console > +&uart1 { > + status = "okay"; > +}; How are you managing the host console? Enabling UART1 for that on the BMC piques my interest :) Apologies for the delay in reviewing. Andrew
Dear Supreeth, Thank you for your patch. Should you resent three more nits. Am 26.02.24 um 07:55 schrieb Andrew Jeffery: > On Tue, 2024-01-09 at 21:35 -0600, Supreeth Venkatesh wrote: >> This patch adds initial device tree and makefile updates for >> AMD Onyx platform. >> >> AMD Onyx platform is an AMD customer reference board with an Aspeed >> ast2600 BMC manufactured by AMD. >> It describes I2c devices, Fans, Kcs devices, Uarts, Mac, LEDs, etc. >> present on AMD Onyx platform. I’d capitalize I2Cs, UARTs, and MAC, but would write fans. Also, could you please make the summary a statement by adding a verb in imperative mood. Maybe: Add initial device tree for AMD Onyx Platform I’d also add spaces after the colons in the commit message summary. […] Kind regards, Paul
On 01/03/2024 22:02, Supreeth Venkatesh wrote: >>>> --- >>>> arch/arm/boot/dts/aspeed/Makefile | 1 + >>>> .../boot/dts/aspeed/aspeed-bmc-amd-onyx.dts | 98 +++++++++++++++++++ >>>> 2 files changed, 99 insertions(+) >>>> create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts >>>> >>>> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile >>>> index fb9cc95f1b60..2b27d377aae2 100644 >>>> --- a/arch/arm/boot/dts/aspeed/Makefile >>>> +++ b/arch/arm/boot/dts/aspeed/Makefile >>>> @@ -5,6 +5,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \ >>>> aspeed-ast2600-evb.dtb \ >>>> aspeed-bmc-amd-daytonax.dtb \ >>>> aspeed-bmc-amd-ethanolx.dtb \ >>>> + aspeed-bmc-amd-onyx.dtb \ >>>> aspeed-bmc-ampere-mtjade.dtb \ >>>> aspeed-bmc-ampere-mtmitchell.dtb \ >>>> aspeed-bmc-arm-stardragon4800-rep2.dtb \ >>>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts >>>> new file mode 100644 >>>> index 000000000000..a7056cd29553 >>>> --- /dev/null >>>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts >>>> @@ -0,0 +1,98 @@ >>>> +// SPDX-License-Identifier: GPL-2.0+ >>>> +// Copyright (c) 2021 - 2024 AMD Inc. >>>> +// Author: Supreeth Venkatesh <supreeth.venkatesh@amd.com> >>>> + >>>> +/dts-v1/; >>>> + >>>> +#include "aspeed-g6.dtsi" >>>> +#include <dt-bindings/gpio/aspeed-gpio.h> >>>> + >>>> +/ { >>>> + model = "AMD Onyx BMC"; >>>> + compatible = "amd,onyx-bmc", "aspeed,ast2600"; >> Undocumented compatibles. > This is a new device tree file. It's obvious, so what does it even mean? Anyway, fix warnings. Best regards, Krzysztof
diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile index fb9cc95f1b60..2b27d377aae2 100644 --- a/arch/arm/boot/dts/aspeed/Makefile +++ b/arch/arm/boot/dts/aspeed/Makefile @@ -5,6 +5,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \ aspeed-ast2600-evb.dtb \ aspeed-bmc-amd-daytonax.dtb \ aspeed-bmc-amd-ethanolx.dtb \ + aspeed-bmc-amd-onyx.dtb \ aspeed-bmc-ampere-mtjade.dtb \ aspeed-bmc-ampere-mtmitchell.dtb \ aspeed-bmc-arm-stardragon4800-rep2.dtb \ diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts new file mode 100644 index 000000000000..a7056cd29553 --- /dev/null +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: GPL-2.0+ +// Copyright (c) 2021 - 2024 AMD Inc. +// Author: Supreeth Venkatesh <supreeth.venkatesh@amd.com> + +/dts-v1/; + +#include "aspeed-g6.dtsi" +#include <dt-bindings/gpio/aspeed-gpio.h> + +/ { + model = "AMD Onyx BMC"; + compatible = "amd,onyx-bmc", "aspeed,ast2600"; + + aliases { + serial0 = &uart1; + serial4 = &uart5; + }; + + chosen { + stdout-path = &uart5; + bootargs = "console=ttyS4,115200 earlyprintk vmalloc=512MB"; + }; + + memory@80000000 { + device_type = "memory"; + reg = <0x80000000 0x80000000>; + }; + +}; + +&mdio0 { + status = "okay"; + + ethphy0: ethernet-phy@0 { + compatible = "ethernet-phy-ieee802.3-c22"; + reg = <0>; + }; +}; + +&mac3 { + status = "okay"; + phy-mode = "rgmii"; + phy-handle = <ðphy0>; + + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_rgmii4_default>; +}; + + + +&fmc { + status = "okay"; + flash@0 { + compatible = "jedec,spi-nor"; + status = "okay"; + #include "openbmc-flash-layout-128.dtsi" + }; +}; + +//Host Console +&uart1 { + status = "okay"; +}; + +//BMC Console +&uart5 { + status = "okay"; +}; + +&gpio0 { + gpio-line-names = + /*A0-A7*/ "","","","","","","","", + /*B0-B7*/ "","","","","MON_POST_COMPLETE","P0_PRESENT_L","","", + /*C0-C7*/ "","","","","","","","", + /*D0-D7*/ "","","","","","","","", + /*E0-E7*/ "","","","","","","","", + /*F0-F7*/ "","","","","","","","", + /*G0-G7*/ "","","","","","","","", + /*H0-H7*/ "","ASSERT_WARM_RST_BTN_L","ASSERT_SOC_RST_BTN_L","","","","","", + /*I0-I7*/ "","","","","","","","P0_I3C_APML_ALERT_L", + /*J0-J7*/ "","","","","","","","", + /*K0-K7*/ "","","","","","","","", + /*L0-L7*/ "","","","","","","","", + /*M0-M7*/ "","","","","","","","", + /*N0-N7*/ "","","","","","","PSP_SOFT_FUSE_NOTIFY","ASSERT_BMC_READY", + /*O0-O7*/ "","","HDT_SEL","HDT_XTRIG5","HDT_XTRIG6","JTAG_TRST_N","","", + /*P0-P7*/ "MON_RST_BTN_L","ASSERT_RST_BTN_L","MON_PWR_BTN_L","ASSERT_PWR_BTN_L","HPM_FPGA_LOCKOUT","ASSERT_NMI_BTN_L","MON_PWR_GOOD","", + /*Q0-Q7*/ "","","HDT_DBREQ_L","","BIOS_SPD_MUX_CTRL_RELEASED_L","","","", + /*R0-R7*/ "","","","","","","","", + /*S0-S7*/ "","","","","","","P0_DIMM_AF_ERROR","P0_DIMM_GL_ERROR", + /*T0-T7*/ "","","","","","","","", + /*U0-U7*/ "","","","","","","","", + /*V0-V7*/ "","","","","","","","", + /*W0-W7*/ "","","","","","","","", + /*X0-X7*/ "","","","","","","","", + /*Y0-Y7*/ "","","","","","","","", + /*Z0-Z7*/ "","","","","","","",""; +};
This patch adds initial device tree and makefile updates for AMD Onyx platform. AMD Onyx platform is an AMD customer reference board with an Aspeed ast2600 BMC manufactured by AMD. It describes I2c devices, Fans, Kcs devices, Uarts, Mac, LEDs, etc. present on AMD Onyx platform. Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@amd.com> --- arch/arm/boot/dts/aspeed/Makefile | 1 + .../boot/dts/aspeed/aspeed-bmc-amd-onyx.dts | 98 +++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-amd-onyx.dts