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

[Serge Vakulenko]

On Wed, Jul 1, 2015 at 10:56 PM, Antony Pavlov <address@hidden> wrote:
> On Tue, 30 Jun 2015 21:12:34 -0700
> Serge Vakulenko <address@hidden> 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
>>
>> 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 0000000..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