Skip to contentSkip to conversation

ata: rework support for PIIX-alike PATA controllers

PATCH New Bartlomiej Zolnierkiewicz 4 participants 68 messages Feb 8, 2011 – Feb 24, 2011 libata

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ata: rework support for PIIX-alike PATA controllers

Hi,

The following patchset reworks support for PIIX-alike PATA controllers.

The first part rewrites code for programming PIO and MWDMA timings in the affected host drivers, while also fixing other issues discovered during the main task. Then the second part adds support for non-Intel PIIX-alike controllers to ata_piix host driver and removes separate host drivers.

All in all this greatly simplifies the total code needed for all PIIX-alikes support (~1400 LOC are gone and 5 host drivers are removed) and hopefully makes it easier to maintain it in the long-term (some drivers already got slightly out-of-sync with ata_piix host driver, i.e. pata_rdc lacked locking fixes and Power Management support, both issues are fixed by patchset).

All testing was done using QEMU's PIIX3 controller emulation so any testing with real EFAR, IT8213, old PIIX, RDC and Radisys R82600 PATA controllers would be really appreciated..

-- Bartlomiej

The following changes since commit 3c0eee3fe6a3a1c745379547c7e7c904aa64f6d5:

Linux 2.6.37 (2011-01-04 16:50:19 -0800)

are available:

Bartlomiej Zolnierkiewicz (20):
ata_piix: SITRE handling fix
ata_piix: unify code for programming PIO and MWDMA timings
pata_efar: fix register naming used in efar_set_piomode()
pata_efar: unify code for programming PIO and MWDMA timings
pata_efar: always program master_data before slave_data
pata_it8213: fix register naming used in it8213_set_piomode()
pata_it8213: unify code for programming PIO and MWDMA timings
pata_it8213: add UDMA100 and UDMA133 support
pata_oldpiix: unify code for programming PIO and MWDMA timings
pata_radisys: unify code for programming PIO and MWDMA timings
pata_rdc: unify code for programming PIO and MWDMA timings
pata_rdc: parallel scanning needs an extra locking
pata_rdc: add Power Management support
pata_oldpiix: add locking for parallel scanning
pata_oldpiix: enable parallel scan
ata_piix: add EFAR SLC90E66 support
ata_piix: add IT8213 support
ata_piix: add RDC support
ata_piix: add Intel old PIIX support
ata_piix: add Radisys R82600 support

 drivers/ata/Kconfig        |    5 +
 drivers/ata/Makefile       |    5 -
 drivers/ata/ata_piix.c     |  410 +++++++++++++++++++++++++++++++++-----------
 drivers/ata/pata_efar.c    |  319 ----------------------------------
 drivers/ata/pata_it8213.c  |  313 ---------------------------------
 drivers/ata/pata_oldpiix.c |  289 -------------------------------
 drivers/ata/pata_radisys.c |  268 -----------------------------
 drivers/ata/pata_rdc.c     |  401 -------------------------------------------
 8 files changed, 315 insertions(+), 1695 deletions(-)
 delete mode 100644 drivers/ata/pata_efar.c
 delete mode 100644 drivers/ata/pata_it8213.c
 delete mode 100644 drivers/ata/pata_oldpiix.c
 delete mode 100644 drivers/ata/pata_radisys.c
 delete mode 100644 drivers/ata/pata_rdc.c
#1 lore raw

PATCH 1/20 ata_piix: SITRE handling fix

Quoted 1 line

From 7b6d27a9d65fef8795fdeee3733316450f840aa6 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:24 +0100
Subject: [PATCH 01/20] ata_piix: SITRE handling fix

Set SITRE bit also in master-only configurations.

---
 drivers/ata/ata_piix.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/ata/ata_piix.c +3 -2
index 6cb14ca..0f4856d 100644
@@ -751,8 +751,6 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
 	if (is_slave) {
 		/* clear TIME1|IE1|PPE1|DTE1 */
 		master_data &= 0xff0f;
-		/* Enable SITRE (separate slave timing register) */
-		master_data |= 0x4000;
 		/* enable PPE1, IE1 and TIME1 as needed */
 		master_data |= (control << 4);
 		pci_read_config_byte(dev, slave_port, &slave_data);
@@ -770,6 +768,9 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
 			(timings[pio][0] << 12) |
 			(timings[pio][1] << 8);
 	}
+
+	/* Enable SITRE (separate slave timing register) */
+	master_data |= 0x4000;
 	pci_write_config_word(dev, master_port, master_data);
 	if (is_slave)
 		pci_write_config_byte(dev, slave_port, slave_data);
Signature · 1 line
1.7.1
#2 lore raw

Re: [PATCH 01/20] ata_piix: SITRE handling fix

Quoted 6 lines · On Tue, 08 Feb 2011 13:23:37 +0100 Bartlomiej Zolnierkiewicz…
Quoted 1 line

From 7b6d27a9d65fef8795fdeee3733316450f840aa6 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:24 +0100
Subject: [PATCH 01/20] ata_piix: SITRE handling fix

Set SITRE bit also in master-only configurations.

Why ?

#3 lore raw

Re: [PATCH 01/20] ata_piix: SITRE handling fix

Quoted 11 lines · Alan Cox
Quoted 6 lines · On Tue, 08 Feb 2011 13:23:37 +0100 Bartlomiej Zolnierkiewicz…
Quoted 1 line

From 7b6d27a9d65fef8795fdeee3733316450f840aa6 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:24 +0100
Subject: [PATCH 01/20] ata_piix: SITRE handling fix

Set SITRE bit also in master-only configurations.

Why ?

Mainly to match other PIIX-alike drivers and make further changes easier..

#4 lore raw

PATCH 2/20 ata_piix: unify code for programming PIO and MWDMA timings

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From ca6c1fbc7d853829524b87a3bcbbb1f67796281c Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
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.

---
 drivers/ata/ata_piix.c |  112 ++++++++++++++++--------------------------------
 1 files changed, 37 insertions(+), 75 deletions(-)
drivers/ata/ata_piix.c +37 -75
index 0f4856d..c954f91 100644
@@ -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);
 }
 
 /**
Signature · 1 line
1.7.1
#5 lore raw

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

Quoted 12 lines · Bartlomiej Zolnierkiewicz

From ca6c1fbc7d853829524b87a3bcbbb1f67796281c Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz<[email protected]>
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.

[...]

Quoted 30 lines
drivers/ata/ata_piix.c +2 -13
index 0f4856d..c954f91 100644
@@ -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;

drivers/ata/ata_piix.c static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) · line 724

Quoted 5 lines
drivers/ata/ata_piix.c +1 -1
@@ -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...

drivers/ata/ata_piix.c static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) · line 727

Quoted 4 lines
drivers/ata/ata_piix.c +1 -1
  		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.

drivers/ata/ata_piix.c static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) · line 732

Quoted 6 lines
drivers/ata/ata_piix.c +2 -1
-
  	/* 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
*/

drivers/ata/ata_piix.c static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) · line 733

Quoted 1 line
drivers/ata/ata_piix.c +1 -0
+	if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))

Parens not needed around +.

Quoted 12 lines
drivers/ata/ata_piix.c +0 -4
@@ -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 },

drivers/ata/ata_piix.c static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in · line 823

Quoted 10 lines
drivers/ata/ata_piix.c +0 -10
-			    { 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...

Quoted 9 lines
drivers/ata/ata_piix.c +4 -0
@@ -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 {

[...]

drivers/ata/ata_piix.c static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in · line 904

Quoted 2 lines
drivers/ata/ata_piix.c +0 -2
-		if (ap->udma_mask)
-			udma_enable &= ~(1 << devid);

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

drivers/ata/ata_piix.c static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in · line 877

Quoted 12 lines
drivers/ata/ata_piix.c +1 -7
-
-		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

#6 lore raw

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

Quoted 5 lines

drivers/ata/ata_piix.c static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) · line 727

Quoted 2 lines
drivers/ata/ata_piix.c +1 -0
+	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

#7 lore raw

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

Hello.

Quoted 7 lines · Alan Cox
Quoted 5 lines

drivers/ata/ata_piix.c static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) · line 727

Quoted 2 lines
drivers/ata/ata_piix.c +1 -0
+	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.

Quoted 4 lines

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.

Quoted 1 line

Alan

WBR, Sergei

#8 lore raw

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

Quoted 3 lines

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.

#9 lore raw

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

Hello.

Quoted 5 lines · Alan Cox
Quoted 3 lines

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.

Quoted 2 lines

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

#10 lore raw

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

Hello.

Quoted 6 lines · Sergei Shtylyov
Quoted 5 lines
Quoted 3 lines

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

Quoted 7 lines

Yes, I mean this.

Quoted 2 lines

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

#11 lore raw

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

Quoted 5 lines
Quoted 2 lines

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.

#12 lore raw

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

In reply to Sergei Shtylyov
Quoted 10 lines · On Mon, 21 Feb 2011 14:38:37 +0300 Sergei Shtylyov <[email protected]> wrote:

Hello.

Quoted 5 lines · Alan Cox
Quoted 3 lines

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

Quoted 7 lines

Yes, I mean this.

Quoted 2 lines

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

#13 lore raw

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

Hello.

Quoted 6 lines · Alan Cox
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines

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

Quoted 7 lines
Quoted 7 lines

Yes, I mean this.

Quoted 2 lines

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.

Quoted 3 lines

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?

Quoted 1 line

Alan

WBR, Sergei

#14 lore raw

PATCH 3/20 pata_efar: fix register naming used in efar_set_piomode()

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From efbad2a0611e47772c8696a2c5b2037c78554378 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:25 +0100
Subject: [PATCH 03/20] pata_efar: fix register naming used in efar_set_piomode()

Rename 'idetm_port' and 'idetm_data' variables to 'master_port' and 'master_data' respectively to match register naming used in efar_set_dma_mode() and in ata_piix.c.

Fix efar_set_piomode() documentation and 'master_port' type while at it.

---
 drivers/ata/pata_efar.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)
drivers/ata/pata_efar.c +11 -11
index a088347..afe92b7 100644
@@ -73,7 +73,7 @@ static DEFINE_SPINLOCK(efar_lock);
 /**
  *	efar_set_piomode - Initialize host controller PATA PIO timings
  *	@ap: Port whose timings we are configuring
- *	@adev: um
+ *	@adev: Device to program
  *
  *	Set PIO mode for device, in host controller PCI config space.
  *
@@ -85,9 +85,9 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
 {
 	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	unsigned int idetm_port= ap->port_no ? 0x42 : 0x40;
 	unsigned long flags;
-	u16 idetm_data;
+	u8 master_port		= ap->port_no ? 0x42 : 0x40;
+	u16 master_data;
 	u8 udma_enable;
 	int control = 0;
 
@@ -113,20 +113,20 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
 
 	spin_lock_irqsave(&efar_lock, flags);
 
-	pci_read_config_word(dev, idetm_port, &idetm_data);
+	pci_read_config_word(dev, master_port, &master_data);
 
 	/* Set PPE, IE, and TIME as appropriate */
 	if (adev->devno == 0) {
-		idetm_data &= 0xCCF0;
-		idetm_data |= control;
-		idetm_data |= (timings[pio][0] << 12) |
+		master_data &= 0xCCF0;
+		master_data |= control;
+		master_data |= (timings[pio][0] << 12) |
 			(timings[pio][1] << 8);
 	} else {
 		int shift = 4 * ap->port_no;
 		u8 slave_data;
 
-		idetm_data &= 0xFF0F;
-		idetm_data |= (control << 4);
+		master_data &= 0xFF0F;
+		master_data |= (control << 4);
 
 		/* Slave timing in separate register */
 		pci_read_config_byte(dev, 0x44, &slave_data);
@@ -135,8 +135,8 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_write_config_byte(dev, 0x44, slave_data);
 	}
 
-	idetm_data |= 0x4000;	/* Ensure SITRE is set */
-	pci_write_config_word(dev, idetm_port, idetm_data);
+	master_data |= 0x4000;	/* Ensure SITRE is set */
+	pci_write_config_word(dev, master_port, master_data);
 
 	pci_read_config_byte(dev, 0x48, &udma_enable);
 	udma_enable &= ~(1 << (2 * ap->port_no + adev->devno));
Signature · 1 line
1.7.1
#15 lore raw

PATCH 4/20 pata_efar: unify code for programming PIO and MWDMA timings

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 2de7db4bcc16b7e95cca9ceb5921a6f620be76b7 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:25 +0100
Subject: [PATCH 04/20] pata_efar: unify code for programming PIO and MWDMA timings

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

---
 drivers/ata/pata_efar.c |  104 +++++++++++++++++------------------------------
 1 files changed, 37 insertions(+), 67 deletions(-)
drivers/ata/pata_efar.c +37 -67
index afe92b7..1e2ff7d 100644
@@ -70,20 +70,9 @@ static int efar_cable_detect(struct ata_port *ap)
 
 static DEFINE_SPINLOCK(efar_lock);
 
-/**
- *	efar_set_piomode - Initialize host controller PATA PIO timings
- *	@ap: Port whose timings we are configuring
- *	@adev: Device to program
- *
- *	Set PIO mode for device, in host controller PCI config space.
- *
- *	LOCKING:
- *	None (inherited from caller).
- */
-
-static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
+static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
+			     u8 pio, bool use_mwdma)
 {
-	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	unsigned long flags;
 	u8 master_port		= ap->port_no ? 0x42 : 0x40;
@@ -103,13 +92,18 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
 			    { 2, 1 },
 			    { 2, 3 }, };
 
-	if (pio > 1)
+	if (pio > 1 || use_mwdma)
 		control |= 1;	/* TIME */
-	if (ata_pio_need_iordy(adev))	/* PIO 3/4 require IORDY */
+	if (ata_pio_need_iordy(adev) || use_mwdma)
 		control |= 2;	/* IE */
 	/* Intel specifies that the prefetch/posting is for disk only */
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE */
+	/* 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(&efar_lock, flags);
 
@@ -145,6 +139,22 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
 }
 
 /**
+ *	efar_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: Port whose timings we are configuring
+ *	@adev: Device to program
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void efar_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	efar_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);
+}
+
+/**
  *	efar_set_dmamode - Initialize host controller PATA DMA timings
  *	@ap: Port whose timings we are configuring
  *	@adev: Device to program
@@ -158,29 +168,19 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
 static void efar_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	u8 master_port		= ap->port_no ? 0x42 : 0x40;
-	u16 master_data;
 	u8 speed		= adev->dma_mode;
 	int devid		= adev->devno + 2 * ap->port_no;
 	unsigned long flags;
 	u8 udma_enable;
 
-	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },
-			    { 0, 0 },
-			    { 1, 0 },
-			    { 2, 1 },
-			    { 2, 3 }, };
-
-	spin_lock_irqsave(&efar_lock, flags);
-
-	pci_read_config_word(dev, master_port, &master_data);
-	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;
 
+		spin_lock_irqsave(&efar_lock, flags);
+
+		pci_read_config_byte(dev, 0x48, &udma_enable);
+
 		udma_enable |= (1 << devid);
 
 		/* Load the UDMA mode number */
@@ -188,50 +188,20 @@ static void efar_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 		udma_timing &= ~(7 << (4 * devid));
 		udma_timing |= udma << (4 * devid);
 		pci_write_config_word(dev, 0x4A, udma_timing);
+
+		pci_write_config_byte(dev, 0x48, udma_enable);
+
+		spin_unlock_irqrestore(&efar_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);
-		}
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+		efar_set_timings(ap, adev, pio, 1);
 	}
-	pci_write_config_byte(dev, 0x48, udma_enable);
-	spin_unlock_irqrestore(&efar_lock, flags);
 }
 
 static struct scsi_host_template efar_sht = {
Signature · 1 line
1.7.1
#16 lore raw

Re: [PATCH 04/20] pata_efar: unify code for programming PIO and MWDMA timings

Hello.

Quoted 10 lines · Bartlomiej Zolnierkiewicz

From 2de7db4bcc16b7e95cca9ceb5921a6f620be76b7 Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz<[email protected]>
Date: Tue, 8 Feb 2011 12:39:25 +0100
Subject: [PATCH 04/20] pata_efar: unify code for programming PIO and MWDMA timings

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

[...]

Quoted 4 lines
drivers/ata/pata_efar.c +0 -0
index afe92b7..1e2ff7d 100644

[...]

drivers/ata/pata_efar.c static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 98

Quoted 10 lines
drivers/ata/pata_efar.c +2 -2
@@ -103,13 +92,18 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
  			    { 2, 1 },
  			    { 2, 3 }, };
 
-	if (pio>  1)
+	if (pio>  1 || use_mwdma)
  		control |= 1;	/* TIME */
-	if (ata_pio_need_iordy(adev))	/* PIO 3/4 require IORDY */
+	if (ata_pio_need_iordy(adev) || use_mwdma)
  		control |= 2;	/* IE */

Same comments as for ata_piix for these two bits...

drivers/ata/pata_efar.c static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 103

Quoted 5 lines
drivers/ata/pata_efar.c +2 -0
  	/* Intel specifies that the prefetch/posting is for disk only */
  	if (adev->class == ATA_DEV_ATA)
  		control |= 4;	/* PPE */
+	/* If the drive MWDMA is faster than it can do PIO then
+	   we must force PIO into PIO0 */

Fix comment style, like in ata_piix.

drivers/ata/pata_efar.c static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 104

Quoted 1 line
drivers/ata/pata_efar.c +1 -0
+	if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))

Parens not needed around +.

Quoted 7 lines
drivers/ata/pata_efar.c +3 -0
@@ -145,6 +139,22 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
  }
 
  /**
+ *	efar_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: Port whose timings we are configuring
+ *	@adev: Device to program

drivers/ata/pata_efar.c static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 154

Quoted 10 lines
drivers/ata/pata_efar.c +10 -0
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void efar_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	efar_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);

s/0/false/? Likewise in ata_piix...

Quoted 21 lines
drivers/ata/pata_efar.c +0 -12
@@ -158,29 +168,19 @@ static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev)
  static void efar_set_dmamode (struct ata_port *ap, struct ata_device *adev)
  {
  	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	u8 master_port		= ap->port_no ? 0x42 : 0x40;
-	u16 master_data;
  	u8 speed		= adev->dma_mode;
  	int devid		= adev->devno + 2 * ap->port_no;
  	unsigned long flags;
  	u8 udma_enable;
 
-	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },
-			    { 0, 0 },
-			    { 1, 0 },
-			    { 2, 1 },
-			    { 2, 3 }, };
-
-	spin_lock_irqsave(&efar_lock, flags);
-
-	pci_read_config_word(dev, master_port, &master_data);

drivers/ata/pata_efar.c static void efar_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 182

Quoted 10 lines
drivers/ata/pata_efar.c +4 -3
-	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;
 
+		spin_lock_irqsave(&efar_lock, flags);
+
+		pci_read_config_byte(dev, 0x48, &udma_enable);

Don't think you should have moved this...

Quoted 5 lines
drivers/ata/pata_efar.c +1 -0
+
  		udma_enable |= (1 << devid);
 
  		/* Load the UDMA mode number */
@@ -188,50 +188,20 @@ static void efar_set_dmamode (struct ata_port *ap, struct ata_device *adev)

[...]

drivers/ata/pata_efar.c static void efar_set_dmamode (struct ata_port *ap, struct ata_device *adev) · line 231

Quoted 2 lines
drivers/ata/pata_efar.c +0 -2
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);

No, you should disable UDMA when programming MWDMA.

drivers/ata/pata_efar.c static void efar_set_dmamode (struct ata_port *ap, struct ata_device *adev) · line 203

Quoted 1 line
drivers/ata/pata_efar.c +1 -0
+		efar_set_timings(ap, adev, pio, 1);

s/1/true/? Likewise in ata_piix...

WBR, Sergei

#17 lore raw

PATCH 5/20 pata_efar: always program master_data before slave_data

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 8acc9371550ce5f98da56f6888ae4b898096ed2c Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:25 +0100
Subject: [PATCH 05/20] pata_efar: always program master_data before slave_data

We may need to set SITRE before programming slave_data.

This makes pata_efar match the behavior of IDE's slc90e66 host driver and also of libata's ata_piix one.

---
 drivers/ata/pata_efar.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
drivers/ata/pata_efar.c +6 -3
index 1e2ff7d..7f564d7 100644
@@ -74,10 +74,12 @@ static void efar_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 int is_slave	= (adev->devno != 0);
 	unsigned long flags;
 	u8 master_port		= ap->port_no ? 0x42 : 0x40;
 	u16 master_data;
 	u8 udma_enable;
+	u8 slave_data;
 	int control = 0;
 
 	/*
@@ -110,14 +112,13 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
 	pci_read_config_word(dev, master_port, &master_data);
 
 	/* Set PPE, IE, and TIME as appropriate */
-	if (adev->devno == 0) {
+	if (is_slave == 0) {
 		master_data &= 0xCCF0;
 		master_data |= control;
 		master_data |= (timings[pio][0] << 12) |
 			(timings[pio][1] << 8);
 	} else {
 		int shift = 4 * ap->port_no;
-		u8 slave_data;
 
 		master_data &= 0xFF0F;
 		master_data |= (control << 4);
@@ -126,12 +127,14 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
 		pci_read_config_byte(dev, 0x44, &slave_data);
 		slave_data &= ap->port_no ? 0x0F : 0xF0;
 		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << shift;
-		pci_write_config_byte(dev, 0x44, slave_data);
 	}
 
 	master_data |= 0x4000;	/* Ensure SITRE is set */
 	pci_write_config_word(dev, master_port, master_data);
 
+	if (is_slave)
+		pci_write_config_byte(dev, 0x44, slave_data);
+
 	pci_read_config_byte(dev, 0x48, &udma_enable);
 	udma_enable &= ~(1 << (2 * ap->port_no + adev->devno));
 	pci_write_config_byte(dev, 0x48, udma_enable);
Signature · 1 line
1.7.1
#18 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 6 lines · On Tue, 08 Feb 2011 13:24:09 +0100 Bartlomiej Zolnierkiewicz…
Quoted 1 line

From 8acc9371550ce5f98da56f6888ae4b898096ed2c Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:25 +0100
Subject: [PATCH 05/20] pata_efar: always program master_data before slave_data

We may need to set SITRE before programming slave_data.

Do you have a documentation cite for this or is it random fiddling with a driver you can't test ?

#19 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 4 lines · Alan Cox
Quoted 1 line · On Tue, 08 Feb 2011 13:24:09 +0100 Bartlomiej Zolnierkiewicz…
Quoted 1 line

From 8acc9371550ce5f98da56f6888ae4b898096ed2c Mon Sep 17 00:00:00 2001

Quoted 8 lines
Quoted 5 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:25 +0100
Subject: [PATCH 05/20] pata_efar: always program master_data before slave_data

We may need to set SITRE before programming slave_data.

Do you have a documentation cite for this or is it random fiddling with a driver you can't test ?

It is good to enable SITRE register before programming it and all Intel controllers
(from which EFAR chipset is derived) are programmed this way (per ata_piix.c).

#20 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

We may need to set SITRE before programming slave_data.

Do you have a documentation cite for this or is it random fiddling with a driver you can't test ?

It is good to enable SITRE register before programming it and all Intel controllers

That sounds like someone quoting religion. Documentation cite please.

#21 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 9 lines · Alan Cox
Quoted 7 lines
Quoted 4 lines
Quoted 1 line

We may need to set SITRE before programming slave_data.

Do you have a documentation cite for this or is it random fiddling with a driver you can't test ?

It is good to enable SITRE register before programming it and all Intel controllers

That sounds like someone quoting religion. Documentation cite please.

s/religion/common sense/

#22 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 3 lines
Quoted 1 line

That sounds like someone quoting religion. Documentation cite please.

s/religion/common sense/

Anyone who programs ATA controllers on the basis of common sense rather than documentation, errata sheets and actually testing rather than speculating is naïve. In the case of ATA more so than most hardware.

Stil waiting a documentation cite for your otherwise gratuitious and untested change

#23 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 7 lines · Alan Cox
Quoted 3 lines
Quoted 1 line

That sounds like someone quoting religion. Documentation cite please.

s/religion/common sense/

Anyone who programs ATA controllers on the basis of common sense rather than documentation, errata sheets and actually testing rather than speculating is naïve. In the case of ATA more so than most hardware.

Fully agreed.

Quoted 2 lines

Stil waiting a documentation cite for your otherwise gratuitious and untested change

Alan, I'm not doing real-time consulting with you. The code is provided in the best effort manner and it most likely will be underrated (like all ATA/IDE work is) anyway so I do not really care that much if it will be merged ever. Once it will be merged it may only mean more unpaid support time for me since some maintainers are
not doing any real maintenance work these days.. Please at least
respect some of my time spent on all this burdensome silly little driver differences comparisons and read the whole patch set before making comments on individual changes (which you certainly haven't done given timing of your review mails and complexity of changes)..

#24 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 4 lines

respect some of my time spent on all this burdensome silly little driver differences comparisons and read the whole patch set before making comments on individual changes (which you certainly haven't done given timing of your review mails and complexity of changes)..

I was hoping you'd improved but apparently not.

Any untested change is dangerous. An untested change that merges drivers together simply means you can break lots of stuff for no gain at all.

If these were all PCI card devices it might make some sense but given they are all motherboard chipsets putting them into one driver merely increases memory use as well.

As far as stuff like

  unsigned int has_sitre  = (dev->vendor != 0x8086 || dev->device != 0x1230);

and the even worse mess you generate with the added patch all the other PIIX code does this by flags, and if you had a HAS_SITRE (or NO_SITRE) flag in the device data it would be obvious to anyone reading stuff how it all fitted together.

Alan

#25 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 6 lines · Alan Cox
Quoted 4 lines

respect some of my time spent on all this burdensome silly little driver differences comparisons and read the whole patch set before making comments on individual changes (which you certainly haven't done given timing of your review mails and complexity of changes)..

I was hoping you'd improved but apparently not.

I'm not the person who doesn't change..

Quoted 2 lines

Any untested change is dangerous. An untested change that merges drivers together simply means you can break lots of stuff for no gain at all.

This is why patches were posted to mailing list with a request for a real hardware testing:

"All testing was done using QEMU's PIIX3 controller emulation so any testing with real EFAR, IT8213, old PIIX, RDC and Radisys R82600 PATA controllers would be really appreciated.."

instead of request for a merge. It was all there in initial mail.

Besides decreased complexity,1400 LOC gone and 5 drivers removed cannot be really called "no gain at all".

Quoted 3 lines

If these were all PCI card devices it might make some sense but given they are all motherboard chipsets putting them into one driver merely increases memory use as well.

We can improve situation here by doing generic ATA or SCSI changes instead, not worth keeping separate drivers when you make similar effect but more maintainable and generic changes.

Quoted 9 lines

As far as stuff like

   unsigned int has_sitre  = (dev->vendor != 0x8086 || dev->device != 0x1230);

and the even worse mess you generate with the added patch all the other PIIX code does this by flags, and if you had a HAS_SITRE (or NO_SITRE) flag in the device data it would be obvious to anyone reading stuff how it all fitted together.

Now that's what is a real review work. It will be changed in the next revision, thanks!

-- Bartlomiej

#26 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

In reply to Alan Cox

Hello.

Quoted 9 lines · Alan Cox
Quoted 7 lines
Quoted 4 lines
Quoted 1 line

We may need to set SITRE before programming slave_data.

Do you have a documentation cite for this or is it random fiddling with a driver you can't test ?

It is good to enable SITRE register before programming it and all Intel controllers

That sounds like someone quoting religion. Documentation cite please.

SLC90E66 datasheet only says that SIDETIM register has no effect without
SITRE bit set.

WBR, Sergei

#27 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Hi,

Quoted 10 lines · Sergei Shtylyov

Hello.

Quoted 5 lines · Alan Cox
Quoted 5 lines
Quoted 5 lines
Quoted 1 line

We may need to set SITRE before programming slave_data.

Do you have a documentation cite for this or is it random fiddling with a
driver you can't test ?

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

It is good to enable SITRE register before programming it and all Intel controllers

That sounds like someone quoting religion. Documentation cite please.

  SLC90E66 datasheet only says that SIDETIM register has no effect without SITRE bit set.

Alan, is this explanation sufficient to ACK this patch? (Thanks Sergei for digging it out.)

Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there are no further concerns with them (thus leaving the merging of PIIX-like drivers for later)? They got additional testing on ICH4 and they look mostly safe & straight-forward compared to #16-21.

Thanks,
Bartlomiej

#28 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 2 lines
Quoted 2 lines

  SLC90E66 datasheet only says that SIDETIM register has no effect without SITRE bit set.

Which means the current setup is fine yes ?

Quoted 4 lines

Alan, is this explanation sufficient to ACK this patch? (Thanks Sergei for digging it out.)

Jeff, would it be possible to queue patches #01-15 for 2.6.39

Have they been tested on the relevant hardware yet - I don't believe the changes should be made for the untested stuff and you indicated you agreed with that.

The tidyups for the easily tested cases (PIIX4 etc) look sensible

#29 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 4 lines · Alan Cox
Quoted 2 lines
Quoted 2 lines

  SLC90E66 datasheet only says that SIDETIM register has no effect without SITRE bit set.

Which means the current setup is fine yes ?

That's the problem, the current setup is buggy in slave-only setups:

                slave_data |= ((timings[pio][0] << 2) |
timings[pio][1]) << shift;
                pci_write_config_byte(dev, 0x44, slave_data);
        }

        idetm_data |= 0x4000;   /* Ensure SITRE is set */
        pci_write_config_word(dev, idetm_port, idetm_data);

idetm_port needs to be programmed _before_ slave_data to "ensure SITRE is set".

IOW We currently depend on the BIOS here to set SITRE bit for us..

Quoted 8 lines
Quoted 4 lines

Alan, is this explanation sufficient to ACK this patch?  (Thanks Sergei for digging it out.)

Jeff, would it be possible to queue patches #01-15 for 2.6.39

Have they been tested on the relevant hardware yet - I don't believe the changes should be made for the untested stuff and you indicated you agreed with that.

Unless they are obvious or risk is higher than benefit (bugfixes based on code review). I don't think that ATA code has became recently special in this regard compared to the rest of the kernel.

Quoted 1 line

The tidyups for the easily tested cases (PIIX4 etc) look sensible

Please take look at the changes #01-15 then:

ata_piix: SITRE handling fix
ata_piix: unify code for programming PIO and MWDMA timings
pata_efar: fix register naming used in efar_set_piomode()
pata_efar: unify code for programming PIO and MWDMA timings
pata_efar: always program master_data before slave_data
pata_it8213: fix register naming used in it8213_set_piomode()
pata_it8213: unify code for programming PIO and MWDMA timings
pata_it8213: add UDMA100 and UDMA133 support
pata_oldpiix: unify code for programming PIO and MWDMA timings
pata_radisys: unify code for programming PIO and MWDMA timings
pata_rdc: unify code for programming PIO and MWDMA timings
pata_rdc: parallel scanning needs an extra locking
pata_rdc: add Power Management support
pata_oldpiix: add locking for parallel scanning
pata_oldpiix: enable parallel scan

Most patches has been posted months ago in the past as the part of atang tree. They don't contain risky stuff (it was intentionally pushed at the end of the patch queue). We have here obvious register naming unification, PIO/DMA folding (it was tested with ata_piix and only later ported to other PIIX-like drivers) and:

* para_rdc: locking bug-fix and Power Management support

* pata_it8213: IDE -> libata regression fix (UDMA100/133 support based on vendor / old IDE driver)

* pata_oldpiix: locking bug-fix (ACK-ed by you in the past) and parallel scanning support

I see a lot of magnitude more risky stuff going elsewhere in the kernel, including ATA itself and if there are any concerns left those patches (#01-15) I'll be happy to address them.

Thanks,
Bartlomiej

#30 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 4 lines
        idetm_data |= 0x4000;   /* Ensure SITRE is set */
        pci_write_config_word(dev, idetm_port, idetm_data);

idetm_port needs to be programmed _before_ slave_data to "ensure SITRE is set".

That isn't what the documentation seems to say. It says it has no effect unless SITRE is set not that you can't program the registers.

Quoted 3 lines

Unless they are obvious or risk is higher than benefit (bugfixes based on code review). I don't think that ATA code has became recently special in this regard compared to the rest of the kernel.

They aren't special - random unneeded code changes that haven't been tested shouldn't be going into the code unless there is a big gain. For obscure ancient devices there isn't a gain.

Quoted 1 line

ata_piix: unify code for programming PIO and MWDMA timings

Which as I said makes sense

Quoted 3 lines

pata_efar: fix register naming used in efar_set_piomode()
pata_efar: unify code for programming PIO and MWDMA timings
pata_efar: always program master_data before slave_data

All untested

Quoted 3 lines

pata_it8213: fix register naming used in it8213_set_piomode()
pata_it8213: unify code for programming PIO and MWDMA timings
pata_it8213: add UDMA100 and UDMA133 support

All untested

Quoted 1 line

pata_oldpiix: unify code for programming PIO and MWDMA timings

Untested

Quoted 1 line

pata_radisys: unify code for programming PIO and MWDMA timings

Untested

Quoted 3 lines

pata_rdc: unify code for programming PIO and MWDMA timings
pata_rdc: parallel scanning needs an extra locking
pata_rdc: add Power Management support

All untested but the locking is a clear bug fix and I think should go in

Quoted 2 lines

pata_oldpiix: add locking for parallel scanning
pata_oldpiix: enable parallel scan

This is an ancient device on some old pentium class boxes, it's not worth adding this stuff really is it ? Well maybe putting the locking in or at least comments that it will be needed ?

Quoted 2 lines

Most patches has been posted months ago in the past as the part of atang tree.

So - where are the test reports

Quoted 2 lines

* pata_it8213: IDE -> libata regression fix (UDMA100/133 support based on vendor / old IDE driver)

I didn't see it tested in the old IDE driver either.

Quoted 2 lines

* pata_oldpiix: locking bug-fix (ACK-ed by you in the past) and parallel scanning support

I'm happy with the locking fix, I don't see the point in the parallel scan - and that wants testing because I don't know how the hardware will react. Most controllers were not designed for parallel scan and its a path windows will never have exercised therefore may well never have been tested out.

In the PIIX4+ cases Jeff insisted we chased this down with the hardware folks in Intel to be sure it was ok. I'm not sure there is anyone who remembers original PIIX however.

Quoted 2 lines

I see a lot of magnitude more risky stuff going elsewhere in the kernel, including ATA itself

And being tested.

efar/it8213/radisys are going to be tricky to find testers for as they are rare chips but the RDC is found in some of the embedded CPU/ATA/Net/etc in a device embedded x86 devices.

Alan

#31 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

In reply to Bartlomiej Zolnierkiewicz
Quoted 4 lines · Bartlomiej Zolnierkiewicz

Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there are no further concerns with them (thus leaving the merging of PIIX-like drivers for later)? They got additional testing on ICH4 and they look mostly safe& straight-forward compared to #16-21.

This seems to directly contradict what you wrote earlier in the thread,

This is why patches were posted to mailing list with a request for a real hardware testing:

"All testing was done using QEMU's PIIX3 controller emulation so any testing with real EFAR, IT8213, old PIIX, RDC and Radisys R82600 PATA controllers would be really appreciated.."

instead of request for a merge. It was all there in initial mail.

and

I do not really care that much if it will be merged ever

Regardless of this self-contradictory attitude, I do want useful patches and many of these patches seem useful.

So I will continue watching the Bart/Alan/Sergei threads play out, and then look at merging the result. In the midst of all the arguing, productive work / forward progress is occurring, so the end result should be positive.

It would be nice if we could get at least an "it works" test for the older hardware, since those are the changes /least/ likely to be tested by queueing to linux-next.

Jeff

#32 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Hi,

Quoted 19 lines · Jeff Garzik
Quoted 5 lines · Bartlomiej Zolnierkiewicz

Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there are no further concerns with them (thus leaving the merging of PIIX-like drivers for later)?  They got additional testing on ICH4 and they look mostly safe&  straight-forward compared to #16-21.

This seems to directly contradict what you wrote earlier in the thread,

       This is why patches were posted to mailing list with a request for a real hardware testing:

       "All testing was done using QEMU's PIIX3 controller emulation so any testing with real EFAR, IT8213, old PIIX, RDC and Radisys R82600 PATA controllers would be really appreciated.."

       instead of request for a merge.  It was all there in initial mail.

Quoted 6 lines

and

       I do not really care that much if it will be merged ever

Regardless of this self-contradictory attitude, I do want useful patches and many of these patches seem useful.

Nothing self-contradictory there. :)

First quote is about patches #01-15 only, not whole patchset (#01-20) like the second one, and I still don't care _that_ much personally if it gets merged since it is all unpaid & voluntary work.

Quoted 7 lines

So I will continue watching the Bart/Alan/Sergei threads play out, and then look at merging the result.  In the midst of all the arguing, productive work / forward progress is occurring, so the end result should be positive.

It would be nice if we could get at least an "it works" test for the older hardware, since those are the changes /least/ likely to be tested by queueing to linux-next.

I was thinking about re-doing ata_piix part in a way that we could merge it now by adding support for older PIIX-alikes to ata_piix and making it enabled only if "all_piixalikes" module parameter is
specified. This way older drivers would be left untouched for now
and we can easily get in-tree testing for a new code. Does it sound as a viable alternative?

Thanks,
Bartlomiej

#33 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 6 lines

I was thinking about re-doing ata_piix part in a way that we could merge it now by adding support for older PIIX-alikes to ata_piix and making it enabled only if "all_piixalikes" module parameter is
specified. This way older drivers would be left untouched for now
and we can easily get in-tree testing for a new code. Does it sound as a viable alternative?

That is the bit to me that makes the least sense. Each of the devices in question is found only in the chipset so they can never be in combination on a board (except multiple PIIX4 which is already covered)

Merging them makes it more likely stuff breaks and increases memory usage
- its a lose/lose situation.

Alan

#34 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 7 lines · Alan Cox
Quoted 6 lines

I was thinking about re-doing ata_piix part in a way that we could merge it now by adding support for older PIIX-alikes to ata_piix and making it enabled only if "all_piixalikes" module parameter is specified.   This way older drivers would be left untouched for now and we can easily get in-tree testing for a new code.  Does it sound as a viable alternative?

Quoted 6 lines

That is the bit to me that makes the least sense. Each of the devices in question is found only in the chipset so they can never be in combination on a board (except multiple PIIX4 which is already covered)

Merging them makes it more likely stuff breaks and increases memory usage
- its a lose/lose situation.

Each device is very similar to other one, so keeping 6 drivers for the (almost) same stuff makes a little sense from the long-term maintainability POV, It is a question of different trade-offs since on the other side of equation we have 5 drivers less, 1400 LOCs less, no risk of losing bugfixes and features (which is true already w/ pata_rdc lacking locking fixes + Power Management and pata_oldpiix lacking parallel scanning feature).

As for increased memory usage -- we are talking here only about 10-20k more. If it really is a problem maybe ata_piix can be redesigned into ata_generic-style manner so with the help of existing config options we can keep code size / memory usage on a existing level.

Thanks,
Bartlomiej

#35 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 4 lines · Bartlomiej Zolnierkiewicz

As for increased memory usage -- we are talking here only about 10-20k more.  If it really is a problem maybe ata_piix can be redesigned into ata_generic-style manner so with the help of existing config options we can keep code size / memory usage on a existing level.

s/ata_generic/pata_legacy/ of course -- what I have in mind is making Intel specific code depended on ATA_PIIX and adding new config option ATA_PIIXALIKE for generic code which would be selected by existing PATA_[EFAR,IT8213, OLDPIIX,RADISYS,RDC] options.

Thanks,
Bartlomiej

#36 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 8 lines · Bartlomiej Zolnierkiewicz
Quoted 4 lines · Bartlomiej Zolnierkiewicz

As for increased memory usage -- we are talking here only about 10-20k more. If it really is a problem maybe ata_piix can be redesigned into ata_generic-style manner so with the help of existing config options we can keep code size / memory usage on a existing level.

Quoted 4 lines

s/ata_generic/pata_legacy/ of course -- what I have in mind is making Intel specific code depended on ATA_PIIX and adding new config option ATA_PIIXALIKE for generic code which would be selected by existing PATA_[EFAR,IT8213, OLDPIIX,RADISYS,RDC] options.

*Draft* patch against my current "piixalike" tree to just show the idea and implement the final conversion stage (after optional code in ata_piix gets tested and we are ready to remove separate drivers).

CONFIG_PATA_EFAR=y only:

before:

   text    data     bss     dec     hex filename
   1578    1008       8    2594     a22 drivers/ata/pata_efar.o

after:

   text    data     bss     dec     hex filename
   2778    1152       8    3938     f62 drivers/ata/ata_piix.o

so it pretty much solves code size / memory usage issue (I'm counting in my other patch https://lkml.org/lkml/2011/2/9/150)..

---
 drivers/ata/Kconfig            |  102 +++---
 drivers/ata/Makefile           |    7 
 drivers/ata/ata_piix.c         |  652 ++++++++++++++++++-----------------------
 drivers/ata/ata_piix_efar.h    |   28 +
 drivers/ata/ata_piix_it8213.h  |   28 +
 drivers/ata/ata_piix_oldpiix.h |   49 +++
 6 files changed, 454 insertions(+), 412 deletions(-)
drivers/ata/Kconfig +56 -46
Index: b/drivers/ata/Kconfig
===================================================================
@@ -167,9 +167,19 @@ if ATA_BMDMA
 
 comment "SATA SFF controllers with BMDMA"
 
+config ATA_PIIXALIKE
+	tristate "Intel PIIX-alikes PATA/SATA support"
+	depends on PCI
+	default y
+	help
+	  This option enables support needed for all Intel PIIX-alike
+	  Serial ATA and PATA host controllers.
+
+	  If unsure, say Y.
+
 config ATA_PIIX
 	tristate "Intel ESB, ICH, PIIX3, PIIX4 PATA/SATA support"
-	depends on PCI
+	depends on ATA_PIIXALIKE
 	help
 	  This option enables support for ICH5/6/7/8 Serial ATA
 	  and support for PATA on the Intel ESB/ICH/PIIX3/PIIX4 series
@@ -177,6 +187,51 @@ config ATA_PIIX
 
 	  If unsure, say N.
 
+config PATA_EFAR
+	bool "EFAR SLC90E66 support"
+	depends on ATA_PIIXALIKE
+	help
+	  This option enables support for the EFAR SLC90E66
+	  IDE controller found on some older machines.
+
+	  If unsure, say N.
+
+config PATA_IT8213
+	bool "IT8213 PATA support (Experimental)"
+	depends on ATA_PIIXALIKE && EXPERIMENTAL
+	help
+	  This option enables support for the ITE 821 PATA
+          controllers via the new ATA layer.
+
+	  If unsure, say N.
+
+config PATA_OLDPIIX
+	bool "Intel PATA old PIIX support"
+	depends on ATA_PIIXALIKE
+	help
+	  This option enables support for early PIIX PATA support.
+
+	  If unsure, say N.
+
+config PATA_RADISYS
+	bool "RADISYS 82600 PATA support (Experimental)"
+	depends on ATA_PIIXALIKE && EXPERIMENTAL
+	help
+	  This option enables support for the RADISYS 82600
+	  PATA controllers via the new ATA layer
+
+	  If unsure, say N.
+
+config PATA_RDC
+	bool "RDC PATA support"
+	depends on ATA_PIIXALIKE
+	help
+	  This option enables basic support for the later RDC PATA controllers
+	  controllers via the new ATA layer. For the RDC 1010, you need to
+	  enable the IT821X driver instead.
+
+	  If unsure, say N.
+
 config SATA_DWC
 	tristate "DesignWare Cores SATA support"
 	depends on 460EX
@@ -372,15 +427,6 @@ config PATA_CYPRESS
 
 	  If unsure, say N.
 
-config PATA_EFAR
-	tristate "EFAR SLC90E66 support"
-	depends on PCI
-	help
-	  This option enables support for the EFAR SLC90E66
-	  IDE controller found on some older machines.
-
-	  If unsure, say N.
-
 config PATA_HPT366
 	tristate "HPT 366/368 PATA support"
 	depends on PCI
@@ -433,15 +479,6 @@ config PATA_ICSIDE
 	  interface card.  This is not required for ICS partition support.
 	  If you are unsure, say N to this.
 
-config PATA_IT8213
-	tristate "IT8213 PATA support (Experimental)"
-	depends on PCI && EXPERIMENTAL
-	help
-	  This option enables support for the ITE 821 PATA
-          controllers via the new ATA layer.
-
-	  If unsure, say N.
-
 config PATA_IT821X
 	tristate "IT8211/2 PATA support"
 	depends on PCI
@@ -518,14 +555,6 @@ config PATA_NS87415
 
 	  If unsure, say N.
 
-config PATA_OLDPIIX
-	tristate "Intel PATA old PIIX support"
-	depends on PCI
-	help
-	  This option enables support for early PIIX PATA support.
-
-	  If unsure, say N.
-
 config PATA_OPTIDMA
 	tristate "OPTI FireStar PATA support (Very Experimental)"
 	depends on PCI && EXPERIMENTAL
@@ -553,25 +582,6 @@ config PATA_PDC_OLD
 
 	  If unsure, say N.
 
-config PATA_RADISYS
-	tristate "RADISYS 82600 PATA support (Experimental)"
-	depends on PCI && EXPERIMENTAL
-	help
-	  This option enables support for the RADISYS 82600
-	  PATA controllers via the new ATA layer
-
-	  If unsure, say N.
-
-config PATA_RDC
-	tristate "RDC PATA support"
-	depends on PCI
-	help
-	  This option enables basic support for the later RDC PATA controllers
-	  controllers via the new ATA layer. For the RDC 1010, you need to
-	  enable the IT821X driver instead.
-
-	  If unsure, say N.
-
 config PATA_SC1200
 	tristate "SC1200 PATA support"
 	depends on PCI
drivers/ata/Makefile +1 -6
Index: b/drivers/ata/Makefile
===================================================================
@@ -16,7 +16,7 @@ obj-$(CONFIG_SATA_QSTOR)	+= sata_qstor.o
 obj-$(CONFIG_SATA_SX4)		+= sata_sx4.o
 
 # SFF SATA w/ BMDMA
-obj-$(CONFIG_ATA_PIIX)		+= ata_piix.o
+obj-$(CONFIG_ATA_PIIXALIKE)	+= ata_piix.o
 obj-$(CONFIG_SATA_MV)		+= sata_mv.o
 obj-$(CONFIG_SATA_NV)		+= sata_nv.o
 obj-$(CONFIG_SATA_PROMISE)	+= sata_promise.o
@@ -40,13 +40,11 @@ obj-$(CONFIG_PATA_CS5530)	+= pata_cs5530
 obj-$(CONFIG_PATA_CS5535)	+= pata_cs5535.o
 obj-$(CONFIG_PATA_CS5536)	+= pata_cs5536.o
 obj-$(CONFIG_PATA_CYPRESS)	+= pata_cypress.o
-obj-$(CONFIG_PATA_EFAR)		+= pata_efar.o
 obj-$(CONFIG_PATA_HPT366)	+= pata_hpt366.o
 obj-$(CONFIG_PATA_HPT37X)	+= pata_hpt37x.o
 obj-$(CONFIG_PATA_HPT3X2N)	+= pata_hpt3x2n.o
 obj-$(CONFIG_PATA_HPT3X3)	+= pata_hpt3x3.o
 obj-$(CONFIG_PATA_ICSIDE)	+= pata_icside.o
-obj-$(CONFIG_PATA_IT8213)	+= pata_it8213.o
 obj-$(CONFIG_PATA_IT821X)	+= pata_it821x.o
 obj-$(CONFIG_PATA_JMICRON)	+= pata_jmicron.o
 obj-$(CONFIG_PATA_MACIO)	+= pata_macio.o
@@ -55,12 +53,9 @@ obj-$(CONFIG_PATA_MPC52xx)	+= pata_mpc52
 obj-$(CONFIG_PATA_NETCELL)	+= pata_netcell.o
 obj-$(CONFIG_PATA_NINJA32)	+= pata_ninja32.o
 obj-$(CONFIG_PATA_NS87415)	+= pata_ns87415.o
-obj-$(CONFIG_PATA_OLDPIIX)	+= pata_oldpiix.o
 obj-$(CONFIG_PATA_OPTIDMA)	+= pata_optidma.o
 obj-$(CONFIG_PATA_PDC2027X)	+= pata_pdc2027x.o
 obj-$(CONFIG_PATA_PDC_OLD)	+= pata_pdc202xx_old.o
-obj-$(CONFIG_PATA_RADISYS)	+= pata_radisys.o
-obj-$(CONFIG_PATA_RDC)		+= pata_rdc.o
 obj-$(CONFIG_PATA_SC1200)	+= pata_sc1200.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_SCH)		+= pata_sch.o
drivers/ata/ata_piix.c +292 -360

775 lines not on this page · raw message

drivers/ata/ata_piix_efar.h new file +28 -0
Index: b/drivers/ata/ata_piix_efar.h
===================================================================
@@ -0,0 +1,28 @@
+#ifdef CONFIG_PATA_EFAR
+/**
+ *	efar_pata_cable_detect	-	check for 40/80 pin
+ *	@ap: Port
+ *
+ *	Perform cable detection for the EFAR ATA interface. This is
+ *	different to the PIIX arrangement
+ */
+
+static int efar_pata_cable_detect(struct ata_port *ap)
+{
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	u8 tmp;
+
+	pci_read_config_byte(pdev, 0x47, &tmp);
+	if (tmp & (2 >> ap->port_no))
+		return ATA_CBL_PATA40;
+	return ATA_CBL_PATA80;
+}
+
+static struct ata_port_operations efar_pata_ops = {
+	.inherits		= &ata_bmdma_port_ops,
+	.set_piomode		= piix_set_piomode,
+	.set_dmamode		= piix_set_dmamode,
+	.prereset		= piix_pata_prereset,
+	.cable_detect		= efar_pata_cable_detect,
+};
+#endif
drivers/ata/ata_piix_it8213.h new file +28 -0
Index: b/drivers/ata/ata_piix_it8213.h
===================================================================
@@ -0,0 +1,28 @@
+#ifdef CONFIG_PATA_IT8213
+/**
+ *	it8213_pata_cable_detect	-	check for 40/80 pin
+ *	@ap: Port
+ *
+ *	Perform cable detection for the 8213 ATA interface. This is
+ *	different to the PIIX arrangement
+ */
+
+static int it8213_pata_cable_detect(struct ata_port *ap)
+{
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	u8 tmp;
+
+	pci_read_config_byte(pdev, 0x42, &tmp);
+	if (tmp & 2)	/* The initial docs are incorrect */
+		return ATA_CBL_PATA40;
+	return ATA_CBL_PATA80;
+}
+
+static struct ata_port_operations it8213_pata_ops = {
+	.inherits		= &ata_bmdma_port_ops,
+	.set_piomode		= piix_set_piomode,
+	.set_dmamode		= ich_set_dmamode,
+	.prereset		= piix_pata_prereset,
+	.cable_detect		= it8213_pata_cable_detect,
+};
+#endif
drivers/ata/ata_piix_oldpiix.h new file +49 -0

49 lines not on this page · raw message

#37 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

In reply to Bartlomiej Zolnierkiewicz
Quoted 4 lines

on the other side of equation we have 5 drivers less, 1400 LOCs less, no risk of losing bugfixes and features (which is true already w/ pata_rdc lacking locking fixes + Power Management and pata_oldpiix lacking parallel scanning feature).

The parallel scanning feature is somewhat irrelevant on an original PIIX, if the hardware even supports it which is somewhat of an unknown. If you have a minor boot time performance concern you'd buy a real computer instead.

And the other stuff you actually create a problem rather than solving one
- the difficulting testing the rare chips makes it harder to fix ata_piix bugs and do full testing.

Alan

#38 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 5 lines · Alan Cox
Quoted 4 lines

on the other side of equation we have 5 drivers less, 1400 LOCs less, no risk of losing bugfixes and features (which is true already w/ pata_rdc lacking locking fixes + Power Management and pata_oldpiix lacking parallel scanning feature).

Quoted 8 lines

The parallel scanning feature is somewhat irrelevant on an original PIIX, if the hardware even supports it which is somewhat of an unknown. If you have a minor boot time performance concern you'd buy a real computer instead.

And the other stuff you actually create a problem rather than solving one
- the difficulting testing the rare chips makes it harder to fix ata_piix bugs and do full testing.

We don't have to insist on merging all PIIX-alikes (like oldPIIX and Radisys) support to ata_piix, i.e. for RDC it looks much cleaner:

per patch from the original patch series:

 4 files changed, 13 insertions(+), 387 deletions(-)

pata_rdc driver is nearly identical to ata_piix PATA support (modulo bugs in pata_rdc and SATA stuff in ata_piix)

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ata_piix: add RDC support

Add RDC PATA support to ata_piix and remove no longer needed pata_rdc driver.

---
 drivers/ata/Kconfig    |    1 
 drivers/ata/Makefile   |    1 
 drivers/ata/ata_piix.c |   14 +
 drivers/ata/pata_rdc.c |  384 -------------------------------------------------
 4 files changed, 13 insertions(+), 387 deletions(-)
drivers/ata/Kconfig +1 -0
Index: b/drivers/ata/Kconfig
===================================================================
@@ -567,6 +567,7 @@ config PATA_RADISYS
 config PATA_RDC
 	tristate "RDC PATA support"
 	depends on PCI
+	select ATA_PIIX
 	help
 	  This option enables basic support for the later RDC PATA controllers
 	  controllers via the new ATA layer. For the RDC 1010, you need to
drivers/ata/Makefile +0 -1
Index: b/drivers/ata/Makefile
===================================================================
@@ -58,7 +58,6 @@ obj-$(CONFIG_PATA_OPTIDMA)	+= pata_optid
 obj-$(CONFIG_PATA_PDC2027X)	+= pata_pdc2027x.o
 obj-$(CONFIG_PATA_PDC_OLD)	+= pata_pdc202xx_old.o
 obj-$(CONFIG_PATA_RADISYS)	+= pata_radisys.o
-obj-$(CONFIG_PATA_RDC)		+= pata_rdc.o
 obj-$(CONFIG_PATA_SC1200)	+= pata_sc1200.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_SCH)		+= pata_sch.o
drivers/ata/ata_piix.c +12 -2
Index: b/drivers/ata/ata_piix.c
===================================================================
@@ -1,6 +1,9 @@
 /*
  *    ata_piix.c - Intel PATA/SATA controllers
  *
+ *    This is actually a driver for hardware meeting
+ *    INCITS 370-2004 (1510D): ATA Host Adapter Standards
+ *
  *    Maintained by:  Jeff Garzik <[email protected]>
  *    		    Please ALWAYS copy [email protected]
  *		    on emails.
@@ -255,6 +258,10 @@ static const struct pci_device_id piix_p
 	/* IT8213 */
 	{ 0x1283, 0x8213, PCI_ANY_ID, PCI_ANY_ID, 0, 0, it8213_pata },
 
+	/* RDC is ICH like */
+	{ 0x17F3, 0x1011, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+	{ 0x17F3, 0x1012, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+
 	/* SATA ports */
 	
 	/* 82801EB (ICH5) */
@@ -671,6 +678,7 @@ MODULE_DEVICE_TABLE(pci, piix_pci_tbl);
 MODULE_VERSION(DRV_VERSION);
 MODULE_ALIAS("pata_efar");
 MODULE_ALIAS("pata_it8213");
+MODULE_ALIAS("pata_rdc");
 
 struct ich_laptop {
 	u16 device;
@@ -1676,7 +1684,8 @@ static int __devinit piix_init_one(struc
 	 * because some ACPI implementations mess up cable related
 	 * bits on _STM.  Reported on kernel bz#11879.
 	 */
-	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL ||
+	    pdev->vendor == PCI_VENDOR_ID_RDC)
 		pci_read_config_dword(pdev, PIIX_IOCFG, &hpriv->saved_iocfg);
 
 	/* ICH6R may be driven by either ata_piix or ahci driver
@@ -1741,7 +1750,8 @@ static void piix_remove_one(struct pci_d
 	struct ata_host *host = dev_get_drvdata(&pdev->dev);
 	struct piix_host_priv *hpriv = host->private_data;
 
-	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL ||
+	    pdev->vendor == PCI_VENDOR_ID_RDC)
 		pci_write_config_dword(pdev, PIIX_IOCFG, hpriv->saved_iocfg);
 
 	ata_pci_remove_one(pdev);
drivers/ata/pata_rdc.c deleted +0 -384

384 lines not on this page · raw message

#39 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Quoted 5 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ata_piix: add RDC support

Add RDC PATA support to ata_piix and remove no longer needed pata_rdc driver.

Now that one does make sense given the similarity. I'll go over both later today and see if I agree they match this way.

#40 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

In reply to Bartlomiej Zolnierkiewicz

drivers/ata/ata_piix.c static const struct pci_device_id piix_p · line 264

Quoted 4 lines
drivers/ata/ata_piix.c +4 -0
+	/* RDC is ICH like */
+	{ 0x17F3, 0x1011, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+	{ 0x17F3, 0x1012, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+

Cable detect is now a bit iffy - probably should add the device vendor to the table for each if you do this.

Other than that it this specific one could be pretty sensible - but it does makes the drivers bigger especially on an embedded RDC device.

#41 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

In reply to Bartlomiej Zolnierkiewicz

Hello.

On 08-02-2011 15:24, Bartlomiej Zolnierkiewicz wrote:
[...]

Quoted 6 lines

We may need to set SITRE before programming slave_data.

This makes pata_efar match the behavior of IDE's slc90e66 host driver and also of libata's ata_piix one.

[...]

drivers/ata/pata_efar.c static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, · line 77

Quoted 9 lines
drivers/ata/pata_efar.c +1 -0
index 1e2ff7d..7f564d7 100644
@@ -74,10 +74,12 @@ static void efar_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 int is_slave	= (adev->devno != 0);

What's the point of this variable? To save one pointer dereference? :-)

Quoted 9 lines
drivers/ata/pata_efar.c +1 -0
  	unsigned long flags;
  	u8 master_port		= ap->port_no ? 0x42 : 0x40;
  	u16 master_data;
  	u8 udma_enable;
+	u8 slave_data;
  	int control = 0;
 
  	/*

drivers/ata/pata_efar.c static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, · line 115

Quoted 5 lines
drivers/ata/pata_efar.c +1 -1
@@ -110,14 +112,13 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
  	pci_read_config_word(dev, master_port,&master_data);
 
  	/* Set PPE, IE, and TIME as appropriate */
-	if (adev->devno == 0) {
+	if (is_slave == 0) {

!is_slave appeals more to me. :-)

Quoted 6 lines
drivers/ata/pata_efar.c +0 -1
@@ -126,12 +127,14 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
  		pci_read_config_byte(dev, 0x44,&slave_data);
  		slave_data&= ap->port_no ? 0x0F : 0xF0;
  		slave_data |= ((timings[pio][0]<<  2) | timings[pio][1])<<  shift;
-		pci_write_config_byte(dev, 0x44, slave_data);
  	}

drivers/ata/pata_efar.c static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, · line 140

Quoted 10 lines
drivers/ata/pata_efar.c +3 -0
 
  	master_data |= 0x4000;	/* Ensure SITRE is set */
  	pci_write_config_word(dev, master_port, master_data);
 
+	if (is_slave)
+		pci_write_config_byte(dev, 0x44, slave_data);
+
  	pci_read_config_byte(dev, 0x48,&udma_enable);
  	udma_enable&= ~(1<<  (2 * ap->port_no + adev->devno));
  	pci_write_config_byte(dev, 0x48, udma_enable);

WBR, Sergei

#42 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Hi,

Quoted 14 lines · Sergei Shtylyov

Hello.

On 08-02-2011 15:24, Bartlomiej Zolnierkiewicz wrote:
[...]

Quoted 6 lines

We may need to set SITRE before programming slave_data.

This makes pata_efar match the behavior of IDE's slc90e66 host driver and also of libata's ata_piix one.

[...]

Quoted 12 lines

drivers/ata/pata_efar.c static void efar_set_timings(struct ata_port *ap, · line 78

Quoted 10 lines
drivers/ata/pata_efar.c +1 -0
index 1e2ff7d..7f564d7 100644
@@ -74,10 +74,12 @@ static void efar_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 int is_slave   = (adev->devno != 0);

  What's the point of this variable? To save one pointer dereference? :-)

Make code more similar to ata_piix.c and thus easier for comparisons through 'diff -ub'.

In reality it doesn't matter now that much as pata_efar (same for pata_oldpiix) vanishes completely at the end of the patch series.. :-)

Thanks,
Bartlomiej

#43 lore raw

Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

Hello.

Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 10 lines · Bartlomiej Zolnierkiewicz

[...]

Quoted 5 lines

We may need to set SITRE before programming slave_data. This makes pata_efar match the behavior of IDE's slc90e66 host driver and also of libata's ata_piix one.

[...]

Quoted 12 lines

drivers/ata/pata_efar.c static void efar_set_timings(struct ata_port *ap, · line 78

Quoted 10 lines
drivers/ata/pata_efar.c +1 -0
index 1e2ff7d..7f564d7 100644
@@ -74,10 +74,12 @@ static void efar_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 int is_slave   = (adev->devno != 0);

What's the point of this variable? To save one pointer dereference? :-)

Quoted 5 lines

Make code more similar to ata_piix.c and thus easier for comparisons through 'diff -ub'.

In reality it doesn't matter now that much as pata_efar (same for pata_oldpiix) vanishes completely at the end of the patch series.. :-)

Yeah, I understood that. Just don't have time to review the full series yet.

Quoted 2 lines

Thanks,
Bartlomiej

WBR, Sergei

#44 lore raw

PATCH 6/20 pata_it8213: fix register naming used in it8213_set_piomode()

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From fd377660531ca5b5d0802fa282ae7c964d0fddd7 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:25 +0100
Subject: [PATCH 06/20] pata_it8213: fix register naming used in it8213_set_piomode()

Rename 'idetm_port' and 'idetm_data' variables to 'master_port' and 'master_data' respectively to match register naming used in it8213_set_dma_mode() and in ata_piix.c.

Fix 'master_port' type while at it.

---
 drivers/ata/pata_it8213.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)
drivers/ata/pata_it8213.c +10 -10
index 4d142a2..911f50b 100644
@@ -76,8 +76,8 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
 {
 	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	unsigned int idetm_port= ap->port_no ? 0x42 : 0x40;
-	u16 idetm_data;
+	u8 master_port		= ap->port_no ? 0x42 : 0x40;
+	u16 master_data;
 	int control = 0;
 
 	/*
@@ -100,19 +100,19 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->class != ATA_DEV_ATA)
 		control |= 4;	/* PPE */
 
-	pci_read_config_word(dev, idetm_port, &idetm_data);
+	pci_read_config_word(dev, master_port, &master_data);
 
 	/* Set PPE, IE, and TIME as appropriate */
 	if (adev->devno == 0) {
-		idetm_data &= 0xCCF0;
-		idetm_data |= control;
-		idetm_data |= (timings[pio][0] << 12) |
+		master_data &= 0xCCF0;
+		master_data |= control;
+		master_data |= (timings[pio][0] << 12) |
 			(timings[pio][1] << 8);
 	} else {
 		u8 slave_data;
 
-		idetm_data &= 0xFF0F;
-		idetm_data |= (control << 4);
+		master_data &= 0xFF0F;
+		master_data |= (control << 4);
 
 		/* Slave timing in separate register */
 		pci_read_config_byte(dev, 0x44, &slave_data);
@@ -121,8 +121,8 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		pci_write_config_byte(dev, 0x44, slave_data);
 	}
 
-	idetm_data |= 0x4000;	/* Ensure SITRE is set */
-	pci_write_config_word(dev, idetm_port, idetm_data);
+	master_data |= 0x4000;	/* Ensure SITRE is set */
+	pci_write_config_word(dev, master_port, master_data);
 }
 
 /**
Signature · 1 line
1.7.1
#45 lore raw

PATCH 7/20 pata_it8213: unify code for programming PIO and MWDMA timings

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 57632718f2e761e69b96e14f6900a487bad5d586 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:26 +0100
Subject: [PATCH 07/20] pata_it8213: unify code for programming PIO and MWDMA timings

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

---
 drivers/ata/pata_it8213.c |   89 +++++++++++++++------------------------------
 1 files changed, 30 insertions(+), 59 deletions(-)
drivers/ata/pata_it8213.c +30 -59
index 911f50b..ccfd1ae 100644
@@ -61,20 +61,9 @@ static int it8213_cable_detect(struct ata_port *ap)
 	return ATA_CBL_PATA80;
 }
 
-/**
- *	it8213_set_piomode - Initialize host controller PATA PIO timings
- *	@ap: Port whose timings we are configuring
- *	@adev: Device whose timings we are configuring
- *
- *	Set PIO mode for device, in host controller PCI config space.
- *
- *	LOCKING:
- *	None (inherited from caller).
- */
-
-static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
+static void it8213_set_timings(struct ata_port *ap, struct ata_device *adev,
+			       u8 pio, bool use_mwdma)
 {
-	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	u8 master_port		= ap->port_no ? 0x42 : 0x40;
 	u16 master_data;
@@ -92,13 +81,18 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
 			    { 2, 1 },
 			    { 2, 3 }, };
 
-	if (pio > 1)
+	if (pio > 1 || use_mwdma)
 		control |= 1;	/* TIME */
-	if (ata_pio_need_iordy(adev))	/* PIO 3/4 require IORDY */
+	if (ata_pio_need_iordy(adev) || use_mwdma)
 		control |= 2;	/* IE */
 	/* Bit 2 is set for ATAPI on the IT8213 - reverse of ICH/PIIX */
 	if (adev->class != ATA_DEV_ATA)
 		control |= 4;	/* PPE */
+	/* 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 */
 
 	pci_read_config_word(dev, master_port, &master_data);
 
@@ -126,6 +120,22 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
 }
 
 /**
+ *	it8213_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: Port whose timings we are configuring
+ *	@adev: Device whose timings we are configuring
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void it8213_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	it8213_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);
+}
+
+/**
  *	it8213_set_dmamode - Initialize host controller PATA DMA timings
  *	@ap: Port whose timings we are configuring
  *	@adev: Device to program
@@ -140,23 +150,14 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
 static void it8213_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	u16 master_data;
 	u8 speed		= adev->dma_mode;
 	int devid		= adev->devno;
 	u8 udma_enable;
 
-	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },
-			    { 0, 0 },
-			    { 1, 0 },
-			    { 2, 1 },
-			    { 2, 3 }, };
-
-	pci_read_config_word(dev, 0x40, &master_data);
 	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;
@@ -184,46 +185,16 @@ static void it8213_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 		ideconf |= u_clock << devid;
 		pci_write_config_word(dev, 0x54, ideconf);
 	} 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;
 		static 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 (devid) {	/* Slave */
-			master_data &= 0xFF4F;  /* Mask out IORDY|TIME1|DMAONLY */
-			master_data |= control << 4;
-			pci_read_config_byte(dev, 0x44, &slave_data);
-			slave_data &= 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);
-		}
+		it8213_set_timings(ap, adev, pio, 1);
+
 		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, 0x40, master_data);
 	}
 	pci_write_config_byte(dev, 0x48, udma_enable);
 }
Signature · 1 line
1.7.1
#46 lore raw

Re: [PATCH 07/20] pata_it8213: unify code for programming PIO and MWDMA timings

Hello.

Quoted 10 lines · Bartlomiej Zolnierkiewicz

From 57632718f2e761e69b96e14f6900a487bad5d586 Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz<[email protected]>
Date: Tue, 8 Feb 2011 12:39:26 +0100
Subject: [PATCH 07/20] pata_it8213: unify code for programming PIO and MWDMA timings

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

[...]

Quoted 4 lines
drivers/ata/pata_it8213.c +0 -0
index 911f50b..ccfd1ae 100644

[...]

Quoted 5 lines
drivers/ata/pata_it8213.c +0 -1
@@ -92,13 +81,18 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
  			    { 2, 1 },
  			    { 2, 3 }, };
 
-	if (pio > 1)

drivers/ata/pata_it8213.c static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 92

Quoted 10 lines
drivers/ata/pata_it8213.c +4 -1
+	if (pio > 1 || use_mwdma)
  		control |= 1;	/* TIME */
-	if (ata_pio_need_iordy(adev))	/* PIO 3/4 require IORDY */
+	if (ata_pio_need_iordy(adev) || use_mwdma)
  		control |= 2;	/* IE */
  	/* Bit 2 is set for ATAPI on the IT8213 - reverse of ICH/PIIX */
  	if (adev->class != ATA_DEV_ATA)
  		control |= 4;	/* PPE */
+	/* If the drive MWDMA is faster than it can do PIO then
+	   we must force PIO into PIO0 */

Fix the comment style, please.

drivers/ata/pata_it8213.c static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 93

Quoted 1 line
drivers/ata/pata_it8213.c +1 -0
+	if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))

