[PATCH linux dev-4.10] Initial introduction of Mellanox switches of MSNXXXX family equipped with Aspeed 2520 BMC SoC. Initial minimal commit to support Mellanox MSNXXXX switches, including: Adding Mellanox msn platform early initialization. Adding ...
Mykola Kostenok
c_mykolak at mellanox.com
Fri May 19 18:22:57 AEST 2017
Hi, Joel.
Thanks for reply.
Please look at my comments below.
Best regards. Mykola Kostenok.
> -----Original Message-----
> From: joel.stan at gmail.com [mailto:joel.stan at gmail.com] On Behalf Of Joel
> Stanley
> Sent: Friday, May 19, 2017 6:07 AM
> To: Mykola Kostenok <c_mykolak at mellanox.com>
> Cc: OpenBMC Maillist <openbmc at lists.ozlabs.org>
> Subject: Re: [PATCH linux dev-4.10] Initial introduction of Mellanox switches
> of MSNXXXX family equipped with Aspeed 2520 BMC SoC. Initial minimal
> commit to support Mellanox MSNXXXX switches, including: Adding Mellanox
> msn platform early initialization. Adding ...
>
> Hello Mykola,
>
> Thanks for the patch! It's a great start. I have a few things that you need to
> clean up first.
>
> Make your commit messages in this format. The important part is to have a
> short (~50 character title) that becomes the email subject, and then a longer
> discussion below. For example:
>
> ARM: aspeed: Add Mellanox MSX machine
[Mykola Kostenok] Ok. But its MSN.
>
> Initial introduction of Mellanox switches of MSNXXXX family equipped with
> Aspeed 2520 BMC SoC. This adds the platform early initialization and msn
> platform device tree file.
[Mykola Kostenok] Ok, great. I'll do it.
>
> Signed-off-by: Mykola Kostenok <c_mykolak at mellanox.com>
>
> When you send your next version, use git-format-patch -v2 to indicate that
> this is the second revision.
>
> On Thu, May 18, 2017 at 5:51 PM, Mykola Kostenok
> <c_mykolak at mellanox.com> wrote:
> > Signed-off-by: Mykola Kostenok <c_mykolak at mellanox.com>
> > ---
> > .../devicetree/bindings/vendor-prefixes.txt | 1 +
> > arch/arm/boot/dts/Makefile | 1 +
> > .../dts/aspeed-bmc-mellanox-msn-flash-layout.dtsi | 32 ++++
> > arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts | 205
> +++++++++++++++++++++
> > arch/arm/configs/aspeed_g5_defconfig | 2 +
> > arch/arm/mach-aspeed/aspeed.c | 28 +++
> > 6 files changed, 269 insertions(+)
> > create mode 100644
> > arch/arm/boot/dts/aspeed-bmc-mellanox-msn-flash-layout.dtsi
> > create mode 100644 arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
> >
> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > index 16d3b5e7f5d1..84601d869a1b 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > @@ -172,6 +172,7 @@ meas Measurement Specialties
> > mediatek MediaTek Inc.
> > melexis Melexis N.V.
> > melfas MELFAS Inc.
> > +mellanox Mellanox Technologies
> > memsic MEMSIC Inc.
> > merrii Merrii Technology Co., Ltd.
> > micrel Micrel Inc.
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 30fe65627f30..3dba6c633686 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -990,6 +990,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += aspeed-bmc-
> opp-palmetto.dtb \
> > aspeed-bmc-opp-witherspoon.dtb \
> > aspeed-bmc-opp-zaius.dtb \
> > aspeed-bmc-opp-lanyang.dtb \
> > + aspeed-bmc-mellanox-msn.dtb \
> > aspeed-ast2500-evb.dtb
> > endif
> >
> > diff --git
> > a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn-flash-layout.dtsi
> > b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn-flash-layout.dtsi
> > new file mode 100644
> > index 000000000000..cf8ee54f13c2
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn-flash-layout.dtsi
> > @@ -0,0 +1,32 @@
> > +/* This file is the label for the bmc primary flash and its
> > +partitions */
>
> Having a separate dsti for the flash layout is okay, and we do this for
> openpower machines so we can share the layout without cut/paste.
>
> Since you only have one machine, you could include this in the main dts?
[Mykola Kostenok] We are working now on two another systems (both based on ARM host, but with the same BMC mezzanine). And in the future we can have more.
All these systems are supposed to have same FLASH layout.
How it critical to add our proprietary flash dtsi?
If it's real issue I'll embed it into our main dts file.
>
> > + label = "bmc";
> > + #address-cells = < 1 >;
> > + #size-cells = < 1 >;
> > + u-boot {
> > + reg = < 0 0x60000 >;
> > + label = "u-boot";
> > + };
> > + u-boot-env {
> > + reg = < 0x60000 0x10000>;
> > + label = "u-boot-env";
> > + };
> > + kernel {
> > + reg = < 0x70000 0x280000 >;
> > + label = "kernel";
> > + };
> > + dtb {
> > + reg = < 0x2f0000 0x10000 >;
> > + label = "dtb";
> > + };
> > + initramfs {
> > + reg = < 0x300000 0x1c0000 >;
> > + label = "initramfs";
> > + };
> > + rofs {
> > + reg = < 0x4c0000 0x1740000 >;
> > + label = "rofs";
> > + };
> > + rwfs {
> > + reg = < 0x1c00000 0x400000 >;
> > + label = "rwfs";
> > + };
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
> > b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
> > new file mode 100644
> > index 000000000000..372ce2cd5320
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
> > @@ -0,0 +1,205 @@
> > +/dts-v1/;
> > +
> > +#include <dt-bindings/thermal/thermal.h> #include
> > +<dt-bindings/pwm/pwm.h> #include <dt-bindings/gpio/gpio.h> #include
> > +<dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/gpio/aspeed-gpio.h> #include "aspeed-g5.dtsi"
> > +
> > +/ {
> > + model = "MSNXXXX BMC";
> > + compatible = "mellanox,msnxxxx-bmc", "aspeed,ast2500";
>
> Does MSN stand for something?
[Mykola Kostenok] MSN is stands for Mellanox Ethernet switches equipped with 32x100G ASICs Spectrum and Spectrum-2.
Mellanox convention for products name are MSN for switxches equipped with 100G Ethernet, MSX for 36x40G Ethernet/InfiniBand ASICs (SwitchX, SwitchX-2, MSB for 36x100g InfiniBand (Switch-IB, Switch-IB2).
F.e. in drivers/platform/x86/mlx-platform.c
MODULE_LICENSE("Dual BSD/GPL");
MODULE_ALIAS("dmi:*:*Mellanox*:MSN24*:");
MODULE_ALIAS("dmi:*:*Mellanox*:MSN27*:");
MODULE_ALIAS("dmi:*:*Mellanox*:MSB*:");
MODULE_ALIAS("dmi:*:*Mellanox*:MSX*:");
MODULE_ALIAS("dmi:*:*Mellanox*:MSN21*:");
The 1-sy system equipped with BMC is MSN240.
So we used MSNXXXX. And of course we can change it to MSN.
>
> The xxxx is a bit unsual looking. Could you instead just use mellanox,msn-
> bmc?
[Mykola Kostenok] Yes.
>
> > +
> > + aliases {
> > + serial0 = &uart1;
> > + serial4 = &uart5;
> > + };
> > +
> > + chosen {
> > + stdout-path = &uart5;
> > + bootargs = "console=ttyS4,115200n8 earlyprintk";
> > + };
> > +
> > + memory {
> > + /* 512MiB SDRAM DDR4 @ 0x8000_0000 */
> > + reg = <0x80000000 0x20000000>;
> > + };
> > +
> > + ahb {
> > + bmc_pnor: fmc at 1e620000 {
> > + reg = < 0x1e620000 0xc4
> > + 0x20000000 0x02000000 >;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "aspeed,ast2500-fmc";
> > + interrupts = <19>;
> > + flash at 0 {
> > + reg = < 0 >;
> > + compatible = "jedec,spi-nor" ;
> > +#include "aspeed-bmc-mellanox-msn-flash-layout.dtsi"
> > + };
> > + };
> > +
> > + host_pnor: spi2 at 1e631000 {
> > + reg = < 0x1e631000 0xc4
> > + 0x38000000 0x08000000 >;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "aspeed,ast2500-spi";
> > + status = "disabled";
>
> Disabled?
[Mykola Kostenok] Yes.
In our architecture this controller is used to access different SPI devices on host side, in particular NC-SI flash and BIOS flash.
So this is dynamic devices, which are selected through sysfs request and MUX to open a device is controlled by CPLD.
When selection is requested, dynamic device like /dev/mtd8 is created, when de-selected - destroyed.
In CPLD drive, which we are going to submit later, we have code as below:
if (val) {
/* Enable and create platform device. */
of_update_property(data->np, &device_en);
data->pdev = of_platform_device_create(
data->np, NULL, NULL);
priv->en_dynamic_node = true;
} else {
/* Disable and and unregister platform device. */
of_update_property(data->np, &device_dis);
of_device_unregister(data->pdev);
of_node_clear_flag(data->np, OF_POPULATED);
priv->en_dynamic_node = false;
}
In drivers/mtd/spi-nor/aspeed-smc.c we used patch for removing device:
@@ -568,8 +568,15 @@ static int aspeed_smc_unregister(struct aspeed_smc_controller *controller)
for (n = 0; n < controller->info->nce; n++) {
chip = controller->chips[n];
- if (chip)
+ if (chip) {
+ struct platform_device *cdev =
+
+ to_platform_device(chip->nor.dev);
+
mtd_device_unregister(&chip->nor.mtd);
+ of_device_unregister(cdev);
+ of_node_clear_flag(chip->nor.dev->of_node,
+ OF_POPULATED);
+ }
}
>
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_spi2ck_default
> > + &pinctrl_spi2cs0_default
> > + &pinctrl_spi2miso_default
> > + &pinctrl_spi2mosi_default>;
> > +
> > + host_flash {
> > + reg = < 0 >;
> > + compatible = "jedec,spi-nor";
> > + label = "host_flash";
> > + #address-cells = < 1 >;
> > + #size-cells = < 1 >;
> > + };
> > + };
> > + };
> > +};
> > +
> > +&uart5 {
> > + status = "okay";
> > +};
> > +
> > +&uart1 {
> > + status = "okay";
> > +};
> > +
> > +&mac0 {
> > + status = "okay";
> > + use-ncsi;
> > + no-hw-checksum;
>
> I don't think we need no-hw-checksum for ast2500 on 4.10.
[Mykola Kostenok] OK, I'll remove it.
>
> > diff --git a/arch/arm/mach-aspeed/aspeed.c
> > b/arch/arm/mach-aspeed/aspeed.c index 0f1a536ba1b2..25fbc6c1adb7
> > 100644
> > --- a/arch/arm/mach-aspeed/aspeed.c
> > +++ b/arch/arm/mach-aspeed/aspeed.c
> > @@ -27,6 +27,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_WD 0x1E785000 /* Watchdog */
> >
> > static struct map_desc aspeed_io_desc[] __initdata __maybe_unused = {
> > {
> > @@ -188,6 +189,31 @@ static void __init do_lanyang_setup(void)
> > writel(reg & ~BIT(4), AST_IO(AST_BASE_LPC | 0x98)); }
> >
> > +static void __init do_mellanox_setup(void) {
> > + unsigned long reg;
> > +
> > + do_common_setup();
> > +
> > + /* Set strap to RGMII for dedicated PHY networking */
> > + reg = readl(AST_IO(AST_BASE_SCU | 0x70));
> > + reg |= BIT(7);
> > + reg &= ~BIT(6);
> > + writel(reg, AST_IO(AST_BASE_SCU | 0x70));
>
>
> This is a software workaround for incorrect hardware strapping. Will the
> strapping be fixed on a future revision of your hardware?
[Mykola Kostenok] Possibly, but in nearest future we should live with it.
>
> > + /* Disable UART1 Reset from LPC */
> > + writel(0x00000A00, AST_IO(AST_BASE_LPC | 0x98));
> > +
> > + reg = readl(AST_IO(AST_BASE_SCU | 0x48));
> > + reg |= BIT(29);
> > + writel(reg, AST_IO(AST_BASE_SCU | 0x48));
>
> Please add a comment for this. I think it's enabling RMII1 50MHz RCLK output.
[Mykola Kostenok] OK.
>
> > + /* Disable WD2 */
> > + reg = readl(AST_IO(AST_BASE_WD | 0x2c));
> > + reg &= ~BIT(0);
> > + writel(reg, AST_IO(AST_BASE_WD | 0x2c));
>
> Instead of disabling the watchdog here, you should load a driver for it. To do
> this, add the following to your device tree:
>
> &wdt2 {
> status = "okay";
> };
[Mykola Kostenok] When u-boot passed control to kernel, WD2 is enabled on our system. In kernel we wanted to clear WD2 at early init, in other case BMC will fall to a backup flash.
Should loading WD driver just clear WD2?
If yes, it should be OK for our system.
>
> Cheers,
>
> Joel
More information about the openbmc
mailing list