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

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(-)

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

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]