[PATCH v2 0/7] iommu/exynos: Add basic support for SysMMU v7

[PATCH v2 0/7] iommu/exynos: Add basic support for SysMMU v7

[Sam Protsenko]

Add minimal viable support for SysMMU v7.x, which can be found in modern
Exynos chips (like Exynos850 or Google's GS101). SysMMU v7.x may
implement VM register set, and those registers should be initialized
properly if present. Usually 8 translation domains are supported via VM
registers (0..7), but only n=0 (default) is used for now.

Existing exynos-iommu driver only supports SysMMU versions up to v5. But
it's pretty much ready for basic usage with SysMMU v7, only small
changes have to be done. As SysMMU version is tested dynamically (by
reading the corresponding register), there is no need to introduce new
compatible string.

The only major change is that SysMMU v7 can have different register
layouts:
  - with Virtual Machine support
  - without Virtual Machine support

That can be checked by reading the capability registers. In the case if
SysMMU IP-core is VM-capable, the VM registers have to be used, and some
additional initialization is needed. That is the case on E850-96 board,
which non-secure SysMMU (v7.4) implements VM-capable register set.

Another required change to make SysMMU v7 functional (at least the one
that have VM registers), is to enable default VM instance. That should
be added to the code enabling MMU itself. Insights for that change were
taken by comparing the I/O dump (writel() / readl() operations) for the
vendor driver and this upstream driver.

The patch series was tested on E850-96 board. Because at the moment
there are no SysMMU users for that board, the testing was done using so
called "Emulated Translation" registers available on SysMMU v7. That
allows one to initiate the translation from CPU, by writing to those
registers, and then reading the corresponding TLB registers to find out
the translation result. The testing driver can be found in [1] tree.

Thanks to Marek, who did let me know it only takes a slight change of
registers to make this driver work with v7.

[1] https://github.com/joe-skb7/linux/tree/e850-96-mainline-iommu

Changes in v2:
  - Addressed all comments on review
  - Reworked commit messages correspondingly
  - Added new patch: "iommu/exynos: Handle failed registration properly"
  - Added new patch: "iommu/exynos: Add SysMMU v7 register sets"
  - Added new patch: "iommu/exynos: Reuse SysMMU constants for page size
    and order"

Sam Protsenko (7):
  iommu/exynos: Reuse SysMMU constants for page size and order
  iommu/exynos: Handle failed IOMMU device registration properly
  iommu/exynos: Set correct dma mask for SysMMU v5+
  iommu/exynos: Use lookup based approach to access registers
  iommu/exynos: Check if SysMMU v7 has VM registers
  iommu/exynos: Add SysMMU v7 register sets
  iommu/exynos: Enable default VM instance on SysMMU v7

 drivers/iommu/exynos-iommu.c | 219 ++++++++++++++++++++++++++---------
 1 file changed, 166 insertions(+), 53 deletions(-)

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/7] iommu/exynos: Reuse SysMMU constants for page size and order

[Sam Protsenko]

Using SZ_4K in context of SysMMU driver is better than using PAGE_SIZE,
as PAGE_SIZE might have different value on different platforms. Though
it would be even better to use more specific constants, already existing
in SysMMU driver. Make the code more strict by using SPAGE_ORDER and
SPAGE_SIZE constants.

It also makes sense, as __sysmmu_tlb_invalidate_entry() also uses
SPAGE_* constants for further calculations with num_inv param, so it's
logical that num_inv should be previously calculated using also SPAGE_*
values.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none) This patch is new and added in v2

 drivers/iommu/exynos-iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 79729892eb48..8f80aaa35092 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -340,7 +340,7 @@ static void __sysmmu_set_ptbase(struct sysmmu_drvdata *data, phys_addr_t pgd)
 	if (MMU_MAJ_VER(data->version) < 5)
 		writel(pgd, data->sfrbase + REG_PT_BASE_ADDR);
 	else
-		writel(pgd / SZ_4K, data->sfrbase + REG_V5_PT_BASE_PFN);
+		writel(pgd >> SPAGE_ORDER, data->sfrbase + REG_V5_PT_BASE_PFN);
 
 	__sysmmu_tlb_invalidate(data);
 }
@@ -550,7 +550,7 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 		 * 64KB page can be one of 16 consecutive sets.
 		 */
 		if (MMU_MAJ_VER(data->version) == 2)
-			num_inv = min_t(unsigned int, size / SZ_4K, 64);
+			num_inv = min_t(unsigned int, size / SPAGE_SIZE, 64);
 
 		if (sysmmu_block(data)) {
 			__sysmmu_tlb_invalidate_entry(data, iova, num_inv);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/7] iommu/exynos: Handle failed IOMMU device registration properly

[Sam Protsenko]

If iommu_device_register() fails in exynos_sysmmu_probe(), the previous
calls have to be cleaned up. In this case, the iommu_device_sysfs_add()
should be cleaned up, by calling its remove counterpart call.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none) This patch is new and added in v2

 drivers/iommu/exynos-iommu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 8f80aaa35092..c85db9dab851 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -629,7 +629,7 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 
 	ret = iommu_device_register(&data->iommu, &exynos_iommu_ops, dev);
 	if (ret)
-		return ret;
+		goto err_iommu_register;
 
 	platform_set_drvdata(pdev, data);
 
@@ -656,6 +656,10 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 	pm_runtime_enable(dev);
 
 	return 0;
+
+err_iommu_register:
+	iommu_device_sysfs_remove(&data->iommu);
+	return ret;
 }
 
 static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/7] iommu/exynos: Set correct dma mask for SysMMU v5+

[Sam Protsenko]

SysMMU v5+ supports 36 bit physical address space. Set corresponding DMA
mask to avoid falling back to SWTLBIO usage in dma_map_single() because
of failed dma_capable() check.

The original code for this fix was suggested by Marek.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Co-developed-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Changes in v2:
  - Handled failed dma_set_mask() call
  - Replaced "Originally-by" tag by "Co-developed-by" + SoB tags

 drivers/iommu/exynos-iommu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index c85db9dab851..494f7d7aa9c5 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -646,6 +646,14 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (MMU_MAJ_VER(data->version) >= 5) {
+		ret = dma_set_mask(dev, DMA_BIT_MASK(36));
+		if (ret) {
+			dev_err(dev, "Unable to set DMA mask: %d\n", ret);
+			goto err_dma_set_mask;
+		}
+	}
+
 	/*
 	 * use the first registered sysmmu device for performing
 	 * dma mapping operations on iommu page tables (cpu cache flush)
@@ -657,6 +665,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_dma_set_mask:
+	iommu_device_unregister(&data->iommu);
 err_iommu_register:
 	iommu_device_sysfs_remove(&data->iommu);
 	return ret;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/7] iommu/exynos: Use lookup based approach to access registers

[Sam Protsenko]

At the moment the driver supports SysMMU v1..v5 versions. SysMMU v5 has
different register layout than SysMMU v1..v3. Instead of checking the
version each time before reading/writing the registers, let's create
corresponding register table for each SysMMU version and set the needed
table on init, checking the SysMMU version one single time. This way is
faster and more elegant.

No functional change here, just a refactoring patch.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Reworked existing code (SysMMU v1..v5) to use this approach
  - Extracted v7 registers to the separate patches
  - Replaced MMU_REG() with corresponding SysMMU read/write functions
  - Improved the comment for 0x1 offsets triggering an unaligned access
    exception
  - Removed support for VMID number, as only VMID=0 (default) is used
    for now
  - Renamed register index names to reflect the old SysMMU version
    register names

 drivers/iommu/exynos-iommu.c | 141 ++++++++++++++++++++++-------------
 1 file changed, 90 insertions(+), 51 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 494f7d7aa9c5..0cb1ce10db51 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -136,9 +136,6 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
 #define CFG_FLPDCACHE	(1 << 20) /* System MMU 3.2+ only */
 
 /* common registers */
-#define REG_MMU_CTRL		0x000
-#define REG_MMU_CFG		0x004
-#define REG_MMU_STATUS		0x008
 #define REG_MMU_VERSION		0x034
 
 #define MMU_MAJ_VER(val)	((val) >> 7)
@@ -148,31 +145,57 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
 #define MAKE_MMU_VER(maj, min)	((((maj) & 0xF) << 7) | ((min) & 0x7F))
 
 /* v1.x - v3.x registers */
-#define REG_MMU_FLUSH		0x00C
-#define REG_MMU_FLUSH_ENTRY	0x010
-#define REG_PT_BASE_ADDR	0x014
-#define REG_INT_STATUS		0x018
-#define REG_INT_CLEAR		0x01C
-
 #define REG_PAGE_FAULT_ADDR	0x024
 #define REG_AW_FAULT_ADDR	0x028
 #define REG_AR_FAULT_ADDR	0x02C
 #define REG_DEFAULT_SLAVE_ADDR	0x030
 
 /* v5.x registers */
-#define REG_V5_PT_BASE_PFN	0x00C
-#define REG_V5_MMU_FLUSH_ALL	0x010
-#define REG_V5_MMU_FLUSH_ENTRY	0x014
-#define REG_V5_MMU_FLUSH_RANGE	0x018
-#define REG_V5_MMU_FLUSH_START	0x020
-#define REG_V5_MMU_FLUSH_END	0x024
-#define REG_V5_INT_STATUS	0x060
-#define REG_V5_INT_CLEAR	0x064
 #define REG_V5_FAULT_AR_VA	0x070
 #define REG_V5_FAULT_AW_VA	0x080
 
 #define has_sysmmu(dev)		(dev_iommu_priv_get(dev) != NULL)
 
+enum {
+	REG_SET_V1,
+	REG_SET_V5,
+	MAX_REG_SET
+};
+
+enum {
+	IDX_CTRL,
+	IDX_CFG,
+	IDX_STATUS,
+	IDX_PT_BASE,
+	IDX_FLUSH_ALL,
+	IDX_FLUSH_ENTRY,
+	IDX_FLUSH_RANGE,
+	IDX_FLUSH_START,
+	IDX_FLUSH_END,
+	IDX_INT_STATUS,
+	IDX_INT_CLEAR,
+	MAX_REG_IDX
+};
+
+/*
+ * Some SysMMU versions might not implement some registers from this set, thus
+ * those registers shouldn't be accessed. Set the offsets for those registers to
+ * 0x1 to trigger an unaligned access exception, which can help one to debug
+ * related issues.
+ */
+static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
+	/* SysMMU v1..v3 */
+	{
+		0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
+		0x18, 0x1c,
+	},
+	/* SysMMU v5 */
+	{
+		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
+		0x60, 0x64,
+	},
+};
+
 static struct device *dma_dev;
 static struct kmem_cache *lv2table_kmem_cache;
 static sysmmu_pte_t *zero_lv2_table;
