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 <damien.he...@greensocs.com>
>> ---
>> 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.
>
