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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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