Re: [PATCH] irqchip/gic-v3: workaround for ASR8601 when reading mpidr

[Marc Zyngier]

On Wed, 17 May 2023 11:45:22 +0100,
Yan Zheng（严政） <zhengyan@xxxxxxxxxxxx> wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier [mailto:maz@xxxxxxxxxx]
> > Sent: Wednesday, May 17, 2023 4:32 PM
> > To: Yan Zheng（严政） <zhengyan@xxxxxxxxxxxx>
> > Cc: tglx@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Gao Meitao（高玫涛）
> > <meitaogao@xxxxxxxxxxxx>; Zhou Qiao(周侨) <qiaozhou@xxxxxxxxxxxx>;
> > Zhang Zhizhou(张治洲) <zhizhouzhang@xxxxxxxxxxxx>
> > Subject: Re: [PATCH] irqchip/gic-v3: workaround for ASR8601 when reading
> > mpidr
> > 
> > On Wed, 17 May 2023 08:55:00 +0100,
> > zhengyan <zhengyan@xxxxxxxxxxxx> wrote:
> > >
> > > This patch add workaround for ASR8601, which uses an armv8.2 processor
> > > with a gic-500. ARMv8.2 uses Multiprocessor Affinity Register to
> > > identify the logical address of the core by
> > > | cluster | core | thread |.
> > 
> > Not quite. The ARMv8.2 architecture doesn't say *any* of that. It is ARM's
> > *implementations* that follow this scheme.
> > 
> 
> Really thank you for rapid response, 
> Yes, as arm documents
> https://developer.arm.com/docuentation/ka002107/latest said

This page doesn't exist.

> It comes from armv8.2 get 3 types for affinity (arm v8.0 cpus only get 2 types)
> And it's an implementations issue.

Again, this has nothing to do with the ARMv8.2 architecture. Nor the
ARMv8.0 architecture. Please read the ARM ARM, which says absolutely
*nothing* of what the various affinity levels are for.

> 
> > > However, gic-500 only supports topologies with affinity levels less
> > > than 2 as
> > > | cluster | core|.
> > >
> > > So it needs this patch to shift the MPIDR values to ensure proper
> > > functionality
> > >
> > > Signed-off-by: zhengyan <zhengyan@xxxxxxxxxxxx>
> > > ---
> > >  drivers/irqchip/irq-gic-v3.c | 28 +++++++++++++++++++++++++++-
> > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/irqchip/irq-gic-v3.c
> > > b/drivers/irqchip/irq-gic-v3.c index 6fcee221f201..435b98a8641e 100644
> > > --- a/drivers/irqchip/irq-gic-v3.c
> > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > @@ -39,6 +39,7 @@
> > >
> > >  #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996	(1ULL << 0)
> > >  #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539	(1ULL << 1)
> > > +#define FLAGS_WORKAROUND_MPIDR_ASR8601		(1ULL << 2)
> > 
> > What is ASR8601? Is it a system? Or an erratum number? For issues that are the
> > result of a HW integration issue, please provide an official erratum number, and
> > update Documentation/arm64/silicon-errata.rst.
> > 
> 
> ASR8601 is our soc's name, and yes it’s a kind of HW integration issue
> But maybe it’s not an erratum since our HW design is like that, although
> Arm doesn't recommend this way.

Yes, for a good reason: it doesn't work. So this is *definitely* an
erratum, no ifs, no buts.

> And I would like to add more comments
> Under the next part before *desc	= "GICv3: ASR 8601 MPIDR shift"*
> Maybe this is a better way? Or add something under Documentation？

Documentation/arm64/silicon-errata.rst is the place to put it. Nowhere
else.

	M.

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