Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Tue, Apr 04, 2017 at 06:56:30AM -0700, Tony Lindgren wrote: > * Mark Brown[170404 05:22]: > > It is sadly far too common for people to implement interrupt controllers > > that only do edge triggers, I've no idea why even on what are supposed > > to be relatively high end SoCs. It seems to be a hardware designer > > thing, AFAICT they think for something to be useful it needs to be a bit > > more complicated. > For edge only GPIO controllers handling level interrupts might be > somewhat fixable in software. The GPIO controller could have a loop > reading of the GPIO line status in the interrupt handler and comparing > it to the configured triggering. Yeah, it's doable if annoying. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Tue, Apr 04, 2017 at 06:56:30AM -0700, Tony Lindgren wrote: > * Mark Brown [170404 05:22]: > > It is sadly far too common for people to implement interrupt controllers > > that only do edge triggers, I've no idea why even on what are supposed > > to be relatively high end SoCs. It seems to be a hardware designer > > thing, AFAICT they think for something to be useful it needs to be a bit > > more complicated. > For edge only GPIO controllers handling level interrupts might be > somewhat fixable in software. The GPIO controller could have a loop > reading of the GPIO line status in the interrupt handler and comparing > it to the configured triggering. Yeah, it's doable if annoying. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Mark Brown[170404 05:22]: > On Mon, Apr 03, 2017 at 08:03:00PM -0700, Tony Lindgren wrote: > > > So I'll drop the genirq/regmap_irq related hacks and resend just > > the minimal MFD fixes. Similar misconfiguration may be the root > > cause for other drivers too.. > > It is sadly far too common for people to implement interrupt controllers > that only do edge triggers, I've no idea why even on what are supposed > to be relatively high end SoCs. It seems to be a hardware designer > thing, AFAICT they think for something to be useful it needs to be a bit > more complicated. For edge only GPIO controllers handling level interrupts might be somewhat fixable in software. The GPIO controller could have a loop reading of the GPIO line status in the interrupt handler and comparing it to the configured triggering. Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Mark Brown [170404 05:22]: > On Mon, Apr 03, 2017 at 08:03:00PM -0700, Tony Lindgren wrote: > > > So I'll drop the genirq/regmap_irq related hacks and resend just > > the minimal MFD fixes. Similar misconfiguration may be the root > > cause for other drivers too.. > > It is sadly far too common for people to implement interrupt controllers > that only do edge triggers, I've no idea why even on what are supposed > to be relatively high end SoCs. It seems to be a hardware designer > thing, AFAICT they think for something to be useful it needs to be a bit > more complicated. For edge only GPIO controllers handling level interrupts might be somewhat fixable in software. The GPIO controller could have a loop reading of the GPIO line status in the interrupt handler and comparing it to the configured triggering. Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Mon, Apr 03, 2017 at 08:03:00PM -0700, Tony Lindgren wrote: > So I'll drop the genirq/regmap_irq related hacks and resend just > the minimal MFD fixes. Similar misconfiguration may be the root > cause for other drivers too.. It is sadly far too common for people to implement interrupt controllers that only do edge triggers, I've no idea why even on what are supposed to be relatively high end SoCs. It seems to be a hardware designer thing, AFAICT they think for something to be useful it needs to be a bit more complicated. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Mon, Apr 03, 2017 at 08:03:00PM -0700, Tony Lindgren wrote: > So I'll drop the genirq/regmap_irq related hacks and resend just > the minimal MFD fixes. Similar misconfiguration may be the root > cause for other drivers too.. It is sadly far too common for people to implement interrupt controllers that only do edge triggers, I've no idea why even on what are supposed to be relatively high end SoCs. It seems to be a hardware designer thing, AFAICT they think for something to be useful it needs to be a bit more complicated. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
Hi, * Tony Lindgren[170328 10:13]: > * Mark Brown [170328 09:51]: > > On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote: > > > * Mark Brown [170328 08:21]: > > > > > > Right, my thinking here is that by pushing into genirq we minimise the > > > > need even further since it'll also be available to drivers not using > > > > regmap-irq. > > > > > > > like, handle until we get IRQ_NONE? :) > > > > > > Well, that's what the per driver emulation does so... yeah. Probably > > > > with an upper limit on the number of times we do that. > > > > > OK let's first see how that would work. I'll send a patch > > > for that. > > > > Thanks. Can you keep me on the CC please? It's something I keep > > thinking about looking at myself. > > Sure will do, you'll get some shared flames on it :) So I found the real problem after thinking what you guys commented on the "level device connected to an edge only GPIO". I added some printks to verify that's not the case just to find out that the dts GPIO interrupt edge configuration got only passed to the SPI driver :) So the level IRQ changes I did earlier to the dts file did nothing. The fix is simply to call devm_regmap_add_irq_chip() with irq_get_trigger_type(cpcap->spi->irq) to get the configured triggering passed properly from the dts. So I'll drop the genirq/regmap_irq related hacks and resend just the minimal MFD fixes. Similar misconfiguration may be the root cause for other drivers too.. Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
Hi, * Tony Lindgren [170328 10:13]: > * Mark Brown [170328 09:51]: > > On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote: > > > * Mark Brown [170328 08:21]: > > > > > > Right, my thinking here is that by pushing into genirq we minimise the > > > > need even further since it'll also be available to drivers not using > > > > regmap-irq. > > > > > > > like, handle until we get IRQ_NONE? :) > > > > > > Well, that's what the per driver emulation does so... yeah. Probably > > > > with an upper limit on the number of times we do that. > > > > > OK let's first see how that would work. I'll send a patch > > > for that. > > > > Thanks. Can you keep me on the CC please? It's something I keep > > thinking about looking at myself. > > Sure will do, you'll get some shared flames on it :) So I found the real problem after thinking what you guys commented on the "level device connected to an edge only GPIO". I added some printks to verify that's not the case just to find out that the dts GPIO interrupt edge configuration got only passed to the SPI driver :) So the level IRQ changes I did earlier to the dts file did nothing. The fix is simply to call devm_regmap_add_irq_chip() with irq_get_trigger_type(cpcap->spi->irq) to get the configured triggering passed properly from the dts. So I'll drop the genirq/regmap_irq related hacks and resend just the minimal MFD fixes. Similar misconfiguration may be the root cause for other drivers too.. Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Wed, 22 Mar 2017, Tony Lindgren wrote: > At least Motorola CPCAP PMIC needs it's device interrupts re-read > until there are no more interrupts. Otherwise the PMIC interrupt to > the SoC will eventually stop toggling. This seems to be a bug in the > CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC > with pending CPCAP interrupts. > > Let's allow handling issues like this by introducing a flag for > handle_reread and by splitting regmap_irq_thread() into two separate > functions for regmap_read_irq_status() and regmap_irq_handle_pending(). > > Cc: Charles Keepax> Cc: Lee Jones > Cc: Marcel Partap > Cc: Michael Scott > Tested-by: Sebastian Reichel > Signed-off-by: Tony Lindgren > --- > drivers/base/regmap/regmap-irq.c | 77 > +++- > include/linux/regmap.h | 2 ++ > 2 files changed, 55 insertions(+), 24 deletions(-) Looks like this is required for the MFD patches. I'm removing them for now. > diff --git a/drivers/base/regmap/regmap-irq.c > b/drivers/base/regmap/regmap-irq.c > --- a/drivers/base/regmap/regmap-irq.c > +++ b/drivers/base/regmap/regmap-irq.c > @@ -259,27 +259,11 @@ static const struct irq_chip regmap_irq_chip = { > .irq_set_wake = regmap_irq_set_wake, > }; > > -static irqreturn_t regmap_irq_thread(int irq, void *d) > +static int regmap_read_irq_status(struct regmap_irq_chip_data *data) > { > - struct regmap_irq_chip_data *data = d; > const struct regmap_irq_chip *chip = data->chip; > struct regmap *map = data->map; > int ret, i; > - bool handled = false; > - u32 reg; > - > - if (chip->handle_pre_irq) > - chip->handle_pre_irq(chip->irq_drv_data); > - > - if (chip->runtime_pm) { > - ret = pm_runtime_get_sync(map->dev); > - if (ret < 0) { > - dev_err(map->dev, "IRQ thread failed to resume: %d\n", > - ret); > - pm_runtime_put(map->dev); > - goto exit; > - } > - } > > /* >* Read in the statuses, using a single bulk read if possible > @@ -299,7 +283,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) > if (ret != 0) { > dev_err(map->dev, "Failed to read IRQ status: %d\n", > ret); > - goto exit; > + return ret; > } > > for (i = 0; i < data->chip->num_regs; i++) { > @@ -315,7 +299,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) > break; > default: > BUG(); > - goto exit; > + return ret; > } > } > > @@ -330,13 +314,21 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) > dev_err(map->dev, > "Failed to read IRQ status: %d\n", > ret); > - if (chip->runtime_pm) > - pm_runtime_put(map->dev); > - goto exit; > + return ret; > } > } > } > > + return 0; > +} > + > +static int regmap_irq_handle_pending(struct regmap_irq_chip_data *data) > +{ > + const struct regmap_irq_chip *chip = data->chip; > + struct regmap *map = data->map; > + int ret, i, handled = 0; > + u32 reg; > + > /* >* Ignore masked IRQs and ack if we need to; we ack early so >* there is no race between handling and acknowleding the > @@ -361,14 +353,51 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) > if (data->status_buf[chip->irqs[i].reg_offset / >map->reg_stride] & chip->irqs[i].mask) { > handle_nested_irq(irq_find_mapping(data->domain, i)); > - handled = true; > + handled++; > + } > + } > + > + return handled; > +} > + > +static irqreturn_t regmap_irq_thread(int irq, void *d) > +{ > + struct regmap_irq_chip_data *data = d; > + const struct regmap_irq_chip *chip = data->chip; > + struct regmap *map = data->map; > + int ret, handled = 0; > + > + if (chip->handle_pre_irq) > + chip->handle_pre_irq(chip->irq_drv_data); > + > + if (chip->runtime_pm) { > + ret = pm_runtime_get_sync(map->dev); > + if (ret < 0) { > + dev_err(map->dev, "IRQ thread failed to resume: %d\n", > + ret); > + goto out_runtime_put; > } >
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Wed, 22 Mar 2017, Tony Lindgren wrote: > At least Motorola CPCAP PMIC needs it's device interrupts re-read > until there are no more interrupts. Otherwise the PMIC interrupt to > the SoC will eventually stop toggling. This seems to be a bug in the > CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC > with pending CPCAP interrupts. > > Let's allow handling issues like this by introducing a flag for > handle_reread and by splitting regmap_irq_thread() into two separate > functions for regmap_read_irq_status() and regmap_irq_handle_pending(). > > Cc: Charles Keepax > Cc: Lee Jones > Cc: Marcel Partap > Cc: Michael Scott > Tested-by: Sebastian Reichel > Signed-off-by: Tony Lindgren > --- > drivers/base/regmap/regmap-irq.c | 77 > +++- > include/linux/regmap.h | 2 ++ > 2 files changed, 55 insertions(+), 24 deletions(-) Looks like this is required for the MFD patches. I'm removing them for now. > diff --git a/drivers/base/regmap/regmap-irq.c > b/drivers/base/regmap/regmap-irq.c > --- a/drivers/base/regmap/regmap-irq.c > +++ b/drivers/base/regmap/regmap-irq.c > @@ -259,27 +259,11 @@ static const struct irq_chip regmap_irq_chip = { > .irq_set_wake = regmap_irq_set_wake, > }; > > -static irqreturn_t regmap_irq_thread(int irq, void *d) > +static int regmap_read_irq_status(struct regmap_irq_chip_data *data) > { > - struct regmap_irq_chip_data *data = d; > const struct regmap_irq_chip *chip = data->chip; > struct regmap *map = data->map; > int ret, i; > - bool handled = false; > - u32 reg; > - > - if (chip->handle_pre_irq) > - chip->handle_pre_irq(chip->irq_drv_data); > - > - if (chip->runtime_pm) { > - ret = pm_runtime_get_sync(map->dev); > - if (ret < 0) { > - dev_err(map->dev, "IRQ thread failed to resume: %d\n", > - ret); > - pm_runtime_put(map->dev); > - goto exit; > - } > - } > > /* >* Read in the statuses, using a single bulk read if possible > @@ -299,7 +283,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) > if (ret != 0) { > dev_err(map->dev, "Failed to read IRQ status: %d\n", > ret); > - goto exit; > + return ret; > } > > for (i = 0; i < data->chip->num_regs; i++) { > @@ -315,7 +299,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) > break; > default: > BUG(); > - goto exit; > + return ret; > } > } > > @@ -330,13 +314,21 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) > dev_err(map->dev, > "Failed to read IRQ status: %d\n", > ret); > - if (chip->runtime_pm) > - pm_runtime_put(map->dev); > - goto exit; > + return ret; > } > } > } > > + return 0; > +} > + > +static int regmap_irq_handle_pending(struct regmap_irq_chip_data *data) > +{ > + const struct regmap_irq_chip *chip = data->chip; > + struct regmap *map = data->map; > + int ret, i, handled = 0; > + u32 reg; > + > /* >* Ignore masked IRQs and ack if we need to; we ack early so >* there is no race between handling and acknowleding the > @@ -361,14 +353,51 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) > if (data->status_buf[chip->irqs[i].reg_offset / >map->reg_stride] & chip->irqs[i].mask) { > handle_nested_irq(irq_find_mapping(data->domain, i)); > - handled = true; > + handled++; > + } > + } > + > + return handled; > +} > + > +static irqreturn_t regmap_irq_thread(int irq, void *d) > +{ > + struct regmap_irq_chip_data *data = d; > + const struct regmap_irq_chip *chip = data->chip; > + struct regmap *map = data->map; > + int ret, handled = 0; > + > + if (chip->handle_pre_irq) > + chip->handle_pre_irq(chip->irq_drv_data); > + > + if (chip->runtime_pm) { > + ret = pm_runtime_get_sync(map->dev); > + if (ret < 0) { > + dev_err(map->dev, "IRQ thread failed to resume: %d\n", > + ret); > + goto out_runtime_put; > } > } > > + do { > + ret = regmap_read_irq_status(data); > + if (ret) > + goto
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Mark Brown[170328 09:51]: > On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote: > > * Mark Brown [170328 08:21]: > > > > Right, my thinking here is that by pushing into genirq we minimise the > > > need even further since it'll also be available to drivers not using > > > regmap-irq. > > > > > like, handle until we get IRQ_NONE? :) > > > > Well, that's what the per driver emulation does so... yeah. Probably > > > with an upper limit on the number of times we do that. > > > OK let's first see how that would work. I'll send a patch > > for that. > > Thanks. Can you keep me on the CC please? It's something I keep > thinking about looking at myself. Sure will do, you'll get some shared flames on it :) Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Mark Brown [170328 09:51]: > On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote: > > * Mark Brown [170328 08:21]: > > > > Right, my thinking here is that by pushing into genirq we minimise the > > > need even further since it'll also be available to drivers not using > > > regmap-irq. > > > > > like, handle until we get IRQ_NONE? :) > > > > Well, that's what the per driver emulation does so... yeah. Probably > > > with an upper limit on the number of times we do that. > > > OK let's first see how that would work. I'll send a patch > > for that. > > Thanks. Can you keep me on the CC please? It's something I keep > thinking about looking at myself. Sure will do, you'll get some shared flames on it :) Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote: > * Mark Brown[170328 08:21]: > > Right, my thinking here is that by pushing into genirq we minimise the > > need even further since it'll also be available to drivers not using > > regmap-irq. > > > like, handle until we get IRQ_NONE? :) > > Well, that's what the per driver emulation does so... yeah. Probably > > with an upper limit on the number of times we do that. > OK let's first see how that would work. I'll send a patch > for that. Thanks. Can you keep me on the CC please? It's something I keep thinking about looking at myself. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote: > * Mark Brown [170328 08:21]: > > Right, my thinking here is that by pushing into genirq we minimise the > > need even further since it'll also be available to drivers not using > > regmap-irq. > > > like, handle until we get IRQ_NONE? :) > > Well, that's what the per driver emulation does so... yeah. Probably > > with an upper limit on the number of times we do that. > OK let's first see how that would work. I'll send a patch > for that. Thanks. Can you keep me on the CC please? It's something I keep thinking about looking at myself. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Mark Brown[170328 08:21]: > On Mon, Mar 27, 2017 at 05:36:48PM -0700, Tony Lindgren wrote: > > * Mark Brown [170327 10:52]: > > > > So, I see your use case but the fact is that as Charles observed this is > > > exactly the code used for emulating level triggered IRQs with edge > > > triggered interrupt controllers. This means someone is doubtless going > > > to end up using it for precisely that. This makes me uncomfortable, we > > > do have this open coded into various drivers already but this is more of > > > a core thing and it feels like this should be in genirq rather than > > > here... that said, looking at the code: > > > Yes.. But then again we might avoid piling up yet more driver > > specific hacks. I don't know what the genirq solution would look > > Right, my thinking here is that by pushing into genirq we minimise the > need even further since it'll also be available to drivers not using > regmap-irq. > > > like, handle until we get IRQ_NONE? :) > > Well, that's what the per driver emulation does so... yeah. Probably > with an upper limit on the number of times we do that. OK let's first see how that would work. I'll send a patch for that. > > > There's no protection against screaming interrupts here, I'd really like > > > to see that. Also some tracing of the number of times we spin. > > > Good idea. How about something like below where handle_reread checks > > for the total time spent in the thread loop with a large enough > > timeout? Or do you have some better ideas in mind? > > > I tested I can hit that warning with timeout set to much lower > > 10 ms with retries being 1 or 2 at that point. > > That looks sensible, yes. OK Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Mark Brown [170328 08:21]: > On Mon, Mar 27, 2017 at 05:36:48PM -0700, Tony Lindgren wrote: > > * Mark Brown [170327 10:52]: > > > > So, I see your use case but the fact is that as Charles observed this is > > > exactly the code used for emulating level triggered IRQs with edge > > > triggered interrupt controllers. This means someone is doubtless going > > > to end up using it for precisely that. This makes me uncomfortable, we > > > do have this open coded into various drivers already but this is more of > > > a core thing and it feels like this should be in genirq rather than > > > here... that said, looking at the code: > > > Yes.. But then again we might avoid piling up yet more driver > > specific hacks. I don't know what the genirq solution would look > > Right, my thinking here is that by pushing into genirq we minimise the > need even further since it'll also be available to drivers not using > regmap-irq. > > > like, handle until we get IRQ_NONE? :) > > Well, that's what the per driver emulation does so... yeah. Probably > with an upper limit on the number of times we do that. OK let's first see how that would work. I'll send a patch for that. > > > There's no protection against screaming interrupts here, I'd really like > > > to see that. Also some tracing of the number of times we spin. > > > Good idea. How about something like below where handle_reread checks > > for the total time spent in the thread loop with a large enough > > timeout? Or do you have some better ideas in mind? > > > I tested I can hit that warning with timeout set to much lower > > 10 ms with retries being 1 or 2 at that point. > > That looks sensible, yes. OK Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Mon, Mar 27, 2017 at 05:36:48PM -0700, Tony Lindgren wrote: > * Mark Brown[170327 10:52]: > > So, I see your use case but the fact is that as Charles observed this is > > exactly the code used for emulating level triggered IRQs with edge > > triggered interrupt controllers. This means someone is doubtless going > > to end up using it for precisely that. This makes me uncomfortable, we > > do have this open coded into various drivers already but this is more of > > a core thing and it feels like this should be in genirq rather than > > here... that said, looking at the code: > Yes.. But then again we might avoid piling up yet more driver > specific hacks. I don't know what the genirq solution would look Right, my thinking here is that by pushing into genirq we minimise the need even further since it'll also be available to drivers not using regmap-irq. > like, handle until we get IRQ_NONE? :) Well, that's what the per driver emulation does so... yeah. Probably with an upper limit on the number of times we do that. > > There's no protection against screaming interrupts here, I'd really like > > to see that. Also some tracing of the number of times we spin. > Good idea. How about something like below where handle_reread checks > for the total time spent in the thread loop with a large enough > timeout? Or do you have some better ideas in mind? > I tested I can hit that warning with timeout set to much lower > 10 ms with retries being 1 or 2 at that point. That looks sensible, yes. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Mon, Mar 27, 2017 at 05:36:48PM -0700, Tony Lindgren wrote: > * Mark Brown [170327 10:52]: > > So, I see your use case but the fact is that as Charles observed this is > > exactly the code used for emulating level triggered IRQs with edge > > triggered interrupt controllers. This means someone is doubtless going > > to end up using it for precisely that. This makes me uncomfortable, we > > do have this open coded into various drivers already but this is more of > > a core thing and it feels like this should be in genirq rather than > > here... that said, looking at the code: > Yes.. But then again we might avoid piling up yet more driver > specific hacks. I don't know what the genirq solution would look Right, my thinking here is that by pushing into genirq we minimise the need even further since it'll also be available to drivers not using regmap-irq. > like, handle until we get IRQ_NONE? :) Well, that's what the per driver emulation does so... yeah. Probably with an upper limit on the number of times we do that. > > There's no protection against screaming interrupts here, I'd really like > > to see that. Also some tracing of the number of times we spin. > Good idea. How about something like below where handle_reread checks > for the total time spent in the thread loop with a large enough > timeout? Or do you have some better ideas in mind? > I tested I can hit that warning with timeout set to much lower > 10 ms with retries being 1 or 2 at that point. That looks sensible, yes. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Mark Brown[170327 10:52]: > On Wed, Mar 22, 2017 at 10:10:49AM -0700, Tony Lindgren wrote: > > At least Motorola CPCAP PMIC needs it's device interrupts re-read > > until there are no more interrupts. Otherwise the PMIC interrupt to > > the SoC will eventually stop toggling. This seems to be a bug in the > > CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC > > with pending CPCAP interrupts. > > > Let's allow handling issues like this by introducing a flag for > > handle_reread and by splitting regmap_irq_thread() into two separate > > functions for regmap_read_irq_status() and regmap_irq_handle_pending(). > > So, I see your use case but the fact is that as Charles observed this is > exactly the code used for emulating level triggered IRQs with edge > triggered interrupt controllers. This means someone is doubtless going > to end up using it for precisely that. This makes me uncomfortable, we > do have this open coded into various drivers already but this is more of > a core thing and it feels like this should be in genirq rather than > here... that said, looking at the code: Yes.. But then again we might avoid piling up yet more driver specific hacks. I don't know what the genirq solution would look like, handle until we get IRQ_NONE? :) > > + do { > > + ret = regmap_read_irq_status(data); > > + if (ret) > > + goto out_runtime_put; > > + > > + ret = regmap_irq_handle_pending(data); > > + if (ret < 0) > > + goto out_runtime_put; > > + > > + if (!ret) > > + break; > > + > > + handled += ret; > > + } while (chip->handle_reread); > > There's no protection against screaming interrupts here, I'd really like > to see that. Also some tracing of the number of times we spin. Good idea. How about something like below where handle_reread checks for the total time spent in the thread loop with a large enough timeout? Or do you have some better ideas in mind? I tested I can hit that warning with timeout set to much lower 10 ms with retries being 1 or 2 at that point. Regards, Tony 8< --- >From tony Mon Sep 17 00:00:00 2001 From: Tony Lindgren Date: Mon, 27 Mar 2017 08:09:36 -0700 Subject: [PATCH] regmap: irq: Fix lost interrupts by introducing handle_reread At least Motorola CPCAP PMIC needs it's device interrupts re-read until there are no more interrupts. Otherwise the PMIC interrupt to the SoC will eventually stop toggling. This seems to be a bug in the CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC with pending CPCAP interrupts. Let's allow handling issues like this by introducing a flag for handle_reread and by splitting regmap_irq_thread() into two separate functions for regmap_read_irq_status() and regmap_irq_handle_pending(). Cc: Charles Keepax Cc: Lee Jones Cc: Marcel Partap Cc: Michael Scott Signed-off-by: Tony Lindgren --- drivers/base/regmap/regmap-irq.c | 97 ++-- include/linux/regmap.h | 2 + 2 files changed, 75 insertions(+), 24 deletions(-) diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -21,6 +21,8 @@ #include "internal.h" +#define REGMAP_IRQ_THREAD_MAX_MSECS3000 + struct regmap_irq_chip_data { struct mutex lock; struct irq_chip irq_chip; @@ -259,27 +261,11 @@ static const struct irq_chip regmap_irq_chip = { .irq_set_wake = regmap_irq_set_wake, }; -static irqreturn_t regmap_irq_thread(int irq, void *d) +static int regmap_read_irq_status(struct regmap_irq_chip_data *data) { - struct regmap_irq_chip_data *data = d; const struct regmap_irq_chip *chip = data->chip; struct regmap *map = data->map; int ret, i; - bool handled = false; - u32 reg; - - if (chip->handle_pre_irq) - chip->handle_pre_irq(chip->irq_drv_data); - - if (chip->runtime_pm) { - ret = pm_runtime_get_sync(map->dev); - if (ret < 0) { - dev_err(map->dev, "IRQ thread failed to resume: %d\n", - ret); - pm_runtime_put(map->dev); - goto exit; - } - } /* * Read in the statuses, using a single bulk read if possible @@ -299,7 +285,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) if (ret != 0) { dev_err(map->dev, "Failed to read IRQ status: %d\n", ret); - goto exit; + return ret; } for (i = 0; i <
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Mark Brown [170327 10:52]: > On Wed, Mar 22, 2017 at 10:10:49AM -0700, Tony Lindgren wrote: > > At least Motorola CPCAP PMIC needs it's device interrupts re-read > > until there are no more interrupts. Otherwise the PMIC interrupt to > > the SoC will eventually stop toggling. This seems to be a bug in the > > CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC > > with pending CPCAP interrupts. > > > Let's allow handling issues like this by introducing a flag for > > handle_reread and by splitting regmap_irq_thread() into two separate > > functions for regmap_read_irq_status() and regmap_irq_handle_pending(). > > So, I see your use case but the fact is that as Charles observed this is > exactly the code used for emulating level triggered IRQs with edge > triggered interrupt controllers. This means someone is doubtless going > to end up using it for precisely that. This makes me uncomfortable, we > do have this open coded into various drivers already but this is more of > a core thing and it feels like this should be in genirq rather than > here... that said, looking at the code: Yes.. But then again we might avoid piling up yet more driver specific hacks. I don't know what the genirq solution would look like, handle until we get IRQ_NONE? :) > > + do { > > + ret = regmap_read_irq_status(data); > > + if (ret) > > + goto out_runtime_put; > > + > > + ret = regmap_irq_handle_pending(data); > > + if (ret < 0) > > + goto out_runtime_put; > > + > > + if (!ret) > > + break; > > + > > + handled += ret; > > + } while (chip->handle_reread); > > There's no protection against screaming interrupts here, I'd really like > to see that. Also some tracing of the number of times we spin. Good idea. How about something like below where handle_reread checks for the total time spent in the thread loop with a large enough timeout? Or do you have some better ideas in mind? I tested I can hit that warning with timeout set to much lower 10 ms with retries being 1 or 2 at that point. Regards, Tony 8< --- >From tony Mon Sep 17 00:00:00 2001 From: Tony Lindgren Date: Mon, 27 Mar 2017 08:09:36 -0700 Subject: [PATCH] regmap: irq: Fix lost interrupts by introducing handle_reread At least Motorola CPCAP PMIC needs it's device interrupts re-read until there are no more interrupts. Otherwise the PMIC interrupt to the SoC will eventually stop toggling. This seems to be a bug in the CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC with pending CPCAP interrupts. Let's allow handling issues like this by introducing a flag for handle_reread and by splitting regmap_irq_thread() into two separate functions for regmap_read_irq_status() and regmap_irq_handle_pending(). Cc: Charles Keepax Cc: Lee Jones Cc: Marcel Partap Cc: Michael Scott Signed-off-by: Tony Lindgren --- drivers/base/regmap/regmap-irq.c | 97 ++-- include/linux/regmap.h | 2 + 2 files changed, 75 insertions(+), 24 deletions(-) diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -21,6 +21,8 @@ #include "internal.h" +#define REGMAP_IRQ_THREAD_MAX_MSECS3000 + struct regmap_irq_chip_data { struct mutex lock; struct irq_chip irq_chip; @@ -259,27 +261,11 @@ static const struct irq_chip regmap_irq_chip = { .irq_set_wake = regmap_irq_set_wake, }; -static irqreturn_t regmap_irq_thread(int irq, void *d) +static int regmap_read_irq_status(struct regmap_irq_chip_data *data) { - struct regmap_irq_chip_data *data = d; const struct regmap_irq_chip *chip = data->chip; struct regmap *map = data->map; int ret, i; - bool handled = false; - u32 reg; - - if (chip->handle_pre_irq) - chip->handle_pre_irq(chip->irq_drv_data); - - if (chip->runtime_pm) { - ret = pm_runtime_get_sync(map->dev); - if (ret < 0) { - dev_err(map->dev, "IRQ thread failed to resume: %d\n", - ret); - pm_runtime_put(map->dev); - goto exit; - } - } /* * Read in the statuses, using a single bulk read if possible @@ -299,7 +285,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) if (ret != 0) { dev_err(map->dev, "Failed to read IRQ status: %d\n", ret); - goto exit; + return ret; } for (i = 0; i < data->chip->num_regs; i++) { @@ -315,7 +301,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) break;
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Wed, Mar 22, 2017 at 10:10:49AM -0700, Tony Lindgren wrote: > At least Motorola CPCAP PMIC needs it's device interrupts re-read > until there are no more interrupts. Otherwise the PMIC interrupt to > the SoC will eventually stop toggling. This seems to be a bug in the > CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC > with pending CPCAP interrupts. > Let's allow handling issues like this by introducing a flag for > handle_reread and by splitting regmap_irq_thread() into two separate > functions for regmap_read_irq_status() and regmap_irq_handle_pending(). So, I see your use case but the fact is that as Charles observed this is exactly the code used for emulating level triggered IRQs with edge triggered interrupt controllers. This means someone is doubtless going to end up using it for precisely that. This makes me uncomfortable, we do have this open coded into various drivers already but this is more of a core thing and it feels like this should be in genirq rather than here... that said, looking at the code: > + do { > + ret = regmap_read_irq_status(data); > + if (ret) > + goto out_runtime_put; > + > + ret = regmap_irq_handle_pending(data); > + if (ret < 0) > + goto out_runtime_put; > + > + if (!ret) > + break; > + > + handled += ret; > + } while (chip->handle_reread); There's no protection against screaming interrupts here, I'd really like to see that. Also some tracing of the number of times we spin. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Wed, Mar 22, 2017 at 10:10:49AM -0700, Tony Lindgren wrote: > At least Motorola CPCAP PMIC needs it's device interrupts re-read > until there are no more interrupts. Otherwise the PMIC interrupt to > the SoC will eventually stop toggling. This seems to be a bug in the > CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC > with pending CPCAP interrupts. > Let's allow handling issues like this by introducing a flag for > handle_reread and by splitting regmap_irq_thread() into two separate > functions for regmap_read_irq_status() and regmap_irq_handle_pending(). So, I see your use case but the fact is that as Charles observed this is exactly the code used for emulating level triggered IRQs with edge triggered interrupt controllers. This means someone is doubtless going to end up using it for precisely that. This makes me uncomfortable, we do have this open coded into various drivers already but this is more of a core thing and it feels like this should be in genirq rather than here... that said, looking at the code: > + do { > + ret = regmap_read_irq_status(data); > + if (ret) > + goto out_runtime_put; > + > + ret = regmap_irq_handle_pending(data); > + if (ret < 0) > + goto out_runtime_put; > + > + if (!ret) > + break; > + > + handled += ret; > + } while (chip->handle_reread); There's no protection against screaming interrupts here, I'd really like to see that. Also some tracing of the number of times we spin. signature.asc Description: PGP signature
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Tue, Mar 21, 2017 at 08:41:22AM -0700, Tony Lindgren wrote: > * Charles Keepax[170321 02:24]: > > On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote: > > > * Charles Keepax [170320 08:15]: > > > > On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > > That sounds a lot like a level triggered IRQ. If they are > > repeatedly reading the GPIO line until it returns to high to know > > they need to process more IRQs, that implies the line is staying > > low whilst IRQs need handling which is level triggered. > > Yeah.. Actually my description above is a bit wrong sorry. It seems > the GPIO line changes status too early in some cases meaning the > interrupts stop. So it's like a buggy implementation of level IRQ > that stops driving the GPIO interrupt line to the SoC in some cases > even with PMIC interrupts pending. So it seems like a bug in the > CPCAP PMIC. > > So the handling needs to be "read while CPCAP interrupts in the > registers even if the GPIO line to SoC has cleared stopped > signaling interrupts" :) > Ah ok thanks for taking the time to explain that. Yeah that sounds like the PMIC hardware is a bit buggy so you will presumably need something to support this. Thanks, Charles
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Tue, Mar 21, 2017 at 08:41:22AM -0700, Tony Lindgren wrote: > * Charles Keepax [170321 02:24]: > > On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote: > > > * Charles Keepax [170320 08:15]: > > > > On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > > That sounds a lot like a level triggered IRQ. If they are > > repeatedly reading the GPIO line until it returns to high to know > > they need to process more IRQs, that implies the line is staying > > low whilst IRQs need handling which is level triggered. > > Yeah.. Actually my description above is a bit wrong sorry. It seems > the GPIO line changes status too early in some cases meaning the > interrupts stop. So it's like a buggy implementation of level IRQ > that stops driving the GPIO interrupt line to the SoC in some cases > even with PMIC interrupts pending. So it seems like a bug in the > CPCAP PMIC. > > So the handling needs to be "read while CPCAP interrupts in the > registers even if the GPIO line to SoC has cleared stopped > signaling interrupts" :) > Ah ok thanks for taking the time to explain that. Yeah that sounds like the PMIC hardware is a bit buggy so you will presumably need something to support this. Thanks, Charles
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Charles Keepax[170321 02:24]: > On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote: > > * Charles Keepax [170320 08:15]: > > > On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > > > > At least Motorola CPCAP PMIC needs it's device interrupts re-read > > > > until there are no more interrupts. Otherwise the PMIC interrupt to > > > > the SoC will eventually stop toggling. > > > > > > > > Let's allow doing that by introducing a flag for handle_reread > > > > and by splitting regmap_irq_thread() into two separate functions > > > > for regmap_read_irq_status() and regmap_irq_handle_pending(). > > > > > > > > > > Is this actually a property of this hardware or is this just a > > > result of connecting a device that generates level IRQs to a host > > > that is expecting an edge triggered IRQ? > > > > Well the CPCAP PMIC interrupt is connected to a GPIO edge interrupt > > on the SoC in the Motorola v3.8 based kernel tree. But what the CPCAP > > PMIC interrupt handler does in the Motorola kernel is keep handling > > the CPCAP internal interrupts (and also keep reading the SoC GPIO > > line status!) until the GPIO line changes status. So yeah it's a > > property of the CPCAP PMIC hardware. > > > > That sounds a lot like a level triggered IRQ. If they are > repeatedly reading the GPIO line until it returns to high to know > they need to process more IRQs, that implies the line is staying > low whilst IRQs need handling which is level triggered. Yeah.. Actually my description above is a bit wrong sorry. It seems the GPIO line changes status too early in some cases meaning the interrupts stop. So it's like a buggy implementation of level IRQ that stops driving the GPIO interrupt line to the SoC in some cases even with PMIC interrupts pending. So it seems like a bug in the CPCAP PMIC. So the handling needs to be "read while CPCAP interrupts in the registers even if the GPIO line to SoC has cleared stopped signaling interrupts" :) > > Changing the GPIO interrupt to level makes no difference here. It's > > not clear why they had marked the CPCAP PMIC to SoC GPIO interrupt > > as edge though in the Motorola kernel. My guess is that some PMIC > > to SoC wake-up events are edge interrupts. So we have it set up as > > edge interrupt in the mainline kernel too. > > > > I guess my only comment here is are we sure this isn't a bug in > supporting level IRQs in the GPIO driver, of course it could be > that the SoC simply doesn't support level IRQs that is fairly > common as well. But I guess we should be clear in the commit > message if this is adding emulation of level triggered IRQs and > possible add some gating on type of IRQ requested. Yeah well the SoC variants in this case have been supporting edge and level interrupts with gpio-omap.c for quite some time. I doubt that's the issue here, there are not even that many interrupts coming from the CPCAP. And I've verified the same issue happens with both edge and level configured GPIOs. There is nothing pending on the GPIO line. There is one pending GPIO regression fix when using gpio-omap.c with gpio-ir-recv.c affecting edge interrupts, but I've also tested that fix and it makes no difference. Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Charles Keepax [170321 02:24]: > On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote: > > * Charles Keepax [170320 08:15]: > > > On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > > > > At least Motorola CPCAP PMIC needs it's device interrupts re-read > > > > until there are no more interrupts. Otherwise the PMIC interrupt to > > > > the SoC will eventually stop toggling. > > > > > > > > Let's allow doing that by introducing a flag for handle_reread > > > > and by splitting regmap_irq_thread() into two separate functions > > > > for regmap_read_irq_status() and regmap_irq_handle_pending(). > > > > > > > > > > Is this actually a property of this hardware or is this just a > > > result of connecting a device that generates level IRQs to a host > > > that is expecting an edge triggered IRQ? > > > > Well the CPCAP PMIC interrupt is connected to a GPIO edge interrupt > > on the SoC in the Motorola v3.8 based kernel tree. But what the CPCAP > > PMIC interrupt handler does in the Motorola kernel is keep handling > > the CPCAP internal interrupts (and also keep reading the SoC GPIO > > line status!) until the GPIO line changes status. So yeah it's a > > property of the CPCAP PMIC hardware. > > > > That sounds a lot like a level triggered IRQ. If they are > repeatedly reading the GPIO line until it returns to high to know > they need to process more IRQs, that implies the line is staying > low whilst IRQs need handling which is level triggered. Yeah.. Actually my description above is a bit wrong sorry. It seems the GPIO line changes status too early in some cases meaning the interrupts stop. So it's like a buggy implementation of level IRQ that stops driving the GPIO interrupt line to the SoC in some cases even with PMIC interrupts pending. So it seems like a bug in the CPCAP PMIC. So the handling needs to be "read while CPCAP interrupts in the registers even if the GPIO line to SoC has cleared stopped signaling interrupts" :) > > Changing the GPIO interrupt to level makes no difference here. It's > > not clear why they had marked the CPCAP PMIC to SoC GPIO interrupt > > as edge though in the Motorola kernel. My guess is that some PMIC > > to SoC wake-up events are edge interrupts. So we have it set up as > > edge interrupt in the mainline kernel too. > > > > I guess my only comment here is are we sure this isn't a bug in > supporting level IRQs in the GPIO driver, of course it could be > that the SoC simply doesn't support level IRQs that is fairly > common as well. But I guess we should be clear in the commit > message if this is adding emulation of level triggered IRQs and > possible add some gating on type of IRQ requested. Yeah well the SoC variants in this case have been supporting edge and level interrupts with gpio-omap.c for quite some time. I doubt that's the issue here, there are not even that many interrupts coming from the CPCAP. And I've verified the same issue happens with both edge and level configured GPIOs. There is nothing pending on the GPIO line. There is one pending GPIO regression fix when using gpio-omap.c with gpio-ir-recv.c affecting edge interrupts, but I've also tested that fix and it makes no difference. Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote: > * Charles Keepax[170320 08:15]: > > On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > > > At least Motorola CPCAP PMIC needs it's device interrupts re-read > > > until there are no more interrupts. Otherwise the PMIC interrupt to > > > the SoC will eventually stop toggling. > > > > > > Let's allow doing that by introducing a flag for handle_reread > > > and by splitting regmap_irq_thread() into two separate functions > > > for regmap_read_irq_status() and regmap_irq_handle_pending(). > > > > > > > Is this actually a property of this hardware or is this just a > > result of connecting a device that generates level IRQs to a host > > that is expecting an edge triggered IRQ? > > Well the CPCAP PMIC interrupt is connected to a GPIO edge interrupt > on the SoC in the Motorola v3.8 based kernel tree. But what the CPCAP > PMIC interrupt handler does in the Motorola kernel is keep handling > the CPCAP internal interrupts (and also keep reading the SoC GPIO > line status!) until the GPIO line changes status. So yeah it's a > property of the CPCAP PMIC hardware. > That sounds a lot like a level triggered IRQ. If they are repeatedly reading the GPIO line until it returns to high to know they need to process more IRQs, that implies the line is staying low whilst IRQs need handling which is level triggered. > Changing the GPIO interrupt to level makes no difference here. It's > not clear why they had marked the CPCAP PMIC to SoC GPIO interrupt > as edge though in the Motorola kernel. My guess is that some PMIC > to SoC wake-up events are edge interrupts. So we have it set up as > edge interrupt in the mainline kernel too. > I guess my only comment here is are we sure this isn't a bug in supporting level IRQs in the GPIO driver, of course it could be that the SoC simply doesn't support level IRQs that is fairly common as well. But I guess we should be clear in the commit message if this is adding emulation of level triggered IRQs and possible add some gating on type of IRQ requested. Thanks, Charles
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote: > * Charles Keepax [170320 08:15]: > > On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > > > At least Motorola CPCAP PMIC needs it's device interrupts re-read > > > until there are no more interrupts. Otherwise the PMIC interrupt to > > > the SoC will eventually stop toggling. > > > > > > Let's allow doing that by introducing a flag for handle_reread > > > and by splitting regmap_irq_thread() into two separate functions > > > for regmap_read_irq_status() and regmap_irq_handle_pending(). > > > > > > > Is this actually a property of this hardware or is this just a > > result of connecting a device that generates level IRQs to a host > > that is expecting an edge triggered IRQ? > > Well the CPCAP PMIC interrupt is connected to a GPIO edge interrupt > on the SoC in the Motorola v3.8 based kernel tree. But what the CPCAP > PMIC interrupt handler does in the Motorola kernel is keep handling > the CPCAP internal interrupts (and also keep reading the SoC GPIO > line status!) until the GPIO line changes status. So yeah it's a > property of the CPCAP PMIC hardware. > That sounds a lot like a level triggered IRQ. If they are repeatedly reading the GPIO line until it returns to high to know they need to process more IRQs, that implies the line is staying low whilst IRQs need handling which is level triggered. > Changing the GPIO interrupt to level makes no difference here. It's > not clear why they had marked the CPCAP PMIC to SoC GPIO interrupt > as edge though in the Motorola kernel. My guess is that some PMIC > to SoC wake-up events are edge interrupts. So we have it set up as > edge interrupt in the mainline kernel too. > I guess my only comment here is are we sure this isn't a bug in supporting level IRQs in the GPIO driver, of course it could be that the SoC simply doesn't support level IRQs that is fairly common as well. But I guess we should be clear in the commit message if this is adding emulation of level triggered IRQs and possible add some gating on type of IRQ requested. Thanks, Charles
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Charles Keepax[170320 08:15]: > On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > > At least Motorola CPCAP PMIC needs it's device interrupts re-read > > until there are no more interrupts. Otherwise the PMIC interrupt to > > the SoC will eventually stop toggling. > > > > Let's allow doing that by introducing a flag for handle_reread > > and by splitting regmap_irq_thread() into two separate functions > > for regmap_read_irq_status() and regmap_irq_handle_pending(). > > > > Is this actually a property of this hardware or is this just a > result of connecting a device that generates level IRQs to a host > that is expecting an edge triggered IRQ? Well the CPCAP PMIC interrupt is connected to a GPIO edge interrupt on the SoC in the Motorola v3.8 based kernel tree. But what the CPCAP PMIC interrupt handler does in the Motorola kernel is keep handling the CPCAP internal interrupts (and also keep reading the SoC GPIO line status!) until the GPIO line changes status. So yeah it's a property of the CPCAP PMIC hardware. Changing the GPIO interrupt to level makes no difference here. It's not clear why they had marked the CPCAP PMIC to SoC GPIO interrupt as edge though in the Motorola kernel. My guess is that some PMIC to SoC wake-up events are edge interrupts. So we have it set up as edge interrupt in the mainline kernel too. Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
* Charles Keepax [170320 08:15]: > On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > > At least Motorola CPCAP PMIC needs it's device interrupts re-read > > until there are no more interrupts. Otherwise the PMIC interrupt to > > the SoC will eventually stop toggling. > > > > Let's allow doing that by introducing a flag for handle_reread > > and by splitting regmap_irq_thread() into two separate functions > > for regmap_read_irq_status() and regmap_irq_handle_pending(). > > > > Is this actually a property of this hardware or is this just a > result of connecting a device that generates level IRQs to a host > that is expecting an edge triggered IRQ? Well the CPCAP PMIC interrupt is connected to a GPIO edge interrupt on the SoC in the Motorola v3.8 based kernel tree. But what the CPCAP PMIC interrupt handler does in the Motorola kernel is keep handling the CPCAP internal interrupts (and also keep reading the SoC GPIO line status!) until the GPIO line changes status. So yeah it's a property of the CPCAP PMIC hardware. Changing the GPIO interrupt to level makes no difference here. It's not clear why they had marked the CPCAP PMIC to SoC GPIO interrupt as edge though in the Motorola kernel. My guess is that some PMIC to SoC wake-up events are edge interrupts. So we have it set up as edge interrupt in the mainline kernel too. Regards, Tony
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > At least Motorola CPCAP PMIC needs it's device interrupts re-read > until there are no more interrupts. Otherwise the PMIC interrupt to > the SoC will eventually stop toggling. > > Let's allow doing that by introducing a flag for handle_reread > and by splitting regmap_irq_thread() into two separate functions > for regmap_read_irq_status() and regmap_irq_handle_pending(). > Is this actually a property of this hardware or is this just a result of connecting a device that generates level IRQs to a host that is expecting an edge triggered IRQ? Thanks, Charles
Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread
On Thu, Mar 16, 2017 at 05:36:30PM -0700, Tony Lindgren wrote: > At least Motorola CPCAP PMIC needs it's device interrupts re-read > until there are no more interrupts. Otherwise the PMIC interrupt to > the SoC will eventually stop toggling. > > Let's allow doing that by introducing a flag for handle_reread > and by splitting regmap_irq_thread() into two separate functions > for regmap_read_irq_status() and regmap_irq_handle_pending(). > Is this actually a property of this hardware or is this just a result of connecting a device that generates level IRQs to a host that is expecting an edge triggered IRQ? Thanks, Charles