[PATCH] pata_cmd64x: Set up MWDMA modes properly

[PATCH] pata_cmd64x: Set up MWDMA modes properly

[Alan Cox]

Set the MWDMA timing by updating the correct registers. Split the PIO
path as this is mostly shared code. Wants testing.

Signed-off-by: Alan Cox <alan@redhat.com>

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c linux-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c
--- linux.vanilla-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c	2007-07-26 15:01:52.000000000 +0100
+++ linux-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c	2007-08-08 11:52:23.000000000 +0100
@@ -31,7 +31,7 @@
 #include <linux/libata.h>
 
 #define DRV_NAME "pata_cmd64x"
-#define DRV_VERSION "0.2.3"
+#define DRV_VERSION "0.2.4"
 
 /*
  * CMD64x specific registers definition.
@@ -88,14 +88,15 @@
 }
 
 /**
- *	cmd64x_set_piomode	-	set initial PIO mode data
+ *	cmd64x_set_piomode	-	set PIO and MWDMA timing
  *	@ap: ATA interface
  *	@adev: ATA device
+ *	@mode: mode
  *
- *	Called to do the PIO mode setup.
+ *	Called to do the PIO and MWDMA mode setup.
  */
 
-static void cmd64x_set_piomode(struct ata_port *ap, struct ata_device *adev)
+static void cmd64x_set_timing(struct ata_port *ap, struct ata_device *adev, u8 mode)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
 	struct ata_timing t;
@@ -117,8 +118,9 @@
 	int arttim = arttim_port[ap->port_no][adev->devno];
 	int drwtim = drwtim_port[ap->port_no][adev->devno];
 
-
-	if (ata_timing_compute(adev, adev->pio_mode, &t, T, 0) < 0) {
+	/* ata_timing_compute is smart and will produce timings for MWDMA
+	   that don't violate the drives PIO capabilities. */
+	if (ata_timing_compute(adev, mode, &t, T, 0) < 0) {
 		printk(KERN_ERR DRV_NAME ": mode computation failed.\n");
 		return;
 	}
@@ -168,6 +170,20 @@
 }
 
 /**
+ *	cmd64x_set_piomode	-	set initial PIO mode data
+ *	@ap: ATA interface
+ *	@adev: ATA device
+ *
+ *	Used when configuring the devices ot set the PIO timings. All the
+ *	actual work is done by the PIO/MWDMA setting helper
+ */
+
+static void cmd64x_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	cmd64x_set_timing(ap, adev, adev->pio_mode);
+}
+
+/**
  *	cmd64x_set_dmamode	-	set initial DMA mode data
  *	@ap: ATA interface
  *	@adev: ATA device
@@ -180,9 +196,6 @@
 	static const u8 udma_data[] = {
 		0x30, 0x20, 0x10, 0x20, 0x10, 0x00
 	};
-	static const u8 mwdma_data[] = {
-		0x30, 0x20, 0x10
-	};
 
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
 	u8 regU, regD;
@@ -208,8 +221,10 @@
 		regU |= 1 << adev->devno; /* UDMA on */
 		if (adev->dma_mode > 2)	/* 15nS timing */
 			regU |= 4 << adev->devno;
-	} else
-		regD |= mwdma_data[adev->dma_mode - XFER_MW_DMA_0] << shift;
+	} else {
+		regU &= ~ (1 << adev->devno);	/* UDMA off */
+		cmd64x_set_timing(ap, adev, adev->dma_mode);
+	}
 
 	regD |= 0x20 << adev->devno;
 

Re: [PATCH] pata_cmd64x: Set up MWDMA modes properly

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

> Set the MWDMA timing by updating the correct registers. Split the PIO
> path as this is mostly shared code. Wants testing.

    Cool! So much simpler than my fix to the old IDE driver...