Parens not needed around +.

Quoted 12 lines
drivers/ata/pata_it8213.c +5 -0
+		/* Enable DMA timing only */
+		control |= 8;	/* PIO cycles in PIO0 */
 
  	pci_read_config_word(dev, master_port,&master_data);
 
@@ -126,6 +120,22 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
  }
 
  /**
+ *	it8213_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: Port whose timings we are configuring
+ *	@adev: Device whose timings we are configuring

drivers/ata/pata_it8213.c static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 135

Quoted 10 lines
drivers/ata/pata_it8213.c +10 -0
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void it8213_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	it8213_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);
    s/0/false/?
Quoted 8 lines
drivers/ata/pata_it8213.c +0 -1
@@ -140,23 +150,14 @@ static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev)
  static void it8213_set_dmamode (struct ata_port *ap, struct ata_device *adev)
  {
  	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	u16 master_data;
  	u8 speed		= adev->dma_mode;
  	int devid		= adev->devno;
  	u8 udma_enable;

drivers/ata/pata_it8213.c static void it8213_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 157

Quoted 10 lines
drivers/ata/pata_it8213.c +0 -8
 
-	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },
-			    { 0, 0 },
-			    { 1, 0 },
-			    { 2, 1 },
-			    { 2, 3 }, };
-
-	pci_read_config_word(dev, 0x40,&master_data);
  	pci_read_config_byte(dev, 0x48,&udma_enable);

Strange, here you're not moving that read and nnot removing disabling of
UDMA -- is that because of the missing lock?

Quoted 1 line
drivers/ata/pata_it8213.c +0 -0
@@ -184,46 +185,16 @@ static void it8213_set_dmamode (struct ata_port *ap, struct ata_device *adev)

[...]

drivers/ata/pata_it8213.c static void it8213_set_dmamode (struct ata_port *ap, struct ata_device *adev) · line 195

Quoted 1 line
drivers/ata/pata_it8213.c +1 -0
+		it8213_set_timings(ap, adev, pio, 1);

s/1/true/?

drivers/ata/pata_it8213.c static void it8213_set_dmamode (struct ata_port *ap, struct ata_device *adev) · line 200

Quoted 6 lines
drivers/ata/pata_it8213.c +1 -1
+
  		udma_enable&= ~(1 << devid);
-		pci_write_config_word(dev, 0x40, master_data);
  	}
  	pci_write_config_byte(dev, 0x48, udma_enable);
  }

WBR, Sergei

#47 lore raw

PATCH 8/20 pata_it8213: add UDMA100 and UDMA133 support

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From d7d8c9c8372230ec596814ca3281564156f94926 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:26 +0100
Subject: [PATCH 08/20] pata_it8213: add UDMA100 and UDMA133 support

Fixes IDE -> libata regression.

IDE's it8213 host driver has been supporting those modes (per official documentation) since the initial driver's merge on Feb 7 2007 (commit 9c6712c0 "ide: add it8213 IDE driver").

---
 drivers/ata/pata_it8213.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/ata/pata_it8213.c +2 -2
index ccfd1ae..e045eab 100644
@@ -164,7 +164,7 @@ static void it8213_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 
 		/* Clocks follow the PIIX style */
 		u_speed = min(2 - (udma & 1), udma);