@@ -274,6 +297,7 @@ struct sysmmu_drvdata {
 	unsigned int version;		/* our version */
 
 	struct iommu_device iommu;	/* IOMMU core handle */
+	const unsigned int *regs;	/* register set */
 };
 
 static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
@@ -281,20 +305,30 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
 	return container_of(dom, struct exynos_iommu_domain, domain);
 }
 
+static void sysmmu_write(struct sysmmu_drvdata *data, size_t idx, u32 val)
+{
+	writel(val, data->sfrbase + data->regs[idx]);
+}
+
+static u32 sysmmu_read(struct sysmmu_drvdata *data, size_t idx)
+{
+	return readl(data->sfrbase + data->regs[idx]);
+}
+
 static void sysmmu_unblock(struct sysmmu_drvdata *data)
 {
-	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
+	sysmmu_write(data, IDX_CTRL, CTRL_ENABLE);
 }
 
 static bool sysmmu_block(struct sysmmu_drvdata *data)
 {
 	int i = 120;
 
-	writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
-	while ((i > 0) && !(readl(data->sfrbase + REG_MMU_STATUS) & 1))
+	sysmmu_write(data, IDX_CTRL, CTRL_BLOCK);
+	while (i > 0 && !(sysmmu_read(data, IDX_STATUS) & 0x1))
 		--i;
 
-	if (!(readl(data->sfrbase + REG_MMU_STATUS) & 1)) {
+	if (!(sysmmu_read(data, IDX_STATUS) & 0x1)) {
 		sysmmu_unblock(data);
 		return false;
 	}
@@ -304,10 +338,7 @@ static bool sysmmu_block(struct sysmmu_drvdata *data)
 
 static void __sysmmu_tlb_invalidate(struct sysmmu_drvdata *data)
 {
-	if (MMU_MAJ_VER(data->version) < 5)
-		writel(0x1, data->sfrbase + REG_MMU_FLUSH);
-	else
-		writel(0x1, data->sfrbase + REG_V5_MMU_FLUSH_ALL);
+	sysmmu_write(data, IDX_FLUSH_ALL, 0x1);
 }
 
 static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
@@ -317,31 +348,33 @@ static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 
 	if (MMU_MAJ_VER(data->version) < 5) {
 		for (i = 0; i < num_inv; i++) {
-			writel((iova & SPAGE_MASK) | 1,
-				     data->sfrbase + REG_MMU_FLUSH_ENTRY);
+			sysmmu_write(data, IDX_FLUSH_ENTRY,
+				     (iova & SPAGE_MASK) | 0x1);
 			iova += SPAGE_SIZE;
 		}
 	} else {
 		if (num_inv == 1) {
-			writel((iova & SPAGE_MASK) | 1,
-				     data->sfrbase + REG_V5_MMU_FLUSH_ENTRY);
+			sysmmu_write(data, IDX_FLUSH_ENTRY,
+				     (iova & SPAGE_MASK) | 0x1);
 		} else {
-			writel((iova & SPAGE_MASK),
-				     data->sfrbase + REG_V5_MMU_FLUSH_START);
-			writel((iova & SPAGE_MASK) + (num_inv - 1) * SPAGE_SIZE,
-				     data->sfrbase + REG_V5_MMU_FLUSH_END);
-			writel(1, data->sfrbase + REG_V5_MMU_FLUSH_RANGE);
+			sysmmu_write(data, IDX_FLUSH_START, iova & SPAGE_MASK);
+			sysmmu_write(data, IDX_FLUSH_END, (iova & SPAGE_MASK) +
+				     (num_inv - 1) * SPAGE_SIZE);
+			sysmmu_write(data, IDX_FLUSH_RANGE, 0x1);
 		}
 	}
 }
 
 static void __sysmmu_set_ptbase(struct sysmmu_drvdata *data, phys_addr_t pgd)
 {
+	u32 pt_base;
+
 	if (MMU_MAJ_VER(data->version) < 5)
-		writel(pgd, data->sfrbase + REG_PT_BASE_ADDR);
+		pt_base = pgd;
 	else
-		writel(pgd >> SPAGE_ORDER, data->sfrbase + REG_V5_PT_BASE_PFN);
+		pt_base = pgd >> SPAGE_ORDER;
 
+	sysmmu_write(data, IDX_PT_BASE, pt_base);
 	__sysmmu_tlb_invalidate(data);
 }
 
@@ -365,8 +398,7 @@ static void __sysmmu_get_version(struct sysmmu_drvdata *data)
 {
 	u32 ver;
 
-	__sysmmu_enable_clocks(data);
-
+	/* Don't use sysmmu_read() here, as data->regs is not set yet */
 	ver = readl(data->sfrbase + REG_MMU_VERSION);
 
 	/* controllers on some SoCs don't report proper version */
@@ -377,6 +409,17 @@ static void __sysmmu_get_version(struct sysmmu_drvdata *data)
 
 	dev_dbg(data->sysmmu, "hardware version: %d.%d\n",
 		MMU_MAJ_VER(data->version), MMU_MIN_VER(data->version));
+}
+
+static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
+{
+	__sysmmu_enable_clocks(data);
+
+	__sysmmu_get_version(data);
+	if (MMU_MAJ_VER(data->version) < 5)
+		data->regs = sysmmu_regs[REG_SET_V1];
+	else
+		data->regs = sysmmu_regs[REG_SET_V5];
 
 	__sysmmu_disable_clocks(data);
 }
@@ -405,19 +448,14 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	const struct sysmmu_fault_info *finfo;
 	unsigned int i, n, itype;
 	sysmmu_iova_t fault_addr;
-	unsigned short reg_status, reg_clear;
 	int ret = -ENOSYS;
 
 	WARN_ON(!data->active);
 
 	if (MMU_MAJ_VER(data->version) < 5) {
-		reg_status = REG_INT_STATUS;
-		reg_clear = REG_INT_CLEAR;
 		finfo = sysmmu_faults;
 		n = ARRAY_SIZE(sysmmu_faults);
 	} else {
-		reg_status = REG_V5_INT_STATUS;
-		reg_clear = REG_V5_INT_CLEAR;
 		finfo = sysmmu_v5_faults;
 		n = ARRAY_SIZE(sysmmu_v5_faults);
 	}
@@ -426,7 +464,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 
 	clk_enable(data->clk_master);
 
-	itype = __ffs(readl(data->sfrbase + reg_status));
+	itype = __ffs(sysmmu_read(data, IDX_INT_STATUS));
 	for (i = 0; i < n; i++, finfo++)
 		if (finfo->bit == itype)
 			break;
@@ -443,7 +481,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	/* fault is not recovered by fault handler */
 	BUG_ON(ret != 0);
 
-	writel(1 << itype, data->sfrbase + reg_clear);
+	sysmmu_write(data, IDX_INT_CLEAR, 1 << itype);
 
 	sysmmu_unblock(data);
 
@@ -461,8 +499,8 @@ static void __sysmmu_disable(struct sysmmu_drvdata *data)
 	clk_enable(data->clk_master);
 
 	spin_lock_irqsave(&data->lock, flags);
-	writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL);
-	writel(0, data->sfrbase + REG_MMU_CFG);
+	sysmmu_write(data, IDX_CTRL, CTRL_DISABLE);
+	sysmmu_write(data, IDX_CFG, 0x0);
 	data->active = false;
 	spin_unlock_irqrestore(&data->lock, flags);
 
@@ -482,7 +520,7 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
 
 	cfg |= CFG_EAP; /* enable access protection bits check */
 
-	writel(cfg, data->sfrbase + REG_MMU_CFG);
+	sysmmu_write(data, IDX_CFG, cfg);
 }
 
 static void __sysmmu_enable(struct sysmmu_drvdata *data)
@@ -492,10 +530,10 @@ static void __sysmmu_enable(struct sysmmu_drvdata *data)
 	__sysmmu_enable_clocks(data);
 
 	spin_lock_irqsave(&data->lock, flags);
-	writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
+	sysmmu_write(data, IDX_CTRL, CTRL_BLOCK);
 	__sysmmu_init_config(data);
 	__sysmmu_set_ptbase(data, data->pgtable);
-	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
+	sysmmu_write(data, IDX_CTRL, CTRL_ENABLE);
 	data->active = true;
 	spin_unlock_irqrestore(&data->lock, flags);
 
@@ -622,6 +660,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 	data->sysmmu = dev;
 	spin_lock_init(&data->lock);
 
+	sysmmu_get_hw_info(data);
+
 	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
 				     dev_name(data->sysmmu));
 	if (ret)
@@ -633,7 +673,6 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
-	__sysmmu_get_version(data);
 	if (PG_ENT_SHIFT < 0) {
 		if (MMU_MAJ_VER(data->version) < 5) {
 			PG_ENT_SHIFT = SYSMMU_PG_ENT_SHIFT;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/7] iommu/exynos: Check if SysMMU v7 has VM registers

[Sam Protsenko]

SysMMU v7 can have Virtual Machine registers, which implement multiple
translation domains. The driver should know if it's true or not, as VM
registers shouldn't be accessed if not present. Read corresponding
capabilities register to obtain that info, and store it in driver data.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Removed the 'const' qualifier for local non-pointer variables

 drivers/iommu/exynos-iommu.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 0cb1ce10db51..48681189ccf8 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -135,6 +135,9 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
 #define CFG_SYSSEL	(1 << 22) /* System MMU 3.2 only */
 #define CFG_FLPDCACHE	(1 << 20) /* System MMU 3.2+ only */
 
+#define CAPA0_CAPA1_EXIST		BIT(11)
+#define CAPA1_VCR_ENABLED		BIT(14)
+
 /* common registers */
 #define REG_MMU_VERSION		0x034
 
@@ -154,6 +157,10 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
 #define REG_V5_FAULT_AR_VA	0x070
 #define REG_V5_FAULT_AW_VA	0x080
 
+/* v7.x registers */
+#define REG_V7_CAPA0		0x870
+#define REG_V7_CAPA1		0x874
+
 #define has_sysmmu(dev)		(dev_iommu_priv_get(dev) != NULL)
 
 enum {
@@ -298,6 +305,9 @@ struct sysmmu_drvdata {
 
 	struct iommu_device iommu;	/* IOMMU core handle */
 	const unsigned int *regs;	/* register set */
+
+	/* v7 fields */
+	bool has_vcr;			/* virtual machine control register */
 };
 
 static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
@@ -411,11 +421,27 @@ static void __sysmmu_get_version(struct sysmmu_drvdata *data)
 		MMU_MAJ_VER(data->version), MMU_MIN_VER(data->version));
 }
 
