Re: [PATCH 3/6] iommu/arm-smmu: add support for iova_to_phys through ATS1PR

Hi Mitch,

On Tue, Aug 19, 2014 at 07:12:41PM +0100, Mitchel Humpherys wrote:

On Tue, Aug 19 2014 at 05:44:32 AM, Will Deacon [off-list ref] wrote:

quoted

We don't have writeq for arch/arm/.

Ah yes looks like this is an MSM-ism that never made it upstream since
it wouldn't be guaranteed to be atomic. I'll make sure to do arm32
compiles on upstream kernels for future patches, sorry!

I guess we could use <asm-generic/io-64-nonatomic-lo-hi.h> but I can
also re-work this to be two separate writel's.

Yeah, just do two writels.

quoted

quoted

+	}
+
+	mb();

Why?

My thought was that if we start polling ATSR_ACTIVE prematurely (before
the write to ATS1PR actually finishes) all heck could break loose? Not
sure if that's a bogus assumption due to device memory being strongly
ordered?

I think the device-memory guarantees should be enough. If not, we need a
comment explaining why.

quoted

quoted

+	while (readl_relaxed(cb_base + ARM_SMMU_CB_ATSR) & ATSR_ACTIVE) {
+		if (++count == ATSR_LOOP_TIMEOUT) {
+			dev_err(dev,
+				"iova to phys timed out on 0x%pa for %s. Falling back to software table walk.\n",
+				&iova, dev_name(dev));
+			arm_smmu_disable_clocks(smmu);
+			return arm_smmu_iova_to_phys_soft(domain, iova);
+		}
+		cpu_relax();
+	}

Do you know what happened to Olav's patches to make this sort of code
generic?

I assume you're talking about this, right?

    http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267943.html

Yeah looks like he never sent an update since it was part of a series
that wasn't going to make it in (the qsmmu driver). I can always bring
that patch (actually Matt Wagantall's patch) in here and rework this to
use that.

Yup, I think it would be useful to revive that as a separate series.

quoted

quoted

@@ -2005,6 +2073,11 @@ int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		return -ENODEV;
 	}
 
+	if (smmu->version == 1 || (!(id & ID0_ATOSNS) && (id & ID0_S1TS))) {

Are you sure about this? The v2 spec says that is ATOSNS is clear then S1TS
is also clear.

I was looking at Section 4.1.1 of ARM IHI 0062C ID091613 which states:

    In SMMUv2, the address translation registers are OPTIONAL. The
    address translation registers are implemented only when both:

        o The SMMU_IDR0.S1TS bit is set to 1.
        o The SMMU_IDR0.ATOSNS bit is set to 0.

I assume you're referring to section 9.6.1 of the same document:

    ATOSNS, bit[26]
    Address Translation Operations Not Supported. The possible values of
    this bit are:

        0 Address translation operations are supported. Stage 1
          translation is not supported, that is, the S1TS bit is set to 0.

        1 Address translation operations are not supported. Stage 1
          translation is supported, that is, the S1TS bit is set to 1.

If that really means that S1TS and ATOSNS always have the same value
then Section 4.1.1 doesn't make any sense. Or am I missing something?

I'll get this checked, as those two paragraphs don't make an awful lot of
sense together.

Will