[PATCHSET] ata_piix: improve combined mode handling

[PATCHSET] ata_piix: improve combined mode handling

[Tejun Heo]

Hello, all.

This patchset is against the current upstream[1] + kill illegal
kfree() patch[2].

This patchset improves combined handling of ata_piix.  Proper port map
is implemented such that ata_piix knows exactly how PIIX SATA ports
are mapped to ATA devices.  This patchset also should fix device
detection problem which seems to be caused by honoring present bits
while the datasheet declares them reserved on 6300ESB.  (Jean & 0602,
can you guys please test this patchset?)

I couldn't find datasheets for ESB2 and ICH8, so I assumed ESB2 is
similar to ICH6, ICH8 to ICH6 and ICH8M to ICH6M.  Is this correct?

Jeff, I'm trying to implement SCR access on ICH6/7's on top of these
changes and have a question about libata-bmdma.c::ata_pci_init_one().
ata_piix needs its own version of this function as it should try to
map ABAR for SCR (and fall back if it fails), so I'm trying to factor
functions out of ata_pci_init_one() and call them from ata_piix.

ata_pci_init_one() currently uses two separate probe_ent for each port
if the controller is in legacy mode, which makes the ports use
separate host_set and thus separate spin locks.  Do they need to use
separate spinlocks?  Or are there other reasons legacy initialization
is done this way?

Oh.. Another question.  When you were talking about getting PCS bits
wrong before [3], were you talking about port map (like port 1 and 3
map to primary master/slave) or is there something else PCS + MAP
doesn't cover?

Thanks.

--
tejun

[1] cccc65a3b60edaf721cdee5a14f68ba009341822
[2] http://article.gmane.org/gmane.linux.ide/8324

[PATCH 1/6] libata: seperate out ata_class_present()

[Tejun Heo]

Seperate out ata_class_present() from ata_dev_present().  This is
useful because new reset mechanism deals with classes[] directly.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 include/linux/libata.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

d5044307293c38dd897722d170dcffecb069660f
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 66b6847..22e86cb 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -645,10 +645,14 @@ static inline unsigned int ata_tag_valid
 	return (tag < ATA_MAX_QUEUE) ? 1 : 0;
 }
 
+static inline unsigned int ata_class_present(unsigned int class)
+{
+	return class == ATA_DEV_ATA || class == ATA_DEV_ATAPI;
+}
+
 static inline unsigned int ata_dev_present(const struct ata_device *dev)
 {
-	return ((dev->class == ATA_DEV_ATA) ||
-		(dev->class == ATA_DEV_ATAPI));
+	return ata_class_present(dev->class);
 }
 
 static inline u8 ata_chk_status(struct ata_port *ap)
-- 
1.2.1

Re: [PATCH 1/6] libata: seperate out ata_class_present()

[Jeff Garzik]

Tejun Heo wrote:
> Seperate out ata_class_present() from ata_dev_present().  This is
> useful because new reset mechanism deals with classes[] directly.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied 1-5

[PATCH 4/6] ata_piix: implement proper port map

[Tejun Heo]

Replace combined mode handling via PIIX_COMB/COMB_PATA_P0 with proper
port map.  PIIX now prints port configuration during initialization.
ATA_FLAG_SLAVE_POSS is now turned on for SATA ports only when the
slave device is actually avaliable.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ata_piix.c |  182 ++++++++++++++++++++++++++++++++---------------
 1 files changed, 122 insertions(+), 60 deletions(-)

21ef3f5076ba7d651b17cbbf217cdba03d5c58f6
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 987a7cc..4080b8b 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -112,9 +112,6 @@ enum {
 	/* combined mode.  if set, PATA is channel 0.
 	 * if clear, PATA is channel 1.
 	 */
-	PIIX_COMB_PATA_P0	= (1 << 1),
-	PIIX_COMB		= (1 << 2), /* combined mode enabled? */
-
 	PIIX_PORT_ENABLED	= (1 << 0),
 	PIIX_PORT_PRESENT	= (1 << 4),
 
@@ -130,9 +127,23 @@ enum {
 	ich6_sata_ahci		= 5,
 	ich6m_sata_ahci		= 6,
 
+	/* constants for mapping table */
+	P0			= 0,  /* port 0 */
+	P1			= 1,  /* port 1 */
+	P2			= 2,  /* port 2 */
+	P3			= 3,  /* port 3 */
+	IDE			= -1, /* IDE */
+	NA			= -2, /* not avaliable */
+	RV			= -3, /* reserved */
+
 	PIIX_AHCI_DEVICE	= 6,
 };
 
+struct piix_map_db {
+	const u32 mask;
+	const int map[][4];
+};
+
 static int piix_init_one (struct pci_dev *pdev,
 				    const struct pci_device_id *ent);
 
@@ -272,6 +283,43 @@ static const struct ata_port_operations 
 	.host_stop		= ata_host_stop,
 };
 
