Re: [PATCH] Adding architectural support for HPE's GXP BMC. This is the first of a series of patches to support HPE's BMC with Linux Kernel.

From: Verdun, Jean-Marie
Date: Tue Jan 25 2022 - 20:50:07 EST

Hello Arnd,

I work with Nick on upstreaming the initial code for our GXP asic. Many thanks for your feedbacks.

We will update accordingly. I must admit that I am a little bit lost regarding the process we shall follow to introduce a new SoC. We took the path to send first the DT side and then the drivers through a set of patch per driver. Andrew, seems to guide us into a direction where we shall have a very small DT initially and we will expand it in a step by step manner when we will get drivers approved, this might lead us into a process which might be very sequential. What is the best recommendation to follow ? Either way is ok on our side, I am just looking at the easiest solution for the code Maintainers.

Most of this code is intended to be used with OpenBMC and u-boot. We didn't have yet upstream anything into the bootloader, and wanted to follow a step by step approach by initially publishing into the kernel (that explain why some init also are still hardcoded in the case the bootloader doesn't provide the data, that is still work in progress, but we can have end user testing the infrastructure). We have a very small user space environment to validate that the kernel boot properly by using u-root, before getting OpenBMC fully loaded. Last but not least, as this is a BMC code, which is new to our end users, it would be just great to have default fall back if the u-boot environment is not properly setup (roughly we could code the MAC address into the umac driver, or the DT to address such cases). We plan to update uboot in the next couple of days by the way.

We do not use dtsi at all for the moment, as we are generating a dtb out of the dts file and load it into our SPI image. Probably not the best approach, but this is the way it is implemented currently. The dtb is compiled outside the kernel tree for the moment using dtc compiler. We will add that step into the dts boot Makefile, it make sense. Does the dtsi is mandatory for every SoC ? I can build one if needed. But as this SoC is a BMC, the current dts is an example of what shall be configured. Many other datas related to the hardware target platform are defined into OpenBMC layers while we build for various ProLiant servers. We wanted our kernel code being readily testable that is why we have that generic dts. (GPIOS mapping is machine dependent)

vejmarie

On 1/25/22, 4:22 PM, "Arnd Bergmann" <arnd@xxxxxxxx> wrote:

'On Tue, Jan 25, 2022 at 8:46 PM <nick.hawkins@xxxxxxx> wrote:
>
> From: Nick Hawkins <nick.hawkins@xxxxxxx>
>
> Signed-off-by: Nick Hawkins <nick.hawkins@xxxxxxx>

Hi Nick,

Thanks for your submission, it's always nice to see support for a new platform.

I assume that you have a number of other drivers that are required for
an initial
support, at least to get you booting into a shell. I recommend to keep
those together
as a series, and we can merge them through the soc tree initially, with an Ack
from the corresponding subsystem maintainers. For later updates to the drivers,
you should send them to the maintainers directly, same for any
non-essential drivers

Krzysztof already commented on most issues I see, here are a few more things
to consider:

>
> +GXP ARCHITECTURE

Make this "ARM/HPE GXP ARCHITECTURE", so it does not get mistaken
for a separate instruction set architecture, or something else with that three
letter acronym.

> +
> +/dts-v1/;
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "HPE,GXP";
> + model = "GXP";

Make this the specific machine rather than the SoC, unless you can guarantee
that there won't ever be another board revision made from the same SoC (family).

> + chosen {
> + bootargs = "earlyprintk console=ttyS0,115200 user_debug=31";
> + };

The bootargs should be set by the bootloader. In particular there should be
not 'earlyprintk' by default, and the console should be selected using the
'stdout-path' property.

You seem to be missing CPU nodes.

> +
> + usb0: ehci@cefe0000 {
> + compatible = "generic-ehci";
> + reg = <0xcefe0000 0x100>;
> + interrupts = <7>;
> + interrupt-parent = <&vic0>;
> + };
> +
> + usb1: ohci@cefe0100 {
> + compatible = "generic-ohci";
> + reg = <0xcefe0100 0x110>;
> + interrupts = <6>;
> + interrupt-parent = <&vic0>;
> + };

