[PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Bartlomiej Zolnierkiewicz]

Maximum supported UDMA mode for AEC6280[R] is UDMA5 (not UDMA4)
and for AEC6880[R] it is UDMA6 (not UDMA5):

* Fix the problem by adding missing struct ata_port_info to artop_init_one().

* Use the right naming (s/626/628/).

* Bump driver version.

Fixes IDE->libata regression, problem was never present in IDE aec62xx driver.

Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/pata_artop.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Index: b/drivers/ata/pata_artop.c
===================================================================
--- a/drivers/ata/pata_artop.c
+++ b/drivers/ata/pata_artop.c
@@ -2,6 +2,7 @@
  *    pata_artop.c - ARTOP ATA controller driver
  *
  *	(C) 2006 Red Hat <alan@redhat.com>
+ *	(C) 2007 Bartlomiej Zolnierkiewicz
  *
  *    Based in part on drivers/ide/pci/aec62xx.c
  *	Copyright (C) 1999-2002	Andre Hedrick <andre@linux-ide.org>
@@ -28,7 +29,7 @@
 #include <linux/ata.h>
 
 #define DRV_NAME	"pata_artop"
-#define DRV_VERSION	"0.4.3"
+#define DRV_VERSION	"0.4.4"
 
 /*
  *	The ARTOP has 33 Mhz and "over clocked" timing tables. Until we
@@ -430,7 +431,7 @@ static int artop_init_one (struct pci_de
 		.udma_mask 	= ATA_UDMA4,
 		.port_ops	= &artop6260_ops,
 	};
-	static const struct ata_port_info info_626x_fast = {
+	static const struct ata_port_info info_628x = {
 		.sht		= &artop_sht,
 		.flags		= ATA_FLAG_SLAVE_POSS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
@@ -438,6 +439,14 @@ static int artop_init_one (struct pci_de
 		.udma_mask 	= ATA_UDMA5,
 		.port_ops	= &artop6260_ops,
 	};
+	static const struct ata_port_info info_628x_fast = {
+		.sht		= &artop_sht,
+		.flags		= ATA_FLAG_SLAVE_POSS,
+		.pio_mask	= 0x1f,	/* pio0-4 */
+		.mwdma_mask	= 0x07, /* mwdma0-2 */
+		.udma_mask 	= ATA_UDMA6,
+		.port_ops	= &artop6260_ops,
+	};
 	const struct ata_port_info *ppi[] = { NULL, NULL };
 
 	if (!printed_version++)
@@ -455,13 +464,13 @@ static int artop_init_one (struct pci_de
 	}
 	else if (id->driver_data == 1)	/* 6260 */
 		ppi[0] = &info_626x;
-	else if (id->driver_data == 2)	{ /* 6260 or 6260 + fast */
+	else if (id->driver_data == 2)	{ /* 6280 or 6280 + fast */
 		unsigned long io = pci_resource_start(pdev, 4);
 		u8 reg;
 
-		ppi[0] = &info_626x;
+		ppi[0] = &info_628x;
 		if (inb(io) & 0x10)
-			ppi[0] = &info_626x_fast;
+			ppi[0] = &info_628x_fast;
 		/* Mac systems come up with some registers not set as we
 		   will need them */
 

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Alan Cox]

On Thu, 9 Aug 2007 23:19:34 +0200
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> 
> Maximum supported UDMA mode for AEC6280[R] is UDMA5 (not UDMA4)
> and for AEC6880[R] it is UDMA6 (not UDMA5):
> 
> * Fix the problem by adding missing struct ata_port_info to artop_init_one().
> 
> * Use the right naming (s/626/628/).
> 
> * Bump driver version.
> 
> Fixes IDE->libata regression, problem was never present in IDE aec62xx driver.

Have you tested this ??

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Bartlomiej Zolnierkiewicz]

On Friday 10 August 2007, Alan Cox wrote:
> On Thu, 9 Aug 2007 23:19:34 +0200
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > 
> > Maximum supported UDMA mode for AEC6280[R] is UDMA5 (not UDMA4)
> > and for AEC6880[R] it is UDMA6 (not UDMA5):
> > 
> > * Fix the problem by adding missing struct ata_port_info to artop_init_one().
> > 
> > * Use the right naming (s/626/628/).
> > 
> > * Bump driver version.
> > 
> > Fixes IDE->libata regression, problem was never present in IDE aec62xx driver.
> 
> Have you tested this ??

