[PATCH] hw/i2c/imx_i2c: Always set interrupt status bit if interrupt condition occurs

[PATCH] hw/i2c/imx_i2c: Always set interrupt status bit if interrupt condition occurs

[Bernhard Beschow]

According to the i.MX 8M Plus reference manual, the status flag I2C_I2SR[IIF]
continues to be set when an interrupt condition occurs even when I2C interrupts
are disabled (I2C_I2CR[IIEN] is clear). However, the device model only sets the
flag when I2C interrupts are enabled which causes U-Boot to loop forever. Fix
the device model by always setting the flag and let I2C_I2CR[IIEN] guard I2C
interrupts only.

Also remove the comment in the code since it merely stated the obvious and would
be outdated now.

Fixes: 20d0f9cf6a41 ("i.MX: Add I2C controller emulator")
cc: qemu-stable
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i2c/imx_i2c.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
index 91f84c2ad7..d26177c85d 100644
--- a/hw/i2c/imx_i2c.c
+++ b/hw/i2c/imx_i2c.c
@@ -79,13 +79,12 @@ static void imx_i2c_reset(DeviceState *dev)
 
 static inline void imx_i2c_raise_interrupt(IMXI2CState *s)
 {
-    /*
-     * raise an interrupt if the device is enabled and it is configured
-     * to generate some interrupts.
-     */
-    if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
+    if (imx_i2c_is_enabled(s)) {
         s->i2sr |= I2SR_IIF;
-        qemu_irq_raise(s->irq);
+
+        if (imx_i2c_interrupt_is_enabled(s)) {
+            qemu_irq_raise(s->irq);
+        }
     }
 }
 
-- 
2.49.0

Re: [PATCH] hw/i2c/imx_i2c: Always set interrupt status bit if interrupt condition occurs

[Corey Minyard]

On Wed, May 07, 2025 at 02:40:40PM +0200, Bernhard Beschow wrote:
> According to the i.MX 8M Plus reference manual, the status flag I2C_I2SR[IIF]
> continues to be set when an interrupt condition occurs even when I2C interrupts
> are disabled (I2C_I2CR[IIEN] is clear). However, the device model only sets the
> flag when I2C interrupts are enabled which causes U-Boot to loop forever. Fix
> the device model by always setting the flag and let I2C_I2CR[IIEN] guard I2C
> interrupts only.
> 
> Also remove the comment in the code since it merely stated the obvious and would
> be outdated now.

This looks good to me.  I can give you an:

Acked-by: Corey Minyard <cminyard@mvista.com>

or I can take it into my tree.

-corey

> 
> Fixes: 20d0f9cf6a41 ("i.MX: Add I2C controller emulator")
> cc: qemu-stable
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/i2c/imx_i2c.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
> index 91f84c2ad7..d26177c85d 100644
> --- a/hw/i2c/imx_i2c.c
> +++ b/hw/i2c/imx_i2c.c
> @@ -79,13 +79,12 @@ static void imx_i2c_reset(DeviceState *dev)
>  
>  static inline void imx_i2c_raise_interrupt(IMXI2CState *s)
>  {
> -    /*
> -     * raise an interrupt if the device is enabled and it is configured
> -     * to generate some interrupts.
> -     */
> -    if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
> +    if (imx_i2c_is_enabled(s)) {
>          s->i2sr |= I2SR_IIF;
> -        qemu_irq_raise(s->irq);
> +
> +        if (imx_i2c_interrupt_is_enabled(s)) {
> +            qemu_irq_raise(s->irq);
> +        }
>      }
>  }
>  
> -- 
> 2.49.0
> 
> 

Re: [PATCH] hw/i2c/imx_i2c: Always set interrupt status bit if interrupt condition occurs

[Philippe Mathieu-Daudé]

On 7/5/25 15:06, Corey Minyard wrote:
> On Wed, May 07, 2025 at 02:40:40PM +0200, Bernhard Beschow wrote:
>> According to the i.MX 8M Plus reference manual, the status flag I2C_I2SR[IIF]
>> continues to be set when an interrupt condition occurs even when I2C interrupts
>> are disabled (I2C_I2CR[IIEN] is clear). However, the device model only sets the
>> flag when I2C interrupts are enabled which causes U-Boot to loop forever. Fix
>> the device model by always setting the flag and let I2C_I2CR[IIEN] guard I2C
>> interrupts only.
>>
>> Also remove the comment in the code since it merely stated the obvious and would
>> be outdated now.
> 
> This looks good to me.  I can give you an:
> 
> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
> or I can take it into my tree.

I have to respin my hw-misc PR so I'll squeeze this.

Thanks!

Phil.

Re: [PATCH] hw/i2c/imx_i2c: Always set interrupt status bit if interrupt condition occurs

[Philippe Mathieu-Daudé]

On 8/5/25 09:14, Philippe Mathieu-Daudé wrote:
> On 7/5/25 15:06, Corey Minyard wrote:
>> On Wed, May 07, 2025 at 02:40:40PM +0200, Bernhard Beschow wrote:
>>> According to the i.MX 8M Plus reference manual, the status flag 
>>> I2C_I2SR[IIF]
>>> continues to be set when an interrupt condition occurs even when I2C 
>>> interrupts
>>> are disabled (I2C_I2CR[IIEN] is clear). However, the device model 
>>> only sets the
>>> flag when I2C interrupts are enabled which causes U-Boot to loop 
>>> forever. Fix
>>> the device model by always setting the flag and let I2C_I2CR[IIEN] 
>>> guard I2C
>>> interrupts only.
>>>
>>> Also remove the comment in the code since it merely stated the 
>>> obvious and would
>>> be outdated now.
>>
>> This looks good to me.  I can give you an:
>>
>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>
>> or I can take it into my tree.
> 
> I have to respin my hw-misc PR so I'll squeeze this.
> 
> Thanks!
> 
> Phil.

FWIW I noticed the patch subject is truncated to my default git-view
because it is over 72 chars. Since there is no enforcement on patch
subject / description lines length in checkpatch.pl I suppose nobody
really cares about that so I'll merge as is.

Re: [PATCH] hw/i2c/imx_i2c: Always set interrupt status bit if interrupt condition occurs

[Peter Maydell]

On Thu, 8 May 2025 at 11:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> FWIW I noticed the patch subject is truncated to my default git-view
> because it is over 72 chars. Since there is no enforcement on patch
> subject / description lines length in checkpatch.pl I suppose nobody
> really cares about that so I'll merge as is.

I treat subject length stuff as an "it would be nice to be
below this" thing -- I try to keep them from being overlong
lines. But sometimes you really can't express what the
patch is doing in that length limit, and in those cases
I favour having a slightly long subject line over having
one that doesn't say what the patch is doing.

In the description part, we should be keeping away from
overlength lines by having them wrap at 75 chars or
less, unless there's a strong reason to not wrap (usually
it's a URL or we're quoting an error message).

thanks
-- PMM