[PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin Yao]

A crash is triggered on the ASUS T100TA Baytrail-T because of a irq 
descriptor conflict. There are two gpio triggered acpi events in this
device, GPIO 6 and 18. These gpios are translated to irqs by calling
gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset).
irq_create_mapping will take care of allocating the irq descriptor, taking
the first available number starting from the given value (6 in our case).
The 0-15 are already reserved by legacy ISA code, so it gets the first
free irq descriptor which is number 16. The i915 driver also uses irq 16,
it loads later than gpio and crashes in probe.

The bug is reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=68291

The rootcause we know now is a low level irq issue. It needs a long term
solution to fix the issue in irq system.

This patch changes the Baytrail GPIO driver to avoid the irq descriptor
conflict. It still uses the irq domain to allocate irq descriptor but start
from a predefined irq base number (256) to avoid the conflict.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 drivers/pinctrl/pinctrl-baytrail.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index 6e8301f..45b2d81 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -124,6 +124,18 @@ static struct pinctrl_gpio_range byt_ranges[] = {
 	},
 };
 
+/*
+ * Start from an irq base number above x86 ioapic range to work around some
+ * nasty, which is still in 3.14 unresolved irq descriptor conflicts.
+ */
+#define BYT_GPIO_PIN_IRQBASE	256
+
+static int byt_pin_irqbase[] = {
+	BYT_GPIO_PIN_IRQBASE,
+	BYT_GPIO_PIN_IRQBASE + BYT_NGPIO_SCORE,
+	BYT_GPIO_PIN_IRQBASE + BYT_NGPIO_SCORE + BYT_NGPIO_NCORE,
+};
+
 struct byt_gpio {
 	struct gpio_chip		chip;
 	struct irq_domain		*domain;
@@ -131,6 +143,7 @@ struct byt_gpio {
 	spinlock_t			lock;
 	void __iomem			*reg_base;
 	struct pinctrl_gpio_range	*range;
+	int				pin_irqbase;
 };
 
 #define to_byt_gpio(c)	container_of(c, struct byt_gpio, chip)
@@ -481,7 +494,7 @@ static int byt_gpio_probe(struct platform_device *pdev)
 	struct pinctrl_gpio_range *range;
 	acpi_handle handle = ACPI_HANDLE(dev);
 	unsigned hwirq;
-	int ret;
+	int ret, i;
 
 	if (acpi_bus_get_device(handle, &acpi_dev))
 		return -ENODEV;
@@ -496,6 +509,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
 		if (!strcmp(acpi_dev->pnp.unique_id, range->name)) {
 			vg->chip.ngpio = range->npins;
 			vg->range = range;
+			ret = kstrtol(range->name, 10, &i);
+			if (ret != 0)
+				return ret;
+
+			i--;
+			vg->pin_irqbase = byt_pin_irqbase[i];
 			break;
 		}
 	}
@@ -527,19 +546,14 @@ static int byt_gpio_probe(struct platform_device *pdev)
 	gc->can_sleep = false;
 	gc->dev = dev;
 
-	ret = gpiochip_add(gc);
-	if (ret) {
-		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
-		return ret;
-	}
-
 	/* set up interrupts  */
 	irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_rc && irq_rc->start) {
 		hwirq = irq_rc->start;
 		gc->to_irq = byt_gpio_to_irq;
 
-		vg->domain = irq_domain_add_linear(NULL, gc->ngpio,
+		vg->domain = irq_domain_add_simple(NULL, gc->ngpio,
+						   vg->pin_irqbase,
 						   &byt_gpio_irq_ops, vg);
 		if (!vg->domain)
 			return -ENXIO;
@@ -550,6 +564,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
 		irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
 	}
 
+	ret = gpiochip_add(gc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
+		return ret;
+	}
+
 	pm_runtime_enable(dev);
 
 	return 0;
@@ -572,6 +592,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
 
 static const struct acpi_device_id byt_gpio_acpi_match[] = {
 	{ "INT33B2", 0 },
+	{ "INT33FC", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
-- 
1.8.3.2

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Benjamin Tissoires]

On Wed, Apr 16, 2014 at 8:05 AM, Jin Yao <yao.jin@linux.intel.com> wrote:
> A crash is triggered on the ASUS T100TA Baytrail-T because of a irq
> descriptor conflict. There are two gpio triggered acpi events in this
> device, GPIO 6 and 18. These gpios are translated to irqs by calling
> gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset).
> irq_create_mapping will take care of allocating the irq descriptor, taking
> the first available number starting from the given value (6 in our case).
> The 0-15 are already reserved by legacy ISA code, so it gets the first
> free irq descriptor which is number 16. The i915 driver also uses irq 16,
> it loads later than gpio and crashes in probe.
>
> The bug is reported here:
> https://bugzilla.kernel.org/show_bug.cgi?id=68291

Adam, the reporter of this bug told me that his touchscreen was broken
in its tablet.
I strongly suspect this patch to be the root cause of this, because
the touchscreen uses i2c_hid. i2c_hid relies on an IRQ declared in the
DSDT when it is acpi enumerated, and since the inclusion of this
patch, no irq are triggered from a driver point of view.

Adam should still confirm that the revert of the patch makes the
touchscreen back alive, but if I understood correctly the bug report,
without the patch, his tablet oopses at boot.

Still, it would be good if you could check that shifting the irqs in
the pinctrl is or is not a problem with the irqs used for i2c devices
(and others) declared in the dsdt.

Cheers,
Benjamin

>
> The rootcause we know now is a low level irq issue. It needs a long term
> solution to fix the issue in irq system.
>
> This patch changes the Baytrail GPIO driver to avoid the irq descriptor
> conflict. It still uses the irq domain to allocate irq descriptor but start
> from a predefined irq base number (256) to avoid the conflict.
>
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Adam Williamson]

On Fri, 2014-04-18 at 16:44 -0400, Benjamin Tissoires wrote:
> On Wed, Apr 16, 2014 at 8:05 AM, Jin Yao <yao.jin@linux.intel.com> wrote:
> > A crash is triggered on the ASUS T100TA Baytrail-T because of a irq
> > descriptor conflict. There are two gpio triggered acpi events in this
> > device, GPIO 6 and 18. These gpios are translated to irqs by calling
> > gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset).
> > irq_create_mapping will take care of allocating the irq descriptor, taking
> > the first available number starting from the given value (6 in our case).
> > The 0-15 are already reserved by legacy ISA code, so it gets the first
> > free irq descriptor which is number 16. The i915 driver also uses irq 16,
> > it loads later than gpio and crashes in probe.
> >
> > The bug is reported here:
> > https://bugzilla.kernel.org/show_bug.cgi?id=68291
> 
> Adam, the reporter of this bug told me that his touchscreen was broken
> in its tablet.
> I strongly suspect this patch to be the root cause of this, because
> the touchscreen uses i2c_hid. i2c_hid relies on an IRQ declared in the
> DSDT when it is acpi enumerated, and since the inclusion of this
> patch, no irq are triggered from a driver point of view.
> 
> Adam should still confirm that the revert of the patch makes the
> touchscreen back alive, but if I understood correctly the bug report,
> without the patch, his tablet oopses at boot.

if IRQ allocation is enabled, yeah. What I can do is test with the
#68291 patch dropped and IRQ allocation disabled in pinctrl-baytrail.c -
as is done in Doug Johnson's patch,
http://dougvj.net/baytrail_gpio_quirk.patch , for #67921 , with these
two blocks:

@@ -532,8 +535,8 @@ static int byt_gpio_probe(struct platform_device
*pdev)
 		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
 		return ret;
 	}
-
 	/* set up interrupts  */
+/*
 	irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_rc && irq_rc->start) {
 		hwirq = irq_rc->start;
@@ -548,7 +551,7 @@ static int byt_gpio_probe(struct platform_device
*pdev)
 
 		irq_set_handler_data(hwirq, vg);
 		irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
-	}
+	}*/
 
 	pm_runtime_enable(dev);
 

so, I'll do a test of that setup, and see how it goes.
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/19 4:44, Benjamin Tissoires wrote:
> On Wed, Apr 16, 2014 at 8:05 AM, Jin Yao <yao.jin@linux.intel.com> wrote:
>> A crash is triggered on the ASUS T100TA Baytrail-T because of a irq
>> descriptor conflict. There are two gpio triggered acpi events in this
>> device, GPIO 6 and 18. These gpios are translated to irqs by calling
>> gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset).
>> irq_create_mapping will take care of allocating the irq descriptor, taking
>> the first available number starting from the given value (6 in our case).
>> The 0-15 are already reserved by legacy ISA code, so it gets the first
>> free irq descriptor which is number 16. The i915 driver also uses irq 16,
>> it loads later than gpio and crashes in probe.
>>
>> The bug is reported here:
>> https://bugzilla.kernel.org/show_bug.cgi?id=68291
> 
> Adam, the reporter of this bug told me that his touchscreen was broken
> in its tablet.
This bug is triggered when system boots, or rather, it's triggered in
i915 probe, because i915 still uses irq 16 but it's allocated for gpio
yet. It's not a touchscreen case.

