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

[Marc Zyngier]

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.

> > +			      <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.

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