[PATCH] pata_sil680: PIO1 taskfile transfers oveclocking fix

[PATCH] pata_sil680: PIO1 taskfile transfers oveclocking fix

[Sergei Shtylyov]

Fix PIO mode 1 overclocked taskfile transfers -- probably a typo carried over
from drivers/ide/pci/siimage.c where I've found it by documentation check...

 drivers/ata/pata_sil680.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/ata/pata_sil680.c
===================================================================
--- linux-2.6.orig/drivers/ata/pata_sil680.c
+++ linux-2.6/drivers/ata/pata_sil680.c
@@ -135,7 +135,7 @@ static void sil680_error_handler(struct 
 static void sil680_set_piomode(struct ata_port *ap, struct ata_device *adev)
 {
 	static u16 speed_p[5] = { 0x328A, 0x2283, 0x1104, 0x10C3, 0x10C1 };
-	static u16 speed_t[5] = { 0x328A, 0x1281, 0x1281, 0x10C3, 0x10C1 };
+	static u16 speed_t[5] = { 0x328A, 0x2283, 0x1281, 0x10C3, 0x10C1 };
 
 	unsigned long tfaddr = sil680_selreg(ap, 0x02);
 	unsigned long addr = sil680_seldev(ap, adev, 0x04);

[RFC] libata IORDY handling

[Sergei Shtylyov]

Hello.

Sergei Shtylyov wrote:

> Fix PIO mode 1 overclocked taskfile transfers -- probably a typo carried over
> from drivers/ide/pci/siimage.c where I've found it by documentation check...

>  drivers/ata/pata_sil680.c |    2 +-
>  1 files changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/ata/pata_sil680.c
> ===================================================================
> --- linux-2.6.orig/drivers/ata/pata_sil680.c
> +++ linux-2.6/drivers/ata/pata_sil680.c
> @@ -135,7 +135,7 @@ static void sil680_error_handler(struct 
>  static void sil680_set_piomode(struct ata_port *ap, struct ata_device *adev)

    It's sad to say but there's another bug in this function (even a 
regression from drivers/ide/pci/siimage.c) -- the 16-bit IORDY is not enabled 
when setting PIO mode (there's code that twiddles IORDY enable but that's 
actually only for *taskfile* accesses, 16-bit IORDY is controlled by the same 
PCI  config. registers 80h/84h that enable DMA/UDMA transfer on SiI 680).
    I looked into fixing this but had a feeling that the thing wasn't right 
from the very start, including ata_pio_need_iordy().  In my understanding of 
the ANSI T13 stadrads, when one issues Set Features subcommand Set Transfer 
Mode with sector count register of 0x8 thru 0xC this means that IORDY *must* 
be enabled. That's what the ATA/ATAPI-6 says, for example:

         Table 45 - Transfer mode values
Mode                             Bits (7:3) Bits (2:0)
PIO default mode                 00000b     000b
PIO default mode, disable IORDY  00000b     001b
PIO *flow control* transfer mode 00001b     mode

    Comments?

MBR, Sergei

Re: [PATCH] pata_sil680: PIO1 taskfile transfers oveclocking fix

[Alan]

On Sun, 28 Jan 2007 20:24:17 +0300
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> Fix PIO mode 1 overclocked taskfile transfers -- probably a typo carried over
> from drivers/ide/pci/siimage.c where I've found it by documentation check...
> 
>  drivers/ata/pata_sil680.c |    2 +-
>  1 files changed, 1 insertion(+), 1 deletion(-)
> 

Looks right to me , thanks - but does need a signoff

Re: [RFC] libata IORDY handling

[Sergei Shtylyov]

Hello.

Alan wrote:

>>    I looked into fixing this but had a feeling that the thing wasn't right 
>>from the very start, including ata_pio_need_iordy().  In my understanding of 
>>the ANSI T13 stadrads, when one issues Set Features subcommand Set Transfer 
>>Mode with sector count register of 0x8 thru 0xC this means that IORDY *must* 
>>be enabled. 

> Yes. It is more complex than the current code handles. That's one reason
> I added ata_pio_need_iordy(), because it would need to change and

    I also meant this function: if IORDY *must* be enabled even for PIO mode 0 
(being set the way liabata sets it, via the mentioned subcommand), what's the 
point of checking if we need to enable IORDY?  The only reason for this 
function as it seems is to check if we can do *without* IORDY still...
    Even if so, we must check if any of these both drives need IORDY to decide 
whether we need to enable IORDY checking on taskfile accesses or not -- which 
this function fails to do...

> hardcoding it would be particularly ugly.

    Yeah, hardcoding is ugly, no doubt about it. This is still a problem with 
both pdc202xx_new and pata_pdc2027x, for example...

> This will matter for supporting some utterly ancient junk.

    Well, SiI680 seemed to me quite well designed. :-)

> Alan

MBR, Sergei

Re: [RFC] libata IORDY handling

[Alan]

>     I looked into fixing this but had a feeling that the thing wasn't right 
> from the very start, including ata_pio_need_iordy().  In my understanding of 
> the ANSI T13 stadrads, when one issues Set Features subcommand Set Transfer 
> Mode with sector count register of 0x8 thru 0xC this means that IORDY *must* 
> be enabled. 

Yes. It is more complex than the current code handles. That's one reason
I added ata_pio_need_iordy(), because it would need to change and
hardcoding it would be particularly ugly.

This will matter for supporting some utterly ancient junk.

Alan

Re: [RFC] libata IORDY handling

[Alan]

>     It's sad to say but there's another bug in this function (even a 
> regression from drivers/ide/pci/siimage.c) -- the 16-bit IORDY is not enabled 
> when setting PIO mode (there's code that twiddles IORDY enable but that's 
> actually only for *taskfile* accesses, 16-bit IORDY is controlled by the same 
> PCI  config. registers 80h/84h that enable DMA/UDMA transfer on SiI 680).

Fixed - also fixed clearing the bits when going into PIO mode from DMA,
which fixes a potential DMA changedown bug.

Also redone the iordy stuff as your emails reminded me that it needed
finishing off and sorting out.

Alan

Re: [RFC] libata IORDY handling

[Sergei Shtylyov]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

Hello.

Alan wrote:

>>    It's sad to say but there's another bug in this function (even a 
>>regression from drivers/ide/pci/siimage.c) -- the 16-bit IORDY is not enabled 
>>when setting PIO mode (there's code that twiddles IORDY enable but that's 
>>actually only for *taskfile* accesses, 16-bit IORDY is controlled by the same 
>>PCI  config. registers 80h/84h that enable DMA/UDMA transfer on SiI 680).

> Fixed - also fixed clearing the bits when going into PIO mode from DMA,
> which fixes a potential DMA changedown bug.

     Yeah, this is an issue in siimage.c...

> Also redone the iordy stuff as your emails reminded me that it needed
> finishing off and sorting out.

     Atttaching my (already obsolete) patch, just in case...

> Alan

MBR, Sergei


[-- Attachment #2: sil680_iordy-fixes.patch --]
[-- Type: text/x-patch, Size: 1895 bytes --]

Index: linux-2.6/drivers/ata/pata_sil680.c
===================================================================
--- linux-2.6.orig/drivers/ata/pata_sil680.c
+++ linux-2.6/drivers/ata/pata_sil680.c
@@ -139,9 +139,12 @@ static void sil680_set_piomode(struct at
 
 	unsigned long tfaddr = sil680_selreg(ap, 0x02);
 	unsigned long addr = sil680_seldev(ap, adev, 0x04);
+	unsigned long addr_mask = 0x80 + 4 * ap->port_no;
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	int port_shift = adev->devno * 4;
 	int pio = adev->pio_mode - XFER_PIO_0;
 	int lowest_pio = pio;
+	u8  mode;
 	u16 reg;
 
 	struct ata_device *pair = ata_dev_pair(adev);
@@ -152,11 +155,31 @@ static void sil680_set_piomode(struct at
 	pci_write_config_word(pdev, addr, speed_p[pio]);
 	pci_write_config_word(pdev, tfaddr, speed_t[lowest_pio]);
 
-	pci_read_config_word(pdev, tfaddr-2, &reg);
-	reg &= ~0x0200;			/* Clear IORDY */
+	/*
+	 * Let's see if we need to enable IORDY checking on data transfer...
+	 *
+	 * NOTE: Selecting any DMA mode also enables IORDY  checkign,
+	 * so we must first check if not already in DMA mode beforehand
+	 * to avoid disabling it...
+	 */
+	pci_read_config_byte(pdev, addr_mask, &mode);
+	if (ata_pio_need_iordy(adev) && ((mode >> port_shift) & 0x03) == 0x00)
+		mode |= 0x01 << port_shift;
+	else if (((mode >> port_shift) & 0x03) == 0x01)
+		mode &= ~(0x03 << port_shift);
+	pci_write_config_byte(pdev, addr_mask, mode);
+
+	/*
+	 * Let's see if we need to enable IORDY checking on taskfile.
+	 *
+	 * NOTE: We can only disable IORDY if both drives agree to it.
+	 */
+	pci_read_config_word(pdev, tfaddr - 2, &reg);
 	if (ata_pio_need_iordy(adev))
-		reg |= 0x0200;		/* Enable IORDY */
-	pci_write_config_word(pdev, tfaddr-2, reg);
+		reg |=  0x0200;
+	else if (pair == NULL || !ata_pio_need_iordy(pair))
+		reg &= ~0x0200;
+	pci_write_config_word(pdev, tfaddr - 2, reg);
 }
 
 /**

Re: [RFC] libata IORDY handling

[Alan]

>      Atttaching my (already obsolete) patch, just in case...
> 

Thanks. Double checking my code it looks similar although I don't bother
with the DMA check as the drivers know that in DMA cases the PIO call will
be made before the DMA mode is set when DMA modes are in use.