+static struct piix_map_db ich5_map_db = {
+	.mask = 0x7,
+	.map = {
+		/* PM   PS   SM   SS       MAP  */
+		{  P0,  NA,  P1,  NA }, /* 000b */
+		{  P1,  NA,  P0,  NA }, /* 001b */
+		{  RV,  RV,  RV,  RV },
+		{  RV,  RV,  RV,  RV },
+		{  P0,  P1, IDE, IDE }, /* 100b */
+		{  P1,  P0, IDE, IDE }, /* 101b */
+		{ IDE, IDE,  P0,  P1 }, /* 110b */
+		{ IDE, IDE,  P1,  P0 }, /* 111b */
+	},
+};
+
+static struct piix_map_db ich6_map_db = {
+	.mask = 0x3,
+	.map = {
+		/* PM   PS   SM   SS       MAP */
+		{  P0,  P1,  P2,  P3 }, /* 00b */
+		{ IDE, IDE,  P1,  P3 }, /* 01b */
+		{  P0,  P2, IDE, IDE }, /* 10b */
+		{  RV,  RV,  RV,  RV },
+	},
+};
+
+static struct piix_map_db ich6m_map_db = {
+	.mask = 0x3,
+	.map = {
+		/* PM   PS   SM   SS       MAP */
+		{  P0,  P1,  P2,  P3 }, /* 00b */
+		{  RV,  RV,  RV,  RV },
+		{  P0,  P2, IDE, IDE }, /* 10b */
+		{  RV,  RV,  RV,  RV },
+	},
+};
+
 static struct ata_port_info piix_port_info[] = {
 	/* piix4_pata */
 	{
@@ -310,6 +358,7 @@ static struct ata_port_info piix_port_in
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.private_data	= &ich5_map_db,
 	},
 
 	/* i6300esb_sata */
@@ -321,42 +370,45 @@ static struct ata_port_info piix_port_in
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.private_data	= &ich5_map_db,
 	},
 
 	/* ich6_sata */
 	{
 		.sht		= &piix_sht,
 		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
-				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS |
-				  PIIX_FLAG_SCR,
+				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.private_data	= &ich6_map_db,
 	},
 
 	/* ich6_sata_ahci */
 	{
 		.sht		= &piix_sht,
 		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
-				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS |
-				  PIIX_FLAG_SCR | PIIX_FLAG_AHCI,
+				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR |
+				  PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.private_data	= &ich6_map_db,
 	},
 
 	/* ich6m_sata_ahci */
 	{
 		.sht		= &piix_sht,
 		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
-				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS |
-				  PIIX_FLAG_SCR | PIIX_FLAG_AHCI,
+				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR |
+				  PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.private_data	= &ich6m_map_db,
 	},
 };
 
@@ -708,6 +760,54 @@ static int __devinit piix_check_450nx_er
 	return no_piix_dma;
 }		
 