> Signed-off-by: Alan Cox <alan@redhat.com>

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c linux-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c
> --- linux.vanilla-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c	2007-07-26 15:01:52.000000000 +0100
> +++ linux-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c	2007-08-08 11:52:23.000000000 +0100
[...]
> @@ -117,8 +118,9 @@
>  	int arttim = arttim_port[ap->port_no][adev->devno];
>  	int drwtim = drwtim_port[ap->port_no][adev->devno];
>  
> -
> -	if (ata_timing_compute(adev, adev->pio_mode, &t, T, 0) < 0) {
> +	/* ata_timing_compute is smart and will produce timings for MWDMA
> +	   that don't violate the drives PIO capabilities. */
> +	if (ata_timing_compute(adev, mode, &t, T, 0) < 0) {
>  		printk(KERN_ERR DRV_NAME ": mode computation failed.\n");
>  		return;
>  	}

    That function rocks (except I didn't get what the address setup timings 
mean to SW/MW DMA)...

> @@ -168,6 +170,20 @@
>  }
>  
>  /**
> + *	cmd64x_set_piomode	-	set initial PIO mode data
> + *	@ap: ATA interface
> + *	@adev: ATA device
> + *
> + *	Used when configuring the devices ot set the PIO timings. All the

    What's "ot"?
    Hm, writing comments (and even the code) in boustrophedon manner (odd 
lines from the left to the right, and even lines from the right to to the 
left) is certainly a challenging idea. :-)

> + *	actual work is done by the PIO/MWDMA setting helper
> + */
> +
> +static void cmd64x_set_piomode(struct ata_port *ap, struct ata_device *adev)
> +{
> +	cmd64x_set_timing(ap, adev, adev->pio_mode);
> +}
> +
> +/**
>   *	cmd64x_set_dmamode	-	set initial DMA mode data
>   *	@ap: ATA interface
>   *	@adev: ATA device
> @@ -180,9 +196,6 @@
>  	static const u8 udma_data[] = {
>  		0x30, 0x20, 0x10, 0x20, 0x10, 0x00
>  	};
> -	static const u8 mwdma_data[] = {
> -		0x30, 0x20, 0x10
> -	};
>  
>  	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>  	u8 regU, regD;
> @@ -208,8 +221,10 @@
>  		regU |= 1 << adev->devno; /* UDMA on */
>  		if (adev->dma_mode > 2)	/* 15nS timing */
>  			regU |= 4 << adev->devno;
> -	} else
> -		regD |= mwdma_data[adev->dma_mode - XFER_MW_DMA_0] << shift;
> +	} else {
> +		regU &= ~ (1 << adev->devno);	/* UDMA off */
> +		cmd64x_set_timing(ap, adev, adev->dma_mode);
> +	}
>  
>  	regD |= 0x20 << adev->devno;

    Hm, is setting of "drive DMA capable" bits really needed in the driver. 
Doesn't libata core itself do this?

MBR, Sergei

Re: [PATCH] pata_cmd64x: Set up MWDMA modes properly

[Alan Cox]

>     That function rocks (except I didn't get what the address setup timings 
> mean to SW/MW DMA)...

They don't, it merges the DMA and PIO ones and you end up with the PIO
ones.

> > + *	Used when configuring the devices ot set the PIO timings. All the
> 
>     What's "ot"?

to - will fix

> >  	regD |= 0x20 << adev->devno;
> 
>     Hm, is setting of "drive DMA capable" bits really needed in the driver. 
> Doesn't libata core itself do this?

For most hardware it doesn't matter. The CMD64x driver code I have all
bothers to set it. I don't know if its needed, someone with the bits
handy could find out I guess, but if we arent sure it does no harm.

Re: [PATCH] pata_cmd64x: Set up MWDMA modes properly

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>    That function rocks (except I didn't get what the address setup timings 
>>mean to SW/MW DMA)...

> They don't, it merges the DMA and PIO ones and you end up with the PIO
> ones.

    What I meant there was this table from libata-core.c:

/*
  * PIO 0-4, MWDMA 0-2 and UDMA 0-6 timings (in nanoseconds).
  * These were taken from ATA/ATAPI-6 standard, rev 0a, except
  * for UDMA6, which is currently supported only by Maxtor drives.
  *
  * For PIO 5/6 MWDMA 3/4 see the CFA specification 3.0.
  */

static const struct ata_timing ata_timing[] = {

         { XFER_UDMA_6,     0,   0,   0,   0,   0,   0,   0,  15 },
         { XFER_UDMA_5,     0,   0,   0,   0,   0,   0,   0,  20 },
         { XFER_UDMA_4,     0,   0,   0,   0,   0,   0,   0,  30 },
         { XFER_UDMA_3,     0,   0,   0,   0,   0,   0,   0,  45 },

         { XFER_MW_DMA_4,  25,   0,   0,   0,  55,  20,  80,   0 },
         { XFER_MW_DMA_3,  25,   0,   0,   0,  65,  25, 100,   0 },
         { XFER_UDMA_2,     0,   0,   0,   0,   0,   0,   0,  60 },
         { XFER_UDMA_1,     0,   0,   0,   0,   0,   0,   0,  80 },
         { XFER_UDMA_0,     0,   0,   0,   0,   0,   0,   0, 120 },

/*      { XFER_UDMA_SLOW,  0,   0,   0,   0,   0,   0,   0, 150 }, */
 
 

         { XFER_MW_DMA_2,  25,   0,   0,   0,  70,  25, 120,   0 },
         { XFER_MW_DMA_1,  45,   0,   0,   0,  80,  50, 150,   0 },
         { XFER_MW_DMA_0,  60,   0,   0,   0, 215, 215, 480,   0 },

         { XFER_SW_DMA_2,  60,   0,   0,   0, 120, 120, 240,   0 },
         { XFER_SW_DMA_1,  90,   0,   0,   0, 240, 240, 480,   0 },
         { XFER_SW_DMA_0, 120,   0,   0,   0, 480, 480, 960,   0 },

         { XFER_PIO_6,     10,  55,  20,  80,  55,  20,  80,   0 },
         { XFER_PIO_5,     15,  65,  25, 100,  65,  25, 100,   0 },
         { XFER_PIO_4,     25,  70,  25, 120,  70,  25, 120,   0 },
         { XFER_PIO_3,     30,  80,  70, 180,  80,  70, 180,   0 },

         { XFER_PIO_2,     30, 290,  40, 330, 100,  90, 240,   0 },
         { XFER_PIO_1,     50, 290,  93, 383, 125, 100, 383,   0 },
         { XFER_PIO_0,     70, 290, 240, 600, 165, 150, 600,   0 },

/*      { XFER_PIO_SLOW, 120, 290, 240, 960, 290, 240, 960,   0 }, */

         { 0xFF }
};

    Note non-zero values in column 2 of the initilaizers for SW/MW DMA lines 
