On 02/07/2022 23:37, 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. This way is
> faster and more elegant than checking corresponding condition (if it's
> VM or non-VM SysMMU) each time before accessing v7 registers. For now
> the register table contains only most basic registers needed to add the
> SysMMU v7 support.
>
> This patch is based on downstream work of next authors:
> - Janghyuck Kim <janghyuck.kim@xxxxxxxxxxx>
> - Daniel Mentz <danielmentz@xxxxxxxxxx>
>
> Signed-off-by: Sam Protsenko <semen.protsenko@xxxxxxxxxx>
> ---
> drivers/iommu/exynos-iommu.c | 46 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index df6ddbebbe2b..47017e8945c5 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -180,6 +180,47 @@ static u32 lv2ent_offset(sysmmu_iova_t iova)
>
> #define has_sysmmu(dev) (dev_iommu_priv_get(dev) != NULL)
>
> +#define MMU_REG(data, idx) \
> + ((data)->sfrbase + (data)->regs[idx].off)
I would expect to see users of this - convert all existing regs.
> +#define MMU_VM_REG(data, idx, vmid) \
> + (MMU_REG(data, idx) + (vmid) * (data)->regs[idx].mult)
> +
> +enum {
> + REG_SET_NON_VM,
> + REG_SET_VM,
> + MAX_REG_SET
> +};
> +
> +enum {
> + IDX_CTRL_VM,
> + IDX_CFG_VM,
> + IDX_FLPT_BASE,
Isn't this called REG_MMU_FLUSH? If yes, it's a bit confusing to see
different name used.
> + IDX_ALL_INV,
Isn't this called REG_MMU_FLUSH_ENTRY?
> + MAX_REG_IDX
> +};
> +
> +struct sysmmu_vm_reg {
> + unsigned int off; /* register offset */
> + unsigned int mult; /* VM index offset multiplier */
> +};
> +
> +static const struct sysmmu_vm_reg sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
> + /* Default register set (non-VM) */
> + {
> + /*
> + * SysMMUs without VM support do not have CTRL_VM and CFG_VM
> + * registers. Setting the offsets to 1 will trigger an unaligned
> + * access exception.
So why are you setting offset 1? To trigger unaligned access?
> + */
> + {0x1}, {0x1}, {0x000c}, {0x0010},
> + },
> + /* VM capable register set */
> + {
> + {0x8000, 0x1000}, {0x8004, 0x1000}, {0x800c, 0x1000},
> + {0x8010, 0x1000},
> + },
You add here quite a bit of dead code and some hard-coded numbers.
> +};
> +
> static struct device *dma_dev;
> static struct kmem_cache *lv2table_kmem_cache;
> static sysmmu_pte_t *zero_lv2_table;
> @@ -284,6 +325,7 @@ struct sysmmu_drvdata {
>
> /* v7 fields */
> bool has_vcr; /* virtual machine control register */
> + const struct sysmmu_vm_reg *regs; /* register set */
> };
>
> static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> @@ -407,6 +449,10 @@ static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
> __sysmmu_get_version(data);
> if (MMU_MAJ_VER(data->version) >= 7 && __sysmmu_has_capa1(data))
> __sysmmu_get_vcr(data);
> + if (data->has_vcr)
> + data->regs = sysmmu_regs[REG_SET_VM];
> + else
> + data->regs = sysmmu_regs[REG_SET_NON_VM];
This is set and not read.
>
> __sysmmu_disable_clocks(data);
> }
Best regards,
Krzysztof
