[PATCH] irqchip/irq-mst: Support polarity configuration

[PATCH] irqchip/irq-mst: Support polarity configuration

[Mark-PK Tsai]

Support irq polarity configuration and save and restore the config
when system suspend and resume.

Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
---
 drivers/irqchip/irq-mst-intc.c | 87 ++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
index 143657b0cf28..979a4d55bcb1 100644
--- a/drivers/irqchip/irq-mst-intc.c
+++ b/drivers/irqchip/irq-mst-intc.c
@@ -13,15 +13,25 @@
 #include <linux/of_irq.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/syscore_ops.h>
 
-#define INTC_MASK	0x0
-#define INTC_EOI	0x20
+#define INTC_MASK		0x0
+#define INTC_REV_POLARITY	0x10
+#define INTC_EOI		0x20
+
+#ifdef CONFIG_PM_SLEEP
+static LIST_HEAD(mst_intc_list);
+#endif
 
 struct mst_intc_chip_data {
 	raw_spinlock_t	lock;
 	unsigned int	irq_start, nr_irqs;
 	void __iomem	*base;
 	bool		no_eoi;
+#ifdef CONFIG_PM_SLEEP
+	struct list_head entry;
+	u16 saved_polarity_conf[DIV_ROUND_UP(64, 16)];
+#endif
 };
 
 static void mst_set_irq(struct irq_data *d, u32 offset)
@@ -78,6 +88,16 @@ static void mst_intc_eoi_irq(struct irq_data *d)
 	irq_chip_eoi_parent(d);
 }
 
+static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type)
+{
+	if (type == IRQ_TYPE_LEVEL_LOW) {
+		mst_set_irq(data, INTC_REV_POLARITY);
+		type = IRQ_TYPE_LEVEL_HIGH;
+	}
+
+	return irq_chip_set_type_parent(data, type);
+}
+
 static struct irq_chip mst_intc_chip = {
 	.name			= "mst-intc",
 	.irq_mask		= mst_intc_mask_irq,
@@ -87,13 +107,62 @@ static struct irq_chip mst_intc_chip = {
 	.irq_set_irqchip_state	= irq_chip_set_parent_state,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
 	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
-	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_type		= mst_irq_chip_set_type,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.flags			= IRQCHIP_SET_TYPE_MASKED |
 				  IRQCHIP_SKIP_SET_WAKE |
 				  IRQCHIP_MASK_ON_SUSPEND,
 };
 
+#ifdef CONFIG_PM_SLEEP
+static void mst_intc_polarity_save(struct mst_intc_chip_data *cd)
+{
+	int i;
+	void __iomem *addr = cd->base + INTC_REV_POLARITY;
+
+	for (i = 0; i < DIV_ROUND_UP(cd->nr_irqs, 16); i++)
+		cd->saved_polarity_conf[i] = readw_relaxed(addr + i * 4);
+}
+
+static void mst_intc_polarity_restore(struct mst_intc_chip_data *cd)
+{
+	int i;
+	void __iomem *addr = cd->base + INTC_REV_POLARITY;
+
+	for (i = 0; i < DIV_ROUND_UP(cd->nr_irqs, 16); i++)
+		writew_relaxed(cd->saved_polarity_conf[i], addr + i * 4);
+}
+
+static void mst_irq_resume(void)
+{
+	struct mst_intc_chip_data *cd;
+
+	list_for_each_entry(cd, &mst_intc_list, entry)
+		mst_intc_polarity_restore(cd);
+}
+
+static int mst_irq_suspend(void)
+{
+	struct mst_intc_chip_data *cd;
+
+	list_for_each_entry(cd, &mst_intc_list, entry)
+		mst_intc_polarity_save(cd);
+	return 0;
+}
+
+static struct syscore_ops mst_irq_syscore_ops = {
+	.suspend	= mst_irq_suspend,
+	.resume		= mst_irq_resume,
+};
+
+static int __init mst_irq_pm_init(void)
+{
+	register_syscore_ops(&mst_irq_syscore_ops);
+	return 0;
+}
+late_initcall(mst_irq_pm_init);
+#endif
+
 static int mst_intc_domain_translate(struct irq_domain *d,
 				     struct irq_fwspec *fwspec,
 				     unsigned long *hwirq,
@@ -145,6 +214,14 @@ static int mst_intc_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	parent_fwspec = *fwspec;
 	parent_fwspec.fwnode = domain->parent->fwnode;
 	parent_fwspec.param[1] = cd->irq_start + hwirq;
+
+	/*
+	 * If the irq signal is active low, configure it to active high
+	 * to meet GIC SPI spec in mst_irq_chip_set_type via REV_POLARITY bit
+	 */
+	if (fwspec->param[2] == IRQ_TYPE_LEVEL_LOW)
+		parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
+
 	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec);
 }
 
@@ -193,6 +270,10 @@ static int __init mst_intc_of_init(struct device_node *dn,
 		return -ENOMEM;
 	}
 
+#ifdef CONFIG_PM_SLEEP
+	INIT_LIST_HEAD(&cd->entry);
+	list_add_tail(&cd->entry, &mst_intc_list);
+#endif
 	return 0;
 }
 
-- 
2.18.0

Re: [PATCH] irqchip/irq-mst: Support polarity configuration

[Daniel Palmer]

Hi Mark-PK,

I'm trying to understand the logic behind the changes.
It seems like the polarity of interrupts is always the same between
the MStar intc and the GIC? Low level interrupts are handled in the
mstar intc and become high level interrupts to the GIC?
I think for the Mstar MSC313(e) and SigmaStar chips all of the
internal interrupts are high level so I never noticed this behaviour.
I can't remember seeing anything that handled this in the MStar kernel
code I looked at.
Is this specific to a certain chip or does it apply for everything
with this intc?
The register values being lost if the chip goes into suspend to memory
makes sense for the MStar chips too I think as everything that is not
in the "pmsleep" register group seems to be lost.

Thanks,

Daniel

Re: [PATCH] irqchip/irq-mst: Support polarity configuration

[Marc Zyngier]

On Sat, 06 Mar 2021 17:06:51 +0000,
Daniel Palmer <daniel@0x0f.com> wrote:
> 
> Hi Mark-PK,
> 
> I'm trying to understand the logic behind the changes.
> It seems like the polarity of interrupts is always the same between
> the MStar intc and the GIC? Low level interrupts are handled in the
> mstar intc and become high level interrupts to the GIC?

That's because the GIC only supports level-high input interrupts when
they are level triggered (and rising edge when edge triggered).

Thanks,

	M.

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

Re: [PATCH] irqchip/irq-mst: Support polarity configuration

[Marc Zyngier]

