[PATCH] pdc202xx_old: fix resetproc() method

[PATCH] pdc202xx_old: fix resetproc() method

[Sergei Shtylyov]

pdc202xx_reset() calls pdc202xx_reset_host() twice, for both channels, while
that function actually twiddles the single, shared software reset bit -- the
net effect is a duplicated reset and horrendous 4 second delay happening not
only on a channel reset but also when dma_lost_irq() and dma_clear() methods
are called.  Fold pdc202xx_reset_host() into pdc202xx_reset(), fix printk(),
and move it before the actual reset...

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
The patch is against the ide-2.6.git master branch...

Bart, I know you have the docs... from the now removed source code I was able
to figure out that the bit in question most probably drives RST- signal on both
channels (it seems to require re-tuning drives on both channels which we are
currently not doing). If I'm right, why the hell we're twiddling it on a normal
SRST reset, and what's worse on an interrupt timeout conditions? Anyway, it's
hardly a good idea to reset both channels when you only have problem with only
one of them, and I don't see any justification to doing that... So maybe this
patch should've actually wiped out that whole reset insanity for good?..

 drivers/ide/pdc202xx_old.c |   19 +++++--------------
 1 files changed, 5 insertions(+), 14 deletions(-)

Index: ide-2.6/drivers/ide/pdc202xx_old.c
===================================================================
--- ide-2.6.orig/drivers/ide/pdc202xx_old.c
+++ ide-2.6/drivers/ide/pdc202xx_old.c
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 1998-2002		Andre Hedrick <andre@linux-ide.org>
- *  Copyright (C) 2006-2007		MontaVista Software, Inc.
+ *  Copyright (C) 2006-2007, 2009	MontaVista Software, Inc.
  *  Copyright (C) 2007			Bartlomiej Zolnierkiewicz
  *
  *  Portions Copyright (C) 1999 Promise Technology, Inc.
@@ -227,28 +227,19 @@ somebody_else:
 	return (dma_stat & 4) == 4;	/* return 1 if INTR asserted */
 }
 
-static void pdc202xx_reset_host (ide_hwif_t *hwif)
+static void pdc202xx_reset(ide_drive_t *drive)
 {
+	ide_hwif_t *hwif	= drive->hwif;
 	unsigned long high_16	= hwif->extra_base - 16;
 	u8 udma_speed_flag	= inb(high_16 | 0x001f);
 
+	printk(KERN_WARNING "PDC202xx: software reset...\n");
+
 	outb(udma_speed_flag | 0x10, high_16 | 0x001f);
 	mdelay(100);
 	outb(udma_speed_flag & ~0x10, high_16 | 0x001f);
 	mdelay(2000);	/* 2 seconds ?! */
 
-	printk(KERN_WARNING "PDC202XX: %s channel reset.\n",
-		hwif->channel ? "Secondary" : "Primary");
-}
-
-static void pdc202xx_reset (ide_drive_t *drive)
-{
-	ide_hwif_t *hwif	= drive->hwif;
-	ide_hwif_t *mate	= hwif->mate;
-
-	pdc202xx_reset_host(hwif);
-	pdc202xx_reset_host(mate);
-
 	ide_set_max_pio(drive);
 }
 

Re: [PATCH] pdc202xx_old: fix resetproc() method

[Alan Cox]

On Sat, 30 May 2009 00:07:12 +0400
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> pdc202xx_reset() calls pdc202xx_reset_host() twice, for both channels, while
> that function actually twiddles the single, shared software reset bit 

Including while the other channel is active it appears - that doesn't
look good if so

Re: [PATCH] pdc202xx_old: fix resetproc() method

[Bartlomiej Zolnierkiewicz]

On Friday 29 May 2009 22:07:12 Sergei Shtylyov wrote:
> pdc202xx_reset() calls pdc202xx_reset_host() twice, for both channels, while
> that function actually twiddles the single, shared software reset bit -- the
> net effect is a duplicated reset and horrendous 4 second delay happening not
> only on a channel reset but also when dma_lost_irq() and dma_clear() methods
> are called.  Fold pdc202xx_reset_host() into pdc202xx_reset(), fix printk(),
> and move it before the actual reset...
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
> ---
> The patch is against the ide-2.6.git master branch...
> 
> Bart, I know you have the docs... from the now removed source code I was able
> to figure out that the bit in question most probably drives RST- signal on both
> channels (it seems to require re-tuning drives on both channels which we are
> currently not doing). If I'm right, why the hell we're twiddling it on a normal
> SRST reset, and what's worse on an interrupt timeout conditions? Anyway, it's

