[PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Prasad Kumpatla]

With the existing logic by using regmap_write() all the bits in
the register are being updated which is not expected. To update only the
interrupt raised bit and not tocuhing other bits, replace regmap_write()
with regmap_irq_update_bits().

This patch is to fix the issue observed in MBHC button press/release events.

Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
---
Changes Since V1:
-- Update commit message.

 drivers/base/regmap/regmap-irq.c | 52 ++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index d2656581a608..bb9d07960dd0 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -184,16 +184,18 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 
 			/* some chips ack by write 0 */
 			if (d->chip->ack_invert)
-				ret = regmap_write(map, reg, ~d->mask_buf[i]);
+				ret = regmap_irq_update_bits(d, reg,
+						d->mask_buf[i], 0x00);
 			else
-				ret = regmap_write(map, reg, d->mask_buf[i]);
+				ret = regmap_irq_update_bits(d, reg,
+						d->mask_buf[i], d->mask_buf[i]);
 			if (d->chip->clear_ack) {
 				if (d->chip->ack_invert && !ret)
-					ret = regmap_write(map, reg,
-							   d->mask_buf[i]);
+					ret = regmap_irq_update_bits(d, reg,
+						d->mask_buf[i], d->mask_buf[i]);
 				else if (!ret)
-					ret = regmap_write(map, reg,
-							   ~d->mask_buf[i]);
+					ret = regmap_irq_update_bits(d, reg,
+						d->mask_buf[i], 0x00);
 			}
 			if (ret != 0)
 				dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
@@ -549,18 +551,20 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 			reg = sub_irq_reg(data, data->chip->ack_base, i);
 
 			if (chip->ack_invert)
-				ret = regmap_write(map, reg,
-						~data->status_buf[i]);
+				ret = regmap_irq_update_bits(d, reg,
+						data->status_buf[i], 0x00);
 			else
-				ret = regmap_write(map, reg,
+				ret = regmap_irq_update_bits(d, reg,
+						data->status_buf[i],
 						data->status_buf[i]);
 			if (chip->clear_ack) {
 				if (chip->ack_invert && !ret)
-					ret = regmap_write(map, reg,
-							data->status_buf[i]);
+					ret = regmap_irq_update_bits(d, reg,
+						data->status_buf[i],
+						data->status_buf[i]);
 				else if (!ret)
-					ret = regmap_write(map, reg,
-							~data->status_buf[i]);
+					ret = regmap_irq_update_bits(d, reg,
+						data->status_buf[i], 0x00);
 			}
 			if (ret != 0)
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
@@ -810,20 +814,22 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) {
 			reg = sub_irq_reg(d, d->chip->ack_base, i);
 			if (chip->ack_invert)
-				ret = regmap_write(map, reg,
-					~(d->status_buf[i] & d->mask_buf[i]));
+				ret = regmap_irq_update_bits(d, reg,
+					(d->status_buf[i] & d->mask_buf[i]),
+					0x00);
 			else
-				ret = regmap_write(map, reg,
-					d->status_buf[i] & d->mask_buf[i]);
+				ret = regmap_irq_update_bits(d, reg,
+					(d->status_buf[i] & d->mask_buf[i]),
+					(d->status_buf[i] & d->mask_buf[i]));
 			if (chip->clear_ack) {
 				if (chip->ack_invert && !ret)
-					ret = regmap_write(map, reg,
-						(d->status_buf[i] &
-						 d->mask_buf[i]));
+					ret = regmap_irq_update_bits(d, reg,
+					(d->status_buf[i] & d->mask_buf[i]),
+					(d->status_buf[i] & d->mask_buf[i]));
 				else if (!ret)
-					ret = regmap_write(map, reg,
-						~(d->status_buf[i] &
-						  d->mask_buf[i]));
+					ret = regmap_irq_update_bits(d, reg,
+					(d->status_buf[i] & d->mask_buf[i]),
+					0x00);
 			}
 			if (ret != 0) {
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
-- 
2.17.1

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Mark Brown]

On Wed, 19 Jan 2022 19:59:53 +0530, Prasad Kumpatla wrote:
> With the existing logic by using regmap_write() all the bits in
> the register are being updated which is not expected. To update only the
> interrupt raised bit and not tocuhing other bits, replace regmap_write()
> with regmap_irq_update_bits().
> 
> This patch is to fix the issue observed in MBHC button press/release events.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-linus

Thanks!

[1/1] regmap-irq: Use regmap_irq_update_bits instead of regmap_write
      commit: 8419f8e559a78a3d1050dd8132ef04727e8881c1

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Marek Szyprowski]