-		if (udma == 5)
+		if (udma > 4)
 			u_clock = 0x1000;	/* 100Mhz */
 		else if (udma > 2)
 			u_clock = 1;		/* 66Mhz */
@@ -234,7 +234,7 @@ static int it8213_init_one (struct pci_dev *pdev, const struct pci_device_id *en
 		.flags		= ATA_FLAG_SLAVE_POSS,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY,
-		.udma_mask 	= ATA_UDMA4, /* FIXME: want UDMA 100? */
+		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &it8213_ops,
 	};
 	/* Current IT8213 stuff is single port */
Signature · 1 line
1.7.1
#48 lore raw

PATCH 9/20 pata_oldpiix: unify code for programming PIO and MWDMA timings

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 30f6c60ad1e2f9c640352b74279fdf8a930847bf Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:26 +0100
Subject: [PATCH 09/20] pata_oldpiix: unify code for programming PIO and MWDMA timings

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

---
 drivers/ata/pata_oldpiix.c |   88 +++++++++++++------------------------------
 1 files changed, 27 insertions(+), 61 deletions(-)
drivers/ata/pata_oldpiix.c +27 -61
index b811c16..433d2fc 100644
@@ -50,20 +50,9 @@ static int oldpiix_pre_reset(struct ata_link *link, unsigned long deadline)
 	return ata_sff_prereset(link, deadline);
 }
 