+static void __devinit piix_init_sata_map(struct pci_dev *pdev,
+					 struct ata_port_info *pinfo)
+{
+	struct piix_map_db *map_db = pinfo[0].private_data;
+	const unsigned int *map;
+	int i, invalid_map = 0;
+	u8 map_value;
+
+	pci_read_config_byte(pdev, ICH5_PMR, &map_value);
+
+	map = map_db->map[map_value & map_db->mask];
+
+	dev_printk(KERN_INFO, &pdev->dev, "MAP [");
+	for (i = 0; i < 4; i++) {
+		switch (map[i]) {
+		case RV:
+			invalid_map = 1;
+			printk(" XX");
+			break;
+
+		case NA:
+			printk(" --");
+			break;
+
+		case IDE:
+			WARN_ON((i & 1) || map[i + 1] != IDE);
+			pinfo[i / 2] = piix_port_info[ich5_pata];
+			i++;
+			printk(" IDE IDE");
+			break;
+
+		default:
+			printk(" P%d", map[i]);
+			if (i & 1)
+				pinfo[i / 2].host_flags |= ATA_FLAG_SLAVE_POSS;
+			break;
+		}
+	}
+	printk(" ]\n");
+
+	if (invalid_map)
+		dev_printk(KERN_ERR, &pdev->dev,
+			   "invalid MAP value %u\n", map_value);
+
+	pinfo[0].private_data = (void *)map;
+	pinfo[1].private_data = (void *)map;
+}
+
 /**
  *	piix_init_one - Register PIIX ATA PCI device with kernel services
  *	@pdev: PCI device to register
@@ -726,9 +826,8 @@ static int __devinit piix_check_450nx_er
 static int piix_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
-	struct ata_port_info *port_info[2];
-	unsigned int combined = 0;
-	unsigned int pata_chan = 0, sata_chan = 0;
+	struct ata_port_info port_info[2];
+	struct ata_port_info *ppinfo[2] = { &port_info[0], &port_info[1] };
 	unsigned long host_flags;
 
 	if (!printed_version++)
@@ -739,10 +838,10 @@ static int piix_init_one (struct pci_dev
 	if (!in_module_init)
 		return -ENODEV;
 
-	port_info[0] = &piix_port_info[ent->driver_data];
-	port_info[1] = &piix_port_info[ent->driver_data];
+	port_info[0] = piix_port_info[ent->driver_data];
+	port_info[1] = piix_port_info[ent->driver_data];
 
-	host_flags = port_info[0]->host_flags;
+	host_flags = port_info[0].host_flags;
 
 	if (host_flags & PIIX_FLAG_AHCI) {
 		u8 tmp;
@@ -754,37 +853,9 @@ static int piix_init_one (struct pci_dev
 		}
 	}
 
-	if (host_flags & PIIX_FLAG_COMBINED) {
-		u8 tmp;
-		pci_read_config_byte(pdev, ICH5_PMR, &tmp);
-
-		if (host_flags & PIIX_FLAG_COMBINED_ICH6) {
-			switch (tmp & 0x3) {
-			case 0:
-				break;
-			case 1:
-				combined = 1;
-				sata_chan = 1;
-				break;
-			case 2:
-				combined = 1;
-				pata_chan = 1;
-				break;
-			case 3:
-				dev_printk(KERN_WARNING, &pdev->dev,
-					   "invalid MAP value %u\n", tmp);
-				break;
-			}
-		} else {
-			if (tmp & PIIX_COMB) {
-				combined = 1;
-				if (tmp & PIIX_COMB_PATA_P0)
-					sata_chan = 1;
-				else
-					pata_chan = 1;
-			}
-		}
-	}
+	/* Initialize SATA map */
+	if (host_flags & ATA_FLAG_SATA)
+		piix_init_sata_map(pdev, port_info);
 
 	/* On ICH5, some BIOSen disable the interrupt using the
 	 * PCI_COMMAND_INTX_DISABLE bit added in PCI 2.3.
@@ -795,25 +866,16 @@ static int piix_init_one (struct pci_dev
 	if (host_flags & PIIX_FLAG_CHECKINTR)
 		pci_intx(pdev, 1);
 
-	if (combined) {
-		port_info[sata_chan] = &piix_port_info[ent->driver_data];
-		port_info[sata_chan]->host_flags |= ATA_FLAG_SLAVE_POSS;
-		port_info[pata_chan] = &piix_port_info[ich5_pata];
-
-		dev_printk(KERN_WARNING, &pdev->dev,
-			   "combined mode detected (p=%u, s=%u)\n",
-			   pata_chan, sata_chan);
-	}
 	if (piix_check_450nx_errata(pdev)) {
 		/* This writes into the master table but it does not
 		   really matter for this errata as we will apply it to
 		   all the PIIX devices on the board */
-		port_info[0]->mwdma_mask = 0;
-		port_info[0]->udma_mask = 0;
-		port_info[1]->mwdma_mask = 0;
-		port_info[1]->udma_mask = 0;
+		port_info[0].mwdma_mask = 0;
+		port_info[0].udma_mask = 0;
+		port_info[1].mwdma_mask = 0;
+		port_info[1].udma_mask = 0;
 	}
-	return ata_pci_init_one(pdev, port_info, 2);
+	return ata_pci_init_one(pdev, ppinfo, 2);
 }
 
 static int __init piix_init(void)
