On Thu, Nov 18, 2021 at 12:16:13AM +0100, Hans de Goede wrote:
> It is impossible to use the same intr-line for 2 pins, this will result
> in the interrupts only being delivered to the irq-handler for the pin for
IRQ handler
> 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
interrupt line
(Ditto everywhere).
> and the GPD win models where both INT33FF:02 pin 8, used by the powerbutton
power button
> 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
power button
(Ditto everywhere)
> 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 drawing tablet, 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: [Firmware Bug]: 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
> 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
> powerbutton but that only works on systems with the AXP288 PMIC.
> Once this fix has been merged that workaround can be removed.
>
> Cc: Yauhen Kharuzhy <jekhor@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/pinctrl/intel/pinctrl-cherryview.c | 69 +++++++++++++++++++---
> 1 file changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index a46f9e5a4748..491b234812cd 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] == INTR_LINE_UNUSED) {
> irq_set_handler_locked(d, handler);
> + dev_dbg(pctrl->dev, "Using interrupt-line %u for IRQ_TYPE_NONE IRQ on pin %u\n",
> + intsel, pin);
"using interrupt line ..."
> 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] == INTR_LINE_UNUSED) {
> + dev_dbg(pctrl->dev, "Using interrupt-line %u for pin %u\n", intsel, pin);
"using interrupt line ..."
> + 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);
"interrupt line %u ..."
> +
> + 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] == INTR_LINE_UNUSED)
> + break;
> + }
> + if (i < 0)
> + return -EBUSY;
> +
> + dev_info(pctrl->dev, "Changing the interrupt-line for pin %u to %d\n", pin, i);
"changing the ..."
> +
> + 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)
> --
> 2.31.1
