Re: [PATCH v6 01/13] hw/misc: Add NPCM7xx System Global Control Registers device model

[Philippe Mathieu-Daudé]

On 7/17/20 8:02 AM, Havard Skinnemoen wrote:
> Implement a device model for the System Global Control Registers in the
> NPCM730 and NPCM750 BMC SoCs.
> 
> This is primarily used to enable SMP boot (the boot ROM spins reading
> the SCRPAD register) and DDR memory initialization; other registers are
> best effort for now.
> 
> The reset values of the MDLR and PWRON registers are determined by the
> SoC variant (730 vs 750) and board straps respectively.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>  include/hw/misc/npcm7xx_gcr.h |  76 ++++++++++++
>  hw/misc/npcm7xx_gcr.c         | 227 ++++++++++++++++++++++++++++++++++
>  MAINTAINERS                   |   8 ++
>  hw/arm/Kconfig                |   3 +
>  hw/misc/Makefile.objs         |   1 +
>  hw/misc/trace-events          |   4 +
>  6 files changed, 319 insertions(+)
>  create mode 100644 include/hw/misc/npcm7xx_gcr.h
>  create mode 100644 hw/misc/npcm7xx_gcr.c
...

> +static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp)
> +{
> +    ERRP_GUARD();
> +    NPCM7xxGCRState *s = NPCM7XX_GCR(dev);
> +    uint64_t dram_size;
> +    Object *obj;
> +
> +    obj = object_property_get_link(OBJECT(dev), "dram-mr", errp);
> +    if (!obj) {
> +        error_prepend(errp, "%s: required dram-mr link not found: ", 
> __func__);
> +        return;
> +    }
> +    dram_size = memory_region_size(MEMORY_REGION(obj));
> +    if (!is_power_of_2(dram_size) ||
> +        dram_size < NPCM7XX_GCR_MIN_DRAM_SIZE ||
> +        dram_size > NPCM7XX_GCR_MAX_DRAM_SIZE) {
> +        error_setg(errp, "%s: unsupported DRAM size %" PRIu64,
> +                   __func__, dram_size);

Nitpicking if you ever have to respin, please use size_to_str() here,

> +        error_append_hint(errp,
> +                          "DRAM size must be a power of two between %" PRIu64
> +                          " and %" PRIu64 " MiB, inclusive.\n",
> +                          NPCM7XX_GCR_MIN_DRAM_SIZE / MiB,
> +                          NPCM7XX_GCR_MAX_DRAM_SIZE / MiB);

and here.

> +        return;
> +    }
> +
> +    /* Power-on reset value */
> +    s->reset_intcr3 = 0x00001002;
> +
> +    /*
> +     * The GMMAP (Graphics Memory Map) field is used by u-boot to detect the
> +     * DRAM size, and is normally initialized by the boot block as part of 
> DRAM
> +     * training. However, since we don't have a complete emulation of the
> +     * memory controller and try to make it look like it has already been
> +     * initialized, the boot block will skip this initialization, and we need
> +     * to make sure this field is set correctly up front.
> +     *
> +     * WARNING: some versions of u-boot only looks at bits 8 and 9, so 2 GiB 
> of
> +     * DRAM will be interpreted as 128 MiB.
> +     *
> +     * 
> https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244
> +     */
> +    s->reset_intcr3 |= ctz64(dram_size / NPCM7XX_GCR_MIN_DRAM_SIZE) << 8;

Nice :)

> +}
[...]