From: Sam Protsenko <semen.protsenko@linaro.org> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>, Janghyuck Kim <janghyuck.kim@samsung.com>, Cho KyongHo <pullip.cho@samsung.com>, Daniel Mentz <danielmentz@google.com>, Sumit Semwal <sumit.semwal@linaro.org>, iommu@lists.linux-foundation.org, iommu@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] iommu/exynos: Use lookup based approach to access v7 registers Date: Fri, 8 Jul 2022 21:13:06 +0300 [thread overview] Message-ID: <CAPLW+4kc86aOVspBz52vt+uhs8GXDNfekd-7jKhyNjUpi8XwLQ@mail.gmail.com> (raw) In-Reply-To: <925751c3-83e8-02b8-6a8b-549290522a9e@linaro.org> On Sun, 3 Jul 2022 at 22:29, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.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@samsung.com> > > - Daniel Mentz <danielmentz@google.com> > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.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
WARNING: multiple messages have this Message-ID (diff)
From: Sam Protsenko <semen.protsenko@linaro.org> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>, Janghyuck Kim <janghyuck.kim@samsung.com>, Cho KyongHo <pullip.cho@samsung.com>, Daniel Mentz <danielmentz@google.com>, Sumit Semwal <sumit.semwal@linaro.org>, iommu@lists.linux-foundation.org, iommu@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] iommu/exynos: Use lookup based approach to access v7 registers Date: Fri, 8 Jul 2022 21:13:06 +0300 [thread overview] Message-ID: <CAPLW+4kc86aOVspBz52vt+uhs8GXDNfekd-7jKhyNjUpi8XwLQ@mail.gmail.com> (raw) In-Reply-To: <925751c3-83e8-02b8-6a8b-549290522a9e@linaro.org> On Sun, 3 Jul 2022 at 22:29, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.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@samsung.com> > > - Daniel Mentz <danielmentz@google.com> > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-07-08 18:13 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-02 21:37 [PATCH 0/4] iommu/exynos: Add basic support for SysMMU v7 Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-02 21:37 ` [PATCH 1/4] iommu/exynos: Set correct dma mask for SysMMU v5+ Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-03 18:50 ` Krzysztof Kozlowski 2022-07-03 18:50 ` Krzysztof Kozlowski 2022-07-03 18:50 ` Krzysztof Kozlowski 2022-07-08 13:18 ` Sam Protsenko 2022-07-08 13:18 ` Sam Protsenko 2022-07-11 12:27 ` Krzysztof Kozlowski 2022-07-11 12:27 ` Krzysztof Kozlowski 2022-07-11 12:59 ` Robin Murphy 2022-07-11 12:59 ` Robin Murphy 2022-07-02 21:37 ` [PATCH 2/4] iommu/exynos: Check if SysMMU v7 has VM registers Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-03 19:10 ` Krzysztof Kozlowski 2022-07-03 19:10 ` Krzysztof Kozlowski 2022-07-03 19:10 ` Krzysztof Kozlowski 2022-07-08 13:34 ` Sam Protsenko 2022-07-08 13:34 ` Sam Protsenko 2022-07-02 21:37 ` [PATCH 3/4] iommu/exynos: Use lookup based approach to access v7 registers Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-03 19:29 ` Krzysztof Kozlowski 2022-07-03 19:29 ` Krzysztof Kozlowski 2022-07-03 19:29 ` Krzysztof Kozlowski 2022-07-08 18:13 ` Sam Protsenko [this message] 2022-07-08 18:13 ` Sam Protsenko 2022-07-02 21:37 ` [PATCH 4/4] iommu/exynos: Add minimal support for SysMMU v7 with VM registers Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-02 21:37 ` Sam Protsenko 2022-07-02 21:48 ` [PATCH 0/4] iommu/exynos: Add basic support for SysMMU v7 Sam Protsenko 2022-07-02 21:48 ` Sam Protsenko 2022-07-02 21:48 ` Sam Protsenko 2022-07-03 12:47 ` David Virag 2022-07-03 12:47 ` David Virag 2022-07-03 12:47 ` David Virag 2022-07-06 14:24 ` Sam Protsenko 2022-07-06 14:24 ` Sam Protsenko 2022-07-06 14:24 ` Sam Protsenko 2022-07-10 23:04 ` Sam Protsenko 2022-07-10 23:04 ` Sam Protsenko 2022-07-13 14:14 ` Sam Protsenko 2022-07-13 14:14 ` Sam Protsenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAPLW+4kc86aOVspBz52vt+uhs8GXDNfekd-7jKhyNjUpi8XwLQ@mail.gmail.com \ --to=semen.protsenko@linaro.org \ --cc=danielmentz@google.com \ --cc=iommu@lists.linux-foundation.org \ --cc=iommu@lists.linux.dev \ --cc=janghyuck.kim@samsung.com \ --cc=joro@8bytes.org \ --cc=krzysztof.kozlowski@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=m.szyprowski@samsung.com \ --cc=pullip.cho@samsung.com \ --cc=robin.murphy@arm.com \ --cc=sumit.semwal@linaro.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.