-/**
- *	oldpiix_set_piomode - Initialize host controller PATA PIO timings
- *	@ap: Port whose timings we are configuring
- *	@adev: Device whose timings we are configuring
- *
- *	Set PIO mode for device, in host controller PCI config space.
- *
- *	LOCKING:
- *	None (inherited from caller).
- */
-
-static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev)
+static void oldpiix_set_timings(struct ata_port *ap, struct ata_device *adev,
+				u8 pio, bool use_mwdma)
 {
-	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	unsigned int idetm_port= ap->port_no ? 0x42 : 0x40;
 	u16 idetm_data;
@@ -82,14 +71,18 @@ static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 			    { 2, 1 },
 			    { 2, 3 }, };
 
-	if (pio > 1)
+	if (pio > 1 || use_mwdma)
 		control |= 1;	/* TIME */
-	if (ata_pio_need_iordy(adev))
+	if (ata_pio_need_iordy(adev) || use_mwdma)
 		control |= 2;	/* IE */
-
 	/* Intel specifies that the prefetch/posting is for disk only */
 	if (adev->class == ATA_DEV_ATA)
 		control |= 4;	/* PPE */
+	/* 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 */
 
 	pci_read_config_word(dev, idetm_port, &idetm_data);
 
@@ -113,6 +106,22 @@ static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 }
 
 /**
+ *	oldpiix_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: Port whose timings we are configuring
+ *	@adev: Device whose timings we are configuring
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void oldpiix_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	oldpiix_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);
+}
+
+/**
  *	oldpiix_set_dmamode - Initialize host controller PATA DMA timings
  *	@ap: Port whose timings we are configuring
  *	@adev: Device to program
@@ -125,58 +134,15 @@ static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev)
 
 static void oldpiix_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 {
-	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	u8 idetm_port		= ap->port_no ? 0x42 : 0x40;
-	u16 idetm_data;
-
-	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },
-			    { 0, 0 },
-			    { 1, 0 },
-			    { 2, 1 },
-			    { 2, 3 }, };
-
-	/*
-	 * 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.
-	 */
+	/* MWDMA is driven by the PIO timings. */
 
 	unsigned int mwdma	= adev->dma_mode - XFER_MW_DMA_0;
-	unsigned int control;
 	const unsigned int needed_pio[3] = {
 		XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
 	};
 	int pio = needed_pio[mwdma] - XFER_PIO_0;
 
-	pci_read_config_word(dev, idetm_port, &idetm_data);
-
-	control = 3;	/* IORDY|TIME0 */
-	/* 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 (adev->pio_mode < needed_pio[mwdma])
-		/* Enable DMA timing only */
-		control |= 8;	/* PIO cycles in PIO0 */
-
-	/* Mask out the relevant control and timing bits we will load. Also
-	   clear the other drive TIME register as a precaution */
-	if (adev->devno == 0) {
-		idetm_data &= 0xCCE0;
-		idetm_data |= control;
-	} else {
-		idetm_data &= 0xCC0E;
-		idetm_data |= (control << 4);
-	}
-	idetm_data |= (timings[pio][0] << 12) | (timings[pio][1] << 8);
-	pci_write_config_word(dev, idetm_port, idetm_data);
-
-	/* Track which port is configured */
-	ap->private_data = adev;
+	oldpiix_set_timings(ap, adev, pio, 1);
 }
 
 /**
Signature · 1 line
1.7.1
#49 lore raw

Re: [PATCH 09/20] pata_oldpiix: unify code for programming PIO and MWDMA timings

Hello.

Quoted 10 lines · Bartlomiej Zolnierkiewicz

From 30f6c60ad1e2f9c640352b74279fdf8a930847bf Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz<[email protected]>
Date: Tue, 8 Feb 2011 12:39:26 +0100
Subject: [PATCH 09/20] pata_oldpiix: unify code for programming PIO and MWDMA timings

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

[...]

Quoted 4 lines
drivers/ata/pata_oldpiix.c +0 -0
index b811c16..433d2fc 100644

[...]

drivers/ata/pata_oldpiix.c static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 74

Quoted 6 lines
drivers/ata/pata_oldpiix.c +1 -1
@@ -82,14 +71,18 @@ static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev)
  			    { 2, 1 },
  			    { 2, 3 }, };
 
-	if (pio>  1)
+	if (pio > 1 || use_mwdma)

'use_mwdma' check is superfluous...

drivers/ata/pata_oldpiix.c static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 82

Quoted 10 lines
drivers/ata/pata_oldpiix.c +3 -2
  		control |= 1;	/* TIME */
-	if (ata_pio_need_iordy(adev))
+	if (ata_pio_need_iordy(adev) || use_mwdma)
  		control |= 2;	/* IE */
-
  	/* Intel specifies that the prefetch/posting is for disk only */
  	if (adev->class == ATA_DEV_ATA)
  		control |= 4;	/* PPE */
+	/* If the drive MWDMA is faster than it can do PIO then
+	   we must force PIO into PIO0 */

Please fix the comment style.

drivers/ata/pata_oldpiix.c static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 83

Quoted 1 line
drivers/ata/pata_oldpiix.c +1 -0
+	if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))

Parens not needed around +.

Quoted 12 lines
drivers/ata/pata_oldpiix.c +5 -0
+		/* Enable DMA timing only */
+		control |= 8;	/* PIO cycles in PIO0 */
 
  	pci_read_config_word(dev, idetm_port,&idetm_data);
 
@@ -113,6 +106,22 @@ static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev)
  }
 
  /**
+ *	oldpiix_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: Port whose timings we are configuring
+ *	@adev: Device whose timings we are configuring

drivers/ata/pata_oldpiix.c static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 121

Quoted 10 lines
drivers/ata/pata_oldpiix.c +10 -0
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void oldpiix_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	oldpiix_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);

s/0/false/?

Quoted 7 lines
drivers/ata/pata_oldpiix.c +3 -0
+}
+
+/**
   *	oldpiix_set_dmamode - Initialize host controller PATA DMA timings
   *	@ap: Port whose timings we are configuring
   *	@adev: Device to program
@@ -125,58 +134,15 @@ static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev)

[...]

drivers/ata/pata_oldpiix.c static void oldpiix_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 145

Quoted 1 line
drivers/ata/pata_oldpiix.c +1 -0
+	oldpiix_set_timings(ap, adev, pio, 1);

s/1/true/?

WBR, Sergei

#50 lore raw

PATCH 10/20 pata_radisys: unify code for programming PIO and MWDMA timings

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 656a1d1e17b035c7de7e7ea602f03724fdcbb336 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:26 +0100
Subject: [PATCH 10/20] pata_radisys: unify code for programming PIO and MWDMA timings

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

---
 drivers/ata/pata_radisys.c |   66 ++++++++++++++++---------------------------
 1 files changed, 25 insertions(+), 41 deletions(-)
drivers/ata/pata_radisys.c +25 -41
index 8574b31..e85bb38 100644
@@ -26,20 +26,9 @@
 #define DRV_NAME	"pata_radisys"
 #define DRV_VERSION	"0.4.4"
 
-/**
- *	radisys_set_piomode - Initialize host controller PATA PIO timings
- *	@ap: ATA port
- *	@adev: Device whose timings we are configuring
- *
- *	Set PIO mode for device, in host controller PCI config space.
- *
- *	LOCKING:
- *	None (inherited from caller).
- */
-
-static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
+static void radisys_set_timings(struct ata_port *ap, struct ata_device *adev,
+				u8 pio, bool use_mwdma)
 {
-	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	u16 idetm_data;
 	int control = 0;
@@ -52,7 +41,7 @@ static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
 	 */
 
 	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },	/* Check me */
+	u8 timings[][2]	= { { 0, 0 },
 			    { 0, 0 },
 			    { 1, 1 },
 			    { 2, 2 },
@@ -62,6 +51,10 @@ static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
 		control |= 1;	/* TIME1 enable */
 	if (ata_pio_need_iordy(adev))
 		control |= 2;	/* IE IORDY */
+	/* If the drive MWDMA is faster than it can do PIO then
+	   we must force PIO0 for PIO cycles. */
+	if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))
+		control = 1;
 
 	pci_read_config_word(dev, 0x40, &idetm_data);
 
@@ -78,6 +71,22 @@ static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
 }
 
 /**
+ *	radisys_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: ATA port
+ *	@adev: Device whose timings we are configuring
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void radisys_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	radisys_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);
+}
+
+/**
  *	radisys_set_dmamode - Initialize host controller PATA DMA timings
  *	@ap: Port whose timings we are configuring
  *	@adev: Device to program
@@ -91,22 +100,10 @@ static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
 static void radisys_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	u16 idetm_data;
 	u8 udma_enable;
 
-	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },
-			    { 0, 0 },
-			    { 1, 1 },
-			    { 2, 2 },
-			    { 3, 3 }, };
-
-	/*
-	 * MWDMA is driven by the PIO timings. We must also enable
-	 * IORDY unconditionally.
-	 */
+	/* MWDMA is driven by the PIO timings. */
 
-	pci_read_config_word(dev, 0x40, &idetm_data);
 	pci_read_config_byte(dev, 0x48, &udma_enable);
 
 	if (adev->dma_mode < XFER_UDMA_0) {
@@ -115,20 +112,8 @@ static void radisys_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 			XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
 		};
 		int pio = needed_pio[mwdma] - XFER_PIO_0;
-		int control = 3;	/* IORDY|TIME0 */
-
-		/* If the drive MWDMA is faster than it can do PIO then
-		   we must force PIO0 for PIO cycles. */
-
-		if (adev->pio_mode < needed_pio[mwdma])
-			control = 1;
 
-		/* Mask out the relevant control and timing bits we will load. Also
-		   clear the other drive TIME register as a precaution */
-
-		idetm_data &= 0xCCCC;
-		idetm_data |= control << (4 * adev->devno);
-		idetm_data |= (timings[pio][0] << 12) | (timings[pio][1] << 8);
+		radisys_set_timings(ap, adev, pio, 1);
 
 		udma_enable &= ~(1 << adev->devno);
 	} else {
@@ -147,7 +132,6 @@ static void radisys_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 
 		udma_enable |= (1 << adev->devno);
 	}
-	pci_write_config_word(dev, 0x40, idetm_data);
 	pci_write_config_byte(dev, 0x48, udma_enable);
 
 	/* Track which port is configured */
Signature · 1 line
1.7.1
#51 lore raw

Re: [PATCH 10/20] pata_radisys: unify code for programming PIO and MWDMA timings

Hello.

Quoted 10 lines · Bartlomiej Zolnierkiewicz

From 656a1d1e17b035c7de7e7ea602f03724fdcbb336 Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz<[email protected]>
Date: Tue, 8 Feb 2011 12:39:26 +0100
Subject: [PATCH 10/20] pata_radisys: unify code for programming PIO and MWDMA timings

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

[...]

Quoted 4 lines
drivers/ata/pata_radisys.c +0 -0
index 8574b31..e85bb38 100644

[...]

drivers/ata/pata_radisys.c static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 55

Quoted 6 lines
drivers/ata/pata_radisys.c +2 -0
@@ -62,6 +51,10 @@ static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
  		control |= 1;	/* TIME1 enable */
  	if (ata_pio_need_iordy(adev))
  		control |= 2;	/* IE IORDY */
+	/* If the drive MWDMA is faster than it can do PIO then
+	   we must force PIO0 for PIO cycles. */

Fix the comment style please.

drivers/ata/pata_radisys.c static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 57

Quoted 2 lines
drivers/ata/pata_radisys.c +2 -0
+	if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio))
+		control = 1;

Wait, this bit is enabling fast timings for both PIO and DMA, no?

Quoted 7 lines
drivers/ata/pata_radisys.c +3 -0
@@ -78,6 +71,22 @@ static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
  }
 
  /**
+ *	radisys_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: ATA port
+ *	@adev: Device whose timings we are configuring

drivers/ata/pata_radisys.c static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 86

Quoted 10 lines
drivers/ata/pata_radisys.c +10 -0
+ *
+ *	Set PIO mode for device, in host controller PCI config space.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void radisys_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	radisys_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);

s/0/false/?

Quoted 12 lines
drivers/ata/pata_radisys.c +0 -6
@@ -91,22 +100,10 @@ static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev)
  static void radisys_set_dmamode (struct ata_port *ap, struct ata_device *adev)
  {
  	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	u16 idetm_data;
  	u8 udma_enable;
 
-	static const	 /* ISP  RTC */
-	u8 timings[][2]	= { { 0, 0 },
-			    { 0, 0 },
-			    { 1, 1 },
-			    { 2, 2 },

drivers/ata/pata_radisys.c static void radisys_set_piomode (struct ata_port *ap, struct ata_device *adev) · line 107

Quoted 10 lines
drivers/ata/pata_radisys.c +1 -7
-			    { 3, 3 }, };
-
-	/*
-	 * MWDMA is driven by the PIO timings. We must also enable
-	 * IORDY unconditionally.
-	 */
+	/* MWDMA is driven by the PIO timings. */
 
-	pci_read_config_word(dev, 0x40,&idetm_data);
  	pci_read_config_byte(dev, 0x48,&udma_enable);

You're not touching this read here also...

Quoted 1 line
drivers/ata/pata_radisys.c +0 -0
@@ -115,20 +112,8 @@ static void radisys_set_dmamode (struct ata_port *ap, struct ata_device *adev)

[...]

drivers/ata/pata_radisys.c static void radisys_set_dmamode (struct ata_port *ap, struct ata_device *adev) · line 116

Quoted 1 line
drivers/ata/pata_radisys.c +1 -0
+		radisys_set_timings(ap, adev, pio, 1);

s/1/true/?

