RE: [PATCH v1 3/8] aspeed/sdmc: Add AST2700 support

[Jamin Lin]

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Thursday, February 29, 2024 5:17 PM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater <clg@kaod.org>;
> Peter Maydell <peter.maydell@linaro.org>; Andrew Jeffery
> <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; Alistair
> Francis <alistair@alistair23.me>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v1 3/8] aspeed/sdmc: Add AST2700 support
> 
> Hi Jamin,
> 
> On 29/2/24 08:23, Jamin Lin via 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.
> >
> > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/misc/aspeed_sdmc.c         | 215
> ++++++++++++++++++++++++++++++----
> >   include/hw/misc/aspeed_sdmc.h |   4 +-
> >   2 files changed, 198 insertions(+), 21 deletions(-)
> 
> 
> > @@ -231,7 +270,10 @@ static void aspeed_sdmc_realize(DeviceState *dev,
> Error **errp)
> >       AspeedSDMCState *s = ASPEED_SDMC(dev);
> >       AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
> >
> > -    assert(asc->max_ram_size < 4 * GiB); /* 32-bit address bus */
> > +    if (!asc->is_aarch64) {
> 
> Maybe name it 'bus64bit'? Because this isn't really related to Aarch64.
> 
> > +        assert(asc->max_ram_size < 4 * GiB); /* 32-bit address bus */
> > +    }
> 
> 
> > +static void aspeed_2700_sdmc_class_init(ObjectClass *klass, void
> > +*data) {
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass);
> > +
> > +    dc->desc = "ASPEED 2700 SDRAM Memory Controller";
> > +    dc->reset = aspeed_2700_sdmc_reset;
> > +
> > +    asc->is_aarch64 = true;
> > +    asc->max_ram_size = 8 * GiB;
> > +    asc->compute_conf = aspeed_2700_sdmc_compute_conf;
> > +    asc->write = aspeed_2700_sdmc_write;
> > +    asc->valid_ram_sizes = aspeed_2700_ram_sizes; }
> 
> 
> > @@ -51,6 +52,7 @@ struct AspeedSDMCClass {
> >       const uint64_t *valid_ram_sizes;
> >       uint32_t (*compute_conf)(AspeedSDMCState *s, uint32_t data);
> >       void (*write)(AspeedSDMCState *s, uint32_t reg, uint32_t data);
> > +    uint32_t is_aarch64;
> 
> bool.
> 
> >   };
> >
> >   #endif /* ASPEED_SDMC_H */
Thanks for review and will fix.