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@xxxxxxxxxx>
> ---
> 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