-ENODEV so no and testing is welcomed.

However I went over both drivers to make sure that this change is safe
and correct.

BTW presence of the above bugs would strongly indicate that pata_artop has
never been tested (properly) with AEC6x80[R], otherwise these bugs should
have been noticed and fixed much earlier.

Thanks,
Bart

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Alan Cox]

> BTW presence of the above bugs would strongly indicate that pata_artop has
> never been tested (properly) with AEC6x80[R], otherwise these bugs should
> have been noticed and fixed much earlier.

People don't seem to notice minor speed limiting errors, and its a pretty
obscure controller too. Not suprised (it took ages for them to notice the
CMD64x one too).

It has however been tested and works pretty well for most cases. There's
one platform where you have to hack both drivers to make them work which
I need to investigate further.

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Alan Cox]

On Thu, 9 Aug 2007 23:19:34 +0200
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> 
> Maximum supported UDMA mode for AEC6280[R] is UDMA5 (not UDMA4)
> and for AEC6880[R] it is UDMA6 (not UDMA5):
> 
> * Fix the problem by adding missing struct ata_port_info to artop_init_one().
> 
> * Use the right naming (s/626/628/).
> 
> * Bump driver version.
> 
> Fixes IDE->libata regression, problem was never present in IDE aec62xx driver.
> 
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> Maximum supported UDMA mode for AEC6280[R] is UDMA5 (not UDMA4)
> and for AEC6880[R] it is UDMA6 (not UDMA5):
> 
> * Fix the problem by adding missing struct ata_port_info to artop_init_one().
> 
> * Use the right naming (s/626/628/).
> 
> * Bump driver version.
> 
> Fixes IDE->libata regression, problem was never present in IDE aec62xx driver.
> 
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ata/pata_artop.c |   19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)

applied to #upstream-fixes

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Mikael Pettersson]

On Fri, 10 Aug 2007 01:45:38 +0200, Bartlomiej Zolnierkiewicz wrote:
> On Friday 10 August 2007, Alan Cox wrote:
> > On Thu, 9 Aug 2007 23:19:34 +0200
> > Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > 
> > > 
> > > Maximum supported UDMA mode for AEC6280[R] is UDMA5 (not UDMA4)
> > > and for AEC6880[R] it is UDMA6 (not UDMA5):
> > > 
> > > * Fix the problem by adding missing struct ata_port_info to artop_init_one().
> > > 
> > > * Use the right naming (s/626/628/).
> > > 
> > > * Bump driver version.
> > > 
> > > Fixes IDE->libata regression, problem was never present in IDE aec62xx driver.
> > 
> > Have you tested this ??
> 
> -ENODEV so no and testing is welcomed.
> 
> However I went over both drivers to make sure that this change is safe
> and correct.
> 
> BTW presence of the above bugs would strongly indicate that pata_artop has
> never been tested (properly) with AEC6x80[R], otherwise these bugs should
> have been noticed and fixed much earlier.

Tested on ATP865 (1191:0008 rev 07) in a DS101 box (an ARM-based NAS).
Seems to work fine.

However, I'm gettting consistently lower read throughput with pata_artop
(13-19 MB/s) than with aec62xx (22 MB/s) on an oldish WD drive, and using
pata_artop triggers a WARN_ON(irqs_disabled()) in arch/arm/mm/consistent.c:

WARNING: at arch/arm/mm/consistent.c:364 dma_free_coherent()
[<c001fe7c>] (dump_stack+0x0/0x14) from [<c00210ec>] (dma_free_coherent+0x38/0x200)
[<c00210b4>] (dma_free_coherent+0x0/0x200) from [<c00253a8>] (dma_unmap_sg+0x15c/0x198)
[<c002524c>] (dma_unmap_sg+0x0/0x198) from [<c0111984>] (ata_sg_clean+0xe4/0x1dc)
[<c01118a0>] (ata_sg_clean+0x0/0x1dc) from [<c0111ac8>] (__ata_qc_complete+0x4c/0xcc)
 r8:c02eca20 r7:00000004 r6:c02ec000 r5:c02ec000 r4:c02eca20