Hi Prasad,

On 19.01.2022 15:29, Prasad Kumpatla wrote:
> With the existing logic by using regmap_write() all the bits in
> the register are being updated which is not expected. To update only the
> interrupt raised bit and not tocuhing other bits, replace regmap_write()
> with regmap_irq_update_bits().
>
> This patch is to fix the issue observed in MBHC button press/release events.
>
> Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>

There is something wrong with this patch. Since it landed in linux-next 
(20220204) I get an interrupt storm on two of my test devices:

1. ARM 32bit Exynos4412-based Trats2 ("wm8994-codec wm8994-codec: FIFO 
error" message)

2. ARM 64bit Exynos5433-based TM2e ("arizona spi1.0: Mixer dropped 
sample" message)

Definitely the interrupts are not acknowledged properly. Once I find 
some spare time, I will check it further which regmap configuration 
triggers the issue, but it is definitely related to this patch. 
Reverting it on top of current linux-next fixes the issue.

> ---
> Changes Since V1:
> -- Update commit message.
>
>   drivers/base/regmap/regmap-irq.c | 52 ++++++++++++++++++--------------
>   1 file changed, 29 insertions(+), 23 deletions(-)
>
> ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Charles Keepax]

On Tue, Feb 08, 2022 at 01:29:55PM +0100, Marek Szyprowski wrote:
> Hi Prasad,
> 
> On 19.01.2022 15:29, Prasad Kumpatla wrote:
> > With the existing logic by using regmap_write() all the bits in
> > the register are being updated which is not expected. To update only the
> > interrupt raised bit and not tocuhing other bits, replace regmap_write()
> > with regmap_irq_update_bits().
> >
> > This patch is to fix the issue observed in MBHC button press/release events.
> >
> > Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> > Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> 
> There is something wrong with this patch. Since it landed in linux-next 
> (20220204) I get an interrupt storm on two of my test devices:
> 
> 1. ARM 32bit Exynos4412-based Trats2 ("wm8994-codec wm8994-codec: FIFO 
> error" message)
> 
> 2. ARM 64bit Exynos5433-based TM2e ("arizona spi1.0: Mixer dropped 
> sample" message)
> 
> Definitely the interrupts are not acknowledged properly. Once I find 
> some spare time, I will check it further which regmap configuration 
> triggers the issue, but it is definitely related to this patch. 
> Reverting it on top of current linux-next fixes the issue.
> 

Yeah I was just looking at this patch it looks a bit weird. Like
most IRQ acks are write 1 to clear, so doing an update_bits seems
unlikely to work, as it will ack every pending interrupt. As the
whole idea of doing a regmap_write was to only write 1 to the bits
that need ACKed.

I suspect what is needed here is the inverted case wants an
update bits but the normal case needs to stay as a regmap_write.

Thanks,
Charles

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On Tue, Feb 08, 2022 at 01:29:55PM +0100, Marek Szyprowski wrote:
> Hi Prasad,
> 
> On 19.01.2022 15:29, Prasad Kumpatla wrote:
> > With the existing logic by using regmap_write() all the bits in
> > the register are being updated which is not expected. To update only the
> > interrupt raised bit and not tocuhing other bits, replace regmap_write()
> > with regmap_irq_update_bits().
> >
> > This patch is to fix the issue observed in MBHC button press/release events.
> >
> > Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> > Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> 
> There is something wrong with this patch. Since it landed in linux-next 
> (20220204) I get an interrupt storm on two of my test devices:

I'll just revert it for now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Charles Keepax]

On Tue, Feb 08, 2022 at 01:39:57PM +0000, Charles Keepax wrote:
> On Tue, Feb 08, 2022 at 01:29:55PM +0100, Marek Szyprowski wrote:
> > Hi Prasad,
> > 
> > On 19.01.2022 15:29, Prasad Kumpatla wrote:
> > > With the existing logic by using regmap_write() all the bits in
> > > the register are being updated which is not expected. To update only the
> > > interrupt raised bit and not tocuhing other bits, replace regmap_write()
> > > with regmap_irq_update_bits().
> > >
> > > This patch is to fix the issue observed in MBHC button press/release events.
> > >
> > > Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> > > Signed-off-by: Prasad Kumpatla <quic_pkumpatl@quicinc.com>
> > 
> > There is something wrong with this patch. Since it landed in linux-next 
> > (20220204) I get an interrupt storm on two of my test devices:
> > 
> > 1. ARM 32bit Exynos4412-based Trats2 ("wm8994-codec wm8994-codec: FIFO 
> > error" message)
> > 
> > 2. ARM 64bit Exynos5433-based TM2e ("arizona spi1.0: Mixer dropped 
> > sample" message)
> > 
> > Definitely the interrupts are not acknowledged properly. Once I find 
> > some spare time, I will check it further which regmap configuration 
> > triggers the issue, but it is definitely related to this patch. 
> > Reverting it on top of current linux-next fixes the issue.
> > 
> 
> Yeah I was just looking at this patch it looks a bit weird. Like
> most IRQ acks are write 1 to clear, so doing an update_bits seems
> unlikely to work, as it will ack every pending interrupt. As the
> whole idea of doing a regmap_write was to only write 1 to the bits
> that need ACKed.
> 
> I suspect what is needed here is the inverted case wants an
> update bits but the normal case needs to stay as a regmap_write.
> 

Apologies for the multiple emails, yeah looking at this I think
need some more information on how the hardware that patch was
addressing works. I don't quite understand what was wrong with
the old code even in the inverted case, the old code wrote a 1 to
every bit except the interrupt being cleared which gets a 0. This
feels like how I would have thought a write 0 to clear IRQ would
work, you don't want to clear any other bits so you write 1 to
them.

The update_bits is really problematic as even in the write 0 to
clear case, if a new interrupt asserts between the regmap_read
and regmap_write that make up the update_bits, you will clear that
new interrupt without ever noticing it.

Thanks,
Charles

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

On Tue, Feb 08, 2022 at 01:50:36PM +0000, Charles Keepax wrote:

> Apologies for the multiple emails, yeah looking at this I think
> need some more information on how the hardware that patch was
> addressing works. I don't quite understand what was wrong with
> the old code even in the inverted case, the old code wrote a 1 to
> every bit except the interrupt being cleared which gets a 0. This
> feels like how I would have thought a write 0 to clear IRQ would
> work, you don't want to clear any other bits so you write 1 to
> them.

> The update_bits is really problematic as even in the write 0 to
> clear case, if a new interrupt asserts between the regmap_read
> and regmap_write that make up the update_bits, you will clear that
> new interrupt without ever noticing it.

My understanding was that they'd mixed interrupt handling in as a
bitfield in another register.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Charles Keepax]

On Tue, Feb 08, 2022 at 01:56:20PM +0000, Mark Brown wrote:
> On Tue, Feb 08, 2022 at 01:50:36PM +0000, Charles Keepax wrote:
> > The update_bits is really problematic as even in the write 0 to
> > clear case, if a new interrupt asserts between the regmap_read
> > and regmap_write that make up the update_bits, you will clear that
> > new interrupt without ever noticing it.
> 
> My understanding was that they'd mixed interrupt handling in as a
> bitfield in another register.

Eek.. what a courageous choice. I guess that might work as
long as there is only a single IRQ status bit in the register,
if there are multiple bits this really needs more complex
handling, like you basically need the old behaviour for the
IRQ part of the register, and the new behaviour for the not
IRQ part of the register. So perhaps a new mask to denote which
bit of the register is being used for IRQ stuff?

Thanks,
Charles

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Tue, Feb 08, 2022 at 02:33:16PM +0000, Charles Keepax wrote:
> On Tue, Feb 08, 2022 at 01:56:20PM +0000, Mark Brown wrote:

> > My understanding was that they'd mixed interrupt handling in as a
> > bitfield in another register.

> Eek.. what a courageous choice. I guess that might work as
> long as there is only a single IRQ status bit in the register,
> if there are multiple bits this really needs more complex
> handling, like you basically need the old behaviour for the
> IRQ part of the register, and the new behaviour for the not
> IRQ part of the register. So perhaps a new mask to denote which
> bit of the register is being used for IRQ stuff?

Yeah, I think that's what I misread the code as doing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Prasad Kumpatla]

On 2/8/2022 5:59 PM, Marek Szyprowski wrote:

> There is something wrong with this patch. Since it landed in linux-next
> (20220204) I get an interrupt storm on two of my test devices:
>
> 1. ARM 32bit Exynos4412-based Trats2 ("wm8994-codec wm8994-codec: FIFO
> error" message)
>
> 2. ARM 64bit Exynos5433-based TM2e ("arizona spi1.0: Mixer dropped
> sample" message)
>
> Definitely the interrupts are not acknowledged properly. Once I find
> some spare time, I will check it further which regmap configuration
> triggers the issue, but it is definitely related to this patch.
> Reverting it on top of current linux-next fixes the issue.

Change is needed to handle the interrupt ack properly to clear the ack.

I observed that the regmap_irq_update_bits() writes the register only if 
it finds a difference b/w existing reg value to update reg value.

This may be causing the interrupt storm issue mentioned above.

Setting the mask_writeonly flag to 1, to force write the register may 
resolve the interrupt storm issue.

--Prasad

Re: [PATCH v2] regmap-irq: Use regmap_irq_update_bits instead of regmap_write

[Charles Keepax]

On Tue, Feb 15, 2022 at 03:06:24PM +0530, Prasad Kumpatla wrote:
> On 2/8/2022 8:11 PM, Mark Brown wrote:
> >On Tue, Feb 08, 2022 at 02:33:16PM +0000, Charles Keepax wrote:
> >>On Tue, Feb 08, 2022 at 01:56:20PM +0000, Mark Brown wrote:
> ISR = Interrupt status register
> ICR = Interrupt clear register
> 
> When the Invert_ack is
>  *
>    *FALSE*: For ICR,writing 1 will clear , writing 0 has no effect.
>  *
>    *TRUE* : For ICR, writing 0 will clear , writing 1 has no effect
> and clear_ack
> 
>  *
>    *TRUE* :  Some hardware doesn’t support auto clear for ICR,  Need
>    explicitly clear the ICR
>  *
>    *FALSE *:  No need to clear ICR, as HW will reset.
> 
> Consider the following scenario,  where Invert_ack is false and
> clear_ack is true.
> 
> Say Default ISR=0x00 & ICR=0x00 and ISR is triggered with 2
> interrupts making ISR = 0x11.
> 
> *Step1:* Say ISR is set 0x11 (store status_buff = ISR). ISR needs to
> be cleared with the help of ICR once the Interrupt is processed.
> 
> *Step 2:* Write ICR = 0x11 (status_buff), this will clear the ISR to 0x00.
> 
> *Step 3:* Issue - In the existing code, ICR is written with ICR =
> ~(status_buff) i.e ICR = 0xEE à This will block all the interrupts
> from raising except for interrupts 0 and 4.
> 

Ok yes I the issue here now. I hadn't quite realised there was
hardware where you actually had to manually clear the ACK bits.

> Code snippet for step 3
> *if*(chip->clear_ack) {
> *if*(chip->ack_invert && !ret)
> ret = regmap_write(map, reg,
> data->status_buf[i]);
> *else**if*(!ret)
> ret = regmap_write(map, reg,
> ~data->status_buf[i]);
> 
> Solution: Since ICR does not clear automatically, writing 0x00 into
> ICR explicitly will clear the ICR (ICR = 0x00) allowing all the
> interrupts to raise.
> 
> So I’m thinking to implement as below.
> if (chip->clear_ack) {
> if (chip->ack_invert && !ret)
> - ret = regmap_write(map, reg,
> - data->status_buf[i]);
> + ret = regmap_write(map, reg, 0xff);
> else if (!ret)
> - ret = regmap_write(map, reg,
> - ~data->status_buf[i]);
> + ret = regmap_write(map, reg, 0x00);
> }

I think this looks much safer, the values you are writing should
have no effect, other than clearing the ACKs you just set.

Thanks,
Charles