ata_piix: fix pio/mwdma programming (for testing, don't apply)

PATCH In review Tejun Heo 7 participants 33 messages Feb 2, 2007 – May 28, 2007 libata

lore

Share & tools

lore

Tejun Heo

author

2007-02-02 15:19 UTC #0 lore raw

PATCH 1/1 ata_piix: fix pio/mwdma programming (for testing, don't apply)

Hello, Art Haas, Alan.

Okay, here's another try at fixing the detection bug. I went through intel ich docs and compared with the ide piix driver. This patch fixes the following problems.

* Control bits in the timing register wasn't cleared properly while programming PIO mode.

* MWDMA mode programming cleared the wrong part of control bits. I think this can fix your problem.

* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.

Please test and report the result. Thanks.

FOR TESTING. DO NOT APPLY.

drivers/ata/ata_piix.c +17 -6

index c6bf1a3..51f55a0 100644
@@ -728,8 +728,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;
 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */

@@ -737,12 +743,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);

@@ -859,7 +867,7 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);

@@ -871,8 +879,11 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}
 	}
 	/* Don't scribble on 0x48 if the controller does not support UDMA */
 	if (ap->udma_mask)

Sergei Shtylyov

2007-02-02 15:34 UTC #1 lore raw

Hello.

Quoted 9 lines · Tejun Heo

Okay, here's another try at fixing the detection bug. I went through intel ich docs and compared with the ide piix driver. This patch fixes the following problems.
drivers/ata/ata_piix.c +0 -0
index c6bf1a3..51f55a0 100644

drivers/ata/ata_piix.c static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 734

Quoted 7 lines

drivers/ata/ata_piix.c +4 -0

@@ -728,8 +728,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */

Actually, I think ata_timing_merge() should just be performed when setting
MWDMA mode... This should be the right thing to do in most cases (however, this hardware has some complications in the form of only 2-bit wide active/recovery counts and 2 fast timing bank select bits)...

drivers/ata/ata_piix.c static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 738

Quoted 4 lines

drivers/ata/ata_piix.c +2 -0

 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;

Yeah, I've fixed this oversight in piix.c...

Quoted 31 lines

drivers/ata/ata_piix.c +6 -4

 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */

@@ -737,12 +743,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);

@@ -859,7 +867,7 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);

drivers/ata/ata_piix.c static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i · line 886

Quoted 10 lines

drivers/ata/ata_piix.c +5 -2

@@ -871,8 +879,11 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}

I've also noticed that this is done at the end of piix_set_piomode() and I
see no reason why. Isn't it just a leftover from the piix.c brokenness? This driver coupled PIO and UDMA timing updates for no conceivable reason?

MBR, Sergei

Jeff Garzik

2007-02-02 16:38 UTC #2 lore raw

It's amazing how poorly we have programmed PIIX, for the lifespan of Linux...

Jeff

Mark Lord

2007-02-02 16:57 UTC #3 lore raw

Quoted 1 line · Jeff Garzik

It's amazing how poorly we have programmed PIIX, for the lifespan of Linux...

Actually, we've only been fiddling timings on PIIX since 1998 or so, when AH began taking over Linux IDE. Still, 10 years is an eternity.

Cheers

Sergei Shtylyov

2007-02-02 18:35 UTC #4 lore raw

Hello.

Quoted 4 lines · Mark Lord

Quoted 1 line
It's amazing how poorly we have programmed PIIX, for the lifespan of Linux...
Actually, we've only been fiddling timings on PIIX since 1998 or so, when AH began taking over Linux IDE. Still, 10 years is an eternity.

The most funny thing is that despite being called piix.c the driver seems
to have always been broken for the original PIIX which didn't have separate mastre/slave timings. :-)

Quoted 1 line

Cheers

MBR, Sergei

Tejun Heo

author

2007-02-02 16:41 UTC #5 lore raw

In reply to Sergei Shtylyov

Quoted 9 lines · Sergei Shtylyov

drivers/ata/ata_piix.c static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 734
Quoted 4 lines
drivers/ata/ata_piix.c +4 -0
+    /* PIO configuration clears DTE unconditionally.  It will be
+     * programmed in set_dmamode which is guaranteed to be called
+     * after set_piomode if any DMA mode is available.
+     */
Actually, I think ata_timing_merge() should just be performed when
setting MWDMA mode... This should be the right thing to do in most cases (however, this hardware has some complications in the form of only 2-bit wide active/recovery counts and 2 fast timing bank select bits)...

Yeap, that'll be nice. Dunno whether modifying piix/ata_piix too much would be a good idea tho considering the wide usage.

Quoted 6 lines

drivers/ata/ata_piix.c static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 738
Quoted 4 lines
     pci_read_config_word(dev, master_port, &master_data);
     if (is_slave) {
drivers/ata/ata_piix.c +2 -0
+        /* clear TIME1|IE1|PPE1|DTE1 */
+        master_data &= 0xff0f;
Yeah, I've fixed this oversight in piix.c...

Great, please consider updating ata_piix together next time. libata can really use some help from a someone who knows a lot about PATA including mode programming.

Quoted 9 lines

drivers/ata/ata_piix.c static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i · line 886
Quoted 4 lines
drivers/ata/ata_piix.c +4 -0
+        if (ap->udma_mask) {
+            udma_enable &= ~(1 << devid);
+            pci_write_config_word(dev, master_port, master_data);
+        }
I've also noticed that this is done at the end of piix_set_piomode()
and I see no reason why. Isn't it just a leftover from the piix.c brokenness? This driver coupled PIO and UDMA timing updates for no conceivable reason?

In both mwdma and pio cases, they're just turning off UDMA. Don't know whether it's actually necessary but still afraid to change it unless there is a good reason.

Signature · 1 line

tejun

Alan

2007-02-02 18:37 UTC #6 lore raw

Quoted 7 lines · On Sat, 03 Feb 2007 01:41:41 +0900 Tejun Heo <[email protected]> wrote:

Quoted 4 lines
Actually, I think ata_timing_merge() should just be performed when
setting MWDMA mode... This should be the right thing to do in most cases (however, this hardware has some complications in the form of only 2-bit wide active/recovery counts and 2 fast timing bank select bits)...
Yeap, that'll be nice. Dunno whether modifying piix/ata_piix too much would be a good idea tho considering the wide usage.

If I remember rightly the 2bits is ok because you can set the bit to say that timings are for DMA only, the device then uses slower than PIO0 for all other cycles.

Quoted 3 lines

In both mwdma and pio cases, they're just turning off UDMA. Don't know whether it's actually necessary but still afraid to change it unless there is a good reason.

The tuning manual I have does it, so I do it 8)

Alan

Sergei Shtylyov

2007-02-02 19:04 UTC #7 lore raw

Hello.

Quoted 10 lines · Alan

Quoted 7 lines
Quoted 4 lines
Actually, I think ata_timing_merge() should just be performed when
setting MWDMA mode... This should be the right thing to do in most cases (however, this hardware has some complications in the form of only 2-bit wide active/recovery counts and 2 fast timing bank select bits)...
Yeap, that'll be nice. Dunno whether modifying piix/ata_piix too much would be a good idea tho considering the wide usage.
If I remember rightly the 2bits is ok because you can set the bit to say that timings are for DMA only,

Yeah, after thinking a bit more, the current logic seems good enough, i.
e. it's better to slow down PIO to mode 0 than further slow down DMA to match the current PIO speed -- however, this is already happening with MWDMA1 which is actually faster than PIO3 (150 vs 180 ns). But well, who cares... :-)

Quoted 1 line

the device then uses slower than PIO0 for all other cycles.

Well, thankfully, compatible mode is PIO0 exactly (except for taskfile
accesses which are way slower indeed).

Quoted 5 lines

Quoted 3 lines
In both mwdma and pio cases, they're just turning off UDMA. Don't know whether it's actually necessary but still afraid to change it unless there is a good reason.
The tuning manual I have does it, so I do it 8)

Do you mean 29860004.pdf? Actually, I'm not seeing anything alike here.
It would've been stupid idea to couple UDMA to PIO but well... after looking at the HighPoint datasheets, one becomes hard to surprise. :-)

Quoted 1 line

Alan

Well, after looking at do_pata_set_dmamode(), I have another question:
why IORDY enable is forced here? It has *nothing* to do with the IDE DMA protocol. (This also seems to be an issue with piix.c...)

MBR, Sergei

Alan

2007-02-02 17:30 UTC #8 lore raw

In reply to Tejun Heo

Quoted 2 lines

* Control bits in the timing register wasn't cleared properly while programming PIO mode.

Yep and if the BIOS programmed the slave into DMA that might not be ideal.

Quoted 2 lines

* MWDMA mode programming cleared the wrong part of control bits. I think this can fix your problem.

Your change does nothing here.

	0x0F + 1 * 0xE1 = 0xF0

It's just a construct to avoid the use of the ugly C "?" operator. The rest of the code uses ? so the change makes sense for style, but it doesn't appear to be a bug.

Quoted 2 lines

* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.

Or on the actual hardware.

Tejun Heo

author

2007-02-03 01:40 UTC #9 lore raw

Quoted 4 lines · Alan

Quoted 2 lines
* Control bits in the timing register wasn't cleared properly while programming PIO mode.
Yep and if the BIOS programmed the slave into DMA that might not be ideal.

How so? The bit will be programmed by set_dmamode() right after set_piomode() is complete.

Quoted 10 lines

