[PATCH 1/5] sata_sil24: fix timeout calculation in sil24_softreset

[PATCH 1/5] sata_sil24: fix timeout calculation in sil24_softreset

[Tejun Heo]

sil24_softreset calculated timeout by adding ATA_TMOUT_BOOT * HZ to
jiffies; however, as ATA_TMOUT_BOOT is already in jiffies, multiplying
by HZ makes the value way off.  Fix it.

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

---

 drivers/scsi/sata_sil24.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

d95c3c50429bdb09fadb3286128b7b0154116e80
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 60dd6f1..512ee6f 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -433,7 +433,7 @@ static int sil24_softreset(struct ata_po
 	struct sil24_port_priv *pp = ap->private_data;
 	struct sil24_prb *prb = &pp->cmd_block[0].ata.prb;
 	dma_addr_t paddr = pp->cmd_block_dma;
-	unsigned long timeout = jiffies + ATA_TMOUT_BOOT * HZ;
+	unsigned long timeout = jiffies + ATA_TMOUT_BOOT;
 	u32 irq_enable, irq_stat;
 
 	DPRINTK("ENTER\n");
-- 
1.2.4

[PATCHSET] libata: implement and use ata_wait_register()

[Tejun Heo]

Hello, Jeff.

This patchset implements ata_wait_register() and uses it in ahci and
sata_sil24.  The function is generalized form of ahci_poll_register()
with the following differences.

* Uses ioread32.
* The first read is done before calculating timeout such that
  unflushed io write doesn't affect timeout.  (no flush required
  before calling this function.)
* Test conditions is negated.  As it waits for something to change,
  negated test is more natural/powerful and allows this function to be
  used for wider purpos.
* Returns the last read value of @reg.

This patchset contains five patches.

#01-02	fix & prep sata_sil24 for following changes
#03-05	implement and use ata_wait_register()

This patchset is against.

     upstream [1]
     + libata reset updates patchset (3 patches) [2]

Thanks.

--
tejun

[1] c2a6585296009379e0f4eff39cdcb108b457ebf2
[2] http://article.gmane.org/gmane.linux.ide/9414

[PATCH 2/5] sata_sil24: better error message from softreset

[Tejun Heo]

Improve softreset error message.

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

---

 drivers/scsi/sata_sil24.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

