On Thu, Jan 05, 2017 at 02:00:05PM +0000, Will Deacon wrote: > On Thu, Jan 05, 2017 at 12:08:57PM +0000, Mark Rutland wrote: > > On Thu, Jan 05, 2017 at 11:55:29AM +0000, Will Deacon wrote: > > > On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote: > > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > > index ef465b0..5f405a6 100644 > > > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt > > > > @@ -68,6 +68,9 @@ conditions. > > > > aliases of secure registers have to be used during > > > > SMMU configuration. > > > > > > > > +- arm,smmu-enable-stall : Enable stall mode to stall memory transactions > > > > + and resume after fault is handled > > > > The wording here seems to describe a policy rather than a property. > > > > Can you elaborate on when/why this is required/preferred/valid? > > It's not a policy, it's a hardware capability. There are some non-probeable > reasons why stalling mode is unsafe or unusable: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/474530.html Ok. My point was that the wording above is an imperative -- it tells the kernel to enable stall mode, not if/why it is safe to do so (i.e. it is a policy, not a property). It sounds like that's just a wording issue. Something like "arm,stalling-is-usable" (along with a descrition of when that can/should be in the DT) would be vastly better. > Some of these are specific to the SMMU implementation (e.g. whether or not > the SS bit can remain set without reasserting the IRQ) and some are specific > to the integration (e.g. whether or not stalling an endpoint can deadlock > the SoC). The key point is that, without support from both the > implementation and the integration, stalls are unusable. Ok. > > > > static irqreturn_t arm_smmu_global_fault(int irq, void *dev) > > > > @@ -824,6 +852,8 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, > > > > > > > > /* SCTLR */ > > > > reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M; > > > > + if (smmu->options & ARM_SMMU_OPT_ENABLE_STALL) > > > > + reg |= SCTLR_CFCFG; > > > > > > I wonder if this should also be predicated on the compatible string, so > > > that the "arm,smmu-enable-stall" property is ignored (with a warning) if > > > the compatible string isn't specific enough to identify an implementation > > > with the required SS behaviour? On the other hand, it feels pretty > > > redundant and a single "stalling works" property is all we need. > > > > Can you elaborate on what "stalling works" entails? Is that just the SS > > bit behaviour? are there integration or endpoint-specific things that we > > need to care about? > > See above. The "stalling works" property (arm,smmu-enable-stall) would > indicate that both the implementation *and* the integration are such > that stalling is usable for demand paging. I suspect there are endpoints > that can't deal with stalls (e.g. they might timeout and signal a RAS > event), but in that case their respective device drivers should ensure > that any DMA buffers are pinned and/or register a fault handler to > request termination of the faulting transaction. Ok. It would be good to elaborate on what "stalling is useable" means in the property description. i.e. what specificallty the implementation and integration need to ensure. Thanks, Mark. -- 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
