[PATCH v1 1/2] pinctrl: baytrail: Do not add all GPIOs to IRQ domain

[PATCH v1 1/2] pinctrl: baytrail: Do not add all GPIOs to IRQ domain

[Andy Shevchenko]

It turns out that for some GPIO pins interrupts are bypassing standard
chain.

Now the reason why some events such as touchscreen communication on ASuS
T100TA does not work if we mask all the interrupts is that in order to
generate either interrupts or GPEs the INTMASK register must have that
particular interrupt unmasked. In case of GPEs the CPU does not trigger
normal interrupt (and thus the GPIO driver does not see it) but instead
it causes SCI (System Control Interrupt) to be triggered with the GPE in
question set.

To make this all work as expected we add those GPIOs to the IRQ
domain that can actually generate interrupts and skip others.

Fixes: 3ae02c14d964 ("pinctrl: intel: set default handler to be handle_bad_irq()")
Reported-by: Robert R. Howell <rhowell@uwyo.edu>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 67e92699b84e..c123488266ce 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1623,6 +1623,8 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
 
 static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
 {
+	struct gpio_chip *gc = &vg->chip;
+	struct device *dev = &vg->pdev->dev;
 	void __iomem *reg;
 	u32 base, value;
 	int i;
@@ -1644,10 +1646,12 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
 		}
 
 		value = readl(reg);
-		if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i) &&
-		    !(value & BYT_DIRECT_IRQ_EN)) {
+		if (value & BYT_DIRECT_IRQ_EN) {
+			clear_bit(i, gc->irq_valid_mask);
+			dev_dbg(dev, "excluding GPIO %d from IRQ domain\n", i);
+		} else if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i)) {
 			byt_gpio_clear_triggering(vg, i);
-			dev_dbg(&vg->pdev->dev, "disabling GPIO %d\n", i);
+			dev_dbg(dev, "disabling GPIO %d\n", i);
 		}
 	}
 
@@ -1686,6 +1690,7 @@ static int byt_gpio_probe(struct byt_gpio *vg)
 	gc->can_sleep	= false;
 	gc->parent	= &vg->pdev->dev;
 	gc->ngpio	= vg->soc_data->npins;
+	gc->irq_need_valid_mask	= true;
 
 #ifdef CONFIG_PM_SLEEP
 	vg->saved_context = devm_kcalloc(&vg->pdev->dev, gc->ngpio,
-- 
2.11.0

[PATCH v1 2/2] pinctrl: baytrail: Convert to use devm_*()

[Andy Shevchenko]

This simplifies error handling and allows us to drop error path handlers
completely.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index c123488266ce..e696a01365cb 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1696,7 +1696,7 @@ static int byt_gpio_probe(struct byt_gpio *vg)
 	vg->saved_context = devm_kcalloc(&vg->pdev->dev, gc->ngpio,
 				       sizeof(*vg->saved_context), GFP_KERNEL);
 #endif
-	ret = gpiochip_add_data(gc, vg);
+	ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg);
 	if (ret) {
 		dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n");
 		return ret;
@@ -1706,7 +1706,7 @@ static int byt_gpio_probe(struct byt_gpio *vg)
 				     0, 0, vg->soc_data->npins);
 	if (ret) {
 		dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n");
-		goto fail;
+		return ret;
 	}
 
 	/* set up interrupts  */
@@ -1717,7 +1717,7 @@ static int byt_gpio_probe(struct byt_gpio *vg)
 					   handle_bad_irq, IRQ_TYPE_NONE);
 		if (ret) {
 			dev_err(&vg->pdev->dev, "failed to add irqchip\n");
-			goto fail;
+			return ret;
 		}
 
 		gpiochip_set_chained_irqchip(gc, &byt_irqchip,
@@ -1726,11 +1726,6 @@ static int byt_gpio_probe(struct byt_gpio *vg)
 	}
 
 	return ret;
