[RFC PATCH] pata_hpt366: fix mode configuration

[RFC PATCH] pata_hpt366: fix mode configuration

[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.

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(-)

diff --git a/drivers/ata/pata_hpt366.c b/drivers/ata/pata_hpt366.c
index f2b83ea..64b3107 100644
--- a/drivers/ata/pata_hpt366.c
+++ b/drivers/ata/pata_hpt366.c
@@ -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:

Re: [RFC PATCH] pata_hpt366: fix mode configuration

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

> 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.
>   

   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.

> 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)...

> 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.

> 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.
>   
Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> diff --git a/drivers/ata/pata_hpt366.c b/drivers/ata/pata_hpt366.c
> index f2b83ea..64b3107 100644
> --- a/drivers/ata/pata_hpt366.c
> +++ b/drivers/ata/pata_hpt366.c
> @@ -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++;
>  	}
>   

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

> @@ -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...

> @ -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 */
>   

   FIFO bit doesn't affect DMA modes.

> -	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

Re: [RFC PATCH] pata_hpt366: fix mode configuration

[Rob OpenSuSE]

> 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.

Re: [RFC PATCH] pata_hpt366: fix mode configuration

[Mark Lord]

Rob OpenSuSE wrote:
> 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!

Re: [RFC PATCH] pata_hpt366: fix mode configuration

[Rob OpenSuSE]

2008/12/9 Mark Lord <liml@rtr.ca>:
> 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.

> Nice work, Tejun!

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

Re: [RFC PATCH] pata_hpt366: fix mode configuration

[Tejun Heo]

Rob OpenSuSE wrote:
> 2008/12/9 Mark Lord <liml@rtr.ca>:
>> 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.

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

>> 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.

-- 
tejun