| Message ID | 20210308060538.11164-1-mark-pk.tsai@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] irqchip/irq-mst: Support polarity configuration | expand |
Hi Mark-PK, On Mon, 8 Mar 2021 at 15:05, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > +static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type) > +{ > + if (type != IRQ_TYPE_LEVEL_LOW && type != IRQ_TYPE_LEVEL_HIGH) > + return -EINVAL; > + Does this mean we can't do rising or falling edge interrupts? Thanks, Daniel
From: Daniel Palmer <daniel@0x0f.com> >On Mon, 8 Mar 2021 at 15:05, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: >> +static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type) > > +{ >> + if (type != IRQ_TYPE_LEVEL_LOW && type != IRQ_TYPE_LEVEL_HIGH) >> + return -EINVAL; >> + > >Does this mean we can't do rising or falling edge interrupts? Yes, the interrupt of mst-intc is either level high or level low. Actually the input signal can be pulse, but it will be converted to level by the latch in mst-intc.
Hi Mark-PK, On Mon, 8 Mar 2021 at 23:30, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > From: Daniel Palmer <daniel@0x0f.com> > >On Mon, 8 Mar 2021 at 15:05, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > >> +static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type) > > > +{ > >> + if (type != IRQ_TYPE_LEVEL_LOW && type != IRQ_TYPE_LEVEL_HIGH) > >> + return -EINVAL; > >> + > > > >Does this mean we can't do rising or falling edge interrupts? > > Yes, the interrupt of mst-intc is either level high or level low. > Actually the input signal can be pulse, but it will be converted to level > by the latch in mst-intc. Are the GPIO connected interrupts meant to be configured as level interrupts then? For the MStar MSC313(e) there are 4 (that I know of) GPIO lines that are wired into the mst-intc that requires EOI. Until this patch with those lines configured as a rising edge a single interrupt came each time the GPIO was pulled up as far as I remember. I'm probably misunderstanding but a level interrupt doesn't seem to make sense for a GPIO as it can't be serviced to clear the interrupt. Thanks, Daniel
From: Daniel Palmer <daniel@0x0f.com> >On Mon, 8 Mar 2021 at 23:30, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: >> From: Daniel Palmer <daniel@0x0f.com> >> >On Mon, 8 Mar 2021 at 15:05, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: >> >> +static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type) >> > > +{ >> >> + if (type != IRQ_TYPE_LEVEL_LOW && type != IRQ_TYPE_LEVEL_HIGH) >> >> + return -EINVAL; >> >> + >> > >> >Does this mean we can't do rising or falling edge interrupts? >> >> Yes, the interrupt of mst-intc is either level high or level low. >> Actually the input signal can be pulse, but it will be converted to level >> by the latch in mst-intc. > >Are the GPIO connected interrupts meant to be configured as level >interrupts then? >For the MStar MSC313(e) there are 4 (that I know of) GPIO lines that >are wired into the mst-intc that requires EOI. >Until this patch with those lines configured as a rising edge a single >interrupt came each time the GPIO was pulled up as far as I remember. > >I'm probably misunderstanding but a level interrupt doesn't seem to >make sense for a GPIO as it can't be serviced to clear the interrupt. As I understand it, there are 2 types of mst-intc, one we need do EOI operation(fiq) and the other we don't(irq). For a fiq controller, the input edge signal will be convert to level and keep the interrupt status until we do EOI operation. That means if a rising edge input if trigger the ouput line will keep high until we clear the interrupt status. And I think you're right, make these kind of interrupts edge may make more sense. So I will modify the patch as following: static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type) { - if (type != IRQ_TYPE_LEVEL_LOW && type != IRQ_TYPE_LEVEL_HIGH) - return -EINVAL; - - if (type == IRQ_TYPE_LEVEL_LOW) { + if (type == IRQ_TYPE_EDGE_FALLING) { + mst_set_irq(data, INTC_REV_POLARITY); + type = IRQ_TYPE_EDGE_RISING; + } else if (type == IRQ_TYPE_LEVEL_LOW) { mst_set_irq(data, INTC_REV_POLARITY); type = IRQ_TYPE_LEVEL_HIGH; }
Hi Mark-PK. On Thu, 11 Mar 2021 at 12:12, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > For a fiq controller, the input edge signal will be convert to level and > keep the interrupt status until we do EOI operation. > That means if a rising edge input if trigger the ouput line will keep high > until we clear the interrupt status. I think maybe the fiq is always edge triggered? It seems like it latches on an edge and holds it's output to the GIC high until it is reset by eoi and then only triggers again on another edge. I can experiment to confirm that's what it actually does for the chips I have. Then it seems like the irq version is almost just a configurable inverter that passes either the input signal or the inverted input signal to the GIC. So maybe fiq should only accept edge type interrupts and irq could accept either? > static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type) > { > - if (type != IRQ_TYPE_LEVEL_LOW && type != IRQ_TYPE_LEVEL_HIGH) > - return -EINVAL; > - > - if (type == IRQ_TYPE_LEVEL_LOW) { > + if (type == IRQ_TYPE_EDGE_FALLING) { > + mst_set_irq(data, INTC_REV_POLARITY); > + type = IRQ_TYPE_EDGE_RISING; > + } else if (type == IRQ_TYPE_LEVEL_LOW) { > mst_set_irq(data, INTC_REV_POLARITY); > type = IRQ_TYPE_LEVEL_HIGH; > } I think this still needs the logic to check that type is something we can handle (not IRQ_TYPE_EDGE_BOTH) and maybe if the fiq controller can only do edge interrupts level types should return -EINVAL? Thanks, Daniel
From: Daniel Palmer <daniel@0x0f.com> > On Thu, 11 Mar 2021 at 12:12, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > > For a fiq controller, the input edge signal will be convert to level and > > keep the interrupt status until we do EOI operation. > > That means if a rising edge input if trigger the ouput line will keep high > > until we clear the interrupt status. > > I think maybe the fiq is always edge triggered? > It seems like it latches on an edge and holds it's output to the GIC > high until it is reset by eoi and then only triggers again on another > edge. > I can experiment to confirm that's what it actually does for the chips I have. > > Then it seems like the irq version is almost just a configurable > inverter that passes either the input signal or the inverted input > signal to the GIC. > > So maybe fiq should only accept edge type interrupts and irq could > accept either? Why irq could accept either? And if it's a fiq controller, we can just remove 'mstar,intc-no-eoi' from the device node. Whether the controller is fiq or irq, the interrupt request from mst-intc to parent GIC is level sensitive. Because if the source is edge triggered, mst-intc always latch it. > > > static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type) > > { > > - if (type != IRQ_TYPE_LEVEL_LOW && type != IRQ_TYPE_LEVEL_HIGH) > > - return -EINVAL; > > - > > - if (type == IRQ_TYPE_LEVEL_LOW) { > > + if (type == IRQ_TYPE_EDGE_FALLING) { > > + mst_set_irq(data, INTC_REV_POLARITY); > > + type = IRQ_TYPE_EDGE_RISING; The interrupt triggered type of the parent GIC should be active high, I will modify it. > > + } else if (type == IRQ_TYPE_LEVEL_LOW) { > > mst_set_irq(data, INTC_REV_POLARITY); > > type = IRQ_TYPE_LEVEL_HIGH; > > } > > I think this still needs the logic to check that type is something we > can handle (not IRQ_TYPE_EDGE_BOTH) and maybe if the fiq controller Agree. > can only do edge interrupts level types should return -EINVAL? No matter what triggered type of an irq we write in dts is edge or level triggered, the init and handling flow is the same except we need to do eoi if the controller doesn't have the property 'mstar,intc-no-eoi'. So maybe we don't need to do extra work to check the type for an fiq or irq controller? And I will update the patch as following: diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c index 841b9b1c2699..f46ade7b1775 100644 --- a/drivers/irqchip/irq-mst-intc.c +++ b/drivers/irqchip/irq-mst-intc.c @@ -90,13 +90,14 @@ static void mst_intc_eoi_irq(struct irq_data *d) static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type) { - if (type != IRQ_TYPE_LEVEL_LOW && type != IRQ_TYPE_LEVEL_HIGH) + if (type != IRQ_TYPE_EDGE_RISING && type != IRQ_TYPE_EDGE_FALLING && + type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_LEVEL_LOW) return -EINVAL; - if (type == IRQ_TYPE_LEVEL_LOW) { + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) mst_set_irq(data, INTC_REV_POLARITY); - type = IRQ_TYPE_LEVEL_HIGH; - } + + type = IRQ_TYPE_LEVEL_HIGH; return irq_chip_set_type_parent(data, type); } @@ -219,11 +220,12 @@ static int mst_intc_domain_alloc(struct irq_domain *domain, unsigned int virq, 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 + * mst-intc latch the interrupt request if it's edge triggered, + * so the output signal to parent GIC is always level sensitive. + * And 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; + parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec); }
On Fri, 12 Mar 2021 at 01:11, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > Why irq could accept either? As the irq intc has no way to clear it's triggered state (no eoi) it must just pass the signal through instead of latching it? Otherwise it would latch once and never again right? That's what I really didn't understand. If it just passes the signal through and maybe inverts it then the GIC can use edge or level I think. > So maybe we don't need to do extra work to check the type for an fiq or irq controller? I think without the eoi callback for the fiq it would only ever fire once. I don't think doing the same eoi callback for the irq intc hurts anything but it wouldn't do anything either from what I can tell. > And I will update the patch as following: I think maybe Marc or someone else that knows better than I do should comment on what needs to happen. My input is just that the fiq controller seems to trigger on an edge, holds it's signal to the GIC high until eoi happens and then only triggers again on an edge. I guess it doesn't matter if it's an edge or level if that's how it works but you'd only get one interrupt out of it per edge even if configured as a level interrupt. The main thing I didn't want was filtering out edge interrupts entirely as that breaks using edge interrupts with gpios i.e. using gpiomon. With the changes to set the polarity it can now detect rising or falling edge gpio events. :) Thanks, Daniel
From: Daniel Palmer <daniel@0x0f.com> > On Fri, 12 Mar 2021 at 01:11, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > > Why irq could accept either? > > As the irq intc has no way to clear it's triggered state (no eoi) it > must just pass the signal through instead of latching it? > Otherwise it would latch once and never again right? That's what I > really didn't understand. > If it just passes the signal through and maybe inverts it then the GIC > can use edge or level I think. Yes, but if we accidentally loss a irq and the interrupt is edge triggered which is latch to level by mst-intc, we will miss all the follow irqs because the driver didn't reset the interrupt status. Actually, I'm not sure if it's possible. But even if it's not, I think use level for parent GIC can better match the hardware signal processing. > > > So maybe we don't need to do extra work to check the type for an fiq or irq controller? > > I think without the eoi callback for the fiq it would only ever fire > once. I don't think doing the same eoi callback for the irq intc hurts > anything but it wouldn't do anything either from what I can tell. The reason why I don't do the same eoi callback for irq intc is that it's not ont spec. And some of MTK TV SoC use it for certain debug function which may cause unexpected result. > > > And I will update the patch as following: > > I think maybe Marc or someone else that knows better than I do should > comment on what needs to happen. > My input is just that the fiq controller seems to trigger on an edge, > holds it's signal to the GIC high until eoi happens and then only > triggers again on an edge. > I guess it doesn't matter if it's an edge or level if that's how it > works but you'd only get one interrupt out of it per edge even if > configured as a level interrupt. > > The main thing I didn't want was filtering out edge interrupts > entirely as that breaks using edge interrupts with gpios i.e. using > gpiomon. > With the changes to set the polarity it can now detect rising or > falling edge gpio events. :) Thanks for your feedback and I will send patch v4 which includes the change I proposed in this thread.
diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c index 143657b0cf28..841b9b1c2699 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,19 @@ 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 && type != IRQ_TYPE_LEVEL_HIGH) + return -EINVAL; + + 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 +110,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 +217,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 +273,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 | 90 ++++++++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 3 deletions(-)