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
