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

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

On 25-03-03 18:49:58, Marc Zyngier wrote:
> > > > +
> > > > +     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?
> 
> This isn't about changing the PPI. It is about matching CPUs with
> their PMU. Here, you are saying "both PMU types are connected to all
> the CPUs using PPI7".
> 
> That's obviously not the case.
> 
> > I prefer changing pmu nodes as one generic Armv8 PMU node. Is it accepted?
> 
> No, that's not acceptable.
> 
> > Or must I keep both pmu for A520 and A720, and add PPI affinities to
> > describe hardware well?
> 
> This is an established practice on all big-little systems: each PMU
> node has an affinity that indicates which CPUs they are connected
> to. For GICv3+, this is carried by the interrupt specifier.
> 
> Please look at existing SoCs supported, such as rk3399, for example.

I see. I will add ppi-partitions for gic-v3 node.

> > >
> > > This will need to be bumped up to 4, and all the interrupt specifiers adjusted.
> >
> > Depends on if PPI affinities is must.
> 
> Definitely a must, unless you want to completely remove all traces of
> the PMU, which is of course silly, but a valid alternative.

I will change #interrupt-cells to 4, and applies to all interrupt
specifiers.

> 
> > > > +             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.
> 
> This is not about the timer. This is about the global counter. If your
> counter stops ticking when you're in idle or suspended, your system is
> broken and you need this property. If the timer (or more precisely the
> comparator) is turned off because the CPU is off, then that's the
> expected behaviour and you don't need this property.
> 

I will delete this property.

-- 

Best regards,
Peter