| Message ID | 20190422044539.16085-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: intel: Clear interrupt status in unmask callback | expand |
Hi, at 02:22, <hotwater438@tutanota.com> <hotwater438@tutanota.com> wrote: > Hi. > I've just applied this patch, and touchpad woorks smoothly, but suspend > issue is still present. > After suspend, i2c_hid module bursts i2c_hid i2c-ELAN1200:00: > i2c_hid_get_input: incomplete report (16/65535) messages (more than 50 > reports/sec). > In dmesg I can see a frequency of reporting every 0.0007 - 0.001 dmesg > time units. What’s the default suspend mode on the platform? This is a common issue for system that defaults to Suspend-to-idle, but S3 is in use. The root cause is that the power of the touchpad doesn’t get cut off during S3 by platform firmware. Do you also see this issue if S2I is in use? Kai-Heng > > Though I can sucessfully restart module and after restarting it works as > good as it was. > > So suspend issue is still present. > > Regards, > Vladislav. > > > Apr 22, 2019, 7:45 AM by kai.heng.feng@canonical.com: > Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was > added because clearing interrupt status bit is required to avoid > unexpected behavior. > > Turns out the unmask callback also needs the fix, which can solve weird > IRQ triggering issues on I2C touchpad ELAN1200. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 35 ++++----------------------- > 1 file changed, 5 insertions(+), 30 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c > b/drivers/pinctrl/intel/pinctrl-intel.c > index 3b1818184207..53878604537e 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -913,35 +913,6 @@ static void intel_gpio_irq_ack(struct irq_data *d) > } > } > > -static void intel_gpio_irq_enable(struct irq_data *d) > -{ > - struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > - struct intel_pinctrl *pctrl = gpiochip_get_data(gc); > - const struct intel_community *community; > - const struct intel_padgroup *padgrp; > - int pin; > - > - pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp); > - if (pin >= 0) { > - unsigned int gpp, gpp_offset, is_offset; > - unsigned long flags; > - u32 value; > - > - gpp = padgrp->reg_num; > - gpp_offset = padgroup_offset(padgrp, pin); > - is_offset = community->is_offset + gpp * 4; > - > - raw_spin_lock_irqsave(&pctrl->lock, flags); > - /* Clear interrupt status first to avoid unexpected interrupt */ > - writel(BIT(gpp_offset), community->regs + is_offset); > - > - value = readl(community->regs + community->ie_offset + gpp * 4); > - value |= BIT(gpp_offset); > - writel(value, community->regs + community->ie_offset + gpp * 4); > - raw_spin_unlock_irqrestore(&pctrl->lock, flags); > - } > -} > - > static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > @@ -963,6 +934,11 @@ static void intel_gpio_irq_mask_unmask(struct > irq_data *d, bool mask) > reg = community->regs + community->ie_offset + gpp * 4; > > raw_spin_lock_irqsave(&pctrl->lock, flags); > + > + /* Clear interrupt status first to avoid unexpected interrupt */ > + if (!mask) > + writel(BIT(gpp_offset), community->regs + community->is_offset + > gpp * 4); > + > value = readl(reg); > if (mask) > value &= ~BIT(gpp_offset); > @@ -1106,7 +1082,6 @@ static irqreturn_t intel_gpio_irq(int irq, void > *data) > > static struct irq_chip intel_gpio_irqchip = { > .name = "intel-gpio", > - .irq_enable = intel_gpio_irq_enable, > .irq_ack = intel_gpio_irq_ack, > .irq_mask = intel_gpio_irq_mask, > .irq_unmask = intel_gpio_irq_unmask, > -- > 2.17.1
On Tue, Apr 23, 2019 at 11:00:48AM +0200, hotwater438@tutanota.com wrote: > Hi. > > Honestly, I can't find any information about my acpi suspend type. > There are no options in my BIOS to change it, and I can't determine > which exactly I have. But I suppose I have a S3 because I have suspend > issue. > > Can I somehow determine it in Linux? I did a research but found nothing > on the internet. You can read the default mode from /sys/power/mem_sleep. "s2idle" means, well suspend-to-idle and "deep" means S3.
at 17:40, <hotwater438@tutanota.com> <hotwater438@tutanota.com> wrote: > Hi. Thank's for a hint! > So catting this file gives me next content: > s2idle [deep] Which kernel version do you use? The default switches to s2idle based on FADT flag and _DSM on relative new kernels. Most systems preload with Windows 8.1 should default to use s2idle. > > So I suppose I have s2idle with deep-mode available? > > I've tried to select deep by creating mem_sleep_default file, but I can't > (Permission error). > > Could you explain how do I switch suspend mode in Linux, if you know? # echo s2idle > /sys/power/mem_sleep > And to be honest, I don't think that's a correct fix of touchpad problem. If the system defaults to use S2I, then it’s better to stick with it. Lots of ODM/OEM don’t really test S3. > > Can we somehow cut off the power by hands or send a signal to a system to > shut off a touchpad? Or there are no approaches like that? I don’t think it’s possible. Kai-Heng > > Regards, > Vladislav. > > > Apr 23, 2019, 12:08 PM by mika.westerberg@linux.intel.com: > On Tue, Apr 23, 2019 at 11:00:48AM +0200, hotwater438@tutanota.com wrote: > Hi. > > Honestly, I can't find any information about my acpi suspend type. > There are no options in my BIOS to change it, and I can't determine > which exactly I have. But I suppose I have a S3 because I have suspend > issue. > > Can I somehow determine it in Linux? I did a research but found nothing > on the internet. > > You can read the default mode from /sys/power/mem_sleep. "s2idle" means, > well suspend-to-idle and "deep" means S3.
at 9:49 PM, <hotwater438@tutanota.com> <hotwater438@tutanota.com> wrote: > Hi. I did the suggested echo and, if I clearly understood, switched to > s2idle mode without deep-mode. > For tests I use 4.19.28 on Debian-like system (Parrot OS). > > If the system defaults to use S2I, then it’s better to stick with it. > > Lots of ODM/OEM don’t really test S3. > So S3 is the same as S2 but with deep mode enabled? > The default switches to s2idle based on FADT flag and _DSM on relative > new kernels. > Are those some SSDT flags? > It actually sounds weird to me, why restarting touchpad after deep > suspend affects it work. I mean, if it starts sucessfully at boot, why it > doesn't after suspend? Can we debug it somehow? Can you see if i2c_hid_hwreset() helps? Refer to [1] as an example. Anyway, the resume issue is a different bug. If possible please merge this patch. [1] https://lore.kernel.org/lkml/20190211070040.4569-1-jbroadus@gmail.com/ Kai-Heng > > Regards, > Vladislav. > Apr 23, 2019, 12:47 PM by kai.heng.feng@canonical.com: > at 17:40, <hotwater438@tutanota.com> <hotwater438@tutanota.com> wrote: > Hi. Thank's for a hint! > So catting this file gives me next content: > s2idle [deep] > > Which kernel version do you use? > > > Most systems preload with Windows 8.1 should default to use s2idle. > > So I suppose I have s2idle with deep-mode available? > > I've tried to select deep by creating mem_sleep_default file, but I can't > (Permission error). > > Could you explain how do I switch suspend mode in Linux, if you know? > > # echo s2idle > /sys/power/mem_sleep > And to be honest, I don't think that's a correct fix of touchpad problem. > > > > Can we somehow cut off the power by hands or send a signal to a system to > shut off a touchpad? Or there are no approaches like that? > > I don’t think it’s possible. > > Kai-Heng > > Regards, > Vladislav. > > > Apr 23, 2019, 12:08 PM by mika.westerberg@linux.intel.com: > On Tue, Apr 23, 2019 at 11:00:48AM +0200, hotwater438@tutanota.com wrote: > Hi. > > Honestly, I can't find any information about my acpi suspend type. > There are no options in my BIOS to change it, and I can't determine > which exactly I have. But I suppose I have a S3 because I have suspend > issue. > > Can I somehow determine it in Linux? I did a research but found nothing > on the internet. > > You can read the default mode from /sys/power/mem_sleep. "s2idle" means, > well suspend-to-idle and "deep" means S3.
On Mon, Apr 22, 2019 at 12:45:39PM +0800, Kai-Heng Feng wrote: > Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was > added because clearing interrupt status bit is required to avoid > unexpected behavior. > > Turns out the unmask callback also needs the fix, which can solve weird > IRQ triggering issues on I2C touchpad ELAN1200. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Mon, Apr 22, 2019 at 12:45:39PM +0800, Kai-Heng Feng wrote: > Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was > added because clearing interrupt status bit is required to avoid > unexpected behavior. > > Turns out the unmask callback also needs the fix, which can solve weird > IRQ triggering issues on I2C touchpad ELAN1200. > -static void intel_gpio_irq_enable(struct irq_data *d) > -{ > - struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > - struct intel_pinctrl *pctrl = gpiochip_get_data(gc); > - const struct intel_community *community; > - const struct intel_padgroup *padgrp; > - int pin; > - > - pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp); > - if (pin >= 0) { > - unsigned int gpp, gpp_offset, is_offset; > - unsigned long flags; > - u32 value; > - > - gpp = padgrp->reg_num; > - gpp_offset = padgroup_offset(padgrp, pin); > - is_offset = community->is_offset + gpp * 4; > - > - raw_spin_lock_irqsave(&pctrl->lock, flags); > - /* Clear interrupt status first to avoid unexpected interrupt */ > - writel(BIT(gpp_offset), community->regs + is_offset); > - > - value = readl(community->regs + community->ie_offset + gpp * 4); > - value |= BIT(gpp_offset); > - writel(value, community->regs + community->ie_offset + gpp * 4); > - raw_spin_unlock_irqrestore(&pctrl->lock, flags); > - } > -} > - > static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > @@ -963,6 +934,11 @@ static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask) > reg = community->regs + community->ie_offset + gpp * 4; > > raw_spin_lock_irqsave(&pctrl->lock, flags); > + > + /* Clear interrupt status first to avoid unexpected interrupt */ > + if (!mask) Can we do this unconditionally? > + writel(BIT(gpp_offset), community->regs + community->is_offset + gpp * 4); I would rather prefer to follow the below pattern, like reg = ...; writel(..., reg); or, to decrease calculus under spin lock, something like reg = ->regs + gpp * 4; writel(..., reg + is_offset); readl(reg + ie_offset); etc. > + > value = readl(reg); > if (mask) > value &= ~BIT(gpp_offset); > @@ -1106,7 +1082,6 @@ static irqreturn_t intel_gpio_irq(int irq, void *data) > > static struct irq_chip intel_gpio_irqchip = { > .name = "intel-gpio", > - .irq_enable = intel_gpio_irq_enable, Is it possible scenario when IRQ enable is called, but not masking callbacks? For _AEI or GPE? > .irq_ack = intel_gpio_irq_ack, > .irq_mask = intel_gpio_irq_mask, > .irq_unmask = intel_gpio_irq_unmask,
at 05:47, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Apr 22, 2019 at 12:45:39PM +0800, Kai-Heng Feng wrote: >> Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was >> added because clearing interrupt status bit is required to avoid >> unexpected behavior. >> >> Turns out the unmask callback also needs the fix, which can solve weird >> IRQ triggering issues on I2C touchpad ELAN1200. > >> -static void intel_gpio_irq_enable(struct irq_data *d) >> -{ >> - struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> - struct intel_pinctrl *pctrl = gpiochip_get_data(gc); >> - const struct intel_community *community; >> - const struct intel_padgroup *padgrp; >> - int pin; >> - >> - pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp); >> - if (pin >= 0) { >> - unsigned int gpp, gpp_offset, is_offset; >> - unsigned long flags; >> - u32 value; >> - >> - gpp = padgrp->reg_num; >> - gpp_offset = padgroup_offset(padgrp, pin); >> - is_offset = community->is_offset + gpp * 4; >> - >> - raw_spin_lock_irqsave(&pctrl->lock, flags); >> - /* Clear interrupt status first to avoid unexpected interrupt */ >> - writel(BIT(gpp_offset), community->regs + is_offset); >> - >> - value = readl(community->regs + community->ie_offset + gpp * 4); >> - value |= BIT(gpp_offset); >> - writel(value, community->regs + community->ie_offset + gpp * 4); >> - raw_spin_unlock_irqrestore(&pctrl->lock, flags); >> - } >> -} >> - >> static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask) >> { >> struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> @@ -963,6 +934,11 @@ static void intel_gpio_irq_mask_unmask(struct >> irq_data *d, bool mask) >> reg = community->regs + community->ie_offset + gpp * 4; >> >> raw_spin_lock_irqsave(&pctrl->lock, flags); >> + >> + /* Clear interrupt status first to avoid unexpected interrupt */ > >> + if (!mask) > > Can we do this unconditionally? Yes I think so. > >> + writel(BIT(gpp_offset), community->regs + >> community->is_offset + gpp * 4); > > I would rather prefer to follow the below pattern, like > > reg = ...; > writel(..., reg); > > or, to decrease calculus under spin lock, something like > > reg = ->regs + gpp * 4; > > writel(..., reg + is_offset); > > readl(reg + ie_offset); > > etc. Ok, will do. > >> + >> value = readl(reg); >> if (mask) >> value &= ~BIT(gpp_offset); >> @@ -1106,7 +1082,6 @@ static irqreturn_t intel_gpio_irq(int irq, void >> *data) >> >> static struct irq_chip intel_gpio_irqchip = { >> .name = "intel-gpio", > >> - .irq_enable = intel_gpio_irq_enable, > > Is it possible scenario when IRQ enable is called, but not masking > callbacks? > For _AEI or GPE? I am unfamiliar with both of them, what are the callbacks to be used for _AEI and GPE case? Seems like both gpiolib and irqchip call irq_unmask() when irq_enable() is absent. Kai-Heng > >> .irq_ack = intel_gpio_irq_ack, >> .irq_mask = intel_gpio_irq_mask, >> .irq_unmask = intel_gpio_irq_unmask, > > -- > With Best Regards, > Andy Shevchenko
On Mon, Apr 29, 2019 at 05:16:16PM +0800, Kai-Heng Feng wrote: > at 05:47, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Apr 22, 2019 at 12:45:39PM +0800, Kai-Heng Feng wrote: > > > Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was > > > added because clearing interrupt status bit is required to avoid > > > unexpected behavior. > > > > > > Turns out the unmask callback also needs the fix, which can solve weird > > > IRQ triggering issues on I2C touchpad ELAN1200. > > Is it possible scenario when IRQ enable is called, but not masking > > callbacks? > > For _AEI or GPE? > > I am unfamiliar with both of them, what are the callbacks to be used for > _AEI and GPE case? > Seems like both gpiolib and irqchip call irq_unmask() when irq_enable() is > absent. Yes, that's correct, thank you for double checking. * @irq_enable: enable the interrupt (defaults to chip->unmask if NULL) Wait for v2 with mentioned earlier changes and gathered tags.
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 3b1818184207..53878604537e 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -913,35 +913,6 @@ static void intel_gpio_irq_ack(struct irq_data *d) } } -static void intel_gpio_irq_enable(struct irq_data *d) -{ - struct gpio_chip *gc = irq_data_get_irq_chip_data(d); - struct intel_pinctrl *pctrl = gpiochip_get_data(gc); - const struct intel_community *community; - const struct intel_padgroup *padgrp; - int pin; - - pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp); - if (pin >= 0) { - unsigned int gpp, gpp_offset, is_offset; - unsigned long flags; - u32 value; - - gpp = padgrp->reg_num; - gpp_offset = padgroup_offset(padgrp, pin); - is_offset = community->is_offset + gpp * 4; - - raw_spin_lock_irqsave(&pctrl->lock, flags); - /* Clear interrupt status first to avoid unexpected interrupt */ - writel(BIT(gpp_offset), community->regs + is_offset); - - value = readl(community->regs + community->ie_offset + gpp * 4); - value |= BIT(gpp_offset); - writel(value, community->regs + community->ie_offset + gpp * 4); - raw_spin_unlock_irqrestore(&pctrl->lock, flags); - } -} - static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); @@ -963,6 +934,11 @@ static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask) reg = community->regs + community->ie_offset + gpp * 4; raw_spin_lock_irqsave(&pctrl->lock, flags); + + /* Clear interrupt status first to avoid unexpected interrupt */ + if (!mask) + writel(BIT(gpp_offset), community->regs + community->is_offset + gpp * 4); + value = readl(reg); if (mask) value &= ~BIT(gpp_offset); @@ -1106,7 +1082,6 @@ static irqreturn_t intel_gpio_irq(int irq, void *data) static struct irq_chip intel_gpio_irqchip = { .name = "intel-gpio", - .irq_enable = intel_gpio_irq_enable, .irq_ack = intel_gpio_irq_ack, .irq_mask = intel_gpio_irq_mask, .irq_unmask = intel_gpio_irq_unmask,
Commit a939bb57cd47 ("pinctrl: intel: implement gpio_irq_enable") was added because clearing interrupt status bit is required to avoid unexpected behavior. Turns out the unmask callback also needs the fix, which can solve weird IRQ triggering issues on I2C touchpad ELAN1200. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/pinctrl/intel/pinctrl-intel.c | 35 ++++----------------------- 1 file changed, 5 insertions(+), 30 deletions(-)