Re: [Qemu-devel] [PATCH v2 01/20] arm: add Faraday a36x SoC platform support

[Andreas Färber]

Hi,

Am 01.02.2013 02:39, schrieb Kuo-Jung Su:
> 2013/2/1 Igor Mitsyanko <address@hidden>
>>
>> On 01/25/2013 12:19 PM, Kuo-Jung Su wrote:
>>>
>>> +/* Board init. */
>>> +static void
>>> +a360_device_init(a360_state *s)
>>> +{
>>> +    qemu_irq *pic;
>>> +    qemu_irq ack, req;
>>> +    qemu_irq cs_line;
>>> +    DeviceState *ds;
>>> +    int i, done_nic = 0, nr_flash = 1;
>>> +    SSIBus *spi;
>>> +    DeviceState *fl;
>>> +
>>> +    /* Interrupt Controller */
>>> +    pic = ftintc020_init(0x98800000, s->cpu);
>>
>>
>>
>> You haven't introduced this interrupt controller yet, patches should be 
>> arranged in such an order that they at least wouldn't break a build.
>> Same goes for ftintc020_init and ftgmac100_init.
> 
> 
> I thought that's why patch set is designed for.
> And I susposed to split up each component and send out patches one by one?
[...]
>>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>>> index 6d049e7..c7bb10e 100644
>>> --- a/hw/arm/Makefile.objs
>>> +++ b/hw/arm/Makefile.objs
>>> @@ -1,4 +1,10 @@
>>>   obj-y = integratorcp.o versatilepb.o arm_pic.o
>>> +obj-y += a360.o a369.o \
>>> +                               rom.o ftdmac020.o ftapbbrg020.o \
>>> +                               ftintc020.o fttmr010.o ftpwmtmr010.o \
>>> +                               ftspi020.o ftssp010.o fti2c010.o \
>>> +                               ftrtc011.o ftwdt010.o ftmac110.o 
>>> ftgmac100.o ftlcdc200.o \
>>> +                               fttsc010.o ftkbc010.o ftnandc021.o 
>>> ftsdc010.o
>>
>>
>> No such files exist at this point, you should add them here one by one in a 
>> corresponding patch.
>> And tabs should be replaced with spaces.
> 
> It looks like that I really have to split up each of one by one.
> That's good to me, because I don't have any common sense about patch process,
> Each component in one patch would be much more easier to me.
> 
>>>   obj-y += arm_boot.o
>>>   obj-y += xilinx_zynq.o zynq_slcr.o
>>>   obj-y += xilinx_spips.o
>>> diff --git a/hw/faraday.h b/hw/faraday.h
>>> new file mode 100644
>>> index 0000000..f4fe0cc
>>> --- /dev/null
>>> +++ b/hw/faraday.h
>>
>>
>>
>> None of three function prototyped in this file exists at this point, I think 
>> you should add this file later in the patch set.
> 
> Same issue.
> I'm studying patchwork now, because I don't want to [act] like a idiot,
> I'll send out the new patch when I 'though' I'm ready.

The criteria is that every patch must be compilable on its own, we say
'bisectable' because it allows to work with git-bisect command for error
searching. Someone who is debugging an x86 problem might land on an arm
a36x commit and otherwise not be able to compile.

You can compare my Tegra patchset here (not polished yet):
http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/tegra

You can add a stub version of your machine and step by step add devices
to it until it is complete. My Tegra2 SoC modeling with a QOM object is
not final yet but might serve as design inspiration. I.e., all A36x SoC
devices should have their state structs either in their own header or in
a SoC-specific header, so that they can be embedded as fields within a
SoC state struct - be it within your patchset or as a later step.

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