[PATCH] irqchip: armada: Fix chained per-cpu interrupts

[PATCH] irqchip: armada: Fix chained per-cpu interrupts

[Maxime Ripard]

On the Cortex-A9-based Armada SoCs, the MPIC is not the primary interrupt
controller. Yet, it still has to handle some per-cpu interrupt.

To do so, it is chained with the GIC using a per-cpu interrupt. However, the
current code only call irq_set_chained_handler, which is called and enable that
interrupt only on the boot CPU, which means that the parent per-CPU interrupt
is never unmasked on the secondary CPUs, preventing the per-CPU interrupt to
actually work as expected.

This was not seen until now since the only MPIC PPI users were the Marvell
timers that were not working, but not used either since the system use the ARM
TWD by default, and the ethernet controllers, that are faking there interrupts
as SPI, and don't really expect to have interrupts on the secondary cores
anyway.

Add a CPU notifier that will enable the PPI on the secondary cores when they
are brought up.

Cc: <stable@kernel.org> # 3.15+
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 463c235acbdc..137ee37a33ed 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -69,6 +69,7 @@ static void __iomem *per_cpu_int_base;
 static void __iomem *main_int_base;
 static struct irq_domain *armada_370_xp_mpic_domain;
 static u32 doorbell_mask_reg;
+static int parent_irq;
 #ifdef CONFIG_PCI_MSI
 static struct irq_domain *armada_370_xp_msi_domain;
 static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
@@ -356,6 +357,7 @@ static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
 {
 	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
 		armada_xp_mpic_smp_cpu_init();
+
 	return NOTIFY_OK;
 }
 
@@ -364,6 +366,20 @@ static struct notifier_block armada_370_xp_mpic_cpu_notifier = {
 	.priority = 100,
 };
 
