Skip to contentSkip to conversation

pata_hpt366: fix cable detection,

PATCH In review Tejun Heo 4 participants 12 messages Dec 9, 2008 – Dec 16, 2008 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/3 pata_hpt366: fix cable detection,

pata_hpt366 is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register 0x5A indicates cable for both channels.

Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>
---
 drivers/ata/pata_hpt366.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/ata/pata_hpt366.c +6 -2
Index: work/drivers/ata/pata_hpt366.c
===================================================================
@@ -211,11 +211,15 @@ static u32 hpt36x_find_mode(struct ata_p
 
 static int hpt36x_cable_detect(struct ata_port *ap)
 {
-	u8 ata66;
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	u8 ata66;
 
+	/*
+	 * Each channel of pata_hpt366 occupies separate PCI function
+	 * as the primary channel and bit1 indicates the cable type.
+	 */
 	pci_read_config_byte(pdev, 0x5A, &ata66);
-	if (ata66 & (1 << ap->port_no))
+	if (ata66 & 2)
 		return ATA_CBL_PATA40;
 	return ATA_CBL_PATA80;
 }
author
#1 lore raw

PATCH 2/3 pata_hpt366: no ATAPI DMA

IDE hpt366 driver doesn't allow DMA for ATAPI devices and MWDMA2 on ATAPI device locks up pata_hpt366. Follow the suit.

---
 drivers/ata/pata_hpt366.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/ata/pata_hpt366.c +3 -1
Index: work/drivers/ata/pata_hpt366.c
===================================================================
@@ -183,7 +183,9 @@ static unsigned long hpt366_filter(struc
 			mask &= ~(0xF8 << ATA_SHIFT_UDMA);
 		if (hpt_dma_blacklisted(adev, "UDMA4", bad_ata66_4))
 			mask &= ~(0xF0 << ATA_SHIFT_UDMA);
-	}
+	} else if (adev->class == ATA_DEV_ATAPI)
+		mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
+
 	return ata_bmdma_mode_filter(adev, mask);
 }
 
author
#2 lore raw

PATCH 3/3 pata_hpt366: reimplement mode programming

Reimplement mode programming logic of pata_hpt366 such that it's identical to that of IDE hpt366 driver. The differences were...

* pata_hpt366 used 0xCFFF8FFFF to mask pio modes and 0x3FFFFFFF dma modes. IDE hpt366 uses 0xC1F8FFFF for PIO, 0x303800FF for MWDMA and 0x30070000 for UDMA.

* pata_hpt366 doesn't set 0x08000000 for PIO unless it's already set and always turns it on for MWDMA/UDMA. IDE hpt366 doesn't bother with the bit. It always uses what was there.

* IDE hpt366 always clears 0xC0000000. pata_hpt366 doesn't.

Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>

---
Unified pio/dma paths as per Sergei's suggestion.

 drivers/ata/pata_hpt366.c |  109 ++++++++++++++++++----------------------------
 1 file changed, 44 insertions(+), 65 deletions(-)
drivers/ata/pata_hpt366.c +44 -65
Index: work/drivers/ata/pata_hpt366.c
===================================================================
@@ -30,7 +30,7 @@
 #define DRV_VERSION	"0.6.2"
 
 struct hpt_clock {
-	u8	xfer_speed;
+	u8	xfer_mode;
 	u32	timing;
 };
 
@@ -189,28 +189,6 @@ static unsigned long hpt366_filter(struc
 	return ata_bmdma_mode_filter(adev, mask);
 }
 
