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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3920 bytes --]

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 <revol@free.fr>
> > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > 
> > > > 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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]