Re: [Qemu-arm] [Qemu-devel] [RFC PATCH 1/6] qdev: add a power and clock gating support

[Damien Hedde]

Hi Philippe,

On 07/27/2018 06:33 PM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
> 
> On 07/27/2018 11:37 AM, Damien Hedde wrote:
>> Add two boolean new fields _powered_ and _clocked_ to hold the gating
>> state. Also add methods to act on each gating change.
>> The power/clock gating is controlled by 2 functions *device_set_power* and
>> *device_set_clock*.
>>
>> Add a default behavior to do a device_reset at power-up.
>>
>> Signed-off-by: Damien Hedde <address@hidden>
>> ---
>>  include/hw/qdev-core.h | 30 ++++++++++++++++++++++++
>>  hw/core/qdev.c         | 52 ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 82 insertions(+)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index f1fd0f8736..659287e185 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -34,6 +34,7 @@ typedef void (*DeviceUnrealize)(DeviceState *dev, Error 
>> **errp);
>>  typedef void (*DeviceReset)(DeviceState *dev);
>>  typedef void (*BusRealize)(BusState *bus, Error **errp);
>>  typedef void (*BusUnrealize)(BusState *bus, Error **errp);
>> +typedef void (*DeviceGatingUpdate)(DeviceState *dev);
>>  
>>  struct VMStateDescription;
>>  
>> @@ -109,6 +110,8 @@ typedef struct DeviceClass {
>>      DeviceReset reset;
>>      DeviceRealize realize;
>>      DeviceUnrealize unrealize;
>> +    DeviceGatingUpdate power_update;
>> +    DeviceGatingUpdate clock_update;
> 
> Update(boolean) sounds weird to me.
> 
> Why not name this set_power() or power_set(), or even power_switch()?

You're right. power_switch (or switch_power) seems also better to me.

> 
>>  
>>      /* device state */
>>      const struct VMStateDescription *vmsd;
>> @@ -151,6 +154,9 @@ struct DeviceState {
>>      int num_child_bus;
>>      int instance_id_alias;
>>      int alias_required_for_version;
>> +
>> +    bool powered;
>> +    bool clocked;
>>  };
>>  
>>  struct DeviceListener {
>> @@ -404,6 +410,12 @@ void device_class_set_parent_realize(DeviceClass *dc,
>>  void device_class_set_parent_unrealize(DeviceClass *dc,
>>                                         DeviceUnrealize dev_unrealize,
>>                                         DeviceUnrealize *parent_unrealize);
>> +void device_class_set_parent_power_update(DeviceClass *dc,
>> +                                          DeviceGatingUpdate 
>> dev_power_update,
>> +                                          DeviceGatingUpdate 
>> *parent_power_update);
>> +void device_class_set_parent_clock_update(DeviceClass *dc,
>> +                                          DeviceGatingUpdate 
>> dev_clock_update,
>> +                                          DeviceGatingUpdate 
>> *parent_clock_update);
>>  
>>  const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
>>  
>> @@ -434,4 +446,22 @@ static inline bool qbus_is_hotpluggable(BusState *bus)
>>  void device_listener_register(DeviceListener *listener);
>>  void device_listener_unregister(DeviceListener *listener);
>>  
>> +/**
>> + * device_set_power:
>> + * Enable/Disable the power of a device
>> + *
>> + * @dev: device to update
>> + * @en: true to enable, false to disable
>> + */
>> +void device_set_power(DeviceState *dev, bool en);
> 
> Maybe en -> enabled?

ok.

> 
>> +
>> +/**
>> + * device_set_clock:
>> + * Enable/Disable the clock of a device
>> + *
>> + * @dev: device to update
>> + * @en: true to enable, false to disable
>> + */
>> +void device_set_clock(DeviceState *dev, bool en);
>> +
>>  #endif
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 529b82de18..bb6d36eab9 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -950,6 +950,8 @@ static void device_initfn(Object *obj)
>>  
>>      dev->instance_id_alias = -1;
>>      dev->realized = false;
>> +    dev->powered = true;
>> +    dev->clocked = true;
>>  
>>      object_property_add_bool(obj, "realized",
>>                               device_get_realized, device_set_realized, 
>> NULL);
>> @@ -1038,6 +1040,13 @@ static void device_unparent(Object *obj)
>>      }
>>  }
>>  
>> +static void device_power_update(DeviceState *dev)
>> +{
>> +    if (dev->powered) {
>> +        device_reset(dev);
> 
> Hmm now we call device_reset() two times on device creation?
> 
> So now this is hard/cold reset, ...

Yes it corresponds to cold_reset.
I did not want to modify current platform init behavior: powered/clocked
are initialized to true (See device_initfn above).
So a device is already powered at startup and device_power_update will
only be called if powering-down the device. With this implementation I
think device_reset is called only once.

> 
>> +    }
>> +}
>> +
>>  static void device_class_init(ObjectClass *class, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(class);
>> @@ -1052,6 +1061,8 @@ static void device_class_init(ObjectClass *class, void 
>> *data)
>>       */
>>      dc->hotpluggable = true;
>>      dc->user_creatable = true;
>> +
>> +    dc->power_update = device_power_update;
>>  }
>>  
>>  void device_class_set_parent_reset(DeviceClass *dc,
>> @@ -1078,6 +1089,22 @@ void device_class_set_parent_unrealize(DeviceClass 
>> *dc,
>>      dc->unrealize = dev_unrealize;
>>  }
>>  
>> +void device_class_set_parent_power_update(DeviceClass *dc,
>> +                                       DeviceGatingUpdate dev_power_update,
>> +                                       DeviceGatingUpdate 
>> *parent_power_update)
>> +{
>> +    *parent_power_update = dc->power_update;
>> +    dc->power_update = dev_power_update;
>> +}
>> +
>> +void device_class_set_parent_clock_update(DeviceClass *dc,
>> +                                       DeviceGatingUpdate dev_clock_update,
>> +                                       DeviceGatingUpdate 
>> *parent_clock_update)
>> +{
>> +    *parent_clock_update = dc->clock_update;
>> +    dc->clock_update = dev_clock_update;
>> +}
>> +
>>  void device_reset(DeviceState *dev)
>>  {
> 
> ... and this is soft/hot reset.
> 
> All devices currently use this as hard reset.
> 
> Should we rename this? At least we should add comments about it.

I think this function could stay hard/cold reset.
Right now there is no soft/hot reset (and no default/generic way to
trigger such a reset if I'm not mistaken). If a device need different
implementation between external cold/hot reset, it have to be handled
someway. But maybe it is beyond the scope of theses patches since
power-gating will only trigger cold reset ?

Anyway, I will add comments about this to make things clear.

> 
>>      DeviceClass *klass = DEVICE_GET_CLASS(dev);
>> @@ -1087,6 +1114,31 @@ void device_reset(DeviceState *dev)
>>      }
>>  }
>>  
>> +void device_set_power(DeviceState *dev, bool en)
>> +{
>> +    DeviceClass *klass = DEVICE_GET_CLASS(dev);
>> +
> 
> /* TODO: trace events */

I will add theses.

> 
>> +    if (en != dev->powered) {
>> +        dev->powered = en;
>> +        if (klass->power_update) {
>> +            klass->power_update(dev);
>> +        }
>> +    }
>> +}
>> +
>> +void device_set_clock(DeviceState *dev, bool en)
>> +{
>> +    DeviceClass *klass = DEVICE_GET_CLASS(dev);
>> +
> 
> /* TODO: trace events */>>> +    if (en != dev->clocked) {
>> +        dev->clocked = en;
>> +        if (klass->clock_update) {
>> +            klass->clock_update(dev);
>> +        }
>> +    }
>> +}
>> +
>> +
>>  Object *qdev_get_machine(void)
>>  {
>>      static Object *dev;
>>
> This looks promising :)
> 
> Regards,
> 
> Phil.
>