| Message ID | 20240412160809.1260625-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reset: Add RESET_TYPE_SNAPSHOT_LOAD | expand |
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<peter.maydell@linaro.org> > --- > hw/misc/npcm7xx_clk.c | 13 +++---------- > hw/misc/npcm7xx_gcr.c | 12 ++++-------- > 2 files changed, 7 insertions(+), 18 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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 <peter.maydell@linaro.org> > --- > hw/misc/npcm7xx_clk.c | 13 +++---------- > hw/misc/npcm7xx_gcr.c | 12 ++++-------- > 2 files changed, 7 insertions(+), 18 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 <peter.maydell@linaro.org> Reviewed-by: Luc Michel <luc.michel@amd.com> > --- > 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 > > --
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)
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 <peter.maydell@linaro.org> --- hw/misc/npcm7xx_clk.c | 13 +++---------- hw/misc/npcm7xx_gcr.c | 12 ++++-------- 2 files changed, 7 insertions(+), 18 deletions(-)