[PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups

[PATCH 0/7] iommu/ipmmu-vmsa: Suspend/resume support and assorted cleanups

[Geert Uytterhoeven]

	Hi Jörg, Magnus,

On R-Car Gen3 systems with PSCI, PSCI may power down the SoC during
system suspend, thus losing all IOMMU state.  Hence after s2ram, devices
behind an IPMMU (e.g. SATA), and configured to use it, will fail to
complete their I/O operations.

This patch series adds suspend/resume support to the Renesas IPMMU-VMSA
IOMMU driver, and performs some smaller cleanups and fixes during the
process.  Most patches are fairly independent, except for patch 7/7,
which depends on patches 5/7 and 6/7.

This has been tested on Salvator-XS with R-Car H3 ES2.0, with IPMMU
suport for SATA enabled.  To play safe, the resume operation has also
been tested on R-Car M2-W, where it is currently not enabled due to the
absence of PSCI in the firmware.

Thanks for your comments!

Geert Uytterhoeven (7):
  iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs
  iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding
  iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses
  iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned
  iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features
  iommu/ipmmu-vmsa: Extract hardware context initialization
  iommu/ipmmu-vmsa: Add suspend/resume support

 drivers/iommu/ipmmu-vmsa.c | 194 +++++++++++++++++++++++++------------
 1 file changed, 131 insertions(+), 63 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/7] iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs

[Geert Uytterhoeven]

As of commit 7af9a5fdb9e0ca33 ("iommu/ipmmu-vmsa: Use
iommu_device_sysfs_add()/remove()"), IOMMU devices show up under
/sys/class/iommus/, but their "devices" subdirectories are empty.
Likewise, devices tied to an IOMMU do not have an "iommu" backlink.

Make sure all links are created, on both arm32 and arm64.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/iommu/ipmmu-vmsa.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9a380c10655e182d..9f2b781e20a0eba6 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -885,27 +885,37 @@ static int ipmmu_init_arm_mapping(struct device *dev)
 
 static int ipmmu_add_device(struct device *dev)
 {
+	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
 	struct iommu_group *group;
+	int ret;
 
 	/*
 	 * Only let through devices that have been verified in xlate()
 	 */
-	if (!to_ipmmu(dev))
+	if (!mmu)
 		return -ENODEV;
 
-	if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA))
-		return ipmmu_init_arm_mapping(dev);
+	if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA)) {
+		ret = ipmmu_init_arm_mapping(dev);
+		if (ret)
+			return ret;
+	} else {
+		group = iommu_group_get_for_dev(dev);
+		if (IS_ERR(group))
+			return PTR_ERR(group);
 
-	group = iommu_group_get_for_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
+		iommu_group_put(group);
+	}
 
-	iommu_group_put(group);
+	iommu_device_link(&mmu->iommu, dev);
 	return 0;
 }
 
 static void ipmmu_remove_device(struct device *dev)
 {
+	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
+
+	iommu_device_unlink(&mmu->iommu, dev);
 	arm_iommu_detach_device(dev);
 	iommu_group_remove_device(dev);
 }
-- 
2.17.1

[PATCH 2/7] iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding

[Geert Uytterhoeven]

There is a helper to write to the root IPMMU instance's registers, so
let's use it.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/iommu/ipmmu-vmsa.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9f2b781e20a0eba6..ac70cb967ff821c6 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -280,8 +280,7 @@ static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain,
 		ipmmu_write(domain->mmu,
 			    domain->context_id * IM_CTX_SIZE + reg, data);
 
-	ipmmu_write(domain->mmu->root,
-		    domain->context_id * IM_CTX_SIZE + reg, data);
+	ipmmu_ctx_write_root(domain, reg, data);
 }
 
 /* -----------------------------------------------------------------------------
-- 
2.17.1

[PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses

[Geert Uytterhoeven]

On R-Car Gen3, the faulting virtual address is a 40-bit address, and
comprised of two registers.  Read the upper address part, and combine
both parts, when running on a 64-bit system.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Apart from this, the driver doesn't support 40-bit IOVA addresses yet.
---
 drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index ac70cb967ff821c6..4d07c26c97848b65 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -186,7 +186,9 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev)
 #define IMMAIR_ATTR_IDX_WBRWA		1
 #define IMMAIR_ATTR_IDX_DEV		2
 
-#define IMEAR				0x0030
+#define IMEAR				0x0030	/* R-Car Gen2 */
+#define IMELAR				IMEAR	/* R-Car Gen3 */
+#define IMEUAR				0x0034	/* R-Car Gen3 */
 
 #define IMPCTR				0x0200
 #define IMPSTR				0x0208
@@ -521,14 +523,16 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
 {
 	const u32 err_mask = IMSTR_MHIT | IMSTR_ABORT | IMSTR_PF | IMSTR_TF;
 	struct ipmmu_vmsa_device *mmu = domain->mmu;
+	unsigned long iova;
 	u32 status;
-	u32 iova;
 
 	status = ipmmu_ctx_read_root(domain, IMSTR);
 	if (!(status & err_mask))
 		return IRQ_NONE;
 
-	iova = ipmmu_ctx_read_root(domain, IMEAR);
+	iova = ipmmu_ctx_read_root(domain, IMELAR);
+	if (IS_ENABLED(CONFIG_64BIT))
+		iova |= (u64)ipmmu_ctx_read_root(domain, IMEUAR) << 32;
 
 	/*
 	 * Clear the error status flags. Unlike traditional interrupt flag
@@ -540,10 +544,10 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
 
 	/* Log fatal errors. */
 	if (status & IMSTR_MHIT)
-		dev_err_ratelimited(mmu->dev, "Multiple TLB hits @0x%08x\n",
+		dev_err_ratelimited(mmu->dev, "Multiple TLB hits @0x%lx\n",
 				    iova);
 	if (status & IMSTR_ABORT)
-		dev_err_ratelimited(mmu->dev, "Page Table Walk Abort @0x%08x\n",
+		dev_err_ratelimited(mmu->dev, "Page Table Walk Abort @0x%lx\n",
 				    iova);
 
 	if (!(status & (IMSTR_PF | IMSTR_TF)))
@@ -559,7 +563,7 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
 		return IRQ_HANDLED;
 
 	dev_err_ratelimited(mmu->dev,
-			    "Unhandled fault: status 0x%08x iova 0x%08x\n",
+			    "Unhandled fault: status 0x%08x iova 0x%lx\n",
 			    status, iova);
 
 	return IRQ_HANDLED;
-- 
2.17.1

[PATCH 4/7] iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned

[Geert Uytterhoeven]

Make the IPMMU_CTX_MAX constant unsigned, to match the type of
ipmmu_features.number_of_contexts.

This allows to use plain min() instead of type-casting min_t().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/iommu/ipmmu-vmsa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 4d07c26c97848b65..4e3134a9a52f8d87 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -36,7 +36,7 @@
 #define arm_iommu_detach_device(...)	do {} while (0)
 #endif
 
-#define IPMMU_CTX_MAX 8
+#define IPMMU_CTX_MAX 8U
 
 struct ipmmu_features {
 	bool use_ns_alias_offset;
@@ -1060,8 +1060,7 @@ static int ipmmu_probe(struct platform_device *pdev)
 	if (mmu->features->use_ns_alias_offset)
 		mmu->base += IM_NS_ALIAS_OFFSET;
 
-	mmu->num_ctx = min_t(unsigned int, IPMMU_CTX_MAX,
-			     mmu->features->number_of_contexts);
+	mmu->num_ctx = min(IPMMU_CTX_MAX, mmu->features->number_of_contexts);
 
 	irq = platform_get_irq(pdev, 0);
 
-- 
2.17.1

[PATCH 5/7] iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features

[Geert Uytterhoeven]

The maximum number of micro-TLBs per IPMMU instance is not fixed, but
depends on the SoC type.  Hence move it from struct ipmmu_vmsa_device to
struct ipmmu_features, and set up the correct value for both R-Car Gen2
and Gen3 SoCs.

Note that currently no code uses this value.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/iommu/ipmmu-vmsa.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 4e3134a9a52f8d87..0a21e734466eb1bd 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -42,6 +42,7 @@ struct ipmmu_features {
 	bool use_ns_alias_offset;
 	bool has_cache_leaf_nodes;
 	unsigned int number_of_contexts;
+	unsigned int num_utlbs;
 	bool setup_imbuscr;
 	bool twobit_imttbcr_sl0;
 	bool reserved_context;
@@ -53,7 +54,6 @@ struct ipmmu_vmsa_device {
 	struct iommu_device iommu;
 	struct ipmmu_vmsa_device *root;
 	const struct ipmmu_features *features;
-	unsigned int num_utlbs;
 	unsigned int num_ctx;
 	spinlock_t lock;			/* Protects ctx and domains[] */
 	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
@@ -972,6 +972,7 @@ static const struct ipmmu_features ipmmu_features_default = {
 	.use_ns_alias_offset = true,
 	.has_cache_leaf_nodes = false,
 	.number_of_contexts = 1, /* software only tested with one context */
+	.num_utlbs = 32,
 	.setup_imbuscr = true,
 	.twobit_imttbcr_sl0 = false,
 	.reserved_context = false,
@@ -981,6 +982,7 @@ static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
 	.use_ns_alias_offset = false,
 	.has_cache_leaf_nodes = true,
 	.number_of_contexts = 8,
+	.num_utlbs = 48,
 	.setup_imbuscr = false,
 	.twobit_imttbcr_sl0 = true,
 	.reserved_context = true,
@@ -1033,7 +1035,6 @@ static int ipmmu_probe(struct platform_device *pdev)
 	}
 
 	mmu->dev = &pdev->dev;
-	mmu->num_utlbs = 48;
 	spin_lock_init(&mmu->lock);
 	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
 	mmu->features = of_device_get_match_data(&pdev->dev);
-- 
2.17.1

[PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization

[Geert Uytterhoeven]

ipmmu_domain_init_context() takes care of (1) initializing the software
domain, and (2) initializing the hardware context for the domain.

Extract the code to initialize the hardware context into a new subroutine
ipmmu_context_init(), to prepare for later reuse.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/iommu/ipmmu-vmsa.c | 91 ++++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 43 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 0a21e734466eb1bd..92a766dd8b459f0c 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -404,52 +404,10 @@ static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
 	spin_unlock_irqrestore(&mmu->lock, flags);
 }
 
-static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
+static void ipmmu_context_init(struct ipmmu_vmsa_domain *domain)
 {
 	u64 ttbr;
 	u32 tmp;
-	int ret;
-
-	/*
-	 * Allocate the page table operations.
-	 *
-	 * VMSA states in section B3.6.3 "Control of Secure or Non-secure memory
-	 * access, Long-descriptor format" that the NStable bit being set in a
-	 * table descriptor will result in the NStable and NS bits of all child
-	 * entries being ignored and considered as being set. The IPMMU seems
-	 * not to comply with this, as it generates a secure access page fault
-	 * if any of the NStable and NS bits isn't set when running in
-	 * non-secure mode.
-	 */
-	domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
-	domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
-	domain->cfg.ias = 32;
-	domain->cfg.oas = 40;
-	domain->cfg.tlb = &ipmmu_gather_ops;
-	domain->io_domain.geometry.aperture_end = DMA_BIT_MASK(32);
-	domain->io_domain.geometry.force_aperture = true;
-	/*
-	 * TODO: Add support for coherent walk through CCI with DVM and remove
-	 * cache handling. For now, delegate it to the io-pgtable code.
-	 */
-	domain->cfg.iommu_dev = domain->mmu->root->dev;
-
-	/*
-	 * Find an unused context.
-	 */
-	ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
-	if (ret < 0)
-		return ret;
-
-	domain->context_id = ret;
-
-	domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
-					   domain);
-	if (!domain->iop) {
-		ipmmu_domain_free_context(domain->mmu->root,
-					  domain->context_id);
-		return -EINVAL;
-	}
 
 	/* TTBR0 */
 	ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
@@ -495,7 +453,54 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
 	 */
 	ipmmu_ctx_write_all(domain, IMCTR,
 			    IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
+}
+
+static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
+{
+	int ret;
+
+	/*
+	 * Allocate the page table operations.
+	 *
+	 * VMSA states in section B3.6.3 "Control of Secure or Non-secure memory
+	 * access, Long-descriptor format" that the NStable bit being set in a
+	 * table descriptor will result in the NStable and NS bits of all child
+	 * entries being ignored and considered as being set. The IPMMU seems
+	 * not to comply with this, as it generates a secure access page fault
+	 * if any of the NStable and NS bits isn't set when running in
+	 * non-secure mode.
+	 */
+	domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
+	domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
+	domain->cfg.ias = 32;
+	domain->cfg.oas = 40;
+	domain->cfg.tlb = &ipmmu_gather_ops;
+	domain->io_domain.geometry.aperture_end = DMA_BIT_MASK(32);
+	domain->io_domain.geometry.force_aperture = true;
+	/*
+	 * TODO: Add support for coherent walk through CCI with DVM and remove
+	 * cache handling. For now, delegate it to the io-pgtable code.
+	 */
+	domain->cfg.iommu_dev = domain->mmu->root->dev;
+
+	/*
+	 * Find an unused context.
+	 */
+	ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
+	if (ret < 0)
+		return ret;
+
+	domain->context_id = ret;
+
+	domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
+					   domain);
+	if (!domain->iop) {
+		ipmmu_domain_free_context(domain->mmu->root,
+					  domain->context_id);
+		return -EINVAL;
+	}
 
+	ipmmu_context_init(domain);
 	return 0;
 }
 
-- 
2.17.1

[PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support

[Geert Uytterhoeven]

During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
IPMMU state is lost.  Hence after s2ram, devices wired behind an IPMMU,
and configured to use it, will see their DMA operations hang.

To fix this, restore all IPMMU contexts, and re-enable all active
micro-TLBs during system resume.

To avoid overhead on platforms not needing it, the resume code has a
build time dependency on sleep and PSCI support, and a runtime
dependency on PSCI.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This patch takes a different approach than the BSP, which implements a
bulk save/restore of all registers during system suspend/resume.

 drivers/iommu/ipmmu-vmsa.c | 52 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 92a766dd8b459f0c..5d22139914e8f033 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -22,6 +22,7 @@
 #include <linux/of_iommu.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/psci.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
@@ -36,7 +37,10 @@
 #define arm_iommu_detach_device(...)	do {} while (0)
 #endif
 
-#define IPMMU_CTX_MAX 8U
+#define IPMMU_CTX_MAX		8U
+#define IPMMU_CTX_INVALID	-1
+
+#define IPMMU_UTLB_MAX		48U
 
 struct ipmmu_features {
 	bool use_ns_alias_offset;
@@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
 	spinlock_t lock;			/* Protects ctx and domains[] */
 	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
 	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
+	s8 utlb_ctx[IPMMU_UTLB_MAX];
 
 	struct iommu_group *group;
 	struct dma_iommu_mapping *mapping;
@@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
 	ipmmu_write(mmu, IMUCTR(utlb),
 		    IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
 		    IMUCTR_MMUEN);
+	mmu->utlb_ctx[utlb] = domain->context_id;
 }
 
 /*
@@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
 	struct ipmmu_vmsa_device *mmu = domain->mmu;
 
 	ipmmu_write(mmu, IMUCTR(utlb), 0);
+	mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
 }
 
 static void ipmmu_tlb_flush_all(void *cookie)
@@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
 	spin_lock_init(&mmu->lock);
 	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
 	mmu->features = of_device_get_match_data(&pdev->dev);
+	memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
 	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
 
 	/* Map I/O memory and request IRQ. */
@@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
+static int ipmmu_resume_noirq(struct device *dev)
+{
+	struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
+	unsigned int i;
+
+	/* This is the best we can do to check for the presence of PSCI */
+	if (!psci_ops.cpu_suspend)
+		return 0;
+
+	/* Reset root MMU and restore contexts */
+	if (ipmmu_is_root(mmu)) {
+		ipmmu_device_reset(mmu);
+
+		for (i = 0; i < mmu->num_ctx; i++) {
+			if (!mmu->domains[i])
+				continue;
+
+			ipmmu_context_init(mmu->domains[i]);
+		}
+	}
+
+	/* Re-enable active micro-TLBs */
+	for (i = 0; i < mmu->features->num_utlbs; i++) {
+		if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
+			continue;
+
+		ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops ipmmu_pm  = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
+};
+#define DEV_PM_OPS	&ipmmu_pm
+#else
+#define DEV_PM_OPS	NULL
+#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
+
 static struct platform_driver ipmmu_driver = {
 	.driver = {
 		.name = "ipmmu-vmsa",
 		.of_match_table = of_match_ptr(ipmmu_of_ids),
+		.pm = DEV_PM_OPS,
 	},
 	.probe = ipmmu_probe,
 	.remove	= ipmmu_remove,
-- 
2.17.1

Re: [PATCH 1/7] iommu/ipmmu-vmsa: Link IOMMUs and devices in sysfs

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wed, Feb 20, 2019 at 04:05:25PM +0100, Geert Uytterhoeven wrote:
> As of commit 7af9a5fdb9e0ca33 ("iommu/ipmmu-vmsa: Use
> iommu_device_sysfs_add()/remove()"), IOMMU devices show up under
> /sys/class/iommus/, but their "devices" subdirectories are empty.
> Likewise, devices tied to an IOMMU do not have an "iommu" backlink.
> 
> Make sure all links are created, on both arm32 and arm64.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/iommu/ipmmu-vmsa.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 9a380c10655e182d..9f2b781e20a0eba6 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -885,27 +885,37 @@ static int ipmmu_init_arm_mapping(struct device *dev)
>  
>  static int ipmmu_add_device(struct device *dev)
>  {
> +	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
>  	struct iommu_group *group;
> +	int ret;
>  
>  	/*
>  	 * Only let through devices that have been verified in xlate()
>  	 */
> -	if (!to_ipmmu(dev))
> +	if (!mmu)
>  		return -ENODEV;
>  
> -	if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA))
> -		return ipmmu_init_arm_mapping(dev);
> +	if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA)) {
> +		ret = ipmmu_init_arm_mapping(dev);
> +		if (ret)
> +			return ret;
> +	} else {
> +		group = iommu_group_get_for_dev(dev);
> +		if (IS_ERR(group))
> +			return PTR_ERR(group);
>  
> -	group = iommu_group_get_for_dev(dev);
> -	if (IS_ERR(group))
> -		return PTR_ERR(group);
> +		iommu_group_put(group);
> +	}
>  
> -	iommu_group_put(group);
> +	iommu_device_link(&mmu->iommu, dev);
>  	return 0;
>  }
>  
>  static void ipmmu_remove_device(struct device *dev)
>  {
> +	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
> +
> +	iommu_device_unlink(&mmu->iommu, dev);
>  	arm_iommu_detach_device(dev);
>  	iommu_group_remove_device(dev);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] iommu/ipmmu-vmsa: Call ipmmu_ctx_write_root() instead of open coding

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wed, Feb 20, 2019 at 04:05:26PM +0100, Geert Uytterhoeven wrote:
> There is a helper to write to the root IPMMU instance's registers, so
> let's use it.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/iommu/ipmmu-vmsa.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 9f2b781e20a0eba6..ac70cb967ff821c6 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -280,8 +280,7 @@ static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain,
>  		ipmmu_write(domain->mmu,
>  			    domain->context_id * IM_CTX_SIZE + reg, data);
>  
> -	ipmmu_write(domain->mmu->root,
> -		    domain->context_id * IM_CTX_SIZE + reg, data);
> +	ipmmu_ctx_write_root(domain, reg, data);

This makes the code harder to read in my opinion, and I don't think it
bring much optimization.

>  }
>  
>  /* -----------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wed, Feb 20, 2019 at 04:05:27PM +0100, Geert Uytterhoeven wrote:
> On R-Car Gen3, the faulting virtual address is a 40-bit address, and
> comprised of two registers.  Read the upper address part, and combine
> both parts, when running on a 64-bit system.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Apart from this, the driver doesn't support 40-bit IOVA addresses yet.
> ---
>  drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index ac70cb967ff821c6..4d07c26c97848b65 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -186,7 +186,9 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev)
>  #define IMMAIR_ATTR_IDX_WBRWA		1
>  #define IMMAIR_ATTR_IDX_DEV		2
>  
> -#define IMEAR				0x0030
> +#define IMEAR				0x0030	/* R-Car Gen2 */
> +#define IMELAR				IMEAR	/* R-Car Gen3 */

I would have defined that as a single macro.

> +#define IMEUAR				0x0034	/* R-Car Gen3 */
>  
>  #define IMPCTR				0x0200
>  #define IMPSTR				0x0208
> @@ -521,14 +523,16 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
>  {
>  	const u32 err_mask = IMSTR_MHIT | IMSTR_ABORT | IMSTR_PF | IMSTR_TF;
>  	struct ipmmu_vmsa_device *mmu = domain->mmu;
> +	unsigned long iova;

Isn't there a more appropriate type, such as dma_addr_t possibly ?

>  	u32 status;
> -	u32 iova;
>  
>  	status = ipmmu_ctx_read_root(domain, IMSTR);
>  	if (!(status & err_mask))
>  		return IRQ_NONE;
>  
> -	iova = ipmmu_ctx_read_root(domain, IMEAR);
> +	iova = ipmmu_ctx_read_root(domain, IMELAR);
> +	if (IS_ENABLED(CONFIG_64BIT))
> +		iova |= (u64)ipmmu_ctx_read_root(domain, IMEUAR) << 32;
>  
>  	/*
>  	 * Clear the error status flags. Unlike traditional interrupt flag
> @@ -540,10 +544,10 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
>  
>  	/* Log fatal errors. */
>  	if (status & IMSTR_MHIT)
> -		dev_err_ratelimited(mmu->dev, "Multiple TLB hits @0x%08x\n",
> +		dev_err_ratelimited(mmu->dev, "Multiple TLB hits @0x%lx\n",
>  				    iova);
>  	if (status & IMSTR_ABORT)
> -		dev_err_ratelimited(mmu->dev, "Page Table Walk Abort @0x%08x\n",
> +		dev_err_ratelimited(mmu->dev, "Page Table Walk Abort @0x%lx\n",
>  				    iova);
>  
>  	if (!(status & (IMSTR_PF | IMSTR_TF)))
> @@ -559,7 +563,7 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
>  		return IRQ_HANDLED;
>  
>  	dev_err_ratelimited(mmu->dev,
> -			    "Unhandled fault: status 0x%08x iova 0x%08x\n",
> +			    "Unhandled fault: status 0x%08x iova 0x%lx\n",
>  			    status, iova);
>  
>  	return IRQ_HANDLED;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] iommu/ipmmu-vmsa: Make IPMMU_CTX_MAX unsigned

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wed, Feb 20, 2019 at 04:05:28PM +0100, Geert Uytterhoeven wrote:
> Make the IPMMU_CTX_MAX constant unsigned, to match the type of
> ipmmu_features.number_of_contexts.
> 
> This allows to use plain min() instead of type-casting min_t().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/iommu/ipmmu-vmsa.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 4d07c26c97848b65..4e3134a9a52f8d87 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -36,7 +36,7 @@
>  #define arm_iommu_detach_device(...)	do {} while (0)
>  #endif
>  
> -#define IPMMU_CTX_MAX 8
> +#define IPMMU_CTX_MAX 8U
>  
>  struct ipmmu_features {
>  	bool use_ns_alias_offset;
> @@ -1060,8 +1060,7 @@ static int ipmmu_probe(struct platform_device *pdev)
>  	if (mmu->features->use_ns_alias_offset)
>  		mmu->base += IM_NS_ALIAS_OFFSET;
>  
> -	mmu->num_ctx = min_t(unsigned int, IPMMU_CTX_MAX,
> -			     mmu->features->number_of_contexts);
> +	mmu->num_ctx = min(IPMMU_CTX_MAX, mmu->features->number_of_contexts);
>  
>  	irq = platform_get_irq(pdev, 0);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wed, Feb 20, 2019 at 04:05:30PM +0100, Geert Uytterhoeven wrote:
> ipmmu_domain_init_context() takes care of (1) initializing the software
> domain, and (2) initializing the hardware context for the domain.
> 
> Extract the code to initialize the hardware context into a new subroutine
> ipmmu_context_init(), to prepare for later reuse.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/iommu/ipmmu-vmsa.c | 91 ++++++++++++++++++++------------------
>  1 file changed, 48 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 0a21e734466eb1bd..92a766dd8b459f0c 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -404,52 +404,10 @@ static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
>  	spin_unlock_irqrestore(&mmu->lock, flags);
>  }
>  
> -static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> +static void ipmmu_context_init(struct ipmmu_vmsa_domain *domain)

ipmmu_context_init() vs. ipmmmu_domain_init_context() is confusing. You
could call this one ipmmu_domain_setup_context() maybe ?

>  {
>  	u64 ttbr;
>  	u32 tmp;
> -	int ret;
> -
> -	/*
> -	 * Allocate the page table operations.
> -	 *
> -	 * VMSA states in section B3.6.3 "Control of Secure or Non-secure memory
> -	 * access, Long-descriptor format" that the NStable bit being set in a
> -	 * table descriptor will result in the NStable and NS bits of all child
> -	 * entries being ignored and considered as being set. The IPMMU seems
> -	 * not to comply with this, as it generates a secure access page fault
> -	 * if any of the NStable and NS bits isn't set when running in
> -	 * non-secure mode.
> -	 */
> -	domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
> -	domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
> -	domain->cfg.ias = 32;
> -	domain->cfg.oas = 40;
> -	domain->cfg.tlb = &ipmmu_gather_ops;
> -	domain->io_domain.geometry.aperture_end = DMA_BIT_MASK(32);
> -	domain->io_domain.geometry.force_aperture = true;
> -	/*
> -	 * TODO: Add support for coherent walk through CCI with DVM and remove
> -	 * cache handling. For now, delegate it to the io-pgtable code.
> -	 */
> -	domain->cfg.iommu_dev = domain->mmu->root->dev;
> -
> -	/*
> -	 * Find an unused context.
> -	 */
> -	ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
> -	if (ret < 0)
> -		return ret;
> -
> -	domain->context_id = ret;
> -
> -	domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
> -					   domain);
> -	if (!domain->iop) {
> -		ipmmu_domain_free_context(domain->mmu->root,
> -					  domain->context_id);
> -		return -EINVAL;
> -	}
>  
>  	/* TTBR0 */
>  	ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> @@ -495,7 +453,54 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>  	 */
>  	ipmmu_ctx_write_all(domain, IMCTR,
>  			    IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
> +}
> +
> +static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> +{
> +	int ret;
> +
> +	/*
> +	 * Allocate the page table operations.
> +	 *
> +	 * VMSA states in section B3.6.3 "Control of Secure or Non-secure memory
> +	 * access, Long-descriptor format" that the NStable bit being set in a
> +	 * table descriptor will result in the NStable and NS bits of all child
> +	 * entries being ignored and considered as being set. The IPMMU seems
> +	 * not to comply with this, as it generates a secure access page fault
> +	 * if any of the NStable and NS bits isn't set when running in
> +	 * non-secure mode.
> +	 */
> +	domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
> +	domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
> +	domain->cfg.ias = 32;
> +	domain->cfg.oas = 40;
> +	domain->cfg.tlb = &ipmmu_gather_ops;
> +	domain->io_domain.geometry.aperture_end = DMA_BIT_MASK(32);
> +	domain->io_domain.geometry.force_aperture = true;
> +	/*
> +	 * TODO: Add support for coherent walk through CCI with DVM and remove
> +	 * cache handling. For now, delegate it to the io-pgtable code.
> +	 */
> +	domain->cfg.iommu_dev = domain->mmu->root->dev;
> +
> +	/*
> +	 * Find an unused context.
> +	 */
> +	ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
> +	if (ret < 0)
> +		return ret;
> +
> +	domain->context_id = ret;
> +
> +	domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
> +					   domain);
> +	if (!domain->iop) {
> +		ipmmu_domain_free_context(domain->mmu->root,
> +					  domain->context_id);
> +		return -EINVAL;
> +	}
>  
> +	ipmmu_context_init(domain);
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/7] iommu/ipmmu-vmsa: Prepare to handle 40-bit error addresses

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Feb 20, 2019 at 4:31 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Feb 20, 2019 at 04:05:27PM +0100, Geert Uytterhoeven wrote:
> > On R-Car Gen3, the faulting virtual address is a 40-bit address, and
> > comprised of two registers.  Read the upper address part, and combine
> > both parts, when running on a 64-bit system.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Apart from this, the driver doesn't support 40-bit IOVA addresses yet.
> > ---
> >  drivers/iommu/ipmmu-vmsa.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> > index ac70cb967ff821c6..4d07c26c97848b65 100644
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -186,7 +186,9 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev)
> >  #define IMMAIR_ATTR_IDX_WBRWA                1
> >  #define IMMAIR_ATTR_IDX_DEV          2
> >
> > -#define IMEAR                                0x0030
> > +#define IMEAR                                0x0030  /* R-Car Gen2 */
> > +#define IMELAR                               IMEAR   /* R-Car Gen3 */
>
> I would have defined that as a single macro.

Fair enough.

> > +#define IMEUAR                               0x0034  /* R-Car Gen3 */
> >
> >  #define IMPCTR                               0x0200
> >  #define IMPSTR                               0x0208
> > @@ -521,14 +523,16 @@ static irqreturn_t ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
> >  {
> >       const u32 err_mask = IMSTR_MHIT | IMSTR_ABORT | IMSTR_PF | IMSTR_TF;
> >       struct ipmmu_vmsa_device *mmu = domain->mmu;
> > +     unsigned long iova;
>
> Isn't there a more appropriate type, such as dma_addr_t possibly ?

Good question! Thanks for opening the can of worms ;-)

I used "unsigned long", as that's what the "iova" parameter of
report_iommu_fault() (called in this function) takes.

However, dma_addr_t would probably be a better choice.

Note that most iommu_ops methods take unsigned long, except for
.iova_to_phys(), which takes dma_addr_t.
Also, all io_pgtable_ops methods take unsigned long, including
.iova_to_phys().

The latter is interesting, as several IOMMU drivers forward the iova
received in their iommu_ops.iova_to_phys() methods to
io_pgtable_ops.iova_to_phys(), thus truncating the value on e.g. arm32
with LPAE.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> IPMMU state is lost.  Hence after s2ram, devices wired behind an IPMMU,
> and configured to use it, will see their DMA operations hang.
> 
> To fix this, restore all IPMMU contexts, and re-enable all active
> micro-TLBs during system resume.
> 
> To avoid overhead on platforms not needing it, the resume code has a
> build time dependency on sleep and PSCI support, and a runtime
> dependency on PSCI.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch takes a different approach than the BSP, which implements a
> bulk save/restore of all registers during system suspend/resume.

I like this approach better too.

>  drivers/iommu/ipmmu-vmsa.c | 52 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 92a766dd8b459f0c..5d22139914e8f033 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -22,6 +22,7 @@
>  #include <linux/of_iommu.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/psci.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
>  #include <linux/sys_soc.h>
> @@ -36,7 +37,10 @@
>  #define arm_iommu_detach_device(...)	do {} while (0)
>  #endif
>  
> -#define IPMMU_CTX_MAX 8U
> +#define IPMMU_CTX_MAX		8U
> +#define IPMMU_CTX_INVALID	-1
> +
> +#define IPMMU_UTLB_MAX		48U
>  
>  struct ipmmu_features {
>  	bool use_ns_alias_offset;
> @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
>  	spinlock_t lock;			/* Protects ctx and domains[] */
>  	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>  	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> +	s8 utlb_ctx[IPMMU_UTLB_MAX];

How about making this a bitmask instead to save memory ? I would also
rename it as utlb_ctx doesn't really carry the meaning of the field,
whose purpose is to store whether the µTLB is enabled or disabled.

>  
>  	struct iommu_group *group;
>  	struct dma_iommu_mapping *mapping;
> @@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
>  	ipmmu_write(mmu, IMUCTR(utlb),
>  		    IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
>  		    IMUCTR_MMUEN);
> +	mmu->utlb_ctx[utlb] = domain->context_id;
>  }
>  
>  /*
> @@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
>  	struct ipmmu_vmsa_device *mmu = domain->mmu;
>  
>  	ipmmu_write(mmu, IMUCTR(utlb), 0);
> +	mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
>  }
>  
>  static void ipmmu_tlb_flush_all(void *cookie)
> @@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
>  	spin_lock_init(&mmu->lock);
>  	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
>  	mmu->features = of_device_get_match_data(&pdev->dev);
> +	memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
>  	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
>  
>  	/* Map I/O memory and request IRQ. */
> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> +static int ipmmu_resume_noirq(struct device *dev)
> +{
> +	struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> +	unsigned int i;
> +
> +	/* This is the best we can do to check for the presence of PSCI */
> +	if (!psci_ops.cpu_suspend)
> +		return 0;

PSCI suspend disabling power to the SoC completely may be a common
behaviour on our development boards, but isn't mandated by the PSCI
specification if I'm not mistaken. Is there a way to instead detect that
power has been lost, perhaps by checking whether a register has been
reset to its default value ?

> +
> +	/* Reset root MMU and restore contexts */

I think the rest of the code adds a period at the end of sentences in
comments.

> +	if (ipmmu_is_root(mmu)) {
> +		ipmmu_device_reset(mmu);
> +
> +		for (i = 0; i < mmu->num_ctx; i++) {
> +			if (!mmu->domains[i])
> +				continue;
> +
> +			ipmmu_context_init(mmu->domains[i]);
> +		}
> +	}
> +
> +	/* Re-enable active micro-TLBs */
> +	for (i = 0; i < mmu->features->num_utlbs; i++) {
> +		if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> +			continue;
> +
> +		ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops ipmmu_pm  = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> +};
> +#define DEV_PM_OPS	&ipmmu_pm
> +#else
> +#define DEV_PM_OPS	NULL
> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> +
>  static struct platform_driver ipmmu_driver = {
>  	.driver = {
>  		.name = "ipmmu-vmsa",
>  		.of_match_table = of_match_ptr(ipmmu_of_ids),
> +		.pm = DEV_PM_OPS,

I would have used conditional compilation here instead of using a
DEV_PM_OPS macro, as I think the macro decreases readability (and also
given how its generic name could later conflict with something else).

>  	},
>  	.probe = ipmmu_probe,
>  	.remove	= ipmmu_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] iommu/ipmmu-vmsa: Move num_utlbs to SoC-specific features

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wed, Feb 20, 2019 at 04:05:29PM +0100, Geert Uytterhoeven wrote:
> The maximum number of micro-TLBs per IPMMU instance is not fixed, but
> depends on the SoC type.  Hence move it from struct ipmmu_vmsa_device to
> struct ipmmu_features, and set up the correct value for both R-Car Gen2
> and Gen3 SoCs.
> 
> Note that currently no code uses this value.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/iommu/ipmmu-vmsa.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 4e3134a9a52f8d87..0a21e734466eb1bd 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -42,6 +42,7 @@ struct ipmmu_features {
>  	bool use_ns_alias_offset;
>  	bool has_cache_leaf_nodes;
>  	unsigned int number_of_contexts;
> +	unsigned int num_utlbs;
>  	bool setup_imbuscr;
>  	bool twobit_imttbcr_sl0;
>  	bool reserved_context;
> @@ -53,7 +54,6 @@ struct ipmmu_vmsa_device {
>  	struct iommu_device iommu;
>  	struct ipmmu_vmsa_device *root;
>  	const struct ipmmu_features *features;
> -	unsigned int num_utlbs;
>  	unsigned int num_ctx;
>  	spinlock_t lock;			/* Protects ctx and domains[] */
>  	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> @@ -972,6 +972,7 @@ static const struct ipmmu_features ipmmu_features_default = {
>  	.use_ns_alias_offset = true,
>  	.has_cache_leaf_nodes = false,
>  	.number_of_contexts = 1, /* software only tested with one context */
> +	.num_utlbs = 32,
>  	.setup_imbuscr = true,
>  	.twobit_imttbcr_sl0 = false,
>  	.reserved_context = false,
> @@ -981,6 +982,7 @@ static const struct ipmmu_features ipmmu_features_rcar_gen3 = {
>  	.use_ns_alias_offset = false,
>  	.has_cache_leaf_nodes = true,
>  	.number_of_contexts = 8,
> +	.num_utlbs = 48,
>  	.setup_imbuscr = false,
>  	.twobit_imttbcr_sl0 = true,
>  	.reserved_context = true,
> @@ -1033,7 +1035,6 @@ static int ipmmu_probe(struct platform_device *pdev)
>  	}
>  
>  	mmu->dev = &pdev->dev;
> -	mmu->num_utlbs = 48;
>  	spin_lock_init(&mmu->lock);
>  	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
>  	mmu->features = of_device_get_match_data(&pdev->dev);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] iommu/ipmmu-vmsa: Extract hardware context initialization

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Feb 20, 2019 at 4:35 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Feb 20, 2019 at 04:05:30PM +0100, Geert Uytterhoeven wrote:
> > ipmmu_domain_init_context() takes care of (1) initializing the software
> > domain, and (2) initializing the hardware context for the domain.
> >
> > Extract the code to initialize the hardware context into a new subroutine
> > ipmmu_context_init(), to prepare for later reuse.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  drivers/iommu/ipmmu-vmsa.c | 91 ++++++++++++++++++++------------------
> >  1 file changed, 48 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> > index 0a21e734466eb1bd..92a766dd8b459f0c 100644
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -404,52 +404,10 @@ static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> >       spin_unlock_irqrestore(&mmu->lock, flags);
> >  }
> >
> > -static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
> > +static void ipmmu_context_init(struct ipmmu_vmsa_domain *domain)
>
> ipmmu_context_init() vs. ipmmmu_domain_init_context() is confusing. You
> could call this one ipmmu_domain_setup_context() maybe ?

Thanks, that name was actually on my shortlist, and may make most sense.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> > During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> > IPMMU state is lost.  Hence after s2ram, devices wired behind an IPMMU,
> > and configured to use it, will see their DMA operations hang.
> >
> > To fix this, restore all IPMMU contexts, and re-enable all active
> > micro-TLBs during system resume.
> >
> > To avoid overhead on platforms not needing it, the resume code has a
> > build time dependency on sleep and PSCI support, and a runtime
> > dependency on PSCI.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > This patch takes a different approach than the BSP, which implements a
> > bulk save/restore of all registers during system suspend/resume.
>
> I like this approach better too.

Thanks ;-)

> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c

> > @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> >       spinlock_t lock;                        /* Protects ctx and domains[] */
> >       DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> >       struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> > +     s8 utlb_ctx[IPMMU_UTLB_MAX];
>
> How about making this a bitmask instead to save memory ? I would also
> rename it as utlb_ctx doesn't really carry the meaning of the field,
> whose purpose is to store whether the µTLB is enabled or disabled.

This field isn't just a binary flag, but stores the context used for the
uTLB, so we can map from micro-TLB to context.
Given there can be 8 contexts, plus the need to indicate unused contexts,
that means 4 bits/micro-TLB.  So the overhead is just 24 bytes per IPMMU
instance.

I considered allocating the array dynamically (by having s8 utlb_ctx[]
at the end of the structure), but didn't go that route, as the domains[]
array already uses more memory.

> > @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> > +static int ipmmu_resume_noirq(struct device *dev)
> > +{
> > +     struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> > +     unsigned int i;
> > +
> > +     /* This is the best we can do to check for the presence of PSCI */
> > +     if (!psci_ops.cpu_suspend)
> > +             return 0;
>
> PSCI suspend disabling power to the SoC completely may be a common
> behaviour on our development boards, but isn't mandated by the PSCI
> specification if I'm not mistaken. Is there a way to instead detect that
> power has been lost, perhaps by checking whether a register has been
> reset to its default value ?

The approach here is the same as in the clk and pinctrl drivers.

I think we could check if the IMCTR registers for allocated domains in root
IPMMUs are non-zero.  But that's about as expensive as doing the full
restore, I think.  And it may have to be done for each and every IPMMU
instance, or do you trust caching for this?

> > +
> > +     /* Reset root MMU and restore contexts */
>
> I think the rest of the code adds a period at the end of sentences in
> comments.

The balance seems to be just under 50% ;-)

> > +     if (ipmmu_is_root(mmu)) {
> > +             ipmmu_device_reset(mmu);
> > +
> > +             for (i = 0; i < mmu->num_ctx; i++) {
> > +                     if (!mmu->domains[i])
> > +                             continue;
> > +
> > +                     ipmmu_context_init(mmu->domains[i]);
> > +             }
> > +     }
> > +
> > +     /* Re-enable active micro-TLBs */
> > +     for (i = 0; i < mmu->features->num_utlbs; i++) {
> > +             if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> > +                     continue;
> > +
> > +             ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dev_pm_ops ipmmu_pm  = {
> > +     SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> > +};
> > +#define DEV_PM_OPS   &ipmmu_pm
> > +#else
> > +#define DEV_PM_OPS   NULL
> > +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> > +
> >  static struct platform_driver ipmmu_driver = {
> >       .driver = {
> >               .name = "ipmmu-vmsa",
> >               .of_match_table = of_match_ptr(ipmmu_of_ids),
> > +             .pm = DEV_PM_OPS,
>
> I would have used conditional compilation here instead of using a
> DEV_PM_OPS macro, as I think the macro decreases readability (and also
> given how its generic name could later conflict with something else).

You mean

    #ifdef ...
    .pm = &ipmmu_pm,
    #endif

and marking ipmmu_pm __maybe_unused__?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support

[Laurent Pinchart]

Hi Geert,

On Wed, Feb 20, 2019 at 05:05:49PM +0100, Geert Uytterhoeven wrote:
> On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart wrote:
> > On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> >> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> >> IPMMU state is lost.  Hence after s2ram, devices wired behind an IPMMU,
> >> and configured to use it, will see their DMA operations hang.
> >>
> >> To fix this, restore all IPMMU contexts, and re-enable all active
> >> micro-TLBs during system resume.
> >>
> >> To avoid overhead on platforms not needing it, the resume code has a
> >> build time dependency on sleep and PSCI support, and a runtime
> >> dependency on PSCI.
> >>
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> This patch takes a different approach than the BSP, which implements a
> >> bulk save/restore of all registers during system suspend/resume.
> >
> > I like this approach better too.
> 
> Thanks ;-)
> 
> >> --- a/drivers/iommu/ipmmu-vmsa.c
> >> +++ b/drivers/iommu/ipmmu-vmsa.c
> 
> >> @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
> >>       spinlock_t lock;                        /* Protects ctx and domains[] */
> >>       DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> >>       struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> >> +     s8 utlb_ctx[IPMMU_UTLB_MAX];
> >
> > How about making this a bitmask instead to save memory ? I would also
> > rename it as utlb_ctx doesn't really carry the meaning of the field,
> > whose purpose is to store whether the µTLB is enabled or disabled.
> 
> This field isn't just a binary flag, but stores the context used for the
> uTLB, so we can map from micro-TLB to context.
> Given there can be 8 contexts, plus the need to indicate unused contexts,
> that means 4 bits/micro-TLB.  So the overhead is just 24 bytes per IPMMU
> instance.

My bad, I've overlooked that.

> I considered allocating the array dynamically (by having s8 utlb_ctx[]
> at the end of the structure), but didn't go that route, as the domains[]
> array already uses more memory.
> 
> >> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> >>       return 0;
> >>  }
> >>
> >> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> >> +static int ipmmu_resume_noirq(struct device *dev)
> >> +{
> >> +     struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> >> +     unsigned int i;
> >> +
> >> +     /* This is the best we can do to check for the presence of PSCI */
> >> +     if (!psci_ops.cpu_suspend)
> >> +             return 0;
> >
> > PSCI suspend disabling power to the SoC completely may be a common
> > behaviour on our development boards, but isn't mandated by the PSCI
> > specification if I'm not mistaken. Is there a way to instead detect that
> > power has been lost, perhaps by checking whether a register has been
> > reset to its default value ?
> 
> The approach here is the same as in the clk and pinctrl drivers.
> 
> I think we could check if the IMCTR registers for allocated domains in root
> IPMMUs are non-zero.  But that's about as expensive as doing the full
> restore, I think.

Would reading just one register be more expensive that full
reconfiguration ? Or is there no single register that could serve this
purpose ?

> And it may have to be done for each and every IPMMU instance, or do
> you trust caching for this?

If we can find a single register I think that reading it for every IPMMU
instance wouldn't be an issue.

> >> +
> >> +     /* Reset root MMU and restore contexts */
> >
> > I think the rest of the code adds a period at the end of sentences in
> > comments.
> 
> The balance seems to be just under 50% ;-)
> 
> >> +     if (ipmmu_is_root(mmu)) {
> >> +             ipmmu_device_reset(mmu);
> >> +
> >> +             for (i = 0; i < mmu->num_ctx; i++) {
> >> +                     if (!mmu->domains[i])
> >> +                             continue;
> >> +
> >> +                     ipmmu_context_init(mmu->domains[i]);
> >> +             }
> >> +     }
> >> +
> >> +     /* Re-enable active micro-TLBs */
> >> +     for (i = 0; i < mmu->features->num_utlbs; i++) {
> >> +             if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> >> +                     continue;
> >> +
> >> +             ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static const struct dev_pm_ops ipmmu_pm  = {
> >> +     SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> >> +};
> >> +#define DEV_PM_OPS   &ipmmu_pm
> >> +#else
> >> +#define DEV_PM_OPS   NULL
> >> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> >> +
> >>  static struct platform_driver ipmmu_driver = {
> >>       .driver = {
> >>               .name = "ipmmu-vmsa",
> >>               .of_match_table = of_match_ptr(ipmmu_of_ids),
> >> +             .pm = DEV_PM_OPS,
> >
> > I would have used conditional compilation here instead of using a
> > DEV_PM_OPS macro, as I think the macro decreases readability (and also
> > given how its generic name could later conflict with something else).
> 
> You mean
> 
>     #ifdef ...
>     .pm = &ipmmu_pm,
>     #endif
> 
> and marking ipmmu_pm __maybe_unused__?

Yes. Up to you.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Feb 20, 2019 at 5:11 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Feb 20, 2019 at 05:05:49PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Feb 20, 2019 at 4:42 PM Laurent Pinchart wrote:
> > > On Wed, Feb 20, 2019 at 04:05:31PM +0100, Geert Uytterhoeven wrote:
> > >> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> > >> IPMMU state is lost.  Hence after s2ram, devices wired behind an IPMMU,
> > >> and configured to use it, will see their DMA operations hang.
> > >>
> > >> To fix this, restore all IPMMU contexts, and re-enable all active
> > >> micro-TLBs during system resume.
> > >>
> > >> To avoid overhead on platforms not needing it, the resume code has a
> > >> build time dependency on sleep and PSCI support, and a runtime
> > >> dependency on PSCI.

> > >> --- a/drivers/iommu/ipmmu-vmsa.c
> > >> +++ b/drivers/iommu/ipmmu-vmsa.c

> > >> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
> > >>       return 0;
> > >>  }
> > >>
> > >> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> > >> +static int ipmmu_resume_noirq(struct device *dev)
> > >> +{
> > >> +     struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> > >> +     unsigned int i;
> > >> +
> > >> +     /* This is the best we can do to check for the presence of PSCI */
> > >> +     if (!psci_ops.cpu_suspend)
> > >> +             return 0;
> > >
> > > PSCI suspend disabling power to the SoC completely may be a common
> > > behaviour on our development boards, but isn't mandated by the PSCI
> > > specification if I'm not mistaken. Is there a way to instead detect that
> > > power has been lost, perhaps by checking whether a register has been
> > > reset to its default value ?
> >
> > The approach here is the same as in the clk and pinctrl drivers.
> >
> > I think we could check if the IMCTR registers for allocated domains in root
> > IPMMUs are non-zero.  But that's about as expensive as doing the full
> > restore, I think.
>
> Would reading just one register be more expensive that full
> reconfiguration ? Or is there no single register that could serve this
> purpose ?
>
> > And it may have to be done for each and every IPMMU instance, or do
> > you trust caching for this?
>
> If we can find a single register I think that reading it for every IPMMU
> instance wouldn't be an issue.

Upon more thought, probably it can be done by reading the IMCTR
for the first non-zero domain. Will look into it...

> > >> +static const struct dev_pm_ops ipmmu_pm  = {
> > >> +     SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> > >> +};
> > >> +#define DEV_PM_OPS   &ipmmu_pm
> > >> +#else
> > >> +#define DEV_PM_OPS   NULL
> > >> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> > >> +
> > >>  static struct platform_driver ipmmu_driver = {
> > >>       .driver = {
> > >>               .name = "ipmmu-vmsa",
> > >>               .of_match_table = of_match_ptr(ipmmu_of_ids),
> > >> +             .pm = DEV_PM_OPS,
> > >
> > > I would have used conditional compilation here instead of using a
> > > DEV_PM_OPS macro, as I think the macro decreases readability (and also
> > > given how its generic name could later conflict with something else).
> >
> > You mean
> >
> >     #ifdef ...
> >     .pm = &ipmmu_pm,
> >     #endif
> >
> > and marking ipmmu_pm __maybe_unused__?
>
> Yes. Up to you.

I'm not such a big fan of __maybe_unused. It's easy to add, and too
easy to forget to remove it when it's no longer needed (casts have the
same problem).

Usually people just annotate the actual suspend/resume methods with
__maybe_unused, which still leaves the (large) struct dev_pm_ops in
memory.

So I started preferring the DEV_PM_OPS approach...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 7/7] iommu/ipmmu-vmsa: Add suspend/resume support

[Robin Murphy]

Hi Geert,

On 20/02/2019 15:05, Geert Uytterhoeven wrote:
> During PSCI system suspend, R-Car Gen3 SoCs are powered down, and all
> IPMMU state is lost.  Hence after s2ram, devices wired behind an IPMMU,
> and configured to use it, will see their DMA operations hang.
> 
> To fix this, restore all IPMMU contexts, and re-enable all active
> micro-TLBs during system resume.
> 
> To avoid overhead on platforms not needing it, the resume code has a
> build time dependency on sleep and PSCI support, and a runtime
> dependency on PSCI.

Frankly, that aspect looks horribly hacky. It's not overly reasonable 
for random device drivers to be poking at PSCI internals, and while it 
might happen to correlate on some known set of current SoCs with current 
firmware, in general it's really too fragile to be accurate:

- Firmware is free to implement suspend callbacks in a way which doesn't 
actually lose power.
- Support for CPU_SUSPEND does not imply that SYSTEM_SUSPEND is even 
implemented, let alone how it might behave.
- The dev_pm_ops callbacks can also be used for hibernate, wherein it 
doesn't really matter what the firmware may or may not do if the user 
has pulled the plug and resumed us a week later.

Furthermore, I think any attempt to detect whether you need to resume or 
not is inherently fraught with danger - from testing the arm-smmu 
runtime PM ops, I've seen register state take a surprisingly long time 
to decay in a switched-off power domain, to the point where unless you 
check every bit of every register you can't necessarily be certain that 
they're really all still valid, and by that point you're doing far more 
work than just unconditionally reinitialising the whole state anyway. 
Upon resuming from hibernate, the state left by the cold-boot stage 
almost certainly *will* be valid, but it probably won't be the state you 
actually want.

Really, the whole idea smells of the premature optimisation demons 
anyway - is the resume overhead measurably significant?

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch takes a different approach than the BSP, which implements a
> bulk save/restore of all registers during system suspend/resume.
> 
>   drivers/iommu/ipmmu-vmsa.c | 52 +++++++++++++++++++++++++++++++++++++-
>   1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index 92a766dd8b459f0c..5d22139914e8f033 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -22,6 +22,7 @@
>   #include <linux/of_iommu.h>
>   #include <linux/of_platform.h>
>   #include <linux/platform_device.h>
> +#include <linux/psci.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/sys_soc.h>
> @@ -36,7 +37,10 @@
>   #define arm_iommu_detach_device(...)	do {} while (0)
>   #endif
>   
> -#define IPMMU_CTX_MAX 8U
> +#define IPMMU_CTX_MAX		8U
> +#define IPMMU_CTX_INVALID	-1
> +
> +#define IPMMU_UTLB_MAX		48U
>   
>   struct ipmmu_features {
>   	bool use_ns_alias_offset;
> @@ -58,6 +62,7 @@ struct ipmmu_vmsa_device {
>   	spinlock_t lock;			/* Protects ctx and domains[] */
>   	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>   	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> +	s8 utlb_ctx[IPMMU_UTLB_MAX];
>   
>   	struct iommu_group *group;
>   	struct dma_iommu_mapping *mapping;
> @@ -335,6 +340,7 @@ static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
>   	ipmmu_write(mmu, IMUCTR(utlb),
>   		    IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_FLUSH |
>   		    IMUCTR_MMUEN);
> +	mmu->utlb_ctx[utlb] = domain->context_id;
>   }
>   
>   /*
> @@ -346,6 +352,7 @@ static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
>   	struct ipmmu_vmsa_device *mmu = domain->mmu;
>   
>   	ipmmu_write(mmu, IMUCTR(utlb), 0);
> +	mmu->utlb_ctx[utlb] = IPMMU_CTX_INVALID;
>   }
>   
>   static void ipmmu_tlb_flush_all(void *cookie)
> @@ -1043,6 +1050,7 @@ static int ipmmu_probe(struct platform_device *pdev)
>   	spin_lock_init(&mmu->lock);
>   	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
>   	mmu->features = of_device_get_match_data(&pdev->dev);
> +	memset(mmu->utlb_ctx, IPMMU_CTX_INVALID, mmu->features->num_utlbs);
>   	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
>   
>   	/* Map I/O memory and request IRQ. */
> @@ -1158,10 +1166,52 @@ static int ipmmu_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW)
> +static int ipmmu_resume_noirq(struct device *dev)
> +{
> +	struct ipmmu_vmsa_device *mmu = dev_get_drvdata(dev);
> +	unsigned int i;
> +
> +	/* This is the best we can do to check for the presence of PSCI */
> +	if (!psci_ops.cpu_suspend)
> +		return 0;
> +
> +	/* Reset root MMU and restore contexts */
> +	if (ipmmu_is_root(mmu)) {
> +		ipmmu_device_reset(mmu);
> +
> +		for (i = 0; i < mmu->num_ctx; i++) {
> +			if (!mmu->domains[i])
> +				continue;
> +
> +			ipmmu_context_init(mmu->domains[i]);
> +		}

Unless the hardware has some programming sequence requirement, it looks 
like it could be a little more efficient to roll this logic into 
ipmmu_device_reset() itself such that no IMCR gets written twice, much 
like I did with arm_smmu_write_context_bank()/arm-smmu_device_reset().

Robin.

> +	}
> +
> +	/* Re-enable active micro-TLBs */
> +	for (i = 0; i < mmu->features->num_utlbs; i++) {
> +		if (mmu->utlb_ctx[i] == IPMMU_CTX_INVALID)
> +			continue;
> +
> +		ipmmu_utlb_enable(mmu->root->domains[mmu->utlb_ctx[i]], i);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops ipmmu_pm  = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, ipmmu_resume_noirq)
> +};
> +#define DEV_PM_OPS	&ipmmu_pm
> +#else
> +#define DEV_PM_OPS	NULL
> +#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
> +
>   static struct platform_driver ipmmu_driver = {
>   	.driver = {
>   		.name = "ipmmu-vmsa",
>   		.of_match_table = of_match_ptr(ipmmu_of_ids),
> +		.pm = DEV_PM_OPS,
>   	},
>   	.probe = ipmmu_probe,
>   	.remove	= ipmmu_remove,
>