Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
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 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
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. 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
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
On Mon, Jul 6, 2015 at 11:58 AM, 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. > There are better ways to split the patch other than straight per-file though. For example, header patches (e.g. for your structs) should go with their accompanying C code changes. git add -p is the interactive utility for selecting specific changes to be included in a commit. Regards, Peter >> -- >> Best regards, >> Antony Pavlov >
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
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. > -- > Best regards, > Antony Pavlov
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
On Sun, 5 Jul 2015 21:27:04 -0700 Serge Vakulenko wrote: > On Wed, Jul 1, 2015 at 10:56 PM, Antony Pavlov > wrote: > > On Tue, 30 Jun 2015 21:12:34 -0700 > > 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 > >> > >> diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs > >> index 9633f3a..dcbaec9 100644 > >> --- a/hw/mips/Makefile.objs > >> +++ b/hw/mips/Makefile.objs > >> @@ -3,3 +3,6 @@ obj-y += addr.o cputimer.o mips_int.o > >> obj-$(CONFIG_JAZZ) += mips_jazz.o > >> obj-$(CONFIG_FULONG) += mips_fulong2e.o > >> obj-y += gt64xxx_pci.o > >> +obj-y += mips_pic32mz.o mips_pic32mx7.o > >> +obj-y += pic32_load_hex.o pic32_sdcard.o pic32_spi.o pic32_uart.o > >> pic32_gpio.o > >> +obj-y += pic32_ethernet.o > > > > Can we move mips-unrelated stuff to the appropriate dirs? > > E.g. pic32_gpio.c can to go to hw/gpio. > > All these files are pic32-related. They depend on pic32_t data > structure declared in pic32_peripherals.h and register definitions in > pic32mx.h and pic32mz.h. I see no point to move them around. But why hw/gpio/pic32_gpio.c can't include pic32_peripherals.h? You can see that mainline qemu.git/hw/mips/ does not contain any header file, so just put your header files into the proper dir (I suppose qemu.git/include/hw/mips). > > Also please use separate patch for every peripheral controller (see > > Aurelien's comment). > > Agreed. > > >> diff --git a/hw/mips/mips_pic32mx7.c b/hw/mips/mips_pic32mx7.c > >> new file mode 100644 > >> index 000..1d8ffb5 > >> --- /dev/null > >> +++ b/hw/mips/mips_pic32mx7.c > >> @@ -0,0 +1,1652 @@ > >> +/* > >> + * QEMU support for Microchip PIC32MX7 microcontroller. > >> + * > >> + * Copyright (c) 2015 Serge Vakulenko > >> + * > >> + * Permission is hereby granted, free of charge, to any person obtaining > >> a copy > >> + * of this software and associated documentation files (the "Software"), > >> to deal > >> + * in the Software without restriction, including without limitation the > >> rights > >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or > >> sell > >> + * copies of the Software, and to permit persons to whom the Software is > >> + * furnished to do so, subject to the following conditions: > >> + * > >> + * The above copyright notice and this permission notice shall be > >> included in > >> + * all copies or substantial portions of the Software. > >> + * > >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > >> EXPRESS OR > >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > >> MERCHANTABILITY, > >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > >> OTHER > >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > >> ARISING FROM, > >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > >> IN > >> + * THE SOFTWARE. > >> + */ > >> + > >> +/* Only 32-bit little endian mode supported. */ > >> +#include "config.h" > >> +#if !defined TARGET_MIPS64 && !defined TARGET_WORDS_BIGENDIAN > > > > Please don't use C preprocessor directive for conditional compilation of > > the whole file. > > Use Makefile instead. See CONFIG_FULONG for example (fulong2e is > > mips64le-only). > > Makes sense. > > Thanks, > --Serge > > > -- > > Best regards, > > Antony Pavlov -- -- Best regards, Antony Pavlov
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
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. -- Best regards, Antony Pavlov
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
On Wed, Jul 1, 2015 at 10:56 PM, Antony Pavlov wrote: > On Tue, 30 Jun 2015 21:12:34 -0700 > 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 >> >> diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs >> index 9633f3a..dcbaec9 100644 >> --- a/hw/mips/Makefile.objs >> +++ b/hw/mips/Makefile.objs >> @@ -3,3 +3,6 @@ obj-y += addr.o cputimer.o mips_int.o >> obj-$(CONFIG_JAZZ) += mips_jazz.o >> obj-$(CONFIG_FULONG) += mips_fulong2e.o >> obj-y += gt64xxx_pci.o >> +obj-y += mips_pic32mz.o mips_pic32mx7.o >> +obj-y += pic32_load_hex.o pic32_sdcard.o pic32_spi.o pic32_uart.o >> pic32_gpio.o >> +obj-y += pic32_ethernet.o > > Can we move mips-unrelated stuff to the appropriate dirs? > E.g. pic32_gpio.c can to go to hw/gpio. All these files are pic32-related. They depend on pic32_t data structure declared in pic32_peripherals.h and register definitions in pic32mx.h and pic32mz.h. I see no point to move them around. > Also please use separate patch for every peripheral controller (see > Aurelien's comment). Agreed. >> diff --git a/hw/mips/mips_pic32mx7.c b/hw/mips/mips_pic32mx7.c >> new file mode 100644 >> index 000..1d8ffb5 >> --- /dev/null >> +++ b/hw/mips/mips_pic32mx7.c >> @@ -0,0 +1,1652 @@ >> +/* >> + * QEMU support for Microchip PIC32MX7 microcontroller. >> + * >> + * Copyright (c) 2015 Serge Vakulenko >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> copy >> + * of this software and associated documentation files (the "Software"), to >> deal >> + * in the Software without restriction, including without limitation the >> rights >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell >> + * copies of the Software, and to permit persons to whom the Software is >> + * furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be included >> in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >> OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >> OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >> FROM, >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN >> + * THE SOFTWARE. >> + */ >> + >> +/* Only 32-bit little endian mode supported. */ >> +#include "config.h" >> +#if !defined TARGET_MIPS64 && !defined TARGET_WORDS_BIGENDIAN > > Please don't use C preprocessor directive for conditional compilation of the > whole file. > Use Makefile instead. See CONFIG_FULONG for example (fulong2e is > mips64le-only). Makes sense. Thanks, --Serge > -- > Best regards, > Antony Pavlov
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
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. > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
On Tue, 30 Jun 2015 21:12:34 -0700 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 > > diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs > index 9633f3a..dcbaec9 100644 > --- a/hw/mips/Makefile.objs > +++ b/hw/mips/Makefile.objs > @@ -3,3 +3,6 @@ obj-y += addr.o cputimer.o mips_int.o > obj-$(CONFIG_JAZZ) += mips_jazz.o > obj-$(CONFIG_FULONG) += mips_fulong2e.o > obj-y += gt64xxx_pci.o > +obj-y += mips_pic32mz.o mips_pic32mx7.o > +obj-y += pic32_load_hex.o pic32_sdcard.o pic32_spi.o pic32_uart.o > pic32_gpio.o > +obj-y += pic32_ethernet.o Can we move mips-unrelated stuff to the appropriate dirs? E.g. pic32_gpio.c can to go to hw/gpio. Also please use separate patch for every peripheral controller (see Aurelien's comment). > diff --git a/hw/mips/mips_pic32mx7.c b/hw/mips/mips_pic32mx7.c > new file mode 100644 > index 000..1d8ffb5 > --- /dev/null > +++ b/hw/mips/mips_pic32mx7.c > @@ -0,0 +1,1652 @@ > +/* > + * QEMU support for Microchip PIC32MX7 microcontroller. > + * > + * Copyright (c) 2015 Serge Vakulenko > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > copy > + * of this software and associated documentation files (the "Software"), to > deal > + * in the Software without restriction, including without limitation the > rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > + > +/* Only 32-bit little endian mode supported. */ > +#include "config.h" > +#if !defined TARGET_MIPS64 && !defined TARGET_WORDS_BIGENDIAN Please don't use C preprocessor directive for conditional compilation of the whole file. Use Makefile instead. See CONFIG_FULONG for example (fulong2e is mips64le-only). -- Best regards, Antony Pavlov
Re: [Qemu-devel] [PATCH pic32 v2 5/5] Two new machine platforms: pic32mz7 and pic32mz.
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. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net
