Register ARMADA_370_XP_INT_FABRIC_MASK_OFFS is Armada 370 and XP specific
and on new Armada platforms it has different meaning. It does not configure
Performance Counter Overflow interrupt masking. So do not touch this
register on non-A370/XP platforms (A375, A38x and A39x).
Signed-off-by: Pali Rohár <[email protected]>
Cc: [email protected]
---
drivers/irqchip/irq-armada-370-xp.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 5b8d571c041d..1120084cba09 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -308,7 +308,16 @@ static inline int armada_370_xp_msi_init(struct device_node *node,
static void armada_xp_mpic_perf_init(void)
{
- unsigned long cpuid = cpu_logical_map(smp_processor_id());
+ unsigned long cpuid;
+
+ /*
+ * This Performance Counter Overflow interrupt is specific for
+ * Armada 370 and XP. It is not available on Armada 375, 38x and 39x.
+ */
+ if (!of_machine_is_compatible("marvell,armada-370-xp"))
+ return;
+
+ cpuid = cpu_logical_map(smp_processor_id());
/* Enable Performance Counter Overflow interrupts */
writel(ARMADA_370_XP_INT_CAUSE_PERF(cpuid),
--
2.20.1
IRQs 0 and 1 cannot be mapped, they are handled internally by this driver
and this driver does not call generic_handle_domain_irq() for these IRQs.
So do not allow mapping these IRQs and correctly propagate error from the
.irq_map callback.
Signed-off-by: Pali Rohár <[email protected]>
Cc: [email protected]
---
drivers/irqchip/irq-armada-370-xp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 1120084cba09..ebd76ea1c69b 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -546,6 +546,10 @@ static struct irq_chip armada_370_xp_irq_chip = {
static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
unsigned int virq, irq_hw_number_t hw)
{
+ /* IRQs 0 and 1 cannot be mapped, they are handled internally */
+ if (hw <= 1)
+ return -EINVAL;
+
armada_370_xp_irq_mask(irq_get_irq_data(virq));
if (!is_percpu_irq(hw))
writel(hw, per_cpu_int_base +
--
2.20.1
On Friday 29 April 2022 14:23:08 Andrew Lunn wrote:
> On Mon, Apr 25, 2022 at 01:37:05PM +0200, Pali Rohár wrote:
> > Register ARMADA_370_XP_INT_FABRIC_MASK_OFFS is Armada 370 and XP specific
> > and on new Armada platforms it has different meaning. It does not configure
> > Performance Counter Overflow interrupt masking. So do not touch this
> > register on non-A370/XP platforms (A375, A38x and A39x).
>
> Hi Pali
>
> Do the Armada 375, 38x and 39x have an over flow interrupt? I assume
> not.
Hello! According to documentation there is something named performance
counter interrupt, but it is in different register... and this register
is not per-cpu.
> Does this need a fixes tag? Should it be back ported in stable?
git blame show that this functionality appeared in commit 28da06dfd9e4
("irqchip: armada-370-xp: Enable the PMU interrupts").
On Fri, Apr 29, 2022 at 03:05:24PM +0200, Pali Rohár wrote:
> On Friday 29 April 2022 14:23:08 Andrew Lunn wrote:
> > On Mon, Apr 25, 2022 at 01:37:05PM +0200, Pali Rohár wrote:
> > > Register ARMADA_370_XP_INT_FABRIC_MASK_OFFS is Armada 370 and XP specific
> > > and on new Armada platforms it has different meaning. It does not configure
> > > Performance Counter Overflow interrupt masking. So do not touch this
> > > register on non-A370/XP platforms (A375, A38x and A39x).
> >
> > Hi Pali
> >
> > Do the Armada 375, 38x and 39x have an over flow interrupt? I assume
> > not.
>
> Hello! According to documentation there is something named performance
> counter interrupt, but it is in different register... and this register
> is not per-cpu.
O.K, not something which can be quickly added.
> > Does this need a fixes tag? Should it be back ported in stable?
>
> git blame show that this functionality appeared in commit 28da06dfd9e4
> ("irqchip: armada-370-xp: Enable the PMU interrupts").
It is more a question of:
o It must fix a real bug that bothers people (not a, “This could be a
problem…” type thing).
From https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
Have you seen bad things happen because of this?
Andrew
On Friday 29 April 2022 14:30:53 Andrew Lunn wrote:
> On Mon, Apr 25, 2022 at 01:37:06PM +0200, Pali Rohár wrote:
> > IRQs 0 and 1 cannot be mapped, they are handled internally by this driver
> > and this driver does not call generic_handle_domain_irq() for these IRQs.
> > So do not allow mapping these IRQs and correctly propagate error from the
> > .irq_map callback.
>
> So you are referring to this?
>
> /* Check if the interrupt is not masked on current CPU.
> * Test IRQ (0-1) and FIQ (8-9) mask bits.
> */
> if (!(irqsrc & ARMADA_370_XP_INT_IRQ_FIQ_MASK(cpuid)))
> continue;
>
> if (irqn == 1) {
> armada_370_xp_handle_msi_irq(NULL, true);
> continue;
> }
I'm referring to irqn, which is not handled on armada_370_xp_mpic_domain
via generic_handle_domain_irq() when it is == 1.
Also I'm referring to another section:
if (irqnr > 1) {
generic_handle_domain_irq(armada_370_xp_mpic_domain,
irqnr);
continue;
}
/* MSI handling */
if (irqnr == 1)
armada_370_xp_handle_msi_irq(regs, false);
#ifdef CONFIG_SMP
/* IPI Handling */
if (irqnr == 0) {
unsigned long ipimask;
int ipi;
ipimask = readl_relaxed(per_cpu_int_base +
ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
& IPI_DOORBELL_MASK;
for_each_set_bit(ipi, &ipimask, IPI_DOORBELL_END)
generic_handle_domain_irq(ipi_domain, ipi);
}
#endif
First 'if (irqnr > 1)' just cause that irqnr 0 and 1 are not handled on
armada_370_xp_mpic_domain via generic_handle_domain_irq().
>
> Should the two FIQ interrupts also return -EINVAL?
>
> Andrew
No. Following code
irqsrc = readl_relaxed(main_int_base +
ARMADA_370_XP_INT_SOURCE_CTL(irqn));
/* Check if the interrupt is not masked on current CPU.
* Test IRQ (0-1) and FIQ (8-9) mask bits.
*/
if (!(irqsrc & ARMADA_370_XP_INT_IRQ_FIQ_MASK(cpuid)))
continue;
skips processing irqn interrupt if it is masked for the current CPU.
Interrupt irqn is **unmasked** on CPU0 if either bit 0 or 8 is set in
register ARMADA_370_XP_INT_IRQ_FIQ_MASK. And **unmasked** on CPU1 if
either bit 1 or 9 is set.
The reason for this check and skipping is because some per-cpu
interrupts on A375, A38x and A39x can be handled directly via GIC and
also via MPIC subhierarchy (it depends what you put into DTS, both
options are possible and working, just some interrupts are MPIC-only).
So if you unmask some interrupt on GIC and interrupt is triggered then
also MPIC per-CPU cause register contains in bit for that triggered
interrupt (even you have not asked MPIC to unmask interrupt). And once
another MPIC interrupt is triggered then routine for handling MPIC
subhierarchy see that some cause bits are set and tried to call
generic_handle_domain_irq() for all of them, also for those which are
masked. That is why it is needed to check if MPIC interrupt is masked on
the current CPU or not prior processing it.
So those bits 0-1 and 8-9 have nothing with mapping interrupts.
On Saturday 30 April 2022 00:23:34 Andrew Lunn wrote:
> On Fri, Apr 29, 2022 at 03:05:24PM +0200, Pali Rohár wrote:
> > On Friday 29 April 2022 14:23:08 Andrew Lunn wrote:
> > > On Mon, Apr 25, 2022 at 01:37:05PM +0200, Pali Rohár wrote:
> > > > Register ARMADA_370_XP_INT_FABRIC_MASK_OFFS is Armada 370 and XP specific
> > > > and on new Armada platforms it has different meaning. It does not configure
> > > > Performance Counter Overflow interrupt masking. So do not touch this
> > > > register on non-A370/XP platforms (A375, A38x and A39x).
> > >
> > > Hi Pali
> > >
> > > Do the Armada 375, 38x and 39x have an over flow interrupt? I assume
> > > not.
> >
> > Hello! According to documentation there is something named performance
> > counter interrupt, but it is in different register... and this register
> > is not per-cpu.
>
> O.K, not something which can be quickly added.
>
> > > Does this need a fixes tag? Should it be back ported in stable?
> >
> > git blame show that this functionality appeared in commit 28da06dfd9e4
> > ("irqchip: armada-370-xp: Enable the PMU interrupts").
>
> It is more a question of:
>
> o It must fix a real bug that bothers people (not a, “This could be a
> problem…” type thing).
>
> From https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>
> Have you seen bad things happen because of this?
>
> Andrew
On those new Armada SoCs that register is used for unmasking interrupts
from another hierarchy. Until you start using those new interrupts there
probably could not be any issue. But in any case unmasking unwanted
interrupt is not a wise idea...
I spotted this because I started using those extended interrupts from
another hierarchy. I'm planning to send a patch which properly export
them via DTS. Consumer would be pci-mvebu.c driver as in this extended
hierarchy are PCIe AER and PME interrupts.
Anyway, Fixes tag is probably better in this situation than stable tag.
So would it be fine to replace it by?
Fixes: 28da06dfd9e4 ("irqchip: armada-370-xp: Enable the PMU interrupts")
> On those new Armada SoCs that register is used for unmasking interrupts
> from another hierarchy. Until you start using those new interrupts there
> probably could not be any issue. But in any case unmasking unwanted
> interrupt is not a wise idea...
O.K. Please add the Fixes tag.
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Mon, Apr 25, 2022 at 01:37:05PM +0200, Pali Roh?r wrote:
> Register ARMADA_370_XP_INT_FABRIC_MASK_OFFS is Armada 370 and XP specific
> and on new Armada platforms it has different meaning. It does not configure
> Performance Counter Overflow interrupt masking. So do not touch this
> register on non-A370/XP platforms (A375, A38x and A39x).
Hi Pali
Do the Armada 375, 38x and 39x have an over flow interrupt? I assume
not.
Does this need a fixes tag? Should it be back ported in stable?
Andrew
On Mon, Apr 25, 2022 at 01:37:06PM +0200, Pali Roh?r wrote:
> IRQs 0 and 1 cannot be mapped, they are handled internally by this driver
> and this driver does not call generic_handle_domain_irq() for these IRQs.
> So do not allow mapping these IRQs and correctly propagate error from the
> .irq_map callback.
So you are referring to this?
/* Check if the interrupt is not masked on current CPU.
* Test IRQ (0-1) and FIQ (8-9) mask bits.
*/
if (!(irqsrc & ARMADA_370_XP_INT_IRQ_FIQ_MASK(cpuid)))
continue;
if (irqn == 1) {
armada_370_xp_handle_msi_irq(NULL, true);
continue;
}
Should the two FIQ interrupts also return -EINVAL?
Andrew
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: a3d66a76348daf559873f19afc912a2a7c2ccdaf
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/a3d66a76348daf559873f19afc912a2a7c2ccdaf
Author: Pali Rohár <[email protected]>
AuthorDate: Mon, 25 Apr 2022 13:37:05 +02:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Fri, 06 May 2022 12:18:37 +01:00
irqchip/armada-370-xp: Do not touch Performance Counter Overflow on A375, A38x, A39x
Register ARMADA_370_XP_INT_FABRIC_MASK_OFFS is Armada 370 and XP specific
and on new Armada platforms it has different meaning. It does not configure
Performance Counter Overflow interrupt masking. So do not touch this
register on non-A370/XP platforms (A375, A38x and A39x).
Signed-off-by: Pali Rohár <[email protected]>
Cc: [email protected]
Fixes: 28da06dfd9e4 ("irqchip: armada-370-xp: Enable the PMU interrupts")
Reviewed-by: Andrew Lunn <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-armada-370-xp.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index c877285..ee18eb3 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -327,7 +327,16 @@ static inline int armada_370_xp_msi_init(struct device_node *node,
static void armada_xp_mpic_perf_init(void)
{
- unsigned long cpuid = cpu_logical_map(smp_processor_id());
+ unsigned long cpuid;
+
+ /*
+ * This Performance Counter Overflow interrupt is specific for
+ * Armada 370 and XP. It is not available on Armada 375, 38x and 39x.
+ */
+ if (!of_machine_is_compatible("marvell,armada-370-xp"))
+ return;
+
+ cpuid = cpu_logical_map(smp_processor_id());
/* Enable Performance Counter Overflow interrupts */
writel(ARMADA_370_XP_INT_CAUSE_PERF(cpuid),
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: baf78c1078b474aed18864796a8161784dd81fc2
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/baf78c1078b474aed18864796a8161784dd81fc2
Author: Pali Rohár <[email protected]>
AuthorDate: Mon, 25 Apr 2022 13:37:06 +02:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Fri, 06 May 2022 12:20:27 +01:00
irqchip/armada-370-xp: Do not allow mapping IRQ 0 and 1
IRQs 0 and 1 cannot be mapped, they are handled internally by this driver
and this driver does not call generic_handle_domain_irq() for these IRQs.
So do not allow mapping these IRQs and correctly propagate error from the
.irq_map callback.
Signed-off-by: Pali Rohár <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-armada-370-xp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index ee18eb3..ab02b44 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -567,6 +567,10 @@ static struct irq_chip armada_370_xp_irq_chip = {
static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
unsigned int virq, irq_hw_number_t hw)
{
+ /* IRQs 0 and 1 cannot be mapped, they are handled internally */
+ if (hw <= 1)
+ return -EINVAL;
+
armada_370_xp_irq_mask(irq_get_irq_data(virq));
if (!is_percpu_irq(hw))
writel(hw, per_cpu_int_base +
On Mon, 25 Apr 2022 12:37:06 +0100,
Pali Rohár <[email protected]> wrote:
>
> IRQs 0 and 1 cannot be mapped, they are handled internally by this driver
> and this driver does not call generic_handle_domain_irq() for these IRQs.
> So do not allow mapping these IRQs and correctly propagate error from the
> .irq_map callback.
>
> Signed-off-by: Pali Rohár <[email protected]>
> Cc: [email protected]
> ---
> drivers/irqchip/irq-armada-370-xp.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 1120084cba09..ebd76ea1c69b 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -546,6 +546,10 @@ static struct irq_chip armada_370_xp_irq_chip = {
> static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
> unsigned int virq, irq_hw_number_t hw)
> {
> + /* IRQs 0 and 1 cannot be mapped, they are handled internally */
> + if (hw <= 1)
> + return -EINVAL;
> +
> armada_370_xp_irq_mask(irq_get_irq_data(virq));
> if (!is_percpu_irq(hw))
> writel(hw, per_cpu_int_base +
> --
> 2.20.1
>
>
Given that this is completely academic and obviously doesn't affect
anyone, I have removed the Cc: stable from the patch when applying it.
M.
--
Without deviation from the norm, progress is not possible.