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 <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?

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