Re: [Qemu-devel] [PATCH v1] arm: Adding new arm machine, Kinetis K64 MK64FN1M0

[Peter Maydell]

On 19 October 2017 at 13:50,  <address@hidden> wrote:
> From: Gabriel Augusto Costa <address@hidden>
>
> I add a new arm machine with some peripherals. The machine is mk64fn1m0, a
> cortex-m4 microcontroller from NXP Kinetis family. The machine can run a
> simple arm binary file using UART0 in polling mode.
> I prepared two patchs to include this machine:
> PATCH v1: Include the machine and peripherals devices;
> PATCH v2: Change the make file to compile this machine.

Thanks for these patches.

> Also, I made a folder tree to accomodate this machine more or less like 
> u-boot.
> In my opinion put all files in the same folder "/hw/arm" is not a good idea,
> or put all code in an unique file, because machines from the same family
> sharing the same peripherals.
> The folder tree struct is machine/family/peripheral, as an example:
> kinetis/k64/peripheral.
> So, in this way the code will be more maintainable.

There's an inevitable conflict between:
 * these peripherals are all for this board so they should go
   in a subdirectory for these boards
 * these peripherals are all of the same type so they should
   go in a subdirectory for devices of that type
Whichever way you slice things it will split things that in
some sense should go together.

However, QEMU has made a decision that we put devices of a
specific type in the same directory (roughly following what
the Linux kernel does). So can you please follow the structure
we have? Having a mix of the two schemes is much worse than
consistently following either.

> Signed-off-by: Gabriel Augusto Costa <address@hidden>
> ---
>  hw/arm/kinetis/k64/mk64fn1m0.c                    | 170 +++++++++
>  hw/arm/kinetis/k64/peripheral/flextimer.c         | 139 +++++++
>  hw/arm/kinetis/k64/peripheral/mcg.c               | 225 ++++++++++++
>  hw/arm/kinetis/k64/peripheral/pmux.c              | 423 
> ++++++++++++++++++++++
>  hw/arm/kinetis/k64/peripheral/sim.c               | 292 +++++++++++++++
>  hw/arm/kinetis/k64/peripheral/uart.c              | 369 +++++++++++++++++++
>  include/hw/arm/kinetis/k64/peripheral/flextimer.h |  62 ++++
>  include/hw/arm/kinetis/k64/peripheral/mcg.h       |  47 +++
>  include/hw/arm/kinetis/k64/peripheral/pmux.h      |  73 ++++
>  include/hw/arm/kinetis/k64/peripheral/sim.h       |  56 +++
>  include/hw/arm/kinetis/k64/peripheral/uart.h      |  85 +++++
>  11 files changed, 1941 insertions(+)

This patch is much too large to review. Please can you split it up
into a number of smaller logical patchsets? (Usually for a new
board and SoC one patch per new device is what makes most sense.
If you get much over 250 lines in one patch you should consider
whether there's a logical split of it possible.)

https://wiki.qemu.org/Contribute/SubmitAPatch has some other
helpful suggestions for patch submissions and is generally
worth reading I think.

If you could structure your next revision of this series to
include a cover letter email that would also be helpful;
some of our automatic patch handling tools can't deal with
multi-patch series that don't have the cover letter.

thanks
-- PMM