* [RFC] w1: Disable irqs in critical section
@ 2011-09-07 8:48 Jan Weitzel
2011-09-07 17:50 ` Evgeniy Polyakov
0 siblings, 1 reply; 6+ messages in thread
From: Jan Weitzel @ 2011-09-07 8:48 UTC (permalink / raw)
To: linux-kernel; +Cc: zbr, Jan Weitzel
Interrupting w1_delay in w1_read_bit results in missing the low level
on the w1 line and receiving "1" instead of "0".
Adding local_irq_save / local_irq_restore around the critical section
Signed-off-by: Jan Weitzel <j.weitzel@phytec.de>
---
drivers/w1/w1_io.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c
index 8e8b64c..a059eaa 100644
--- a/drivers/w1/w1_io.c
+++ b/drivers/w1/w1_io.c
@@ -158,13 +158,18 @@ EXPORT_SYMBOL_GPL(w1_write_8);
static u8 w1_read_bit(struct w1_master *dev)
{
int result;
+ unsigned long flags;
+ /* sample timing is critical here */
+ local_irq_save(flags);
dev->bus_master->write_bit(dev->bus_master->data, 0);
w1_delay(6);
dev->bus_master->write_bit(dev->bus_master->data, 1);
w1_delay(9);
result = dev->bus_master->read_bit(dev->bus_master->data);
+ local_irq_restore(flags);
+
w1_delay(55);
return result & 0x1;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC] w1: Disable irqs in critical section
2011-09-07 8:48 [RFC] w1: Disable irqs in critical section Jan Weitzel
@ 2011-09-07 17:50 ` Evgeniy Polyakov
2011-09-08 5:46 ` Jan Weitzel
0 siblings, 1 reply; 6+ messages in thread
From: Evgeniy Polyakov @ 2011-09-07 17:50 UTC (permalink / raw)
To: Jan Weitzel; +Cc: linux-kernel
On Wed, Sep 07, 2011 at 10:48:32AM +0200, Jan Weitzel (j.weitzel@phytec.de) wrote:
> Interrupting w1_delay in w1_read_bit results in missing the low level
> on the w1 line and receiving "1" instead of "0".
> Adding local_irq_save / local_irq_restore around the critical section
This means that CPU will be essentially stuck for 15 useconds for every
bit transferred, doesn't really look like a good idea.
Are you absolutely sure that missing bit is because of timings and not
some other bug?
--
Evgeniy Polyakov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] w1: Disable irqs in critical section
2011-09-07 17:50 ` Evgeniy Polyakov
@ 2011-09-08 5:46 ` Jan Weitzel
2011-09-13 22:41 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Jan Weitzel @ 2011-09-08 5:46 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
Am Mittwoch, den 07.09.2011, 21:50 +0400 schrieb Evgeniy Polyakov:
> On Wed, Sep 07, 2011 at 10:48:32AM +0200, Jan Weitzel (j.weitzel@phytec.de) wrote:
> > Interrupting w1_delay in w1_read_bit results in missing the low level
> > on the w1 line and receiving "1" instead of "0".
> > Adding local_irq_save / local_irq_restore around the critical section
>
> This means that CPU will be essentially stuck for 15 useconds for every
> bit transferred, doesn't really look like a good idea.
>
> Are you absolutely sure that missing bit is because of timings and not
> some other bug?
>
I trigger a gpio line after the samplepoint. I case of a wrong bit the
sample is taken after the "0 gap". The cycle time (samplt to sample)is
increased form about 80µs to 95µs.
I did the measurement with the w1-gpio driver on a OMAP4 board.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] w1: Disable irqs in critical section
2011-09-08 5:46 ` Jan Weitzel
@ 2011-09-13 22:41 ` Andrew Morton
2011-09-14 12:23 ` Jan Weitzel
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2011-09-13 22:41 UTC (permalink / raw)
To: J.Weitzel; +Cc: Evgeniy Polyakov, linux-kernel, Andrew Morton
On Thu, 08 Sep 2011 07:46:21 +0200
Jan Weitzel <J.Weitzel@phytec.de> wrote:
> Am Mittwoch, den 07.09.2011, 21:50 +0400 schrieb Evgeniy Polyakov:
> > On Wed, Sep 07, 2011 at 10:48:32AM +0200, Jan Weitzel (j.weitzel@phytec.de) wrote:
> > > Interrupting w1_delay in w1_read_bit results in missing the low level
> > > on the w1 line and receiving "1" instead of "0".
> > > Adding local_irq_save / local_irq_restore around the critical section
> >
> > This means that CPU will be essentially stuck for 15 useconds for every
> > bit transferred, doesn't really look like a good idea.
> >
> > Are you absolutely sure that missing bit is because of timings and not
> > some other bug?
> >
>
> I trigger a gpio line after the samplepoint. I case of a wrong bit the
> sample is taken after the "0 gap". The cycle time (samplt to sample)is
> increased form about 80__s to 95__s.
> I did the measurement with the w1-gpio driver on a OMAP4 board.
>
I'm not clear on how w1 actually works. Is it a bit-banging
protocol in which the timing is provided by the host CPU? If so then
yes, we should carefully disable interrupts in places where an
interrupt would disrupt critical timing. (But what to do about NMIs
and SMIs?)
Disabling interrupts for 15us is pretty obnoxious, but there's a 55us
delay there with interrupts enabled, so the overall effect shouldn't be
too bad.
Finally, can we fine-tune the interrupt-disabled section a bit? For
example, can the local_irq_disable() be moved to after the
write_bit(..., 0)?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] w1: Disable irqs in critical section
2011-09-13 22:41 ` Andrew Morton
@ 2011-09-14 12:23 ` Jan Weitzel
2011-09-14 19:45 ` Evgeniy Polyakov
0 siblings, 1 reply; 6+ messages in thread
From: Jan Weitzel @ 2011-09-14 12:23 UTC (permalink / raw)
To: Andrew Morton; +Cc: Evgeniy Polyakov, linux-kernel, Andrew Morton
Am Dienstag, den 13.09.2011, 15:41 -0700 schrieb Andrew Morton:
> On Thu, 08 Sep 2011 07:46:21 +0200
> Jan Weitzel <J.Weitzel@phytec.de> wrote:
>
> > Am Mittwoch, den 07.09.2011, 21:50 +0400 schrieb Evgeniy Polyakov:
> > > On Wed, Sep 07, 2011 at 10:48:32AM +0200, Jan Weitzel (j.weitzel@phytec.de) wrote:
> > > > Interrupting w1_delay in w1_read_bit results in missing the low level
> > > > on the w1 line and receiving "1" instead of "0".
> > > > Adding local_irq_save / local_irq_restore around the critical section
> > >
> > > This means that CPU will be essentially stuck for 15 useconds for every
> > > bit transferred, doesn't really look like a good idea.
> > >
> > > Are you absolutely sure that missing bit is because of timings and not
> > > some other bug?
> > >
> >
> > I trigger a gpio line after the samplepoint. I case of a wrong bit the
> > sample is taken after the "0 gap". The cycle time (samplt to sample)is
> > increased form about 80__s to 95__s.
> > I did the measurement with the w1-gpio driver on a OMAP4 board.
> >
>
> I'm not clear on how w1 actually works. Is it a bit-banging
> protocol in which the timing is provided by the host CPU? If so then
correct.
> yes, we should carefully disable interrupts in places where an
> interrupt would disrupt critical timing. (But what to do about NMIs
> and SMIs?)
fortunately on our embedded platform we have no SMIs
> Disabling interrupts for 15us is pretty obnoxious, but there's a 55us
> delay there with interrupts enabled, so the overall effect shouldn't be
> too bad.
>
> Finally, can we fine-tune the interrupt-disabled section a bit? For
> example, can the local_irq_disable() be moved to after the
> write_bit(..., 0)?
>
The falling edge from 1 to 0 starts timing for the slave. After the 6µs
the slave should drive the line low (In case the information bit is
"0"). So there is no additional edge on the line. If there is an
interrupt and the low pulse from master is longer, slave did't see it.
Master need to generate the 1 -> 0 transition and measure after 15µs
I tested it anyhow. Behaviour is slightly better than without disabling
interrupts. Only disabling it for 15µs fix the problem.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] w1: Disable irqs in critical section
2011-09-14 12:23 ` Jan Weitzel
@ 2011-09-14 19:45 ` Evgeniy Polyakov
0 siblings, 0 replies; 6+ messages in thread
From: Evgeniy Polyakov @ 2011-09-14 19:45 UTC (permalink / raw)
To: Jan Weitzel; +Cc: Andrew Morton, linux-kernel, Andrew Morton
On Wed, Sep 14, 2011 at 02:23:25PM +0200, Jan Weitzel (J.Weitzel@phytec.de) wrote:
> > Finally, can we fine-tune the interrupt-disabled section a bit? For
> > example, can the local_irq_disable() be moved to after the
> > write_bit(..., 0)?
> >
> The falling edge from 1 to 0 starts timing for the slave. After the 6µs
> the slave should drive the line low (In case the information bit is
> "0"). So there is no additional edge on the line. If there is an
> interrupt and the low pulse from master is longer, slave did't see it.
> Master need to generate the 1 -> 0 transition and measure after 15µs
>
> I tested it anyhow. Behaviour is slightly better than without disabling
> interrupts. Only disabling it for 15µs fix the problem.
Sad story, but if it is the only solution, then we should go with it.
Andrew, please push this patch upstream.
Thank you Jan.
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
--
Evgeniy Polyakov
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-14 19:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-07 8:48 [RFC] w1: Disable irqs in critical section Jan Weitzel
2011-09-07 17:50 ` Evgeniy Polyakov
2011-09-08 5:46 ` Jan Weitzel
2011-09-13 22:41 ` Andrew Morton
2011-09-14 12:23 ` Jan Weitzel
2011-09-14 19:45 ` Evgeniy Polyakov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox