[PATCHSET] sata_sil24: fixes, errata workaround and reset updates

[PATCHSET] sata_sil24: fixes, errata workaround and reset updates

[Tejun Heo]

Hello, Jeff, Carlos.

This patchset updates sata_sil24 and is composed eight patches.

#01	libata prep
#02-03	bug fix and errata workaround
#04	sil24 prep
#05	enable 64bit
#05-06	update softreset
#07	reimplement hardreset

This patchset is against...
  upstream [1]
  + libata reset updates patchset [2]

Thanks.

--
tejun

[1] c2a6585296009379e0f4eff39cdcb108b457ebf2
[2] Message-ID: <20060405131637.GB31998@htj.dyndns.org>
    (marc/gmane doesn't show the first message, don't know why)

[PATCH 2/8] sata_sil24: fix on-memory structure byteorder

[Tejun Heo]

Data structures residing on memory and fetched by the controller
should have LE ordering.  Fix it.

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

---

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

f6545e5248fe3c2c7ded43131c505032bc6ba080
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 60dd6f1..3d301b2 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -454,7 +454,7 @@ static int sil24_softreset(struct ata_po
 	 */
 	msleep(10);
 
-	prb->ctrl = PRB_CTRL_SRST;
+	prb->ctrl = cpu_to_le16(PRB_CTRL_SRST);
 	prb->fis[1] = 0; /* no PM yet */
 
 	writel((u32)paddr, port + PORT_CMD_ACTIVATE);
@@ -530,6 +530,7 @@ static void sil24_qc_prep(struct ata_que
 	union sil24_cmd_block *cb = pp->cmd_block + qc->tag;
 	struct sil24_prb *prb;
 	struct sil24_sge *sge;
+	u16 ctrl = 0;
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_PIO:
@@ -537,7 +538,6 @@ static void sil24_qc_prep(struct ata_que
 	case ATA_PROT_NODATA:
 		prb = &cb->ata.prb;
 		sge = cb->ata.sge;
-		prb->ctrl = 0;
 		break;
 
 	case ATA_PROT_ATAPI:
@@ -550,12 +550,10 @@ static void sil24_qc_prep(struct ata_que
 
 		if (qc->tf.protocol != ATA_PROT_ATAPI_NODATA) {
 			if (qc->tf.flags & ATA_TFLAG_WRITE)
-				prb->ctrl = PRB_CTRL_PACKET_WRITE;
+				ctrl = PRB_CTRL_PACKET_WRITE;
 			else
-				prb->ctrl = PRB_CTRL_PACKET_READ;
-		} else
-			prb->ctrl = 0;
-
+				ctrl = PRB_CTRL_PACKET_READ;
+		}
 		break;
 
 	default:
@@ -564,6 +562,7 @@ static void sil24_qc_prep(struct ata_que
 		BUG();
 	}
 
+	prb->ctrl = cpu_to_le16(ctrl);
 	ata_tf_to_fis(&qc->tf, prb->fis, 0);
 
 	if (qc->flags & ATA_QCFLAG_DMAMAP)
-- 
1.2.4

[PATCH 6/8] sata_sil24: put port into known state before softresetting

[Tejun Heo]

Make sure the controller has no pending commands and ready for command
before issuing SRST.

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

---

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

522c4966f33a80e263e018bbeb1a8a84bd1087e8
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index f50a728..e9058f0 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -470,6 +470,15 @@ static int sil24_softreset(struct ata_po
 	irq_enable = readl(port + PORT_IRQ_ENABLE_SET);
 	writel(irq_enable, port + PORT_IRQ_ENABLE_CLR);
 
+	/* put the port into known state */
+	writel(PORT_CS_INIT, port + PORT_CTRL_STAT);
+	if (sil24_poll_register(port + PORT_CTRL_STAT,
+				PORT_CS_INIT | PORT_CS_RDY, PORT_CS_RDY,
+				10, 100)) {
+		DPRINTK("port not ready\n");
+		return -EIO;
+	}
+
 	/*
 	 * XXX: Not sure whether the following sleep is needed or not.
 	 * The original driver had it.  So....
-- 
1.2.4

[PATCH 7/8] sata_sil24: kill 10ms sleep in softreset

[Tejun Heo]

Nothing, not the datasheet nor the errats, says this delay is
necessary and with the previous PORT_CS_INIT change, we know the
controller is in good state.  Kill 10ms sleep.

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

---

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

5f8f23ba275973ce1582a8816816ba872316d820
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index e9058f0..3d8047e 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -479,12 +479,7 @@ static int sil24_softreset(struct ata_po
 		return -EIO;
 	}
 
-	/*
-	 * XXX: Not sure whether the following sleep is needed or not.
-	 * The original driver had it.  So....
-	 */
-	msleep(10);
-
+	/* do SRST */
 	prb->ctrl = cpu_to_le16(PRB_CTRL_SRST);
 	prb->fis[1] = 0; /* no PM yet */
 
-- 
1.2.4

[PATCH 8/8] sata_sil24: reimplement hardreset

[Tejun Heo]

Reimplement hardreset according to the datasheet.  The old hardreset
didn't reset controller status and the controller might not be ready
after reset.  Also, as SStatus is a bit slow to reflect PHY status
after reset, sata_std_hardrset() doesn't wait long enough before
proceeding.

Note that as we're not depending on SStatus, we can't depend on DET==1
condition to wait for link, so use shorter timeout for no device case.

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

---

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

6d59c820f497b5af4db2532dcafab195759b7d28
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 3d8047e..b468cca 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -518,10 +518,42 @@ static int sil24_softreset(struct ata_po
 
 static int sil24_hardreset(struct ata_port *ap, unsigned int *class)
 {
-	unsigned int dummy_class;
+	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
+	const char *reason;
+	int tout_msec, rc;
+
+	/* sil24 does the right thing (tm) without any protection */
+	ata_set_sata_spd(ap);
+
+	tout_msec = 100;
+	if (sata_dev_present(ap))
+		tout_msec = 5000;
+
+	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
+	rc = sil24_poll_register(port + PORT_CTRL_STAT,
+				 PORT_CS_DEV_RST, 0, 10, tout_msec);
+
+	if (rc) {
+		if (!sata_dev_present(ap))
+			return 0;
+		reason = "link not ready";
+		goto err;
+	}
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
+		reason = "device not ready";
+		goto err;
+	}
+
+	/* SStatus is a bit sluggish (reports 0) on sil24, wait for it */
+	sil24_poll_register(port + PORT_SSTATUS, 0xf, 0x0, 10, 100);
+
+	/* no classification, just return */
+	return 0;
 
-	/* sil24 doesn't report device signature after hard reset */
-	return sata_std_hardreset(ap, &dummy_class);
+ err:
+	printk(KERN_ERR "ata%u: hardreset failed (%s)\n", ap->id, reason);
+	return -EIO;
 }
 
 static int sil24_probe_reset(struct ata_port *ap, unsigned int *classes)
-- 
1.2.4

[PATCH 5/8] sata_sil24: enable 64bit

[Tejun Heo]

Enable 64bit.

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

---

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

5cb76b87a81b68e230343cec82e75b1fb3335c73
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 0941f74..f50a728 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -480,6 +480,7 @@ static int sil24_softreset(struct ata_po
 	prb->fis[1] = 0; /* no PM yet */
 
 	writel((u32)paddr, port + PORT_CMD_ACTIVATE);
+	writel((u64)paddr >> 32, port + PORT_CMD_ACTIVATE + 4);
 
 	do {
 		irq_stat = readl(port + PORT_IRQ_STAT);
@@ -599,6 +600,8 @@ static unsigned int sil24_qc_issue(struc
 	dma_addr_t paddr = pp->cmd_block_dma + qc->tag * sizeof(*pp->cmd_block);
 
 	writel((u32)paddr, port + PORT_CMD_ACTIVATE);
+	writel((u64)paddr >> 32, port + PORT_CMD_ACTIVATE + 4);
+
 	return 0;
 }
 
@@ -935,22 +938,29 @@ static int sil24_init_one(struct pci_dev
 	/*
 	 * Configure the device
 	 */
-	/*
-	 * FIXME: This device is certainly 64-bit capable.  We just
-	 * don't know how to use it.  After fixing 32bit activation in
-	 * this function, enable 64bit masks here.
-	 */
-	rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
-	if (rc) {
-		dev_printk(KERN_ERR, &pdev->dev,
-			   "32-bit DMA enable failed\n");
-		goto out_free;
-	}
-	rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
-	if (rc) {
-		dev_printk(KERN_ERR, &pdev->dev,
-			   "32-bit consistent DMA enable failed\n");
-		goto out_free;
+	if (!pci_set_dma_mask(pdev, DMA_64BIT_MASK)) {
+		rc = pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK);
+		if (rc) {
+			rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
+			if (rc) {
+				dev_printk(KERN_ERR, &pdev->dev,
+					   "64-bit DMA enable failed\n");
+				goto out_free;
+			}
+		}
+	} else {
+		rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+		if (rc) {
+			dev_printk(KERN_ERR, &pdev->dev,
+				   "32-bit DMA enable failed\n");
+			goto out_free;
+		}
+		rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
+		if (rc) {
+			dev_printk(KERN_ERR, &pdev->dev,
+				   "32-bit consistent DMA enable failed\n");
+			goto out_free;
+		}
 	}
 
 	/* GPIO off */
@@ -1007,9 +1017,8 @@ static int sil24_init_one(struct pci_dev
 		writel(0x0000, port + PORT_CRC_ERR_CNT);
 		writel(0x0000, port + PORT_HSHK_ERR_CNT);
 
-		/* FIXME: 32bit activation? */
-		writel(0, port + PORT_ACTIVATE_UPPER_ADDR);
-		writel(PORT_CS_32BIT_ACTV, port + PORT_CTRL_STAT);
+		/* Always use 64bit activation */
+		writel(PORT_CS_32BIT_ACTV, port + PORT_CTRL_CLR);
 
 		/* Configure interrupts */
 		writel(0xffff, port + PORT_IRQ_ENABLE_CLR);
-- 
1.2.4

[PATCH 1/8] libata: export ata_set_sata_spd()

[Tejun Heo]

This will be used by LLDD hardreset implementation.

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

---

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

337e13a27cc1fd10b914af2ad247a01f9a799041
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index ed089a0..8346806 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1736,7 +1736,7 @@ int ata_set_sata_spd_needed(struct ata_p
  *	0 if spd doesn't need to be changed, 1 if spd has been
  *	changed.  -EOPNOTSUPP if SCR registers are inaccessible.
  */
-static int ata_set_sata_spd(struct ata_port *ap)
+int ata_set_sata_spd(struct ata_port *ap)
 {
 	u32 scontrol;
 
@@ -5065,6 +5065,7 @@ EXPORT_SYMBOL_GPL(ata_bmdma_irq_clear);
 EXPORT_SYMBOL_GPL(ata_bmdma_status);
 EXPORT_SYMBOL_GPL(ata_bmdma_stop);
 EXPORT_SYMBOL_GPL(ata_port_probe);
+EXPORT_SYMBOL_GPL(ata_set_sata_spd);
 EXPORT_SYMBOL_GPL(sata_phy_reset);
 EXPORT_SYMBOL_GPL(__sata_phy_reset);
 EXPORT_SYMBOL_GPL(ata_bus_reset);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 6954852..ccb4c3d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -503,6 +503,7 @@ extern void ata_port_probe(struct ata_po
 extern void __sata_phy_reset(struct ata_port *ap);
 extern void sata_phy_reset(struct ata_port *ap);
 extern void ata_bus_reset(struct ata_port *ap);
+extern int ata_set_sata_spd(struct ata_port *ap);
 extern int ata_drive_probe_reset(struct ata_port *ap,
 			ata_probeinit_fn_t probeinit,
 			ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
-- 
1.2.4

[PATCH 3/8] sata_sil24: implement loss of completion interrupt on PCI-X errta fix

[Tejun Heo]

SiI3124 might lose completion interrupt if completion interrupt occurs
shortly after SLOT_STAT register is read for the previous completion
interrupt if it is operating in PCI-X mode.

This currently doesn't trigger as libata never queues more than one
command, but it will with NCQ changes.  This patch implements the
workaround - turning on WoC and explicitly clearing interrupt.

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

---

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

1ecf730e0c67d7a94881897cd955b4623b0b9b58
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 3d301b2..86233ac 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -207,6 +207,9 @@ enum {
 	BID_SIL3132		= 1,
 	BID_SIL3131		= 2,
 
+	/* private host flags */
+	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
+
 	IRQ_STAT_4PORTS		= 0xf,
 };
 
@@ -336,7 +339,8 @@ static struct ata_port_info sil24_port_i
 		.sht		= &sil24_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  SIL24_NPORTS2FLAG(4),
+				  SIL24_NPORTS2FLAG(4) |
+				  SIL24_FLAG_PCIX_IRQ_WOC,
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
@@ -725,6 +729,10 @@ static inline void sil24_host_intr(struc
 	slot_stat = readl(port + PORT_SLOT_STAT);
 	if (!(slot_stat & HOST_SSTAT_ATTN)) {
 		struct sil24_port_priv *pp = ap->private_data;
+
+		if (ap->flags & SIL24_FLAG_PCIX_IRQ_WOC)
+			writel(PORT_IRQ_COMPLETE, port + PORT_IRQ_STAT);
+
 		/*
 		 * !HOST_SSAT_ATTN guarantees successful completion,
 		 * so reading back tf registers is unnecessary for
@@ -856,6 +864,7 @@ static int sil24_init_one(struct pci_dev
 	void __iomem *host_base = NULL;
 	void __iomem *port_base = NULL;
 	int i, rc;
+	u32 tmp;
 
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
@@ -929,13 +938,23 @@ static int sil24_init_one(struct pci_dev
 	/* GPIO off */
 	writel(0, host_base + HOST_FLASH_CMD);
 
+	/* Apply workaround for completion IRQ loss on PCI-X errata */
+	if (probe_ent->host_flags & SIL24_FLAG_PCIX_IRQ_WOC) {
+		tmp = readl(host_base + HOST_CTRL);
+		if (tmp & 0xe0000)
+			dev_printk(KERN_INFO, &pdev->dev,
+				   "Applying completion IRQ loss on PCI-X "
+				   "errata fix\n");
+		else
+			probe_ent->host_flags &= ~SIL24_FLAG_PCIX_IRQ_WOC;
+	}
+
 	/* 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;
@@ -946,6 +965,12 @@ static int sil24_init_one(struct pci_dev
 		/* Initial PHY setting */
 		writel(0x20c, port + PORT_PHY_CFG);
 
+		/* Configure IRQ WoC */
+		if (probe_ent->host_flags & SIL24_FLAG_PCIX_IRQ_WOC)
+			writel(PORT_CS_IRQ_WOC, port + PORT_CTRL_STAT);
+		else
+			writel(PORT_CS_IRQ_WOC, port + PORT_CTRL_CLR);
+
 		/* Clear port RST */
 		tmp = readl(port + PORT_CTRL_STAT);
 		if (tmp & PORT_CS_PORT_RST) {
-- 
1.2.4

[PATCH 4/8] libata: separate out sil24_poll_register()

[Tejun Heo]

This will be used by later patches.

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

---

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

dbf21a7fbc3fe926671416fecebfea70aef78c8a
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 86233ac..0941f74 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -431,6 +431,24 @@ static void sil24_tf_read(struct ata_por
 	*tf = pp->tf;
 }
 
+static int sil24_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 sil24_softreset(struct ata_port *ap, unsigned int *class)
 {
 	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
@@ -949,13 +967,12 @@ static int sil24_init_one(struct pci_dev
 			probe_ent->host_flags &= ~SIL24_FLAG_PCIX_IRQ_WOC;
 	}
 
-	/* 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;
-		int cnt;
 
 		probe_ent->port[i].cmd_addr = portu + PORT_PRB;
 		probe_ent->port[i].scr_addr = portu + PORT_SCONTROL;
@@ -975,16 +992,11 @@ 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;
-			}
-			if (tmp & PORT_CS_PORT_RST)
+			if (sil24_poll_register(port + PORT_CTRL_STAT,
+						PORT_CS_PORT_RST, 0, 10, 100))
 				dev_printk(KERN_ERR, &pdev->dev,
-				           "failed to clear port RST\n");
+					   "failed to clear RST for port %d\n",
+					   i);
 		}
 
 		/* Zero error counters. */
-- 
1.2.4

Re: [PATCHSET] sata_sil24: fixes, errata workaround and reset updates

[Tejun Heo]

Tejun Heo wrote:
> Hello, Jeff, Carlos.
> 
> This patchset updates sata_sil24 and is composed eight patches.
> 
> #01	libata prep
> #02-03	bug fix and errata workaround
> #04	sil24 prep
> #05	enable 64bit
> #05-06	update softreset
> #07	reimplement hardreset
> 
> This patchset is against...
>   upstream [1]
>   + libata reset updates patchset [2]
> 

Further testing shows more delay's are needed during hardreset and I've 
found more bugs in sata_sil24.  Will add those and repost this whole 
set.  Please ignore this patchset.

Thanks.

-- 
tejun

Re: [PATCH 4/8] libata: separate out sil24_poll_register()

[Jeff Garzik]

Tejun Heo wrote:
> This will be used by later patches.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/sata_sil24.c |   34 +++++++++++++++++++++++-----------
>  1 files changed, 23 insertions(+), 11 deletions(-)
> 
> dbf21a7fbc3fe926671416fecebfea70aef78c8a
> diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
> index 86233ac..0941f74 100644
> --- a/drivers/scsi/sata_sil24.c
> +++ b/drivers/scsi/sata_sil24.c
> @@ -431,6 +431,24 @@ static void sil24_tf_read(struct ata_por
>  	*tf = pp->tf;
>  }
>  
> +static int sil24_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));
> +

Let's not duplicate code in AHCI.  Initially the common code could live 
in libata, but let's think about the best place in the kernel for this 
function to live.

Re: [PATCH 8/8] sata_sil24: reimplement hardreset

[Jeff Garzik]

Tejun Heo wrote:
> Reimplement hardreset according to the datasheet.  The old hardreset
> didn't reset controller status and the controller might not be ready
> after reset.  Also, as SStatus is a bit slow to reflect PHY status
> after reset, sata_std_hardrset() doesn't wait long enough before
> proceeding.
> 
> Note that as we're not depending on SStatus, we can't depend on DET==1
> condition to wait for link, so use shorter timeout for no device case.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/sata_sil24.c |   38 +++++++++++++++++++++++++++++++++++---
>  1 files changed, 35 insertions(+), 3 deletions(-)
> 
> 6d59c820f497b5af4db2532dcafab195759b7d28
> diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
> index 3d8047e..b468cca 100644
> --- a/drivers/scsi/sata_sil24.c
> +++ b/drivers/scsi/sata_sil24.c
> @@ -518,10 +518,42 @@ static int sil24_softreset(struct ata_po
>  
>  static int sil24_hardreset(struct ata_port *ap, unsigned int *class)
>  {
> -	unsigned int dummy_class;
> +	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
> +	const char *reason;
> +	int tout_msec, rc;
> +
> +	/* sil24 does the right thing (tm) without any protection */
> +	ata_set_sata_spd(ap);
> +
> +	tout_msec = 100;
> +	if (sata_dev_present(ap))
> +		tout_msec = 5000;
> +
> +	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
> +	rc = sil24_poll_register(port + PORT_CTRL_STAT,
> +				 PORT_CS_DEV_RST, 0, 10, tout_msec);
> +
> +	if (rc) {
> +		if (!sata_dev_present(ap))
> +			return 0;
> +		reason = "link not ready";
> +		goto err;
> +	}
> +
> +	if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
> +		reason = "device not ready";
> +		goto err;
> +	}
> +
> +	/* SStatus is a bit sluggish (reports 0) on sil24, wait for it */
> +	sil24_poll_register(port + PORT_SSTATUS, 0xf, 0x0, 10, 100);

Its weird to poll BSY before polling SStatus.


> +	/* no classification, just return */

bad comment?

> +	return 0;
>  
> -	/* sil24 doesn't report device signature after hard reset */
> -	return sata_std_hardreset(ap, &dummy_class);
> + err:
> +	printk(KERN_ERR "ata%u: hardreset failed (%s)\n", ap->id, reason);
> +	return -EIO;
>  }
>  
>  static int sil24_probe_reset(struct ata_port *ap, unsigned int *classes)

Re: [PATCH 5/8] sata_sil24: enable 64bit

[Jeff Garzik]

Tejun Heo wrote:
> Enable 64bit.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

tested with >4GB of memory, or at least with addresses >4GB?

	Jeff

Re: [PATCH 2/8] sata_sil24: fix on-memory structure byteorder

[Jeff Garzik]

Tejun Heo wrote:
> Data structures residing on memory and fetched by the controller
> should have LE ordering.  Fix it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Do you have access to a big endian machine to test this?

	Jeff

Re: [PATCH 4/8] libata: separate out sil24_poll_register()

[Tejun Heo]

On Thu, Apr 06, 2006 at 12:42:54PM -0400, Jeff Garzik wrote:
> Tejun Heo wrote:
> >This will be used by later patches.
> >
> >Signed-off-by: Tejun Heo <htejun@gmail.com>
> >
> >---
> >
> > drivers/scsi/sata_sil24.c |   34 +++++++++++++++++++++++-----------
> > 1 files changed, 23 insertions(+), 11 deletions(-)
> >
> >dbf21a7fbc3fe926671416fecebfea70aef78c8a
> >diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
> >index 86233ac..0941f74 100644
> >--- a/drivers/scsi/sata_sil24.c
> >+++ b/drivers/scsi/sata_sil24.c
> >@@ -431,6 +431,24 @@ static void sil24_tf_read(struct ata_por
> > 	*tf = pp->tf;
> > }
> > 
> >+static int sil24_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));
> >+
> 
> Let's not duplicate code in AHCI.  Initially the common code could live 
> in libata, but let's think about the best place in the kernel for this 
> function to live.
> 

Three copies are my tipping point.  Yeah, but two are good enough for
me too.  Any ideas where this utility should live?

-- 
tejun

Re: [PATCH 3/8] sata_sil24: implement loss of completion interrupt on PCI-X errta fix

[Jeff Garzik]

Tejun Heo wrote:
> SiI3124 might lose completion interrupt if completion interrupt occurs
> shortly after SLOT_STAT register is read for the previous completion
> interrupt if it is operating in PCI-X mode.
> 
> This currently doesn't trigger as libata never queues more than one
> command, but it will with NCQ changes.  This patch implements the
> workaround - turning on WoC and explicitly clearing interrupt.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/sata_sil24.c |   29 +++++++++++++++++++++++++++--
>  1 files changed, 27 insertions(+), 2 deletions(-)
> 
> 1ecf730e0c67d7a94881897cd955b4623b0b9b58
> diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
> index 3d301b2..86233ac 100644
> --- a/drivers/scsi/sata_sil24.c
> +++ b/drivers/scsi/sata_sil24.c
> @@ -207,6 +207,9 @@ enum {
>  	BID_SIL3132		= 1,
>  	BID_SIL3131		= 2,
>  
> +	/* private host flags */
> +	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
> +
>  	IRQ_STAT_4PORTS		= 0xf,
>  };
>  
> @@ -336,7 +339,8 @@ static struct ata_port_info sil24_port_i
>  		.sht		= &sil24_sht,
>  		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
>  				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
> -				  SIL24_NPORTS2FLAG(4),
> +				  SIL24_NPORTS2FLAG(4) |
> +				  SIL24_FLAG_PCIX_IRQ_WOC,
>  		.pio_mask	= 0x1f,			/* pio0-4 */
>  		.mwdma_mask	= 0x07,			/* mwdma0-2 */
>  		.udma_mask	= 0x3f,			/* udma0-5 */
> @@ -725,6 +729,10 @@ static inline void sil24_host_intr(struc
>  	slot_stat = readl(port + PORT_SLOT_STAT);
>  	if (!(slot_stat & HOST_SSTAT_ATTN)) {
>  		struct sil24_port_priv *pp = ap->private_data;
> +
> +		if (ap->flags & SIL24_FLAG_PCIX_IRQ_WOC)
> +			writel(PORT_IRQ_COMPLETE, port + PORT_IRQ_STAT);
> +
>  		/*
>  		 * !HOST_SSAT_ATTN guarantees successful completion,
>  		 * so reading back tf registers is unnecessary for
> @@ -856,6 +864,7 @@ static int sil24_init_one(struct pci_dev
>  	void __iomem *host_base = NULL;
>  	void __iomem *port_base = NULL;
>  	int i, rc;
> +	u32 tmp;
>  
>  	if (!printed_version++)
>  		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
> @@ -929,13 +938,23 @@ static int sil24_init_one(struct pci_dev
>  	/* GPIO off */
>  	writel(0, host_base + HOST_FLASH_CMD);
>  
> +	/* Apply workaround for completion IRQ loss on PCI-X errata */
> +	if (probe_ent->host_flags & SIL24_FLAG_PCIX_IRQ_WOC) {
> +		tmp = readl(host_base + HOST_CTRL);
> +		if (tmp & 0xe0000)

Please turn this magic number (0xe0000) into a constant.

	Jeff

Re: [PATCH 8/8] sata_sil24: reimplement hardreset

[Tejun Heo]

On Thu, Apr 06, 2006 at 12:44:30PM -0400, Jeff Garzik wrote:
> >+
> >+	if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
> >+		reason = "device not ready";
> >+		goto err;
> >+	}
> >+
> >+	/* SStatus is a bit sluggish (reports 0) on sil24, wait for it */
> >+	sil24_poll_register(port + PORT_SSTATUS, 0xf, 0x0, 10, 100);
> 
> Its weird to poll BSY before polling SStatus.
> 

It turned out that the above delay is not enough.  SStatus oscillates
between zero and valid status for short duration after hardreset
resulting in weird messages like 'ata2: SATA link up <unknown>
(SStatus 0 Scontrol 310)".  I've added unconditional msleep(100) right
afer hardreset and removed this code.

> 
> >+	/* no classification, just return */
> 
> bad comment?

How about...

	/* Sil24 hardreset doesn't report device signature, leave
	 * classes[] alone.
	 */
?

> >+	return 0;
> > 
> >-	/* sil24 doesn't report device signature after hard reset */
> >-	return sata_std_hardreset(ap, &dummy_class);
> >+ err:
> >+	printk(KERN_ERR "ata%u: hardreset failed (%s)\n", ap->id, reason);
> >+	return -EIO;
> > }
> > 
> > static int sil24_probe_reset(struct ata_port *ap, unsigned int *classes)
> 

Thanks.

-- 
tejun

Re: [PATCH 8/8] sata_sil24: reimplement hardreset

[Jeff Garzik]

Tejun Heo wrote:
> On Thu, Apr 06, 2006 at 12:44:30PM -0400, Jeff Garzik wrote:
>>> +
>>> +	if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
>>> +		reason = "device not ready";
>>> +		goto err;
>>> +	}
>>> +
>>> +	/* SStatus is a bit sluggish (reports 0) on sil24, wait for it */
>>> +	sil24_poll_register(port + PORT_SSTATUS, 0xf, 0x0, 10, 100);
>> Its weird to poll BSY before polling SStatus.
>>
> 
> It turned out that the above delay is not enough.  SStatus oscillates
> between zero and valid status for short duration after hardreset
> resulting in weird messages like 'ata2: SATA link up <unknown>
> (SStatus 0 Scontrol 310)".  I've added unconditional msleep(100) right
> afer hardreset and removed this code.
> 
>>> +	/* no classification, just return */
>> bad comment?
> 
> How about...
> 
> 	/* Sil24 hardreset doesn't report device signature, leave
> 	 * classes[] alone.
> 	 */
> ?

Yes, my brain groks this much better :)

	Jeff

Re: [PATCH 5/8] sata_sil24: enable 64bit

[Tejun Heo]

On Thu, Apr 06, 2006 at 12:46:07PM -0400, Jeff Garzik wrote:
> Tejun Heo wrote:
> >Enable 64bit.
> >
> >Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> tested with >4GB of memory, or at least with addresses >4GB?
> 

No, unfortunately don't have such a machine, but I can confirm that
giving wrong upper address breaks on < 4GB machine.

-- 
tejun

Re: [PATCH 4/8] libata: separate out sil24_poll_register()

[Jeff Garzik]

Tejun Heo wrote:
> On Thu, Apr 06, 2006 at 12:42:54PM -0400, Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> This will be used by later patches.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>
>>> ---
>>>
>>> drivers/scsi/sata_sil24.c |   34 +++++++++++++++++++++++-----------
>>> 1 files changed, 23 insertions(+), 11 deletions(-)
>>>
>>> dbf21a7fbc3fe926671416fecebfea70aef78c8a
>>> diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
>>> index 86233ac..0941f74 100644
>>> --- a/drivers/scsi/sata_sil24.c
>>> +++ b/drivers/scsi/sata_sil24.c
>>> @@ -431,6 +431,24 @@ static void sil24_tf_read(struct ata_por
>>> 	*tf = pp->tf;
>>> }
>>>
>>> +static int sil24_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));
>>> +
>> Let's not duplicate code in AHCI.  Initially the common code could live 
>> in libata, but let's think about the best place in the kernel for this 
>> function to live.
>>
> 
> Three copies are my tipping point.  Yeah, but two are good enough for
> me too.  Any ideas where this utility should live?

libata-core.c for now.  Long term?  Tough question.  Probably create a 
new file in lib/ directory, or add it to lib/iomap.c.

	Jeff

Re: [PATCH 2/8] sata_sil24: fix on-memory structure byteorder

[Tejun Heo]

On Thu, Apr 06, 2006 at 12:46:51PM -0400, Jeff Garzik wrote:
> Tejun Heo wrote:
> >Data structures residing on memory and fetched by the controller
> >should have LE ordering.  Fix it.
> >
> >Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> Do you have access to a big endian machine to test this?
> 

No, unfortunately not. :(  It's obviously broken though.

-- 
tejun