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@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 _______________________________________________ Devel mailing list [email protected] http://lists.laptop.org/listinfo/devel
