Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.

[Aurelien Jarno]

On 2015-07-07 10:30, Antony Pavlov wrote:
> On Mon, 6 Jul 2015 11:58:54 -0700
> Serge Vakulenko <address@hidden> wrote:
> 
> > On Mon, Jul 6, 2015 at 12:33 AM, Antony Pavlov <address@hidden> wrote:
> > > On Sun, 5 Jul 2015 21:18:11 -0700
> > > Serge Vakulenko <address@hidden> wrote:
> > >
> > >> On Wed, Jul 1, 2015 at 6:41 AM, Aurelien Jarno <address@hidden> wrote:
> > >> > On 2015-06-30 21:12, Serge Vakulenko wrote:
> > >> >> Signed-off-by: Serge Vakulenko <address@hidden>
> > >> >> ---
> > >> >>  hw/mips/Makefile.objs       |    3 +
> > >> >>  hw/mips/mips_pic32mx7.c     | 1652 +++++++++++++++++++++++++
> > >> >>  hw/mips/mips_pic32mz.c      | 2840 
> > >> >> +++++++++++++++++++++++++++++++++++++++++++
> > >> >>  hw/mips/pic32_ethernet.c    |  557 +++++++++
> > >> >>  hw/mips/pic32_gpio.c        |   39 +
> > >> >>  hw/mips/pic32_load_hex.c    |  238 ++++
> > >> >>  hw/mips/pic32_peripherals.h |  210 ++++
> > >> >>  hw/mips/pic32_sdcard.c      |  428 +++++++
> > >> >>  hw/mips/pic32_spi.c         |  121 ++
> > >> >>  hw/mips/pic32_uart.c        |  228 ++++
> > >> >>  hw/mips/pic32mx.h           | 1290 ++++++++++++++++++++
> > >> >>  hw/mips/pic32mz.h           | 2093 +++++++++++++++++++++++++++++++
> > >> >>  12 files changed, 9699 insertions(+)
> > >> >>  create mode 100644 hw/mips/mips_pic32mx7.c
> > >> >>  create mode 100644 hw/mips/mips_pic32mz.c
> > >> >>  create mode 100644 hw/mips/pic32_ethernet.c
> > >> >>  create mode 100644 hw/mips/pic32_gpio.c
> > >> >>  create mode 100644 hw/mips/pic32_load_hex.c
> > >> >>  create mode 100644 hw/mips/pic32_peripherals.h
> > >> >>  create mode 100644 hw/mips/pic32_sdcard.c
> > >> >>  create mode 100644 hw/mips/pic32_spi.c
> > >> >>  create mode 100644 hw/mips/pic32_uart.c
> > >> >>  create mode 100644 hw/mips/pic32mx.h
> > >> >>  create mode 100644 hw/mips/pic32mz.h
> > >> >
> > >> > This patch is huge, and needs to be splitted to ease the review.
> > >>
> > >> I'll prepare a new patch set, with every new file put into a separate
> > >> message. Other issues fixed as well.
> > >
> > > Putting every new file into a separate message is a nonsense.
> > > Please separate __logical changes__ into a single patch.
> > 
> > Aurelien Jarno asked to split this patch to ease the review.
> 
> IMHO he meant something very different.

I confirm I wanted basically a changeset progressively adding devices,
like one patch per devices.

> Please reread the qemu submitting patch manual carefully
> (see http://wiki.qemu.org/Contribute/SubmitAPatch).
> 
> Here is a quote:
> 
>   Split up longer patches into a patch series of logical code changes.
>   Each change should compile and execute successfully. For instance,
>   don't add a file to the makefile in patch one and then add the file itself
>   in patch two. (This rule is here so that people can later use tools
>   like git bisect without hitting points in the commit history
>   where QEMU doesn't work for reasons unrelated to the bug they're chasing.)
> 
> Also please reread this Peter's comment very very carefully:
>   
>    http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg01430.html
> 
> Peter asks you to rework your device support code: every device should be 
> self-contained.
> E.g. for UART support code this means that:
> 
>    0. Object model is used. Your UART code implements operation of one UART 
> instance.
>       private structure is used for storing UART instance's current state.
>       The SoC code (or even board code) creates as many UART instances as it 
> needs.
> 
>       Also please see this Aurilien's comment: 
> http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg01242.html
> 
>    1. UART C code go to qemu.git/hw/char/;
> 
>    2. externally visible UART stuff (header file) go to 
> qemu.git/include/hw/char/;
> 
>       Pay attention that there is no need to put all UART related macro into 
> header file.
>       If nobody outside your UART C code use these macros then you can keep 
> their definition in the C code.
> 
>    3. UART C code compilation has to be enabled only for mips-softmmu target.
>       So make your UART C code compilation dependendent on a Makefile option,
>       enable this option only in qemu.git/default-configs/mips-softmmu.mak.
> 
>    4. UART support have to be added in a separate patch. So this patch have 
> to contain changes in these files:
> 
>         default-configs/mips-softmmu.mak
>         hw/char/Makefile.objs
>         hw/char/pic32_uart.c
>         include/hw/char/pic32_uart.h
>       
>       This UART support patch has to be submitted __before__ a patch with 
> SoC/board code that use UART.
> 
> 
> As Peter suggests please use 'Netduino 2 Machine Model' patchseries as a 
> model,
>   see http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg03398.html

Thanks for the detailed guidelines, it's quite useful.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net