Re: [PATCH v8 03/14] ARM: dts: Add basic support for Airoha EN7523

[Marc Zyngier <maz@kernel.org>]

On Wed, 22 Dec 2021 16:04:07 +0000,
Felix Fietkau <nbd@nbd.name> wrote:
> 
> 
> On 2021-12-21 16:02, Marc Zyngier wrote:
> > On 2021-12-20 21:18, Felix Fietkau wrote:
> >> From: John Crispin <john@phrozen.org>
> >> 
> >> Add basic support for Airoha EN7523, enough for booting to console.
> >> 
> >> The UART is basically 8250-compatible, except for the clock selection.
> >> A clock-frequency value is synthesized to get this to run at 115200
> >> bps.
> >> 
> >> Signed-off-by: John Crispin <john@phrozen.org>
> >> Signed-off-by: Bert Vermeulen <bert@biot.com>
> >> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> >> ---
> >>  arch/arm/boot/dts/Makefile       |   2 +
> >>  arch/arm/boot/dts/en7523-evb.dts |  27 ++++++++
> >>  arch/arm/boot/dts/en7523.dtsi    | 114 +++++++++++++++++++++++++++++++
> >>  3 files changed, 143 insertions(+)
> >>  create mode 100644 arch/arm/boot/dts/en7523-evb.dts
> >>  create mode 100644 arch/arm/boot/dts/en7523.dtsi
> >> 
> > 
> > [...]
> > 
> >> +	gic: interrupt-controller@9000000 {
> >> +		compatible = "arm,gic-v3";
> >> +		interrupt-controller;
> >> +		#interrupt-cells = <3>;
> >> +		#address-cells = <1>;
> >> +		#size-cells = <1>;
> >> +		reg = <0x09000000 0x20000>, <0x09080000 0x80000>;
> > 
> > You are missing the 3 extra regions implemented by the A53 cores
> > (GICC, GICV, GICH). Please see the binding and the A53 TRM.
> The SoC memory map documentation contains an address for GICC, but not
> for the other two. Maybe this CPU doesn't implement them.

Please read the CPU TRM (it is publicly available from the ARM web
site). GICC, GICV and GICH are all at a fixed offset form each other,
and are always implemented by the A53 when connected to a GICv3.

> I will add GICC in v9

Please add all 3 missing regions.

> 
> >> +		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
> >> +	};
> >> +
> >> +	timer {
> >> +		compatible = "arm,armv7-timer";
> > 
> > This is an ARMv8 CPU, even when used in 32bit mode.
> > 
> >> +		interrupt-parent = <&gic>;
> >> +		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> >> +			     <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> >> +			     <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> >> +			     <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
> >> +		clock-frequency = <25000000>;
> > 
> > Why isn't this properly configured by the firmware?
> I don't know.

Then drop it.

Thanks,

	M.

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