> I strongly suspect this patch to be the root cause of this, because
> the touchscreen uses i2c_hid. i2c_hid relies on an IRQ declared in the
> DSDT when it is acpi enumerated, and since the inclusion of this
> patch, no irq are triggered from a driver point of view.
> 
> Adam should still confirm that the revert of the patch makes the
> touchscreen back alive, but if I understood correctly the bug report,
> without the patch, his tablet oopses at boot.
Do you mean this patch cause the touchscreen doesn't work? I suspect
that because the touch screen works on my T100 with this patch.

> 
> Still, it would be good if you could check that shifting the irqs in
> the pinctrl is or is not a problem with the irqs used for i2c devices
> (and others) declared in the dsdt.
I checked DSDT of ASUS T100, looks no irq conflict after irq shifting.

> 
> Cheers,
> Benjamin
> 
>>
>> The rootcause we know now is a low level irq issue. It needs a long term
>> solution to fix the issue in irq system.
>>
>> This patch changes the Baytrail GPIO driver to avoid the irq descriptor
>> conflict. It still uses the irq domain to allocate irq descriptor but start
>> from a predefined irq base number (256) to avoid the conflict.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

For the issue that touch screen doesn't work, could you check power
state of LPSS devices? For example:

cd /sys/bus/acpi/devices
grep -H . */power_state

If they are D3cold, it should be the reason why touch screen doesn't
work. That's another issue, unrelated to this gpio patch.

On 2014/4/20 18:31, Jin, Yao wrote:
> 
> 
> On 2014/4/19 4:44, Benjamin Tissoires wrote:
>> On Wed, Apr 16, 2014 at 8:05 AM, Jin Yao <yao.jin@linux.intel.com> wrote:
>>> A crash is triggered on the ASUS T100TA Baytrail-T because of a irq
>>> descriptor conflict. There are two gpio triggered acpi events in this
>>> device, GPIO 6 and 18. These gpios are translated to irqs by calling
>>> gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset).
>>> irq_create_mapping will take care of allocating the irq descriptor, taking
>>> the first available number starting from the given value (6 in our case).
>>> The 0-15 are already reserved by legacy ISA code, so it gets the first
>>> free irq descriptor which is number 16. The i915 driver also uses irq 16,
>>> it loads later than gpio and crashes in probe.
>>>
>>> The bug is reported here:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=68291
>>
>> Adam, the reporter of this bug told me that his touchscreen was broken
>> in its tablet.
> This bug is triggered when system boots, or rather, it's triggered in
> i915 probe, because i915 still uses irq 16 but it's allocated for gpio
> yet. It's not a touchscreen case.
> 
>> I strongly suspect this patch to be the root cause of this, because
>> the touchscreen uses i2c_hid. i2c_hid relies on an IRQ declared in the
>> DSDT when it is acpi enumerated, and since the inclusion of this
>> patch, no irq are triggered from a driver point of view.
>>
>> Adam should still confirm that the revert of the patch makes the
>> touchscreen back alive, but if I understood correctly the bug report,
>> without the patch, his tablet oopses at boot.
> Do you mean this patch cause the touchscreen doesn't work? I suspect
> that because the touch screen works on my T100 with this patch.
> 
>>
>> Still, it would be good if you could check that shifting the irqs in
>> the pinctrl is or is not a problem with the irqs used for i2c devices
>> (and others) declared in the dsdt.
> I checked DSDT of ASUS T100, looks no irq conflict after irq shifting.
> 
>>
>> Cheers,
>> Benjamin
>>
>>>
>>> The rootcause we know now is a low level irq issue. It needs a long term
>>> solution to fix the issue in irq system.
>>>
>>> This patch changes the Baytrail GPIO driver to avoid the irq descriptor
>>> conflict. It still uses the irq domain to allocate irq descriptor but start
>>> from a predefined irq base number (256) to avoid the conflict.
>>>
>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Adam Williamson]

On Sun, 2014-04-20 at 20:08 +0800, Jin, Yao wrote:
> For the issue that touch screen doesn't work, could you check power
> state of LPSS devices? For example:
> 
> cd /sys/bus/acpi/devices
> grep -H . */power_state
> 
> If they are D3cold, it should be the reason why touch screen doesn't
> work. That's another issue, unrelated to this gpio patch.

The touch screen worked fine with kernel 3.14, and Benjamin looked at
debug output from the attempt to load the touchscreen driver when
diagnosing the problem, he's not just guessing. I am building a kernel
without your patch to confirm that fixes it.
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/20 23:28, Adam Williamson wrote:
> On Sun, 2014-04-20 at 20:08 +0800, Jin, Yao wrote:
>> For the issue that touch screen doesn't work, could you check power
>> state of LPSS devices? For example:
>>
>> cd /sys/bus/acpi/devices
>> grep -H . */power_state
>>
>> If they are D3cold, it should be the reason why touch screen doesn't
>> work. That's another issue, unrelated to this gpio patch.
> 
> The touch screen worked fine with kernel 3.14, and Benjamin looked at
> debug output from the attempt to load the touchscreen driver when
> diagnosing the problem, he's not just guessing. I am building a kernel
> without your patch to confirm that fixes it.
> 

I tried the clean kernel 3.14 with the boot option "nomodeset text" on
Dell Venue 11 Pro (If without "nomodeset",my ubuntu is being "black
screen", but that should be another i915 issue).

After system starup, I executed the "startx" to launch the xwindow. In
xwindow, the touchscreen work.

I tried the kernel 3.14 again with my gpio patch applied, the result was
the same, the touchscreen work.

I also check the DSDT table of Dell Venue 11 Pro by searching the
keyword "Interrupt", I can't find any clue for the irq conflict.

I'm sorry I don't have a Dell Venue 8 for testing, but I guess it's
similar to Dell Venue 11 Pro.

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/21 14:27, Jin, Yao wrote:
> 
> 
> On 2014/4/20 23:28, Adam Williamson wrote:
>> On Sun, 2014-04-20 at 20:08 +0800, Jin, Yao wrote:
>>> For the issue that touch screen doesn't work, could you check power
>>> state of LPSS devices? For example:
>>>
>>> cd /sys/bus/acpi/devices
>>> grep -H . */power_state
>>>
>>> If they are D3cold, it should be the reason why touch screen doesn't
>>> work. That's another issue, unrelated to this gpio patch.
>>
>> The touch screen worked fine with kernel 3.14, and Benjamin looked at
>> debug output from the attempt to load the touchscreen driver when
>> diagnosing the problem, he's not just guessing. I am building a kernel
>> without your patch to confirm that fixes it.
>>
> 
> I tried the clean kernel 3.14 with the boot option "nomodeset text" on
> Dell Venue 11 Pro (If without "nomodeset",my ubuntu is being "black
> screen", but that should be another i915 issue).
> 
> After system starup, I executed the "startx" to launch the xwindow. In
> xwindow, the touchscreen work.
> 
> I tried the kernel 3.14 again with my gpio patch applied, the result was
> the same, the touchscreen work.
> 
> I also check the DSDT table of Dell Venue 11 Pro by searching the
> keyword "Interrupt", I can't find any clue for the irq conflict.
> 
> I'm sorry I don't have a Dell Venue 8 for testing, but I guess it's
> similar to Dell Venue 11 Pro.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Could you also add following change to disable the runtime PM of HSUART?
After that then check if your touchscreen work? It's to solve a known
issue.The change is based on 3.14.

--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -368,8 +368,6 @@ static int dw8250_probe(struct platform_device *pdev)

        platform_set_drvdata(pdev, data);

-       pm_runtime_set_active(&pdev->dev);
-       pm_runtime_enable(&pdev->dev);

        return 0;
 }

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Benjamin Tissoires]