7b901b3ebd190cbab8424b599221da795375f892
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 512ee6f..c75c228 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -435,6 +435,7 @@ static int sil24_softreset(struct ata_po
 	dma_addr_t paddr = pp->cmd_block_dma;
 	unsigned long timeout = jiffies + ATA_TMOUT_BOOT;
 	u32 irq_enable, irq_stat;
+	const char *reason;
 
 	DPRINTK("ENTER\n");
 
@@ -474,8 +475,11 @@ static int sil24_softreset(struct ata_po
 	writel(irq_enable, port + PORT_IRQ_ENABLE_SET);
 
 	if (!(irq_stat & PORT_IRQ_COMPLETE)) {
-		printk(KERN_ERR "ata%u: softreset failed (timeout)\n", ap->id);
-		return -EIO;
+		if (irq_stat & PORT_IRQ_ERROR)
+			reason = "SRST command error";
+		else
+			reason = "timeout";
+		goto err;
 	}
 
 	sil24_update_tf(ap);
@@ -487,6 +491,10 @@ static int sil24_softreset(struct ata_po
  out:
 	DPRINTK("EXIT, class=%u\n", *class);
 	return 0;
+
+ err:
+	printk(KERN_ERR "ata%u: softreset failed (%s)\n", ap->id, reason);
+	return -EIO;
 }
 
 static int sil24_hardreset(struct ata_port *ap, unsigned int *class)
-- 
1.2.4

[PATCH 4/5] ahci: use ata_wait_register()

[Tejun Heo]

Replace ahci_poll_register() with ata_wait_register().

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

---

 drivers/scsi/ahci.c |   31 ++++++-------------------------
 1 files changed, 6 insertions(+), 25 deletions(-)

67e414ded95efe60ef70993a1b6502c3d56a30f6
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 8f06375..241ed5d 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -517,24 +517,6 @@ static void ahci_fill_cmd_slot(struct ah
 	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
 }
 
-static int ahci_poll_register(void __iomem *reg, u32 mask, u32 val,
-			      unsigned long interval_msec,
-			      unsigned long timeout_msec)
-{
-	unsigned long timeout;
-	u32 tmp;
-
-	timeout = jiffies + (timeout_msec * HZ) / 1000;
-	do {
-		tmp = readl(reg);
-		if ((tmp & mask) == val)
-			return 0;
-		msleep(interval_msec);
-	} while (time_before(jiffies, timeout));
-
-	return -1;
-}
-
 static int ahci_softreset(struct ata_port *ap, unsigned int *class)
 {
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
@@ -544,6 +526,7 @@ static int ahci_softreset(struct ata_por
 	const u32 cmd_fis_len = 5; /* five dwords */
 	const char *reason = NULL;
 	struct ata_taskfile tf;
+	u32 tmp;
 	u8 *fis;
 	int rc;
 
@@ -565,8 +548,6 @@ static int ahci_softreset(struct ata_por
 	/* check BUSY/DRQ, perform Command List Override if necessary */
 	ahci_tf_read(ap, &tf);
 	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
-		u32 tmp;
-
 		if (!(hpriv->cap & HOST_CAP_CLO)) {
 			rc = -EIO;
 			reason = "port busy but no CLO";
@@ -576,10 +557,10 @@ static int ahci_softreset(struct ata_por
 		tmp = readl(port_mmio + PORT_CMD);
 		tmp |= PORT_CMD_CLO;
 		writel(tmp, port_mmio + PORT_CMD);
-		readl(port_mmio + PORT_CMD); /* flush */
 
-		if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0,
-				       1, 500)) {
+		tmp = ata_wait_register(port_mmio + PORT_CMD,
+					PORT_CMD_CLO, PORT_CMD_CLO, 1, 500);
+		if (tmp & PORT_CMD_CLO) {
 			rc = -EIO;
 			reason = "CLO failed";
 			goto fail_restart;
@@ -600,9 +581,9 @@ static int ahci_softreset(struct ata_por
 	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
 
 	writel(1, port_mmio + PORT_CMD_ISSUE);
-	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
 
-	if (ahci_poll_register(port_mmio + PORT_CMD_ISSUE, 0x1, 0x0, 1, 500)) {
+	tmp = ata_wait_register(port_mmio + PORT_CMD_ISSUE, 0x1, 0x1, 1, 500);
+	if (tmp & 0x1) {
 		rc = -EIO;
 		reason = "1st FIS failed";
 		goto fail;
-- 
1.2.4

[PATCH 3/5] libata: implement ata_wait_register()

[Tejun Heo]

As waiting for some register bits to change seems to be a common
operation shared by some controllers, implement helper function
ata_wait_register().  This function also takes care of register write
flushing.

Note that the condition is inverted, the wait is over when the masked
value does NOT match @val.  As we're waiting for bits to change, this
test is more powerful and allows the function to be used in more
places.

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

---

 drivers/scsi/libata-core.c |   47 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h     |    3 +++
 2 files changed, 50 insertions(+), 0 deletions(-)

9a5b48cf585e6fdfdabc64efd8c4de58db2facec
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index ed089a0..56e1cc6 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5029,6 +5029,52 @@ int ata_ratelimit(void)
 	return rc;
 }
 
+/**
+ *	ata_wait_register - wait until register value changes
+ *	@reg: IO-mapped register
+ *	@mask: Mask to apply to read register value
+ *	@val: Wait condition
+ *	@interval_msec: polling interval in milliseconds
+ *	@timeout_msec: timeout in milliseconds
+ *
+ *	Waiting for some bits of register to change is a common
+ *	operation for ATA controllers.  This function reads 32bit LE
+ *	IO-mapped register @reg and tests for the following condition.
+ *
+ *	(*@reg & mask) != val
+ *
+ *	If the condition is met, it returns; otherwise, the process is
+ *	repeated after @interval_msec until timeout.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	The final register value.
+ */
+u32 ata_wait_register(void __iomem *reg, u32 mask, u32 val,
+		      unsigned long interval_msec,
+		      unsigned long timeout_msec)
+{
+	unsigned long timeout;
+	u32 tmp;
+
+	tmp = ioread32(reg);
+
+	/* Calculate timeout _after_ the first read to make sure
+	 * preceding writes reach the controller before starting to
+	 * eat away the timeout.
+	 */
+	timeout = jiffies + (timeout_msec * HZ) / 1000;
+
+	while ((tmp & mask) == val && time_before(jiffies, timeout)) {
+		msleep(interval_msec);
+		tmp = ioread32(reg);
+	}
+
+	return tmp;
+}
+
 /*
  * libata is essentially a library of internal helper functions for
  * low-level ATA host controller drivers.  As such, the API/ABI is
@@ -5079,6 +5125,7 @@ EXPORT_SYMBOL_GPL(ata_dev_classify);
 EXPORT_SYMBOL_GPL(ata_dev_pair);
 EXPORT_SYMBOL_GPL(ata_port_disable);
 EXPORT_SYMBOL_GPL(ata_ratelimit);
+EXPORT_SYMBOL_GPL(ata_wait_register);
 EXPORT_SYMBOL_GPL(ata_busy_sleep);
 EXPORT_SYMBOL_GPL(ata_port_queue_task);
 EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 6954852..bf2f334 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -540,6 +540,9 @@ extern unsigned int ata_busy_sleep(struc
 				   unsigned long timeout);
 extern void ata_port_queue_task(struct ata_port *ap, void (*fn)(void *),
 				void *data, unsigned long delay);
+extern u32 ata_wait_register(void __iomem *reg, u32 mask, u32 val,
+			     unsigned long interval_msec,
+			     unsigned long timeout_msec);
 
 /*
  * Default driver ops implementations
-- 
1.2.4

[PATCH 5/5] sata_sil24: use ata_wait_register()

[Tejun Heo]

Replace hard-coded waiting loops in sata_sil24 to ata_wait_register().

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

---

 drivers/scsi/sata_sil24.c |   30 ++++++++++--------------------
 1 files changed, 10 insertions(+), 20 deletions(-)

27b5423eb7db51a79751f46fd73a17df4a39f851
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index c75c228..edb62c3 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -433,8 +433,7 @@ static int sil24_softreset(struct ata_po
 	struct sil24_port_priv *pp = ap->private_data;
 	struct sil24_prb *prb = &pp->cmd_block[0].ata.prb;
 	dma_addr_t paddr = pp->cmd_block_dma;
-	unsigned long timeout = jiffies + ATA_TMOUT_BOOT;
-	u32 irq_enable, irq_stat;
+	u32 mask, irq_enable, irq_stat;
 	const char *reason;
 
 	DPRINTK("ENTER\n");
@@ -460,16 +459,12 @@ static int sil24_softreset(struct ata_po
 
 	writel((u32)paddr, port + PORT_CMD_ACTIVATE);
 
-	do {
-		irq_stat = readl(port + PORT_IRQ_STAT);
-		writel(irq_stat, port + PORT_IRQ_STAT);		/* clear irq */
+	mask = (PORT_IRQ_COMPLETE | PORT_IRQ_ERROR) << PORT_IRQ_RAW_SHIFT;
+	irq_stat = ata_wait_register(port + PORT_IRQ_STAT, mask, 0x0,
+				     100, ATA_TMOUT_BOOT / HZ * 1000);
 
-		irq_stat >>= PORT_IRQ_RAW_SHIFT;
-		if (irq_stat & (PORT_IRQ_COMPLETE | PORT_IRQ_ERROR))
-			break;
-
-		msleep(100);
-	} while (time_before(jiffies, timeout));
+	writel(irq_stat, port + PORT_IRQ_STAT); /* clear IRQs */
+	irq_stat >>= PORT_IRQ_RAW_SHIFT;
 
 	/* restore IRQs */
 	writel(irq_enable, port + PORT_IRQ_ENABLE_SET);
@@ -938,14 +933,13 @@ static int sil24_init_one(struct pci_dev
 	/* GPIO off */
 	writel(0, host_base + HOST_FLASH_CMD);
 
-	/* Mask interrupts during initialization */
+	/* clear global reset & mask interrupts during initialization */
 	writel(0, host_base + HOST_CTRL);
 
 	for (i = 0; i < probe_ent->n_ports; i++) {
 		void __iomem *port = port_base + i * PORT_REGS_SIZE;
 		unsigned long portu = (unsigned long)port;
 		u32 tmp;
-		int cnt;
 
 		probe_ent->port[i].cmd_addr = portu + PORT_PRB;
 		probe_ent->port[i].scr_addr = portu + PORT_SCONTROL;
@@ -959,13 +953,9 @@ static int sil24_init_one(struct pci_dev
 		tmp = readl(port + PORT_CTRL_STAT);
 		if (tmp & PORT_CS_PORT_RST) {
 			writel(PORT_CS_PORT_RST, port + PORT_CTRL_CLR);
-			readl(port + PORT_CTRL_STAT);	/* sync */
-			for (cnt = 0; cnt < 10; cnt++) {
-				msleep(10);
-				tmp = readl(port + PORT_CTRL_STAT);
-				if (!(tmp & PORT_CS_PORT_RST))
-					break;
-			}
+			tmp = ata_wait_register(port + PORT_CTRL_STAT,
+						PORT_CS_PORT_RST,
+						PORT_CS_PORT_RST, 10, 100);
 			if (tmp & PORT_CS_PORT_RST)
 				dev_printk(KERN_ERR, &pdev->dev,
 				           "failed to clear port RST\n");
-- 
1.2.4

[PATCH 3/5] libata: implement ata_wait_register()

[Tejun Heo]

As waiting for some register bits to change seems to be a common
operation shared by some controllers, implement helper function
ata_wait_register().  This function also takes care of register write
flushing.

Note that the condition is inverted, the wait is over when the masked
value does NOT match @val.  As we're waiting for bits to change, this
test is more powerful and allows the function to be used in more
places.

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

---

 drivers/scsi/libata-core.c |   47 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h     |    3 +++
 2 files changed, 50 insertions(+), 0 deletions(-)

f91be25a88af45c5dc9c7162ebe84f0a08ca191a
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index ed089a0..56e1cc6 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5029,6 +5029,52 @@ int ata_ratelimit(void)
 	return rc;
 }
 
+/**
+ *	ata_wait_register - wait until register value changes
+ *	@reg: IO-mapped register
+ *	@mask: Mask to apply to read register value
+ *	@val: Wait condition
+ *	@interval_msec: polling interval in milliseconds
+ *	@timeout_msec: timeout in milliseconds
+ *
+ *	Waiting for some bits of register to change is a common
+ *	operation for ATA controllers.  This function reads 32bit LE
+ *	IO-mapped register @reg and tests for the following condition.
+ *
+ *	(*@reg & mask) != val
+ *
+ *	If the condition is met, it returns; otherwise, the process is
+ *	repeated after @interval_msec until timeout.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	The final register value.
+ */
+u32 ata_wait_register(void __iomem *reg, u32 mask, u32 val,
+		      unsigned long interval_msec,
+		      unsigned long timeout_msec)
+{
+	unsigned long timeout;
+	u32 tmp;
+
+	tmp = ioread32(reg);
+
+	/* Calculate timeout _after_ the first read to make sure
+	 * preceding writes reach the controller before starting to
+	 * eat away the timeout.
+	 */
+	timeout = jiffies + (timeout_msec * HZ) / 1000;
+
+	while ((tmp & mask) == val && time_before(jiffies, timeout)) {
+		msleep(interval_msec);
+		tmp = ioread32(reg);
+	}
+
+	return tmp;
+}
+
 /*
  * libata is essentially a library of internal helper functions for
  * low-level ATA host controller drivers.  As such, the API/ABI is
@@ -5079,6 +5125,7 @@ EXPORT_SYMBOL_GPL(ata_dev_classify);
 EXPORT_SYMBOL_GPL(ata_dev_pair);
 EXPORT_SYMBOL_GPL(ata_port_disable);
 EXPORT_SYMBOL_GPL(ata_ratelimit);
+EXPORT_SYMBOL_GPL(ata_wait_register);
 EXPORT_SYMBOL_GPL(ata_busy_sleep);
 EXPORT_SYMBOL_GPL(ata_port_queue_task);
 EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 6954852..bf2f334 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -540,6 +540,9 @@ extern unsigned int ata_busy_sleep(struc
 				   unsigned long timeout);
 extern void ata_port_queue_task(struct ata_port *ap, void (*fn)(void *),
 				void *data, unsigned long delay);
+extern u32 ata_wait_register(void __iomem *reg, u32 mask, u32 val,
+			     unsigned long interval_msec,
+			     unsigned long timeout_msec);
 
 /*
  * Default driver ops implementations
-- 
1.2.4