> -----Original Message-----
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Monday, March 4, 2024 10:47 PM
> To: Jamin Lin <jamin_...@aspeedtech.com>; Peter Maydell
> <peter.mayd...@linaro.org>; Andrew Jeffery <and...@codeconstruct.com.au>;
> Joel Stanley <j...@jms.id.au>; Alistair Francis <alist...@alistair23.me>; 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 v2 3/9] aspeed/sdmc: Add AST2700 support
>
Hi Cedrice,
Thanks for review and sorry reply you late.
> On 3/4/24 10:29, 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.
> >
> > Signed-off-by: Troy Lee <troy_...@aspeedtech.com>
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > hw/misc/aspeed_sdmc.c | 215
> ++++++++++++++++++++++++++++++----
> > include/hw/misc/aspeed_sdmc.h | 4 +-
> > 2 files changed, 198 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c index
> > 64cd1a81dc..63fb7936c4 100644
> > --- a/hw/misc/aspeed_sdmc.c
> > +++ b/hw/misc/aspeed_sdmc.c
> > @@ -27,6 +27,7 @@
> > #define PROT_SOFTLOCKED 0x00
> >
> > #define PROT_KEY_UNLOCK 0xFC600309
> > +#define PROT_2700_KEY_UNLOCK 0x1688A8A8
> > #define PROT_KEY_HARDLOCK 0xDEADDEAD /* AST2600 */
> >
> > /* Configuration Register */
> > @@ -54,6 +55,46 @@
> > #define R_DRAM_TIME (0x8c / 4)
> > #define R_ECC_ERR_INJECT (0xb4 / 4)
> >
> > +/* AST2700 Register */
> > +#define R_2700_PROT (0x00 / 4)
> > +#define R_INT_STATUS (0x04 / 4)
> > +#define R_INT_CLEAR (0x08 / 4)
> > +#define R_INT_MASK (0x0c / 4)
> > +#define R_MAIN_CONF (0x10 / 4)
> > +#define R_MAIN_CONTROL (0x14 / 4)
> > +#define R_MAIN_STATUS (0x18 / 4)
> > +#define R_ERR_STATUS (0x1c / 4)
> > +#define R_ECC_FAIL_STATUS (0x78 / 4)
> > +#define R_ECC_FAIL_ADDR (0x7c / 4)
> > +#define R_ECC_TESTING_CONTROL (0x80 / 4)
> > +#define R_PROT_REGION_LOCK_STATUS (0x94 / 4)
> > +#define R_TEST_FAIL_ADDR (0xd4 / 4)
> > +#define R_TEST_FAIL_D0 (0xd8 / 4)
> > +#define R_TEST_FAIL_D1 (0xdc / 4)
> > +#define R_TEST_FAIL_D2 (0xe0 / 4)
> > +#define R_TEST_FAIL_D3 (0xe4 / 4)
> > +#define R_DBG_STATUS (0xf4 / 4)
> > +#define R_PHY_INTERFACE_STATUS (0xf8 / 4)
> > +#define R_GRAPHIC_MEM_BASE_ADDR (0x10c / 4)
> > +#define R_PORT0_INTERFACE_MONITOR0 (0x240 / 4) #define
> > +R_PORT0_INTERFACE_MONITOR1 (0x244 / 4) #define
> > +R_PORT0_INTERFACE_MONITOR2 (0x248 / 4) #define
> > +R_PORT1_INTERFACE_MONITOR0 (0x2c0 / 4) #define
> > +R_PORT1_INTERFACE_MONITOR1 (0x2c4 / 4) #define
> > +R_PORT1_INTERFACE_MONITOR2 (0x2c8 / 4) #define
> > +R_PORT2_INTERFACE_MONITOR0 (0x340 / 4) #define
> > +R_PORT2_INTERFACE_MONITOR1 (0x344 / 4) #define
> > +R_PORT2_INTERFACE_MONITOR2 (0x348 / 4) #define
> > +R_PORT3_INTERFACE_MONITOR0 (0x3c0 / 4) #define
> > +R_PORT3_INTERFACE_MONITOR1 (0x3c4 / 4) #define
> > +R_PORT3_INTERFACE_MONITOR2 (0x3c8 / 4) #define
> > +R_PORT4_INTERFACE_MONITOR0 (0x440 / 4) #define
> > +R_PORT4_INTERFACE_MONITOR1 (0x444 / 4) #define
> > +R_PORT4_INTERFACE_MONITOR2 (0x448 / 4) #define
> > +R_PORT5_INTERFACE_MONITOR0 (0x4c0 / 4) #define
> > +R_PORT5_INTERFACE_MONITOR1 (0x4c4 / 4) #define
> > +R_PORT5_INTERFACE_MONITOR2 (0x4c8 / 4)
> > +
> > /*
> > * Configuration register Ox4 (for Aspeed AST2400 SOC)
> > *
> > @@ -76,10 +117,6 @@
> > #define ASPEED_SDMC_VGA_32MB 0x2
> > #define ASPEED_SDMC_VGA_64MB 0x3
> > #define ASPEED_SDMC_DRAM_SIZE(x) (x & 0x3)
> > -#define ASPEED_SDMC_DRAM_64MB 0x0
> > -#define ASPEED_SDMC_DRAM_128MB 0x1
> > -#define ASPEED_SDMC_DRAM_256MB 0x2
> > -#define ASPEED_SDMC_DRAM_512MB 0x3
> >
> > #define ASPEED_SDMC_READONLY_MASK \
> > (ASPEED_SDMC_RESERVED | ASPEED_SDMC_VGA_COMPAT | \
> > @@ -100,22 +137,24 @@
> > #define ASPEED_SDMC_CACHE_ENABLE (1 << 10) /* differs
> from AST2400 */
> > #define ASPEED_SDMC_DRAM_TYPE (1 << 4) /* differs
> from AST2400 */
> >
> > -/* DRAM size definitions differs */
> > -#define ASPEED_SDMC_AST2500_128MB 0x0
> > -#define ASPEED_SDMC_AST2500_256MB 0x1
> > -#define ASPEED_SDMC_AST2500_512MB 0x2
> > -#define ASPEED_SDMC_AST2500_1024MB 0x3
> > -
> > -#define ASPEED_SDMC_AST2600_256MB 0x0
> > -#define ASPEED_SDMC_AST2600_512MB 0x1
> > -#define ASPEED_SDMC_AST2600_1024MB 0x2
> > -#define ASPEED_SDMC_AST2600_2048MB 0x3
> > -
> Please move the removal above in a separate patch.
Will fix
>
> > #define ASPEED_SDMC_AST2500_READONLY_MASK
> \
> > (ASPEED_SDMC_HW_VERSION(0xf) |
> ASPEED_SDMC_CACHE_INITIAL_DONE | \
> > ASPEED_SDMC_AST2500_RESERVED |
> ASPEED_SDMC_VGA_COMPAT | \
> > ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB))
> >
> > +/*
> > + * Main Configuration register Ox10 (for Aspeed AST2700 SOC and
> > +higher)
> > + *
> > + */
> > +#define ASPEED_SDMC_AST2700_RESERVED 0xFFFF2082 /* 31:16,
> 13, 7, 1 */
> > +#define ASPEED_SDMC_AST2700_DATA_SCRAMBLE (1 << 8)
> > +#define ASPEED_SDMC_AST2700_ECC_ENABLE (1 << 6)
> > +#define ASPEED_SDMC_AST2700_PAGE_MATCHING_ENABLE (1 << 5)
> > +#define ASPEED_SDMC_AST2700_DRAM_SIZE(x) ((x & 0x7)
> << 2)
> > +
> > +#define ASPEED_SDMC_AST2700_READONLY_MASK \
> > + (ASPEED_SDMC_AST2700_RESERVED)
> > +
> > static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned
> size)
> > {
> > AspeedSDMCState *s = ASPEED_SDMC(opaque); @@ -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_bus64bit) {
> > + assert(asc->max_ram_size < 4 * GiB); /* 32-bit address bus */
> > + }
>
> May be :
>
> assert(asc->max_ram_size < 4 * GiB || asc->is_bus64bit);
>
> ?
Will fix
>
> > +
> > s->max_ram_size = asc->max_ram_size;
> >
> > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_sdmc_ops,
> s,
> > @@ -311,7 +353,8 @@ static void
> aspeed_2400_sdmc_write(AspeedSDMCState *s, uint32_t reg,
> > uint32_t data)
> > {
> > if (reg == R_PROT) {
> > - s->regs[reg] = (data == PROT_KEY_UNLOCK) ? PROT_UNLOCKED :
> PROT_SOFTLOCKED;
> > + s->regs[reg] =
> > + (data == PROT_KEY_UNLOCK) ? PROT_UNLOCKED :
> > + PROT_SOFTLOCKED;
> > return;
> > }
> >
> > @@ -369,7 +412,8 @@ static void
> aspeed_2500_sdmc_write(AspeedSDMCState *s, uint32_t reg,
> > uint32_t data)
> > {
> > if (reg == R_PROT) {
> > - s->regs[reg] = (data == PROT_KEY_UNLOCK) ? PROT_UNLOCKED :
> PROT_SOFTLOCKED;
> > + s->regs[reg] =
> > + (data == PROT_KEY_UNLOCK) ? PROT_UNLOCKED :
> > + PROT_SOFTLOCKED;
> > return;
> > }
> >
> > @@ -449,8 +493,9 @@ static void
> aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
> > }
> >
> > if (s->regs[R_PROT] == PROT_HARDLOCKED) {
> > - qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked until
> system reset!\n",
> > - __func__);
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: SDMC is locked until system reset!\n",
> > + __func__);
> > return;
> > }
>
> The checkpatch changes could be in a separate patch. This is minor.
Will fix
>
> >
> > @@ -512,12 +557,142 @@ static const TypeInfo aspeed_2600_sdmc_info = {
> > .class_init = aspeed_2600_sdmc_class_init,
> > };
> >
> > +static void aspeed_2700_sdmc_reset(DeviceState *dev) {
> > + AspeedSDMCState *s = ASPEED_SDMC(dev);
> > + AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
> > +
> > + memset(s->regs, 0, sizeof(s->regs));
> > +
> > + /* Set ram size bit and defaults values */
> > + s->regs[R_MAIN_CONF] = asc->compute_conf(s, 0);
> > + s->regs[R_2700_PROT] = PROT_UNLOCKED;
>
> The default reset value is unlocked ?
>
The sdmc controller is unlocked at the SPL stage for AST2700.
However, currently QEMU only support to emulate that AST2700 booting start at
the u-boot stage.
That was why I set the default value was unlocked, so firmware(u-boot)
able to write the sdmc registers.
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/board/aspeed/ibex_ast2700/sdram_ast2700.c#L716
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/board/aspeed/ibex_ast2700/sdram_ast2700.c#L277
Thanks-Jamin
>
> Thanks,
>
> C.
>
>
>
> > +}
> > +
> > +static uint32_t aspeed_2700_sdmc_compute_conf(AspeedSDMCState *s,
> > +uint32_t data) {
> > + uint32_t fixed_conf =
> ASPEED_SDMC_AST2700_PAGE_MATCHING_ENABLE |
> > +
> ASPEED_SDMC_AST2700_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
> > +
> > + /* Make sure readonly bits are kept */
> > + data &= ~ASPEED_SDMC_AST2700_READONLY_MASK;
> > +
> > + return data | fixed_conf;
> > +}
> > +
> > +static void aspeed_2700_sdmc_write(AspeedSDMCState *s, uint32_t reg,
> > + uint32_t data) {
> > + /* Unprotected registers */
> > + switch (reg) {
> > + case R_INT_STATUS:
> > + case R_INT_CLEAR:
> > + case R_INT_MASK:
> > + case R_MAIN_STATUS:
> > + case R_ERR_STATUS:
> > + case R_ECC_FAIL_STATUS:
> > + case R_ECC_FAIL_ADDR:
> > + case R_PROT_REGION_LOCK_STATUS:
> > + case R_TEST_FAIL_ADDR:
> > + case R_TEST_FAIL_D0:
> > + case R_TEST_FAIL_D1:
> > + case R_TEST_FAIL_D2:
> > + case R_TEST_FAIL_D3:
> > + case R_DBG_STATUS:
> > + case R_PHY_INTERFACE_STATUS:
> > + case R_GRAPHIC_MEM_BASE_ADDR:
> > + case R_PORT0_INTERFACE_MONITOR0:
> > + case R_PORT0_INTERFACE_MONITOR1:
> > + case R_PORT0_INTERFACE_MONITOR2:
> > + case R_PORT1_INTERFACE_MONITOR0:
> > + case R_PORT1_INTERFACE_MONITOR1:
> > + case R_PORT1_INTERFACE_MONITOR2:
> > + case R_PORT2_INTERFACE_MONITOR0:
> > + case R_PORT2_INTERFACE_MONITOR1:
> > + case R_PORT2_INTERFACE_MONITOR2:
> > + case R_PORT3_INTERFACE_MONITOR0:
> > + case R_PORT3_INTERFACE_MONITOR1:
> > + case R_PORT3_INTERFACE_MONITOR2:
> > + case R_PORT4_INTERFACE_MONITOR0:
> > + case R_PORT4_INTERFACE_MONITOR1:
> > + case R_PORT4_INTERFACE_MONITOR2:
> > + case R_PORT5_INTERFACE_MONITOR0:
> > + case R_PORT5_INTERFACE_MONITOR1:
> > + case R_PORT5_INTERFACE_MONITOR2:
> > + s->regs[reg] = data;
> > + return;
> > + }
> > +
> > + if (s->regs[R_2700_PROT] == PROT_HARDLOCKED) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: SDMC is locked until system reset!\n",
> > + __func__);
> > + return;
> > + }
> > +
> > + if (reg != R_2700_PROT && s->regs[R_2700_PROT] ==
> PROT_SOFTLOCKED) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: SDMC is locked! (write to MCR%02x
> blocked)\n",
> > + __func__, reg * 4);
> > + return;
> > + }
> > +
> > + switch (reg) {
> > + case R_2700_PROT:
> > + if (data == PROT_2700_KEY_UNLOCK) {
> > + data = PROT_UNLOCKED;
> > + } else if (data == PROT_KEY_HARDLOCK) {
> > + data = PROT_HARDLOCKED;
> > + } else {
> > + data = PROT_SOFTLOCKED;
> > + }
> > + break;
> > + case R_MAIN_CONF:
> > + data = aspeed_2700_sdmc_compute_conf(s, data);
> > + break;
> > + case R_MAIN_STATUS:
> > + /* Will never return 'busy'. */
> > + data &= ~PHY_BUSY_STATE;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + s->regs[reg] = data;
> > +}
> > +
> > +static const uint64_t
> > + aspeed_2700_ram_sizes[] = { 256 * MiB, 512 * MiB, 1024 * MiB,
> > + 2048 * MiB, 4096 * MiB, 8192 *
> MiB,
> > +0};
> > +
> > +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_bus64bit = 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; }
> > +
> > +static const TypeInfo aspeed_2700_sdmc_info = {
> > + .name = TYPE_ASPEED_2700_SDMC,
> > + .parent = TYPE_ASPEED_SDMC,
> > + .class_init = aspeed_2700_sdmc_class_init, };
> > +
> > static void aspeed_sdmc_register_types(void)
> > {
> > type_register_static(&aspeed_sdmc_info);
> > type_register_static(&aspeed_2400_sdmc_info);
> > type_register_static(&aspeed_2500_sdmc_info);
> > type_register_static(&aspeed_2600_sdmc_info);
> > + type_register_static(&aspeed_2700_sdmc_info);
> > }
> >
> > type_init(aspeed_sdmc_register_types);
> > diff --git a/include/hw/misc/aspeed_sdmc.h
> > b/include/hw/misc/aspeed_sdmc.h index ec2d59a14f..6df2f0a3b7 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)
> >
> > struct AspeedSDMCState {
> > /*< private >*/
> > @@ -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);
> > + bool is_bus64bit;
> > };
> >
> > #endif /* ASPEED_SDMC_H */
