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 |.
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 <[email protected]>
---
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)
#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
@@ -659,6 +660,9 @@ static u64 gic_mpidr_to_affinity(unsigned long mpidr)
{
u64 aff;
+ if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
+ mpidr >>= 8;
+
aff = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
@@ -970,6 +974,9 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
* Convert affinity to a 32bit value that can be matched to
* GICR_TYPER bits [63:32].
*/
+ if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
+ mpidr >>= 8;
+
aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
@@ -1265,6 +1272,8 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
unsigned long mpidr = cpu_logical_map(cpu);
u16 tlist = 0;
+ if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
+ mpidr >>= 8;
while (cpu < nr_cpu_ids) {
tlist |= 1 << (mpidr & 0xf);
@@ -1274,7 +1283,8 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
cpu = next_cpu;
mpidr = cpu_logical_map(cpu);
-
+ if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
+ mpidr >>= 8;
if (cluster_id != MPIDR_TO_SGI_CLUSTER_ID(mpidr)) {
cpu--;
goto out;
@@ -1321,6 +1331,8 @@ static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
u16 tlist;
+ if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
+ cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu) >> 8);
tlist = gic_compute_target_list(&cpu, mask, cluster_id);
gic_send_sgi(cluster_id, tlist, d->hwirq);
}
@@ -1729,6 +1741,15 @@ static bool gic_enable_quirk_cavium_38539(void *data)
return true;
}
+static bool gic_enable_quirk_asr8601(void *data)
+{
+ struct gic_chip_data *d = data;
+
+ d->flags |= FLAGS_WORKAROUND_MPIDR_ASR8601;
+
+ return true;
+}
+
static bool gic_enable_quirk_hip06_07(void *data)
{
struct gic_chip_data *d = data;
@@ -1823,6 +1844,11 @@ static const struct gic_quirk gic_quirks[] = {
.mask = 0xffffffff,
.init = gic_enable_quirk_nvidia_t241,
},
+ {
+ .desc = "GICv3: ASR 8601 MPIDR SHIFT",
+ .compatible = "asr,asr8601-gic-v3",
+ .init = gic_enable_quirk_asr8601,
+ },
{
}
};
--
2.25.1
On Wed, 17 May 2023 08:55:00 +0100,
zhengyan <[email protected]> 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.
> 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 <[email protected]>
> ---
> 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.
>
> #define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
>
> @@ -659,6 +660,9 @@ static u64 gic_mpidr_to_affinity(unsigned long mpidr)
> {
> u64 aff;
>
> + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> + mpidr >>= 8;
> +
> aff = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
> @@ -970,6 +974,9 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
> * Convert affinity to a 32bit value that can be matched to
> * GICR_TYPER bits [63:32].
> */
> + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> + mpidr >>= 8;
> +
> aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
> MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
> @@ -1265,6 +1272,8 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
> unsigned long mpidr = cpu_logical_map(cpu);
> u16 tlist = 0;
>
> + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> + mpidr >>= 8;
> while (cpu < nr_cpu_ids) {
> tlist |= 1 << (mpidr & 0xf);
>
> @@ -1274,7 +1283,8 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
> cpu = next_cpu;
>
> mpidr = cpu_logical_map(cpu);
> -
> + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> + mpidr >>= 8;
> if (cluster_id != MPIDR_TO_SGI_CLUSTER_ID(mpidr)) {
> cpu--;
> goto out;
> @@ -1321,6 +1331,8 @@ static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
> u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
> u16 tlist;
>
> + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> + cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu) >> 8);
You've written the same check 5 times. Maybe you could start by
refactoring that code so that the hack can be in a single place?
> tlist = gic_compute_target_list(&cpu, mask, cluster_id);
> gic_send_sgi(cluster_id, tlist, d->hwirq);
> }
> @@ -1729,6 +1741,15 @@ static bool gic_enable_quirk_cavium_38539(void *data)
> return true;
> }
>
> +static bool gic_enable_quirk_asr8601(void *data)
> +{
> + struct gic_chip_data *d = data;
> +
> + d->flags |= FLAGS_WORKAROUND_MPIDR_ASR8601;
> +
> + return true;
> +}
> +
> static bool gic_enable_quirk_hip06_07(void *data)
> {
> struct gic_chip_data *d = data;
> @@ -1823,6 +1844,11 @@ static const struct gic_quirk gic_quirks[] = {
> .mask = 0xffffffff,
> .init = gic_enable_quirk_nvidia_t241,
> },
> + {
> + .desc = "GICv3: ASR 8601 MPIDR SHIFT",
s/SHIFT/shift/
> + .compatible = "asr,asr8601-gic-v3",
So ASR8601 *is* a system... Is it DT only?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
> -----Original Message-----
> From: Marc Zyngier [mailto:[email protected]]
> Sent: Wednesday, May 17, 2023 4:32 PM
> To: Yan Zheng???????? <[email protected]>
> Cc: [email protected]; [email protected]; Gao Meitao????õ?Σ?
> <[email protected]>; Zhou Qiao(????) <[email protected]>;
> Zhang Zhizhou(??????) <[email protected]>
> Subject: Re: [PATCH] irqchip/gic-v3: workaround for ASR8601 when reading
> mpidr
>
> On Wed, 17 May 2023 08:55:00 +0100,
> zhengyan <[email protected]> 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
It comes from armv8.2 get 3 types for affinity (arm v8.0 cpus only get 2 types)
And it's an implementations issue.
> > 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 <[email protected]>
> > ---
> > 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.
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??
> >
> > #define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
> >
> > @@ -659,6 +660,9 @@ static u64 gic_mpidr_to_affinity(unsigned long
> > mpidr) {
> > u64 aff;
> >
> > + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> > + mpidr >>= 8;
> > +
> > aff = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> > MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> > MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 | @@ -970,6 +974,9
> @@
> > static int __gic_populate_rdist(struct redist_region *region, void __iomem
> *ptr)
> > * Convert affinity to a 32bit value that can be matched to
> > * GICR_TYPER bits [63:32].
> > */
> > + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> > + mpidr >>= 8;
> > +
> > aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
> > MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> > MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 | @@ -1265,6 +1272,8
> @@
> > static u16 gic_compute_target_list(int *base_cpu, const struct cpumask
> *mask,
> > unsigned long mpidr = cpu_logical_map(cpu);
> > u16 tlist = 0;
> >
> > + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> > + mpidr >>= 8;
> > while (cpu < nr_cpu_ids) {
> > tlist |= 1 << (mpidr & 0xf);
> >
> > @@ -1274,7 +1283,8 @@ static u16 gic_compute_target_list(int *base_cpu,
> const struct cpumask *mask,
> > cpu = next_cpu;
> >
> > mpidr = cpu_logical_map(cpu);
> > -
> > + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> > + mpidr >>= 8;
> > if (cluster_id != MPIDR_TO_SGI_CLUSTER_ID(mpidr)) {
> > cpu--;
> > goto out;
> > @@ -1321,6 +1331,8 @@ static void gic_ipi_send_mask(struct irq_data *d,
> const struct cpumask *mask)
> > u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
> > u16 tlist;
> >
> > + if (gic_data.flags & FLAGS_WORKAROUND_MPIDR_ASR8601)
> > + cluster_id =
> MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu) >> 8);
>
> You've written the same check 5 times. Maybe you could start by refactoring
> that code so that the hack can be in a single place?
>
Okay, I'll try to refactor it
> > tlist = gic_compute_target_list(&cpu, mask, cluster_id);
> > gic_send_sgi(cluster_id, tlist, d->hwirq);
> > }
> > @@ -1729,6 +1741,15 @@ static bool gic_enable_quirk_cavium_38539(void
> *data)
> > return true;
> > }
> >
> > +static bool gic_enable_quirk_asr8601(void *data) {
> > + struct gic_chip_data *d = data;
> > +
> > + d->flags |= FLAGS_WORKAROUND_MPIDR_ASR8601;
> > +
> > + return true;
> > +}
> > +
> > static bool gic_enable_quirk_hip06_07(void *data) {
> > struct gic_chip_data *d = data;
> > @@ -1823,6 +1844,11 @@ static const struct gic_quirk gic_quirks[] = {
> > .mask = 0xffffffff,
> > .init = gic_enable_quirk_nvidia_t241,
> > },
> > + {
> > + .desc = "GICv3: ASR 8601 MPIDR SHIFT",
>
> s/SHIFT/shift/
>
Okay
> > + .compatible = "asr,asr8601-gic-v3",
>
> So ASR8601 *is* a system... Is it DT only?
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Yes, asr8601 is our soc, and we want to use compatible node in devices-tree to control it,
As I mentioned at previous part, it might works well under
armv8.2(3 types of affinity) with gic500, but these code get strongly order with HW integration
Thanks again,
On Wed, 17 May 2023 11:45:22 +0100,
Yan Zheng（严政） <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: Marc Zyngier [mailto:[email protected]]
> > Sent: Wednesday, May 17, 2023 4:32 PM
> > To: Yan Zheng（严政） <[email protected]>
> > Cc: [email protected]; [email protected]; Gao Meitao（高玫涛）
> > <[email protected]>; Zhou Qiao(周侨) <[email protected]>;
> > Zhang Zhizhou(张治洲) <[email protected]>
> > Subject: Re: [PATCH] irqchip/gic-v3: workaround for ASR8601 when reading
> > mpidr
> >
> > On Wed, 17 May 2023 08:55:00 +0100,
> > zhengyan <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
> -----Original Message-----
> From: Marc Zyngier [mailto:[email protected]]
> Sent: Wednesday, May 17, 2023 8:59 PM
> To: Yan Zheng（严政） <[email protected]>
> Cc: [email protected]; [email protected]; Gao Meitao（高玫涛）
> <[email protected]>; Zhou Qiao(周侨) <[email protected]>;
> Zhang Zhizhou(张治洲) <[email protected]>
> Subject: Re: [PATCH] irqchip/gic-v3: workaround for ASR8601 when reading
> mpidr
>
> On Wed, 17 May 2023 11:45:22 +0100,
> Yan Zheng（严政） <[email protected]> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Marc Zyngier [mailto:[email protected]]
> > > Sent: Wednesday, May 17, 2023 4:32 PM
> > > To: Yan Zheng（严政） <[email protected]>
> > > Cc: [email protected]; [email protected]; Gao
> > > Meitao（高玫涛）
> > > <[email protected]>; Zhou Qiao(周侨) <[email protected]>;
> > > Zhang Zhizhou(张治洲) <[email protected]>
> > > Subject: Re: [PATCH] irqchip/gic-v3: workaround for ASR8601 when
> > > reading mpidr
> > >
> > > On Wed, 17 May 2023 08:55:00 +0100,
> > > zhengyan <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.
On Thu, 18 May 2023 04:15:54 +0100,
Yan Zheng（严政） <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: Marc Zyngier [mailto:[email protected]]
> > Sent: Wednesday, May 17, 2023 8:59 PM
> > To: Yan Zheng（严政） <[email protected]>
> > Cc: [email protected]; [email protected]; Gao Meitao（高玫涛）
> > <[email protected]>; Zhou Qiao(周侨) <[email protected]>;
> > Zhang Zhizhou(张治洲) <[email protected]>
> > Subject: Re: [PATCH] irqchip/gic-v3: workaround for ASR8601 when reading
> > mpidr
> >
> > On Wed, 17 May 2023 11:45:22 +0100,
> > Yan Zheng（严政） <[email protected]> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Marc Zyngier [mailto:[email protected]]
> > > > Sent: Wednesday, May 17, 2023 4:32 PM
> > > > To: Yan Zheng（严政） <[email protected]>
> > > > Cc: [email protected]; [email protected]; Gao
> > > > Meitao（高玫涛）
> > > > <[email protected]>; Zhou Qiao(周侨) <[email protected]>;
> > > > Zhang Zhizhou(张治洲) <[email protected]>
> > > > Subject: Re: [PATCH] irqchip/gic-v3: workaround for ASR8601 when
> > > > reading mpidr
> > > >
> > > > On Wed, 17 May 2023 08:55:00 +0100,
> > > > zhengyan <[email protected]> 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.
All these are implementations. Nothing to do with the architecture
(which only mentions the notion of 'cluster' when talking about
caches).
Anyway, let's move on.
>
> > > 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.
You can say that GIC500 is incompatible with ARMv8.2 implementations
from ARM, and that a workaround is needed for that.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
This patch add workaround for ASR8601, which uses an armv8.2
processor with a gic-500. But gic-500 is incompatible with
ARMv8.2 implementations from ARM.
ARMv8.2 from ARM implementation uses Multiprocessor Affinity
Register to identify the logical address of the core by
| cluster | core | thread |.
However, gic-500 only supports topologies with
affinity levels less than 2 as
| cluster | core|.
So we need this patch as workaround to shift the MPIDR values
to ensure proper functionality
Signed-off-by: zhengyan <[email protected]>
---
Documentation/arm64/silicon-errata.rst | 4 ++++
drivers/irqchip/irq-gic-v3.c | 30 ++++++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/Documentation/arm64/silicon-errata.rst b/Documentation/arm64/silicon-errata.rst
index 9e311bc43e05..d6430ade349d 100644
--- a/Documentation/arm64/silicon-errata.rst
+++ b/Documentation/arm64/silicon-errata.rst
@@ -214,3 +214,7 @@ stable kernels.
+----------------+-----------------+-----------------+-----------------------------+
| Fujitsu | A64FX | E#010001 | FUJITSU_ERRATUM_010001 |
+----------------+-----------------+-----------------+-----------------------------+
+
++----------------+-----------------+-----------------+-----------------------------+
+| ASR | ASR8601 | #8601001 | N/A |
++----------------+-----------------+-----------------+-----------------------------+
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 6fcee221f201..cf64783dfe70 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -39,6 +39,9 @@
#define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0)
#define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1)
+#define FLAGS_WORKAROUND_ASR_ERRATUM_8601001 (1ULL << 2)
+
+#define ASR8601_AFF_QUIRK(aff) (aff >> 8)
#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
@@ -659,6 +662,9 @@ static u64 gic_mpidr_to_affinity(unsigned long mpidr)
{
u64 aff;
+ if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
+ mpidr = ASR8601_AFF_QUIRK(mpidr);
+
aff = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
@@ -970,6 +976,9 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
* Convert affinity to a 32bit value that can be matched to
* GICR_TYPER bits [63:32].
*/
+ if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
+ mpidr = ASR8601_AFF_QUIRK(mpidr);
+
aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
@@ -1265,6 +1274,9 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
unsigned long mpidr = cpu_logical_map(cpu);
u16 tlist = 0;
+ if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
+ mpidr = ASR8601_AFF_QUIRK(mpidr);
+
while (cpu < nr_cpu_ids) {
tlist |= 1 << (mpidr & 0xf);
@@ -1275,6 +1287,8 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
mpidr = cpu_logical_map(cpu);
+ if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
+ mpidr = ASR8601_AFF_QUIRK(mpidr);
if (cluster_id != MPIDR_TO_SGI_CLUSTER_ID(mpidr)) {
cpu--;
goto out;
@@ -1321,6 +1335,8 @@ static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
u16 tlist;
+ if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
+ cluster_id = MPIDR_TO_SGI_CLUSTER_ID(ASR8601_AFF_QUIRK(cpu_logical_map(cpu)));
tlist = gic_compute_target_list(&cpu, mask, cluster_id);
gic_send_sgi(cluster_id, tlist, d->hwirq);
}
@@ -1786,6 +1802,15 @@ static bool gic_enable_quirk_nvidia_t241(void *data)
return true;
}
+static bool gic_enable_quirk_asr8601(void *data)
+{
+ struct gic_chip_data *d = data;
+
+ d->flags |= FLAGS_WORKAROUND_ASR_ERRATUM_8601001;
+
+ return true;
+}
+
static const struct gic_quirk gic_quirks[] = {
{
.desc = "GICv3: Qualcomm MSM8996 broken firmware",
@@ -1823,6 +1848,11 @@ static const struct gic_quirk gic_quirks[] = {
.mask = 0xffffffff,
.init = gic_enable_quirk_nvidia_t241,
},
+ {
+ .desc = "GICv3: ASR erratum 8601001",
+ .compatible = "asr,asr8601-gic-v3",
+ .init = gic_enable_quirk_asr8601,
+ },
{
}
};
--
2.25.1
On Mon, 22 May 2023 12:06:43 +0100,
zhengyan <[email protected]> wrote:
>
> This patch add workaround for ASR8601, which uses an armv8.2
> processor with a gic-500. But gic-500 is incompatible with
> ARMv8.2 implementations from ARM.
>
> ARMv8.2 from ARM implementation uses Multiprocessor Affinity
> Register to identify the logical address of the core by
> | cluster | core | thread |.
> However, gic-500 only supports topologies with
> affinity levels less than 2 as
> | cluster | core|.
>
> So we need this patch as workaround to shift the MPIDR values
> to ensure proper functionality
>
> Signed-off-by: zhengyan <[email protected]>
> ---
> Documentation/arm64/silicon-errata.rst | 4 ++++
> drivers/irqchip/irq-gic-v3.c | 30 ++++++++++++++++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/Documentation/arm64/silicon-errata.rst b/Documentation/arm64/silicon-errata.rst
> index 9e311bc43e05..d6430ade349d 100644
> --- a/Documentation/arm64/silicon-errata.rst
> +++ b/Documentation/arm64/silicon-errata.rst
> @@ -214,3 +214,7 @@ stable kernels.
> +----------------+-----------------+-----------------+-----------------------------+
> | Fujitsu | A64FX | E#010001 | FUJITSU_ERRATUM_010001 |
> +----------------+-----------------+-----------------+-----------------------------+
> +
> ++----------------+-----------------+-----------------+-----------------------------+
> +| ASR | ASR8601 | #8601001 | N/A |
> ++----------------+-----------------+-----------------+-----------------------------+
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 6fcee221f201..cf64783dfe70 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -39,6 +39,9 @@
>
> #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0)
> #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1)
> +#define FLAGS_WORKAROUND_ASR_ERRATUM_8601001 (1ULL << 2)
> +
> +#define ASR8601_AFF_QUIRK(aff) (aff >> 8)
This is wrong. There are more than just affinity bits in MPIDR, and
you're making a mess of the result.
>
> #define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
>
> @@ -659,6 +662,9 @@ static u64 gic_mpidr_to_affinity(unsigned long mpidr)
> {
> u64 aff;
>
> + if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
> + mpidr = ASR8601_AFF_QUIRK(mpidr);
> +
> aff = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
> @@ -970,6 +976,9 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
> * Convert affinity to a 32bit value that can be matched to
> * GICR_TYPER bits [63:32].
> */
> + if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
> + mpidr = ASR8601_AFF_QUIRK(mpidr);
It really wasn't what I had in mind when I asked you to place this
inside a helper. The whole thing looks horrible, and I really don't
want to have to maintain anything like it.
I came up with the following patch, which keeps the workaround *in a
single spot*.
Let me know if that works for you.
M.
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 6fcee221f201..b7d69ef4da9f 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_ASR_ERRATUM_8601001 (1ULL << 2)
#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
@@ -655,10 +656,16 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
return 0;
}
-static u64 gic_mpidr_to_affinity(unsigned long mpidr)
+static u64 gic_cpu_to_affinity(int cpu)
{
+ u64 mpidr = cpu_logical_map(cpu);
u64 aff;
+ /* ASR8601 needs to have its affinities shifted down... */
+ if (unlikely(gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001))
+ mpidr = (MPIDR_AFFINITY_LEVEL(mpidr, 1) |
+ (MPIDR_AFFINITY_LEVEL(mpidr, 2) << 8));
+
aff = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
@@ -913,7 +920,7 @@ static void __init gic_dist_init(void)
* Set all global interrupts to the boot CPU only. ARE must be
* enabled.
*/
- affinity = gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id()));
+ affinity = gic_cpu_to_affinity(smp_processor_id());
for (i = 32; i < GIC_LINE_NR; i++)
gic_write_irouter(affinity, base + GICD_IROUTER + i * 8);
@@ -962,7 +969,7 @@ static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *))
static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
{
- unsigned long mpidr = cpu_logical_map(smp_processor_id());
+ unsigned long mpidr;
u64 typer;
u32 aff;
@@ -970,6 +977,8 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
* Convert affinity to a 32bit value that can be matched to
* GICR_TYPER bits [63:32].
*/
+ mpidr = gic_cpu_to_affinity(smp_processor_id());
+
aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
@@ -1262,9 +1271,11 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
unsigned long cluster_id)
{
int next_cpu, cpu = *base_cpu;
- unsigned long mpidr = cpu_logical_map(cpu);
+ unsigned long mpidr;
u16 tlist = 0;
+ mpidr = gic_cpu_to_affinity(cpu);
+
while (cpu < nr_cpu_ids) {
tlist |= 1 << (mpidr & 0xf);
@@ -1273,8 +1284,7 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
goto out;
cpu = next_cpu;
- mpidr = cpu_logical_map(cpu);
-
+ mpidr = gic_cpu_to_affinity(cpu);
if (cluster_id != MPIDR_TO_SGI_CLUSTER_ID(mpidr)) {
cpu--;
goto out;
@@ -1318,7 +1328,7 @@ static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
dsb(ishst);
for_each_cpu(cpu, mask) {
- u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
+ u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(gic_cpu_to_affinity(cpu));
u16 tlist;
tlist = gic_compute_target_list(&cpu, mask, cluster_id);
@@ -1376,7 +1386,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
offset = convert_offset_index(d, GICD_IROUTER, &index);
reg = gic_dist_base(d) + offset + (index * 8);
- val = gic_mpidr_to_affinity(cpu_logical_map(cpu));
+ val = gic_cpu_to_affinity(cpu);
gic_write_irouter(val, reg);
@@ -1786,6 +1796,15 @@ static bool gic_enable_quirk_nvidia_t241(void *data)
return true;
}
+static bool gic_enable_quirk_asr8601(void *data)
+{
+ struct gic_chip_data *d = data;
+
+ d->flags |= FLAGS_WORKAROUND_ASR_ERRATUM_8601001;
+
+ return true;
+}
+
static const struct gic_quirk gic_quirks[] = {
{
.desc = "GICv3: Qualcomm MSM8996 broken firmware",
@@ -1823,6 +1842,11 @@ static const struct gic_quirk gic_quirks[] = {
.mask = 0xffffffff,
.init = gic_enable_quirk_nvidia_t241,
},
+ {
+ .desc = "GICv3: ASR erratum 8601001",
+ .compatible = "asr,asr8601-gic-v3",
+ .init = gic_enable_quirk_asr8601,
+ },
{
}
};
--
Without deviation from the norm, progress is not possible.
> -----Original Message-----
> From: Marc Zyngier [mailto:[email protected]]
> Sent: Monday, May 29, 2023 11:02 PM
> To: Yan Zheng（严政） <[email protected]>
> Cc: [email protected]; Gao Meitao（高玫涛）
> <[email protected]>; Zhou Qiao(周侨) <[email protected]>;
> [email protected]; Zhang Zhizhou(张治洲) <[email protected]>
> Subject: Re: [PATCH v2] irqchip/gic-v3: workaround for ASR8601 when reading
> mpidr
>
> On Mon, 22 May 2023 12:06:43 +0100,
> zhengyan <[email protected]> wrote:
> >
> > This patch add workaround for ASR8601, which uses an armv8.2 processor
> > with a gic-500. But gic-500 is incompatible with
> > ARMv8.2 implementations from ARM.
> >
> > ARMv8.2 from ARM implementation uses Multiprocessor Affinity Register
> > to identify the logical address of the core by
> > | cluster | core | thread |.
> > However, gic-500 only supports topologies with affinity levels less
> > than 2 as
> > | cluster | core|.
> >
> > So we need this patch as workaround to shift the MPIDR values to
> > ensure proper functionality
> >
> > Signed-off-by: zhengyan <[email protected]>
> > ---
> > Documentation/arm64/silicon-errata.rst | 4 ++++
> > drivers/irqchip/irq-gic-v3.c | 30
> ++++++++++++++++++++++++++
> > 2 files changed, 34 insertions(+)
> >
> > diff --git a/Documentation/arm64/silicon-errata.rst
> > b/Documentation/arm64/silicon-errata.rst
> > index 9e311bc43e05..d6430ade349d 100644
> > --- a/Documentation/arm64/silicon-errata.rst
> > +++ b/Documentation/arm64/silicon-errata.rst
> > @@ -214,3 +214,7 @@ stable kernels.
> > +----------------+-----------------+-----------------+-----------------------------+
> > | Fujitsu | A64FX | E#010001 |
> FUJITSU_ERRATUM_010001 |
> >
> > +----------------+-----------------+-----------------+----------------
> > -------------+
> > +
> > ++----------------+-----------------+-----------------+-----------------------------+
> > +| ASR | ASR8601 | #8601001 | N/A
> |
> > ++----------------+-----------------+-----------------+-----------------------------+
> > diff --git a/drivers/irqchip/irq-gic-v3.c
> > b/drivers/irqchip/irq-gic-v3.c index 6fcee221f201..cf64783dfe70 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -39,6 +39,9 @@
> >
> > #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0)
> > #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1)
> > +#define FLAGS_WORKAROUND_ASR_ERRATUM_8601001 (1ULL << 2)
> > +
> > +#define ASR8601_AFF_QUIRK(aff) (aff >> 8)
>
> This is wrong. There are more than just affinity bits in MPIDR, and you're making
> a mess of the result.
>
Yes, I made a mistake here.
> >
> > #define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
> >
> > @@ -659,6 +662,9 @@ static u64 gic_mpidr_to_affinity(unsigned long
> > mpidr) {
> > u64 aff;
> >
> > + if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
> > + mpidr = ASR8601_AFF_QUIRK(mpidr);
> > +
> > aff = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> > MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> > MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 | @@ -970,6 +976,9
> @@
> > static int __gic_populate_rdist(struct redist_region *region, void __iomem
> *ptr)
> > * Convert affinity to a 32bit value that can be matched to
> > * GICR_TYPER bits [63:32].
> > */
> > + if (gic_data.flags & FLAGS_WORKAROUND_ASR_ERRATUM_8601001)
> > + mpidr = ASR8601_AFF_QUIRK(mpidr);
>
> It really wasn't what I had in mind when I asked you to place this inside a helper.
> The whole thing looks horrible, and I really don't want to have to maintain
> anything like it.
>
> I came up with the following patch, which keeps the workaround *in a single
> spot*.
>
> Let me know if that works for you.
>
> M.
>
Really thanks for your help, this patch works well for us.
My previous patch was too ugly, this one looks better. I didn't find this good solution.
Should I send a patch for v3, considering that you're the author?
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index
> 6fcee221f201..b7d69ef4da9f 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_ASR_ERRATUM_8601001 (1ULL << 2)
>
> #define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
>
> @@ -655,10 +656,16 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d,
> void *vcpu)
> return 0;
> }
>
> -static u64 gic_mpidr_to_affinity(unsigned long mpidr)
> +static u64 gic_cpu_to_affinity(int cpu)
> {
> + u64 mpidr = cpu_logical_map(cpu);
> u64 aff;
>
> + /* ASR8601 needs to have its affinities shifted down... */
> + if (unlikely(gic_data.flags &
> FLAGS_WORKAROUND_ASR_ERRATUM_8601001))
> + mpidr = (MPIDR_AFFINITY_LEVEL(mpidr, 1) |
> + (MPIDR_AFFINITY_LEVEL(mpidr, 2) << 8));
> +
> aff = ((u64)MPIDR_AFFINITY_LEVEL(mpidr, 3) << 32 |
> MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 | @@ -913,7 +920,7
> @@ static void __init gic_dist_init(void)
> * Set all global interrupts to the boot CPU only. ARE must be
> * enabled.
> */
> - affinity = gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id()));
> + affinity = gic_cpu_to_affinity(smp_processor_id());
> for (i = 32; i < GIC_LINE_NR; i++)
> gic_write_irouter(affinity, base + GICD_IROUTER + i * 8);
>
> @@ -962,7 +969,7 @@ static int gic_iterate_rdists(int (*fn)(struct
> redist_region *, void __iomem *))
>
> static int __gic_populate_rdist(struct redist_region *region, void __iomem
> *ptr) {
> - unsigned long mpidr = cpu_logical_map(smp_processor_id());
> + unsigned long mpidr;
> u64 typer;
> u32 aff;
>
> @@ -970,6 +977,8 @@ static int __gic_populate_rdist(struct redist_region
> *region, void __iomem *ptr)
> * Convert affinity to a 32bit value that can be matched to
> * GICR_TYPER bits [63:32].
> */
> + mpidr = gic_cpu_to_affinity(smp_processor_id());
> +
> aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
> MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 | @@ -1262,9 +1271,11
> @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask
> *mask,
> unsigned long cluster_id)
> {
> int next_cpu, cpu = *base_cpu;
> - unsigned long mpidr = cpu_logical_map(cpu);
> + unsigned long mpidr;
> u16 tlist = 0;
>
> + mpidr = gic_cpu_to_affinity(cpu);
> +
> while (cpu < nr_cpu_ids) {
> tlist |= 1 << (mpidr & 0xf);
>
> @@ -1273,8 +1284,7 @@ static u16 gic_compute_target_list(int *base_cpu,
> const struct cpumask *mask,
> goto out;
> cpu = next_cpu;
>
> - mpidr = cpu_logical_map(cpu);
> -
> + mpidr = gic_cpu_to_affinity(cpu);
> if (cluster_id != MPIDR_TO_SGI_CLUSTER_ID(mpidr)) {
> cpu--;
> goto out;
> @@ -1318,7 +1328,7 @@ static void gic_ipi_send_mask(struct irq_data *d,
> const struct cpumask *mask)
> dsb(ishst);
>
> for_each_cpu(cpu, mask) {
> - u64 cluster_id = MPIDR_TO_SGI_CLUSTER_ID(cpu_logical_map(cpu));
> + u64 cluster_id =
> MPIDR_TO_SGI_CLUSTER_ID(gic_cpu_to_affinity(cpu));
> u16 tlist;
>
> tlist = gic_compute_target_list(&cpu, mask, cluster_id); @@ -1376,7
> +1386,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask
> *mask_val,
>
> offset = convert_offset_index(d, GICD_IROUTER, &index);
> reg = gic_dist_base(d) + offset + (index * 8);
> - val = gic_mpidr_to_affinity(cpu_logical_map(cpu));
> + val = gic_cpu_to_affinity(cpu);
>
> gic_write_irouter(val, reg);
>
> @@ -1786,6 +1796,15 @@ static bool gic_enable_quirk_nvidia_t241(void
> *data)
> return true;
> }
>
> +static bool gic_enable_quirk_asr8601(void *data) {
> + struct gic_chip_data *d = data;
> +
> + d->flags |= FLAGS_WORKAROUND_ASR_ERRATUM_8601001;
> +
> + return true;
> +}
> +
> static const struct gic_quirk gic_quirks[] = {
> {
> .desc = "GICv3: Qualcomm MSM8996 broken firmware",
> @@ -1823,6 +1842,11 @@ static const struct gic_quirk gic_quirks[] = {
> .mask = 0xffffffff,
> .init = gic_enable_quirk_nvidia_t241,
> },
> + {
> + .desc = "GICv3: ASR erratum 8601001",
> + .compatible = "asr,asr8601-gic-v3",
> + .init = gic_enable_quirk_asr8601,
> + },
> {
> }
> };
>
>
> --
> Without deviation from the norm, progress is not possible.
On Tue, 30 May 2023 07:07:21 +0100,
Yan Zheng（严政） <[email protected]> wrote:
>
> > I came up with the following patch, which keeps the workaround *in
> > a single spot*.
> >
> > Let me know if that works for you.
> >
> > M.
> >
>
> Really thanks for your help, this patch works well for us.
> My previous patch was too ugly, this one looks better. I didn't find this good solution.
> Should I send a patch for v3, considering that you're the author?
See [1]. I've split the refactoring in its own patch, and kept your
authorship for what is left of the workaround code.
No need to resend, I'll queue that for 6.5.
M.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-v3-asr8601
--
Without deviation from the norm, progress is not possible.