On Sun, 3 Jul 2022 at 22:29, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> 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.
>
I was thinking about doing that as a consequent patch (adding SysMMU
v1/v5 register sets). But ok, will add in v2. And will probably split
it per 2 patches:
1. Rework existing driver to use register sets (lookup table). To
keep it as "no functional change" patch.
2. Add SysMMU v7 support.
Also, I will probably replace MMU_REG() with more standard approach,
i.e. will add sysmmu_read() / sysmmu_write() functions instead. It
should make the code tidier.
> > +#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.
>
I used v7 registers naming, as I only added support for v7 register
set in this patch series. As I said above, I'll add SysMMU v1/v5
register sets in v2, and rename those indexes to have old register
names. Despite the differences between register names in SysMMU v1/v5
and v7, their purpose is the same.
> > + 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?
>
Yes, as comment suggests, 0x1 offset is set intentionally to cause the
exception. That might be useful for debugging (if driver is trying to
access some non-existing register on some particular SysMMU version).
I'll improve the comment in v2.
> > + */
> > + {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.
>
Ok, will remove those multiplier bits for now. It can be added later,
when implementing domains support (to use VM registers other than n=0
instance).
> > +};
> > +
> > 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.
>
It's used in patch 4/4. But as discussed above, I will convert
existing code (SysMMU v1/v3/v5) to benefit from register set as well.
Will send in v2.
Thanks for the review!
> >
> > __sysmmu_disable_clocks(data);
> > }
>
>
> Best regards,
> Krzysztof