+static int armada_38x_mpic_secondary_init(struct notifier_block *nfb,
+					 unsigned long action, void *hcpu)
+{
+	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
+		enable_percpu_irq(parent_irq, IRQ_TYPE_NONE);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block armada_38x_mpic_cpu_notifier = {
+	.notifier_call = armada_38x_mpic_secondary_init,
+	.priority = 100,
+};
+
 #endif /* CONFIG_SMP */
 
 static struct irq_domain_ops armada_370_xp_mpic_irq_ops = {
@@ -539,7 +555,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 					     struct device_node *parent)
 {
 	struct resource main_int_res, per_cpu_int_res;
-	int parent_irq, nr_irqs, i;
+	int nr_irqs, i;
 	u32 control;
 
 	BUG_ON(of_address_to_resource(node, 0, &main_int_res));
@@ -587,6 +603,9 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 		register_cpu_notifier(&armada_370_xp_mpic_cpu_notifier);
 #endif
 	} else {
+#ifdef CONFIG_SMP
+		register_cpu_notifier(&armada_38x_mpic_cpu_notifier);
+#endif
 		irq_set_chained_handler(parent_irq,
 					armada_370_xp_mpic_handle_cascade_irq);
 	}
-- 
2.3.0

[PATCH] irqchip: armada: Fix chained per-cpu interrupts

[Gregory CLEMENT]

Hi Maxime,

On 26/02/2015 10:55, Maxime Ripard wrote:
> On the Cortex-A9-based Armada SoCs, the MPIC is not the primary interrupt
> controller. Yet, it still has to handle some per-cpu interrupt.
> 
> To do so, it is chained with the GIC using a per-cpu interrupt. However, the
> current code only call irq_set_chained_handler, which is called and enable that
> interrupt only on the boot CPU, which means that the parent per-CPU interrupt
> is never unmasked on the secondary CPUs, preventing the per-CPU interrupt to
> actually work as expected.
> 
> This was not seen until now since the only MPIC PPI users were the Marvell
> timers that were not working, but not used either since the system use the ARM
> TWD by default, and the ethernet controllers, that are faking there interrupts
> as SPI, and don't really expect to have interrupts on the secondary cores
> anyway.
> 
> Add a CPU notifier that will enable the PPI on the secondary cores when they
> are brought up.
> 
> Cc: <stable@kernel.org> # 3.15+
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/irqchip/irq-armada-370-xp.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 463c235acbdc..137ee37a33ed 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -69,6 +69,7 @@ static void __iomem *per_cpu_int_base;
>  static void __iomem *main_int_base;
>  static struct irq_domain *armada_370_xp_mpic_domain;
>  static u32 doorbell_mask_reg;
> +static int parent_irq;
>  #ifdef CONFIG_PCI_MSI
>  static struct irq_domain *armada_370_xp_msi_domain;
>  static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
> @@ -356,6 +357,7 @@ static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
>  {
>  	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
>  		armada_xp_mpic_smp_cpu_init();
> +
>  	return NOTIFY_OK;
>  }
>  
> @@ -364,6 +366,20 @@ static struct notifier_block armada_370_xp_mpic_cpu_notifier = {
>  	.priority = 100,
>  };
>  
The following function is called as soon as the MPIC is used as a secondary
interrupt controller. So it will be the case for the Armada 375 and Armada 39x too. It
also seems to not be related to be used in an SoC or an other, so I think that the
function name is misleading. What about just using mpic_secondary_init and
mpic_cpu_notifier ?

I know we prefixed the mpic function with armada_370_xp or armada_xp, but looking
back, it was a mistake.

Thanks,

Gregory


> +static int armada_38x_mpic_secondary_init(struct notifier_block *nfb,
> +					 unsigned long action, void *hcpu)
> +{
> +	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
> +		enable_percpu_irq(parent_irq, IRQ_TYPE_NONE);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block armada_38x_mpic_cpu_notifier = {
> +	.notifier_call = armada_38x_mpic_secondary_init,
> +	.priority = 100,
> +};
> +
>  #endif /* CONFIG_SMP */
>  
>  static struct irq_domain_ops armada_370_xp_mpic_irq_ops = {
> @@ -539,7 +555,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
>  					     struct device_node *parent)
>  {
>  	struct resource main_int_res, per_cpu_int_res;
> -	int parent_irq, nr_irqs, i;
> +	int nr_irqs, i;
>  	u32 control;
>  
>  	BUG_ON(of_address_to_resource(node, 0, &main_int_res));
> @@ -587,6 +603,9 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
>  		register_cpu_notifier(&armada_370_xp_mpic_cpu_notifier);
>  #endif
>  	} else {
> +#ifdef CONFIG_SMP
> +		register_cpu_notifier(&armada_38x_mpic_cpu_notifier);
> +#endif
>  		irq_set_chained_handler(parent_irq,
>  					armada_370_xp_mpic_handle_cascade_irq);
>  	}
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] irqchip: armada: Fix chained per-cpu interrupts

[Maxime Ripard]

Hi,

On Thu, Feb 26, 2015 at 11:26:28AM +0100, Gregory CLEMENT wrote:
> Hi Maxime,
> 
> On 26/02/2015 10:55, Maxime Ripard wrote:
> > On the Cortex-A9-based Armada SoCs, the MPIC is not the primary interrupt
> > controller. Yet, it still has to handle some per-cpu interrupt.
> > 
> > To do so, it is chained with the GIC using a per-cpu interrupt. However, the
> > current code only call irq_set_chained_handler, which is called and enable that
> > interrupt only on the boot CPU, which means that the parent per-CPU interrupt
> > is never unmasked on the secondary CPUs, preventing the per-CPU interrupt to
> > actually work as expected.
> > 
> > This was not seen until now since the only MPIC PPI users were the Marvell
> > timers that were not working, but not used either since the system use the ARM
> > TWD by default, and the ethernet controllers, that are faking there interrupts
> > as SPI, and don't really expect to have interrupts on the secondary cores
> > anyway.
> > 
> > Add a CPU notifier that will enable the PPI on the secondary cores when they
> > are brought up.
> > 
> > Cc: <stable@kernel.org> # 3.15+
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/irqchip/irq-armada-370-xp.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> > index 463c235acbdc..137ee37a33ed 100644
> > --- a/drivers/irqchip/irq-armada-370-xp.c
> > +++ b/drivers/irqchip/irq-armada-370-xp.c
> > @@ -69,6 +69,7 @@ static void __iomem *per_cpu_int_base;
> >  static void __iomem *main_int_base;
> >  static struct irq_domain *armada_370_xp_mpic_domain;
> >  static u32 doorbell_mask_reg;
> > +static int parent_irq;
> >  #ifdef CONFIG_PCI_MSI
> >  static struct irq_domain *armada_370_xp_msi_domain;
> >  static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
> > @@ -356,6 +357,7 @@ static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
> >  {
> >  	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
> >  		armada_xp_mpic_smp_cpu_init();
> > +
> >  	return NOTIFY_OK;
> >  }
> >  
> > @@ -364,6 +366,20 @@ static struct notifier_block armada_370_xp_mpic_cpu_notifier = {
> >  	.priority = 100,
> >  };
> >  
> The following function is called as soon as the MPIC is used as a secondary
> interrupt controller. So it will be the case for the Armada 375 and Armada 39x too. It
> also seems to not be related to be used in an SoC or an other, so I think that the
> function name is misleading. What about just using mpic_secondary_init and
> mpic_cpu_notifier ?
> 
> I know we prefixed the mpic function with armada_370_xp or armada_xp, but looking
> back, it was a mistake.

I don't know, that code needs to be run only in the cases where the
MPIC is a secondary interrupt controller, which rules out the armada
370/XP.

I was trying to make such a distinction, but indeed the wording is
quite poor.

Do you have some suggestions?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150226/4bbcca38/attachment.sig>

[PATCH] irqchip: armada: Fix chained per-cpu interrupts

[Gregory CLEMENT]

Hi Maxime,

n 26/02/2015 11:47, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Feb 26, 2015 at 11:26:28AM +0100, Gregory CLEMENT wrote:
>> Hi Maxime,
>>
>> On 26/02/2015 10:55, Maxime Ripard wrote:
>>> On the Cortex-A9-based Armada SoCs, the MPIC is not the primary interrupt
>>> controller. Yet, it still has to handle some per-cpu interrupt.
>>>
>>> To do so, it is chained with the GIC using a per-cpu interrupt. However, the
>>> current code only call irq_set_chained_handler, which is called and enable that
>>> interrupt only on the boot CPU, which means that the parent per-CPU interrupt
>>> is never unmasked on the secondary CPUs, preventing the per-CPU interrupt to
>>> actually work as expected.
>>>
>>> This was not seen until now since the only MPIC PPI users were the Marvell
>>> timers that were not working, but not used either since the system use the ARM
>>> TWD by default, and the ethernet controllers, that are faking there interrupts
>>> as SPI, and don't really expect to have interrupts on the secondary cores
>>> anyway.
>>>
>>> Add a CPU notifier that will enable the PPI on the secondary cores when they
>>> are brought up.
>>>
>>> Cc: <stable@kernel.org> # 3.15+
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>>  drivers/irqchip/irq-armada-370-xp.c | 21 ++++++++++++++++++++-
>>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
>>> index 463c235acbdc..137ee37a33ed 100644
>>> --- a/drivers/irqchip/irq-armada-370-xp.c
>>> +++ b/drivers/irqchip/irq-armada-370-xp.c
>>> @@ -69,6 +69,7 @@ static void __iomem *per_cpu_int_base;
>>>  static void __iomem *main_int_base;
>>>  static struct irq_domain *armada_370_xp_mpic_domain;
>>>  static u32 doorbell_mask_reg;
>>> +static int parent_irq;
>>>  #ifdef CONFIG_PCI_MSI
>>>  static struct irq_domain *armada_370_xp_msi_domain;
>>>  static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
>>> @@ -356,6 +357,7 @@ static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
>>>  {
>>>  	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
>>>  		armada_xp_mpic_smp_cpu_init();
>>> +
>>>  	return NOTIFY_OK;
>>>  }
>>>  
>>> @@ -364,6 +366,20 @@ static struct notifier_block armada_370_xp_mpic_cpu_notifier = {
>>>  	.priority = 100,
>>>  };
>>>  
>> The following function is called as soon as the MPIC is used as a secondary
>> interrupt controller. So it will be the case for the Armada 375 and Armada 39x too. It
>> also seems to not be related to be used in an SoC or an other, so I think that the
>> function name is misleading. What about just using mpic_secondary_init and
>> mpic_cpu_notifier ?
>>
>> I know we prefixed the mpic function with armada_370_xp or armada_xp, but looking
>> back, it was a mistake.
> 
> I don't know, that code needs to be run only in the cases where the
> MPIC is a secondary interrupt controller, which rules out the armada
> 370/XP.
> 
> I was trying to make such a distinction, but indeed the wording is
> quite poor.
> 
> Do you have some suggestions?

Yes using  mpic_secondary_init and  mpic_cpu_notifier, because what is important
is the fact that the MPIC is used as a secondary interrupt controller.


Thanks,

Gregory


> 
> Maxime
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] irqchip: armada: Fix chained per-cpu interrupts