-- 
1.2.1

[PATCH 6/6] ata_piix: don't scan non-existent device

[Tejun Heo]

ata_std_softreset() sometimes reports presence of dev 1 even when it
doesn't exist (PCS-wise).  This usually happens reading dev 1's
signature returns that of dev 0 (e.g. on ICH6M).  This patch turns off
non-existent device on such cases.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ata_piix.c |   25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

cda8686b4ad82a9272748af4388d47475c6acb2b
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 72e38e6..625db11 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -566,14 +566,31 @@ static unsigned int piix_sata_probe (str
  */
 static int piix_sata_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
-	if (!piix_sata_probe(ap)) {
+	unsigned int mask;
+	int i, rc;
+
+	mask = piix_sata_probe(ap);
+	if (!mask) {
 		printk(KERN_INFO "ata%u: SATA port has no device.\n", ap->id);
 		return 0;
 	}
 
-	return ata_drive_probe_reset(ap, ata_std_probeinit,
-				     ata_std_softreset, NULL,
-				     ata_std_postreset, classes);
+	rc = ata_drive_probe_reset(ap, ata_std_probeinit,
+				   ata_std_softreset, NULL,
+				   ata_std_postreset, classes);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < 2; i++) {
+		if (ata_class_present(classes[i]) && !(mask & (1 << i))) {
+			printk(KERN_WARNING "ata%u: dev %u class %d for "
+			       "non-existent device, fixing up\n",
+			       ap->id, i, classes[i]);
+			classes[i] = ATA_DEV_NONE;
+		}
+	}
+
+	return 0;
 }
 
 /**
-- 
1.2.1

Re: [PATCH 6/6] ata_piix: don't scan non-existent device

[Jeff Garzik]

Tejun Heo wrote:
> ata_std_softreset() sometimes reports presence of dev 1 even when it
> doesn't exist (PCS-wise).  This usually happens reading dev 1's
> signature returns that of dev 0 (e.g. on ICH6M).  This patch turns off
> non-existent device on such cases.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

I'm going to NAK this one for now.  This doesn't smell like a 
piix-specific behavior.  Perhaps there is a device selection bug in 
libata core.

	Jeff

[PATCH 5/6] ata_piix: reimplement piix_sata_probe()

[Tejun Heo]

Reimplement piix_sata_probe() such that it turns on PCS enable bits on
all avaliable ports and check present bits after a while to determine
device presence.  This should help broken BIOSes.  After device
presence detection is complete, PCS enable bits of unoccupied bits are
turned off unless the controller supports AHCI (ICH6/7 docs mandate
all enables bits are always set on AHCI capable controllers).

Note that PCS present bits are ignored on 6300ESB as described in the
datasheet.  This should fix device detection problems reported with
the controller.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ata_piix.c |   59 +++++++++++++++++++++++++++++------------------
 1 files changed, 37 insertions(+), 22 deletions(-)

cea33259b68294395b5d855aff308f342e9913a4
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 4080b8b..72e38e6 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -499,44 +499,59 @@ static int piix_pata_probe_reset(struct 
  *	piix_sata_probe - Probe PCI device for present SATA devices
  *	@ap: Port associated with the PCI device we wish to probe
  *
- *	Reads SATA PCI device's PCI config register Port Configuration
- *	and Status (PCS) to determine port and device availability.
+ *	Reads and configures SATA PCI device's PCI config register
+ *	Port Configuration and Status (PCS) to determine port and
+ *	device availability.
  *
  *	LOCKING:
  *	None (inherited from caller).
  *
  *	RETURNS:
- *	Non-zero if port is enabled, it may or may not have a device
- *	attached in that case (PRESENT bit would only be set if BIOS probe
- *	was done). Zero is returned if port is disabled.
+ *	Mask of avaliable devices on the port.
  */
-static int piix_sata_probe (struct ata_port *ap)
+static unsigned int piix_sata_probe (struct ata_port *ap)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
-	int combined = (ap->flags & ATA_FLAG_SLAVE_POSS);
-	int orig_mask, mask, i;
+	const unsigned int *map = ap->host_set->private_data;
+	int base = 2 * ap->hard_port_no;
+	unsigned int present_mask = 0;
+	int port, i;
 	u8 pcs;
 
 	pci_read_config_byte(pdev, ICH5_PCS, &pcs);
