[PATCH 04/10] bus: introduce an Marvell EBU MBus driver

[thomas.petazzoni@free-electrons.com (Thomas Petazzoni)]

Dear Arnd Bergmann,

On Thu, 7 Mar 2013 03:37:43 +0000, Arnd Bergmann wrote:
> On Wednesday 06 March 2013, Jason Gunthorpe wrote:
> > On Wed, Mar 06, 2013 at 08:27:10PM +0100, Thomas Petazzoni wrote:
> 
> > Part of the point of all this DT stuff is to give HW designers some
> > reasonable flexability in their HW design without requiring code
> > changes to Linux to boot it. So if Marvell makes a new Armada varient
> > that re-uses all the same pre existing IP, just with different mbus
> > targets, they should be able to make Linux run on it simply by writing
> > a SOC specific firmware and DT.
> 
> Agreed. Further, the device tree should accurately describe the mapping
> of addresses, and we have a lot of infrastructure to do exactly that.

Why should the Device Tree describe something that isn't a description
of the hardware? The mapping of addresses used for address decoding
windows is inherently dynamic.

Do I have to, again, repeat the PCIe discussion? We *cannot* describe
the PCIe address decoding windows in the Device Tree. It is NOT
possible.

We have too many PCIe interfaces (up to 10), too few windows (only 20),
and not enough physical address space, to cover, statically, all
possible combinations of PCIe devices.

In addition, the PCIe bus is very nice because it is dynamic: you
discover devices, how much memory and I/O space they require, and you
set up the windows accordingly.

Please always think about this PCIe use case when proposing any sort of
solution about address decoding windows. Any solution that doesn't take
into account the dynamic nature of PCIe windows is simply useless.

> 
> > > Also, I don't believe that windows should be described in the Device
> > > Tree. My long term plan is rather to make the allocation of the base
> > > address of each window entirely dynamic, because there is absolutely no
> > > reason for those addresses to be fixed in stone in the Device Tree.
> > 
> > How will that work? Device tree necessarily must have CPU addresses
> > for the things it describes. Today we have the problem that those
> > addresses must match the values hard coded into Linux, ie DT is
> > describing what Linux expects, not what the HW provides..
> 
> Exactly.

No, the Device Tree does not need to have CPU addresses.

The hardware description of a NOR is :

 * It is connected to chip-select X
 * Its size is 16 MB
 * Its model/description/characteristics are <foo>

So the NOR would be:

	reg = <0 0x1000000>;
	cs = <3>;

We encode this information in the Device Tree, because they describe
the hardware. Then, in the driver, we do:

	struct resource nor_resource;

	/* Get the size of the NOR from DT */
	of_address_to_resource(np, 0, &nor_resource);

	/* Find an available physical range to map the NOR */
	allocate_resource(&iomem_res, &nor_resource,
			  resource_size(&nor_resource),
			  0, 0, 0, NULL, NULL);

	/* Create the address decoding window */
	mvebu_mbus_add_window(nor, nor_resource.start,
			      resource_size(&nor_resource));

And that's it. The physical address at which the NOR is mapped is *NOT*
a description of the hardware.

We are not hardcoding virtual addresses in the DT, right?

So for the same reason, we should not hardcode the address decoding
windows and their base address into the DT.

Please explain why you think hardcoding in the Device Tree things that
is not a hardware description is a good idea.

> > This idea that Linux should setup windows on its own has moved away
> > from what OF was intended for, I'm not sure there is an overarching
> > plan on how to handle those cases - but hardcoding addresses in Linux
> > and then requiring the DT to match them exactly certainly seems wrong.
> > 
> > As for dynamic allocation, they way to do it via DT is through the
> > ranges property, similar to how I noted. The DT can be modified
> > in-memory, so a dyanmic mbus driver would ignore the CPU target
> > address in the ranges, select its own CPU address and then update the
> > in-memory DT with the corrected value. Everything else would then work
> > properly.
> 
> Yes. We can even use the index of the "ranges" property to refer directly
> to the number of the window, so ranges replaces the kernel internal
> struct mvebu_mbus_mapping.

Doesn't work with PCIe windows that have to be dynamically created.

It really strikes me that in the name of the Device Tree, we would have
to hardcode in stone things that are inherently dynamic.

> Getting the binding right is certainly a priority here, but I also think
> that we don't need to rush the code to use that binding (although having
> the code work would make me more comfortable thinking that the binding
> actually works).

The current binding is just a compatible string, not more. So it can
easily be extended in the future to have more properties below it
giving more configuration details. Of course, I continue to
fundamentally disagree with the proposal you and Jason are making, but
if you want to implement something that does that, it will always be
possible to extend the DT binding in the future.

Also remember that the mvebu-mbus driver has to be compatible with
non-DT platforms. We haven't migrated yet all the SoC families to the
DT, and it will take a bit more time to convert all of them to the
Device Tree.

Would it be possible to agree that this driver is a first step that
consolidates the existing address decoding code, which doesn't prevent
further improvements, and get the driver code merged in this current
state (of course, after fixing all the bugs, issues, that will be
discovered during review and testing) ?

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com