[Maxime Ripard]

On Thu, Feb 26, 2015 at 11:52:52AM +0100, Gregory CLEMENT wrote:
> >> The following function is called as soon as the MPIC is used as a secondary
> >> interrupt controller. So it will be the case for the Armada 375 and Armada 39x too. It
> >> also seems to not be related to be used in an SoC or an other, so I think that the
> >> function name is misleading. What about just using mpic_secondary_init and
> >> mpic_cpu_notifier ?
> >>
> >> I know we prefixed the mpic function with armada_370_xp or armada_xp, but looking
> >> back, it was a mistake.
> > 
> > I don't know, that code needs to be run only in the cases where the
> > MPIC is a secondary interrupt controller, which rules out the armada
> > 370/XP.
> > 
> > I was trying to make such a distinction, but indeed the wording is
> > quite poor.
> > 
> > Do you have some suggestions?
> 
> Yes using  mpic_secondary_init and  mpic_cpu_notifier, because what is important
> is the fact that the MPIC is used as a secondary interrupt controller.

The thing is, the armada xp notifier is called
armada_xp_mpic_secondary_init, with in this case secondary meaning
secondary CPU. Won't that be really confusing to have another callback
called mpic_secondary_init, what would only be called on !armada
370/XP, and with secondary meaning secondary interrupt controller?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150226/dc3db871/attachment.sig>

