[Qemu-devel] [RFC 0/5] add initial support for Canon DIGIC SoC

[Qemu-devel] [RFC 0/5] add initial support for Canon DIGIC SoC

[Antony Pavlov]

[RFC 1/5] target-arm: add ARM946E-S CPU
[RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC
[RFC 3/5] hw/arm/digic: add timer support
[RFC 4/5] hw/arm/digic: add UART support
[RFC 5/5] hw/arm/digic: add NOR ROM support

DIGIC is Canon Inc.'s name for a family of SoC
for digital cameras and camcorders.

See http://en.wikipedia.org/wiki/DIGIC for details.

There is no publicly available specification for
DIGIC chips. All information about DIGIC chip
internals is based on reverse engineering efforts
made by CHDK (http://chdk.wikia.com) and
Magic Lantern (http://www.magiclantern.fm) projects
contributors.

Also this patch series adds initial support for Canon
PowerShot A1100 IS compact camera (it is my only camera
with connected UART interface). As the DIGIC-based cameras
differences mostly are unsignificant (e.g. RAM-size,
ROM type and size, GPIO usage) the other compact
and DSLR cameras support can be easely added.

This DIGIC support patch series is inspired
by EOS QEMU from Magic Lantern project.
The main differences:
 * EOS QEMU uses home-brew all-in-one monolith design;
 this patch series uses conventional qemu object-centric design;
 * EOS QEMU tries provide simplest emulation for most
 controllers inside SoC to run Magic Lantern firmware;
 this patch series provide more complete support
 only for core devices to run barebox bootloader.
  ** EOS QEMU does not support timer counting
  (this patch series emulate 1 MHz counting);
  ** EOS QEMU support DIGIC UART only for output
  character to stderr; (this patch series emulate
  introduces full blown UART interface);
  ** EOS QEMU has incomplete ROM support;
  (this patch series uses conventional qemu pflash).

This initial DIGIC support can't be used to run
the original camera firmware, but it can successfully
run experimental version of barebox bootloader
(see http://www.barebox.org).

The last sources of barebox for PowerShot A1100 can be
obtained here:
  https://github.com/frantony/barebox/tree/next.digic.20130829

The precompiled ROM image usable with qemu can be
obtained here:
  https://github.com/frantony/barebox/blob/next.digic.20130829/canon-a1100-rom1.bin

This ROM image (after "dancing bit" encoding) can be run on
real Canon A1100 camera.

The short build instruction for __previous__ DIGIC barebox
version (it can be used with more recent sources too) can
be obtained here:
  http://lists.infradead.org/pipermail/barebox/2013-August/016007.html

[Qemu-devel] [RFC 1/5] target-arm: add ARM946E-S CPU

[Antony Pavlov]

This is slightly altered version of ARM946E-S CPU code
from EOS QEMU (Magic Lantern project) so nearly all
credits go to @a1ex.

ARM946E-S Technical Reference Manual can be found here:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/index.html

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 target-arm/cpu.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index b2556c6..409a311 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -322,6 +322,18 @@ static void arm926_initfn(Object *obj)
     cpu->reset_sctlr = 0x00090078;
 }
 
+static void arm946es_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    set_feature(&cpu->env, ARM_FEATURE_V5);
+    set_feature(&cpu->env, ARM_FEATURE_MPU);
+    set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
+    cpu->midr = 0x41059461;
+    cpu->ctr = (7 << 25) | (1 << 24) | (4 << 18) | (4 << 15) \
+             | (2 << 12) | (4 << 6) | (4 << 3) | (2 << 0);
+    cpu->reset_sctlr = 0x00000078;
+}
+
 static void arm946_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -843,6 +855,7 @@ typedef struct ARMCPUInfo {
 
 static const ARMCPUInfo arm_cpus[] = {
     { .name = "arm926",      .initfn = arm926_initfn },
+    { .name = "arm946e-s",   .initfn = arm946es_initfn },
     { .name = "arm946",      .initfn = arm946_initfn },
     { .name = "arm1026",     .initfn = arm1026_initfn },
     /* What QEMU calls "arm1136-r2" is actually the 1136 r0p2, i.e. an
-- 
1.8.4.rc3

[Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Antony Pavlov]

DIGIC is Canon Inc.'s name for a family of SoC
for digital cameras and camcorders.

There is no publicly available specification for
DIGIC chips. All information about DIGIC chip
internals is based on reverse engineering efforts
made by CHDK (http://chdk.wikia.com) and
Magic Lantern (http://www.magiclantern.fm) projects
contributors.

Also this patch adds initial support for Canon
PowerShot A1100 IS compact camera.

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 default-configs/arm-softmmu.mak |  1 +
 hw/arm/Makefile.objs            |  2 +-
 hw/arm/digic.c                  | 85 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/digic.c

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ac0815d..0d1d783 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -63,6 +63,7 @@ CONFIG_FRAMEBUFFER=y
 CONFIG_XILINX_SPIPS=y
 
 CONFIG_A9SCU=y
+CONFIG_DIGIC=y
 CONFIG_MARVELL_88W8618=y
 CONFIG_OMAP=y
 CONFIG_TSC210X=y
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 3671b42..53d5ffd 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -1,4 +1,4 @@
-obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
+obj-y += boot.o collie.o digic.o exynos4_boards.o gumstix.o highbank.o
 obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
 obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
 obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o
diff --git a/hw/arm/digic.c b/hw/arm/digic.c
new file mode 100644
index 0000000..3259b38
--- /dev/null
+++ b/hw/arm/digic.c
@@ -0,0 +1,85 @@
+/*
+ * QEMU model of the Canon SoC.
+ *
+ * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
+ *
+ * This model is based on reverse engineering efforts
+ * made by CHDK (http://chdk.wikia.com) and
+ * Magic Lantern (http://www.magiclantern.fm) projects
+ * contributors.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "exec/address-spaces.h"
+#include "hw/sysbus.h"
+#include "hw/boards.h"
+
+typedef struct {
+    ARMCPU *cpu;
+    MemoryRegion ram;
+} DigicState;
+
+typedef struct {
+    hwaddr ram_size;
+} DigicBoard;
+
+static DigicState *digic4_create(void)
+{
+    DigicState *s = g_new(DigicState, 1);
+
+    s->cpu = cpu_arm_init("arm946e-s");
+    if (!s->cpu) {
+        fprintf(stderr, "Unable to find CPU definition\n");
+        exit(1);
+    }
+
+    return s;
+}
+
+static void digic4_setup_ram(DigicState *s, hwaddr ram_size)
+{
+    memory_region_init_ram(&s->ram, NULL, "ram", ram_size);
+    memory_region_add_subregion(get_system_memory(), 0, &s->ram);
+    vmstate_register_ram_global(&s->ram);
+}
+
+static void init_digic4_board(DigicBoard *board)
+{
+    DigicState *s = digic4_create();
+
+    digic4_setup_ram(s, board->ram_size);
+}
+
+static DigicBoard a1100_board = {
+    .ram_size = 64 * 1024 * 1024,
+};
+
+static void init_a1100(QEMUMachineInitArgs *args)
+{
+    init_digic4_board(&a1100_board);
+}
+
+static QEMUMachine canon_a1100 = {
+    .name = "canon-a1100",
+    .desc = "Canon PowerShot A1100 IS",
+    .init = &init_a1100,
+};
+
+static void digic_machine_init(void)
+{
+    qemu_register_machine(&canon_a1100);
+}
+machine_init(digic_machine_init);
-- 
1.8.4.rc3

[Qemu-devel] [RFC 3/5] hw/arm/digic: add timer support

[Antony Pavlov]

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 hw/arm/digic.c         |   8 +++
 hw/timer/Makefile.objs |   1 +
 hw/timer/digic-timer.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 hw/timer/digic-timer.c

diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index 3259b38..4ddf338 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -27,6 +27,10 @@
 #include "hw/sysbus.h"
 #include "hw/boards.h"
 
+#define DIGIC4_TIMER0    0xc0210000
+#define DIGIC4_TIMER1    0xc0210100
+#define DIGIC4_TIMER2    0xc0210200
+
 typedef struct {
     ARMCPU *cpu;
     MemoryRegion ram;
@@ -46,6 +50,10 @@ static DigicState *digic4_create(void)
         exit(1);
     }
 
+    sysbus_create_simple("digic-timer", DIGIC4_TIMER0, NULL);
+    sysbus_create_simple("digic-timer", DIGIC4_TIMER1, NULL);
+    sysbus_create_simple("digic-timer", DIGIC4_TIMER2, NULL);
+
     return s;
 }
 
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index eca5905..5479aee 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -25,5 +25,6 @@ obj-$(CONFIG_OMAP) += omap_synctimer.o
 obj-$(CONFIG_PXA2XX) += pxa2xx_timer.o
 obj-$(CONFIG_SH4) += sh_timer.o
 obj-$(CONFIG_TUSB6010) += tusb6010.o
+obj-$(CONFIG_DIGIC) += digic-timer.o
 
 obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
diff --git a/hw/timer/digic-timer.c b/hw/timer/digic-timer.c
new file mode 100644
index 0000000..8c4cad8
--- /dev/null
+++ b/hw/timer/digic-timer.c
@@ -0,0 +1,140 @@
+/*
+ * QEMU model of the Canon Digic timer block.
+ *
+ * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
+ *
+ * This model is based on reverse engineering efforts
+ * made by CHDK (http://chdk.wikia.com) and
+ * Magic Lantern (http://www.magiclantern.fm) projects
+ * contributors.
+ *
+ * See "Timer/Clock Module" docs here:
+ *   http://magiclantern.wikia.com/wiki/Register_Map
+ *
+ * The QEMU model of the OSTimer in PKUnity SoC by Guan Xuetao
+ * is used as a template.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "hw/sysbus.h"
+#include "hw/ptimer.h"
+#include "qemu/main-loop.h"
+
+#ifdef DEBUG_DIGIC_TIMER
+#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define TYPE_DIGIC_TIMER "digic-timer"
+#define DIGIC_TIMER(obj) OBJECT_CHECK(DigicTimerState, (obj), TYPE_DIGIC_TIMER)
+
+# define DIGIC_TIMER_CONTROL 0x00
+# define DIGIC_TIMER_VALUE 0x0c
+
+typedef struct DigicTimerState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    QEMUBH *bh;
+    ptimer_state *ptimer;
+} DigicTimerState;
+
+static uint64_t digic_timer_read(void *opaque, hwaddr offset,
+        unsigned size)
+{
+    DigicTimerState *s = opaque;
+    uint32_t ret = 0;
+
+    switch (offset) {
+    case DIGIC_TIMER_VALUE:
+        ret = (uint32_t)ptimer_get_count(s->ptimer);
+        ret = ret & 0xffff;
+        break;
+    default:
+        DPRINTF("Bad offset %x\n", (int)offset);
+    }
+
+    DPRINTF("offset 0x%x, value 0x%x\n", offset, ret);
+    return ret;
+}
+
+static void digic_timer_write(void *opaque, hwaddr offset,
+        uint64_t value, unsigned size)
+{
+    DigicTimerState *s = opaque;
+
+    /* FIXME: just now we ignore timer enable bit */
+    ptimer_set_limit(s->ptimer, 0x0000ffff, 1);
+    ptimer_run(s->ptimer, 1);
+}
+
+static const MemoryRegionOps digic_timer_ops = {
+    .read = digic_timer_read,
+    .write = digic_timer_write,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void digic_timer_tick(void *opaque)
+{
+    DigicTimerState *s = opaque;
+
+    ptimer_run(s->ptimer, 1);
+}
+
+static int digic_timer_init(SysBusDevice *dev)
+{
+    DigicTimerState *s = DIGIC_TIMER(dev);
+
+    s->bh = qemu_bh_new(digic_timer_tick, s);
+    s->ptimer = ptimer_init(s->bh);
+
+    /* FIXME: there is no documentation on Digic timer
+     * frquency setup so let's it always run on 1 MHz
+     * */
+    ptimer_set_freq(s->ptimer, 1 * 1000 * 1000);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &digic_timer_ops, s,
+            TYPE_DIGIC_TIMER, 0x100);
+    sysbus_init_mmio(dev, &s->iomem);
+
+    return 0;
+}
+
+static void digic_timer_class_init(ObjectClass *klass, void *data)
+{
+    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+
+    sdc->init = digic_timer_init;
+}
+
+static const TypeInfo digic_timer_info = {
+    .name = TYPE_DIGIC_TIMER,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(DigicTimerState),
+    .class_init = digic_timer_class_init,
+};
+
+static void digic_timer_register_type(void)
+{
+    type_register_static(&digic_timer_info);
+}
+
+type_init(digic_timer_register_type)
-- 
1.8.4.rc3

[Qemu-devel] [RFC 4/5] hw/arm/digic: add UART support

[Antony Pavlov]

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 hw/arm/digic.c        |   3 +
 hw/char/Makefile.objs |   1 +
 hw/char/digic-uart.c  | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 211 insertions(+)
 create mode 100644 hw/char/digic-uart.c

diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index 4ddf338..20a06a7 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -30,6 +30,7 @@
 #define DIGIC4_TIMER0    0xc0210000
 #define DIGIC4_TIMER1    0xc0210100
 #define DIGIC4_TIMER2    0xc0210200
+#define DIGIC4_UART      0xc0800000
 
 typedef struct {
     ARMCPU *cpu;
@@ -54,6 +55,8 @@ static DigicState *digic4_create(void)
     sysbus_create_simple("digic-timer", DIGIC4_TIMER1, NULL);
     sysbus_create_simple("digic-timer", DIGIC4_TIMER2, NULL);
 
+    sysbus_create_simple("digic-uart", DIGIC4_UART, NULL);
+
     return s;
 }
 
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index f8f3dbc..00d37ac 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -14,6 +14,7 @@ obj-$(CONFIG_COLDFIRE) += mcf_uart.o
 obj-$(CONFIG_OMAP) += omap_uart.o
 obj-$(CONFIG_SH4) += sh_serial.o
 obj-$(CONFIG_PSERIES) += spapr_vty.o
+obj-$(CONFIG_DIGIC) += digic-uart.o
 
 common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
 common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
diff --git a/hw/char/digic-uart.c b/hw/char/digic-uart.c
new file mode 100644
index 0000000..0bea32e
--- /dev/null
+++ b/hw/char/digic-uart.c
@@ -0,0 +1,207 @@
+/*
+ * QEMU model of the Canon Digic UART block.
+ *
+ * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
+ *
+ * This model is based on reverse engineering efforts
+ * made by CHDK (http://chdk.wikia.com) and
+ * Magic Lantern (http://www.magiclantern.fm) projects
+ * contributors.
+ *
+ * See "Serial terminal" docs here:
+ *   http://magiclantern.wikia.com/wiki/Register_Map#Misc_Registers
+ *
+ * The QEMU model of the Milkymist UART block by Michael Walle
+ * is used as a template.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+#include "sysemu/char.h"
+#include "qemu/error-report.h"
+
+enum {
+    R_TX = 0x00,
+    R_RX,
+    R_ST = (0x14 >> 2),
+    R_MAX
+};
+
+enum {
+    ST_RX_RDY = (1 << 0),
+    ST_TX_RDY = (1 << 1),
+};
+
+#define TYPE_DIGIC_UART "digic-uart"
+#define DIGIC_UART(obj) \
+    OBJECT_CHECK(DigicUartState, (obj), TYPE_DIGIC_UART)
+
+struct DigicUartState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion regs_region;
+    CharDriverState *chr;
+
+    uint32_t regs[R_MAX];
+};
+typedef struct DigicUartState DigicUartState;
+
+static uint64_t uart_read(void *opaque, hwaddr addr,
+                          unsigned size)
+{
+    DigicUartState *s = opaque;
+    uint32_t r = 0;
+
+    addr >>= 2;
+
+    switch (addr) {
+    case R_RX:
+        r = s->regs[addr];
+        s->regs[R_ST] &= ~(ST_RX_RDY);
+        break;
+
+    case R_ST:
+        r = s->regs[addr];
+        break;
+
+    default:
+        error_report("digic_uart: read access to unknown register 0x"
+                TARGET_FMT_plx, addr << 2);
+        break;
+    }
+
+    return r;
+}
+
+static void uart_write(void *opaque, hwaddr addr, uint64_t value,
+                       unsigned size)
+{
+    DigicUartState *s = opaque;
+    unsigned char ch = value;
+
+    addr >>= 2;
+
+    switch (addr) {
+    case R_TX:
+        if (s->chr) {
+            qemu_chr_fe_write(s->chr, &ch, 1);
+        }
+        break;
+
+    case R_ST:
+        /* ignore */
+        break;
+
+    default:
+        error_report("digic_uart: write access to unknown register 0x"
+                TARGET_FMT_plx, addr << 2);
+        break;
+    }
+}
+
+static const MemoryRegionOps uart_mmio_ops = {
+    .read = uart_read,
+    .write = uart_write,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void uart_rx(void *opaque, const uint8_t *buf, int size)
+{
+    DigicUartState *s = opaque;
+
+    assert(!(s->regs[R_ST] & ST_RX_RDY));
+
+    s->regs[R_ST] |= ST_RX_RDY;
+    s->regs[R_RX] = *buf;
+}
+
+static int uart_can_rx(void *opaque)
+{
+    DigicUartState *s = opaque;
+
+    return !(s->regs[R_ST] & ST_RX_RDY);
+}
+
+static void uart_event(void *opaque, int event)
+{
+}
+
+static void digic_uart_reset(DeviceState *d)
+{
+    DigicUartState *s = DIGIC_UART(d);
+    int i;
+
+    for (i = 0; i < R_MAX; i++) {
+        s->regs[i] = 0;
+    }
+    s->regs[R_ST] = ST_TX_RDY;
+}
+
+static int digic_uart_init(SysBusDevice *dev)
+{
+    DigicUartState *s = DIGIC_UART(dev);
+
+    memory_region_init_io(&s->regs_region, OBJECT(s), &uart_mmio_ops, s,
+                          "digic-uart", R_MAX * 4);
+    sysbus_init_mmio(dev, &s->regs_region);
+
+    s->chr = qemu_char_get_next_serial();
+    if (s->chr) {
+        qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_digic_uart = {
+    .name = "digic-uart",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, DigicUartState, R_MAX),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void digic_uart_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = digic_uart_init;
+    dc->reset = digic_uart_reset;
+    dc->vmsd = &vmstate_digic_uart;
+}
+
+static const TypeInfo digic_uart_info = {
+    .name          = TYPE_DIGIC_UART,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(DigicUartState),
+    .class_init    = digic_uart_class_init,
+};
+
+static void digic_uart_register_types(void)
+{
+    type_register_static(&digic_uart_info);
+}
+
+type_init(digic_uart_register_types)
-- 
1.8.4.rc3

Re: [Qemu-devel] [RFC 1/5] target-arm: add ARM946E-S CPU

[Peter Maydell]

On 29 August 2013 10:33, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> This is slightly altered version of ARM946E-S CPU code
> from EOS QEMU (Magic Lantern project) so nearly all
> credits go to @a1ex.
>
> ARM946E-S Technical Reference Manual can be found here:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/index.html

I think the CPU we label "arm946" *is* an ARM946E-S -- there's
no other TRM or variant of the 946 listed on the ARM website.
If we're inaccurate about something we should just fix it (and
that should be safe since there's no existing board model which
uses the 946).

-- PMM

Re: [Qemu-devel] [RFC 1/5] target-arm: add ARM946E-S CPU

[Antony Pavlov]

On Thu, 29 Aug 2013 11:44:38 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 29 August 2013 10:33, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > This is slightly altered version of ARM946E-S CPU code
> > from EOS QEMU (Magic Lantern project) so nearly all
> > credits go to @a1ex.
> >
> > ARM946E-S Technical Reference Manual can be found here:
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/index.html
> 
> I think the CPU we label "arm946" *is* an ARM946E-S -- there's
> no other TRM or variant of the 946 listed on the ARM website.
> If we're inaccurate about something we should just fix it (and
> that should be safe since there's no existing board model which
> uses the 946).

Thanks! I'll take a look.

-- 
Best regards,
  Antony Pavlov

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Andreas Färber]

