Patchwork [02/20] ata_piix: unify code for programming PIO and MWDMA timings

login
register
mail settings
Submitter Bartlomiej Zolnierkiewicz
Date Feb. 8, 2011, 12:23 p.m.
Message ID <20110208122346.19110.8441.sendpatchset@linux-mhg7.site>
Download mbox | patch
Permalink /patch/82323/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Bartlomiej Zolnierkiewicz - Feb. 8, 2011, 12:23 p.m.
From ca6c1fbc7d853829524b87a3bcbbb1f67796281c Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 8 Feb 2011 12:39:24 +0100
Subject: [PATCH 02/20] ata_piix: unify code for programming PIO and MWDMA timings

Besides making things noticably simpler it results in ~2% decrease in
the driver LOC count and also ~2% decrease in the driver binary size
(as measured on x86-32).

Fix piix_set_piomode() documentation while at it.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/ata_piix.c |  112 ++++++++++++++++--------------------------------
 1 files changed, 37 insertions(+), 75 deletions(-)
Sergei Shtylyov - Feb. 20, 2011, 1:36 p.m.
On 08.02.2011 15:23, Bartlomiej Zolnierkiewicz wrote:

>  From ca6c1fbc7d853829524b87a3bcbbb1f67796281c Mon Sep 17 00:00:00 2001
> From: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
> Date: Tue, 8 Feb 2011 12:39:24 +0100
> Subject: [PATCH 02/20] ata_piix: unify code for programming PIO and MWDMA timings

> Besides making things noticably simpler it results in ~2% decrease in
> the driver LOC count and also ~2% decrease in the driver binary size
> (as measured on x86-32).

> Fix piix_set_piomode() documentation while at it.

> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
[...]

> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 0f4856d..c954f91 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -696,22 +696,11 @@ static int piix_pata_prereset(struct ata_link *link, unsigned long deadline)
>
>   static DEFINE_SPINLOCK(piix_lock);
>
> -/**
> - *	piix_set_piomode - Initialize host controller PATA PIO timings
> - *	@ap: Port whose timings we are configuring
> - *	@adev: um
> - *
> - *	Set PIO mode for device, in host controller PCI config space.
> - *
> - *	LOCKING:
> - *	None (inherited from caller).
> - */
> -
> -static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
> +static void piix_set_timings(struct ata_port *ap, struct ata_device *adev,
> +			     u8 pio, bool use_mwdma)
>   {
>   	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
>   	unsigned long flags;
> -	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
>   	unsigned int is_slave	= (adev->devno != 0);
>   	unsigned int master_port= ap->port_no ? 0x42 : 0x40;
>   	unsigned int slave_port	= 0x44;
> @@ -732,14 +721,18 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
>   			    { 2, 1 },
>   			    { 2, 3 }, };
>
> -	if (pio >= 2)
> +	if (pio >= 2 || use_mwdma)

    The 'use_mwdma' check is actually superfluous...

>   		control |= 1;	/* TIME1 enable */
> -	if (ata_pio_need_iordy(adev))
> +	if (ata_pio_need_iordy(adev) || use_mwdma)
>   		control |= 2;	/* IE enable */

    Why IORDY is enabled for MWDMA has always been beyond me... I understand 
that the stupid Intel docs are to be blamed here.

> -
>   	/* Intel specifies that the PPE functionality is for disk only */
>   	if (adev->class == ATA_DEV_ATA)
>   		control |= 4;	/* PPE enable */
> +	/* If the drive MWDMA is faster than it can do PIO then
> +	   we must force PIO into PIO0 */

    It's not the preferred multi-line comment style:

/*
  * bla
  * bla
  */

> +	if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))

    Parens not needed around +.

