Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-85-gd6d859e0cf-fm-20230116.001-gd6d859e0
Mime-Version: 1.0
Message-Id: <e9b0338e-3b2f-4b54-8547-ab8babf7c0e1@app.fastmail.com>
In-Reply-To: <20230113160651.51201-2-nick.hawkins@hpe.com>
References: <20230113160651.51201-1-nick.hawkins@hpe.com>
 <20230113160651.51201-2-nick.hawkins@hpe.com>
Date:   Wed, 25 Jan 2023 09:07:57 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Hawkins, Nick" <nick.hawkins@hpe.com>, soc@kernel.org
Cc:     "Guenter Roeck" <linux@roeck-us.net>,
        "Verdun, Jean-Marie" <verdun@hpe.com>,
        "Rob Herring" <robh+dt@kernel.org>,
        krzysztof.kozlowski+dt@linaro.org,
        "Russell King" <linux@armlinux.org.uk>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 1/2] ARM: dts: add GXP Support for fans and SPI
Content-Type: text/plain
Precedence: bulk

On Fri, Jan 13, 2023, at 17:06, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
>
> Reorganize the base address of AHB to accommodate the SPI and fan driver
> register requirements. Add the hpe,gxp-spifi and hpe,gxp-fan-ctrl
> compatibles. Add comments to make the register range more clear.

The changelog describes three separate things, which usually means
you should split up the patch into three smaller ones to make
it easier to review.

It sounds like the third one is no longer part of the patch anyway.

> @@ -52,76 +52,102 @@
>  			cache-level = <2>;
>  		};
> 
> -		ahb@c0000000 {
> +		ahb@80000000 {
>  			compatible = "simple-bus";
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> -			ranges = <0x0 0xc0000000 0x30000000>;
> +			ranges = <0x0 0x80000000 0xf000000>,
> +				<0x40000000 0xc0000000 0x40000000>;

In the changelog text for the first patch that moves the
ranges down, it would make sense to describe why this specific
move is done. "to accommodate the SPI and fan driver
register requirements" does not actually tell me why it was
first thought that the bus starts at 0xc0000000 but now starts
at 0x80000000 and has a weird hole.

Please explain how you determined the location of the hole and
the 0x80000000 offset. Are these from the datasheet, from
the hardware design or did you make them up because you thought
this is what I want?

>  			dma-ranges;

Having a 1:1 translation for DMA addresses is actually an indication
that the MMIO addresses on the bus might also be directly
mapped, rather than offset: If AHB addresses 0x0-0x80000000
refer to the local MMIO registers, there is no more room
for addressing RAM in the same addresses.

> -			vic1: interrupt-controller@80f00000 {
> +			vic1: interrupt-controller@f00000 {
>  				compatible = "arm,pl192-vic";
> -				reg = <0x80f00000 0x1000>;
> +				reg = <0xf00000 0x1000>;
>  				interrupt-controller;
>  				#interrupt-cells = <1>;
>  			};

Since you said that the earlier version of this was broken,
it would also make sense to split this bit out into a separate
bugfix patch, or at least describe it in the changelog text.

       Arnd