Am 29.08.2013 11:33, schrieb Antony Pavlov:
> DIGIC is Canon Inc.'s name for a family of SoC
> for digital cameras and camcorders.
> 
> There is no publicly available specification for
> DIGIC chips. All information about DIGIC chip
> internals is based on reverse engineering efforts
> made by CHDK (http://chdk.wikia.com) and
> Magic Lantern (http://www.magiclantern.fm) projects
> contributors.
> 
> Also this patch adds initial support for Canon
> PowerShot A1100 IS compact camera.
> 
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  default-configs/arm-softmmu.mak |  1 +
>  hw/arm/Makefile.objs            |  2 +-
>  hw/arm/digic.c                  | 85 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/digic.c
> 
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index ac0815d..0d1d783 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -63,6 +63,7 @@ CONFIG_FRAMEBUFFER=y
>  CONFIG_XILINX_SPIPS=y
>  
>  CONFIG_A9SCU=y
> +CONFIG_DIGIC=y
>  CONFIG_MARVELL_88W8618=y
>  CONFIG_OMAP=y
>  CONFIG_TSC210X=y
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 3671b42..53d5ffd 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -1,4 +1,4 @@
> -obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
> +obj-y += boot.o collie.o digic.o exynos4_boards.o gumstix.o highbank.o
>  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
>  obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
>  obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o
> diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> new file mode 100644
> index 0000000..3259b38
> --- /dev/null
> +++ b/hw/arm/digic.c
> @@ -0,0 +1,85 @@
> +/*
> + * QEMU model of the Canon SoC.
> + *
> + * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
> + *
> + * This model is based on reverse engineering efforts
> + * made by CHDK (http://chdk.wikia.com) and
> + * Magic Lantern (http://www.magiclantern.fm) projects
> + * contributors.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "exec/address-spaces.h"
> +#include "hw/sysbus.h"
> +#include "hw/boards.h"
> +
> +typedef struct {

structs should not be anonymous, just reuse the typedef name.

> +    ARMCPU *cpu;
> +    MemoryRegion ram;
> +} DigicState;
> +
> +typedef struct {
> +    hwaddr ram_size;
> +} DigicBoard;
> +
> +static DigicState *digic4_create(void)
> +{
> +    DigicState *s = g_new(DigicState, 1);
> +
> +    s->cpu = cpu_arm_init("arm946e-s");
> +    if (!s->cpu) {
> +        fprintf(stderr, "Unable to find CPU definition\n");
> +        exit(1);
> +    }
> +
> +    return s;
> +}

Please separate the SoC from the boards by placing them in different
files (and commits).

DigicState should be a QOM type derived from TYPE_DEVICE. Since you're
hardcoding the CPU type, please use object_initialize() to create it
in-place - note we're about to extend that function but rebase will be
trivial.

> +
> +static void digic4_setup_ram(DigicState *s, hwaddr ram_size)
> +{
> +    memory_region_init_ram(&s->ram, NULL, "ram", ram_size);
> +    memory_region_add_subregion(get_system_memory(), 0, &s->ram);
> +    vmstate_register_ram_global(&s->ram);
> +}

Is the RAM on the board or on the SoC? It's in DigicState but
initialized from the board init. If it's on the SoC, then
_add_subregion and _register_ram_global should be in its realizefn.
Otherwise please separate it from the SoC state.

> +
> +static void init_digic4_board(DigicBoard *board)

digic4_init_board to have a common prefix?

> +{
> +    DigicState *s = digic4_create();
> +
> +    digic4_setup_ram(s, board->ram_size);
> +}
> +
> +static DigicBoard a1100_board = {

canon_a110_board? Please use consistent identifiers either with or
without canon_.

> +    .ram_size = 64 * 1024 * 1024,
> +};
> +
> +static void init_a1100(QEMUMachineInitArgs *args)

canon_a1100_init?

> +{
> +    init_digic4_board(&a1100_board);
> +}
> +
> +static QEMUMachine canon_a1100 = {
> +    .name = "canon-a1100",
> +    .desc = "Canon PowerShot A1100 IS",
> +    .init = &init_a1100,
> +};
> +
> +static void digic_machine_init(void)

Better name this digic_register_machines to avoid confusion with machine
init above.

> +{
> +    qemu_register_machine(&canon_a1100);
> +}
> +machine_init(digic_machine_init);

No semicolon after machine_init() please, and please add a white-line
before.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Condello]

On Thu, Aug 29, 2013 at 11:33 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index ac0815d..0d1d783 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -63,6 +63,7 @@ CONFIG_FRAMEBUFFER=y
>  CONFIG_XILINX_SPIPS=y
>
>  CONFIG_A9SCU=y
> +CONFIG_DIGIC=y

IMHO CONFIG_DIGIC should really be CONFIG_DIGIC_4. We don't know how
the DIGIC package will change in the future but it doesn't hurt since
everything else references digic4 already.

Cheers,
Giovanni

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Antony Pavlov]

On Thu, 29 Aug 2013 16:29:30 +0200
Condello <condellog@gmail.com> wrote:

> On Thu, Aug 29, 2013 at 11:33 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> > index ac0815d..0d1d783 100644
> > --- a/default-configs/arm-softmmu.mak
> > +++ b/default-configs/arm-softmmu.mak
> > @@ -63,6 +63,7 @@ CONFIG_FRAMEBUFFER=y
> >  CONFIG_XILINX_SPIPS=y
> >
> >  CONFIG_A9SCU=y
> > +CONFIG_DIGIC=y
> 
> IMHO CONFIG_DIGIC should really be CONFIG_DIGIC_4. We don't know how
> the DIGIC package will change in the future but it doesn't hurt since
> everything else references digic4 already.

There are two reasons to use CONFIG_DIGIC, but not CONFIG_DIGIC4:

1. CONFIG_DIGIC is used not only for SoC support enabling but for IP-block (UART and TIMER) enabling too. But this blocks are used not only in DIGIC4, so if you use
CONFIG_DIGIC4 for SoC enabling you need use something like CONFIG_DIGIC_UART and
CONFIG_DIGIC_TIMER for IP-block enabling. But if you enable your CONFIG_DIGIC4
but CONFIG_DIGIC_UART is disabled then qemu fails to start digic board
(I unintentionally checked this fact yesterday :).
Does we really need this overcomplication?

Just now it is not evident how to make the partition of the code.

2. AFAIR the digic chips are different but the core functionality is the same.
I have red in CHDK forum about a programm for DIGIC3 that run well on DIGIC4.

-- 
Best regards,
  Antony Pavlov

Re: [Qemu-devel] [RFC 1/5] target-arm: add ARM946E-S CPU

[Antony Pavlov]

On Thu, 29 Aug 2013 11:44:38 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 29 August 2013 10:33, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > This is slightly altered version of ARM946E-S CPU code
> > from EOS QEMU (Magic Lantern project) so nearly all
> > credits go to @a1ex.
> >
> > ARM946E-S Technical Reference Manual can be found here:
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/index.html
> 
> I think the CPU we label "arm946" *is* an ARM946E-S -- there's
> no other TRM or variant of the 946 listed on the ARM website.
> If we're inaccurate about something we should just fix it (and
> that should be safe since there's no existing board model which
> uses the 946).

I have just run barebox on conventional "arm946" qemu CPU. It works fine.
IMHO just now we can drop this patch.

If Magic Lantern or CHDK need some specific initial CPU CP15 register state
then we can fix it in the platform code without defining new CPU type.

Any comments?

-- 
Best regards,
  Antony Pavlov

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Antony Pavlov]

On Thu, 29 Aug 2013 14:15:40 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 29.08.2013 11:33, schrieb Antony Pavlov:
> > DIGIC is Canon Inc.'s name for a family of SoC
> > for digital cameras and camcorders.
> > 
> > There is no publicly available specification for
> > DIGIC chips. All information about DIGIC chip
> > internals is based on reverse engineering efforts
> > made by CHDK (http://chdk.wikia.com) and
> > Magic Lantern (http://www.magiclantern.fm) projects
> > contributors.
> > 
> > Also this patch adds initial support for Canon
> > PowerShot A1100 IS compact camera.
> > 
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > ---
> >  default-configs/arm-softmmu.mak |  1 +
> >  hw/arm/Makefile.objs            |  2 +-
> >  hw/arm/digic.c                  | 85 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 87 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/arm/digic.c
> > 
> > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> > index ac0815d..0d1d783 100644
> > --- a/default-configs/arm-softmmu.mak
> > +++ b/default-configs/arm-softmmu.mak
> > @@ -63,6 +63,7 @@ CONFIG_FRAMEBUFFER=y
> >  CONFIG_XILINX_SPIPS=y
> >  
> >  CONFIG_A9SCU=y
> > +CONFIG_DIGIC=y
> >  CONFIG_MARVELL_88W8618=y
> >  CONFIG_OMAP=y
> >  CONFIG_TSC210X=y
> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> > index 3671b42..53d5ffd 100644
> > --- a/hw/arm/Makefile.objs
> > +++ b/hw/arm/Makefile.objs
> > @@ -1,4 +1,4 @@
> > -obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
> > +obj-y += boot.o collie.o digic.o exynos4_boards.o gumstix.o highbank.o
> >  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
> >  obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
> >  obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o
> > diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> > new file mode 100644
> > index 0000000..3259b38
> > --- /dev/null
> > +++ b/hw/arm/digic.c
> > @@ -0,0 +1,85 @@
> > +/*
> > + * QEMU model of the Canon SoC.
> > + *
> > + * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
> > + *
> > + * This model is based on reverse engineering efforts
> > + * made by CHDK (http://chdk.wikia.com) and
> > + * Magic Lantern (http://www.magiclantern.fm) projects
> > + * contributors.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> > +#include "exec/address-spaces.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/boards.h"
> > +
> > +typedef struct {
> 
> structs should not be anonymous, just reuse the typedef name.
> 
> > +    ARMCPU *cpu;
> > +    MemoryRegion ram;
> > +} DigicState;
> > +
> > +typedef struct {
> > +    hwaddr ram_size;
> > +} DigicBoard;
> > +
> > +static DigicState *digic4_create(void)
> > +{
> > +    DigicState *s = g_new(DigicState, 1);
> > +
> > +    s->cpu = cpu_arm_init("arm946e-s");
> > +    if (!s->cpu) {
> > +        fprintf(stderr, "Unable to find CPU definition\n");
> > +        exit(1);
> > +    }
> > +
> > +    return s;
> > +}
> 
> Please separate the SoC from the boards by placing them in different
> files (and commits).

I'm agree.

> DigicState should be a QOM type derived from TYPE_DEVICE. Since you're
> hardcoding the CPU type, please use object_initialize() to create it
> in-place - note we're about to extend that function but rebase will be
> trivial.

I have just examinied platforms with hardcoded cpu: pxa2xx, exynos4210.
They don't use object_initialize().
Is there any significant difference between hardcoded cpu type
and variable cpu type with selected default cpu type?

I try to find a example of object_initialize() usage for cpu initialisation
in repo but I have no success. Can you point me one or explain your design
pattern?
 
> > +
> > +static void digic4_setup_ram(DigicState *s, hwaddr ram_size)
> > +{
> > +    memory_region_init_ram(&s->ram, NULL, "ram", ram_size);
> > +    memory_region_add_subregion(get_system_memory(), 0, &s->ram);
> > +    vmstate_register_ram_global(&s->ram);
> > +}
> 
> Is the RAM on the board or on the SoC? It's in DigicState but
> initialized from the board init. If it's on the SoC, then
> _add_subregion and _register_ram_global should be in its realizefn.
> Otherwise please separate it from the SoC state.

It's not a trivial question!
See "Digging Into 'DIGIC 4' Image Processor (2)" (http://techon.nikkeibp.co.jp/english/NEWS_EN/20090218/165866/).
The authors removed the upper package with a chemical solution and exposed the chips.
The 'DIGIC 4' contains 3 chips in one package:
 * processor itself;
 * 64-Mbit NOR flash memory, the "K8P6415UQB" (note that I have found another flash K8P3215UQB on my Canon A1100: Manufacturer ID: 0xEC, Device ID: 0x7E0301);
 * 512-Mbit SDRAM, the "K4X51323PE" (just the same 64 M RAM as I see with barebox).

So we can assume taht these memory chips are inalienable parts of the SoC.

But the DSLR cameras has much more memory, they need much memory for serial shooting.
E.g. even rather 8-years old DIGIC2-based Canon 20D has 1 GB (!) of RAM.
Another example: DIGIC4-based Canon 600D camera has 32 M of ROM.
You can see that all this DSLR extra RAM and ROM can't be located inside DIGIC4 package!
It is necessery to carry out addition tests.

> > +
> > +static void init_digic4_board(DigicBoard *board)
> 
> digic4_init_board to have a common prefix?
> 
> > +{
> > +    DigicState *s = digic4_create();
> > +
> > +    digic4_setup_ram(s, board->ram_size);
> > +}
> > +
> > +static DigicBoard a1100_board = {
> 
> canon_a110_board? Please use consistent identifiers either with or
> without canon_.
> 
> > +    .ram_size = 64 * 1024 * 1024,
> > +};
> > +
> > +static void init_a1100(QEMUMachineInitArgs *args)
> 
> canon_a1100_init?
> 
> > +{
> > +    init_digic4_board(&a1100_board);
> > +}
> > +
> > +static QEMUMachine canon_a1100 = {
> > +    .name = "canon-a1100",
> > +    .desc = "Canon PowerShot A1100 IS",
> > +    .init = &init_a1100,
> > +};
> > +
> > +static void digic_machine_init(void)
> 
> Better name this digic_register_machines to avoid confusion with machine
> init above.
> 
> > +{
> > +    qemu_register_machine(&canon_a1100);
> > +}
> > +machine_init(digic_machine_init);
> 
> No semicolon after machine_init() please, and please add a white-line
> before.

Already fixed!

-- 
Best regards,
  Antony Pavlov

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Peter Maydell]

On 29 August 2013 20:36, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> On Thu, 29 Aug 2013 14:15:40 +0200
> Andreas Färber <afaerber@suse.de> wrote:
>> DigicState should be a QOM type derived from TYPE_DEVICE. Since you're
>> hardcoding the CPU type, please use object_initialize() to create it
>> in-place - note we're about to extend that function but rebase will be
>> trivial.
>
> I have just examinied platforms with hardcoded cpu: pxa2xx, exynos4210.
> They don't use object_initialize().

You'll find that QEMU is full of old code that hasn't been updated
to the current "best practice". pxa2xx in particular is pretty elderly
and probably a bad example to copy from. object_initialize()
is pretty new, which is why you can't find many examples yet.

>> Is the RAM on the board or on the SoC? It's in DigicState but
>> initialized from the board init. If it's on the SoC, then
>> _add_subregion and _register_ram_global should be in its realizefn.
>> Otherwise please separate it from the SoC state.
>
> It's not a trivial question!
> See "Digging Into 'DIGIC 4' Image Processor (2)" (http://techon.nikkeibp.co.jp/english/NEWS_EN/20090218/165866/).
> The authors removed the upper package with a chemical solution and exposed the chips.
> The 'DIGIC 4' contains 3 chips in one package:
>  * processor itself;
>  * 64-Mbit NOR flash memory, the "K8P6415UQB" (note that I have found another flash K8P3215UQB on my Canon A1100: Manufacturer ID: 0xEC, Device ID: 0x7E0301);
>  * 512-Mbit SDRAM, the "K4X51323PE" (just the same 64 M RAM as I see with barebox).
>
> So we can assume taht these memory chips are inalienable parts of the SoC.

Package-on-Package is really just a funky way of connecting
up separate components (the layers are only connected
together at PCB assembly time, wikipedia tells me), and
indeed you can have different component combinations
(as you've found with the flash memory). So I would suggest
that either:
 (a) just model them all as separate components
   (SoC, memory, flash) instantiated by the board
 (b) model them as separate components, and have a
   "container" component which puts them together and
   then the board just instantiates that.

Whether (b) is worthwhile depends on whether we
expect to have lots of boards that differ but have the
same PoP stack. I suspect (a) is better.

thanks
-- PMM

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Peter Crosthwaite]

Hi,


On Fri, Aug 30, 2013 at 6:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On 29 August 2013 20:36, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > On Thu, 29 Aug 2013 14:15:40 +0200
> > Andreas Färber <afaerber@suse.de> wrote:
> >> DigicState should be a QOM type derived from TYPE_DEVICE. Since you're
> >> hardcoding the CPU type, please use object_initialize() to create it
> >> in-place - note we're about to extend that function but rebase will be
> >> trivial.
> >
> > I have just examinied platforms with hardcoded cpu: pxa2xx, exynos4210.
> > They don't use object_initialize().
>
> You'll find that QEMU is full of old code that hasn't been updated
> to the current "best practice". pxa2xx in particular is pretty elderly
> and probably a bad example to copy from. object_initialize()
> is pretty new, which is why you can't find many examples yet.
>
> >> Is the RAM on the board or on the SoC? It's in DigicState but
> >> initialized from the board init. If it's on the SoC, then
> >> _add_subregion and _register_ram_global should be in its realizefn.
> >> Otherwise please separate it from the SoC state.
> >
> > It's not a trivial question!
> > See "Digging Into 'DIGIC 4' Image Processor (2)" (http://techon.nikkeibp.co.jp/english/NEWS_EN/20090218/165866/).
> > The authors removed the upper package with a chemical solution and exposed the chips.
> > The 'DIGIC 4' contains 3 chips in one package:
> >  * processor itself;

For clarity, do you mean SoC here?

>
> >  * 64-Mbit NOR flash memory, the "K8P6415UQB" (note that I have found another flash K8P3215UQB on my Canon A1100: Manufacturer ID: 0xEC, Device ID: 0x7E0301);
> >  * 512-Mbit SDRAM, the "K4X51323PE" (just the same 64 M RAM as I see with barebox).
> >
> > So we can assume taht these memory chips are inalienable parts of the SoC.
>
> Package-on-Package is really just a funky way of connecting
> up separate components (the layers are only connected
> together at PCB assembly time, wikipedia tells me), and
> indeed you can have different component combinations
> (as you've found with the flash memory). So I would suggest
> that either:
>  (a) just model them all as separate components
>    (SoC, memory, flash) instantiated by the board
>  (b) model them as separate components, and have a
>    "container" component which puts them together and
>    then the board just instantiates that.
>
> Whether (b) is worthwhile depends on whether we
> expect to have lots of boards that differ but have the
> same PoP stack. I suspect (a) is better.

I like (b). If something is sold, branded or soldered as a self
contained package then I think its worth having its own device. FWIW,
I want to do this with Zynq sooner or later as its a SoC that's
modelled as a board.

Regards,
Peter

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [RFC 1/5] target-arm: add ARM946E-S CPU

[Peter Crosthwaite]

On Fri, Aug 30, 2013 at 4:17 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> On Thu, 29 Aug 2013 11:44:38 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 29 August 2013 10:33, Antony Pavlov <antonynpavlov@gmail.com> wrote:
>> > This is slightly altered version of ARM946E-S CPU code
>> > from EOS QEMU (Magic Lantern project) so nearly all
>> > credits go to @a1ex.
>> >
>> > ARM946E-S Technical Reference Manual can be found here:
>> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/index.html
>>
>> I think the CPU we label "arm946" *is* an ARM946E-S -- there's
>> no other TRM or variant of the 946 listed on the ARM website.
>> If we're inaccurate about something we should just fix it (and
>> that should be safe since there's no existing board model which
>> uses the 946).
>
> I have just run barebox on conventional "arm946" qemu CPU. It works fine.
> IMHO just now we can drop this patch.
>
> If Magic Lantern or CHDK need some specific initial CPU CP15 register state
> then we can fix it in the platform code without defining new CPU type.
>
> Any comments?

Object properties the solution?

Regards,
Peter

>
> --
> Best regards,
>   Antony Pavlov
>

Re: [Qemu-devel] [RFC 4/5] hw/arm/digic: add UART support

[Peter Crosthwaite]

Hi Antony,

On Thu, Aug 29, 2013 at 7:33 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  hw/arm/digic.c        |   3 +
>  hw/char/Makefile.objs |   1 +
>  hw/char/digic-uart.c  | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 211 insertions(+)
>  create mode 100644 hw/char/digic-uart.c
>
> diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> index 4ddf338..20a06a7 100644
> --- a/hw/arm/digic.c
> +++ b/hw/arm/digic.c
> @@ -30,6 +30,7 @@
>  #define DIGIC4_TIMER0    0xc0210000
>  #define DIGIC4_TIMER1    0xc0210100
>  #define DIGIC4_TIMER2    0xc0210200
> +#define DIGIC4_UART      0xc0800000
>
>  typedef struct {
>      ARMCPU *cpu;
> @@ -54,6 +55,8 @@ static DigicState *digic4_create(void)
>      sysbus_create_simple("digic-timer", DIGIC4_TIMER1, NULL);
>      sysbus_create_simple("digic-timer", DIGIC4_TIMER2, NULL);
>
> +    sysbus_create_simple("digic-uart", DIGIC4_UART, NULL);
> +
>      return s;
>  }
>
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index f8f3dbc..00d37ac 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -14,6 +14,7 @@ obj-$(CONFIG_COLDFIRE) += mcf_uart.o
>  obj-$(CONFIG_OMAP) += omap_uart.o
>  obj-$(CONFIG_SH4) += sh_serial.o
>  obj-$(CONFIG_PSERIES) += spapr_vty.o
> +obj-$(CONFIG_DIGIC) += digic-uart.o
>
>  common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
>  common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
> diff --git a/hw/char/digic-uart.c b/hw/char/digic-uart.c
> new file mode 100644
> index 0000000..0bea32e
> --- /dev/null
> +++ b/hw/char/digic-uart.c
> @@ -0,0 +1,207 @@
> +/*
> + * QEMU model of the Canon Digic UART block.
> + *
> + * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
> + *
> + * This model is based on reverse engineering efforts
> + * made by CHDK (http://chdk.wikia.com) and
> + * Magic Lantern (http://www.magiclantern.fm) projects
> + * contributors.
> + *
> + * See "Serial terminal" docs here:
> + *   http://magiclantern.wikia.com/wiki/Register_Map#Misc_Registers
> + *
> + * The QEMU model of the Milkymist UART block by Michael Walle
> + * is used as a template.
> + *

FWIW, I think that's an older one.

> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/char.h"
> +#include "qemu/error-report.h"
> +
> +enum {
> +    R_TX = 0x00,
> +    R_RX,
> +    R_ST = (0x14 >> 2),
> +    R_MAX
> +};
> +
> +enum {
> +    ST_RX_RDY = (1 << 0),
> +    ST_TX_RDY = (1 << 1),
> +};
> +
> +#define TYPE_DIGIC_UART "digic-uart"
> +#define DIGIC_UART(obj) \
> +    OBJECT_CHECK(DigicUartState, (obj), TYPE_DIGIC_UART)
> +
> +struct DigicUartState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion regs_region;
> +    CharDriverState *chr;
> +
> +    uint32_t regs[R_MAX];
> +};
> +typedef struct DigicUartState DigicUartState;
> +
> +static uint64_t uart_read(void *opaque, hwaddr addr,
> +                          unsigned size)

I think Andreas may have commented on other patches, but can you use a
descriptive prefix to fn names? For one, it makes your code easier to
debug in gdb when you have unique names for all fns tree wide.

> +{
> +    DigicUartState *s = opaque;
> +    uint32_t r = 0;
> +
> +    addr >>= 2;
> +
> +    switch (addr) {
> +    case R_RX:
> +        r = s->regs[addr];
> +        s->regs[R_ST] &= ~(ST_RX_RDY);
> +        break;
> +
> +    case R_ST:
> +        r = s->regs[addr];
> +        break;
> +
> +    default:
> +        error_report("digic_uart: read access to unknown register 0x"
> +                TARGET_FMT_plx, addr << 2);

qemu_log(LOG_GUEST_ERROR, ... here instead of error_report I think.
error_report is more about QEMU misbehaving, not the guest sw.

> +        break;
> +    }
> +
> +    return r;

Can you just return s->regs[addr] directly here to avoid unneeded
variable r and its repeated setting to the same expression?

> +}
> +
> +static void uart_write(void *opaque, hwaddr addr, uint64_t value,
> +                       unsigned size)
> +{
> +    DigicUartState *s = opaque;
> +    unsigned char ch = value;
> +
> +    addr >>= 2;
> +
> +    switch (addr) {
> +    case R_TX:
> +        if (s->chr) {
> +            qemu_chr_fe_write(s->chr, &ch, 1);

qemu_chr_fe_write() is capable of returning 0 to indicate EAGAIN (and
friends) and you don't handle this. You can just use
qemu_chr_fe_write_all() to fix.

> +        }
> +        break;
> +
> +    case R_ST:
> +        /* ignore */

This is probably a guest error as you are writing a read only
register? If so, I'd convert the check below to guest error (as I have
commented above) and just drop this special case.

> +        break;
> +
> +    default:
> +        error_report("digic_uart: write access to unknown register 0x"
> +                TARGET_FMT_plx, addr << 2);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps uart_mmio_ops = {
> +    .read = uart_read,
> +    .write = uart_write,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void uart_rx(void *opaque, const uint8_t *buf, int size)
> +{
> +    DigicUartState *s = opaque;
> +
> +    assert(!(s->regs[R_ST] & ST_RX_RDY));
> +

You could just call uart_can_rx instead of replicated expression to
make what your asserting a little more self documenting.

> +    s->regs[R_ST] |= ST_RX_RDY;
> +    s->regs[R_RX] = *buf;
> +}
> +
> +static int uart_can_rx(void *opaque)
> +{
> +    DigicUartState *s = opaque;
> +
> +    return !(s->regs[R_ST] & ST_RX_RDY);
> +}
> +
> +static void uart_event(void *opaque, int event)
> +{
> +}
> +
> +static void digic_uart_reset(DeviceState *d)
> +{
> +    DigicUartState *s = DIGIC_UART(d);
> +    int i;
> +
> +    for (i = 0; i < R_MAX; i++) {
> +        s->regs[i] = 0;
> +    }
> +    s->regs[R_ST] = ST_TX_RDY;
> +}
> +
> +static int digic_uart_init(SysBusDevice *dev)
> +{
> +    DigicUartState *s = DIGIC_UART(dev);
> +
> +    memory_region_init_io(&s->regs_region, OBJECT(s), &uart_mmio_ops, s,
> +                          "digic-uart", R_MAX * 4);
> +    sysbus_init_mmio(dev, &s->regs_region);
> +
> +    s->chr = qemu_char_get_next_serial();
> +    if (s->chr) {
> +        qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_digic_uart = {
> +    .name = "digic-uart",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, DigicUartState, R_MAX),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void digic_uart_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +

Use of SysBusDevice::init is deprecated. Please use Device::realize
instead of SysBusDevice::init. Check dma/pl330.c for an example of the
pattern.

Regards,
Peter

> +    k->init = digic_uart_init;
> +    dc->reset = digic_uart_reset;
> +    dc->vmsd = &vmstate_digic_uart;
> +}
> +
> +static const TypeInfo digic_uart_info = {
> +    .name          = TYPE_DIGIC_UART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(DigicUartState),
> +    .class_init    = digic_uart_class_init,
> +};
> +
> +static void digic_uart_register_types(void)
> +{
> +    type_register_static(&digic_uart_info);
> +}
> +
> +type_init(digic_uart_register_types)
> --
> 1.8.4.rc3
>
>

Re: [Qemu-devel] [RFC 1/5] target-arm: add ARM946E-S CPU

[Peter Maydell]

On 30 August 2013 06:09, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Fri, Aug 30, 2013 at 4:17 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
>> If Magic Lantern or CHDK need some specific initial CPU CP15 register state
>> then we can fix it in the platform code without defining new CPU type.
>>
>> Any comments?
>
> Object properties the solution?

Well, if we need them, yes, but as I say there is no other board
using our 946 so the simplest thing is probably to make that 946 model
behave the way this board's CPU does.

-- PMM

Re: [Qemu-devel] [RFC 1/5] target-arm: add ARM946E-S CPU

[Antony Pavlov]

On Fri, 30 Aug 2013 08:29:09 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 30 August 2013 06:09, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> > On Fri, Aug 30, 2013 at 4:17 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> >> If Magic Lantern or CHDK need some specific initial CPU CP15 register state
> >> then we can fix it in the platform code without defining new CPU type.
> >>
> >> Any comments?
> >
> > Object properties the solution?
> 
> Well, if we need them, yes, but as I say there is no other board
> using our 946 so the simplest thing is probably to make that 946 model
> behave the way this board's CPU does.

I just propose postpone this event as just now there is no user (CHDK or Magic Lantern)
that can get any good of this change.
w
-- 
Best regards,
  Antony Pavlov

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Antony Pavlov]

On Fri, 30 Aug 2013 15:07:49 +1000
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> Hi,
> 
> 
> On Fri, Aug 30, 2013 at 6:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On 29 August 2013 20:36, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > > On Thu, 29 Aug 2013 14:15:40 +0200
> > > Andreas Färber <afaerber@suse.de> wrote:
> > >> DigicState should be a QOM type derived from TYPE_DEVICE. Since you're
> > >> hardcoding the CPU type, please use object_initialize() to create it
> > >> in-place - note we're about to extend that function but rebase will be
> > >> trivial.
> > >
> > > I have just examinied platforms with hardcoded cpu: pxa2xx, exynos4210.
> > > They don't use object_initialize().
> >
> > You'll find that QEMU is full of old code that hasn't been updated
> > to the current "best practice". pxa2xx in particular is pretty elderly
> > and probably a bad example to copy from. object_initialize()
> > is pretty new, which is why you can't find many examples yet.
> >
> > >> Is the RAM on the board or on the SoC? It's in DigicState but
> > >> initialized from the board init. If it's on the SoC, then
> > >> _add_subregion and _register_ram_global should be in its realizefn.
> > >> Otherwise please separate it from the SoC state.
> > >
> > > It's not a trivial question!
> > > See "Digging Into 'DIGIC 4' Image Processor (2)" (http://techon.nikkeibp.co.jp/english/NEWS_EN/20090218/165866/).
> > > The authors removed the upper package with a chemical solution and exposed the chips.
> > > The 'DIGIC 4' contains 3 chips in one package:
> > >  * processor itself;
> 
> For clarity, do you mean SoC here?
> 
> >
> > >  * 64-Mbit NOR flash memory, the "K8P6415UQB" (note that I have found another flash K8P3215UQB on my Canon A1100: Manufacturer ID: 0xEC, Device ID: 0x7E0301);
> > >  * 512-Mbit SDRAM, the "K4X51323PE" (just the same 64 M RAM as I see with barebox).
> > >
> > > So we can assume taht these memory chips are inalienable parts of the SoC.
> >
> > Package-on-Package is really just a funky way of connecting
> > up separate components (the layers are only connected
> > together at PCB assembly time, wikipedia tells me), and
> > indeed you can have different component combinations
> > (as you've found with the flash memory). So I would suggest
> > that either:
> >  (a) just model them all as separate components
> >    (SoC, memory, flash) instantiated by the board
> >  (b) model them as separate components, and have a
> >    "container" component which puts them together and
> >    then the board just instantiates that.
> >
> > Whether (b) is worthwhile depends on whether we
> > expect to have lots of boards that differ but have the
> > same PoP stack. I suspect (a) is better.
> 
> I like (b). If something is sold, branded or soldered as a self
> contained package then I think its worth having its own device. FWIW,
> I want to do this with Zynq sooner or later as its a SoC that's
> modelled as a board.

The current RFCv1 patch series tends to (a). So I'll keep this till RFCv2 code review :)

-- 
Best regards,
  Antony Pavlov

Re: [Qemu-devel] [RFC 4/5] hw/arm/digic: add UART support

[Antony Pavlov]

On Fri, 30 Aug 2013 15:16:42 +1000
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> Hi Antony,
> 
> On Thu, Aug 29, 2013 at 7:33 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > ---
> >  hw/arm/digic.c        |   3 +
> >  hw/char/Makefile.objs |   1 +
> >  hw/char/digic-uart.c  | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 211 insertions(+)
> >  create mode 100644 hw/char/digic-uart.c
> >
> > diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> > index 4ddf338..20a06a7 100644
> > --- a/hw/arm/digic.c
> > +++ b/hw/arm/digic.c
> > @@ -30,6 +30,7 @@
> >  #define DIGIC4_TIMER0    0xc0210000
> >  #define DIGIC4_TIMER1    0xc0210100
> >  #define DIGIC4_TIMER2    0xc0210200
> > +#define DIGIC4_UART      0xc0800000
> >
> >  typedef struct {
> >      ARMCPU *cpu;
> > @@ -54,6 +55,8 @@ static DigicState *digic4_create(void)
> >      sysbus_create_simple("digic-timer", DIGIC4_TIMER1, NULL);
> >      sysbus_create_simple("digic-timer", DIGIC4_TIMER2, NULL);
> >
> > +    sysbus_create_simple("digic-uart", DIGIC4_UART, NULL);
> > +
> >      return s;
> >  }
> >
> > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> > index f8f3dbc..00d37ac 100644
> > --- a/hw/char/Makefile.objs
> > +++ b/hw/char/Makefile.objs
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_COLDFIRE) += mcf_uart.o
> >  obj-$(CONFIG_OMAP) += omap_uart.o
> >  obj-$(CONFIG_SH4) += sh_serial.o
> >  obj-$(CONFIG_PSERIES) += spapr_vty.o
> > +obj-$(CONFIG_DIGIC) += digic-uart.o
> >
> >  common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
> >  common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
> > diff --git a/hw/char/digic-uart.c b/hw/char/digic-uart.c
> > new file mode 100644
> > index 0000000..0bea32e
> > --- /dev/null
> > +++ b/hw/char/digic-uart.c
> > @@ -0,0 +1,207 @@
> > +/*
> > + * QEMU model of the Canon Digic UART block.
> > + *
> > + * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
> > + *
> > + * This model is based on reverse engineering efforts
> > + * made by CHDK (http://chdk.wikia.com) and
> > + * Magic Lantern (http://www.magiclantern.fm) projects
> > + * contributors.
> > + *
> > + * See "Serial terminal" docs here:
> > + *   http://magiclantern.wikia.com/wiki/Register_Map#Misc_Registers
> > + *
> > + * The QEMU model of the Milkymist UART block by Michael Walle
> > + * is used as a template.
> > + *
> 
> FWIW, I think that's an older one.
> 
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> > +#include "hw/hw.h"
> > +#include "hw/sysbus.h"
> > +#include "sysemu/char.h"
> > +#include "qemu/error-report.h"
> > +
> > +enum {
> > +    R_TX = 0x00,
> > +    R_RX,
> > +    R_ST = (0x14 >> 2),
> > +    R_MAX
> > +};
> > +
> > +enum {
> > +    ST_RX_RDY = (1 << 0),
> > +    ST_TX_RDY = (1 << 1),
> > +};
> > +
> > +#define TYPE_DIGIC_UART "digic-uart"
> > +#define DIGIC_UART(obj) \
> > +    OBJECT_CHECK(DigicUartState, (obj), TYPE_DIGIC_UART)
> > +
> > +struct DigicUartState {
> > +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion regs_region;
> > +    CharDriverState *chr;
> > +
> > +    uint32_t regs[R_MAX];
> > +};
> > +typedef struct DigicUartState DigicUartState;
> > +
> > +static uint64_t uart_read(void *opaque, hwaddr addr,
> > +                          unsigned size)
> 
> I think Andreas may have commented on other patches, but can you use a
> descriptive prefix to fn names? For one, it makes your code easier to
> debug in gdb when you have unique names for all fns tree wide.
> 
> > +{
> > +    DigicUartState *s = opaque;
> > +    uint32_t r = 0;
> > +
> > +    addr >>= 2;
> > +
> > +    switch (addr) {
> > +    case R_RX:
> > +        r = s->regs[addr];
> > +        s->regs[R_ST] &= ~(ST_RX_RDY);
> > +        break;
> > +
> > +    case R_ST:
> > +        r = s->regs[addr];
> > +        break;
> > +
> > +    default:
> > +        error_report("digic_uart: read access to unknown register 0x"
> > +                TARGET_FMT_plx, addr << 2);
> 
> qemu_log(LOG_GUEST_ERROR, ... here instead of error_report I think.
> error_report is more about QEMU misbehaving, not the guest sw.
> 
> > +        break;
> > +    }
> > +
> > +    return r;
> 
> Can you just return s->regs[addr] directly here to avoid unneeded
> variable r and its repeated setting to the same expression?
> 
> > +}
> > +
> > +static void uart_write(void *opaque, hwaddr addr, uint64_t value,
> > +                       unsigned size)
> > +{
> > +    DigicUartState *s = opaque;
> > +    unsigned char ch = value;
> > +
> > +    addr >>= 2;
> > +
> > +    switch (addr) {
> > +    case R_TX:
> > +        if (s->chr) {
> > +            qemu_chr_fe_write(s->chr, &ch, 1);
> 
> qemu_chr_fe_write() is capable of returning 0 to indicate EAGAIN (and
> friends) and you don't handle this. You can just use
> qemu_chr_fe_write_all() to fix.
> 
> > +        }
> > +        break;
> > +
> > +    case R_ST:
> > +        /* ignore */

Thanks for your profitable comments! I'm agree with all of them but thisone.

> This is probably a guest error as you are writing a read only
> register? If so, I'd convert the check below to guest error (as I have
> commented above) and just drop this special case.

There is no guest error here.
The point is that this register is actively used during receiving
and transmitting symbols, but we don't know the function of most of bits.

Please see the barebox digic uart driver:
https://github.com/frantony/barebox/blob/next.digic.20130829/drivers/serial/serial_digic.c#L57

and expecially the comments on the digic_serial_tstc() function:
https://github.com/frantony/barebox/blob/next.digic.20130829/drivers/serial/serial_digic.c#L76

Ignoring writes to R_ST is only a simplification of the model that has no perceptible side effects for existing guests.

Moreover, using existing canon disassembled code I can't unambiguously restore the R_ST behaviour rules.

> > +        break;
> > +
> > +    default:
> > +        error_report("digic_uart: write access to unknown register 0x"
> > +                TARGET_FMT_plx, addr << 2);
> > +        break;
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps uart_mmio_ops = {
> > +    .read = uart_read,
> > +    .write = uart_write,
> > +    .valid = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 4,
> > +    },
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> > +static void uart_rx(void *opaque, const uint8_t *buf, int size)
> > +{
> > +    DigicUartState *s = opaque;
> > +
> > +    assert(!(s->regs[R_ST] & ST_RX_RDY));
> > +
> 
> You could just call uart_can_rx instead of replicated expression to
> make what your asserting a little more self documenting.
> 
> > +    s->regs[R_ST] |= ST_RX_RDY;
> > +    s->regs[R_RX] = *buf;
> > +}
> > +
> > +static int uart_can_rx(void *opaque)
> > +{
> > +    DigicUartState *s = opaque;
> > +
> > +    return !(s->regs[R_ST] & ST_RX_RDY);
> > +}
> > +
> > +static void uart_event(void *opaque, int event)
> > +{
> > +}
> > +
> > +static void digic_uart_reset(DeviceState *d)
> > +{
> > +    DigicUartState *s = DIGIC_UART(d);
> > +    int i;
> > +
> > +    for (i = 0; i < R_MAX; i++) {
> > +        s->regs[i] = 0;
> > +    }
> > +    s->regs[R_ST] = ST_TX_RDY;
> > +}
> > +
> > +static int digic_uart_init(SysBusDevice *dev)
> > +{
> > +    DigicUartState *s = DIGIC_UART(dev);
> > +
> > +    memory_region_init_io(&s->regs_region, OBJECT(s), &uart_mmio_ops, s,
> > +                          "digic-uart", R_MAX * 4);
> > +    sysbus_init_mmio(dev, &s->regs_region);
> > +
> > +    s->chr = qemu_char_get_next_serial();
> > +    if (s->chr) {
> > +        qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_digic_uart = {
> > +    .name = "digic-uart",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .minimum_version_id_old = 1,
> > +    .fields      = (VMStateField[]) {
> > +        VMSTATE_UINT32_ARRAY(regs, DigicUartState, R_MAX),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void digic_uart_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> > +
> 
> Use of SysBusDevice::init is deprecated. Please use Device::realize
> instead of SysBusDevice::init. Check dma/pl330.c for an example of the
> pattern.
> 
> Regards,
> Peter
> 
> > +    k->init = digic_uart_init;
> > +    dc->reset = digic_uart_reset;
> > +    dc->vmsd = &vmstate_digic_uart;
> > +}
> > +
> > +static const TypeInfo digic_uart_info = {
> > +    .name          = TYPE_DIGIC_UART,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(DigicUartState),
> > +    .class_init    = digic_uart_class_init,
> > +};
> > +
> > +static void digic_uart_register_types(void)
> > +{
> > +    type_register_static(&digic_uart_info);
> > +}
> > +
> > +type_init(digic_uart_register_types)
> > --
> > 1.8.4.rc3
> >
> >


-- 
-- 
Best regards,
  Antony Pavlov

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Andreas Färber]

Am 29.08.2013 21:36, schrieb Antony Pavlov:
> On Thu, 29 Aug 2013 14:15:40 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Am 29.08.2013 11:33, schrieb Antony Pavlov:
>>> DIGIC is Canon Inc.'s name for a family of SoC
>>> for digital cameras and camcorders.
>>>
>>> There is no publicly available specification for
>>> DIGIC chips. All information about DIGIC chip
>>> internals is based on reverse engineering efforts
>>> made by CHDK (http://chdk.wikia.com) and
>>> Magic Lantern (http://www.magiclantern.fm) projects
>>> contributors.
>>>
>>> Also this patch adds initial support for Canon
>>> PowerShot A1100 IS compact camera.
>>>
>>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
>>> ---
>>>  default-configs/arm-softmmu.mak |  1 +
>>>  hw/arm/Makefile.objs            |  2 +-
>>>  hw/arm/digic.c                  | 85 +++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 87 insertions(+), 1 deletion(-)
>>>  create mode 100644 hw/arm/digic.c
>>>
>>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>>> index ac0815d..0d1d783 100644
>>> --- a/default-configs/arm-softmmu.mak
>>> +++ b/default-configs/arm-softmmu.mak
>>> @@ -63,6 +63,7 @@ CONFIG_FRAMEBUFFER=y
>>>  CONFIG_XILINX_SPIPS=y
>>>  
>>>  CONFIG_A9SCU=y
>>> +CONFIG_DIGIC=y
>>>  CONFIG_MARVELL_88W8618=y
>>>  CONFIG_OMAP=y
>>>  CONFIG_TSC210X=y
>>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>>> index 3671b42..53d5ffd 100644
>>> --- a/hw/arm/Makefile.objs
>>> +++ b/hw/arm/Makefile.objs
>>> @@ -1,4 +1,4 @@
>>> -obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
>>> +obj-y += boot.o collie.o digic.o exynos4_boards.o gumstix.o highbank.o
>>>  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
>>>  obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
>>>  obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o
>>> diff --git a/hw/arm/digic.c b/hw/arm/digic.c
>>> new file mode 100644
>>> index 0000000..3259b38
>>> --- /dev/null
>>> +++ b/hw/arm/digic.c
>>> @@ -0,0 +1,85 @@
>>> +/*
>>> + * QEMU model of the Canon SoC.
>>> + *
>>> + * Copyright (C) 2013 Antony Pavlov <antonynpavlov@gmail.com>
>>> + *
>>> + * This model is based on reverse engineering efforts
>>> + * made by CHDK (http://chdk.wikia.com) and
>>> + * Magic Lantern (http://www.magiclantern.fm) projects
>>> + * contributors.
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>> + *
>>> + */
>>> +
>>> +#include "exec/address-spaces.h"
>>> +#include "hw/sysbus.h"
>>> +#include "hw/boards.h"
>>> +
>>> +typedef struct {
>>
>> structs should not be anonymous, just reuse the typedef name.
>>
>>> +    ARMCPU *cpu;
>>> +    MemoryRegion ram;
>>> +} DigicState;
>>> +
>>> +typedef struct {
>>> +    hwaddr ram_size;
>>> +} DigicBoard;
>>> +
>>> +static DigicState *digic4_create(void)
>>> +{
>>> +    DigicState *s = g_new(DigicState, 1);
>>> +
>>> +    s->cpu = cpu_arm_init("arm946e-s");
>>> +    if (!s->cpu) {
>>> +        fprintf(stderr, "Unable to find CPU definition\n");
>>> +        exit(1);
>>> +    }
>>> +
>>> +    return s;
>>> +}
>>
>> Please separate the SoC from the boards by placing them in different
>> files (and commits).
> 
> I'm agree.
> 
>> DigicState should be a QOM type derived from TYPE_DEVICE. Since you're
>> hardcoding the CPU type, please use object_initialize() to create it
>> in-place - note we're about to extend that function but rebase will be
>> trivial.
> 
> I have just examinied platforms with hardcoded cpu: pxa2xx, exynos4210.
> They don't use object_initialize().
> Is there any significant difference between hardcoded cpu type
> and variable cpu type with selected default cpu type?
> 
> I try to find a example of object_initialize() usage for cpu initialisation
> in repo but I have no success. Can you point me one or explain your design
> pattern?

My Tegra2 SoC emulation uses the new pattern:
http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/tegra

But you can just look at existing non-CPU users of object_initialize().
For a non-x86 CPU you don't need qdev_set_parent_bus(), but you do need
object_property_set_bool(obj, true, "realized", &err) in your realize
function and error_propagate(errp, err) to your caller.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC

[Andreas Färber]

Am 30.08.2013 07:07, schrieb Peter Crosthwaite:
> Hi,
> 
> 
> On Fri, Aug 30, 2013 at 6:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On 29 August 2013 20:36, Antony Pavlov <antonynpavlov@gmail.com> wrote:
>>> On Thu, 29 Aug 2013 14:15:40 +0200
>>> Andreas Färber <afaerber@suse.de> wrote:
>>>> DigicState should be a QOM type derived from TYPE_DEVICE. Since you're
>>>> hardcoding the CPU type, please use object_initialize() to create it
>>>> in-place - note we're about to extend that function but rebase will be
>>>> trivial.
>>>
>>> I have just examinied platforms with hardcoded cpu: pxa2xx, exynos4210.
>>> They don't use object_initialize().
>>
>> You'll find that QEMU is full of old code that hasn't been updated
>> to the current "best practice". pxa2xx in particular is pretty elderly
>> and probably a bad example to copy from. object_initialize()
>> is pretty new, which is why you can't find many examples yet.
>>
>>>> Is the RAM on the board or on the SoC? It's in DigicState but
>>>> initialized from the board init. If it's on the SoC, then
>>>> _add_subregion and _register_ram_global should be in its realizefn.
>>>> Otherwise please separate it from the SoC state.
>>>
>>> It's not a trivial question!
>>> See "Digging Into 'DIGIC 4' Image Processor (2)" (http://techon.nikkeibp.co.jp/english/NEWS_EN/20090218/165866/).
>>> The authors removed the upper package with a chemical solution and exposed the chips.
>>> The 'DIGIC 4' contains 3 chips in one package:
>>>  * processor itself;
> 
> For clarity, do you mean SoC here?
> 
>>
>>>  * 64-Mbit NOR flash memory, the "K8P6415UQB" (note that I have found another flash K8P3215UQB on my Canon A1100: Manufacturer ID: 0xEC, Device ID: 0x7E0301);
>>>  * 512-Mbit SDRAM, the "K4X51323PE" (just the same 64 M RAM as I see with barebox).
>>>
>>> So we can assume taht these memory chips are inalienable parts of the SoC.
>>
>> Package-on-Package is really just a funky way of connecting
>> up separate components (the layers are only connected
>> together at PCB assembly time, wikipedia tells me), and
>> indeed you can have different component combinations
>> (as you've found with the flash memory). So I would suggest
>> that either:
>>  (a) just model them all as separate components
>>    (SoC, memory, flash) instantiated by the board
>>  (b) model them as separate components, and have a
>>    "container" component which puts them together and
>>    then the board just instantiates that.
>>
>> Whether (b) is worthwhile depends on whether we
>> expect to have lots of boards that differ but have the
>> same PoP stack. I suspect (a) is better.
> 
> I like (b). If something is sold, branded or soldered as a self
> contained package then I think its worth having its own device. FWIW,
> I want to do this with Zynq sooner or later as its a SoC that's
> modelled as a board.

Personally I tend to (a) in this case. As I understood it, this SoC/PoP
only occurs in cameras, not as a reusable component, so the amount of
RAM and variant of the SoC is uniquely identified by the machine and we
wouldn't know how else to name the PoP container object.

A reason for (b) might be if multiple cameras contain the same
pre-produced PoP - not sure if that is the case, and we only have one
board for now.

You are of course right that if we don't think about this now, we risk
having future contributors copy-and-pasting code without reconsidering;
but the struct defining the per-board RAM size is already set up for
sharing code among boards, so I think instantiating them at machine
level will not be a problem for now.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg