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

Chester Lin
2021-08-20T15:16:15Z

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@suse.de> 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@suse.com>
> > > ---
> > > 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.
>