Re: [PATCH v2 17/17] iommu/arm-smmu: Add context init implementation hook

[Vivek Gautam <vivek.gautam@codeaurora.org>]

[-- Attachment #1: Type: text/plain, Size: 8220 bytes --]



On 8/16/2019 12:07 AM, Robin Murphy wrote:
> Allocating and initialising a context for a domain is another point
> where certain implementations are known to want special behaviour.
> Currently the other half of the Cavium workaround comes into play here,
> so let's finish the job to get the whole thing right out of the way.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/arm-smmu-impl.c | 42 ++++++++++++++++++++++++++---
>   drivers/iommu/arm-smmu.c      | 51 +++++++----------------------------
>   drivers/iommu/arm-smmu.h      | 42 +++++++++++++++++++++++++++--
>   3 files changed, 87 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c
> index 4dc8b1c4befb..e22e9004f449 100644
> --- a/drivers/iommu/arm-smmu-impl.c
> +++ b/drivers/iommu/arm-smmu-impl.c
> @@ -48,25 +48,60 @@ const struct arm_smmu_impl calxeda_impl = {
>   };
>   
>   
> +struct cavium_smmu {
> +	struct arm_smmu_device smmu;
> +	u32 id_base;
> +};
> +
>   static int cavium_cfg_probe(struct arm_smmu_device *smmu)
>   {
>   	static atomic_t context_count = ATOMIC_INIT(0);
> +	struct cavium_smmu *cs = container_of(smmu, struct cavium_smmu, smmu);
>   	/*
>   	 * Cavium CN88xx erratum #27704.
>   	 * Ensure ASID and VMID allocation is unique across all SMMUs in
>   	 * the system.
>   	 */
> -	smmu->cavium_id_base = atomic_fetch_add(smmu->num_context_banks,
> -						   &context_count);
> +	cs->id_base = atomic_fetch_add(smmu->num_context_banks, &context_count);
>   	dev_notice(smmu->dev, "\tenabling workaround for Cavium erratum 27704\n");
>   
>   	return 0;
>   }
>   
> +int cavium_init_context(struct arm_smmu_domain *smmu_domain)
> +{
> +	struct cavium_smmu *cs = container_of(smmu_domain->smmu,
> +					      struct cavium_smmu, smmu);
> +
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2)
> +		smmu_domain->cfg.vmid += cs->id_base;
> +	else
> +		smmu_domain->cfg.asid += cs->id_base;
> +
> +	return 0;
> +}
> +
>   const struct arm_smmu_impl cavium_impl = {
>   	.cfg_probe = cavium_cfg_probe,
> +	.init_context = cavium_init_context,
>   };
>   
> +struct arm_smmu_device *cavium_smmu_impl_init(struct arm_smmu_device *smmu)
> +{
> +	struct cavium_smmu *cs;
> +
> +	cs = devm_kzalloc(smmu->dev, sizeof(*cs), GFP_KERNEL);
> +	if (!cs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cs->smmu = *smmu;
> +	cs->smmu.impl = &cavium_impl;
> +
> +	devm_kfree(smmu->dev, smmu);
> +
> +	return &cs->smmu;
> +}
> +
>   
>   #define ARM_MMU500_ACTLR_CPRE		(1 << 1)
>   
> @@ -126,8 +161,7 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
>   		smmu->impl = &arm_mmu500_impl;
>   		break;
>   	case CAVIUM_SMMUV2:
> -		smmu->impl = &cavium_impl;
> -		break;
> +		return cavium_smmu_impl_init(smmu);
>   	default:
>   		break;
>   	}
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index fc98992d120d..b8628e2ab579 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -27,7 +27,6 @@
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
>   #include <linux/io-64-nonatomic-hi-lo.h>
> -#include <linux/io-pgtable.h>
>   #include <linux/iopoll.h>
>   #include <linux/init.h>
>   #include <linux/moduleparam.h>
> @@ -111,44 +110,6 @@ struct arm_smmu_master_cfg {
>   #define for_each_cfg_sme(fw, i, idx) \
>   	for (i = 0; idx = fwspec_smendx(fw, i), i < fw->num_ids; ++i)
>   
> -enum arm_smmu_context_fmt {
> -	ARM_SMMU_CTX_FMT_NONE,
> -	ARM_SMMU_CTX_FMT_AARCH64,
> -	ARM_SMMU_CTX_FMT_AARCH32_L,
> -	ARM_SMMU_CTX_FMT_AARCH32_S,
> -};
> -
> -struct arm_smmu_cfg {
> -	u8				cbndx;
> -	u8				irptndx;
> -	union {
> -		u16			asid;
> -		u16			vmid;
> -	};
> -	enum arm_smmu_cbar_type		cbar;
> -	enum arm_smmu_context_fmt	fmt;
> -};
> -#define INVALID_IRPTNDX			0xff
> -
> -enum arm_smmu_domain_stage {
> -	ARM_SMMU_DOMAIN_S1 = 0,
> -	ARM_SMMU_DOMAIN_S2,
> -	ARM_SMMU_DOMAIN_NESTED,
> -	ARM_SMMU_DOMAIN_BYPASS,
> -};
> -
> -struct arm_smmu_domain {
> -	struct arm_smmu_device		*smmu;
> -	struct io_pgtable_ops		*pgtbl_ops;
> -	const struct iommu_gather_ops	*tlb_ops;
> -	struct arm_smmu_cfg		cfg;
> -	enum arm_smmu_domain_stage	stage;
> -	bool				non_strict;
> -	struct mutex			init_mutex; /* Protects smmu pointer */
> -	spinlock_t			cb_lock; /* Serialises ATS1* ops and TLB syncs */
> -	struct iommu_domain		domain;
> -};
> -
>   static bool using_legacy_binding, using_generic_binding;
>   
>   static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> @@ -749,9 +710,16 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	}
>   
>   	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2)
> -		cfg->vmid = cfg->cbndx + 1 + smmu->cavium_id_base;
> +		cfg->vmid = cfg->cbndx + 1;
>   	else
> -		cfg->asid = cfg->cbndx + smmu->cavium_id_base;
> +		cfg->asid = cfg->cbndx;
> +
> +	smmu_domain->smmu = smmu;
> +	if (smmu->impl && smmu->impl->init_context) {
> +		ret = smmu->impl->init_context(smmu_domain);
> +		if (ret)
> +			goto out_unlock;
> +	}
>   
>   	pgtbl_cfg = (struct io_pgtable_cfg) {
>   		.pgsize_bitmap	= smmu->pgsize_bitmap,
> @@ -765,7 +733,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	if (smmu_domain->non_strict)
>   		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
>   
> -	smmu_domain->smmu = smmu;
>   	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
>   	if (!pgtbl_ops) {
>   		ret = -ENOMEM;
> diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> index ddafe872a396..611ed742e56f 100644
> --- a/drivers/iommu/arm-smmu.h
> +++ b/drivers/iommu/arm-smmu.h
> @@ -14,6 +14,7 @@
>   #include <linux/bits.h>
>   #include <linux/clk.h>
>   #include <linux/device.h>
> +#include <linux/io-pgtable.h>
>   #include <linux/iommu.h>
>   #include <linux/mutex.h>
>   #include <linux/spinlock.h>
> @@ -270,14 +271,50 @@ struct arm_smmu_device {
>   	struct clk_bulk_data		*clks;
>   	int				num_clks;
>   
> -	u32				cavium_id_base; /* Specific to Cavium */
> -
>   	spinlock_t			global_sync_lock;
>   
>   	/* IOMMU core code handle */
>   	struct iommu_device		iommu;
>   };
>   
> +enum arm_smmu_context_fmt {
> +	ARM_SMMU_CTX_FMT_NONE,
> +	ARM_SMMU_CTX_FMT_AARCH64,
> +	ARM_SMMU_CTX_FMT_AARCH32_L,
> +	ARM_SMMU_CTX_FMT_AARCH32_S,
> +};
> +
> +struct arm_smmu_cfg {
> +	u8				cbndx;
> +	u8				irptndx;
> +	union {
> +		u16			asid;
> +		u16			vmid;
> +	};
> +	enum arm_smmu_cbar_type		cbar;
> +	enum arm_smmu_context_fmt	fmt;
> +};
> +#define INVALID_IRPTNDX			0xff
> +
> +enum arm_smmu_domain_stage {
> +	ARM_SMMU_DOMAIN_S1 = 0,
> +	ARM_SMMU_DOMAIN_S2,
> +	ARM_SMMU_DOMAIN_NESTED,
> +	ARM_SMMU_DOMAIN_BYPASS,
> +};
> +
> +struct arm_smmu_domain {
> +	struct arm_smmu_device		*smmu;
> +	struct io_pgtable_ops		*pgtbl_ops;
> +	const struct iommu_gather_ops	*tlb_ops;
> +	struct arm_smmu_cfg		cfg;
> +	enum arm_smmu_domain_stage	stage;
> +	bool				non_strict;
> +	struct mutex			init_mutex; /* Protects smmu pointer */
> +	spinlock_t			cb_lock; /* Serialises ATS1* ops and TLB syncs */
> +	struct iommu_domain		domain;
> +};
> +
>   
>   /* Implementation details, yay! */
>   struct arm_smmu_impl {
> @@ -289,6 +326,7 @@ struct arm_smmu_impl {
>   			    u64 val);
>   	int (*cfg_probe)(struct arm_smmu_device *smmu);
>   	int (*reset)(struct arm_smmu_device *smmu);
> +	int (*init_context)(struct arm_smmu_domain *smmu_domain);

Hi Robin,

Sorry for responding late to this series. I have couple of doubts here 
that I wanted to discuss.

Are we standardizing these implementation specific ops? Each vendor 
implementations will have something peculiar to take care. Things are 
good right now with 'reset', 'cfg_probe', and 'init_context' hooks.
But, on top of vendor implementation details, there can be SoC specific 
errata changes that need to be added.
Moreover, adding implementation data based on __model__ may not suffice 
for long. Do you suggest adding any other data variable in the 
ARM_SMMU_MATCH_DATA?
To show SoC specific needs, I have the change attached in this email to 
take care of the SDM845 'wait-for-safe' sequence.
Please take a look.

Thanks & Regards
Vivek

>   };
>   
>   static inline void __iomem *arm_smmu_page(struct arm_smmu_device *smmu, int n)


[-- Attachment #2: 0003-iommu-arm-smmu-impl-Add-SDM845-specific-implementati.patch --]
[-- Type: text/plain, Size: 3417 bytes --]

From 3830ec7e22deb49de72b6bc29bd965f7b07b9669 Mon Sep 17 00:00:00 2001
From: Vivek Gautam <vivek.gautam@codeaurora.org>
Date: Tue, 20 Aug 2019 15:28:16 +0530
Subject: [PATCH 3/4] iommu: arm-smmu-impl: Add SDM845 specific implementation
 hook

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/iommu/arm-smmu-impl.c | 31 +++++++++++++++++++++++++++++++
 drivers/iommu/arm-smmu.c      |  2 ++
 drivers/iommu/arm-smmu.h      |  1 +
 3 files changed, 34 insertions(+)

diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c
index 3f88cd078dd5..0e6f5ab0e0ce 100644
--- a/drivers/iommu/arm-smmu-impl.c
+++ b/drivers/iommu/arm-smmu-impl.c
@@ -6,6 +6,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/of.h>
+#include <linux/qcom_scm.h>
 
 #include "arm-smmu.h"
 
@@ -148,6 +149,32 @@ static const struct arm_smmu_impl arm_mmu500_impl = {
 };
 
 
+static int qcom_sdm845_smmu500_cfg_probe(struct arm_smmu_device *smmu)
+{
+	int ret;
+
+	/*
+	 * To address performance degradation in non-real time clients,
+	 * such as USB and UFS, turn off wait-for-safe on sdm845 platforms,
+	 * such as MTP, whose firmwares implement corresponding secure monitor
+	 * call handlers.
+	 */
+	if (of_property_read_bool(smmu->dev->of_node,
+				  "qcom,smmu-500-fw-impl-safe-errata")) {
+		ret = qcom_scm_qsmmu500_wait_safe_toggle(0);
+		if (ret)
+			dev_warn(smmu->dev, "Failed to turn off SAFE logic\n");
+	}
+
+	return 0;
+}
+
+const struct arm_smmu_impl qcom_sdm845_smmu500_impl = {
+	.reset = arm_mmu500_reset,
+	.cfg_probe = qcom_sdm845_smmu500_cfg_probe,
+};
+
+
 struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
 {
 	/*
@@ -170,5 +197,9 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
 				  "calxeda,smmu-secure-config-access"))
 		smmu->impl = &calxeda_impl;
 
+	if (smmu->model == QCOM_SMMU500 &&
+	    of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-500"))
+		smmu->impl = &qcom_sdm845_smmu500_impl;
+
 	return smmu;
 }
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index dd7f78a8e146..f3e5e20ebe9c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1825,6 +1825,7 @@ ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
 ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
 ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
 ARM_SMMU_MATCH_DATA(qcom_smmuv2, ARM_SMMU_V2, QCOM_SMMUV2);
+ARM_SMMU_MATCH_DATA(qcom_smmu500, ARM_SMMU_V2, QCOM_SMMU500);
 
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
@@ -1834,6 +1835,7 @@ static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,mmu-500", .data = &arm_mmu500 },
 	{ .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 },
 	{ .compatible = "qcom,smmu-v2", .data = &qcom_smmuv2 },
+	{ .compatible = "qcom,sdm845-smmu-500", .data = &qcom_smmu500 },
 	{ },
 };
 
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index ac9eac966cf5..48211aad32ea 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -220,6 +220,7 @@ enum arm_smmu_implementation {
 	ARM_MMU500,
 	CAVIUM_SMMUV2,
 	QCOM_SMMUV2,
+	QCOM_SMMU500,
 };
 
 struct arm_smmu_device {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


[-- Attachment #3: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu