[06/13] mxs/imx23: Add digctl driver

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 <buserror@gmail.com>
---
 hw/arm/Makefile.objs  |   1 +
 hw/arm/imx23_digctl.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)
 create mode 100644 hw/arm/imx23_digctl.c

On 11 December 2013 13:56, Michel Pollet <buserror@gmail.com> 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 <buserror@gmail.com>
> ---
>  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 <buserror@gmail.com>
> + *
> + * 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

On 6 January 2014 15:46, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 11 December 2013 13:56, Michel Pollet <buserror@gmail.com> 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 <buserror@gmail.com>
> > ---
> >  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 <buserror@gmail.com>
> > + *
> > + * 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

On 8 January 2014 18:39, M P <buserror@gmail.com> wrote:
> 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.

Well, we should be implementing what the hardware does,
generally. Misimplementing things as "read as written"
isn't really any better than misimplementing them as
RAZ/WI, it's just differently wrong.

thanks
-- PMM

On Wed, Dec 11, 2013 at 11:56 PM, Michel Pollet <buserror@gmail.com> 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 <buserror@gmail.com>
> ---
>  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 <buserror@gmail.com>
> + *
> + * 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

"being", "Linux".

Regards,

Peter

> + * 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;
> +
> +static uint64_t imx23_digctl_read(
> +        void *opaque, hwaddr offset, unsigned size)
> +{
> +    imx23_digctl_state *s = (imx23_digctl_state *)opaque;
> +    uint32_t res = 0;
> +
> +    switch (offset >> 4) {
> +       case 0 ... 0x2000/4:
> +               res = s->reg[offset >> 4];
> +               break;
> +        default:
> +               qemu_log_mask(LOG_GUEST_ERROR,
> +                               "%s: bad offset 0x%x\n", __func__, (int)offset);
> +               return 0;
> +    }
> +    return res;
> +}
> +
> +static void imx23_digctl_write(
> +        void *opaque, hwaddr offset, uint64_t value, unsigned size)
> +{
> +    imx23_digctl_state *s = (imx23_digctl_state *) opaque;
> +    uint32_t * dst = NULL;
> +
> +    switch (offset >> 4) {
> +        case 0 ... 0x2000 / 4:
> +            dst = &s->reg[(offset >> 4)];
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                    "%s: bad offset 0x%x\n", __func__, (int) offset);
> +            return;
> +    }
> +    if (!dst) {
> +        return;
> +    }
> +    mxs_write(dst, offset, value, size);
> +}
> +
> +static const MemoryRegionOps imx23_digctl_ops = {
> +    .read = imx23_digctl_read,
> +    .write = imx23_digctl_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int imx23_digctl_init(SysBusDevice *dev)
> +{
> +    imx23_digctl_state *s = OBJECT_CHECK(imx23_digctl_state, dev, "imx23_digctl");
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &imx23_digctl_ops, s,
> +            "imx23_digctl", 0x2000);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    s->reg[HW_DIGCTL_RAMCTL] = 0x6d676953;  /* default reset value */
> +    s->reg[HW_DIGCTL_CHIPID] = 0x37800000;  /* i.mX233 */

Move to rst function.

> +    return 0;
> +}
> +
> +static void imx23_digctl_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    sdc->init = imx23_digctl_init;
> +}
> +
> +static TypeInfo digctl_info = {
> +    .name          = "imx23_digctl",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(imx23_digctl_state),
> +    .class_init    = imx23_digctl_class_init,
> +};
> +
> +static void imx23_digctl_register(void)
> +{
> +    type_register_static(&digctl_info);
> +}
> +
> +type_init(imx23_digctl_register)
> --
> 1.8.5.1
>
>

On Thu, Jan 9, 2014 at 4:55 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 January 2014 18:39, M P <buserror@gmail.com> wrote:
>> 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.

I would suggest you do the ones you care about properly, and
RAZ/WI+qemu_log_mask(LOG_UNIMP, the rest. When it comes to the
unimplemented, hard obvious failure from a totally unresponsive
register is preferrable to subtle infidelities (like being able to
write to RO register).

Regards,
Peter

>
> Well, we should be implementing what the hardware does,
> generally. Misimplementing things as "read as written"
> isn't really any better than misimplementing them as
> RAZ/WI, it's just differently wrong.
>
> thanks
> -- PMM
>