[PATCH] irqchip: armada: Fix chained per-cpu interrupts

[Gregory CLEMENT]

On 26/02/2015 12:11, Maxime Ripard wrote:
> On Thu, Feb 26, 2015 at 11:52:52AM +0100, Gregory CLEMENT wrote:
>>>> The following function is called as soon as the MPIC is used as a secondary
>>>> interrupt controller. So it will be the case for the Armada 375 and Armada 39x too. It
>>>> also seems to not be related to be used in an SoC or an other, so I think that the
>>>> function name is misleading. What about just using mpic_secondary_init and
>>>> mpic_cpu_notifier ?
>>>>
>>>> I know we prefixed the mpic function with armada_370_xp or armada_xp, but looking
>>>> back, it was a mistake.
>>>
>>> I don't know, that code needs to be run only in the cases where the
>>> MPIC is a secondary interrupt controller, which rules out the armada
>>> 370/XP.
>>>
>>> I was trying to make such a distinction, but indeed the wording is
>>> quite poor.
>>>
>>> Do you have some suggestions?
>>
>> Yes using  mpic_secondary_init and  mpic_cpu_notifier, because what is important
>> is the fact that the MPIC is used as a secondary interrupt controller.
> 
> The thing is, the armada xp notifier is called
> armada_xp_mpic_secondary_init, with in this case secondary meaning
> secondary CPU. Won't that be really confusing to have another callback
> called mpic_secondary_init, what would only be called on !armada
> 370/XP, and with secondary meaning secondary interrupt controller?
> 

I see let's call it mpic_cascaded_init then.

> Maxime
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] irqchip: armada: Fix chained per-cpu interrupts

[Maxime Ripard]

On Thu, Feb 26, 2015 at 01:57:46PM +0100, Gregory CLEMENT wrote:
> On 26/02/2015 12:11, Maxime Ripard wrote:
> > On Thu, Feb 26, 2015 at 11:52:52AM +0100, Gregory CLEMENT wrote:
> >>>> The following function is called as soon as the MPIC is used as a secondary
> >>>> interrupt controller. So it will be the case for the Armada 375 and Armada 39x too. It
> >>>> also seems to not be related to be used in an SoC or an other, so I think that the
> >>>> function name is misleading. What about just using mpic_secondary_init and
> >>>> mpic_cpu_notifier ?
> >>>>
> >>>> I know we prefixed the mpic function with armada_370_xp or armada_xp, but looking
> >>>> back, it was a mistake.
> >>>
> >>> I don't know, that code needs to be run only in the cases where the
> >>> MPIC is a secondary interrupt controller, which rules out the armada
> >>> 370/XP.
> >>>
> >>> I was trying to make such a distinction, but indeed the wording is
> >>> quite poor.
> >>>
> >>> Do you have some suggestions?
> >>
> >> Yes using  mpic_secondary_init and  mpic_cpu_notifier, because what is important
> >> is the fact that the MPIC is used as a secondary interrupt controller.
> > 
> > The thing is, the armada xp notifier is called
> > armada_xp_mpic_secondary_init, with in this case secondary meaning
> > secondary CPU. Won't that be really confusing to have another callback
> > called mpic_secondary_init, what would only be called on !armada
> > 370/XP, and with secondary meaning secondary interrupt controller?
> 
> I see let's call it mpic_cascaded_init then.

ACK.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150226/fe2f2396/attachment.sig>