Message-ID: <CAPLW+4kc86aOVspBz52vt+uhs8GXDNfekd-7jKhyNjUpi8XwLQ@mail.gmail.com>
Date:   Fri, 8 Jul 2022 21:13:06 +0300
From:   Sam Protsenko <semen.protsenko@...aro.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Marek Szyprowski <m.szyprowski@...sung.com>,
        Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Janghyuck Kim <janghyuck.kim@...sung.com>,
        Cho KyongHo <pullip.cho@...sung.com>,
        Daniel Mentz <danielmentz@...gle.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        iommu@...ts.linux-foundation.org, iommu@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] iommu/exynos: Use lookup based approach to access v7 registers

On Sun, 3 Jul 2022 at 22:29, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> 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@...sung.com>
> >   - Daniel Mentz <danielmentz@...gle.com>
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@...aro.org>
> > ---
> >  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

Powered by blists - more mailing lists