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

[KONRAD Frederic]

On 10/20/2017 04:01 PM, Gabriel Costa wrote:
> Thanks for your fast response and tips. This is my first time sending
> patchs!
>
> Yes, I will follow the QEmu struct. I saw what other developers done
> (stm32f2xx) and I put the files in following folders:
> hw/arm/mk64fn1m0.c                 This file define the SoC
> hw/char/kinetis_k64_uart.c         UART peripheral
> hw/misc/kinetis_k64_flextimer.c  Timers peripherals

I'd say hw/timer/kinetis_k64_flextimer.c here.

Fred

> hw/misc/kinetis_k64_mcg.c        Clock Manager
> hw/misc/kinetis_k64_pmux.c      MUX selector
> hw/misc/kinetis_k64_sim.c         More Clock selection
> that is ok?
>
> I will solve the code style issues and prepare a new patch series.
> Also, I received an error about a build fail (among others, of course), I
> believe that this happened because the first patch was not applied.
>
> Regards to all,
>
> Gabriel Costa
>
> On Thu, Oct 19, 2017 at 1:02 PM, Peter Maydell <address@hidden>
> wrote:
>
> > 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