On Mon, Apr 21, 2014 at 9:28 AM, Jin, Yao <yao.jin@linux.intel.com> wrote:
>
>
> On 2014/4/21 14:27, Jin, Yao wrote:
>>
>>
>> On 2014/4/20 23:28, Adam Williamson wrote:
>>> On Sun, 2014-04-20 at 20:08 +0800, Jin, Yao wrote:
>>>> For the issue that touch screen doesn't work, could you check power
>>>> state of LPSS devices? For example:
>>>>
>>>> cd /sys/bus/acpi/devices
>>>> grep -H . */power_state
>>>>
>>>> If they are D3cold, it should be the reason why touch screen doesn't
>>>> work. That's another issue, unrelated to this gpio patch.
>>>
>>> The touch screen worked fine with kernel 3.14, and Benjamin looked at
>>> debug output from the attempt to load the touchscreen driver when
>>> diagnosing the problem, he's not just guessing. I am building a kernel
>>> without your patch to confirm that fixes it.
>>>
>>
>> I tried the clean kernel 3.14 with the boot option "nomodeset text" on
>> Dell Venue 11 Pro (If without "nomodeset",my ubuntu is being "black
>> screen", but that should be another i915 issue).
>>
>> After system starup, I executed the "startx" to launch the xwindow. In
>> xwindow, the touchscreen work.
>>
>> I tried the kernel 3.14 again with my gpio patch applied, the result was
>> the same, the touchscreen work.
>>
>> I also check the DSDT table of Dell Venue 11 Pro by searching the
>> keyword "Interrupt", I can't find any clue for the irq conflict.
>>
>> I'm sorry I don't have a Dell Venue 8 for testing, but I guess it's
>> similar to Dell Venue 11 Pro.

Sorry, my bad. Actually, this patch does not break the touchscreen,
but the other Adam applied does:
https://github.com/AdamWill/baytrail-m/blob/master/kernel/baytrail_gpio_quirk.patch

This one was to enable wifi, but it actually breaks the touchscreen.
I'll try to figure out which part of the patch breaks the touchscreen.

The good point is that I have a reproducer now on the Lenovo Miix 2,
so I'll be able to spot the pb easily.

Sorry for having been rude, and thanks for the tests.

Cheers,
Benjamin

>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
> Could you also add following change to disable the runtime PM of HSUART?
> After that then check if your touchscreen work? It's to solve a known
> issue.The change is based on 3.14.
>
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -368,8 +368,6 @@ static int dw8250_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, data);
>
> -       pm_runtime_set_active(&pdev->dev);
> -       pm_runtime_enable(&pdev->dev);
>
>         return 0;
>  }

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Adam Williamson]

On Mon, 2014-04-21 at 10:30 -0400, Benjamin Tissoires wrote:
> On Mon, Apr 21, 2014 at 9:28 AM, Jin, Yao <yao.jin@linux.intel.com> wrote:
> >
> >
> > On 2014/4/21 14:27, Jin, Yao wrote:
> >>
> >>
> >> On 2014/4/20 23:28, Adam Williamson wrote:
> >>> On Sun, 2014-04-20 at 20:08 +0800, Jin, Yao wrote:
> >>>> For the issue that touch screen doesn't work, could you check power
> >>>> state of LPSS devices? For example:
> >>>>
> >>>> cd /sys/bus/acpi/devices
> >>>> grep -H . */power_state
> >>>>
> >>>> If they are D3cold, it should be the reason why touch screen doesn't
> >>>> work. That's another issue, unrelated to this gpio patch.
> >>>
> >>> The touch screen worked fine with kernel 3.14, and Benjamin looked at
> >>> debug output from the attempt to load the touchscreen driver when
> >>> diagnosing the problem, he's not just guessing. I am building a kernel
> >>> without your patch to confirm that fixes it.
> >>>
> >>
> >> I tried the clean kernel 3.14 with the boot option "nomodeset text" on
> >> Dell Venue 11 Pro (If without "nomodeset",my ubuntu is being "black
> >> screen", but that should be another i915 issue).
> >>
> >> After system starup, I executed the "startx" to launch the xwindow. In
> >> xwindow, the touchscreen work.
> >>
> >> I tried the kernel 3.14 again with my gpio patch applied, the result was
> >> the same, the touchscreen work.
> >>
> >> I also check the DSDT table of Dell Venue 11 Pro by searching the
> >> keyword "Interrupt", I can't find any clue for the irq conflict.
> >>
> >> I'm sorry I don't have a Dell Venue 8 for testing, but I guess it's
> >> similar to Dell Venue 11 Pro.
> 
> Sorry, my bad. Actually, this patch does not break the touchscreen,
> but the other Adam applied does:
> https://github.com/AdamWill/baytrail-m/blob/master/kernel/baytrail_gpio_quirk.patch
> 
> This one was to enable wifi, but it actually breaks the touchscreen.
> I'll try to figure out which part of the patch breaks the touchscreen.
> 
> The good point is that I have a reproducer now on the Lenovo Miix 2,
> so I'll be able to spot the pb easily.
> 
> Sorry for having been rude, and thanks for the tests.

Cool, thanks for checking, Ben - my test build failed as I was trying to
apply the display detection fix in i915 as well, but it doesn't apply
against stock 3.15, and I didn't have time to fix it and fire again.
I'll try and confirm your findings today.

I'll let Doug know on https://bugzilla.kernel.org/show_bug.cgi?id=67921
that his patch appears to break touchscreen for us.
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Adam Williamson]

On Mon, 2014-04-21 at 10:30 -0400, Benjamin Tissoires wrote:
> On Mon, Apr 21, 2014 at 9:28 AM, Jin, Yao <yao.jin@linux.intel.com> wrote:
> >
> >
> > On 2014/4/21 14:27, Jin, Yao wrote:
> >>
> >>
> >> On 2014/4/20 23:28, Adam Williamson wrote:
> >>> On Sun, 2014-04-20 at 20:08 +0800, Jin, Yao wrote:
> >>>> For the issue that touch screen doesn't work, could you check power
> >>>> state of LPSS devices? For example:
> >>>>
> >>>> cd /sys/bus/acpi/devices
> >>>> grep -H . */power_state
> >>>>
> >>>> If they are D3cold, it should be the reason why touch screen doesn't
> >>>> work. That's another issue, unrelated to this gpio patch.
> >>>
> >>> The touch screen worked fine with kernel 3.14, and Benjamin looked at
> >>> debug output from the attempt to load the touchscreen driver when
> >>> diagnosing the problem, he's not just guessing. I am building a kernel
> >>> without your patch to confirm that fixes it.
> >>>
> >>
> >> I tried the clean kernel 3.14 with the boot option "nomodeset text" on
> >> Dell Venue 11 Pro (If without "nomodeset",my ubuntu is being "black
> >> screen", but that should be another i915 issue).
> >>
> >> After system starup, I executed the "startx" to launch the xwindow. In
> >> xwindow, the touchscreen work.
> >>
> >> I tried the kernel 3.14 again with my gpio patch applied, the result was
> >> the same, the touchscreen work.
> >>
> >> I also check the DSDT table of Dell Venue 11 Pro by searching the
> >> keyword "Interrupt", I can't find any clue for the irq conflict.
> >>
> >> I'm sorry I don't have a Dell Venue 8 for testing, but I guess it's
> >> similar to Dell Venue 11 Pro.
> 
> Sorry, my bad. Actually, this patch does not break the touchscreen,
> but the other Adam applied does:
> https://github.com/AdamWill/baytrail-m/blob/master/kernel/baytrail_gpio_quirk.patch
> 
> This one was to enable wifi, but it actually breaks the touchscreen.
> I'll try to figure out which part of the patch breaks the touchscreen.
> 
> The good point is that I have a reproducer now on the Lenovo Miix 2,
> so I'll be able to spot the pb easily.
> 
> Sorry for having been rude, and thanks for the tests.

Well, I can't actually concur. See my results in
https://bugzilla.kernel.org/show_bug.cgi?id=68291#c44 .

1. A kernel with neither patch applied (and no hid-rmi driver) results
in a working touchscreen.
2. A kernel with only v3 of Doug's patch from
https://bugzilla.kernel.org/show_bug.cgi?id=67921 results in a working
touchscreen.
3. A kernel with both v3 of Doug's patch and this IRQ descriptor
conflict "fix" results in a broken touchscreen.

Seems to me there really is some kind of problem with this patch...
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Mika Westerberg]

On Tue, Apr 22, 2014 at 10:16:50PM -0700, Adam Williamson wrote:
> Well, I can't actually concur. See my results in
> https://bugzilla.kernel.org/show_bug.cgi?id=68291#c44 .
> 
> 1. A kernel with neither patch applied (and no hid-rmi driver) results
> in a working touchscreen.
> 2. A kernel with only v3 of Doug's patch from
> https://bugzilla.kernel.org/show_bug.cgi?id=67921 results in a working
> touchscreen.
> 3. A kernel with both v3 of Doug's patch and this IRQ descriptor
> conflict "fix" results in a broken touchscreen.
> 
> Seems to me there really is some kind of problem with this patch...

Can you try so that you have both patches applied and then this one? I'm
suspecting that the ACPI GPIO operation region support might do something
unexpected in this case.

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index 555af733d5eb..1a9e0ab27141 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -557,6 +557,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
 	gc->can_sleep = false;
 	gc->dev = dev;
 
+	ret = gpiochip_add(gc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
+		return ret;
+	}
+
 	/* set up interrupts  */
 	irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_rc && irq_rc->start) {
@@ -575,12 +581,6 @@ static int byt_gpio_probe(struct platform_device *pdev)
 		irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
 	}
 
-	ret = gpiochip_add(gc);
-	if (ret) {
-		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
-		return ret;
-	}
-
 	pm_runtime_enable(dev);
 
 	return 0;

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Mika Westerberg]

On Wed, Apr 23, 2014 at 11:34:30AM +0300, Mika Westerberg wrote:
> On Tue, Apr 22, 2014 at 10:16:50PM -0700, Adam Williamson wrote:
> > Well, I can't actually concur. See my results in
> > https://bugzilla.kernel.org/show_bug.cgi?id=68291#c44 .
> > 
> > 1. A kernel with neither patch applied (and no hid-rmi driver) results
> > in a working touchscreen.
> > 2. A kernel with only v3 of Doug's patch from
> > https://bugzilla.kernel.org/show_bug.cgi?id=67921 results in a working
> > touchscreen.
> > 3. A kernel with both v3 of Doug's patch and this IRQ descriptor
> > conflict "fix" results in a broken touchscreen.
> > 
> > Seems to me there really is some kind of problem with this patch...
> 
> Can you try so that you have both patches applied and then this one? I'm
> suspecting that the ACPI GPIO operation region support might do something
> unexpected in this case.

I'm able to reproduce this problem here now and it seems not related to the
ACPI GPIO operation regions.

This patch changes call to irq_domain_add_linear() to
irq_domain_add_simple() and somehow that changes the behaviour so that I
get non-working touchscreen:

...
[   37.434998] i2c_hid i2c-ATML1000:00: failed to reset device.
[   37.435009] i2c_hid i2c-ATML1000:00: i2c_hid_set_power
[   37.435021] i2c_hid i2c-ATML1000:00: __i2c_hid_command: cmd=fb 00 01 08
[   38.439897] i2c_hid i2c-ATML1000:00: can't add hid device: -61
[   38.440749] i2c_hid: probe of i2c-ATML1000:00 failed with error -61

It never gets an interrupt when the device reset is ready.

Jin, do you have any idea what is going on?

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Adam Williamson]

On Wed, 2014-04-23 at 15:23 +0300, Mika Westerberg wrote:
> On Wed, Apr 23, 2014 at 11:34:30AM +0300, Mika Westerberg wrote:
> > On Tue, Apr 22, 2014 at 10:16:50PM -0700, Adam Williamson wrote:
> > > Well, I can't actually concur. See my results in
> > > https://bugzilla.kernel.org/show_bug.cgi?id=68291#c44 .
> > > 
> > > 1. A kernel with neither patch applied (and no hid-rmi driver) results
> > > in a working touchscreen.
> > > 2. A kernel with only v3 of Doug's patch from
> > > https://bugzilla.kernel.org/show_bug.cgi?id=67921 results in a working
> > > touchscreen.
> > > 3. A kernel with both v3 of Doug's patch and this IRQ descriptor
> > > conflict "fix" results in a broken touchscreen.
> > > 
> > > Seems to me there really is some kind of problem with this patch...
> > 
> > Can you try so that you have both patches applied and then this one? I'm
> > suspecting that the ACPI GPIO operation region support might do something
> > unexpected in this case.
> 
> I'm able to reproduce this problem here now and it seems not related to the
> ACPI GPIO operation regions.
> 
> This patch changes call to irq_domain_add_linear() to
> irq_domain_add_simple() and somehow that changes the behaviour so that I
> get non-working touchscreen:
> 
> ...
> [   37.434998] i2c_hid i2c-ATML1000:00: failed to reset device.
> [   37.435009] i2c_hid i2c-ATML1000:00: i2c_hid_set_power
> [   37.435021] i2c_hid i2c-ATML1000:00: __i2c_hid_command: cmd=fb 00 01 08
> [   38.439897] i2c_hid i2c-ATML1000:00: can't add hid device: -61
> [   38.440749] i2c_hid: probe of i2c-ATML1000:00 failed with error -61
> 
> It never gets an interrupt when the device reset is ready.
> 
> Jin, do you have any idea what is going on?

Well, hum - I did an encore:

4. A kernel with this IRQ descriptor conflict 'fix' but *without* Doug's
patch results in a working touch screen.

So unless I made a mistake somewhere, it looks like it's really the
combination of patches that causes trouble. Neither
http://dougvj.net/baytrail_gpio_quirk_v3.patch nor this patch alone
breaks the touch screen, but the combination of the two does.
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/23 20:23, Mika Westerberg wrote:
> On Wed, Apr 23, 2014 at 11:34:30AM +0300, Mika Westerberg wrote:
>> On Tue, Apr 22, 2014 at 10:16:50PM -0700, Adam Williamson wrote:
>>> Well, I can't actually concur. See my results in
>>> https://bugzilla.kernel.org/show_bug.cgi?id=68291#c44 .
>>>
>>> 1. A kernel with neither patch applied (and no hid-rmi driver) results
>>> in a working touchscreen.
>>> 2. A kernel with only v3 of Doug's patch from
>>> https://bugzilla.kernel.org/show_bug.cgi?id=67921 results in a working
>>> touchscreen.
>>> 3. A kernel with both v3 of Doug's patch and this IRQ descriptor
>>> conflict "fix" results in a broken touchscreen.
>>>
>>> Seems to me there really is some kind of problem with this patch...
>>
>> Can you try so that you have both patches applied and then this one? I'm
>> suspecting that the ACPI GPIO operation region support might do something
>> unexpected in this case.
> 
> I'm able to reproduce this problem here now and it seems not related to the
> ACPI GPIO operation regions.
> 
> This patch changes call to irq_domain_add_linear() to
> irq_domain_add_simple() and somehow that changes the behaviour so that I
> get non-working touchscreen:
> 
> ...
> [   37.434998] i2c_hid i2c-ATML1000:00: failed to reset device.
> [   37.435009] i2c_hid i2c-ATML1000:00: i2c_hid_set_power
> [   37.435021] i2c_hid i2c-ATML1000:00: __i2c_hid_command: cmd=fb 00 01 08
> [   38.439897] i2c_hid i2c-ATML1000:00: can't add hid device: -61
> [   38.440749] i2c_hid: probe of i2c-ATML1000:00 failed with error -61
> 
> It never gets an interrupt when the device reset is ready.
> 
> Jin, do you have any idea what is going on?
> 

Maybe neither the patch (http://dougvj.net/baytrail_gpio_quirk_v3.patch)
nor my patch breaks the touch screen.

I have tried the clean 3.15-rc2 with following patch which just adds
back the ACPIID "INT33FC", but the touch screen still doesn't work (To
avoid the i915 crash issue, I use with the boot option "nomodeset" in test).

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 69e29f4..d79c6d7 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -180,6 +180,7 @@ static const struct acpi_device_id
acpi_lpss_device_ids[] = {
        { "80860F14", (unsigned long)&byt_sdio_dev_desc },
        { "80860F41", (unsigned long)&byt_i2c_dev_desc },
        { "INT33B2", },
+       { "INT33FC", },

        { "INT3430", (unsigned long)&lpt_dev_desc },
        { "INT3431", (unsigned long)&lpt_dev_desc },
diff --git a/drivers/pinctrl/pinctrl-baytrail.c
b/drivers/pinctrl/pinctrl-baytrail.c
index 6e8301f..447f1dc 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {

 static const struct acpi_device_id byt_gpio_acpi_match[] = {
        { "INT33B2", 0 },
+       { "INT33FC", 0 },
        { }
 };
 MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);

Since the clean 3.15-rc2 doesn't contain "INT33FC", so the baytrail gpio
doesn't actually go into effect.

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Adam Williamson]