-	orig_mask = (int) pcs & 0xff;
+	DPRINTK("ata%u: ENTER, pcs=0x%x base=%d\n", ap->id, pcs, base);
 
-	/* TODO: this is vaguely wrong for ICH6 combined mode,
-	 * where only two of the four SATA ports are mapped
-	 * onto a single ATA channel.  It is also vaguely inaccurate
-	 * for ICH5, which has only two ports.  However, this is ok,
-	 * as further device presence detection code will handle
-	 * any false positives produced here.
-	 */
+	/* enable all ports on this ap and wait for them to settle */
+	for (i = 0; i < 2; i++) {
+		port = map[base + i];
+		if (port >= 0)
+			pcs |= 1 << port;
+	}
 
-	for (i = 0; i < 4; i++) {
-		mask = (PIIX_PORT_ENABLED << i);
+	pci_write_config_byte(pdev, ICH5_PCS, pcs);
+	msleep(100);
+
+	/* let's see which devices are present */
+	pci_read_config_byte(pdev, ICH5_PCS, &pcs);
 
-		if ((orig_mask & mask) == mask)
-			if (combined || (i == ap->hard_port_no))
-				return 1;
+	for (i = 0; i < 2; i++) {
+		port = map[base + i];
+		if (port < 0)
+			continue;
+		if (ap->flags & PIIX_FLAG_IGN_PRESENT || pcs & 1 << (4 + port))
+			present_mask |= 1 << i;
+		else
+			pcs &= ~(1 << port);
 	}
 
-	return 0;
+	/* disable offline ports on non-AHCI controllers */
+	if (!(ap->flags & PIIX_FLAG_AHCI))
+		pci_write_config_byte(pdev, ICH5_PCS, pcs);
+
+	DPRINTK("ata%u: LEAVE, pcs=0x%x present_mask=0x%x\n",
+		ap->id, pcs, present_mask);
+
+	return present_mask;
 }
 
 /**
-- 
1.2.1

[PATCH 3/6] ata_piix: add a couple of flags

[Tejun Heo]

Add PIIX_FLAG_IGN_PRESENT and SCR flags.  Thi patch doesn't cause any
functional change.  To be used by later init/scr updates.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ata_piix.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

1d0bf89eeba0a60799460a23afae05363a395590
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 5988d5f..987a7cc 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -101,6 +101,8 @@ enum {
 	ICH5_PCS		= 0x92,	/* port control and status */
 	PIIX_SCC		= 0x0A, /* sub-class code register */
 
+	PIIX_FLAG_IGN_PRESENT	= (1 << 25), /* ignore PCS present bits */
+	PIIX_FLAG_SCR		= (1 << 26), /* SCR available */
 	PIIX_FLAG_AHCI		= (1 << 27), /* AHCI possible */
 	PIIX_FLAG_CHECKINTR	= (1 << 28), /* make sure PCI INTx enabled */
 	PIIX_FLAG_COMBINED	= (1 << 29), /* combined mode possible */
