[for-5.2,16/19] aspeed/sdmc: Perform memory training

From: Joel Stanley <joel@jms.id.au>

This allows qemu to run the "normal" power on reset boot path through
u-boot, where the DDR is trained.

An enhancement would be to have the SCU bit stick across qemu reboots,
but be unset on initial boot.

Proper modelling would be to discard all writes to the phy setting regs
at offset 0x100 - 0x400 and to model the phy status regs at offset
0x400.

The status regs model would only need to account for offets 0x00,
0x50, 0x68 and 0x7c.

Signed-off-by: Joel Stanley <joel@jms.id.au>
[ clg: checkpatch fixes ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/misc/aspeed_sdmc.h | 13 ++++++++++++-
 hw/misc/aspeed_scu.c          |  2 +-
 hw/misc/aspeed_sdmc.c         | 19 +++++++++++++++++--
 3 files changed, 30 insertions(+), 4 deletions(-)

On 8/6/20 3:21 PM, Cédric Le Goater wrote:
> From: Joel Stanley <joel@jms.id.au>
> 
> This allows qemu to run the "normal" power on reset boot path through
> u-boot, where the DDR is trained.
> 
> An enhancement would be to have the SCU bit stick across qemu reboots,
> but be unset on initial boot.
> 
> Proper modelling would be to discard all writes to the phy setting regs
> at offset 0x100 - 0x400 and to model the phy status regs at offset
> 0x400.
> 
> The status regs model would only need to account for offets 0x00,
> 0x50, 0x68 and 0x7c.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> [ clg: checkpatch fixes ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/misc/aspeed_sdmc.h | 13 ++++++++++++-
>  hw/misc/aspeed_scu.c          |  2 +-
>  hw/misc/aspeed_sdmc.c         | 19 +++++++++++++++++--
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index cea1e67fe365..c6226957dd3d 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -17,7 +17,18 @@
>  #define TYPE_ASPEED_2500_SDMC TYPE_ASPEED_SDMC "-ast2500"
>  #define TYPE_ASPEED_2600_SDMC TYPE_ASPEED_SDMC "-ast2600"
>  
> -#define ASPEED_SDMC_NR_REGS (0x174 >> 2)
> +/*
> + * SDMC has 174 documented registers. In addition the u-boot device tree
> + * describes the following regions:
> + *  - PHY status regs at offset 0x400, length 0x200
> + *  - PHY setting regs at offset 0x100, length 0x300
> + *
> + * There are two sets of MRS (Mode Registers) configuration in ast2600 memory
> + * system: one is in the SDRAM MC (memory controller) which is used in run
> + * time, and the other is in the DDR-PHY IP which is used during DDR-PHY
> + * training.
> + */
> +#define ASPEED_SDMC_NR_REGS (0x500 >> 2)
>  
>  typedef struct AspeedSDMCState {
>      /*< private >*/
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 764222404bef..dc6dd87c22f4 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -656,7 +656,7 @@ static const uint32_t ast2600_a1_resets[ASPEED_AST2600_SCU_NR_REGS] = {
>      [AST2600_SYS_RST_CTRL2]     = 0xFFFFFFFC,
>      [AST2600_CLK_STOP_CTRL]     = 0xFFFF7F8A,
>      [AST2600_CLK_STOP_CTRL2]    = 0xFFF0FFF0,
> -    [AST2600_SDRAM_HANDSHAKE]   = 0x00000040,  /* SoC completed DRAM init */
> +    [AST2600_SDRAM_HANDSHAKE]   = 0x00000000,
>      [AST2600_HPLL_PARAM]        = 0x1000405F,
>      [AST2600_CHIP_ID0]          = 0x1234ABCD,
>      [AST2600_CHIP_ID1]          = 0x88884444,
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 855848b7d23a..ff2809a09965 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -113,7 +113,7 @@ static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
>      if (addr >= ARRAY_SIZE(s->regs)) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
> -                      __func__, addr);
> +                      __func__, addr * 4);
>          return 0;
>      }
>  
> @@ -206,6 +206,19 @@ static void aspeed_sdmc_reset(DeviceState *dev)
>  
>      /* Set ram size bit and defaults values */
>      s->regs[R_CONF] = asc->compute_conf(s, 0);
> +
> +    /*
> +     * PHY status:
> +     *  - set phy status ok (set bit 1)
> +     *  - initial PVT calibration ok (clear bit 3)
> +     *  - runtime calibration ok (clear bit 5)
> +     */
> +    s->regs[0x100] = BIT(1);

This is usually implemented with a one-shot timer, see
sd_ocr_powerup() in hw/sd/sd.c (migration is handled).

> +
> +    /* PHY eye window: set all as passing */
> +    s->regs[0x100 | (0x68 / 4)] = 0xff;
> +    s->regs[0x100 | (0x7c / 4)] = 0xff;
> +    s->regs[0x100 | (0x50 / 4)] = 0xfffffff;
>  }
>  
>  static void aspeed_sdmc_get_ram_size(Object *obj, Visitor *v, const char *name,
> @@ -443,7 +456,9 @@ static void aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
>      }
>  
>      if (reg != R_PROT && s->regs[R_PROT] == PROT_SOFTLOCKED) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: SDMC is locked! (write to MCR%02x blocked)\n",
> +                      __func__, reg * 4);
>          return;
>      }
>  
>

On Thu, 6 Aug 2020 at 13:38, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> > @@ -206,6 +206,19 @@ static void aspeed_sdmc_reset(DeviceState *dev)
> >
> >      /* Set ram size bit and defaults values */
> >      s->regs[R_CONF] = asc->compute_conf(s, 0);
> > +
> > +    /*
> > +     * PHY status:
> > +     *  - set phy status ok (set bit 1)
> > +     *  - initial PVT calibration ok (clear bit 3)
> > +     *  - runtime calibration ok (clear bit 5)
> > +     */
> > +    s->regs[0x100] = BIT(1);
>
> This is usually implemented with a one-shot timer, see
> sd_ocr_powerup() in hw/sd/sd.c (migration is handled).

You mean we could have the calibration done bits become set at a later time?

It would be hard to work out what to do. We have no documentation for
the register, I modelled it based on the code in u-boot doing this:

         /* make sure DDR-PHY is ready before access */
        do {
                reg = readl(priv->phy_status) & BIT(1);
        } while(reg == 0);

So I think there would be limited value in modelling it.

Thanks for the suggestion though, I'll keep the one shot timer idea in
mind for future models.

Cheers,

Joel