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

[Robin Murphy <robin.murphy@arm.com>]

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);
>   }