Skip to contentSkip to conversation

pata_hpt366: fix mode configuration

RFC PATCH In review Tejun Heo 4 participants 6 messages Dec 8, 2008 – Dec 10, 2008 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

RFC PATCH 1/1 pata_hpt366: fix mode configuration

There have been reports of pata_hpt366 locking up the whole machine and as Mark kindly sent me his hpt366 about a year ago, I played with it a bit today.

The controller worked perfectly fine with the IDE driver but with pata_hpt366, PIO works but DMA mode locks up the machine solidly. After comparing the sources and lspci outputs, I found the following differences.

1. The case values for PCI clock speed selection seems wrong. IDE hpt366 uses 9 for 40Mhz and 5 for 25 not the other way around.

2. IDE hpt366 uses different mask values for PIO, MWDMA and UDMA when combining the old and new timing values but libata uses one value for all.

3. IDe hpt366 always turns off 0xC0000000 in the timing value but pata_hpt366 doesn't.

Removing the above three differences from pata_hpt366 makes it work happily, but I don't know anything about this controller so no sign-off yet.

Alan, Mark, does this change look sane to you?

Alan, it definitely seems we got something about the cable detection wrong. libata can't see my 80c cable but IDE can.

Thanks.

RFC PATCH. PLEASE DONT APPLY YET.

---
 drivers/ata/pata_hpt366.c |   54 ++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 23 deletions(-)
drivers/ata/pata_hpt366.c +31 -23
index f2b83ea..64b3107 100644
@@ -188,25 +188,41 @@ static unsigned long hpt366_filter(struct ata_device *adev, unsigned long mask)
 }
 
 /**
- *	hpt36x_find_mode	-	reset the hpt36x bus
+ *	hpt36x_calc_timing	-	calculate timing value for transfer mode
  *	@ap: ATA port
- *	@speed: transfer mode
+ *	@cur_timing: current timing value
+ *	@speed: target transfer mode
  *
  *	Return the 32bit register programming information for this channel
  *	that matches the speed provided.
  */
 
-static u32 hpt36x_find_mode(struct ata_port *ap, int speed)
+static u32 hpt36x_calc_timing(struct ata_port *ap, u32 cur_timing, int speed)
 {
 	struct hpt_clock *clocks = ap->host->private_data;
+	u32 mask;
 
-	while(clocks->xfer_speed) {
+	if (speed < XFER_MW_DMA_0)
+		mask = 0xc1f8ffff;
+	else if (speed < XFER_UDMA_0)
+		mask = 0x303800ff;
+	else
+		mask = 0x30070000;
+
+	while (clocks->xfer_speed) {
 		if (clocks->xfer_speed == speed)
-			return clocks->timing;
+			break;
 		clocks++;
 	}
-	BUG();
-	return 0xffffffffU;	/* silence compiler warning */
+	if (!clocks->xfer_speed)
+		BUG();
+
+	/*
+	 * Combine new mode bits with old config bits and disable
+	 * on-chip PIO FIFO/buffer (and PIO MST mode as well) to avoid
+	 * problems handling I/O errors later.
+	 */
+	return ((cur_timing & ~mask) | (clocks->timing & mask)) & ~0xc0000000;
 }
 
 static int hpt36x_cable_detect(struct ata_port *ap)

@@ -232,8 +248,7 @@ static void hpt366_set_piomode(struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
 	u32 addr1, addr2;
-	u32 reg;
-	u32 mode;
+	u32 reg, timing;
 	u8 fast;
 
 	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);

@@ -247,11 +262,8 @@ static void hpt366_set_piomode(struct ata_port *ap, struct ata_device *adev)
 	}
 
 	pci_read_config_dword(pdev, addr1, &reg);