-/**
- *	hpt36x_find_mode	-	reset the hpt36x bus
- *	@ap: ATA port
- *	@speed: 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)
-{
-	struct hpt_clock *clocks = ap->host->private_data;
-
-	while(clocks->xfer_speed) {
-		if (clocks->xfer_speed == speed)
-			return clocks->timing;
-		clocks++;
-	}
-	BUG();
-	return 0xffffffffU;	/* silence compiler warning */
-}
-
 static int hpt36x_cable_detect(struct ata_port *ap)
 {
 	u8 ata66;
@@ -222,25 +200,16 @@ static int hpt36x_cable_detect(struct at
 	return ATA_CBL_PATA80;
 }
 
-/**
- *	hpt366_set_piomode		-	PIO setup
- *	@ap: ATA interface
- *	@adev: device on the interface
- *
- *	Perform PIO mode setup.
- */
-
-static void hpt366_set_piomode(struct ata_port *ap, struct ata_device *adev)
+static void hpt366_set_mode(struct ata_port *ap, struct ata_device *adev,
+			    u8 mode)
 {
+	struct hpt_clock *clocks = ap->host->private_data;
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
-	u32 addr1, addr2;
-	u32 reg;
-	u32 mode;
+	u32 addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
+	u32 addr2 = 0x51 + 4 * ap->port_no;
+	u32 mask, reg;
 	u8 fast;
 
-	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
-	addr2 = 0x51 + 4 * ap->port_no;
-
 	/* Fast interrupt prediction disable, hold off interrupt disable */
 	pci_read_config_byte(pdev, addr2, &fast);
 	if (fast & 0x80) {
@@ -248,12 +217,43 @@ static void hpt366_set_piomode(struct at
 		pci_write_config_byte(pdev, addr2, fast);
 	}
 
+	/* determine timing mask and find matching clock entry */
+	if (mode < XFER_MW_DMA_0)
+		mask = 0xc1f8ffff;
+	else if (mode < XFER_UDMA_0)
+		mask = 0x303800ff;
+	else
+		mask = 0x30070000;
+
+	while (clocks->xfer_mode) {
+		if (clocks->xfer_mode == mode)
+			break;
+		clocks++;
+	}
+	if (!clocks->xfer_mode)
+		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.
+	 */
 	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);
+	reg = ((reg & ~mask) | (clocks->timing & mask)) & ~0xc0000000;
+	pci_write_config_dword(pdev, addr1, reg);
+}
+
+/**
+ *	hpt366_set_piomode		-	PIO setup
+ *	@ap: ATA interface
+ *	@adev: device on the interface
+ *
+ *	Perform PIO mode setup.
+ */
+
+static void hpt366_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	hpt366_set_mode(ap, adev, adev->pio_mode);
 }
 
 /**
@@ -267,28 +267,7 @@ static void hpt366_set_piomode(struct at
 
 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;
-	u8 fast;
-
-	addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
-	addr2 = 0x51 + 4 * ap->port_no;
-
-	/* Fast interrupt prediction disable, hold off interrupt disable */
-	pci_read_config_byte(pdev, addr2, &fast);
-	if (fast & 0x80) {
-		fast &= ~0x80;
-		pci_write_config_byte(pdev, addr2, fast);
-	}
-
-	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);
+	hpt366_set_mode(ap, adev, adev->dma_mode);
 }
 
 static struct scsi_host_template hpt36x_sht = {
Acked-by on v1 3/3
#3 lore raw

Re: [PATCH #upstream-fixes 3/3] pata_hpt366: reimplement mode programming

Hello.

Quoted 7 lines · Tejun Heo

Reimplement mode programming logic of pata_hpt366 such that it's identical to that of IDE hpt366 driver. The differences were...

* pata_hpt366 used 0xCFFF8FFFF to mask pio modes and 0x3FFFFFFF dma modes. IDE hpt366 uses 0xC1F8FFFF for PIO, 0x303800FF for MWDMA and 0x30070000 for UDMA.

Yeah, so it changes only the applicable timing bits for each mode.

Quoted 4 lines

* pata_hpt366 doesn't set 0x08000000 for PIO unless it's already set and always turns it on for MWDMA/UDMA. IDE hpt366 doesn't bother with the bit. It always uses what was there.

This bit (unless you meant 0x80000000 :-) is just reserved, along
with 2 neigboring lower order bits.

Quoted 2 lines

* IDE hpt366 always clears 0xC0000000. pata_hpt366 doesn't.

This was indeed a big mistake. The table values have PIO_MST bit set
which enables bus mastering support for PIO modes...

Quoted 4 lines
Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Quoted 4 lines

Index: work/drivers/ata/pata_hpt366.c
===================================================================

drivers/ata/pata_hpt366.c +0 -0

[...]

Quoted 14 lines
drivers/ata/pata_hpt366.c +1 -9
@@ -222,25 +200,16 @@ static int hpt36x_cable_detect(struct at
 	return ATA_CBL_PATA80;
 }
 
-/**
- *	hpt366_set_piomode		-	PIO setup
- *	@ap: ATA interface
- *	@adev: device on the interface
- *
- *	Perform PIO mode setup.
- */
-
-static void hpt366_set_piomode(struct ata_port *ap, struct ata_device *adev)
+static void hpt366_set_mode(struct ata_port *ap, struct ata_device *adev,

drivers/ata/pata_hpt366.c static int hpt36x_cable_detect(struct at · line 210

Quoted 10 lines
drivers/ata/pata_hpt366.c +4 -3
+			    u8 mode)
 {
+	struct hpt_clock *clocks = ap->host->private_data;
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
-	u32 addr1, addr2;
-	u32 reg;
-	u32 mode;
+	u32 addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
+	u32 addr2 = 0x51 + 4 * ap->port_no;
 

Note that ap->port_no can be safely excluded form all the math due to
single channel per funcion.

MBR, Sergei

#4 lore raw

Re: [PATCH #upstream-fixes 2/3] pata_hpt366: no ATAPI DMA

In reply to Tejun Heo
Quoted 13 lines · On Tue, 09 Dec 2008 17:14:04 +0900 Tejun Heo <[email protected]> wrote:

IDE hpt366 driver doesn't allow DMA for ATAPI devices and MWDMA2 on ATAPI device locks up pata_hpt366. Follow the suit.

---
 drivers/ata/pata_hpt366.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: work/drivers/ata/pata_hpt366.c
===================================================================

drivers/ata/pata_hpt366.c static unsigned long hpt366_filter(struc · line 187

Quoted 6 lines
drivers/ata/pata_hpt366.c +2 -1
@@ -183,7 +183,9 @@ static unsigned long hpt366_filter(struc
 			mask &= ~(0xF8 << ATA_SHIFT_UDMA);
 		if (hpt_dma_blacklisted(adev, "UDMA4", bad_ata66_4))
 			mask &= ~(0xF0 << ATA_SHIFT_UDMA);
-	}
+	} else if (adev->class == ATA_DEV_ATAPI)
+		mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);

Ack this one

Alan

author
#5 lore raw
In reply to Tejun Heo
Quoted 7 lines · Tejun Heo

pata_hpt366 is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register 0x5A indicates cable for both channels.

Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>

I tested this patch for all four combinations of 40/80c cables and it always worked correctly.

Thanks.

Signature · 1 line
tejun
#6 lore raw

Hello.

Quoted 10 lines · Tejun Heo
Quoted 7 lines

pata_hpt366 is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register 0x5A indicates cable for both channels.

Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>

I tested this patch for all four combinations of 40/80c cables and it always worked correctly.

So that means that the datasheet is wrong. May well be...

Quoted 1 line

Thanks.

MBR, Sergei

#7 lore raw
In reply to Tejun Heo
Quoted 7 lines · Tejun Heo

pata_hpt366 is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register 0x5A indicates cable for both channels.

Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>

Alas, NAK.

Quoted 6 lines

Index: work/drivers/ata/pata_hpt366.c
===================================================================

drivers/ata/pata_hpt366.c +0 -0
@@ -211,11 +211,15 @@ static u32 hpt36x_find_mode(struct ata_p
 

drivers/ata/pata_hpt366.c static u32 hpt36x_find_mode(struct ata_p · line 220

Quoted 10 lines
drivers/ata/pata_hpt366.c +5 -1
 static int hpt36x_cable_detect(struct ata_port *ap)
 {
-	u8 ata66;
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	u8 ata66;
 
+	/*
+	 * Each channel of pata_hpt366 occupies separate PCI function
+	 * as the primary channel and bit1 indicates the cable type.
+	 */

No, it doesn't for the secondary channel (which is fucntion 1).

drivers/ata/pata_hpt366.c static u32 hpt36x_find_mode(struct ata_p · line 222

Quoted 3 lines
drivers/ata/pata_hpt366.c +1 -1
 	pci_read_config_byte(pdev, 0x5A, &ata66);
-	if (ata66 & (1 << ap->port_no))
+	if (ata66 & 2)

You need to use (pdev->devfn & 1) as a right shift count instead. Not very
sophisticated indeed. :-)

MBR, Sergei

#8 lore raw
Quoted 9 lines · Hello, I wrote:
Quoted 7 lines

pata_hpt366 is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register 0x5A indicates cable for both channels.

Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>

Alas, NAK.

If you say that it does work, then the datasheet appears to be wrong, so
have my:

Quoted 19 lines
Quoted 6 lines

Index: work/drivers/ata/pata_hpt366.c
===================================================================

drivers/ata/pata_hpt366.c +0 -0
@@ -211,11 +211,15 @@ static u32 hpt36x_find_mode(struct ata_p
 

drivers/ata/pata_hpt366.c static u32 hpt36x_find_mode(struct ata_p · line 220

Quoted 10 lines
drivers/ata/pata_hpt366.c +5 -1
 static int hpt36x_cable_detect(struct ata_port *ap)
 {
-    u8 ata66;
     struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+    u8 ata66;
 
+    /*
+     * Each channel of pata_hpt366 occupies separate PCI function
+     * as the primary channel and bit1 indicates the cable type.
+     */

No, it doesn't for the secondary channel (which is fucntion 1).

Quoted 6 lines

drivers/ata/pata_hpt366.c static u32 hpt36x_find_mode(struct ata_p · line 222

Quoted 3 lines
drivers/ata/pata_hpt366.c +1 -1
     pci_read_config_byte(pdev, 0x5A, &ata66);
-    if (ata66 & (1 << ap->port_no))
+    if (ata66 & 2)

You need to use (pdev->devfn & 1) as a right shift count instead. Not
very sophisticated indeed. :-)

Well, then I won't have to fix hpt366.c if that's not needed. :-)

MBR, Sergei

author
#9 lore raw

Hello, Sergei.

Quoted 6 lines · Sergei Shtylyov
Quoted 1 line

Alas, NAK.

If you say that it does work, then the datasheet appears to be wrong,
so have my:

Yeah, AFAICS, it does work. The ata66 register is always 0x3 with 40c cable and 0x1 with 80c regardless of the PCI function.

Thanks.

Signature · 1 line
tejun
#10 lore raw
In reply to Tejun Heo
Quoted 10 lines · Tejun Heo

pata_hpt366 is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register 0x5A indicates cable for both channels.

Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>
---
 drivers/ata/pata_hpt366.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

applied 1-2 of 3 (the last looked risky for -rc?)

author
#11 lore raw
Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Tejun Heo

pata_hpt366 is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register 0x5A indicates cable for both channels.

Quoted 8 lines
Quoted 6 lines
Cc Alan Cox <[email protected]>
Cc Sergei Shtylyov <[email protected]>
---
 drivers/ata/pata_hpt366.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

applied 1-2 of 3 (the last looked risky for -rc?)

I think it would be better to postpone it till the -rc1 window opens. As it has been broken for so long, I don't think many people will complain much about one more broken release. :-)

Signature · 1 line
tejun