Hi,
On 25-10-2019 16:06, Andy Shevchenko wrote:
There is a logical continuation of the commit 5fbe5b5883f8 ("gpio: Initialize
the irqchip valid_mask with a callback") to split IRQ initialization to
hardware and valid mask setup parts.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
So I've been running some tests based on https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=for-next
+ this patch.
That combo causes several issues, so I tried reverting the patches one by one,
if I drop this patch and use just:
https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=for-next
Then the kernel does not even boot. I believe this is caused by:
88583e340a0e ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip")
The problem here is that gpiochip_add_data_with_key() calls gpiochip_irqchip_init_hw()
before it calls gpiochip_irqchip_init_valid_mask(), so after commit 88583e340a0e
when byt_gpio_irq_init_hw runs gc->irq.valid_mask is NULL and we crash with a NULL
pointer exception (or so I believe, the kernel never gets far enough to get
any info out of it without extra work).
Note that this ("[PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback")
patch fixes this since it moves the gc->irq.valid_mask accesses to
byt_init_irq_valid_mask.
But this change itself triggers another variant of this ordering issue,
it causes these 2 new errors to get logged:
byt_gpio INT33FC:01: GPIO interrupt error, pins misconfigured. INT_STAT0: 0x01800000
byt_gpio INT33FC:02: GPIO interrupt error, pins misconfigured. INT_STAT0: 0x00400000
The problem is that before this change the code calculating the valid_mask
would also disable interrupts on GPIOs which do not have their
BYT_DIRECT_IRQ_EN bit set. This now happens after the check done in
byt_gpio_irq_init_hw() causing these false-positive error messages.
Even if we ignore the NULL pointer deref problem for now and we ignore
these 2 new error messages for now. Things are still broken with the
current changes in pinctrl/intel.git/for-next switching to letting
devm_gpiochip_add_data register the irqchip means that
acpi_gpiochip_request_interrupts() gets called before
gpiochip_add_pin_range() is called from pinctrl-baytrail.c, causing
the GPIO lookup of any ACPI _AEI handlers to fail, resulting in
errors like this one:
byt_gpio INT33FC:02: Failed to request GPIO for pin 0x13: -517
And none of the _AEI handlers working
TL;DR: commit 88583e340a0e ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip")
breaks a bunch of stuff and should be dropped from pinctrl/intel.git/for-next
and this needs some more work before it is ready for mainline.
Regards,
Hans
---
drivers/pinctrl/intel/pinctrl-baytrail.c | 25 ++++++++++--------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index beb26550c25f..08e2b940cc11 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1432,22 +1432,10 @@ static void byt_init_irq_valid_mask(struct gpio_chip *chip,
unsigned long *valid_mask,
unsigned int ngpios)
{
- /*
- * FIXME: currently the valid_mask is filled in as part of
- * initializing the irq_chip below in byt_gpio_irq_init_hw().
- * when converting this driver to the new way of passing the
- * gpio_irq_chip along when adding the gpio_chip, move the
- * mask initialization into this callback instead. Right now
- * this callback is here to make sure the mask gets allocated.
- */
-}
-
-static int byt_gpio_irq_init_hw(struct gpio_chip *gc)
-{
- struct byt_gpio *vg = gpiochip_get_data(gc);
+ struct byt_gpio *vg = gpiochip_get_data(chip);
struct device *dev = &vg->pdev->dev;
void __iomem *reg;
- u32 base, value;
+ u32 value;
int i;
/*
@@ -1468,13 +1456,20 @@ static int byt_gpio_irq_init_hw(struct gpio_chip *gc)
value = readl(reg);
if (value & BYT_DIRECT_IRQ_EN) {
- clear_bit(i, gc->irq.valid_mask);
+ clear_bit(i, 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(dev, "disabling GPIO %d\n", i);
}
}
+}
+
+static int byt_gpio_irq_init_hw(struct gpio_chip *gc)
+{
+ struct byt_gpio *vg = gpiochip_get_data(gc);
+ void __iomem *reg;
+ u32 base, value;
/* clear interrupt status trigger registers */
for (base = 0; base < vg->soc_data->npins; base += 32) {