Quoted 2 lines
* MWDMA mode programming cleared the wrong part of control bits. I think this can fix your problem.
Your change does nothing here.
	0x0F + 1 * 0xE1 = 0xF0
It's just a construct to avoid the use of the ugly C "?" operator. The rest of the code uses ? so the change makes sense for style, but it doesn't appear to be a bug.

(0x0F + 0xE1 * ap->port_no)

1. when ap->port_no == 0

	(0x0f + 0xe1 * 0) == 0x0f
2. when ap->port-no == 1
	(0x0f + 0xe1 * 1) == 0xf0

(ap->port_no ? 0x0f : 0xf0)

1. when ap->port_no == 0

	(0 ? 0x0f : 0xf0) == 0xf0
2. when ap->port_no == 1
	(1 ? 0x0f : 0xf0) == 0x0f

See the difference? Smart one liners are dangerous. ?* is _much_ better than cryptic arithmetic.

Quoted 4 lines

Quoted 2 lines
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.
Or on the actual hardware.

I was trying to make it more consistent with pio counterpart. We can remove if() from set_piomode too. Let's just keep things in sync between stuff including ide piix driver.

Signature · 1 line

tejun

Alan

2007-02-03 19:52 UTC #10 lore raw

Quoted 4 lines

Quoted 1 line
Yep and if the BIOS programmed the slave into DMA that might not be ideal.
How so? The bit will be programmed by set_dmamode() right after set_piomode() is complete.

IFF we also put the device into a DMA mode. A blacklisted device would be wrong.

Quoted 2 lines

See the difference? Smart one liners are dangerous. ?* is _much_ better than cryptic arithmetic.

Agreed

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.
Or on the actual hardware.
I was trying to make it more consistent with pio counterpart. We can remove if() from set_piomode too. Let's just keep things in sync between stuff including ide piix driver.

The one you must not touch is the UDMA register rather than added UDMA bits in older registers.

Alan

Tejun Heo

author

2007-02-04 02:47 UTC #11 lore raw

Quoted 6 lines · Alan

Quoted 3 lines
Quoted 1 line
Yep and if the BIOS programmed the slave into DMA that might not be ideal.
How so? The bit will be programmed by set_dmamode() right after set_piomode() is complete.
IFF we also put the device into a DMA mode. A blacklisted device would be wrong.

Hmmm... I might be misunderstanding, but if libata is going to put the device into PIO mode, why does it matter what BIOS configured slave to?
The slave is going to be put into PIO mode libata selected so the
control bits should match, no?

[--snip--]

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.
Or on the actual hardware.
I was trying to make it more consistent with pio counterpart. We can remove if() from set_piomode too. Let's just keep things in sync between stuff including ide piix driver.
The one you must not touch is the UDMA register rather than added UDMA bits in older registers.

OIC. I was confused there.

Thanks.

Signature · 1 line

tejun

Art Haas

2007-02-02 21:14 UTC #12 lore raw

In reply to Tejun Heo

Quoted 9 lines · Tejun Heo

Hello, Art Haas, Alan.
Okay, here's another try at fixing the detection bug. I went through intel ich docs and compared with the ide piix driver. This patch fixes the following problems.
* Control bits in the timing register wasn't cleared properly while programming PIO mode.

Quoted 7 lines

* MWDMA mode programming cleared the wrong part of control bits. I think this can fix your problem.
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.
Please test and report the result. Thanks.

Hi. Sorry to say the CD-ROM is still not found. Full 'dmesg' output listed below in hopes it provides clues.

$ dmesg
Linux version 2.6.20-rc7-ajh (arth@pcdebian) (gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)) #3 Fri Feb 2 12:15:03 CST 2007
BIOS-provided physical RAM map:
sanitize start
sanitize end
copy_e820_map() start: 0000000000000000 size: 00000000000a0000 end: 00000000000a0000 type: 1
copy_e820_map() type is E820_RAM
copy_e820_map() start: 00000000000f0000 size: 0000000000010000 end: 0000000000100000 type: 2
copy_e820_map() start: 0000000000100000 size: 000000001fef0000 end: 000000001fff0000 type: 1
copy_e820_map() type is E820_RAM
copy_e820_map() start: 000000001fff0000 size: 0000000000003000 end: 000000001fff3000 type: 4
copy_e820_map() start: 000000001fff3000 size: 000000000000d000 end: 0000000020000000 type: 3
copy_e820_map() start: 00000000ffff0000 size: 0000000000010000 end: 0000000100000000 type: 2
 BIOS-e820: 0000000000000000 - 00000000000a0000 (usable)
 BIOS-e820: 00000000000f0000 - 0000000000100000 (reserved)
 BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
 BIOS-e820: 000000001fff0000 - 000000001fff3000 (ACPI NVS)
 BIOS-e820: 000000001fff3000 - 0000000020000000 (ACPI data)
 BIOS-e820: 00000000ffff0000 - 0000000100000000 (reserved)
511MB LOWMEM available.
Entering add_active_range(0, 0, 131056) 0 entries of 256 used
Zone PFN ranges:
  DMA             0 ->     4096
  Normal       4096 ->   131056
early_node_map[1] active PFN ranges
    0:        0 ->   131056
On node 0 totalpages: 131056
  DMA zone: 32 pages used for memmap
  DMA zone: 0 pages reserved
  DMA zone: 4064 pages, LIFO batch:0
  Normal zone: 991 pages used for memmap
  Normal zone: 125969 pages, LIFO batch:31
DMI 2.1 present.
ACPI: RSDP (v000 GBT                                   ) @ 0x000f6c60
ACPI: RSDT (v001 GBT    AWRDACPI 0x00000000  0x00000000) @ 0x1fff3000
ACPI: FADT (v001 GBT    AWRDACPI 0x00000000  0x00000000) @ 0x1fff3040
ACPI: DSDT (v001 GBT    AWRDACPI 0x00001000 MSFT 0x01000007) @ 0x00000000
ACPI: PM-Timer IO Port: 0x4008
Allocating PCI resources starting at 30000000 (gap: 20000000:dfff0000)
Detected 398.969 MHz processor.
Built 1 zonelists.  Total pages: 130033
Kernel command line: BOOT_IMAGE=2.6.20-rc7-ajh ro root=LABEL=/ root=/dev/sda1
Local APIC disabled by BIOS -- you can enable it with "lapic"
mapped APIC to ffffd000 (01402000)
Enabling fast FPU save and restore... done.
Initializing CPU#0
PID hash table entries: 2048 (order: 11, 8192 bytes)
Console: colour VGA+ 80x25
Dentry cache hash table entries: 65536 (order: 6, 262144 bytes)
Inode-cache hash table entries: 32768 (order: 5, 131072 bytes)
Memory: 516956k/524224k available (1461k kernel code, 6724k reserved, 484k data, 148k init, 0k highmem)
virtual kernel memory layout:
    fixmap  : 0xffff7000 - 0xfffff000   (  32 kB)
    vmalloc : 0xe0800000 - 0xffff5000   ( 503 MB)
    lowmem  : 0xc0000000 - 0xdfff0000   ( 511 MB)
      .init : 0xc02ea000 - 0xc030f000   ( 148 kB)
      .data : 0xc026d68f - 0xc02e6a50   ( 484 kB)
      .text : 0xc0100000 - 0xc026d68f   (1461 kB)
Checking if this processor honours the WP bit even in supervisor mode... Ok.
Calibrating delay using timer specific routine.. 798.28 BogoMIPS (lpj=399143)
Mount-cache hash table entries: 512
CPU: After generic identify, caps: 0183f9ff 00000000 00000000 00000000 00000000 00000000 00000000
CPU: L1 I cache: 16K, L1 D cache: 16K
CPU: L2 cache: 512K
CPU: After all inits, caps: 0183f9ff 00000000 00000000 00000040 00000000 00000000 00000000
Intel machine check architecture supported.
Intel machine check reporting enabled on CPU#0.
CPU: Intel Pentium II (Deschutes) stepping 01
Checking 'hlt' instruction... OK.
ACPI: Core revision 20060707
ACPI: setting ELCR to 0200 (from 1c00)
NET: Registered protocol family 16
ACPI: bus type pci registered
PCI: PCI BIOS revision 2.10 entry at 0xfb3c0, last bus=1
PCI: Using configuration type 1
Setting up standard PCI resources
ACPI: Interpreter enabled
ACPI: Using PIC for interrupt routing
ACPI: PCI Root Bridge [PCI0] (0000:00)
PCI: Probing PCI hardware (bus 00)
ACPI: Assume root bridge [\_SB_.PCI0] bus is 0
* Found PM-Timer Bug on the chipset. Due to workarounds for a bug,
* this clock source is slow. Consider trying other clock sources
PCI quirk: region 4000-403f claimed by PIIX4 ACPI
PCI quirk: region 5000-500f claimed by PIIX4 SMB
Boot video device is 0000:00:08.0
ACPI: PCI Interrupt Routing Table [\_SB_.PCI0._PRT]
ACPI: PCI Interrupt Link [LNKA] (IRQs 3 4 5 6 7 *10 11 12 14 15)
ACPI: PCI Interrupt Link [LNKB] (IRQs 3 4 5 6 7 10 11 12 14 15) *0, disabled.
ACPI: PCI Interrupt Link [LNKC] (IRQs 3 4 5 6 7 10 11 *12 14 15)
ACPI: PCI Interrupt Link [LNKD] (IRQs 3 4 5 6 7 10 *11 12 14 15)
ACPI: Power Resource [PFAN] (on)
Linux Plug and Play Support v0.97 (c) Adam Belay
pnp: PnP ACPI init
pnp: ACPI device : hid PNP0A03
pnp: ACPI device : hid PNP0C01
pnp: ACPI device : hid PNP0C02
pnp: ACPI device : hid PNP0200
pnp: ACPI device : hid PNP0B00
pnp: ACPI device : hid PNP0800
pnp: ACPI device : hid PNP0C04
pnp: ACPI device : hid PNP0700
pnp: ACPI device : hid PNP0501
pnp: ACPI device : hid PNP0501
pnp: ACPI device : hid PNP0400
pnp: ACPI device : hid PNP0303
pnp: PnP ACPI: found 12 devices
PnPBIOS: Disabled by ACPI PNP
SCSI subsystem initialized
libata version 2.00 loaded.
PCI: Using ACPI for IRQ routing
PCI: If a device doesn't work, try "pci=routeirq".  If it helps, post a report
pnp: the driver 'system' has been registered
pnp: match found with the PnP device '00:01' and the driver 'system'
pnp: match found with the PnP device '00:02' and the driver 'system'
PCI: Bridge: 0000:00:01.0
  IO window: d000-dfff
  MEM window: disabled.
  PREFETCH window: disabled.