-	mode = hpt36x_find_mode(ap, adev->pio_mode);
-	mode &= ~0x8000000;	/* No FIFO in PIO */
-	mode &= ~0x30070000;	/* Leave config bits alone */
-	reg &= 0x30070000;	/* Strip timing bits */
-	pci_write_config_dword(pdev, addr1, reg | mode);
+	timing = hpt36x_calc_timing(ap, reg, adev->pio_mode);
+	pci_write_config_dword(pdev, addr1, timing);
 }
 
 /**

@@ -267,8 +279,7 @@ static void hpt366_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
 	u32 addr1, addr2;
-	u32 reg;
-	u32 mode;
+	u32 reg, timing;
 	u8 fast;
 
 	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);

@@ -282,11 +293,8 @@ static void hpt366_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 	}
 
 	pci_read_config_dword(pdev, addr1, &reg);
-	mode = hpt36x_find_mode(ap, adev->dma_mode);
-	mode |= 0x8000000;	/* FIFO in MWDMA or UDMA */
-	mode &= ~0xC0000000;	/* Leave config bits alone */
-	reg &= 0xC0000000;	/* Strip timing bits */
-	pci_write_config_dword(pdev, addr1, reg | mode);
+	timing = hpt36x_calc_timing(ap, reg, adev->dma_mode);
+	pci_write_config_dword(pdev, addr1, timing);
 }
 
 static struct scsi_host_template hpt36x_sht = {

@@ -382,10 +390,10 @@ static int hpt36x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 	/* PCI clocking determines the ATA timing values to use */
 	/* info_hpt366 is safe against re-entry so we can scribble on it */
 	switch((reg1 & 0x700) >> 8) {
-		case 5:
+		case 9:
 			hpriv = &hpt366_40;
 			break;
-		case 9:
+		case 5:
 			hpriv = &hpt366_25;
 			break;
 		default:
Acked-by on v1 1/1
#1 lore raw

Hello.

Quoted 5 lines · Tejun Heo

There have been reports of pata_hpt366 locking up the whole machine and as Mark kindly sent me his hpt366 about a year ago, I played with it a bit today.

Quoted 8 lines

The controller worked perfectly fine with the IDE driver but with pata_hpt366, PIO works but DMA mode locks up the machine solidly. After comparing the sources and lspci outputs, I found the following differences.

1. The case values for PCI clock speed selection seems wrong. IDE hpt366 uses 9 for 40Mhz and 5 for 25 not the other way around.

Yes, but that only concerns original HPT36x and should hardly matter
as the BIOS seems to setup the timing registers only for 33 MHz PCI, and these 2 shouldn't even be eached.

Quoted 4 lines

2. IDE hpt366 uses different mask values for PIO, MWDMA and UDMA when combining the old and new timing values but libata uses one value for all.

Yes, I've fixed that stupidity. UltraDMA modes shouldn't force PIO4
timings. However, the PIO and DMA masks were different from the start (though incorrect for HPT37x, IIRC)...

Quoted 3 lines

3. IDe hpt366 always turns off 0xC0000000 in the timing value but pata_hpt366 doesn't.

Hm, that's strange... PIO_MST *must* be cleared and it's set in the
timing tables.

Quoted 4 lines

Removing the above three differences from pata_hpt366 makes it work happily, but I don't know anything about this controller so no sign-off yet.

Quoted 9 lines

Alan, Mark, does this change look sane to you?

Alan, it definitely seems we got something about the cable detection wrong. libata can't see my 80c cable but IDE can.

Thanks.

RFC PATCH. PLEASE DONT APPLY YET.

Acked-by Sergei Shtylyov <[email protected]>
Quoted 30 lines
drivers/ata/pata_hpt366.c +9 -4
index f2b83ea..64b3107 100644
@@ -188,25 +188,41 @@ static unsigned long hpt366_filter(struct ata_device *adev, unsigned long mask)
 }
 
 /**
- *	hpt36x_find_mode	-	reset the hpt36x bus
+ *	hpt36x_calc_timing	-	calculate timing value for transfer mode
  *	@ap: ATA port
- *	@speed: transfer mode
+ *	@cur_timing: current timing value
+ *	@speed: target transfer mode
  *
  *	Return the 32bit register programming information for this channel
  *	that matches the speed provided.
  */
 
-static u32 hpt36x_find_mode(struct ata_port *ap, int speed)
+static u32 hpt36x_calc_timing(struct ata_port *ap, u32 cur_timing, int speed)
 {
 	struct hpt_clock *clocks = ap->host->private_data;
+	u32 mask;
 
-	while(clocks->xfer_speed) {
+	if (speed < XFER_MW_DMA_0)
+		mask = 0xc1f8ffff;
+	else if (speed < XFER_UDMA_0)
+		mask = 0x303800ff;

drivers/ata/pata_hpt366.c static unsigned long hpt366_filter(struct ata_device *adev, unsigned long mask) · line 217

Quoted 10 lines
drivers/ata/pata_hpt366.c +5 -1
+	else
+		mask = 0x30070000;
+
+	while (clocks->xfer_speed) {
 		if (clocks->xfer_speed == speed)
-			return clocks->timing;
+			break;
 		clocks++;
 	}

Should really have been a *for* loop...

drivers/ata/pata_hpt366.c static void hpt366_set_piomode(struct ata_port *ap, struct ata_device *adev) · line 265

Quoted 8 lines
drivers/ata/pata_hpt366.c +0 -3
@@ -247,11 +262,8 @@ static void hpt366_set_piomode(struct ata_port *ap, struct ata_device *adev)
 	}
 
 	pci_read_config_dword(pdev, addr1, &reg);
-	mode = hpt36x_find_mode(ap, adev->pio_mode);
-	mode &= ~0x8000000;	/* No FIFO in PIO */
-	mode &= ~0x30070000;	/* Leave config bits alone */

These are DMA enable bits and UDMA timings...

Quoted 11 lines
drivers/ata/pata_hpt366.c +1 -2
 @ -267,8 +279,7 @@ static void hpt366_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
 	u32 addr1, addr2;
-	u32 reg;
-	u32 mode;
+	u32 reg, timing;
 	u8 fast;
 
 	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);

drivers/ata/pata_hpt366.c static void hpt366_set_dmamode(struct ata_port *ap, struct ata_device *adev) · line 296

Quoted 6 lines
drivers/ata/pata_hpt366.c +0 -2
@@ -282,11 +293,8 @@ static void hpt366_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 	}
 
 	pci_read_config_dword(pdev, addr1, &reg);
