* [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt @ 2021-01-21 7:51 Hailong Fan 2021-01-21 8:55 ` Nicolas Boichat 0 siblings, 1 reply; 13+ messages in thread From: Hailong Fan @ 2021-01-21 7:51 UTC (permalink / raw) To: Sean Wang, Linus Walleij, Matthias Brugger Cc: linux-mediatek, linux-gpio, linux-arm-kernel, linux-kernel, youlin.pei, Nicolas Boichat, srv_heupstream, chentsung, gtk_pangao, hanks.chen, yong.wu, Hailong Fan When flipping the polarity will be generated interrupt under certain circumstances, but GPIO external signal has not changed. Then, mask the interrupt before polarity setting, and clear the unexpected interrupt after trigger type setting completed. Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> --- Resend since some server reject. --- drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c index 22736f60c16c..3acda6bb401e 100644 --- a/drivers/pinctrl/mediatek/mtk-eint.c +++ b/drivers/pinctrl/mediatek/mtk-eint.c @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) static int mtk_eint_set_type(struct irq_data *d, unsigned int type) { struct mtk_eint *eint = irq_data_get_irq_chip_data(d); + unsigned int unmask; u32 mask = BIT(d->hwirq & 0x1f); void __iomem *reg; @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) else eint->dual_edge[d->hwirq] = 0; + if (!mtk_eint_get_mask(eint, d->hwirq)) { + mtk_eint_mask(d); + unmask = 1; + } else { + unmask = 0; + } + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); writel(mask, reg); @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) writel(mask, reg); } - if (eint->dual_edge[d->hwirq]) - mtk_eint_flip_edge(eint, d->hwirq); + mtk_eint_ack(d); + if (unmask == 1) + mtk_eint_unmask(d); return 0; } -- 2.18.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt 2021-01-21 7:51 [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt Hailong Fan 2021-01-21 8:55 ` Nicolas Boichat @ 2021-01-21 8:55 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-21 8:55 UTC (permalink / raw) To: Hailong Fan Cc: Sean Wang, Linus Walleij, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list:GPIO SUBSYSTEM, linux-arm Mailing List, lkml, youlin.pei, srv_heupstream, Chen-Tsung Hsieh, gtk_pangao, Hanks Chen, Yong Wu On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > When flipping the polarity will be generated interrupt under certain > circumstances, but GPIO external signal has not changed. > Then, mask the interrupt before polarity setting, and clear the > unexpected interrupt after trigger type setting completed. > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > --- > Resend since some server reject. > --- > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > index 22736f60c16c..3acda6bb401e 100644 > --- a/drivers/pinctrl/mediatek/mtk-eint.c > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > { > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > + unsigned int unmask; bool? > u32 mask = BIT(d->hwirq & 0x1f); > void __iomem *reg; > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > else > eint->dual_edge[d->hwirq] = 0; > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > + mtk_eint_mask(d); > + unmask = 1; > + } else { > + unmask = 0; > + } > + > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > writel(mask, reg); > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > writel(mask, reg); > } > > - if (eint->dual_edge[d->hwirq]) > - mtk_eint_flip_edge(eint, d->hwirq); Why are you dropping this? Aren't we at risk to miss the first edge after mtk_eint_set_type is called? > + mtk_eint_ack(d); > + if (unmask == 1) Just `if (unmask)` > + mtk_eint_unmask(d); > > return 0; > } > -- > 2.18.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt @ 2021-01-21 8:55 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-21 8:55 UTC (permalink / raw) To: Hailong Fan Cc: youlin.pei, Hanks Chen, srv_heupstream, Chen-Tsung Hsieh, Linus Walleij, Sean Wang, lkml, open list:GPIO SUBSYSTEM, moderated list:ARM/Mediatek SoC support, Yong Wu, Matthias Brugger, linux-arm Mailing List, gtk_pangao On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > When flipping the polarity will be generated interrupt under certain > circumstances, but GPIO external signal has not changed. > Then, mask the interrupt before polarity setting, and clear the > unexpected interrupt after trigger type setting completed. > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > --- > Resend since some server reject. > --- > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > index 22736f60c16c..3acda6bb401e 100644 > --- a/drivers/pinctrl/mediatek/mtk-eint.c > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > { > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > + unsigned int unmask; bool? > u32 mask = BIT(d->hwirq & 0x1f); > void __iomem *reg; > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > else > eint->dual_edge[d->hwirq] = 0; > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > + mtk_eint_mask(d); > + unmask = 1; > + } else { > + unmask = 0; > + } > + > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > writel(mask, reg); > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > writel(mask, reg); > } > > - if (eint->dual_edge[d->hwirq]) > - mtk_eint_flip_edge(eint, d->hwirq); Why are you dropping this? Aren't we at risk to miss the first edge after mtk_eint_set_type is called? > + mtk_eint_ack(d); > + if (unmask == 1) Just `if (unmask)` > + mtk_eint_unmask(d); > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt @ 2021-01-21 8:55 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-21 8:55 UTC (permalink / raw) To: Hailong Fan Cc: youlin.pei, Hanks Chen, srv_heupstream, Chen-Tsung Hsieh, Linus Walleij, Sean Wang, lkml, open list:GPIO SUBSYSTEM, moderated list:ARM/Mediatek SoC support, Yong Wu, Matthias Brugger, linux-arm Mailing List, gtk_pangao On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > When flipping the polarity will be generated interrupt under certain > circumstances, but GPIO external signal has not changed. > Then, mask the interrupt before polarity setting, and clear the > unexpected interrupt after trigger type setting completed. > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > --- > Resend since some server reject. > --- > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > index 22736f60c16c..3acda6bb401e 100644 > --- a/drivers/pinctrl/mediatek/mtk-eint.c > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > { > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > + unsigned int unmask; bool? > u32 mask = BIT(d->hwirq & 0x1f); > void __iomem *reg; > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > else > eint->dual_edge[d->hwirq] = 0; > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > + mtk_eint_mask(d); > + unmask = 1; > + } else { > + unmask = 0; > + } > + > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > writel(mask, reg); > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > writel(mask, reg); > } > > - if (eint->dual_edge[d->hwirq]) > - mtk_eint_flip_edge(eint, d->hwirq); Why are you dropping this? Aren't we at risk to miss the first edge after mtk_eint_set_type is called? > + mtk_eint_ack(d); > + if (unmask == 1) Just `if (unmask)` > + mtk_eint_unmask(d); > > return 0; > } > -- > 2.18.0 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt 2021-01-21 8:55 ` Nicolas Boichat (?) (?) @ 2021-01-21 12:09 ` mtk15103 2021-01-21 12:13 ` Nicolas Boichat -1 siblings, 1 reply; 13+ messages in thread From: mtk15103 @ 2021-01-21 12:09 UTC (permalink / raw) To: Nicolas Boichat Cc: Sean Wang, Linus Walleij, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list:GPIO SUBSYSTEM, linux-arm Mailing List, lkml, youlin.pei, srv_heupstream, Chen-Tsung Hsieh, gtk_pangao, Hanks Chen, Yong Wu On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > When flipping the polarity will be generated interrupt under certain > > circumstances, but GPIO external signal has not changed. > > Then, mask the interrupt before polarity setting, and clear the > > unexpected interrupt after trigger type setting completed. > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > --- > > Resend since some server reject. > > --- > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > index 22736f60c16c..3acda6bb401e 100644 > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > { > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > + unsigned int unmask; > > bool? Yes,thanks. > > > u32 mask = BIT(d->hwirq & 0x1f); > > void __iomem *reg; > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > else > > eint->dual_edge[d->hwirq] = 0; > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > + mtk_eint_mask(d); > > + unmask = 1; > > + } else { > > + unmask = 0; > > + } > > + > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > writel(mask, reg); > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > writel(mask, reg); > > } > > > > - if (eint->dual_edge[d->hwirq]) > > - mtk_eint_flip_edge(eint, d->hwirq); > > Why are you dropping this? Aren't we at risk to miss the first edge > after mtk_eint_set_type is called? mtk_eint_unmask() will do it. If unmask != 1, user need to call mtk_eint_unmask() to enable the interrupt before use it, thanks. > > + mtk_eint_ack(d); > > + if (unmask == 1) > > Just `if (unmask)` Yes,thanks. > > + mtk_eint_unmask(d); > > > > return 0; > > } > > -- > > 2.18.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt 2021-01-21 12:09 ` mtk15103 2021-01-21 12:13 ` Nicolas Boichat @ 2021-01-21 12:13 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-21 12:13 UTC (permalink / raw) To: mtk15103 Cc: Sean Wang, Linus Walleij, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list:GPIO SUBSYSTEM, linux-arm Mailing List, lkml, youlin.pei, srv_heupstream, Chen-Tsung Hsieh, gtk_pangao, Hanks Chen, Yong Wu On Thu, Jan 21, 2021 at 8:09 PM mtk15103 <hailong.fan@mediatek.com> wrote: > > On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > > > When flipping the polarity will be generated interrupt under certain > > > circumstances, but GPIO external signal has not changed. > > > Then, mask the interrupt before polarity setting, and clear the > > > unexpected interrupt after trigger type setting completed. > > > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > > --- > > > Resend since some server reject. > > > --- > > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > > index 22736f60c16c..3acda6bb401e 100644 > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > { > > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > > + unsigned int unmask; > > > > bool? > Yes,thanks. > > > > > u32 mask = BIT(d->hwirq & 0x1f); > > > void __iomem *reg; > > > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > else > > > eint->dual_edge[d->hwirq] = 0; > > > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > > + mtk_eint_mask(d); > > > + unmask = 1; > > > + } else { > > > + unmask = 0; > > > + } > > > + > > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > > writel(mask, reg); > > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > writel(mask, reg); > > > } > > > > > > - if (eint->dual_edge[d->hwirq]) > > > - mtk_eint_flip_edge(eint, d->hwirq); > > > > Why are you dropping this? Aren't we at risk to miss the first edge > > after mtk_eint_set_type is called? > mtk_eint_unmask() will do it. > If unmask != 1, user need to call mtk_eint_unmask() to enable the > interrupt before use it, thanks. Makes sense, I just have one more worry: https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/mtk-eint.c#L122 mtk_eint_unmask unmasks the interrupt _before_ the edge is flipped, could this cause a spurious interrupt? On any unmask operation -- not just on mtk_eint_set_type (so this is technically another problem, that should be fixed as a separate patch) > > > + mtk_eint_ack(d); > > > + if (unmask == 1) > > > > Just `if (unmask)` > Yes,thanks. > > > + mtk_eint_unmask(d); > > > > > > return 0; > > > } > > > -- > > > 2.18.0 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt @ 2021-01-21 12:13 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-21 12:13 UTC (permalink / raw) To: mtk15103 Cc: youlin.pei, Hanks Chen, srv_heupstream, Chen-Tsung Hsieh, Linus Walleij, Sean Wang, lkml, open list:GPIO SUBSYSTEM, moderated list:ARM/Mediatek SoC support, Yong Wu, Matthias Brugger, linux-arm Mailing List, gtk_pangao On Thu, Jan 21, 2021 at 8:09 PM mtk15103 <hailong.fan@mediatek.com> wrote: > > On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > > > When flipping the polarity will be generated interrupt under certain > > > circumstances, but GPIO external signal has not changed. > > > Then, mask the interrupt before polarity setting, and clear the > > > unexpected interrupt after trigger type setting completed. > > > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > > --- > > > Resend since some server reject. > > > --- > > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > > index 22736f60c16c..3acda6bb401e 100644 > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > { > > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > > + unsigned int unmask; > > > > bool? > Yes,thanks. > > > > > u32 mask = BIT(d->hwirq & 0x1f); > > > void __iomem *reg; > > > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > else > > > eint->dual_edge[d->hwirq] = 0; > > > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > > + mtk_eint_mask(d); > > > + unmask = 1; > > > + } else { > > > + unmask = 0; > > > + } > > > + > > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > > writel(mask, reg); > > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > writel(mask, reg); > > > } > > > > > > - if (eint->dual_edge[d->hwirq]) > > > - mtk_eint_flip_edge(eint, d->hwirq); > > > > Why are you dropping this? Aren't we at risk to miss the first edge > > after mtk_eint_set_type is called? > mtk_eint_unmask() will do it. > If unmask != 1, user need to call mtk_eint_unmask() to enable the > interrupt before use it, thanks. Makes sense, I just have one more worry: https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/mtk-eint.c#L122 mtk_eint_unmask unmasks the interrupt _before_ the edge is flipped, could this cause a spurious interrupt? On any unmask operation -- not just on mtk_eint_set_type (so this is technically another problem, that should be fixed as a separate patch) > > > + mtk_eint_ack(d); > > > + if (unmask == 1) > > > > Just `if (unmask)` > Yes,thanks. > > > + mtk_eint_unmask(d); > > > > > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt @ 2021-01-21 12:13 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-21 12:13 UTC (permalink / raw) To: mtk15103 Cc: youlin.pei, Hanks Chen, srv_heupstream, Chen-Tsung Hsieh, Linus Walleij, Sean Wang, lkml, open list:GPIO SUBSYSTEM, moderated list:ARM/Mediatek SoC support, Yong Wu, Matthias Brugger, linux-arm Mailing List, gtk_pangao On Thu, Jan 21, 2021 at 8:09 PM mtk15103 <hailong.fan@mediatek.com> wrote: > > On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > > > When flipping the polarity will be generated interrupt under certain > > > circumstances, but GPIO external signal has not changed. > > > Then, mask the interrupt before polarity setting, and clear the > > > unexpected interrupt after trigger type setting completed. > > > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > > --- > > > Resend since some server reject. > > > --- > > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > > index 22736f60c16c..3acda6bb401e 100644 > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > { > > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > > + unsigned int unmask; > > > > bool? > Yes,thanks. > > > > > u32 mask = BIT(d->hwirq & 0x1f); > > > void __iomem *reg; > > > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > else > > > eint->dual_edge[d->hwirq] = 0; > > > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > > + mtk_eint_mask(d); > > > + unmask = 1; > > > + } else { > > > + unmask = 0; > > > + } > > > + > > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > > writel(mask, reg); > > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > writel(mask, reg); > > > } > > > > > > - if (eint->dual_edge[d->hwirq]) > > > - mtk_eint_flip_edge(eint, d->hwirq); > > > > Why are you dropping this? Aren't we at risk to miss the first edge > > after mtk_eint_set_type is called? > mtk_eint_unmask() will do it. > If unmask != 1, user need to call mtk_eint_unmask() to enable the > interrupt before use it, thanks. Makes sense, I just have one more worry: https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/mtk-eint.c#L122 mtk_eint_unmask unmasks the interrupt _before_ the edge is flipped, could this cause a spurious interrupt? On any unmask operation -- not just on mtk_eint_set_type (so this is technically another problem, that should be fixed as a separate patch) > > > + mtk_eint_ack(d); > > > + if (unmask == 1) > > > > Just `if (unmask)` > Yes,thanks. > > > + mtk_eint_unmask(d); > > > > > > return 0; > > > } > > > -- > > > 2.18.0 > _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt 2021-01-21 12:13 ` Nicolas Boichat (?) (?) @ 2021-01-22 2:43 ` mtk15103 2021-01-22 10:54 ` Nicolas Boichat -1 siblings, 1 reply; 13+ messages in thread From: mtk15103 @ 2021-01-22 2:43 UTC (permalink / raw) To: Nicolas Boichat Cc: Sean Wang, Linus Walleij, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list:GPIO SUBSYSTEM, linux-arm Mailing List, lkml, youlin.pei, srv_heupstream, Chen-Tsung Hsieh, gtk_pangao, Hanks Chen, Yong Wu, zhengnan.chen On Thu, 2021-01-21 at 20:13 +0800, Nicolas Boichat wrote: > On Thu, Jan 21, 2021 at 8:09 PM mtk15103 <hailong.fan@mediatek.com> wrote: > > > > On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > > > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > > > > > When flipping the polarity will be generated interrupt under certain > > > > circumstances, but GPIO external signal has not changed. > > > > Then, mask the interrupt before polarity setting, and clear the > > > > unexpected interrupt after trigger type setting completed. > > > > > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > > > --- > > > > Resend since some server reject. > > > > --- > > > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > > > index 22736f60c16c..3acda6bb401e 100644 > > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > { > > > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > > > + unsigned int unmask; > > > > > > bool? > > Yes,thanks. > > > > > > > u32 mask = BIT(d->hwirq & 0x1f); > > > > void __iomem *reg; > > > > > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > else > > > > eint->dual_edge[d->hwirq] = 0; > > > > > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > > > + mtk_eint_mask(d); > > > > + unmask = 1; > > > > + } else { > > > > + unmask = 0; > > > > + } > > > > + > > > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > > > writel(mask, reg); > > > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > writel(mask, reg); > > > > } > > > > > > > > - if (eint->dual_edge[d->hwirq]) > > > > - mtk_eint_flip_edge(eint, d->hwirq); > > > > > > Why are you dropping this? Aren't we at risk to miss the first edge > > > after mtk_eint_set_type is called? > > mtk_eint_unmask() will do it. > > If unmask != 1, user need to call mtk_eint_unmask() to enable the > > interrupt before use it, thanks. > > Makes sense, I just have one more worry: > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/mtk-eint.c#L122 > > mtk_eint_unmask unmasks the interrupt _before_ the edge is flipped, > could this cause a spurious interrupt? On any unmask operation -- not > just on mtk_eint_set_type (so this is technically another problem, > that should be fixed as a separate patch) This situation will not happen. Spurious interrupt occur condition: edge irq polarity value is same with input signal. e.g. Default value: GPIO input is high, trigger type is falling_edge. User want modify trigger type is rising_edge under some certain circumstances, it will generate spurious interrupt but external signal maintain high. So we need ack interrupt after trigger_type setting is completed. mtk_eint_flip_edge is for dual_edge, the polarity setting based on current gpio input signal: if (input == high) polarity = low; /* falling_edge */ else polarity = high; /* rising_edge */ Then it's not cause a spurious interrupt. > > > > + mtk_eint_ack(d); > > > > + if (unmask == 1) > > > > > > Just `if (unmask)` > > Yes,thanks. > > > > + mtk_eint_unmask(d); > > > > > > > > return 0; > > > > } > > > > -- > > > > 2.18.0 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt 2021-01-22 2:43 ` mtk15103 2021-01-22 10:54 ` Nicolas Boichat @ 2021-01-22 10:54 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-22 10:54 UTC (permalink / raw) To: mtk15103 Cc: Sean Wang, Linus Walleij, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list:GPIO SUBSYSTEM, linux-arm Mailing List, lkml, youlin.pei, srv_heupstream, Chen-Tsung Hsieh, gtk_pangao, Hanks Chen, Yong Wu, zhengnan.chen On Fri, Jan 22, 2021 at 10:44 AM mtk15103 <hailong.fan@mediatek.com> wrote: > > On Thu, 2021-01-21 at 20:13 +0800, Nicolas Boichat wrote: > > On Thu, Jan 21, 2021 at 8:09 PM mtk15103 <hailong.fan@mediatek.com> wrote: > > > > > > On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > > > > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > > > > > > > When flipping the polarity will be generated interrupt under certain > > > > > circumstances, but GPIO external signal has not changed. > > > > > Then, mask the interrupt before polarity setting, and clear the > > > > > unexpected interrupt after trigger type setting completed. > > > > > > > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > > > > --- > > > > > Resend since some server reject. > > > > > --- > > > > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > > > > index 22736f60c16c..3acda6bb401e 100644 > > > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > > > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > > > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > { > > > > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > > > > + unsigned int unmask; > > > > > > > > bool? > > > Yes,thanks. > > > > > > > > > u32 mask = BIT(d->hwirq & 0x1f); > > > > > void __iomem *reg; > > > > > > > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > else > > > > > eint->dual_edge[d->hwirq] = 0; > > > > > > > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > > > > + mtk_eint_mask(d); > > > > > + unmask = 1; > > > > > + } else { > > > > > + unmask = 0; > > > > > + } > > > > > + > > > > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > > > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > > > > writel(mask, reg); > > > > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > writel(mask, reg); > > > > > } > > > > > > > > > > - if (eint->dual_edge[d->hwirq]) > > > > > - mtk_eint_flip_edge(eint, d->hwirq); > > > > > > > > Why are you dropping this? Aren't we at risk to miss the first edge > > > > after mtk_eint_set_type is called? > > > mtk_eint_unmask() will do it. > > > If unmask != 1, user need to call mtk_eint_unmask() to enable the > > > interrupt before use it, thanks. > > > > Makes sense, I just have one more worry: > > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/mtk-eint.c#L122 > > > > mtk_eint_unmask unmasks the interrupt _before_ the edge is flipped, > > could this cause a spurious interrupt? On any unmask operation -- not > > just on mtk_eint_set_type (so this is technically another problem, > > that should be fixed as a separate patch) > > This situation will not happen. > > Spurious interrupt occur condition: edge irq polarity value is same with > input signal. > e.g. > Default value: GPIO input is high, trigger type is falling_edge. > User want modify trigger type is rising_edge under some certain > circumstances, it will generate spurious interrupt but external signal > maintain high. > So we need ack interrupt after trigger_type setting is completed. > > mtk_eint_flip_edge is for dual_edge, the polarity setting based on > current gpio input signal: > if (input == high) > polarity = low; /* falling_edge */ > else > polarity = high; /* rising_edge */ > Then it's not cause a spurious interrupt. Okay let me try to make sure I follow: - Say interrupt is currently IRQ_TYPE_EDGE_FALLING (and GPIO is high and stable) - mtk_eint_set_type(d, IRQ_TYPE_EDGE_BOTH) - mtk_eint_mask(d) - (no mask changed, just eint->dual_edge) - mtk_eint_ack(d) (not actually needed in this example) - mtk_eint_unmask(d) - HW reg unmask (can't generate an interrupt as anything that would have happened is acked already -- even with different previous/new types than my example) - flip_edge (will swap polarity as needed but can't generate interrupt as it won't set the polarity to be the same as the current GPIO state) Okay, I think I'm convinced. Please fix the nits and then you can add Reviewed-by: Nicolas Boichat <drinkcat@chromium.org> > > > > > + mtk_eint_ack(d); > > > > > + if (unmask == 1) > > > > > > > > Just `if (unmask)` > > > Yes,thanks. > > > > > + mtk_eint_unmask(d); > > > > > > > > > > return 0; > > > > > } > > > > > -- > > > > > 2.18.0 > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt @ 2021-01-22 10:54 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-22 10:54 UTC (permalink / raw) To: mtk15103 Cc: youlin.pei, Hanks Chen, srv_heupstream, Chen-Tsung Hsieh, Linus Walleij, Sean Wang, lkml, zhengnan.chen, open list:GPIO SUBSYSTEM, moderated list:ARM/Mediatek SoC support, Yong Wu, Matthias Brugger, linux-arm Mailing List, gtk_pangao On Fri, Jan 22, 2021 at 10:44 AM mtk15103 <hailong.fan@mediatek.com> wrote: > > On Thu, 2021-01-21 at 20:13 +0800, Nicolas Boichat wrote: > > On Thu, Jan 21, 2021 at 8:09 PM mtk15103 <hailong.fan@mediatek.com> wrote: > > > > > > On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > > > > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > > > > > > > When flipping the polarity will be generated interrupt under certain > > > > > circumstances, but GPIO external signal has not changed. > > > > > Then, mask the interrupt before polarity setting, and clear the > > > > > unexpected interrupt after trigger type setting completed. > > > > > > > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > > > > --- > > > > > Resend since some server reject. > > > > > --- > > > > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > > > > index 22736f60c16c..3acda6bb401e 100644 > > > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > > > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > > > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > { > > > > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > > > > + unsigned int unmask; > > > > > > > > bool? > > > Yes,thanks. > > > > > > > > > u32 mask = BIT(d->hwirq & 0x1f); > > > > > void __iomem *reg; > > > > > > > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > else > > > > > eint->dual_edge[d->hwirq] = 0; > > > > > > > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > > > > + mtk_eint_mask(d); > > > > > + unmask = 1; > > > > > + } else { > > > > > + unmask = 0; > > > > > + } > > > > > + > > > > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > > > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > > > > writel(mask, reg); > > > > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > writel(mask, reg); > > > > > } > > > > > > > > > > - if (eint->dual_edge[d->hwirq]) > > > > > - mtk_eint_flip_edge(eint, d->hwirq); > > > > > > > > Why are you dropping this? Aren't we at risk to miss the first edge > > > > after mtk_eint_set_type is called? > > > mtk_eint_unmask() will do it. > > > If unmask != 1, user need to call mtk_eint_unmask() to enable the > > > interrupt before use it, thanks. > > > > Makes sense, I just have one more worry: > > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/mtk-eint.c#L122 > > > > mtk_eint_unmask unmasks the interrupt _before_ the edge is flipped, > > could this cause a spurious interrupt? On any unmask operation -- not > > just on mtk_eint_set_type (so this is technically another problem, > > that should be fixed as a separate patch) > > This situation will not happen. > > Spurious interrupt occur condition: edge irq polarity value is same with > input signal. > e.g. > Default value: GPIO input is high, trigger type is falling_edge. > User want modify trigger type is rising_edge under some certain > circumstances, it will generate spurious interrupt but external signal > maintain high. > So we need ack interrupt after trigger_type setting is completed. > > mtk_eint_flip_edge is for dual_edge, the polarity setting based on > current gpio input signal: > if (input == high) > polarity = low; /* falling_edge */ > else > polarity = high; /* rising_edge */ > Then it's not cause a spurious interrupt. Okay let me try to make sure I follow: - Say interrupt is currently IRQ_TYPE_EDGE_FALLING (and GPIO is high and stable) - mtk_eint_set_type(d, IRQ_TYPE_EDGE_BOTH) - mtk_eint_mask(d) - (no mask changed, just eint->dual_edge) - mtk_eint_ack(d) (not actually needed in this example) - mtk_eint_unmask(d) - HW reg unmask (can't generate an interrupt as anything that would have happened is acked already -- even with different previous/new types than my example) - flip_edge (will swap polarity as needed but can't generate interrupt as it won't set the polarity to be the same as the current GPIO state) Okay, I think I'm convinced. Please fix the nits and then you can add Reviewed-by: Nicolas Boichat <drinkcat@chromium.org> > > > > > + mtk_eint_ack(d); > > > > > + if (unmask == 1) > > > > > > > > Just `if (unmask)` > > > Yes,thanks. > > > > > + mtk_eint_unmask(d); > > > > > > > > > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt @ 2021-01-22 10:54 ` Nicolas Boichat 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Boichat @ 2021-01-22 10:54 UTC (permalink / raw) To: mtk15103 Cc: youlin.pei, Hanks Chen, srv_heupstream, Chen-Tsung Hsieh, Linus Walleij, Sean Wang, lkml, zhengnan.chen, open list:GPIO SUBSYSTEM, moderated list:ARM/Mediatek SoC support, Yong Wu, Matthias Brugger, linux-arm Mailing List, gtk_pangao On Fri, Jan 22, 2021 at 10:44 AM mtk15103 <hailong.fan@mediatek.com> wrote: > > On Thu, 2021-01-21 at 20:13 +0800, Nicolas Boichat wrote: > > On Thu, Jan 21, 2021 at 8:09 PM mtk15103 <hailong.fan@mediatek.com> wrote: > > > > > > On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > > > > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > > > > > > > When flipping the polarity will be generated interrupt under certain > > > > > circumstances, but GPIO external signal has not changed. > > > > > Then, mask the interrupt before polarity setting, and clear the > > > > > unexpected interrupt after trigger type setting completed. > > > > > > > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > > > > --- > > > > > Resend since some server reject. > > > > > --- > > > > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > > > > index 22736f60c16c..3acda6bb401e 100644 > > > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > > > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > > > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > { > > > > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > > > > + unsigned int unmask; > > > > > > > > bool? > > > Yes,thanks. > > > > > > > > > u32 mask = BIT(d->hwirq & 0x1f); > > > > > void __iomem *reg; > > > > > > > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > else > > > > > eint->dual_edge[d->hwirq] = 0; > > > > > > > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > > > > + mtk_eint_mask(d); > > > > > + unmask = 1; > > > > > + } else { > > > > > + unmask = 0; > > > > > + } > > > > > + > > > > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > > > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > > > > writel(mask, reg); > > > > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > writel(mask, reg); > > > > > } > > > > > > > > > > - if (eint->dual_edge[d->hwirq]) > > > > > - mtk_eint_flip_edge(eint, d->hwirq); > > > > > > > > Why are you dropping this? Aren't we at risk to miss the first edge > > > > after mtk_eint_set_type is called? > > > mtk_eint_unmask() will do it. > > > If unmask != 1, user need to call mtk_eint_unmask() to enable the > > > interrupt before use it, thanks. > > > > Makes sense, I just have one more worry: > > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/mtk-eint.c#L122 > > > > mtk_eint_unmask unmasks the interrupt _before_ the edge is flipped, > > could this cause a spurious interrupt? On any unmask operation -- not > > just on mtk_eint_set_type (so this is technically another problem, > > that should be fixed as a separate patch) > > This situation will not happen. > > Spurious interrupt occur condition: edge irq polarity value is same with > input signal. > e.g. > Default value: GPIO input is high, trigger type is falling_edge. > User want modify trigger type is rising_edge under some certain > circumstances, it will generate spurious interrupt but external signal > maintain high. > So we need ack interrupt after trigger_type setting is completed. > > mtk_eint_flip_edge is for dual_edge, the polarity setting based on > current gpio input signal: > if (input == high) > polarity = low; /* falling_edge */ > else > polarity = high; /* rising_edge */ > Then it's not cause a spurious interrupt. Okay let me try to make sure I follow: - Say interrupt is currently IRQ_TYPE_EDGE_FALLING (and GPIO is high and stable) - mtk_eint_set_type(d, IRQ_TYPE_EDGE_BOTH) - mtk_eint_mask(d) - (no mask changed, just eint->dual_edge) - mtk_eint_ack(d) (not actually needed in this example) - mtk_eint_unmask(d) - HW reg unmask (can't generate an interrupt as anything that would have happened is acked already -- even with different previous/new types than my example) - flip_edge (will swap polarity as needed but can't generate interrupt as it won't set the polarity to be the same as the current GPIO state) Okay, I think I'm convinced. Please fix the nits and then you can add Reviewed-by: Nicolas Boichat <drinkcat@chromium.org> > > > > > + mtk_eint_ack(d); > > > > > + if (unmask == 1) > > > > > > > > Just `if (unmask)` > > > Yes,thanks. > > > > > + mtk_eint_unmask(d); > > > > > > > > > > return 0; > > > > > } > > > > > -- > > > > > 2.18.0 > > > > _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt 2021-01-22 10:54 ` Nicolas Boichat (?) (?) @ 2021-01-25 3:16 ` mtk15103 -1 siblings, 0 replies; 13+ messages in thread From: mtk15103 @ 2021-01-25 3:16 UTC (permalink / raw) To: Nicolas Boichat Cc: Sean Wang, Linus Walleij, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list:GPIO SUBSYSTEM, linux-arm Mailing List, lkml, youlin.pei, srv_heupstream, Chen-Tsung Hsieh, gtk_pangao, Hanks Chen, Yong Wu, zhengnan.chen, hailong.fan On Fri, 2021-01-22 at 18:54 +0800, Nicolas Boichat wrote: > On Fri, Jan 22, 2021 at 10:44 AM mtk15103 <hailong.fan@mediatek.com> wrote: > > > > On Thu, 2021-01-21 at 20:13 +0800, Nicolas Boichat wrote: > > > On Thu, Jan 21, 2021 at 8:09 PM mtk15103 <hailong.fan@mediatek.com> wrote: > > > > > > > > On Thu, 2021-01-21 at 16:55 +0800, Nicolas Boichat wrote: > > > > > On Thu, Jan 21, 2021 at 3:52 PM Hailong Fan <hailong.fan@mediatek.com> wrote: > > > > > > > > > > > > When flipping the polarity will be generated interrupt under certain > > > > > > circumstances, but GPIO external signal has not changed. > > > > > > Then, mask the interrupt before polarity setting, and clear the > > > > > > unexpected interrupt after trigger type setting completed. > > > > > > > > > > > > Signed-off-by: Hailong Fan <hailong.fan@mediatek.com> > > > > > > --- > > > > > > Resend since some server reject. > > > > > > --- > > > > > > drivers/pinctrl/mediatek/mtk-eint.c | 13 +++++++++++-- > > > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c > > > > > > index 22736f60c16c..3acda6bb401e 100644 > > > > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c > > > > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c > > > > > > @@ -157,6 +157,7 @@ static void mtk_eint_ack(struct irq_data *d) > > > > > > static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > > { > > > > > > struct mtk_eint *eint = irq_data_get_irq_chip_data(d); > > > > > > + unsigned int unmask; > > > > > > > > > > bool? > > > > Yes,thanks. > > > > > > > > > > > u32 mask = BIT(d->hwirq & 0x1f); > > > > > > void __iomem *reg; > > > > > > > > > > > > @@ -173,6 +174,13 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > > else > > > > > > eint->dual_edge[d->hwirq] = 0; > > > > > > > > > > > > + if (!mtk_eint_get_mask(eint, d->hwirq)) { > > > > > > + mtk_eint_mask(d); > > > > > > + unmask = 1; > > > > > > + } else { > > > > > > + unmask = 0; > > > > > > + } > > > > > > + > > > > > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) { > > > > > > reg = mtk_eint_get_offset(eint, d->hwirq, eint->regs->pol_clr); > > > > > > writel(mask, reg); > > > > > > @@ -189,8 +197,9 @@ static int mtk_eint_set_type(struct irq_data *d, unsigned int type) > > > > > > writel(mask, reg); > > > > > > } > > > > > > > > > > > > - if (eint->dual_edge[d->hwirq]) > > > > > > - mtk_eint_flip_edge(eint, d->hwirq); > > > > > > > > > > Why are you dropping this? Aren't we at risk to miss the first edge > > > > > after mtk_eint_set_type is called? > > > > mtk_eint_unmask() will do it. > > > > If unmask != 1, user need to call mtk_eint_unmask() to enable the > > > > interrupt before use it, thanks. > > > > > > Makes sense, I just have one more worry: > > > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/mtk-eint.c#L122 > > > > > > mtk_eint_unmask unmasks the interrupt _before_ the edge is flipped, > > > could this cause a spurious interrupt? On any unmask operation -- not > > > just on mtk_eint_set_type (so this is technically another problem, > > > that should be fixed as a separate patch) > > > > This situation will not happen. > > > > Spurious interrupt occur condition: edge irq polarity value is same with > > input signal. > > e.g. > > Default value: GPIO input is high, trigger type is falling_edge. > > User want modify trigger type is rising_edge under some certain > > circumstances, it will generate spurious interrupt but external signal > > maintain high. > > So we need ack interrupt after trigger_type setting is completed. > > > > mtk_eint_flip_edge is for dual_edge, the polarity setting based on > > current gpio input signal: > > if (input == high) > > polarity = low; /* falling_edge */ > > else > > polarity = high; /* rising_edge */ > > Then it's not cause a spurious interrupt. > > Okay let me try to make sure I follow: > > - Say interrupt is currently IRQ_TYPE_EDGE_FALLING (and GPIO is high and stable) > - mtk_eint_set_type(d, IRQ_TYPE_EDGE_BOTH) > - mtk_eint_mask(d) > - (no mask changed, just eint->dual_edge) > - mtk_eint_ack(d) (not actually needed in this example) > - mtk_eint_unmask(d) > - HW reg unmask (can't generate an interrupt as anything that would > have happened is acked already -- even with different previous/new > types than my example) > - flip_edge (will swap polarity as needed but can't generate > interrupt as it won't set the polarity to be the same as the current > GPIO state) > > Okay, I think I'm convinced. > > Please fix the nits and then you can add Thanks a lot for your comments. V2 patch has been sent to you, please help review it, thx. > > Reviewed-by: Nicolas Boichat <drinkcat@chromium.org> > > > > > > > + mtk_eint_ack(d); > > > > > > + if (unmask == 1) > > > > > > > > > > Just `if (unmask)` > > > > Yes,thanks. > > > > > > + mtk_eint_unmask(d); > > > > > > > > > > > > return 0; > > > > > > } > > > > > > -- > > > > > > 2.18.0 > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-01-25 3:18 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-21 7:51 [PATCH RESEND] pinctrl: mediatek: Fix trigger type setting follow for unexpected interrupt Hailong Fan 2021-01-21 8:55 ` Nicolas Boichat 2021-01-21 8:55 ` Nicolas Boichat 2021-01-21 8:55 ` Nicolas Boichat 2021-01-21 12:09 ` mtk15103 2021-01-21 12:13 ` Nicolas Boichat 2021-01-21 12:13 ` Nicolas Boichat 2021-01-21 12:13 ` Nicolas Boichat 2021-01-22 2:43 ` mtk15103 2021-01-22 10:54 ` Nicolas Boichat 2021-01-22 10:54 ` Nicolas Boichat 2021-01-22 10:54 ` Nicolas Boichat 2021-01-25 3:16 ` mtk15103
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.