On 16/12/16 15:56, Stuart Yoder wrote: >>>>> The existing iommu-map binding did not account for the situation where >>>>> #iommu-cells == 2, as permitted in the ARM SMMU binding. The 2nd cell >>>>> of the IOMMU specifier being the SMR mask. The existing binding defines >>>>> the mapping as: >>>>> Any RID r in the interval [rid-base, rid-base + length) is associated with >>>>> the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base). >>>>> >>>>> ...and that does not work if iommu-base is 2 cells, the second being the >>>>> SMR mask. >>>>> >>>>> While this can be worked around by always having length=1, it seems we >>>>> can get this cleaned up by updating the binding definition for iommu-map. >>>> >>>> To reiterate, I'm very much not keen on the pci-iommu binding having >>>> knowledge of the decomposition of iommu-specifiers from other bindings. >>> >>> With the current definition of iommu-map we already have baked in an >>> assumption that an iommu-specifier is a value that can be incremented >>> by 1 to get to the next sequential specifier. So the binding already >>> has "knowledge" that applies in most, but not all cases. >>> >>> The generic iommu binding also defines a case where #iommu-cells=4 >>> for some IOMMUs. >>> >>> Since the ARM SMMU is a special case, perhaps the intepretation >>> of an iommu-specifier in the context of iommu-map should be moved >>> into the SMMU binding. >>> >>>> As mentioned previously, there's an intended interpretation [1] that we >>>> need to fix up the pci-iommu binding with. With that, I don't think it's >>>> even necessary to bodge iommu-cells = <1> AFAICT. >>> >>> You had said in the previous thread: >>> >>> > I had intended that the offset was added to the final cell of the >>> > iommu-specifier (i.e. that the iommu-specifier was treated as a large >>> > number). >>> >>> > You can handle this case by adding additional entries in the map table, >>> > with a single entry length. >>> >>> I understand that, and it works, but I don't see why the definition has >>> to be that the offset is added to the "final cell". >> >> Because the cells of a specifier form a single opaque big-endian value. >> Were DT little-endian it would be the "first cell". To be pedantic, both >> of those descriptions are technically wrong because they fail to account >> for overflow and carry up into the next cell (in whichever direction). > > If it is really opaque how can we reliably add 1 to it? The pci-iommu binding isn't adding 1 to an opaque value. It is *generating* an IOMMU specifier of the form of a single numeric value, as defined by some linear translation of a PCI RID relative to a numeric base value appropriate for the IOMMU topology. It is explicit therein that a single numeric value must be the appropriate interpretation of that specifier. That happens to be true of a 1-cell arm-smmu specifier, therefore iommu-map can be used with arm-smmu with #iommu-cells=1. It is also true of a 2-cell some-other-iommu specifier, therefore iommu-map can be used with some-other-iommu with #iommu-cells=2 (if we fix the fact that the current implementation fails to consider more than one cell). It is not, however, true of a 2-cell arm-smmu specifier, therefore iommu-map cannot be used with arm-smmu with #iommu-cells=2, although the fact that of_pci_iommu_configure() unconditionally generates a 1-cell specifier at the moment does happen to sidestep that. The point of the 2-cell arm-smmu specifier is really to handle the devices (which exist) with dozens or hundreds of stream IDs, which are *only* usable via SMR masking, and particularly with a hand-crafted mask that is able to assume the non-existence of overlapping IDs (that aspect being actually quite significant for optimal allocation - one of the reasons my automatic-mask-generation prototype is now gathering dust). The MMU-500 TBU use-case is really an entirely different kettle of fish, hence the RFC I posted earlier. Robin. >>> Why can't it be >>> an iommu specific definition that makes sense for a given IOMMU? >> >> Because the implementation would then become a nightmarish rabbit-warren >> of special-cases, largely defeating the point of a *generic* binding. At >> the very least it'd have to chase every phandle and determine its >> compatible just to work out what to do with any given specifier - merely >> thinking about the complexity of the error handling for the number of >> additional failure modes that introduces is enough to put me off. > > In order to decode an iommu-map at all we have to chase every phandle, no? > Isn't that how we know how many cells an iommu-map entry has? > > I have not thought through the implementation, but my thought was that > the code could use a callback to handle the IOMMU-specific case. If > callback==NULL then the default case is what we have today. An IOMMU like > the SMMU can implement a simple callback that can return the mapping. > > Not sure how this is that much harder than any other map, such as interrupt-map. > > Stuart > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