WBR, Sergei

#52 lore raw

PATCH 11/20 pata_rdc: unify code for programming PIO and MWDMA timings

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 0bde1b0fa0d0f05659c147a204d3e4d16c05c276 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:27 +0100
Subject: [PATCH 11/20] pata_rdc: unify code for programming PIO and MWDMA timings

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

Fix rdc_set_piomode() documentation while at it.

---
 drivers/ata/pata_rdc.c |   99 ++++++++++++++++--------------------------------
 1 files changed, 33 insertions(+), 66 deletions(-)
drivers/ata/pata_rdc.c +33 -66
index 5fbe9b1..d32b252 100644
@@ -86,20 +86,9 @@ static int rdc_pata_prereset(struct ata_link *link, unsigned long deadline)
 	return ata_sff_prereset(link, deadline);
 }
 
-/**
- *	rdc_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 rdc_set_piomode(struct ata_port *ap, struct ata_device *adev)
+static void rdc_set_timings(struct ata_port *ap, struct ata_device *adev,
+			    u8 pio, bool use_mwdma)
 {
-	unsigned int pio	= adev->pio_mode - XFER_PIO_0;
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	unsigned int is_slave	= (adev->devno != 0);
 	unsigned int master_port= ap->port_no ? 0x42 : 0x40;
@@ -116,13 +105,17 @@ static void rdc_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 */
-
 	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 */
 
 	/* PIO configuration clears DTE unconditionally.  It will be
 	 * programmed in set_dmamode which is guaranteed to be called
@@ -164,6 +157,22 @@ static void rdc_set_piomode(struct ata_port *ap, struct ata_device *adev)
 }
 
 /**
+ *	rdc_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 rdc_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	rdc_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0);
+}
+
+/**
  *	rdc_set_dmamode - Initialize host controller PATA PIO timings
  *	@ap: Port whose timings we are configuring
  *	@adev: Drive in question
@@ -177,28 +186,18 @@ static void rdc_set_piomode(struct ata_port *ap, struct ata_device *adev)
 static void rdc_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
-	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 }, };
-
-	pci_read_config_word(dev, master_port, &master_data);
-	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;
 
+		pci_read_config_byte(dev, 0x48, &udma_enable);
+
 		/*
 		 * UDMA is handled by a combination of clock switching and
 		 * selection of dividers
@@ -227,50 +226,18 @@ static void rdc_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 		ideconf &= ~(0x1001 << devid);
 		ideconf |= u_clock << devid;
 		pci_write_config_word(dev, 0x54, ideconf);
+
+		pci_write_config_byte(dev, 0x48, udma_enable);
 	} 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);
-		}
-
-		udma_enable &= ~(1 << devid);
-		pci_write_config_word(dev, master_port, master_data);
+		rdc_set_timings(ap, adev, pio, 1);
 	}
-	pci_write_config_byte(dev, 0x48, udma_enable);
 }
 
 static struct ata_port_operations rdc_pata_ops = {
Signature · 1 line
1.7.1
#53 lore raw

PATCH 12/20 pata_rdc: parallel scanning needs an extra locking

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 540001238b8ebbc4bedd46ca4e2672fcf3787998 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:27 +0100
Subject: [PATCH 12/20] pata_rdc: parallel scanning needs an extra locking

This is similar change as commit 60c3be3 for ata_piix host driver.

---
 drivers/ata/pata_rdc.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/ata/pata_rdc.c +12 -0
index d32b252..f55ff82 100644
@@ -86,10 +86,13 @@ static int rdc_pata_prereset(struct ata_link *link, unsigned long deadline)
 	return ata_sff_prereset(link, deadline);
 }
 
+static DEFINE_SPINLOCK(rdc_lock);
+
 static void rdc_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 is_slave	= (adev->devno != 0);
 	unsigned int master_port= ap->port_no ? 0x42 : 0x40;
 	unsigned int slave_port	= 0x44;
@@ -117,6 +120,8 @@ static void rdc_set_timings(struct ata_port *ap, struct ata_device *adev,
 		/* Enable DMA timing only */
 		control |= 8;	/* PIO cycles in PIO0 */
 
+	spin_lock_irqsave(&rdc_lock, flags);
+
 	/* PIO configuration clears DTE unconditionally.  It will be
 	 * programmed in set_dmamode which is guaranteed to be called
 	 * after set_piomode if any DMA mode is available.
@@ -154,6 +159,8 @@ static void rdc_set_timings(struct ata_port *ap, struct ata_device *adev,
 	pci_read_config_byte(dev, 0x48, &udma_enable);
 	udma_enable &= ~(1 << (2 * ap->port_no + adev->devno));
 	pci_write_config_byte(dev, 0x48, udma_enable);
+
+	spin_unlock_irqrestore(&rdc_lock, flags);
 }
 
 /**
@@ -186,6 +193,7 @@ static void rdc_set_piomode(struct ata_port *ap, struct ata_device *adev)
 static void rdc_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
+	unsigned long flags;
 	u8 speed		= adev->dma_mode;
 	int devid		= adev->devno + 2 * ap->port_no;
 	u8 udma_enable		= 0;
@@ -196,6 +204,8 @@ static void rdc_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 		u16 ideconf;
 		int u_clock, u_speed;
 
+		spin_lock_irqsave(&rdc_lock, flags);
+
 		pci_read_config_byte(dev, 0x48, &udma_enable);
 
 		/*
@@ -228,6 +238,8 @@ static void rdc_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 		pci_write_config_word(dev, 0x54, ideconf);
 
 		pci_write_config_byte(dev, 0x48, udma_enable);
+
+		spin_unlock_irqrestore(&rdc_lock, flags);
 	} else {
 		/* MWDMA is driven by the PIO timings. */
 		unsigned int mwdma = speed - XFER_MW_DMA_0;
Signature · 1 line
1.7.1
#54 lore raw

Re: [PATCH 12/20] pata_rdc: parallel scanning needs an extra locking

Hello.

Quoted 6 lines · Bartlomiej Zolnierkiewicz

From 540001238b8ebbc4bedd46ca4e2672fcf3787998 Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz<[email protected]>
Date: Tue, 8 Feb 2011 12:39:27 +0100
Subject: [PATCH 12/20] pata_rdc: parallel scanning needs an extra locking

This is similar change as commit 60c3be3 for ata_piix host driver.

Linus asks to also specify the commit summary in parens.

Quoted 1 line

WBR, Sergei

#55 lore raw

PATCH 13/20 pata_rdc: add Power Management support

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From fa27d376b5fa43f73305154c1ca58aa93e1d4138 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:27 +0100
Subject: [PATCH 13/20] pata_rdc: add Power Management support

---
 drivers/ata/pata_rdc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/ata/pata_rdc.c +4 -0
index f55ff82..a4a0d88 100644
@@ -357,6 +357,10 @@ static struct pci_driver rdc_pci_driver = {
 	.id_table		= rdc_pci_tbl,
 	.probe			= rdc_init_one,
 	.remove			= rdc_remove_one,
+#ifdef CONFIG_PM
+	.suspend		= ata_pci_device_suspend,
+	.resume			= ata_pci_device_resume,
+#endif
 };
 
 
Signature · 1 line
1.7.1
#56 lore raw

PATCH 14/20 pata_oldpiix: add locking for parallel scanning

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From bad44bb254a6f81776a373e9d46d30c67ef8f124 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:27 +0100
Subject: [PATCH 14/20] pata_oldpiix: add locking for parallel scanning

Add an extra locking for parallel scanning.

This is similar change as commit 60c3be3 for ata_piix host driver and while pata_oldpiix doesn't enable parallel scan yet the race could probably also be triggered by requesting re-scanning of both ports at the same time using SCSI sysfs interface.

Fix documentation while at it.

---
 drivers/ata/pata_oldpiix.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)
drivers/ata/pata_oldpiix.c +9 -1
index 433d2fc..845ede1 100644
@@ -1,7 +1,8 @@
 /*
- *    pata_oldpiix.c - Intel PATA/SATA controllers
+ *    pata_oldpiix.c - older Intel PATA controllers
  *
  *	(C) 2005 Red Hat
+ *	(C) 2010 Bartlomiej Zolnierkiewicz
  *
  *    Some parts based on ata_piix.c by Jeff Garzik and others.
  *
@@ -50,10 +51,13 @@ static int oldpiix_pre_reset(struct ata_link *link, unsigned long deadline)
 	return ata_sff_prereset(link, deadline);
 }
 
+static DEFINE_SPINLOCK(piix_lock);
+
 static void oldpiix_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 idetm_port= ap->port_no ? 0x42 : 0x40;
 	u16 idetm_data;
 	int control = 0;
@@ -84,6 +88,8 @@ static void oldpiix_set_timings(struct ata_port *ap, struct ata_device *adev,
 		/* Enable DMA timing only */
 		control |= 8;	/* PIO cycles in PIO0 */
 
+	spin_lock_irqsave(&piix_lock, flags);
+
 	pci_read_config_word(dev, idetm_port, &idetm_data);
 
 	/*
@@ -101,6 +107,8 @@ static void oldpiix_set_timings(struct ata_port *ap, struct ata_device *adev,
 			(timings[pio][1] << 8);
 	pci_write_config_word(dev, idetm_port, idetm_data);
 
+	spin_unlock_irqrestore(&piix_lock, flags);
+
 	/* Track which port is configured */
 	ap->private_data = adev;
 }
Signature · 1 line
1.7.1
#57 lore raw

Re: [PATCH 14/20] pata_oldpiix: add locking for parallel scanning

Hello.

On 08-02-2011 15:25, Bartlomiej Zolnierkiewicz wrote:
[...]

Quoted 3 lines

Add an extra locking for parallel scanning.

This is similar change as commit 60c3be3 for ata_piix host driver

Linus asks to also specify the commit summary in parens.

Quoted 8 lines

and while pata_oldpiix doesn't enable parallel scan yet the race could probably also be triggered by requesting re-scanning of both ports at the same time using SCSI sysfs interface.

Fix documentation while at it.

[...]

drivers/ata/pata_oldpiix.c line 2

