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.mur...@arm.com>
>> ---
>> 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
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
