[PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 11 June 2013 15:50:23 Jason Gunthorpe wrote:
> On Tue, Jun 11, 2013 at 05:26:47PM +0200, Arnd Bergmann wrote:
> 
> > That looks ok to me, yes. If you have just one device under some of the nodes
> > however, I think it's easier use an empty ranges property and do
> > 
> >  	devbus-bootcs {
> >  		ranges;
> >  		nor {
> >  			reg = <0x012f0000 0 0x1000000>;
> >  		};
> > 
> > The 'ranges' here is most useful if you have multiple devices
> > add different offsets. I would also predefine those ranges to
> > be as large as possible so you don't have to adapt them when
> > the child device grows beyond it.
> 
> It isn't super critical, but the ranges does keep the 0x012f0000 value
> in the SOC .dtsi and the board level doesn't have to be exposed to the
> value, it just uses 0 to setup the NOR.

Yes, good point.

> Also, it makes it much simpler for the mbus driver to detect which
> target id's are required so they can be allocated/setup - the rule
> would be every child stanza should have a ranges with the target(s) it
> needs.

Not sure if this is a good rule: As you said, it makes sense to
put the 'ranges' for the devbus-bootcs node into the per-soc .dtsi
file so the board file doesn't have to care. However the mbus code
needs both the 0x012f0000 and the start/length of the child. Mapping
the entire window would work in theory but is likely going to cause
a significant waste of physical address space, because the (per-soc)
ranges property has to be set up for the largest possible external
device connected to the bus, but the mbus window only needs to cover the
device that is actually connected.

> The full version probably looks like this:
> 
> mbus {
>     ranges ....
> 
>                         devbus-bootcs at 10400 {
>                                 compatible = "marvell,mvebu-devbus";
>                                 reg = <INTERNAL_REGS + 0x10400 0x8>;
>                                 ranges = <0 DEVBUS_BOOTCS 0x1000000>;
>                                 #address-cells = <1>;
>                                 #size-cells = <1>;
>                                 clocks = <&coreclk 0>;
> 
> 				// In board .dts:
>                                 nor at 0 {
>                                         compatible = "cfi-flash";
>                                         reg = <0 0x1000000>;
>                                         bank-width = <2>;
>                                 };
>                         };
> };
> 
> And notice here that we need to offset INTERNAL_REGS to make this
> work. To me, this is the primary reason why the 2nd cell of the mbus
> address format *must* be the address offset within the target, and not
> anything else.

Right.

> > > 2. There's no need to use an algorithm to 'find' a base address for decoding
> > >    windows (such as first-fit as Arnd suggested in another mail).
> > >    The base address is already there in the ranges property, so the mbus can
> > >    simply allocate the window on that base address.
> > > 
> > > So, am I right or completely lost?
> > 
> > Since you already need code to update the ranges property, I think
> > it's best to discard the ranges at boot time and create a new mapping
> > that just maps the devices you actually want to use. That would make
> > it much easier to cope with boot loaders that don't set up the
> > mappings in the same way that is listed in the device tree. Just
> > put the one entry for the internal-regs in the ranges, since that
> > is needed to map do the mbus setup.
> 
> So, I think we can/have agree that the ranges in the FDT should
> reflect the bootloader's settings, and if the ranges is missing an
> element it means the bootloader didn't set it up.
> 
> A compatible bootloader should dump the entire mbus register table
> into the ranges. The address encoding is such that we can represent
> every mbus window entry in ranges. If no bootloader support is present
> then the ranges value is forced into the mbus windows anyhow.
> 
> If there isn't a ranges entry, but there is a child that requires it
> (how do we figure this out?)

I think we have to walk the entire tree of devices underneath mbus,
at least any device node that has a 'reg' property, following children
of any device node with a 'ranges' property. We might need to
add a variant of of_get_address() that does a partial translation
up to a given node (the mbus) instead of all the way to the root.

> then dynamic assignment of that window
> makes sense to me. (however, this also looks a bit tricky, how do you
> avoid hitting the PCI-E window reservations, for instance?)

The PCI-E window would have 'ranges' but no 'regs', so we automatically
skip it.

> Unconditional dynamic assignment is a bit more tricky, for instance
> the boot rom has to be located at a specific address since it is used
> as the SMP secondary startup trampoline. (Ezequiel: IMHO, you should
> check for this and bail secondary startup if it is not setup)

I suppose we can identify the devices we don't want to reassign by
their compatible values.

> I'm not sure there is a good reason to reject the address map in the
> DT?

You mean keep all windows that are listed by the boot loader but
reassign the others? I guess it would be simpler but may result in
a less optimal address map.

	Arnd