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

[Sebastian Hesselbarth]

On 06/11/2013 11:50 PM, 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.

Jason, Ezequiel, Arnd,

sorry to kick into this thread that late but Ezequiel made me think of
address windows when asking on IRC. From what I can see from your and
Arnd's proposal the only real difference is that having it Arnd's way
allows you to have two or more windows mapped to e.g. devbus with
totally independent address ranges - while yours requires one large
window for all child devices?

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

Given the limited number of windows, I personally prefer your proposal
above - but nevertheless I can also think of separating target id from
ranges completely. (I know this discussion has been here for a while,
and I apologize if below was already NACKed before)

Although, not thought to the bare end this could also work and maybe
reflects the actual hardware more accuate:

mbus {
	compatible = "marvell,kirkwood-mbus";
	ranges; /* 1:1 mapping */

	internal-regs {
		ranges = <0 0xf1000000 0x800000>;
		/* target-id target-attr size */
		marvell,mbus-target = <0xff 0x12 0x800000>;

		nand {
			compatible = "marvell,orion-nand";
			reg = <0x10400 0x80>;
			marvell,mbus-target = <0x01 0x2f 0x100000>;

			/* in board dts */
			spansion-flash {
				...
			};
		};
	};

	sram {
		marvell,mbus-target = <...>;
	};
};

>>> 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?) 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?)
>
> 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)

When removing size from marvell,mbus-target above, mbus driver could
also probe for required max size from the reg properties of the child
nodes. Arnd already said that mbus isn't "simple-bus" anymore, why
can't it just walk through the nodes and collect required windows?

Sebastian