[PATCH] hw/i2c/mpc_i2c.c: Fix mmio region size

[PATCH] hw/i2c/mpc_i2c.c: Fix mmio region size

[BALATON Zoltan]

The last register of this device is at offset 0x14 occupying 8 bits so
to cover it the mmio region needs to be 0x15 bytes long. Also correct
the name of the field storing this register value to match the
register name.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/i2c/mpc_i2c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
index cb051a520f..06d4ce7d68 100644
--- a/hw/i2c/mpc_i2c.c
+++ b/hw/i2c/mpc_i2c.c
@@ -82,7 +82,7 @@ struct MPCI2CState {
     uint8_t cr;
     uint8_t sr;
     uint8_t dr;
-    uint8_t dfssr;
+    uint8_t dfsrr;
 };
 
 static bool mpc_i2c_is_enabled(MPCI2CState *s)
@@ -293,7 +293,7 @@ static void mpc_i2c_write(void *opaque, hwaddr addr,
         }
         break;
     case MPC_I2C_DFSRR:
-        s->dfssr = value;
+        s->dfsrr = value;
         break;
     default:
         DPRINTF("ERROR: Bad write addr 0x%x\n", (unsigned int)addr);
@@ -319,7 +319,7 @@ static const VMStateDescription mpc_i2c_vmstate = {
         VMSTATE_UINT8(cr, MPCI2CState),
         VMSTATE_UINT8(sr, MPCI2CState),
         VMSTATE_UINT8(dr, MPCI2CState),
-        VMSTATE_UINT8(dfssr, MPCI2CState),
+        VMSTATE_UINT8(dfsrr, MPCI2CState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -329,7 +329,7 @@ static void mpc_i2c_realize(DeviceState *dev, Error **errp)
     MPCI2CState  *i2c = MPC_I2C(dev);
     sysbus_init_irq(SYS_BUS_DEVICE(dev), &i2c->irq);
     memory_region_init_io(&i2c->iomem, OBJECT(i2c), &i2c_ops, i2c,
-                          "mpc-i2c", 0x14);
+                          "mpc-i2c", 0x15);
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &i2c->iomem);
     i2c->bus = i2c_init_bus(dev, "i2c");
 }
-- 
2.30.9

Re: [PATCH] hw/i2c/mpc_i2c.c: Fix mmio region size

[Philippe Mathieu-Daudé]

+Amit & Andrew

On 22/7/24 00:55, BALATON Zoltan wrote:
> The last register of this device is at offset 0x14 occupying 8 bits so
> to cover it the mmio region needs to be 0x15 bytes long. Also correct
> the name of the field storing this register value to match the
> register name.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/i2c/mpc_i2c.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)


> @@ -329,7 +329,7 @@ static void mpc_i2c_realize(DeviceState *dev, Error **errp)
>       MPCI2CState  *i2c = MPC_I2C(dev);
>       sysbus_init_irq(SYS_BUS_DEVICE(dev), &i2c->irq);
>       memory_region_init_io(&i2c->iomem, OBJECT(i2c), &i2c_ops, i2c,
> -                          "mpc-i2c", 0x14);
> +                          "mpc-i2c", 0x15);

Personally I'm not a big fan of non-pow2 regions, so I'd have picked
0x20 or 0x100 where the 2nd i2c controller starts. Amit, what do you
think?

Anyhow,

Fixes: 7abb479c7a ("PPC: E500: Add FSL I2C controller")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &i2c->iomem);
>       i2c->bus = i2c_init_bus(dev, "i2c");
>   }

Re: [PATCH] hw/i2c/mpc_i2c.c: Fix mmio region size

[BALATON Zoltan]

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

On Mon, 22 Jul 2024, Philippe Mathieu-Daudé wrote:
> +Amit & Andrew
>
> On 22/7/24 00:55, BALATON Zoltan wrote:
>> The last register of this device is at offset 0x14 occupying 8 bits so
>> to cover it the mmio region needs to be 0x15 bytes long. Also correct
>> the name of the field storing this register value to match the
>> register name.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/i2c/mpc_i2c.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>
>
>> @@ -329,7 +329,7 @@ static void mpc_i2c_realize(DeviceState *dev, Error 
>> **errp)
>>       MPCI2CState  *i2c = MPC_I2C(dev);
>>       sysbus_init_irq(SYS_BUS_DEVICE(dev), &i2c->irq);
>>       memory_region_init_io(&i2c->iomem, OBJECT(i2c), &i2c_ops, i2c,
>> -                          "mpc-i2c", 0x14);
>> +                          "mpc-i2c", 0x15);
>
> Personally I'm not a big fan of non-pow2 regions, so I'd have picked
> 0x20 or 0x100 where the 2nd i2c controller starts. Amit, what do you

Coverung unused areas isn't a good idea because that would omit invalid 
read/write warnings when those are enabled so it's harder to catch 
unimplemented registers that way. So 0x100 is definitely overkill, 0x20 
could be acceptable as other regs are also covering some unused area after 
them but is also unnecessary when we can cover exactly the area where the 
register is.

Regards,
BALATON Zoltan

> think?
>
> Anyhow,
>
> Fixes: 7abb479c7a ("PPC: E500: Add FSL I2C controller")
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>>       sysbus_init_mmio(SYS_BUS_DEVICE(dev), &i2c->iomem);
>>       i2c->bus = i2c_init_bus(dev, "i2c");
>>   }
>
>

Re: [PATCH] hw/i2c/mpc_i2c.c: Fix mmio region size

[Philippe Mathieu-Daudé]

On 22/7/24 12:16, BALATON Zoltan wrote:
> On Mon, 22 Jul 2024, Philippe Mathieu-Daudé wrote:
>> +Amit & Andrew
>>
>> On 22/7/24 00:55, BALATON Zoltan wrote:
>>> The last register of this device is at offset 0x14 occupying 8 bits so
>>> to cover it the mmio region needs to be 0x15 bytes long. Also correct
>>> the name of the field storing this register value to match the
>>> register name.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/i2c/mpc_i2c.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>>
>>> @@ -329,7 +329,7 @@ static void mpc_i2c_realize(DeviceState *dev, 
>>> Error **errp)
>>>       MPCI2CState  *i2c = MPC_I2C(dev);
>>>       sysbus_init_irq(SYS_BUS_DEVICE(dev), &i2c->irq);
>>>       memory_region_init_io(&i2c->iomem, OBJECT(i2c), &i2c_ops, i2c,
>>> -                          "mpc-i2c", 0x14);
>>> +                          "mpc-i2c", 0x15);
>>
>> Personally I'm not a big fan of non-pow2 regions, so I'd have picked
>> 0x20 or 0x100 where the 2nd i2c controller starts. Amit, what do you
> 
> Coverung unused areas isn't a good idea because that would omit invalid 
> read/write warnings when those are enabled so it's harder to catch 
> unimplemented registers that way. So 0x100 is definitely overkill, 0x20 
> could be acceptable as other regs are also covering some unused area 
> after them but is also unnecessary when we can cover exactly the area 
> where the register is.

Yeah. I'm queuing this patch via hw-misc, thanks.