Re: [Qemu-devel] [RFC 00/17] multi-phase reset mechanism

[Damien Hedde]

Hi All,

Any comment about this ?

Thanks,
Damien

On 3/25/19 12:01 PM, Damien Hedde wrote:
> Hi all,
> 
> This series is a proposal to implement the multi-phase reset we've discussed
> here (https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00310.html) and
> more recently there
> (https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00081.html).
> 
> To summarize, we need a multi-phase reset to allow for a more complex
> initialization of a platform. In particular we need to "propagate" a clock
> tree, but we have to ensure that every device is initialized first.
> 
> To solve this problem, the following 3-phases reset mechanism is proposed 
> (I've
> removed the 4th given our last discussion).
> 
> #DESCRIPTION
> 
> INIT PHASE: Reset the object internal state, put a resetting flag and do the
>     same for the reset subtree. No side effect on other devices to guarantee
>     that, in a reset domain, everything get initialized first. This 
> corresponds
>     mostly to what is currently done in the device/bus reset method.
> 
> HOLD PHASE: This phase allows to control a reset with a I/O. When a I/O 
> control
>     a reset procedure based on the I/O level (not edge), we may need to assert
>     the reset, wait some time, and finally de-assert the reset. The 
> consequence
>     is that such a device can stay in a "resetting state" and may need to show
>     this state to other devices through its outputs. For example, a clock
>     controller will typically shutdown its clocks when it is in resetting 
> state.
> 
> EXIT PHASE (previously named 'release'): This phase sets outputs to state 
> after
>      reset. For a clock controller it starts the clocks. It also clears the
>      "resetting" flag. A device should not react to inputs until this flag has
>      been cleared. During this phase, outputs are propagated in the reset 
> domain
>      (and outside the reset domain).
> 
> To implement this, this series add a Resettable interface containing 3 
> methods:
> one for each phase. The init phase's method takes a boolean argument allowing 
> to
> distinguish cold and warm resets.
> 
> In this series, Device and Bus implement the interface. A vmstate description
> subsection is added to allow migration of reset related state for device.
> 3 methods (one per phase) are also added in Device's class. They correspond to
> the local part (see the code example below) of the reset. They avoid device
> specialization to have to handle the sub-domain part and "resetting" state.
> 
> Functions to add gpio control over the reset are added. It is possible to add
> an active low/high reset pin to control the warm reset and an active low/high
> power gating pin to control the cold reset.
> 
> The bus/device reset tree is converted to this 3-phases mechanism. I also
> added a new ResetDomain object which is just a Resettable container. It is
> used to have a global "system reset domain" to handle main 3-phases reset and
> replace the current single-phase-reset handler mechanism.
> 
> As an example, in the xilinx_zynq machine, I've converted the slcr (the resets
> and clocks controller device) and the uart peripheral to this 3-phases reset
> mechanism. Gpio are added between the devices to transmit reset control from
> the slcr to the uarts.
> 
> All changes have been made so that existing reset behavior is not modified.
> 
> #INTERFACE CHOICE
> 
> To be honest, I have some doubt about the interface. I've kept it minimal and
> the consequence is implementation is complex (and kind of duplicated in every
> base implementation like Device or Bus). One of the problem is the `resetting`
> flag, which in practical must be a counter to handle reset "reentrance".
> Indeed nothing forbids to reset some device that is already held in 
> "resetting"
> state by some other means. As an example, in the zynq machine, there can be 
> a global/system reset while an uart is reset by the slcr. It it also possible
> to cold and warm resets triggered concurrently.
> 
> The Resettable methods implementation have to looks this (methods are called
> with iothread locked):
> ```
> void init_phase(Device *dev, bool cold)
> {
>     /* call sub-resettable init phases */
> 
>     dev->resetting += 1;
>     /* do local init phase (eg: state reset) */
> }
> void hold_phase(Device *dev)
> {
>     /* call sub-resettable hold phases */
> 
>     /* do local hold phase (eg: set some I/O level) */
> }
> void exit_phase(Device *dev)
> {
>     /* call sub-resettable exit phases (independently of resetting value 
> since,
>        every resettable have its own resetting counter) */
>    
>     dev->resetting -= 1;
>     if (dev->resetting == 0) {
>         /* do local exit phase (eg: set some I/O level) only if the device is
>            really leaving the resetting state */
>     }
> }
> ```
> Since I don't think specialization should care about sub-resettable and the
> resetting counter, I've added the local init/hold/exit phases as DeviceClass
> methods.
> Otherwise, I see two other solutions:
> + keep the interface as it is
> + add some state knowledge in the interface (with some kind of get_state 
> method)
>   so that resetting counter and some kind of sub-resettable list are handled 
> in
>   the common code in the interface. The 3 methods will then handle only the 
> local
>   actions.
> + have 6 methods in the interface, one for the local actions, one for the
>   sub-resettable so that sub-resettable is not handled in the common code. 
> And we
>   need also a get_resetting method to access/update the counter.
> 
> #DEVICE RESET CHOICE
> 
> The Device is a special case, it has 2 reset entry point: `qdev_reset_all` and
> `device_reset`. The latter doing a device reset only, while the former also
> reset all buses hierarchy under the device.
> 
> I choose the put the sub-buses into the device reset domain, so that the
> behavior of the resettable interface on Device is the same as qdev_reset_all.
> I don't know if `device_reset` has some real meaning (resetting only the
> Device). It is not often used and I think every time it is used there is no
> sub-buses so the behavior is the same for both functions.
> 
> If I am mistaken about putting buses into device reset domain, it is possible
> to make a special bus-hierarchy-reset-domain for every Device/Bus that differs
> from the Resettable interface on Device/Bus.
> 
> # SYSBUS SUPPORT
> 
> Regarding the sysbus support Edgar mentioned in the prevous discussion: In 
> this
> series, there is no support in sysbus base class for disabling memory regions.
> Having each sysbus device specialization, in every memory region handler, to
> check if `resetting` is set or not is not user-friendly. But for we can't 
> modify memory region `enabled` flags since devices may already set/unset them.
> Maybe we could have some kind of super-switch to disable all memory regions in
> a sysbus device but I don't know how this could be done.
> 
> The series is organised as follow:
> Patches 1 and 2 adds Resettable interface and ResetDomain object.
> Patches 3 to 7 converts Device and Bus to Resettable.
> Patches 8 to 12 handles the global system reset domain
> Patches 13 to 17 do the zynq implementation (patch 13 is an already-reviewed
> patch from the clock api patch series)
> 
> Thank you for your feedback,
> Damien
> 
> Damien Hedde (17):
>   Create Resettable QOM interface
>   Create the ResetDomain QOM object
>   make Device and Bus Resettable
>   Add local reset methods in Device class
>   add vmstate description for device reset state
>   Add function to control reset with gpio inputs
>   convert qdev/bus_reset_all to Resettable
>   Add a global ResetDomain object for system emulation
>   global ResetDomain support for legacy reset handlers
>   Delete the system ResetDomain at the end of emulation
>   Put orphan buses in system reset domain
>   Put default sysbus in system reset domain
>   hw/misc/zynq_slcr: use standard register definition
>   convert cadence_uart to 3-phases reset
>   Convert zynq's slcr to 3-phases reset
>   Add uart reset support in zynq_slcr
>   Connect the uart reset gpios in the zynq platform
> 
>  hw/arm/xilinx_zynq.c           |  14 +-
>  hw/char/cadence_uart.c         |  48 ++-
>  hw/core/Makefile.objs          |   3 +
>  hw/core/bus.c                  |  64 +++-
>  hw/core/qdev-vmstate.c         |  27 ++
>  hw/core/qdev.c                 | 166 ++++++++++-
>  hw/core/reset-domain.c         | 121 ++++++++
>  hw/core/reset.c                | 149 +++++++++-
>  hw/core/resettable.c           |  69 +++++
>  hw/misc/zynq_slcr.c            | 515 ++++++++++++++++++---------------
>  include/hw/char/cadence_uart.h |  10 +-
>  include/hw/qdev-core.h         |  97 +++++++
>  include/hw/reset-domain.h      |  49 ++++
>  include/hw/resettable.h        |  83 ++++++
>  include/sysemu/reset.h         |  47 +++
>  vl.c                           |   3 +-
>  16 files changed, 1195 insertions(+), 270 deletions(-)
>  create mode 100644 hw/core/qdev-vmstate.c
>  create mode 100644 hw/core/reset-domain.c
>  create mode 100644 hw/core/resettable.c
>  create mode 100644 include/hw/reset-domain.h
>  create mode 100644 include/hw/resettable.h
>