+static bool __sysmmu_has_capa1(struct sysmmu_drvdata *data)
+{
+	u32 capa0 = readl(data->sfrbase + REG_V7_CAPA0);
+
+	return capa0 & CAPA0_CAPA1_EXIST;
+}
+
+static void __sysmmu_get_vcr(struct sysmmu_drvdata *data)
+{
+	u32 capa1 = readl(data->sfrbase + REG_V7_CAPA1);
+
+	data->has_vcr = capa1 & CAPA1_VCR_ENABLED;
+}
+
 static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
 {
 	__sysmmu_enable_clocks(data);
 
 	__sysmmu_get_version(data);
+	if (MMU_MAJ_VER(data->version) >= 7 && __sysmmu_has_capa1(data))
+		__sysmmu_get_vcr(data);
 	if (MMU_MAJ_VER(data->version) < 5)
 		data->regs = sysmmu_regs[REG_SET_V1];
 	else
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/7] iommu/exynos: Add SysMMU v7 register sets

[Sam Protsenko]

SysMMU v7 might have different register layouts (VM capable or non-VM
capable). Check which layout is implemented in current SysMMU module and
prepare the corresponding register table for futher usage.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - (none) This patch is new and added in v2

 drivers/iommu/exynos-iommu.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 48681189ccf8..64bf3331064f 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -166,6 +166,8 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
 enum {
 	REG_SET_V1,
 	REG_SET_V5,
+	REG_SET_V7_NON_VM,
+	REG_SET_V7_VM,
 	MAX_REG_SET
 };
 
@@ -201,6 +203,16 @@ static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
 		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
 		0x60, 0x64,
 	},
+	/* SysMMU v7: Default register set (non-VM) */
+	{
+		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
+		0x60, 0x64,
+	},
+	/* SysMMU v7: VM capable register set */
+	{
+		0x00, 0x04, 0x08, 0x800c, 0x8010, 0x8014, 0x8018, 0x8020,
+		0x8024, 0x60, 0x64,
+	},
 };
 
 static struct device *dma_dev;
@@ -440,12 +452,18 @@ static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
 	__sysmmu_enable_clocks(data);
 
 	__sysmmu_get_version(data);
-	if (MMU_MAJ_VER(data->version) >= 7 && __sysmmu_has_capa1(data))
-		__sysmmu_get_vcr(data);
-	if (MMU_MAJ_VER(data->version) < 5)
+	if (MMU_MAJ_VER(data->version) < 5) {
 		data->regs = sysmmu_regs[REG_SET_V1];
-	else
+	} else if (MMU_MAJ_VER(data->version) < 7) {
 		data->regs = sysmmu_regs[REG_SET_V5];
+	} else {
+		if (__sysmmu_has_capa1(data))
+			__sysmmu_get_vcr(data);
+		if (data->has_vcr)
+			data->regs = sysmmu_regs[REG_SET_V7_VM];
+		else
+			data->regs = sysmmu_regs[REG_SET_V7_NON_VM];
+	}
 
 	__sysmmu_disable_clocks(data);
 }
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/7] iommu/exynos: Enable default VM instance on SysMMU v7

[Sam Protsenko]

In order to enable SysMMU v7 with VM register layout, at least the
default VM instance (n=0) must be enabled, in addition to enabling the
SysMMU itself. To do so, add corresponding write to MMU_CTRL_VM[0]
register, before writing to MMU_CTRL register.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Extracted VM enabling code to the separate function
  - Used new SysMMU read/write functions to access the registers

 drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 64bf3331064f..2b333e137f57 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -135,6 +135,8 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
 #define CFG_SYSSEL	(1 << 22) /* System MMU 3.2 only */
 #define CFG_FLPDCACHE	(1 << 20) /* System MMU 3.2+ only */
 
+#define CTRL_VM_ENABLE			BIT(0)
+#define CTRL_VM_FAULT_MODE_STALL	BIT(3)
 #define CAPA0_CAPA1_EXIST		BIT(11)
 #define CAPA1_VCR_ENABLED		BIT(14)
 
@@ -183,6 +185,7 @@ enum {
 	IDX_FLUSH_END,
 	IDX_INT_STATUS,
 	IDX_INT_CLEAR,
+	IDX_CTRL_VM,
 	MAX_REG_IDX
 };
 
@@ -196,22 +199,22 @@ static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
 	/* SysMMU v1..v3 */
 	{
 		0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
-		0x18, 0x1c,
+		0x18, 0x1c, 0x1,
 	},
 	/* SysMMU v5 */
 	{
 		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
-		0x60, 0x64,
+		0x60, 0x64, 0x1,
 	},
 	/* SysMMU v7: Default register set (non-VM) */
 	{
 		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
-		0x60, 0x64,
+		0x60, 0x64, 0x1,
 	},
 	/* SysMMU v7: VM capable register set */
 	{
 		0x00, 0x04, 0x08, 0x800c, 0x8010, 0x8014, 0x8018, 0x8020,
-		0x8024, 0x60, 0x64,
+		0x8024, 0x60, 0x64, 0x8000,
 	},
 };
 
@@ -567,6 +570,18 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
 	sysmmu_write(data, IDX_CFG, cfg);
 }
 
+static void __sysmmu_enable_vid(struct sysmmu_drvdata *data)
+{
+	u32 ctrl;
+
+	if (MMU_MAJ_VER(data->version) < 7 || !data->has_vcr)
+		return;
+
+	ctrl = sysmmu_read(data, IDX_CTRL_VM);
+	ctrl |= CTRL_VM_ENABLE | CTRL_VM_FAULT_MODE_STALL;
+	sysmmu_write(data, IDX_CTRL_VM, ctrl);
+}
+
 static void __sysmmu_enable(struct sysmmu_drvdata *data)
 {
 	unsigned long flags;
@@ -577,6 +592,7 @@ static void __sysmmu_enable(struct sysmmu_drvdata *data)
 	sysmmu_write(data, IDX_CTRL, CTRL_BLOCK);
 	__sysmmu_init_config(data);
 	__sysmmu_set_ptbase(data, data->pgtable);
+	__sysmmu_enable_vid(data);
 	sysmmu_write(data, IDX_CTRL, CTRL_ENABLE);
 	data->active = true;
 	spin_unlock_irqrestore(&data->lock, flags);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] iommu/exynos: Reuse SysMMU constants for page size and order

[Marek Szyprowski]

On 11.07.2022 01:05, Sam Protsenko wrote:
> Using SZ_4K in context of SysMMU driver is better than using PAGE_SIZE,
> as PAGE_SIZE might have different value on different platforms. Though
> it would be even better to use more specific constants, already existing
> in SysMMU driver. Make the code more strict by using SPAGE_ORDER and
> SPAGE_SIZE constants.
>
> It also makes sense, as __sysmmu_tlb_invalidate_entry() also uses
> SPAGE_* constants for further calculations with num_inv param, so it's
> logical that num_inv should be previously calculated using also SPAGE_*
> values.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> Changes in v2:
>    - (none) This patch is new and added in v2
>
>   drivers/iommu/exynos-iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 79729892eb48..8f80aaa35092 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -340,7 +340,7 @@ static void __sysmmu_set_ptbase(struct sysmmu_drvdata *data, phys_addr_t pgd)
>   	if (MMU_MAJ_VER(data->version) < 5)
>   		writel(pgd, data->sfrbase + REG_PT_BASE_ADDR);
>   	else
> -		writel(pgd / SZ_4K, data->sfrbase + REG_V5_PT_BASE_PFN);
> +		writel(pgd >> SPAGE_ORDER, data->sfrbase + REG_V5_PT_BASE_PFN);
>   
>   	__sysmmu_tlb_invalidate(data);
>   }
> @@ -550,7 +550,7 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
>   		 * 64KB page can be one of 16 consecutive sets.
>   		 */
>   		if (MMU_MAJ_VER(data->version) == 2)
> -			num_inv = min_t(unsigned int, size / SZ_4K, 64);
> +			num_inv = min_t(unsigned int, size / SPAGE_SIZE, 64);
>   
>   		if (sysmmu_block(data)) {
>   			__sysmmu_tlb_invalidate_entry(data, iova, num_inv);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] iommu/exynos: Handle failed IOMMU device registration properly

[Marek Szyprowski]

On 11.07.2022 01:05, Sam Protsenko wrote:
> If iommu_device_register() fails in exynos_sysmmu_probe(), the previous
> calls have to be cleaned up. In this case, the iommu_device_sysfs_add()
> should be cleaned up, by calling its remove counterpart call.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Changes in v2:
>    - (none) This patch is new and added in v2
>
>   drivers/iommu/exynos-iommu.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 8f80aaa35092..c85db9dab851 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -629,7 +629,7 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>   
>   	ret = iommu_device_register(&data->iommu, &exynos_iommu_ops, dev);
>   	if (ret)
> -		return ret;
> +		goto err_iommu_register;
>   
>   	platform_set_drvdata(pdev, data);
>   
> @@ -656,6 +656,10 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>   	pm_runtime_enable(dev);
>   
>   	return 0;
> +
> +err_iommu_register:
> +	iommu_device_sysfs_remove(&data->iommu);
> +	return ret;
>   }
>   
>   static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/7] iommu/exynos: Use lookup based approach to access registers

[Marek Szyprowski]

On 11.07.2022 01:06, Sam Protsenko wrote:
> At the moment the driver supports SysMMU v1..v5 versions. SysMMU v5 has
> different register layout than SysMMU v1..v3. Instead of checking the
> version each time before reading/writing the registers, let's create
> corresponding register table for each SysMMU version and set the needed
> table on init, checking the SysMMU version one single time. This way is
> faster and more elegant.
>
> No functional change here, just a refactoring patch.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Changes in v2:
>    - Reworked existing code (SysMMU v1..v5) to use this approach
>    - Extracted v7 registers to the separate patches
>    - Replaced MMU_REG() with corresponding SysMMU read/write functions
>    - Improved the comment for 0x1 offsets triggering an unaligned access
>      exception
>    - Removed support for VMID number, as only VMID=0 (default) is used
>      for now
>    - Renamed register index names to reflect the old SysMMU version
>      register names
>
>   drivers/iommu/exynos-iommu.c | 141 ++++++++++++++++++++++-------------
>   1 file changed, 90 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 494f7d7aa9c5..0cb1ce10db51 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -136,9 +136,6 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>   #define CFG_FLPDCACHE	(1 << 20) /* System MMU 3.2+ only */
>   
>   /* common registers */
> -#define REG_MMU_CTRL		0x000
> -#define REG_MMU_CFG		0x004
> -#define REG_MMU_STATUS		0x008
>   #define REG_MMU_VERSION		0x034
>   
>   #define MMU_MAJ_VER(val)	((val) >> 7)
> @@ -148,31 +145,57 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>   #define MAKE_MMU_VER(maj, min)	((((maj) & 0xF) << 7) | ((min) & 0x7F))
>   
>   /* v1.x - v3.x registers */
> -#define REG_MMU_FLUSH		0x00C
> -#define REG_MMU_FLUSH_ENTRY	0x010
> -#define REG_PT_BASE_ADDR	0x014
> -#define REG_INT_STATUS		0x018
> -#define REG_INT_CLEAR		0x01C
> -
>   #define REG_PAGE_FAULT_ADDR	0x024
>   #define REG_AW_FAULT_ADDR	0x028
>   #define REG_AR_FAULT_ADDR	0x02C
>   #define REG_DEFAULT_SLAVE_ADDR	0x030
>   
>   /* v5.x registers */
> -#define REG_V5_PT_BASE_PFN	0x00C
> -#define REG_V5_MMU_FLUSH_ALL	0x010
> -#define REG_V5_MMU_FLUSH_ENTRY	0x014
> -#define REG_V5_MMU_FLUSH_RANGE	0x018
> -#define REG_V5_MMU_FLUSH_START	0x020
> -#define REG_V5_MMU_FLUSH_END	0x024
> -#define REG_V5_INT_STATUS	0x060
> -#define REG_V5_INT_CLEAR	0x064
>   #define REG_V5_FAULT_AR_VA	0x070
>   #define REG_V5_FAULT_AW_VA	0x080
>   
>   #define has_sysmmu(dev)		(dev_iommu_priv_get(dev) != NULL)
>   
> +enum {
> +	REG_SET_V1,
> +	REG_SET_V5,
> +	MAX_REG_SET
> +};
> +
> +enum {
> +	IDX_CTRL,
> +	IDX_CFG,
> +	IDX_STATUS,
> +	IDX_PT_BASE,
> +	IDX_FLUSH_ALL,
> +	IDX_FLUSH_ENTRY,
> +	IDX_FLUSH_RANGE,
> +	IDX_FLUSH_START,
> +	IDX_FLUSH_END,
> +	IDX_INT_STATUS,
> +	IDX_INT_CLEAR,
> +	MAX_REG_IDX
> +};
> +
> +/*
> + * Some SysMMU versions might not implement some registers from this set, thus
> + * those registers shouldn't be accessed. Set the offsets for those registers to
> + * 0x1 to trigger an unaligned access exception, which can help one to debug
> + * related issues.
> + */
> +static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
> +	/* SysMMU v1..v3 */
> +	{
> +		0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
> +		0x18, 0x1c,
> +	},
> +	/* SysMMU v5 */
> +	{
> +		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
> +		0x60, 0x64,
> +	},
> +};
> +
>   static struct device *dma_dev;
>   static struct kmem_cache *lv2table_kmem_cache;
>   static sysmmu_pte_t *zero_lv2_table;
> @@ -274,6 +297,7 @@ struct sysmmu_drvdata {
>   	unsigned int version;		/* our version */
>   
>   	struct iommu_device iommu;	/* IOMMU core handle */
> +	const unsigned int *regs;	/* register set */
>   };
>   
>   static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> @@ -281,20 +305,30 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
>   	return container_of(dom, struct exynos_iommu_domain, domain);
>   }
>   
> +static void sysmmu_write(struct sysmmu_drvdata *data, size_t idx, u32 val)
> +{
> +	writel(val, data->sfrbase + data->regs[idx]);
> +}
> +
> +static u32 sysmmu_read(struct sysmmu_drvdata *data, size_t idx)
> +{
> +	return readl(data->sfrbase + data->regs[idx]);
> +}
> +
>   static void sysmmu_unblock(struct sysmmu_drvdata *data)
>   {
> -	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
> +	sysmmu_write(data, IDX_CTRL, CTRL_ENABLE);
>   }
>   
>   static bool sysmmu_block(struct sysmmu_drvdata *data)
>   {
>   	int i = 120;
>   
> -	writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
> -	while ((i > 0) && !(readl(data->sfrbase + REG_MMU_STATUS) & 1))
> +	sysmmu_write(data, IDX_CTRL, CTRL_BLOCK);
> +	while (i > 0 && !(sysmmu_read(data, IDX_STATUS) & 0x1))
>   		--i;
>   
> -	if (!(readl(data->sfrbase + REG_MMU_STATUS) & 1)) {
> +	if (!(sysmmu_read(data, IDX_STATUS) & 0x1)) {
>   		sysmmu_unblock(data);
>   		return false;
>   	}
> @@ -304,10 +338,7 @@ static bool sysmmu_block(struct sysmmu_drvdata *data)
>   
>   static void __sysmmu_tlb_invalidate(struct sysmmu_drvdata *data)
>   {
> -	if (MMU_MAJ_VER(data->version) < 5)
> -		writel(0x1, data->sfrbase + REG_MMU_FLUSH);
> -	else
> -		writel(0x1, data->sfrbase + REG_V5_MMU_FLUSH_ALL);
> +	sysmmu_write(data, IDX_FLUSH_ALL, 0x1);
>   }
>   
>   static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> @@ -317,31 +348,33 @@ static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
>   
>   	if (MMU_MAJ_VER(data->version) < 5) {
>   		for (i = 0; i < num_inv; i++) {
> -			writel((iova & SPAGE_MASK) | 1,
> -				     data->sfrbase + REG_MMU_FLUSH_ENTRY);
> +			sysmmu_write(data, IDX_FLUSH_ENTRY,
> +				     (iova & SPAGE_MASK) | 0x1);
>   			iova += SPAGE_SIZE;
>   		}
>   	} else {
>   		if (num_inv == 1) {
> -			writel((iova & SPAGE_MASK) | 1,
> -				     data->sfrbase + REG_V5_MMU_FLUSH_ENTRY);
> +			sysmmu_write(data, IDX_FLUSH_ENTRY,
> +				     (iova & SPAGE_MASK) | 0x1);
>   		} else {
> -			writel((iova & SPAGE_MASK),
> -				     data->sfrbase + REG_V5_MMU_FLUSH_START);
> -			writel((iova & SPAGE_MASK) + (num_inv - 1) * SPAGE_SIZE,
> -				     data->sfrbase + REG_V5_MMU_FLUSH_END);
> -			writel(1, data->sfrbase + REG_V5_MMU_FLUSH_RANGE);
> +			sysmmu_write(data, IDX_FLUSH_START, iova & SPAGE_MASK);
> +			sysmmu_write(data, IDX_FLUSH_END, (iova & SPAGE_MASK) +
> +				     (num_inv - 1) * SPAGE_SIZE);
> +			sysmmu_write(data, IDX_FLUSH_RANGE, 0x1);
>   		}
>   	}
>   }
>   
>   static void __sysmmu_set_ptbase(struct sysmmu_drvdata *data, phys_addr_t pgd)
>   {
> +	u32 pt_base;
> +
>   	if (MMU_MAJ_VER(data->version) < 5)
> -		writel(pgd, data->sfrbase + REG_PT_BASE_ADDR);
> +		pt_base = pgd;
>   	else
> -		writel(pgd >> SPAGE_ORDER, data->sfrbase + REG_V5_PT_BASE_PFN);
> +		pt_base = pgd >> SPAGE_ORDER;
>   
> +	sysmmu_write(data, IDX_PT_BASE, pt_base);
>   	__sysmmu_tlb_invalidate(data);
>   }
>   
> @@ -365,8 +398,7 @@ static void __sysmmu_get_version(struct sysmmu_drvdata *data)
>   {
>   	u32 ver;
>   
> -	__sysmmu_enable_clocks(data);
> -
> +	/* Don't use sysmmu_read() here, as data->regs is not set yet */
>   	ver = readl(data->sfrbase + REG_MMU_VERSION);
>   
>   	/* controllers on some SoCs don't report proper version */
> @@ -377,6 +409,17 @@ static void __sysmmu_get_version(struct sysmmu_drvdata *data)
>   
>   	dev_dbg(data->sysmmu, "hardware version: %d.%d\n",
>   		MMU_MAJ_VER(data->version), MMU_MIN_VER(data->version));
> +}
> +
> +static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
> +{
> +	__sysmmu_enable_clocks(data);
> +
> +	__sysmmu_get_version(data);
> +	if (MMU_MAJ_VER(data->version) < 5)
> +		data->regs = sysmmu_regs[REG_SET_V1];
> +	else
> +		data->regs = sysmmu_regs[REG_SET_V5];
>   
>   	__sysmmu_disable_clocks(data);
>   }
> @@ -405,19 +448,14 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
>   	const struct sysmmu_fault_info *finfo;
>   	unsigned int i, n, itype;
>   	sysmmu_iova_t fault_addr;
> -	unsigned short reg_status, reg_clear;
>   	int ret = -ENOSYS;
>   
>   	WARN_ON(!data->active);
>   
>   	if (MMU_MAJ_VER(data->version) < 5) {
> -		reg_status = REG_INT_STATUS;
> -		reg_clear = REG_INT_CLEAR;
>   		finfo = sysmmu_faults;
>   		n = ARRAY_SIZE(sysmmu_faults);
>   	} else {
> -		reg_status = REG_V5_INT_STATUS;
> -		reg_clear = REG_V5_INT_CLEAR;
>   		finfo = sysmmu_v5_faults;
>   		n = ARRAY_SIZE(sysmmu_v5_faults);
>   	}
> @@ -426,7 +464,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
>   
>   	clk_enable(data->clk_master);
>   
> -	itype = __ffs(readl(data->sfrbase + reg_status));
> +	itype = __ffs(sysmmu_read(data, IDX_INT_STATUS));
>   	for (i = 0; i < n; i++, finfo++)
>   		if (finfo->bit == itype)
>   			break;
> @@ -443,7 +481,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
>   	/* fault is not recovered by fault handler */
>   	BUG_ON(ret != 0);
>   
> -	writel(1 << itype, data->sfrbase + reg_clear);
> +	sysmmu_write(data, IDX_INT_CLEAR, 1 << itype);
>   
>   	sysmmu_unblock(data);
>   
> @@ -461,8 +499,8 @@ static void __sysmmu_disable(struct sysmmu_drvdata *data)
>   	clk_enable(data->clk_master);
>   
>   	spin_lock_irqsave(&data->lock, flags);
> -	writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL);
> -	writel(0, data->sfrbase + REG_MMU_CFG);
> +	sysmmu_write(data, IDX_CTRL, CTRL_DISABLE);
> +	sysmmu_write(data, IDX_CFG, 0x0);
>   	data->active = false;
>   	spin_unlock_irqrestore(&data->lock, flags);
>   
> @@ -482,7 +520,7 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
>   
>   	cfg |= CFG_EAP; /* enable access protection bits check */
>   
> -	writel(cfg, data->sfrbase + REG_MMU_CFG);
> +	sysmmu_write(data, IDX_CFG, cfg);
>   }
>   
>   static void __sysmmu_enable(struct sysmmu_drvdata *data)
> @@ -492,10 +530,10 @@ static void __sysmmu_enable(struct sysmmu_drvdata *data)
>   	__sysmmu_enable_clocks(data);
>   
>   	spin_lock_irqsave(&data->lock, flags);
> -	writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
> +	sysmmu_write(data, IDX_CTRL, CTRL_BLOCK);
>   	__sysmmu_init_config(data);
>   	__sysmmu_set_ptbase(data, data->pgtable);
> -	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
> +	sysmmu_write(data, IDX_CTRL, CTRL_ENABLE);
>   	data->active = true;
>   	spin_unlock_irqrestore(&data->lock, flags);
>   
> @@ -622,6 +660,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>   	data->sysmmu = dev;
>   	spin_lock_init(&data->lock);
>   
> +	sysmmu_get_hw_info(data);
> +
>   	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
>   				     dev_name(data->sysmmu));
>   	if (ret)
> @@ -633,7 +673,6 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, data);
>   
> -	__sysmmu_get_version(data);
>   	if (PG_ENT_SHIFT < 0) {
>   		if (MMU_MAJ_VER(data->version) < 5) {
>   			PG_ENT_SHIFT = SYSMMU_PG_ENT_SHIFT;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/7] iommu/exynos: Check if SysMMU v7 has VM registers

[Marek Szyprowski]

On 11.07.2022 01:06, Sam Protsenko wrote:
> SysMMU v7 can have Virtual Machine registers, which implement multiple
> translation domains. The driver should know if it's true or not, as VM
> registers shouldn't be accessed if not present. Read corresponding
> capabilities register to obtain that info, and store it in driver data.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

I would merge this with the next one. Imho this change doesn't make much 
sense on it's own.

> ---
> Changes in v2:
>    - Removed the 'const' qualifier for local non-pointer variables
>
>   drivers/iommu/exynos-iommu.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 0cb1ce10db51..48681189ccf8 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -135,6 +135,9 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>   #define CFG_SYSSEL	(1 << 22) /* System MMU 3.2 only */
>   #define CFG_FLPDCACHE	(1 << 20) /* System MMU 3.2+ only */
>   
> +#define CAPA0_CAPA1_EXIST		BIT(11)
> +#define CAPA1_VCR_ENABLED		BIT(14)
> +
>   /* common registers */
>   #define REG_MMU_VERSION		0x034
>   
> @@ -154,6 +157,10 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>   #define REG_V5_FAULT_AR_VA	0x070
>   #define REG_V5_FAULT_AW_VA	0x080
>   
> +/* v7.x registers */
> +#define REG_V7_CAPA0		0x870
> +#define REG_V7_CAPA1		0x874
> +
>   #define has_sysmmu(dev)		(dev_iommu_priv_get(dev) != NULL)
>   
>   enum {
> @@ -298,6 +305,9 @@ struct sysmmu_drvdata {
>   
>   	struct iommu_device iommu;	/* IOMMU core handle */
>   	const unsigned int *regs;	/* register set */
> +
> +	/* v7 fields */
> +	bool has_vcr;			/* virtual machine control register */
>   };
>   
>   static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> @@ -411,11 +421,27 @@ static void __sysmmu_get_version(struct sysmmu_drvdata *data)
>   		MMU_MAJ_VER(data->version), MMU_MIN_VER(data->version));
>   }
>   
> +static bool __sysmmu_has_capa1(struct sysmmu_drvdata *data)
> +{
> +	u32 capa0 = readl(data->sfrbase + REG_V7_CAPA0);
> +
> +	return capa0 & CAPA0_CAPA1_EXIST;
> +}
> +
> +static void __sysmmu_get_vcr(struct sysmmu_drvdata *data)
> +{
> +	u32 capa1 = readl(data->sfrbase + REG_V7_CAPA1);
> +
> +	data->has_vcr = capa1 & CAPA1_VCR_ENABLED;
> +}
> +
>   static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
>   {
>   	__sysmmu_enable_clocks(data);
>   
>   	__sysmmu_get_version(data);
> +	if (MMU_MAJ_VER(data->version) >= 7 && __sysmmu_has_capa1(data))
> +		__sysmmu_get_vcr(data);
>   	if (MMU_MAJ_VER(data->version) < 5)
>   		data->regs = sysmmu_regs[REG_SET_V1];
>   	else

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 7/7] iommu/exynos: Enable default VM instance on SysMMU v7

