Re: Issues with irq handling in mfd/intel_soc_pmic_bxtwc.c

[Andy Shevchenko]

(Cc: Vincent, he might be interested in that since he is working on
Basin Cove ADC driver for the moment)

On Mon, Feb 27, 2017 at 2:38 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi All,
>
> While working on Whiskey Cove PMIC support for Cherrytrail devices
> I noticed that the irq handling in the Broxton Whiskey Cove MFD code
> seems to be wrong.
>
> There seems to be some misunderstanding here about how the irqchip
> stuff should work.

No surprises to me. I remember we (our team) didn't had chance to go
through comprehensive review on it.

> If I understand the hardware correctly, there are 2 levels
> of interrupt masks first there is BXTWC_MIRQLVL1 which
> creates a number of interrupt groups, like thermal, bcu, etc.
>
> Then per interrupt group there is not only 1 or more
> write 1 to clear status registers to indicate the exact
> reason of the interrupt but also to possibility to mask
> out certain status bits from contributing to raising the
> interrupt for that group if not masked in BXTWC_MIRQLVL1.
>
> The current code defines 2 separate interrupt chips
> for this (well 3 really, but the 3th one is not relevant
> for this discussion).
>
> But instead of chaining these interrupt chips, as they
> are in hardware, they are setup as separate chips sharing the
> same interrupt.
>
> And all interrupt listeners (callers of request_interrupt
> on the irq-chip registered interrupts) only listen to
> the 2nd level irq-chip irqs.
>
> Since no one has requested any interrupts on the 1st level
> irq chip, this will cause ALL interrupt groups to get masked
> there by the interrupt core (unused interrupts are always
> masked), causing the 2nd level interrupts to never trigger.
>
> Currently this is being worked around for charging interrupts
> by the last hunk of this commit:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mfd/intel_soc_pmic_bxtwc.c?id=9c6235c8633210cc2da0882e2e9d6ff90aa37503
>
> Which blames the hardware for masking the lvl1 interrupt, but
> this really is expected behavior of the interrupt chip since
> no-one has requested the BXTWC_CHGR_LVL1_IRQ.

I Cc'ed to Heikki who is supposed our best guy in knowledge of Whiskey
Cove hardware. Perhaps he can confirm your analysis.

> I hope the above properly explains what I believe is wrong
> here. If not please ask.

To me your analysis sounds very sane. Unfortunately I have too many
tasks right now and this one has a bit less priority than few others.

> Now on to a solution, if you look at almost all the consumers
> then they are actually interested in all interrupts in a
> group, so it seems that there really is no need for the
> bxtwc_regmap_irq_chip_level2
>
> Taking the thermal irq as example we could simple replace:
>
> static struct resource thermal_resources[] = {
>         DEFINE_RES_IRQ(BXTWC_THRM0_IRQ),
>         DEFINE_RES_IRQ(BXTWC_THRM1_IRQ),
>         DEFINE_RES_IRQ(BXTWC_THRM2_IRQ),
> };
>
> With:
>
> static struct resource thermal_resources[] = {
>         DEFINE_RES_IRQ(BXTWC_THRM_LVL1_IRQ),
> };
>
> And make the thermal driver responsible for masking
> any status bits it is not interested in and write-1-clearing
> BXTWC_THRM0IRQ - BXTWC_THRM2IRQ in its interrupt handler,
> where it needs to read them anyway to find out the
> exact cause(s) of the interrupt.
>
> Note the driver needs to do a read, check set bits
> (and act upon them) and then write back the read value
> for these registers. It is important to write back the
> read value and not e.g. 0xff to makes sure that no interrupts
> are missed due to races, e.g. blindy writing 0xff to ack
> may cause some status bits which get set while the interrupt
> handler runs to get missed by the driver.
>
> This is how this is done in devices similar like this in
> general and I believe that the same should be done for the
> bxtwc code.
>
> One special case here seems to be the CHGR interrupt
> which gets used by 2 sub-devices. The standard solution
> for this would be to simple have both drivers request
> the same level1 irq with the SHARED flag, and have them
> both only check + write-1-clear the bits they are
> interested in. This does mean that both drivers
> must be loaded (or none) to avoid an interrupt storm.
>
> Alternatively you could register a 2nd level interrupt
> chip chained to the level1 chip (so listening to the
> BXTWC_CHGR_LVL1_IRQ not to pmic->irq) and have that
> split out things into separate irqs, that avoids the
> need to have both drivers successfully loaded to
> avoid interrupt storms. But this needs to be chained
> to the lvl1 chgr irq and not directly listen to
> pmic->irq.
>
> Another option would be to keep the current design and
> add a 2nd level irqchip per 1st level irq listening to
> the 1st level irq, but that would require 8 additional
> irq chips and for all consumers except the chgr consumers
> there is no added value in doing that.

If your analysis is fully correct, I would vote for option 1 (1st
level IRQs + special case for charger).

> There also is a bug in the code adding the TMU irqchip:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/mfd/intel_soc_pmic_bxtwc.c?id=957ae5098185e763b5c06be6c3b4b6e98c048712
>
> Since this is a new irqchip it should use a new irq enum
> and not add its irqs to the bxtwc_irqs_level2 enum.
>

> Note that irqchip can be removed entirely instead
> directly listening to the lvl1 TMU irq, which would
> also fix this.

I like an option that removes something from the kernel ;)

-- 
With Best Regards,
Andy Shevchenko