Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Date:   Mon, 25 Oct 2021 12:15:04 +0100
From:   Marc Zyngier <maz@kernel.org>
To:     Mark Rutland <mark.rutland@arm.com>, Brad Larson <brad@pensando.io>
Cc:     linux-arm-kernel@lists.infradead.org, arnd@arndb.de,
        linus.walleij@linaro.org, bgolaszewski@baylibre.com,
        broonie@kernel.org, fancer.lancer@gmail.com,
        adrian.hunter@intel.com, ulf.hansson@linaro.org, olof@lixom.net,
        linux-gpio@vger.kernel.org, linux-spi@vger.kernel.org,
        linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 11/11] arm64: dts: Add Pensando Elba SoC support
In-Reply-To: <20211025091731.GA2001@C02TD0UTHF1T.local>
References: <20211025015156.33133-1-brad@pensando.io>
 <20211025015156.33133-12-brad@pensando.io>
 <20211025091731.GA2001@C02TD0UTHF1T.local>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <a20805de16e1196c2ed46dd949473c9a@kernel.org>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2021-10-25 10:17, Mark Rutland wrote:
> Hi,
> 
> On Sun, Oct 24, 2021 at 06:51:56PM -0700, Brad Larson wrote:
>> Add Pensando common and Elba SoC specific device nodes
>> 
>> Signed-off-by: Brad Larson <brad@pensando.io>
> 
> [...]
> 
>> +	timer {
>> +		compatible = "arm,armv8-timer";
>> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(1) |
>> +					IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(1) |
>> +					IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(1) |
>> +					IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(1) |
>> +					IRQ_TYPE_LEVEL_LOW)>;
>> +	};
> 
> The GIC_CPU_MASK_SIMPLE() stuff is meant for GICv2, but as below you
> have GICv3, where this is not valid, so this should go.
> 
> Also, beware that GIC_CPU_MASK_SIMPLE(1) means a single CPU, which
> doesn't mak sense for the 16 CPUs you have.
> 
>> +		gic: interrupt-controller@800000 {
>> +			compatible = "arm,gic-v3";
>> +			#interrupt-cells = <3>;
>> +			#address-cells = <2>;
>> +			#size-cells = <2>;
>> +			ranges;
>> +			interrupt-controller;
>> +			reg = <0x0 0x800000 0x0 0x200000>,	/* GICD */
>> +			      <0x0 0xa00000 0x0 0x200000>;	/* GICR */

This is missing the GICv2 compat regions that the CPUs implement.

>> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +			gic_its: msi-controller@820000 {
>> +				compatible = "arm,gic-v3-its";
>> +				msi-controller;
>> +				#msi-cells = <1>;
>> +				reg = <0x0 0x820000 0x0 0x10000>;
>> +				socionext,synquacer-pre-its =
>> +							<0xc00000 0x1000000>;
>> +			};
>> +		};
> 
> Is there any shared lineage with Synquacer? The commit message didn't
> describe this quirk.

Funny, it looks like there is a sudden outburst of stupid copy/paste
among HW designers. TI did the exact same thing recently.

This totally negates all the advantages of having an ITS and makes
sure that you have all the overhead. Facepalm...

         M.
-- 
Jazz is not dead. It just smells funny...