Re: [Qemu-devel] [PATCH 01/12] e500: add board config options

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

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

On Wed, Nov 22, 2017 at 11:55:04AM -0600, Michael Davidsaver wrote:
> On 11/21/2017 09:28 PM, David Gibson wrote:
> > On Sun, Nov 19, 2017 at 09:24:09PM -0600, Michael Davidsaver wrote:
> >> allow board code to skip common NIC and guest image setup
> >> and configure decrementor frequency.
> >> Existing boards unchanged.
> >>
> >> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> > 
> > So, it's spelled "decrementer".
> > 
> > Other than that, the patch looks correct.  However having a big common
> > function for overall init with a pile of ad-hoc configuration
> > parameters is usually not a great way to go.  I think what we want
> > instead is to eliminate ppce500_init(), instead doing the setup logic
> > separately in each of the e500 machines.   The large common slabs of
> > code can be helpers in e500.c, but the overall logic - including most
> > of the things controlled by the current params - would be under the
> > individual machine's control.
> 
> I agree that ppce500_init() is unwieldy, and am willing to spend some
> time on cleanup.  I'm just not sure what to do.
> 
> I can see moving initialization of at least the serial, i2c, gpio, and
> possibly MPIC and PCI host bridge into the e500-ccsr class.
> 
> I'm not sure what to do with all the device tree construction code in
> e500.c, which has a bunch of hard coded register offsets.  A comment
> here summarizes the problem nicely.

So, those aren't bad ideas for cleanups, but I'm not going to insist
on that before merging this stuff.  What I'm after is something
simpler.  At the moment the structure looks something like this:

	machine_init() {
		common_init(big pile of parameters);
	}

	common_init() {
		chunk1;
		if (param1)
			chunk2;
		chunk3;
		if (param2)
			chunk4;
		...
	}

What I would prefer is:

	machine_init1() {
		chunk1();
		chunk2(param1)
		chunk3(param3);
	}

	machine_init2() {
		chunk1();
		chunk3(param3);
		chunk4(param2);
	}

> 
> > /* TODO: parameterize */
> > #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> > #define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
> 
> It seems desirable to avoid having these offsets appear in two different
> files, which could allow them to get out of sync.
> 
> I had the idea of using an Interface to split up device tree
> construction, and was encouraged to find PnvXScomInterfaceClass which
> seems be a way of combining device tree construction in a device class.
> Is this the way to go?

At some point, quite likely (I've actually had thoughts on making
devicetree construction more convenient qemu wide, but haven't had
time to do much about it).

-- 
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 --]