| Message ID | 44173fd4e8efd27d670cadc6b30e215243a14099.1460391217.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 13, 2016 at 06:13:00PM +0100, Robin Murphy wrote: > Whilst commit 9439eb3ab9d1 ("asm-generic: io: implement relaxed > accessor macros as conditional wrappers") makes the *_relaxed forms of > I/O accessors universally available to drivers, in cases where writeq() > is implemented via the io-64-nonatomic helpers, writeq_relaxed() will > end up falling back to writel() regardless of whether writel_relaxed() > is available (identically for s/write/read/). > > Add corresponding relaxed forms of the nonatomic helpers to delegate > to the equivalent 32-bit accessors as appropriate. > > CC: Arnd Bergmann <arnd@arndb.de> > CC: Christoph Hellwig <hch@lst.de> > CC: Darren Hart <dvhart@linux.intel.com> > CC: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ > include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/include/linux/io-64-nonatomic-hi-lo.h b/include/linux/io-64-nonatomic-hi-lo.h > index 11d7e84..1a85566 100644 > --- a/include/linux/io-64-nonatomic-hi-lo.h > +++ b/include/linux/io-64-nonatomic-hi-lo.h > @@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) > writel(val, addr); > } > > +static inline __u64 hi_lo_readq_relaxed(const volatile void __iomem *addr) > +{ > + const volatile u32 __iomem *p = addr; > + u32 low, high; > + > + high = readl_relaxed(p + 1); > + low = readl_relaxed(p); > + > + return low + ((u64)high << 32); > +} > + > +static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr) > +{ > + writel_relaxed(val >> 32, addr + 4); > + writel_relaxed(val, addr); > +} Could we not generate the _relaxed variants with some macro magic? Will
On 21/04/16 17:18, Will Deacon wrote: > On Wed, Apr 13, 2016 at 06:13:00PM +0100, Robin Murphy wrote: >> Whilst commit 9439eb3ab9d1 ("asm-generic: io: implement relaxed >> accessor macros as conditional wrappers") makes the *_relaxed forms of >> I/O accessors universally available to drivers, in cases where writeq() >> is implemented via the io-64-nonatomic helpers, writeq_relaxed() will >> end up falling back to writel() regardless of whether writel_relaxed() >> is available (identically for s/write/read/). >> >> Add corresponding relaxed forms of the nonatomic helpers to delegate >> to the equivalent 32-bit accessors as appropriate. >> >> CC: Arnd Bergmann <arnd@arndb.de> >> CC: Christoph Hellwig <hch@lst.de> >> CC: Darren Hart <dvhart@linux.intel.com> >> CC: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ >> include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ >> 2 files changed, 50 insertions(+) >> >> diff --git a/include/linux/io-64-nonatomic-hi-lo.h b/include/linux/io-64-nonatomic-hi-lo.h >> index 11d7e84..1a85566 100644 >> --- a/include/linux/io-64-nonatomic-hi-lo.h >> +++ b/include/linux/io-64-nonatomic-hi-lo.h >> @@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) >> writel(val, addr); >> } >> >> +static inline __u64 hi_lo_readq_relaxed(const volatile void __iomem *addr) >> +{ >> + const volatile u32 __iomem *p = addr; >> + u32 low, high; >> + >> + high = readl_relaxed(p + 1); >> + low = readl_relaxed(p); >> + >> + return low + ((u64)high << 32); >> +} >> + >> +static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr) >> +{ >> + writel_relaxed(val >> 32, addr + 4); >> + writel_relaxed(val, addr); >> +} > > Could we not generate the _relaxed variants with some macro magic? We _could_ - indeed I started doing that, but then decided that the obfuscation of horrible macro-templated functions wasn't worth saving a couple of hundred bytes in some code that isn't exactly difficult to maintain and has needed touching once in 4 years. If you did want to go down the macro route, I may as well also generate both lo-hi and hi-lo headers all from a single template, it'd be really clever... <alarm bells> ;) Robin. > > Will >
On Fri, Apr 22, 2016 at 06:08:46PM +0100, Robin Murphy wrote: > On 21/04/16 17:18, Will Deacon wrote: > >On Wed, Apr 13, 2016 at 06:13:00PM +0100, Robin Murphy wrote: > >>Whilst commit 9439eb3ab9d1 ("asm-generic: io: implement relaxed > >>accessor macros as conditional wrappers") makes the *_relaxed forms of > >>I/O accessors universally available to drivers, in cases where writeq() > >>is implemented via the io-64-nonatomic helpers, writeq_relaxed() will > >>end up falling back to writel() regardless of whether writel_relaxed() > >>is available (identically for s/write/read/). > >> > >>Add corresponding relaxed forms of the nonatomic helpers to delegate > >>to the equivalent 32-bit accessors as appropriate. > >> > >>CC: Arnd Bergmann <arnd@arndb.de> > >>CC: Christoph Hellwig <hch@lst.de> > >>CC: Darren Hart <dvhart@linux.intel.com> > >>CC: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > >>Signed-off-by: Robin Murphy <robin.murphy@arm.com> > >>--- > >> include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ > >> include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ > >> 2 files changed, 50 insertions(+) > >> > >>diff --git a/include/linux/io-64-nonatomic-hi-lo.h b/include/linux/io-64-nonatomic-hi-lo.h > >>index 11d7e84..1a85566 100644 > >>--- a/include/linux/io-64-nonatomic-hi-lo.h > >>+++ b/include/linux/io-64-nonatomic-hi-lo.h > >>@@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) > >> writel(val, addr); > >> } > >> > >>+static inline __u64 hi_lo_readq_relaxed(const volatile void __iomem *addr) > >>+{ > >>+ const volatile u32 __iomem *p = addr; > >>+ u32 low, high; > >>+ > >>+ high = readl_relaxed(p + 1); > >>+ low = readl_relaxed(p); > >>+ > >>+ return low + ((u64)high << 32); > >>+} > >>+ > >>+static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr) > >>+{ > >>+ writel_relaxed(val >> 32, addr + 4); > >>+ writel_relaxed(val, addr); > >>+} > > > >Could we not generate the _relaxed variants with some macro magic? > > We _could_ - indeed I started doing that, but then decided that the > obfuscation of horrible macro-templated functions wasn't worth saving a > couple of hundred bytes in some code that isn't exactly difficult to > maintain and has needed touching once in 4 years. > > If you did want to go down the macro route, I may as well also generate both > lo-hi and hi-lo headers all from a single template, it'd be really clever... > <alarm bells> ;) I certainly wasn't suggesting any more than the obvious macroisation, but I'll leave it up to Arnd, as I think this falls on his lap. Will
On Monday 25 April 2016 14:32:42 Will Deacon wrote: > > >> > > >>+static inline __u64 hi_lo_readq_relaxed(const volatile void __iomem *addr) > > >>+{ > > >>+ const volatile u32 __iomem *p = addr; > > >>+ u32 low, high; > > >>+ > > >>+ high = readl_relaxed(p + 1); > > >>+ low = readl_relaxed(p); > > >>+ > > >>+ return low + ((u64)high << 32); > > >>+} > > >>+ > > >>+static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr) > > >>+{ > > >>+ writel_relaxed(val >> 32, addr + 4); > > >>+ writel_relaxed(val, addr); > > >>+} > > > > > >Could we not generate the _relaxed variants with some macro magic? > > > > We _could_ - indeed I started doing that, but then decided that the > > obfuscation of horrible macro-templated functions wasn't worth saving a > > couple of hundred bytes in some code that isn't exactly difficult to > > maintain and has needed touching once in 4 years. > > > > If you did want to go down the macro route, I may as well also generate both > > lo-hi and hi-lo headers all from a single template, it'd be really clever... > > <alarm bells> > > I certainly wasn't suggesting any more than the obvious macroisation, > but I'll leave it up to Arnd, as I think this falls on his lap. I'd prefer the open-coded variant as well. Arnd
Hi Arnd, On 25/04/16 16:21, Arnd Bergmann wrote: > On Monday 25 April 2016 14:32:42 Will Deacon wrote: >>>>> >>>>> +static inline __u64 hi_lo_readq_relaxed(const volatile void __iomem *addr) >>>>> +{ >>>>> + const volatile u32 __iomem *p = addr; >>>>> + u32 low, high; >>>>> + >>>>> + high = readl_relaxed(p + 1); >>>>> + low = readl_relaxed(p); >>>>> + >>>>> + return low + ((u64)high << 32); >>>>> +} >>>>> + >>>>> +static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr) >>>>> +{ >>>>> + writel_relaxed(val >> 32, addr + 4); >>>>> + writel_relaxed(val, addr); >>>>> +} >>>> >>>> Could we not generate the _relaxed variants with some macro magic? >>> >>> We _could_ - indeed I started doing that, but then decided that the >>> obfuscation of horrible macro-templated functions wasn't worth saving a >>> couple of hundred bytes in some code that isn't exactly difficult to >>> maintain and has needed touching once in 4 years. >>> >>> If you did want to go down the macro route, I may as well also generate both >>> lo-hi and hi-lo headers all from a single template, it'd be really clever... >>> <alarm bells> >> >> I certainly wasn't suggesting any more than the obvious macroisation, >> but I'll leave it up to Arnd, as I think this falls on his lap. > > I'd prefer the open-coded variant as well. By that, do you mean sticking with the smmu_writeq() implementation in the driver and dropping this patch, or merging this patch as-is without further macro-magic? Thanks, Robin. > > Arnd >
On Monday 25 April 2016 16:28:01 Robin Murphy wrote: > >>> > >>> We _could_ - indeed I started doing that, but then decided that the > >>> obfuscation of horrible macro-templated functions wasn't worth saving a > >>> couple of hundred bytes in some code that isn't exactly difficult to > >>> maintain and has needed touching once in 4 years. > >>> > >>> If you did want to go down the macro route, I may as well also generate both > >>> lo-hi and hi-lo headers all from a single template, it'd be really clever... > >>> <alarm bells> > >> > >> I certainly wasn't suggesting any more than the obvious macroisation, > >> but I'll leave it up to Arnd, as I think this falls on his lap. > > > > I'd prefer the open-coded variant as well. > > By that, do you mean sticking with the smmu_writeq() implementation in > the driver and dropping this patch, or merging this patch as-is without > further macro-magic? > Sorry, that was really ambiguous on my end. I meant leaving patch 4/7 as it is in the version you posted. However, leaving the open-coded writel_relaxed() in the driver or just using the non-relaxed hi_lo_readq() would be totally fine too. Arnd
On Mon, Apr 25, 2016 at 05:41:21PM +0200, Arnd Bergmann wrote: > On Monday 25 April 2016 16:28:01 Robin Murphy wrote: > > >>> > > >>> We _could_ - indeed I started doing that, but then decided that the > > >>> obfuscation of horrible macro-templated functions wasn't worth saving a > > >>> couple of hundred bytes in some code that isn't exactly difficult to > > >>> maintain and has needed touching once in 4 years. > > >>> > > >>> If you did want to go down the macro route, I may as well also generate both > > >>> lo-hi and hi-lo headers all from a single template, it'd be really clever... > > >>> <alarm bells> > > >> > > >> I certainly wasn't suggesting any more than the obvious macroisation, > > >> but I'll leave it up to Arnd, as I think this falls on his lap. > > > > > > I'd prefer the open-coded variant as well. > > > > By that, do you mean sticking with the smmu_writeq() implementation in > > the driver and dropping this patch, or merging this patch as-is without > > further macro-magic? > > > > Sorry, that was really ambiguous on my end. I meant leaving patch 4/7 > as it is in the version you posted. I'm happy with that. Could I have your ack, so that I can queue it with the related SMMU patches, please? Will
On Wednesday 13 April 2016 18:13:00 Robin Murphy wrote: > Whilst commit 9439eb3ab9d1 ("asm-generic: io: implement relaxed > accessor macros as conditional wrappers") makes the *_relaxed forms of > I/O accessors universally available to drivers, in cases where writeq() > is implemented via the io-64-nonatomic helpers, writeq_relaxed() will > end up falling back to writel() regardless of whether writel_relaxed() > is available (identically for s/write/read/). > > Add corresponding relaxed forms of the nonatomic helpers to delegate > to the equivalent 32-bit accessors as appropriate. > > CC: Arnd Bergmann <arnd@arndb.de> > CC: Christoph Hellwig <hch@lst.de> > CC: Darren Hart <dvhart@linux.intel.com> > CC: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > Acked-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/include/linux/io-64-nonatomic-hi-lo.h b/include/linux/io-64-nonatomic-hi-lo.h index 11d7e84..1a85566 100644 --- a/include/linux/io-64-nonatomic-hi-lo.h +++ b/include/linux/io-64-nonatomic-hi-lo.h @@ -21,6 +21,23 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) writel(val, addr); } +static inline __u64 hi_lo_readq_relaxed(const volatile void __iomem *addr) +{ + const volatile u32 __iomem *p = addr; + u32 low, high; + + high = readl_relaxed(p + 1); + low = readl_relaxed(p); + + return low + ((u64)high << 32); +} + +static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr) +{ + writel_relaxed(val >> 32, addr + 4); + writel_relaxed(val, addr); +} + #ifndef readq #define readq hi_lo_readq #endif @@ -29,4 +46,12 @@ static inline void hi_lo_writeq(__u64 val, volatile void __iomem *addr) #define writeq hi_lo_writeq #endif +#ifndef readq_relaxed +#define readq_relaxed hi_lo_readq_relaxed +#endif + +#ifndef writeq_relaxed +#define writeq hi_lo_writeq_relaxed +#endif + #endif /* _LINUX_IO_64_NONATOMIC_HI_LO_H_ */ diff --git a/include/linux/io-64-nonatomic-lo-hi.h b/include/linux/io-64-nonatomic-lo-hi.h index 1a4315f..1b7411d 100644 --- a/include/linux/io-64-nonatomic-lo-hi.h +++ b/include/linux/io-64-nonatomic-lo-hi.h @@ -21,6 +21,23 @@ static inline void lo_hi_writeq(__u64 val, volatile void __iomem *addr) writel(val >> 32, addr + 4); } +static inline __u64 lo_hi_readq_relaxed(const volatile void __iomem *addr) +{ + const volatile u32 __iomem *p = addr; + u32 low, high; + + low = readl_relaxed(p); + high = readl_relaxed(p + 1); + + return low + ((u64)high << 32); +} + +static inline void lo_hi_writeq_relaxed(__u64 val, volatile void __iomem *addr) +{ + writel_relaxed(val, addr); + writel_relaxed(val >> 32, addr + 4); +} + #ifndef readq #define readq lo_hi_readq #endif @@ -29,4 +46,12 @@ static inline void lo_hi_writeq(__u64 val, volatile void __iomem *addr) #define writeq lo_hi_writeq #endif +#ifndef readq_relaxed +#define readq_relaxed hi_lo_readq_relaxed +#endif + +#ifndef writeq_relaxed +#define writeq hi_lo_writeq_relaxed +#endif + #endif /* _LINUX_IO_64_NONATOMIC_LO_HI_H_ */
Whilst commit 9439eb3ab9d1 ("asm-generic: io: implement relaxed accessor macros as conditional wrappers") makes the *_relaxed forms of I/O accessors universally available to drivers, in cases where writeq() is implemented via the io-64-nonatomic helpers, writeq_relaxed() will end up falling back to writel() regardless of whether writel_relaxed() is available (identically for s/write/read/). Add corresponding relaxed forms of the nonatomic helpers to delegate to the equivalent 32-bit accessors as appropriate. CC: Arnd Bergmann <arnd@arndb.de> CC: Christoph Hellwig <hch@lst.de> CC: Darren Hart <dvhart@linux.intel.com> CC: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- include/linux/io-64-nonatomic-hi-lo.h | 25 +++++++++++++++++++++++++ include/linux/io-64-nonatomic-lo-hi.h | 25 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+)