[Qemu-devel] [RFC PATCH 0/6] Clock and power gating support

[Qemu-devel] [RFC PATCH 0/6] Clock and power gating support

[Damien Hedde]

This set of patches add support for power and clock gating in device objects.
It adds two booleans to the state, one for power state and one of clock state.

The state is controlled trough 2 functions, one for power and one for clock.
Two new methods *power_update* and *clock_update* is added to the device class
which are called on state change and can be overriden.

Default behavior is the following:
+ Device are initialized in powered and clocked state.
+ reset method is called when powering-up.

This set also implements this support in the cadence_uart device in the
xilinx_zynq platform as an example.

The 1st patch add the gating support to the base device object. The 2nd patch
add functions to act on a whole bus tree. The 3rd patch overrides the 
*power/clock_update* methods for sysbus devices to enable/disable the memory
regions according to the state.

The 4th patch updates the cadence_uart device and the 5th patch adds
uart clock gating support to the zynq clock controller *slcr*. The 6th patch
finally adds the support to *xilinx-zynq* machine.

This is an RFC as it remains open questions about the strategy to implement
this kind of support. Here's some remarks:

+ This set adds power and clock, some reset state could be added too. Although
  default behavior is tricky to implement because there is many kind of reset
  (eg syncronous/asynchronous).
+ I used methods which require to store links of controlled devices in
  clock/power controller. Using gpios or specifics child objects to
  achieve the same functionnaly is possible.
+ Power and clock state could be merged to a single 3-state (powered-off,
  unclocked, on) since clock does not really matters when powered-off.
+ Regarding migration, it's problematic to add the VMStateDescription in qdev
  so we let it to the specialization (eg in cadence_uart) to handle it. Is
  this a problem ? Anyway support of clock gating requires modification in
  specialization. It may not be necessary to store the gating states in each
  device, since it is stored already in the controller registers. But we would
  then have to set the gating state of devices in the controller's post_load
  which may occurs before or after devices VMState load: It seems tricky to
  ensure device state is correct at the end if gating update do side-effects.
+ theses patches add a simple vision (1 power, 1 clock). Devices can be more
  complex. For example, the zynq's uart has 2 independant clock domains:
  1 for the bus interface, 1 for uart operations. I'm not sure if we should
  keep 1, add the bus/device separation or do something more configurable on
  a per-specialization basis.

Feel free to comment,

Damien Hedde (6):
  qdev: add a power and clock gating support
  qdev: add power/clock gating control on bus tree
  sysbus: Specialize gating_update to enable/disable memory regions
  cadence_uart: add clock/power gating support
  zynq_slcr: add uart clock gating and soft reset support
  xilinx_zynq: add uart clock gating support

 include/hw/qdev-core.h |  50 ++++++++++++++++++++
 include/hw/sysbus.h    |   3 ++
 hw/arm/xilinx_zynq.c   |  20 +++++---
 hw/char/cadence_uart.c |  25 +++++++++-
 hw/core/qdev.c         | 103 +++++++++++++++++++++++++++++++++++++++++
 hw/core/sysbus.c       |  39 ++++++++++++++++
 hw/misc/zynq_slcr.c    |  63 +++++++++++++++++++++++++
 7 files changed, 296 insertions(+), 7 deletions(-)

-- 
2.18.0

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

[Damien Hedde]

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.hedde@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;
 
     /* 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);
+
+/**
+ * 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);
+    }
+}
+
 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)
 {
     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);
+
+    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);
+
+    if (en != dev->clocked) {
+        dev->clocked = en;
+        if (klass->clock_update) {
+            klass->clock_update(dev);
+        }
+    }
+}
+
+
 Object *qdev_get_machine(void)
 {
     static Object *dev;
-- 
2.18.0

[Qemu-devel] [RFC PATCH 2/6] qdev: add power/clock gating control on bus tree

[Damien Hedde]

Add functions [qdev|qbus]_set_[power|clock]_all(_fn).
Theses allow to control power and clock gating along a bus hierarchy.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 include/hw/qdev-core.h | 20 +++++++++++++++++
 hw/core/qdev.c         | 51 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 659287e185..607c367738 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -464,4 +464,24 @@ void device_set_power(DeviceState *dev, bool en);
  */
 void device_set_clock(DeviceState *dev, bool en);
 
