Re: [Qemu-devel] [PATCH 06/13] mxs/imx23: Add digctl driver

[Peter Maydell]

On 11 December 2013 13:56, Michel Pollet <address@hidden> wrote:
> This implements just enough of the digctl IO block to allow
> linux to believe it's running on (currently only) an imx23.
>
> Signed-off-by: Michel Pollet <address@hidden>
> ---
>  hw/arm/Makefile.objs  |   1 +
>  hw/arm/imx23_digctl.c | 110 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+)
>  create mode 100644 hw/arm/imx23_digctl.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 78b5614..9adcb96 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -5,3 +5,4 @@ obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o 
> z2.o
>
>  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>  obj-y += omap1.o omap2.o strongarm.o
> +obj-$(CONFIG_MXS) += imx23_digctl.o
> diff --git a/hw/arm/imx23_digctl.c b/hw/arm/imx23_digctl.c
> new file mode 100644
> index 0000000..b7cd1ff
> --- /dev/null
> +++ b/hw/arm/imx23_digctl.c
> @@ -0,0 +1,110 @@
> +/*
> + * imx23_digctl.c
> + *
> + * Copyright: Michel Pollet <address@hidden>
> + *
> + * QEMU Licence
> + */
> +
> +/*
> + * This module implements a very basic IO block for the digctl of the imx23
> + * Basically there is no real logic, just constant registers return, the most
> + * used one bing the "chip id" that is used by the various linux drivers
> + * to differentiate between imx23 and 28.
> + *
> + * The module consists mostly of read/write registers that the bootloader and
> + * kernel are quite happy to 'set' to whatever value they believe they set...
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/arm/mxs.h"
> +
> +enum {
> +    HW_DIGCTL_RAMCTL = 0x3,
> +    HW_DIGCTL_CHIPID = 0x31,
> +};
> +
> +typedef struct imx23_digctl_state {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +
> +    uint32_t   reg[0x2000 / 4];
> +} imx23_digctl_state;

I'm not generally a fan of "big reg[] array" like this.
In real hardware are these typically constant read/only
registers, or do they actually do something? Does the
hardware really have a full set of 2048 registers here,
or are there gaps?

I'd rather have us implement just the minimal set
required for things to boot, with LOG_UNIMP (and
read-zero/write-ignored) for the rest. That makes
it easier to add actual implementations later
(and your migration state is not 0x2000 bytes of random
undifferentiated stuff).

thanks
-- PMM