(regression) kernel/timeconst.h bugs with HZ=128
I see these warnings on 32 bit ARM systems: CC kernel/time.o kernel/time.c: In function 'msecs_to_jiffies': kernel/time.c:472: warning: integer constant is too large for 'long' type kernel/time.c: In function 'usecs_to_jiffies': kernel/time.c:487: warning: integer constant is too large for 'long' type Line 472: return ((u64)MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32) line 487: return ((u64)USEC_TO_HZ_MUL32 * u + USEC_TO_HZ_ADJ32) The problem seems to be that these constants from kernel/timeconst.h have too many digits: #define ONLY_THIRTYTWO_BITS 0x01234567 #define MSEC_TO_HZ_ADJ320x3f7ced916 #define USEC_TO_HZ_ADJ320xfffbce4217d Those *_ADJ32 constants should have "ULL" suffixes, yes? Adding that by hand resolves the problem, but only until the next time that header file gets regenerated. Someone with observable Perl-fu should fix this ... - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
On Tuesday 26 February 2008, Peter Zijlstra wrote:
>
> On Mon, 2008-02-25 at 14:33 -0800, David Brownell wrote:
>
> > > +#ifdef CONFIG_LOCKDEP
> > > +
> > > +/* tell lockdep that this IRQ's locks and its parent's locks are in
> > > + * different categories, so that it won't detect false recursion.
> > > + */
> > > +static struct lock_class_key gpio_lock_class;
> > > +
> > > +static inline void mark_gpio_locking(unsigned gpio_irq)
> > > +{
> > > + lockdep_set_class(_desc[gpio_irq].lock, _lock_class);
> > > +}
> > > +
> > > +#else
> > > +
> > > + ...
>
> Glad to hear this works out for you.
Yeah, glad to have a localized fix rather than mucking with genirq.
> Just one note, you don't need the #ifdef mess here. struct
> lock_class_key is 0 bytes on !LOCKDEP and lockdep_set_class*() is
> defined away as well.
Hmm, then kernel/irq/handle.c shouldn't need #ifdefs either ... ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
On Tuesday 26 February 2008, Haavard Skinnemoen wrote: > > Another way to address that rm9200 issue would be to just rate > > the TC clockevent source lower than the one based on the system > > timer, so it's set up but never enabled ... and remember "t2_clk", > > calling clk_enable() only when that clockevent device is active. > > > > That would address one half of the suspend/resume equation too, > > ensuring that clock is disabled during suspend... > > Yes, we could probably do that. Which means we can just remove all the > ifdeffery? There'd still be the #ifdef for CONFIG_GENERIC_CLOCKEVENTS, unless all the platforms get updated to support that. Right now it's a "patches available but not merged" situation. > > The other half being: how to clk_disable(t0_clk) during system > > suspend? (And t1_clk on some systems.) There's currently no > > clocksource.set_mode() call; evidently there's an assumption that > > such counters cost no power, so they can always be left running. > > Yes...that would be a clocksource core issue I guess. Better leave that > for later... My thoughts exactly. ;) Plus a bit of puzzlement why that didn't trigger at least a warning during AT91 suspend testing. There used to be warnings there about clocks which were wrongly left enabled... - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 1/2] atmel_tc library
On Tuesday 26 February 2008, Haavard Skinnemoen wrote: > Ok. Let's drop the clock references... > > > > and it will always need a pointer through which to access the > > > registers, so the mid-layer might as well do those things. > > > > True about doing the ioremap. > > ...and keep the regs pointer, and possibly add the iomem resource. > > Ok? I'd be OK with clocks *and* irqs, but don't feel religious at all. > Btw, I'm not saying that you're the one who has to make these changes. > Once we agree, I can send a patch to do the things we agreed on. I'll ask you to do that then, yes. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-pm] Fundamental flaw in system suspend, exposed by freezer removal
On Tuesday 26 February 2008, David Newall wrote: > > Hardware can be inserted and removed while we're in a suspend state; and > there's nothing that we can do about it until we resume. Is it fair to > say, then, that having started suspend, we could reasonably ignore any > device insertion and removal, and handle it on resume? "Ignore" seems a bit strong; those events may be wakeup triggers, which would cause the hardware to make it a very short suspend state. "Defer handling" is more to the point, be it by hardware or software. > Presumably we need to scan for hardware changes on resume. Not on most busses I work with; the hardware issues notifications whenever the devices are removable. Which is a Good Thing ... scanning, as you suggest, is inherently not reliable. If a mouse is swapped, it likely doesn't matter a lot whether it's the "same" or not. But ... how about if some removable storage media was taken out, updated on a different system, then restored before the resume? Not many filesystems handle that sanely. You *really* want to have hardware which reports disconnect/reconnect events, or which physically prevents them (locked case etc). - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-pm] Fundamental flaw in system suspend, exposed by freezer removal
On Tuesday 26 February 2008, David Newall wrote: Hardware can be inserted and removed while we're in a suspend state; and there's nothing that we can do about it until we resume. Is it fair to say, then, that having started suspend, we could reasonably ignore any device insertion and removal, and handle it on resume? Ignore seems a bit strong; those events may be wakeup triggers, which would cause the hardware to make it a very short suspend state. Defer handling is more to the point, be it by hardware or software. Presumably we need to scan for hardware changes on resume. Not on most busses I work with; the hardware issues notifications whenever the devices are removable. Which is a Good Thing ... scanning, as you suggest, is inherently not reliable. If a mouse is swapped, it likely doesn't matter a lot whether it's the same or not. But ... how about if some removable storage media was taken out, updated on a different system, then restored before the resume? Not many filesystems handle that sanely. You *really* want to have hardware which reports disconnect/reconnect events, or which physically prevents them (locked case etc). - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 1/2] atmel_tc library
On Tuesday 26 February 2008, Haavard Skinnemoen wrote: Ok. Let's drop the clock references... and it will always need a pointer through which to access the registers, so the mid-layer might as well do those things. True about doing the ioremap. ...and keep the regs pointer, and possibly add the iomem resource. Ok? I'd be OK with clocks *and* irqs, but don't feel religious at all. Btw, I'm not saying that you're the one who has to make these changes. Once we agree, I can send a patch to do the things we agreed on. I'll ask you to do that then, yes. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
On Tuesday 26 February 2008, Haavard Skinnemoen wrote: Another way to address that rm9200 issue would be to just rate the TC clockevent source lower than the one based on the system timer, so it's set up but never enabled ... and remember t2_clk, calling clk_enable() only when that clockevent device is active. That would address one half of the suspend/resume equation too, ensuring that clock is disabled during suspend... Yes, we could probably do that. Which means we can just remove all the ifdeffery? There'd still be the #ifdef for CONFIG_GENERIC_CLOCKEVENTS, unless all the platforms get updated to support that. Right now it's a patches available but not merged situation. The other half being: how to clk_disable(t0_clk) during system suspend? (And t1_clk on some systems.) There's currently no clocksource.set_mode() call; evidently there's an assumption that such counters cost no power, so they can always be left running. Yes...that would be a clocksource core issue I guess. Better leave that for later... My thoughts exactly. ;) Plus a bit of puzzlement why that didn't trigger at least a warning during AT91 suspend testing. There used to be warnings there about clocks which were wrongly left enabled... - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
On Tuesday 26 February 2008, Peter Zijlstra wrote:
On Mon, 2008-02-25 at 14:33 -0800, David Brownell wrote:
+#ifdef CONFIG_LOCKDEP
+
+/* tell lockdep that this IRQ's locks and its parent's locks are in
+ * different categories, so that it won't detect false recursion.
+ */
+static struct lock_class_key gpio_lock_class;
+
+static inline void mark_gpio_locking(unsigned gpio_irq)
+{
+ lockdep_set_class(irq_desc[gpio_irq].lock, gpio_lock_class);
+}
+
+#else
+
+ ...
Glad to hear this works out for you.
Yeah, glad to have a localized fix rather than mucking with genirq.
Just one note, you don't need the #ifdef mess here. struct
lock_class_key is 0 bytes on !LOCKDEP and lockdep_set_class*() is
defined away as well.
Hmm, then kernel/irq/handle.c shouldn't need #ifdefs either ... ;)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
(regression) kernel/timeconst.h bugs with HZ=128
I see these warnings on 32 bit ARM systems: CC kernel/time.o kernel/time.c: In function 'msecs_to_jiffies': kernel/time.c:472: warning: integer constant is too large for 'long' type kernel/time.c: In function 'usecs_to_jiffies': kernel/time.c:487: warning: integer constant is too large for 'long' type Line 472: return ((u64)MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32) line 487: return ((u64)USEC_TO_HZ_MUL32 * u + USEC_TO_HZ_ADJ32) The problem seems to be that these constants from kernel/timeconst.h have too many digits: #define ONLY_THIRTYTWO_BITS 0x01234567 #define MSEC_TO_HZ_ADJ320x3f7ced916 #define USEC_TO_HZ_ADJ320xfffbce4217d Those *_ADJ32 constants should have ULL suffixes, yes? Adding that by hand resolves the problem, but only until the next time that header file gets regenerated. Someone with observable Perl-fu should fix this ... - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-pm] Fundamental flaw in system suspend, exposed by freezer removal
This "flaw" isn't a new thing, of course. I remember pointing out the rather annoying proclivity of the PM framework to deadlock when suspend() tried to remove USB devices ... back around 2.6.10 or so. Things have shuffled around a bit, and gotten better in some cases, but not fundamentally changed. It may be more accurate to say that now we understand some constraints on device tree management policies ... ones we had previously assumed should not be issues. (But AFAICT, without actually considering the question. Now we know the right question to ask!) On Monday 25 February 2008, Rafael J. Wysocki wrote: > IMO the device driver should assure that no new children will be registered > concurrently with the ->suspend() method (IOW, ->suspend() should wait for > all such registrations to complete and should prevent any new ones from > being started) and it should make it impossible to register any new children > after ->suspend() has run. It's the driver's problem how to achieve that. There's also the case where it's framework code that handles the additions rather than the parent device. That would be typical for many bridge, hub, or adapter type drivers ... you may be thinking mostly about drivers acting as "leaf" nodes in the device tree, at least in terms of real hardware nodes. Yes, "require that policy from such framework code too". Just trying to be sure the description doesn't have gaping holes in the middle. :) I can think of a bunch of serial busses where framework code has that sort of responsiblity. USB, SPI, I2C ... "legacy" I2C drivers would all need to be taught not to create/remove children during those intervals, lacking "new style" conversion (which makes them work more like normal drivers). - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
> So I'm think that the reason this only _changes_ the false
> recursion notification ...
Whoops, it's because of the following typo:
> --- a/arch/arm/plat-omap/gpio.c 2008-02-24 19:02:32.0 -0800
> +++ b/arch/arm/plat-omap/gpio.c 2008-02-25 10:54:29.0 -0800
> @@ -1332,6 +1332,28 @@ static struct clk *gpio_fclks[OMAP34XX_N
> static struct clk *gpio_iclks[OMAP34XX_NR_GPIOS];
> #endif
>
> +#ifdef CONFIG_LOCKDEP
> +
> +/* tell lockdep that this IRQ's locks and its parent's locks are in
> + * different categories, so that it won't detect false recursion.
> + */
> +static struct lock_class_key gpio_lock_class;
> +
> +static inline void mark_gpio_locking(unsigned gpio_irq)
> +{
> + lockdep_set_class(_desc[gpio_irq].lock, _lock_class);
> +}
> +
> +#else
> +
> +static inline void mark_gpio_locking(unsigned gpio_irq)
> +{
> + /* NOP */
> +}
> +
> +#endif
> +
> +
> static int __init _omap_gpio_init(void)
> {
> int i;
> @@ -1532,6 +1554,7 @@ static int __init _omap_gpio_init(void)
> set_irq_chip(j, _irq_chip);
> set_irq_handler(j, handle_simple_irq);
> set_irq_flags(j, IRQF_VALID);
> + mark_gpio_locking(i);
should be mark_gpio_locking(j).
That's an off-by-one error in ASCII or QWERTY (adjacency), and
in terms of typefaces it's a few pixels. Sigh.
Since that seems to behave, I'll send along a couple patches
like this ... unless you say this isn't really, after all, the
right way to address this problem. Note that neither IRQ has
ever been triggered at this point ... I'll move the marking up
a bit to ensure the lock has never been taken either.
- Dave
> }
> set_irq_chained_handler(bank->irq, gpio_irq_handler);
> set_irq_data(bank->irq, bank);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
> > I thought the way to use the *_nested() calls was "consistently"!
>
> Very much depends on your view of consistent :-)
>
> > That is, if one instance of a lock access uses it, they all should,
> > since that's the only way lockdep learns about equivalence classes.
> > Also, locks shouldn't move between those equivalence classes... so
> > the raw lockdep data stays correct.
>
> Ah, see, you're missing a detail (see below for the full story). Its the
> irq state that must be consistent. So if at any one point you take the
> lock in an irq safe context, you must always take it irq safe.
IRQ state is not a factor; that's already coded correctly.
The issue here is lockdep falsely reporting recursive locking.
Again, the locking is correct; it's lockdep getting confused,
because it puts all irq_desc[].lock instances in the same bag
instead of treating parent and child locks differently. So
when something holding a child lock makes a call that grabs
a parent lock, it thinks that could be recursive.
> Also, have you looked at explicit lock_class_key usage per chip?
Never had reason to look at such stuff; you didn't mention the
option in the earlier chitchat...
> That
> way you can avoid using the _nested annotation. You can set a class on a
> lock right after spin_lock_init() using lockdep_set_class*().
All this stuff is set up in kernel/irq/handle.c ... spinlocks
are initialized statically, lock classes are set up even before
the kernel banner is printed, by early_init_irq_lock_class().
So I'm think that the reason this only _changes_ the false
recursion notification is that it doesn't support overriding
such class annotations; it doesn't seem to forget about the
first one set up. Note that the code I posted earlier, using
the _nested annotations, isn't confused like this...
- Dave
=== CUT
Try to remove some false lockdep warnings about lock recursion,
by marking putting GPIO irq_desc locks into their own class.
But that doesn't seem to work; all it does is change the fault
report to be more confusing:
=
[ INFO: possible recursive locking detected ]
2.6.25-rc3 #80
-
swapper/1 is trying to acquire lock:
(_desc_lock_class){+...}, at: [] set_irq_wake+0x34/0xe8
but task is already holding lock:
(_desc_lock_class){+...}, at: [] set_irq_wake+0x34/0xe8
other info that might help us debug this:
2 locks held by swapper/1:
#0: (_desc_lock_class){+...}, at: [] set_irq_wake+0x34/0xe8
#1: (>lock){}, at: [] _set_gpio_wakeup+0x38/0xa4
stack backtrace:
[] (dump_stack+0x0/0x14) from []
(print_deadlock_bug+0xa0/0xcc)
[] (print_deadlock_bug+0x0/0xcc) from []
(check_deadlock+0x6c/0x84)
r6:c1c1834c r5: r4:
[] (check_deadlock+0x0/0x84) from []
(validate_chain+0x1d8/0x2c4)
r7:c1c1834c r6: r5:01403805 r4:c04c0658
[] (validate_chain+0x0/0x2c4) from []
(__lock_acquire+0x500/0x5bc)
[] (__lock_acquire+0x0/0x5bc) from []
(lock_acquire+0x70/0x88)
[] (lock_acquire+0x0/0x88) from []
(_spin_lock_irqsave+0x50/0x64)
r6:2093 r5:c007150c r4:c02cf6c8
[] (_spin_lock_irqsave+0x0/0x64) from []
(set_irq_wake+0x34/0xe8)
r6:0001 r5:0025 r4:c02cf698
[] (set_irq_wake+0x0/0xe8) from []
(_set_gpio_wakeup+0x5c/0xa4)
[] (_set_gpio_wakeup+0x0/0xa4) from []
(gpio_wake_enable+0x48/0x50)
r8:c00393c8 r7:c02d5ecc r6:0001 r5:0162 r4:00c2
[] (gpio_wake_enable+0x0/0x50) from []
(set_irq_wake+0xbc/0xe8)
r6:0001 r5:0162 r4:c02d5e9c
[] (set_irq_wake+0x0/0xe8) from []
(osk_mistral_init+0x160/0x1c8)
[] (osk_mistral_init+0x0/0x1c8) from []
(osk_init+0xac/0xd4)
r4:c001f3f8
[] (osk_init+0x0/0xd4) from []
(customize_machine+0x20/0x2c)
r4:c001e000
[] (customize_machine+0x0/0x2c) from []
(do_initcalls+0x78/0x200)
[] (do_initcalls+0x0/0x200) from []
(do_basic_setup+0x20/0x24)
[] (do_basic_setup+0x0/0x24) from []
(kernel_init+0x44/0x90)
[] (kernel_init+0x0/0x90) from [] (do_exit+0x0/0x30c)
r4:
---
arch/arm/plat-omap/gpio.c | 23 +++
1 files changed, 23 insertions(+)
--- a/arch/arm/plat-omap/gpio.c 2008-02-24 19:02:32.0 -0800
+++ b/arch/arm/plat-omap/gpio.c 2008-02-25 10:54:29.0 -0800
@@ -1332,6 +1332,28 @@ static struct clk *gpio_fclks[OMAP34XX_N
static struct clk *gpio_iclks[OMAP34XX_NR_GPIOS];
#endif
+#ifdef CONFIG_LOCKDEP
+
+/* tell lockdep that this IRQ's locks and its parent's locks are in
+ * different categories, so that it won't detect false recursion.
+ */
+static struct lock_class_key gpio_lock_class;
+
+static inline void mark_gpio_locking(unsigned gpio_irq)
+{
+ lockdep_set_class(_desc[gpio_irq].lock, _lock_class);
+}
+
+#else
+
+static inline void mark_gpio_locking(unsigned gpio_irq)
+{
+ /* NOP */
+}
+
+#endif
+
+
static int __init _omap_gpio_init(void)
{
int i;
@@ -1532,6 +1554,7 @@ static int __init
Re: [patch 2.6.25-rc2-git 1/2] atmel_tc library
> > > Which reminds me...you were talking about a patch that adds oneshot > > > support for the count/compare clocksource and more cleanups, but I > > > don't think I've seen it...? > > > > I avoid sending non-working patches, and hadn't made time to > > work on that issue recently. > > I was thinking that I could perhaps help you get it working... OK, if you want I'll send it. > > > I've never really seen the point of indenting those defines at all. > > > > Without them, it's harder to discern the logical structure of > > all the various bitfields and their contents. > > I prefer to separate the registers from the bitfields and the other > stuff. That way, no indentation is necessary. But that way it's no longer clear which symbols apply to which registers. It's not "needed" because that information became harder to find ... as in , which ISTR isn't even complete in its bitfield definitions. > > > I thought about that, but while the driver can safely call clk_enable() > > > on the same clock several times, I'm not sure if it's such a great idea > > > to call request_irq() on the same interrupt several times. So the > > > driver probably needs to know how many irqs there really are and might > > > as well use platform_get_irq() to find out. > > > > I thought the whole point of passing the clocks was to avoid needing > > to ask for them!! If trying one or three platform_get_irq() calls is > > OK, then surely trying one or three clk_get() calls is also OK... > > If you want to go down that path, surely reserving the iomem resource > is fine too? Why don't we just kill the whole tclib layer, the driver > can certainly do everything itself? There's one clear need for tclib: to hand out timers from the (small) pool of hardware entities. Each one can be used for different purposes by different drivers. The *current* tclib addresses only that minimal need. You're the one who wants to add more to it; I was just pointing out that you were being inconsistent. > Of course the driver should be responsible for calling clk_enable() and > clk_disable(). Otherwise, power management will be tricky. And since > the driver may need to make a decision about which interrupts to > request, it might as well call platform_get_irq() directly. You're being inconsistent here. It has to make the same kinds of decision about which clocks to enable/disable. So why should it have to platform_get_irq() but not clk_get()? > On the other hand, the driver will _always_ need a reference to each > clock, No; it doesn't need references to clocks for unused TC channels. > and it will always need a pointer through which to access the > registers, so the mid-layer might as well do those things. True about doing the ioremap. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
> > > > +static cycle_t tc_get_cycles(void)
> > > > +{
> > > > + unsigned long flags;
> > > > + u32 lower, upper;
> > > > +
> > > > + raw_local_irq_save(flags);
> > >
> > > Why do you need to use the raw version?
> >
> > This is part of the system timer code, and it should never be a
> > preemption point. Plus I didn't want to waste any instruction
> > cycles in code that runs before e.g. the DBGU IRQ handler would
> > get called... observably, such extra cycles *do* hurt.
>
> I don't understand what you mean by preemption point, but I guess the
> non-raw version may consume some extra cycles when lockdep is enabled.
A preemption point is where CONFIG_PREEMPT kicks in task switching
logic; lockdep is different.
> If we really expect using TC as a clocksource but not as a clockevent
> is going to be a common usage, perhaps we should move the decision into
> Kconfig?
Maybe, but I already spent lots more time on this than I wanted. :(
Another way to address that rm9200 issue would be to just rate
the TC clockevent source lower than the one based on the system
timer, so it's set up but never enabled ... and remember "t2_clk",
calling clk_enable() only when that clockevent device is active.
That would address one half of the suspend/resume equation too,
ensuring that clock is disabled during suspend...
The other half being: how to clk_disable(t0_clk) during system
suspend? (And t1_clk on some systems.) There's currently no
clocksource.set_mode() call; evidently there's an assumption that
such counters cost no power, so they can always be left running.
> > > I don't think it is safe to assume that one clock per channel always
> > > means one irq per channel as well...
> >
> > On current chips, that's how it works.
>
> Indeed. I just don't see any fundamental reason why it has to work that
> way, which is why I don't think we should depend on it.
AT91 chips share identifiers between clocks and interrupts; that's
fundamental, yes?
If some future chip works differently, that's a good time to change
things. Otherwise I see little point in caring about such issues.
- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
> > > > > ==> LOCKDEP feature is evidently missing:
> > > > > spin_lock_irq_nested(lock_ptr, lock_class)
> > > >
> > > > This rant is more lines than adding the API :-/ the reason for it not
> > > > being there is simple, it wasn't needed up until now.
> > >
> > > I suspected that was the case, but for all I knew there was some
> > > religious objection.
> >
> > Does this look about right? Or, I suppose it could just call
> > the _spin_lock_irqsave_nested() routine and discard the result.
>
> Before I look at the code, and with a notice that I haven't had my
> morning juice yet...
>
> It seems to me a spin_lock_irq_nested() thing is redundant, because:
>
> The lock must obviously be held hardirq safe and nested implies one is
> already held.
I thought the way to use the *_nested() calls was "consistently"!
That is, if one instance of a lock access uses it, they all should,
since that's the only way lockdep learns about equivalence classes.
Also, locks shouldn't move between those equivalence classes... so
the raw lockdep data stays correct.
The IRQ framework uses spin_lock_irq() in only one place that I saw:
in kernel/irq/autoprobe.c for the probe_irq_{on,off,mask}() calls.
Those calls will grab locks at their "top level", and then the
irqchip methods they call might need to grab locks for other irqs.
Potential example: chip->startup() and chip->shutdown() could
need to ensure a *parent* controller starts/stops, and that should
involve mutual exclusion using the parent's irq lock (as well as
the child's). So the chip and its parent should be in different
lock classes, else lockdep will wrongly warn of recursion.
> Hence the context is already hardirq safe thus using
> spin_lock_irq/spin_unlock_irq is wrong because it will enable irqs and
> destroy the irqsafe guarantee for the parent lock.
That's not how the autoprobe() stuff works. The other calls in
the genirq framework don't use the *_irq() variants though, so
your intuition is right there. (Only the autoprobe paths had
the FIXME comments in that patch I sent earlier, related to the
lack of the $SUBJECT primitive.)
> Obviously I'm missing something here.. otherwise you wouldn't need it.
>
> As I'm very much not familiar with the IRQ code, could you spell it out
> to me?
The probe_irq_*() calls are made from task context, not hardirq
context, but they access the same locks involved in IRQ management
and processing. So either they need to pass the same lock class
annodations to lockdep, or there's something that's unusually
counter-intuitive going on with respect to those annotations in
simple tree data structures.
- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Thursday 21 February 2008, Alan Stern wrote: > > = CUT HERE > > Modify EHCI irq handling on the theory that at least some of the > > "lost" IRQs are caused by goofage between multiple lowlevel IRQ > > acking mechanisms: try rescanning before we exit the handler, in > > case the EHCI-internal ack (by clearing the irq status) doesn't > > always suffice for IRQs triggered nearly back-to-back. > > > > --- > > drivers/usb/host/ehci-hcd.c | 8 > > 1 file changed, 8 insertions(+) > > > > --- g26.orig/drivers/usb/host/ehci-hcd.c 2008-02-20 13:26:00.0 > > -0800 > > +++ g26/drivers/usb/host/ehci-hcd.c 2008-02-20 13:54:37.0 -0800 > > @@ -638,6 +638,8 @@ static irqreturn_t ehci_irq (struct usb_ > > return IRQ_NONE; > > } > > > > +retrigger: > > + > > /* clear (just) interrupts */ > > ehci_writel(ehci, status, >regs->status); > > cmd = ehci_readl(ehci, >regs->command); > > @@ -725,6 +727,12 @@ dead: > > > > if (bh) > > ehci_work (ehci); > > + > > + status = ehci_readl(ehci, >regs->status); > > + status &= INTR_MASK; > > + if (status) > > + goto retrigger; > > + > > spin_unlock (>lock); > > if (pcd_status & STS_PCD) > > usb_hcd_poll_rh_status(hcd); > > There's one little problem here. As a result of this change, the line > where pcd_status gets set (not shown in this patch) needs to be changed > to: > > pcd_status |= (status & STS_PCD); Actually, no change is needed. It's initialized to zero, then set to "status" given "if (status & STS_PCD)", and never cleared. So if it's ever set, it stays set... > > Then the test shown above can be simplified to: > > if (pcd_status) True with the current code too ... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Thursday 21 February 2008, Alan Stern wrote: = CUT HERE Modify EHCI irq handling on the theory that at least some of the lost IRQs are caused by goofage between multiple lowlevel IRQ acking mechanisms: try rescanning before we exit the handler, in case the EHCI-internal ack (by clearing the irq status) doesn't always suffice for IRQs triggered nearly back-to-back. --- drivers/usb/host/ehci-hcd.c | 8 1 file changed, 8 insertions(+) --- g26.orig/drivers/usb/host/ehci-hcd.c 2008-02-20 13:26:00.0 -0800 +++ g26/drivers/usb/host/ehci-hcd.c 2008-02-20 13:54:37.0 -0800 @@ -638,6 +638,8 @@ static irqreturn_t ehci_irq (struct usb_ return IRQ_NONE; } +retrigger: + /* clear (just) interrupts */ ehci_writel(ehci, status, ehci-regs-status); cmd = ehci_readl(ehci, ehci-regs-command); @@ -725,6 +727,12 @@ dead: if (bh) ehci_work (ehci); + + status = ehci_readl(ehci, ehci-regs-status); + status = INTR_MASK; + if (status) + goto retrigger; + spin_unlock (ehci-lock); if (pcd_status STS_PCD) usb_hcd_poll_rh_status(hcd); There's one little problem here. As a result of this change, the line where pcd_status gets set (not shown in this patch) needs to be changed to: pcd_status |= (status STS_PCD); Actually, no change is needed. It's initialized to zero, then set to status given if (status STS_PCD), and never cleared. So if it's ever set, it stays set... Then the test shown above can be simplified to: if (pcd_status) True with the current code too ... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
== LOCKDEP feature is evidently missing:
spin_lock_irq_nested(lock_ptr, lock_class)
This rant is more lines than adding the API :-/ the reason for it not
being there is simple, it wasn't needed up until now.
I suspected that was the case, but for all I knew there was some
religious objection.
Does this look about right? Or, I suppose it could just call
the _spin_lock_irqsave_nested() routine and discard the result.
Before I look at the code, and with a notice that I haven't had my
morning juice yet...
It seems to me a spin_lock_irq_nested() thing is redundant, because:
The lock must obviously be held hardirq safe and nested implies one is
already held.
I thought the way to use the *_nested() calls was consistently!
That is, if one instance of a lock access uses it, they all should,
since that's the only way lockdep learns about equivalence classes.
Also, locks shouldn't move between those equivalence classes... so
the raw lockdep data stays correct.
The IRQ framework uses spin_lock_irq() in only one place that I saw:
in kernel/irq/autoprobe.c for the probe_irq_{on,off,mask}() calls.
Those calls will grab locks at their top level, and then the
irqchip methods they call might need to grab locks for other irqs.
Potential example: chip-startup() and chip-shutdown() could
need to ensure a *parent* controller starts/stops, and that should
involve mutual exclusion using the parent's irq lock (as well as
the child's). So the chip and its parent should be in different
lock classes, else lockdep will wrongly warn of recursion.
Hence the context is already hardirq safe thus using
spin_lock_irq/spin_unlock_irq is wrong because it will enable irqs and
destroy the irqsafe guarantee for the parent lock.
That's not how the autoprobe() stuff works. The other calls in
the genirq framework don't use the *_irq() variants though, so
your intuition is right there. (Only the autoprobe paths had
the FIXME comments in that patch I sent earlier, related to the
lack of the $SUBJECT primitive.)
Obviously I'm missing something here.. otherwise you wouldn't need it.
As I'm very much not familiar with the IRQ code, could you spell it out
to me?
The probe_irq_*() calls are made from task context, not hardirq
context, but they access the same locks involved in IRQ management
and processing. So either they need to pass the same lock class
annodations to lockdep, or there's something that's unusually
counter-intuitive going on with respect to those annotations in
simple tree data structures.
- Dave
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
+static cycle_t tc_get_cycles(void)
+{
+ unsigned long flags;
+ u32 lower, upper;
+
+ raw_local_irq_save(flags);
Why do you need to use the raw version?
This is part of the system timer code, and it should never be a
preemption point. Plus I didn't want to waste any instruction
cycles in code that runs before e.g. the DBGU IRQ handler would
get called... observably, such extra cycles *do* hurt.
I don't understand what you mean by preemption point, but I guess the
non-raw version may consume some extra cycles when lockdep is enabled.
A preemption point is where CONFIG_PREEMPT kicks in task switching
logic; lockdep is different.
If we really expect using TC as a clocksource but not as a clockevent
is going to be a common usage, perhaps we should move the decision into
Kconfig?
Maybe, but I already spent lots more time on this than I wanted. :(
Another way to address that rm9200 issue would be to just rate
the TC clockevent source lower than the one based on the system
timer, so it's set up but never enabled ... and remember t2_clk,
calling clk_enable() only when that clockevent device is active.
That would address one half of the suspend/resume equation too,
ensuring that clock is disabled during suspend...
The other half being: how to clk_disable(t0_clk) during system
suspend? (And t1_clk on some systems.) There's currently no
clocksource.set_mode() call; evidently there's an assumption that
such counters cost no power, so they can always be left running.
I don't think it is safe to assume that one clock per channel always
means one irq per channel as well...
On current chips, that's how it works.
Indeed. I just don't see any fundamental reason why it has to work that
way, which is why I don't think we should depend on it.
AT91 chips share identifiers between clocks and interrupts; that's
fundamental, yes?
If some future chip works differently, that's a good time to change
things. Otherwise I see little point in caring about such issues.
- Dave
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 1/2] atmel_tc library
Which reminds me...you were talking about a patch that adds oneshot support for the count/compare clocksource and more cleanups, but I don't think I've seen it...? I avoid sending non-working patches, and hadn't made time to work on that issue recently. I was thinking that I could perhaps help you get it working... OK, if you want I'll send it. I've never really seen the point of indenting those defines at all. Without them, it's harder to discern the logical structure of all the various bitfields and their contents. I prefer to separate the registers from the bitfields and the other stuff. That way, no indentation is necessary. But that way it's no longer clear which symbols apply to which registers. It's not needed because that information became harder to find ... as in include/asm-avr32/arch-at32ap/time.h, which ISTR isn't even complete in its bitfield definitions. I thought about that, but while the driver can safely call clk_enable() on the same clock several times, I'm not sure if it's such a great idea to call request_irq() on the same interrupt several times. So the driver probably needs to know how many irqs there really are and might as well use platform_get_irq() to find out. I thought the whole point of passing the clocks was to avoid needing to ask for them!! If trying one or three platform_get_irq() calls is OK, then surely trying one or three clk_get() calls is also OK... If you want to go down that path, surely reserving the iomem resource is fine too? Why don't we just kill the whole tclib layer, the driver can certainly do everything itself? There's one clear need for tclib: to hand out timers from the (small) pool of hardware entities. Each one can be used for different purposes by different drivers. The *current* tclib addresses only that minimal need. You're the one who wants to add more to it; I was just pointing out that you were being inconsistent. Of course the driver should be responsible for calling clk_enable() and clk_disable(). Otherwise, power management will be tricky. And since the driver may need to make a decision about which interrupts to request, it might as well call platform_get_irq() directly. You're being inconsistent here. It has to make the same kinds of decision about which clocks to enable/disable. So why should it have to platform_get_irq() but not clk_get()? On the other hand, the driver will _always_ need a reference to each clock, No; it doesn't need references to clocks for unused TC channels. and it will always need a pointer through which to access the registers, so the mid-layer might as well do those things. True about doing the ioremap. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
I thought the way to use the *_nested() calls was consistently!
Very much depends on your view of consistent :-)
That is, if one instance of a lock access uses it, they all should,
since that's the only way lockdep learns about equivalence classes.
Also, locks shouldn't move between those equivalence classes... so
the raw lockdep data stays correct.
Ah, see, you're missing a detail (see below for the full story). Its the
irq state that must be consistent. So if at any one point you take the
lock in an irq safe context, you must always take it irq safe.
IRQ state is not a factor; that's already coded correctly.
The issue here is lockdep falsely reporting recursive locking.
Again, the locking is correct; it's lockdep getting confused,
because it puts all irq_desc[].lock instances in the same bag
instead of treating parent and child locks differently. So
when something holding a child lock makes a call that grabs
a parent lock, it thinks that could be recursive.
Also, have you looked at explicit lock_class_key usage per chip?
Never had reason to look at such stuff; you didn't mention the
option in the earlier chitchat...
That
way you can avoid using the _nested annotation. You can set a class on a
lock right after spin_lock_init() using lockdep_set_class*().
All this stuff is set up in kernel/irq/handle.c ... spinlocks
are initialized statically, lock classes are set up even before
the kernel banner is printed, by early_init_irq_lock_class().
So I'm think that the reason this only _changes_ the false
recursion notification is that it doesn't support overriding
such class annotations; it doesn't seem to forget about the
first one set up. Note that the code I posted earlier, using
the _nested annotations, isn't confused like this...
- Dave
=== CUT
Try to remove some false lockdep warnings about lock recursion,
by marking putting GPIO irq_desc locks into their own class.
But that doesn't seem to work; all it does is change the fault
report to be more confusing:
=
[ INFO: possible recursive locking detected ]
2.6.25-rc3 #80
-
swapper/1 is trying to acquire lock:
(irq_desc_lock_class){+...}, at: [c007150c] set_irq_wake+0x34/0xe8
but task is already holding lock:
(irq_desc_lock_class){+...}, at: [c007150c] set_irq_wake+0x34/0xe8
other info that might help us debug this:
2 locks held by swapper/1:
#0: (irq_desc_lock_class){+...}, at: [c007150c] set_irq_wake+0x34/0xe8
#1: (bank-lock){}, at: [c0038d80] _set_gpio_wakeup+0x38/0xa4
stack backtrace:
[c0029d78] (dump_stack+0x0/0x14) from [c0064a48]
(print_deadlock_bug+0xa0/0xcc)
[c00649a8] (print_deadlock_bug+0x0/0xcc) from [c0064ae0]
(check_deadlock+0x6c/0x84)
r6:c1c1834c r5: r4:
[c0064a74] (check_deadlock+0x0/0x84) from [c0066500]
(validate_chain+0x1d8/0x2c4)
r7:c1c1834c r6: r5:01403805 r4:c04c0658
[c0066328] (validate_chain+0x0/0x2c4) from [c0066aec]
(__lock_acquire+0x500/0x5bc)
[c00665ec] (__lock_acquire+0x0/0x5bc) from [c006705c]
(lock_acquire+0x70/0x88)
[c0066fec] (lock_acquire+0x0/0x88) from [c02260d0]
(_spin_lock_irqsave+0x50/0x64)
r6:2093 r5:c007150c r4:c02cf6c8
[c0226080] (_spin_lock_irqsave+0x0/0x64) from [c007150c]
(set_irq_wake+0x34/0xe8)
r6:0001 r5:0025 r4:c02cf698
[c00714d8] (set_irq_wake+0x0/0xe8) from [c0038da4]
(_set_gpio_wakeup+0x5c/0xa4)
[c0038d48] (_set_gpio_wakeup+0x0/0xa4) from [c0039410]
(gpio_wake_enable+0x48/0x50)
r8:c00393c8 r7:c02d5ecc r6:0001 r5:0162 r4:00c2
[c00393c8] (gpio_wake_enable+0x0/0x50) from [c0071594]
(set_irq_wake+0xbc/0xe8)
r6:0001 r5:0162 r4:c02d5e9c
[c00714d8] (set_irq_wake+0x0/0xe8) from [c000c8d0]
(osk_mistral_init+0x160/0x1c8)
[c000c770] (osk_mistral_init+0x0/0x1c8) from [c000c9e4]
(osk_init+0xac/0xd4)
r4:c001f3f8
[c000c938] (osk_init+0x0/0xd4) from [c0009db4]
(customize_machine+0x20/0x2c)
r4:c001e000
[c0009d94] (customize_machine+0x0/0x2c) from [c0008684]
(do_initcalls+0x78/0x200)
[c000860c] (do_initcalls+0x0/0x200) from [c000882c]
(do_basic_setup+0x20/0x24)
[c000880c] (do_basic_setup+0x0/0x24) from [c0008bd0]
(kernel_init+0x44/0x90)
[c0008b8c] (kernel_init+0x0/0x90) from [c004576c] (do_exit+0x0/0x30c)
r4:
---
arch/arm/plat-omap/gpio.c | 23 +++
1 files changed, 23 insertions(+)
--- a/arch/arm/plat-omap/gpio.c 2008-02-24 19:02:32.0 -0800
+++ b/arch/arm/plat-omap/gpio.c 2008-02-25 10:54:29.0 -0800
@@ -1332,6 +1332,28 @@ static struct clk *gpio_fclks[OMAP34XX_N
static struct clk *gpio_iclks[OMAP34XX_NR_GPIOS];
#endif
+#ifdef CONFIG_LOCKDEP
+
+/* tell lockdep that this IRQ's locks and its parent's locks are in
+ * different categories, so that it won't detect false recursion.
+ */
+static struct lock_class_key gpio_lock_class;
+
+static inline void
Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
So I'm think that the reason this only _changes_ the false
recursion notification ...
Whoops, it's because of the following typo:
--- a/arch/arm/plat-omap/gpio.c 2008-02-24 19:02:32.0 -0800
+++ b/arch/arm/plat-omap/gpio.c 2008-02-25 10:54:29.0 -0800
@@ -1332,6 +1332,28 @@ static struct clk *gpio_fclks[OMAP34XX_N
static struct clk *gpio_iclks[OMAP34XX_NR_GPIOS];
#endif
+#ifdef CONFIG_LOCKDEP
+
+/* tell lockdep that this IRQ's locks and its parent's locks are in
+ * different categories, so that it won't detect false recursion.
+ */
+static struct lock_class_key gpio_lock_class;
+
+static inline void mark_gpio_locking(unsigned gpio_irq)
+{
+ lockdep_set_class(irq_desc[gpio_irq].lock, gpio_lock_class);
+}
+
+#else
+
+static inline void mark_gpio_locking(unsigned gpio_irq)
+{
+ /* NOP */
+}
+
+#endif
+
+
static int __init _omap_gpio_init(void)
{
int i;
@@ -1532,6 +1554,7 @@ static int __init _omap_gpio_init(void)
set_irq_chip(j, gpio_irq_chip);
set_irq_handler(j, handle_simple_irq);
set_irq_flags(j, IRQF_VALID);
+ mark_gpio_locking(i);
should be mark_gpio_locking(j).
That's an off-by-one error in ASCII or QWERTY (adjacency), and
in terms of typefaces it's a few pixels. Sigh.
Since that seems to behave, I'll send along a couple patches
like this ... unless you say this isn't really, after all, the
right way to address this problem. Note that neither IRQ has
ever been triggered at this point ... I'll move the marking up
a bit to ensure the lock has never been taken either.
- Dave
}
set_irq_chained_handler(bank-irq, gpio_irq_handler);
set_irq_data(bank-irq, bank);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-pm] Fundamental flaw in system suspend, exposed by freezer removal
This flaw isn't a new thing, of course. I remember pointing out the rather annoying proclivity of the PM framework to deadlock when suspend() tried to remove USB devices ... back around 2.6.10 or so. Things have shuffled around a bit, and gotten better in some cases, but not fundamentally changed. It may be more accurate to say that now we understand some constraints on device tree management policies ... ones we had previously assumed should not be issues. (But AFAICT, without actually considering the question. Now we know the right question to ask!) On Monday 25 February 2008, Rafael J. Wysocki wrote: IMO the device driver should assure that no new children will be registered concurrently with the -suspend() method (IOW, -suspend() should wait for all such registrations to complete and should prevent any new ones from being started) and it should make it impossible to register any new children after -suspend() has run. It's the driver's problem how to achieve that. There's also the case where it's framework code that handles the additions rather than the parent device. That would be typical for many bridge, hub, or adapter type drivers ... you may be thinking mostly about drivers acting as leaf nodes in the device tree, at least in terms of real hardware nodes. Yes, require that policy from such framework code too. Just trying to be sure the description doesn't have gaping holes in the middle. :) I can think of a bunch of serial busses where framework code has that sort of responsiblity. USB, SPI, I2C ... legacy I2C drivers would all need to be taught not to create/remove children during those intervals, lacking new style conversion (which makes them work more like normal drivers). - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
> > > ==> LOCKDEP feature is evidently missing:
> > > spin_lock_irq_nested(lock_ptr, lock_class)
> >
> > This rant is more lines than adding the API :-/ the reason for it not
> > being there is simple, it wasn't needed up until now.
>
> I suspected that was the case, but for all I knew there was some
> religious objection.
Does this look about right? Or, I suppose it could just call
the _spin_lock_irqsave_nested() routine and discard the result.
- Dave
=CUT HERE
Add new spin_lock_irq_nested() call, so that lockdep can work with the
code which uses spin_*_irq() calls that don't save/restore flags.
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
---
Against 2.6.25-rc3
include/linux/spinlock.h |6 ++
include/linux/spinlock_api_smp.h |2 ++
kernel/spinlock.c| 21 +++--
3 files changed, 27 insertions(+), 2 deletions(-)
--- a/include/linux/spinlock.h 2008-02-24 18:50:50.0 -0800
+++ b/include/linux/spinlock.h 2008-02-24 19:02:39.0 -0800
@@ -196,9 +196,13 @@ do {
\
#define write_lock_irqsave(lock, flags)flags =
_write_lock_irqsave(lock)
#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define spin_lock_irq_nested(lock, subclass) \
+ _spin_lock_irq_nested(lock, subclass)
#define spin_lock_irqsave_nested(lock, flags, subclass) \
flags = _spin_lock_irqsave_nested(lock, subclass)
#else
+#define spin_lock_irq_nested(lock, subclass) \
+ _spin_lock_irq(lock)
#define spin_lock_irqsave_nested(lock, flags, subclass) \
flags = _spin_lock_irqsave(lock)
#endif
@@ -208,6 +212,8 @@ do {
\
#define spin_lock_irqsave(lock, flags) _spin_lock_irqsave(lock, flags)
#define read_lock_irqsave(lock, flags) _read_lock_irqsave(lock, flags)
#define write_lock_irqsave(lock, flags)_write_lock_irqsave(lock, flags)
+#define spin_lock_irq_nested(lock, subclass) \
+ spin_lock_irq(lock)
#define spin_lock_irqsave_nested(lock, flags, subclass)\
spin_lock_irqsave(lock, flags)
--- a/include/linux/spinlock_api_smp.h 2008-02-24 18:50:50.0 -0800
+++ b/include/linux/spinlock_api_smp.h 2008-02-24 19:02:39.0 -0800
@@ -28,6 +28,8 @@ void __lockfunc _spin_lock_bh(spinlock_t
void __lockfunc _read_lock_bh(rwlock_t *lock) __acquires(lock);
void __lockfunc _write_lock_bh(rwlock_t *lock) __acquires(lock);
void __lockfunc _spin_lock_irq(spinlock_t *lock) __acquires(lock);
+void __lockfunc _spin_lock_irq_nested(spinlock_t *lock, int subclass)
+ __acquires(lock);
void __lockfunc _read_lock_irq(rwlock_t *lock) __acquires(lock);
void __lockfunc _write_lock_irq(rwlock_t *lock)
__acquires(lock);
unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
--- a/kernel/spinlock.c 2008-02-24 18:50:50.0 -0800
+++ b/kernel/spinlock.c 2008-02-24 19:02:39.0 -0800
@@ -290,8 +290,26 @@ void __lockfunc _spin_lock_nested(spinlo
spin_acquire(>dep_map, subclass, 0, _RET_IP_);
LOCK_CONTENDED(lock, _raw_spin_trylock, _raw_spin_lock);
}
-
EXPORT_SYMBOL(_spin_lock_nested);
+
+void __lockfunc _spin_lock_irq_nested(spinlock_t *lock, int subclass)
+{
+ local_irq_disable();
+ preempt_disable();
+ spin_acquire(>dep_map, subclass, 0, _RET_IP_);
+ /*
+* On lockdep we dont want the hand-coded irq-enable of
+* _raw_spin_lock_flags() code, because lockdep assumes
+* that interrupts are not re-enabled during lock-acquire:
+*/
+#ifdef CONFIG_LOCKDEP
+ LOCK_CONTENDED(lock, _raw_spin_trylock, _raw_spin_lock);
+#else
+ _raw_spin_lock_flags(lock, );
+#endif
+}
+EXPORT_SYMBOL(_spin_lock_irq_nested);
+
unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int
subclass)
{
unsigned long flags;
@@ -311,7 +329,6 @@ unsigned long __lockfunc _spin_lock_irqs
#endif
return flags;
}
-
EXPORT_SYMBOL(_spin_lock_irqsave_nested);
#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 1/2] atmel_tc library
> > > > Note that this won't be usable until the AT91 and AT32 platforms
> > > > incorporate patches to configure the relevant platform devices.
> > > > Those changes are probably 2.6.26 material.
> >
> > More specifically (and all those patches are available now):
> >
> > - AT91 needs clocksource/clockevent support for the SAM9 PIT timer;
> > - AVR32 needs more extensive clocksource/clockevent updates;
>
> Which reminds me...you were talking about a patch that adds oneshot
> support for the count/compare clocksource and more cleanups, but I
> don't think I've seen it...?
I avoid sending non-working patches, and hadn't made time to
work on that issue recently.
> Yes, getting the fundamental stuff into mainline now would help a lot.
Or at least, towards the front of the merge queue, ahead of
the various platform-specific patches.
> But I was thinking about Linus' suggestions that we exploit the
> distributed nature of git and do cross-tree merges to synchronize
> changes to common code.
>
> Setting up a separate git tree would allow the changes to go into the
> arm tree without littering it with possibly unstable avr32 changes as
> well, and it would allow me to merge them and put more stuff on top.
Doing that with ARM patches is Russell's call; he hasn't been too
keen on merging from non-Linus GIT trees when that came up before.
> > > > +#define ATMEL_TC_BMR 0xc4/* TC Block Mode Register */
> > > > +#define ATMEL_TC_TC0XC0S (3 << 0)/* external clock 0
> > > > source */
> > > > +#defineATMEL_TC_TC0XC0S_TCLK0 (0 << 0)
> > >
> > > Hmm. Indentation using spaces? I didn't know you were into that sort of
> > > thing ;-)
> >
> > It's way better than indenting off the right margin of the page!
>
> I've never really seen the point of indenting those defines at all.
Without them, it's harder to discern the logical structure of
all the various bitfields and their contents.
> > > Anyway, my main issue is that I think we should add a data structure
> > > with information about each device, similar to struct ssc_device in the
> > > atmel-ssc driver. How about something like this?
> > >
> > > struct atmel_tc_block {
> > > void __iomem*regs; /* non-NULL when busy */
> > > struct platform_device *pdev;
> > > struct clk *clk[3];
> > > struct list_headnode;
> > > };
> >
> > And per-channel IRQs too...
>
> I thought about that, but while the driver can safely call clk_enable()
> on the same clock several times, I'm not sure if it's such a great idea
> to call request_irq() on the same interrupt several times. So the
> driver probably needs to know how many irqs there really are and might
> as well use platform_get_irq() to find out.
I thought the whole point of passing the clocks was to avoid needing
to ask for them!! If trying one or three platform_get_irq() calls is
OK, then surely trying one or three clk_get() calls is also OK...
- Dave
> > Well, if you want to take these patches off my hands and then be
> > responsible for merging them upstream, I won't object. :)
>
> I can do that.
>
> It's getting late around here...I'll reply to your other mail tomorrow.
>
> Haavard
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
> Again, sorry for the delay...it really sucks that I haven't been able
> to look at this stuff closely until now. Hopefully a late review is
> better than no review.
Yes. The review I've gotten so far has been of the "it works for me,
thanks" variety.
(The only issue reported to me is that NO_HZ on AT91 seems to make the
DBGU lose characters sometimes ... that one-byte "fifo" makes trouble
whenever there's the slightest delay in IRQ handling, and NO_HZ can
easily add such delays as part of ensuring that "jiffies" is current
before invoking handlers.)
> On Fri, 22 Feb 2008 17:28:37 -0800
> David Brownell <[EMAIL PROTECTED]> wrote:
>
> > +static cycle_t tc_get_cycles(void)
> > +{
> > + unsigned long flags;
> > + u32 lower, upper;
> > +
> > + raw_local_irq_save(flags);
>
> Why do you need to use the raw version?
This is part of the system timer code, and it should never be a
preemption point. Plus I didn't want to waste any instruction
cycles in code that runs before e.g. the DBGU IRQ handler would
get called... observably, such extra cycles *do* hurt.
> > +retry:
> > + upper = __raw_readl(tcaddr + ATMEL_TC_REG(1, CV));
> > + lower = __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
> > + if (upper != __raw_readl(tcaddr + ATMEL_TC_REG(1, CV)))
> > + goto retry;
>
> Did you just open-code a do/while loop using goto? ;-)
I take that back about late review being better than none. ;)
> > +#ifdef CONFIG_GENERIC_CLOCKEVENTS
> > +#define USE_TC_CLKEVT
> > +#endif
> > +
> > +#ifdef CONFIG_ARCH_AT91RM9200
> > +/* The AT91rm9200 system timer is a oneshot-capable 32k timer that's
> > + * always running ... configuring a TC channel to work the same way
> > + * would just waste some power.
> > + */
> > +#undef USE_TC_CLKEVT
> > +#endif
> > +
> > +#ifdef USE_TC_CLKEVT
>
> Can't you just use #ifndef CONFIG_ARCH_AT91RM9200 and avoid the whole
> ifdef/define/undef dance above?
I can't know that some other platform won't have a better system
timer solution, and didn't want to assume that only that single
venerable chip had such a solution ... it's kind of puzzling to
me (software guy) that none of the newest Atmel SOCs have better
timer infrastructure than they do. ;)
> > + case CLOCK_EVT_MODE_ONESHOT:
> > + /* slow clock, count up to RC, then irq and stop */
>
> Hmm. Do you really want to stop it? Won't you get better accuracy if
> you let it run and program the next event as (prev_event + delta)?
No, ONESHOT means "stop after the IRQ I asked for". And when
tc_next_event() is asked to trigger that IRQ, it's relative to
the instant it's requested, not relative to the last one that
was requested (which may not have triggered yet, or may have
been quite some time ago).
> > +static struct irqaction tc_irqaction = {
> > + .name = "tc_clkevt",
> > + .flags = IRQF_TIMER | IRQF_DISABLED,
> > + .handler= ch2_irq,
> > +};
>
> I don't think you need to define this statically. You can call
> request_irq() at arch_initcall() time.
That could be done, yes ... and using request_irq()/free_irq()
would also let this be linked as a module, not just statically.
On the other hand, doing it this way doesn't hurt either does it?
Unless a modular build is important.
> > +static void __init setup_clkevents(struct platform_device *pdev,
> > + struct clk *t0_clk, int clk32k_divisor_idx)
> > +{
> > + struct clk *t2_clk = clk_get(>dev, "t2_clk");
> > + int irq;
> > +
> > + /* SOCs have either one IRQ and one clock for the whole
> > +* TC block; or one each per channel. We use channel 2.
> > +*/
> > + if (IS_ERR(t2_clk)) {
> > + t2_clk = t0_clk;
> > + irq = platform_get_irq(pdev, 0);
> > + } else {
> > + irq = platform_get_irq(pdev, 2);
> > + clk_enable(t2_clk);
> > + }
>
> I don't think it is safe to assume that one clock per channel always
> means one irq per channel as well...
On current chips, that's how it works.
> What's wrong with
>
> irq = platform_get_irq(pdev, 2);
> if (irq < 0)
> irq = platform_get_irq(pdev, 0);
Nothing much, except that I took the more concise path. Got patch?
> > +config ATMEL_TCB_CLKSRC
> > + bool "TC Block Clocksource"
> > + depends on ATMEL_TCLIB && GENERIC_TIME
> > + default y
> > + help
> > + Select this to get a high precision clocksource based on a
> > + TC block with a 5+ MHz base clock rate. Two
Re: [patch 2.6.25-rc2-git 1/2] atmel_tc library
On Sun, 24 Feb 2008 18:45:54 +0100
Haavard Skinnemoen <[EMAIL PROTECTED]> wrote:
>
> On Fri, 22 Feb 2008 17:23:23 -0800
> David Brownell <[EMAIL PROTECTED]> wrote:
>
> > Create based on and the
> > at91sam9263 and at32ap7000 datasheets. Most AT91 and AT32 SOCs have one
> > or two of these TC blocks, which include three 16-bit timers that can be
> > interconnected in various ways.
> >
> > These TC blocks can be used for external interfacing (such as PWM and
> > measurement), or used as somewhat quirky sixteen-bit timers.
> >
> > Signed-off-by: David Brownell <[EMAIL PROTECTED]>
> > ---
>
> Sorry for the delay...some late comments coming up.
Happens. :)
> > Note that this won't be usable until the AT91 and AT32 platforms
> > incorporate patches to configure the relevant platform devices.
> > Those changes are probably 2.6.26 material.
More specifically (and all those patches are available now):
- AT91 needs clocksource/clockevent support for the SAM9 PIT timer;
- AVR32 needs more extensive clocksource/clockevent updates;
- Both also need platform device setup;
Merging these two patches before those is a perfectly safe NOP.
> Right...and there are a few funny dependencies we need to watch out
> for. AVR32 currently uses one of the TC blocks as a system timer. That
> code needs to be ripped out, but that will cause a fourfold increase in
> the power consumption of the AP7000 CPU.
Because the AVR32 count/compare clocksource (architectural registers)
precludes using the "idle" state in the idle loop ... it counts CPU
cycles, which don't happen in the "idle" state.
> So I want to keep the window
> between ripping out the old code and using the new code as small as
> possible.
Right.
> I see the following possibilities:
> * I switch avr32 over to use the new code after it has been merged
> into mainline. This means the new code won't be tested on avr32
> until near the end of the next merge window -- not good.
Well, "more widely" tested. We've both observed it to work; basically
the same code has worked on AT91 for about a year now, too.
And that's why I suggest merging these parts, now that they're known to
work on both architectures. The arch-specific bits can follow at their
own pace, neither one blocking the other.
> * I forward the patches that switch avr32 over to Andrew so that they
> can be included in -mm right after the tclib support. Not a bad
> option, but I'm planning to do more AVR32 PM work before 2.6.26, so
> there might be conflicts.
Conflicts there would be the easy part to deal with. They're all
specific to AVR32, and you can merge those in any convenient order.
> * I take the whole thing through my avr32 git tree.
Doesn't work as smoothly for the AT91 side ... but Andrew Victor has
very limited time any more for such stuff (these AT91 patches will
go through him first then Russell) so I'm not sure any added delays
there could hurt much.
> * I (or someone else) set up a new git tree for tclib + AT91 and
> AVR32 platform support and ask Stephen to pull it into the -next
> tree. Or, once it's stable, rmk and I can both pull it into our
> respective trees. But that obviously means no rebasing and funny
> games from that point on...
>
> I think the last option is the best one. What do you think?
In effect, I've had that last option as a series of quilt patches,
and posting these two is the first step in that merge sequence...
heck, they could merge right now into 2.6.25-rc3 and that would let
the two arch series merge at their own paces. (AVR32 direct from
you, AT91 very indirectly through Andrew then Russell.)
My personal priority is to get these patches into the merge queue(s)
so that they're no longer sitting on my disks and so that when I
want those platforms to have HRT and/or NO_HZ, it's already there.
> > +#if defined(CONFIG_AVR32)
> > +/* AVR32 has these divide PBB */
> > +const u8 atmel_tc_divisors[5] = { 0, 4, 8, 16, 32, };
> > +EXPORT_SYMBOL(atmel_tc_divisors);
> > +
> > +#elif defined(CONFIG_ARCH_AT91)
> > +/* AT91 has these divide MCK */
> > +const u8 atmel_tc_divisors[5] = { 2, 8, 32, 128, 0, };
> > +EXPORT_SYMBOL(atmel_tc_divisors);
> > +
> > +#endif
> > +
> > +/* we "know" that there will be either one or two TC blocks */
> > +static struct platform_device *blocks[2];
>
> You seem to know more about future Atmel chips than I do :-P
I only "know" about currently announced ones. ;)
> I'm not a huge fan of such static limitations.
Me either, but at this point doing more than that seems to me
to land in that "overengineering" category...
>
Re: [patch 2.6.25-rc2-git 1/2] atmel_tc library
On Sun, 24 Feb 2008 18:45:54 +0100
Haavard Skinnemoen [EMAIL PROTECTED] wrote:
On Fri, 22 Feb 2008 17:23:23 -0800
David Brownell [EMAIL PROTECTED] wrote:
Create linux/atmel_tc.h based on asm-arm/arch-at91/at91-tc.h and the
at91sam9263 and at32ap7000 datasheets. Most AT91 and AT32 SOCs have one
or two of these TC blocks, which include three 16-bit timers that can be
interconnected in various ways.
These TC blocks can be used for external interfacing (such as PWM and
measurement), or used as somewhat quirky sixteen-bit timers.
Signed-off-by: David Brownell [EMAIL PROTECTED]
---
Sorry for the delay...some late comments coming up.
Happens. :)
Note that this won't be usable until the AT91 and AT32 platforms
incorporate patches to configure the relevant platform devices.
Those changes are probably 2.6.26 material.
More specifically (and all those patches are available now):
- AT91 needs clocksource/clockevent support for the SAM9 PIT timer;
- AVR32 needs more extensive clocksource/clockevent updates;
- Both also need platform device setup;
Merging these two patches before those is a perfectly safe NOP.
Right...and there are a few funny dependencies we need to watch out
for. AVR32 currently uses one of the TC blocks as a system timer. That
code needs to be ripped out, but that will cause a fourfold increase in
the power consumption of the AP7000 CPU.
Because the AVR32 count/compare clocksource (architectural registers)
precludes using the idle state in the idle loop ... it counts CPU
cycles, which don't happen in the idle state.
So I want to keep the window
between ripping out the old code and using the new code as small as
possible.
Right.
I see the following possibilities:
* I switch avr32 over to use the new code after it has been merged
into mainline. This means the new code won't be tested on avr32
until near the end of the next merge window -- not good.
Well, more widely tested. We've both observed it to work; basically
the same code has worked on AT91 for about a year now, too.
And that's why I suggest merging these parts, now that they're known to
work on both architectures. The arch-specific bits can follow at their
own pace, neither one blocking the other.
* I forward the patches that switch avr32 over to Andrew so that they
can be included in -mm right after the tclib support. Not a bad
option, but I'm planning to do more AVR32 PM work before 2.6.26, so
there might be conflicts.
Conflicts there would be the easy part to deal with. They're all
specific to AVR32, and you can merge those in any convenient order.
* I take the whole thing through my avr32 git tree.
Doesn't work as smoothly for the AT91 side ... but Andrew Victor has
very limited time any more for such stuff (these AT91 patches will
go through him first then Russell) so I'm not sure any added delays
there could hurt much.
* I (or someone else) set up a new git tree for tclib + AT91 and
AVR32 platform support and ask Stephen to pull it into the -next
tree. Or, once it's stable, rmk and I can both pull it into our
respective trees. But that obviously means no rebasing and funny
games from that point on...
I think the last option is the best one. What do you think?
In effect, I've had that last option as a series of quilt patches,
and posting these two is the first step in that merge sequence...
heck, they could merge right now into 2.6.25-rc3 and that would let
the two arch series merge at their own paces. (AVR32 direct from
you, AT91 very indirectly through Andrew then Russell.)
My personal priority is to get these patches into the merge queue(s)
so that they're no longer sitting on my disks and so that when I
want those platforms to have HRT and/or NO_HZ, it's already there.
+#if defined(CONFIG_AVR32)
+/* AVR32 has these divide PBB */
+const u8 atmel_tc_divisors[5] = { 0, 4, 8, 16, 32, };
+EXPORT_SYMBOL(atmel_tc_divisors);
+
+#elif defined(CONFIG_ARCH_AT91)
+/* AT91 has these divide MCK */
+const u8 atmel_tc_divisors[5] = { 2, 8, 32, 128, 0, };
+EXPORT_SYMBOL(atmel_tc_divisors);
+
+#endif
+
+/* we know that there will be either one or two TC blocks */
+static struct platform_device *blocks[2];
You seem to know more about future Atmel chips than I do :-P
I only know about currently announced ones. ;)
I'm not a huge fan of such static limitations.
Me either, but at this point doing more than that seems to me
to land in that overengineering category...
+/**
+ * atmel_tc_alloc - allocate a specified TC block
+ * @block: which block to allocate
+ * @iomem: used to return its IO memory resource
+ *
+ * Caller allocates a block. If it is available, its I/O space is
requested
+ * and returned through the iomem pointer, and the device node for the
block
+ * is returned. When it is not available, NULL is returned.
Any reason why it can't
Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
Again, sorry for the delay...it really sucks that I haven't been able
to look at this stuff closely until now. Hopefully a late review is
better than no review.
Yes. The review I've gotten so far has been of the it works for me,
thanks variety.
(The only issue reported to me is that NO_HZ on AT91 seems to make the
DBGU lose characters sometimes ... that one-byte fifo makes trouble
whenever there's the slightest delay in IRQ handling, and NO_HZ can
easily add such delays as part of ensuring that jiffies is current
before invoking handlers.)
On Fri, 22 Feb 2008 17:28:37 -0800
David Brownell [EMAIL PROTECTED] wrote:
+static cycle_t tc_get_cycles(void)
+{
+ unsigned long flags;
+ u32 lower, upper;
+
+ raw_local_irq_save(flags);
Why do you need to use the raw version?
This is part of the system timer code, and it should never be a
preemption point. Plus I didn't want to waste any instruction
cycles in code that runs before e.g. the DBGU IRQ handler would
get called... observably, such extra cycles *do* hurt.
+retry:
+ upper = __raw_readl(tcaddr + ATMEL_TC_REG(1, CV));
+ lower = __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
+ if (upper != __raw_readl(tcaddr + ATMEL_TC_REG(1, CV)))
+ goto retry;
Did you just open-code a do/while loop using goto? ;-)
I take that back about late review being better than none. ;)
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+#define USE_TC_CLKEVT
+#endif
+
+#ifdef CONFIG_ARCH_AT91RM9200
+/* The AT91rm9200 system timer is a oneshot-capable 32k timer that's
+ * always running ... configuring a TC channel to work the same way
+ * would just waste some power.
+ */
+#undef USE_TC_CLKEVT
+#endif
+
+#ifdef USE_TC_CLKEVT
Can't you just use #ifndef CONFIG_ARCH_AT91RM9200 and avoid the whole
ifdef/define/undef dance above?
I can't know that some other platform won't have a better system
timer solution, and didn't want to assume that only that single
venerable chip had such a solution ... it's kind of puzzling to
me (software guy) that none of the newest Atmel SOCs have better
timer infrastructure than they do. ;)
+ case CLOCK_EVT_MODE_ONESHOT:
+ /* slow clock, count up to RC, then irq and stop */
Hmm. Do you really want to stop it? Won't you get better accuracy if
you let it run and program the next event as (prev_event + delta)?
No, ONESHOT means stop after the IRQ I asked for. And when
tc_next_event() is asked to trigger that IRQ, it's relative to
the instant it's requested, not relative to the last one that
was requested (which may not have triggered yet, or may have
been quite some time ago).
+static struct irqaction tc_irqaction = {
+ .name = tc_clkevt,
+ .flags = IRQF_TIMER | IRQF_DISABLED,
+ .handler= ch2_irq,
+};
I don't think you need to define this statically. You can call
request_irq() at arch_initcall() time.
That could be done, yes ... and using request_irq()/free_irq()
would also let this be linked as a module, not just statically.
On the other hand, doing it this way doesn't hurt either does it?
Unless a modular build is important.
+static void __init setup_clkevents(struct platform_device *pdev,
+ struct clk *t0_clk, int clk32k_divisor_idx)
+{
+ struct clk *t2_clk = clk_get(pdev-dev, t2_clk);
+ int irq;
+
+ /* SOCs have either one IRQ and one clock for the whole
+* TC block; or one each per channel. We use channel 2.
+*/
+ if (IS_ERR(t2_clk)) {
+ t2_clk = t0_clk;
+ irq = platform_get_irq(pdev, 0);
+ } else {
+ irq = platform_get_irq(pdev, 2);
+ clk_enable(t2_clk);
+ }
I don't think it is safe to assume that one clock per channel always
means one irq per channel as well...
On current chips, that's how it works.
What's wrong with
irq = platform_get_irq(pdev, 2);
if (irq 0)
irq = platform_get_irq(pdev, 0);
Nothing much, except that I took the more concise path. Got patch?
+config ATMEL_TCB_CLKSRC
+ bool TC Block Clocksource
+ depends on ATMEL_TCLIB GENERIC_TIME
+ default y
+ help
+ Select this to get a high precision clocksource based on a
+ TC block with a 5+ MHz base clock rate. Two timer channels
+ are combined to make a single 32-bit timer.
+
+ When GENERIC_CLOCKEVENTS is defined, the third timer channel
+ may be used as a clock event device supporting oneshot mode
+ (delays of up to two seconds) based on the 32 KiHz clock.
+
+config ATMEL_TCB_CLKSRC_BLOCK
+ int
+ depends on ATMEL_TCB_CLKSRC
+ prompt TC Block if ARCH_AT91RM9200 || ARCH_AT91SAM9260 ||
CPU_AT32AP700X
+ default 0
+ range 0 1
Hmm...I don't like the direction this is going...
How about we make tcb_clksrc_init() global and call it from the
platform code with whatever TCB the platform thinks
Re: [patch 2.6.25-rc2-git 1/2] atmel_tc library
Note that this won't be usable until the AT91 and AT32 platforms
incorporate patches to configure the relevant platform devices.
Those changes are probably 2.6.26 material.
More specifically (and all those patches are available now):
- AT91 needs clocksource/clockevent support for the SAM9 PIT timer;
- AVR32 needs more extensive clocksource/clockevent updates;
Which reminds me...you were talking about a patch that adds oneshot
support for the count/compare clocksource and more cleanups, but I
don't think I've seen it...?
I avoid sending non-working patches, and hadn't made time to
work on that issue recently.
Yes, getting the fundamental stuff into mainline now would help a lot.
Or at least, towards the front of the merge queue, ahead of
the various platform-specific patches.
But I was thinking about Linus' suggestions that we exploit the
distributed nature of git and do cross-tree merges to synchronize
changes to common code.
Setting up a separate git tree would allow the changes to go into the
arm tree without littering it with possibly unstable avr32 changes as
well, and it would allow me to merge them and put more stuff on top.
Doing that with ARM patches is Russell's call; he hasn't been too
keen on merging from non-Linus GIT trees when that came up before.
+#define ATMEL_TC_BMR 0xc4/* TC Block Mode Register */
+#define ATMEL_TC_TC0XC0S (3 0)/* external clock 0
source */
+#defineATMEL_TC_TC0XC0S_TCLK0 (0 0)
Hmm. Indentation using spaces? I didn't know you were into that sort of
thing ;-)
It's way better than indenting off the right margin of the page!
I've never really seen the point of indenting those defines at all.
Without them, it's harder to discern the logical structure of
all the various bitfields and their contents.
Anyway, my main issue is that I think we should add a data structure
with information about each device, similar to struct ssc_device in the
atmel-ssc driver. How about something like this?
struct atmel_tc_block {
void __iomem*regs; /* non-NULL when busy */
struct platform_device *pdev;
struct clk *clk[3];
struct list_headnode;
};
And per-channel IRQs too...
I thought about that, but while the driver can safely call clk_enable()
on the same clock several times, I'm not sure if it's such a great idea
to call request_irq() on the same interrupt several times. So the
driver probably needs to know how many irqs there really are and might
as well use platform_get_irq() to find out.
I thought the whole point of passing the clocks was to avoid needing
to ask for them!! If trying one or three platform_get_irq() calls is
OK, then surely trying one or three clk_get() calls is also OK...
- Dave
Well, if you want to take these patches off my hands and then be
responsible for merging them upstream, I won't object. :)
I can do that.
It's getting late around here...I'll reply to your other mail tomorrow.
Haavard
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()
== LOCKDEP feature is evidently missing:
spin_lock_irq_nested(lock_ptr, lock_class)
This rant is more lines than adding the API :-/ the reason for it not
being there is simple, it wasn't needed up until now.
I suspected that was the case, but for all I knew there was some
religious objection.
Does this look about right? Or, I suppose it could just call
the _spin_lock_irqsave_nested() routine and discard the result.
- Dave
=CUT HERE
Add new spin_lock_irq_nested() call, so that lockdep can work with the
code which uses spin_*_irq() calls that don't save/restore flags.
Signed-off-by: David Brownell [EMAIL PROTECTED]
---
Against 2.6.25-rc3
include/linux/spinlock.h |6 ++
include/linux/spinlock_api_smp.h |2 ++
kernel/spinlock.c| 21 +++--
3 files changed, 27 insertions(+), 2 deletions(-)
--- a/include/linux/spinlock.h 2008-02-24 18:50:50.0 -0800
+++ b/include/linux/spinlock.h 2008-02-24 19:02:39.0 -0800
@@ -196,9 +196,13 @@ do {
\
#define write_lock_irqsave(lock, flags)flags =
_write_lock_irqsave(lock)
#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define spin_lock_irq_nested(lock, subclass) \
+ _spin_lock_irq_nested(lock, subclass)
#define spin_lock_irqsave_nested(lock, flags, subclass) \
flags = _spin_lock_irqsave_nested(lock, subclass)
#else
+#define spin_lock_irq_nested(lock, subclass) \
+ _spin_lock_irq(lock)
#define spin_lock_irqsave_nested(lock, flags, subclass) \
flags = _spin_lock_irqsave(lock)
#endif
@@ -208,6 +212,8 @@ do {
\
#define spin_lock_irqsave(lock, flags) _spin_lock_irqsave(lock, flags)
#define read_lock_irqsave(lock, flags) _read_lock_irqsave(lock, flags)
#define write_lock_irqsave(lock, flags)_write_lock_irqsave(lock, flags)
+#define spin_lock_irq_nested(lock, subclass) \
+ spin_lock_irq(lock)
#define spin_lock_irqsave_nested(lock, flags, subclass)\
spin_lock_irqsave(lock, flags)
--- a/include/linux/spinlock_api_smp.h 2008-02-24 18:50:50.0 -0800
+++ b/include/linux/spinlock_api_smp.h 2008-02-24 19:02:39.0 -0800
@@ -28,6 +28,8 @@ void __lockfunc _spin_lock_bh(spinlock_t
void __lockfunc _read_lock_bh(rwlock_t *lock) __acquires(lock);
void __lockfunc _write_lock_bh(rwlock_t *lock) __acquires(lock);
void __lockfunc _spin_lock_irq(spinlock_t *lock) __acquires(lock);
+void __lockfunc _spin_lock_irq_nested(spinlock_t *lock, int subclass)
+ __acquires(lock);
void __lockfunc _read_lock_irq(rwlock_t *lock) __acquires(lock);
void __lockfunc _write_lock_irq(rwlock_t *lock)
__acquires(lock);
unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
--- a/kernel/spinlock.c 2008-02-24 18:50:50.0 -0800
+++ b/kernel/spinlock.c 2008-02-24 19:02:39.0 -0800
@@ -290,8 +290,26 @@ void __lockfunc _spin_lock_nested(spinlo
spin_acquire(lock-dep_map, subclass, 0, _RET_IP_);
LOCK_CONTENDED(lock, _raw_spin_trylock, _raw_spin_lock);
}
-
EXPORT_SYMBOL(_spin_lock_nested);
+
+void __lockfunc _spin_lock_irq_nested(spinlock_t *lock, int subclass)
+{
+ local_irq_disable();
+ preempt_disable();
+ spin_acquire(lock-dep_map, subclass, 0, _RET_IP_);
+ /*
+* On lockdep we dont want the hand-coded irq-enable of
+* _raw_spin_lock_flags() code, because lockdep assumes
+* that interrupts are not re-enabled during lock-acquire:
+*/
+#ifdef CONFIG_LOCKDEP
+ LOCK_CONTENDED(lock, _raw_spin_trylock, _raw_spin_lock);
+#else
+ _raw_spin_lock_flags(lock, flags);
+#endif
+}
+EXPORT_SYMBOL(_spin_lock_irq_nested);
+
unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int
subclass)
{
unsigned long flags;
@@ -311,7 +329,6 @@ unsigned long __lockfunc _spin_lock_irqs
#endif
return flags;
}
-
EXPORT_SYMBOL(_spin_lock_irqsave_nested);
#endif
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] regulator: consumer driver regulator control framework.
On Saturday 23 February 2008, Andrew Morton wrote: > On Wed, 20 Feb 2008 17:08:53 + Liam Girdwood <[EMAIL PROTECTED]> wrote: > > +#define mV_to_uV(mV) (mV * 1000) > > +#define uV_to_mV(uV) (uV / 1000) > > +#define V_to_uV(V) (mV_to_uV(V * 1000)) > > +#define uV_to_V(uV)(uV_to_mV(uV) / 1000) > > +#define mA_to_uA(mA) (mA * 1000) > > +#define uA_to_mA(uA) (uA / 1000) > > +#define A_to_uA(A) (mA_to_uA(A * 1000)) > > +#define uA_to_A(uA)(uA_to_mA(uA) / 1000) > > hm. It might be nicer to make these static inline functions. Especially > if the code is consistent about what C type is used to represent voltage > and current. And if not ... then be sure to use e.g. ((mV) * 1000), ((uV) / 1000) etc so that params like "x + 23" can't cause chaos. (FWIW oth these points -- use inlines, parenthesize params -- are also found in Documentation/CodingStyle.) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] regulator: consumer driver regulator control framework.
On Saturday 23 February 2008, Andrew Morton wrote: On Wed, 20 Feb 2008 17:08:53 + Liam Girdwood [EMAIL PROTECTED] wrote: +#define mV_to_uV(mV) (mV * 1000) +#define uV_to_mV(uV) (uV / 1000) +#define V_to_uV(V) (mV_to_uV(V * 1000)) +#define uV_to_V(uV)(uV_to_mV(uV) / 1000) +#define mA_to_uA(mA) (mA * 1000) +#define uA_to_mA(uA) (uA / 1000) +#define A_to_uA(A) (mA_to_uA(A * 1000)) +#define uA_to_A(uA)(uA_to_mA(uA) / 1000) hm. It might be nicer to make these static inline functions. Especially if the code is consistent about what C type is used to represent voltage and current. And if not ... then be sure to use e.g. ((mV) * 1000), ((uV) / 1000) etc so that params like x + 23 can't cause chaos. (FWIW oth these points -- use inlines, parenthesize params -- are also found in Documentation/CodingStyle.) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_spi: support zero length transfer
> > > David, do you think writing 0 bytes is a valid use of this API? > > > > Just a zero byte transfer ... no, though it depends what you mean > > by "valid". (I'm not sure I'd expect all controller drivers to > > reject such requests.) That has no effect on bits-on-the-wire, > > and would make trouble for various DMA engines. > > So, the behaviour is undefined, something between 'crash my dma engine', > 'assert chip select and wait some time', or 'do_nothing'... No, it's fully defined. "Crash my engine" is not OK. The delay is controlled by transfer.delay_usecs ... possibly zero, which is best viewed as a degenerate case. > Is this kind of device so common that we need to do all that work? or can we > leave it as is (verified to work only with atmel_spi)? You can't avoid testing each combination of SPI protocol driver and SPI master controller driver you rely on ... especially when protocol tweaking options (cs_change, delay_usecs, bits_per_word, etc) are used at the per-transfer level. This driver stack isn't as readily tested as, say, USB. However, protocol drivers should be able to rely on controller drivers to reject requests that they don't even try to handle; that's basic. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer
On Friday 22 February 2008, Ned Forrester wrote: > > So, what I think you said is that it would be better for pxa2xx_spi to > silently ignore a zero-length message, passing it back with the rest of > the message when all is complete, than to reject the message. Yes. Remember that the reason to _use_ such a message is to get the inline delay ... and that if you've got code to handle that case (transfer.len == 0 && transfer.delay_usecs != 0) it's almost trivial to support the degenerate "delay_usecs == 0" case too. > I see no > reason why that could not be done, though it may be tricky to set other > things like SSP modes and chip select and *not* start the DMA. It would > have to be tested, so I'm not sure when I could try that. Normally I'd expect it's little more than a "goto", but the pxa2xx_spi structure is relatively complex. > >> I agree with Marc: any such delay will be undefined, in the general > >> case. It might work for a specific driver implementation. > > > > Is that what Marc said? I couldn't tell. In any case, I disagree; > > the semantics of that delay are clearly define. > > Maybe I am missing something. Aren't we talking about a transfer in a > message, with or without other transfers, who's only unique > characteristic is that that its length is zero? There were two cases ... delay_usecs being zero, or not. In either case, the semantics are the same: after the transfer (len == 0), delay that many microseconds (zero or more). > Or are you and Atsushi talking about using spi_transfer.delay_usecs > *with* a zero length transfer to effectively put a delay between the > assertion of CS and the start of the first clock? If so, then I guess I > missed the original point. Sorry. As I noted, there are two cases. The reason to use a zero length transfer is to get a delay ... in his case, specifically before the first clock, but in general *anywhere* in the transfer. But the degenerate case is "delay for zero microseconds". > -- > > By the way, reading spi.h again, it looks like spi_transfer.delay_usecs > is supposed to be implemented between the last bit movement of a > transfer and any change of CS at the end of a transfer. Is that right? Yes. > I think that pxa2xx_spi is dropping CS, if requested, immediately at > the end of transfer, and then putting spi_transfer.delay_usecs between > that transfer and the next. That's a bug then; it will matter with some drivers. > >> If it were necessary to scan a > >> whole message for zero-length transfers and refuse to queue an offending > >> message, then that adds burden to all messages. > > > > Sanity checking messages as they're submitted is easy; and it's > > also the natural point for setting up DMA mappings (and making > > those cachelines available for better use). > > Hmmm Obviously I have much to learn about modern computers. It had > not occurred to me, even after reading "Linux Device Drivers", Corbet, > et.al, that by DMA mapping, one frees the cache for other uses. It > makes sense. That's certainly what happens on systems where the buffer is from "normal" memory and the cache is DMA-incoherent ... like most ARMs, and probably the majority of non-x86 hardware. Think of that as being a level or two deeper than what LDD covers. > In my application I pass many large buffers to the master > driver, and I map them in the protocol driver. I did that without good > reason, but now I see it was the proper choice. > > Unfortunately, pxa2xx_spi does any DMA mapping not done by the protocol > driver in pump_transfers, as each transfer is submitted to the hardware. That's not wrong ... but it's sub-optimal: those cache lines could have been doing better things all that time, and cleaning them out in the middle of a transfer will slow it down by some amount. > There is not currently any message checking done in transfer(), the > only error return from that is if the master driver is shutdown (queue > stopped). The current scheme is to return the message with non-zero > spi_message.status if it has failed *after* execution has been > attempted. Again ... not wrong, but sub-optimal: when it happens (which won't be common, fortunately!) the SPI slave will be left in a rude state. And the protocol driver may not know how to recover. > I don't look forward to making major changes, if we really > want all DMA mapping and error checking to be in transfer(), though I > see no reason why it could not be done. There's no rush on cleanups like that. They'd be reasonable tasks for someone with some time to spare, who wanted to get their feet wet in some driver updates and who could set up some kind of test rig. (Like one with a programmable slave that could be made to do all sorts of stuff that might otherwise be a bit uncommon. A microcontroller slave would be easy ... programming a CPLD or FPGA would be much trickier!) - Dave -- To unsubscribe from this list: send the line "unsubscribe
[patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
Clocksource and clockevent device based on the Atmel TC blocks.
The clockevent device handles both periodic and oneshot modes, so this
enables NO_HZ and high res timers on some platforms that previously
couldn't use those mechanisms.
This works on both AVR32 and AT91 chips, given relevant patches for
tclib support (always) and clockevents (or else this will only look
like a higher precision clocksource). It's an updated and modularized
version of an AT91-only patch that has circulated for some time now.
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
---
As with the preceding patch, this depends on platform updates that
won't merge before 2.6.26 ... in this case, updates to switch over
to clocksources (at91sam9 chips, but not at91rm9200) and clockevents
(avr32 and at91sam9), on top of platform_device setup.
drivers/clocksource/Makefile |1
drivers/clocksource/tcb_clksrc.c | 303 +++
drivers/misc/Kconfig | 25 +++
3 files changed, 329 insertions(+)
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_ATMEL_TCB_CLKSRC) += tcb_clksrc.o
obj-$(CONFIG_X86_CYCLONE_TIMER)+= cyclone.o
obj-$(CONFIG_X86_PM_TIMER) += acpi_pm.o
obj-$(CONFIG_SCx200HR_TIMER) += scx200_hrt.o
--- /dev/null
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -0,0 +1,303 @@
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+
+/*
+ * We're configured to use a specific TC block, one that's not hooked
+ * up to external hardware, to provide a time solution:
+ *
+ * - Two channels combine to create a free-running 32 bit counter
+ * with a base rate of 5+ MHz, packaged as a clocksource (with
+ * resolution better than 200 nsec).
+ *
+ * - The third channel may be used to provide a 16-bit clockevent
+ * source, used in either periodic or oneshot mode. This runs
+ * at 32 KiHZ, and can handle delays of up to two seconds.
+ *
+ * A boot clocksource and clockevent source are also currently needed,
+ * unless the relevant platforms (ARM/AT91, AVR32/AT32) are changed so
+ * this code can be used when init_timers() is called, well before most
+ * devices are set up. (Some low end AT91 parts, which can run uClinux,
+ * have only the timers in one TC block... they currently don't support
+ * the tclib code, because of that initialization issue.)
+ *
+ * REVISIT behavior during system suspend states... we should disable
+ * all clocks and save the power. Easily done for clockevent devices,
+ * but clocksources won't necessarily get the needed notifications.
+ * For deeper system sleep states, this will be mandatory...
+ */
+
+static void __iomem *tcaddr;
+
+static cycle_t tc_get_cycles(void)
+{
+ unsigned long flags;
+ u32 lower, upper;
+
+ raw_local_irq_save(flags);
+retry:
+ upper = __raw_readl(tcaddr + ATMEL_TC_REG(1, CV));
+ lower = __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
+ if (upper != __raw_readl(tcaddr + ATMEL_TC_REG(1, CV)))
+ goto retry;
+
+ raw_local_irq_restore(flags);
+ return (upper << 16) | lower;
+}
+
+static struct clocksource clksrc = {
+ .name = "tcb_clksrc",
+ .rating = 200,
+ .read = tc_get_cycles,
+ .mask = CLOCKSOURCE_MASK(32),
+ .shift = 18,
+ .flags = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+#define USE_TC_CLKEVT
+#endif
+
+#ifdef CONFIG_ARCH_AT91RM9200
+/* The AT91rm9200 system timer is a oneshot-capable 32k timer that's
+ * always running ... configuring a TC channel to work the same way
+ * would just waste some power.
+ */
+#undef USE_TC_CLKEVT
+#endif
+
+#ifdef USE_TC_CLKEVT
+
+/* For now, we always use the 32K clock ... this optimizes for NO_HZ,
+ * because using one of the divided clocks would usually mean the
+ * tick rate can never be less than several dozen Hz (vs 0.5 Hz).
+ *
+ * A divided clock could be good for high resolution timers, since
+ * 30.5 usec resolution can seem "low".
+ */
+static u32 timer_clock;
+
+static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
+{
+ __raw_writel(0xff, tcaddr + ATMEL_TC_REG(2, IDR));
+ __raw_writel(ATMEL_TC_CLKDIS, tcaddr + ATMEL_TC_REG(2, CCR));
+
+ switch (m) {
+
+ /* By not making the gentime core emulate periodic mode on top
+* of oneshot, we get lower overhead and improved accuracy.
+*/
+ case CLOCK_EVT_MODE_PERIODIC:
+ /* slow clock, count up to RC, then irq and restart */
+ __raw_writel(timer_clock
+ | ATMEL_TC_WAVE | ATMEL_TC_WAVESEL_UP_AUTO,
+ tcaddr + ATMEL_TC_REG(2, CMR));
+ __raw_writel((32768 + HZ/2) / HZ, tcaddr + ATMEL_TC_REG(2, RC));
+
+ /* E
[patch 2.6.25-rc2-git 1/2] atmel_tc library
Create based on and the
at91sam9263 and at32ap7000 datasheets. Most AT91 and AT32 SOCs have one
or two of these TC blocks, which include three 16-bit timers that can be
interconnected in various ways.
These TC blocks can be used for external interfacing (such as PWM and
measurement), or used as somewhat quirky sixteen-bit timers.
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
---
Note that this won't be usable until the AT91 and AT32 platforms
incorporate patches to configure the relevant platform devices.
Those changes are probably 2.6.26 material.
drivers/misc/Kconfig |8 +
drivers/misc/Makefile |1
drivers/misc/atmel_tclib.c | 107 +
include/linux/atmel_tc.h | 221 +
4 files changed, 337 insertions(+)
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -22,6 +22,14 @@ config ATMEL_PWM
purposes including software controlled power-efficent backlights
on LCD displays, motor control, and waveform generation.
+config ATMEL_TCLIB
+ bool "Atmel AT32/AT91 Timer/Counter Library"
+ depends on (AVR32 || ARCH_AT91)
+ help
+ Select this if you want a library to allocate the Timer/Counter
+ blocks found on many Atmel processors. This facilitates using
+ these blocks by different drivers despite processor differences.
+
config IBM_ASM
tristate "Device driver for IBM RSA service processor"
depends on X86 && PCI && INPUT && EXPERIMENTAL
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ACER_WMI) += acer-wmi.o
obj-$(CONFIG_ASUS_LAPTOP) += asus-laptop.o
obj-$(CONFIG_ATMEL_PWM)+= atmel_pwm.o
obj-$(CONFIG_ATMEL_SSC)+= atmel-ssc.o
+obj-$(CONFIG_ATMEL_TCLIB) += atmel_tclib.o
obj-$(CONFIG_TC1100_WMI) += tc1100-wmi.o
obj-$(CONFIG_LKDTM)+= lkdtm.o
obj-$(CONFIG_TIFM_CORE)+= tifm_core.o
--- /dev/null
+++ b/drivers/misc/atmel_tclib.c
@@ -0,0 +1,107 @@
+#include
+#include
+#include
+#include
+#include
+#include
+
+
+/*
+ * This is a thin library to solve the problem of how to portably allocate
+ * one of the TC blocks. For simplicity, it doesn't currently expect to
+ * share individual timers between different drivers.
+ */
+
+#if defined(CONFIG_AVR32)
+/* AVR32 has these divide PBB */
+const u8 atmel_tc_divisors[5] = { 0, 4, 8, 16, 32, };
+EXPORT_SYMBOL(atmel_tc_divisors);
+
+#elif defined(CONFIG_ARCH_AT91)
+/* AT91 has these divide MCK */
+const u8 atmel_tc_divisors[5] = { 2, 8, 32, 128, 0, };
+EXPORT_SYMBOL(atmel_tc_divisors);
+
+#endif
+
+/* we "know" that there will be either one or two TC blocks */
+static struct platform_device *blocks[2];
+
+
+/**
+ * atmel_tc_alloc - allocate a specified TC block
+ * @block: which block to allocate
+ * @iomem: used to return its IO memory resource
+ *
+ * Caller allocates a block. If it is available, its I/O space is requested
+ * and returned through the iomem pointer, and the device node for the block
+ * is returned. When it is not available, NULL is returned.
+ *
+ * On some platforms, each TC channel has its own clocks and IRQs. Drivers
+ * should clk_get() and clk_enable() "t0_clk", "t1_clk, and "t2_clk".
+ * In the same vein, they should platform_get_irq() for irqs 0, 1, and 2.
+ * On other platforms, only irq 0 and "t0_clk" will be available; drivers
+ * should handle with both configurations.
+ */
+struct platform_device *atmel_tc_alloc(unsigned block, struct resource **iomem)
+{
+ struct platform_device *tc;
+ struct resource *r;
+
+ if (block >= ARRAY_SIZE(blocks) || !iomem)
+ return NULL;
+
+ tc = blocks[block];
+ if (tc) {
+ r = platform_get_resource(tc, IORESOURCE_MEM, 0);
+ if (r)
+ r = request_mem_region(r->start, 256, NULL);
+ *iomem = r;
+ if (!r)
+ tc = NULL;
+ }
+
+ return tc;
+}
+EXPORT_SYMBOL_GPL(atmel_tc_alloc);
+
+/**
+ * atmel_tc_free - release a specified TC block
+ * @block: which block to release
+ *
+ * This reverses the effect of atmel_tc_alloc(), invalidating the resource
+ * returned by that routine and making the TC available to other drivers.
+ */
+void atmel_tc_free(struct platform_device *tc)
+{
+ if (tc->id >= 0 && tc->id < ARRAY_SIZE(blocks)) {
+ struct resource *r;
+
+ r = platform_get_resource(tc, IORESOURCE_MEM, 0);
+ release_mem_region(r->start, 256);
+ }
+}
+EXPORT_SYMBOL_GPL(atmel_tc_free);
+
+static int __init tc_probe(struct platform_device *pdev)
+{
+ static char __initdata e2big[] =
+ KERN_ERR "tclib: can't record TC block %d\n";
+
+ if (pdev->id < 0
[patch 2.6.25-rc2-git] gpio: and "no GPIO support here" stubs
Add a defining fail/warn stubs for GPIO calls on platforms
that don't support the GPIO programming interface. That includes the
arch-specific implementation glue otherwise.
This facilitates a new model for GPIO usage: drivers that can use GPIOs
if they're available, but don't require them. One example of such a
driver is NAND driver for various FreeScale chips. On platforms update
with GPIO support, they can be used instead of a worst-case delay to
verify that the BUSY signal is off.
(Also includes a couple minor unrelated doc updates.)
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
---
Please include before 2.6.25-final ...
Documentation/gpio.txt | 16 ++--
include/linux/gpio.h | 95 +
2 files changed, 107 insertions(+), 4 deletions(-)
--- g26.orig/Documentation/gpio.txt 2008-02-22 16:20:36.0 -0800
+++ g26/Documentation/gpio.txt 2008-02-22 17:02:00.0 -0800
@@ -2,6 +2,9 @@ GPIO Interfaces
This provides an overview of GPIO access conventions on Linux.
+These calls use the gpio_* naming prefix. No other calls should use that
+prefix, or the related __gpio_* prefix.
+
What is a GPIO?
===
@@ -69,11 +72,13 @@ in this document, but drivers acting as
not care how it's implemented.)
That said, if the convention is supported on their platform, drivers should
-use it when possible. Platforms should declare GENERIC_GPIO support in
-Kconfig (boolean true), which multi-platform drivers can depend on when
-using the include file:
+use it when possible. Platforms must declare GENERIC_GPIO support in their
+Kconfig (boolean true), and provide an file. Drivers that can't
+work without standard GPIO calls should have Kconfig entries which depend
+on GENERIC_GPIO. The GPIO calls are available, either as "real code" or as
+optimized-away stubs, when drivers use the include file:
- #include
+ #include
If you stick to this convention then it'll be easier for other developers to
see what your code is doing, and help maintain it.
@@ -326,6 +331,9 @@ pulldowns integrated on some platforms.
or support them in the same way; and any given board might use external
pullups (or pulldowns) so that the on-chip ones should not be used.
(When a circuit needs 5 kOhm, on-chip 100 kOhm resistors won't do.)
+Likewise drive strength (2 mA vs 20 mA) and voltage (1.8V vs 3.3V) is a
+platform-specific issue, as are models like (not) having a one-to-one
+correspondence between configurable pins and GPIOs.
There are other system-specific mechanisms that are not specified here,
like the aforementioned options for input de-glitching and wire-OR output.
--- /dev/null 1970-01-01 00:00:00.0 +
+++ g26/include/linux/gpio.h2008-02-22 17:05:48.0 -0800
@@ -0,0 +1,95 @@
+#ifndef __LINUX_GPIO_H
+#define __LINUX_GPIO_H
+
+/* see Documentation/gpio.txt */
+
+#ifdef CONFIG_GENERIC_GPIO
+#include
+
+#else
+
+/*
+ * Some platforms don't support the GPIO programming interface.
+ *
+ * In case some driver uses it anyway (it should normally have
+ * depended on GENERIC_GPIO), these routines help the compiler
+ * optimize out much GPIO-related code ... or trigger a runtime
+ * warning when something is wrongly called.
+ */
+
+static inline int gpio_is_valid(int number)
+{
+ return 0;
+}
+
+static inline int gpio_request(unsigned gpio, const char *label)
+{
+ return -ENOSYS;
+}
+
+static inline void gpio_free(unsigned gpio)
+{
+ /* GPIO can never have been requested */
+ WARN_ON(1);
+}
+
+static inline int gpio_direction_input(unsigned gpio)
+{
+ return -ENOSYS;
+}
+
+static inline int gpio_direction_output(unsigned gpio, int value)
+{
+ return -ENOSYS;
+}
+
+static inline int gpio_get_value(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline void gpio_set_value(unsigned gpio, int value)
+{
+ /* GPIO can never have been requested or set as output */
+ WARN_ON(1);
+}
+
+static inline int gpio_cansleep(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline int gpio_get_value_cansleep(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline void gpio_set_value_cansleep(unsigned gpio, int value)
+{
+ /* GPIO can never have been requested or set as output */
+ WARN_ON(1);
+}
+
+static inline int gpio_to_irq(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as input */
+ WARN_ON(1);
+ return -EINVAL;
+}
+
+static inline int irq_to_gpio(unsigned irq)
+{
+ /* irq can never have been returned from gpio_to_irq() */
+ WARN_ON(1);
+ return -EINVAL;
+}
+
+#endif
+
+#endif /* __LINUX_GPIO_H */
--
To unsubscrib
Re: [PATCH 1/4 resend] [x86] Add generic GPIO support to x86
On Friday 22 February 2008, Anton Vorontsov wrote:
> > +static inline int gpio_request(unsigned gpio, const char *label)
> > +{
> > + return -EINVAL;
>
> Could we return -ENOSYS instead, so drivers will able to distinguish
> between "something really failed" and "there is no gpio support,
> fallback to other methods"? Without this, the notorious NAND driver
> will hardly benefit from this change. ;-)
Sure; that'll be gpio_request() and the gpio_direction_*() calls only
though, since those are the initial setup calls drivers make.
- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [UPDATED v2][PATCH 0/6] regulator: voltage and current regulator framework
> > For those PMIC that covers additional features, like > > - usb vbus detection (or pull-up/pull-down) > > - audio codec > > - touch screen > > - battery monitor/ fuel gauge > > - battery charger > > - possible many others Certainly many others ... like MMC transceivers, high speed USB transceivers, RTCs, and lots of other analog and "high voltage" (more than 1.8V, say) circuits that should be offloaded from SOCs for systems built with 90nm (and smaller) processes. > > How do you plan to handle them? > > The WM8350 and MC13783 are both multi feature PMIC's like above. We > handle WM8350 PMIC access via a bus manager. I think most such PMIC cases are best modeled as multi-function devices, plugging into numerous different frameworks. That may be implied by "bus manager". There are already at least two PMIC drivers in drivers/i2c/chips in the mainstream kernel (tps65010.c and menelaus.c) and I'd think one factor to review of this framework is whether those chips could reasonbly be used in this framework. So for example the tps65010 presumes that fuel gauging is done by a separate chip (presumptively using HDQ/1-Wire), and in fact that may be part of a battery pack. And the DAC functions, like touchscreens and (input) codecs, use other dedicated chips. As time goes by, those PMIC chips integrate lots more functionality.. - Dave > This controls IO access to > the WM8350 so that client driver (including the regulator driver) IO > does not collide. We also cache non volatile PMIC registers to speed up > access. Please have a look at drivers/regulator/wm8350/wm8350-bus.c in > the imx31 branch for details. > > Liam > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer
> > If the driver could not handle zero length transfer, then the driver > > should reject it (just like unsupported transfer mode). Then the > > behavior will be 'assert chip select and wait some time' or 'rejected > > by the driver'. > > This would be OK. It would not be hard to fix pxa2xx_spi, for example, > to reject zero-length transfers in DMA mode, as long as it is acceptable > to reject the message in mid-message. Such "illegal message" rejection is best done early; "fail-fast". Mid-message rejection isn't wrong, but it's a rude policy. It'd be best to fix pxa2xx_spi to not start zero-length DMAs. > If it were necessary to scan a > whole message for zero-length transfers and refuse to queue an offending > message, then that adds burden to all messages. Sanity checking messages as they're submitted is easy; and it's also the natural point for setting up DMA mappings (and making those cachelines available for better use). - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer
> >> David, do you think writing 0 bytes is a valid use of this API? > > > > Just a zero byte transfer ... no, though it depends what you mean > > by "valid". (I'm not sure I'd expect all controller drivers to > > reject such requests.) That has no effect on bits-on-the-wire, > > and would make trouble for various DMA engines. > > FWIW, the pxa2xx_spi driver does not, near as I can tell, reject zero > length transfers, it will go through the motions, the same as for any > other transfer. Makes sense to me ... although: > However, if the transfer is by DMA, note that the PXA255 and PXA270 > Developer's Manuals have the following language regarding DMA lengths: > > LEN = 0 means zero bytes for descriptor-fetch transactions. > LEN = 0 is an invalid setting for no-descriptor-fetch > transactions. ... > > Because the pxa2xx_spi driver does not currently use DMA descriptors, > zero length DMAs are invalid. In that case the pxa2xx_spi driver should add a special case to avoid starting such transfers in DMA mode. > > Passing zero bytes to get an inline delay at an exact spot in the > > overall protocol message ... I don't see why not. Better than > > adding delay fields for every spot it might be needed by various > > oddball devices, for sure!! > > I agree with Marc: any such delay will be undefined, in the general > case. It might work for a specific driver implementation. Is that what Marc said? I couldn't tell. In any case, I disagree; the semantics of that delay are clearly defined. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer
Quoth Atsushi Nemoto on Fri, 22 Feb 2008: > On Fri, 22 Feb 2008 10:30:31 +0100, Marc Pignat <[EMAIL PROTECTED]> wrote: > > > > David, do you think writing 0 bytes is a valid use of this API? > > > > > > Just a zero byte transfer ... no, though it depends what you mean > > > by "valid". (I'm not sure I'd expect all controller drivers to > > > reject such requests.) That has no effect on bits-on-the-wire, > > > and would make trouble for various DMA engines. > > > > So, the behaviour is undefined, Not what I said. To repeat: it makes sense to pass zero bytes in at least one case, which is not "just" a zero byte transfer. And in a practical sense, until we have some kind of regression testing scheme -- with some kind of "golden device" -- it's not very sensible for any SPI Protocol Driver to expect that all SPI Master Controller Drivers act consistently in such cases. > > something between 'crash my dma engine', > > 'assert chip select and wait some time', or 'do_nothing'... > > If the driver could not handle zero length transfer, then the driver > should reject it (just like unsupported transfer mode). Exactly. Behaviors like "crash my DMA engine" are clearly "invalid", in *ALL* cases. Bugs to get fixed as soon as they're noticed. > Then the > behavior will be 'assert chip select and wait some time' or 'rejected > by the driver'. The "wait" mode is what started this thread -- not "just" a zero byte transfer, but one which does real work. For "just" a zero byte transfer, I see two main implementation options ... with no compelling reason to force either one. - "ignored" ... the implementation sibling of "wait" - "rejected" ... more work The argument for "rejected" would seem to be only that this is a case of "protocol drivers should not do this". But if they don't, then the difference doesn't matter. > > > And it would probably deserve a mode flag (sigh) unless someone > > > can update every master controller driver. > > > > Supporting the zero-len-write means checking and perhpaps updating > > each driver for the benefit of having an unknown length delay. > > > > We should add the delay field in the spi_device, but this means more work. > > > > Is this kind of device so common that we need to do all that work? or can we > > leave it as is (verified to work only with atmel_spi)? > > I think my case is not so common. But if the driver could support > zero length transfer easily, there is no reason to reject it. > > And if nobody wanted to support zero length transfer on that driver, > it would be no reason to update it ;) So long as the controller driver doesn't misbehave, I can't see any reason to worry about this behavior. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer
Quoth Atsushi Nemoto on Fri, 22 Feb 2008: On Fri, 22 Feb 2008 10:30:31 +0100, Marc Pignat [EMAIL PROTECTED] wrote: David, do you think writing 0 bytes is a valid use of this API? Just a zero byte transfer ... no, though it depends what you mean by valid. (I'm not sure I'd expect all controller drivers to reject such requests.) That has no effect on bits-on-the-wire, and would make trouble for various DMA engines. So, the behaviour is undefined, Not what I said. To repeat: it makes sense to pass zero bytes in at least one case, which is not just a zero byte transfer. And in a practical sense, until we have some kind of regression testing scheme -- with some kind of golden device -- it's not very sensible for any SPI Protocol Driver to expect that all SPI Master Controller Drivers act consistently in such cases. something between 'crash my dma engine', 'assert chip select and wait some time', or 'do_nothing'... If the driver could not handle zero length transfer, then the driver should reject it (just like unsupported transfer mode). Exactly. Behaviors like crash my DMA engine are clearly invalid, in *ALL* cases. Bugs to get fixed as soon as they're noticed. Then the behavior will be 'assert chip select and wait some time' or 'rejected by the driver'. The wait mode is what started this thread -- not just a zero byte transfer, but one which does real work. For just a zero byte transfer, I see two main implementation options ... with no compelling reason to force either one. - ignored ... the implementation sibling of wait - rejected ... more work The argument for rejected would seem to be only that this is a case of protocol drivers should not do this. But if they don't, then the difference doesn't matter. And it would probably deserve a mode flag (sigh) unless someone can update every master controller driver. Supporting the zero-len-write means checking and perhpaps updating each driver for the benefit of having an unknown length delay. We should add the delay field in the spi_device, but this means more work. Is this kind of device so common that we need to do all that work? or can we leave it as is (verified to work only with atmel_spi)? I think my case is not so common. But if the driver could support zero length transfer easily, there is no reason to reject it. And if nobody wanted to support zero length transfer on that driver, it would be no reason to update it ;) So long as the controller driver doesn't misbehave, I can't see any reason to worry about this behavior. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer
David, do you think writing 0 bytes is a valid use of this API? Just a zero byte transfer ... no, though it depends what you mean by valid. (I'm not sure I'd expect all controller drivers to reject such requests.) That has no effect on bits-on-the-wire, and would make trouble for various DMA engines. FWIW, the pxa2xx_spi driver does not, near as I can tell, reject zero length transfers, it will go through the motions, the same as for any other transfer. Makes sense to me ... although: However, if the transfer is by DMA, note that the PXA255 and PXA270 Developer's Manuals have the following language regarding DMA lengths: LEN = 0 means zero bytes for descriptor-fetch transactions. LEN = 0 is an invalid setting for no-descriptor-fetch transactions. ... Because the pxa2xx_spi driver does not currently use DMA descriptors, zero length DMAs are invalid. In that case the pxa2xx_spi driver should add a special case to avoid starting such transfers in DMA mode. Passing zero bytes to get an inline delay at an exact spot in the overall protocol message ... I don't see why not. Better than adding delay fields for every spot it might be needed by various oddball devices, for sure!! I agree with Marc: any such delay will be undefined, in the general case. It might work for a specific driver implementation. Is that what Marc said? I couldn't tell. In any case, I disagree; the semantics of that delay are clearly defined. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer
If the driver could not handle zero length transfer, then the driver should reject it (just like unsupported transfer mode). Then the behavior will be 'assert chip select and wait some time' or 'rejected by the driver'. This would be OK. It would not be hard to fix pxa2xx_spi, for example, to reject zero-length transfers in DMA mode, as long as it is acceptable to reject the message in mid-message. Such illegal message rejection is best done early; fail-fast. Mid-message rejection isn't wrong, but it's a rude policy. It'd be best to fix pxa2xx_spi to not start zero-length DMAs. If it were necessary to scan a whole message for zero-length transfers and refuse to queue an offending message, then that adds burden to all messages. Sanity checking messages as they're submitted is easy; and it's also the natural point for setting up DMA mappings (and making those cachelines available for better use). - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [UPDATED v2][PATCH 0/6] regulator: voltage and current regulator framework
For those PMIC that covers additional features, like - usb vbus detection (or pull-up/pull-down) - audio codec - touch screen - battery monitor/ fuel gauge - battery charger - possible many others Certainly many others ... like MMC transceivers, high speed USB transceivers, RTCs, and lots of other analog and high voltage (more than 1.8V, say) circuits that should be offloaded from SOCs for systems built with 90nm (and smaller) processes. How do you plan to handle them? The WM8350 and MC13783 are both multi feature PMIC's like above. We handle WM8350 PMIC access via a bus manager. I think most such PMIC cases are best modeled as multi-function devices, plugging into numerous different frameworks. That may be implied by bus manager. There are already at least two PMIC drivers in drivers/i2c/chips in the mainstream kernel (tps65010.c and menelaus.c) and I'd think one factor to review of this framework is whether those chips could reasonbly be used in this framework. So for example the tps65010 presumes that fuel gauging is done by a separate chip (presumptively using HDQ/1-Wire), and in fact that may be part of a battery pack. And the DAC functions, like touchscreens and (input) codecs, use other dedicated chips. As time goes by, those PMIC chips integrate lots more functionality.. - Dave This controls IO access to the WM8350 so that client driver (including the regulator driver) IO does not collide. We also cache non volatile PMIC registers to speed up access. Please have a look at drivers/regulator/wm8350/wm8350-bus.c in the imx31 branch for details. Liam -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4 resend] [x86] Add generic GPIO support to x86
On Friday 22 February 2008, Anton Vorontsov wrote:
+static inline int gpio_request(unsigned gpio, const char *label)
+{
+ return -EINVAL;
Could we return -ENOSYS instead, so drivers will able to distinguish
between something really failed and there is no gpio support,
fallback to other methods? Without this, the notorious NAND driver
will hardly benefit from this change. ;-)
Sure; that'll be gpio_request() and the gpio_direction_*() calls only
though, since those are the initial setup calls drivers make.
- Dave
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch 2.6.25-rc2-git] gpio: linux/gpio.h and no GPIO support here stubs
Add a linux/gpio.h defining fail/warn stubs for GPIO calls on platforms
that don't support the GPIO programming interface. That includes the
arch-specific implementation glue otherwise.
This facilitates a new model for GPIO usage: drivers that can use GPIOs
if they're available, but don't require them. One example of such a
driver is NAND driver for various FreeScale chips. On platforms update
with GPIO support, they can be used instead of a worst-case delay to
verify that the BUSY signal is off.
(Also includes a couple minor unrelated doc updates.)
Signed-off-by: David Brownell [EMAIL PROTECTED]
---
Please include before 2.6.25-final ...
Documentation/gpio.txt | 16 ++--
include/linux/gpio.h | 95 +
2 files changed, 107 insertions(+), 4 deletions(-)
--- g26.orig/Documentation/gpio.txt 2008-02-22 16:20:36.0 -0800
+++ g26/Documentation/gpio.txt 2008-02-22 17:02:00.0 -0800
@@ -2,6 +2,9 @@ GPIO Interfaces
This provides an overview of GPIO access conventions on Linux.
+These calls use the gpio_* naming prefix. No other calls should use that
+prefix, or the related __gpio_* prefix.
+
What is a GPIO?
===
@@ -69,11 +72,13 @@ in this document, but drivers acting as
not care how it's implemented.)
That said, if the convention is supported on their platform, drivers should
-use it when possible. Platforms should declare GENERIC_GPIO support in
-Kconfig (boolean true), which multi-platform drivers can depend on when
-using the include file:
+use it when possible. Platforms must declare GENERIC_GPIO support in their
+Kconfig (boolean true), and provide an asm/gpio.h file. Drivers that can't
+work without standard GPIO calls should have Kconfig entries which depend
+on GENERIC_GPIO. The GPIO calls are available, either as real code or as
+optimized-away stubs, when drivers use the include file:
- #include asm/gpio.h
+ #include linux/gpio.h
If you stick to this convention then it'll be easier for other developers to
see what your code is doing, and help maintain it.
@@ -326,6 +331,9 @@ pulldowns integrated on some platforms.
or support them in the same way; and any given board might use external
pullups (or pulldowns) so that the on-chip ones should not be used.
(When a circuit needs 5 kOhm, on-chip 100 kOhm resistors won't do.)
+Likewise drive strength (2 mA vs 20 mA) and voltage (1.8V vs 3.3V) is a
+platform-specific issue, as are models like (not) having a one-to-one
+correspondence between configurable pins and GPIOs.
There are other system-specific mechanisms that are not specified here,
like the aforementioned options for input de-glitching and wire-OR output.
--- /dev/null 1970-01-01 00:00:00.0 +
+++ g26/include/linux/gpio.h2008-02-22 17:05:48.0 -0800
@@ -0,0 +1,95 @@
+#ifndef __LINUX_GPIO_H
+#define __LINUX_GPIO_H
+
+/* see Documentation/gpio.txt */
+
+#ifdef CONFIG_GENERIC_GPIO
+#include asm/gpio.h
+
+#else
+
+/*
+ * Some platforms don't support the GPIO programming interface.
+ *
+ * In case some driver uses it anyway (it should normally have
+ * depended on GENERIC_GPIO), these routines help the compiler
+ * optimize out much GPIO-related code ... or trigger a runtime
+ * warning when something is wrongly called.
+ */
+
+static inline int gpio_is_valid(int number)
+{
+ return 0;
+}
+
+static inline int gpio_request(unsigned gpio, const char *label)
+{
+ return -ENOSYS;
+}
+
+static inline void gpio_free(unsigned gpio)
+{
+ /* GPIO can never have been requested */
+ WARN_ON(1);
+}
+
+static inline int gpio_direction_input(unsigned gpio)
+{
+ return -ENOSYS;
+}
+
+static inline int gpio_direction_output(unsigned gpio, int value)
+{
+ return -ENOSYS;
+}
+
+static inline int gpio_get_value(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline void gpio_set_value(unsigned gpio, int value)
+{
+ /* GPIO can never have been requested or set as output */
+ WARN_ON(1);
+}
+
+static inline int gpio_cansleep(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline int gpio_get_value_cansleep(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline void gpio_set_value_cansleep(unsigned gpio, int value)
+{
+ /* GPIO can never have been requested or set as output */
+ WARN_ON(1);
+}
+
+static inline int gpio_to_irq(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as input */
+ WARN_ON(1);
+ return -EINVAL;
+}
+
+static inline int irq_to_gpio(unsigned irq)
+{
+ /* irq can never have been returned from gpio_to_irq() */
+ WARN_ON(1);
+ return -EINVAL;
+}
+
+#endif
+
+#endif
[patch 2.6.25-rc2-git 1/2] atmel_tc library
Create linux/atmel_tc.h based on asm-arm/arch-at91/at91-tc.h and the
at91sam9263 and at32ap7000 datasheets. Most AT91 and AT32 SOCs have one
or two of these TC blocks, which include three 16-bit timers that can be
interconnected in various ways.
These TC blocks can be used for external interfacing (such as PWM and
measurement), or used as somewhat quirky sixteen-bit timers.
Signed-off-by: David Brownell [EMAIL PROTECTED]
---
Note that this won't be usable until the AT91 and AT32 platforms
incorporate patches to configure the relevant platform devices.
Those changes are probably 2.6.26 material.
drivers/misc/Kconfig |8 +
drivers/misc/Makefile |1
drivers/misc/atmel_tclib.c | 107 +
include/linux/atmel_tc.h | 221 +
4 files changed, 337 insertions(+)
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -22,6 +22,14 @@ config ATMEL_PWM
purposes including software controlled power-efficent backlights
on LCD displays, motor control, and waveform generation.
+config ATMEL_TCLIB
+ bool Atmel AT32/AT91 Timer/Counter Library
+ depends on (AVR32 || ARCH_AT91)
+ help
+ Select this if you want a library to allocate the Timer/Counter
+ blocks found on many Atmel processors. This facilitates using
+ these blocks by different drivers despite processor differences.
+
config IBM_ASM
tristate Device driver for IBM RSA service processor
depends on X86 PCI INPUT EXPERIMENTAL
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ACER_WMI) += acer-wmi.o
obj-$(CONFIG_ASUS_LAPTOP) += asus-laptop.o
obj-$(CONFIG_ATMEL_PWM)+= atmel_pwm.o
obj-$(CONFIG_ATMEL_SSC)+= atmel-ssc.o
+obj-$(CONFIG_ATMEL_TCLIB) += atmel_tclib.o
obj-$(CONFIG_TC1100_WMI) += tc1100-wmi.o
obj-$(CONFIG_LKDTM)+= lkdtm.o
obj-$(CONFIG_TIFM_CORE)+= tifm_core.o
--- /dev/null
+++ b/drivers/misc/atmel_tclib.c
@@ -0,0 +1,107 @@
+#include linux/init.h
+#include linux/kernel.h
+#include linux/mutex.h
+#include linux/ioport.h
+#include linux/platform_device.h
+#include linux/atmel_tc.h
+
+
+/*
+ * This is a thin library to solve the problem of how to portably allocate
+ * one of the TC blocks. For simplicity, it doesn't currently expect to
+ * share individual timers between different drivers.
+ */
+
+#if defined(CONFIG_AVR32)
+/* AVR32 has these divide PBB */
+const u8 atmel_tc_divisors[5] = { 0, 4, 8, 16, 32, };
+EXPORT_SYMBOL(atmel_tc_divisors);
+
+#elif defined(CONFIG_ARCH_AT91)
+/* AT91 has these divide MCK */
+const u8 atmel_tc_divisors[5] = { 2, 8, 32, 128, 0, };
+EXPORT_SYMBOL(atmel_tc_divisors);
+
+#endif
+
+/* we know that there will be either one or two TC blocks */
+static struct platform_device *blocks[2];
+
+
+/**
+ * atmel_tc_alloc - allocate a specified TC block
+ * @block: which block to allocate
+ * @iomem: used to return its IO memory resource
+ *
+ * Caller allocates a block. If it is available, its I/O space is requested
+ * and returned through the iomem pointer, and the device node for the block
+ * is returned. When it is not available, NULL is returned.
+ *
+ * On some platforms, each TC channel has its own clocks and IRQs. Drivers
+ * should clk_get() and clk_enable() t0_clk, t1_clk, and t2_clk.
+ * In the same vein, they should platform_get_irq() for irqs 0, 1, and 2.
+ * On other platforms, only irq 0 and t0_clk will be available; drivers
+ * should handle with both configurations.
+ */
+struct platform_device *atmel_tc_alloc(unsigned block, struct resource **iomem)
+{
+ struct platform_device *tc;
+ struct resource *r;
+
+ if (block = ARRAY_SIZE(blocks) || !iomem)
+ return NULL;
+
+ tc = blocks[block];
+ if (tc) {
+ r = platform_get_resource(tc, IORESOURCE_MEM, 0);
+ if (r)
+ r = request_mem_region(r-start, 256, NULL);
+ *iomem = r;
+ if (!r)
+ tc = NULL;
+ }
+
+ return tc;
+}
+EXPORT_SYMBOL_GPL(atmel_tc_alloc);
+
+/**
+ * atmel_tc_free - release a specified TC block
+ * @block: which block to release
+ *
+ * This reverses the effect of atmel_tc_alloc(), invalidating the resource
+ * returned by that routine and making the TC available to other drivers.
+ */
+void atmel_tc_free(struct platform_device *tc)
+{
+ if (tc-id = 0 tc-id ARRAY_SIZE(blocks)) {
+ struct resource *r;
+
+ r = platform_get_resource(tc, IORESOURCE_MEM, 0);
+ release_mem_region(r-start, 256);
+ }
+}
+EXPORT_SYMBOL_GPL(atmel_tc_free);
+
+static int __init tc_probe(struct platform_device *pdev)
+{
+ static char __initdata e2big[] =
+ KERN_ERR tclib: can't record TC block %d\n;
+
+ if (pdev-id 0 || pdev-id = ARRAY_SIZE(blocks
[patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
Clocksource and clockevent device based on the Atmel TC blocks.
The clockevent device handles both periodic and oneshot modes, so this
enables NO_HZ and high res timers on some platforms that previously
couldn't use those mechanisms.
This works on both AVR32 and AT91 chips, given relevant patches for
tclib support (always) and clockevents (or else this will only look
like a higher precision clocksource). It's an updated and modularized
version of an AT91-only patch that has circulated for some time now.
Signed-off-by: David Brownell [EMAIL PROTECTED]
---
As with the preceding patch, this depends on platform updates that
won't merge before 2.6.26 ... in this case, updates to switch over
to clocksources (at91sam9 chips, but not at91rm9200) and clockevents
(avr32 and at91sam9), on top of platform_device setup.
drivers/clocksource/Makefile |1
drivers/clocksource/tcb_clksrc.c | 303 +++
drivers/misc/Kconfig | 25 +++
3 files changed, 329 insertions(+)
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_ATMEL_TCB_CLKSRC) += tcb_clksrc.o
obj-$(CONFIG_X86_CYCLONE_TIMER)+= cyclone.o
obj-$(CONFIG_X86_PM_TIMER) += acpi_pm.o
obj-$(CONFIG_SCx200HR_TIMER) += scx200_hrt.o
--- /dev/null
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -0,0 +1,303 @@
+#include linux/init.h
+#include linux/clocksource.h
+#include linux/clockchips.h
+#include linux/interrupt.h
+#include linux/irq.h
+
+#include linux/clk.h
+#include linux/err.h
+#include linux/ioport.h
+#include linux/io.h
+#include linux/platform_device.h
+#include linux/atmel_tc.h
+
+
+/*
+ * We're configured to use a specific TC block, one that's not hooked
+ * up to external hardware, to provide a time solution:
+ *
+ * - Two channels combine to create a free-running 32 bit counter
+ * with a base rate of 5+ MHz, packaged as a clocksource (with
+ * resolution better than 200 nsec).
+ *
+ * - The third channel may be used to provide a 16-bit clockevent
+ * source, used in either periodic or oneshot mode. This runs
+ * at 32 KiHZ, and can handle delays of up to two seconds.
+ *
+ * A boot clocksource and clockevent source are also currently needed,
+ * unless the relevant platforms (ARM/AT91, AVR32/AT32) are changed so
+ * this code can be used when init_timers() is called, well before most
+ * devices are set up. (Some low end AT91 parts, which can run uClinux,
+ * have only the timers in one TC block... they currently don't support
+ * the tclib code, because of that initialization issue.)
+ *
+ * REVISIT behavior during system suspend states... we should disable
+ * all clocks and save the power. Easily done for clockevent devices,
+ * but clocksources won't necessarily get the needed notifications.
+ * For deeper system sleep states, this will be mandatory...
+ */
+
+static void __iomem *tcaddr;
+
+static cycle_t tc_get_cycles(void)
+{
+ unsigned long flags;
+ u32 lower, upper;
+
+ raw_local_irq_save(flags);
+retry:
+ upper = __raw_readl(tcaddr + ATMEL_TC_REG(1, CV));
+ lower = __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
+ if (upper != __raw_readl(tcaddr + ATMEL_TC_REG(1, CV)))
+ goto retry;
+
+ raw_local_irq_restore(flags);
+ return (upper 16) | lower;
+}
+
+static struct clocksource clksrc = {
+ .name = tcb_clksrc,
+ .rating = 200,
+ .read = tc_get_cycles,
+ .mask = CLOCKSOURCE_MASK(32),
+ .shift = 18,
+ .flags = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+#define USE_TC_CLKEVT
+#endif
+
+#ifdef CONFIG_ARCH_AT91RM9200
+/* The AT91rm9200 system timer is a oneshot-capable 32k timer that's
+ * always running ... configuring a TC channel to work the same way
+ * would just waste some power.
+ */
+#undef USE_TC_CLKEVT
+#endif
+
+#ifdef USE_TC_CLKEVT
+
+/* For now, we always use the 32K clock ... this optimizes for NO_HZ,
+ * because using one of the divided clocks would usually mean the
+ * tick rate can never be less than several dozen Hz (vs 0.5 Hz).
+ *
+ * A divided clock could be good for high resolution timers, since
+ * 30.5 usec resolution can seem low.
+ */
+static u32 timer_clock;
+
+static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
+{
+ __raw_writel(0xff, tcaddr + ATMEL_TC_REG(2, IDR));
+ __raw_writel(ATMEL_TC_CLKDIS, tcaddr + ATMEL_TC_REG(2, CCR));
+
+ switch (m) {
+
+ /* By not making the gentime core emulate periodic mode on top
+* of oneshot, we get lower overhead and improved accuracy.
+*/
+ case CLOCK_EVT_MODE_PERIODIC:
+ /* slow clock, count up to RC, then irq and restart */
+ __raw_writel(timer_clock
+ | ATMEL_TC_WAVE | ATMEL_TC_WAVESEL_UP_AUTO,
+ tcaddr
Re: [spi-devel-general] [PATCH] atmel_spi: support zero length transfer
On Friday 22 February 2008, Ned Forrester wrote: So, what I think you said is that it would be better for pxa2xx_spi to silently ignore a zero-length message, passing it back with the rest of the message when all is complete, than to reject the message. Yes. Remember that the reason to _use_ such a message is to get the inline delay ... and that if you've got code to handle that case (transfer.len == 0 transfer.delay_usecs != 0) it's almost trivial to support the degenerate delay_usecs == 0 case too. I see no reason why that could not be done, though it may be tricky to set other things like SSP modes and chip select and *not* start the DMA. It would have to be tested, so I'm not sure when I could try that. Normally I'd expect it's little more than a goto, but the pxa2xx_spi structure is relatively complex. I agree with Marc: any such delay will be undefined, in the general case. It might work for a specific driver implementation. Is that what Marc said? I couldn't tell. In any case, I disagree; the semantics of that delay are clearly define. Maybe I am missing something. Aren't we talking about a transfer in a message, with or without other transfers, who's only unique characteristic is that that its length is zero? There were two cases ... delay_usecs being zero, or not. In either case, the semantics are the same: after the transfer (len == 0), delay that many microseconds (zero or more). Or are you and Atsushi talking about using spi_transfer.delay_usecs *with* a zero length transfer to effectively put a delay between the assertion of CS and the start of the first clock? If so, then I guess I missed the original point. Sorry. As I noted, there are two cases. The reason to use a zero length transfer is to get a delay ... in his case, specifically before the first clock, but in general *anywhere* in the transfer. But the degenerate case is delay for zero microseconds. -- By the way, reading spi.h again, it looks like spi_transfer.delay_usecs is supposed to be implemented between the last bit movement of a transfer and any change of CS at the end of a transfer. Is that right? Yes. I think that pxa2xx_spi is dropping CS, if requested, immediately at the end of transfer, and then putting spi_transfer.delay_usecs between that transfer and the next. That's a bug then; it will matter with some drivers. If it were necessary to scan a whole message for zero-length transfers and refuse to queue an offending message, then that adds burden to all messages. Sanity checking messages as they're submitted is easy; and it's also the natural point for setting up DMA mappings (and making those cachelines available for better use). Hmmm Obviously I have much to learn about modern computers. It had not occurred to me, even after reading Linux Device Drivers, Corbet, et.al, that by DMA mapping, one frees the cache for other uses. It makes sense. That's certainly what happens on systems where the buffer is from normal memory and the cache is DMA-incoherent ... like most ARMs, and probably the majority of non-x86 hardware. Think of that as being a level or two deeper than what LDD covers. In my application I pass many large buffers to the master driver, and I map them in the protocol driver. I did that without good reason, but now I see it was the proper choice. Unfortunately, pxa2xx_spi does any DMA mapping not done by the protocol driver in pump_transfers, as each transfer is submitted to the hardware. That's not wrong ... but it's sub-optimal: those cache lines could have been doing better things all that time, and cleaning them out in the middle of a transfer will slow it down by some amount. There is not currently any message checking done in transfer(), the only error return from that is if the master driver is shutdown (queue stopped). The current scheme is to return the message with non-zero spi_message.status if it has failed *after* execution has been attempted. Again ... not wrong, but sub-optimal: when it happens (which won't be common, fortunately!) the SPI slave will be left in a rude state. And the protocol driver may not know how to recover. I don't look forward to making major changes, if we really want all DMA mapping and error checking to be in transfer(), though I see no reason why it could not be done. There's no rush on cleanups like that. They'd be reasonable tasks for someone with some time to spare, who wanted to get their feet wet in some driver updates and who could set up some kind of test rig. (Like one with a programmable slave that could be made to do all sorts of stuff that might otherwise be a bit uncommon. A microcontroller slave would be easy ... programming a CPLD or FPGA would be much trickier!) - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at
Re: [PATCH] atmel_spi: support zero length transfer
David, do you think writing 0 bytes is a valid use of this API? Just a zero byte transfer ... no, though it depends what you mean by valid. (I'm not sure I'd expect all controller drivers to reject such requests.) That has no effect on bits-on-the-wire, and would make trouble for various DMA engines. So, the behaviour is undefined, something between 'crash my dma engine', 'assert chip select and wait some time', or 'do_nothing'... No, it's fully defined. Crash my engine is not OK. The delay is controlled by transfer.delay_usecs ... possibly zero, which is best viewed as a degenerate case. Is this kind of device so common that we need to do all that work? or can we leave it as is (verified to work only with atmel_spi)? You can't avoid testing each combination of SPI protocol driver and SPI master controller driver you rely on ... especially when protocol tweaking options (cs_change, delay_usecs, bits_per_word, etc) are used at the per-transfer level. This driver stack isn't as readily tested as, say, USB. However, protocol drivers should be able to rely on controller drivers to reject requests that they don't even try to handle; that's basic. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-pm] power_state: get rid of write-only variable in SATA
> libata uses it under the assumption that "other parts" of the system are > aware of this variable. > > May I assume that the API has changed such that this is no longer necessary? Yes. The original motivation for dev->power.power_state was to let the writes to /sys/devices/.../power/state support a PCI-specific policy for device power state transition. And the motivation for any driver (or framework) to modify that was that the PM core didn't update it in all the relevant code paths (it wan't clear how to do that either), which mean those sysfs files could make trouble. That file is gone; everything supporting its usage can now go away. (And the PCI-specific policy -- "only transitions to/from D0 are allowed" -- can be managed by PCI drivers, and ignored by others on more flexible hardware.) The minor problem removing it is that there's some code which has used fields in power_state for some driver state. But if all that usage is write-only in the drivers (is that true now for SATA?), then it's safe to remove the /sys/devices/.../power/state support from framework code and elsewhere. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_spi: support zero length transfer
> David, do you think writing 0 bytes is a valid use of this API? Just a zero byte transfer ... no, though it depends what you mean by "valid". (I'm not sure I'd expect all controller drivers to reject such requests.) That has no effect on bits-on-the-wire, and would make trouble for various DMA engines. Passing zero bytes to get an inline delay at an exact spot in the overall protocol message ... I don't see why not. Better than adding delay fields for every spot it might be needed by various oddball devices, for sure!! > IMHO, we should add a fied to the spi_transfer struct. What would that do, exactly? This *specific* usage might arguably belong in spi_device, since it's a delay required after chipselect goes active and before any bytes get transferred. It's clearly not a per-transfer thing, and should also apply after a temporary mid-message chip deselect. And it would probably deserve a mode flag (sigh) unless someone can update every master controller driver. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atmel_spi: support zero length transfer
David, do you think writing 0 bytes is a valid use of this API? Just a zero byte transfer ... no, though it depends what you mean by valid. (I'm not sure I'd expect all controller drivers to reject such requests.) That has no effect on bits-on-the-wire, and would make trouble for various DMA engines. Passing zero bytes to get an inline delay at an exact spot in the overall protocol message ... I don't see why not. Better than adding delay fields for every spot it might be needed by various oddball devices, for sure!! IMHO, we should add a fied to the spi_transfer struct. What would that do, exactly? This *specific* usage might arguably belong in spi_device, since it's a delay required after chipselect goes active and before any bytes get transferred. It's clearly not a per-transfer thing, and should also apply after a temporary mid-message chip deselect. And it would probably deserve a mode flag (sigh) unless someone can update every master controller driver. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-pm] power_state: get rid of write-only variable in SATA
libata uses it under the assumption that other parts of the system are aware of this variable. May I assume that the API has changed such that this is no longer necessary? Yes. The original motivation for dev-power.power_state was to let the writes to /sys/devices/.../power/state support a PCI-specific policy for device power state transition. And the motivation for any driver (or framework) to modify that was that the PM core didn't update it in all the relevant code paths (it wan't clear how to do that either), which mean those sysfs files could make trouble. That file is gone; everything supporting its usage can now go away. (And the PCI-specific policy -- only transitions to/from D0 are allowed -- can be managed by PCI drivers, and ignored by others on more flexible hardware.) The minor problem removing it is that there's some code which has used fields in power_state for some driver state. But if all that usage is write-only in the drivers (is that true now for SATA?), then it's safe to remove the /sys/devices/.../power/state support from framework code and elsewhere. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Wednesday 20 February 2008, Haavard Skinnemoen wrote: > > > > Unfortunately it did not work. The clock state did not change by > > writing MR register. > > Ok, thanks for testing. > > In that case, I think the best fix is to let NPCS0 stay selected > permanently in MR and overwrite CSR0 with to the new slave's settings > before asserting CS. But that's a more complicated change, and I don't > know how it will affect the AT91RM9200 special cases. The rm9200 special cases which, ISTR, still don't work right ... > So I suggest we merge your patch for 2.6.25, and try to optimize it > for 2.6.26. > > David, do you want me to pass on the patch with my signoff or just ask > Andrew to add my Acked-by to the patch already in mm? It'd be good to let Andrew have your ack. It's already in MM, so if I don't need to sign off it's nice to have less to do there. :) - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Wednesday 20 February 2008, Atsushi Nemoto wrote: > > David, do you want me to pass on the patch with my signoff or just ask > > Andrew to add my Acked-by to the patch already in mm? > > The patch in mm also lacks my Signed-off line. I had thought Andrew > never take a patch without Signed-off line ;) > > Should I resend my patch with my Signed-off and Haavard's Acked-by? Good point. I think that's the way to go. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Wednesday 20 February 2008, Andre Tomt wrote: > David Brownell wrote: > > On Wednesday 20 February 2008, Andre Tomt wrote: > >> It has not crashed yet with the patch though. > > > > It seems that one of the tweks in this patch made the watchdog > > act better than before. So unless I hear from you (before the > > start of next week) that some other message appears, or that your > > oops re-appears, I'll submit some version of this patch for RC3. > > OOPS'ed again after some hours. The OOPS looks identical to me besides > all kind of other crap mixed in the trace due to a lot of unrelated > activity going on. > > Quite a lot of the same IAA messages (status 8029 and 8028, cmd 10021) > in /var/log/debug prior to the crash, over the entire uptime time span. > > This was with the first patch posted only. Not any of the other ones. Hmm ... I'd have expected some other IAA/IAAD message too. > > And if you're up for it, I may have another patch for you > > to try on top of this one ... I had an idea about IRQ trigger > > modes that might be causing this problem. > > It'll have to be tomorrow. Should I throw in the anti-oops patch too? Sure. I expect you'll see the stacktrace then, instead of oopsing. You might turn that one IAA message into an ehci_vdbg() call instead of an ehci_dbg() call, since the data it gives isn't useful. That would reduce the amount of log noise you seee. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Wednesday 20 February 2008, Alan Stern wrote: > On Wed, 20 Feb 2008, David Brownell wrote: > > > On Wednesday 20 February 2008, Alan Stern wrote: > > > > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 > > > > > > lines in the log brings up some ideas that have been percolating in my > > > mind for a while. They have to do with the possibility of a race > > > between the watchdog routine and assertion of IAA. > > > > The curious bit IMO being STS_INT (0001), which should also have > > triggered an IRQ. Suggesting to me that the race might be lower > > level than that ... at the level of a conflict between the various > > mechanisms to ack irqs. > > Maybe it did trigger an IRQ. Inside the watchdog routine interrupts > are disabled. > > > > In fact, if the timing comes out just wrong then it's possible (on SMP > > > systems) for an IAA interrupt to arrive when the watchdog > > > routine has already started running. Then end_unlink_async() might get > > > called right at the start of a new IAA cycle, or when the reclaim list > > > is empty. > > > > The driver's spinlock should prevent that particular problem from > > appearing. > > I don't think so: > > CPU 0 CPU 1 > - - > Watchdog timer expires > Timer routine acquires spinlock > IAA IRQ arrives > ehci_irq tries to acquire > spinlock... There's another condition here, and another action. The condition is that ehci->reclaim must first be set; the action is to clear STS_IAA (and, given the previous patch, maybe IAAD). And this "either" is more concisely written as "call end_unlink_async()" (point made just for clarity). > Timer routine either sets > ehci->reclaim to NULL > or else starts a new > IAA cycle > Timer routine releases spinlock > and returns > ehci_irq acquires spinlock > and sees IAA is set Can only happen if a new IAA cycle was started by CPU0, and the IAA condition triggered that quickly. > Call end_unlink_async()! > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Wednesday 20 February 2008, Alan Stern wrote: > > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 > > lines in the log brings up some ideas that have been percolating in my > mind for a while. They have to do with the possibility of a race > between the watchdog routine and assertion of IAA. The curious bit IMO being STS_INT (0001), which should also have triggered an IRQ. Suggesting to me that the race might be lower level than that ... at the level of a conflict between the various mechanisms to ack irqs. See the appended patch (Andre, this is the additional one I meant) for a tweak at that level. > In fact, if the timing comes out just wrong then it's possible (on SMP > systems) for an IAA interrupt to arrive when the watchdog > routine has already started running. Then end_unlink_async() might get > called right at the start of a new IAA cycle, or when the reclaim list > is empty. The driver's spinlock should prevent that particular problem from appearing. - Dave = CUT HERE Modify EHCI irq handling on the theory that at least some of the "lost" IRQs are caused by goofage between multiple lowlevel IRQ acking mechanisms: try rescanning before we exit the handler, in case the EHCI-internal ack (by clearing the irq status) doesn't always suffice for IRQs triggered nearly back-to-back. --- drivers/usb/host/ehci-hcd.c |8 1 file changed, 8 insertions(+) --- g26.orig/drivers/usb/host/ehci-hcd.c2008-02-20 13:26:00.0 -0800 +++ g26/drivers/usb/host/ehci-hcd.c 2008-02-20 13:54:37.0 -0800 @@ -638,6 +638,8 @@ static irqreturn_t ehci_irq (struct usb_ return IRQ_NONE; } +retrigger: + /* clear (just) interrupts */ ehci_writel(ehci, status, >regs->status); cmd = ehci_readl(ehci, >regs->command); @@ -725,6 +727,12 @@ dead: if (bh) ehci_work (ehci); + + status = ehci_readl(ehci, >regs->status); + status &= INTR_MASK; + if (status) + goto retrigger; + spin_unlock (>lock); if (pcd_status & STS_PCD) usb_hcd_poll_rh_status(hcd); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Wednesday 20 February 2008, Andre Tomt wrote: > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8028 cmd 10021 > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8028 cmd 10021 > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8028 cmd 10021 > ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 ... etc. OK, the relevant bits are: status 0001 == some transaction completed normally (ignored here) status 0020 == IAA set, which should have triggered an IRQ command 0040 == IAAD clear, meaning IAA should have triggered Meaning the hardware is misbehaving in a "traditional" way, one that the watchdog is supposed to catch: IAA set, but no IRQ. If you see any "IAA" messages *other* than those, please report them ASAP. They'll indicate "nontraditional" misbehavior. > It has not crashed yet with the patch though. It seems that one of the tweks in this patch made the watchdog act better than before. So unless I hear from you (before the start of next week) that some other message appears, or that your oops re-appears, I'll submit some version of this patch for RC3. And if you're up for it, I may have another patch for you to try on top of this one ... I had an idea about IRQ trigger modes that might be causing this problem. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Wednesday 20 February 2008, Andre Tomt wrote: ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8028 cmd 10021 ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8028 cmd 10021 ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8028 cmd 10021 ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 ... etc. OK, the relevant bits are: status 0001 == some transaction completed normally (ignored here) status 0020 == IAA set, which should have triggered an IRQ command 0040 == IAAD clear, meaning IAA should have triggered Meaning the hardware is misbehaving in a traditional way, one that the watchdog is supposed to catch: IAA set, but no IRQ. If you see any IAA messages *other* than those, please report them ASAP. They'll indicate nontraditional misbehavior. It has not crashed yet with the patch though. It seems that one of the tweks in this patch made the watchdog act better than before. So unless I hear from you (before the start of next week) that some other message appears, or that your oops re-appears, I'll submit some version of this patch for RC3. And if you're up for it, I may have another patch for you to try on top of this one ... I had an idea about IRQ trigger modes that might be causing this problem. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Wednesday 20 February 2008, Alan Stern wrote: ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 lines in the log brings up some ideas that have been percolating in my mind for a while. They have to do with the possibility of a race between the watchdog routine and assertion of IAA. The curious bit IMO being STS_INT (0001), which should also have triggered an IRQ. Suggesting to me that the race might be lower level than that ... at the level of a conflict between the various mechanisms to ack irqs. See the appended patch (Andre, this is the additional one I meant) for a tweak at that level. In fact, if the timing comes out just wrong then it's possible (on SMP systems) for an IAA interrupt to arrive when the watchdog routine has already started running. Then end_unlink_async() might get called right at the start of a new IAA cycle, or when the reclaim list is empty. The driver's spinlock should prevent that particular problem from appearing. - Dave = CUT HERE Modify EHCI irq handling on the theory that at least some of the lost IRQs are caused by goofage between multiple lowlevel IRQ acking mechanisms: try rescanning before we exit the handler, in case the EHCI-internal ack (by clearing the irq status) doesn't always suffice for IRQs triggered nearly back-to-back. --- drivers/usb/host/ehci-hcd.c |8 1 file changed, 8 insertions(+) --- g26.orig/drivers/usb/host/ehci-hcd.c2008-02-20 13:26:00.0 -0800 +++ g26/drivers/usb/host/ehci-hcd.c 2008-02-20 13:54:37.0 -0800 @@ -638,6 +638,8 @@ static irqreturn_t ehci_irq (struct usb_ return IRQ_NONE; } +retrigger: + /* clear (just) interrupts */ ehci_writel(ehci, status, ehci-regs-status); cmd = ehci_readl(ehci, ehci-regs-command); @@ -725,6 +727,12 @@ dead: if (bh) ehci_work (ehci); + + status = ehci_readl(ehci, ehci-regs-status); + status = INTR_MASK; + if (status) + goto retrigger; + spin_unlock (ehci-lock); if (pcd_status STS_PCD) usb_hcd_poll_rh_status(hcd); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Wednesday 20 February 2008, Alan Stern wrote: On Wed, 20 Feb 2008, David Brownell wrote: On Wednesday 20 February 2008, Alan Stern wrote: ehci_hcd :00:1d.7: IAA watchdog, lost IAA: status 8029 cmd 10021 lines in the log brings up some ideas that have been percolating in my mind for a while. They have to do with the possibility of a race between the watchdog routine and assertion of IAA. The curious bit IMO being STS_INT (0001), which should also have triggered an IRQ. Suggesting to me that the race might be lower level than that ... at the level of a conflict between the various mechanisms to ack irqs. Maybe it did trigger an IRQ. Inside the watchdog routine interrupts are disabled. In fact, if the timing comes out just wrong then it's possible (on SMP systems) for an IAA interrupt to arrive when the watchdog routine has already started running. Then end_unlink_async() might get called right at the start of a new IAA cycle, or when the reclaim list is empty. The driver's spinlock should prevent that particular problem from appearing. I don't think so: CPU 0 CPU 1 - - Watchdog timer expires Timer routine acquires spinlock IAA IRQ arrives ehci_irq tries to acquire spinlock... There's another condition here, and another action. The condition is that ehci-reclaim must first be set; the action is to clear STS_IAA (and, given the previous patch, maybe IAAD). And this either is more concisely written as call end_unlink_async() (point made just for clarity). Timer routine either sets ehci-reclaim to NULL or else starts a new IAA cycle Timer routine releases spinlock and returns ehci_irq acquires spinlock and sees IAA is set Can only happen if a new IAA cycle was started by CPU0, and the IAA condition triggered that quickly. Call end_unlink_async()! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Wednesday 20 February 2008, Andre Tomt wrote: David Brownell wrote: On Wednesday 20 February 2008, Andre Tomt wrote: It has not crashed yet with the patch though. It seems that one of the tweks in this patch made the watchdog act better than before. So unless I hear from you (before the start of next week) that some other message appears, or that your oops re-appears, I'll submit some version of this patch for RC3. OOPS'ed again after some hours. The OOPS looks identical to me besides all kind of other crap mixed in the trace due to a lot of unrelated activity going on. Quite a lot of the same IAA messages (status 8029 and 8028, cmd 10021) in /var/log/debug prior to the crash, over the entire uptime time span. This was with the first patch posted only. Not any of the other ones. Hmm ... I'd have expected some other IAA/IAAD message too. And if you're up for it, I may have another patch for you to try on top of this one ... I had an idea about IRQ trigger modes that might be causing this problem. It'll have to be tomorrow. Should I throw in the anti-oops patch too? Sure. I expect you'll see the stacktrace then, instead of oopsing. You might turn that one IAA message into an ehci_vdbg() call instead of an ehci_dbg() call, since the data it gives isn't useful. That would reduce the amount of log noise you seee. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Wednesday 20 February 2008, Haavard Skinnemoen wrote: Unfortunately it did not work. The clock state did not change by writing MR register. Ok, thanks for testing. In that case, I think the best fix is to let NPCS0 stay selected permanently in MR and overwrite CSR0 with to the new slave's settings before asserting CS. But that's a more complicated change, and I don't know how it will affect the AT91RM9200 special cases. The rm9200 special cases which, ISTR, still don't work right ... So I suggest we merge your patch for 2.6.25, and try to optimize it for 2.6.26. David, do you want me to pass on the patch with my signoff or just ask Andrew to add my Acked-by to the patch already in mm? It'd be good to let Andrew have your ack. It's already in MM, so if I don't need to sign off it's nice to have less to do there. :) - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: atmel_spi clock polarity
On Wednesday 20 February 2008, Atsushi Nemoto wrote: David, do you want me to pass on the patch with my signoff or just ask Andrew to add my Acked-by to the patch already in mm? The patch in mm also lacks my Signed-off line. I had thought Andrew never take a patch without Signed-off line ;) Should I resend my patch with my Signed-off and Haavard's Acked-by? Good point. I think that's the way to go. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Tuesday 19 February 2008, Andre Tomt wrote: > > > Can you try this diagnostic patch, to see if it reports any messages > > about IAA and/or IAAD oddities? There's surely a quick workaround > > for this, but I'd rather understand the root cause before patching. > > Doesn't seem to have triggered anything. dmesg attached in case I missed > anything. You don't seem to have enabled CONFIG_USB_DEBUG, as the patch instructions say is needed to get such diagnostics ... I can tell because the startup messages from USB are pretty minimal. (See appended, vs what you sent...) Please try again with USB debugging enabled. - Dave ehci_hcd :00:02.2: new USB bus registered, assigned bus number 3 ehci_hcd :00:02.2: reset hcs_params 0x102486 dbg=1 cc=2 pcc=4 !ppc ports=6 ehci_hcd :00:02.2: reset portroute 0 0 1 1 1 0 ehci_hcd :00:02.2: reset hcc_params a086 caching frame 256/512/1024 park ehci_hcd :00:02.2: park 0 ehci_hcd :00:02.2: reset command 080b02 park=3 ithresh=8 period=1024 Reset HALT PCI: cache line size of 64 is not supported by device :00:02.2 ehci_hcd :00:02.2: supports USB remote wakeup ehci_hcd :00:02.2: irq 22, io mem 0xe8004000 ehci_hcd :00:02.2: reset command 080b02 park=3 ithresh=8 period=1024 Reset HALT ehci_hcd :00:02.2: init command 010009 (park)=0 ithresh=1 period=256 RUN ehci_hcd :00:02.2: USB 2.0 started, EHCI 1.00, driver 10 Dec 2004 ... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Tuesday 19 February 2008, David Miller wrote:
> From: Andre Tomt <[EMAIL PROTECTED]>
> Date: Tue, 19 Feb 2008 16:19:08 +0100
>
> > Got this on a serial console today, using 2.6.25-rc2-git1. Machine was
> > not doing anything interesting at the time, but has its / and kernel on
> > a usb-storage device (usb pen drive).
> >
> > Intel ICH8R chipset (and USB controller), running x86_64 kernel. I'll
> > post .config and some additional info when I get home later if it isn't
> > obvious what broke.
>
> FWIW, I've seen a near identical crash on my Niagara system.
Please try that diagnostic patch I sent ... with CONFIG_USB_DEBUG.
Near as I can tell this is caused by some hardware oddity that needs
to be worked around. We seem to be at stage where we've fixed some
problems, nudging code paths around so another one shows up, and have
incidentally had a new silicion-specific hardware erratum reported
in this area. So more info is needed...
A quick anti-oops patch is appended, it should work OK on top of that
diagnostic patch, but won't necessarily resolve the underlying problem.
- Dave
--- g26.orig/drivers/usb/host/ehci-q.c 2008-02-19 16:15:04.0 -0800
+++ g26/drivers/usb/host/ehci-q.c 2008-02-19 16:15:59.0 -0800
@@ -993,6 +993,11 @@ static void end_unlink_async (struct ehc
iaa_watchdog_done(ehci);
+ if (!qh) {
+ WARN_ON(1);
+ return;
+ }
+
// qh->hw_next = cpu_to_hc32(qh->qh_dma);
qh->qh_state = QH_STATE_IDLE;
qh->qh_next.qh = NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] suspend/resume self-test
On Tuesday 19 February 2008, Ingo Molnar wrote: > * Pavel Machek <[EMAIL PROTECTED]> wrote: > > > > Thing is, this will catch not just regressions ... but cases where > > > STR never worked in the first place. Video problems, etc. Also > > > various system startup races, as in the PCMCIA and MMC/SD/SDIO cases > > > I noted. > > > > David is right here. At minimum, s2ram needs acpi_sleep=... options to > > tell it how to set up the video. That is not issue for you, but it > > means we should not be doing it by default. > > nowhere did i suggest that it should be default-enabled ... The patch you asked to be merged *DID* have it be default-enabled! So you did more than just "suggest"... if that option is enabled in Kconfig, this test is always forced on and it will cause failures on systems where STR has never worked. The patch comments even pointed out that flaw. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Tuesday 19 February 2008, Andre Tomt wrote:
> Got this on a serial console today, using 2.6.25-rc2-git1. Machine was
> not doing anything interesting at the time, but has its / and kernel on
> a usb-storage device (usb pen drive).
Can you try this diagnostic patch, to see if it reports any messages
about IAA and/or IAAD oddities? There's surely a quick workaround
for this, but I'd rather understand the root cause before patching.
- Dave
Work around for an evident bug in one EHCI controller: IAA didn't get
set when IAAD was cleared. Evidently writing the status register can
prevent setting IAA; someone's VHDL (or whatever) code was wrong.
This workaround catches that specific bug (in the IRQ handler and in
the IAA watchdog) and treats it as if IAA was properly set.
The patch also adds *LOTS* of related paranoia, insisting IAAD is clear
(or set, as appropriate) at various points, and adding code to improve
the handling of some such cases. It also raises the volume and precision
of debug messaging related to IAA problems.
This patch is EXPERIMENTAL and DIAGNOSTIC ... not intended for merge.
It's also in *addition* to the IAA watchdog timer rework that's already
been merged into 2.6.25-rc1, which will help some systems.
If you use this, run with CONFIG_USB_DEBUG enabled. Most messages
with IAA or IAAD should then be "interesting" in the sense that they
will indicate something odd happening ... maybe something that's
fully worked around, maybe not.
---
drivers/usb/host/ehci-hcd.c | 38 ++
drivers/usb/host/ehci-q.c | 27 ++-
2 files changed, 56 insertions(+), 9 deletions(-)
--- g26.orig/drivers/usb/host/ehci-hcd.c2008-02-11 19:18:39.0
-0800
+++ g26/drivers/usb/host/ehci-hcd.c 2008-02-13 15:30:56.0 -0800
@@ -255,21 +255,33 @@ static void ehci_iaa_watchdog(unsigned l
u32 status, cmd;
spin_lock_irqsave (>lock, flags);
- WARN_ON(!ehci->reclaim);
status = ehci_readl(ehci, >regs->status);
cmd = ehci_readl(ehci, >regs->command);
- ehci_dbg(ehci, "IAA watchdog: status %x cmd %x\n", status, cmd);
/* lost IAA irqs wedge things badly; seen first with a vt8235 */
if (ehci->reclaim) {
- if (status & STS_IAA) {
- ehci_vdbg (ehci, "lost IAA\n");
+ /* STS_IAA means we got the status, but no IRQ. (Or at
+* best, the IRQ took until just now to arrive.) Missing
+* CMD_IAAD means we got the effect, but no status or IRQ.
+*/
+ if ((status & STS_IAA) || !(cmd & CMD_IAAD)) {
COUNT (ehci->stats.lost_iaa);
- ehci_writel(ehci, STS_IAA, >regs->status);
+ if (status & STS_IAA)
+ ehci_writel(ehci, STS_IAA,
+ >regs->status);
}
- ehci_writel(ehci, cmd & ~CMD_IAAD, >regs->command);
+ ehci_dbg(ehci, "IAA watchdog%s: status %x cmd %x\n",
+ ((status & STS_IAA) || !(cmd & CMD_IAAD))
+ ? ", lost IAA" : "",
+ status, cmd);
end_unlink_async(ehci);
+
+ } else if (status & STS_IAA) {
+ ehci_writel(ehci, STS_IAA, >regs->status);
+ ehci_dbg(ehci, "IAA watchdog%s: status %x cmd %x\n",
+ ", IAA with empty reclaim",
+ status, cmd);
}
spin_unlock_irqrestore(>lock, flags);
@@ -602,7 +614,7 @@ static int ehci_run (struct usb_hcd *hcd
static irqreturn_t ehci_irq (struct usb_hcd *hcd)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
- u32 status, pcd_status = 0;
+ u32 status, pcd_status = 0, cmd;
int bh;
spin_lock (>lock);
@@ -623,7 +635,7 @@ static irqreturn_t ehci_irq (struct usb_
/* clear (just) interrupts */
ehci_writel(ehci, status, >regs->status);
- ehci_readl(ehci, >regs->command); /* unblock posted write */
+ cmd = ehci_readl(ehci, >regs->command);
bh = 0;
#ifdef VERBOSE_DEBUG
@@ -642,6 +654,16 @@ static irqreturn_t ehci_irq (struct usb_
bh = 1;
}
+ /* Cope with silicon bug where IAA sometimes isn't set, but
+* IAAD is cleared ... clearing should be a side effect of
+* setting IAA. So assume that when we expect IAA but neither
+* IAA nor IAAD are set, we should act as if IAA was reported.
+*/
+ if (ehci->reclaim && !(status & STS_IAA) && !(cmd & CMD_IAAD)) {
+ ehci_dbg(ehci, "IAAD cleared without IAA\n");
+ status |= STS_IAA;
+ }
+
/* complete the unlinking of some qh [4.15.2.3] */
Re: USB OOPS 2.6.25-rc2-git1
On Tuesday 19 February 2008, Andre Tomt wrote:
Got this on a serial console today, using 2.6.25-rc2-git1. Machine was
not doing anything interesting at the time, but has its / and kernel on
a usb-storage device (usb pen drive).
Can you try this diagnostic patch, to see if it reports any messages
about IAA and/or IAAD oddities? There's surely a quick workaround
for this, but I'd rather understand the root cause before patching.
- Dave
Work around for an evident bug in one EHCI controller: IAA didn't get
set when IAAD was cleared. Evidently writing the status register can
prevent setting IAA; someone's VHDL (or whatever) code was wrong.
This workaround catches that specific bug (in the IRQ handler and in
the IAA watchdog) and treats it as if IAA was properly set.
The patch also adds *LOTS* of related paranoia, insisting IAAD is clear
(or set, as appropriate) at various points, and adding code to improve
the handling of some such cases. It also raises the volume and precision
of debug messaging related to IAA problems.
This patch is EXPERIMENTAL and DIAGNOSTIC ... not intended for merge.
It's also in *addition* to the IAA watchdog timer rework that's already
been merged into 2.6.25-rc1, which will help some systems.
If you use this, run with CONFIG_USB_DEBUG enabled. Most messages
with IAA or IAAD should then be interesting in the sense that they
will indicate something odd happening ... maybe something that's
fully worked around, maybe not.
---
drivers/usb/host/ehci-hcd.c | 38 ++
drivers/usb/host/ehci-q.c | 27 ++-
2 files changed, 56 insertions(+), 9 deletions(-)
--- g26.orig/drivers/usb/host/ehci-hcd.c2008-02-11 19:18:39.0
-0800
+++ g26/drivers/usb/host/ehci-hcd.c 2008-02-13 15:30:56.0 -0800
@@ -255,21 +255,33 @@ static void ehci_iaa_watchdog(unsigned l
u32 status, cmd;
spin_lock_irqsave (ehci-lock, flags);
- WARN_ON(!ehci-reclaim);
status = ehci_readl(ehci, ehci-regs-status);
cmd = ehci_readl(ehci, ehci-regs-command);
- ehci_dbg(ehci, IAA watchdog: status %x cmd %x\n, status, cmd);
/* lost IAA irqs wedge things badly; seen first with a vt8235 */
if (ehci-reclaim) {
- if (status STS_IAA) {
- ehci_vdbg (ehci, lost IAA\n);
+ /* STS_IAA means we got the status, but no IRQ. (Or at
+* best, the IRQ took until just now to arrive.) Missing
+* CMD_IAAD means we got the effect, but no status or IRQ.
+*/
+ if ((status STS_IAA) || !(cmd CMD_IAAD)) {
COUNT (ehci-stats.lost_iaa);
- ehci_writel(ehci, STS_IAA, ehci-regs-status);
+ if (status STS_IAA)
+ ehci_writel(ehci, STS_IAA,
+ ehci-regs-status);
}
- ehci_writel(ehci, cmd ~CMD_IAAD, ehci-regs-command);
+ ehci_dbg(ehci, IAA watchdog%s: status %x cmd %x\n,
+ ((status STS_IAA) || !(cmd CMD_IAAD))
+ ? , lost IAA : ,
+ status, cmd);
end_unlink_async(ehci);
+
+ } else if (status STS_IAA) {
+ ehci_writel(ehci, STS_IAA, ehci-regs-status);
+ ehci_dbg(ehci, IAA watchdog%s: status %x cmd %x\n,
+ , IAA with empty reclaim,
+ status, cmd);
}
spin_unlock_irqrestore(ehci-lock, flags);
@@ -602,7 +614,7 @@ static int ehci_run (struct usb_hcd *hcd
static irqreturn_t ehci_irq (struct usb_hcd *hcd)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
- u32 status, pcd_status = 0;
+ u32 status, pcd_status = 0, cmd;
int bh;
spin_lock (ehci-lock);
@@ -623,7 +635,7 @@ static irqreturn_t ehci_irq (struct usb_
/* clear (just) interrupts */
ehci_writel(ehci, status, ehci-regs-status);
- ehci_readl(ehci, ehci-regs-command); /* unblock posted write */
+ cmd = ehci_readl(ehci, ehci-regs-command);
bh = 0;
#ifdef VERBOSE_DEBUG
@@ -642,6 +654,16 @@ static irqreturn_t ehci_irq (struct usb_
bh = 1;
}
+ /* Cope with silicon bug where IAA sometimes isn't set, but
+* IAAD is cleared ... clearing should be a side effect of
+* setting IAA. So assume that when we expect IAA but neither
+* IAA nor IAAD are set, we should act as if IAA was reported.
+*/
+ if (ehci-reclaim !(status STS_IAA) !(cmd CMD_IAAD)) {
+ ehci_dbg(ehci, IAAD cleared without IAA\n);
+ status |= STS_IAA;
+ }
+
/* complete the unlinking of some qh [4.15.2.3] */
Re: [patch] suspend/resume self-test
On Tuesday 19 February 2008, Ingo Molnar wrote: * Pavel Machek [EMAIL PROTECTED] wrote: Thing is, this will catch not just regressions ... but cases where STR never worked in the first place. Video problems, etc. Also various system startup races, as in the PCMCIA and MMC/SD/SDIO cases I noted. David is right here. At minimum, s2ram needs acpi_sleep=... options to tell it how to set up the video. That is not issue for you, but it means we should not be doing it by default. nowhere did i suggest that it should be default-enabled ... The patch you asked to be merged *DID* have it be default-enabled! So you did more than just suggest... if that option is enabled in Kconfig, this test is always forced on and it will cause failures on systems where STR has never worked. The patch comments even pointed out that flaw. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Tuesday 19 February 2008, Andre Tomt wrote: Can you try this diagnostic patch, to see if it reports any messages about IAA and/or IAAD oddities? There's surely a quick workaround for this, but I'd rather understand the root cause before patching. Doesn't seem to have triggered anything. dmesg attached in case I missed anything. You don't seem to have enabled CONFIG_USB_DEBUG, as the patch instructions say is needed to get such diagnostics ... I can tell because the startup messages from USB are pretty minimal. (See appended, vs what you sent...) Please try again with USB debugging enabled. - Dave ehci_hcd :00:02.2: new USB bus registered, assigned bus number 3 ehci_hcd :00:02.2: reset hcs_params 0x102486 dbg=1 cc=2 pcc=4 !ppc ports=6 ehci_hcd :00:02.2: reset portroute 0 0 1 1 1 0 ehci_hcd :00:02.2: reset hcc_params a086 caching frame 256/512/1024 park ehci_hcd :00:02.2: park 0 ehci_hcd :00:02.2: reset command 080b02 park=3 ithresh=8 period=1024 Reset HALT PCI: cache line size of 64 is not supported by device :00:02.2 ehci_hcd :00:02.2: supports USB remote wakeup ehci_hcd :00:02.2: irq 22, io mem 0xe8004000 ehci_hcd :00:02.2: reset command 080b02 park=3 ithresh=8 period=1024 Reset HALT ehci_hcd :00:02.2: init command 010009 (park)=0 ithresh=1 period=256 RUN ehci_hcd :00:02.2: USB 2.0 started, EHCI 1.00, driver 10 Dec 2004 ... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB OOPS 2.6.25-rc2-git1
On Tuesday 19 February 2008, David Miller wrote:
From: Andre Tomt [EMAIL PROTECTED]
Date: Tue, 19 Feb 2008 16:19:08 +0100
Got this on a serial console today, using 2.6.25-rc2-git1. Machine was
not doing anything interesting at the time, but has its / and kernel on
a usb-storage device (usb pen drive).
Intel ICH8R chipset (and USB controller), running x86_64 kernel. I'll
post .config and some additional info when I get home later if it isn't
obvious what broke.
FWIW, I've seen a near identical crash on my Niagara system.
Please try that diagnostic patch I sent ... with CONFIG_USB_DEBUG.
Near as I can tell this is caused by some hardware oddity that needs
to be worked around. We seem to be at stage where we've fixed some
problems, nudging code paths around so another one shows up, and have
incidentally had a new silicion-specific hardware erratum reported
in this area. So more info is needed...
A quick anti-oops patch is appended, it should work OK on top of that
diagnostic patch, but won't necessarily resolve the underlying problem.
- Dave
--- g26.orig/drivers/usb/host/ehci-q.c 2008-02-19 16:15:04.0 -0800
+++ g26/drivers/usb/host/ehci-q.c 2008-02-19 16:15:59.0 -0800
@@ -993,6 +993,11 @@ static void end_unlink_async (struct ehc
iaa_watchdog_done(ehci);
+ if (!qh) {
+ WARN_ON(1);
+ return;
+ }
+
// qh-hw_next = cpu_to_hc32(qh-qh_dma);
qh-qh_state = QH_STATE_IDLE;
qh-qh_next.qh = NULL;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] queue usb USB_CDC_GET_ENCAPSULATED_RESPONSE message
On Monday 18 February 2008, Jan Altenberg wrote: > Hi all, > > commit 0cf4f2de0a0f4100795f38ef894d4910678c74f8 introduced a bug, which > prevents sending an USB_CDC_GET_ENCAPSULATED_RESPONSE message. This > breaks the RNDIS initialization (especially / only Windoze machines > dislike this behavior...). > > Signed-off-by: Benedikt Spranger <[EMAIL PROTECTED]> > Signed-off-by: Jan Altenberg <[EMAIL PROTECTED]> Acked-by: David Brownell <[EMAIL PROTECTED]> > --- > drivers/usb/gadget/ether.c |1 + > 1 file changed, 1 insertion(+) > > Index: linux-2.6.24/drivers/usb/gadget/ether.c > === > --- linux-2.6.24.orig/drivers/usb/gadget/ether.c > +++ linux-2.6.24/drivers/usb/gadget/ether.c > @@ -1568,6 +1568,7 @@ done_set_intf: > memcpy(req->buf, buf, n); > req->complete = rndis_response_complete; > rndis_free_response(dev->rndis_config, buf); > + value = n; > } > /* else stalls ... spec says to avoid that */ > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] documentation: move spidev_fdx example to its own source file
On Monday 18 February 2008, Randy Dunlap wrote:
> From: Randy Dunlap <[EMAIL PROTECTED]>
>
> cc: [EMAIL PROTECTED]
> cc: [EMAIL PROTECTED]
>
> Move sample source code to its own source file so that it can be used
> easier and build-tested/check/maintained by anyone.
>
> (Makefile changes are in a separate patch for all of Documentation/.)
>
> Signed-off-by: Randy Dunlap <[EMAIL PROTECTED]>
Acked-by: David Brownell <[EMAIL PROTECTED]>
> ---
> Documentation/spi/spidev | 168 ---
> Documentation/spi/spidev_fdx.c | 158 +
> 2 files changed, 160 insertions(+), 166 deletions(-)
>
> --- linux-2625-rc2-docsrc.orig/Documentation/spi/spidev
> +++ linux-2625-rc2-docsrc/Documentation/spi/spidev
> @@ -126,8 +126,8 @@ NOTES:
> FULL DUPLEX CHARACTER DEVICE API
>
>
> -See the sample program below for one example showing the use of the full
> -duplex programming interface. (Although it doesn't perform a full duplex
> +See the spidev_fdx.c sample program for one example showing the use of the
> +full duplex programming interface. (Although it doesn't perform a full
> duplex
> transfer.) The model is the same as that used in the kernel spi_sync()
> request; the individual transfers offer the same capabilities as are
> available to kernel drivers (except that it's not asynchronous).
> @@ -141,167 +141,3 @@ and bitrate for each transfer segment.)
>
> To make a full duplex request, provide both rx_buf and tx_buf for the
> same transfer. It's even OK if those are the same buffer.
> -
> -
> -SAMPLE PROGRAM
> -==
> -
> - CUT HERE
> -#include
> -#include
> -#include
> -#include
> -#include
> -
> -#include
> -#include
> -#include
> -
> -#include
> -#include
> -
> -
> -static int verbose;
> -
> -static void do_read(int fd, int len)
> -{
> - unsigned char buf[32], *bp;
> - int status;
> -
> - /* read at least 2 bytes, no more than 32 */
> - if (len < 2)
> - len = 2;
> - else if (len > sizeof(buf))
> - len = sizeof(buf);
> - memset(buf, 0, sizeof buf);
> -
> - status = read(fd, buf, len);
> - if (status < 0) {
> - perror("read");
> - return;
> - }
> - if (status != len) {
> - fprintf(stderr, "short read\n");
> - return;
> - }
> -
> - printf("read(%2d, %2d): %02x %02x,", len, status,
> - buf[0], buf[1]);
> - status -= 2;
> - bp = buf + 2;
> - while (status-- > 0)
> - printf(" %02x", *bp++);
> - printf("\n");
> -}
> -
> -static void do_msg(int fd, int len)
> -{
> - struct spi_ioc_transfer xfer[2];
> - unsigned char buf[32], *bp;
> - int status;
> -
> - memset(xfer, 0, sizeof xfer);
> - memset(buf, 0, sizeof buf);
> -
> - if (len > sizeof buf)
> - len = sizeof buf;
> -
> - buf[0] = 0xaa;
> - xfer[0].tx_buf = (__u64) buf;
> - xfer[0].len = 1;
> -
> - xfer[1].rx_buf = (__u64) buf;
> - xfer[1].len = len;
> -
> - status = ioctl(fd, SPI_IOC_MESSAGE(2), xfer);
> - if (status < 0) {
> - perror("SPI_IOC_MESSAGE");
> - return;
> - }
> -
> - printf("response(%2d, %2d): ", len, status);
> - for (bp = buf; len; len--)
> - printf(" %02x", *bp++);
> - printf("\n");
> -}
> -
> -static void dumpstat(const char *name, int fd)
> -{
> - __u8mode, lsb, bits;
> - __u32 speed;
> -
> - if (ioctl(fd, SPI_IOC_RD_MODE, ) < 0) {
> - perror("SPI rd_mode");
> - return;
> - }
> - if (ioctl(fd, SPI_IOC_RD_LSB_FIRST, ) < 0) {
> - perror("SPI rd_lsb_fist");
> - return;
> - }
> - if (ioctl(fd, SPI_IOC_RD_BITS_PER_WORD, ) < 0) {
> - perror("SPI bits_per_word");
> - return;
> - }
> - if (ioctl(fd, SPI_IOC_RD_MAX_SPEED_HZ, ) < 0) {
> - perror("SPI max_speed_hz");
> - return;
> - }
> -
> - printf("%s: spi mode %d, %d bits %sper word, %d Hz max\n",
> - name, mode, bits, lsb ? "(lsb first) " : "", speed);
> -}
> -
> -int main(int argc, char **argv)
> -{
> - int
Re: [patch] suspend/resume self-test
On Monday 18 February 2008, Ingo Molnar wrote: > > * David Brownell <[EMAIL PROTECTED]> wrote: > > > > > - Includes a command line parameter, which needs work yet ... it > > > > currently turns this test off, but it should also let the target > > > > state be specified (and maybe even default to "no test"). > > > > I think "no test" should be the default; STR working sanely on x86 is > > unfortunately too much a surprise. Someone more active in PM testing > > should update that. > > All i'm asking for is to make the self-test easily accessible. Not for > it to blow up in the face of users who do not ask for it. I'm all for that, but also I don't want to see it blow up regularly in the face of people who just enable all the selftest options. The other tests have a much better expectation of working "by default". > And, at least to me, there seems to be a rather apparent correlation > between "suspend/resume regressions caught as early as possible" and the > future, desired state of: "STR working sanely on x86" ;-) Thing is, this will catch not just regressions ... but cases where STR never worked in the first place. Video problems, etc. Also various system startup races, as in the PCMCIA and MMC/SD/SDIO cases I noted. > You really seem to treat S2R suckiness as a fact of life, but it isnt. Until it starts working on a given platform, it *IS* a fact of life. Once it works, then it's fair to enter "no regressions" mode. Despite all the recent improvements, I think it's unwise to pretend that STR works properly on most systems. > Yes, it's a hard field for a number of reasons, but we could be doing _a > lot_ better. One of them would be this "notice s2r breakage when i > create or add the patch that breaks it" angle. Right, and the best way to ensure that it's only *regressions* that break things is to expect someone to have configured the kernel command line appropriately (in grub or whatever). Another way to achieve that is to include the test code based on one config option, and change the test *mode* based on another one. That way a distro could include that in standard kernels with "no test" mode as the default, but it would be easy to enable only for oneshot tests or field troubleshooting ... while developers could turn on the more dangerous "always test STR" (or standby, or hibernate) mode, if they were helping to find and fix problems surfaced by such tests. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] atmel_spi clock polarity
On Monday 18 February 2008, Atsushi Nemoto wrote: >IIRC the clock state follows > CSRn.CPOL just before the real transfer. No ... clock state should be valid *before* chipselect goes active. So I'm thinking the patch from Haavard is likely the right change. > Like this (previous transfer > was MODE 0, new transfer is MODE 3): > >T0T1 T2 > > CS ~~~| So at T0, some chip is selected (and never deselected) ... > > CLK __|~|___|~~~|___|~~~|___|~~~|___ ... and at T1 CPOL is changed?? That's wrong. There should never be a partial clock period while a chipselect is active. While it's inactive, sure -- no chip should care. > > SO ~~|___|~~~|___|~~~|___|~~~|_ >MSB > > T0-T1 was relatively longer then T1-T2. I suppose T1 is not the > point of updating MR register, but the point of starting DMA transfer. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: WARN_ON(): proc_dir_entry 'rtc' already registered
On Monday 18 February 2008, Pavel Machek wrote: > > How to fix ... how about: instead of just warning folk > > off such legacy RTC drivers [1] we just wrap them with > > an "if RTC_LIB != n" so this mistake won't be possible. > > Yes, disabling bad configs in Kconfig seems like way to go. Problem is ... that also disables valid configs that get assembled by picking the right module(s) for the target system. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] suspend/resume self-test
> > - Includes a command line parameter, which needs work yet ... it > > currently turns this test off, but it should also let the target > > state be specified (and maybe even default to "no test"). I think "no test" should be the default; STR working sanely on x86 is unfortunately too much a surprise. Someone more active in PM testing should update that. > > Also includes some Kconfig tweaks to help reduce configuration bugs on > > x86, by avoiding the legacy RTC driver when the generic RTC framework > > is enabled ... those should become a separate patch. That's already been split out in a separate patch, now in MM. Though it may deserve another tweak, forcing off *all* legacy RTC drivers if the new framework is enabled. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] suspend/resume self-test
- Includes a command line parameter, which needs work yet ... it currently turns this test off, but it should also let the target state be specified (and maybe even default to no test). I think no test should be the default; STR working sanely on x86 is unfortunately too much a surprise. Someone more active in PM testing should update that. Also includes some Kconfig tweaks to help reduce configuration bugs on x86, by avoiding the legacy RTC driver when the generic RTC framework is enabled ... those should become a separate patch. That's already been split out in a separate patch, now in MM. Though it may deserve another tweak, forcing off *all* legacy RTC drivers if the new framework is enabled. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: WARN_ON(): proc_dir_entry 'rtc' already registered
On Monday 18 February 2008, Pavel Machek wrote: How to fix ... how about: instead of just warning folk off such legacy RTC drivers [1] we just wrap them with an if RTC_LIB != n so this mistake won't be possible. Yes, disabling bad configs in Kconfig seems like way to go. Problem is ... that also disables valid configs that get assembled by picking the right module(s) for the target system. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [spi-devel-general] atmel_spi clock polarity
On Monday 18 February 2008, Atsushi Nemoto wrote: IIRC the clock state follows CSRn.CPOL just before the real transfer. No ... clock state should be valid *before* chipselect goes active. So I'm thinking the patch from Haavard is likely the right change. Like this (previous transfer was MODE 0, new transfer is MODE 3): T0T1 T2 CS ~~~| So at T0, some chip is selected (and never deselected) ... CLK __|~|___|~~~|___|~~~|___|~~~|___ ... and at T1 CPOL is changed?? That's wrong. There should never be a partial clock period while a chipselect is active. While it's inactive, sure -- no chip should care. SO ~~|___|~~~|___|~~~|___|~~~|_ MSB T0-T1 was relatively longer then T1-T2. I suppose T1 is not the point of updating MR register, but the point of starting DMA transfer. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] suspend/resume self-test
On Monday 18 February 2008, Ingo Molnar wrote: * David Brownell [EMAIL PROTECTED] wrote: - Includes a command line parameter, which needs work yet ... it currently turns this test off, but it should also let the target state be specified (and maybe even default to no test). I think no test should be the default; STR working sanely on x86 is unfortunately too much a surprise. Someone more active in PM testing should update that. All i'm asking for is to make the self-test easily accessible. Not for it to blow up in the face of users who do not ask for it. I'm all for that, but also I don't want to see it blow up regularly in the face of people who just enable all the selftest options. The other tests have a much better expectation of working by default. And, at least to me, there seems to be a rather apparent correlation between suspend/resume regressions caught as early as possible and the future, desired state of: STR working sanely on x86 ;-) Thing is, this will catch not just regressions ... but cases where STR never worked in the first place. Video problems, etc. Also various system startup races, as in the PCMCIA and MMC/SD/SDIO cases I noted. You really seem to treat S2R suckiness as a fact of life, but it isnt. Until it starts working on a given platform, it *IS* a fact of life. Once it works, then it's fair to enter no regressions mode. Despite all the recent improvements, I think it's unwise to pretend that STR works properly on most systems. Yes, it's a hard field for a number of reasons, but we could be doing _a lot_ better. One of them would be this notice s2r breakage when i create or add the patch that breaks it angle. Right, and the best way to ensure that it's only *regressions* that break things is to expect someone to have configured the kernel command line appropriately (in grub or whatever). Another way to achieve that is to include the test code based on one config option, and change the test *mode* based on another one. That way a distro could include that in standard kernels with no test mode as the default, but it would be easy to enable only for oneshot tests or field troubleshooting ... while developers could turn on the more dangerous always test STR (or standby, or hibernate) mode, if they were helping to find and fix problems surfaced by such tests. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] documentation: move spidev_fdx example to its own source file
On Monday 18 February 2008, Randy Dunlap wrote:
From: Randy Dunlap [EMAIL PROTECTED]
cc: [EMAIL PROTECTED]
cc: [EMAIL PROTECTED]
Move sample source code to its own source file so that it can be used
easier and build-tested/check/maintained by anyone.
(Makefile changes are in a separate patch for all of Documentation/.)
Signed-off-by: Randy Dunlap [EMAIL PROTECTED]
Acked-by: David Brownell [EMAIL PROTECTED]
---
Documentation/spi/spidev | 168 ---
Documentation/spi/spidev_fdx.c | 158 +
2 files changed, 160 insertions(+), 166 deletions(-)
--- linux-2625-rc2-docsrc.orig/Documentation/spi/spidev
+++ linux-2625-rc2-docsrc/Documentation/spi/spidev
@@ -126,8 +126,8 @@ NOTES:
FULL DUPLEX CHARACTER DEVICE API
-See the sample program below for one example showing the use of the full
-duplex programming interface. (Although it doesn't perform a full duplex
+See the spidev_fdx.c sample program for one example showing the use of the
+full duplex programming interface. (Although it doesn't perform a full
duplex
transfer.) The model is the same as that used in the kernel spi_sync()
request; the individual transfers offer the same capabilities as are
available to kernel drivers (except that it's not asynchronous).
@@ -141,167 +141,3 @@ and bitrate for each transfer segment.)
To make a full duplex request, provide both rx_buf and tx_buf for the
same transfer. It's even OK if those are the same buffer.
-
-
-SAMPLE PROGRAM
-==
-
- CUT HERE
-#include stdio.h
-#include unistd.h
-#include stdlib.h
-#include fcntl.h
-#include string.h
-
-#include sys/ioctl.h
-#include sys/types.h
-#include sys/stat.h
-
-#include linux/types.h
-#include linux/spi/spidev.h
-
-
-static int verbose;
-
-static void do_read(int fd, int len)
-{
- unsigned char buf[32], *bp;
- int status;
-
- /* read at least 2 bytes, no more than 32 */
- if (len 2)
- len = 2;
- else if (len sizeof(buf))
- len = sizeof(buf);
- memset(buf, 0, sizeof buf);
-
- status = read(fd, buf, len);
- if (status 0) {
- perror(read);
- return;
- }
- if (status != len) {
- fprintf(stderr, short read\n);
- return;
- }
-
- printf(read(%2d, %2d): %02x %02x,, len, status,
- buf[0], buf[1]);
- status -= 2;
- bp = buf + 2;
- while (status-- 0)
- printf( %02x, *bp++);
- printf(\n);
-}
-
-static void do_msg(int fd, int len)
-{
- struct spi_ioc_transfer xfer[2];
- unsigned char buf[32], *bp;
- int status;
-
- memset(xfer, 0, sizeof xfer);
- memset(buf, 0, sizeof buf);
-
- if (len sizeof buf)
- len = sizeof buf;
-
- buf[0] = 0xaa;
- xfer[0].tx_buf = (__u64) buf;
- xfer[0].len = 1;
-
- xfer[1].rx_buf = (__u64) buf;
- xfer[1].len = len;
-
- status = ioctl(fd, SPI_IOC_MESSAGE(2), xfer);
- if (status 0) {
- perror(SPI_IOC_MESSAGE);
- return;
- }
-
- printf(response(%2d, %2d): , len, status);
- for (bp = buf; len; len--)
- printf( %02x, *bp++);
- printf(\n);
-}
-
-static void dumpstat(const char *name, int fd)
-{
- __u8mode, lsb, bits;
- __u32 speed;
-
- if (ioctl(fd, SPI_IOC_RD_MODE, mode) 0) {
- perror(SPI rd_mode);
- return;
- }
- if (ioctl(fd, SPI_IOC_RD_LSB_FIRST, lsb) 0) {
- perror(SPI rd_lsb_fist);
- return;
- }
- if (ioctl(fd, SPI_IOC_RD_BITS_PER_WORD, bits) 0) {
- perror(SPI bits_per_word);
- return;
- }
- if (ioctl(fd, SPI_IOC_RD_MAX_SPEED_HZ, speed) 0) {
- perror(SPI max_speed_hz);
- return;
- }
-
- printf(%s: spi mode %d, %d bits %sper word, %d Hz max\n,
- name, mode, bits, lsb ? (lsb first) : , speed);
-}
-
-int main(int argc, char **argv)
-{
- int c;
- int readcount = 0;
- int msglen = 0;
- int fd;
- const char *name;
-
- while ((c = getopt(argc, argv, hm:r:v)) != EOF) {
- switch (c) {
- case 'm':
- msglen = atoi(optarg);
- if (msglen 0)
- goto usage;
- continue;
- case 'r':
- readcount = atoi(optarg);
- if (readcount 0)
- goto usage;
- continue;
- case 'v':
- verbose++;
- continue;
- case 'h':
- case '?':
-usage
Re: [PATCH] queue usb USB_CDC_GET_ENCAPSULATED_RESPONSE message
On Monday 18 February 2008, Jan Altenberg wrote: Hi all, commit 0cf4f2de0a0f4100795f38ef894d4910678c74f8 introduced a bug, which prevents sending an USB_CDC_GET_ENCAPSULATED_RESPONSE message. This breaks the RNDIS initialization (especially / only Windoze machines dislike this behavior...). Signed-off-by: Benedikt Spranger [EMAIL PROTECTED] Signed-off-by: Jan Altenberg [EMAIL PROTECTED] Acked-by: David Brownell [EMAIL PROTECTED] --- drivers/usb/gadget/ether.c |1 + 1 file changed, 1 insertion(+) Index: linux-2.6.24/drivers/usb/gadget/ether.c === --- linux-2.6.24.orig/drivers/usb/gadget/ether.c +++ linux-2.6.24/drivers/usb/gadget/ether.c @@ -1568,6 +1568,7 @@ done_set_intf: memcpy(req-buf, buf, n); req-complete = rndis_response_complete; rndis_free_response(dev-rndis_config, buf); + value = n; } /* else stalls ... spec says to avoid that */ } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi transfer with zero length
On Saturday 16 February 2008, Atsushi Nemoto wrote: > Hi. Is it legal to use zero for 'len' field of struct spi_transfer? > I mean, len=0, tx_buf=rx_buf=NULL, delay_usecs!=0. Yes that should work ... it's uncommon, but not illegal. Some controller drivers may even handle that right! If the delay were zero and cs_change didn't indicate a need to briefly deselect the chip, it might make sense to reject such a NOP transfer. But that's not the case you identify. > Some SPI devices need slightly long delay before first CLK edge after > CS assertion. For future reference ... could you identify a few such devices, and say what "long" is relative to the clock period? Some folk have just slowed down the clock in such cases, but that's rather sub-optimal. > To achieve this, I think inserting using a zero length > transfer before real transfers. But it seems some drivers do not > handle this case properly. Feel free to submit patches fixing those bugs. > Is this driver's bug, or we need additional delay field in struct > spi_device for such case? I'd like to avoid new parameters to cover case that can already be expressed in the programming interface. Cases that can't be expressed ... different issue. I suspect any patches updating timing parameters should use nanoseconds not microseconds, fwiw. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi transfer with zero length
On Saturday 16 February 2008, Atsushi Nemoto wrote: Hi. Is it legal to use zero for 'len' field of struct spi_transfer? I mean, len=0, tx_buf=rx_buf=NULL, delay_usecs!=0. Yes that should work ... it's uncommon, but not illegal. Some controller drivers may even handle that right! If the delay were zero and cs_change didn't indicate a need to briefly deselect the chip, it might make sense to reject such a NOP transfer. But that's not the case you identify. Some SPI devices need slightly long delay before first CLK edge after CS assertion. For future reference ... could you identify a few such devices, and say what long is relative to the clock period? Some folk have just slowed down the clock in such cases, but that's rather sub-optimal. To achieve this, I think inserting using a zero length transfer before real transfers. But it seems some drivers do not handle this case properly. Feel free to submit patches fixing those bugs. Is this driver's bug, or we need additional delay field in struct spi_device for such case? I'd like to avoid new parameters to cover case that can already be expressed in the programming interface. Cases that can't be expressed ... different issue. I suspect any patches updating timing parameters should use nanoseconds not microseconds, fwiw. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Handshaking on USB serial devices
On Thursday 14 February 2008, David Newall wrote: > RS232 is (normally) so much slower than USB that, on an extended > transmission, the buffer internal to the local hardware can fill well > before the remote device has demanded that transmission stop. In fact, > now that you've mentioned it, I can't see that anything to stop the > driver from overflowing the internal buffer, which is very perplexing. > Would that be right? Only for stupidly designed hardware ... which you might well be using, though I happen to never have seen anything that's quite *that* stupid. (There's always a first time though.) USB has enough control flow to prevent that from happening. If the host sends data that the peripheral isn't ready to accept, the peripheral just refuses to accept it, and the host will retry later. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] call gpio_cansleep only after gpio_request succeeded
On Thursday 14 February 2008, Uwe Kleine-König wrote: > If you have GPIO_LIB gpio_cansleep oopses on an invalid > gpio. So better gpio_request your pin first. > > Signed-off-by: Uwe Kleine-König <[EMAIL PROTECTED]> > Cc: David Brownell <[EMAIL PROTECTED]> Acked-by: David Brownell <[EMAIL PROTECTED]> ... my bad, sorry. The "first do gpio_request(), THEN you can test gpio_cansleep()" issue got resolved in that direction a bit late; this is the only in-tree driver that I know would be affected. My test platforms haven't yet been updated to 2.6.25-rc1 ... :( > Cc: Raphael Assenat <[EMAIL PROTECTED]> > Cc: Richard Purdie <[EMAIL PROTECTED]> > --- > Hello, > > I currently start using GPIO_LIB and don't have any chips yet. The Oops will > vanish after I will have registered the chips for my SoC's gpios, but still > this way the code is more robust. > > Best regards > Uwe > > drivers/leds/leds-gpio.c |8 > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c > index 6c0a9c4..76ddcf3 100644 > --- a/drivers/leds/leds-gpio.c > +++ b/drivers/leds/leds-gpio.c > @@ -79,6 +79,10 @@ static int gpio_led_probe(struct platform_device *pdev) > cur_led = >leds[i]; > led_dat = _data[i]; > > + ret = gpio_request(led_dat->gpio, cur_led->name); > + if (ret < 0) > + goto err; > + > led_dat->cdev.name = cur_led->name; > led_dat->cdev.default_trigger = cur_led->default_trigger; > led_dat->gpio = cur_led->gpio; > @@ -87,10 +91,6 @@ static int gpio_led_probe(struct platform_device *pdev) > led_dat->cdev.brightness_set = gpio_led_set; > led_dat->cdev.brightness = LED_OFF; > > - ret = gpio_request(led_dat->gpio, led_dat->cdev.name); > - if (ret < 0) > - goto err; > - > gpio_direction_output(led_dat->gpio, led_dat->active_low); > > INIT_WORK(_dat->work, gpio_led_work); > -- > 1.5.4.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] call gpio_cansleep only after gpio_request succeeded
On Thursday 14 February 2008, Uwe Kleine-König wrote: If you have GPIO_LIB gpio_cansleep oopses on an invalid gpio. So better gpio_request your pin first. Signed-off-by: Uwe Kleine-König [EMAIL PROTECTED] Cc: David Brownell [EMAIL PROTECTED] Acked-by: David Brownell [EMAIL PROTECTED] ... my bad, sorry. The first do gpio_request(), THEN you can test gpio_cansleep() issue got resolved in that direction a bit late; this is the only in-tree driver that I know would be affected. My test platforms haven't yet been updated to 2.6.25-rc1 ... :( Cc: Raphael Assenat [EMAIL PROTECTED] Cc: Richard Purdie [EMAIL PROTECTED] --- Hello, I currently start using GPIO_LIB and don't have any chips yet. The Oops will vanish after I will have registered the chips for my SoC's gpios, but still this way the code is more robust. Best regards Uwe drivers/leds/leds-gpio.c |8 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c index 6c0a9c4..76ddcf3 100644 --- a/drivers/leds/leds-gpio.c +++ b/drivers/leds/leds-gpio.c @@ -79,6 +79,10 @@ static int gpio_led_probe(struct platform_device *pdev) cur_led = pdata-leds[i]; led_dat = leds_data[i]; + ret = gpio_request(led_dat-gpio, cur_led-name); + if (ret 0) + goto err; + led_dat-cdev.name = cur_led-name; led_dat-cdev.default_trigger = cur_led-default_trigger; led_dat-gpio = cur_led-gpio; @@ -87,10 +91,6 @@ static int gpio_led_probe(struct platform_device *pdev) led_dat-cdev.brightness_set = gpio_led_set; led_dat-cdev.brightness = LED_OFF; - ret = gpio_request(led_dat-gpio, led_dat-cdev.name); - if (ret 0) - goto err; - gpio_direction_output(led_dat-gpio, led_dat-active_low); INIT_WORK(led_dat-work, gpio_led_work); -- 1.5.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Handshaking on USB serial devices
On Thursday 14 February 2008, David Newall wrote: RS232 is (normally) so much slower than USB that, on an extended transmission, the buffer internal to the local hardware can fill well before the remote device has demanded that transmission stop. In fact, now that you've mentioned it, I can't see that anything to stop the driver from overflowing the internal buffer, which is very perplexing. Would that be right? Only for stupidly designed hardware ... which you might well be using, though I happen to never have seen anything that's quite *that* stupid. (There's always a first time though.) USB has enough control flow to prevent that from happening. If the host sends data that the peripheral isn't ready to accept, the peripheral just refuses to accept it, and the host will retry later. - Dave -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4 resend] [x86] Add generic GPIO support to x86
On Wednesday 13 February 2008, Andrew Morton wrote:
> It would be more modern to have a which takes care of
> cruddy details, but it's getting too late for that.
Sort of like this? For drivers that don't want to list themselves
in Kconfig as depending on GENERIC_GPIO (e.g. one NAND driver I heard
about), this lets them use ... existing code can't
break, and it won't hurt if new code uses this.
- Dave
== CUT HERE
Add a defining fail/warn stubs for GPIO calls on
platforms that don't support the GPIO programming interface. It
includes the arch-specific interface glue otherwise.
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
---
include/linux/gpio.h | 93 +++
1 file changed, 93 insertions(+)
--- /dev/null 1970-01-01 00:00:00.0 +
+++ g26/include/linux/gpio.h2008-02-13 17:40:06.0 -0800
@@ -0,0 +1,93 @@
+#ifndef __LINUX_GPIO_H
+#define __LINUX_GPIO_H
+
+#ifdef CONFIG_GENERIC_GPIO
+#include
+
+#else
+
+/*
+ * Some platforms don't support the GPIO programming interface.
+ *
+ * In case some driver uses it anyway (it should normally have
+ * depended on GENERIC_GPIO), these routines help the compiler
+ * optimize out much GPIO-related code ... or trigger a runtime
+ * warning when something is wrongly called.
+ */
+
+static inline int gpio_is_valid(int number)
+{
+ return 0;
+}
+
+static inline int gpio_request(unsigned gpio, const char *label)
+{
+ return -EINVAL;
+}
+
+static inline void gpio_free(unsigned gpio)
+{
+ /* GPIO can never have been requested */
+ WARN_ON(1);
+}
+
+static inline int gpio_direction_input(unsigned gpio)
+{
+ return -EINVAL;
+}
+
+static inline int gpio_direction_output(unsigned gpio, int value)
+{
+ return -EINVAL;
+}
+
+static inline int gpio_get_value(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline void gpio_set_value(unsigned gpio, int value)
+{
+ /* GPIO can never have been requested or set as output */
+ WARN_ON(1);
+}
+
+static int gpio_cansleep(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline int gpio_get_value_cansleep(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as {in,out}put */
+ WARN_ON(1);
+ return 0;
+}
+
+static inline void gpio_set_value_cansleep(unsigned gpio, int value)
+{
+ /* GPIO can never have been requested or set as output */
+ WARN_ON(1);
+}
+
+static inline int gpio_to_irq(unsigned gpio)
+{
+ /* GPIO can never have been requested or set as input */
+ WARN_ON(1);
+ return -EINVAL;
+}
+
+static inline int irq_to_gpio(unsigned irq)
+{
+ /* irq can never have been returned from gpio_to_irq() */
+ WARN_ON(1);
+ return -EINVAL;
+}
+
+#endif
+
+#endif /* __LINUX_GPIO_H */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4 resend] [x86] Add generic GPIO support to x86
On Wednesday 13 February 2008, Andrew Morton wrote:
> Someone please fix.
This is what I was using purely for test builds ... most x86 hardware
actually has no GPIOs, and I sure don't have any of the "unusual" x86
platforms here, so I'd not want to see this version merge. Someone
more clued in on how x86 handles what ARM does with e.g.
should make a better patch.
(And my vote is to **NOT** use the "-I..." magic that PowerPC uses,
that's just confusing as all get-out. And not needed.)
> It would be more modern to have a which takes care of
> cruddy details, but it's getting too late for that.
That could be added eventually ... if the platform has GPIO support
it could include the else it could define all the calls
as error-returning inlines.
- Dave
= CUT HERE
DEBUG ONLY -- make X86_PC use gpiolib.
It's not clear to me how the various x86-ish platforms should
be made to work here, since there seems to be no convention
that each platform type has its own subdir.
---
arch/x86/Kconfig |2 ++
include/asm-x86/gpio.h | 19 ++-
2 files changed, 20 insertions(+), 1 deletion(-)
--- g26.orig/arch/x86/Kconfig 2008-02-10 16:06:30.0 -0800
+++ g26/arch/x86/Kconfig2008-02-10 16:09:44.0 -0800
@@ -228,6 +228,8 @@ choice
config X86_PC
bool "PC-compatible"
+ select GENERIC_GPIO
+ select HAVE_GPIO_LIB
help
Choose this option if your computer is a standard PC or compatible.
--- g26.orig/include/asm-x86/gpio.h 2008-02-10 16:06:30.0 -0800
+++ g26/include/asm-x86/gpio.h 2008-02-10 16:09:44.0 -0800
@@ -1,6 +1,23 @@
#ifndef _ASM_I386_GPIO_H
#define _ASM_I386_GPIO_H
-#include
+// #include
+
+#include/* cansleep wrappers */
+
+#define gpio_get_value __gpio_get_value
+#define gpio_set_value __gpio_set_value
+#define gpio_cansleep __gpio_cansleep
+
+static inline int gpio_to_irq(unsigned gpio)
+{
+ return -ENOSYS;
+}
+
+static inline int irq_to_gpio(unsigned irq)
+{
+ return -EINVAL;
+}
+
#endif /* _ASM_I386_GPIO_H */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: OTG: Fix weirdnesses on enumerating partial otg devices
On Wednesday 13 February 2008, Felipe Balbi wrote:
> On Tue, Feb 12, 2008 at 12:32:34PM -0800, David Brownell wrote:
> >
> > Your proposal is to strike the "is_b_host" check. In terms of the
> > OTG (1.3) state machine, that removes a b_host --> b_peripheral
> > state transition.
>
> Not at all, we're just not doing transition right now.
When *would* it happen then? And why not do it as soon as it's
known that the device on the other end of the cable is unsupported?
> > BUT: notice it doesn't replace it with the ONLY other valid state
> > transition, b_host --> b_idle. In fact, that can not be initiated
> > by the B-device...
> >
> > So the current code *is* correct.
> >
> > ... deletia ...
> >
> > > We should at least try to enumerate a_peripheral, if it can't due to
> > > our power capabilities i.e. 100mA, we'll fall into overcurrent
> > > protection and we'll suspend the bus and disconnect, which would give
> > > a_device another change to enumerate us.
> > >
> > > This sound much better to me.
> >
> > You're overlooking something: this is the "!is_targeted(udev)"
> > case, so we have already enumerated the a_peripheral as much as
> > we can. There is *nothing* more we can do with it. Sitting
> > around in the b_host state would be utterly pointless.
>
> And you're overlooking something: tpl is not an enumeration blocker at
> all. The only blockers are drivers and power limits.
The device has already been enumerated at this point. This is just
whether to set up communication with, and use, the device ... which
activity *IS* predicated on device support, "on the TPL" (as it says
in 6.8.1.4 of the spec for the A_HOST role, later reusing that language
where it describes the B_HOST role).
> Imagine what happens on n810, we have 3 mass storage devices listed on
> its tpl: 770, n800 and itself. Besides that, we allow enumeration of any
> mass storage device as long as it draws less than 100mA.
While it makes sense to me that an OTG device support things like
mass storage class devices, I still see that spec language saying
that classes can't be on the TPL ... and that only devices on the
TPL get communication beyond what's needed for enumeration.
Regardless, that's a red herring. If you allow sane things like
arbitrary flash memory sticks to be used, you've effectively put
a device class on the TPL. But this discussion is about things
that are NOT on the TPL.
> These tips I
> got from otg chairman himself and from OTG Clarification document, you
> can find it in compliance.usb.org.
Well, I'm glad at least one person there is talking sanely about
the TPL, but there is no such clarification that I can find.
I've observed USB-IF to be slow to address bugs in their specs,
maybe that's what's going on here. Bugs are often written into
specs as political compromises, and they can linger for various
reasons. (Including members with hidden agendas to block success,
and people who prefer to deny inconvenient realities.)
> So, what I'm trying to say is if
> a_device let us become host, we shold at least try to do, let's call it,
> full enumeration. Which means attaching any unlisted mass storage device
> should work.
Attaching works, sure. If you are willing to communicate with
that device, then it's on the Targeted Peripherals List (TPL),
so this code branch doesn't apply.
Put it this way: there is no *OTHER* mechanism for deciding not
to talk with a device; and that's the entire purpose of the TPL.
> > > 4. B-device become b_host, checks a_peripheral is not on its tpl, even
> > > though it has support for that device class
> >
> > If it supports a device class I'd say that should be included
> > in the TPL ("whitelist"). Though if it tries to do that, it's
> > an explicit violation of the OTG spec (sect 3.3):
>
> We can't support classes. What I'm trying to say is we have usb mass
> storage support enabled in kernel and we can handle _any_ mass storage
> devices as long as they meet otg requirements.
I think that detail of the OTG spec is stupid and misguided.
Of *course* it should be possible to support, for example,
devices that weren't shipped before your product's TPL was
defined. How ever you define the list of Targeted/supported
peripherals, that's got to be able to include classes and
similar mechanisms. That spec stupidity is one issue;
for the scope of this discussion I'll assume the TPL (which
has an algorithmic check) can pass things like flash sticks.
The other issue is what you call the part of your product
documentation which says what devices it can talk to. The
only term for such stuff in the OTG spec is "Targeted
Peripherals List
Re: [PATCH] USB: OTG: Fix weirdnesses on enumerating partial otg devices
On Wednesday 13 February 2008, Felipe Balbi wrote: On Tue, Feb 12, 2008 at 12:32:34PM -0800, David Brownell wrote: Your proposal is to strike the is_b_host check. In terms of the OTG (1.3) state machine, that removes a b_host -- b_peripheral state transition. Not at all, we're just not doing transition right now. When *would* it happen then? And why not do it as soon as it's known that the device on the other end of the cable is unsupported? BUT: notice it doesn't replace it with the ONLY other valid state transition, b_host -- b_idle. In fact, that can not be initiated by the B-device... So the current code *is* correct. ... deletia ... We should at least try to enumerate a_peripheral, if it can't due to our power capabilities i.e. 100mA, we'll fall into overcurrent protection and we'll suspend the bus and disconnect, which would give a_device another change to enumerate us. This sound much better to me. You're overlooking something: this is the !is_targeted(udev) case, so we have already enumerated the a_peripheral as much as we can. There is *nothing* more we can do with it. Sitting around in the b_host state would be utterly pointless. And you're overlooking something: tpl is not an enumeration blocker at all. The only blockers are drivers and power limits. The device has already been enumerated at this point. This is just whether to set up communication with, and use, the device ... which activity *IS* predicated on device support, on the TPL (as it says in 6.8.1.4 of the spec for the A_HOST role, later reusing that language where it describes the B_HOST role). Imagine what happens on n810, we have 3 mass storage devices listed on its tpl: 770, n800 and itself. Besides that, we allow enumeration of any mass storage device as long as it draws less than 100mA. While it makes sense to me that an OTG device support things like mass storage class devices, I still see that spec language saying that classes can't be on the TPL ... and that only devices on the TPL get communication beyond what's needed for enumeration. Regardless, that's a red herring. If you allow sane things like arbitrary flash memory sticks to be used, you've effectively put a device class on the TPL. But this discussion is about things that are NOT on the TPL. These tips I got from otg chairman himself and from OTG Clarification document, you can find it in compliance.usb.org. Well, I'm glad at least one person there is talking sanely about the TPL, but there is no such clarification that I can find. I've observed USB-IF to be slow to address bugs in their specs, maybe that's what's going on here. Bugs are often written into specs as political compromises, and they can linger for various reasons. (Including members with hidden agendas to block success, and people who prefer to deny inconvenient realities.) So, what I'm trying to say is if a_device let us become host, we shold at least try to do, let's call it, full enumeration. Which means attaching any unlisted mass storage device should work. Attaching works, sure. If you are willing to communicate with that device, then it's on the Targeted Peripherals List (TPL), so this code branch doesn't apply. Put it this way: there is no *OTHER* mechanism for deciding not to talk with a device; and that's the entire purpose of the TPL. 4. B-device become b_host, checks a_peripheral is not on its tpl, even though it has support for that device class If it supports a device class I'd say that should be included in the TPL (whitelist). Though if it tries to do that, it's an explicit violation of the OTG spec (sect 3.3): We can't support classes. What I'm trying to say is we have usb mass storage support enabled in kernel and we can handle _any_ mass storage devices as long as they meet otg requirements. I think that detail of the OTG spec is stupid and misguided. Of *course* it should be possible to support, for example, devices that weren't shipped before your product's TPL was defined. How ever you define the list of Targeted/supported peripherals, that's got to be able to include classes and similar mechanisms. That spec stupidity is one issue; for the scope of this discussion I'll assume the TPL (which has an algorithmic check) can pass things like flash sticks. The other issue is what you call the part of your product documentation which says what devices it can talk to. The only term for such stuff in the OTG spec is Targeted Peripherals List. If you're assuming some other design abstraction packages that notion, that doesn't match what the Linux-USB code does. If you wanted to *add* such an abstraction, you'd have to put it everywhere the current whitelist is used ... better IMO to just update the current whitelist/TPL code to handle classes. Right where the comment says to do it. According to otg tpl clarification, they should work