-- that's the 'setup' field of 'struct ata_timings'. Where did those figures 
come from?
    Ah, I see -- that must be *data* setup/hold times summed up. What I don't 
see is how it connects with the address setup time...

>>> 	regD |= 0x20 << adev->devno;

>>    Hm, is setting of "drive DMA capable" bits really needed in the driver. 
>>Doesn't libata core itself do this?

> For most hardware it doesn't matter. The CMD64x driver code I have all
> bothers to set it. I don't know if its needed, someone with the bits
> handy could find out I guess, but if we arent sure it does no harm.

    I've removed that from the IDE driver -- none yelled yet. :-)

MBR, Sergei

Re: [PATCH] pata_cmd64x: Set up MWDMA modes properly

[Bartlomiej Zolnierkiewicz]

On Saturday 11 August 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Alan Cox wrote:
> 
> > Set the MWDMA timing by updating the correct registers. Split the PIO
> > path as this is mostly shared code. Wants testing.
> 
>     Cool! So much simpler than my fix to the old IDE driver...
> 
> > Signed-off-by: Alan Cox <alan@redhat.com>
> 
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
> > diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c linux-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c
> > --- linux.vanilla-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c	2007-07-26 15:01:52.000000000 +0100
> > +++ linux-2.6.23rc1-mm1/drivers/ata/pata_cmd64x.c	2007-08-08 11:52:23.000000000 +0100
> [...]
> > @@ -117,8 +118,9 @@
> >  	int arttim = arttim_port[ap->port_no][adev->devno];
> >  	int drwtim = drwtim_port[ap->port_no][adev->devno];
> >  
> > -
> > -	if (ata_timing_compute(adev, adev->pio_mode, &t, T, 0) < 0) {
> > +	/* ata_timing_compute is smart and will produce timings for MWDMA
> > +	   that don't violate the drives PIO capabilities. */
> > +	if (ata_timing_compute(adev, mode, &t, T, 0) < 0) {
> >  		printk(KERN_ERR DRV_NAME ": mode computation failed.\n");
> >  		return;
> >  	}
> 
>     That function rocks (except I didn't get what the address setup timings 
> mean to SW/MW DMA)...

JFYI: this function was "borrowed" from drivers/ide/ide-timing.h,
you can use it in IDE host drivers as well... ;)

Bart

Re: [PATCH] pata_cmd64x: Set up MWDMA modes properly

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>@@ -117,8 +118,9 @@
>>> 	int arttim = arttim_port[ap->port_no][adev->devno];
>>> 	int drwtim = drwtim_port[ap->port_no][adev->devno];
>>> 
>>>-
>>>-	if (ata_timing_compute(adev, adev->pio_mode, &t, T, 0) < 0) {
>>>+	/* ata_timing_compute is smart and will produce timings for MWDMA
>>>+	   that don't violate the drives PIO capabilities. */
>>>+	if (ata_timing_compute(adev, mode, &t, T, 0) < 0) {
>>> 		printk(KERN_ERR DRV_NAME ": mode computation failed.\n");
>>> 		return;
>>> 	}

>>    That function rocks (except I didn't get what the address setup timings 
>>mean to SW/MW DMA)...

> JFYI: this function was "borrowed" from drivers/ide/ide-timing.h,
> you can use it in IDE host drivers as well... ;)

    Heh, I know.  What I don't know is where to borrow the time for doing 
this... :-/

> Bart

MBR, Sergei

Re: [PATCH] pata_cmd64x: Set up MWDMA modes properly

[Mikael Pettersson]

On Wed, 8 Aug 2007 14:33:20 +0100, Alan Cox wrote:
> Set the MWDMA timing by updating the correct registers. Split the PIO
> path as this is mostly shared code. Wants testing.

Works fine on my CMD646 SPARC Ultra5.

/Mikael

Re: [PATCH] pata_cmd64x: Set up MWDMA modes properly

[Alan Cox]

On Fri, 10 Aug 2007 01:09:13 +0200 (MEST)
Mikael Pettersson <mikpe@it.uu.se> wrote:

> On Wed, 8 Aug 2007 14:33:20 +0100, Alan Cox wrote:
> > Set the MWDMA timing by updating the correct registers. Split the PIO
> > path as this is mostly shared code. Wants testing.
> 
> Works fine on my CMD646 SPARC Ultra5.

Thanks