Hi Cedric,
> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Tuesday, May 28, 2024 2:34 PM
> To: Jamin Lin <jamin_...@aspeedtech.com>; Philippe Mathieu-Daudé
> <phi...@linaro.org>; Peter Maydell <peter.mayd...@linaro.org>; Andrew
> Jeffery <and...@codeconstruct.com.au>; Joel Stanley <j...@jms.id.au>;
> Alistair Francis <alist...@alistair23.me>; Cleber Rosa <cr...@redhat.com>;
> Wainer dos Santos Moschetta <waine...@redhat.com>; Beraldo Leal
> <bl...@redhat.com>; open list:ASPEED BMCs <qemu-...@nongnu.org>; open
> list:All patches CC here <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_...@aspeedtech.com>; Yunlin Tang
> <yunlin.t...@aspeedtech.com>
> Subject: Re: [PATCH v4 05/16] aspeed/sdmc: Add AST2700 support
>
> On 5/28/24 03:26, Jamin Lin wrote:
> > Hi Philippe, Cedric
> >
> >> On 27/5/24 13:18, Cédric Le Goater wrote:
> >>> On 5/27/24 12:24, Philippe Mathieu-Daudé wrote:
> >>>> Hi Jamin,
> >>>>
> >>>> On 27/5/24 10:02, Jamin Lin wrote:
> >>>>> The SDRAM memory controller(DRAMC) controls the access to external
> >>>>> DDR4 and DDR5 SDRAM and power up to DDR4 and DDR5 PHY.
> >>>>>
> >>>>> The DRAM memory controller of AST2700 is not backward compatible
> >>>>> to previous chips such AST2600, AST2500 and AST2400.
> >>>>>
> >>>>> Max memory is now 8GiB on the AST2700. Introduce new
> >>>>> aspeed_2700_sdmc and class with read/write operation and reset
> >>>>> handlers.
> >>>>>
> >>>>> Define DRAMC necessary protected registers and unprotected
> >>>>> registers for AST2700 and increase the register set to 0x1000.
> >>>>>
> >>>>> Add unlocked property to change controller protected status.
> >>>>>
> >>>>> Signed-off-by: Troy Lee <troy_...@aspeedtech.com>
> >>>>> Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> >>>>> Reviewed-by: Cédric Le Goater <c...@kaod.org>
> >>>>> ---
> >>>>> hw/misc/aspeed_sdmc.c | 190
> >>>>> +++++++++++++++++++++++++++++++++-
> >>>>> include/hw/misc/aspeed_sdmc.h | 5 +-
> >>>>> 2 files changed, 193 insertions(+), 2 deletions(-)
> >>>>
> >>>>
> >>>>> diff --git a/include/hw/misc/aspeed_sdmc.h
> >>>>> b/include/hw/misc/aspeed_sdmc.h index ec2d59a14f..61c979583a
> >>>>> 100644
> >>>>> --- a/include/hw/misc/aspeed_sdmc.h
> >>>>> +++ b/include/hw/misc/aspeed_sdmc.h
> >>>>> @@ -17,6 +17,7 @@ OBJECT_DECLARE_TYPE(AspeedSDMCState,
> >>>>> AspeedSDMCClass, ASPEED_SDMC)
> >>>>> #define TYPE_ASPEED_2400_SDMC TYPE_ASPEED_SDMC
> "-ast2400"
> >>>>> #define TYPE_ASPEED_2500_SDMC TYPE_ASPEED_SDMC
> "-ast2500"
> >>>>> #define TYPE_ASPEED_2600_SDMC TYPE_ASPEED_SDMC
> "-ast2600"
> >>>>> +#define TYPE_ASPEED_2700_SDMC TYPE_ASPEED_SDMC "-ast2700"
> >>>>> /*
> >>>>> * SDMC has 174 documented registers. In addition the u-boot
> >>>>> device tree @@ -29,7 +30,7 @@
> >> OBJECT_DECLARE_TYPE(AspeedSDMCState,
> >>>>> AspeedSDMCClass, ASPEED_SDMC)
> >>>>> * time, and the other is in the DDR-PHY IP which is used
> >>>>> during DDR-PHY
> >>>>> * training.
> >>>>> */
> >>>>> -#define ASPEED_SDMC_NR_REGS (0x500 >> 2)
> >>>>> +#define ASPEED_SDMC_NR_REGS (0x1000 >> 2)
> >>>>
> >>>> This change breaks the migration stream.
> >>>
> >>> Do you mean migration compat ? We never cared much about that for
> >>> the Aspeed machines.
> >>
> >> So let's just remove the VMSTATE to reduce code burden?
> >>
> >> Otherwise incrementing the vmstate.version is enough.
> >>
> >> Regards,
> >>
> >> Phil.
> > If you both okay, I will remove it.
> > Do I need to create a new patch or just update in this patch?
>
> I don't think this is necessary to do so now. Possibly, increase the version
> number in the vmstate when resending a v5.
>
If I understand your request, do you mean to change as following in this patch?
static const VMStateDescription vmstate_aspeed_sdmc = {
.name = "aspeed.sdmc",
.version_id = 1, ---> Change 2
.minimum_version_id = 1, ---> Change 2
};
> Also, all Aspeed models should be addressed and that's beyond the scope of
> this series.
>
And create a new patch series to update all vmstate version for all ASPEED
models?
Take asoeed_gpio.c for example:
static const VMStateDescription vmstate_gpio_regs = {
.name = TYPE_ASPEED_GPIO"/regs",
.version_id = 1, ---> Change 2
.minimum_version_id = 1, ---> Change --> 2
}
Thanks-Jamin
>
> Thanks,
>
> C.
>