+/**
+ * qdev/qbus_set_power_all(_fn)
+ * Enable/Disable the power of a tree starting
+ * at given device or bus
+ */
+void qdev_set_power_all(DeviceState *dev, bool en);
+void qdev_set_power_all_fn(void *opaque, bool en);
+void qbus_set_power_all(BusState *bus, bool en);
+void qbus_set_power_all_fn(void *opaque, bool en);
+
+/**
+ * qdev/qbus_set_clock_all(_fn)
+ * Enable/Disable the clock of a tree starting
+ * at given device or bus
+ */
+void qdev_set_clock_all(DeviceState *dev, bool en);
+void qdev_set_clock_all_fn(void *opaque, bool en);
+void qbus_set_clock_all(BusState *bus, bool en);
+void qbus_set_clock_all_fn(void *opaque, bool en);
+
 #endif
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index bb6d36eab9..24b90bd45f 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1138,6 +1138,57 @@ void device_set_clock(DeviceState *dev, bool en)
     }
 }
 
+static int qdev_set_power_one(DeviceState *dev, void *opaque)
+{
+    device_set_power(dev, *((bool *)opaque));
+    return 0;
+}
+
+void qdev_set_power_all(DeviceState *dev, bool en)
+{
+    qdev_walk_children(dev, NULL, NULL, qdev_set_power_one, NULL, &en);
+}
+
+void qdev_set_power_all_fn(void *opaque, bool en)
+{
+    qdev_set_power_all(DEVICE(opaque), en);
+}
+
+void qbus_set_power_all(BusState *bus, bool en)
+{
+    qbus_walk_children(bus, NULL, NULL, qdev_set_power_one, NULL, &en);
+}
+
+void qbus_set_power_all_fn(void *opaque, bool en)
+{
+    qbus_set_power_all(BUS(opaque), en);
+}
+
+static int qdev_set_clock_one(DeviceState *dev, void *opaque)
+{
+    device_set_clock(dev, *((bool *)opaque));
+    return 0;
+}
+
+void qdev_set_clock_all(DeviceState *dev, bool en)
+{
+    qdev_walk_children(dev, NULL, NULL, qdev_set_clock_one, NULL, &en);
+}
+
+void qdev_set_clock_all_fn(void *opaque, bool en)
+{
+    qdev_set_clock_all(DEVICE(opaque), en);
+}
+
+void qbus_set_clock_all(BusState *bus, bool en)
+{
+    qbus_walk_children(bus, NULL, NULL, qdev_set_clock_one, NULL, &en);
+}
+
+void qbus_set_clock_all_fn(void *opaque, bool en)
+{
+    qbus_set_clock_all(BUS(opaque), en);
+}
 
 Object *qdev_get_machine(void)
 {
-- 
2.18.0

[Qemu-devel] [RFC PATCH 3/6] sysbus: Specialize gating_update to enable/disable memory regions

[Damien Hedde]

The default methods are overriden to add the activation/deactivation
of the memory regions according to the gating state: Regions are
enabled only when powered and clocked.
As powering-up triggers a reset call, memory regions should
be reset in specialized sysbus devices.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 include/hw/sysbus.h |  3 +++
 hw/core/sysbus.c    | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 0b59a3b8d6..e17165e78f 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -59,6 +59,9 @@ typedef struct SysBusDeviceClass {
      */
     char *(*explicit_ofw_unit_address)(const SysBusDevice *dev);
     void (*connect_irq_notifier)(SysBusDevice *dev, qemu_irq irq);
+
+    DeviceGatingUpdate parent_power_update;
+    DeviceGatingUpdate parent_clock_update;
 } SysBusDeviceClass;
 
 struct SysBusDevice {
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 3c8e53b188..4a2dfbe907 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -325,6 +325,39 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
     return get_system_memory();
 }
 
+/*
+ * Action take on power or clock update.
+ */
+static void sysbus_device_gating_update(DeviceState *dev)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    int i;
+
+    for (i = 0;; i++) {
+        MemoryRegion *mr = sysbus_mmio_get_region(sbd, i);
+        if (!mr) {
+            break;
+        }
+        memory_region_set_enabled(mr, dev->powered && dev->clocked);
+    }
+}
+
+/*
+ * Action take on power update.
+ *
+ * Call parent method before doing local action.
+ * So that we override any action taken in parent method (eg if reset
+ * is called due to leaving OFF state)
+ */
+static void sysbus_device_power_update(DeviceState *dev)
+{
+    SysBusDeviceClass *sbdk = SYS_BUS_DEVICE_GET_CLASS(dev);
+
+    sbdk->parent_power_update(dev);
+
+    sysbus_device_gating_update(dev);
+}
+
 static void sysbus_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
