Re: [PATCH 4/8] arm64: dts: add NXP S32G2 support

[Chester Lin]

On Fri, Aug 20, 2021 at 02:12:13PM +0100, Marc Zyngier wrote:
> On Thu, 12 Aug 2021 18:26:28 +0100,
> Andreas Färber <afaerber@xxxxxxx> wrote:
> > 
> > Hi Chester et al.,
> > 
> > On 05.08.21 08:54, Chester Lin wrote:
> > > Add an initial dtsi file for generic SoC features of NXP S32G2.
> > > 
> > > Signed-off-by: Chester Lin <clin@xxxxxxxx>
> > > ---
> > >  arch/arm64/boot/dts/freescale/s32g2.dtsi | 98 ++++++++++++++++++++++++
> > >  1 file changed, 98 insertions(+)
> > >  create mode 100644 arch/arm64/boot/dts/freescale/s32g2.dtsi
> > > 
> > > diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > > new file mode 100644
> > > index 000000000000..3321819c1a2d
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> 
> [...]
> 
> > > +		gic: interrupt-controller@50800000 {
> > > +			compatible = "arm,gic-v3";
> > > +			#interrupt-cells = <3>;
> > > +			interrupt-controller;
> > > +			reg = <0 0x50800000 0 0x10000>,
> > > +			      <0 0x50880000 0 0x200000>,
> 
> That's enough redistributor space for 16 CPUs. However, you only
> describe 4. Either the number of CPUs is wrong, the size is wrong, or
> the GIC has been configured for more cores than the SoC has.

Confirmed the SoC can only find 4 redistributors:

localhost:~ # dmesg | grep CPU
[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd034]
[    0.000000] Detected VIPT I-cache on CPU0
[    0.000000] CPU features: detected: GIC system register CPU interface
[    0.000000] CPU features: detected: ARM erratum 845719
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1
[    0.000000] rcu:     RCU restricting CPUs from NR_CPUS=480 to nr_cpu_ids=4.
[    0.000000] GICv3: CPU0: found redistributor 0 region 0:0x0000000050880000
[    0.063865] smp: Bringing up secondary CPUs ...
[    0.068852] Detected VIPT I-cache on CPU1
[    0.068894] GICv3: CPU1: found redistributor 1 region 0:0x00000000508a0000
[    0.068963] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
[    0.069809] Detected VIPT I-cache on CPU2
[    0.069851] GICv3: CPU2: found redistributor 100 region 0:0x00000000508c0000
[    0.069903] CPU2: Booted secondary processor 0x0000000100 [0x410fd034]
[    0.070698] Detected VIPT I-cache on CPU3
[    0.070722] GICv3: CPU3: found redistributor 101 region 0:0x00000000508e0000
[    0.070749] CPU3: Booted secondary processor 0x0000000101 [0x410fd034]
[    0.070847] smp: Brought up 1 node, 4 CPUs
<..snip..>

I will correct the size to 0x80000, thanks!

> 
> > > +			      <0 0x50400000 0 0x2000>,
> > > +			      <0 0x50410000 0 0x2000>,
> > > +			      <0 0x50420000 0 0x2000>;
> > 
> > Please order reg after compatible by convention, and sort
> > interrupt-controller or at least #interrupt-cells (applying to
> > consumers) last, after the below one applying to this device itself.
> > 
> > > +			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) |
> > > +						 IRQ_TYPE_LEVEL_HIGH)>;
> > > +		};
> > 
> > CC'ing Marc for additional GIC scrutiny, often the sizes are wrong.
> 
> There is more than just sizes. The interrupt specifier for the
> maintenance interrupt is also wrong.
> 
> 	M.

I will remove the wrong interrupt specifier. Thanks!

Chester.

> 
> -- 
> Without deviation from the norm, progress is not possible.
>