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 <address@hidden> 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 <address@hidden>
> > ---
> >  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 <address@hidden>
> > + *
> > + * 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