> @@ -803,31 +812,20 @@ static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in
>   {
>   	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
>   	unsigned long flags;
> -	u8 master_port		= ap->port_no ? 0x42 : 0x40;
> -	u16 master_data;
>   	u8 speed		= adev->dma_mode;
>   	int devid		= adev->devno + 2 * ap->port_no;
>   	u8 udma_enable		= 0;
>
> -	static const	 /* ISP  RTC */
> -	u8 timings[][2]	= { { 0, 0 },
> -			    { 0, 0 },
> -			    { 1, 0 },
> -			    { 2, 1 },
> -			    { 2, 3 }, };
> -
> -	spin_lock_irqsave(&piix_lock, flags);
> -
> -	pci_read_config_word(dev, master_port, &master_data);
> -	if (ap->udma_mask)
> -		pci_read_config_byte(dev, 0x48, &udma_enable);

    I don't think you should have removed that...

> @@ -860,56 +858,20 @@ static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in
>   			   performance (WR_PingPong_En) */
>   			pci_write_config_word(dev, 0x54, ideconf);
>   		}
> +
> +		pci_write_config_byte(dev, 0x48, udma_enable);
> +
> +		spin_unlock_irqrestore(&piix_lock, flags);
>   	} else {
[...]
> -		if (ap->udma_mask)
> -			udma_enable &= ~(1 << devid);

    This does not seem correct -- you must disable UDMA when programming MWDMA.

> -
> -		pci_write_config_word(dev, master_port, master_data);
> +		piix_set_timings(ap, adev, pio, 1);
>   	}
> -	/* Don't scribble on 0x48 if the controller does not support UDMA */
> -	if (ap->udma_mask)
> -		pci_write_config_byte(dev, 0x48, udma_enable);
> -
> -	spin_unlock_irqrestore(&piix_lock, flags);
>   }
>
>   /**

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 20, 2011, 6:59 p.m.
> > +	if (ata_pio_need_iordy(adev) || use_mwdma)
> >   		control |= 2;	/* IE enable */
> 
>     Why IORDY is enabled for MWDMA has always been beyond me... I understand 
> that the stupid Intel docs are to be blamed here.

I fail to see whats stupid about the docs ?

The same timing register set is used for MWDMA and PIO cycles in MWDMA
tuned modes (eg ATAPI). Thus the controller needs to be in an MWDMA mode
whose timings are compatible with the PIO timing and we want IORDY in use
for the PIO transfer parts.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Feb. 20, 2011, 8:42 p.m.
Hello.

On 20-02-2011 21:59, Alan Cox wrote:

>>> +	if (ata_pio_need_iordy(adev) || use_mwdma)
>>>    		control |= 2;	/* IE enable */

>>      Why IORDY is enabled for MWDMA has always been beyond me... I understand
>> that the stupid Intel docs are to be blamed here.

> I fail to see whats stupid about the docs ?

    Association of IORDY with DMA mode is wrong.

> The same timing register set is used for MWDMA and PIO cycles in MWDMA
> tuned modes (eg ATAPI). Thus the controller needs to be in an MWDMA mode
> whose timings are compatible with the PIO timing and we want IORDY in use
> for the PIO transfer parts.

    Yeah, especially if we also set the bit which only enables fast timing for 
DMA. ;-)
    PIO mode is setup by different code, and it takes care of the IORDY 
setting according to the PIO rules (and it gets called). DMA mode setup should 
just ignore the IORDY setting as in all other sane drivers.

> Alan

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 20, 2011, 9:07 p.m.
>     PIO mode is setup by different code, and it takes care of the IORDY 
> setting according to the PIO rules (and it gets called). DMA mode setup should 
> just ignore the IORDY setting as in all other sane drivers.


Well it can't ignore it - but if you mean just keep the bit as is then
that sounds sensible, have to see what the docs say happens if you ever
set MWDMA without IORDY.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Feb. 21, 2011, 11:38 a.m.
Hello.

On 21-02-2011 0:07, Alan Cox wrote:

>>      PIO mode is setup by different code, and it takes care of the IORDY
>> setting according to the PIO rules (and it gets called). DMA mode setup should
>> just ignore the IORDY setting as in all other sane drivers.

> Well it can't ignore it - but if you mean just keep the bit as is then

    Yes, I mean this.

> that sounds sensible, have to see what the docs say happens if you ever
> set MWDMA without IORDY.

    Don't think they say anything on this matter but I can't imagine that 
IORDY matters for DMA.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Feb. 21, 2011, 11:53 a.m.
Hello.

On 21-02-2011 14:38, Sergei Shtylyov wrote:

>>> PIO mode is setup by different code, and it takes care of the IORDY
>>> setting according to the PIO rules (and it gets called). DMA mode setup should
>>> just ignore the IORDY setting as in all other sane drivers.

>> Well it can't ignore it - but if you mean just keep the bit as is then

> Yes, I mean this.

>> that sounds sensible, have to see what the docs say happens if you ever
>> set MWDMA without IORDY.

> Don't think they say anything on this matter but I can't imagine that IORDY
> matters for DMA.

    What it says is that IORDY enable bit has no effect when the fast timing 
bit is cleared. That's from ICH IDE PRM (29860004.pdf).

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 21, 2011, 11:58 a.m.
On Mon, 21 Feb 2011 14:38:37 +0300
Sergei Shtylyov <sshtylyov@mvista.com> wrote:

> Hello.
> 
> On 21-02-2011 0:07, Alan Cox wrote:
> 
> >>      PIO mode is setup by different code, and it takes care of the IORDY
> >> setting according to the PIO rules (and it gets called). DMA mode setup should
> >> just ignore the IORDY setting as in all other sane drivers.
> 
> > Well it can't ignore it - but if you mean just keep the bit as is then
> 
>     Yes, I mean this.
> 
> > that sounds sensible, have to see what the docs say happens if you ever
> > set MWDMA without IORDY.
> 
>     Don't think they say anything on this matter but I can't imagine that 
> IORDY matters for DMA.

Sure - but it does matter for an ATAPI transaction with then has a DMA
phase because those timings will be used for the command transfer which
is PIO.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Feb. 21, 2011, 11:59 a.m.
Hello.

On 21-02-2011 14:58, Alan Cox wrote:

>>>>       PIO mode is setup by different code, and it takes care of the IORDY
>>>> setting according to the PIO rules (and it gets called). DMA mode setup should
>>>> just ignore the IORDY setting as in all other sane drivers.

>>> Well it can't ignore it - but if you mean just keep the bit as is then

>>      Yes, I mean this.

>>> that sounds sensible, have to see what the docs say happens if you ever
>>> set MWDMA without IORDY.

>>      Don't think they say anything on this matter but I can't imagine that
>> IORDY matters for DMA.

    ... unless the IE bit means something special (and undocumented) for DMA, 
if Intel so insists on setting it for every DMA mode.

> Sure - but it does matter for an ATAPI transaction with then has a DMA
> phase because those timings will be used for the command transfer which
> is PIO.

    So what? We've already setup PIO timings before the DMA ones. Why force 
the IE bit?

> Alan

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 21, 2011, noon
> > Don't think they say anything on this matter but I can't imagine that IORDY
> > matters for DMA.
> 
>     What it says is that IORDY enable bit has no effect when the fast timing 
> bit is cleared. That's from ICH IDE PRM (29860004.pdf).

Ok thats the detail I missed - cool.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 0f4856d..c954f91 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -696,22 +696,11 @@  static int piix_pata_prereset(struct ata_link *link, unsigned long deadline)
 
 static DEFINE_SPINLOCK(piix_lock);
 
-/**
- *	piix_set_piomode - Initialize host controller PATA PIO timings
- *	@ap: Port whose timings we are configuring
- *	@adev: um
- *
- *	Set PIO mode for device, in host controller PCI config space.
- *
- *	LOCKING:
- *	None (inherited from caller).
- */
-
-static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
+static void piix_set_timings(struct ata_port *ap, struct ata_device *adev,
+			     u8 pio, bool use_mwdma)
 {
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	unsigned long flags;
-	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
 	unsigned int is_slave	= (adev->devno != 0);
 	unsigned int master_port= ap->port_no ? 0x42 : 0x40;
 	unsigned int slave_port	= 0x44;
@@ -732,14 +721,18 @@  static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
 			    { 2, 1 },
 			    { 2, 3 }, };
 
-	if (pio >= 2)
+	if (pio >= 2 || use_mwdma)
 		control |= 1;	/* TIME1 enable */
-	if (ata_pio_need_iordy(adev))
+	if (ata_pio_need_iordy(adev) || use_mwdma)
 		control |= 2;	/* IE enable */
-
 	/* Intel specifies that the PPE functionality is for disk only */
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE enable */
+	/* If the drive MWDMA is faster than it can do PIO then
+	   we must force PIO into PIO0 */
+	if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))
+		/* Enable DMA timing only */
+		control |= 8;	/* PIO cycles in PIO0 */
 
 	spin_lock_irqsave(&piix_lock, flags);
 
@@ -788,6 +781,22 @@  static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
 }
 
 /**
+ *	piix_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: Port whose timings we are configuring
+ *	@adev: Drive in question
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	piix_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);
+}
+
+/**
  *	do_pata_set_dmamode - Initialize host controller PATA PIO timings
  *	@ap: Port whose timings we are configuring
  *	@adev: Drive in question
@@ -803,31 +812,20 @@  static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in
 {
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	unsigned long flags;
-	u8 master_port		= ap->port_no ? 0x42 : 0x40;
-	u16 master_data;
 	u8 speed		= adev->dma_mode;
 	int devid		= adev->devno + 2 * ap->port_no;
 	u8 udma_enable		= 0;
 
-	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },
-			    { 0, 0 },
-			    { 1, 0 },
-			    { 2, 1 },
-			    { 2, 3 }, };
-
-	spin_lock_irqsave(&piix_lock, flags);
-
-	pci_read_config_word(dev, master_port, &master_data);
-	if (ap->udma_mask)
-		pci_read_config_byte(dev, 0x48, &udma_enable);
-
 	if (speed >= XFER_UDMA_0) {
-		unsigned int udma = adev->dma_mode - XFER_UDMA_0;
+		unsigned int udma = speed - XFER_UDMA_0;
 		u16 udma_timing;
 		u16 ideconf;
 		int u_clock, u_speed;
 
+		spin_lock_irqsave(&piix_lock, flags);
+
+		pci_read_config_byte(dev, 0x48, &udma_enable);
+
 		/*
 		 * UDMA is handled by a combination of clock switching and
 		 * selection of dividers
@@ -860,56 +858,20 @@  static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in
 			   performance (WR_PingPong_En) */
 			pci_write_config_word(dev, 0x54, ideconf);
 		}
+
+		pci_write_config_byte(dev, 0x48, udma_enable);
+
+		spin_unlock_irqrestore(&piix_lock, flags);
 	} else {
-		/*
-		 * MWDMA is driven by the PIO timings. We must also enable
-		 * IORDY unconditionally along with TIME1. PPE has already
-		 * been set when the PIO timing was set.
-		 */
-		unsigned int mwdma	= adev->dma_mode - XFER_MW_DMA_0;
-		unsigned int control;
-		u8 slave_data;
+		/* MWDMA is driven by the PIO timings. */
+		unsigned int mwdma = speed - XFER_MW_DMA_0;
 		const unsigned int needed_pio[3] = {
 			XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
 		};
 		int pio = needed_pio[mwdma] - XFER_PIO_0;
 
-		control = 3;	/* IORDY|TIME1 */
-
-		/* If the drive MWDMA is faster than it can do PIO then
-		   we must force PIO into PIO0 */
-
-		if (adev->pio_mode < needed_pio[mwdma])
-			/* Enable DMA timing only */
-			control |= 8;	/* PIO cycles in PIO0 */
-
-		if (adev->devno) {	/* Slave */
-			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
-			master_data |= control << 4;
-			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= (ap->port_no ? 0x0f : 0xf0);
-			/* Load the matching timing */
-			slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0);
-			pci_write_config_byte(dev, 0x44, slave_data);
-		} else { 	/* Master */
-			master_data &= 0xCCF4;	/* Mask out IORDY|TIME1|DMAONLY
-						   and master timing bits */
-			master_data |= control;
-			master_data |=
-				(timings[pio][0] << 12) |
-				(timings[pio][1] << 8);
-		}
-
-		if (ap->udma_mask)
-			udma_enable &= ~(1 << devid);
-
-		pci_write_config_word(dev, master_port, master_data);
+		piix_set_timings(ap, adev, pio, 1);
 	}
-	/* Don't scribble on 0x48 if the controller does not support UDMA */
-	if (ap->udma_mask)
-		pci_write_config_byte(dev, 0x48, udma_enable);
-
-	spin_unlock_irqrestore(&piix_lock, flags);
 }
 
 /**