On 01/09/16 19:42, Will Deacon wrote: > On Tue, Aug 23, 2016 at 08:05:21PM +0100, Robin Murphy wrote: >> Making S2CRs first-class citizens within the driver with a high-level >> representation of their state offers a neat solution to a few problems: >> >> Firstly, the information about which context a device's stream IDs are >> associated with is already present by necessity in the S2CR. With that >> state easily accessible we can refer directly to it and obviate the need >> to track an IOMMU domain in each device's archdata (its earlier purpose >> of enforcing correct attachment of multi-device groups now being handled >> by the IOMMU core itself). >> >> Secondly, the core API now deprecates explicit domain detach and expects >> domain attach to move devices smoothly from one domain to another; for >> SMMUv2, this notion maps directly to simply rewriting the S2CRs assigned >> to the device. By giving the driver a suitable abstraction of those >> S2CRs to work with, we can massively reduce the overhead of the current >> heavy-handed "detach, free resources, reallocate resources, attach" >> approach. >> >> Thirdly, making the software state hardware-shaped and attached to the >> SMMU instance once again makes suspend/resume of this register group >> that much simpler to implement in future. >> >> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx> >> --- >> drivers/iommu/arm-smmu.c | 159 +++++++++++++++++++++++++++-------------------- >> 1 file changed, 93 insertions(+), 66 deletions(-) > > [...] > >> @@ -1145,9 +1198,16 @@ static void arm_smmu_master_free_smes(struct arm_smmu_device *smmu, >> static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, >> struct arm_smmu_master_cfg *cfg) >> { >> - int i, ret; >> + int i, ret = 0; >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> - void __iomem *gr0_base = ARM_SMMU_GR0(smmu); >> + struct arm_smmu_s2cr *s2cr = smmu->s2crs; >> + enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS; >> + u8 cbndx = smmu_domain->cfg.cbndx; >> + >> + if (cfg->smendx[0] < 0) > > Shouldn't that be INVALID_SMENDX? ...which is defined as -1, and thus less than zero. I have no objection, however, to changing this (and equivalent instances) to an explicit "if (foo == INVALID_SMENDX" if that's clearer, as I can't foresee any reasonable need for additional "invalid" values. Robin. > > Will > -- 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