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

[Antony Pavlov <antonynpavlov@gmail.com>]

On Mon, 6 Jul 2015 11:58:54 -0700
Serge Vakulenko <serge.vakulenko@gmail.com> wrote:

> On Mon, Jul 6, 2015 at 12:33 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > On Sun, 5 Jul 2015 21:18:11 -0700
> > Serge Vakulenko <serge.vakulenko@gmail.com> wrote:
> >
> >> On Wed, Jul 1, 2015 at 6:41 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> >> > On 2015-06-30 21:12, Serge Vakulenko wrote:
> >> >> Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com>
> >> >> ---
> >> >>  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.

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

-- 
Best regards,
  Antony Pavlov