@@ -341,6 +374,12 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data)
      * subclass needs to override it and set user_creatable=true.
      */
     k->user_creatable = false;
+
+    SysBusDeviceClass *sbdk = SYS_BUS_DEVICE_CLASS(klass);
+    device_class_set_parent_power_update(k,
+            sysbus_device_power_update, &sbdk->parent_power_update);
+    device_class_set_parent_clock_update(k,
+            sysbus_device_gating_update, &sbdk->parent_clock_update);
 }
 
 static const TypeInfo sysbus_device_type_info = {
-- 
2.18.0

[Qemu-devel] [RFC PATCH 4/6] cadence_uart: add clock/power gating support

[Damien Hedde]

Only discard input characters when unpowered/unclocked.
As it is a sysbus device, mmio are already disabled when unpowered
or unclocked.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/char/cadence_uart.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index fbdbd463bb..dd51d9a087 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -335,8 +335,14 @@ static void uart_write_tx_fifo(CadenceUARTState *s, const uint8_t *buf,
 static void uart_receive(void *opaque, const uint8_t *buf, int size)
 {
     CadenceUARTState *s = opaque;
+    DeviceState *dev = DEVICE(s);
     uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
 
+    /* ignore characters if unpowered or unclocked */
+    if (!dev->powered || !dev->clocked) {
+        return;
+    }
+
     if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
         uart_write_rx_fifo(opaque, buf, size);
     }
@@ -348,8 +354,14 @@ static void uart_receive(void *opaque, const uint8_t *buf, int size)
 static void uart_event(void *opaque, int event)
 {
     CadenceUARTState *s = opaque;
+    DeviceState *dev = DEVICE(s);
     uint8_t buf = '\0';
 
+    /* ignore event if we're unpowered or unclocked */
+    if (!dev->powered || !dev->clocked) {
+        return;
+    }
+
     if (event == CHR_EVENT_BREAK) {
         uart_write_rx_fifo(opaque, &buf, 1);
     }
@@ -516,10 +528,19 @@ static int cadence_uart_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static int cadence_uart_pre_load(void *opaque)
+{
+    DeviceState *s = opaque;
+    s->clocked = true;
+    s->powered = true;
+    return 0;
+}
+
 static const VMStateDescription vmstate_cadence_uart = {
     .name = "cadence_uart",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 2,
+    .pre_load = cadence_uart_pre_load,
     .post_load = cadence_uart_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
@@ -531,6 +552,8 @@ static const VMStateDescription vmstate_cadence_uart = {
         VMSTATE_UINT32(tx_count, CadenceUARTState),
         VMSTATE_UINT32(rx_wpos, CadenceUARTState),
         VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
+        VMSTATE_BOOL_V(powered, DeviceState, 3),
+        VMSTATE_BOOL_V(clocked, DeviceState, 3),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.18.0

[Qemu-devel] [RFC PATCH 5/6] zynq_slcr: add uart clock gating and soft reset support

[Damien Hedde]

Clock gating and reset of uart0 and uart1 is controlled by
UART_CLK_CTRL and UART_RST_CTRL.
Uart0 and uart1 links are kept in properties to allow taking action.

The CLKACT bit in UART_CLK_CTRL is used to driver the clock gating.

In order to implement the reset behavior, which can be hold: when
reset is asserted, device_reset is called on the uart and the clock
is also disabled until reset is deasserted.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/misc/zynq_slcr.c | 63 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index d6bdd027ef..55dd586066 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -168,6 +168,14 @@ enum {
 #define DDRIOB_LENGTH 14
 };
 
+enum {
+    UART_CLK_CTRL_CLKACT0 = 0x0001,
+    UART_CLK_CTRL_CLKACT1 = 0x0002,
+
+    UART_RST_CTRL_UART0_REF_RST = 0x04,
+    UART_RST_CTRL_UART1_REF_RST = 0x08,
+};
+
 #define ZYNQ_SLCR_MMIO_SIZE     0x1000
 #define ZYNQ_SLCR_NUM_REGS      (ZYNQ_SLCR_MMIO_SIZE / 4)
 
@@ -180,6 +188,9 @@ typedef struct ZynqSLCRState {
     MemoryRegion iomem;
 
     uint32_t regs[ZYNQ_SLCR_NUM_REGS];
+
+    DeviceState *uart0;
+    DeviceState *uart1;
 } ZynqSLCRState;
 
 static void zynq_slcr_reset(DeviceState *d)
@@ -355,6 +366,35 @@ static uint64_t zynq_slcr_read(void *opaque, hwaddr offset,
     return ret;
 }
 
+/*
+ * zynq_slcr_update_clock:
+ * Update a device clock state given its:
+ * + clock enable bit
+ * + reset asserted bit
+ * Since reset cannot be enabled and disabled, the clock is disabled when
+ * reset is asserted
+ */
+static void zynq_slcr_update_clock(DeviceState *dev, bool clken, bool rsten)
+{
+    if (dev) {
+        device_set_clock(dev, clken && !rsten);
+    }
+}
+
+/*
+ * zynq_slcr_update_reset:
+ * Update a device reset state given its:
+ * + reset asserted bit
+ * Also trigger a clock update
+ */
+static void zynq_slcr_update_reset(DeviceState *dev, bool clken, bool rsten)
+{
+    if (dev && rsten) {
+        device_reset(dev);
+    }
+    zynq_slcr_update_clock(dev, clken, rsten);
+}
+
 static void zynq_slcr_write(void *opaque, hwaddr offset,
                           uint64_t val, unsigned size)
 {
@@ -408,6 +448,22 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
             qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
         }
         break;
+    case UART_CLK_CTRL:
+        zynq_slcr_update_clock(s->uart0,
+                s->regs[UART_CLK_CTRL] & UART_CLK_CTRL_CLKACT0,
+                s->regs[UART_RST_CTRL] & UART_RST_CTRL_UART0_REF_RST);
+        zynq_slcr_update_clock(s->uart1,
+                s->regs[UART_CLK_CTRL] & UART_CLK_CTRL_CLKACT1,
+                s->regs[UART_RST_CTRL] & UART_RST_CTRL_UART1_REF_RST);
+        break;
+    case UART_RST_CTRL:
+        zynq_slcr_update_reset(s->uart0,
+                s->regs[UART_CLK_CTRL] & UART_CLK_CTRL_CLKACT0,
+                s->regs[UART_RST_CTRL] & UART_RST_CTRL_UART0_REF_RST);
+        zynq_slcr_update_reset(s->uart1,
+                s->regs[UART_CLK_CTRL] & UART_CLK_CTRL_CLKACT1,
+                s->regs[UART_RST_CTRL] & UART_RST_CTRL_UART1_REF_RST);
+        break;
     }
 }
 
@@ -436,12 +492,19 @@ static const VMStateDescription vmstate_zynq_slcr = {
     }
 };
 
+static Property zynq_slcr_properties[] = {
+    DEFINE_PROP_LINK("uart0", ZynqSLCRState, uart0, TYPE_DEVICE, DeviceState *),
+    DEFINE_PROP_LINK("uart1", ZynqSLCRState, uart1, TYPE_DEVICE, DeviceState *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void zynq_slcr_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_zynq_slcr;
     dc->reset = zynq_slcr_reset;
+    dc->props = zynq_slcr_properties;
 }
 
 static const TypeInfo zynq_slcr_info = {
-- 
2.18.0

[Qemu-devel] [RFC PATCH 6/6] xilinx_zynq: add uart clock gating support

[Damien Hedde]

Add the link between the clock controller _slcr_ and the two
uarts _uart0_ and _uart1_ so that the controller can do the gating.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/arm/xilinx_zynq.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index f1496d2927..2ca2dc32cf 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -166,7 +166,7 @@ static void zynq_init(MachineState *machine)
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
-    DeviceState *dev;
+    DeviceState *dev, *slcr;
     SysBusDevice *busdev;
     qemu_irq pic[64];
     int n;
@@ -212,9 +212,11 @@ static void zynq_init(MachineState *machine)
                           1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
                               0);
 
-    dev = qdev_create(NULL, "xilinx,zynq_slcr");
-    qdev_init_nofail(dev);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
+    /*
+     * Create slcr, initialization is completed below
+     * because we have some properties to set
+     */
+    slcr = qdev_create(NULL, "xilinx,zynq_slcr");
 
     dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
     qdev_prop_set_uint32(dev, "num-cpu", 1);
@@ -235,8 +237,10 @@ static void zynq_init(MachineState *machine)
     sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
     sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
 
-    cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hd(0));
-    cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+    dev = cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hd(0));
+    object_property_set_link(OBJECT(slcr), OBJECT(dev), "uart0", &error_abort);
+    dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+    object_property_set_link(OBJECT(slcr), OBJECT(dev), "uart1", &error_abort);
 
     sysbus_create_varargs("cadence_ttc", 0xF8001000,
             pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
@@ -246,6 +250,10 @@ static void zynq_init(MachineState *machine)
     gem_init(&nd_table[0], 0xE000B000, pic[54-IRQ_OFFSET]);
     gem_init(&nd_table[1], 0xE000C000, pic[77-IRQ_OFFSET]);
 
+    /* Complete the slcr initialization */
+    qdev_init_nofail(slcr);
+    sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
+
     for (n = 0; n < 2; n++) {
         int hci_irq = n ? 79 : 56;
         hwaddr hci_addr = n ? 0xE0101000 : 0xE0100000;
-- 
2.18.0

Re: [Qemu-devel] [RFC PATCH 0/6] Clock and power gating support

[Peter Maydell]

On 27 July 2018 at 15:37, Damien Hedde <damien.hedde@greensocs.com> wrote:
> This set of patches add support for power and clock gating in device objects.
> It adds two booleans to the state, one for power state and one of clock state.
>
> The state is controlled trough 2 functions, one for power and one for clock.
> Two new methods *power_update* and *clock_update* is added to the device class
> which are called on state change and can be overriden.

So, you folks at Greensocs had a series a couple of years ago to try
to add clocktree support (which included handling things like guests
configuring clock rates, some clocks being "downstream" of others, and
so on). It didn't make it into mainline, though it did get a fair amount
of design/code review. How does this approach fit into / compare with
that ?

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 0/6] Clock and power gating support

[KONRAD Frederic]

Le 07/27/2018 à 04:59 PM, Peter Maydell a écrit :
> On 27 July 2018 at 15:37, Damien Hedde <damien.hedde@greensocs.com> wrote:
>> This set of patches add support for power and clock gating in device objects.
>> It adds two booleans to the state, one for power state and one of clock state.
>>
>> The state is controlled trough 2 functions, one for power and one for clock.
>> Two new methods *power_update* and *clock_update* is added to the device class
>> which are called on state change and can be overriden.
> 
> So, you folks at Greensocs had a series a couple of years ago to try
> to add clocktree support (which included handling things like guests
> configuring clock rates, some clocks being "downstream" of others, and
> so on). It didn't make it into mainline, though it did get a fair amount
> of design/code review. How does this approach fit into / compare with
> that ?
> 
> thanks
> -- PMM
> 

Hi all,

I didn't have time to push a new version of the clock series and
I lost the track a little bit (was 13 months ago). Sorry for
that :(.. Would be still nice to have I think.

Cheers,
Fred

Re: [Qemu-devel] [Qemu-arm] [RFC PATCH 2/6] qdev: add power/clock gating control on bus tree

[Philippe Mathieu-Daudé]

Hi Damien,

On 07/27/2018 11:37 AM, Damien Hedde wrote:
> Add functions [qdev|qbus]_set_[power|clock]_all(_fn).
> Theses allow to control power and clock gating along a bus hierarchy.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  include/hw/qdev-core.h | 20 +++++++++++++++++
>  hw/core/qdev.c         | 51 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 659287e185..607c367738 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -464,4 +464,24 @@ void device_set_power(DeviceState *dev, bool en);
>   */
>  void device_set_clock(DeviceState *dev, bool en);
>  
> +/**
> + * qdev/qbus_set_power_all(_fn)
> + * Enable/Disable the power of a tree starting
> + * at given device or bus
> + */
> +void qdev_set_power_all(DeviceState *dev, bool en);
> +void qdev_set_power_all_fn(void *opaque, bool en);
> +void qbus_set_power_all(BusState *bus, bool en);
> +void qbus_set_power_all_fn(void *opaque, bool en);

I think we don't need the _fn() functions, most of the current codebase
directly use DEVICE(x) or BUS(x) to casts.

> +
> +/**
> + * qdev/qbus_set_clock_all(_fn)
> + * Enable/Disable the clock of a tree starting
> + * at given device or bus
> + */
> +void qdev_set_clock_all(DeviceState *dev, bool en);
> +void qdev_set_clock_all_fn(void *opaque, bool en);
> +void qbus_set_clock_all(BusState *bus, bool en);
> +void qbus_set_clock_all_fn(void *opaque, bool en);
> +
>  #endif
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index bb6d36eab9..24b90bd45f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1138,6 +1138,57 @@ void device_set_clock(DeviceState *dev, bool en)
>      }
>  }
>  
> +static int qdev_set_power_one(DeviceState *dev, void *opaque)
> +{
> +    device_set_power(dev, *((bool *)opaque));
> +    return 0;
> +}
> +
> +void qdev_set_power_all(DeviceState *dev, bool en)
> +{
> +    qdev_walk_children(dev, NULL, NULL, qdev_set_power_one, NULL, &en);
> +}
> +
> +void qdev_set_power_all_fn(void *opaque, bool en)
> +{
> +    qdev_set_power_all(DEVICE(opaque), en);
> +}
> +
> +void qbus_set_power_all(BusState *bus, bool en)
> +{
> +    qbus_walk_children(bus, NULL, NULL, qdev_set_power_one, NULL, &en);
> +}
> +
> +void qbus_set_power_all_fn(void *opaque, bool en)
> +{
> +    qbus_set_power_all(BUS(opaque), en);
> +}
> +
> +static int qdev_set_clock_one(DeviceState *dev, void *opaque)
> +{
> +    device_set_clock(dev, *((bool *)opaque));
> +    return 0;
> +}
> +
> +void qdev_set_clock_all(DeviceState *dev, bool en)
> +{
> +    qdev_walk_children(dev, NULL, NULL, qdev_set_clock_one, NULL, &en);
> +}
> +
> +void qdev_set_clock_all_fn(void *opaque, bool en)
> +{
> +    qdev_set_clock_all(DEVICE(opaque), en);
> +}
> +
> +void qbus_set_clock_all(BusState *bus, bool en)
> +{
> +    qbus_walk_children(bus, NULL, NULL, qdev_set_clock_one, NULL, &en);
> +}
> +
> +void qbus_set_clock_all_fn(void *opaque, bool en)
> +{
> +    qbus_set_clock_all(BUS(opaque), en);
> +}
>  
>  Object *qdev_get_machine(void)
>  {
> 

Re: [Qemu-devel] [RFC PATCH 3/6] sysbus: Specialize gating_update to enable/disable memory regions

[Philippe Mathieu-Daudé]

On 07/27/2018 11:37 AM, Damien Hedde wrote:
> The default methods are overriden to add the activation/deactivation
> of the memory regions according to the gating state: Regions are
> enabled only when powered and clocked.
> As powering-up triggers a reset call, memory regions should
> be reset in specialized sysbus devices.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  include/hw/sysbus.h |  3 +++
>  hw/core/sysbus.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index 0b59a3b8d6..e17165e78f 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -59,6 +59,9 @@ typedef struct SysBusDeviceClass {
>       */
>      char *(*explicit_ofw_unit_address)(const SysBusDevice *dev);
>      void (*connect_irq_notifier)(SysBusDevice *dev, qemu_irq irq);
> +
> +    DeviceGatingUpdate parent_power_update;
> +    DeviceGatingUpdate parent_clock_update;
>  } SysBusDeviceClass;
>  
>  struct SysBusDevice {
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 3c8e53b188..4a2dfbe907 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -325,6 +325,39 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
>      return get_system_memory();
>  }
>  
> +/*
> + * Action take on power or clock update.
> + */
> +static void sysbus_device_gating_update(DeviceState *dev)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    int i;
> +
> +    for (i = 0;; i++) {

i < QDEV_MAX_MMIO

> +        MemoryRegion *mr = sysbus_mmio_get_region(sbd, i);
> +        if (!mr) {
> +            break;
> +        }
> +        memory_region_set_enabled(mr, dev->powered && dev->clocked);
> +    }
> +}
> +
> +/*
> + * Action take on power update.
> + *
> + * Call parent method before doing local action.
> + * So that we override any action taken in parent method (eg if reset
> + * is called due to leaving OFF state)
> + */
> +static void sysbus_device_power_update(DeviceState *dev)
> +{
> +    SysBusDeviceClass *sbdk = SYS_BUS_DEVICE_GET_CLASS(dev);
> +
> +    sbdk->parent_power_update(dev);
> +
> +    sysbus_device_gating_update(dev);
> +}
> +
>  static void sysbus_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *k = DEVICE_CLASS(klass);
> @@ -341,6 +374,12 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data)
>       * subclass needs to override it and set user_creatable=true.
>       */
>      k->user_creatable = false;
> +
> +    SysBusDeviceClass *sbdk = SYS_BUS_DEVICE_CLASS(klass);
> +    device_class_set_parent_power_update(k,
> +            sysbus_device_power_update, &sbdk->parent_power_update);
> +    device_class_set_parent_clock_update(k,
> +            sysbus_device_gating_update, &sbdk->parent_clock_update);
>  }
>  
>  static const TypeInfo sysbus_device_type_info = {
> 

Re: [Qemu-devel] [RFC PATCH 4/6] cadence_uart: add clock/power gating support

[Philippe Mathieu-Daudé]

On 07/27/2018 11:37 AM, Damien Hedde wrote:
> Only discard input characters when unpowered/unclocked.
> As it is a sysbus device, mmio are already disabled when unpowered
> or unclocked.

This is common to all Chardev frontends (also Ethernet/CAN devices, see
NetClientInfo).

Maybe qemu_chr_fe_accept_input() can take a boolean, so when we disable
a device (power/clock), the char-fe code can discard characters for all
frontends?

> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  hw/char/cadence_uart.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index fbdbd463bb..dd51d9a087 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -335,8 +335,14 @@ static void uart_write_tx_fifo(CadenceUARTState *s, const uint8_t *buf,
>  static void uart_receive(void *opaque, const uint8_t *buf, int size)
>  {
>      CadenceUARTState *s = opaque;
> +    DeviceState *dev = DEVICE(s);
>      uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
>  
> +    /* ignore characters if unpowered or unclocked */
> +    if (!dev->powered || !dev->clocked) {
> +        return;
> +    }
> +
>      if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
>          uart_write_rx_fifo(opaque, buf, size);
>      }
> @@ -348,8 +354,14 @@ static void uart_receive(void *opaque, const uint8_t *buf, int size)
>  static void uart_event(void *opaque, int event)
>  {
>      CadenceUARTState *s = opaque;
> +    DeviceState *dev = DEVICE(s);
>      uint8_t buf = '\0';
>  
> +    /* ignore event if we're unpowered or unclocked */
> +    if (!dev->powered || !dev->clocked) {
> +        return;
> +    }
> +
>      if (event == CHR_EVENT_BREAK) {
>          uart_write_rx_fifo(opaque, &buf, 1);
>      }
> @@ -516,10 +528,19 @@ static int cadence_uart_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static int cadence_uart_pre_load(void *opaque)
> +{
> +    DeviceState *s = opaque;
> +    s->clocked = true;
> +    s->powered = true;
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_cadence_uart = {
>      .name = "cadence_uart",
> -    .version_id = 2,
> +    .version_id = 3,
>      .minimum_version_id = 2,
> +    .pre_load = cadence_uart_pre_load,
>      .post_load = cadence_uart_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
> @@ -531,6 +552,8 @@ static const VMStateDescription vmstate_cadence_uart = {
>          VMSTATE_UINT32(tx_count, CadenceUARTState),
>          VMSTATE_UINT32(rx_wpos, CadenceUARTState),
>          VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
> +        VMSTATE_BOOL_V(powered, DeviceState, 3),
> +        VMSTATE_BOOL_V(clocked, DeviceState, 3),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> 

Re: [Qemu-devel] [RFC PATCH 0/6] Clock and power gating support

[Mark Burton]

Hi Pete, sorry for the tardy reply, Im not in the office.


Your right  we shoujd have co-ordinated better the 2 patches, sorry for that.

Regarding this new patchset, we think there is a degree of complementary to the
clocktree one.
Here we simply add a couple of generic power states to a device. We
introduce
an interface to control power and clock gating on any devices, with sensible
generic behaviour (reset on power on, disabling memory regions when
asleep or
off). The device can specialize this to adopt specific behaviours.

It allows easy implementation of commonly found "system controller", "pin
controllers", "power management unit", or similar blocks in SoCs.
Specialization of devices can be implemented as-needed when a machine
requires
it.


Cheers
Mark

On 27 July 2018 17:22:30 KONRAD Frederic <frederic.konrad@adacore.com> wrote:

> Le 07/27/2018 à 04:59 PM, Peter Maydell a écrit :
>> On 27 July 2018 at 15:37, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>> This set of patches add support for power and clock gating in device objects.
>>> It adds two booleans to the state, one for power state and one of clock state.
>>>
>>> The state is controlled trough 2 functions, one for power and one for clock.
>>> Two new methods *power_update* and *clock_update* is added to the device class
>>> which are called on state change and can be overriden.
>>
>> So, you folks at Greensocs had a series a couple of years ago to try
>> to add clocktree support (which included handling things like guests
>> configuring clock rates, some clocks being "downstream" of others, and
>> so on). It didn't make it into mainline, though it did get a fair amount
>> of design/code review. How does this approach fit into / compare with
>> that ?
>>
>> thanks
>> -- PMM
>
> Hi all,
>
> I didn't have time to push a new version of the clock series and
> I lost the track a little bit (was 13 months ago). Sorry for
> that :(.. Would be still nice to have I think.
>
> Cheers,
> Fred

Re: [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 <damien.hedde@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.
> 

Re: [Qemu-devel] [Qemu-arm] [RFC PATCH 2/6] qdev: add power/clock gating control on bus tree

[Damien Hedde]

Hi,

On 07/27/2018 06:39 PM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
> 
> On 07/27/2018 11:37 AM, Damien Hedde wrote:
>> Add functions [qdev|qbus]_set_[power|clock]_all(_fn).
>> Theses allow to control power and clock gating along a bus hierarchy.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>  include/hw/qdev-core.h | 20 +++++++++++++++++
>>  hw/core/qdev.c         | 51 ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 71 insertions(+)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 659287e185..607c367738 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -464,4 +464,24 @@ void device_set_power(DeviceState *dev, bool en);
>>   */
>>  void device_set_clock(DeviceState *dev, bool en);
>>  
>> +/**
>> + * qdev/qbus_set_power_all(_fn)
>> + * Enable/Disable the power of a tree starting
>> + * at given device or bus
>> + */
>> +void qdev_set_power_all(DeviceState *dev, bool en);
>> +void qdev_set_power_all_fn(void *opaque, bool en);
>> +void qbus_set_power_all(BusState *bus, bool en);
>> +void qbus_set_power_all_fn(void *opaque, bool en);
> 
> I think we don't need the _fn() functions, most of the current codebase
> directly use DEVICE(x) or BUS(x) to casts.

Ok, I will remove them.

> 
>> +
>> +/**
>> + * qdev/qbus_set_clock_all(_fn)
>> + * Enable/Disable the clock of a tree starting
>> + * at given device or bus
>> + */
>> +void qdev_set_clock_all(DeviceState *dev, bool en);
>> +void qdev_set_clock_all_fn(void *opaque, bool en);
>> +void qbus_set_clock_all(BusState *bus, bool en);
>> +void qbus_set_clock_all_fn(void *opaque, bool en);
>> +
>>  #endif
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index bb6d36eab9..24b90bd45f 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -1138,6 +1138,57 @@ void device_set_clock(DeviceState *dev, bool en)
>>      }
>>  }
>>  
>> +static int qdev_set_power_one(DeviceState *dev, void *opaque)
>> +{
>> +    device_set_power(dev, *((bool *)opaque));
>> +    return 0;
>> +}
>> +
>> +void qdev_set_power_all(DeviceState *dev, bool en)
>> +{
>> +    qdev_walk_children(dev, NULL, NULL, qdev_set_power_one, NULL, &en);
>> +}
>> +
>> +void qdev_set_power_all_fn(void *opaque, bool en)
>> +{
>> +    qdev_set_power_all(DEVICE(opaque), en);
>> +}
>> +
>> +void qbus_set_power_all(BusState *bus, bool en)
>> +{
>> +    qbus_walk_children(bus, NULL, NULL, qdev_set_power_one, NULL, &en);
>> +}
>> +
>> +void qbus_set_power_all_fn(void *opaque, bool en)
>> +{
>> +    qbus_set_power_all(BUS(opaque), en);
>> +}
>> +
>> +static int qdev_set_clock_one(DeviceState *dev, void *opaque)
>> +{
>> +    device_set_clock(dev, *((bool *)opaque));
>> +    return 0;
>> +}
>> +
>> +void qdev_set_clock_all(DeviceState *dev, bool en)
>> +{
>> +    qdev_walk_children(dev, NULL, NULL, qdev_set_clock_one, NULL, &en);
>> +}
>> +
>> +void qdev_set_clock_all_fn(void *opaque, bool en)
>> +{
>> +    qdev_set_clock_all(DEVICE(opaque), en);
>> +}
>> +
>> +void qbus_set_clock_all(BusState *bus, bool en)
>> +{
>> +    qbus_walk_children(bus, NULL, NULL, qdev_set_clock_one, NULL, &en);
>> +}
>> +
>> +void qbus_set_clock_all_fn(void *opaque, bool en)
>> +{
>> +    qbus_set_clock_all(BUS(opaque), en);
>> +}
>>  
>>  Object *qdev_get_machine(void)
>>  {
>>
>