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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Co-developed-by: Marek Szyprowski <[email protected]>
Signed-off-by: Marek Szyprowski <[email protected]>
---
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
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 <[email protected]>
Acked-by: Marek Szyprowski <[email protected]>
> ---
> 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
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 <[email protected]>
Fixes: d2c302b6e8b1 ("iommu/exynos: Make use of iommu_device_register
interface")
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
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
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 <[email protected]>
> Co-developed-by: Marek Szyprowski <[email protected]>
> Signed-off-by: Marek Szyprowski <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
Acked-by: Marek Szyprowski <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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;
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 <[email protected]>
> ---
> 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
On Tue, 12 Jul 2022 at 19:24, Robin Murphy <[email protected]> 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.
On Tue, 12 Jul 2022 at 18:47, Marek Szyprowski <[email protected]> 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 <[email protected]>
>
> 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
>
On Tue, 12 Jul 2022 at 19:52, Krzysztof Kozlowski
<[email protected]> 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