[Marek Szyprowski]

On 11.07.2022 01:06, Sam Protsenko wrote:
> In order to enable SysMMU v7 with VM register layout, at least the
> default VM instance (n=0) must be enabled, in addition to enabling the
> SysMMU itself. To do so, add corresponding write to MMU_CTRL_VM[0]
> register, before writing to MMU_CTRL register.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Changes in v2:
>    - Extracted VM enabling code to the separate function
>    - Used new SysMMU read/write functions to access the registers
>
>   drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 64bf3331064f..2b333e137f57 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -135,6 +135,8 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>   #define CFG_SYSSEL	(1 << 22) /* System MMU 3.2 only */
>   #define CFG_FLPDCACHE	(1 << 20) /* System MMU 3.2+ only */
>   
> +#define CTRL_VM_ENABLE			BIT(0)
> +#define CTRL_VM_FAULT_MODE_STALL	BIT(3)
>   #define CAPA0_CAPA1_EXIST		BIT(11)
>   #define CAPA1_VCR_ENABLED		BIT(14)
>   
> @@ -183,6 +185,7 @@ enum {
>   	IDX_FLUSH_END,
>   	IDX_INT_STATUS,
>   	IDX_INT_CLEAR,
> +	IDX_CTRL_VM,
>   	MAX_REG_IDX
>   };
>   
> @@ -196,22 +199,22 @@ static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
>   	/* SysMMU v1..v3 */
>   	{
>   		0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
> -		0x18, 0x1c,
> +		0x18, 0x1c, 0x1,
>   	},
>   	/* SysMMU v5 */
>   	{
>   		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
> -		0x60, 0x64,
> +		0x60, 0x64, 0x1,
>   	},
>   	/* SysMMU v7: Default register set (non-VM) */
>   	{
>   		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
> -		0x60, 0x64,
> +		0x60, 0x64, 0x1,
>   	},
>   	/* SysMMU v7: VM capable register set */
>   	{
>   		0x00, 0x04, 0x08, 0x800c, 0x8010, 0x8014, 0x8018, 0x8020,
> -		0x8024, 0x60, 0x64,
> +		0x8024, 0x60, 0x64, 0x8000,
>   	},
>   };
>   
> @@ -567,6 +570,18 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
>   	sysmmu_write(data, IDX_CFG, cfg);
>   }
>   
> +static void __sysmmu_enable_vid(struct sysmmu_drvdata *data)
> +{
> +	u32 ctrl;
> +
> +	if (MMU_MAJ_VER(data->version) < 7 || !data->has_vcr)
> +		return;
> +
> +	ctrl = sysmmu_read(data, IDX_CTRL_VM);
> +	ctrl |= CTRL_VM_ENABLE | CTRL_VM_FAULT_MODE_STALL;
> +	sysmmu_write(data, IDX_CTRL_VM, ctrl);
> +}
> +
>   static void __sysmmu_enable(struct sysmmu_drvdata *data)
>   {
>   	unsigned long flags;
> @@ -577,6 +592,7 @@ static void __sysmmu_enable(struct sysmmu_drvdata *data)
>   	sysmmu_write(data, IDX_CTRL, CTRL_BLOCK);
>   	__sysmmu_init_config(data);
>   	__sysmmu_set_ptbase(data, data->pgtable);
> +	__sysmmu_enable_vid(data);
>   	sysmmu_write(data, IDX_CTRL, CTRL_ENABLE);
>   	data->active = true;
>   	spin_unlock_irqrestore(&data->lock, flags);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] iommu/exynos: Reuse SysMMU constants for page size and order

[Krzysztof Kozlowski]

On 11/07/2022 01:05, Sam Protsenko wrote:
> Using SZ_4K in context of SysMMU driver is better than using PAGE_SIZE,
> as PAGE_SIZE might have different value on different platforms. Though
> it would be even better to use more specific constants, already existing
> in SysMMU driver. Make the code more strict by using SPAGE_ORDER and
> SPAGE_SIZE constants.
> 
> It also makes sense, as __sysmmu_tlb_invalidate_entry() also uses
> SPAGE_* constants for further calculations with num_inv param, so it's
> logical that num_inv should be previously calculated using also SPAGE_*
> values.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] iommu/exynos: Handle failed IOMMU device registration properly

[Krzysztof Kozlowski]

On 11/07/2022 01:05, Sam Protsenko wrote:
> If iommu_device_register() fails in exynos_sysmmu_probe(), the previous
> calls have to be cleaned up. In this case, the iommu_device_sysfs_add()
> should be cleaned up, by calling its remove counterpart call.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

Fixes: d2c302b6e8b1 ("iommu/exynos: Make use of iommu_device_register
interface")



Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] iommu/exynos: Set correct dma mask for SysMMU v5+

[Krzysztof Kozlowski]

On 11/07/2022 01:05, Sam Protsenko wrote:
> SysMMU v5+ supports 36 bit physical address space. Set corresponding DMA
> mask to avoid falling back to SWTLBIO usage in dma_map_single() because
> of failed dma_capable() check.
> 
> The original code for this fix was suggested by Marek.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Co-developed-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/7] iommu/exynos: Use lookup based approach to access registers

