MMP interrupt controller DT layout

[Mitch Bradley]

On 6/11/2013 4:21 AM, Daniel Drake wrote:
> Hi,
> 
> I am looking at upstreaming these patches:
> 
> http://dev.laptop.org/~dsd/20130611/0003-ARM-mmp-Fix-MMP2-interrupt-controller-DT-nodes.patch
> 
> http://dev.laptop.org/~dsd/20130611/0004-ARM-mmp-irq-Use-of_get_address-instead-of-of_address.patch
> 
> Could one of you summarise the background here, and confirm my
> understanding below?
> 
> Here is the upstream mmp2 DT which the first patch applies against:
> 
> http://lxr.linux.no/linux+v3.9.5/arch/arm/boot/dts/mmp2.dtsi#L34
> 
> I can see why this is a strange design. The intc-mux registers clearly
> sit inside the intc range, but is this actually forbidden by DT?

Depends on what you mean by "forbidden".  There is an implicit rule that
the DT should describe the hardware accurately and "make sense".
Violating that rule won't necessarily cause things to fall down in a
heap immediately, but over time, lies accumulate and cause conflicts and
other problems.

> 
> The reg values seem to refer to memory address 0x150 and so on (in the
> traditional DT sense), which is obviously odd.

Indeed, it is a lie.  There are no registers at that address, and there
is main memory there.  While one could get away with the lie in this
case, mistakes like this have an evil way of propagating as people use
drivers as templates for future drivers and cite historical precedence
for doing the wrong things later.

> 
> As strange as it may be, all of this is documented reasonably well at
> http://lxr.linux.no/linux+v3.9.5/Documentation/devicetree/bindings/arm/mrvl/intc.txt

Documented nonsense is nonsense reiterated.  Saying it twice does not
toggle the "non" :-)

> 
> Given the oddness here, I can understand OLPC's desires to "go against
> spec" and fix this up in OLPCs DT. But the end result is not entirely
> clear to me. The OLPC DT can be viewed here:
> http://dev.laptop.org/~dsd/20130611/0002-ARM-dts-Add-xo-1-75-mmp2-and-xo-4-mmp3-dts.patch
> 
> To me, the logical design here would be to have intc as a parent
> device and treat it almost like a bus, with:
> 
>   reg = <0xd4282000 0x1000>;
>   #address-cells = <0x1>;
>   #size-cells = <0x1>;
>   ranges = <0x0 0xd4282000 0x1000>;
> 
> Then that would have child devices like:
> 
>   interrupt-controller at 1d0 {
>     compatible = "mrvl,mmp2-mux-intc";
>     reg = <0x1d0 0x4 0x1b8 0x4>;
>   };
> 
> And the 0x1d0 would be translated to real memory address 0xd42821d0,
> thanks to the ranges property.

The problem with this interpretation is that the parent "bus" node not
only propagates some addresses to its mux children, but also claims some
(in fact most) addresses for its own use - i.e. the non-muxed registers.
 The bus model does not include such "hybrid buses"; a memory-mapped bus
controller is assumed to be essentially a transparent pass-through,
perhaps with some fiddling of high-order address bits.

While it would be possible to use the ranges mechanism to describe an
elaborate bus that passes through selected disjoint address ranges,
while letting the bus controller directly handle others, that seemed
like a misuse of the mechanism to me, likely to cause confusion and
unforseen problems.

The model that I instead adopted was to treat the parent node as a
device that, while it has children like a "bus", does not pretend to be
a memory-mapped bus with all of the assumptions that implies (those
assumptions include the ability to use the generic framework to resolve
subordinate addresses and to auto-bind subordinate drivers.)  Instead,
this parent node is responsible for managing its child address space
directly, thus dealing with its idiosyncrasies and hiding them from
generic code that could be confused by the fact that this child address
space does not fit the generic model.

This interpretation is completely "kosher".  The generic framework is
not intended to solve all problems, but rather to make it easier to deal
with devices that do happen to fit the common model.  Devices that are
sufficiently "wrinkly" require some custom code.

> 
> What we have in the OLPC DT is almost this, but it is missing the
> ranges prop. According to booting-without-of.txt this suggests that
> the registers of the child device are not accessible to the parent. As
> the address cannot be translated, of_address_to_resource() cannot be
> used, which leads to the 0004 patch above.

Yeah.  By making the intc node directly responsible for its children,
and taking those children out of the generic domain (where they didn't
properly fit, but rather were shoehorned in), we lost the ability to
apply the resource framework.

I argue that this is a good thing, because in the original formulation,
there was an undetected resource conflict.  The parent intc node needed
to grab the whole range of addresses as a resource, then the mux nodes
also grabbed subsets of that range.

In my formulation, the parent is responsible for managing the
whole-range resource, and then handing off responsibility for sub-ranges
in the privacy of its own driver cluster.  The common Linux resource
management code sees a clean resource ownership structure.

> 
> I am really just interested in clarifying my understand of DT address
> mapping and the background here. For compatibility reasons, I will try
> to go upstream with what we ship in the DT at the moment. Which leads
> to one final question...
> 
> The 0004 patch says: This patch depends on "ARM: mmp: Fix MMP2
> interrupt controller DT nodes".
> 
> I can't see why this is true. As far as I can see, of_get_address()
> will work OK for both the current upstream mmp2.dtsi, and for the OLPC
> intc DT layout which is missing the ranges property. Any hints?

My best guess is that the correct statement is 'This patch was rendered
necessary by "ARM: Fix MMP2 interrupt controller DT nodes" and, while it
might be safe in the absence of that patch, has not been so tested."

> 
> Thanks
> Daniel