On Tue, 12 Jul 2022 at 20:00, Robin Murphy <robin.murphy@xxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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]
