* [PATCH] arm/io.h: add macros to read/write big/little endian register @ 2012-02-23 9:17 Viresh Kumar 2012-02-23 9:53 ` Russell King - ARM Linux 0 siblings, 1 reply; 17+ messages in thread From: Viresh Kumar @ 2012-02-23 9:17 UTC (permalink / raw) To: linux-arm-kernel From: Pratyush Anand <pratyush.anand@st.com> There are some peripheral (e.g dwc otg) whose registers can be configured to work in either little or big endian mode. Therefore macros like out_be32, in_be32, out_le32 and in_le32 have been added to support such peripherals. Signed-off-by: Pratyush Anand <pratyush.anand@st.com> Signed-off-by: Viresh Kumar <viresh.kumar@st.com> --- arch/arm/include/asm/io.h | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 9275828..a1ccac0 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -192,6 +192,16 @@ static inline void __iomem *__typesafe_io(unsigned long addr) #define insw_p(port,to,len) insw(port,to,len) #define insl_p(port,to,len) insl(port,to,len) +/* Big Endian */ +#define out_be32(v, p) ({ __iowmb(); __raw_writel((__force __u32) \ + cpu_to_be32(v), __io(p)); }) +#define in_be32(p) ({ __u32 __v = be32_to_cpu((__force __be32) \ + __raw_readl(__io(p))); __iormb(); __v; }) + +/* Little endian */ +#define out_le32(v, p) outl(v, p) +#define in_le32(p) inl(p) + /* * String version of IO memory access ops: */ -- 1.7.8.110.g4cb5d ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 9:17 [PATCH] arm/io.h: add macros to read/write big/little endian register Viresh Kumar @ 2012-02-23 9:53 ` Russell King - ARM Linux 2012-02-23 10:53 ` Viresh Kumar 0 siblings, 1 reply; 17+ messages in thread From: Russell King - ARM Linux @ 2012-02-23 9:53 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 23, 2012 at 02:47:40PM +0530, Viresh Kumar wrote: > From: Pratyush Anand <pratyush.anand@st.com> > > There are some peripheral (e.g dwc otg) whose registers can be configured to > work in either little or big endian mode. Therefore macros like out_be32, > in_be32, out_le32 and in_le32 have been added to support such peripherals. NAK. 1. Using the PCI/ISA IO macros for non-PCI/ISA IO purposes is silly 2. We should have readb_be() etc instead. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 9:53 ` Russell King - ARM Linux @ 2012-02-23 10:53 ` Viresh Kumar 2012-02-23 11:19 ` Russell King - ARM Linux 0 siblings, 1 reply; 17+ messages in thread From: Viresh Kumar @ 2012-02-23 10:53 UTC (permalink / raw) To: linux-arm-kernel On 2/23/2012 3:23 PM, Russell King - ARM Linux wrote: > 1. Using the PCI/ISA IO macros for non-PCI/ISA IO purposes is silly We mistakenly placed it outside #ifdef __io, #endif Sorry, i am still missing your point. > 2. We should have readb_be() etc instead. Actually, existing drivers are using out_be32(), etc in their implementation. What do you suggest in order to use these drivers for SPEAr/ARM. -- viresh ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 10:53 ` Viresh Kumar @ 2012-02-23 11:19 ` Russell King - ARM Linux 2012-02-23 11:30 ` Russell King - ARM Linux 2012-02-23 11:34 ` Pratyush Anand 0 siblings, 2 replies; 17+ messages in thread From: Russell King - ARM Linux @ 2012-02-23 11:19 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 23, 2012 at 04:23:11PM +0530, Viresh Kumar wrote: > On 2/23/2012 3:23 PM, Russell King - ARM Linux wrote: > > 1. Using the PCI/ISA IO macros for non-PCI/ISA IO purposes is silly > > We mistakenly placed it outside #ifdef __io, #endif > Sorry, i am still missing your point. inb() et.al. are for PCI/ISA IO, not for general platform MMIO. > > 2. We should have readb_be() etc instead. > > Actually, existing drivers are using out_be32(), etc in their implementation. > What do you suggest in order to use these drivers for SPEAr/ARM. Which drivers? ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 11:19 ` Russell King - ARM Linux @ 2012-02-23 11:30 ` Russell King - ARM Linux 2012-02-23 11:34 ` Pratyush Anand 1 sibling, 0 replies; 17+ messages in thread From: Russell King - ARM Linux @ 2012-02-23 11:30 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 23, 2012 at 11:19:06AM +0000, Russell King - ARM Linux wrote: > On Thu, Feb 23, 2012 at 04:23:11PM +0530, Viresh Kumar wrote: > > On 2/23/2012 3:23 PM, Russell King - ARM Linux wrote: > > > 1. Using the PCI/ISA IO macros for non-PCI/ISA IO purposes is silly > > > > We mistakenly placed it outside #ifdef __io, #endif > > Sorry, i am still missing your point. > > inb() et.al. are for PCI/ISA IO, not for general platform MMIO. > > > > 2. We should have readb_be() etc instead. > > > > Actually, existing drivers are using out_be32(), etc in their implementation. > > What do you suggest in order to use these drivers for SPEAr/ARM. > > Which drivers? I see, some PPC folk allowed their private IO accessors to spread into generic drivers despite a comment in their io.h saying that they shouldn't. The right solution is that this crap needs to get fixed, rather than spreading around yet another IO accessor which was supposed to be private to an arch. I've mailed the PPC folk about it. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 11:19 ` Russell King - ARM Linux 2012-02-23 11:30 ` Russell King - ARM Linux @ 2012-02-23 11:34 ` Pratyush Anand 2012-02-23 11:38 ` Russell King - ARM Linux 2012-02-23 12:01 ` Stefan Roese 1 sibling, 2 replies; 17+ messages in thread From: Pratyush Anand @ 2012-02-23 11:34 UTC (permalink / raw) To: linux-arm-kernel On 2/23/2012 4:49 PM, Russell King - ARM Linux wrote: > On Thu, Feb 23, 2012 at 04:23:11PM +0530, Viresh Kumar wrote: >> On 2/23/2012 3:23 PM, Russell King - ARM Linux wrote: >>> 1. Using the PCI/ISA IO macros for non-PCI/ISA IO purposes is silly >> >> We mistakenly placed it outside #ifdef __io, #endif >> Sorry, i am still missing your point. > > inb() et.al. are for PCI/ISA IO, not for general platform MMIO. > Ok.. So, will it be fine if we keep it outside #ifdef __io and do not use __io macro in their implementation? >>> 2. We should have readb_be() etc instead. >> >> Actually, existing drivers are using out_be32(), etc in their implementation. >> What do you suggest in order to use these drivers for SPEAr/ARM. > > Which drivers? > . > We have used these macros in dwc_otg driver, which is still to be added in the main line , but is under discussion. http://comments.gmane.org/gmane.linux.usb.general/53348 However, I do see that these macros have been used in several mainline driver also, e.g. drivers/usb/gadget/fsl_qe_udc.c. Regards Pratyush ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 11:34 ` Pratyush Anand @ 2012-02-23 11:38 ` Russell King - ARM Linux 2012-02-23 12:33 ` Arnd Bergmann 2012-02-23 12:01 ` Stefan Roese 1 sibling, 1 reply; 17+ messages in thread From: Russell King - ARM Linux @ 2012-02-23 11:38 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 23, 2012 at 05:04:28PM +0530, Pratyush Anand wrote: > On 2/23/2012 4:49 PM, Russell King - ARM Linux wrote: >> On Thu, Feb 23, 2012 at 04:23:11PM +0530, Viresh Kumar wrote: >>> On 2/23/2012 3:23 PM, Russell King - ARM Linux wrote: >>>> 1. Using the PCI/ISA IO macros for non-PCI/ISA IO purposes is silly >>> >>> We mistakenly placed it outside #ifdef __io, #endif >>> Sorry, i am still missing your point. >> >> inb() et.al. are for PCI/ISA IO, not for general platform MMIO. >> > > Ok.. So, will it be fine if we keep it outside #ifdef __io and do not > use __io macro in their implementation? No, I refuse to add another set of accessors to ARMs io.h. They're private to PPC - it even says so in their header file. They should not have leaked out of PPC. The fact that 73 drivers in the kernel source use their accessors is their problem to solve. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 11:38 ` Russell King - ARM Linux @ 2012-02-23 12:33 ` Arnd Bergmann 2012-02-23 12:50 ` Russell King - ARM Linux 2012-02-23 20:25 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 17+ messages in thread From: Arnd Bergmann @ 2012-02-23 12:33 UTC (permalink / raw) To: linux-arm-kernel On Thursday 23 February 2012, Russell King - ARM Linux wrote: > On Thu, Feb 23, 2012 at 05:04:28PM +0530, Pratyush Anand wrote: > > On 2/23/2012 4:49 PM, Russell King - ARM Linux wrote: > >> On Thu, Feb 23, 2012 at 04:23:11PM +0530, Viresh Kumar wrote: > >>> On 2/23/2012 3:23 PM, Russell King - ARM Linux wrote: > >>>> 1. Using the PCI/ISA IO macros for non-PCI/ISA IO purposes is silly > >>> > >>> We mistakenly placed it outside #ifdef __io, #endif > >>> Sorry, i am still missing your point. > >> > >> inb() et.al. are for PCI/ISA IO, not for general platform MMIO. > >> > > > > Ok.. So, will it be fine if we keep it outside #ifdef __io and do not > > use __io macro in their implementation? It would be technically correct unlike an implementation based on inb/outb, the question is whether it would be a good solution for the problem at hand. We've had too many ad-hoc fixes in this area leading us to the mess we're in now, so we should be very careful about getting it right this time. > No, I refuse to add another set of accessors to ARMs io.h. They're private > to PPC - it even says so in their header file. They should not have leaked > out of PPC. > > The fact that 73 drivers in the kernel source use their accessors is their > problem to solve. The reason why for ppc to have introduced these is the need for accessors that work for on-chip devices on machines where the PCI readl/writel and ioread32/iowrite32 do something funny like using an indirect register or a hypercall. We have a similar problem on ARM with CONFIG_IXP4XX_INDIRECT_PCI and possibly the new highbank platforms that might be doing the same thing in order to get the most out of the 4GB address space. The mistake on ppc was to make them private to one architecture instead of coming up with a well-defined set of accessors that are provided on all architectures. We certainly have too many accessors already between inl/readl/ioread32/__raw_readl/readl_relaxed/in_le32, which makes this rather confusing, but IMHO there is still the need for cleanup and addition of missing variants like cpu-endian non-relaxed and big-endian relaxed as well as a replacement for __raw_* that actually defines them to something useful across architectures. In case of the dgw otg driver, we can add a simple hack like #if defined(CONFIG_PPC) || defined(CONFIG_MICROBLAZE) || defined(CONFIG_M68K) #define dwg_readl(x) in_le32(x) #else #define dwg_readl(x) readl(x) #endif But there is a danger of spreading such hacks everywhere across the kernel and making the mess bigger. I think a better direction would be to define one set of accessors across architectures that can be safely used in this driver. The choices that I can see are: 1. define the {in,out}_{le,be}{8,16,32,64} accessors for *all* architectures following the ppc semantics (fixed endianess, strict ordering, __iomem but not PCI). On ARM, this would be the same as the readl() family for all platforms that have a memory mapped PCI memory space. 2. change the ppc definition of the readl_relaxed() family to something useful (fixed endian, relaxed ordering, iomem (possibly including PCI -- same as in_le32 but without the ordering guarantee) and make any driver that wants to be portable use those accessors with explicit I/O barriers on spinlocks and DMA. 3. Add a new set of consistent accessors that are explicitly meant for non-PCI on-chip components and make sure we really get them right this time and can retire some of the other ones eventually. This means it needs to be completely arch independent, include relaxed and strict accesses, little-endian/cpu-endian/native-endian and a well-documented address space (or more than one). Arnd ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 12:33 ` Arnd Bergmann @ 2012-02-23 12:50 ` Russell King - ARM Linux 2012-02-23 13:35 ` Arnd Bergmann 2012-02-23 20:25 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 17+ messages in thread From: Russell King - ARM Linux @ 2012-02-23 12:50 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 23, 2012 at 12:33:04PM +0000, Arnd Bergmann wrote: > 3. Add a new set of consistent accessors that are explicitly meant for > non-PCI on-chip components and make sure we really get them right this > time and can retire some of the other ones eventually. This means it > needs to be completely arch independent, include relaxed and strict > accesses, little-endian/cpu-endian/native-endian and a well-documented > address space (or more than one). That won't work. What's behind PCI in one area is on-chip on another. See that with various PXA peripherals appearing behind PCI devices on x86. That's also happening with AMBA peripherals as well. We *really* need to get away from that "PCI is special and has its own accessors" or "on chip is special and has its own accessors" madness. That's already starting to bite as being wrong. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 12:50 ` Russell King - ARM Linux @ 2012-02-23 13:35 ` Arnd Bergmann 2012-02-23 14:14 ` Russell King - ARM Linux 2012-02-23 20:27 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 17+ messages in thread From: Arnd Bergmann @ 2012-02-23 13:35 UTC (permalink / raw) To: linux-arm-kernel On Thursday 23 February 2012, Russell King - ARM Linux wrote: > On Thu, Feb 23, 2012 at 12:33:04PM +0000, Arnd Bergmann wrote: > > 3. Add a new set of consistent accessors that are explicitly meant for > > non-PCI on-chip components and make sure we really get them right this > > time and can retire some of the other ones eventually. This means it > > needs to be completely arch independent, include relaxed and strict > > accesses, little-endian/cpu-endian/native-endian and a well-documented > > address space (or more than one). > > That won't work. What's behind PCI in one area is on-chip on another. > See that with various PXA peripherals appearing behind PCI devices on > x86. That's also happening with AMBA peripherals as well. > > We really need to get away from that "PCI is special and has its own > accessors" or "on chip is special and has its own accessors" madness. > That's already starting to bite as being wrong. This would mean going through an indirect function call for platforms where PCI is fundamentally different in hardware. We already do this for CONFIG_PPC_INDIRECT_PCI, but I'd hope we can avoid this on ARM. Since devices that can bei either PCI or not are relatively rare, I'd rather put them in the same category as those that can either use PCI I/O space or some form of memory space and require that the drivers use the "I don't care what you are, just do what I want" accessors in the ioread/iowrite family that are already going through an indirect function call on most architectures. Also, the PCI aware readl/writel functions are already more heavyweight on most architectures (not on x86 though) than we want them to be, because they have to do extra synchronizing operations against DMA and/or spinlocks on weakly ordered architectures. We now have the readl_relaxed family here, but I believe they are only used for on-chip components at the moment. Arnd ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 13:35 ` Arnd Bergmann @ 2012-02-23 14:14 ` Russell King - ARM Linux 2012-02-23 20:27 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 17+ messages in thread From: Russell King - ARM Linux @ 2012-02-23 14:14 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 23, 2012 at 01:35:04PM +0000, Arnd Bergmann wrote: > Since devices that can bei either PCI or not are relatively rare, I'd > rather put them in the same category as those that can either use > PCI I/O space or some form of memory space and require that the > drivers use the "I don't care what you are, just do what I want" > accessors in the ioread/iowrite family that are already going through > an indirect function call on most architectures. It's not a case of whether they themselves are PCI or not. It's a case that manufacturers are starting to take existing ARM IP and sticking a PCI bridge in front of them. That's why there's someone looking at a set of PCI drivers which create AMBA primecell devices to allow the existing primecell drivers to bind to these peripherals. What that means is that in theory, you could have an AMBA primecell peripheral behind a PCI device on an IXP4xx platform with its weird windows, and that would necessitate the AMBA primecell drivers to use the special PCI space accessors. I would not be surprised if some of these PPC freescale peripheral IPs using their private in_xxx() accessors end up in the same situation. That seems to be the direction the hardware folk are going. So, I think it's a big mistake to distinguish bus types by their accessors. Or put it another way, it's a big mistake to have bus specific accessors. Consider the AMBA primecell behind a PCI device, and if AMBA had its own bus specific accessors. Should we litter the drivers with ifdefs to select between the PCI accessors or the AMBA accessors? Or something equally horrible? ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 13:35 ` Arnd Bergmann 2012-02-23 14:14 ` Russell King - ARM Linux @ 2012-02-23 20:27 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 17+ messages in thread From: Benjamin Herrenschmidt @ 2012-02-23 20:27 UTC (permalink / raw) To: linux-arm-kernel On Thu, 2012-02-23 at 13:35 +0000, Arnd Bergmann wrote: > > This would mean going through an indirect function call for platforms > where PCI is fundamentally different in hardware. We already do this > for CONFIG_PPC_INDIRECT_PCI, but I'd hope we can avoid this on ARM. On PPC we do it a bit more sneakily using a bit in the address. It causes the inline accessor to be a big bigger but avoids the cost of the indirect call for the "normal" case. > Since devices that can bei either PCI or not are relatively rare, I'd > rather put them in the same category as those that can either use > PCI I/O space or some form of memory space and require that the > drivers use the "I don't care what you are, just do what I want" > accessors in the ioread/iowrite family that are already going through > an indirect function call on most architectures. > > Also, the PCI aware readl/writel functions are already more > heavyweight > on most architectures (not on x86 though) than we want them to be, > because they have to do extra synchronizing operations against DMA > and/or spinlocks on weakly ordered architectures. We now have > the readl_relaxed family here, but I believe they are only used > for on-chip components at the moment. On-chip have the same issue vs. DMA and we wouldn't want the defaults to let them leak out of locks etc... Cheers, Ben. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 12:33 ` Arnd Bergmann 2012-02-23 12:50 ` Russell King - ARM Linux @ 2012-02-23 20:25 ` Benjamin Herrenschmidt 2012-02-24 16:22 ` Arnd Bergmann 1 sibling, 1 reply; 17+ messages in thread From: Benjamin Herrenschmidt @ 2012-02-23 20:25 UTC (permalink / raw) To: linux-arm-kernel > The reason why for ppc to have introduced these is the need for accessors > that work for on-chip devices on machines where the PCI readl/writel and > ioread32/iowrite32 do something funny like using an indirect register > or a hypercall. We have a similar problem on ARM with > CONFIG_IXP4XX_INDIRECT_PCI and possibly the new highbank platforms > that might be doing the same thing in order to get the most out of the > 4GB address space. Well, the "generic" ones can do the workarounds conditionally based on bits of the address. However it does add overhead to SoC drivers that don't need it. That's indeed one of the reasons for some drivers to use our specific ones. > The mistake on ppc was to make them private to one architecture instead of > coming up with a well-defined set of accessors that are provided on all > architectures. That was never going to fly, we already have too many. > We certainly have too many accessors already between > inl/readl/ioread32/__raw_readl/readl_relaxed/in_le32, which makes this > rather confusing, but IMHO there is still the need for cleanup and > addition of missing variants like cpu-endian non-relaxed and > big-endian relaxed as well as a replacement for __raw_* that actually > defines them to something useful across architectures. Right, the _relaxed don't have clear semantics and we do need "real" relaxed ones badly as in "don't have memory barriers". Currently, a driver that wants to avoid the sync's we have all over the place in our accessors for performance reasons has to use __raw_* and thus handle endian manually as well, which sucks since it also cannot use the reverse-load/store instructions in that case. I tried to propose a uniform set a while back, but that never went anywhere, I should have fought harder. You are welcome to bring that back on the table :-) > In case of the dgw otg driver, we can add a simple hack like > > #if defined(CONFIG_PPC) || defined(CONFIG_MICROBLAZE) || defined(CONFIG_M68K) > #define dwg_readl(x) in_le32(x) > #else > #define dwg_readl(x) readl(x) > #endif > > But there is a danger of spreading such hacks everywhere across the > kernel and making the mess bigger. I think a better direction would be > to define one set of accessors across architectures that can be > safely used in this driver. The choices that I can see are: Yes. We used to have that sort of hacks in the early days as well, I'd rather not go back down that path. > 1. define the {in,out}_{le,be}{8,16,32,64} accessors for *all* architectures > following the ppc semantics (fixed endianess, strict ordering, __iomem but > not PCI). On ARM, this would be the same as the readl() family for all > platforms that have a memory mapped PCI memory space. I'm sure there's going to be resistance here. A better option would be to properly define __readX/__writeX or something like that (maybe using iomap variants) as having no built-in PCI specific workarounds. That still leaves us with the need for properly defined relaxed ones. > 2. change the ppc definition of the readl_relaxed() family to something > useful (fixed endian, relaxed ordering, iomem (possibly including PCI > -- same as in_le32 but without the ordering guarantee) and make any > driver that wants to be portable use those accessors with explicit I/O > barriers on spinlocks and DMA. Last I looked, __relaxed() had a completely different meaning (relaxed ordering -on-the-bus- which is a totally different thing, so iirc only one arch ever implemented it). > 3. Add a new set of consistent accessors that are explicitly meant for > non-PCI on-chip components and make sure we really get them right this > time and can retire some of the other ones eventually. This means it > needs to be completely arch independent, include relaxed and strict > accesses, little-endian/cpu-endian/native-endian and a well-documented I don't mind either, it's trivial for us to add them, it's all macro generated anyway :-) Cheers, Ben. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 20:25 ` Benjamin Herrenschmidt @ 2012-02-24 16:22 ` Arnd Bergmann 2012-02-24 21:03 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 17+ messages in thread From: Arnd Bergmann @ 2012-02-24 16:22 UTC (permalink / raw) To: linux-arm-kernel On Thursday 23 February 2012, Benjamin Herrenschmidt wrote: > > The mistake on ppc was to make them private to one architecture instead of > > coming up with a well-defined set of accessors that are provided on all > > architectures. > > That was never going to fly, we already have too many. It's certainly not easy to get consensus across arch maintainers, especially for a topic that is so complex as this one. On the other hand, we did add the readl_relaxed family just recently to all architectures, and I think it was good to do it this way. > > We certainly have too many accessors already between > > inl/readl/ioread32/__raw_readl/readl_relaxed/in_le32, which makes this > > rather confusing, but IMHO there is still the need for cleanup and > > addition of missing variants like cpu-endian non-relaxed and > > big-endian relaxed as well as a replacement for __raw_* that actually > > defines them to something useful across architectures. > > Right, the _relaxed don't have clear semantics and we do need "real" > relaxed ones badly as in "don't have memory barriers". Currently, a > driver that wants to avoid the sync's we have all over the place in our > accessors for performance reasons has to use __raw_* and thus handle > endian manually as well, which sucks since it also cannot use the > reverse-load/store instructions in that case. Using __raw_* is actually worse since it's not really well-defined what that means across architectures. We recently had a bug where gcc would create byte-wise accesses on registers that are marked "packed" by a driver writer, because the __raw_writel function just does a cast to a variable of larger alignment, which is undefined (IIRC, but maybe just implementation defined) in C. > I tried to propose a uniform set a while back, but that never went > anywhere, I should have fought harder. You are welcome to bring that > back on the table :-) Since the _relaxed version comes from ARM, it's basically defined to be convenient there, which means it is synchronized against the instruction stream (spinlock, most importantly) but not against DMA, because that tends to be expensive. > > 2. change the ppc definition of the readl_relaxed() family to something > > useful (fixed endian, relaxed ordering, iomem (possibly including PCI > > -- same as in_le32 but without the ordering guarantee) and make any > > driver that wants to be portable use those accessors with explicit I/O > > barriers on spinlocks and DMA. > > Last I looked, __relaxed() had a completely different meaning (relaxed > ordering -on-the-bus- which is a totally different thing, so iirc only > one arch ever implemented it). I think you may be confusing it with relaxed ordering on DMA. Having MMIO with relaxed ordering between the accesses would be silly. The only reason we have it on ARM is to remove the need to do an expensive synchronization against a DMA operation on the same bus. IIRC that would let you do an "eieio" instead of a full "sync" on powerpc but should only be used in drivers that don't do DMA. Arnd ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-24 16:22 ` Arnd Bergmann @ 2012-02-24 21:03 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 17+ messages in thread From: Benjamin Herrenschmidt @ 2012-02-24 21:03 UTC (permalink / raw) To: linux-arm-kernel On Fri, 2012-02-24 at 16:22 +0000, Arnd Bergmann wrote: > > It's certainly not easy to get consensus across arch maintainers, especially > for a topic that is so complex as this one. On the other hand, we did add > the readl_relaxed family just recently to all architectures, and I think > it was good to do it this way. Well, nobody added _relaxed ones to powerpc (at least not meaningful ones) nor do I remember being CCed on the patch going in :-) In any case, see below. > Using __raw_* is actually worse since it's not really well-defined > what that means across architectures. We recently had a bug where > gcc would create byte-wise accesses on registers that are marked > "packed" by a driver writer, because the __raw_writel function just > does a cast to a variable of larger alignment, which is undefined > (IIRC, but maybe just implementation defined) in C. Right, __raw_* is basically useless as an abstraction. > > I tried to propose a uniform set a while back, but that never went > > anywhere, I should have fought harder. You are welcome to bring that > > back on the table :-) > > Since the _relaxed version comes from ARM, it's basically defined > to be convenient there, which means it is synchronized against the > instruction stream (spinlock, most importantly) but not against > DMA, because that tends to be expensive. Argh. Crap. And why did you synchronize it with locks ? Seriously, synchronizing vs. instruction stream is horribly costly for IO and generally non-sensical. (The sync. vs a lock isn't even vs. instruction stream, it's also a storage ordering issue). Anyways, if you require the semantic of synchronizing against locks then power will have to keep sync's in there which totally defeats the purpose for us :-( Was that ever discussed with CC on the most likely to be affected archs such as us ? Looks like not... > > > 2. change the ppc definition of the readl_relaxed() family to something > > > useful (fixed endian, relaxed ordering, iomem (possibly including PCI > > > -- same as in_le32 but without the ordering guarantee) and make any > > > driver that wants to be portable use those accessors with explicit I/O > > > barriers on spinlocks and DMA. > > > > Last I looked, __relaxed() had a completely different meaning (relaxed > > ordering -on-the-bus- which is a totally different thing, so iirc only > > one arch ever implemented it). > > I think you may be confusing it with relaxed ordering on DMA. Having > MMIO with relaxed ordering between the accesses would be silly. Well, my memory might be blurry, but back when I tried to get that sorted at least one arch (and it wasn't ARM from memory) had _relaxed accessors and the meaning of _relaxed was ... something else. Really. Unless I was lied to :-) > only reason we have it on ARM is to remove the need to do an expensive > synchronization against a DMA operation on the same bus. IIRC that > would let you do an "eieio" instead of a full "sync" on powerpc but > should only be used in drivers that don't do DMA. That wouldn't solve the problem with locks. Besides, we'd want "relaxed" to not have eieio either wouldn't we ? Cheers, Ben. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 11:34 ` Pratyush Anand 2012-02-23 11:38 ` Russell King - ARM Linux @ 2012-02-23 12:01 ` Stefan Roese 2012-02-24 4:22 ` Pratyush Anand 1 sibling, 1 reply; 17+ messages in thread From: Stefan Roese @ 2012-02-23 12:01 UTC (permalink / raw) To: linux-arm-kernel Hi Pratyush, On Thursday 23 February 2012 12:34:28 Pratyush Anand wrote: > >> Actually, existing drivers are using out_be32(), etc in their > >> implementation. What do you suggest in order to use these drivers for > >> SPEAr/ARM. > > > > Which drivers? > > . > > We have used these macros in dwc_otg driver, which is still to be added > in the main line , but is under discussion. > > http://comments.gmane.org/gmane.linux.usb.general/53348 Did you notice this recent thread on the USB devel list? http://www.spinics.net/lists/linux-usb/msg58420.html I just wanted to make sure, that you don't continue working on this big and ugly dwc_otg driver (yes, I worked on it as well), when it has small to zero chances for upstreaming. Better to consolidate all efforts on this "smaller" driver with the other SoC vendors (Samsung etc). Thanks, Stefan ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] arm/io.h: add macros to read/write big/little endian register 2012-02-23 12:01 ` Stefan Roese @ 2012-02-24 4:22 ` Pratyush Anand 0 siblings, 0 replies; 17+ messages in thread From: Pratyush Anand @ 2012-02-24 4:22 UTC (permalink / raw) To: linux-arm-kernel On 2/23/2012 5:31 PM, Stefan Roese wrote: > Did you notice this recent thread on the USB devel list? > > http://www.spinics.net/lists/linux-usb/msg58420.html > > I just wanted to make sure, that you don't continue working on this big and > ugly dwc_otg driver (yes, I worked on it as well), when it has small to zero > chances for upstreaming. Better to consolidate all efforts on this "smaller" > driver with the other SoC vendors (Samsung etc). Nice to know that a driver exists in main line under different name. Will discuss about it on linux-usb mailing list. Thanks & Regards Pratyush ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-02-24 21:03 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-23 9:17 [PATCH] arm/io.h: add macros to read/write big/little endian register Viresh Kumar 2012-02-23 9:53 ` Russell King - ARM Linux 2012-02-23 10:53 ` Viresh Kumar 2012-02-23 11:19 ` Russell King - ARM Linux 2012-02-23 11:30 ` Russell King - ARM Linux 2012-02-23 11:34 ` Pratyush Anand 2012-02-23 11:38 ` Russell King - ARM Linux 2012-02-23 12:33 ` Arnd Bergmann 2012-02-23 12:50 ` Russell King - ARM Linux 2012-02-23 13:35 ` Arnd Bergmann 2012-02-23 14:14 ` Russell King - ARM Linux 2012-02-23 20:27 ` Benjamin Herrenschmidt 2012-02-23 20:25 ` Benjamin Herrenschmidt 2012-02-24 16:22 ` Arnd Bergmann 2012-02-24 21:03 ` Benjamin Herrenschmidt 2012-02-23 12:01 ` Stefan Roese 2012-02-24 4:22 ` Pratyush Anand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).