[Robin Murphy]

On 2022-07-11 00:06, Sam Protsenko wrote:
> At the moment the driver supports SysMMU v1..v5 versions. SysMMU v5 has
> different register layout than SysMMU v1..v3. Instead of checking the
> version each time before reading/writing the registers, let's create
> corresponding register table for each SysMMU version and set the needed
> table on init, checking the SysMMU version one single time. This way is
> faster and more elegant.
> 
> No functional change here, just a refactoring patch.

FWIW I'd say that this absolutely *is* a functional change. Achieving 
the same end result, but fundamentally changing the mechanism used to 
get there, is a bit different to simply moving code around.

> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>    - Reworked existing code (SysMMU v1..v5) to use this approach
>    - Extracted v7 registers to the separate patches
>    - Replaced MMU_REG() with corresponding SysMMU read/write functions
>    - Improved the comment for 0x1 offsets triggering an unaligned access
>      exception
>    - Removed support for VMID number, as only VMID=0 (default) is used
>      for now
>    - Renamed register index names to reflect the old SysMMU version
>      register names
> 
>   drivers/iommu/exynos-iommu.c | 141 ++++++++++++++++++++++-------------
>   1 file changed, 90 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 494f7d7aa9c5..0cb1ce10db51 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -136,9 +136,6 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>   #define CFG_FLPDCACHE	(1 << 20) /* System MMU 3.2+ only */
>   
>   /* common registers */
> -#define REG_MMU_CTRL		0x000
> -#define REG_MMU_CFG		0x004
> -#define REG_MMU_STATUS		0x008
>   #define REG_MMU_VERSION		0x034
>   
>   #define MMU_MAJ_VER(val)	((val) >> 7)
> @@ -148,31 +145,57 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>   #define MAKE_MMU_VER(maj, min)	((((maj) & 0xF) << 7) | ((min) & 0x7F))
>   
>   /* v1.x - v3.x registers */
> -#define REG_MMU_FLUSH		0x00C
> -#define REG_MMU_FLUSH_ENTRY	0x010
> -#define REG_PT_BASE_ADDR	0x014
> -#define REG_INT_STATUS		0x018
> -#define REG_INT_CLEAR		0x01C
> -
>   #define REG_PAGE_FAULT_ADDR	0x024
>   #define REG_AW_FAULT_ADDR	0x028
>   #define REG_AR_FAULT_ADDR	0x02C
>   #define REG_DEFAULT_SLAVE_ADDR	0x030
>   
>   /* v5.x registers */
> -#define REG_V5_PT_BASE_PFN	0x00C
> -#define REG_V5_MMU_FLUSH_ALL	0x010
> -#define REG_V5_MMU_FLUSH_ENTRY	0x014
> -#define REG_V5_MMU_FLUSH_RANGE	0x018
> -#define REG_V5_MMU_FLUSH_START	0x020
> -#define REG_V5_MMU_FLUSH_END	0x024
> -#define REG_V5_INT_STATUS	0x060
> -#define REG_V5_INT_CLEAR	0x064
>   #define REG_V5_FAULT_AR_VA	0x070
>   #define REG_V5_FAULT_AW_VA	0x080
>   
>   #define has_sysmmu(dev)		(dev_iommu_priv_get(dev) != NULL)
>   
> +enum {
> +	REG_SET_V1,
> +	REG_SET_V5,
> +	MAX_REG_SET
> +};
> +
> +enum {
> +	IDX_CTRL,
> +	IDX_CFG,
> +	IDX_STATUS,
> +	IDX_PT_BASE,
> +	IDX_FLUSH_ALL,
> +	IDX_FLUSH_ENTRY,
> +	IDX_FLUSH_RANGE,
> +	IDX_FLUSH_START,
> +	IDX_FLUSH_END,
> +	IDX_INT_STATUS,
> +	IDX_INT_CLEAR,
> +	MAX_REG_IDX
> +};
> +
> +/*
> + * Some SysMMU versions might not implement some registers from this set, thus
> + * those registers shouldn't be accessed. Set the offsets for those registers to
> + * 0x1 to trigger an unaligned access exception, which can help one to debug
> + * related issues.
> + */
> +static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {

Do we really need MAX_REG_SET? Maybe there's a consistency argument, I 
guess :/

> +	/* SysMMU v1..v3 */
> +	{
> +		0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
> +		0x18, 0x1c,

This looks fragile and unnecessarily difficult to follow and maintain - 
designated initialisers would be a lot better in all respects, i.e.:

	[REG_SET_V1] = {
		...
		[IDX_PT_BASE] = REG_PT_BASE_ADDR,
		...

etc.

> +	},
> +	/* SysMMU v5 */
> +	{
> +		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
> +		0x60, 0x64,
> +	},
> +};
> +
>   static struct device *dma_dev;
>   static struct kmem_cache *lv2table_kmem_cache;
>   static sysmmu_pte_t *zero_lv2_table;
> @@ -274,6 +297,7 @@ struct sysmmu_drvdata {
>   	unsigned int version;		/* our version */
>   
>   	struct iommu_device iommu;	/* IOMMU core handle */
> +	const unsigned int *regs;	/* register set */
>   };
>   
>   static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> @@ -281,20 +305,30 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
>   	return container_of(dom, struct exynos_iommu_domain, domain);
>   }
>   
> +static void sysmmu_write(struct sysmmu_drvdata *data, size_t idx, u32 val)
> +{
> +	writel(val, data->sfrbase + data->regs[idx]);
> +}
> +
> +static u32 sysmmu_read(struct sysmmu_drvdata *data, size_t idx)
> +{
> +	return readl(data->sfrbase + data->regs[idx]);
> +}
> +
>   static void sysmmu_unblock(struct sysmmu_drvdata *data)
>   {
> -	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
> +	sysmmu_write(data, IDX_CTRL, CTRL_ENABLE);
>   }
>   
>   static bool sysmmu_block(struct sysmmu_drvdata *data)
>   {
>   	int i = 120;
>   
> -	writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
> -	while ((i > 0) && !(readl(data->sfrbase + REG_MMU_STATUS) & 1))
> +	sysmmu_write(data, IDX_CTRL, CTRL_BLOCK);
> +	while (i > 0 && !(sysmmu_read(data, IDX_STATUS) & 0x1))
>   		--i;
>   
> -	if (!(readl(data->sfrbase + REG_MMU_STATUS) & 1)) {
> +	if (!(sysmmu_read(data, IDX_STATUS) & 0x1)) {
>   		sysmmu_unblock(data);
>   		return false;
>   	}
> @@ -304,10 +338,7 @@ static bool sysmmu_block(struct sysmmu_drvdata *data)
>   
>   static void __sysmmu_tlb_invalidate(struct sysmmu_drvdata *data)
>   {
> -	if (MMU_MAJ_VER(data->version) < 5)
> -		writel(0x1, data->sfrbase + REG_MMU_FLUSH);
> -	else
> -		writel(0x1, data->sfrbase + REG_V5_MMU_FLUSH_ALL);
> +	sysmmu_write(data, IDX_FLUSH_ALL, 0x1);
>   }
>   
>   static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> @@ -317,31 +348,33 @@ static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
>   
>   	if (MMU_MAJ_VER(data->version) < 5) {
>   		for (i = 0; i < num_inv; i++) {
> -			writel((iova & SPAGE_MASK) | 1,
> -				     data->sfrbase + REG_MMU_FLUSH_ENTRY);
> +			sysmmu_write(data, IDX_FLUSH_ENTRY,
> +				     (iova & SPAGE_MASK) | 0x1);
>   			iova += SPAGE_SIZE;
>   		}
>   	} else {
>   		if (num_inv == 1) {

You could merge this condition into the one above now. That much I'd 
call non-functional refactoring ;)

> -			writel((iova & SPAGE_MASK) | 1,
> -				     data->sfrbase + REG_V5_MMU_FLUSH_ENTRY);
> +			sysmmu_write(data, IDX_FLUSH_ENTRY,
> +				     (iova & SPAGE_MASK) | 0x1);
>   		} else {
> -			writel((iova & SPAGE_MASK),
> -				     data->sfrbase + REG_V5_MMU_FLUSH_START);
> -			writel((iova & SPAGE_MASK) + (num_inv - 1) * SPAGE_SIZE,
> -				     data->sfrbase + REG_V5_MMU_FLUSH_END);
> -			writel(1, data->sfrbase + REG_V5_MMU_FLUSH_RANGE);
> +			sysmmu_write(data, IDX_FLUSH_START, iova & SPAGE_MASK);
> +			sysmmu_write(data, IDX_FLUSH_END, (iova & SPAGE_MASK) +
> +				     (num_inv - 1) * SPAGE_SIZE);
> +			sysmmu_write(data, IDX_FLUSH_RANGE, 0x1);
>   		}
>   	}
>   }
>   
>   static void __sysmmu_set_ptbase(struct sysmmu_drvdata *data, phys_addr_t pgd)
>   {
> +	u32 pt_base;
> +
>   	if (MMU_MAJ_VER(data->version) < 5)
> -		writel(pgd, data->sfrbase + REG_PT_BASE_ADDR);
> +		pt_base = pgd;
>   	else
> -		writel(pgd >> SPAGE_ORDER, data->sfrbase + REG_V5_PT_BASE_PFN);
> +		pt_base = pgd >> SPAGE_ORDER;
>   
> +	sysmmu_write(data, IDX_PT_BASE, pt_base);
>   	__sysmmu_tlb_invalidate(data);
>   }
>   
> @@ -365,8 +398,7 @@ static void __sysmmu_get_version(struct sysmmu_drvdata *data)
>   {
>   	u32 ver;
>   
> -	__sysmmu_enable_clocks(data);
> -
> +	/* Don't use sysmmu_read() here, as data->regs is not set yet */
>   	ver = readl(data->sfrbase + REG_MMU_VERSION);
>   
>   	/* controllers on some SoCs don't report proper version */
> @@ -377,6 +409,17 @@ static void __sysmmu_get_version(struct sysmmu_drvdata *data)
>   
>   	dev_dbg(data->sysmmu, "hardware version: %d.%d\n",
>   		MMU_MAJ_VER(data->version), MMU_MIN_VER(data->version));
> +}
> +
> +static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
> +{

Seems a bit unnecessary to split the call up like this - I'd say the 
register set is fundamentally connected to the version, and 
"get_hw_info" is even less meaningfully descriptive than just having 
"get_version" take care of one more assignment, but hey ho, it's not my 
driver.

Thanks,
Robin.

> +	__sysmmu_enable_clocks(data);
> +
> +	__sysmmu_get_version(data);
> +	if (MMU_MAJ_VER(data->version) < 5)
> +		data->regs = sysmmu_regs[REG_SET_V1];
> +	else
> +		data->regs = sysmmu_regs[REG_SET_V5];
>   
>   	__sysmmu_disable_clocks(data);
>   }
> @@ -405,19 +448,14 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
>   	const struct sysmmu_fault_info *finfo;
>   	unsigned int i, n, itype;
>   	sysmmu_iova_t fault_addr;
> -	unsigned short reg_status, reg_clear;
>   	int ret = -ENOSYS;
>   
>   	WARN_ON(!data->active);
>   
>   	if (MMU_MAJ_VER(data->version) < 5) {
> -		reg_status = REG_INT_STATUS;
> -		reg_clear = REG_INT_CLEAR;
>   		finfo = sysmmu_faults;
>   		n = ARRAY_SIZE(sysmmu_faults);
>   	} else {
> -		reg_status = REG_V5_INT_STATUS;
> -		reg_clear = REG_V5_INT_CLEAR;
>   		finfo = sysmmu_v5_faults;
>   		n = ARRAY_SIZE(sysmmu_v5_faults);
>   	}
> @@ -426,7 +464,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
>   
>   	clk_enable(data->clk_master);
>   
> -	itype = __ffs(readl(data->sfrbase + reg_status));
> +	itype = __ffs(sysmmu_read(data, IDX_INT_STATUS));
>   	for (i = 0; i < n; i++, finfo++)
>   		if (finfo->bit == itype)
>   			break;
> @@ -443,7 +481,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
>   	/* fault is not recovered by fault handler */
>   	BUG_ON(ret != 0);
>   
> -	writel(1 << itype, data->sfrbase + reg_clear);
> +	sysmmu_write(data, IDX_INT_CLEAR, 1 << itype);
>   
>   	sysmmu_unblock(data);
>   
> @@ -461,8 +499,8 @@ static void __sysmmu_disable(struct sysmmu_drvdata *data)
>   	clk_enable(data->clk_master);
>   
>   	spin_lock_irqsave(&data->lock, flags);
> -	writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL);
> -	writel(0, data->sfrbase + REG_MMU_CFG);
> +	sysmmu_write(data, IDX_CTRL, CTRL_DISABLE);
> +	sysmmu_write(data, IDX_CFG, 0x0);
>   	data->active = false;
>   	spin_unlock_irqrestore(&data->lock, flags);
>   
> @@ -482,7 +520,7 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
>   
>   	cfg |= CFG_EAP; /* enable access protection bits check */
>   
> -	writel(cfg, data->sfrbase + REG_MMU_CFG);
> +	sysmmu_write(data, IDX_CFG, cfg);
>   }
>   
>   static void __sysmmu_enable(struct sysmmu_drvdata *data)
> @@ -492,10 +530,10 @@ static void __sysmmu_enable(struct sysmmu_drvdata *data)
>   	__sysmmu_enable_clocks(data);
>   
>   	spin_lock_irqsave(&data->lock, flags);
> -	writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
> +	sysmmu_write(data, IDX_CTRL, CTRL_BLOCK);
>   	__sysmmu_init_config(data);
>   	__sysmmu_set_ptbase(data, data->pgtable);
> -	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
> +	sysmmu_write(data, IDX_CTRL, CTRL_ENABLE);
>   	data->active = true;
>   	spin_unlock_irqrestore(&data->lock, flags);
>   
> @@ -622,6 +660,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>   	data->sysmmu = dev;
>   	spin_lock_init(&data->lock);
>   
> +	sysmmu_get_hw_info(data);
> +
>   	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
>   				     dev_name(data->sysmmu));
>   	if (ret)
> @@ -633,7 +673,6 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, data);
>   
> -	__sysmmu_get_version(data);
>   	if (PG_ENT_SHIFT < 0) {
>   		if (MMU_MAJ_VER(data->version) < 5) {
>   			PG_ENT_SHIFT = SYSMMU_PG_ENT_SHIFT;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/7] iommu/exynos: Use lookup based approach to access registers

[Krzysztof Kozlowski]

On 11/07/2022 01:06, Sam Protsenko wrote:
> At the moment the driver supports SysMMU v1..v5 versions. SysMMU v5 has
> different register layout than SysMMU v1..v3. Instead of checking the
> version each time before reading/writing the registers, let's create
> corresponding register table for each SysMMU version and set the needed
> table on init, checking the SysMMU version one single time. This way is
> faster and more elegant.
> 
> No functional change here, just a refactoring patch.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Reworked existing code (SysMMU v1..v5) to use this approach
>   - Extracted v7 registers to the separate patches
>   - Replaced MMU_REG() with corresponding SysMMU read/write functions
>   - Improved the comment for 0x1 offsets triggering an unaligned access
>     exception
>   - Removed support for VMID number, as only VMID=0 (default) is used
>     for now
>   - Renamed register index names to reflect the old SysMMU version
>     register names
> 
>  drivers/iommu/exynos-iommu.c | 141 ++++++++++++++++++++++-------------
>  1 file changed, 90 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 494f7d7aa9c5..0cb1ce10db51 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -136,9 +136,6 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>  #define CFG_FLPDCACHE	(1 << 20) /* System MMU 3.2+ only */
>  
>  /* common registers */
> -#define REG_MMU_CTRL		0x000
> -#define REG_MMU_CFG		0x004
> -#define REG_MMU_STATUS		0x008
>  #define REG_MMU_VERSION		0x034
>  
>  #define MMU_MAJ_VER(val)	((val) >> 7)
> @@ -148,31 +145,57 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>  #define MAKE_MMU_VER(maj, min)	((((maj) & 0xF) << 7) | ((min) & 0x7F))
>  
>  /* v1.x - v3.x registers */
> -#define REG_MMU_FLUSH		0x00C
> -#define REG_MMU_FLUSH_ENTRY	0x010
> -#define REG_PT_BASE_ADDR	0x014
> -#define REG_INT_STATUS		0x018
> -#define REG_INT_CLEAR		0x01C
> -
>  #define REG_PAGE_FAULT_ADDR	0x024
>  #define REG_AW_FAULT_ADDR	0x028
>  #define REG_AR_FAULT_ADDR	0x02C
>  #define REG_DEFAULT_SLAVE_ADDR	0x030
>  
>  /* v5.x registers */
> -#define REG_V5_PT_BASE_PFN	0x00C
> -#define REG_V5_MMU_FLUSH_ALL	0x010
> -#define REG_V5_MMU_FLUSH_ENTRY	0x014
> -#define REG_V5_MMU_FLUSH_RANGE	0x018
> -#define REG_V5_MMU_FLUSH_START	0x020
> -#define REG_V5_MMU_FLUSH_END	0x024
> -#define REG_V5_INT_STATUS	0x060
> -#define REG_V5_INT_CLEAR	0x064
>  #define REG_V5_FAULT_AR_VA	0x070
>  #define REG_V5_FAULT_AW_VA	0x080
>  
>  #define has_sysmmu(dev)		(dev_iommu_priv_get(dev) != NULL)
>  
> +enum {
> +	REG_SET_V1,
> +	REG_SET_V5,
> +	MAX_REG_SET
> +};
> +
> +enum {
> +	IDX_CTRL,
> +	IDX_CFG,
> +	IDX_STATUS,
> +	IDX_PT_BASE,
> +	IDX_FLUSH_ALL,
> +	IDX_FLUSH_ENTRY,
> +	IDX_FLUSH_RANGE,
> +	IDX_FLUSH_START,
> +	IDX_FLUSH_END,
> +	IDX_INT_STATUS,
> +	IDX_INT_CLEAR,
> +	MAX_REG_IDX
> +};
> +
> +/*
> + * Some SysMMU versions might not implement some registers from this set, thus
> + * those registers shouldn't be accessed. Set the offsets for those registers to
> + * 0x1 to trigger an unaligned access exception, which can help one to debug
> + * related issues.
> + */
> +static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
> +	/* SysMMU v1..v3 */
> +	{
> +		0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
> +		0x18, 0x1c,
> +	},
> +	/* SysMMU v5 */
> +	{
> +		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
> +		0x60, 0x64,
> +	},
> +};
> +
>  static struct device *dma_dev;
>  static struct kmem_cache *lv2table_kmem_cache;
>  static sysmmu_pte_t *zero_lv2_table;
> @@ -274,6 +297,7 @@ struct sysmmu_drvdata {
>  	unsigned int version;		/* our version */
>  
>  	struct iommu_device iommu;	/* IOMMU core handle */
> +	const unsigned int *regs;	/* register set */
>  };
>  
>  static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> @@ -281,20 +305,30 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
>  	return container_of(dom, struct exynos_iommu_domain, domain);
>  }
>  
> +static void sysmmu_write(struct sysmmu_drvdata *data, size_t idx, u32 val)
> +{
> +	writel(val, data->sfrbase + data->regs[idx]);

Beside what Robin wrote, I also don't think these wrappers actually
help, because you reverse arguments comparing to writel.

How about having a per-variant structure with offsets and using it like:

#define SYSMMU_REG(data, reg) ((data)->sfrbase + (data)->variant->reg)
writel(CTRL_ENABLE, SYSMMU_REG(data, mmu_ctrl_reg))

Would that be more readable?


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/7] iommu/exynos: Add SysMMU v7 register sets

[Robin Murphy]

On 2022-07-11 00:06, Sam Protsenko wrote:
> SysMMU v7 might have different register layouts (VM capable or non-VM
> capable). Check which layout is implemented in current SysMMU module and
> prepare the corresponding register table for futher usage.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>    - (none) This patch is new and added in v2
> 
>   drivers/iommu/exynos-iommu.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 48681189ccf8..64bf3331064f 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -166,6 +166,8 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>   enum {
>   	REG_SET_V1,
>   	REG_SET_V5,
> +	REG_SET_V7_NON_VM,
> +	REG_SET_V7_VM,
>   	MAX_REG_SET
>   };
>   
> @@ -201,6 +203,16 @@ static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
>   		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
>   		0x60, 0x64,
>   	},
> +	/* SysMMU v7: Default register set (non-VM) */
> +	{
> +		0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
> +		0x60, 0x64,
> +	},
> +	/* SysMMU v7: VM capable register set */
> +	{
> +		0x00, 0x04, 0x08, 0x800c, 0x8010, 0x8014, 0x8018, 0x8020,
> +		0x8024, 0x60, 0x64,

Yuck, see, it's turning into an unreadable mess already.

This is also raising the question of whether it's worth abstracting 
accesses to the common registers if it means having an ever-increasing 
number of copies of those same offsets. Personally I'd leave those using 
regular readl/writel, but even if there's an argument for keeping all 
the callsites consistent (modulo the one that already can't be), there's 
no reason the wrappers couldn't pick up the slack, e.g.:

static void sysmmu_write(struct sysmmu_drvdata *data, size_t idx, u32 val)
{
	unsigned int offset;

	if (idx <= IDX_STATUS) {
		offset = idx * 4;
	} else {
		offset = data->regs[idx - IDX_PT_BASE];
		if (WARN_ON(!offset))
			return;
	}
	writel(val, data->sfrbase + offset);
}

Indeed, not abstracting REG_MMU_CTRL via data->regs would then make it 
trivial to be robust against unimplemented registers without even having 
to remember to initialise their offsets to some magic value, which seems 
rather attractive.

(also, as it only strikes me now, why are we passing enum values around 
as size_t? That's just odd)

Thanks,
Robin.

> +	},
>   };
>   
>   static struct device *dma_dev;
> @@ -440,12 +452,18 @@ static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
>   	__sysmmu_enable_clocks(data);
>   
>   	__sysmmu_get_version(data);
> -	if (MMU_MAJ_VER(data->version) >= 7 && __sysmmu_has_capa1(data))
> -		__sysmmu_get_vcr(data);
> -	if (MMU_MAJ_VER(data->version) < 5)
> +	if (MMU_MAJ_VER(data->version) < 5) {
>   		data->regs = sysmmu_regs[REG_SET_V1];
> -	else
> +	} else if (MMU_MAJ_VER(data->version) < 7) {
>   		data->regs = sysmmu_regs[REG_SET_V5];
> +	} else {
> +		if (__sysmmu_has_capa1(data))
> +			__sysmmu_get_vcr(data);
> +		if (data->has_vcr)
> +			data->regs = sysmmu_regs[REG_SET_V7_VM];
> +		else
> +			data->regs = sysmmu_regs[REG_SET_V7_NON_VM];
> +	}
>   
>   	__sysmmu_disable_clocks(data);
>   }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/7] iommu/exynos: Use lookup based approach to access registers

