From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43131) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCTY6-0002Mj-4J for qemu-devel@nongnu.org; Tue, 07 Jul 2015 10:08:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCTY4-0001Id-G8 for qemu-devel@nongnu.org; Tue, 07 Jul 2015 10:08:38 -0400 Received: from hall.aurel32.net ([2001:bc8:30d7:100::1]:54990) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCTY4-0001Ei-26 for qemu-devel@nongnu.org; Tue, 07 Jul 2015 10:08:36 -0400 Date: Tue, 7 Jul 2015 16:08:22 +0200 From: Aurelien Jarno Message-ID: <20150707140822.GC931@aurel32.net> References: <16270e45922ea6a8c8622e76f702c46ce7ce15ac.1435723168.git.serge.vakulenko@gmail.com> <20150701134103.GB8793@aurel32.net> <20150706103304.8eef043a9e27e0d7b514061c@gmail.com> <20150707103057.bba8fe37471c75ddc2712c0f@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150707103057.bba8fe37471c75ddc2712c0f@gmail.com> Subject: Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Antony Pavlov Cc: Leon Alrae , Peter Crosthwaite , Serge Vakulenko , qemu-devel@nongnu.org On 2015-07-07 10:30, Antony Pavlov wrote: > On Mon, 6 Jul 2015 11:58:54 -0700 > Serge Vakulenko wrote: > > > On Mon, Jul 6, 2015 at 12:33 AM, Antony Pavlov wrote: > > > On Sun, 5 Jul 2015 21:18:11 -0700 > > > Serge Vakulenko wrote: > > > > > >> On Wed, Jul 1, 2015 at 6:41 AM, Aurelien Jarno wrote: > > >> > On 2015-06-30 21:12, Serge Vakulenko wrote: > > >> >> Signed-off-by: Serge Vakulenko > > >> >> --- > > >> >> 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 aurelien@aurel32.net http://www.aurel32.net