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

2014-01-11 Thread Peter Crosthwaite 
On Wed, Dec 11, 2013 at 11:56 PM, Michel Pollet buser...@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 buser...@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 000..b7cd1ff
 --- /dev/null
 +++ b/hw/arm/imx23_digctl.c
 @@ -0,0 +1,110 @@
 +/*
 + * imx23_digctl.c
 + *
 + * Copyright: Michel Pollet buser...@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] = 0x3780;  /* 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

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

2014-01-11 Thread Peter Crosthwaite 
On Thu, Jan 9, 2014 at 4:55 AM, Peter Maydell peter.mayd...@linaro.org wrote:
 On 8 January 2014 18:39, M P buser...@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

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

2014-01-08 Thread M P 
On 6 January 2014 15:46, Peter Maydell peter.mayd...@linaro.org wrote:

 On 11 December 2013 13:56, Michel Pollet buser...@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 buser...@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 000..b7cd1ff
  --- /dev/null
  +++ b/hw/arm/imx23_digctl.c
  @@ -0,0 +1,110 @@
  +/*
  + * imx23_digctl.c
  + *
  + * Copyright: Michel Pollet buser...@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

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

2014-01-08 Thread Peter Maydell 
On 8 January 2014 18:39, M P buser...@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

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

2014-01-06 Thread Peter Maydell 
On 11 December 2013 13:56, Michel Pollet buser...@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 buser...@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 000..b7cd1ff
 --- /dev/null
 +++ b/hw/arm/imx23_digctl.c
 @@ -0,0 +1,110 @@
 +/*
 + * imx23_digctl.c
 + *
 + * Copyright: Michel Pollet buser...@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

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

2013-12-11 Thread Michel Pollet 
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 buser...@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 000..b7cd1ff
--- /dev/null
+++ b/hw/arm/imx23_digctl.c
@@ -0,0 +1,110 @@
+/*
+ * imx23_digctl.c
+ *
+ * Copyright: Michel Pollet buser...@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;
+
+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] = 0x3780;  /* i.mX233 */
+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