Re: [Qemu-arm] [PATCH 4/7] aspeed_sdmc: Init status alwlays idle

[Cédric Le Goater]

On 08/07/2018 09:57 AM, Joel Stanley wrote:
> The ast2500 SDRAM training routine busy waits on the 'init cycle busy
> state' bit in DDR PHY Control/Status register #1 (MCR60).
> 
> This ensures the bit always reads zero, and allows training to
> complete with upstream u-boot on the ast2500-evb.
> 
> Signed-off-by: Joel Stanley <address@hidden>



Reviewed-by: Cédric Le Goater <address@hidden>

One comment below, no need to resend for that.

> ---
>  hw/misc/aspeed_sdmc.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 9ece545c4ffa..522e01ef8c0d 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -23,6 +23,10 @@
>  /* Configuration Register */
>  #define R_CONF            (0x04 / 4)
>  
> +/* Control/Status Register #1 (ast2500) */
> +#define R_STATUS1         (0x60 / 4)
> +#define   PHY_BUSY_STATE      BIT(0)
> +
>  /*
>   * Configuration register Ox4 (for Aspeed AST2400 SOC)
>   *
> @@ -137,6 +141,17 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, 
> uint64_t data,
>              g_assert_not_reached();
>          }
>      }
> +    if (s->silicon_rev == AST2500_A0_SILICON_REV ||
> +            s->silicon_rev == AST2500_A1_SILICON_REV) {

Maybe use the ASPEED_IS_AST2500() ? 

It would be nice to have a set of macros we could use in the different
models testing the SoC silicon revision. How about :

#define ASPEED_AST2400 0x2
#define ASPEED_AST2500 0x4
#define ASPEED_AST2600 0x6

#define ASPEED_REVISION_MAJOR(si_rev)     (((si_rev) >> 24) & 0xff)

#define ASPEED_IS_AST2400(si_rev)  (ASPEED_REVISION_MAJOR(si_rev) == 
ASPEED_AST2400)
#define ASPEED_IS_AST2500(si_rev)  (ASPEED_REVISION_MAJOR(si_rev) == 
ASPEED_AST2500)
#define ASPEED_IS_AST2600(si_rev)  (ASPEED_REVISION_MAJOR(si_rev) == 
ASPEED_AST2600)


Thanks,

C.


> +        switch (addr) {
> +        case R_STATUS1:
> +            /* Will never return 'busy' */
> +            data &= ~PHY_BUSY_STATE;
> +            break;
> +        default:
> +            break;
> +        }
> +    }
>  
>      s->regs[addr] = data;
>  }
>