Re: [Qemu-devel] [PATCH v3 08/33] Add function to control reset with gpio inputs

[Damien Hedde <damien.hedde@greensocs.com>]

On 8/7/19 5:18 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> It adds the possibility to add 2 gpios to control the warm and cold reset.
>> With theses ios, the reset can be maintained during some time.
>> Each io is associated with a state to detect level changes.
>>
>> Vmstate subsections are also added to the existsing device_reset
>> subsection.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>  hw/core/qdev-vmstate.c | 15 ++++++++++
>>  hw/core/qdev.c         | 65 ++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/qdev-core.h | 57 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 137 insertions(+)
>>
>> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
>> index 24f8465c61..72f84c6cee 100644
>> --- a/hw/core/qdev-vmstate.c
>> +++ b/hw/core/qdev-vmstate.c
>> @@ -24,10 +24,23 @@ static int device_vmstate_reset_post_load(void *opaque, int version_id)
>>  {
>>      DeviceState *dev = (DeviceState *) opaque;
>>      BusState *bus;
>> +    uint32_t io_count = 0;
>> +
>>      QLIST_FOREACH(bus, &dev->child_bus, sibling) {
>>          bus->resetting = dev->resetting;
>>          bus->reset_is_cold = dev->reset_is_cold;
>>      }
>> +
>> +    if (dev->cold_reset_input.state) {
>> +        io_count += 1;
>> +    }
>> +    if (dev->warm_reset_input.state) {
>> +        io_count += 1;
>> +    }
>> +    /* ensure resetting count is coherent with io states */
>> +    if (dev->resetting < io_count) {
>> +        return -1;
>> +    }
>>      return 0;
>>  }
>>
>> @@ -40,6 +53,8 @@ const struct VMStateDescription device_vmstate_reset = {
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32(resetting, DeviceState),
>>          VMSTATE_BOOL(reset_is_cold, DeviceState),
>> +        VMSTATE_BOOL(cold_reset_input.state, DeviceState),
>> +        VMSTATE_BOOL(warm_reset_input.state, DeviceState),
> 
> If we're just adding these fields to this VMStateDescription
> then this patch should come earlier in the series than the
> patch where we create and start using the fields. Otherwise
> there's a migration compat break between a QEMU just
> before this patch and a QEMU with it. I think the simplest
> fix is to put this patch before patches 6/7 and have a note
> in the commit message that this functionality can't be used
> until after the patch which adds the migration support.

Independently of the compat break you mention, maybe it is better to
have 'conditional' subsection for each input ?

> 
>>          VMSTATE_END_OF_LIST()
>>      },
>>  };
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 88387d3743..11a4de55ea 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -450,6 +450,67 @@ void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
>>      qdev_init_gpio_in_named(dev, handler, NULL, n);
>>  }
>>
>> +static DeviceResetInputState *device_get_reset_input_state(DeviceState *dev,
>> +                                                            bool cold)
>> +{
>> +    return cold ? &dev->cold_reset_input : &dev->warm_reset_input;
>> +}
>> +
>> +static void device_reset_handler(DeviceState *dev, bool cold, bool level)
>> +{
>> +    DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
>> +
>> +    if (dris->type == DEVICE_RESET_ACTIVE_LOW) {
>> +        level = !level;
>> +    }
>> +
>> +    if (dris->state == level) {
>> +        /* io state has not changed */
>> +        return;
>> +    }
>> +
>> +    dris->state = level;
>> +
>> +    if (level) {
>> +        resettable_assert_reset(OBJECT(dev), cold);
>> +    } else {
>> +        resettable_deassert_reset(OBJECT(dev));
>> +    }
>> +}
>> +
>> +static void device_cold_reset_handler(void *opaque, int n, int level)
>> +{
>> +    device_reset_handler((DeviceState *) opaque, true, level);
>> +}
>> +
>> +static void device_warm_reset_handler(void *opaque, int n, int level)
>> +{
>> +    device_reset_handler((DeviceState *) opaque, false, level);
>> +}
>> +
>> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
>> +                                   bool cold, DeviceResetActiveType type)
>> +{
>> +    DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
>> +    qemu_irq_handler handler;
>> +
>> +    switch (type) {
>> +    case DEVICE_RESET_ACTIVE_LOW:
>> +    case DEVICE_RESET_ACTIVE_HIGH:
>> +        break;
>> +    default:
>> +        assert(false);
>> +        break;
> 
> The usual way to write this is
>     g_assert_not_reached();
> (and no following 'break').
> 
> 
> But the whole switch statement seems to be a complicated way
> of writing
>    assert(type == DEVICE_RESET_ACTIVE_LOW || type == DEVICE_RESET_ACTIVE_HIGH);
> 
>> +    }
>> +
>> +    assert(!dris->exists);
>> +    dris->exists = true;
>> +    dris->type = type;
>> +
>> +    handler = cold ? device_cold_reset_handler : device_warm_reset_handler;
>> +    qdev_init_gpio_in_named(dev, handler, name, 1);
>> +}
> 
> thanks
> -- PMM
>