Re: pinctrl-cherryview regression in linux-next on preproduction Braswell

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 1/4/22 16:26, Mika Westerberg wrote:
> Hi,
> 
> On Tue, Jan 04, 2022 at 11:22:53AM +0100, Hans de Goede wrote:
>> Andy, Mika, why don't we just mask out all IRQs at boot unconditionally:
>>
>> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> index 683b95e9639a..8981755a5d83 100644
>> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
>> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> @@ -1552,19 +1552,10 @@ static int chv_gpio_irq_init_hw(struct gpio_chip *chip)
>>  	const struct intel_community *community = &pctrl->communities[0];
>>  
>>  	/*
>> -	 * The same set of machines in chv_no_valid_mask[] have incorrectly
>> -	 * configured GPIOs that generate spurious interrupts so we use
>> -	 * this same list to apply another quirk for them.
>> -	 *
>> -	 * See also https://bugzilla.kernel.org/show_bug.cgi?id=197953.
>> +	 * Start with all normal interrupts in the community masked,
>> +	 * but leave the ones that can only generate GPEs unmasked.
>>  	 */
>> -	if (!pctrl->chip.irq.init_valid_mask) {
>> -		/*
>> -		 * Mask all interrupts the community is able to generate
>> -		 * but leave the ones that can only generate GPEs unmasked.
>> -		 */
>> -		chv_pctrl_writel(pctrl, CHV_INTMASK, GENMASK(31, community->nirqs));
>> -	}
>> +	chv_pctrl_writel(pctrl, CHV_INTMASK, GENMASK(31, community->nirqs));
>>  
>>  	/* Clear all interrupts */
>>  	chv_pctrl_writel(pctrl, CHV_INTSTAT, 0xffff);
>>
>> ?
> 
> IIRC masking them all broke some systems at the time. Unfortunately I
> don't remember the details anymore :(

Ok, so since this may hit other devices to I think we should go with one
of my first fix attempts for this then:

>From 46aba0f423b890a8ee21c76b5d460d8ba5c205f8 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Mon, 3 Jan 2022 11:16:00 +0100
Subject: [PATCH 1/2] pinctrl: cherryview: Trigger hwirq0 for interrupt-lines
 without a mapping

Commit bdfbef2d29dc ("pinctrl: cherryview: Don't use selection 0 to mark
an interrupt line as unused") made the code properly differentiate
between unset vs (hwirq) 0 entries in the GPIO-controller interrupt-line
to GPIO pinnumber/hwirq mapping.

This is causing some boards to not boot. This commit restores the old
behavior of triggering hwirq 0 when receiving an interrupt on an
interrupt-line for which there is no mapping.

Reported-and-tested-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index abffda1fd51e..1d5818269076 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1471,8 +1471,9 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
 
 		offset = cctx->intr_lines[intr_line];
 		if (offset == CHV_INVALID_HWIRQ) {
-			dev_err(dev, "interrupt on unused interrupt line %u\n", intr_line);
-			continue;
+			dev_warn_once(dev, "interrupt on unmapped interrupt line %u\n", intr_line);
+			/* Some boards expect hwirq 0 to trigger in this case */
+			offset = 0;
 		}
 
 		generic_handle_domain_irq(gc->irq.domain, offset);
-- 
2.33.1


Which works around this because calling generic_handle_domain_irq(gc->irq.domain, 0)
somehow shuts up the IRQ until it gets assigned.  I guess it ends up getting masked
by the low-level handler because there is no action assigned to it.

But I cannot find the code-path doing that masking for an irq_desc
with its handler set to handle_bad_irq, which is what the handler
for offset 0 should be at this point in time AFAICT...  Anyways this
fix has been tested and works (it basically restores the old behavior
for unmapped interrupt-lines, with a dev_want_once added).

Given that the hardware which Jarkko is using is pre-production hw
I believe that there is no need for a DMI quirk for that special hw then,
as this fix is sufficient to fix things there.

I'll submit this patch upstream in the usual manner right away.

Regards,

Hans