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

[Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

Hi Jason, Arnd:

Thanks for your reviews!

I agree with most of your suggestions so far. However, I'd like to discuss
one point before we move forward with the other (imo, less importants)
issues. See below.

On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote:
[...]
> 
> 
> Is the ranges right though? I was expecting this:
> 
>  ranges = <0  0x012f0000 0  0x8000000>
> 
> The 2nd address cell in the 2dword space should almost always be 0.
> 
> The 2nd address cell should be interprited as the offset within the
> target's window, not as some kind of physical base address.
> 
> >>+ (Note that the windowid cell is encoding the target ID = 0x01 and attribute
> >>+ ID = 0x2f, and the selected base address for the window is 0xe8000000).
> 
> ... The proper place to indicate the base address for the window is in
> the mbus ranges:
> 
> mbus {
>    ranges = <0x012f0000 0  0xe8000000  0x8000000>
>    devbus-bootcs {
>        ranges = <0  0x012f0000 0  0x8000000>
>    }
> }
> 
> We shouldn't mangle the DT format just to make it convenient for
> humans to write - if this is a major problem then I'd try to use the
> preprocessor first.. There are several reasonable solutions down that
> path, IMHO.
> 

Right. I think we have two options here for laying the DT ranges.

1) This is the proposal implied in the patchset I sent:

mbus {
	ranges = < we only put the internal-reg translation here>
	devbus-bootcs {
		ranges = <0 {target_id/attribute} {window_physical_base} {size}>
	}
}

Of course the above DT will be actually incomplete, for it'll lack a proper ranges
entry to translate the devbus-bootcs address. So we chosed to do it dynamically
in the mbus driver (see patch 05/14), and add the missing entry.

The information of the physical window base address is in this case in
each child (devbus-bootcs, bootrom, and so on). The MBus driver walks
each of its first-level children and allocates the window based on the
address declared in the ranges property of each child, as shown above.

This is done mostly to avoid having that in the mbus node, and the nightmare
to maintain it produces. See below.

2) This is what Jason is proposing in his mail:

mbus {
	ranges = <{target_id/attribute} 0 {window_physical_base} {size}>
	devbus-bootcs {
		ranges = <0 {target_id/attribute} 0 {size}>
	}
}

Of course this looks much cleaner, but it forces a lot of duplication
in the DT files. Now, if you see some of the recent patches we've been
sending, I think this duplication is very error-prone, and it'll be a
nightmare to maintain. Let me propose an example to show this
duplication:

Let's suppose we have a board "A" with its armada-A.dts,
and a common one armada.dtsi.

The common dtsi file would have this ranges property:

/* armada.dtsi */
mbus {
	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
		         bootrom_id 0       bootrom_base       bootrom_size >
}

The A board has a NOR connected to some devbus, so we need to add it
to the ranges, but also need to duplicate the ones in the common dtsi:

/* armada-A.dts */
mbus {
	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
		         bootrom_id 0       bootrom_base       bootrom_size
		         devbus0_id 0       devbus0_base       devbus0_size >
}

Now, if we add something at the common level, and extend the ranges
property in the common armada.dtsi, we also have to go through *each* of
the per-board dts files (for *each* board) adding that entry, because
entries *need* to be duplicated. Otherwise you're effectively
"shadowing" the entries.

It is precisely for this reason that I've decided to adopt option #1
instead! Now, I'm not saying I like that option particularly.
In fact it has a couple issues as well:

  1. The DT is *incomplete* and needs to be completed by the MBus
     driver which, IMHO, sucks.

  2. Changing the DT dynamically in the kernel, means that new
     properties are allocated to replace old ones, but the old ones
     are *never* released. So if for any reason we do this often,
     we're effectively "leaking" memory.

So, I'm not saying option #1 is nice, but rather that it seems it'll
be less trouble to maintain.

What do you think?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss