From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54420) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VF18g-0006pZ-Fk for qemu-devel@nongnu.org; Thu, 29 Aug 2013 08:15:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VF18b-0002rY-BS for qemu-devel@nongnu.org; Thu, 29 Aug 2013 08:15:50 -0400 Received: from cantor2.suse.de ([195.135.220.15]:53985 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VF18a-0002qI-UE for qemu-devel@nongnu.org; Thu, 29 Aug 2013 08:15:45 -0400 Message-ID: <521F3B6C.6040207@suse.de> Date: Thu, 29 Aug 2013 14:15:40 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1377768833-11400-1-git-send-email-antonynpavlov@gmail.com> <1377768833-11400-3-git-send-email-antonynpavlov@gmail.com> In-Reply-To: <1377768833-11400-3-git-send-email-antonynpavlov@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC 2/5] hw/arm: add very initial support for Canon DIGIC SoC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Antony Pavlov Cc: Alex Dumitrache , Peter Crosthwaite , Giovanni Condello , g3gg0 , Peter Maydell , qemu-devel@nongnu.org, Paul Brook , Paolo Bonzini 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. >=20 > 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. >=20 > Also this patch adds initial support for Canon > PowerShot A1100 IS compact camera. >=20 > Signed-off-by: Antony Pavlov > --- > 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 >=20 > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-soft= mmu.mak > index ac0815d..0d1d783 100644 > --- a/default-configs/arm-softmmu.mak > +++ b/default-configs/arm-softmmu.mak > @@ -63,6 +63,7 @@ CONFIG_FRAMEBUFFER=3Dy > CONFIG_XILINX_SPIPS=3Dy > =20 > CONFIG_A9SCU=3Dy > +CONFIG_DIGIC=3Dy > CONFIG_MARVELL_88W8618=3Dy > CONFIG_OMAP=3Dy > CONFIG_TSC210X=3Dy > 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 +=3D boot.o collie.o exynos4_boards.o gumstix.o highbank.o > +obj-y +=3D boot.o collie.o digic.o exynos4_boards.o gumstix.o highbank= .o > obj-y +=3D integratorcp.o kzm.o mainstone.o musicpal.o nseries.o > obj-y +=3D omap_sx1.o palm.o realview.o spitz.o stellaris.o > obj-y +=3D 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 > + * > + * 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 . > + * > + */ > + > +#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 =3D g_new(DigicState, 1); > + > + s->cpu =3D 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 =3D digic4_create(); > + > + digic4_setup_ram(s, board->ram_size); > +} > + > +static DigicBoard a1100_board =3D { canon_a110_board? Please use consistent identifiers either with or without canon_. > + .ram_size =3D 64 * 1024 * 1024, > +}; > + > +static void init_a1100(QEMUMachineInitArgs *args) canon_a1100_init? > +{ > + init_digic4_board(&a1100_board); > +} > + > +static QEMUMachine canon_a1100 =3D { > + .name =3D "canon-a1100", > + .desc =3D "Canon PowerShot A1100 IS", > + .init =3D &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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg