| Message ID | 20210305120930.14297-1-mark-pk.tsai@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/irq-mst: Support polarity configuration | expand |
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
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.
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.
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.
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.
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.
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.
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; }
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(-)