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

[M P]

On 6 January 2014 15:46, Peter Maydell <address@hidden> wrote:

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?

This block contains most of the 'general purpose' registers, ram timing and all that jazz; there are a lot of write to it at init time, but it's otherwise mostly ignored. Also, there's very little to do about it functionally for qemu's purpose.

 

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).

I will re-add the trace for both write and read and see if I can narrow the range down; it will be linux specific, tho, that's why I thought a 'catchall' block was more appropriate.

 

thanks
-- PMM

Thanks,

M