-
-fail:
-	gpiochip_remove(&vg->chip);
-
-	return ret;
 }
 
 static int byt_set_soc_data(struct byt_gpio *vg,
@@ -1813,7 +1808,7 @@ static int byt_pinctrl_probe(struct platform_device *pdev)
 	vg->pctl_desc.pins	= vg->soc_data->pins;
 	vg->pctl_desc.npins	= vg->soc_data->npins;
 
-	vg->pctl_dev = pinctrl_register(&vg->pctl_desc, &pdev->dev, vg);
+	vg->pctl_dev = devm_pinctrl_register(&pdev->dev, &vg->pctl_desc, vg);
 	if (IS_ERR(vg->pctl_dev)) {
 		dev_err(&pdev->dev, "failed to register pinctrl driver\n");
 		return PTR_ERR(vg->pctl_dev);
@@ -1822,10 +1817,8 @@ static int byt_pinctrl_probe(struct platform_device *pdev)
 	raw_spin_lock_init(&vg->lock);
 
 	ret = byt_gpio_probe(vg);
-	if (ret) {
-		pinctrl_unregister(vg->pctl_dev);
+	if (ret)
 		return ret;
-	}
 
 	platform_set_drvdata(pdev, vg);
 	pm_runtime_enable(&pdev->dev);
-- 
2.11.0

Re: [PATCH v1 1/2] pinctrl: baytrail: Do not add all GPIOs to IRQ domain

[Andy Shevchenko]

On Tue, 2017-01-10 at 22:11 +0200, Andy Shevchenko wrote:
> It turns out that for some GPIO pins interrupts are bypassing standard
> chain.
> 
> Now the reason why some events such as touchscreen communication on
> ASuS
> T100TA does not work if we mask all the interrupts is that in order to
> generate either interrupts or GPEs the INTMASK register must have that
> particular interrupt unmasked. In case of GPEs the CPU does not
> trigger
> normal interrupt (and thus the GPIO driver does not see it) but
> instead
> it causes SCI (System Control Interrupt) to be triggered with the GPE
> in
> question set.
> 
> To make this all work as expected we add those GPIOs to the IRQ
> domain that can actually generate interrupts and skip others.
> 

Suggested-by: Mika.

Mika, please check if I wrote description clearly.

> Fixes: 3ae02c14d964 ("pinctrl: intel: set default handler to be
> handle_bad_irq()")
> Reported-by: Robert R. Howell <rhowell@uwyo.edu>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-baytrail.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c
> b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index 67e92699b84e..c123488266ce 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1623,6 +1623,8 @@ static void byt_gpio_irq_handler(struct irq_desc
> *desc)
>  
>  static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
>  {
> +	struct gpio_chip *gc = &vg->chip;
> +	struct device *dev = &vg->pdev->dev;
>  	void __iomem *reg;
>  	u32 base, value;
>  	int i;
> @@ -1644,10 +1646,12 @@ static void byt_gpio_irq_init_hw(struct
> byt_gpio *vg)
>  		}
>  
>  		value = readl(reg);
> -		if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i)
> &&
> -		    !(value & BYT_DIRECT_IRQ_EN)) {
> +		if (value & BYT_DIRECT_IRQ_EN) {
> +			clear_bit(i, gc->irq_valid_mask);
> +			dev_dbg(dev, "excluding GPIO %d from IRQ
> domain\n", i);
> +		} else if ((value & BYT_PIN_MUX) ==
> byt_get_gpio_mux(vg, i)) {
>  			byt_gpio_clear_triggering(vg, i);
> -			dev_dbg(&vg->pdev->dev, "disabling GPIO
> %d\n", i);
> +			dev_dbg(dev, "disabling GPIO %d\n", i);
>  		}
>  	}
>  
> @@ -1686,6 +1690,7 @@ static int byt_gpio_probe(struct byt_gpio *vg)
>  	gc->can_sleep	= false;
>  	gc->parent	= &vg->pdev->dev;
>  	gc->ngpio	= vg->soc_data->npins;
> +	gc->irq_need_valid_mask	= true;
>  
>  #ifdef CONFIG_PM_SLEEP
>  	vg->saved_context = devm_kcalloc(&vg->pdev->dev, gc->ngpio,

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/2] pinctrl: baytrail: Do not add all GPIOs to IRQ domain

[Mika Westerberg]

On Tue, Jan 10, 2017 at 10:11:38PM +0200, Andy Shevchenko wrote:
> It turns out that for some GPIO pins interrupts are bypassing standard
> chain.
> 
> Now the reason why some events such as touchscreen communication on ASuS
> T100TA does not work if we mask all the interrupts is that in order to
> generate either interrupts or GPEs the INTMASK register must have that
> particular interrupt unmasked. In case of GPEs the CPU does not trigger
> normal interrupt (and thus the GPIO driver does not see it) but instead
> it causes SCI (System Control Interrupt) to be triggered with the GPE in
> question set.
> 
> To make this all work as expected we add those GPIOs to the IRQ
> domain that can actually generate interrupts and skip others.

Actually what happens is that when DIRECT_IRQ_EN is set, the pin is
routed directly to the IO-APIC bypassing the GPIO driver completely.
However, the mask register is still used to determine if the pin is
supposed to generate IRQ or not.

So with commit 3ae02c14d964 the IRQ core masks all IRQs (because of
handle_bad_irq()) the pin connected to the touchscreen gets masked as
well and hence no interrupts.

This has nothing to do with GPEs, though.

The fix itself looks good to me.

> Fixes: 3ae02c14d964 ("pinctrl: intel: set default handler to be handle_bad_irq()")
> Reported-by: Robert R. Howell <rhowell@uwyo.edu>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-baytrail.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index 67e92699b84e..c123488266ce 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1623,6 +1623,8 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
>  
>  static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
>  {
> +	struct gpio_chip *gc = &vg->chip;
> +	struct device *dev = &vg->pdev->dev;
>  	void __iomem *reg;
>  	u32 base, value;
>  	int i;
> @@ -1644,10 +1646,12 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
>  		}
>  
>  		value = readl(reg);
> -		if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i) &&
> -		    !(value & BYT_DIRECT_IRQ_EN)) {
> +		if (value & BYT_DIRECT_IRQ_EN) {
> +			clear_bit(i, gc->irq_valid_mask);
> +			dev_dbg(dev, "excluding GPIO %d from IRQ domain\n", i);
> +		} else if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i)) {
>  			byt_gpio_clear_triggering(vg, i);
> -			dev_dbg(&vg->pdev->dev, "disabling GPIO %d\n", i);
> +			dev_dbg(dev, "disabling GPIO %d\n", i);
>  		}
>  	}
>  
> @@ -1686,6 +1690,7 @@ static int byt_gpio_probe(struct byt_gpio *vg)
>  	gc->can_sleep	= false;
>  	gc->parent	= &vg->pdev->dev;
>  	gc->ngpio	= vg->soc_data->npins;
> +	gc->irq_need_valid_mask	= true;
>  
>  #ifdef CONFIG_PM_SLEEP
>  	vg->saved_context = devm_kcalloc(&vg->pdev->dev, gc->ngpio,
> -- 
> 2.11.0

Re: [PATCH v1 2/2] pinctrl: baytrail: Convert to use devm_*()

[Mika Westerberg]

On Tue, Jan 10, 2017 at 10:11:39PM +0200, Andy Shevchenko wrote:
> This simplifies error handling and allows us to drop error path handlers
> completely.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 1/2] pinctrl: baytrail: Do not add all GPIOs to IRQ domain