On Fri, 05 Mar 2021 12:09:30 +0000,
Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
> 
> Support irq polarity configuration and save and restore the config
> when system suspend and resume.
> 
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
>  drivers/irqchip/irq-mst-intc.c | 87 ++++++++++++++++++++++++++++++++--
>  1 file changed, 84 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
> index 143657b0cf28..979a4d55bcb1 100644
> --- a/drivers/irqchip/irq-mst-intc.c
> +++ b/drivers/irqchip/irq-mst-intc.c
> @@ -13,15 +13,25 @@
>  #include <linux/of_irq.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/syscore_ops.h>
>  
> -#define INTC_MASK	0x0
> -#define INTC_EOI	0x20
> +#define INTC_MASK		0x0
> +#define INTC_REV_POLARITY	0x10
> +#define INTC_EOI		0x20
> +
> +#ifdef CONFIG_PM_SLEEP
> +static LIST_HEAD(mst_intc_list);
> +#endif
>  
>  struct mst_intc_chip_data {
>  	raw_spinlock_t	lock;
>  	unsigned int	irq_start, nr_irqs;
>  	void __iomem	*base;
>  	bool		no_eoi;
> +#ifdef CONFIG_PM_SLEEP
> +	struct list_head entry;
> +	u16 saved_polarity_conf[DIV_ROUND_UP(64, 16)];

Where is this 64 coming from?

> +#endif
>  };
>  
>  static void mst_set_irq(struct irq_data *d, u32 offset)
> @@ -78,6 +88,16 @@ static void mst_intc_eoi_irq(struct irq_data *d)
>  	irq_chip_eoi_parent(d);
>  }
>  
> +static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type)
> +{
> +	if (type == IRQ_TYPE_LEVEL_LOW) {
> +		mst_set_irq(data, INTC_REV_POLARITY);
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +	}

If you are introducing a irq_set_type callback, you need to return an
error for the settings you don't handle.

Thanks,

	M.

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

RE: [PATCH] irqchip/irq-mst: Support polarity configuration

[Mark-PK Tsai (蔡沛剛)]

> Hi Mark-PK,
>
> I'm trying to understand the logic behind the changes.
> It seems like the polarity of interrupts is always the same between the MStar intc and the GIC? Low level interrupts are handled in the mstar intc and become high level interrupts to the GIC?
> I think for the Mstar MSC313(e) and SigmaStar chips all of the internal interrupts are high level so I never noticed this behaviour.
> I can't remember seeing anything that handled this in the MStar kernel code I looked at.
> Is this specific to a certain chip or does it apply for everything with this intc?

I suppose Mstar SoCs also need this patch which depends on what kind of interrupt source the HW designer wire to this intc.
If an interrupt source is active low, we need to set the corresponding bit to reverse the polarity to meet GIC SPI requirement as Marc mentioned.

> The register values being lost if the chip goes into suspend to memory makes sense for the MStar chips too I think as everything that is not in the "pmsleep" register group seems to be lost.

There are mask and eoi bits I did not handle here.
That's because kernel will handle the mask and eoi status when system going to suspend/resume in suspend_device_irqs/ resume_device_irqs.
And all the irqs of Mstar intc are masked by default when the IP powered on.


Best regards,
Mark-PK Tsai


-----Original Message-----
From: Marc Zyngier [mailto:maz@kernel.org] 
Sent: Sunday, March 7, 2021 2:28 AM
To: Daniel Palmer <daniel@0x0f.com>
Cc: Mark-PK Tsai (蔡沛剛) <Mark-PK.Tsai@mediatek.com>; Daniel Palmer <daniel@thingy.jp>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Matthias Brugger <matthias.bgg@gmail.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; linux-mediatek@lists.infradead.org; YJ Chiang (江英杰) <yj.chiang@mediatek.com>
Subject: Re: [PATCH] irqchip/irq-mst: Support polarity configuration

On Sat, 06 Mar 2021 17:06:51 +0000,
Daniel Palmer <daniel@0x0f.com> wrote:
> 
> Hi Mark-PK,
> 
> I'm trying to understand the logic behind the changes.
> It seems like the polarity of interrupts is always the same between 
> the MStar intc and the GIC? Low level interrupts are handled in the 
> mstar intc and become high level interrupts to the GIC?

That's because the GIC only supports level-high input interrupts when they are level triggered (and rising edge when edge triggered).

Thanks,

	M.

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

Re: [PATCH] irqchip/irq-mst: Support polarity configuration

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

> > 
> > Support irq polarity configuration and save and restore the config
> > when system suspend and resume.
> > 
> > Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> > ---
> >  drivers/irqchip/irq-mst-intc.c | 87 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 84 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
> > index 143657b0cf28..979a4d55bcb1 100644
> > --- a/drivers/irqchip/irq-mst-intc.c
> > +++ b/drivers/irqchip/irq-mst-intc.c
> > @@ -13,15 +13,25 @@
> >  #include <linux/of_irq.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/syscore_ops.h>
> >  
> > -#define INTC_MASK	0x0
> > -#define INTC_EOI	0x20
> > +#define INTC_MASK		0x0
> > +#define INTC_REV_POLARITY	0x10
> > +#define INTC_EOI		0x20
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static LIST_HEAD(mst_intc_list);
> > +#endif
> >  
> >  struct mst_intc_chip_data {
> >  	raw_spinlock_t	lock;
> >  	unsigned int	irq_start, nr_irqs;
> >  	void __iomem	*base;
> >  	bool		no_eoi;
> > +#ifdef CONFIG_PM_SLEEP
> > +	struct list_head entry;
> > +	u16 saved_polarity_conf[DIV_ROUND_UP(64, 16)];
> 
> Where is this 64 coming from?

The maximum number of interrupts a mst-intc supports is 64 in
Mstar and Mediatek SoCs now.
So I just use the maximum number of interrupts here.

> 
> > +#endif
> >  };
> >  
> >  static void mst_set_irq(struct irq_data *d, u32 offset)
> > @@ -78,6 +88,16 @@ static void mst_intc_eoi_irq(struct irq_data *d)
> >  	irq_chip_eoi_parent(d);
> >  }
> >  
> > +static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type)
> > +{
> > +	if (type == IRQ_TYPE_LEVEL_LOW) {
> > +		mst_set_irq(data, INTC_REV_POLARITY);
> > +		type = IRQ_TYPE_LEVEL_HIGH;
> > +	}
> 
> If you are introducing a irq_set_type callback, you need to return an
> error for the settings you don't handle.

Got it, thanks for the comment.
I will add it in the next patch.

Re: [PATCH] irqchip/irq-mst: Support polarity configuration

[Marc Zyngier]

On 2021-03-08 03:01, Mark-PK Tsai wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
>> >
>> > Support irq polarity configuration and save and restore the config
>> > when system suspend and resume.
>> >
>> > Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
>> > ---
>> >  drivers/irqchip/irq-mst-intc.c | 87 ++++++++++++++++++++++++++++++++--
>> >  1 file changed, 84 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
>> > index 143657b0cf28..979a4d55bcb1 100644
>> > --- a/drivers/irqchip/irq-mst-intc.c
>> > +++ b/drivers/irqchip/irq-mst-intc.c
>> > @@ -13,15 +13,25 @@
>> >  #include <linux/of_irq.h>
>> >  #include <linux/slab.h>
>> >  #include <linux/spinlock.h>
>> > +#include <linux/syscore_ops.h>
>> >
>> > -#define INTC_MASK	0x0
>> > -#define INTC_EOI	0x20
>> > +#define INTC_MASK		0x0
>> > +#define INTC_REV_POLARITY	0x10
>> > +#define INTC_EOI		0x20
>> > +
>> > +#ifdef CONFIG_PM_SLEEP
>> > +static LIST_HEAD(mst_intc_list);
>> > +#endif
>> >
>> >  struct mst_intc_chip_data {
>> >  	raw_spinlock_t	lock;
>> >  	unsigned int	irq_start, nr_irqs;
>> >  	void __iomem	*base;
>> >  	bool		no_eoi;
>> > +#ifdef CONFIG_PM_SLEEP
>> > +	struct list_head entry;
>> > +	u16 saved_polarity_conf[DIV_ROUND_UP(64, 16)];
>> 
>> Where is this 64 coming from?
> 
> The maximum number of interrupts a mst-intc supports is 64 in
> Mstar and Mediatek SoCs now.
> So I just use the maximum number of interrupts here.

Then please use a named constant instead of a magic number.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqchip/irq-mst: Support polarity configuration

[Daniel Palmer]

Hi Mark-PK,

On Mon, 8 Mar 2021 at 11:42, Mark-PK Tsai (蔡沛剛)
<Mark-PK.Tsai@mediatek.com> wrote:
> > It seems like the polarity of interrupts is always the same between the MStar intc and the GIC? Low level interrupts are handled in the mstar intc and become high level interrupts to the GIC?
> > I think for the Mstar MSC313(e) and SigmaStar chips all of the internal interrupts are high level so I never noticed this behaviour.
> > I can't remember seeing anything that handled this in the MStar kernel code I looked at.
> > Is this specific to a certain chip or does it apply for everything with this intc?
>
> I suppose Mstar SoCs also need this patch which depends on what kind of interrupt source the HW designer wire to this intc.
> If an interrupt source is active low, we need to set the corresponding bit to reverse the polarity to meet GIC SPI requirement as Marc mentioned.

That makes sense. Looking back at the original MStar driver again they
did handle reversing the polarity but they passed the original
polarity through to the GIC too.
So maybe it never worked properly. :)

I think I can test this with GPIOs that are wired into the mstar intc
on the MStar/SigmaStar chips. I'll get back to you with how that works
out.

Thanks,

Daniel



>
> > The register values being lost if the chip goes into suspend to memory makes sense for the MStar chips too I think as everything that is not in the "pmsleep" register group seems to be lost.
>
> There are mask and eoi bits I did not handle here.
> That's because kernel will handle the mask and eoi status when system going to suspend/resume in suspend_device_irqs/ resume_device_irqs.
> And all the irqs of Mstar intc are masked by default when the IP powered on.
>
>
> Best regards,
> Mark-PK Tsai
>
>
> -----Original Message-----
> From: Marc Zyngier [mailto:maz@kernel.org]
> Sent: Sunday, March 7, 2021 2:28 AM
> To: Daniel Palmer <daniel@0x0f.com>
> Cc: Mark-PK Tsai (蔡沛剛) <Mark-PK.Tsai@mediatek.com>; Daniel Palmer <daniel@thingy.jp>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Matthias Brugger <matthias.bgg@gmail.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; linux-mediatek@lists.infradead.org; YJ Chiang (江英杰) <yj.chiang@mediatek.com>
> Subject: Re: [PATCH] irqchip/irq-mst: Support polarity configuration
>
> On Sat, 06 Mar 2021 17:06:51 +0000,
> Daniel Palmer <daniel@0x0f.com> wrote:
> >
> > Hi Mark-PK,
> >
> > I'm trying to understand the logic behind the changes.
> > It seems like the polarity of interrupts is always the same between
> > the MStar intc and the GIC? Low level interrupts are handled in the
> > mstar intc and become high level interrupts to the GIC?
>
> That's because the GIC only supports level-high input interrupts when they are level triggered (and rising edge when edge triggered).
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip/irq-mst: Support polarity configuration

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

>On 2021-03-08 03:01, Mark-PK Tsai wrote:
>> From: Marc Zyngier <maz@kernel.org>
>> 
>>> >
>>> > Support irq polarity configuration and save and restore the config
>>> > when system suspend and resume.
>>> >
>>> > Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
>>> > ---
>>> >  drivers/irqchip/irq-mst-intc.c | 87 ++++++++++++++++++++++++++++++++--
>>> >  1 file changed, 84 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
>>> > index 143657b0cf28..979a4d55bcb1 100644
>>> > --- a/drivers/irqchip/irq-mst-intc.c
>>> > +++ b/drivers/irqchip/irq-mst-intc.c
>>> > @@ -13,15 +13,25 @@
>>> >  #include <linux/of_irq.h>
>>> >  #include <linux/slab.h>
>>> >  #include <linux/spinlock.h>
>>> > +#include <linux/syscore_ops.h>
>>> >
>>> > -#define INTC_MASK	0x0
>>> > -#define INTC_EOI	0x20
>>> > +#define INTC_MASK		0x0
>>> > +#define INTC_REV_POLARITY	0x10
>>> > +#define INTC_EOI		0x20
>>> > +
>>> > +#ifdef CONFIG_PM_SLEEP
>>> > +static LIST_HEAD(mst_intc_list);
>>> > +#endif
>>> >
>>> >  struct mst_intc_chip_data {
>>> >  	raw_spinlock_t	lock;
>>> >  	unsigned int	irq_start, nr_irqs;
>>> >  	void __iomem	*base;
>>> >  	bool		no_eoi;
>>> > +#ifdef CONFIG_PM_SLEEP
>>> > +	struct list_head entry;
>>> > +	u16 saved_polarity_conf[DIV_ROUND_UP(64, 16)];
>>> 
>>> Where is this 64 coming from?
>> 
>> The maximum number of interrupts a mst-intc supports is 64 in
>> Mstar and Mediatek SoCs now.
>> So I just use the maximum number of interrupts here.
>
>Then please use a named constant instead of a magic number.
>

Okay, I will update in patch v3.