Re: [Qemu-ppc] [PATCH v3 09/33] add doc about Resettable interface

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde <address@hidden> wrote:
>
> Signed-off-by: Damien Hedde <address@hidden>
> ---
>  docs/devel/reset.txt | 165 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 docs/devel/reset.txt
>
> diff --git a/docs/devel/reset.txt b/docs/devel/reset.txt
> new file mode 100644
> index 0000000000..c7a1eb068f
> --- /dev/null
> +++ b/docs/devel/reset.txt
> @@ -0,0 +1,165 @@
> +
> +=====
> +Reset
> +=====
> +
> +The reset of qemu objects is handled using the Resettable interface declared
> +in *include/hw/resettable.h*.
> +As of now DeviceClass and BusClass implement this interface.
> +> +
> +Triggering reset
> +----------------

I think this would be clearer if we documented it the other way around:

  This section documents the APIs which "users" of a resettable
  object should use to control it.

  A resettable object can be put into its "in reset" state and
  held there indefinitely.

  [documentation for resettable_assert/deassert_reset goes here]

  If you want to just trigger a reset event but not leave the
  object in reset for any period of time, you can use
  resettable_reset(), which is a convenience function identical
  in behaviour to calling resettable_assert() and then immediately
  calling resettable_deassert().

  [resettable_reset() docs here]

> +
> +The function *resettable_reset* is used to trigger a reset on a given
> +object.
> +void resettable_reset(Object *obj, bool cold)
> +
> +The parameter *obj* must implement the Resettable interface.
> +The parameter *cold* is a boolean specifying whether to do a cold or warm
> +reset
> +
> +For Devices and Buses there is also the corresponding helpers:
> +void device_reset(Device *dev, bool cold)
> +void bus_reset(Device *dev, bool cold)

We should explain what these do.

> +
> +If one wants to put an object into a reset state. There is the
> +*resettable_assert_reset* function.
> +void resettable_assert_reset(Object *obj, bool cold)
> +
> +One must eventually call the function *resettable_deassert_reset* to end the
> +reset state:
> +void resettable_deassert_reset(Object *obj, bool cold)
> +
> +Calling *resettable_assert_reset* then *resettable_deassert_reset* is the
> +same as calling *resettable_reset*.
> +
> +It is possible to interleave multiple calls to
> + - resettable_reset,
> + - resettable_assert_reset, and
> + - resettable_deassert_reset.
> +The only constraint is that *resettable_deassert_reset* must be called once
> +per *resettable_assert_reset* call so that the object leaves the reset state.
> +
> +Therefore there may be several reset sources/controllers of a given object.
> +The interface handle everything and the controllers do not need to know

"handles"

> +anything about each others. The object will leave reset state only when all

"each other"

> +controllers released their reset.

"release"

> +
> +All theses functions must called while holding the iothread lock.

"these"; "be called"

> +
> +
> +Implementing reset for a Resettable object : Multi-phase reset
> +--------------------------------------------------------------

  This section documents the APIs that an implementation of a
  resettable object must provide.

> +
> +The Resettable uses a multi-phase mechanism to handle some ordering 
> constraints
> +when resetting multiple object at the same time. For a given object the reset

"objects"

> +procedure is split into three different phases executed in order:
> + 1 INIT: This phase should set/reset the state of the Resettable it has when 
> is
> +         in reset state. Side-effects to others object is forbidden (such as
> +         setting IO level).
> + 2 HOLD: This phase corresponds to the external side-effects due to staying 
> into
> +         the reset state.
> + 3 EXIT: This phase corresponds to leaving the reset state. It have both
> +         local and external effects.

We should be a bit more detailed here. I had some text in
my review of patch 1 for the doc-comments which we can put here too.

> +
> +*resettable_assert_reset* does the INIT and HOLD phases. While
> +*resettable_deassert_reset* does the EXIT phase.

Delete this bit -- we don't want to muddle the reader up between
the 'user' APIs and the 'implementer' APIs.

> +
> +When resetting multiple object at the same time. The interface executes the

Comma, not full stop.

> +given phase of the objects before going to the next phase. This guarantee 
> that

"guarantees"

> +all INIT phases are done before any HOLD phase and so on.
> +
> +There is three methods in the interface so must be implemented in an object.

"are"; "that must"

> +The methods corresponds to the three phases:

"correspond"

> +```
> +typedef void (*ResettableInitPhase)(Object *obj);
> +typedef void (*ResettableHoldPhase)(Object *obj);
> +typedef void (*ResettableExitPhase)(Object *obj);
> +typedef struct ResettableClass {
> +    InterfaceClass parent_class;
> +
> +    struct ResettablePhases {
> +        ResettableInitPhase init;
> +        ResettableHoldPhase hold;
> +        ResettableExitPhase exit;
> +    } phases;
> +    [...]
> +} ResettableClass;
> +```
> +
> +Theses methods should be updated when specializing an object. For this the

"These"

> +helper function *resettable_class_set_parent_reset_phases* can be used to
> +backup parent methods while changing the specialized ones.
> +void resettable_class_set_parent_reset_phases(ResettableClass *rc,
> +                                              ResettableInitPhase init,
> +                                              ResettableHoldPhase hold,
> +                                              ResettableExitPhase exit,
> +
> +For Devices and Buses, some helper exists to know if a device/bus is under

"helpers"

> +reset and what type of reset it is:
> +```
> +bool device_is_resetting(DeviceState *dev);
> +bool device_is_reset_cold(DeviceState *dev);
> +bool bus_is_resetting(BusState *bus);
> +bool bus_is_reset_cold(BusState *bus);
> +```

What are these for? Are they part of the API intended for 'users'
of a resettable device', or something that the 'implementor' might
want to use as part of their implementation. More detail would be good.

> +
> +
> +Implementing the base Resettable behavior : Re-entrance, Hierarchy and 
> Cold/Warm
> +--------------------------------------------------------------------------------

  This section documents parts of the reset mechanism that you only
  need to know about if you are extending it to work with a new
  base class other than DeviceClass or BusClass, or maintaining
  the existing code in those classes. Most people can ignore it.

> +
> +There is five others methods in the interface to handle the base mechanics

"are five other methods"

> +of the Resettable interface. The methods should be implemented in object
> +base class. DeviceClass and BusClass implement them.
> +
> +```
> +typedef bool (*ResettableSetCold)(Object *obj, bool cold);
> +typedef bool (*ResettableSetHoldNeeded)(Object *obj, bool hold_needed);
> +typedef uint32_t (*ResettableGetCount)(Object *obj);
> +typedef uint32_t (*ResettableIncrementCount)(Object *obj);
> +typedef uint32_t (*ResettableDecrementCount)(Object *obj);
> +typedef void (*ResettableForeachChild)(Object *obj, void (*visitor)(Object 
> *));
> +typedef struct ResettableClass {
> +    InterfaceClass parent_class;
> +
> +    [...]
> +
> +    ResettableSetCold set_cold;
> +    ResettableSetHoldNeeded set_hold_needed;
> +    ResettableGetCount get_count;
> +    ResettableIncrementCount increment_count;
> +    ResettableDecrementCount decrement_count;
> +    ResettableForeachChild foreach_child;
> +} ResettableClass;
> +```
> +
> +*set_cold* is used when entering reset, before calling the init phase, to
> +indicate the reset type.
> +
> +*set_hold_needed* is used to set/clear and retrieve an "hold_needed" flag.
> +This flag allows to omly execute the hold pahse when required.
> +
> +As stated above, several reset procedures can be concurrent on an object.
> +This is handled with the three methods *get_count*, *increment_count* and
> +*decrement_count*. An object is in reset state if the count is non-zero.
> +
> +The reset hierarchy is handled using the *foreach_child* method. This method
> +executes a given function on every reset "child".
> +
> +In DeviceClass and BusClass the base behavior is to mimic the legacy qdev
> +reset. Reset hierarchy follows the qdev/qbus tree.

We should document the behaviour of the new system specifically,
without saying it's like the old legacy reset. After all the idea
is that the legacy reset is going to go away, so readers should be
able to understand the new system without first knowing about the
old one.

> +
> +Reset control through GPIO
> +--------------------------
> +
> +For devices, two reset inputs can be added: one for the cold, one the warm
> +reset. This is done using the following function.
> +```
> +typedef enum DeviceResetActiveType {
> +    DEVICE_RESET_ACTIVE_LOW,
> +    DEVICE_RESET_ACTIVE_HIGH,
> +} DeviceResetActiveType;
> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
> +                                   bool cold, DeviceResetActiveType type);
> +```

Rather than just quoting the prototype of the function, we should
provide a more descriptive documentation of what this feature is
for and why you'd want to use it. Then we can just refer the
reader to the function (which should have its own doc comment)
for the exact details of how the API works.

thanks
-- PMM