| Message ID | 20110530084307.GA28953@riccoc20.at.omicron.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Richard Cochran <richardcochran@gmail.com> writes: > Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > clocksource: convert ARM 32-bit up counting clocksources > > broke the build for ixp4xx and made big endian operation impossible. > This commit restores the original behaviour. Thanks. I can't ATM do anything Linux-related but will do ASAP. Two-three weeks I hope.
On Wed, Jun 01, 2011 at 05:08:59PM +0200, Krzysztof Halasa wrote: > Richard Cochran <richardcochran@gmail.com> writes: > > > Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > > > clocksource: convert ARM 32-bit up counting clocksources > > > > broke the build for ixp4xx and made big endian operation impossible. > > This commit restores the original behaviour. > > Thanks. > > I can't ATM do anything Linux-related but will do ASAP. Two-three weeks > I hope. I don't complain that you have a lack of time for this issue. Still, it is a serious bug for IXP, and it should be fixed in the 3.0 release. Russell, can you please, please take a look? Thanks, Richard
On Mon, May 30, 2011 at 10:43:07AM +0200, Richard Cochran wrote: > Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > clocksource: convert ARM 32-bit up counting clocksources > > broke the build for ixp4xx and made big endian operation impossible. > This commit restores the original behaviour. I'm really not happy about using the MMIO clocksource stuff with random other read functions like this - it defeats the entire purpose of the MMIO clocksource stuff. Maybe we should just undo the change for IXP4xx and treat it as "special" for the time being. Thomas - do you have any other views? > > Signed-off-by: Richard Cochran <richard.cochran@omicron.at> > --- > arch/arm/mach-ixp4xx/common.c | 10 ++++++++-- > 1 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c > index 74ed81a..0777257 100644 > --- a/arch/arm/mach-ixp4xx/common.c > +++ b/arch/arm/mach-ixp4xx/common.c > @@ -419,14 +419,20 @@ static void notrace ixp4xx_update_sched_clock(void) > /* > * clocksource > */ > + > +static cycle_t ixp4xx_clocksource_read(struct clocksource *c) > +{ > + return *IXP4XX_OSTS; > +} > + > unsigned long ixp4xx_timer_freq = IXP4XX_TIMER_FREQ; > EXPORT_SYMBOL(ixp4xx_timer_freq); > static void __init ixp4xx_clocksource_init(void) > { > init_sched_clock(&cd, ixp4xx_update_sched_clock, 32, ixp4xx_timer_freq); > > - clocksource_mmio_init(&IXP4XX_OSTS, "OSTS", ixp4xx_timer_freq, 200, 32, > - clocksource_mmio_readl_up); > + clocksource_mmio_init(NULL, "OSTS", ixp4xx_timer_freq, 200, 32, > + ixp4xx_clocksource_read); > } > > /* > -- > 1.7.0.4 >
Russell, On Wed, 1 Jun 2011, Russell King - ARM Linux wrote: > On Mon, May 30, 2011 at 10:43:07AM +0200, Richard Cochran wrote: > > Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > > > clocksource: convert ARM 32-bit up counting clocksources > > > > broke the build for ixp4xx and made big endian operation impossible. > > This commit restores the original behaviour. > > I'm really not happy about using the MMIO clocksource stuff with random > other read functions like this - it defeats the entire purpose of the > MMIO clocksource stuff. Maybe we should just undo the change for IXP4xx > and treat it as "special" for the time being. > > Thomas - do you have any other views? I have no objections to have special cased read functions as long as all the other copied code is gone. We have the same problem with the generic irq chip and I did not come up with a good decision function where to draw the line. As for everything we come up with in the consolidation space we need to apply common sense and keep an eye on the real abusers. Though in the mmio clocksource case we might ask the question whether read[l|w]() is really necessary in the generic implemetation or not. [too tired to answer that now ] Thanks, tglx
Richard Cochran <richardcochran@gmail.com> writes: > I don't complain that you have a lack of time for this issue. Still, > it is a serious bug for IXP, and it should be fixed in the 3.0 > release. Sure, shouldn't be a problem though.
On Wed, Jun 01, 2011 at 11:58:26PM +0100, Russell King - ARM Linux wrote: > On Mon, May 30, 2011 at 10:43:07AM +0200, Richard Cochran wrote: > > Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > > > clocksource: convert ARM 32-bit up counting clocksources > > > > broke the build for ixp4xx and made big endian operation impossible. > > This commit restores the original behaviour. > > I'm really not happy about using the MMIO clocksource stuff with random > other read functions like this - it defeats the entire purpose of the > MMIO clocksource stuff. Maybe we should just undo the change for IXP4xx > and treat it as "special" for the time being. I took another look at the whole MMIO clocksource implementation and can't understand why all of the accessors use le_to_cpu. This seems to imply that all peripheral buses are little endian (except for ixp, which no one cares about, anyhow). I understand (or have been told) that almost all arm implementations out there are little endian. Why not use use a normal load to read the timer? Are there machines with LE peripheral buses but BE cores? Thanks, Richard
Richard Cochran writes: > On Wed, Jun 01, 2011 at 11:58:26PM +0100, Russell King - ARM Linux wrote: > > On Mon, May 30, 2011 at 10:43:07AM +0200, Richard Cochran wrote: > > > Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > > > > > clocksource: convert ARM 32-bit up counting clocksources > > > > > > broke the build for ixp4xx and made big endian operation impossible. > > > This commit restores the original behaviour. > > > > I'm really not happy about using the MMIO clocksource stuff with random > > other read functions like this - it defeats the entire purpose of the > > MMIO clocksource stuff. Maybe we should just undo the change for IXP4xx > > and treat it as "special" for the time being. > > I took another look at the whole MMIO clocksource implementation and > can't understand why all of the accessors use le_to_cpu. This seems to > imply that all peripheral buses are little endian (except for ixp, > which no one cares about, anyhow). "no one"? ixp4xx is still being used. > I understand (or have been told) that almost all arm implementations > out there are little endian. Why not use use a normal load to read the > timer? Are there machines with LE peripheral buses but BE cores? ARM cores often have software-controllable endianess (see the ARM ARM). Off the top of my head: - ixp4xx: natively BE core and peripherals, but some people like to switch the core to LE for user-space SW compatibility reasons (I run my ixp4xx in BE as intended though) - Kirkwood: natively LE core and peripherals, but the core is switchable to BE, although driver support for that appears to be incomplete And then there were the old chips where the integer core and the FPA (ancient FPU) could run with different endianesses ... lots of software had problems with that; I'm glad EABI and VFP put an end to that insanity.
On Sat, Jun 11, 2011 at 01:22:18PM +0200, Mikael Pettersson wrote: > Richard Cochran writes: > > I took another look at the whole MMIO clocksource implementation and > > can't understand why all of the accessors use le_to_cpu. This seems to > > imply that all peripheral buses are little endian (except for ixp, > > which no one cares about, anyhow). > > "no one"? ixp4xx is still being used. Yes, I know, but I was trying and failed to sound sacastic. > > I understand (or have been told) that almost all arm implementations > > out there are little endian. Why not use use a normal load to read the > > timer? Are there machines with LE peripheral buses but BE cores? > > ARM cores often have software-controllable endianess (see the ARM ARM). > Off the top of my head: > - ixp4xx: natively BE core and peripherals, but some people like to > switch the core to LE for user-space SW compatibility reasons > (I run my ixp4xx in BE as intended though) > - Kirkwood: natively LE core and peripherals, but the core is switchable > to BE, although driver support for that appears to be incomplete So, would you say that using le_to_cpu in the MMIO clocksource reading functions makes good sense, or not? Thanks, Richard
Richard Cochran writes: > > > I understand (or have been told) that almost all arm implementations > > > out there are little endian. Why not use use a normal load to read the > > > timer? Are there machines with LE peripheral buses but BE cores? > > > > ARM cores often have software-controllable endianess (see the ARM ARM). > > Off the top of my head: > > - ixp4xx: natively BE core and peripherals, but some people like to > > switch the core to LE for user-space SW compatibility reasons > > (I run my ixp4xx in BE as intended though) > > - Kirkwood: natively LE core and peripherals, but the core is switchable > > to BE, although driver support for that appears to be incomplete > > So, would you say that using le_to_cpu in the MMIO clocksource reading > functions makes good sense, or not? There has to be a place for optional endianess conversion (le_to_cpu, be_to_cpu, or none at all) somewhere in the MMIO access paths, and it has to be controlled by platform code. I don't know the new clocksource code well enough to suggest exactly where that should be done.
On Sat, Jun 11, 2011 at 07:15:10AM +0200, Richard Cochran wrote: > On Wed, Jun 01, 2011 at 11:58:26PM +0100, Russell King - ARM Linux wrote: > > On Mon, May 30, 2011 at 10:43:07AM +0200, Richard Cochran wrote: > > > Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > > > > > clocksource: convert ARM 32-bit up counting clocksources > > > > > > broke the build for ixp4xx and made big endian operation impossible. > > > This commit restores the original behaviour. > > > > I'm really not happy about using the MMIO clocksource stuff with random > > other read functions like this - it defeats the entire purpose of the > > MMIO clocksource stuff. Maybe we should just undo the change for IXP4xx > > and treat it as "special" for the time being. > > I took another look at the whole MMIO clocksource implementation and > can't understand why all of the accessors use le_to_cpu. This seems to > imply that all peripheral buses are little endian (except for ixp, > which no one cares about, anyhow). What would they use as an ioremap-compatible interface instead of readl()/readw() ? readl_be()/readw_be() doesn't work for the majority of platforms using this stuff. We also don't have native endian equivalents across the kernel. Not everyone provides the relaxed versions. It's basically a problem, and its one of the unfortunate prices paid by consolidating code - things become harder and less efficient to do. In this case, I think we need a set of _be() clocksource accessors in mmio.c, and leave it up to the platform to select the correct accessor.
On Sun, Jun 12, 2011 at 02:51:59PM +0100, Russell King - ARM Linux wrote: > > It's basically a problem, and its one of the unfortunate prices paid > by consolidating code - things become harder and less efficient to do. > > In this case, I think we need a set of _be() clocksource accessors in > mmio.c, and leave it up to the platform to select the correct accessor. So, is IXP the only machine that would need such accessors? If so, then I think just using my patch as a single exception would be fine (maybe adding a comment justifying the exception). Thanks, Richard
Mikael Pettersson <mikpe@it.uu.se> writes: > ARM cores often have software-controllable endianess (see the ARM ARM). > Off the top of my head: > - ixp4xx: natively BE core and peripherals, but some people like to > switch the core to LE for user-space SW compatibility reasons > (I run my ixp4xx in BE as intended though) Ok I'm back to work. IXP4xx have some problems working in LE mode, namely: - the network drivers (at least Ethernet and HSS) have to swap buffers because the NPE (network coprocessors) are BE, - the hw crypto stuff doesn't mostly work on LE - parts work, but the rest seems to be broken in firmware (NPE microcode) - or so it seems (swapping the data buffers would work). There is a special hardware in IXP4xx which makes it possible to effectively switch the NPEs to LE. Unfortunately the first CPU revision (IXP42x rev. A0) doesn't support it. And while I have the code working, it's not "yet" upstream. In short: - all IXP4xx can work BE, - IXP42x rev. A0 can work LE with impaired network transfers and hw crypto, - later IXP4xx can work "fully" LE (not upstream).
> Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > clocksource: convert ARM 32-bit up counting clocksources > > broke the build for ixp4xx and made big endian operation impossible. > This commit restores the original behaviour. So, what do we do with this? It would be nice to have the fix in the tree before 3.0. I guess I'll add this if nothing better shows up. Richard Cochran <richardcochran@gmail.com> writes: > > Signed-off-by: Richard Cochran <richard.cochran@omicron.at> > --- > arch/arm/mach-ixp4xx/common.c | 10 ++++++++-- > 1 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c > index 74ed81a..0777257 100644 > --- a/arch/arm/mach-ixp4xx/common.c > +++ b/arch/arm/mach-ixp4xx/common.c > @@ -419,14 +419,20 @@ static void notrace ixp4xx_update_sched_clock(void) > /* > * clocksource > */ > + > +static cycle_t ixp4xx_clocksource_read(struct clocksource *c) > +{ > + return *IXP4XX_OSTS; > +} > + > unsigned long ixp4xx_timer_freq = IXP4XX_TIMER_FREQ; > EXPORT_SYMBOL(ixp4xx_timer_freq); > static void __init ixp4xx_clocksource_init(void) > { > init_sched_clock(&cd, ixp4xx_update_sched_clock, 32, ixp4xx_timer_freq); > > - clocksource_mmio_init(&IXP4XX_OSTS, "OSTS", ixp4xx_timer_freq, 200, 32, > - clocksource_mmio_readl_up); > + clocksource_mmio_init(NULL, "OSTS", ixp4xx_timer_freq, 200, 32, > + ixp4xx_clocksource_read); > } > > /*
On Sun, Jun 26, 2011 at 03:38:05PM +0200, Krzysztof Halasa wrote: > > Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 > > > > clocksource: convert ARM 32-bit up counting clocksources > > > > broke the build for ixp4xx and made big endian operation impossible. > > This commit restores the original behaviour. > > So, what do we do with this? > It would be nice to have the fix in the tree before 3.0. > I guess I'll add this if nothing better shows up. I'll say it once again: If IXP is the only machine that needs this exception, then I think my patch is okay. Otherwise, we would want a set of big-endian-friendly clock reading methods. But I really don't know whether IXP is all alone in this... Just my 2 cents, Richard
On Thu, 2 Jun 2011, Thomas Gleixner wrote: > > I'm really not happy about using the MMIO clocksource stuff with random > > other read functions like this - it defeats the entire purpose of the > > MMIO clocksource stuff. Maybe we should just undo the change for IXP4xx > > and treat it as "special" for the time being. > > > > Thomas - do you have any other views? > > I have no objections to have special cased read functions as long as > all the other copied code is gone. We have the same problem with the > generic irq chip and I did not come up with a good decision function > where to draw the line. As for everything we come up with in the > consolidation space we need to apply common sense and keep an eye on > the real abusers. > > Though in the mmio clocksource case we might ask the question whether > read[l|w]() is really necessary in the generic implemetation or > not. [too tired to answer that now ] Thinking more about it we should add BE accessor functions to the mmio clocksource as this might be useful for other architectures as well. Thanks, tglx
Thomas Gleixner <tglx@linutronix.de> writes: > Thinking more about it we should add BE accessor functions to the mmio > clocksource as this might be useful for other architectures as well. Why not simply use a value-preserving accessors like readl/writel? Aren't they value-preserving on all ARM CPUs? IMHO the CPU/platform code should supply certain accessors (value- and order-preserving), and the generic code should simply use the correct one. Perhaps we should have (for ARM only) another set of register_readl() etc.? (PCI and register endianness may differ, a separate set would mean no runtime overhead in such case).
diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c index 74ed81a..0777257 100644 --- a/arch/arm/mach-ixp4xx/common.c +++ b/arch/arm/mach-ixp4xx/common.c @@ -419,14 +419,20 @@ static void notrace ixp4xx_update_sched_clock(void) /* * clocksource */ + +static cycle_t ixp4xx_clocksource_read(struct clocksource *c) +{ + return *IXP4XX_OSTS; +} + unsigned long ixp4xx_timer_freq = IXP4XX_TIMER_FREQ; EXPORT_SYMBOL(ixp4xx_timer_freq); static void __init ixp4xx_clocksource_init(void) { init_sched_clock(&cd, ixp4xx_update_sched_clock, 32, ixp4xx_timer_freq); - clocksource_mmio_init(&IXP4XX_OSTS, "OSTS", ixp4xx_timer_freq, 200, 32, - clocksource_mmio_readl_up); + clocksource_mmio_init(NULL, "OSTS", ixp4xx_timer_freq, 200, 32, + ixp4xx_clocksource_read); } /*
Commit 234b6ceddb4fc2a4bc5b9a7670f070f6e69e0868 clocksource: convert ARM 32-bit up counting clocksources broke the build for ixp4xx and made big endian operation impossible. This commit restores the original behaviour. Signed-off-by: Richard Cochran <richard.cochran@omicron.at> --- arch/arm/mach-ixp4xx/common.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)