Unfortunately the documentation doesn't say anything more than that the changing
of the bit value results in SRST on both channels...

IIRC from some old discussions this is required by the chipset sometimes..

> hardly a good idea to reset both channels when you only have problem with only
> one of them, and I don't see any justification to doing that... So maybe this
> patch should've actually wiped out that whole reset insanity for good?..

I'm pretty sure that there are valid reasons behind some of this insanity,
OTOH (per mail from Alan) it completely ignores the fact that the other port
may be active.. 

We may try removing it in incremental patch so it is easier to track/revert
it if things go wrong..

Re: [PATCH] pdc202xx_old: fix resetproc() method

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>pdc202xx_reset() calls pdc202xx_reset_host() twice, for both channels, while
>>that function actually twiddles the single, shared software reset bit -- the
>>net effect is a duplicated reset and horrendous 4 second delay happening not
>>only on a channel reset but also when dma_lost_irq() and dma_clear() methods
>>are called.  Fold pdc202xx_reset_host() into pdc202xx_reset(), fix printk(),
>>and move it before the actual reset...

>>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>>---
>>The patch is against the ide-2.6.git master branch...

>>Bart, I know you have the docs... from the now removed source code I was able
>>to figure out that the bit in question most probably drives RST- signal on both
>>channels (it seems to require re-tuning drives on both channels which we are
>>currently not doing). If I'm right, why the hell we're twiddling it on a normal
>>SRST reset, and what's worse on an interrupt timeout conditions? Anyway, it's

> Unfortunately the documentation doesn't say anything more than that the changing
> of the bit value results in SRST on both channels...

    I find it somewhat hard to believe that this bit controls SRST bits in 
the device control registers -- we have them both under our control anyway, 
why would we need such a "shortcut". I'd rather believe that this bit 
controls RESET- signal on both channels, with the neigboring bit 3 setting 
it to tristate mode the same way as it works on HPT37x (where BTW the 
datasheets call RESET- "SRST_") -- all this probably having to do with the 
hotswap support (busproc() method that manipulated bit 3 was removed from 
that driver by me 3 years ago)...

> IIRC from some old discussions this is required by the chipset sometimes..

    If that bit indeed controls SRST bits, I don't see how setting it in 
resetproc() helps (it can only harm, unwittingly resetting another channel). 
If this bit controls RESET-, I again don't see how/why it helps to 
hard-reset the channels in addition to the soft reset just performed. When
pdc202xx_reset() is called as the dma_clear() method, the channel is 
probably going be reset soon due to BSY=1, and when pdc202xx_reset() is 
called from dma_lost_irq() method, it seems a clear overkill...
	
>>hardly a good idea to reset both channels when you only have problem with only
>>one of them, and I don't see any justification to doing that... So maybe this
>>patch should've actually wiped out that whole reset insanity for good?..

> I'm pretty sure that there are valid reasons behind some of this insanity,
> OTOH (per mail from Alan) it completely ignores the fact that the other port
> may be active.. 

> We may try removing it in incremental patch so it is easier to track/revert
> it if things go wrong..

    OK, sounds like a plan...

MBR, Sergei

Re: [PATCH] pdc202xx_old: fix resetproc() method

[Bartlomiej Zolnierkiewicz]

On Friday 29 May 2009 22:07:12 Sergei Shtylyov wrote:
> pdc202xx_reset() calls pdc202xx_reset_host() twice, for both channels, while
> that function actually twiddles the single, shared software reset bit -- the
> net effect is a duplicated reset and horrendous 4 second delay happening not
> only on a channel reset but also when dma_lost_irq() and dma_clear() methods
> are called.  Fold pdc202xx_reset_host() into pdc202xx_reset(), fix printk(),
> and move it before the actual reset...
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

applied