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

[PATCH v2] 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 | 90 ++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 3 deletions(-)

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;
 }
 
-- 
2.18.0

[PATCH v2] 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 | 90 ++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 3 deletions(-)

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;
 }
 
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2] 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 | 90 ++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 3 deletions(-)

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;
 }
 
-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Palmer]

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

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

[Daniel Palmer]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Daniel Palmer]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark-PK Tsai]

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.

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

[Mark-PK Tsai]

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.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Mark-PK Tsai]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Palmer]

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

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

[Daniel Palmer]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Daniel Palmer]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark-PK Tsai]

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;
        }

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

[Mark-PK Tsai]

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;
        }
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Mark-PK Tsai]

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;
        }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Palmer]

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

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

[Daniel Palmer]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Daniel Palmer]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark-PK Tsai]

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);
 }

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

[Mark-PK Tsai]

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);
 }
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Mark-PK Tsai]

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);
 }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Daniel Palmer]

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

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

[Daniel Palmer]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Daniel Palmer]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark-PK Tsai]

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.

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

[Mark-PK Tsai]

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.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Mark-PK Tsai]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel