Re: [PATCH v4 08/10] hw/core/resettable: add support for warm reset

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

Hi Damien,

On 8/21/19 6:33 PM, Damien Hedde wrote:
> Add the RESET_TYPE_WARM reset type.
> Expand the actual implementation to support several types.
> 
> I used values which can be or'ed together. This way we can what reset
> has been triggered.
> 
> Documentation is added in a following patch.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   hw/core/resettable.c    | 25 +++++++++++++++++++------
>   include/hw/resettable.h | 22 +++++++++++++++++-----
>   2 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
> index b534c2c7a4..80674292b3 100644
> --- a/hw/core/resettable.c
> +++ b/hw/core/resettable.c
> @@ -34,12 +34,17 @@ static void resettable_init_reset(Object *obj, ResetType type)
>       ResetState *s = rc->get_state(obj);
>       bool action_needed = false;
>   
> -    /* Only take action if we really enter reset for the 1st time. */
> +    /* ensure type is empty if no reset is in progress */
> +    if (s->count == 0) {
> +        s->type = 0;
> +    }
> +
>       /*
> -     * TODO: if adding more ResetType support, some additional checks
> -     * are probably needed here.
> +     * Only take action if:
> +     * + we are not already in cold reset,
> +     * + and we enter a new type of reset.
>        */
> -    if (s->count == 0) {
> +    if ((s->type & RESET_TYPE_COLD) == 0 && (s->type & type) == 0) {
>           action_needed = true;
>       }
>       s->count += 1;
> @@ -62,6 +67,7 @@ static void resettable_init_reset(Object *obj, ResetType type)
>   
>       /* exec init phase */
>       if (action_needed) {
> +        s->type |= type;
>           s->hold_phase_needed = true;
>           if (rc->phases.init) {
>               rc->phases.init(obj, type);
> @@ -133,8 +139,7 @@ static void resettable_exit_reset(Object *obj)
>   
>   void resettable_reset(Object *obj, ResetType type)
>   {
> -    /* TODO: change that when adding support for other reset types */
> -    assert(type == RESET_TYPE_COLD);
> +    assert(type == RESET_TYPE_COLD || type == RESET_TYPE_WARM);
>       trace_resettable_reset(obj, type);
>       resettable_init_reset(obj, type);
>       resettable_hold_reset(obj);
> @@ -154,6 +159,14 @@ bool resettable_is_resetting(Object *obj)
>       return (s->count > 0);
>   }
>   
> +ResetType resettable_get_type(Object *obj)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResetState *s = rc->get_state(obj);
> +
> +    return s->type;
> +}
> +
>   void resettable_class_set_parent_phases(ResettableClass *rc,
>                                           ResettableInitPhase init,
>                                           ResettableHoldPhase hold,
> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> index 5808c3c187..1e77cbd75b 100644
> --- a/include/hw/resettable.h
> +++ b/include/hw/resettable.h
> @@ -12,15 +12,14 @@ typedef struct ResetState ResetState;
>   
>   /**
>    * ResetType:
> - * Types of reset.
> + * Types of reset, values can be OR'ed together.
>    *
>    * + Cold: reset resulting from a power cycle of the object.
> - *
> - * TODO: Support has to be added to handle more types. In particular,
> - * ResetState structure needs to be expanded.
> + * + Warm: reset without power cycling.
>    */
>   typedef enum ResetType {
> -    RESET_TYPE_COLD,
> +    RESET_TYPE_COLD = 0x1,
> +    RESET_TYPE_WARM = 0x2,

I'm a bit lost with the various iterations, what is the plan with warm 
reset, is this blocked due to discussion, API, something else?

>   } ResetType;
>   
>   /*
> @@ -107,11 +106,13 @@ typedef struct ResettablePhases ResettablePhases;
>    *
>    * @count: Number of reset level the object is into. It is incremented when
>    * the reset operation starts and decremented when it finishes.
> + * @type: Type of the in-progress reset. Valid only when count is non-zero.
>    * @hold_phase_needed: flag which indicates that we need to invoke the 'hold'
>    * phase handler for this object.
>    */
>   struct ResetState {
>       uint32_t count;
> +    ResetType type;
>       bool hold_phase_needed;
>   };
>   
> @@ -123,6 +124,17 @@ struct ResetState {
>    */
>   bool resettable_is_resetting(Object *obj);
>   
> +/**
> + * resettable_get_type:
> + * Return the current type of reset @obj is under.
> + *
> + * @obj must implement Resettable interface. Result is only valid if
> + * @resettable_is_resetting is true.
> + *
> + * Note: this return an OR'ed value of type if several reset were triggered
> + */
> +ResetType resettable_get_type(Object *obj);
> +
>   /**
>    * resettable_reset:
>    * Trigger a reset on a object @obj of type @type. @obj must implement
>