Hi all,
Here are few fixes to make CPCAP PMIC interrupts work reliably when
used with multiple drivers. While working on the ADC, charger and
USB PHY drivers I noticed that the PMIC interrupt to the SoC would
eventually stop working.
All these can wait for v4.12 merge window as these issues don't show
up currently. Two of the patches are also harmless currently, so I've
kept them in this series to avoid pointless merge conflicts.
Regards,
Tony
Tony Lindgren (4):
regmap: irq: Fix lost interrupts by introducing handle_reread
mfd: cpcap: Use handle_reread flag for interrupts
mfd: cpcap: Use ack_invert interrupts
mfd: cpcap: Fix bad use of IRQ sense register
drivers/base/regmap/regmap-irq.c | 77 ++++++++++++++++++++++++++++------------
drivers/mfd/motorola-cpcap.c | 7 +++-
include/linux/regmap.h | 2 ++
3 files changed, 62 insertions(+), 24 deletions(-)
--
2.11.1
On CPCAP we need to keep reading interrupts until there are no
more interrupts. Otherwise the PMIC interrupt to the SoC will at
some point stop toggling. This seems to happen because new CPCAP
device interrupts show up while we're handling.
Cc: Lee Jones <[email protected]>
Cc: Marcel Partap <[email protected]>
Cc: Michael Scott <[email protected]>
Cc: Sebastian Reichel <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
drivers/mfd/motorola-cpcap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -96,6 +96,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
.ack_base = CPCAP_REG_MI1,
.mask_base = CPCAP_REG_MIM1,
.use_ack = true,
+ .handle_reread = true,
},
{
.name = "cpcap-m2",
@@ -104,6 +105,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
.ack_base = CPCAP_REG_MI2,
.mask_base = CPCAP_REG_MIM2,
.use_ack = true,
+ .handle_reread = true,
},
{
.name = "cpcap1-4",
@@ -113,6 +115,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
.mask_base = CPCAP_REG_INTM1,
.type_base = CPCAP_REG_INTS1,
.use_ack = true,
+ .handle_reread = true,
},
};
--
2.11.1
The cpcap INTS registers are for getting the value of the line,
not for configuring the type.
Cc: Lee Jones <[email protected]>
Cc: Marcel Partap <[email protected]>
Cc: Michael Scott <[email protected]>
Cc: Sebastian Reichel <[email protected]>
Signed-off-by: Tony Lindgrne <[email protected]>
---
drivers/mfd/motorola-cpcap.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -115,7 +115,6 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
.status_base = CPCAP_REG_INT1,
.ack_base = CPCAP_REG_INT1,
.mask_base = CPCAP_REG_INTM1,
- .type_base = CPCAP_REG_INTS1,
.use_ack = true,
.handle_reread = true,
.ack_invert = true,
--
2.11.1
We should use ack_invert as the int_read_and_clear() in the Motorola
kernel tree does "ireg_val & ~mreg_val" before writing to the mask
register.
Cc: Lee Jones <[email protected]>
Cc: Marcel Partap <[email protected]>
Cc: Michael Scott <[email protected]>
Cc: Sebastian Reichel <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
drivers/mfd/motorola-cpcap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -97,6 +97,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
.mask_base = CPCAP_REG_MIM1,
.use_ack = true,
.handle_reread = true,
+ .ack_invert = true,
},
{
.name = "cpcap-m2",
@@ -106,6 +107,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
.mask_base = CPCAP_REG_MIM2,
.use_ack = true,
.handle_reread = true,
+ .ack_invert = true,
},
{
.name = "cpcap1-4",
@@ -116,6 +118,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
.type_base = CPCAP_REG_INTS1,
.use_ack = true,
.handle_reread = true,
+ .ack_invert = true,
},
};
--
2.11.1
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().
Cc: Lee Jones <[email protected]>
Cc: Marcel Partap <[email protected]>
Cc: Michael Scott <[email protected]>
Cc: Sebastian Reichel <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
drivers/base/regmap/regmap-irq.c | 77 ++++++++++++++++++++++++++++------------
include/linux/regmap.h | 2 ++
2 files changed, 56 insertions(+), 23 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
@@ -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,10 +353,49 @@ 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);
+ pm_runtime_put(map->dev);
+ goto exit;
+ }
+ }
+
+ 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);
+
+out_runtime_put:
if (chip->runtime_pm)
pm_runtime_put(map->dev);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -897,6 +897,7 @@ struct regmap_irq {
* @ack_invert: Inverted ack register: cleared bits for ack.
* @wake_invert: Inverted wake register: cleared bits are wake enabled.
* @type_invert: Invert the type flags.
+ * @handle_reread: Read interrupt status until no more interrupts are seen.
* @runtime_pm: Hold a runtime PM lock on the device when accessing it.
*
* @num_regs: Number of registers in each control bank.
@@ -934,6 +935,7 @@ struct regmap_irq_chip {
bool wake_invert:1;
bool runtime_pm:1;
bool type_invert:1;
+ bool handle_reread:1;
int num_regs;
--
2.11.1
On Thu, Mar 16, 2017 at 05:36:33PM -0700, Tony Lindgren wrote:
> The cpcap INTS registers are for getting the value of the line,
> not for configuring the type.
>
> Cc: Lee Jones <[email protected]>
> Cc: Marcel Partap <[email protected]>
> Cc: Michael Scott <[email protected]>
> Cc: Sebastian Reichel <[email protected]>
Reviewed-By: Sebastian Reichel <[email protected]>
> Signed-off-by: Tony Lindgrne <[email protected]>
https://lwn.net/Articles/717281/ ;)
-- Sebastian
> ---
> drivers/mfd/motorola-cpcap.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> --- a/drivers/mfd/motorola-cpcap.c
> +++ b/drivers/mfd/motorola-cpcap.c
> @@ -115,7 +115,6 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
> .status_base = CPCAP_REG_INT1,
> .ack_base = CPCAP_REG_INT1,
> .mask_base = CPCAP_REG_INTM1,
> - .type_base = CPCAP_REG_INTS1,
> .use_ack = true,
> .handle_reread = true,
> .ack_invert = true,
> --
> 2.11.1
* Sebastian Reichel <[email protected]> [170318 19:03]:
> On Thu, Mar 16, 2017 at 05:36:33PM -0700, Tony Lindgren wrote:
> > The cpcap INTS registers are for getting the value of the line,
> > not for configuring the type.
> >
> > Cc: Lee Jones <[email protected]>
> > Cc: Marcel Partap <[email protected]>
> > Cc: Michael Scott <[email protected]>
> > Cc: Sebastian Reichel <[email protected]>
>
> Reviewed-By: Sebastian Reichel <[email protected]>
>
> > Signed-off-by: Tony Lindgrne <[email protected]>
>
> https://lwn.net/Articles/717281/ ;)
Nah, just a time to clean-up the keyboard again!
Tony
On Thu, 16 Mar 2017, Tony Lindgren wrote:
> The cpcap INTS registers are for getting the value of the line,
> not for configuring the type.
>
> Cc: Lee Jones <[email protected]>
> Cc: Marcel Partap <[email protected]>
> Cc: Michael Scott <[email protected]>
> Cc: Sebastian Reichel <[email protected]>
> Signed-off-by: Tony Lindgrne <[email protected]>
Please resubmit with acquired Acks and your name spelt correctly.
Acked-by: Lee Jones <[email protected]>
> ---
> drivers/mfd/motorola-cpcap.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> --- a/drivers/mfd/motorola-cpcap.c
> +++ b/drivers/mfd/motorola-cpcap.c
> @@ -115,7 +115,6 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
> .status_base = CPCAP_REG_INT1,
> .ack_base = CPCAP_REG_INT1,
> .mask_base = CPCAP_REG_INTM1,
> - .type_base = CPCAP_REG_INTS1,
> .use_ack = true,
> .handle_reread = true,
> .ack_invert = true,
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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
* Charles Keepax <[email protected]> [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
On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote:
> * Charles Keepax <[email protected]> [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
* Charles Keepax <[email protected]> [170321 02:24]:
> On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote:
> > * Charles Keepax <[email protected]> [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
On Tue, Mar 21, 2017 at 08:41:22AM -0700, Tony Lindgren wrote:
> * Charles Keepax <[email protected]> [170321 02:24]:
> > On Mon, Mar 20, 2017 at 08:33:42AM -0700, Tony Lindgren wrote:
> > > * Charles Keepax <[email protected]> [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
Hi,
On Thu, Mar 16, 2017 at 05:36:29PM -0700, Tony Lindgren wrote:
> Here are few fixes to make CPCAP PMIC interrupts work reliably when
> used with multiple drivers. While working on the ADC, charger and
> USB PHY drivers I noticed that the PMIC interrupt to the SoC would
> eventually stop working.
>
> All these can wait for v4.12 merge window as these issues don't show
> up currently. Two of the patches are also harmless currently, so I've
> kept them in this series to avoid pointless merge conflicts.
The series is
Tested-by: Sebastian Reichel <[email protected]>
-- Sebastian