On Wed, 19 Feb 2025 at 18:46, Hao Wu <wuhao...@google.com> wrote:
>
> These 2 values are different between NPCM7XX and NPCM8XX
> GCRs. So we add them to the class and assign different values
> to them.
>
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
> Signed-off-by: Hao Wu <wuhao...@google.com>
> @@ -156,10 +157,12 @@ static const struct MemoryRegionOps npcm_gcr_ops = {
> static void npcm7xx_gcr_enter_reset(Object *obj, ResetType type)
> {
> NPCMGCRState *s = NPCM_GCR(obj);
> + NPCMGCRClass *c = NPCM_GCR_GET_CLASS(obj);
>
> - QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
> -
> - memcpy(s->regs, cold_reset_values, sizeof(s->regs));
> + g_assert(sizeof(s->regs) >= sizeof(c->cold_reset_values));
> + g_assert(sizeof(s->regs) >= c->nr_regs * sizeof(uint32_t));
> + memcpy(s->regs, c->cold_reset_values, c->nr_regs * sizeof(uint32_t));
I looked again at this code after seeing the fix to the
similar code in npcm_clk that Pierrick just sent, and this
one looks broken in a different way:
c->cold_reset_values is a pointer, not an array, so
g_assert(sizeof(s->regs) >= sizeof(c->cold_reset_values))
is asserting that the s->regs[] array is bigger than the size
of a pointer (8 bytes), which probably isn't what you meant.
Other than that, this is now the same as the fixed npcm_clk code,
except that we could do the same as that does and use a local
variable for sizeof_regs:
size_t sizeof_regs = c->nr_regs * sizeof(uint32_t);
g_assert(sizeof(s->regs) >= sizeof_regs);
memcpy(s->regs, c->cold_reset_values, sizeof_regs);
But also, in this device (unlike npcm_clk) we have separate
reset method functions for npcm7xx and npcm8xx. So we don't
really need the s->cold_reset_values pointer at all, because
we could make npcm7xx_gcr_enter_reset() directly copy from
npcm7xx_cold_reset_values[] and similarly fro the npcm8xx function.
I think we should at least delete the assert() that isn't
doing what it looks like it's doing; I'll leave it to you
whether you want to also do one of the other two suggestions.
thanks
-- PMM