NET: Registered protocol family 2
IP route cache hash table entries: 4096 (order: 2, 16384 bytes)
TCP established hash table entries: 16384 (order: 4, 65536 bytes)
TCP bind hash table entries: 8192 (order: 3, 32768 bytes)
TCP: Hash tables configured (established 16384 bind 8192)
TCP reno registered
io scheduler noop registered
io scheduler anticipatory registered (default)
io scheduler deadline registered
io scheduler cfq registered
Limiting direct PCI/PCI transfers.
isapnp: Scanning for PnP cards...
isapnp: No Plug & Play device found
Serial: 8250/16550 driver $Revision: 1.90 $ 4 ports, IRQ sharing disabled
serial8250: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
serial8250: ttyS1 at I/O 0x2f8 (irq = 3) is a 16550A
pnp: the driver 'serial' has been registered
pnp: match found with the PnP device '00:08' and the driver 'serial'
00:08: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
pnp: match found with the PnP device '00:09' and the driver 'serial'
00:09: ttyS1 at I/O 0x2f8 (irq = 3) is a 16550A
ata_piix 0000:00:07.1: version 2.00ac7
ata1: PATA max UDMA/33 cmd 0x1F0 ctl 0x3F6 bmdma 0xF000 irq 14
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xF008 irq 15
scsi0 : ata_piix
ata1.00: ATA-2, max UDMA/33, 6303024 sectors: LBA 
ata1.00: ata1: dev 0 multi count 16
ata1.01: ATA-4, max UDMA/66, 16514064 sectors: LBA 
ata1.01: ata1: dev 1 multi count 16
ata1.00: configured for UDMA/33
ata1.01: configured for UDMA/33
scsi1 : ata_piix
ata2.00: ATAPI, max MWDMA1
ata2.00: revalidation failed (errno=-2)
ata2.00: limiting speed to PIO4
ata2: failed to recover some devices, retrying in 5 secs
ata2.00: revalidation failed (errno=-2)
ata2.00: limiting speed to PIO0
ata2: failed to recover some devices, retrying in 5 secs
ata2.00: revalidation failed (errno=-2)
ata2.00: disabled
scsi 0:0:0:0: Direct-Access     ATA      ST33232A         3.02 PQ: 0 ANSI: 5
SCSI device sda: 6303024 512-byte hdwr sectors (3227 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: write cache: disabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sda: 6303024 512-byte hdwr sectors (3227 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: write cache: disabled, read cache: enabled, doesn't support DPO or FUA
 sda: sda1 sda2 < sda5 sda6 sda7 >
sd 0:0:0:0: Attached scsi disk sda
scsi 0:0:1:0: Direct-Access     ATA      FUJITSU MPD3084A DD-0 PQ: 0 ANSI: 5
SCSI device sdb: 16514064 512-byte hdwr sectors (8455 MB)
sdb: Write Protect is off
sdb: Mode Sense: 00 3a 00 00
SCSI device sdb: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sdb: 16514064 512-byte hdwr sectors (8455 MB)
sdb: Write Protect is off
sdb: Mode Sense: 00 3a 00 00
SCSI device sdb: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sdb: sdb1 sdb2 < sdb5 sdb6 > sdb3
sd 0:0:1:0: Attached scsi disk sdb
pnp: the driver 'i8042 kbd' has been registered
pnp: match found with the PnP device '00:0b' and the driver 'i8042 kbd'
pnp: the driver 'i8042 aux' has been registered
PNP: PS/2 Controller [PNP0303:PS2K] at 0x60,0x64 irq 1
PNP: PS/2 controller doesn't have AUX irq; using default 12
serio: i8042 KBD port at 0x60,0x64 irq 1
mice: PS/2 mouse device common for all mice
input: AT Translated Set 2 keyboard as /class/input/input0
TCP cubic registered
Using IPI Shortcut mode
Time: tsc clocksource has been installed.
VFS: Mounted root (ext2 filesystem) readonly.
Freeing unused kernel memory: 148k freed
NET: Registered protocol family 1
Floppy drive(s): fd0 is 1.44M
FDC 0 is a post-1991 82077
Real Time Clock Driver v1.12ac
pnp: the driver 'parport_pc' has been registered
pnp: match found with the PnP device '00:0a' and the driver 'parport_pc'
parport: PnPBIOS parport detected.
parport0: PC-style at 0x378, irq 7 [PCSPP(,...)]
8139too Fast Ethernet driver 0.9.28
ACPI: PCI Interrupt Link [LNKC] enabled at IRQ 12
PCI: setting IRQ 12 as level-triggered
ACPI: PCI Interrupt 0000:00:0a.0[A] -> Link [LNKC] -> GSI 12 (level, low) -> IRQ 12
eth0: RealTek RTL8139 at 0xe0816000, 00:05:5d:45:47:96, IRQ 12
eth0:  Identified 8139 chip type 'RTL-8139C'
Adding 100760k swap on /dev/sda7.  Priority:-1 extents:1 across:100760k
kjournald starting.  Commit interval 5 seconds
EXT3-fs: mounted filesystem with ordered data mode.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: mounted filesystem with ordered data mode.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: mounted filesystem with ordered data mode.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: mounted filesystem with ordered data mode.
NET: Registered protocol family 17
eth0: link up, 100Mbps, full-duplex, lpa 0x45E1
eth0: link up, 100Mbps, full-duplex, lpa 0x45E1
ttyS3: LSR safety check engaged!
ttyS3: LSR safety check engaged!
$

Signature · 4 lines

Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Tejun Heo

author

2007-02-03 02:09 UTC #13 lore raw

(cc'ing Albert, Mark and Sergei. Hi!)

Quoted 2 lines · Art Haas

Hi. Sorry to say the CD-ROM is still not found. Full 'dmesg' output listed below in hopes it provides clues.

Erggghh.. I'm sorry too. I thought I found it this time.

Quoted 2 lines

ata_piix 0000:00:07.1: version 2.00ac7
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xF008 irq 15

You have UDMA/33 PATA port.

Quoted 1 line

ata2.00: ATAPI, max MWDMA1

The initial reset and PACKET_IDENTIFY went well.

Quoted 1 line

ata2.00: revalidation failed (errno=-2)

Then, PACKET_IDENTIFY after configuring transfer mode fails with -ENOENT. Meaning it saw (status & (ATA_BUSY|ATA_DRQ|ATA_ERR|ATA_DF)) == 0 in HSM_ST.

libata resets silently and tries to revalidate the device.

Quoted 7 lines

ata2.00: limiting speed to PIO4
ata2: failed to recover some devices, retrying in 5 secs ata2.00: revalidation failed (errno=-2) ata2.00: limiting speed to PIO0
ata2: failed to recover some devices, retrying in 5 secs ata2.00: revalidation failed (errno=-2) ata2.00: disabled

All of those tries fail and libata gives up the device.

* Something went wrong with mode programming. It seems pretty old (reporting max UDMA/33). Art, can you post the result of 'lspci -nn'?

* After mode programming, it might need some delay between clearing ATA_BUSY and setting ATA_DRQ. Dunno if this is possible. Just throwing out thoughts here.

So, PATA gurus, can you bless us with enlightenment? :-)

Signature · 1 line

tejun

Art Haas

2007-02-03 14:35 UTC #14 lore raw

Quoted 33 lines · Tejun Heo

(cc'ing Albert, Mark and Sergei. Hi!)
Quoted 2 lines · Art Haas
Hi. Sorry to say the CD-ROM is still not found. Full 'dmesg' output listed below in hopes it provides clues.
Erggghh.. I'm sorry too. I thought I found it this time.
Quoted 2 lines
ata_piix 0000:00:07.1: version 2.00ac7
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xF008 irq 15
You have UDMA/33 PATA port.
Quoted 1 line
ata2.00: ATAPI, max MWDMA1
The initial reset and PACKET_IDENTIFY went well.
Quoted 1 line
ata2.00: revalidation failed (errno=-2)
Then, PACKET_IDENTIFY after configuring transfer mode fails with -ENOENT. Meaning it saw (status & (ATA_BUSY|ATA_DRQ|ATA_ERR|ATA_DF)) == 0 in HSM_ST.
libata resets silently and tries to revalidate the device.
Quoted 7 lines
ata2.00: limiting speed to PIO4
ata2: failed to recover some devices, retrying in 5 secs ata2.00: revalidation failed (errno=-2) ata2.00: limiting speed to PIO0
ata2: failed to recover some devices, retrying in 5 secs ata2.00: revalidation failed (errno=-2) ata2.00: disabled

Quoted 4 lines

All of those tries fail and libata gives up the device.
* Something went wrong with mode programming. It seems pretty old (reporting max UDMA/33). Art, can you post the result of 'lspci -nn'?

Here you are ...

# lspci -nn
00:00.0 Host bridge [0600]: Intel Corporation 440BX/ZX/DX - 82443BX/ZX/DX Host bridge [8086:7190] (rev 02)
00:01.0 PCI bridge [0604]: Intel Corporation 440BX/ZX/DX - 82443BX/ZX/DX AGP bridge [8086:7191] (rev 02)
00:07.0 ISA bridge [0601]: Intel Corporation 82371AB/EB/MB PIIX4 ISA [8086:7110] (rev 02)
00:07.1 IDE interface [0101]: Intel Corporation 82371AB/EB/MB PIIX4 IDE [8086:7111] (rev 01)
00:07.2 USB Controller [0c03]: Intel Corporation 82371AB/EB/MB PIIX4 USB [8086:7112] (rev 01)
00:07.3 Bridge [0680]: Intel Corporation 82371AB/EB/MB PIIX4 ACPI [8086:7113] (rev 02)
00:08.0 VGA compatible controller [0300]: S3 Inc. ViRGE/DX or /GX [5333:8a01] (rev 01)
00:0a.0 Ethernet controller [0200]: D-Link System Inc RTL8139 Ethernet [1186:1300] (rev 10)
#

Please let me know of anything thing else I can send that may help.

Art Haas

Signature · 4 lines

Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Mark Lord

2007-02-03 20:10 UTC #15 lore raw

In reply to Tejun Heo

Quoted 4 lines · Tejun Heo

..
Then, PACKET_IDENTIFY after configuring transfer mode fails with -ENOENT. Meaning it saw (status & (ATA_BUSY|ATA_DRQ|ATA_ERR|ATA_DF)) == 0 in HSM_ST.

Quoted 1 line

So, PATA gurus, can you bless us with enlightenment? :-)

Heh.. guaranteeing detection of all the strange implementations out there is part black magic.

But the simple thing to do here is, just for fun, hack the code
to do the infamous 50 millisecond hard wait before issuing the PACKET_IDENTIFY. If that fixes it, then it's just a matter of tuning to discover the real amount of delay required, and a nicer way of doing the delay.

Also, zero out the features register before issuing PACKET_IDENTIFY, if the code isn't already doing that.

After the drive asserts BUSY, and later deasserts BUSY, there might be a slight delay before the drive asserts DRQ. So, it is possible for the status to read zeros in the important bits.

My suggestion is to wait up to the infamous 50 milliseconds again here, if needed.

????

Cheers

Signature · 3 lines

Mark Lord
Real-Time Remedies Inc.
[email protected]

Tejun Heo

author

2007-02-06 09:11 UTC #16 lore raw

Quoted 8 lines · Mark Lord

Quoted 4 lines · Tejun Heo
..
Then, PACKET_IDENTIFY after configuring transfer mode fails with -ENOENT. Meaning it saw (status & (ATA_BUSY|ATA_DRQ|ATA_ERR|ATA_DF)) == 0 in HSM_ST.
..
Quoted 1 line
So, PATA gurus, can you bless us with enlightenment? :-)

Quoted 8 lines

Heh.. guaranteeing detection of all the strange implementations out there is part black magic.
But the simple thing to do here is, just for fun, hack the code to do the infamous 50 millisecond hard wait before issuing the PACKET_IDENTIFY.
If that fixes it, then it's just a matter of tuning to discover the real amount of delay required, and a nicer way of doing the delay.

Okay.

Quoted 2 lines

Also, zero out the features register before issuing PACKET_IDENTIFY, if the code isn't already doing that.

Okay.

Quoted 6 lines

After the drive asserts BUSY, and later deasserts BUSY, there might be a slight delay before the drive asserts DRQ. So, it is possible for the status to read zeros in the important bits.
My suggestion is to wait up to the infamous 50 milliseconds again here, if needed.

Okay, the attached patch does what Mark suggested. Art, can you please give it a shot and report dmesg? My thanks for sticking around till now.

Signature · 1 line

tejun

drivers/ata/libata-core.c +14 -1

index 667acd2..4c81433 100644
@@ -1478,7 +1478,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	}
 
 	tf.protocol = ATA_PROT_PIO;
-	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
+	/* POLLING for polling detection, ISADDR|DEVICE to clear TF */
+	tf.flags |= ATA_TFLAG_POLLING | ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     id, sizeof(id[0]) * ATA_ID_WORDS);

@@ -4477,6 +4478,18 @@ fsm_start:
 				goto fsm_start;
 
 		} else {
+			unsigned long timeout = jiffies + msecs_to_jiffies(50);
+
+			while (time_before(jiffies, timeout)) {
+				if (status & ATA_DRQ)
+					break;
+				ata_dev_printk(qc->dev, KERN_INFO,
+					"XXX: waiting for DRQ, status=0x%x\n",
+					status);
+				msleep(10);
+				status = ata_chk_status(ap);
+			}
+
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
 				/* handle BSY=0, DRQ=0 as error */

Art Haas

2007-02-06 16:33 UTC #17 lore raw

Quoted 29 lines · Tejun Heo

Quoted 8 lines · Mark Lord
Quoted 4 lines · Tejun Heo
..
Then, PACKET_IDENTIFY after configuring transfer mode fails with -ENOENT. Meaning it saw (status & (ATA_BUSY|ATA_DRQ|ATA_ERR|ATA_DF)) == 0 in HSM_ST.
..
Quoted 1 line
So, PATA gurus, can you bless us with enlightenment? :-)
Quoted 8 lines
Heh.. guaranteeing detection of all the strange implementations out there is part black magic.
But the simple thing to do here is, just for fun, hack the code to do the infamous 50 millisecond hard wait before issuing the PACKET_IDENTIFY.
If that fixes it, then it's just a matter of tuning to discover the real amount of delay required, and a nicer way of doing the delay.
Okay.
Quoted 2 lines
Also, zero out the features register before issuing PACKET_IDENTIFY, if the code isn't already doing that.
Okay.
Quoted 4 lines
After the drive asserts BUSY, and later deasserts BUSY, there might be a slight delay before the drive asserts DRQ. So, it is possible for the status to read zeros in the important bits.

Quoted 6 lines

Quoted 2 lines
My suggestion is to wait up to the infamous 50 milliseconds again here, if needed.
Okay, the attached patch does what Mark suggested. Art, can you please give it a shot and report dmesg? My thanks for sticking around till now.

SUCCESS!!!!!

$ dmesg
Linux version 2.6.20-ajh (arth@pcdebian) (gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)) #2 Tue Feb 6 08:36:33 CST 2007
BIOS-provided physical RAM map:
sanitize start
sanitize end
copy_e820_map() start: 0000000000000000 size: 00000000000a0000 end: 00000000000a0000 type: 1
copy_e820_map() type is E820_RAM
copy_e820_map() start: 00000000000f0000 size: 0000000000010000 end: 0000000000100000 type: 2
copy_e820_map() start: 0000000000100000 size: 000000001fef0000 end: 000000001fff0000 type: 1
copy_e820_map() type is E820_RAM
copy_e820_map() start: 000000001fff0000 size: 0000000000003000 end: 000000001fff3000 type: 4
copy_e820_map() start: 000000001fff3000 size: 000000000000d000 end: 0000000020000000 type: 3
copy_e820_map() start: 00000000ffff0000 size: 0000000000010000 end: 0000000100000000 type: 2
 BIOS-e820: 0000000000000000 - 00000000000a0000 (usable)
 BIOS-e820: 00000000000f0000 - 0000000000100000 (reserved)
 BIOS-e820: 0000000000100000 - 000000001fff0000 (usable)
 BIOS-e820: 000000001fff0000 - 000000001fff3000 (ACPI NVS)
 BIOS-e820: 000000001fff3000 - 0000000020000000 (ACPI data)
 BIOS-e820: 00000000ffff0000 - 0000000100000000 (reserved)
511MB LOWMEM available.
Entering add_active_range(0, 0, 131056) 0 entries of 256 used
Zone PFN ranges:
  DMA             0 ->     4096
  Normal       4096 ->   131056
early_node_map[1] active PFN ranges
    0:        0 ->   131056
On node 0 totalpages: 131056
  DMA zone: 32 pages used for memmap
  DMA zone: 0 pages reserved
  DMA zone: 4064 pages, LIFO batch:0
  Normal zone: 991 pages used for memmap
  Normal zone: 125969 pages, LIFO batch:31
