pata_hpt366: fix cable detection,
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.
--- drivers/ata/pata_hpt366.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/ata/pata_hpt366.c +6 -2
@@ -211,11 +211,15 @@ static u32 hpt36x_find_mode(struct ata_pstatic 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 ; }
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
@@ -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); }
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.
---
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
@@ -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(strucreturn 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 atreturn 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 atpci_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, ®); - 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 atstatic 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, ®); - 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 = {
Re: [PATCH #upstream-fixes 3/3] pata_hpt366: reimplement mode programming
Hello.
Quoted 7 lines · Tejun Heo
Reimplement mode programming logic of
pata_hpt366such that it's identical to that of IDE hpt366 driver. The differences were...*
pata_hpt366used0xCFFF8FFFFto mask pio modes and0x3FFFFFFFdma modes. IDE hpt366 uses0xC1F8FFFFfor PIO,0x303800FFfor MWDMA and0x30070000for UDMA.
Yeah, so it changes only the applicable timing bits for each mode.
Quoted 4 lines
*
pata_hpt366doesn't set0x08000000for 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_hpt366doesn'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
Signed-off-by Tejun Heo <[email protected]>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 atreturn 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
Re: [PATCH #upstream-fixes 2/3] pata_hpt366: no ATAPI DMA
In reply to Tejun HeoQuoted 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.Signed-off-by Tejun Heo <[email protected]>--- 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
Quoted 7 lines · Tejun Heo
pata_hpt366is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register0x5Aindicates cable for both channels.Signed-off-by Tejun Heo <[email protected]>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
Hello.
Quoted 10 lines · Tejun Heo
Quoted 7 lines
pata_hpt366is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register0x5Aindicates cable for both channels.Signed-off-by Tejun Heo <[email protected]>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
Quoted 7 lines · Tejun Heo
pata_hpt366is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register0x5Aindicates cable for both channels.Signed-off-by Tejun Heo <[email protected]>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
-1static 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
-1pci_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
Sergei Shtylyov
Acked-byQuoted 9 lines · Hello, I wrote:
Quoted 7 lines
pata_hpt366is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register0x5Aindicates cable for both channels.Signed-off-by Tejun Heo <[email protected]>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_pdrivers/ata/pata_hpt366.c static u32 hpt36x_find_mode(struct ata_p · line 220
Quoted 10 lines
drivers/ata/pata_hpt366.c +5
-1static 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
-1pci_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
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:Acked-by Sergei Shtylyov <[email protected]>
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
Quoted 10 lines · Tejun Heo
pata_hpt366is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register0x5Aindicates cable for both channels.Signed-off-by Tejun Heo <[email protected]>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?)
Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Tejun Heo
pata_hpt366is strange in that its two channels occupy two PCI functions and both are primary channels and bit1 of PCI configuration register0x5Aindicates cable for both channels.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>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