Add a custom compatible string as a specialization in case you ever
need to work around some quirk on these devices.

> + spifi0: spifi@c0000200 {
> + compatible = "hpe,gxp-spifi";
> + reg = <0xc0000200 0x80>, <0xc000c000 0x100>, <0xf8000000 0x8000000>;
> + interrupts = <20>;
> + interrupt-parent = <&vic0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + flash@0 {
> + compatible = "jedec,spi-nor";
> + reg = <0>;
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + bmc@0 {
> + label = "bmc";
> + reg = <0x0 0x2000000>;
> + };
> + u-boot@0 {
> + label = "u-boot";
> + reg = <0x0 0x60000>;
> + };


The partitions should ideally be set by the bootloader as well, or
at least be in the .dts file separately from the soc .dtsi file.

> diff --git a/arch/arm/configs/gxp_defconfig b/arch/arm/configs/gxp_defconfig
> new file mode 100644
> index 000000000000..f37c6630e06d
> --- /dev/null
> +++ b/arch/arm/configs/gxp_defconfig

Do you have a strong reason for needing a custom defconfig file?
Usually this should
work with the normal multi_v7_defconfig.


> diff --git a/arch/arm/mach-hpe/Kconfig b/arch/arm/mach-hpe/Kconfig
> new file mode 100644
> index 000000000000..9b27f97c6536
> --- /dev/null
> +++ b/arch/arm/mach-hpe/Kconfig
> @@ -0,0 +1,20 @@
> +menuconfig ARCH_HPE
> + bool "HPE SoC support"
> + help
> + This enables support for HPE ARM based SoC chips
> +if ARCH_HPE
> +
> +config ARCH_HPE_GXP
> + bool "HPE GXP SoC"
> + select ARM_VIC
> + select PINCTRL
> + select IRQ_DOMAIN
> + select GENERIC_IRQ_CHIP
> + select MULTI_IRQ_HANDLER
> + select SPARSE_IRQ
> + select CLKSRC_MMIO
> + depends on ARCH_MULTI_V7


Most of the symbols you select are implied by ARCH_MULTI_V7, so you
can remove them here.

> +#define IOP_REGS_PHYS_BASE 0xc0000000
> +#define IOP_REGS_VIRT_BASE 0xf0000000
> +#define IOP_REGS_SIZE (240*SZ_1M)

We don't normally do custom mappings any more, these should come from
the device tree and get mapped by the corresponding drivers.

> +#define IOP_EHCI_USBCMD 0x0efe0010
> +
> +static struct map_desc gxp_io_desc[] __initdata = {
> + {
> + .virtual = (unsigned long)IOP_REGS_VIRT_BASE,
> + .pfn = __phys_to_pfn(IOP_REGS_PHYS_BASE),
> + .length = IOP_REGS_SIZE,
> + .type = MT_DEVICE,
> + },
> +};
> +
> +void __init gxp_map_io(void)
> +{
> + iotable_init(gxp_io_desc, ARRAY_SIZE(gxp_io_desc));
> +}
> +
> +static void __init gxp_dt_init(void)
> +{
> + //reset EHCI host controller for clear start
> + __raw_writel(0x00080002,
> + (void __iomem *)(IOP_REGS_VIRT_BASE + IOP_EHCI_USBCMD));

This belongs into the bootloader, or the EHCI driver, see the comment about a
custom compatible value above ;-)

> +static void gxp_restart(enum reboot_mode mode, const char *cmd)
> +{
> + pr_info("gpx restart");
> + __raw_writel(1, (void __iomem *) IOP_REGS_VIRT_BASE);
> +}

This should be a reset driver, see
drivers/power/reset/syscon-reboot.c either as an example, or something you
can use directly.

Arnd