[Sam Protsenko]

On Tue, 12 Jul 2022 at 19:24, Robin Murphy <robin.murphy@arm.com> wrote:

[snip]

> > No functional change here, just a refactoring patch.
>
> FWIW I'd say that this absolutely *is* a functional change. Achieving
> the same end result, but fundamentally changing the mechanism used to
> get there, is a bit different to simply moving code around.
>

As I understand, usually the "functional change" means some change
that can be observed from the user's point of view (i.e. user of this
driver). But ok, I'll clarify this bit in the commit message.

[snip]

> > +/*
> > + * Some SysMMU versions might not implement some registers from this set, thus
> > + * those registers shouldn't be accessed. Set the offsets for those registers to
> > + * 0x1 to trigger an unaligned access exception, which can help one to debug
> > + * related issues.
> > + */
> > +static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
>
> Do we really need MAX_REG_SET? Maybe there's a consistency argument, I
> guess :/
>

Here and below: I reworked the register table using approach suggested
by Krzysztof, so those enums won't be present in v2 at all.

> > +     /* SysMMU v1..v3 */
> > +     {
> > +             0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
> > +             0x18, 0x1c,
>
> This looks fragile and unnecessarily difficult to follow and maintain -
> designated initialisers would be a lot better in all respects, i.e.:
>
>         [REG_SET_V1] = {
>                 ...
>                 [IDX_PT_BASE] = REG_PT_BASE_ADDR,
>                 ...
>
> etc.
>

[snip]

> >   static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> > @@ -317,31 +348,33 @@ static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> >
> >       if (MMU_MAJ_VER(data->version) < 5) {
> >               for (i = 0; i < num_inv; i++) {
> > -                     writel((iova & SPAGE_MASK) | 1,
> > -                                  data->sfrbase + REG_MMU_FLUSH_ENTRY);
> > +                     sysmmu_write(data, IDX_FLUSH_ENTRY,
> > +                                  (iova & SPAGE_MASK) | 0x1);
> >                       iova += SPAGE_SIZE;
> >               }
> >       } else {
> >               if (num_inv == 1) {
>
> You could merge this condition into the one above now. That much I'd
> call non-functional refactoring ;)
>

Done, thanks.

[snip]

> > +
> > +static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
> > +{
>
> Seems a bit unnecessary to split the call up like this - I'd say the
> register set is fundamentally connected to the version, and
> "get_hw_info" is even less meaningfully descriptive than just having
> "get_version" take care of one more assignment, but hey ho, it's not my
> driver.
>

Guess I was looking into downstream vendor's kernel too much :) They
do a bit more things in this function -- like getting TLBs number and
"no block mode" capability; that's why I renamed it. Anyway, don't
have a strong opinion on this one, will use the old name in v2.

Thanks for the review!

> Thanks,
> Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/7] iommu/exynos: Use lookup based approach to access registers

[Sam Protsenko]

On Tue, 12 Jul 2022 at 19:52, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:

[snip]

> > +static void sysmmu_write(struct sysmmu_drvdata *data, size_t idx, u32 val)
> > +{
> > +     writel(val, data->sfrbase + data->regs[idx]);
>
> Beside what Robin wrote, I also don't think these wrappers actually
> help, because you reverse arguments comparing to writel.
>
> How about having a per-variant structure with offsets and using it like:
>
> #define SYSMMU_REG(data, reg) ((data)->sfrbase + (data)->variant->reg)
> writel(CTRL_ENABLE, SYSMMU_REG(data, mmu_ctrl_reg))
>
> Would that be more readable?
>

Yes, this looks better for my taste too. I tend to get a tunnel vision
when working with downstream code for a while. But I noticed that that
approach is used sometimes as well, e.g. in
drivers/pinctrl/samsung/pinctrl-exynos-arm64.c (in struct
samsung_pin_bank_type). Anyway, I've reworked it exactly as you
suggested, will send v2 soon. Thanks!

>
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/7] iommu/exynos: Check if SysMMU v7 has VM registers

[Sam Protsenko]

On Tue, 12 Jul 2022 at 18:47, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
>
> On 11.07.2022 01:06, Sam Protsenko wrote:
> > SysMMU v7 can have Virtual Machine registers, which implement multiple
> > translation domains. The driver should know if it's true or not, as VM
> > registers shouldn't be accessed if not present. Read corresponding
> > capabilities register to obtain that info, and store it in driver data.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>
> I would merge this with the next one. Imho this change doesn't make much
> sense on it's own.
>

Will do in v2, thanks!

[snip]

>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/7] iommu/exynos: Add SysMMU v7 register sets

[Sam Protsenko]

On Tue, 12 Jul 2022 at 20:00, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2022-07-11 00:06, Sam Protsenko wrote:
> > SysMMU v7 might have different register layouts (VM capable or non-VM
> > capable). Check which layout is implemented in current SysMMU module and
> > prepare the corresponding register table for futher usage.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> > Changes in v2:
> >    - (none) This patch is new and added in v2
> >
> >   drivers/iommu/exynos-iommu.c | 26 ++++++++++++++++++++++----
> >   1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> > index 48681189ccf8..64bf3331064f 100644
> > --- a/drivers/iommu/exynos-iommu.c
> > +++ b/drivers/iommu/exynos-iommu.c
> > @@ -166,6 +166,8 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
> >   enum {
> >       REG_SET_V1,
> >       REG_SET_V5,
> > +     REG_SET_V7_NON_VM,
> > +     REG_SET_V7_VM,
> >       MAX_REG_SET
> >   };
> >
> > @@ -201,6 +203,16 @@ static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
> >               0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
> >               0x60, 0x64,
> >       },
> > +     /* SysMMU v7: Default register set (non-VM) */
> > +     {
> > +             0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, 0x18, 0x20, 0x24,
> > +             0x60, 0x64,
> > +     },
> > +     /* SysMMU v7: VM capable register set */
> > +     {
> > +             0x00, 0x04, 0x08, 0x800c, 0x8010, 0x8014, 0x8018, 0x8020,
> > +             0x8024, 0x60, 0x64,
>
> Yuck, see, it's turning into an unreadable mess already.
>

Will be reworked in v2, using variant struct approach suggested by Krzysztof.

> This is also raising the question of whether it's worth abstracting
> accesses to the common registers if it means having an ever-increasing
> number of copies of those same offsets. Personally I'd leave those using
> regular readl/writel, but even if there's an argument for keeping all
> the callsites consistent (modulo the one that already can't be), there's
> no reason the wrappers couldn't pick up the slack, e.g.:
>

Agreed. Gonna leave the common regs out of it in v2, having only
non-common registers in the variant structure. Also in v2 gonna stick
with plain readl/writel calls, using SYSMMU_REG() wrapper suggested by
Krzysztof.

> static void sysmmu_write(struct sysmmu_drvdata *data, size_t idx, u32 val)
> {
>         unsigned int offset;
>
>         if (idx <= IDX_STATUS) {
>                 offset = idx * 4;
>         } else {
>                 offset = data->regs[idx - IDX_PT_BASE];
>                 if (WARN_ON(!offset))
>                         return;
>         }
>         writel(val, data->sfrbase + offset);
> }
>
> Indeed, not abstracting REG_MMU_CTRL via data->regs would then make it
> trivial to be robust against unimplemented registers without even having
> to remember to initialise their offsets to some magic value, which seems
> rather attractive.
>

Just on the discussion point (as this function won't be present in
v2): one reason for this rework is to avoid using if-else branching,
AFAIU those might have some performance impact (caches/branch
prediction). Also the code looks better that way. Of course, in this
particular driver those I/O calls don't happen very often, but still.
One-line static function I used in v1 would be probably inlined by the
compiler. Also SysMMU register layout(s) doesn't seem to be very
consistent, w.r.t. offset values :)

Anyway, I hope the way it's done in v2 will be more to your liking.

> (also, as it only strikes me now, why are we passing enum values around
> as size_t? That's just odd)
>
> Thanks,
> Robin.
>

[snip]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel