[PATCH] Add Supermicro X13DEM BMC machine
Ryan Sie
RyanS at supermicro.com.tw
Mon Jan 30 19:41:21 AEDT 2023
Hi Andrew,
Thanks for the reminder, i write the review in red letters.
-----Original Message-----
From: Andrew Jeffery andrew at aj.id.au<mailto:andrew at aj.id.au>
Sent: Monday, January 23, 2023 10:01 AM
To: Ryan Sie lesly895 at gmail.com<mailto:lesly895 at gmail.com>; openbmc at lists.ozlabs.org<mailto:openbmc at lists.ozlabs.org>
Cc: Ryan Sie - TW (HW 1) RyanS at supermicro.com.tw<mailto:RyanS at supermicro.com.tw>
Subject: Re: [PATCH] Add Supermicro X13DEM BMC machine
[CAUTION: External Mail]
Hi Ryan,
Thanks for the patch. Please make sure to Cc the upstream lists:
* linux-aspeed at lists.ozlabs.org<mailto:linux-aspeed at lists.ozlabs.org>
* devicetree at vger.kernel.org<mailto:devicetree at vger.kernel.org>
and any other people or lists that are found by running scripts/get_maintainer.pl.
Also, please run your patch through scripts/checkpatch.pl
I have a few other comments and queries:
On Wed, 28 Dec 2022, at 16:52, Ryan Sie wrote:
> Signed-off-by: Ryan Sie <ryans at supermicro.com.tw<mailto:ryans at supermicro.com.tw>>
> ---
> arch/arm/boot/dts/Makefile | 1 +
> .../boot/dts/aspeed-bmc-supermicro-x13dem.dts | 476
> ++++++++++++++++++
> 2 files changed, 477 insertions(+)
> create mode 100644 arch/arm/boot/dts/aspeed-bmc-supermicro-x13dem.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 2ee9c043218b..3b89006fa008 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1623,6 +1623,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
> aspeed-bmc-quanta-q71l.dtb \
> aspeed-bmc-quanta-s6q.dtb \
> aspeed-bmc-supermicro-x11spi.dtb \
> + aspeed-bmc-supermicro-x13dem.dtb \
> aspeed-bmc-inventec-transformers.dtb \
> aspeed-bmc-tyan-s7106.dtb \
> aspeed-bmc-tyan-s8036.dtb \
> diff --git a/arch/arm/boot/dts/aspeed-bmc-supermicro-x13dem.dts
> b/arch/arm/boot/dts/aspeed-bmc-supermicro-x13dem.dts
> new file mode 100644
> index 000000000000..b94783a52999
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-supermicro-x13dem.dts
> @@ -0,0 +1,476 @@
> +/dts-v1/;
> +
> +#include "aspeed-g6.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h> #include
> +<dt-bindings/i2c/i2c.h>
> +
> +/ {
> + model = "AST2600 BMC";
> + compatible = "aspeed,ast2600";
> +
> + chosen {
> + stdout-path = &uart5;
> + bootargs = "console=ttyS4,115200 earlyprintk";
> + };
> +
> + memory at 80000000 {
> + device_type = "memory";
> + reg = <0x80000000 0x40000000>;
> + };
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + gfx_memory: framebuffer {
> + size = <0x01000000>;
> + alignment = <0x01000000>;
> + compatible = "shared-dma-pool";
> + reusable;
> + };
> +
> + video_engine_memory: jpegbuffer {
> + size = <0x02000000>; /* 32M */
> + alignment = <0x01000000>;
> + compatible = "shared-dma-pool";
> + reusable;
> + };
> +
> + ssp_memory: ssp_memory {
> + size = <0x00200000>;
> + alignment = <0x00100000>;
> + compatible = "shared-dma-pool";
> + no-map;
> + };
How is this reserved memory used?
Supermicro internal use, i will remove it.
> +
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + powerfail {
> + default-state = "off";
> + gpios = <&gpio0 ASPEED_GPIO(G, 5) GPIO_ACTIVE_LOW>;
> + };
> + };
> +
> + iio-hwmon {
> + compatible = "iio-hwmon";
> + io-channels = <&adc0 0>, <&adc0 1>, <&adc0 2>, <&adc0 3>,
> + <&adc0 4>, <&adc0 5>, <&adc0 6>, <&adc0 7>,
> + <&adc1 0>, <&adc1 1>, <&adc1 2>, <&adc1 3>,
> + <&adc1 4>, <&adc1 5>, <&adc1 6>, <&adc1 7>;
> + };
> +
> +#if 0
> + aliases {
> + i2c18 = &imux18;
> + i2c19 = &imux19;
> + i2c20 = &imux20;
> + i2c21 = &imux21;
> + i2c22 = &imux22;
> + mmc0 = &emmc;
> + };
> +#else
> + aliases {
> + mmc0 = &emmc;
> + };
> +#endif
Please remove the ifdefery.
I will remove it.
> +
> +};
> +
> +&emmc_controller {
> + status = "okay";
> + timing-phase = <0x700FF>;
This is not a valid property. Instead you need to use the clk-phase* properties that upstream provides:
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/mmc-controller.yaml?h=v6.2-rc5*n348__;Iw!!ORmjBF0Gq6jo!Ycod3yQsReZptIU0NgDMRyLN3q37O4r90hvpuRQyDxlG5EdTGrcNLCKR51sA_ndx8on6ng4dVgBm16nGTg4$<https://urldefense.com/v3/__https:/git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/mmc-controller.yaml?h=v6.2-rc5*n348__;Iw!!ORmjBF0Gq6jo!Ycod3yQsReZptIU0NgDMRyLN3q37O4r90hvpuRQyDxlG5EdTGrcNLCKR51sA_ndx8on6ng4dVgBm16nGTg4$>
These require knowing how much phase correction you need for a given bus speed.
I will remove timing-phase property, use default first.
> +};
> +
> +&emmc {
> + status = "okay";
> +
> + non-removable;
> + max-frequency = <100000000>;
> + sdhci-drive-type = /bits/ 8 <3>;
> +#if 1
> + bus-width = <4>;
> +#else
> + bus-width = <8>;
> + pinctrl-0 = <&pinctrl_emmc_default
> + &pinctrl_emmcdat4_default
> + &pinctrl_emmcdat5_default
> + &pinctrl_emmcdat6_default
> + &pinctrl_emmcdat7_default>;
> +#endif
Please remove the ifdefery
I will remove it.
> +};
> +
> +//&jtag1 {
> +// status = "okay";
> +//};
Remove commented nodes.
I will remove it.
> +
> +&fmc {
> + status = "okay";
> + flash at 0 {
> + status = "okay";
> + label = "bmc";
> + spi-max-frequency = <25000000>;
> + spi-tx-bus-width = <1>;
> + m25p,fast-read;
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + u-boot at 0 {
> + reg = <0x0 0x100000>;
> + label = "u-boot";
> + };
> + u-boot-env at 3f0000 {
> + reg = <0x3f0000 0x10000>;
> + label = "u-boot-env";
> + };
> + u-boot-env-redund at 7f0000 {
> + reg = <0x7f0000 0x10000>;
> + label = "u-boot-env-redund";
> + };
> + };
> + };
> +};
Do you actually have both SPI-NOR and eMMC storage for the BMC?
Yes, nor-flash for uboot, emmc for kernel+filesystem+supermicro features
> +
> +&spi1 {
> + status = "okay";
> + flash at 0 {
> + status = "okay";
> + label = "spi1:0";
> + spi-max-frequency = <25000000>;
> + spi-bus-width = <1>;
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + all-bios at 0 {
> + label = "all_part_bios";
> + reg = <0x000000000 0x02000000>;
> + };
> + };
> + };
> +
> +};
> +
> +&spi2 {
> + status = "disabled";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_spi2_default &pinctrl_spi2cs1_default
> &pinctrl_spi2cs2_default
> + &pinctrl_qspi2_default>;
> +
> + flash at 0 {
> + status = "okay";
> + label = "spi2:0";
> + spi-max-frequency = <100000000>;
> + spi-bus-width = <2>;
> + m25p,fast-read;
> + };
> +};
> +
> +//&peci0 {
> +// status = "okay";
> +//};
Delete commented nodes
I will remove it.
> +
> +&syscon {
> + uart-clock-high-speed;
This isn't a valid property. Please run your devicetree through `make dtbs_check`.
I will remove it.
> + status = "okay";
> +};
> +
> +&adc0 {
> + status = "okay";
You must also add the pinctrl properties here to ensure your ADC lines aren't accidentally muxed for other purposes.
I will modify it.
> +};
> +
> +&adc1 {
> + status = "okay";
Again here.
I will modify it.
> +};
> +
> +&gpio0 {
> + status = "okay";
> + gpio-line-names =
> + /*A0-A7*/ "","","","","","","","",
> + /*B0-B7*/ "","","","","","","","",
> + /*C0-C7*/ "","","","","","","","",
> + /*D0-D7*/ "","","","","","","","",
> + /*E0-E7*/ "","","","","","","","",
> + /*F0-F7*/ "","","","","","","","",
> + /*G0-G7*/ "","","","","","","","",
> + /*H0-H7*/ "","NMI_N","PWRBTN_N","RST_N","","","","",
> + /*I0-I7*/ "","","","","","","","",
> + /*J0-J7*/ "","","","","","","","",
> + /*K0-K7*/ "","","","","","","","",
> + /*L0-L7*/ "","","","","","","","",
> + /*M0-M7*/ "","","","","","","","",
> + /*N0-N7*/ "","","","","","","","",
> + /*O0-O7*/ "","","","","","","","",
> + /*P0-P7*/ "","","","","","","","",
> + /*Q0-Q7*/ "","","","","","","","",
> + /*R0-R7*/ "","","","","","","","",
> + /*S0-S7*/ "","","","","PWROK_IN","","","",
> + /*T0-T7*/ "","","","","","","","",
> + /*U0-U7*/ "","","","","","","","",
> + /*V0-V7*/ "","","","","","","","",
> + /*W0-W7*/ "","","","","","","","",
> + /*X0-X7*/ "","BIOS_CMP_IN","","","","","","",
> + /*Y0-Y7*/ "","","","","","","","",
> + /*Z0-Z7*/ "","","PWRBTN_IN","","","","","";
> +};
> +
> +&gpio1 {
> + status = "disabled";
> +};
> +
> +&sgpiom0 {
> + status = "disabled";
> + gpio-line-names =
> + /* SGPIO output lines */
> + /*OA0-OA7*/ "","","","","","","","",
> + /*OB0-OB7*/ "","","","","","","","",
> + /*OC0-OC7*/ "","","","","","","","",
> + /*OD0-OD7*/ "","","","","","","","",
> + /*OE0-OE7*/ "","","","","","","","",
> + /*OF0-OF7*/ "","","","","","","","",
> + /*OG0-OG7*/ "","","","","","","","",
> + /*OH0-OH7*/ "","","","","","","","",
> + /*OI0-OI7*/ "","","","","","","","",
> + /*OJ0-OJ7*/ "","","","","","","","",
> + /*DUMMY*/ "","","","","","","","",
> + /*DUMMY*/ "","","","","","","","",
> +
> + /* SGPIO input lines */
> + /*IA0-IA7*/ "","","","","","","","",
> + /*IB0-IB7*/ "","","","","","","","",
> + /*IC0-IC7*/ "","","","","","","","",
> + /*ID0-ID7*/ "","","","","","","","",
> + /*IE0-IE7*/ "","","","","","","","",
> + /*IF0-IF7*/ "","","","","","","","",
> + /*IG0-IG7*/ "","","","","","","","",
> + /*IH0-IH7*/ "","","","","","","","",
> + /*II0-II7*/ "","","","","","","","",
> + /*IJ0-IJ7*/ "","","","","","","","";
> +};
> +
> +&kcs3 {
> + aspeed,lpc-io-reg = <0xCA2>;
> + status = "okay";
> +};
> +
> +&kcs4 {
> + aspeed,lpc-io-reg = <0xCA4>;
> + status = "okay";
> +};
> +
> +//&lpc_sio {
> +// status = "okay";
> +//};
Delete commented nodes.
I don't think this node is even legitimate. Does this devicetree compile?
I confirm that it has been compiled
> +
> +&lpc_snoop {
> + snoop-ports = <0x80>;
> + status = "okay";
> +};
> +
> +//&mbox {
> +// status = "okay";
> +//};
Delete commented node.
I will modify it.
> +
> +&uart1 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_txd1_default
> + &pinctrl_rxd1_default
> + &pinctrl_nrts1_default
> + &pinctrl_ndtr1_default
> + &pinctrl_ndsr1_default
> + &pinctrl_ncts1_default
> + &pinctrl_ndcd1_default
> + &pinctrl_nri1_default>;
> +};
> +
> +&uart2 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_txd2_default
> + &pinctrl_rxd2_default
> + &pinctrl_nrts2_default
> + &pinctrl_ndtr2_default
> + &pinctrl_ndsr2_default
> + &pinctrl_ncts2_default
> + &pinctrl_ndcd2_default
> + &pinctrl_nri2_default>;
> +};
> +
> +&uart3 {
> + status = "okay";
> +};
> +
> +&uart4 {
> + status = "okay";
> +};
> +
> +&uart5 {
> + status = "okay";
> +};
> +
> +&uart_routing {
> + status = "okay";
> +};
> +
> +&mdio0 {
> + status = "okay";
> +
> + ethphy0: ethernet-phy at 0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };
> +};
> +
> +&mac0 {
> + status = "okay";
> +
> + phy-mode = "rgmii";
> + phy-handle = <ðphy0>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_rgmii1_default>; };
> +
> +&mdio1 {
> + status = "disabled";
> +
> + ethphy1: ethernet-phy at 0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };
> +};
> +
> +&mac1 {
> + status = "disabled";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_rgmii2_default>;
> + phy-mode = "rgmii";
> + phy-handle = <ðphy1>;
> +};
> +
> +&mdio2 {
> + status = "disabled";
> +
> + ethphy2: ethernet-phy at 0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };
> +};
> +
> +&mac2 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_rmii3_default>;
> + use-ncsi;
> +};
> +
> +&mdio3 {
> + status = "disabled";
> +
> + ethphy3: ethernet-phy at 0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };
> +};
> +
> +&mac3 {
> + status = "disabled";
> + pinctrl-names = "default";
> + phy-handle = <ðphy3>;
> + phy-mode = "rgmii";
> +
> +};
> +
> +&i2c0 {
> + status = "okay";
> +};
> +
> +&i2c1 {
> + status = "disabled";
> +};
> +
> +&i2c2 {
> + multi-master;
> + status = "okay";
> +#if 0
> + i2c-switch at 71 {
> + compatible = "nxp,pca9548";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x71>;
> + i2c-mux-idle-disconnect;
> +
> + imux18: i2c at 2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> + };
> +
> + imux19: i2c at 3 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <3>;
> + };
> +
> + imux20: i2c at 4 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <4>;
> + };
> +
> + imux21: i2c at 5 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <5>;
> + };
> +
> + imux22: i2c at 6 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <6>;
> + };
> + };
> +#endif
Delete the ifdefery
I will remove it.
The rest looks okay.
Cheers,
Andrew
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20230130/80a5ea3d/attachment-0001.htm>
More information about the openbmc
mailing list