On 02/10/2018 08:54 PM, David Gibson wrote: > On Mon, Feb 05, 2018 at 09:24:34AM -0600, Andrew F. Davis wrote: >> On 02/01/2018 10:35 PM, Rob Herring wrote: >>> On Thu, Feb 1, 2018 at 5:04 PM, Andrew F. Davis <afd@xxxxxx> wrote: >>>> From: Benjamin Fair <b-fair@xxxxxx> >>>> >>>> This patch extends the capability of libfdt to parse the contents of device >>>> trees in a similar manner to fdt_address_cells and fdt_size_cells. >>>> >>>> It adds a helper function which reads the address and size of a device from >>>> the reg property and performs basic sanity checks. >>>> >>>> It does not perform translation to a physical address using the ranges >>>> properties of parents, but this enhancement may be added as a separate >>>> function in the future. >>> >>> I'm concerned about merging this without translation support. First, >>> I'd question its usefulness without it. Second, it may encourage >>> people to write their DTs without any ranges just so this will work. >> >> It can be useful for nodes where ranges is not appropriate for whatever >> reason, or more likely as a base for other helpers that do perform >> translation (hence the "simple" in this ones name), it will need to >> still be extended for many use-cases. > > Yeah, I have very mixed feelings about this. If I'd just seen the > patches without the threads associated with them, I'd have been happy > enough. Although it certainly doesn't cover anything, there are > definitely cases where an untranslated retrieval is useful. Not least > as something for a translating helper to use underneath, but also > because this can be useful for buses which have no direct mapping into > MMIO space (/cpus, i2c amongst others). > > However a number of the comments on versions of the patch - and indeed > the function types on this revision - seem to specifically imply this > is intended for the case where an untranslated fetch is somewhere > between fragile and dead wrong. Like Rob, I'm concerned that will > encourage poor ways of doing things. > > Perhaps a version of this which will error if the parent node has any > 'ranges' property (including empty)? That restricts it to the case of > buses that are not mapped into their parent address space. > So what would you like to have us do here? We could just error on ranges but this will reduce this helpers usefulness as we want to use this for regs mapped in a parent address space. Currently the only case where this helper would be wrong is if the node reg is part of the parents address space but not identity mapped into it, in which case to have this helper handle this "correctly" it would have to walk all the way back up to the top address space, doing all the mapping translations along the way. Then after all that you get an address you don't really know what to do with unless you manually also walk back up the hierarchy to see what it is you are getting. That would not make sense and is not what this helper is for, it just reads the reg property, that is all it does and all it claims to do. Users of this helper are free to use it build more "complete" solutions, or check for ranges and error out, but I see no one size fits all helper showing up anytime soon. I'll post a v7 with the couple fixes suggested on v6. Andrew >>> I tried to upstream the kernel and u-boot implementations to libfdt in >>> April 2014 (I couldn't find an archive). We've got to get it >>> relicensed and then David didn't like the implementation. He said he >>> was about to post his version, but then it derailed into PCI handling. > > Ugh.. sorry. Clearly I completely lost that train of thought. > >>> There's another implementation in the kernel powerpc boot code that >>> may be easier to re-license. >>> >> >> My worry with adding translation is this kind of feature creep. PCI and >> other odd cases will block this effort with little gain for the users >> where this simple helper is sufficient. >> >>> Rob >>> > -- To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