@@ -314,7 +316,7 @@ static struct ata_port_info piix_port_in
 	{
 		.sht		= &piix_sht,
 		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED |
-				  PIIX_FLAG_CHECKINTR,
+				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_IGN_PRESENT,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
@@ -325,7 +327,8 @@ static struct ata_port_info piix_port_in
 	{
 		.sht		= &piix_sht,
 		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
-				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS,
+				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS |
+				  PIIX_FLAG_SCR,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
@@ -337,7 +340,7 @@ static struct ata_port_info piix_port_in
 		.sht		= &piix_sht,
 		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
 				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS |
-				  PIIX_FLAG_AHCI,
+				  PIIX_FLAG_SCR | PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
@@ -349,7 +352,7 @@ static struct ata_port_info piix_port_in
 		.sht		= &piix_sht,
 		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
 				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS |
-				  PIIX_FLAG_AHCI,
+				  PIIX_FLAG_SCR | PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
-- 
1.2.1

Re: [PATCH 3/6] ata_piix: add a couple of flags

[Jeff Garzik]

Tejun Heo wrote:
> Add PIIX_FLAG_IGN_PRESENT and SCR flags.  Thi patch doesn't cause any
> functional change.  To be used by later init/scr updates.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Please rename PIIX_FLAG_IGN_PRESENT to PIIX_FLAG_IGNORE_PCS or 
PIIX_FLAG_IGN_PCS in an update patch.

	Jeff

[PATCH] ata_piix: rename PIIX_FLAG_IGN_PRESENT to PIIX_FLAG_IGNORE_PCS

[Tejun Heo]

Rename PIIX_FLAG_IGN_PRESENT to PIIX_FLAG_IGNORE_PCS as Jeff
requested.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Jeff Garzik <jgarzik@pobox.com>

diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 72e38e6..9327b62 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -101,7 +101,7 @@ enum {
 	ICH5_PCS		= 0x92,	/* port control and status */
 	PIIX_SCC		= 0x0A, /* sub-class code register */
 
-	PIIX_FLAG_IGN_PRESENT	= (1 << 25), /* ignore PCS present bits */
+	PIIX_FLAG_IGNORE_PCS	= (1 << 25), /* ignore PCS present bits */
 	PIIX_FLAG_SCR		= (1 << 26), /* SCR available */
 	PIIX_FLAG_AHCI		= (1 << 27), /* AHCI possible */
 	PIIX_FLAG_CHECKINTR	= (1 << 28), /* make sure PCI INTx enabled */
@@ -365,7 +365,7 @@ static struct ata_port_info piix_port_in
 	{
 		.sht		= &piix_sht,
 		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED |
-				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_IGN_PRESENT,
+				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_IGNORE_PCS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
@@ -538,7 +538,7 @@ static unsigned int piix_sata_probe (str
 		port = map[base + i];
 		if (port < 0)
 			continue;
-		if (ap->flags & PIIX_FLAG_IGN_PRESENT || pcs & 1 << (4 + port))
+		if (ap->flags & PIIX_FLAG_IGNORE_PCS || pcs & 1 << (4 + port))
 			present_mask |= 1 << i;
 		else
 			pcs &= ~(1 << port);

Re: [PATCH] ata_piix: rename PIIX_FLAG_IGN_PRESENT to PIIX_FLAG_IGNORE_PCS

[Jeff Garzik]

Tejun Heo wrote:
> Rename PIIX_FLAG_IGN_PRESENT to PIIX_FLAG_IGNORE_PCS as Jeff
> requested.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Jeff Garzik <jgarzik@pobox.com>

applied

[PATCH 2/6] ata_piix: finer-grained port_info

[Tejun Heo]

Make port_info finer-grained.  This patch doesn't cause any functional
change.  Later init reimplementation will make use of it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ata_piix.c |   77 +++++++++++++++++++++++++++++++++++------------
 1 files changed, 58 insertions(+), 19 deletions(-)

cd78103a27cf7b7f6bdfe30515708a683171d00d
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index c662bf5..5988d5f 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -119,11 +119,14 @@ enum {
 	PIIX_80C_PRI		= (1 << 5) | (1 << 4),
 	PIIX_80C_SEC		= (1 << 7) | (1 << 6),
 
-	ich5_pata		= 0,
-	ich5_sata		= 1,
-	piix4_pata		= 2,
-	ich6_sata		= 3,
-	ich6_sata_ahci		= 4,
+	/* controller IDs */
+	piix4_pata		= 0,
+	ich5_pata		= 1,
+	ich5_sata		= 2,
+	esb_sata		= 3,
+	ich6_sata		= 4,
+	ich6_sata_ahci		= 5,
+	ich6m_sata_ahci		= 6,
 
 	PIIX_AHCI_DEVICE	= 6,
 };
@@ -149,19 +152,32 @@ static const struct pci_device_id piix_p
 	 * list in drivers/pci/quirks.c.
 	 */
 
+	/* 82801EB (ICH5) */
 	{ 0x8086, 0x24d1, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich5_sata },
+	/* 82801EB (ICH5) */
 	{ 0x8086, 0x24df, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich5_sata },
-	{ 0x8086, 0x25a3, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich5_sata },
-	{ 0x8086, 0x25b0, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich5_sata },
+	/* 6300ESB (ICH5 variant with broken PCS present bits) */
+	{ 0x8086, 0x25a3, PCI_ANY_ID, PCI_ANY_ID, 0, 0, esb_sata },
+	/* 6300ESB pretending RAID */
+	{ 0x8086, 0x25b0, PCI_ANY_ID, PCI_ANY_ID, 0, 0, esb_sata },
+	/* 82801FB/FW (ICH6/ICH6W) */
 	{ 0x8086, 0x2651, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata },
+	/* 82801FR/FRW (ICH6R/ICH6RW) */
 	{ 0x8086, 0x2652, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
-	{ 0x8086, 0x2653, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
+	/* 82801FBM ICH6M (ICH6R with only port 0 and 2 implemented) */
+	{ 0x8086, 0x2653, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6m_sata_ahci },
+	/* 82801GB/GR/GH (ICH7, identical to ICH6) */
 	{ 0x8086, 0x27c0, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
-	{ 0x8086, 0x27c4, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
+	/* 2801GBM/GHM (ICH7M, identical to ICH6M) */
+	{ 0x8086, 0x27c4, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6m_sata_ahci },
+	/* Enterprise Southbridge 2 (where's the datasheet?) */
 	{ 0x8086, 0x2680, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
+	/* SATA Controller 1 IDE (ICH8, no datasheet yet) */
 	{ 0x8086, 0x2820, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
+	/* SATA Controller 2 IDE (ICH8, ditto) */
 	{ 0x8086, 0x2825, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
-	{ 0x8086, 0x2828, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata_ahci },
+	/* Mobile SATA Controller IDE (ICH8M, ditto) */
+	{ 0x8086, 0x2828, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6m_sata_ahci },
 
 	{ }	/* terminate list */
 };
@@ -255,6 +271,20 @@ static const struct ata_port_operations 
 };
 
 static struct ata_port_info piix_port_info[] = {
+	/* piix4_pata */
+	{
+		.sht		= &piix_sht,
+		.host_flags	= ATA_FLAG_SLAVE_POSS,
+		.pio_mask	= 0x1f,	/* pio0-4 */
+#if 0
+		.mwdma_mask	= 0x06, /* mwdma1-2 */
+#else
+		.mwdma_mask	= 0x00, /* mwdma broken */
+#endif
+		.udma_mask	= ATA_UDMA_MASK_40C,
+		.port_ops	= &piix_pata_ops,
+	},
+
 	/* ich5_pata */
 	{
 		.sht		= &piix_sht,
@@ -280,18 +310,15 @@ static struct ata_port_info piix_port_in
 		.port_ops	= &piix_sata_ops,
 	},
 
-	/* piix4_pata */
+	/* i6300esb_sata */
 	{
 		.sht		= &piix_sht,
-		.host_flags	= ATA_FLAG_SLAVE_POSS,
+		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED |
+				  PIIX_FLAG_CHECKINTR,
 		.pio_mask	= 0x1f,	/* pio0-4 */
-#if 0
-		.mwdma_mask	= 0x06, /* mwdma1-2 */
-#else
-		.mwdma_mask	= 0x00, /* mwdma broken */
-#endif
-		.udma_mask	= ATA_UDMA_MASK_40C,
-		.port_ops	= &piix_pata_ops,
+		.mwdma_mask	= 0x07, /* mwdma0-2 */
+		.udma_mask	= 0x7f,	/* udma0-6 */
+		.port_ops	= &piix_sata_ops,
 	},
 
 	/* ich6_sata */
@@ -316,6 +343,18 @@ static struct ata_port_info piix_port_in
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
 	},
+
+	/* ich6m_sata_ahci */
+	{
+		.sht		= &piix_sht,
+		.host_flags	= ATA_FLAG_SATA | PIIX_FLAG_COMBINED_ICH6 |
+				  PIIX_FLAG_CHECKINTR | ATA_FLAG_SLAVE_POSS |
+				  PIIX_FLAG_AHCI,
+		.pio_mask	= 0x1f,	/* pio0-4 */
+		.mwdma_mask	= 0x07, /* mwdma0-2 */
+		.udma_mask	= 0x7f,	/* udma0-6 */
+		.port_ops	= &piix_sata_ops,
+	},
 };
 
 static struct pci_bits piix_enable_bits[] = {
-- 
1.2.1

Re: [PATCHSET] ata_piix: improve combined mode handling

[Jeff Garzik]

Tejun Heo wrote:
> Hello, all.
> 
> This patchset is against the current upstream[1] + kill illegal
> kfree() patch[2].

30-second review:  looks OK, though I worry about patch #6.  I'll answer 
your questions and review in-depth later on.

	Jeff

Re: [PATCHSET] ata_piix: improve combined mode handling

[Tejun Heo]

On Tue, Feb 28, 2006 at 01:17:27PM -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> >Hello, all.
> >
> >This patchset is against the current upstream[1] + kill illegal
> >kfree() patch[2].
> 
> 30-second review:  looks OK, though I worry about patch #6.  I'll answer 
> your questions and review in-depth later on.
> 

I'm also a little bit about #6 (the reason why it's not part of #5).
I also thought about masking with available ports but we are trusting
PCS enable bits when both bits are off (early exit when present_mask
is zero), so...

-- 
tejun

Re: [PATCHSET] ata_piix: improve combined mode handling

[Jeff Garzik]

Tejun Heo wrote:
> Hello, all.
> 
> This patchset is against the current upstream[1] + kill illegal
> kfree() patch[2].

Testing this now, and will re-review in-depth and apply if successful.

	Jeff

Re: [PATCHSET] ata_piix: improve combined mode handling

[Jeff Garzik]

Tejun Heo wrote:
> I couldn't find datasheets for ESB2 and ICH8, so I assumed ESB2 is
> similar to ICH6, ICH8 to ICH6 and ICH8M to ICH6M.  Is this correct?

That would be my guess.  Best bet would be to email Jason Gaston (CC'd), 
as he's the person who has been submitted PCI ID patches from Intel.


> Jeff, I'm trying to implement SCR access on ICH6/7's on top of these
> changes and have a question about libata-bmdma.c::ata_pci_init_one().
> ata_piix needs its own version of this function as it should try to
> map ABAR for SCR (and fall back if it fails), so I'm trying to factor
> functions out of ata_pci_init_one() and call them from ata_piix.
> 
> ata_pci_init_one() currently uses two separate probe_ent for each port
> if the controller is in legacy mode, which makes the ports use
> separate host_set and thus separate spin locks.  Do they need to use
> separate spinlocks?  Or are there other reasons legacy initialization
> is done this way?

They are two totally independent ports and irqs, when in legacy mode.  I 
would turn that question around on you:  why do you feel the need to 
serialize two independent irqs with a single spinlock?  ;-)

On a side note, if you are poking in this area, also consider a related 
problem:  PATA ports on SATA controllers.  Promise, SiS, ULi and VIA all 
need some way to support programming the PATA port, but right now the 
probe_ent/port_info stuff makes it difficult to have two separate 
classes of ports, with two vastly different capabilities.


> Oh.. Another question.  When you were talking about getting PCS bits
> wrong before [3], were you talking about port map (like port 1 and 3
> map to primary master/slave) or is there something else PCS + MAP
> doesn't cover?

In the ICH6 and ICH7 docs, both PCS and MAP registers are peppered with 
"(mobile only)" and "(desktop only)" comments.

	Jeff

[PATCHSET] ata_piix: improve combined mode handling

[Tejun Heo]

Subject: [PATCHSET] put some intelligence into speed down sequence

Hello,

The current EH speed down code is more of a feature demonstration and
goes through rdiculously many meaningless steps when condition is met.
This patchset tries to put some intelligence into speed down sequence.
The goal is to achieve reasonable number of speed down steps
reasonably spaced from one another and consider NCQ, cable type and
the current protocol when determining speed down steps, while not
bloating the code too much with nitty gritty details.

Roughly, the rules are...

1. If NCQ and protocol/timeout/unknown dev errors occur, turn off NCQ

2. If excessive transfer errors occur, speed down within the current
   transfer mode (UDMA/MWDMA/PIO).  If UDMA, it's first adjusted down
   a step, if error conditions persist, 40c limit is applied.  Speed
   down is done only twice.

3. If PATA && used up all DMA speed down steps && a LOT of
   transmission/unknown errors occur, switch to PIO.  So, we never
   automatically step down to PIO on SATA.  This is intended.  Some
   SATA hdd even seems to have problem with PIO data transfer
   commands.

The last patch makes ahci report HSM violation error on spurious
completion of NCQ commands, thus causing NCQ off after several such
incidents.  These drives should be blacklisted for DMA eventually.

This patchset is against...

  upstream (eb0e63cca36a3389f0ccab4584f6d479b983fad5)
+ [1] pata_platform-fix-devres-conversion
+ [2] libata-convert-to-iomap

Ric, I guess this resolves the to-do item from you which has been
sitting in my mailbox for way too long.  What do you think about the
rules?

Thanks.

--
tejun