On 25-02-28 15:10:24, Marc Zyngier wrote:

Hi Marc,

Thanks for your detail review.

quoted

+
+             cpu10: cpu@a00 {
+                     compatible = "arm,cortex-a720";
+                     enable-method = "psci";
+                     reg = <0x0 0xa00>;
+                     device_type = "cpu";
+                     capacity-dmips-mhz = <1024>;
+             };
+
+             cpu11: cpu@b00 {
+                     compatible = "arm,cortex-a720";
+                     enable-method = "psci";
+                     reg = <0x0 0xb00>;
+                     device_type = "cpu";
+                     capacity-dmips-mhz = <1024>;
+             };

Given that half the A720s are advertised with lower clock speed, how
comes they all have the same capacity?

According to Documentation/devicetree/bindings/cpu/cpu-capacity.txt
"capacity-dmips-mhz" is u32 value representing CPU capacity expressed
in normalized DMIPS/MHz, it means CPU capability per MHz. For sky1
SoC, both middle and big cores are A720, so their capability per MHz
are the same.

quoted

+
+     pmu-a520 {
+             compatible = "arm,cortex-a520-pmu";
+             interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
+     };
+
+     pmu-a720 {
+             compatible = "arm,cortex-a720-pmu";
+             interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
+     };

This is wrong. The default configuration for PPIs is to expose the
*same* device on all CPUs. You must use PPI affinities for your PMUs.
Please see the GICv3 binding for the details.

We have discussed internally, we have not seen the benefits routing
different PPI interrupt to dedicated CPUs. Any use cases?

I prefer changing pmu nodes as one generic Armv8 PMU node. Is it accepted?
Or must I keep both pmu for A520 and A720, and add PPI affinities to
describe hardware well?

quoted

+
+     pmu-spe {
+             compatible = "arm,statistical-profiling-extension-v1";
+             interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
+     };
+
+     psci {
+             compatible = "arm,psci-1.0";
+             method = "smc";
+     };
+
+     soc@0 {
+             compatible = "simple-bus";
+             ranges = <0 0 0 0 0x20 0>;
+             dma-ranges;
+             #address-cells = <2>;
+             #size-cells = <2>;
+
+             gic: interrupt-controller@e010000 {
+                     compatible = "arm,gic-v3";
+                     reg = <0x0 0x0e010000 0 0x10000>,       /* GICD */
+                           <0x0 0x0e090000 0 0x300000>;       /* GICR * 12 */
+                     interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
+                     #interrupt-cells = <3>;

This will need to be bumped up to 4, and all the interrupt specifiers adjusted.

Depends on if PPI affinities is must.

quoted

+                     interrupt-controller;
+                     #redistributor-regions = <1>;

Drop this, this is useless. It is pretty obvious that there is a
single RD region, and 1 is the default.

quoted

+                     redistributor-stride = <0 0x40000>;

Drop this. This is a standard GIC700 that doesn't need any help
computing the stride as it obeys the architecture.

Will drop above two properties.

quoted

+                     #address-cells = <2>;
+                     #size-cells = <2>;

I don't understand why you repeat this on every sub-nodes.

Because there is a child node for gic_its below

quoted

+                     ranges;
+
+                     gic_its: msi-controller@e050000 {
+                             compatible = "arm,gic-v3-its";
+                             reg = <0x0 0x0e050000 0x0 0x30000>;
+                             msi-controller;
+                             #msi-cells = <1>;
+                     };
+             };
+     };
+
+     timer {
+             compatible = "arm,armv8-timer";
+             interrupt-names = "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt";
+             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>,
+                          <GIC_PPI 12 IRQ_TYPE_LEVEL_LOW>;
+             clock-frequency = <1000000000>;

Drop this. The firmware already sets CNTFRQ_EL0 to the correct value,
it seems. And if it doesn't, please fix the firmware.

Yes, you are right, firmware configures it, I will delete it at next
version.

quoted

+             arm,no-tick-in-suspend;

Why do you need this? Is the HW so broken that you have implemented
the global counter in a power domain that isn't always on?

Not hardware broken, just arch timer will be powered off at cpu idle
and system suspend due to power consumption reason.

-- 

Best regards,
Peter