diff mbox series

[2/4] hw/arm/stellaris: Convert I2C controller to Resettable interface

Message ID 20240130190317.25692-3-philmd@linaro.org
State New
Headers show
Series hw/arm/stellaris: QOM/QDev cleanups | expand

Commit Message

Philippe Mathieu-Daudé Jan. 30, 2024, 7:03 p.m. UTC 
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/stellaris.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Peter Maydell Feb. 1, 2024, 4:24 p.m. UTC | #1 
On Tue, 30 Jan 2024 at 19:03, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/stellaris.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index afbc83f1e6..284b95005f 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -607,8 +607,11 @@ static void stellaris_i2c_write(void *opaque, hwaddr offset,
>      stellaris_i2c_update(s);
>  }
>
> -static void stellaris_i2c_reset(stellaris_i2c_state *s)
> +static void stellaris_i2c_reset_exit(Object *obj)
>  {
> +    stellaris_i2c_state *s = STELLARIS_I2C(obj);
> +
> +    /* ??? For now we only implement the master interface.  */
>      if (s->mcs & STELLARIS_I2C_MCS_BUSBSY)
>          i2c_end_transfer(s->bus);
>
> @@ -658,8 +661,6 @@ static void stellaris_i2c_init(Object *obj)
>      memory_region_init_io(&s->iomem, obj, &stellaris_i2c_ops, s,
>                            "i2c", 0x1000);
>      sysbus_init_mmio(sbd, &s->iomem);
> -    /* ??? For now we only implement the master interface.  */

I'm not 100%, but I think this comment is a general one,
not reset specific, so it should stay in the init function.

> -    stellaris_i2c_reset(s);
>  }

I think that the i2c_end_transfer() should be in the
"enter" phase, and the clearing of the state fields in
"hold", and then the stellaris_i2c_update() call in "exit".
Though usually we don't bother to do an update in reset
if it's just "device has reset and now its outbound IRQ
line is not set", so we could alternatively just delete that.

>  /* Analogue to Digital Converter.  This is only partially implemented,
> @@ -1382,7 +1383,9 @@ type_init(stellaris_machine_init)
>  static void stellaris_i2c_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
>
> +    rc->phases.exit = stellaris_i2c_reset_exit;
>      dc->vmsd = &vmstate_stellaris_i2c;
>  }

Also on the subject of resetting i2c controllers and buses:
https://patchew.org/QEMU/20240126005541.1839038-1-komlodi@google.com/
That patchset proposes that the bus should basically drop
any pending transactions on the floor at reset; it would
also be possible for the bus to do the "end any transfer
in progress", so the individual controller model doesn't
have to. No idea what the overall Right Thing is, so
let's not block this tidyup on figuring out general i2c reset
cleanups.

thanks
-- PMM
Philippe Mathieu-Daudé Feb. 13, 2024, 3:50 p.m. UTC | #2 
On 1/2/24 17:24, Peter Maydell wrote:
> On Tue, 30 Jan 2024 at 19:03, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/stellaris.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)


>> @@ -658,8 +661,6 @@ static void stellaris_i2c_init(Object *obj)
>>       memory_region_init_io(&s->iomem, obj, &stellaris_i2c_ops, s,
>>                             "i2c", 0x1000);
>>       sysbus_init_mmio(sbd, &s->iomem);
>> -    /* ??? For now we only implement the master interface.  */
> 
> I'm not 100%, but I think this comment is a general one,
> not reset specific, so it should stay in the init function.
> 
>> -    stellaris_i2c_reset(s);
>>   }
> 
> I think that the i2c_end_transfer() should be in the
> "enter" phase, and the clearing of the state fields in
> "hold", and then the stellaris_i2c_update() call in "exit".

Indeed.

> Though usually we don't bother to do an update in reset
> if it's just "device has reset and now its outbound IRQ
> line is not set", so we could alternatively just delete that.

I'll let that as an possible cleanup on top.

Thanks for the review,

Phil.
diff mbox series

Patch

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index afbc83f1e6..284b95005f 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -607,8 +607,11 @@  static void stellaris_i2c_write(void *opaque, hwaddr offset,
     stellaris_i2c_update(s);
 }
 
-static void stellaris_i2c_reset(stellaris_i2c_state *s)
+static void stellaris_i2c_reset_exit(Object *obj)
 {
+    stellaris_i2c_state *s = STELLARIS_I2C(obj);
+
+    /* ??? For now we only implement the master interface.  */
     if (s->mcs & STELLARIS_I2C_MCS_BUSBSY)
         i2c_end_transfer(s->bus);
 
@@ -658,8 +661,6 @@  static void stellaris_i2c_init(Object *obj)
     memory_region_init_io(&s->iomem, obj, &stellaris_i2c_ops, s,
                           "i2c", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
-    /* ??? For now we only implement the master interface.  */
-    stellaris_i2c_reset(s);
 }
 
 /* Analogue to Digital Converter.  This is only partially implemented,
@@ -1382,7 +1383,9 @@  type_init(stellaris_machine_init)
 static void stellaris_i2c_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
+    rc->phases.exit = stellaris_i2c_reset_exit;
     dc->vmsd = &vmstate_stellaris_i2c;
 }