Re: [PATCH 1/6] hw/misc: Don't special case RESET_TYPE_COLD in npcm7xx_clk, gcr

2024-04-16 Thread Luc Michel
On 17:08 Fri 12 Apr , Peter Maydell wrote:
> Caution: This message originated from an External Source. Use proper caution 
> when opening attachments, clicking links, or responding.
> 
> 
> The npcm7xx_clk and npcm7xx_gcr device reset methods look at
> the ResetType argument and only handle RESET_TYPE_COLD,
> producing a warning if another reset type is passed. This
> is different from how every other three-phase-reset method
> we have works, and makes it difficult to add new reset types.
> 
> A better pattern is "assume that any reset type you don't know
> about should be handled like RESET_TYPE_COLD"; switch these
> devices to do that. Then adding a new reset type will only
> need to touch those devices where its behaviour really needs
> to be different from the standard cold reset.
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Luc Michel 

> ---
>  hw/misc/npcm7xx_clk.c | 13 +++--
>  hw/misc/npcm7xx_gcr.c | 12 
>  2 files changed, 7 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
> index ac1622c38aa..2098c85ee01 100644
> --- a/hw/misc/npcm7xx_clk.c
> +++ b/hw/misc/npcm7xx_clk.c
> @@ -873,20 +873,13 @@ static void npcm7xx_clk_enter_reset(Object *obj, 
> ResetType type)
> 
>  QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
> 
> -switch (type) {
> -case RESET_TYPE_COLD:
> -memcpy(s->regs, cold_reset_values, sizeof(cold_reset_values));
> -s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> -npcm7xx_clk_update_all_clocks(s);
> -return;
> -}
> -
> +memcpy(s->regs, cold_reset_values, sizeof(cold_reset_values));
> +s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +npcm7xx_clk_update_all_clocks(s);
>  /*
>   * A small number of registers need to be reset on a core domain reset,
>   * but no such reset type exists yet.
>   */
> -qemu_log_mask(LOG_UNIMP, "%s: reset type %d not implemented.",
> -  __func__, type);
>  }
> 
>  static void npcm7xx_clk_init_clock_hierarchy(NPCM7xxCLKState *s)
> diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c
> index 9252f9d1488..c4c4e246d7e 100644
> --- a/hw/misc/npcm7xx_gcr.c
> +++ b/hw/misc/npcm7xx_gcr.c
> @@ -159,14 +159,10 @@ static void npcm7xx_gcr_enter_reset(Object *obj, 
> ResetType type)
> 
>  QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
> 
> -switch (type) {
> -case RESET_TYPE_COLD:
> -memcpy(s->regs, cold_reset_values, sizeof(s->regs));
> -s->regs[NPCM7XX_GCR_PWRON] = s->reset_pwron;
> -s->regs[NPCM7XX_GCR_MDLR] = s->reset_mdlr;
> -s->regs[NPCM7XX_GCR_INTCR3] = s->reset_intcr3;
> -break;
> -}
> +memcpy(s->regs, cold_reset_values, sizeof(s->regs));
> +s->regs[NPCM7XX_GCR_PWRON] = s->reset_pwron;
> +s->regs[NPCM7XX_GCR_MDLR] = s->reset_mdlr;
> +s->regs[NPCM7XX_GCR_INTCR3] = s->reset_intcr3;
>  }
> 
>  static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp)
> --
> 2.34.1
> 
> 

-- 



Re: [PATCH 1/6] hw/misc: Don't special case RESET_TYPE_COLD in npcm7xx_clk, gcr

2024-04-12 Thread Philippe Mathieu-Daudé

On 12/4/24 18:08, Peter Maydell wrote:

The npcm7xx_clk and npcm7xx_gcr device reset methods look at
the ResetType argument and only handle RESET_TYPE_COLD,
producing a warning if another reset type is passed. This
is different from how every other three-phase-reset method
we have works, and makes it difficult to add new reset types.

A better pattern is "assume that any reset type you don't know
about should be handled like RESET_TYPE_COLD"; switch these
devices to do that. Then adding a new reset type will only
need to touch those devices where its behaviour really needs
to be different from the standard cold reset.

Signed-off-by: Peter Maydell 
---
  hw/misc/npcm7xx_clk.c | 13 +++--
  hw/misc/npcm7xx_gcr.c | 12 
  2 files changed, 7 insertions(+), 18 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 




Re: [PATCH 1/6] hw/misc: Don't special case RESET_TYPE_COLD in npcm7xx_clk, gcr

2024-04-12 Thread Richard Henderson

On 4/12/24 09:08, Peter Maydell wrote:

The npcm7xx_clk and npcm7xx_gcr device reset methods look at
the ResetType argument and only handle RESET_TYPE_COLD,
producing a warning if another reset type is passed. This
is different from how every other three-phase-reset method
we have works, and makes it difficult to add new reset types.

A better pattern is "assume that any reset type you don't know
about should be handled like RESET_TYPE_COLD"; switch these
devices to do that. Then adding a new reset type will only
need to touch those devices where its behaviour really needs
to be different from the standard cold reset.

Signed-off-by: Peter Maydell
---
  hw/misc/npcm7xx_clk.c | 13 +++--
  hw/misc/npcm7xx_gcr.c | 12 
  2 files changed, 7 insertions(+), 18 deletions(-)


Reviewed-by: Richard Henderson 

r~