* Re: w1/ds1wm regression after 2.6.39: "bus error, retrying"
@ 2011-06-26 0:58 Paul Parsons
2011-06-27 13:17 ` [PATCH] w1: ds1wm: add a reset recovery parameter Jean-François Dagenais
0 siblings, 1 reply; 6+ messages in thread
From: Paul Parsons @ 2011-06-26 0:58 UTC (permalink / raw)
To: Jean-François Dagenais; +Cc: linux-kernel, johnpol
[-- Attachment #1: Type: text/plain, Size: 555 bytes --]
Oops, yes the msleep() had been left in. I've attached debug output from the original ds1wm.c. Not sure how the mailing list copes with bzip2 compressed attachments; hopefully it will work.
Regards,
Paul
--- On Sat, 25/6/11, Jean-François Dagenais <dagenaisj@sonatest.com> wrote:
> This seems to be the output after you have restored the
> msleep. Could you provide it without so we can get a better
> idea as to what happened. If indeed this is a major
> regression for many devices, we'll have to submit a patch to
> the main branch.
> /jfd
[-- Attachment #2: dmesg.bz2 --]
[-- Type: application/x-bzip2, Size: 6987 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] w1: ds1wm: add a reset recovery parameter 2011-06-26 0:58 w1/ds1wm regression after 2.6.39: "bus error, retrying" Paul Parsons @ 2011-06-27 13:17 ` Jean-François Dagenais 2011-06-28 9:59 ` Paul Parsons 2011-07-05 21:46 ` Andrew Morton 0 siblings, 2 replies; 6+ messages in thread From: Jean-François Dagenais @ 2011-06-27 13:17 UTC (permalink / raw) To: linux-kernel; +Cc: johnpol, Paul Parsons This follows the regression on 3.0 reported by Paul Parsons regarding the removal of the msleep(1) in the ds1wm_reset() function. This sleep should not be required on normal circuitry provided the pull-ups on the bus are correctly adapted to the slaves. Unfortunately, this is not always the case. The sleep is restored but as a parameter to the probe function in the pdata. --- drivers/mfd/asic3.c | 1 + drivers/mfd/htc-pasic3.c | 1 + drivers/w1/masters/ds1wm.c | 5 +++++ include/linux/mfd/ds1wm.h | 7 +++++++ 4 files changed, 14 insertions(+), 0 deletions(-) diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c index c27fd1f..c71ae09 100644 --- a/drivers/mfd/asic3.c +++ b/drivers/mfd/asic3.c @@ -619,6 +619,7 @@ static void asic3_clk_disable(struct asic3 *asic, struct asic3_clk *clk) /* MFD cells (SPI, PWM, LED, DS1WM, MMC) */ static struct ds1wm_driver_data ds1wm_pdata = { .active_high = 1, + .reset_recover_delay = 1, }; static struct resource ds1wm_resources[] = { diff --git a/drivers/mfd/htc-pasic3.c b/drivers/mfd/htc-pasic3.c index 2808bd1..04c7093 100644 --- a/drivers/mfd/htc-pasic3.c +++ b/drivers/mfd/htc-pasic3.c @@ -99,6 +99,7 @@ static int ds1wm_disable(struct platform_device *pdev) static struct ds1wm_driver_data ds1wm_pdata = { .active_high = 0, + .reset_recover_delay = 1, }; static struct resource ds1wm_resources[] __initdata = { diff --git a/drivers/w1/masters/ds1wm.c b/drivers/w1/masters/ds1wm.c index ad57593..66bf82f 100644 --- a/drivers/w1/masters/ds1wm.c +++ b/drivers/w1/masters/ds1wm.c @@ -109,6 +109,7 @@ struct ds1wm_data { /* byte to write that makes all intr disabled, */ /* considering active_state (IAS) (optimization) */ u8 int_en_reg_none; + unsigned int reset_recover_delay; /* see ds1wm.h */ }; static inline void ds1wm_write_register(struct ds1wm_data *ds1wm_data, u32 reg, @@ -187,6 +188,9 @@ static int ds1wm_reset(struct ds1wm_data *ds1wm_data) return 1; } + if(ds1wm_data->reset_recover_delay) + msleep(ds1wm_data->reset_recover_delay); + return 0; } @@ -490,6 +494,7 @@ static int ds1wm_probe(struct platform_device *pdev) } ds1wm_data->irq = res->start; ds1wm_data->int_en_reg_none = (plat->active_high ? DS1WM_INTEN_IAS : 0); + ds1wm_data->reset_recover_delay = plat->reset_recover_delay; if (res->flags & IORESOURCE_IRQ_HIGHEDGE) irq_set_irq_type(ds1wm_data->irq, IRQ_TYPE_EDGE_RISING); diff --git a/include/linux/mfd/ds1wm.h b/include/linux/mfd/ds1wm.h index be469a3..068044f 100644 --- a/include/linux/mfd/ds1wm.h +++ b/include/linux/mfd/ds1wm.h @@ -3,4 +3,11 @@ struct ds1wm_driver_data { int active_high; int clock_rate; + /* in milliseconds, the amount of time to */ + /* sleep following a reset pulse. Zero */ + /* should work if your bus devices recover*/ + /* time respects the 1-wire spec since the*/ + /* ds1wm implements the precise timings of*/ + /* a reset pulse/presence detect sequence.*/ + unsigned int reset_recover_delay; }; -- 1.7.5.4 Jean-Francois Dagenais Software Architect - B.Sc.A Experience the new veo phased array flaw detector at www.sonatestveo.com. Sonatest AP 2900 chemin des Quatre-Bourgeois Bureau 305 Quebec City Quebec G1V 1Y4 T| +1 (418) 683 6222 x106 F| +1 (418) 683 7032 M| W| www.sonatest.com This message (and any associated files) is intended only for the use of linux-kernel@vger.kernel.org, johnpol@2ka.mipt.ru, lost.distance@yahoo.com and may contain information that is confidential, subject to copyright or constitutes a trade secret. If you are not the above recipient(s) you are hereby notified that any dissemination, copying or distribution of this message, or files associated with this message, is strictly prohibited. If you have received this message in error, please notify us immediately by replying to the message and deleting it from your computer. Any views or opinions presented are solely those of the author and do not necessarily represent those of the company. Think green - help the environment by not printing this email. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] w1: ds1wm: add a reset recovery parameter 2011-06-27 13:17 ` [PATCH] w1: ds1wm: add a reset recovery parameter Jean-François Dagenais @ 2011-06-28 9:59 ` Paul Parsons 2011-07-05 21:46 ` Andrew Morton 1 sibling, 0 replies; 6+ messages in thread From: Paul Parsons @ 2011-06-28 9:59 UTC (permalink / raw) To: linux-kernel, Jean-François Dagenais; +Cc: johnpol Thanks Jean-François; that patch set has resolved the problem. Tested-by: Paul Parsons <lost.distance@yahoo.com> --- On Mon, 27/6/11, Jean-François Dagenais <dagenaisj@sonatest.com> wrote: > From: Jean-François Dagenais <dagenaisj@sonatest.com> > Subject: [PATCH] w1: ds1wm: add a reset recovery parameter > To: linux-kernel@vger.kernel.org > Cc: johnpol@2ka.mipt.ru, "Paul Parsons" <lost.distance@yahoo.com> > Date: Monday, 27 June, 2011, 14:17 > This follows the regression on 3.0 > reported by Paul Parsons regarding > the removal of the msleep(1) in the ds1wm_reset() function. > This > sleep should not be required on normal circuitry provided > the > pull-ups on the bus are correctly adapted to the slaves. > Unfortunately, this is not always the case. The sleep is > restored > but as a parameter to the probe function in the pdata. > --- > drivers/mfd/asic3.c | > 1 + > drivers/mfd/htc-pasic3.c | 1 > + > drivers/w1/masters/ds1wm.c | 5 +++++ > include/linux/mfd/ds1wm.h | 7 +++++++ > 4 files changed, 14 insertions(+), 0 deletions(-) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] w1: ds1wm: add a reset recovery parameter 2011-06-27 13:17 ` [PATCH] w1: ds1wm: add a reset recovery parameter Jean-François Dagenais 2011-06-28 9:59 ` Paul Parsons @ 2011-07-05 21:46 ` Andrew Morton 2011-07-06 13:16 ` Jean-François Dagenais 1 sibling, 1 reply; 6+ messages in thread From: Andrew Morton @ 2011-07-05 21:46 UTC (permalink / raw) To: Jean-François Dagenais; +Cc: linux-kernel, johnpol, Paul Parsons On Mon, 27 Jun 2011 09:17:58 -0400 Jean-Fran__ois Dagenais <dagenaisj@sonatest.com> wrote: > This follows the regression on 3.0 reported by Paul Parsons regarding > the removal of the msleep(1) in the ds1wm_reset() function. This > sleep should not be required on normal circuitry provided the > pull-ups on the bus are correctly adapted to the slaves. > Unfortunately, this is not always the case. The sleep is restored > but as a parameter to the probe function in the pdata. A few things. - When fixing a bug, please fully describe that bug in the changelog. So that others can determine whether your patch might fix a bug which they are observing. That's not so important when we're fixing a regression which was introduced since the most recent major kernel release, but it's good practice and who knows, someone might have backported the regression-introducing patch. - The patch was missing your signed-off-by. I added it. Please ack this. - It's conventional to give bug-reporters a hat tip via the Reported-by: tag. Here's what I merged: From: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com> This fixes a regression on 3.0 reported by Paul Parsons regarding the removal of the msleep(1) in the ds1wm_reset() function: : The linux-3.0-rc4 DS1WM 1-wire driver is logging "bus error, retrying" : error messages on an HP iPAQ hx4700 PDA (XScale-PXA270): : : <snip> : Driver for 1-wire Dallas network protocol. : DS1WM w1 busmaster driver - (c) 2004 Szabolcs Gyurko : 1-Wire driver for the DS2760 battery monitor chip - (c) 2004-2005, Szabolcs Gyurko : ds1wm ds1wm: pass: 1 bus error, retrying : ds1wm ds1wm: pass: 2 bus error, retrying : ds1wm ds1wm: pass: 3 bus error, retrying : ds1wm ds1wm: pass: 4 bus error, retrying : ds1wm ds1wm: pass: 5 bus error, retrying : ... : : The visible result is that the battery charging LED is erratic; sometimes : it works, mostly it doesn't. : : The linux-2.6.39 DS1WM 1-wire driver worked OK. I haven't tried 3.0-rc1, : 3.0-rc2, or 3.0-rc3. This sleep should not be required on normal circuitry provided the pull-ups on the bus are correctly adapted to the slaves. Unfortunately, this is not always the case. The sleep is restored but as a parameter to the probe function in the pdata. Reported-by: Paul Parsons <lost.distance@yahoo.com> Tested-by: Paul Parsons <lost.distance@yahoo.com> Signed-off-by: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com> Cc: Evgeniy Polyakov <johnpol@2ka.mipt.ru> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- drivers/mfd/asic3.c | 1 + drivers/mfd/htc-pasic3.c | 1 + drivers/w1/masters/ds1wm.c | 5 +++++ include/linux/mfd/ds1wm.h | 7 +++++++ 4 files changed, 14 insertions(+) diff -puN drivers/mfd/asic3.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/mfd/asic3.c --- a/drivers/mfd/asic3.c~w1-ds1wm-add-a-reset-recovery-parameter +++ a/drivers/mfd/asic3.c @@ -619,6 +619,7 @@ static void asic3_clk_disable(struct asi /* MFD cells (SPI, PWM, LED, DS1WM, MMC) */ static struct ds1wm_driver_data ds1wm_pdata = { .active_high = 1, + .reset_recover_delay = 1, }; static struct resource ds1wm_resources[] = { diff -puN drivers/mfd/htc-pasic3.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/mfd/htc-pasic3.c --- a/drivers/mfd/htc-pasic3.c~w1-ds1wm-add-a-reset-recovery-parameter +++ a/drivers/mfd/htc-pasic3.c @@ -99,6 +99,7 @@ static int ds1wm_disable(struct platform static struct ds1wm_driver_data ds1wm_pdata = { .active_high = 0, + .reset_recover_delay = 1, }; static struct resource ds1wm_resources[] __initdata = { diff -puN drivers/w1/masters/ds1wm.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/w1/masters/ds1wm.c --- a/drivers/w1/masters/ds1wm.c~w1-ds1wm-add-a-reset-recovery-parameter +++ a/drivers/w1/masters/ds1wm.c @@ -109,6 +109,7 @@ struct ds1wm_data { /* byte to write that makes all intr disabled, */ /* considering active_state (IAS) (optimization) */ u8 int_en_reg_none; + unsigned int reset_recover_delay; /* see ds1wm.h */ }; static inline void ds1wm_write_register(struct ds1wm_data *ds1wm_data, u32 reg, @@ -187,6 +188,9 @@ static int ds1wm_reset(struct ds1wm_data return 1; } + if(ds1wm_data->reset_recover_delay) + msleep(ds1wm_data->reset_recover_delay); + return 0; } @@ -490,6 +494,7 @@ static int ds1wm_probe(struct platform_d } ds1wm_data->irq = res->start; ds1wm_data->int_en_reg_none = (plat->active_high ? DS1WM_INTEN_IAS : 0); + ds1wm_data->reset_recover_delay = plat->reset_recover_delay; if (res->flags & IORESOURCE_IRQ_HIGHEDGE) irq_set_irq_type(ds1wm_data->irq, IRQ_TYPE_EDGE_RISING); diff -puN include/linux/mfd/ds1wm.h~w1-ds1wm-add-a-reset-recovery-parameter include/linux/mfd/ds1wm.h --- a/include/linux/mfd/ds1wm.h~w1-ds1wm-add-a-reset-recovery-parameter +++ a/include/linux/mfd/ds1wm.h @@ -3,4 +3,11 @@ struct ds1wm_driver_data { int active_high; int clock_rate; + /* in milliseconds, the amount of time to */ + /* sleep following a reset pulse. Zero */ + /* should work if your bus devices recover*/ + /* time respects the 1-wire spec since the*/ + /* ds1wm implements the precise timings of*/ + /* a reset pulse/presence detect sequence.*/ + unsigned int reset_recover_delay; }; _ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] w1: ds1wm: add a reset recovery parameter 2011-07-05 21:46 ` Andrew Morton @ 2011-07-06 13:16 ` Jean-François Dagenais 2011-07-06 21:25 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: Jean-François Dagenais @ 2011-07-06 13:16 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel On Jul 5, 2011, at 17:46, Andrew Morton wrote: > On Mon, 27 Jun 2011 09:17:58 -0400 > Jean-Fran__ois Dagenais <dagenaisj@sonatest.com> wrote: > >> This follows the regression on 3.0 reported by Paul Parsons regarding >> the removal of the msleep(1) in the ds1wm_reset() function. This >> sleep should not be required on normal circuitry provided the >> pull-ups on the bus are correctly adapted to the slaves. >> Unfortunately, this is not always the case. The sleep is restored >> but as a parameter to the probe function in the pdata. > > A few things. > > - When fixing a bug, please fully describe that bug in the changelog. > So that others can determine whether your patch might fix a bug > which they are observing. > > That's not so important when we're fixing a regression which was > introduced since the most recent major kernel release, but it's good > practice and who knows, someone might have backported the > regression-introducing patch. > > - The patch was missing your signed-off-by. I added it. Please > ack this. > > - It's conventional to give bug-reporters a hat tip via the > Reported-by: tag. > All noted. Thanks for the constructive feedback. ds1wm mods are my first contrib to the kernel. > Here's what I merged: > > > From: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com> If the c-cedilla doesn't work please replace the '_' by a regular 'c'... actually I will re-submit the patch as v2. > > This fixes a regression on 3.0 reported by Paul Parsons regarding the > removal of the msleep(1) in the ds1wm_reset() function: > > : The linux-3.0-rc4 DS1WM 1-wire driver is logging "bus error, retrying" > : error messages on an HP iPAQ hx4700 PDA (XScale-PXA270): > : > : <snip> > : Driver for 1-wire Dallas network protocol. > : DS1WM w1 busmaster driver - (c) 2004 Szabolcs Gyurko > : 1-Wire driver for the DS2760 battery monitor chip - (c) 2004-2005, Szabolcs Gyurko > : ds1wm ds1wm: pass: 1 bus error, retrying > : ds1wm ds1wm: pass: 2 bus error, retrying > : ds1wm ds1wm: pass: 3 bus error, retrying > : ds1wm ds1wm: pass: 4 bus error, retrying > : ds1wm ds1wm: pass: 5 bus error, retrying > : ... > : > : The visible result is that the battery charging LED is erratic; sometimes > : it works, mostly it doesn't. > : > : The linux-2.6.39 DS1WM 1-wire driver worked OK. I haven't tried 3.0-rc1, > : 3.0-rc2, or 3.0-rc3. > > This sleep should not be required on normal circuitry provided the > pull-ups on the bus are correctly adapted to the slaves. Unfortunately, > this is not always the case. The sleep is restored but as a parameter to > the probe function in the pdata. > > Reported-by: Paul Parsons <lost.distance@yahoo.com> > Tested-by: Paul Parsons <lost.distance@yahoo.com> > Signed-off-by: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com> again, the '_' > Cc: Evgeniy Polyakov <johnpol@2ka.mipt.ru> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > --- > > drivers/mfd/asic3.c | 1 + > drivers/mfd/htc-pasic3.c | 1 + > drivers/w1/masters/ds1wm.c | 5 +++++ > include/linux/mfd/ds1wm.h | 7 +++++++ > 4 files changed, 14 insertions(+) > > diff -puN drivers/mfd/asic3.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/mfd/asic3.c > --- a/drivers/mfd/asic3.c~w1-ds1wm-add-a-reset-recovery-parameter > +++ a/drivers/mfd/asic3.c > @@ -619,6 +619,7 @@ static void asic3_clk_disable(struct asi > /* MFD cells (SPI, PWM, LED, DS1WM, MMC) */ > static struct ds1wm_driver_data ds1wm_pdata = { > .active_high = 1, > + .reset_recover_delay = 1, > }; > > static struct resource ds1wm_resources[] = { > diff -puN drivers/mfd/htc-pasic3.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/mfd/htc-pasic3.c > --- a/drivers/mfd/htc-pasic3.c~w1-ds1wm-add-a-reset-recovery-parameter > +++ a/drivers/mfd/htc-pasic3.c > @@ -99,6 +99,7 @@ static int ds1wm_disable(struct platform > > static struct ds1wm_driver_data ds1wm_pdata = { > .active_high = 0, > + .reset_recover_delay = 1, > }; > > static struct resource ds1wm_resources[] __initdata = { > diff -puN drivers/w1/masters/ds1wm.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/w1/masters/ds1wm.c > --- a/drivers/w1/masters/ds1wm.c~w1-ds1wm-add-a-reset-recovery-parameter > +++ a/drivers/w1/masters/ds1wm.c > @@ -109,6 +109,7 @@ struct ds1wm_data { > /* byte to write that makes all intr disabled, */ > /* considering active_state (IAS) (optimization) */ > u8 int_en_reg_none; > + unsigned int reset_recover_delay; /* see ds1wm.h */ > }; > > static inline void ds1wm_write_register(struct ds1wm_data *ds1wm_data, u32 reg, > @@ -187,6 +188,9 @@ static int ds1wm_reset(struct ds1wm_data > return 1; > } > > + if(ds1wm_data->reset_recover_delay) > + msleep(ds1wm_data->reset_recover_delay); > + > return 0; > } > > @@ -490,6 +494,7 @@ static int ds1wm_probe(struct platform_d > } > ds1wm_data->irq = res->start; > ds1wm_data->int_en_reg_none = (plat->active_high ? DS1WM_INTEN_IAS : 0); > + ds1wm_data->reset_recover_delay = plat->reset_recover_delay; > > if (res->flags & IORESOURCE_IRQ_HIGHEDGE) > irq_set_irq_type(ds1wm_data->irq, IRQ_TYPE_EDGE_RISING); > diff -puN include/linux/mfd/ds1wm.h~w1-ds1wm-add-a-reset-recovery-parameter include/linux/mfd/ds1wm.h > --- a/include/linux/mfd/ds1wm.h~w1-ds1wm-add-a-reset-recovery-parameter > +++ a/include/linux/mfd/ds1wm.h > @@ -3,4 +3,11 @@ > struct ds1wm_driver_data { > int active_high; > int clock_rate; > + /* in milliseconds, the amount of time to */ > + /* sleep following a reset pulse. Zero */ > + /* should work if your bus devices recover*/ > + /* time respects the 1-wire spec since the*/ > + /* ds1wm implements the precise timings of*/ > + /* a reset pulse/presence detect sequence.*/ > + unsigned int reset_recover_delay; > }; > _ > > Jean-Francois Dagenais Software Architect - B.Sc.A Experience the new veo phased array flaw detector at www.sonatestveo.com. Sonatest AP 2900 chemin des Quatre-Bourgeois Bureau 305 Quebec City Quebec G1V 1Y4 T| +1 (418) 683 6222 x106 F| +1 (418) 683 7032 M| W| www.sonatest.com This message (and any associated files) is intended only for the use of akpm@linux-foundation.org, linux-kernel@vger.kernel.org and may contain information that is confidential, subject to copyright or constitutes a trade secret. If you are not the above recipient(s) you are hereby notified that any dissemination, copying or distribution of this message, or files associated with this message, is strictly prohibited. If you have received this message in error, please notify us immediately by replying to the message and deleting it from your computer. Any views or opinions presented are solely those of the author and do not necessarily represent those of the company. Think green - help the environment by not printing this email. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] w1: ds1wm: add a reset recovery parameter 2011-07-06 13:16 ` Jean-François Dagenais @ 2011-07-06 21:25 ` Andrew Morton 0 siblings, 0 replies; 6+ messages in thread From: Andrew Morton @ 2011-07-06 21:25 UTC (permalink / raw) To: Jean-François Dagenais; +Cc: linux-kernel On Wed, 6 Jul 2011 09:16:57 -0400 Jean-François Dagenais <dagenaisj@Sonatest.com> wrote: > > From: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com> > If the c-cedilla doesn't work please replace the '_' by a regular 'c'... actually I will re-submit the patch as v2. It seems that a python script whcih Uwe wrote for me has broken. Let me see if I can send it back for repair.. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-06 21:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-06-26 0:58 w1/ds1wm regression after 2.6.39: "bus error, retrying" Paul Parsons 2011-06-27 13:17 ` [PATCH] w1: ds1wm: add a reset recovery parameter Jean-François Dagenais 2011-06-28 9:59 ` Paul Parsons 2011-07-05 21:46 ` Andrew Morton 2011-07-06 13:16 ` Jean-François Dagenais 2011-07-06 21:25 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox