Re: [PATCH v2 07/86] arm:aspeed: convert valid RAM sizes to data

[Joel Stanley]

On Wed, 15 Jan 2020 at 15:10, Igor Mammedov <address@hidden> wrote:
>
> various foo_rambits() hardcode mapping of RAM sizes to RAM feature bits,
> which is hard to reuse and repeats over and over.
>
> Convert maps into GLib's hash tables and perform mapping using
> common mapping function.

Thanks for the patch.

I find the existing code straight forward to understand, and for this
reason I would prefer to leave it as it is. Would you mind dropping
this patch from your series?

>
> Follow up patch will reuse tables for actually checking ram-size
> property.
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> ---
>  include/hw/misc/aspeed_sdmc.h |   2 +
>  hw/misc/aspeed_sdmc.c         | 116 
> ++++++++++++++++--------------------------
>  2 files changed, 47 insertions(+), 71 deletions(-)
>
> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
> index 5dbde59..de1501f 100644
> --- a/include/hw/misc/aspeed_sdmc.h
> +++ b/include/hw/misc/aspeed_sdmc.h
> @@ -39,6 +39,8 @@ typedef struct AspeedSDMCState {
>  typedef struct AspeedSDMCClass {
>      SysBusDeviceClass parent_class;
>
> +    GHashTable *ram2feat;
> +    int fallback_ram_size;
>      uint64_t max_ram_size;
>      uint32_t (*compute_conf)(AspeedSDMCState *s, uint32_t data);
>      void (*write)(AspeedSDMCState *s, uint32_t reg, uint32_t data);
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 2df3244..3fc80f0 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -148,72 +148,6 @@ static const MemoryRegionOps aspeed_sdmc_ops = {
>      .valid.max_access_size = 4,
>  };
>
> -static int ast2400_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 64:
> -        return ASPEED_SDMC_DRAM_64MB;
> -    case 128:
> -        return ASPEED_SDMC_DRAM_128MB;
> -    case 256:
> -        return ASPEED_SDMC_DRAM_256MB;
> -    case 512:
> -        return ASPEED_SDMC_DRAM_512MB;
> -    default:
> -        break;
> -    }
> -
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 256M",
> -                s->ram_size);
> -    s->ram_size = 256 << 20;
> -    return ASPEED_SDMC_DRAM_256MB;
> -}
> -
> -static int ast2500_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 128:
> -        return ASPEED_SDMC_AST2500_128MB;
> -    case 256:
> -        return ASPEED_SDMC_AST2500_256MB;
> -    case 512:
> -        return ASPEED_SDMC_AST2500_512MB;
> -    case 1024:
> -        return ASPEED_SDMC_AST2500_1024MB;
> -    default:
> -        break;
> -    }
> -
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
> -                s->ram_size);
> -    s->ram_size = 512 << 20;
> -    return ASPEED_SDMC_AST2500_512MB;
> -}
> -
> -static int ast2600_rambits(AspeedSDMCState *s)
> -{
> -    switch (s->ram_size >> 20) {
> -    case 256:
> -        return ASPEED_SDMC_AST2600_256MB;
> -    case 512:
> -        return ASPEED_SDMC_AST2600_512MB;
> -    case 1024:
> -        return ASPEED_SDMC_AST2600_1024MB;
> -    case 2048:
> -        return ASPEED_SDMC_AST2600_2048MB;
> -    default:
> -        break;
> -    }
> -
> -    /* use a common default */
> -    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 1024M",
> -                s->ram_size);
> -    s->ram_size = 1024 << 20;
> -    return ASPEED_SDMC_AST2600_1024MB;
> -}
> -
>  static void aspeed_sdmc_reset(DeviceState *dev)
>  {
>      AspeedSDMCState *s = ASPEED_SDMC(dev);
> @@ -257,11 +191,14 @@ static Property aspeed_sdmc_properties[] = {
>  static void aspeed_sdmc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    AspeedSDMCClass *asc = ASPEED_SDMC_CLASS(klass);
> +
>      dc->realize = aspeed_sdmc_realize;
>      dc->reset = aspeed_sdmc_reset;
>      dc->desc = "ASPEED SDRAM Memory Controller";
>      dc->vmsd = &vmstate_aspeed_sdmc;
>      dc->props = aspeed_sdmc_properties;
> +    asc->ram2feat = g_hash_table_new(g_direct_hash, g_direct_equal);
>  }
>
>  static const TypeInfo aspeed_sdmc_info = {
> @@ -273,10 +210,28 @@ static const TypeInfo aspeed_sdmc_info = {
>      .abstract   = true,
>  };
>
> +static int aspeed_get_ram_feat(AspeedSDMCState *s)
> +{
> +    AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
> +    int ram_mb = s->ram_size >> 20;
> +    gpointer val;
> +
> +    if (g_hash_table_contains(asc->ram2feat, GINT_TO_POINTER(ram_mb))) {
> +        val = g_hash_table_lookup(asc->ram2feat, GINT_TO_POINTER(ram_mb));
> +        return GPOINTER_TO_INT(val);
> +    }
> +
> +    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default %dM",
> +                 s->ram_size, asc->fallback_ram_size);
> +    s->ram_size = asc->fallback_ram_size << 20;
> +    val = g_hash_table_lookup(asc->ram2feat, &asc->fallback_ram_size);
> +    return GPOINTER_TO_INT(val);
> +}
> +
>  static uint32_t aspeed_2400_sdmc_compute_conf(AspeedSDMCState *s, uint32_t 
> data)
>  {
> -    uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT |
> -        ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
> +    int ram_f = aspeed_get_ram_feat(s);
> +    uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT | 
> ASPEED_SDMC_DRAM_SIZE(ram_f);
>
>      /* Make sure readonly bits are kept */
>      data &= ~ASPEED_SDMC_READONLY_MASK;
> @@ -298,6 +253,9 @@ static void aspeed_2400_sdmc_write(AspeedSDMCState *s, 
> uint32_t reg,
>      s->regs[reg] = data;
>  }
>
> +#define REGISTER_RAM_SIZE(h, k, v) \
> +    g_hash_table_insert(h->ram2feat, GINT_TO_POINTER(k), GINT_TO_POINTER(v))
> +
>  static void aspeed_2400_sdmc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -307,6 +265,11 @@ static void aspeed_2400_sdmc_class_init(ObjectClass 
> *klass, void *data)
>      asc->max_ram_size = 512 << 20;
>      asc->compute_conf = aspeed_2400_sdmc_compute_conf;
>      asc->write = aspeed_2400_sdmc_write;
> +    asc->fallback_ram_size = 256;
> +    REGISTER_RAM_SIZE(asc, 64, ASPEED_SDMC_DRAM_64MB);
> +    REGISTER_RAM_SIZE(asc, 128, ASPEED_SDMC_DRAM_128MB);
> +    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_DRAM_256MB);
> +    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_DRAM_512MB);
>  }
>
>  static const TypeInfo aspeed_2400_sdmc_info = {
> @@ -317,10 +280,10 @@ static const TypeInfo aspeed_2400_sdmc_info = {
>
>  static uint32_t aspeed_2500_sdmc_compute_conf(AspeedSDMCState *s, uint32_t 
> data)
>  {
> +    int ram_f = aspeed_get_ram_feat(s);
>      uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
>          ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> -        ASPEED_SDMC_CACHE_INITIAL_DONE |
> -        ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
> +        ASPEED_SDMC_CACHE_INITIAL_DONE | ASPEED_SDMC_DRAM_SIZE(ram_f);
>
>      /* Make sure readonly bits are kept */
>      data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> @@ -360,6 +323,11 @@ static void aspeed_2500_sdmc_class_init(ObjectClass 
> *klass, void *data)
>      asc->max_ram_size = 1024 << 20;
>      asc->compute_conf = aspeed_2500_sdmc_compute_conf;
>      asc->write = aspeed_2500_sdmc_write;
> +    asc->fallback_ram_size = 512;
> +    REGISTER_RAM_SIZE(asc, 128, ASPEED_SDMC_AST2500_128MB);
> +    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_AST2500_256MB);
> +    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_AST2500_512MB);
> +    REGISTER_RAM_SIZE(asc, 1024, ASPEED_SDMC_AST2500_1024MB);
>  }
>
>  static const TypeInfo aspeed_2500_sdmc_info = {
> @@ -370,9 +338,10 @@ static const TypeInfo aspeed_2500_sdmc_info = {
>
>  static uint32_t aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t 
> data)
>  {
> +    int ram_f = aspeed_get_ram_feat(s);
>      uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(3) |
>          ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
> -        ASPEED_SDMC_DRAM_SIZE(ast2600_rambits(s));
> +        ASPEED_SDMC_DRAM_SIZE(ram_f);
>
>      /* Make sure readonly bits are kept (use ast2500 mask) */
>      data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> @@ -413,6 +382,11 @@ static void aspeed_2600_sdmc_class_init(ObjectClass 
> *klass, void *data)
>      asc->max_ram_size = 2048 << 20;
>      asc->compute_conf = aspeed_2600_sdmc_compute_conf;
>      asc->write = aspeed_2600_sdmc_write;
> +    asc->fallback_ram_size = 512;
> +    REGISTER_RAM_SIZE(asc, 256, ASPEED_SDMC_AST2600_256MB);
> +    REGISTER_RAM_SIZE(asc, 512, ASPEED_SDMC_AST2600_512MB);
> +    REGISTER_RAM_SIZE(asc, 1024, ASPEED_SDMC_AST2600_1024MB);
> +    REGISTER_RAM_SIZE(asc, 2048, ASPEED_SDMC_AST2600_2048MB);
>  }
>
>  static const TypeInfo aspeed_2600_sdmc_info = {
> --
> 2.7.4
>