On Thu, 2014-04-24 at 21:30 +0800, Jin, Yao wrote:
> 
> On 2014/4/23 20:23, Mika Westerberg wrote:
> > On Wed, Apr 23, 2014 at 11:34:30AM +0300, Mika Westerberg wrote:
> >> On Tue, Apr 22, 2014 at 10:16:50PM -0700, Adam Williamson wrote:
> >>> Well, I can't actually concur. See my results in
> >>> https://bugzilla.kernel.org/show_bug.cgi?id=68291#c44 .
> >>>
> >>> 1. A kernel with neither patch applied (and no hid-rmi driver) results
> >>> in a working touchscreen.
> >>> 2. A kernel with only v3 of Doug's patch from
> >>> https://bugzilla.kernel.org/show_bug.cgi?id=67921 results in a working
> >>> touchscreen.
> >>> 3. A kernel with both v3 of Doug's patch and this IRQ descriptor
> >>> conflict "fix" results in a broken touchscreen.
> >>>
> >>> Seems to me there really is some kind of problem with this patch...
> >>
> >> Can you try so that you have both patches applied and then this one? I'm
> >> suspecting that the ACPI GPIO operation region support might do something
> >> unexpected in this case.
> > 
> > I'm able to reproduce this problem here now and it seems not related to the
> > ACPI GPIO operation regions.
> > 
> > This patch changes call to irq_domain_add_linear() to
> > irq_domain_add_simple() and somehow that changes the behaviour so that I
> > get non-working touchscreen:
> > 
> > ...
> > [   37.434998] i2c_hid i2c-ATML1000:00: failed to reset device.
> > [   37.435009] i2c_hid i2c-ATML1000:00: i2c_hid_set_power
> > [   37.435021] i2c_hid i2c-ATML1000:00: __i2c_hid_command: cmd=fb 00 01 08
> > [   38.439897] i2c_hid i2c-ATML1000:00: can't add hid device: -61
> > [   38.440749] i2c_hid: probe of i2c-ATML1000:00 failed with error -61
> > 
> > It never gets an interrupt when the device reset is ready.
> > 
> > Jin, do you have any idea what is going on?
> > 
> 
> Maybe neither the patch (http://dougvj.net/baytrail_gpio_quirk_v3.patch)
> nor my patch breaks the touch screen.
> 
> I have tried the clean 3.15-rc2 with following patch which just adds
> back the ACPIID "INT33FC", but the touch screen still doesn't work (To
> avoid the i915 crash issue, I use with the boot option "nomodeset" in test).
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 69e29f4..d79c6d7 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -180,6 +180,7 @@ static const struct acpi_device_id
> acpi_lpss_device_ids[] = {
>         { "80860F14", (unsigned long)&byt_sdio_dev_desc },
>         { "80860F41", (unsigned long)&byt_i2c_dev_desc },
>         { "INT33B2", },
> +       { "INT33FC", },
> 
>         { "INT3430", (unsigned long)&lpt_dev_desc },
>         { "INT3431", (unsigned long)&lpt_dev_desc },
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
> b/drivers/pinctrl/pinctrl-baytrail.c
> index 6e8301f..447f1dc 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
> 
>  static const struct acpi_device_id byt_gpio_acpi_match[] = {
>         { "INT33B2", 0 },
> +       { "INT33FC", 0 },
>         { }
>  };
>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
> 
> Since the clean 3.15-rc2 doesn't contain "INT33FC", so the baytrail gpio
> doesn't actually go into effect.

Grmph. Well, I'm almost sure that my test with only v3 of Doug's patch
included both of those changes - and for me, that resulted in a working
touchscreen - but it's just vaguely possible I'd dropped one of them
(the two patches both try and add INT33FC to pinctrl-baytrail.c , so I
have to keep jiggling that chunk between the patches depending on which
combination I'm trying to apply to the build I'm doing). I can try and
do yet another test build today...

I don't know if you're building from upstream or basing on the Fedora
Rawhide kernel, but if you're doing the latter, you need to use a very
recent build or revert the patch that adds the hid-rmi driver that's
queued for 3.16 upstream, or else you'll run into a *different* bug that
breaks the touchscreen, btw. I kinda assumed I'm the only one building
modified Fedora kernels and everyone else is just building the upstream
code directly, but maybe not.
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Adam Williamson]

On Thu, 2014-04-24 at 21:30 +0800, Jin, Yao wrote:

> > Jin, do you have any idea what is going on?
> > 
> 
> Maybe neither the patch (http://dougvj.net/baytrail_gpio_quirk_v3.patch)
> nor my patch breaks the touch screen.
> 
> I have tried the clean 3.15-rc2 with following patch which just adds
> back the ACPIID "INT33FC", but the touch screen still doesn't work (To
> avoid the i915 crash issue, I use with the boot option "nomodeset" in test).
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 69e29f4..d79c6d7 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -180,6 +180,7 @@ static const struct acpi_device_id
> acpi_lpss_device_ids[] = {
>         { "80860F14", (unsigned long)&byt_sdio_dev_desc },
>         { "80860F41", (unsigned long)&byt_i2c_dev_desc },
>         { "INT33B2", },
> +       { "INT33FC", },
> 
>         { "INT3430", (unsigned long)&lpt_dev_desc },
>         { "INT3431", (unsigned long)&lpt_dev_desc },
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
> b/drivers/pinctrl/pinctrl-baytrail.c
> index 6e8301f..447f1dc 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
> 
>  static const struct acpi_device_id byt_gpio_acpi_match[] = {
>         { "INT33B2", 0 },
> +       { "INT33FC", 0 },
>         { }
>  };
>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
> 
> Since the clean 3.15-rc2 doesn't contain "INT33FC", so the baytrail gpio
> doesn't actually go into effect.

Well, I just ran another test too. I built a kernel (3.15rc2) with both
v3 of Doug's SDIO device enumeration patch -
http://dougvj.net/baytrail_gpio_quirk_v3.patch - and the new patch for
the IRQ allocation issue by Thomas Gleixner,
https://patchwork.kernel.org/patch/4051581/ . That kernel gives me a
working touchscreen. Note that
http://dougvj.net/baytrail_gpio_quirk_v3.patch includes both the blocks
you mention (i.e. it adds INT33FC in both places).

So, it really seems like for me at least, it's the combination of Doug's
patch and your approach to fixing the IRQ allocation issue that breaks
the touchscreen. Thomas' approach, even combined with Doug's patch,
seems to work fine.
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/25 5:33, Adam Williamson wrote:
> On Thu, 2014-04-24 at 21:30 +0800, Jin, Yao wrote:
> 
>>> Jin, do you have any idea what is going on?
>>>
>>
>> Maybe neither the patch (http://dougvj.net/baytrail_gpio_quirk_v3.patch)
>> nor my patch breaks the touch screen.
>>
>> I have tried the clean 3.15-rc2 with following patch which just adds
>> back the ACPIID "INT33FC", but the touch screen still doesn't work (To
>> avoid the i915 crash issue, I use with the boot option "nomodeset" in test).
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index 69e29f4..d79c6d7 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -180,6 +180,7 @@ static const struct acpi_device_id
>> acpi_lpss_device_ids[] = {
>>         { "80860F14", (unsigned long)&byt_sdio_dev_desc },
>>         { "80860F41", (unsigned long)&byt_i2c_dev_desc },
>>         { "INT33B2", },
>> +       { "INT33FC", },
>>
>>         { "INT3430", (unsigned long)&lpt_dev_desc },
>>         { "INT3431", (unsigned long)&lpt_dev_desc },
>> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
>> b/drivers/pinctrl/pinctrl-baytrail.c
>> index 6e8301f..447f1dc 100644
>> --- a/drivers/pinctrl/pinctrl-baytrail.c
>> +++ b/drivers/pinctrl/pinctrl-baytrail.c
>> @@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
>>
>>  static const struct acpi_device_id byt_gpio_acpi_match[] = {
>>         { "INT33B2", 0 },
>> +       { "INT33FC", 0 },
>>         { }
>>  };
>>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
>>
>> Since the clean 3.15-rc2 doesn't contain "INT33FC", so the baytrail gpio
>> doesn't actually go into effect.
> 
> Well, I just ran another test too. I built a kernel (3.15rc2) with both
> v3 of Doug's SDIO device enumeration patch -
> http://dougvj.net/baytrail_gpio_quirk_v3.patch - and the new patch for
> the IRQ allocation issue by Thomas Gleixner,
> https://patchwork.kernel.org/patch/4051581/ . That kernel gives me a
> working touchscreen. Note that
> http://dougvj.net/baytrail_gpio_quirk_v3.patch includes both the blocks
> you mention (i.e. it adds INT33FC in both places).
> 
> So, it really seems like for me at least, it's the combination of Doug's
> patch and your approach to fixing the IRQ allocation issue that breaks
> the touchscreen. Thomas' approach, even combined with Doug's patch,
> seems to work fine.
> 

I'm trying the clean upstream 3.15-rc2 with the patch by Thomas Gleixner
(https://patchwork.kernel.org/patch/4051581/) and the patch as following
which only adds INT33FC, the touch screen doesn't work. I didn't add
Doug's SDIO device enumeration patch in this test.

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 69e29f4..d79c6d7 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -180,6 +180,7 @@ static const struct acpi_device_id
acpi_lpss_device_ids[] = {
        { "80860F14", (unsigned long)&byt_sdio_dev_desc },
        { "80860F41", (unsigned long)&byt_i2c_dev_desc },
        { "INT33B2", },
+       { "INT33FC", },

        { "INT3430", (unsigned long)&lpt_dev_desc },
        { "INT3431", (unsigned long)&lpt_dev_desc },
diff --git a/drivers/pinctrl/pinctrl-baytrail.c
b/drivers/pinctrl/pinctrl-baytrail.c
index 6e8301f..447f1dc 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {

 static const struct acpi_device_id byt_gpio_acpi_match[] = {
        { "INT33B2", 0 },
+       { "INT33FC", 0 },
        { }
 };
 MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);

It's still not clear to me why the touch screen doesn't work. As above
test, my patch is not added in.

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Mika Westerberg]

[+Aubrey]

On Fri, Apr 25, 2014 at 03:27:42PM +0800, Jin, Yao wrote:
> 
> 
> On 2014/4/25 5:33, Adam Williamson wrote:
> > On Thu, 2014-04-24 at 21:30 +0800, Jin, Yao wrote:
> > 
> >>> Jin, do you have any idea what is going on?
> >>>
> >>
> >> Maybe neither the patch (http://dougvj.net/baytrail_gpio_quirk_v3.patch)
> >> nor my patch breaks the touch screen.
> >>
> >> I have tried the clean 3.15-rc2 with following patch which just adds
> >> back the ACPIID "INT33FC", but the touch screen still doesn't work (To
> >> avoid the i915 crash issue, I use with the boot option "nomodeset" in test).
> >>
> >> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> >> index 69e29f4..d79c6d7 100644
> >> --- a/drivers/acpi/acpi_lpss.c
> >> +++ b/drivers/acpi/acpi_lpss.c
> >> @@ -180,6 +180,7 @@ static const struct acpi_device_id
> >> acpi_lpss_device_ids[] = {
> >>         { "80860F14", (unsigned long)&byt_sdio_dev_desc },
> >>         { "80860F41", (unsigned long)&byt_i2c_dev_desc },
> >>         { "INT33B2", },
> >> +       { "INT33FC", },
> >>
> >>         { "INT3430", (unsigned long)&lpt_dev_desc },
> >>         { "INT3431", (unsigned long)&lpt_dev_desc },
> >> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
> >> b/drivers/pinctrl/pinctrl-baytrail.c
> >> index 6e8301f..447f1dc 100644
> >> --- a/drivers/pinctrl/pinctrl-baytrail.c
> >> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> >> @@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
> >>
> >>  static const struct acpi_device_id byt_gpio_acpi_match[] = {
> >>         { "INT33B2", 0 },
> >> +       { "INT33FC", 0 },
> >>         { }
> >>  };
> >>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
> >>
> >> Since the clean 3.15-rc2 doesn't contain "INT33FC", so the baytrail gpio
> >> doesn't actually go into effect.
> > 
> > Well, I just ran another test too. I built a kernel (3.15rc2) with both
> > v3 of Doug's SDIO device enumeration patch -
> > http://dougvj.net/baytrail_gpio_quirk_v3.patch - and the new patch for
> > the IRQ allocation issue by Thomas Gleixner,
> > https://patchwork.kernel.org/patch/4051581/ . That kernel gives me a
> > working touchscreen. Note that
> > http://dougvj.net/baytrail_gpio_quirk_v3.patch includes both the blocks
> > you mention (i.e. it adds INT33FC in both places).
> > 
> > So, it really seems like for me at least, it's the combination of Doug's
> > patch and your approach to fixing the IRQ allocation issue that breaks
> > the touchscreen. Thomas' approach, even combined with Doug's patch,
> > seems to work fine.
> > 
> 
> I'm trying the clean upstream 3.15-rc2 with the patch by Thomas Gleixner
> (https://patchwork.kernel.org/patch/4051581/) and the patch as following
> which only adds INT33FC, the touch screen doesn't work. I didn't add
> Doug's SDIO device enumeration patch in this test.
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 69e29f4..d79c6d7 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -180,6 +180,7 @@ static const struct acpi_device_id
> acpi_lpss_device_ids[] = {
>         { "80860F14", (unsigned long)&byt_sdio_dev_desc },
>         { "80860F41", (unsigned long)&byt_i2c_dev_desc },
>         { "INT33B2", },
> +       { "INT33FC", },
> 
>         { "INT3430", (unsigned long)&lpt_dev_desc },
>         { "INT3431", (unsigned long)&lpt_dev_desc },
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
> b/drivers/pinctrl/pinctrl-baytrail.c
> index 6e8301f..447f1dc 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
> 
>  static const struct acpi_device_id byt_gpio_acpi_match[] = {
>         { "INT33B2", 0 },
> +       { "INT33FC", 0 },
>         { }
>  };
>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
> 
> It's still not clear to me why the touch screen doesn't work. As above
> test, my patch is not added in.

With patch from Thomas and below patch, I got working touchscreen. Note
that you need to disable runtime PM from SDHCI (or from some other LPSS
driver) so that the LPSS stays functional. Once all devices are powered
off, the block doesn't wake up anymore. Aubrey is investigating that.

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 69e29f409d4c..d79c6d7f598e 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -180,6 +180,7 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = {
 	{ "80860F14", (unsigned long)&byt_sdio_dev_desc },
 	{ "80860F41", (unsigned long)&byt_i2c_dev_desc },
 	{ "INT33B2", },
+	{ "INT33FC", },
 
 	{ "INT3430", (unsigned long)&lpt_dev_desc },
 	{ "INT3431", (unsigned long)&lpt_dev_desc },
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index ebb3f392b589..c5d4185380b1 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -130,8 +130,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
 };
 
 static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sd = {
-	.flags   = SDHCI_ACPI_SD_CD | SDHCI_ACPI_SD_CD_OVERRIDE_LEVEL |
-		   SDHCI_ACPI_RUNTIME_PM,
+	.flags   = SDHCI_ACPI_SD_CD | SDHCI_ACPI_SD_CD_OVERRIDE_LEVEL,
 	.quirks2 = SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON,
 };
 
diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index 6e8301f77187..7c65c9dab215 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -527,12 +527,6 @@ static int byt_gpio_probe(struct platform_device *pdev)
 	gc->can_sleep = false;
 	gc->dev = dev;
 
-	ret = gpiochip_add(gc);
-	if (ret) {
-		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
-		return ret;
-	}
-
 	/* set up interrupts  */
 	irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (irq_rc && irq_rc->start) {
@@ -550,6 +544,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
 		irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
 	}
 
+	ret = gpiochip_add(gc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
+		return ret;
+	}
+
 	pm_runtime_enable(dev);
 
 	return 0;
@@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
 
 static const struct acpi_device_id byt_gpio_acpi_match[] = {
 	{ "INT33B2", 0 },
+	{ "INT33FC", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/25 17:40, Mika Westerberg wrote:
> [+Aubrey]
> 
> On Fri, Apr 25, 2014 at 03:27:42PM +0800, Jin, Yao wrote:
>>
>>
>> On 2014/4/25 5:33, Adam Williamson wrote:
>>> On Thu, 2014-04-24 at 21:30 +0800, Jin, Yao wrote:
>>>
>>>>> Jin, do you have any idea what is going on?
>>>>>
>>>>
>>>> Maybe neither the patch (http://dougvj.net/baytrail_gpio_quirk_v3.patch)
>>>> nor my patch breaks the touch screen.
>>>>
>>>> I have tried the clean 3.15-rc2 with following patch which just adds
>>>> back the ACPIID "INT33FC", but the touch screen still doesn't work (To
>>>> avoid the i915 crash issue, I use with the boot option "nomodeset" in test).
>>>>
>>>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>>>> index 69e29f4..d79c6d7 100644
>>>> --- a/drivers/acpi/acpi_lpss.c
>>>> +++ b/drivers/acpi/acpi_lpss.c
>>>> @@ -180,6 +180,7 @@ static const struct acpi_device_id
>>>> acpi_lpss_device_ids[] = {
>>>>         { "80860F14", (unsigned long)&byt_sdio_dev_desc },
>>>>         { "80860F41", (unsigned long)&byt_i2c_dev_desc },
>>>>         { "INT33B2", },
>>>> +       { "INT33FC", },
>>>>
>>>>         { "INT3430", (unsigned long)&lpt_dev_desc },
>>>>         { "INT3431", (unsigned long)&lpt_dev_desc },
>>>> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
>>>> b/drivers/pinctrl/pinctrl-baytrail.c
>>>> index 6e8301f..447f1dc 100644
>>>> --- a/drivers/pinctrl/pinctrl-baytrail.c
>>>> +++ b/drivers/pinctrl/pinctrl-baytrail.c
>>>> @@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
>>>>
>>>>  static const struct acpi_device_id byt_gpio_acpi_match[] = {
>>>>         { "INT33B2", 0 },
>>>> +       { "INT33FC", 0 },
>>>>         { }
>>>>  };
>>>>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
>>>>
>>>> Since the clean 3.15-rc2 doesn't contain "INT33FC", so the baytrail gpio
>>>> doesn't actually go into effect.
>>>
>>> Well, I just ran another test too. I built a kernel (3.15rc2) with both
>>> v3 of Doug's SDIO device enumeration patch -
>>> http://dougvj.net/baytrail_gpio_quirk_v3.patch - and the new patch for
>>> the IRQ allocation issue by Thomas Gleixner,
>>> https://patchwork.kernel.org/patch/4051581/ . That kernel gives me a
>>> working touchscreen. Note that
>>> http://dougvj.net/baytrail_gpio_quirk_v3.patch includes both the blocks
>>> you mention (i.e. it adds INT33FC in both places).
>>>
>>> So, it really seems like for me at least, it's the combination of Doug's
>>> patch and your approach to fixing the IRQ allocation issue that breaks
>>> the touchscreen. Thomas' approach, even combined with Doug's patch,
>>> seems to work fine.
>>>
>>
>> I'm trying the clean upstream 3.15-rc2 with the patch by Thomas Gleixner
>> (https://patchwork.kernel.org/patch/4051581/) and the patch as following
>> which only adds INT33FC, the touch screen doesn't work. I didn't add
>> Doug's SDIO device enumeration patch in this test.
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index 69e29f4..d79c6d7 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -180,6 +180,7 @@ static const struct acpi_device_id
>> acpi_lpss_device_ids[] = {
>>         { "80860F14", (unsigned long)&byt_sdio_dev_desc },
>>         { "80860F41", (unsigned long)&byt_i2c_dev_desc },
>>         { "INT33B2", },
>> +       { "INT33FC", },
>>
>>         { "INT3430", (unsigned long)&lpt_dev_desc },
>>         { "INT3431", (unsigned long)&lpt_dev_desc },
>> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
>> b/drivers/pinctrl/pinctrl-baytrail.c
>> index 6e8301f..447f1dc 100644
>> --- a/drivers/pinctrl/pinctrl-baytrail.c
>> +++ b/drivers/pinctrl/pinctrl-baytrail.c
>> @@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
>>
>>  static const struct acpi_device_id byt_gpio_acpi_match[] = {
>>         { "INT33B2", 0 },
>> +       { "INT33FC", 0 },
>>         { }
>>  };
>>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
>>
>> It's still not clear to me why the touch screen doesn't work. As above
>> test, my patch is not added in.
> 
> With patch from Thomas and below patch, I got working touchscreen. Note
> that you need to disable runtime PM from SDHCI (or from some other LPSS
> driver) so that the LPSS stays functional. Once all devices are powered
> off, the block doesn't wake up anymore. Aubrey is investigating that.
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 69e29f409d4c..d79c6d7f598e 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -180,6 +180,7 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = {
>  	{ "80860F14", (unsigned long)&byt_sdio_dev_desc },
>  	{ "80860F41", (unsigned long)&byt_i2c_dev_desc },
>  	{ "INT33B2", },
> +	{ "INT33FC", },
>  
>  	{ "INT3430", (unsigned long)&lpt_dev_desc },
>  	{ "INT3431", (unsigned long)&lpt_dev_desc },
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index ebb3f392b589..c5d4185380b1 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -130,8 +130,7 @@ static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>  };
>  
>  static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sd = {
> -	.flags   = SDHCI_ACPI_SD_CD | SDHCI_ACPI_SD_CD_OVERRIDE_LEVEL |
> -		   SDHCI_ACPI_RUNTIME_PM,
> +	.flags   = SDHCI_ACPI_SD_CD | SDHCI_ACPI_SD_CD_OVERRIDE_LEVEL,
>  	.quirks2 = SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON,
>  };
>  
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
> index 6e8301f77187..7c65c9dab215 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -527,12 +527,6 @@ static int byt_gpio_probe(struct platform_device *pdev)
>  	gc->can_sleep = false;
>  	gc->dev = dev;
>  
> -	ret = gpiochip_add(gc);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
> -		return ret;
> -	}
> -
>  	/* set up interrupts  */
>  	irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>  	if (irq_rc && irq_rc->start) {
> @@ -550,6 +544,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
>  		irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
>  	}
>  
> +	ret = gpiochip_add(gc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
> +		return ret;
> +	}
> +
>  	pm_runtime_enable(dev);
>  
>  	return 0;
> @@ -572,6 +572,7 @@ static const struct dev_pm_ops byt_gpio_pm_ops = {
>  
>  static const struct acpi_device_id byt_gpio_acpi_match[] = {
>  	{ "INT33B2", 0 },
> +	{ "INT33FC", 0 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, byt_gpio_acpi_match);
> 

Yes, I just tried. With the HSUART runtime PM being disabled, the touch
screen works.

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Linus Walleij]

On Thu, Apr 24, 2014 at 11:33 PM, Adam Williamson <awilliam@redhat.com> wrote:

> Well, I just ran another test too. I built a kernel (3.15rc2) with both
> v3 of Doug's SDIO device enumeration patch -
> http://dougvj.net/baytrail_gpio_quirk_v3.patch - and the new patch for
> the IRQ allocation issue by Thomas Gleixner,
> https://patchwork.kernel.org/patch/4051581/ . That kernel gives me a
> working touchscreen. Note that
> http://dougvj.net/baytrail_gpio_quirk_v3.patch includes both the blocks
> you mention (i.e. it adds INT33FC in both places).
>
> So, it really seems like for me at least, it's the combination of Doug's
> patch and your approach to fixing the IRQ allocation issue that breaks
> the touchscreen. Thomas' approach, even combined with Doug's patch,
> seems to work fine.

Please make sure to provide a Tested-by: tag to Thomas so he
knows this is merge material.

Yours,
Linus Walleij

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Adam Williamson]

On Fri, 2014-04-25 at 11:32 +0200, Linus Walleij wrote:
> On Thu, Apr 24, 2014 at 11:33 PM, Adam Williamson <awilliam@redhat.com> wrote:
> 
> > Well, I just ran another test too. I built a kernel (3.15rc2) with both
> > v3 of Doug's SDIO device enumeration patch -
> > http://dougvj.net/baytrail_gpio_quirk_v3.patch - and the new patch for
> > the IRQ allocation issue by Thomas Gleixner,
> > https://patchwork.kernel.org/patch/4051581/ . That kernel gives me a
> > working touchscreen. Note that
> > http://dougvj.net/baytrail_gpio_quirk_v3.patch includes both the blocks
> > you mention (i.e. it adds INT33FC in both places).
> >
> > So, it really seems like for me at least, it's the combination of Doug's
> > patch and your approach to fixing the IRQ allocation issue that breaks
> > the touchscreen. Thomas' approach, even combined with Doug's patch,
> > seems to work fine.
> 
> Please make sure to provide a Tested-by: tag to Thomas so he
> knows this is merge material.

Well, I'd rather someone test on the box that's actually affected by the
IRQ allocation conflict lockups so we can confirm it actually fixes
them :)
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net
http://www.happyassassin.net

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/25 23:13, Adam Williamson wrote:
> On Fri, 2014-04-25 at 11:32 +0200, Linus Walleij wrote:
>> On Thu, Apr 24, 2014 at 11:33 PM, Adam Williamson <awilliam@redhat.com> wrote:
>>
>>> Well, I just ran another test too. I built a kernel (3.15rc2) with both
>>> v3 of Doug's SDIO device enumeration patch -
>>> http://dougvj.net/baytrail_gpio_quirk_v3.patch - and the new patch for
>>> the IRQ allocation issue by Thomas Gleixner,
>>> https://patchwork.kernel.org/patch/4051581/ . That kernel gives me a
>>> working touchscreen. Note that
>>> http://dougvj.net/baytrail_gpio_quirk_v3.patch includes both the blocks
>>> you mention (i.e. it adds INT33FC in both places).
>>>
>>> So, it really seems like for me at least, it's the combination of Doug's
>>> patch and your approach to fixing the IRQ allocation issue that breaks
>>> the touchscreen. Thomas' approach, even combined with Doug's patch,
>>> seems to work fine.
>>
>> Please make sure to provide a Tested-by: tag to Thomas so he
>> knows this is merge material.
> 
> Well, I'd rather someone test on the box that's actually affected by the
> IRQ allocation conflict lockups so we can confirm it actually fixes
> them :)
> 

I have tried the Thomas Gleixner's patch on 3.14.0 with i915 loading,
the irq conflict disappears.

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

On 2014/4/26 11:04, Jin, Yao wrote:
> 
> 
> On 2014/4/25 23:13, Adam Williamson wrote:
>> On Fri, 2014-04-25 at 11:32 +0200, Linus Walleij wrote:
>>> On Thu, Apr 24, 2014 at 11:33 PM, Adam Williamson <awilliam@redhat.com> wrote:
>>>
>>>> Well, I just ran another test too. I built a kernel (3.15rc2) with both
>>>> v3 of Doug's SDIO device enumeration patch -
>>>> http://dougvj.net/baytrail_gpio_quirk_v3.patch - and the new patch for
>>>> the IRQ allocation issue by Thomas Gleixner,
>>>> https://patchwork.kernel.org/patch/4051581/ . That kernel gives me a
>>>> working touchscreen. Note that
>>>> http://dougvj.net/baytrail_gpio_quirk_v3.patch includes both the blocks
>>>> you mention (i.e. it adds INT33FC in both places).
>>>>
>>>> So, it really seems like for me at least, it's the combination of Doug's
>>>> patch and your approach to fixing the IRQ allocation issue that breaks
>>>> the touchscreen. Thomas' approach, even combined with Doug's patch,
>>>> seems to work fine.
>>>
>>> Please make sure to provide a Tested-by: tag to Thomas so he
>>> knows this is merge material.
>>
>> Well, I'd rather someone test on the box that's actually affected by the
>> IRQ allocation conflict lockups so we can confirm it actually fixes
>> them :)
>>
> 
> I have tried the Thomas Gleixner's patch on 3.14.0 with i915 loading,
> the irq conflict disappears.
> --

I think we still need a small modification on pinctrl-baytrail.c, that
is moving gpiochip_add forward, otherwise the to_irq method is not set.

diff --git a/drivers/pinctrl/pinctrl-baytrail.c
b/drivers/pinctrl/pinctrl-baytrail.c
index e599834..fdfb84b 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -491,12 +491,6 @@ static int byt_gpio_probe(struct platform_device *pdev)
        gc->can_sleep = false;
        gc->dev = dev;

-       ret = gpiochip_add(gc);
-       if (ret) {
-               dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
-               return ret;
-       }
-
        /* set up interrupts  */
        irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
        if (irq_rc && irq_rc->start) {
@@ -514,6 +508,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
                irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
        }

+       ret = gpiochip_add(gc);
+       if (ret) {
+               dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
+               return ret;
+       }
+
        pm_runtime_enable(dev);

        return 0;

> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Linus Walleij]

On Fri, Apr 25, 2014 at 8:42 PM, Jin, Yao <yao.jin@linux.intel.com> wrote:
> On 2014/4/26 11:04, Jin, Yao wrote:
(...)
> I think we still need a small modification on pinctrl-baytrail.c, that
> is moving gpiochip_add forward, otherwise the to_irq method is not set.
>
> diff --git a/drivers/pinctrl/pinctrl-baytrail.c
> b/drivers/pinctrl/pinctrl-baytrail.c
> index e599834..fdfb84b 100644
> --- a/drivers/pinctrl/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/pinctrl-baytrail.c
> @@ -491,12 +491,6 @@ static int byt_gpio_probe(struct platform_device *pdev)
>         gc->can_sleep = false;
>         gc->dev = dev;
>
> -       ret = gpiochip_add(gc);
> -       if (ret) {
> -               dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
> -               return ret;
> -       }
> -
>         /* set up interrupts  */
>         irq_rc = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>         if (irq_rc && irq_rc->start) {
> @@ -514,6 +508,12 @@ static int byt_gpio_probe(struct platform_device *pdev)
>                 irq_set_chained_handler(hwirq, byt_gpio_irq_handler);
>         }
>
> +       ret = gpiochip_add(gc);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed adding byt-gpio chip\n");
> +               return ret;
> +       }
> +

Now I feel I'm not following what combo of patches solves the
problem any more.

Do you mean you think that this patch combined with Thomas'
will solve the root cause?

Yours,
Linus Walleij

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Benjamin Tissoires]

On Sun, Apr 20, 2014 at 6:31 AM, Jin, Yao <yao.jin@linux.intel.com> wrote:
>
>
> On 2014/4/19 4:44, Benjamin Tissoires wrote:
>> On Wed, Apr 16, 2014 at 8:05 AM, Jin Yao <yao.jin@linux.intel.com> wrote:
>>> A crash is triggered on the ASUS T100TA Baytrail-T because of a irq
>>> descriptor conflict. There are two gpio triggered acpi events in this
>>> device, GPIO 6 and 18. These gpios are translated to irqs by calling
>>> gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset).
>>> irq_create_mapping will take care of allocating the irq descriptor, taking
>>> the first available number starting from the given value (6 in our case).
>>> The 0-15 are already reserved by legacy ISA code, so it gets the first
>>> free irq descriptor which is number 16. The i915 driver also uses irq 16,
>>> it loads later than gpio and crashes in probe.
>>>
>>> The bug is reported here:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=68291
>>
>> Adam, the reporter of this bug told me that his touchscreen was broken
>> in its tablet.
> This bug is triggered when system boots, or rather, it's triggered in
> i915 probe, because i915 still uses irq 16 but it's allocated for gpio
> yet. It's not a touchscreen case.
>
>> I strongly suspect this patch to be the root cause of this, because
>> the touchscreen uses i2c_hid. i2c_hid relies on an IRQ declared in the
>> DSDT when it is acpi enumerated, and since the inclusion of this
>> patch, no irq are triggered from a driver point of view.
>>
>> Adam should still confirm that the revert of the patch makes the
>> touchscreen back alive, but if I understood correctly the bug report,
>> without the patch, his tablet oopses at boot.
> Do you mean this patch cause the touchscreen doesn't work? I suspect
> that because the touch screen works on my T100 with this patch.

I think this patch breaks his touchscreen, yes.
Adam has a Dell Venue Pro 8 with a touchscreen connected through
i2c-hid, which IRQ is declared in plain in the DSDT (I'll check that
with Adam what is this exact irq in his DSDT).

>
>>
>> Still, it would be good if you could check that shifting the irqs in
>> the pinctrl is or is not a problem with the irqs used for i2c devices
>> (and others) declared in the dsdt.
> I checked DSDT of ASUS T100, looks no irq conflict after irq shifting.

I meant: "Can you check if shifting the irqs in the pinctrl is a
problem if irqs are generally declared in the DSDT?"
I understand that you do not see a conflict with your Asus, otherwise
you wouldn't have proposed the patch. But is the patch _really_ safe
when the OEM uses irqs in the DSDT?

Another way of seeing this is: shouldn't we also patch an other part
of the acpi which retrieve the irqs when dealing with the enumaration
of the devices?

Cheers,
Benjamin

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Mika Westerberg]

On Wed, Apr 16, 2014 at 08:05:59PM +0800, Jin Yao wrote:
> A crash is triggered on the ASUS T100TA Baytrail-T because of a irq 
> descriptor conflict. There are two gpio triggered acpi events in this
> device, GPIO 6 and 18. These gpios are translated to irqs by calling
> gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset).
> irq_create_mapping will take care of allocating the irq descriptor, taking
> the first available number starting from the given value (6 in our case).
> The 0-15 are already reserved by legacy ISA code, so it gets the first
> free irq descriptor which is number 16. The i915 driver also uses irq 16,
> it loads later than gpio and crashes in probe.
> 
> The bug is reported here:
> https://bugzilla.kernel.org/show_bug.cgi?id=68291
> 
> The rootcause we know now is a low level irq issue. It needs a long term
> solution to fix the issue in irq system.
> 
> This patch changes the Baytrail GPIO driver to avoid the irq descriptor
> conflict. It still uses the irq domain to allocate irq descriptor but start
> from a predefined irq base number (256) to avoid the conflict.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>

I'm getting following warnings when compiling this:

drivers/pinctrl/pinctrl-baytrail.c: In function ‘byt_gpio_probe’:
drivers/pinctrl/pinctrl-baytrail.c:512:4: warning: passing argument 3 of
‘kstrtol’ from incompatible pointer type [enabled by default]
    ret = kstrtol(range->name, 10, &i);
    ^
In file included from drivers/pinctrl/pinctrl-baytrail.c:22:0:
include/linux/kernel.h:301:32: note: expected ‘long int *’ but argument is
of type ‘int *’
 static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)

Re: [PATCH] pinctrl-baytrail: fix for irq descriptor conflict on ASUS T100TA

[Jin, Yao]

Thanks Mika, I will fix this warning.

On 2014/4/22 19:51, Mika Westerberg wrote:
> On Wed, Apr 16, 2014 at 08:05:59PM +0800, Jin Yao wrote:
>> A crash is triggered on the ASUS T100TA Baytrail-T because of a irq 
>> descriptor conflict. There are two gpio triggered acpi events in this
>> device, GPIO 6 and 18. These gpios are translated to irqs by calling
>> gpio_to_irq which in turn will call irq_create_mapping(vg->domain, offset).
>> irq_create_mapping will take care of allocating the irq descriptor, taking
>> the first available number starting from the given value (6 in our case).
>> The 0-15 are already reserved by legacy ISA code, so it gets the first
>> free irq descriptor which is number 16. The i915 driver also uses irq 16,
>> it loads later than gpio and crashes in probe.
>>
>> The bug is reported here:
>> https://bugzilla.kernel.org/show_bug.cgi?id=68291
>>
>> The rootcause we know now is a low level irq issue. It needs a long term
>> solution to fix the issue in irq system.
>>
>> This patch changes the Baytrail GPIO driver to avoid the irq descriptor
>> conflict. It still uses the irq domain to allocate irq descriptor but start
>> from a predefined irq base number (256) to avoid the conflict.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> 
> I'm getting following warnings when compiling this:
> 
> drivers/pinctrl/pinctrl-baytrail.c: In function ‘byt_gpio_probe’:
> drivers/pinctrl/pinctrl-baytrail.c:512:4: warning: passing argument 3 of
> ‘kstrtol’ from incompatible pointer type [enabled by default]
>     ret = kstrtol(range->name, 10, &i);
>     ^
> In file included from drivers/pinctrl/pinctrl-baytrail.c:22:0:
> include/linux/kernel.h:301:32: note: expected ‘long int *’ but argument is
> of type ‘int *’
>  static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
>