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> <20220310195229.109477-9-nick.hawkins@hpe.com>
In-Reply-To: <20220310195229.109477-9-nick.hawkins@hpe.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 11 Mar 2022 09:17:06 +0100
Message-ID: <CAK8P3a0Wsd31DhPKZefm9742M-4B3ofg=CsS9+P4nXxM=4HsVg@mail.gmail.com>
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
To:     "Hawkins, Nick" <nick.hawkins@hpe.com>
Cc:     "Verdun, Jean-Marie" <verdun@hpe.com>,
        Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>,
        SoC Team <soc@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        DTML <devicetree@vger.kernel.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 HPE SoC is new to linux. This patch
> creates the basic device tree layout with minimum required
> for linux to boot. This includes timer and watchdog
> support.
>
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       compatible = "hpe,gxp";
> +       model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
> +
> +       chosen {
> +               bootargs = "earlyprintk console=ttyS2,115200";
> +       };

Please drop the bootargs here, you definitely should not have 'earlyprintk'
in the bootargs because that is incompatible with cross-platform kernels.

Instead of passing the console in the bootargs, use the "stdout-path"
property.

The "compatible" property should be a list that contains at least the specific
SoC variant and an identifier for the board. "hpe,gxp" is way too generic
to be the only property here.
> +       gxp-init@cefe0010 {
> +               compatible = "hpe,gxp-cpu-init";
> +               reg = <0xcefe0010 0x04>;
> +       };
> +
> +       memory@40000000 {
> +               device_type = "memory";
> +               reg = <0x40000000 0x20000000>;
> +       };
> +
> +       ahb {
> +               compatible = "simple-bus";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               device_type = "soc";
> +               ranges;
> +
> +               vic0: interrupt-controller@ceff0000 {
> +                       compatible = "arm,pl192-vic";
> +                       interrupt-controller;
> +                       reg = <0xceff0000 0x1000>;
> +                       #interrupt-cells = <1>;
> +               };

I don't think this represents the actual hierarchy of the devices:
the register range of the "vic0" and the "gxp-init" is very close
together, which usually indicates that they are also on the same
bus.


> +               vic1: interrupt-controller@80f00000 {
> +                       compatible = "arm,pl192-vic";
> +                       interrupt-controller;
> +                       reg = <0x80f00000 0x1000>;
> +                       #interrupt-cells = <1>;
> +               };
> +
> +               timer0: timer@c0000080 {
> +                       compatible = "hpe,gxp-timer";
> +                       reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> +                       interrupts = <0>;
> +                       interrupt-parent = <&vic0>;
> +                       clock-frequency = <400000000>;
> +               };
> +
> +               uarta: serial@c00000e0 {
> +                       compatible = "ns16550a";
> +                       reg = <0xc00000e0 0x8>;
> +                       interrupts = <17>;
> +                       interrupt-parent = <&vic0>;
> +                       clock-frequency = <1846153>;
> +                       reg-shift = <0>;
> +               };

In turn, you seem to have a lot of other devices on low addresses
of the 0xc0000000 range, which would be an indication that these
are on a different bus from the others.

Please see if you can find an internal datasheet that describes the
actual device hierarchy, and then try to model this in the device tree.

Use non-empty "ranges" properties to describe the address ranges
and how they get translated between these buses, and add
"dma-ranges" for any high-speed buses that have DMA master
capable devices like USB on them.

> +               i2cg: syscon@c00000f8 {
> +                       compatible = "simple-mfd", "syscon";
> +                       reg = <0xc00000f8 0x08>;
> +               };
> +       };

It's odd to have a "syscon" device that only has 8 bytes worth of registers.

What are the contents of this? Is it possible to have a proper abstraction
for it as something that has a specific purpose rather than being a
random collection of individual bits?

       Arnd