Quoted 8 lines
drivers/ata/pata_oldpiix.c +1 -1
index 433d2fc..845ede1 100644
@@ -1,7 +1,8 @@
  /*
- *    pata_oldpiix.c - Intel PATA/SATA controllers
+ *    pata_oldpiix.c - older Intel PATA controllers

I'd say "controller", since there's only one. :-)

WBR, Sergei

#58 lore raw

PATCH 15/20 pata_oldpiix: enable parallel scan

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 0343103ea4e3f8c35f938d9198f3ea265755ea6a Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:27 +0100
Subject: [PATCH 15/20] pata_oldpiix: enable parallel scan

---
 drivers/ata/pata_oldpiix.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/ata/pata_oldpiix.c +2 -1
index 845ede1..819192d 100644
@@ -222,7 +222,8 @@ static int oldpiix_init_one (struct pci_dev *pdev, const struct pci_device_id *e
 		dev_printk(KERN_DEBUG, &pdev->dev,
 			   "version " DRV_VERSION "\n");
 
-	return ata_pci_bmdma_init_one(pdev, ppi, &oldpiix_sht, NULL, 0);
+	return ata_pci_bmdma_init_one(pdev, ppi, &oldpiix_sht, NULL,
+				      ATA_HOST_PARALLEL_SCAN);
 }
 
 static const struct pci_device_id oldpiix_pci_tbl[] = {
Signature · 1 line
1.7.1
#59 lore raw

PATCH 16/20 ata_piix: add EFAR SLC90E66 support

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 11bed7feff5de752c9440ca58b232846b20e2ed6 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:28 +0100
Subject: [PATCH 16/20] ata_piix: add EFAR SLC90E66 support

Add EFAR SLC90E66 support to ata_piix and remove no longer needed pata_efar driver.

---
 drivers/ata/Kconfig     |    1 +
 drivers/ata/Makefile    |    1 -
 drivers/ata/ata_piix.c  |   74 ++++++++++--
 drivers/ata/pata_efar.c |  292 -----------------------------------------------
 4 files changed, 65 insertions(+), 303 deletions(-)
 delete mode 100644 drivers/ata/pata_efar.c
drivers/ata/Kconfig +1 -0
index 36e2319..402c90c 100644
@@ -375,6 +375,7 @@ config PATA_CYPRESS
 config PATA_EFAR
 	tristate "EFAR SLC90E66 support"
 	depends on PCI
+	select ATA_PIIX
 	help
 	  This option enables support for the EFAR SLC90E66
 	  IDE controller found on some older machines.
drivers/ata/Makefile +0 -1
index 2b67c90..b4252bc 100644
@@ -40,7 +40,6 @@ obj-$(CONFIG_PATA_CS5530)	+= pata_cs5530.o
 obj-$(CONFIG_PATA_CS5535)	+= pata_cs5535.o
 obj-$(CONFIG_PATA_CS5536)	+= pata_cs5536.o
 obj-$(CONFIG_PATA_CYPRESS)	+= pata_cypress.o
-obj-$(CONFIG_PATA_EFAR)		+= pata_efar.o
 obj-$(CONFIG_PATA_HPT366)	+= pata_hpt366.o
 obj-$(CONFIG_PATA_HPT37X)	+= pata_hpt37x.o
 obj-$(CONFIG_PATA_HPT3X2N)	+= pata_hpt3x2n.o
drivers/ata/ata_piix.c +63 -9

167 lines not on this page · raw message

drivers/ata/pata_efar.c deleted +0 -292

292 lines not on this page · raw message

Signature · 1 line
1.7.1
#60 lore raw

Re: [PATCH 16/20] ata_piix: add EFAR SLC90E66 support

Quoted 7 lines · On Tue, 08 Feb 2011 13:25:34 +0100 Bartlomiej Zolnierkiewicz…
Quoted 1 line

From 11bed7feff5de752c9440ca58b232846b20e2ed6 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:28 +0100
Subject: [PATCH 16/20] ata_piix: add EFAR SLC90E66 support

Add EFAR SLC90E66 support to ata_piix and remove no longer needed pata_efar driver.

Jeff specifically asked that these were not all folded into ata_piix originally. This also makes memory usage higher and the system less efficient as these are all motherboard chipsets (except an obscure dual PIIX4 setup) so you are loading more not less code.

It also leads to hideous uglies in the main code paths like this :

+ unsigned int has_sitre = (dev->vendor != 0x8086 ||
+ dev->device != 0x1230);

which also has exactly zero comments.

and it gets better as we go on...

+ unsigned int is_radisys = (dev->vendor ==
PCI_VENDOR_ID_RADISYS &&
+ dev->device == 0x8201);
+ unsigned int has_sitre = (dev->vendor !=
PCI_VENDOR_ID_INTEL ||
+ dev->device != 0x1230) && !is_radisys;

Folding these together just makes no sense at all. The PIO/DMA folds internally do look sensible, but really should be tested before anything goes in. The current code works - its slower than it should be because of core libata bugs, but it works.

Alan

#61 lore raw

Re: [PATCH 16/20] ata_piix: add EFAR SLC90E66 support

Quoted 8 lines · Alan Cox
Quoted 5 lines · On Tue, 08 Feb 2011 13:25:34 +0100 Bartlomiej Zolnierkiewicz…
Quoted 1 line

From 11bed7feff5de752c9440ca58b232846b20e2ed6 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:28 +0100
Subject: [PATCH 16/20] ata_piix: add EFAR SLC90E66 support

Quoted 7 lines
Quoted 2 lines

Add EFAR SLC90E66 support to ata_piix and remove no longer needed pata_efar driver.

Jeff specifically asked that these were not all folded into ata_piix originally. This also makes memory usage higher and the system less efficient as these are all motherboard chipsets (except an obscure dual PIIX4 setup) so you are loading more not less code.

A lot more from a generic SCSI code if you want to optimize things for old or embedded systems. IMHO this is not a best place to look for such optimizations because maintenance cost > potential savings (i.e. making SCSI quirks optional, I have draft patch for this, itself cuts like 10k).

Quoted 6 lines

It also leads to hideous uglies in the main code paths like this :

(quoted diff) +2 -0
+       unsigned int has_sitre  = (dev->vendor != 0x8086 ||
+                                  dev->device != 0x1230);

which also has exactly zero comments.

has_sitre variable name is documentation in itself for anyone knowing the hardware or has read a chipset/code documentation.

Though more comments can certainly be added if needed..

#62 lore raw

Re: [PATCH 16/20] ata_piix: add EFAR SLC90E66 support

Quoted 3 lines

such optimizations because maintenance cost > potential savings (i.e. making SCSI quirks optional, I have draft patch for this, itself cuts like 10k).

Interesting in itself but irrelevant because the current situation is that a piix change cannot break oldpiix, efar, it8213, radisys etc and vice versa. Particuarly important when the other chips are not common so test coverage is difficult, and that far outweighs the maintenance savings for other things, especially as this is code that just doesn't change.

Quoted 10 lines
Quoted 7 lines

It also leads to hideous uglies in the main code paths like this :

(quoted diff) +2 -0
+       unsigned int has_sitre  = (dev->vendor != 0x8086 ||
+                                  dev->device != 0x1230);

which also has exactly zero comments.

has_sitre variable name is documentation in itself for anyone knowing the hardware or has read a chipset/code documentation.

And naturally anyone randomly glancing at the code knows why it's checking 0x8086 & 0x1230, and why the radisys check interacts with it.

Bartlomiej - those are a mess, a complete and total mess. It doesn't necessarily argue against folding them together, but at least do a clean job of it.

#63 lore raw

Re: [PATCH 16/20] ata_piix: add EFAR SLC90E66 support

Quoted 22 lines · Alan Cox
Quoted 3 lines

such optimizations because maintenance cost > potential savings (i.e. making SCSI quirks optional, I have draft patch for this, itself cuts like 10k).

Interesting in itself but irrelevant because the current situation is that a piix change cannot break oldpiix, efar, it8213, radisys etc and vice versa. Particuarly important when the other chips are not common so test coverage is difficult, and that far outweighs the maintenance savings for other things, especially as this is code that just doesn't change.

Quoted 10 lines
Quoted 7 lines

It also leads to hideous uglies in the main code paths like this :

+       unsigned int has_sitre  = (dev->vendor != 0x8086 ||
+                                  dev->device != 0x1230);

which also has exactly zero comments.

has_sitre variable name is documentation in itself for anyone knowing the hardware or has read a chipset/code documentation.

Quoted 6 lines

And naturally anyone randomly glancing at the code knows why it's checking 0x8086 & 0x1230, and why the radisys check interacts with it.

Bartlomiej - those are a mess, a complete and total mess. It doesn't necessarily argue against folding them together, but at least do a clean job of it.

I beg to differ regardless "the mess" comment but well, you can always take my work and "add value" to it like in 2009 (when somehow you miss that your pata_rdc also needs locking fixes but you were more concerned with little differences between my work and your "dreamwork"..)

#64 lore raw

PATCH 17/20 ata_piix: add IT8213 support

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From ecf383059c866b4ab646a4bf1fae4abe4fade58a Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:28 +0100
Subject: [PATCH 17/20] ata_piix: add IT8213 support

Add IT8213 support to ata_piix and remove no longer needed pata_it8213 driver.

---
 drivers/ata/Kconfig       |    1 +
 drivers/ata/Makefile      |    1 -
 drivers/ata/ata_piix.c    |   65 ++++++++++-
 drivers/ata/pata_it8213.c |  284 ---------------------------------------------
 4 files changed, 62 insertions(+), 289 deletions(-)
 delete mode 100644 drivers/ata/pata_it8213.c
drivers/ata/Kconfig +1 -0
index 402c90c..5a92f24 100644
@@ -437,6 +437,7 @@ config PATA_ICSIDE
 config PATA_IT8213
 	tristate "IT8213 PATA support (Experimental)"
 	depends on PCI && EXPERIMENTAL
+	select ATA_PIIX
 	help
 	  This option enables support for the ITE 821 PATA
           controllers via the new ATA layer.
drivers/ata/Makefile +0 -1
index b4252bc..7e78491 100644
@@ -45,7 +45,6 @@ obj-$(CONFIG_PATA_HPT37X)	+= pata_hpt37x.o
 obj-$(CONFIG_PATA_HPT3X2N)	+= pata_hpt3x2n.o
 obj-$(CONFIG_PATA_HPT3X3)	+= pata_hpt3x3.o
 obj-$(CONFIG_PATA_ICSIDE)	+= pata_icside.o
-obj-$(CONFIG_PATA_IT8213)	+= pata_it8213.o
 obj-$(CONFIG_PATA_IT821X)	+= pata_it821x.o
 obj-$(CONFIG_PATA_JMICRON)	+= pata_jmicron.o
 obj-$(CONFIG_PATA_MACIO)	+= pata_macio.o
drivers/ata/ata_piix.c +61 -4

131 lines not on this page · raw message

drivers/ata/pata_it8213.c deleted +0 -284

284 lines not on this page · raw message

Signature · 1 line
1.7.1
#65 lore raw

PATCH 18/20 ata_piix: add RDC support

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 4cc4234e2049fa6e97d5a2d3643bb5499e52f838 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:28 +0100
Subject: [PATCH 18/20] ata_piix: add RDC support

Add RDC PATA support to ata_piix and remove no longer needed pata_rdc driver.

---
 drivers/ata/Kconfig    |    1 +
 drivers/ata/Makefile   |    1 -
 drivers/ata/ata_piix.c |   14 ++-
 drivers/ata/pata_rdc.c |  384 ------------------------------------------------
 4 files changed, 13 insertions(+), 387 deletions(-)
 delete mode 100644 drivers/ata/pata_rdc.c
drivers/ata/Kconfig +1 -0
index 5a92f24..dc9038d 100644
@@ -567,6 +567,7 @@ config PATA_RADISYS
 config PATA_RDC
 	tristate "RDC PATA support"
 	depends on PCI
+	select ATA_PIIX
 	help
 	  This option enables basic support for the later RDC PATA controllers
 	  controllers via the new ATA layer. For the RDC 1010, you need to
drivers/ata/Makefile +0 -1
index 7e78491..e627f6d 100644
@@ -58,7 +58,6 @@ obj-$(CONFIG_PATA_OPTIDMA)	+= pata_optidma.o
 obj-$(CONFIG_PATA_PDC2027X)	+= pata_pdc2027x.o
 obj-$(CONFIG_PATA_PDC_OLD)	+= pata_pdc202xx_old.o
 obj-$(CONFIG_PATA_RADISYS)	+= pata_radisys.o
-obj-$(CONFIG_PATA_RDC)		+= pata_rdc.o
 obj-$(CONFIG_PATA_SC1200)	+= pata_sc1200.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_SCH)		+= pata_sch.o
drivers/ata/ata_piix.c +12 -2
index deb7c96..86d12c2 100644
@@ -1,6 +1,9 @@
 /*
  *    ata_piix.c - Intel PATA/SATA controllers
  *
+ *    This is actually a driver for hardware meeting
+ *    INCITS 370-2004 (1510D): ATA Host Adapter Standards
+ *
  *    Maintained by:  Jeff Garzik <[email protected]>
  *    		    Please ALWAYS copy [email protected]
  *		    on emails.
@@ -249,6 +252,10 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	/* IT8213 */
 	{ 0x1283, 0x8213, PCI_ANY_ID, PCI_ANY_ID, 0, 0, it8213_pata },
 
+	/* RDC is ICH like */
+	{ 0x17F3, 0x1011, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+	{ 0x17F3, 0x1012, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+
 	/* SATA ports */
 	
 	/* 82801EB (ICH5) */
@@ -665,6 +672,7 @@ MODULE_DEVICE_TABLE(pci, piix_pci_tbl);
 MODULE_VERSION(DRV_VERSION);
 MODULE_ALIAS("pata_efar");
 MODULE_ALIAS("pata_it8213");
+MODULE_ALIAS("pata_rdc");
 
 struct ich_laptop {
 	u16 device;
@@ -1670,7 +1678,8 @@ static int __devinit piix_init_one(struct pci_dev *pdev,
 	 * because some ACPI implementations mess up cable related
 	 * bits on _STM.  Reported on kernel bz#11879.
 	 */
-	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL ||
+	    pdev->vendor == PCI_VENDOR_ID_RDC)
 		pci_read_config_dword(pdev, PIIX_IOCFG, &hpriv->saved_iocfg);
 
 	/* ICH6R may be driven by either ata_piix or ahci driver
@@ -1735,7 +1744,8 @@ static void piix_remove_one(struct pci_dev *pdev)
 	struct ata_host *host = dev_get_drvdata(&pdev->dev);
 	struct piix_host_priv *hpriv = host->private_data;
 
-	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL ||
+	    pdev->vendor == PCI_VENDOR_ID_RDC)
 		pci_write_config_dword(pdev, PIIX_IOCFG, hpriv->saved_iocfg);
 
 	ata_pci_remove_one(pdev);
drivers/ata/pata_rdc.c deleted +0 -384

384 lines not on this page · raw message

Signature · 1 line
1.7.1
#66 lore raw

PATCH 19/20 ata_piix: add Intel old PIIX support

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From dc3181ebc3a8735bcd6ba334f2ad7945654e1189 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:28 +0100
Subject: [PATCH 19/20] ata_piix: add Intel old PIIX support

Add Intel old PIIX support to ata_piix and remove no longer needed pata_oldpiix driver.

---
 drivers/ata/Kconfig        |    1 +
 drivers/ata/Makefile       |    1 -
 drivers/ata/ata_piix.c     |   86 +++++++++++++--
 drivers/ata/pata_oldpiix.c |  264 --------------------------------------------
 4 files changed, 78 insertions(+), 274 deletions(-)
 delete mode 100644 drivers/ata/pata_oldpiix.c
drivers/ata/Kconfig +1 -0
index dc9038d..0e6d193 100644
@@ -523,6 +523,7 @@ config PATA_NS87415
 config PATA_OLDPIIX
 	tristate "Intel PATA old PIIX support"
 	depends on PCI
+	select ATA_PIIX
 	help
 	  This option enables support for early PIIX PATA support.
 
drivers/ata/Makefile +0 -1
index e627f6d..55272af 100644
@@ -53,7 +53,6 @@ obj-$(CONFIG_PATA_MPC52xx)	+= pata_mpc52xx.o
 obj-$(CONFIG_PATA_NETCELL)	+= pata_netcell.o
 obj-$(CONFIG_PATA_NINJA32)	+= pata_ninja32.o
 obj-$(CONFIG_PATA_NS87415)	+= pata_ns87415.o
-obj-$(CONFIG_PATA_OLDPIIX)	+= pata_oldpiix.o
 obj-$(CONFIG_PATA_OPTIDMA)	+= pata_optidma.o
 obj-$(CONFIG_PATA_PDC2027X)	+= pata_pdc2027x.o
 obj-$(CONFIG_PATA_PDC_OLD)	+= pata_pdc202xx_old.o
drivers/ata/ata_piix.c +77 -9

171 lines not on this page · raw message

drivers/ata/pata_oldpiix.c deleted +0 -264

264 lines not on this page · raw message

Signature · 1 line
1.7.1
#67 lore raw

PATCH 20/20 ata_piix: add Radisys R82600 support

In reply to Bartlomiej Zolnierkiewicz
Quoted 1 line

From 2172ad66e1ad14599fe08d4fe898d392f10962d3 Mon Sep 17 00:00:00 2001

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 8 Feb 2011 12:39:28 +0100
Subject: [PATCH 20/20] ata_piix: add Radisys R82600 support

Add Radisys R82600 support to ata_piix and remove no longer needed pata_radisys driver.

---
 drivers/ata/Kconfig        |    1 +
 drivers/ata/Makefile       |    1 -
 drivers/ata/ata_piix.c     |   84 ++++++++++++---
 drivers/ata/pata_radisys.c |  252 --------------------------------------------
 4 files changed, 72 insertions(+), 266 deletions(-)
 delete mode 100644 drivers/ata/pata_radisys.c
drivers/ata/Kconfig +1 -0
index 0e6d193..43bbf74 100644
@@ -559,6 +559,7 @@ config PATA_PDC_OLD
 config PATA_RADISYS
 	tristate "RADISYS 82600 PATA support (Experimental)"
 	depends on PCI && EXPERIMENTAL
+	select ATA_PIIX
 	help
 	  This option enables support for the RADISYS 82600
 	  PATA controllers via the new ATA layer
drivers/ata/Makefile +0 -1
index 55272af..cddef6e 100644
@@ -56,7 +56,6 @@ obj-$(CONFIG_PATA_NS87415)	+= pata_ns87415.o
 obj-$(CONFIG_PATA_OPTIDMA)	+= pata_optidma.o
 obj-$(CONFIG_PATA_PDC2027X)	+= pata_pdc2027x.o
 obj-$(CONFIG_PATA_PDC_OLD)	+= pata_pdc202xx_old.o
-obj-$(CONFIG_PATA_RADISYS)	+= pata_radisys.o
 obj-$(CONFIG_PATA_SC1200)	+= pata_sc1200.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_SCH)		+= pata_sch.o
drivers/ata/ata_piix.c +71 -13

167 lines not on this page · raw message

drivers/ata/pata_radisys.c deleted +0 -252

252 lines not on this page · raw message

Signature · 1 line
1.7.1