Re: [PATCH v3 6/6] arm64: dts: cix: add initial CIX P1(SKY1) dts support

[Peter Chen <peter.chen@cixtech.com>]

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

Hi Marc,

Thanks for your detail review.

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

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

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

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

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

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

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