DMI 2.1 present.
ACPI: RSDP (v000 GBT                                   ) @ 0x000f6c60
ACPI: RSDT (v001 GBT    AWRDACPI 0x00000000  0x00000000) @ 0x1fff3000
ACPI: FADT (v001 GBT    AWRDACPI 0x00000000  0x00000000) @ 0x1fff3040
ACPI: DSDT (v001 GBT    AWRDACPI 0x00001000 MSFT 0x01000007) @ 0x00000000
ACPI: PM-Timer IO Port: 0x4008
Allocating PCI resources starting at 30000000 (gap: 20000000:dfff0000)
Detected 398.959 MHz processor.
Built 1 zonelists.  Total pages: 130033
Kernel command line: BOOT_IMAGE=2.6.20-ajh ro root=LABEL=/ root=/dev/sda1
Local APIC disabled by BIOS -- you can enable it with "lapic"
mapped APIC to ffffd000 (01402000)
Enabling fast FPU save and restore... done.
Initializing CPU#0
PID hash table entries: 2048 (order: 11, 8192 bytes)
Console: colour VGA+ 80x25
Dentry cache hash table entries: 65536 (order: 6, 262144 bytes)
Inode-cache hash table entries: 32768 (order: 5, 131072 bytes)
Memory: 516956k/524224k available (1460k kernel code, 6724k reserved, 485k data, 148k init, 0k highmem)
virtual kernel memory layout:
    fixmap  : 0xffff7000 - 0xfffff000   (  32 kB)
    vmalloc : 0xe0800000 - 0xffff5000   ( 503 MB)
    lowmem  : 0xc0000000 - 0xdfff0000   ( 511 MB)
      .init : 0xc02ea000 - 0xc030f000   ( 148 kB)
      .data : 0xc026d38f - 0xc02e6a50   ( 485 kB)
      .text : 0xc0100000 - 0xc026d38f   (1460 kB)
Checking if this processor honours the WP bit even in supervisor mode... Ok.
Calibrating delay using timer specific routine.. 798.30 BogoMIPS (lpj=399150)
Mount-cache hash table entries: 512
CPU: After generic identify, caps: 0183f9ff 00000000 00000000 00000000 00000000 00000000 00000000
CPU: L1 I cache: 16K, L1 D cache: 16K
CPU: L2 cache: 512K
CPU: After all inits, caps: 0183f9ff 00000000 00000000 00000040 00000000 00000000 00000000
Intel machine check architecture supported.
Intel machine check reporting enabled on CPU#0.
CPU: Intel Pentium II (Deschutes) stepping 01
Checking 'hlt' instruction... OK.
ACPI: Core revision 20060707
ACPI: setting ELCR to 0200 (from 1c00)
NET: Registered protocol family 16
ACPI: bus type pci registered
PCI: PCI BIOS revision 2.10 entry at 0xfb3c0, last bus=1
PCI: Using configuration type 1
Setting up standard PCI resources
ACPI: Interpreter enabled
ACPI: Using PIC for interrupt routing
ACPI: PCI Root Bridge [PCI0] (0000:00)
PCI: Probing PCI hardware (bus 00)
ACPI: Assume root bridge [\_SB_.PCI0] bus is 0
* Found PM-Timer Bug on the chipset. Due to workarounds for a bug,
* this clock source is slow. Consider trying other clock sources
PCI quirk: region 4000-403f claimed by PIIX4 ACPI
PCI quirk: region 5000-500f claimed by PIIX4 SMB
Boot video device is 0000:00:08.0
ACPI: PCI Interrupt Routing Table [\_SB_.PCI0._PRT]
ACPI: PCI Interrupt Link [LNKA] (IRQs 3 4 5 6 7 *10 11 12 14 15)
ACPI: PCI Interrupt Link [LNKB] (IRQs 3 4 5 6 7 10 11 12 14 15) *0, disabled.
ACPI: PCI Interrupt Link [LNKC] (IRQs 3 4 5 6 7 10 11 *12 14 15)
ACPI: PCI Interrupt Link [LNKD] (IRQs 3 4 5 6 7 10 *11 12 14 15)
ACPI: Power Resource [PFAN] (on)
Linux Plug and Play Support v0.97 (c) Adam Belay
pnp: PnP ACPI init
pnp: ACPI device : hid PNP0A03
pnp: ACPI device : hid PNP0C01
pnp: ACPI device : hid PNP0C02
pnp: ACPI device : hid PNP0200
pnp: ACPI device : hid PNP0B00
pnp: ACPI device : hid PNP0800
pnp: ACPI device : hid PNP0C04
pnp: ACPI device : hid PNP0700
pnp: ACPI device : hid PNP0501
pnp: ACPI device : hid PNP0501
pnp: ACPI device : hid PNP0400
pnp: ACPI device : hid PNP0303
pnp: PnP ACPI: found 12 devices
PnPBIOS: Disabled by ACPI PNP
SCSI subsystem initialized
libata version 2.00 loaded.
PCI: Using ACPI for IRQ routing
PCI: If a device doesn't work, try "pci=routeirq".  If it helps, post a report
pnp: the driver 'system' has been registered
pnp: match found with the PnP device '00:01' and the driver 'system'
pnp: match found with the PnP device '00:02' and the driver 'system'
PCI: Bridge: 0000:00:01.0
  IO window: d000-dfff
  MEM window: disabled.
  PREFETCH window: disabled.
NET: Registered protocol family 2
IP route cache hash table entries: 4096 (order: 2, 16384 bytes)
TCP established hash table entries: 16384 (order: 4, 65536 bytes)
TCP bind hash table entries: 8192 (order: 3, 32768 bytes)
TCP: Hash tables configured (established 16384 bind 8192)
TCP reno registered
io scheduler noop registered
io scheduler anticipatory registered (default)
io scheduler deadline registered
io scheduler cfq registered
Limiting direct PCI/PCI transfers.
isapnp: Scanning for PnP cards...
isapnp: No Plug & Play device found
Serial: 8250/16550 driver $Revision: 1.90 $ 4 ports, IRQ sharing disabled
serial8250: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
serial8250: ttyS1 at I/O 0x2f8 (irq = 3) is a 16550A
pnp: the driver 'serial' has been registered
pnp: match found with the PnP device '00:08' and the driver 'serial'
00:08: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
pnp: match found with the PnP device '00:09' and the driver 'serial'
00:09: ttyS1 at I/O 0x2f8 (irq = 3) is a 16550A
ata_piix 0000:00:07.1: version 2.00ac7
ata1: PATA max UDMA/33 cmd 0x1F0 ctl 0x3F6 bmdma 0xF000 irq 14
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xF008 irq 15
scsi0 : ata_piix
ata1.00: ATA-2, max UDMA/33, 6303024 sectors: LBA 
ata1.00: ata1: dev 0 multi count 16
ata1.01: ATA-4, max UDMA/66, 16514064 sectors: LBA 
ata1.01: ata1: dev 1 multi count 16
ata1.00: configured for UDMA/33
ata1.01: configured for UDMA/33
scsi1 : ata_piix
ata2.00: ATAPI, max MWDMA1
ata2.00: configured for MWDMA1
scsi 0:0:0:0: Direct-Access     ATA      ST33232A         3.02 PQ: 0 ANSI: 5
SCSI device sda: 6303024 512-byte hdwr sectors (3227 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: write cache: disabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sda: 6303024 512-byte hdwr sectors (3227 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: write cache: disabled, read cache: enabled, doesn't support DPO or FUA
 sda: sda1 sda2 < sda5 sda6 sda7 >
sd 0:0:0:0: Attached scsi disk sda
scsi 0:0:1:0: Direct-Access     ATA      FUJITSU MPD3084A DD-0 PQ: 0 ANSI: 5
SCSI device sdb: 16514064 512-byte hdwr sectors (8455 MB)
sdb: Write Protect is off
sdb: Mode Sense: 00 3a 00 00
SCSI device sdb: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sdb: 16514064 512-byte hdwr sectors (8455 MB)
sdb: Write Protect is off
sdb: Mode Sense: 00 3a 00 00
SCSI device sdb: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sdb: sdb1 sdb2 < sdb5 sdb6 > sdb3
sd 0:0:1:0: Attached scsi disk sdb
scsi 1:0:0:0: CD-ROM                     ATAPI CDROM      1.80 PQ: 0 ANSI: 5
pnp: the driver 'i8042 kbd' has been registered
pnp: match found with the PnP device '00:0b' and the driver 'i8042 kbd'
pnp: the driver 'i8042 aux' has been registered
PNP: PS/2 Controller [PNP0303:PS2K] at 0x60,0x64 irq 1
PNP: PS/2 controller doesn't have AUX irq; using default 12
serio: i8042 KBD port at 0x60,0x64 irq 1
mice: PS/2 mouse device common for all mice
input: AT Translated Set 2 keyboard as /class/input/input0
TCP cubic registered
Using IPI Shortcut mode
Time: tsc clocksource has been installed.
VFS: Mounted root (ext2 filesystem) readonly.
Freeing unused kernel memory: 148k freed
NET: Registered protocol family 1
sr0: scsi3-mmc drive: 4x/24x cd/rw xa/form2 cdda tray
Uniform CD-ROM driver Revision: 3.20
sr 1:0:0:0: Attached scsi CD-ROM sr0
Real Time Clock Driver v1.12ac
pnp: the driver 'parport_pc' has been registered
pnp: match found with the PnP device '00:0a' and the driver 'parport_pc'
parport: PnPBIOS parport detected.
parport0: PC-style at 0x378, irq 7 [PCSPP(,...)]
8139too Fast Ethernet driver 0.9.28
ACPI: PCI Interrupt Link [LNKC] enabled at IRQ 12
PCI: setting IRQ 12 as level-triggered
ACPI: PCI Interrupt 0000:00:0a.0[A] -> Link [LNKC] -> GSI 12 (level, low) -> IRQ 12
eth0: RealTek RTL8139 at 0xe0840000, 00:05:5d:45:47:96, IRQ 12
eth0:  Identified 8139 chip type 'RTL-8139C'
Floppy drive(s): fd0 is 1.44M
FDC 0 is a post-1991 82077
Adding 100760k swap on /dev/sda7.  Priority:-1 extents:1 across:100760k
kjournald starting.  Commit interval 5 seconds
EXT3-fs: mounted filesystem with ordered data mode.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: mounted filesystem with ordered data mode.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: mounted filesystem with ordered data mode.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: mounted filesystem with ordered data mode.
NET: Registered protocol family 17
eth0: link up, 100Mbps, full-duplex, lpa 0x45E1
eth0: link up, 100Mbps, full-duplex, lpa 0x45E1
ttyS3: LSR safety check engaged!
ttyS3: LSR safety check engaged!
EXT3 FS on sda5, internal journal
EXT3 FS on sda6, internal journal
EXT3 FS on sdb5, internal journal
EXT3 FS on sdb6, internal journal
kjournald starting.  Commit interval 5 seconds
EXT3 FS on sdb3, internal journal
EXT3-fs: mounted filesystem with ordered data mode.
$

Yay!

In addition to the patch above, the working kernel also has the 'ata_piix.c' patch applied. The combined diffs are listed below.

Thanks for fixing the problems, and I'm glad I could help test patches. I hope these changes can make it into the 2.6.21 kernel.

Art Haas

drivers/ata/ata_piix.c +17 -6

index 47701b2..71f5756 100644
@@ -723,8 +723,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;
 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */

@@ -732,12 +738,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);

@@ -853,7 +861,7 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);

