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

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