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

[Yan Zheng（严政）]

> -----Original Message-----
> From: Marc Zyngier [mailto:maz@xxxxxxxxxx]
> Sent: Wednesday, May 17, 2023 8:59 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 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.
> 

Sorry for my mistake,
https://developer.arm.com/documentation/ka002107/latest
extract from the docs:
The Arm-v8.0 CPUs use affinity 0 for a CPU and affinity 1 for a cluster. 
All Arm-v8.2 CPUs use affinity 2 for a cluster, 1 for a CPU, and 0 for a thread.
The GIC-500 does not support affinity 2. Therefore, the GIC-500 cannot route 
Shared Peripheral Interrupts (SPIs) or Software Generated Interrupts (SGIs) 
correctly for Arm-v8.2 CPUs. 
The GIC-500 does not have correct buses to connect the system together.

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

As the extract from pervious part mentioned,
It seems like this issue related to that? I just want to mention that
this issue is cause by HW choice in the commit message.

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

Okay, I agree with you. Thanks for your advices.

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