Re: [PATCH 3/9] arm64: dts: rockchip: Prepare Rockchip RK1808

[Marc Zyngier <maz@kernel.org>]

On Mon, 24 May 2021 14:32:41 +0100,
Andreas Färber <afaerber@suse.de> wrote:
> 
> On 17.05.21 11:21, Marc Zyngier wrote:
> > On Mon, 17 May 2021 00:05:45 +0100,
> > Andreas Färber <afaerber@suse.de> wrote:
> >>
> >> Add an initial Device Tree for Rockchip RK1808 SoC.
> >> Based on shipping TB-RK1808M0 DTB.
> >>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> ---
> >>  arch/arm64/boot/dts/rockchip/rk1808.dtsi | 203 +++++++++++++++++++++++
> >>  1 file changed, 203 insertions(+)
> >>  create mode 100644 arch/arm64/boot/dts/rockchip/rk1808.dtsi
> >>
> >> diff --git a/arch/arm64/boot/dts/rockchip/rk1808.dtsi b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> >> new file mode 100644
> >> index 000000000000..af2b51afda7d
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/rockchip/rk1808.dtsi
> [...]
> >> +		gic: interrupt-controller@ff100000 {
> >> +			compatible = "arm,gic-v3";
> >> +			reg = <0xff100000 0x10000>, /* GICD */
> >> +			      <0xff140000 0xc0000>, /* GICR */
> > 
> > This is obviously wrong. You have two CPUs, and yet describe a range
> > that spans 6. I guess this is a copy paste from rk3399 again?
> 
> Not on my part at least. As indicated, these numbers are what ships in
> the DTB on the RK1808 card, as per dtc -I dtb -O dts. Could be a mistake
> by Rockchip, of course.
> 
> Are you suggesting 0xc0000/6*2 = 0x40000 for two CPUs here?  Works
> as bad as before - investigation still ongoing with latest next.
> 
> As for "obviously": The GICv3 YAML binding has no description for me to
> validate those numbers: "GIC Redistributors (GICR), one range per
> redistributor region" - says nothing about correlation to number of CPUs
> or size per CPU, and the examples are not explaining either: 0x200000
> has no number of CPUs associated, and by my calculation 0x800000 for 32
> CPUs results in 0x40000 per CPU; but then again the examples also have
> GICC etc. at diverging 0x2000 size.

The GICv3/v4 architecture spec does apply, and you should really have
a look at what these sizes mean. What is the value of copy-pasting
things without understanding it the first place?

> 
> >> +			      <0xff300000 0x10000>, /* GICC */
> >> +			      <0xff310000 0x10000>, /* GICH */
> >> +			      <0xff320000 0x10000>; /* GICV */
> >> +			interrupt-controller;
> >> +			#interrupt-cells = <3>;
> >> +			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> >> +			#address-cells = <1>;
> >> +			#size-cells = <1>;
> >> +			ranges;
> >> +
> >> +			gic_its: msi-controller@ff120000 {
> >> +				compatible = "arm,gic-v3-its";
> >> +				reg = <0xff120000 0x20000>;
> >> +				msi-controller;
> >> +				#msi-cells = <1>;
> >> +			};
> > 
> > What uses the ITS?
> 
> DT-wise seemingly only the __symbols__ table (named just "its" there, I
> notice), so we could drop (or rename) the label if you prefer.

No, I am asking *what* uses the ITS. Is it just dangling without any
user? No PCI bus making use of it?

	M.

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