-	mode = hpt36x_find_mode(ap, adev->dma_mode);
-	mode |= 0x8000000;	/* FIFO in MWDMA or UDMA */

FIFO bit doesn't affect DMA modes.

drivers/ata/pata_hpt366.c static void hpt366_set_dmamode(struct ata_port *ap, struct ata_device *adev) · line 299

Quoted 2 lines
drivers/ata/pata_hpt366.c +0 -1
-	mode &= ~0xC0000000;	/* Leave config bits alone */

Oh, it's cleared right afterwards...
Looks like with you change the set_dmamode() and set_piomode()
methods are becoming sompletely similar, so it's probably worth to merge them...

MBR, Sergei

#2 lore raw
Quoted 8 lines

There have been reports of pata_hpt366 locking up the whole machine and as Mark kindly sent me his hpt366 about a year ago, I played with it a bit today.

The controller worked perfectly fine with the IDE driver but with pata_hpt366, PIO works but DMA mode locks up the machine solidly. After comparing the sources and lspci outputs, I found the following differences.

Tejun's patch applied to OpenSUSE 11.1-RC1 kernel solves non-booting issues, and has given good performance (which have been regular since 2.6.19 introduced PATA experimental drivers have been shipped by distros).

I can't comment on cable speed details, as I haven't seen such messages.

If parts of it are dubious, please can we start from a minimal changeset which delivers a working driver, based on these mods, and
then move forwards to beautified enhanced code. Though I do wonder
how sensible it is, to spend a lot of time on elegant driver development for this legacy hardware, as it must be nearing practical end of life.

The hang existed in 2.6.22 and 2.6.23 code I tried, then was fixed in updates which made there way into the OpenSUSE distro kernel. So end users have been using pata_hpt366 for quite a while, thus the patch fixes a regression for them.

#3 lore raw
Quoted 4 lines · Rob OpenSuSE

Though I do wonder
how sensible it is, to spend a lot of time on elegant driver development for this legacy hardware, as it must be nearing practical end of life.

..

There are tons of PCI add-on cards still out there with these chips, and they will continue to work fine so long as PCI slots
and PATA drives exist. So we may as well ensure Linux does the Right Things (tm).

:)

Nice work, Tejun!

#4 lore raw

2008/12/9 Mark Lord <[email protected]>:

Quoted 4 lines

Rob OpenSuSE wrote:
There are tons of PCI add-on cards still out there with these chips, and they will continue to work fine so long as PCI slots
and PATA drives exist. So we may as well ensure Linux does the Right Things (tm).

Yes, my point was in support of pragmatism; so that new kernel builds and distro installs can have a chance to succeed in boxes with these cards used, without opening case, reconfiguring, and finding special boot paramters. Those cards need working code now, not next year. So "The Right Thing (tm)" would be bad, if it caused perfectly capable kit to be non-functioning, whilst perfection is under discussion and development.

Quoted 1 line

Nice work, Tejun!

Seconded! If you are the Mark who sent the card, thanks to you to from me to.

author
#5 lore raw
Quoted 6 lines · Rob OpenSuSE

2008/12/9 Mark Lord <[email protected]>:

Quoted 4 lines

Rob OpenSuSE wrote:
There are tons of PCI add-on cards still out there with these chips, and they will continue to work fine so long as PCI slots
and PATA drives exist. So we may as well ensure Linux does the Right Things (tm).

Quoted 7 lines

Yes, my point was in support of pragmatism; so that new kernel builds and distro installs can have a chance to succeed in boxes with these cards used, without opening case, reconfiguring, and finding special boot paramters. Those cards need working code now, not next year. So "The Right Thing (tm)" would be bad, if it caused perfectly capable kit to be non-functioning, whilst perfection is under discussion and development.

I already pushed those patches into openSUSE and in one form or another it will make into upstream and other distros, so no worries.

Quoted 4 lines
Quoted 1 line

Nice work, Tejun!

Seconded! If you are the Mark who sent the card, thanks to you to from me to.

Yeap, he's the Mark who sends me old and weird hardwares from time to time. :-)

Thanks.

Signature · 1 line
tejun