[Linus Walleij]

On Wed, Jan 11, 2017 at 11:27 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Jan 10, 2017 at 10:11:38PM +0200, Andy Shevchenko wrote:
>> It turns out that for some GPIO pins interrupts are bypassing standard
>> chain.
>>
>> Now the reason why some events such as touchscreen communication on ASuS
>> T100TA does not work if we mask all the interrupts is that in order to
>> generate either interrupts or GPEs the INTMASK register must have that
>> particular interrupt unmasked. In case of GPEs the CPU does not trigger
>> normal interrupt (and thus the GPIO driver does not see it) but instead
>> it causes SCI (System Control Interrupt) to be triggered with the GPE in
>> question set.
>>
>> To make this all work as expected we add those GPIOs to the IRQ
>> domain that can actually generate interrupts and skip others.
>
> Actually what happens is that when DIRECT_IRQ_EN is set, the pin is
> routed directly to the IO-APIC bypassing the GPIO driver completely.
> However, the mask register is still used to determine if the pin is
> supposed to generate IRQ or not.
>
> So with commit 3ae02c14d964 the IRQ core masks all IRQs (because of
> handle_bad_irq()) the pin connected to the touchscreen gets masked as
> well and hence no interrupts.
>
> This has nothing to do with GPEs, though.
>
> The fix itself looks good to me.

So I guess I wait for a commit with updates commit message
and then apply that for fixes?

Yours,
Linus Walleij

Re: [PATCH v1 2/2] pinctrl: baytrail: Convert to use devm_*()

[Linus Walleij]

On Tue, Jan 10, 2017 at 9:11 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> This simplifies error handling and allows us to drop error path handlers
> completely.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied with Mika's ACK.

Yours,
Linus Walleij

Re: [PATCH v1 1/2] pinctrl: baytrail: Do not add all GPIOs to IRQ domain

[Andy Shevchenko]

On Wed, 2017-01-11 at 14:12 +0100, Linus Walleij wrote:
> On Wed, Jan 11, 2017 at 11:27 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Jan 10, 2017 at 10:11:38PM +0200, Andy Shevchenko wrote:
> > > It turns out that for some GPIO pins interrupts are bypassing
> > > standard
> > > chain.
> > > 
> > > Now the reason why some events such as touchscreen communication
> > > on ASuS
> > > T100TA does not work if we mask all the interrupts is that in
> > > order to
> > > generate either interrupts or GPEs the INTMASK register must have
> > > that
> > > particular interrupt unmasked. In case of GPEs the CPU does not
> > > trigger
> > > normal interrupt (and thus the GPIO driver does not see it) but
> > > instead
> > > it causes SCI (System Control Interrupt) to be triggered with the
> > > GPE in
> > > question set.
> > > 
> > > To make this all work as expected we add those GPIOs to the IRQ
> > > domain that can actually generate interrupts and skip others.
> > 
> > Actually what happens is that when DIRECT_IRQ_EN is set, the pin is
> > routed directly to the IO-APIC bypassing the GPIO driver completely.
> > However, the mask register is still used to determine if the pin is
> > supposed to generate IRQ or not.
> > 
> > So with commit 3ae02c14d964 the IRQ core masks all IRQs (because of
> > handle_bad_irq()) the pin connected to the touchscreen gets masked
> > as
> > well and hence no interrupts.
> > 
> > This has nothing to do with GPEs, though.
> > 
> > The fix itself looks good to me.
> 
> So I guess I wait for a commit with updates commit message
> and then apply that for fixes?

Correct, I'm doing it right now. Takes time to do and to test of course.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy