From 6b4542664c2d1fc7a770f0a4182ef5e36672d313 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 22 Nov 2021 23:04:23 +0100 Subject: pinctrl: baytrail: Set IRQCHIP_SET_TYPE_MASKED flag on the irqchip The byt_irq_type function ends with the IRQ masked, this means that calls to irq_set_irq_type() while the IRQ is enabled end up masking it, which is wrong. Add the IRQCHIP_SET_TYPE_MASKED flag to fix this. This will make the IRQ core call mask() + unmask() on the IRQ around a set_type() call when the IRQ is enabled at the type of the call. Note in practice irq_set_irq_type() getting called while the IRQ is enabled almost never happens. I hit this with a buggy DSDT where a wrongly active (_STA returns 0xf) I2C ACPI devices point to an IRQ already in use by an _AEI handler, leading to the irq_set_irq_type() call in acpi_dev_gpio_irq_get_by() getting called while the IRQ is enabled. Signed-off-by: Hans de Goede Acked-by: Mika Westerberg Signed-off-by: Andy Shevchenko --- drivers/pinctrl/intel/pinctrl-baytrail.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/pinctrl') diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c index 8f23d126c6a7..4c01333e1406 100644 --- a/drivers/pinctrl/intel/pinctrl-baytrail.c +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c @@ -1577,7 +1577,7 @@ static int byt_gpio_probe(struct intel_pinctrl *vg) vg->irqchip.irq_mask = byt_irq_mask, vg->irqchip.irq_unmask = byt_irq_unmask, vg->irqchip.irq_set_type = byt_irq_type, - vg->irqchip.flags = IRQCHIP_SKIP_SET_WAKE, + vg->irqchip.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_SET_TYPE_MASKED, girq = &gc->irq; girq->chip = &vg->irqchip; -- cgit v1.2.3 From bdfbef2d29dcdc79e2abf3085d4be6a844a06e34 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Thu, 18 Nov 2021 11:56:48 +0100 Subject: pinctrl: cherryview: Don't use selection 0 to mark an interrupt line as unused The selection 0 is a perfectly valid, so stop using it to have the special meaning of interrupt line not used in the intr_lines. Instead introduce a special CHV_INVALID_HWIRQ value, derived from INVALID_HWIRQ. which is never a valid selection and use that to indicate unused interrupt lines. Cc: Yauhen Kharuzhy Signed-off-by: Hans de Goede Acked-by: Mika Westerberg Signed-off-by: Andy Shevchenko --- drivers/pinctrl/intel/pinctrl-cherryview.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) (limited to 'drivers/pinctrl') diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c index 980099028cf8..978eddd89ea2 100644 --- a/drivers/pinctrl/intel/pinctrl-cherryview.c +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c @@ -73,6 +73,8 @@ struct intel_pad_context { u32 padctrl1; }; +#define CHV_INVALID_HWIRQ ((unsigned int)INVALID_HWIRQ) + /** * struct intel_community_context - community context for Cherryview * @intr_lines: Mapping between 16 HW interrupt wires and GPIO offset (in GPIO number space) @@ -812,7 +814,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev, /* Reset the interrupt mapping */ for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++) { if (cctx->intr_lines[i] == offset) { - cctx->intr_lines[i] = 0; + cctx->intr_lines[i] = CHV_INVALID_HWIRQ; break; } } @@ -1319,7 +1321,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d) else handler = handle_edge_irq; - if (!cctx->intr_lines[intsel]) { + if (cctx->intr_lines[intsel] == CHV_INVALID_HWIRQ) { irq_set_handler_locked(d, handler); cctx->intr_lines[intsel] = pin; } @@ -1412,6 +1414,12 @@ static void chv_gpio_irq_handler(struct irq_desc *desc) unsigned int offset; offset = cctx->intr_lines[intr_line]; + if (offset == CHV_INVALID_HWIRQ) { + dev_err(pctrl->dev, "interrupt on unused interrupt line %u\n", + intr_line); + continue; + } + generic_handle_domain_irq(gc->irq.domain, offset); } @@ -1617,11 +1625,13 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function, static int chv_pinctrl_probe(struct platform_device *pdev) { const struct intel_pinctrl_soc_data *soc_data; + struct intel_community_context *cctx; struct intel_community *community; struct device *dev = &pdev->dev; struct acpi_device *adev = ACPI_COMPANION(dev); struct intel_pinctrl *pctrl; acpi_status status; + unsigned int i; int ret, irq; soc_data = intel_pinctrl_get_soc_data(pdev); @@ -1663,6 +1673,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev) if (!pctrl->context.communities) return -ENOMEM; + cctx = &pctrl->context.communities[0]; + for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++) + cctx->intr_lines[i] = CHV_INVALID_HWIRQ; + irq = platform_get_irq(pdev, 0); if (irq < 0) return irq; -- cgit v1.2.3 From 07199dbf8cae36c973a89552fee83dd4e0a75972 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Thu, 18 Nov 2021 11:56:49 +0100 Subject: pinctrl: cherryview: Do not allow the same interrupt line to be used by 2 pins It is impossible to use the same interrupt line for 2 pins, this will result in the interrupts only being delivered to the IRQ handler for the pin for which chv_gpio_irq_type() was called last. The pinctrl-cherryview.c code relies on the BIOS to correctly setup the interrupt line, but there is a BIOS bug on at least the Medion Akoya E1239T and the GPD win models where both INT33FF:02 pin 8, used by the powerbutton and INT33FF:02 pin 21 used as IRQ input for the accelerometer are mapped to interrupt line 0. This causes 2 problems: 1. The accelerometer IRQ does not work, since the power button is probed later taking over the intr_lines[0] slot. 2. Since the accelerometer IRQ is not marked as wakeup, interrupt line 0 gets masked on suspend, causing the power button to not work to wake the system from suspend. Likewise on the Lenovo Yogabook, which has a touchscreen as keyboard and the keyboard half of the tablet also has a Wacom digitizer, the BIOS by default assigns the same interrupt line to the GPIOs used for their interrupts. Fix these problems by adding a check for this and assigning a new interrupt line to the 2nd pin for which chv_gpio_irq_type() gets called. With this fix in place the following 2 messages show up in dmesg on the Medion Akoya E1239T and the GPD win: cherryview-pinctrl INT33FF:02: interrupt line 0 is used by both pin 21 and pin 8 cherryview-pinctrl INT33FF:02: changing the interrupt line for pin 8 to 15 And the following gets logged on the Lenovo Yogabook: cherryview-pinctrl INT33FF:01: interrupt-line 0 is used by both pin 49 and pin 56 cherryview-pinctrl INT33FF:01: changing the interrupt line for pin 56 to 7 Note commit 9747070c11d6 ("Input: axp20x-pek - always register interrupt handlers") was added as a work around for the power button not being able to wakeup the system. This relies on using the PMIC's connection to the power button but that only works on systems with the AXP288 PMIC. Once this fix has been merged that workaround can be removed. Cc: Yauhen Kharuzhy Signed-off-by: Hans de Goede Acked-by: Mika Westerberg Signed-off-by: Andy Shevchenko --- drivers/pinctrl/intel/pinctrl-cherryview.c | 69 ++++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 8 deletions(-) (limited to 'drivers/pinctrl') diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c index 978eddd89ea2..b60a3e456ca4 100644 --- a/drivers/pinctrl/intel/pinctrl-cherryview.c +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c @@ -1323,6 +1323,8 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d) if (cctx->intr_lines[intsel] == CHV_INVALID_HWIRQ) { irq_set_handler_locked(d, handler); + dev_dbg(pctrl->dev, "using interrupt line %u for IRQ_TYPE_NONE on pin %u\n", + intsel, pin); cctx->intr_lines[intsel] = pin; } raw_spin_unlock_irqrestore(&chv_lock, flags); @@ -1332,17 +1334,73 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d) return 0; } +static int chv_gpio_set_intr_line(struct intel_pinctrl *pctrl, unsigned int pin) +{ + struct intel_community_context *cctx = &pctrl->context.communities[0]; + const struct intel_community *community = &pctrl->communities[0]; + u32 value, intsel; + int i; + + value = chv_readl(pctrl, pin, CHV_PADCTRL0); + intsel = (value & CHV_PADCTRL0_INTSEL_MASK) >> CHV_PADCTRL0_INTSEL_SHIFT; + + if (cctx->intr_lines[intsel] == pin) + return 0; + + if (cctx->intr_lines[intsel] == CHV_INVALID_HWIRQ) { + dev_dbg(pctrl->dev, "using interrupt line %u for pin %u\n", intsel, pin); + cctx->intr_lines[intsel] = pin; + return 0; + } + + /* + * The interrupt line selected by the BIOS is already in use by + * another pin, this is a known BIOS bug found on several models. + * But this may also be caused by Linux deciding to use a pin as + * IRQ which was not expected to be used as such by the BIOS authors, + * so log this at info level only. + */ + dev_info(pctrl->dev, "interrupt line %u is used by both pin %u and pin %u\n", + intsel, cctx->intr_lines[intsel], pin); + + if (chv_pad_locked(pctrl, pin)) + return -EBUSY; + + /* + * The BIOS fills the interrupt lines from 0 counting up, start at + * the other end to find a free interrupt line to workaround this. + */ + for (i = community->nirqs - 1; i >= 0; i--) { + if (cctx->intr_lines[i] == CHV_INVALID_HWIRQ) + break; + } + if (i < 0) + return -EBUSY; + + dev_info(pctrl->dev, "changing the interrupt line for pin %u to %d\n", pin, i); + + value = (value & ~CHV_PADCTRL0_INTSEL_MASK) | (i << CHV_PADCTRL0_INTSEL_SHIFT); + chv_writel(pctrl, pin, CHV_PADCTRL0, value); + cctx->intr_lines[i] = pin; + + return 0; +} + static int chv_gpio_irq_type(struct irq_data *d, unsigned int type) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct intel_pinctrl *pctrl = gpiochip_get_data(gc); - struct intel_community_context *cctx = &pctrl->context.communities[0]; unsigned int pin = irqd_to_hwirq(d); unsigned long flags; u32 value; + int ret; raw_spin_lock_irqsave(&chv_lock, flags); + ret = chv_gpio_set_intr_line(pctrl, pin); + if (ret) + goto out_unlock; + /* * Pins which can be used as shared interrupt are configured in * BIOS. Driver trusts BIOS configurations and assigns different @@ -1377,20 +1435,15 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type) chv_writel(pctrl, pin, CHV_PADCTRL1, value); } - value = chv_readl(pctrl, pin, CHV_PADCTRL0); - value &= CHV_PADCTRL0_INTSEL_MASK; - value >>= CHV_PADCTRL0_INTSEL_SHIFT; - - cctx->intr_lines[value] = pin; - if (type & IRQ_TYPE_EDGE_BOTH) irq_set_handler_locked(d, handle_edge_irq); else if (type & IRQ_TYPE_LEVEL_MASK) irq_set_handler_locked(d, handle_level_irq); +out_unlock: raw_spin_unlock_irqrestore(&chv_lock, flags); - return 0; + return ret; } static void chv_gpio_irq_handler(struct irq_desc *desc) -- cgit v1.2.3 From db1b2a8caf5b4954aa62ead5b0580948656eac43 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Fri, 26 Nov 2021 22:35:42 +0200 Subject: pinctrl: cherryview: Use temporary variable for struct device Use temporary variable for struct device to make code neater. Signed-off-by: Andy Shevchenko --- drivers/pinctrl/intel/pinctrl-cherryview.c | 57 +++++++++++++++--------------- 1 file changed, 29 insertions(+), 28 deletions(-) (limited to 'drivers/pinctrl') diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c index b60a3e456ca4..abffda1fd51e 100644 --- a/drivers/pinctrl/intel/pinctrl-cherryview.c +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c @@ -711,6 +711,7 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int function, unsigned int group) { struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + struct device *dev = pctrl->dev; const struct intel_pingroup *grp; unsigned long flags; int i; @@ -722,9 +723,8 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev, /* Check first that the pad is not locked */ for (i = 0; i < grp->npins; i++) { if (chv_pad_locked(pctrl, grp->pins[i])) { - dev_warn(pctrl->dev, "unable to set mode for locked pin %u\n", - grp->pins[i]); raw_spin_unlock_irqrestore(&chv_lock, flags); + dev_warn(dev, "unable to set mode for locked pin %u\n", grp->pins[i]); return -EBUSY; } } @@ -759,8 +759,8 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev, value |= CHV_PADCTRL1_INVRXTX_TXENABLE; chv_writel(pctrl, pin, CHV_PADCTRL1, value); - dev_dbg(pctrl->dev, "configured pin %u mode %u OE %sinverted\n", - pin, mode, invert_oe ? "" : "not "); + dev_dbg(dev, "configured pin %u mode %u OE %sinverted\n", pin, mode, + invert_oe ? "" : "not "); } raw_spin_unlock_irqrestore(&chv_lock, flags); @@ -1060,6 +1060,7 @@ static int chv_config_set(struct pinctrl_dev *pctldev, unsigned int pin, unsigned long *configs, unsigned int nconfigs) { struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + struct device *dev = pctrl->dev; enum pin_config_param param; int i, ret; u32 arg; @@ -1096,8 +1097,7 @@ static int chv_config_set(struct pinctrl_dev *pctldev, unsigned int pin, return -ENOTSUPP; } - dev_dbg(pctrl->dev, "pin %d set config %d arg %u\n", pin, - param, arg); + dev_dbg(dev, "pin %d set config %d arg %u\n", pin, param, arg); } return 0; @@ -1304,6 +1304,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d) if (irqd_get_trigger_type(d) == IRQ_TYPE_NONE) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct intel_pinctrl *pctrl = gpiochip_get_data(gc); + struct device *dev = pctrl->dev; struct intel_community_context *cctx = &pctrl->context.communities[0]; unsigned int pin = irqd_to_hwirq(d); irq_flow_handler_t handler; @@ -1323,7 +1324,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d) if (cctx->intr_lines[intsel] == CHV_INVALID_HWIRQ) { irq_set_handler_locked(d, handler); - dev_dbg(pctrl->dev, "using interrupt line %u for IRQ_TYPE_NONE on pin %u\n", + dev_dbg(dev, "using interrupt line %u for IRQ_TYPE_NONE on pin %u\n", intsel, pin); cctx->intr_lines[intsel] = pin; } @@ -1336,6 +1337,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d) static int chv_gpio_set_intr_line(struct intel_pinctrl *pctrl, unsigned int pin) { + struct device *dev = pctrl->dev; struct intel_community_context *cctx = &pctrl->context.communities[0]; const struct intel_community *community = &pctrl->communities[0]; u32 value, intsel; @@ -1348,7 +1350,7 @@ static int chv_gpio_set_intr_line(struct intel_pinctrl *pctrl, unsigned int pin) return 0; if (cctx->intr_lines[intsel] == CHV_INVALID_HWIRQ) { - dev_dbg(pctrl->dev, "using interrupt line %u for pin %u\n", intsel, pin); + dev_dbg(dev, "using interrupt line %u for pin %u\n", intsel, pin); cctx->intr_lines[intsel] = pin; return 0; } @@ -1360,8 +1362,8 @@ static int chv_gpio_set_intr_line(struct intel_pinctrl *pctrl, unsigned int pin) * IRQ which was not expected to be used as such by the BIOS authors, * so log this at info level only. */ - dev_info(pctrl->dev, "interrupt line %u is used by both pin %u and pin %u\n", - intsel, cctx->intr_lines[intsel], pin); + dev_info(dev, "interrupt line %u is used by both pin %u and pin %u\n", intsel, + cctx->intr_lines[intsel], pin); if (chv_pad_locked(pctrl, pin)) return -EBUSY; @@ -1377,7 +1379,7 @@ static int chv_gpio_set_intr_line(struct intel_pinctrl *pctrl, unsigned int pin) if (i < 0) return -EBUSY; - dev_info(pctrl->dev, "changing the interrupt line for pin %u to %d\n", pin, i); + dev_info(dev, "changing the interrupt line for pin %u to %d\n", pin, i); value = (value & ~CHV_PADCTRL0_INTSEL_MASK) | (i << CHV_PADCTRL0_INTSEL_SHIFT); chv_writel(pctrl, pin, CHV_PADCTRL0, value); @@ -1450,6 +1452,7 @@ static void chv_gpio_irq_handler(struct irq_desc *desc) { struct gpio_chip *gc = irq_desc_get_handler_data(desc); struct intel_pinctrl *pctrl = gpiochip_get_data(gc); + struct device *dev = pctrl->dev; const struct intel_community *community = &pctrl->communities[0]; struct intel_community_context *cctx = &pctrl->context.communities[0]; struct irq_chip *chip = irq_desc_get_chip(desc); @@ -1468,8 +1471,7 @@ static void chv_gpio_irq_handler(struct irq_desc *desc) offset = cctx->intr_lines[intr_line]; if (offset == CHV_INVALID_HWIRQ) { - dev_err(pctrl->dev, "interrupt on unused interrupt line %u\n", - intr_line); + dev_err(dev, "interrupt on unused interrupt line %u\n", intr_line); continue; } @@ -1573,17 +1575,16 @@ static int chv_gpio_irq_init_hw(struct gpio_chip *chip) static int chv_gpio_add_pin_ranges(struct gpio_chip *chip) { struct intel_pinctrl *pctrl = gpiochip_get_data(chip); + struct device *dev = pctrl->dev; const struct intel_community *community = &pctrl->communities[0]; const struct intel_padgroup *gpp; int ret, i; for (i = 0; i < community->ngpps; i++) { gpp = &community->gpps[i]; - ret = gpiochip_add_pin_range(chip, dev_name(pctrl->dev), - gpp->base, gpp->base, - gpp->size); + ret = gpiochip_add_pin_range(chip, dev_name(dev), gpp->base, gpp->base, gpp->size); if (ret) { - dev_err(pctrl->dev, "failed to add GPIO pin range\n"); + dev_err(dev, "failed to add GPIO pin range\n"); return ret; } } @@ -1596,15 +1597,16 @@ static int chv_gpio_probe(struct intel_pinctrl *pctrl, int irq) const struct intel_community *community = &pctrl->communities[0]; const struct intel_padgroup *gpp; struct gpio_chip *chip = &pctrl->chip; + struct device *dev = pctrl->dev; bool need_valid_mask = !dmi_check_system(chv_no_valid_mask); int ret, i, irq_base; *chip = chv_gpio_chip; chip->ngpio = pctrl->soc->pins[pctrl->soc->npins - 1].number + 1; - chip->label = dev_name(pctrl->dev); + chip->label = dev_name(dev); chip->add_pin_ranges = chv_gpio_add_pin_ranges; - chip->parent = pctrl->dev; + chip->parent = dev; chip->base = -1; pctrl->irq = irq; @@ -1626,17 +1628,16 @@ static int chv_gpio_probe(struct intel_pinctrl *pctrl, int irq) if (need_valid_mask) { chip->irq.init_valid_mask = chv_init_irq_valid_mask; } else { - irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0, - pctrl->soc->npins, NUMA_NO_NODE); + irq_base = devm_irq_alloc_descs(dev, -1, 0, pctrl->soc->npins, NUMA_NO_NODE); if (irq_base < 0) { - dev_err(pctrl->dev, "Failed to allocate IRQ numbers\n"); + dev_err(dev, "Failed to allocate IRQ numbers\n"); return irq_base; } } - ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl); + ret = devm_gpiochip_add_data(dev, chip, pctrl); if (ret) { - dev_err(pctrl->dev, "Failed to register gpiochip\n"); + dev_err(dev, "Failed to register gpiochip\n"); return ret; } @@ -1834,15 +1835,15 @@ static int chv_pinctrl_resume_noirq(struct device *dev) val &= ~CHV_PADCTRL0_GPIORXSTATE; if (ctx->padctrl0 != val) { chv_writel(pctrl, desc->number, CHV_PADCTRL0, ctx->padctrl0); - dev_dbg(pctrl->dev, "restored pin %2u ctrl0 0x%08x\n", - desc->number, chv_readl(pctrl, desc->number, CHV_PADCTRL0)); + dev_dbg(dev, "restored pin %2u ctrl0 0x%08x\n", desc->number, + chv_readl(pctrl, desc->number, CHV_PADCTRL0)); } val = chv_readl(pctrl, desc->number, CHV_PADCTRL1); if (ctx->padctrl1 != val) { chv_writel(pctrl, desc->number, CHV_PADCTRL1, ctx->padctrl1); - dev_dbg(pctrl->dev, "restored pin %2u ctrl1 0x%08x\n", - desc->number, chv_readl(pctrl, desc->number, CHV_PADCTRL1)); + dev_dbg(dev, "restored pin %2u ctrl1 0x%08x\n", desc->number, + chv_readl(pctrl, desc->number, CHV_PADCTRL1)); } } -- cgit v1.2.3