Re: [PATCH 2/5] iommu/arm-smmu: Sort out coherency

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

Hi Will,

On 28/07/15 10:43, Will Deacon wrote:
> Hi Robin,
>
> Cheers for doing this. Just a few comments (mainly in relation to being
> consistent with SMMUv3).

Thanks!

> On Mon, Jul 27, 2015 at 07:18:09PM +0100, Robin Murphy wrote:
>> Currently we detect the whether the SMMU has coherent page table walk
>
> "we detect the whether"?

Er, yeah, looks like rain tomorrow afternoon :P

>> capability from the IDR0.CTTW field, and base our cache maintenance
>> decisions on that. In preparation for fixing the bogus DMA API usage,
>> however, we need to ensure that the DMA API agrees about this, which
>> necessitates deferring to the dma-coherent property in the device tree
>> for the final say.
>>
>> As an added bonus, since systems exist where an external CTTW signal
>> has been tied off incorrectly at integration, allowing DT to override
>> it offers a neat workaround for coherency issues with such SMMUs.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/iommu/arm,smmu.txt |  4 ++++
>>   drivers/iommu/arm-smmu.c                             | 17 ++++++++++++++---
>>   2 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> index 0676050..04724fc 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> @@ -43,6 +43,10 @@ conditions.
>>
>>   ** System MMU optional properties:
>>
>> +- dma-coherent  : Presence or absence should correctly reflect whether
>> +                  the SMMU translation table walk interface is
>> +                  cache-coherent with the CPU.
>> +
>
> Please copy the text from:
> Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt#
> and remove the bit about stream table accesses.

Done, although since the resulting "...DMA operations made by the SMMU 
(page table walks)..." is both redundant and horribly clunky it gets 
rewritten as "...page table walks made by the SMMU..."

>>   - calxeda,smmu-secure-config-access : Enable proper handling of buggy
>>                     implementations that always use secure access to
>>                     SMMU configuration registers. In this case non-secure
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 4cd0c29..9a59bef 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1532,6 +1532,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>   	unsigned long size;
>>   	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>>   	u32 id;
>> +	bool cttw_dt, cttw_reg;
>>
>>   	dev_notice(smmu->dev, "probing hardware configuration...\n");
>>   	dev_notice(smmu->dev, "SMMUv%d with:\n", smmu->version);
>> @@ -1571,10 +1572,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>   		dev_notice(smmu->dev, "\taddress translation ops\n");
>>   	}
>>
>> -	if (id & ID0_CTTW) {
>> +	/*
>> +	 * In order for DMA API calls to work properly, we must defer to what
>> +	 * the DT says about coherency, regardless of what the hardware claims.
>> +	 * Fortunately, this also opens up a workaround for systems where the
>> +	 * ID register value has ended up configured incorrectly.
>> +	 */
>> +	cttw_dt = of_property_read_bool(smmu->dev->of_node, "dma-coherent");
>
> Use of_dma_is_coherent(smmu->dev->of_node)

*facepalm*

>> +	if (cttw_dt)
>>   		smmu->features |= ARM_SMMU_FEAT_COHERENT_WALK;
>> -		dev_notice(smmu->dev, "\tcoherent table walk\n");
>> -	}
>> +	cttw_reg = !!(id & ID0_CTTW);
>> +	if (cttw_dt || cttw_reg)
>> +		dev_notice(smmu->dev, "\t%scoherent table walk%s\n",
>> +			   cttw_dt ? "" : "no ",
>> +			   (cttw_dt == cttw_reg) ? "" : " (overridden by DT)");
>
> Similarly here; I think we should just follow the message printed by the
> SMMUv3 driver but s/IDR0.COHACC/ID0.CTTW/.

I note that the v3 driver stays silent as long as the DT and hardware 
agree - I'm reticent to entirely remove the original message here as 
it's proved rather useful (especially in debugging-over-email exchanges) 
to see at a glance from a boot log whether the driver thinks a 
particular SMMU is coherent or not (particularly with my SMMUv1 
many-instances-in-the-same-system hat on). I can certainly make the 
override message more consistent, though.

Robin