@@ -865,8 +873,11 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}
 	}
 	/* Don't scribble on 0x48 if the controller does not support UDMA */
 	if (ap->udma_mask)

drivers/ata/libata-core.c +14 -1

index 667acd2..4c81433 100644
@@ -1478,7 +1478,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	}
 
 	tf.protocol = ATA_PROT_PIO;
-	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
+	/* POLLING for polling detection, ISADDR|DEVICE to clear TF */
+	tf.flags |= ATA_TFLAG_POLLING | ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     id, sizeof(id[0]) * ATA_ID_WORDS);

@@ -4477,6 +4478,18 @@ fsm_start:
 				goto fsm_start;
 
 		} else {
+			unsigned long timeout = jiffies + msecs_to_jiffies(50);
+
+			while (time_before(jiffies, timeout)) {
+				if (status & ATA_DRQ)
+					break;
+				ata_dev_printk(qc->dev, KERN_INFO,
+					"XXX: waiting for DRQ, status=0x%x\n",
+					status);
+				msleep(10);
+				status = ata_chk_status(ap);
+			}
+
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
 				/* handle BSY=0, DRQ=0 as error */

Signature · 4 lines

Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Tejun Heo

author

2007-02-07 17:55 UTC #18 lore raw

Quoted 8 lines · Art Haas

Quoted 8 lines
Quoted 2 lines
Also, zero out the features register before issuing PACKET_IDENTIFY, if the code isn't already doing that.
Okay.
Quoted 4 lines
After the drive asserts BUSY, and later deasserts BUSY, there might be a slight delay before the drive asserts DRQ. So, it is possible for the status to read zeros in the important bits.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
My suggestion is to wait up to the infamous 50 milliseconds again here, if needed.
Okay, the attached patch does what Mark suggested. Art, can you please give it a shot and report dmesg? My thanks for sticking around till now.
SUCCESS!!!!!

Yay!

[--snip--]

Quoted 11 lines

ata_piix 0000:00:07.1: version 2.00ac7
ata1: PATA max UDMA/33 cmd 0x1F0 ctl 0x3F6 bmdma 0xF000 irq 14
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xF008 irq 15
scsi0 : ata_piix
ata1.00: ATA-2, max UDMA/33, 6303024 sectors: LBA ata1.00: ata1: dev 0 multi count 16 ata1.01: ATA-4, max UDMA/66, 16514064 sectors: LBA ata1.01: ata1: dev 1 multi count 16 ata1.00: configured for UDMA/33
ata1.01: configured for UDMA/33
scsi1 : ata_piix
ata2.00: ATAPI, max MWDMA1
ata2.00: configured for MWDMA1

So, it succeeded without any DRQ wait. Can you please apply only the attached patch over vanilla 2.6.20 and see if your problem is fixed?

This problem has been around for quite a while now and there probably have been other users hit by this out there. Thanks a lot, Mark.

Signature · 1 line

tejun

drivers/ata/libata-core.c +10 -1

index 667acd2..d6fcf0a 100644
@@ -1478,7 +1478,16 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	}
 
 	tf.protocol = ATA_PROT_PIO;
-	tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
+
+	/* Some devices choke if TF registers contain garbage.  Make
+	 * sure those are properly initialized.
+	 */
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+
+	/* Device presence detection is unreliable on some
+	 * controllers.  Always poll IDENTIFY if available.
+	 */
+	tf.flags |= ATA_TFLAG_POLLING;
 
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     id, sizeof(id[0]) * ATA_ID_WORDS);

Art Haas

2007-02-07 19:35 UTC #19 lore raw

Quoted 33 lines · Tejun Heo

Quoted 8 lines · Art Haas
Quoted 8 lines
Quoted 2 lines
Also, zero out the features register before issuing PACKET_IDENTIFY, if the code isn't already doing that.
Okay.
Quoted 4 lines
After the drive asserts BUSY, and later deasserts BUSY, there might be a slight delay before the drive asserts DRQ. So, it is possible for the status to read zeros in the important bits.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
My suggestion is to wait up to the infamous 50 milliseconds again here, if needed.
Okay, the attached patch does what Mark suggested. Art, can you please give it a shot and report dmesg? My thanks for sticking around till now.
SUCCESS!!!!!
Yay!
[--snip--]
Quoted 5 lines
ata_piix 0000:00:07.1: version 2.00ac7
ata1: PATA max UDMA/33 cmd 0x1F0 ctl 0x3F6 bmdma 0xF000 irq 14
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xF008 irq 15 scsi0 : ata_piix
ata1.00: ATA-2, max UDMA/33, 6303024 sectors: LBA
Quoted 8 lines
ata1.00: ata1: dev 0 multi count 16 ata1.01: ATA-4, max UDMA/66, 16514064 sectors: LBA ata1.01: ata1: dev 1 multi count 16 ata1.00: configured for UDMA/33
ata1.01: configured for UDMA/33
scsi1 : ata_piix
ata2.00: ATAPI, max MWDMA1
ata2.00: configured for MWDMA1

Quoted 5 lines

So, it succeeded without any DRQ wait. Can you please apply only the attached patch over vanilla 2.6.20 and see if your problem is fixed?
This problem has been around for quite a while now and there probably have been other users hit by this out there. Thanks a lot, Mark.

SUCCESS (again)!!!!!

$ dmesg
[ ... snip ... ]
ata_piix 0000:00:07.1: version 2.00ac7
ata1: PATA max UDMA/33 cmd 0x1F0 ctl 0x3F6 bmdma 0xF000 irq 14
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xF008 irq 15
scsi0 : ata_piix
ata1.00: ATA-2, max UDMA/33, 6303024 sectors: LBA 
ata1.00: ata1: dev 0 multi count 16
ata1.01: ATA-4, max UDMA/66, 16514064 sectors: LBA 
ata1.01: ata1: dev 1 multi count 16
ata1.00: configured for UDMA/33
ata1.01: configured for UDMA/33
scsi1 : ata_piix
ata2.00: ATAPI, max MWDMA1
ata2.00: configured for MWDMA1
scsi 0:0:0:0: Direct-Access     ATA      ST33232A         3.02 PQ: 0 ANSI: 5
SCSI device sda: 6303024 512-byte hdwr sectors (3227 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: write cache: disabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sda: 6303024 512-byte hdwr sectors (3227 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: write cache: disabled, read cache: enabled, doesn't support DPO or FUA
 sda: sda1 sda2 < sda5 sda6 sda7 >
sd 0:0:0:0: Attached scsi disk sda
scsi 0:0:1:0: Direct-Access     ATA      FUJITSU MPD3084A DD-0 PQ: 0 ANSI: 5
SCSI device sdb: 16514064 512-byte hdwr sectors (8455 MB)
sdb: Write Protect is off
sdb: Mode Sense: 00 3a 00 00
SCSI device sdb: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sdb: 16514064 512-byte hdwr sectors (8455 MB)
sdb: Write Protect is off
sdb: Mode Sense: 00 3a 00 00
SCSI device sdb: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sdb: sdb1 sdb2 < sdb5 sdb6 > sdb3
sd 0:0:1:0: Attached scsi disk sdb
scsi 1:0:0:0: CD-ROM                     ATAPI CDROM      1.80 PQ: 0 ANSI: 5
[ ... snip ... ]
$

The minimal patch is a keeper. Nice to have the CD-ROM available using the libata code on this machine. My other machine with the PIIX setup has not had these problems. Guess this box has quirky/old hardware.

Thanks again for the patch(es); I'm glad to help test them out.

Art Haas

Signature · 4 lines

Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Mark Lord

2007-02-07 19:51 UTC #20 lore raw

Quoted 7 lines · Art Haas

Quoted 6 lines · Tejun Heo
Quoted 4 lines · Art Haas
Quoted 3 lines
Quoted 2 lines
Also, zero out the features register before issuing PACKET_IDENTIFY, if the code isn't already doing that.
Okay.
SUCCESS!!!!!
Yay!

...

Quoted 5 lines

Quoted 3 lines
So, it succeeded without any DRQ wait. Can you please apply only the attached patch over vanilla 2.6.20 and see if your problem is fixed?
This problem has been around for quite a while now and there probably have been other users hit by this out there. Thanks a lot, Mark.
SUCCESS (again)!!!!!

Good stuff, guys.
I'm happy to have been of service.

Cheers

Signature · 3 lines

Mark Lord
Real-Time Remedies Inc.
[email protected]

Tejun Heo

author

2007-02-08 10:37 UTC #21 lore raw

PATCH 1/1 libata: clear TF before IDENTIFYing

Some devices chock if Feature is not clear when IDENTIFY is issued. Set ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE for IDENTIFY such that whole TF is cleared when reading ID data.

Kudos to Art Haas for testing various futile patches over several months and Mark Lord for pointing out the fix.

Signed-off-by Tejun Heo <[email protected]>

Cc Art Haas <[email protected]>

Cc Mark Lord <[email protected]>

---
I think this should go into -stable but a little bit hesitant because the code hasn't been tested widely. This patch should really be harmless but who knows. Jeff, Mark, what do you think?

drivers/ata/libata-core.c +0 -0

index 667acd2..d6fcf0a 100644
@@ -1478,7 +1478,16 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,

	}

	tf.protocol = ATA_PROT_PIO;

- tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
+
+ /* Some devices choke if TF registers contain garbage. Make
+ * sure those are properly initialized.
+ */
+ tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+
+ /* Device presence detection is unreliable on some
+ * controllers. Always poll IDENTIFY if available.
+ */
+ tf.flags |= ATA_TFLAG_POLLING;

	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
				     id, sizeof(id[0]) * ATA_ID_WORDS);

