Re: [PATCH v1 4/5] arm64: dts: rockchip: add core dtsi for RK3568 SoC

[Marc Zyngier]

On Sun, 25 Apr 2021 10:16:50 +0100,
陈亮 <cl@xxxxxxxxxxxxxx> wrote:
> 
> Hi Marc，
> 
>     Thanks for reply.
> 
>     See below.
> 
> 在 2021/4/21 下午9:36, Marc Zyngier 写道:
> > On Wed, 21 Apr 2021 07:59:20 +0100,
> > <cl@xxxxxxxxxxxxxx> wrote:
> >> From: Liang Chen <cl@xxxxxxxxxxxxxx>

[...]

> >> +	timer {
> >> +		compatible = "arm,armv8-timer";
> >> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> >> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> >> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> >> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> > This doesn't match the GICv3 binding for PPIs.
> fixed in V2.
> > 
> >> +		arm,no-tick-in-suspend;
> > Oh, really? :-(
> yes, arm arch timer will stop in suspend mode on rk3568.

That the comparator stops is completely expected, as it lives in the
CPU power domain.

But losing the system counter on suspend is a complete violation of
the architecture, which clearly states that "The system counter must
be implemented in an always-on power domain." (ARMv8 ARM G_a, D11.1.2
The system counter).

Are you actually losing the system counter?

> >
> >> +	};
> >> +
> >> +	xin24m: xin24m {
> >> +		compatible = "fixed-clock";
> >> +		#clock-cells = <0>;
> >> +		clock-frequency = <24000000>;
> >> +		clock-output-names = "xin24m";
> >> +	};
> >> +
> >> +	xin32k: xin32k {
> >> +		compatible = "fixed-clock";
> >> +		clock-frequency = <32768>;
> >> +		clock-output-names = "xin32k";
> >> +		#clock-cells = <0>;
> >> +		pinctrl-names = "default";
> >> +		pinctrl-0 = <&clk32k_out0>;
> >> +	};
> >> +
> >> +	scmi_shmem: scmi-shmem@10f000 {
> >> +		compatible = "arm,scmi-shmem";
> >> +		reg = <0x0 0x0010f000 0x0 0x100>;
> >> +	};
> >> +
> >> +	gic: interrupt-controller@fd400000 {
> >> +		compatible = "arm,gic-v3";
> >> +		#interrupt-cells = <3>;
> >> +		#address-cells = <2>;
> >> +		#size-cells = <2>;
> >> +		ranges;
> >> +		interrupt-controller;
> >> +
> >> +		reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
> >> +		      <0x0 0xfd460000 0 0xc0000>; /* GICR */
> >> +		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> > Please add the 'mbi-alias' property, which should map onto the GICA
> > range that GIC600 provides. At least this could be useful to have MSIs
> > despite the lack of a working ITS. We can work out the usable ranges
> > on a per-board basis.
> 
> Thanks, we will try mbi-alias later, but we are afraid that the number
> of SPI is not enough.

Not enough for what? People might want to trade the use of in-SoC
devices for MSIs. Also, the DT should describe the whole of the HW,
including features that you don't find useful.

Thanks,

	M.

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