Message-ID: <20170228111238.GC19555@kuha.fi.intel.com>
Date:   Tue, 28 Feb 2017 13:12:38 +0200
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>
Cc:     Nilesh Bacchewar <nilesh.bacchewar@...el.com>,
        Bin Gao <bin.gao@...ux.intel.com>,
        Lee Jones <lee.jones@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Vincent Pelletier <plr.vincent@...il.com>
Subject: Re: Issues with irq handling in mfd/intel_soc_pmic_bxtwc.c

Hi,

On Tue, Feb 28, 2017 at 12:26:03PM +0200, Andy Shevchenko wrote:
> (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@...hat.com> 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.

Your analysis looks correct to me, though I'm really not an expert on
WhiskeyCove. We did a few changes to the mfd driver for it together
with Felipe (CCd) to prepare it for the USB Type-C PHY driver (which
is still not part of mainline), and that's about all my experiences
with it. Though it was enough to notice that the driver indeed needs
to be refactored.

I believe Felipe noticed some other problems in the driver as well,
like the missing cache type.

> > 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).

I'm don't have an opinion on what would be the best approach at the
moment.

> > 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 ;)


Cheers,

-- 
heikki

Powered by blists - more mailing lists