[<c0111a7c>] (__ata_qc_complete+0x0/0xcc) from [<c0111be8>] (ata_qc_complete+0xa0/0xe4)
 r5:c02eca20 r4:c02eca20
[<c0111b48>] (ata_qc_complete+0x0/0xe4) from [<c01121b8>] (ata_hsm_qc_complete+0x104/0x10c)
 r4:00000000
[<c01120b4>] (ata_hsm_qc_complete+0x0/0x10c) from [<c01127fc>] (ata_hsm_move+0x63c/0x694)
 r6:c02ec000 r5:00000050 r4:00000000
[<c01121c0>] (ata_hsm_move+0x0/0x694) from [<c0116628>] (ata_interrupt+0x188/0x240)
[<c01164a0>] (ata_interrupt+0x0/0x240) from [<c0051b24>] (handle_IRQ_event+0x44/0x84)
[<c0051ae0>] (handle_IRQ_event+0x0/0x84) from [<c005339c>] (handle_level_irq+0xb0/0x108)
 r7:c02250e8 r6:00000000 r5:0000001c r4:c020e600
[<c00532ec>] (handle_level_irq+0x0/0x108) from [<c001b044>] (__exception_text_start+0x44/0x60)
 r5:c020e600 r4:0000001c
[<c001b000>] (__exception_text_start+0x0/0x60) from [<c001ba44>] (__irq_svc+0x24/0x60)
Exception stack(0xc0209f64 to 0xc0209fac)
9f60:          00000000 00000002 00000000 00000000 c001cfa0 c0208000 c0018de8 
9f80: c02250e8 00017570 690541f1 0001746c c0209fc0 c0209fac c0209fac c001cd38 
9fa0: c001cfa4 60000013 ffffffff                                              
 r6:10000000 r5:0000001f r4:ffffffff
[<c001cce4>] (cpu_idle+0x0/0x8c) from [<c018f020>] (rest_init+0x48/0x58)
 r5:c02174d0 r4:c021fa58
[<c018efd8>] (rest_init+0x0/0x58) from [<c0008b7c>] (start_kernel+0x238/0x294)
[<c0008944>] (start_kernel+0x0/0x294) from [<00008038>] (0x8038)

So far I've only seen this happen when I run hdparm -Tt /dev/sda.

So something's still not quite right.

aec62xx is perfectly reliable on this box.

/Mikael

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Alan Cox]

> Tested on ATP865 (1191:0008 rev 07) in a DS101 box (an ARM-based NAS).
> Seems to work fine.

Its tested fairly hard on several NAS boxes (and still needs a fix for
one)

> However, I'm gettting consistently lower read throughput with pata_artop
> (13-19 MB/s) than with aec62xx (22 MB/s) on an oldish WD drive, and using
> pata_artop triggers a WARN_ON(irqs_disabled()) in arch/arm/mm/consistent.c:

It would be nice to know why - is the cable detet coming out right on
this ?

> WARNING: at arch/arm/mm/consistent.c:364 dma_free_coherent()

ARM has a problem with dma_free_coherent with IRQ disabled. Unfortunately
under high load ARM decides to use dma_free_coherent inside
dma_unmap_sg(). This as far as I can see is an ARM problem not a libata
problem and one you can duplicate with other drivers under high load.

If any ARM people can tell me what is going on and help fix it either in
ARM or libata that would be great.

Alan

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Mikael Pettersson]

On Fri, 10 Aug 2007 18:33:43 +0100, Alan Cox wrote:
> > However, I'm gettting consistently lower read throughput with pata_artop
> > (13-19 MB/s) than with aec62xx (22 MB/s) on an oldish WD drive, and using
> > pata_artop triggers a WARN_ON(irqs_disabled()) in arch/arm/mm/consistent.c:
> 
> It would be nice to know why - is the cable detet coming out right on
> this ?

The box has a short 40-wire cable, so pata_artop drops to udma/33
while aec62xx does udma/100 without intervention. I added an override
to artop6260_cable_detect() to make it return PATA40_SHORT on this
platform, and with that it does udma/100 as expected.

Read performance fluctuates quite a bit, but seems to be 1-3 MB/s
slower than aec62xx on average. I compared lspci -vvxxx and the
only differences are a latency setting and some ROM thingy:

--- lspci-ide	2007-08-12 13:07:12.000000000 +0200
+++ lspci-libata	2007-08-12 13:12:55.000000000 +0200
@@ -1,32 +1,32 @@
 00:01.0 Mass storage controller: Artop Electronic Corp ATP865 NO-ROM (rev 07)
 	Subsystem: Artop Electronic Corp ATP865 NO-ROM
 	Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B-
 	Status: Cap+ 66Mhz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
-	Latency: 0 (2750ns min, 1000ns max), Cache Line Size 08
+	Latency: 144 (2750ns min, 1000ns max), Cache Line Size 08
 	Interrupt: pin A routed to IRQ 28
 	Region 0: I/O ports at 1050 [size=8]
 	Region 1: I/O ports at 1060 [size=4]
 	Region 2: I/O ports at 1058 [size=8]
 	Region 3: I/O ports at 1064 [size=4]
 	Region 4: I/O ports at 1040 [size=16]
-	Expansion ROM at 48000000 [disabled by cmd] [size=64K]
+	Expansion ROM at 48000000 [disabled] [size=64K]
 	Capabilities: [58] Power Management version 2
 		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
 		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
-00: 91 11 08 00 45 01 90 02 07 00 80 01 08 00 00 00
+00: 91 11 08 00 45 01 90 02 07 00 80 01 08 90 00 00
 10: 51 10 00 00 61 10 00 00 59 10 00 00 65 10 00 00
 20: 41 10 00 00 00 00 00 00 00 00 00 00 91 11 08 00
-30: 01 00 00 48 58 00 00 00 00 00 00 00 1c 01 0b 04
+30: 00 00 00 48 58 00 00 00 00 00 00 00 1c 01 0b 04
 40: 31 00 00 00 06 00 00 00 70 84 96 00 00 00 00 02
 50: ff ff ff ff f0 ff 34 50 01 00 02 06 00 00 00 00
 60: 31 00 00 00 06 00 00 00 70 84 96 00 00 00 00 02
 70: 00 00 00 00 f0 ff 34 50 01 00 02 06 00 00 00 00
 80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 c0: 31 00 00 00 06 00 00 00 70 84 96 00 00 00 00 02
 d0: 00 00 00 00 f0 ff 34 50 01 00 02 06 00 00 00 00
 e0: 31 00 00 00 06 00 00 00 70 84 96 00 00 00 00 02
 f0: 00 00 00 00 f0 ff 34 50 01 00 02 06 00 00 00 00

> > WARNING: at arch/arm/mm/consistent.c:364 dma_free_coherent()
> 
> ARM has a problem with dma_free_coherent with IRQ disabled. Unfortunately
> under high load ARM decides to use dma_free_coherent inside
> dma_unmap_sg(). This as far as I can see is an ARM problem not a libata
> problem and one you can duplicate with other drivers under high load.

Yes, I now see that happening also with the IDE aec62xx driver.
It used to only be a single-line WARNING, only recently has the
ARM kernel started to also do a stack dump when it triggers.

/Mikael

Re: [PATCH] pata_artop: fix UDMA5 for AEC6280[R] and UDMA6 for AEC6880[R]

[Alan Cox]

> > It would be nice to know why - is the cable detet coming out right on
> > this ?
> 
> The box has a short 40-wire cable, so pata_artop drops to udma/33
> while aec62xx does udma/100 without intervention. I added an override

Curious as both use the same cable detect logic. 

> to artop6260_cable_detect() to make it return PATA40_SHORT on this
> platform, and with that it does udma/100 as expected.

Patch would be good. Looks like there are a couple of cases anyway where
artop needs platform specific uglies


> 
> Read performance fluctuates quite a bit, but seems to be 1-3 MB/s
> slower than aec62xx on average. I compared lspci -vvxxx and the
> only differences are a latency setting and some ROM thingy:

Latency shouldn't matter but you can tweak the driver to verify easily
enough