Mark Lord

2007-02-08 14:56 UTC #22 lore raw

Re: [PATCH] libata: clear TF before IDENTIFYing

Quoted 10 lines · Tejun Heo

Some devices chock if Feature is not clear when IDENTIFY is issued. Set ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE for IDENTIFY such that whole TF is cleared when reading ID data.
Kudos to Art Haas for testing various futile patches over several months and Mark Lord for pointing out the fix.
Signed-off-by Tejun Heo <[email protected]>
Cc Art Haas <[email protected]>
Cc Mark Lord <[email protected]>

Signed-off-by Mark Lord <[email protected]>

Quoted 3 lines

I think this should go into -stable but a little bit hesitant because the code hasn't been tested widely. This patch should really be harmless but who knows. Jeff, Mark, what do you think?

If it was up to me, I'd plunk it directly into 2.6.20.1 and earlier. But Jeff has his own ways..

Quoted 27 lines

drivers/ata/libata-core.c +10 -1

index 667acd2..d6fcf0a 100644
@@ -1478,7 +1478,16 @@ int ata_dev_read_id(struct ata_device *dev, 
 unsigned int *p_class,
    }
 
    tf.protocol = ATA_PROT_PIO;
-    tf.flags |= ATA_TFLAG_POLLING; /* for polling presence detection */
+
+    /* Some devices choke if TF registers contain garbage.  Make
+     * sure those are properly initialized.
+     */
+    tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+
+    /* Device presence detection is unreliable on some
+     * controllers.  Always poll IDENTIFY if available.
+     */
+    tf.flags |= ATA_TFLAG_POLLING;
 
    err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,

                     id, sizeof(id[0]) * ATA_ID_WORDS);

-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Art Haas

2007-02-13 19:38 UTC #23 lore raw

Re: [PATCH] libata: clear TF before IDENTIFYing

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

Some devices chock if Feature is not clear when IDENTIFY is issued. Set ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE for IDENTIFY such that whole TF is cleared when reading ID data.
Kudos to Art Haas for testing various futile patches over several months and Mark Lord for pointing out the fix.
Signed-off-by Tejun Heo <[email protected]>
Cc Art Haas <[email protected]>
Cc Mark Lord <[email protected]>

Quoted 6 lines

---
I think this should go into -stable but a little bit hesitant because the code hasn't been tested widely. This patch should really be harmless but who knows. Jeff, Mark, what do you think?
[ ... snip patch ... ]

Hi.

I applied this patch for a kernel build on another PIIX based machine around here, plus the 'ata_piix.c' patch that was tested earlier, and the patched kernel booted without problems. I'm guessing that between the time the patch was initially posted and now other people have tried it out, but I can confirm the patches did not cause problems on the machine (SMP PIII box).

A little more testing info for those wanting some results. The kernel, btw, was Linus' git tree from this morning.

Art Haas

Signature · 4 lines

Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Jeff Garzik

Applied

2007-02-15 23:09 UTC #24 lore raw

Re: [PATCH] libata: clear TF before IDENTIFYing

In reply to Tejun Heo

Quoted 7 lines · Tejun Heo

Some devices chock if Feature is not clear when IDENTIFY is issued. Set ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE for IDENTIFY such that whole TF is cleared when reading ID data.
Kudos to Art Haas for testing various futile patches over several months and Mark Lord for pointing out the fix.

Quoted 7 lines

Signed-off-by Tejun Heo <[email protected]>
Cc Art Haas <[email protected]>
Cc Mark Lord <[email protected]>
---
I think this should go into -stable but a little bit hesitant because the code hasn't been tested widely. This patch should really be harmless but who knows. Jeff, Mark, what do you think?

applied

Art Haas

2007-04-30 18:29 UTC #25 lore raw

PATCH 1/1 Fix pio/mwdma programming on ata_piix.c

In reply to Tejun Heo

Hi.

Back in the beginning of February I was sent a patch to test during the time the CD-ROM identification bug was affecting my computer. The first posting of the patch appeared on Feb. 3 with Tejun Heo writing:

Quoted 9 lines

Hello, Art Haas, Alan.
Okay, here's another try at fixing the detection bug. I went through intel ich docs and compared with the ide piix driver. This patch fixes the following problems.
* Control bits in the timing register wasn't cleared properly while programming PIO mode.

Quoted 7 lines

* MWDMA mode programming cleared the wrong part of control bits. I think this can fix your problem.
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.
Please test and report the result. Thanks.

The patch and complete thread can be reviewed here:

http://marc.info/?l=linux-ide&m=117042956705812&w=2

Now that 2.6.22 is open for big modifications, and the queued libata changes have been pulled by Linus, I'm wondering if the ata_piix.c patch posted above should be sent. I've built all my kernels since the posting with the patch and had no problems. As you write above, the patch does fix some bugs, and as the thread progressed a cleanup or two in the code was explained as a bug fix - the setting of the 'slave_data' variable between lines 826 and 834 in particular.

Here's the patch against today's Linus tree if you feel it should make it into 2.6.22.

Art Haas

drivers/ata/ata_piix.c +17 -6

index 55d306a..26c8b5f 100644
@@ -695,8 +695,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;
 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */

@@ -704,12 +710,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);

@@ -826,7 +834,7 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);

@@ -838,8 +846,11 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}
 	}
 	/* Don't scribble on 0x48 if the controller does not support UDMA */
 	if (ap->udma_mask)

Signature · 4 lines

Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Tejun Heo

author

2007-05-01 03:03 UTC #26 lore raw

Re: [PATCH] Fix pio/mwdma programming on ata_piix.c

Hello, Art Haas.

Quoted 5 lines · Art Haas

Hi.
Back in the beginning of February I was sent a patch to test during the time the CD-ROM identification bug was affecting my computer. The first posting of the patch appeared on Feb. 3 with Tejun Heo writing:

Ah.. I completely forgot about this one. This wasn't directly related to the detection bug, so I got oblivious as usual. :-)

I think Alan has reviewed it back then. If there's no objection, I'll reformat the patch and submit it properly. Quoting whole message for Jeff.

Quoted 95 lines

Quoted 9 lines
Hello, Art Haas, Alan.
Okay, here's another try at fixing the detection bug. I went through intel ich docs and compared with the ide piix driver. This patch fixes the following problems.
* Control bits in the timing register wasn't cleared properly while programming PIO mode.
Quoted 7 lines
* MWDMA mode programming cleared the wrong part of control bits. I think this can fix your problem.
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.
Please test and report the result. Thanks.
The patch and complete thread can be reviewed here:
http://marc.info/?l=linux-ide&m=117042956705812&w=2
Now that 2.6.22 is open for big modifications, and the queued libata changes have been pulled by Linus, I'm wondering if the ata_piix.c patch posted above should be sent. I've built all my kernels since the posting with the patch and had no problems. As you write above, the patch does fix some bugs, and as the thread progressed a cleanup or two in the code was explained as a bug fix - the setting of the 'slave_data' variable between lines 826 and 834 in particular.
Here's the patch against today's Linus tree if you feel it should make it into 2.6.22.
Art Haas
drivers/ata/ata_piix.c +17 -6
index 55d306a..26c8b5f 100644
@@ -695,8 +695,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;
 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */
@@ -704,12 +710,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);
@@ -826,7 +834,7 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);
@@ -838,8 +846,11 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}
 	}
 	/* Don't scribble on 0x48 if the controller does not support UDMA */
 	if (ap->udma_mask)

Signature · 1 line

tejun

Art Haas

2007-05-24 20:00 UTC #27 lore raw

Re: [PATCH] Fix pio/mwdma programming on ata_piix.c

Quoted 9 lines · Tejun Heo

Hello, Art Haas.
Quoted 5 lines · Art Haas
Hi.
Back in the beginning of February I was sent a patch to test during the time the CD-ROM identification bug was affecting my computer. The first posting of the patch appeared on Feb. 3 with Tejun Heo writing:

Quoted 38 lines

Ah.. I completely forgot about this one. This wasn't directly related to the detection bug, so I got oblivious as usual. :-)
I think Alan has reviewed it back then. If there's no objection, I'll reformat the patch and submit it properly. Quoting whole message for Jeff.
Quoted 32 lines
Quoted 9 lines
Hello, Art Haas, Alan.
Okay, here's another try at fixing the detection bug. I went through intel ich docs and compared with the ide piix driver. This patch fixes the following problems.
* Control bits in the timing register wasn't cleared properly while programming PIO mode.
Quoted 7 lines
* MWDMA mode programming cleared the wrong part of control bits. I think this can fix your problem.
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.
Please test and report the result. Thanks.
The patch and complete thread can be reviewed here:
http://marc.info/?l=linux-ide&m=117042956705812&w=2
Now that 2.6.22 is open for big modifications, and the queued libata changes have been pulled by Linus, I'm wondering if the ata_piix.c patch posted above should be sent. I've built all my kernels since the posting with the patch and had no problems. As you write above, the patch does fix some bugs, and as the thread progressed a cleanup or two in the code was explained as a bug fix - the setting of the 'slave_data' variable between lines 826 and 834 in particular.
Here's the patch against today's Linus tree if you feel it should make it into 2.6.22.
[ ... snip ... ]

Ping?

Here's the patch diffed to this afternoon's tree.

Art Haas

drivers/ata/ata_piix.c +17 -6

index 0458811..51de738 100644
@@ -684,8 +684,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;
 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */

@@ -693,12 +699,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);

@@ -815,7 +823,7 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);

@@ -827,8 +835,11 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}
 	}
 	/* Don't scribble on 0x48 if the controller does not support UDMA */
 	if (ap->udma_mask)

Signature · 4 lines

Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Jeff Garzik

2007-05-24 20:55 UTC #28 lore raw

Re: [PATCH] Fix pio/mwdma programming on ata_piix.c

Quoted 104 lines · Art Haas

Quoted 5 lines · Tejun Heo
Hello, Art Haas.
Quoted 2 lines · Art Haas
Hi.
Quoted 40 lines
Quoted 3 lines
Back in the beginning of February I was sent a patch to test during the time the CD-ROM identification bug was affecting my computer. The first posting of the patch appeared on Feb. 3 with Tejun Heo writing:
Ah.. I completely forgot about this one. This wasn't directly related to the detection bug, so I got oblivious as usual. :-)
I think Alan has reviewed it back then. If there's no objection, I'll reformat the patch and submit it properly. Quoting whole message for Jeff.
Quoted 31 lines
Quoted 9 lines
Hello, Art Haas, Alan.
Okay, here's another try at fixing the detection bug. I went through intel ich docs and compared with the ide piix driver. This patch fixes the following problems.
* Control bits in the timing register wasn't cleared properly while programming PIO mode.
Quoted 7 lines
* MWDMA mode programming cleared the wrong part of control bits. I think this can fix your problem.
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA. This doesn't matter for your case.
Please test and report the result. Thanks.
The patch and complete thread can be reviewed here:
http://marc.info/?l=linux-ide&m=117042956705812&w=2
Now that 2.6.22 is open for big modifications, and the queued libata changes have been pulled by Linus, I'm wondering if the ata_piix.c patch posted above should be sent. I've built all my kernels since the posting with the patch and had no problems. As you write above, the patch does fix some bugs, and as the thread progressed a cleanup or two in the code was explained as a bug fix - the setting of the 'slave_data' variable between lines 826 and 834 in particular.
Here's the patch against today's Linus tree if you feel it should make it into 2.6.22.
[ ... snip ... ]
Ping?
Here's the patch diffed to this afternoon's tree.
Art Haas
drivers/ata/ata_piix.c +12 -4
index 0458811..51de738 100644
@@ -684,8 +684,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;
 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */
@@ -693,12 +699,14 @@ static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);
@@ -815,7 +823,7 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);
@@ -827,8 +835,11 @@ static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}

drivers/ata/ata_piix.c static void do_pata_set_dmamode (struct ata_port *ap, struct ata_device *adev, i · line 845

Quoted 10 lines

drivers/ata/ata_piix.c +5 -2

-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}
 	}
 	/* Don't scribble on 0x48 if the controller does not support UDMA */
 	if (ap->udma_mask)

Please resend with proper sign-offs. All patches in the kernel need that... http://linux.yyz.us/patch-format.html

Thanks,

Jeff

Tejun Heo

author

2007-05-24 21:04 UTC #29 lore raw

Re: [PATCH] Fix pio/mwdma programming on ata_piix.c

Quoted 2 lines · Jeff Garzik

Please resend with proper sign-offs. All patches in the kernel need that... http://linux.yyz.us/patch-format.html

I was supposed to do that and Art reminded me once but I managed to forget again, duh..... I'll do it first thing tomorrow. Sorry, Art.

Signature · 1 line

tejun

Tejun Heo

author

2007-05-25 17:17 UTC #30 lore raw

PATCH 1/1 ata_piix: fix pio/mwdma programming

Fix various bugs in pio/mwdma mode programming.

* Control bits in the timing register wasn't cleared properly while programming PIO mode.

* MWDMA mode programming cleared the wrong part of control bits.

* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA.

Signed-off-by Tejun Heo <[email protected]>

Cc Art Haas <[email protected]>

---
Patch is against libata-dev#upstream. Thanks.

 drivers/ata/ata_piix.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

drivers/ata/ata_piix.c +17 -6

index 9c07b88..924e447 100644
@@ -685,8 +685,14 @@ static void piix_set_piomode (struct ata
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;
 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */

@@ -694,12 +700,14 @@ static void piix_set_piomode (struct ata
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);

@@ -816,7 +824,7 @@ static void do_pata_set_dmamode (struct
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);

@@ -828,8 +836,11 @@ static void do_pata_set_dmamode (struct
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}
 	}
 	/* Don't scribble on 0x48 if the controller does not support UDMA */
 	if (ap->udma_mask)

Alan Cox

Acked-by on v1 1/1

2007-05-25 18:00 UTC #31 lore raw

Re: [PATCH] ata_piix: fix pio/mwdma programming

Quoted 5 lines · On Fri, 25 May 2007 19:16:58 +0200 Tejun Heo <[email protected]> wrote:

Fix various bugs in pio/mwdma mode programming.
* Control bits in the timing register wasn't cleared properly while programming PIO mode.

Quoted 7 lines

* MWDMA mode programming cleared the wrong part of control bits.
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA.
Signed-off-by Tejun Heo <[email protected]>
Cc Art Haas <[email protected]>

Acked-by Alan Cox <[email protected]>

Quoted 66 lines

---
Patch is against libata-dev#upstream. Thanks.

 drivers/ata/ata_piix.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

drivers/ata/ata_piix.c +17 -6

index 9c07b88..924e447 100644
@@ -685,8 +685,14 @@ static void piix_set_piomode (struct ata
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
 
+	/* PIO configuration clears DTE unconditionally.  It will be
+	 * programmed in set_dmamode which is guaranteed to be called
+	 * after set_piomode if any DMA mode is available.
+	 */
 	pci_read_config_word(dev, master_port, &master_data);
 	if (is_slave) {
+		/* clear TIME1|IE1|PPE1|DTE1 */
+		master_data &= 0xff0f;
 		/* Enable SITRE (seperate slave timing register) */
 		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */

@@ -694,12 +700,14 @@ static void piix_set_piomode (struct ata
 		pci_read_config_byte(dev, slave_port, &slave_data);
 		slave_data &= (ap->port_no ? 0x0f : 0xf0);
 		/* Load the timing nibble for this slave */
-		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
+		slave_data |= ((timings[pio][0] << 2) | timings[pio][1])
+						<< (ap->port_no ? 4 : 0);
 	} else {
-		/* Master keeps the bits in a different format */
-		master_data &= 0xccf8;
+		/* clear ISP|RCT|TIME0|IE0|PPE0|DTE0 */
+		master_data &= 0xccf0;
 		/* Enable PPE, IE and TIME as appropriate */
 		master_data |= control;
+		/* load ISP and RCT */
 		master_data |=
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);

@@ -816,7 +824,7 @@ static void do_pata_set_dmamode (struct
 			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
 			master_data |= control << 4;
 			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (0x0F + 0xE1 * ap->port_no);
+			slave_data &= (ap->port_no ? 0x0f : 0xf0);
 			/* Load the matching timing */
 			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
 			pci_write_config_byte(dev, 0x44, slave_data);

@@ -828,8 +836,11 @@ static void do_pata_set_dmamode (struct
 				(timings[pio][0] << 12) |
 				(timings[pio][1] << 8);
 		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+
+		if (ap->udma_mask) {
+			udma_enable &= ~(1 << devid);
+			pci_write_config_word(dev, master_port, master_data);
+		}
 	}
 	/* Don't scribble on 0x48 if the controller does not support UDMA */
 	if (ap->udma_mask)

Signature · 3 lines

--
Sick of rip off UK rail fares ? Learn how to get far cheaper fares
		http://zeniv.linux.org.uk/~alan/GTR/

Jeff Garzik

Applied

2007-05-28 13:02 UTC #32 lore raw

Re: [PATCH] ata_piix: fix pio/mwdma programming

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

Fix various bugs in pio/mwdma mode programming.
* Control bits in the timing register wasn't cleared properly while programming PIO mode.
* MWDMA mode programming cleared the wrong part of control bits.
* MWDMA mode programming cleared udma_mask even when the controller doesn't support UDMA.

Quoted 7 lines

Signed-off-by Tejun Heo <[email protected]>
Cc Art Haas <[email protected]>
---
Patch is against libata-dev#upstream. Thanks.
 drivers/ata/ata_piix.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

applied to #upstream