(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 <hdego...@redhat.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. > 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
