Messages in this thread | | | Subject | Re: [RFC 09/13] iommu/arm-smmu: Make use of dev_64bit_mmio_supported() | From | Nicolas Saenz Julienne <> | Date | Tue, 02 Mar 2021 13:42:16 +0100 |
| |
Hi Arnd, thanks for the reviews!
On Tue, 2021-03-02 at 10:32 +0100, Arnd Bergmann wrote: > On Fri, Feb 26, 2021 at 3:03 PM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > > if (smmu->impl && unlikely(smmu->impl->write_reg)) > > smmu->impl->write_reg(smmu, page, offset, val); > > - else > > + else if (dev_64bit_mmio_supported(smmu->dev)) > > writel_relaxed(val, arm_smmu_page(smmu, page) + offset); > > + else > > + hi_lo_writeq_relaxed(val, arm_smmu_page(smmu, page) + offset); > > } > > This is a writel_relaxed(), not a writeq_relaxed(), so I suppose you don't > have to change it at all.
Yes, that was silly of me. I was worrying about the semantics of the whole thing, and missed basic stuff like this.
> > + else if (dev_64bit_mmio_supported(smmu->dev)) > > + return readq_relaxed(arm_smmu_page(smmu, page) + offset); > > + else > > + return hi_lo_readq_relaxed(arm_smmu_page(smmu, page) + offset); > > } > > > I see this pattern repeat across multiple drivers. I think Christoph > had originally > suggested folding the if/else logic into the writel_relaxed() that is defined in > include/linux/io-64-nonatomic-hi-lo.h, but of course that doesn't work if you > need to pass a device pointer. > > It might still make sense to have another wrapper in that same file though, > something like > > static inline hi_lo_writeq_relaxed_if_possible(struct device *dev, __u64 val, > volatile void __iomem *addr) > { > if (dev_64bit_mmio_supported(smmu->dev)) { > readq_relaxed(arm_smmu_page(smmu, page) + offset); > } else { > writel_relaxed(val >> 32, addr + 4); > writel_relaxed(val, addr); > } > }
I like the idea. I'll try to integrate it into the next revision.
Regards, Nicolas
[unhandled content-type:application/pgp-signature] | |