qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [RFC PATCH 12/12] ppc: Add aCube Sam460ex board


From: BALATON Zoltan
Subject: Re: [Qemu-ppc] [RFC PATCH 12/12] ppc: Add aCube Sam460ex board
Date: Wed, 23 Aug 2017 02:48:00 +0200 (CEST)
User-agent: Alpine 2.21 (BSF 202 2017-01-01)

On Wed, 23 Aug 2017, David Gibson wrote:
On Tue, Aug 22, 2017 at 01:18:02PM +0200, BALATON Zoltan wrote:
On Tue, 22 Aug 2017, David Gibson wrote:
On Fri, Aug 18, 2017 at 02:46:42PM +0200, BALATON Zoltan wrote:
On Fri, 18 Aug 2017, David Gibson wrote:
On Sun, Aug 13, 2017 at 07:04:38PM +0200, BALATON Zoltan wrote:
Add emulation of aCube Sam460ex board based on AMCC 460EX embedded SoC.
This is not a full implementation yet with a lot of components still
missing but enough to start a Linux kernel and the U-Boot firmware.

Signed-off-by: Fran├žois Revol <address@hidden>
Signed-off-by: BALATON Zoltan <address@hidden>

There are a *lot* of devices defined here.  Most of them look like
they belong to the SoC, not the board (since they use DCRs), so it
doesn't really make sense to define them in a board file.  It would
also make it easier to review if they were split up into separate
patches.

I thought it's simpler to review a series with 12 reasonably sized patches
than one with twice as many which only modify a few lines here and there
each. Also adding a lot of code scattered in hw directories is probably less
clear than having them all at one place. But of course each approach can be
reasoned. I thought this might have to be split up but I've left it one
place for now for first review to get some advice on what's preferred.

Well, it depends on the content of the patches.  If splitting it up
means a lot of looking between patches to make sense of what's going
on then that's certainly not good.  But if the small patches are
independent of each other and can be assessed on their own merits,
then that usually makes it easier.

In this case the various new 440 devices should be pretty much
independent of each other, so I think splitting is the better option.

Maybe I should put things that belong to the SoC in ppc440_uc.c (similar to
ppc405uc.c we already have) and move common devices used by both to
ppc4xx_devs.c (which already seems to serve that purpose). If more cleanup
is needed that could be done separately afterwards, I don't think it's a
good idea to mix in too much cleanup now to keep patches relatively simple.
(I already have some moving around included as clean up patches but I'd like
to focus on actual functions than clean up at this point).

Does putting these devices from board code to ppc440_uc.c sound
acceptable?

That'd be ok - though again, I'd prefer to see each device as a
separate patch.  It would probably be preferable to put each device in
a separate file as well though, unless they're _really_ tiny.  Nothing
inherently wrong with small .c files, if they're still more or less
self contained.

Since most of these are just stubs for now and not really full emulation of
the device I'd leave them in one file similar to ppc405 for now. They both
could be cleaned up later if it's found necessary or devices that are deemed
ready and big enough to be moved out could be split off (like I did with i2c
and pcix). If you insist, I can split it now but I think it's more difficult
to work with a lot of small files scattered in hw instead of everything
related to ppc440 in one place until there's still missing fucntionality. I
consider splitting it up cleanup which could be done later separately unless
it's a requirement to get this series in.

Ok, that's a reasonable argument.  Go ahead and put them in a
ppc440_uc.c file.

I already did in the non RFC submission of the series I've sent last Sunday (in [PATCH 14/15] ppc4xx: Add device models found in PPC440 core).
reply via email to

[Prev in Thread] Current Thread [Next in Thread]