Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220310195229.109477-1-nick.hawkins@hpe.com>
In-Reply-To: <20220310195229.109477-1-nick.hawkins@hpe.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 11 Mar 2022 09:06:06 +0100
Message-ID: <CAK8P3a2YeENkriOpXk_a6uvOC0T1Ys2gLm_AZJL_HLmez24KQw@mail.gmail.com>
Subject: Re: [PATCH v3 01/10] arch: arm: mach-hpe: Introduce the HPE GXP architecture
To:     "Hawkins, Nick" <nick.hawkins@hpe.com>
Cc:     "Verdun, Jean-Marie" <verdun@hpe.com>,
        Russell King <linux@armlinux.org.uk>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Mar 10, 2022 at 8:52 PM <nick.hawkins@hpe.com> wrote:
>
> From: Nick Hawkins <nick.hawkins@hpe.com>
>
> The GXP is the HPE BMC SoC that is used in the majority
> of HPE Generation 10 servers. Traditionally the asic will
> last multiple generations of server before being replaced.
> In gxp.c we reset the EHCI controller early to boot the asic.
>
> Info about SoC:
>
> HPE GXP is the name of the HPE Soc. This SoC is used to implement
> many BMC features at HPE. It supports ARMv7 architecture based on
> the Cortex A9 core. It is capable of using an AXI bus to which
> a memory controller is attached. It has multiple SPI interfaces
> to connect boot flash and BIOS flash. It uses a 10/100/1000 MAC
> for network connectivity. It has multiple i2c engines to drive
> connectivity with a host infrastructure. The initial patches
> enable the watchdog and timer enabling the host to be able to
> boot.
>
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>

Please add me to Cc for the entire series when resending.


> +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

By convention, the 'depends on' usually comes first here.

Please drop the 'select' statements for things that are already selected
by ARCH_MULTIPLATFORM or ARCH_MULTI_V7.


> +
> +#define IOP_REGS_PHYS_BASE 0xc0000000
> +#define IOP_REGS_VIRT_BASE 0xf0000000
> +#define IOP_REGS_SIZE (240*SZ_1M)
> +#define RESET_CMD 0x00080002
> +
> +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,
> +       },
> +};

It looks like this is only used for the pxf_restart() function below.
In this case, you should get rid of the static mapping entirely and
use an ioremap() in the gxp_dt_init() function instead, ideally getting
the address from an appropriate device node rather than hardcoding
it here.

If there are other drivers using the static mapping, either explain
here why this is required, or try to change them to dynamic mappings as well.

> +static void __init gxp_dt_init(void)
> +{
> +       void __iomem *gxp_init_regs;
> +       struct device_node *np;
> +
> +       np = of_find_compatible_node(NULL, NULL, "hpe,gxp-cpu-init");
> +       gxp_init_regs = of_iomap(np, 0);
> +
> +       /*it is necessary for our SOC to reset ECHI through this*/
> +       /* register due to a hardware limitation*/
> +       __raw_writel(RESET_CMD,
> +               (gxp_init_regs));

My feeling is still that this should be done in the platform specific EHCI
driver front-end. I think I commented on this before but don't remember
getting an explanation why you can't have it there.

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

With both of these, you should use writel() instead of __raw_write().
Using the __raw accessors breaks big-endian kernels (which you
probably don't need, but shouldn't break for no reason anyway), and
lacks serialization and atomicity of the access.

A better place for the restart logic may be a separate driver in
drivers/power/reset/, at least if this otherwise ends up being the only
platform specific code you need.

          Arnd