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

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 <i.mitsyanko@samsung.com>
>>
>> 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

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

[Kuo-Jung Su]

Hi Andreas:

Thanks for the information, and sorry for the mess I've done.
I'll one-by-one re-send all the patches.

However because most of my patches are new files,
should I send-out the patches with detail change log?

For example:

[PATCH] dumb timer
... [PATCH v2 0/2] dumb timer (Cover letter)
    [PATCH v2 1/2] dumb timer (The one in Patch V1)
    [PATCH v2 2/2] dumb timer: coding style update (Change log for V2)
...... [PATCH v3 0/2] dumb timer (Cover letter)
       [PATCH v3 1/2] dumb timer (The merged file in Patch V1 & v2)
       [PATCH v3 2/2] dumb timer: bug fix (Change log for V3)


Best Wishes
Dante

2013/2/1 Andreas Färber <afaerber@suse.de>:
> Hi,
>
> Am 01.02.2013 02:39, schrieb Kuo-Jung Su:
>> 2013/2/1 Igor Mitsyanko <i.mitsyanko@samsung.com>
>>>
>>> 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



-- 
Best wishes,
Kuo-Jung Su

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

[Andreas Färber]

嗨 國榮,

Am 01.02.2013 09:57, schrieb Kuo-Jung Su:
> Thanks for the information, and sorry for the mess I've done.

You don't need to apologize for every review comment you get. It's
meant for improvements of results, not personal. :)

> I'll one-by-one re-send all the patches.
> 
> However because most of my patches are new files,
> should I send-out the patches with detail change log?
> 
> For example:
> 
> [PATCH] dumb timer
> ... [PATCH v2 0/2] dumb timer (Cover letter)
>     [PATCH v2 1/2] dumb timer (The one in Patch V1)
>     [PATCH v2 2/2] dumb timer: coding style update (Change log for V2)
> ...... [PATCH v3 0/2] dumb timer (Cover letter)
>        [PATCH v3 1/2] dumb timer (The merged file in Patch V1 & v2)
>        [PATCH v3 2/2] dumb timer: bug fix (Change log for V3)

No, no, no. What you should do is just something like:

[PATCH v3 0/x] Add Faraday A36x SoC platform support
    [PATCH v3 1/x] arm: Add Faraday A360 and A369 machines
    [PATCH v3 2/x] faraday_a36x: Add FT... timer
    ...

* v3 cover letter contains a change log going back to v1.
* v3 is not a reply to v2 (no --in-reply-to). This aids a threaded mail
display for reviewing and avoids an old version getting reviewed or applied.
* 1+/x are replies to 0/x (usually automatically by git-send-email).
That helps keep the patches together and in the right order.
* Bug fixes of your own code do not go separate (only if you were fixing
existing code from qemu.git). There's no need to introduce bugs and then
to fix them.
* Adding a stub machine in 1/x has the advantage that the patch is much
smaller and easier to review than first adding all devices and then
adding a machine that uses all of them. And each device being added in
(1+n)/x can be tested (system not fully working of course). I.e., the
machine will grow in functionality patch by patch.
* Maybe you can order EHCI last due to the refactoring work involved?

To aid with the requested reordering and squashing of bug fixes into
patches, `git rebase -i` and `git checkout -p` may be of help to you.

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