Re: [PATCH 1/3] sata_sil: add board ID for 3512

Re: [PATCH 1/3] sata_sil: add board ID for 3512

[Tejun Heo]

Tejun Heo wrote:
> 3512 is slightly different from 3112 errata-wise.  Differentiate it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 

Wow, script mix up. Sorry. Please ignore this series. Will resend.

-- 
tejun

[PATCHSET, RESEND] sata_sil: R_ERR on DMA activate FIS errata workaround

[Tejun Heo]

Hello, Jeff.

This patchset implements workaround for 3512/3114 R_ERR on DMA
activate FIS workaround.  I've confirmed the lockup and workaround on
SiI3114 with ST3120026AS (FW 3.18) and Jon Kare confirmed it on 3512
with ST3200822AS and SP2504C.  This is also consistent with a number
of lock up reports from 3512/3114 users that I personally received and
seen on linux-ide.

Mod15Write workaround masks this errata and, without m15w workaround,
this errata triggers for wider range of drives (including 7200.7's).
This seems to be the main reason for overgrowth of m15w blacklist.

The m15w workaround was applied to 3512 and 3114 until 2.6.13.
2.6.14-rc1 removed it causing lockups for 3512/3114.  I think this is
a regression (and a very bothering one) and hope this fix can slip
into 2.6.16.

Thanks.

--
tejun

[PATCH 1/3] sata_sil: add board ID for 3512

[Tejun Heo]

3512 is slightly different from 3112 errata-wise.  Differentiate it.

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

---

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

57cb119319e74c1d39981cf94669b4d38f326628
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 17f74d3..510c2e0 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -53,7 +53,8 @@ enum {
 
 	sil_3112		= 0,
 	sil_3112_m15w		= 1,
-	sil_3114		= 2,
+	sil_3512		= 2,
+	sil_3114		= 3,
 
 	SIL_FIFO_R0		= 0x40,
 	SIL_FIFO_W0		= 0x41,
@@ -90,7 +91,7 @@ static void sil_post_set_mode (struct at
 static const struct pci_device_id sil_pci_tbl[] = {
 	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
 	{ 0x1095, 0x0240, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
-	{ 0x1095, 0x3512, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
+	{ 0x1095, 0x3512, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3512 },
 	{ 0x1095, 0x3114, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3114 },
 	{ 0x1002, 0x436e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
 	{ 0x1002, 0x4379, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
@@ -185,7 +186,8 @@ static const struct ata_port_info sil_po
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
 		.port_ops	= &sil_ops,
-	}, /* sil_3112_15w - keep it sync'd w/ sil_3112 */
+	},
+	/* sil_3112_15w - keep it sync'd w/ sil_3112 */
 	{
 		.sht		= &sil_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
@@ -195,7 +197,18 @@ static const struct ata_port_info sil_po
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
 		.port_ops	= &sil_ops,
-	}, /* sil_3114 */
+	},
+	/* sil_3512 */
+	{
+		.sht		= &sil_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+		.pio_mask	= 0x1f,			/* pio0-4 */
+		.mwdma_mask	= 0x07,			/* mwdma0-2 */
+		.udma_mask	= 0x3f,			/* udma0-5 */
+		.port_ops	= &sil_ops,
+	},
+	/* sil_3114 */
 	{
 		.sht		= &sil_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-- 
1.2.1

[PATCH 3/3] sata_sil: clean up and add errata/workaround documentation

[Tejun Heo]

This patch collects into one place/update errata documentattion and
adds description of 3114 R_ERR on DMA activate FIS errata.

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

---

 drivers/scsi/sata_sil.c |   97 ++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 75 insertions(+), 22 deletions(-)

6b1cc9dff33ae20157946ae8b06f1a11079b4af6
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 9face3c..75784cd 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -30,8 +30,79 @@
  *  Documentation for SiI 3112:
  *  http://gkernel.sourceforge.net/specs/sii/3112A_SiI-DS-0095-B2.pdf.bz2
  *
+ *  Documentation for SiI 3114:
+ *  http://gkernel.sourceforge.net/specs/sii/sii-0680a-v1.31.pdf.bz2
+ *
  *  Other errata and documentation available under NDA.
  *
+ *  Currently implemented workarounds are...
+ *
+ *  1. Mod15Write errata workaround
+ *
+ *  Errata:
+ *    The Mod15Write bug is caused by the SiI3112 controller splitting
+ *    data FIS in the middle of a 512 byte block when transferring
+ *    data equal to or larger than 8k (not prohibited by the standard
+ *    but still a bit peculiar) and some Seagate drives locking up on
+ *    receiving such data FIS.
+ *
+ *  Workaround:
+ *    This is worked around by limiting max_sectors to 15 such that
+ *    write data is always smaller than 8k.  This slows down IO
+ *    severely.
+ *
+ *    This workaround is applied during sil_dev_config() and the
+ *    affected drives are marked with SIL_QUIRK_MOD15WRITE in
+ *    sil_blacklist.
+ *
+ *    The workaround slows down everything considerably and tends to
+ *    hide other problems when applied.  Due to lack of information,
+ *    this workaround had been applied when some drives show similar
+ *    symptoms (e.g. errata #3).  It seems that the blacklist
+ *    currently contains some number of unaffected drives.
+ *
+ *    As we don't know exactly which drives are affected, slow_down
+ *    module parameter is implemented which turns on this workaround
+ *    manually.
+ *
+ *  2. Maxtor udma5 max errata
+ *
+ *  Errata:
+ *    For certain Maxtor devices, we must not program the drive be
+ *    beyond udma5.
+ *
+ *  Workaround:
+ *    Limit max transfer mode to udma5.
+ *
+ *    This workaround is applied during sli_dev_config() and the
+ *    affected drives are marked with SIL_QUIRK_UDMA5MAX in
+ *    sil_blacklist.
+ *
+ *    Note that udma5 is 100Mbytes/s which is more than sufficient, so
+ *    this workaround doesn't really affect performance.
+ *
+ *  3. SiI 3512/3114 R_ERR on DMA activate FIS errta workaround
+ *
+ *  Errata:
+ *    During DMA write operations with a data length greater than 8
+ *    Kbytes, a PRD entry fetch that occurs at the same time that a
+ *    DMA Activate FIS is received may cause the controller to falsely
+ *    indicate that the DMA Activate FIS has an illegal FIS Type.
+ *    This may cause the controller to send an R_EER in response to
+ *    the DMA Active FIS.
+ *
+ *  Workaround:
+ *    By configuring bit[1:0] of the SFISCfg register to accept FIS
+ *    types other than the standard SATA defined FIS types, the
+ *    controller is prevented from falsely setting the illegal FIS
+ *    Type indicator, thus preventing the improper RERR response.  The
+ *    default value of the SFISCfg register is 0x1040_1555.  To
+ *    implement this workaround, the SFISCfg register should be set to
+ *    a value of 0x1040_1554.
+ *
+ *    This workaround is applied during controller initialization in
+ *    sil_init_one().
+ *
  */
 
 #include <linux/kernel.h>
@@ -328,28 +399,10 @@ static void sil_scr_write (struct ata_po
  *	@dev: Device to be examined
  *
  *	After the IDENTIFY [PACKET] DEVICE step is complete, and a
- *	device is known to be present, this function is called.
- *	We apply two errata fixups which are specific to Silicon Image,
- *	a Seagate and a Maxtor fixup.
- *
- *	For certain Seagate devices, we must limit the maximum sectors
- *	to under 8K.
- *
- *	For certain Maxtor devices, we must not program the drive
- *	beyond udma5.
- *
- *	Both fixups are unfairly pessimistic.  As soon as I get more
- *	information on these errata, I will create a more exhaustive
- *	list, and apply the fixups to only the specific
- *	devices/hosts/firmwares that need it.
- *
- *	20040111 - Seagate drives affected by the Mod15Write bug are blacklisted
- *	The Maxtor quirk is in the blacklist, but I'm keeping the original
- *	pessimistic fix for the following reasons...
- *	- There seems to be less info on it, only one device gleaned off the
- *	Windows	driver, maybe only one is affected.  More info would be greatly
- *	appreciated.
- *	- But then again UDMA5 is hardly anything to complain about
+ *	device is known to be present, this function is called.  This
+ *	function also applies controller/device specific erratas,
+ *	please see the comment at the top of this file for more
+ *	information.
  */
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev)
 {
-- 
1.2.1

[PATCH 2/3] sata_sil: implement R_ERR on DMA activate FIS errata fix

[Tejun Heo]

Silicon Image has disclosed a new sil3114/3152 errata and workaround
which causes the controller to return R_ERR on DMA activate FIS if the
FIS is received while the next PRD is being fetched.  This patch
implements the workaround.

This errata results in lock up and doesn't trigger if m15w workaround
is in effect.  We stopped applying m15w to 3512 and 3114 in 2.6.14-rc1
which makes 3512/3114 lock up with some drives on all kernel versions
since 2.6.14-rc1 upto now (2.6.16-rc4).  This patch should fix the
regression.

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

---

 drivers/scsi/sata_sil.c |   33 +++++++++++++++++++++++++++------
 1 files changed, 27 insertions(+), 6 deletions(-)

005af9eeb133470338eee58701a9dbdbf2e4e939
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 510c2e0..9face3c 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -49,6 +49,7 @@
 #define DRV_VERSION	"0.9"
 
 enum {
+	SIL_FLAG_RERR_ON_DMA_ACT = (1 << 29),
 	SIL_FLAG_MOD15WRITE	= (1 << 30),
 
 	sil_3112		= 0,
@@ -202,7 +203,8 @@ static const struct ata_port_info sil_po
 	{
 		.sht		= &sil_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
+				  SIL_FLAG_RERR_ON_DMA_ACT,
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
@@ -212,7 +214,8 @@ static const struct ata_port_info sil_po
 	{
 		.sht		= &sil_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
+				  SIL_FLAG_RERR_ON_DMA_ACT,
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
@@ -229,12 +232,13 @@ static const struct {
 	unsigned long scr;	/* SATA control register block */
 	unsigned long sien;	/* SATA Interrupt Enable register */
 	unsigned long xfer_mode;/* data transfer mode register */
+	unsigned long sfis_cfg;	/* SATA FIS reception config register */
 } sil_port[] = {
 	/* port 0 ... */
-	{ 0x80, 0x8A, 0x00, 0x100, 0x148, 0xb4 },
-	{ 0xC0, 0xCA, 0x08, 0x180, 0x1c8, 0xf4 },
-	{ 0x280, 0x28A, 0x200, 0x300, 0x348, 0x2b4 },
-	{ 0x2C0, 0x2CA, 0x208, 0x380, 0x3c8, 0x2f4 },
+	{ 0x80, 0x8A, 0x00, 0x100, 0x148, 0xb4, 0x14c },
+	{ 0xC0, 0xCA, 0x08, 0x180, 0x1c8, 0xf4, 0x1cc },
+	{ 0x280, 0x28A, 0x200, 0x300, 0x348, 0x2b4, 0x34c },
+	{ 0x2C0, 0x2CA, 0x208, 0x380, 0x3c8, 0x2f4, 0x3cc },
 	/* ... port 3 */
 };
 
@@ -484,6 +488,23 @@ static int sil_init_one (struct pci_dev 
 		dev_printk(KERN_WARNING, &pdev->dev,
 			 "cache line size not set.  Driver may not function\n");
 
+	/* Apply R_ERR on DMA activate FIS errata workaround */
+	if (probe_ent->host_flags & SIL_FLAG_RERR_ON_DMA_ACT) {
+		int cnt;
+
+		for (i = 0, cnt = 0; i < probe_ent->n_ports; i++) {
+			tmp = readl(mmio_base + sil_port[i].sfis_cfg);
+			if ((tmp & 0x3) != 0x01)
+				continue;
+			if (!cnt)
+				dev_printk(KERN_INFO, &pdev->dev,
+					   "Applying R_ERR on DMA activate "
+					   "FIS errata fix\n");
+			writel(tmp & ~0x3, mmio_base + sil_port[i].sfis_cfg);
+			cnt++;
+		}
+	}
+
 	if (ent->driver_data == sil_3114) {
 		irq_mask = SIL_MASK_4PORT;
 
-- 
1.2.1

Re: [PATCH 3/3] sata_sil: clean up and add errata/workaround documentation

[Jeff Garzik]

Tejun Heo wrote:
> This patch collects into one place/update errata documentattion and
> adds description of 3114 R_ERR on DMA activate FIS errata.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/sata_sil.c |   97 ++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 75 insertions(+), 22 deletions(-)
> 
> 6b1cc9dff33ae20157946ae8b06f1a11079b4af6
> diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
> index 9face3c..75784cd 100644
> --- a/drivers/scsi/sata_sil.c
> +++ b/drivers/scsi/sata_sil.c
> @@ -30,8 +30,79 @@
>   *  Documentation for SiI 3112:
>   *  http://gkernel.sourceforge.net/specs/sii/3112A_SiI-DS-0095-B2.pdf.bz2
>   *
> + *  Documentation for SiI 3114:
> + *  http://gkernel.sourceforge.net/specs/sii/sii-0680a-v1.31.pdf.bz2
> + *
>   *  Other errata and documentation available under NDA.

I'll apply patches 1-2.

For this patch #3, two comments:

* the above URL points to CMD680 docs, not 3114

* I'm unconvinced that long exposition should be in the .c file.  I 
think its far better to put it into Documentation/DocBook/libata.tmpl. 
I have some long term plans to add more hardware details to the docbook 
docs, infact.

	Jeff

Re: [PATCH 3/3] sata_sil: clean up and add errata/workaround documentation

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> This patch collects into one place/update errata documentattion and
>> adds description of 3114 R_ERR on DMA activate FIS errata.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> ---
>>
>>  drivers/scsi/sata_sil.c |   97 
>> ++++++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 75 insertions(+), 22 deletions(-)
>>
>> 6b1cc9dff33ae20157946ae8b06f1a11079b4af6
>> diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
>> index 9face3c..75784cd 100644
>> --- a/drivers/scsi/sata_sil.c
>> +++ b/drivers/scsi/sata_sil.c
>> @@ -30,8 +30,79 @@
>>   *  Documentation for SiI 3112:
>>   *  
>> http://gkernel.sourceforge.net/specs/sii/3112A_SiI-DS-0095-B2.pdf.bz2
>>   *
>> + *  Documentation for SiI 3114:
>> + *  http://gkernel.sourceforge.net/specs/sii/sii-0680a-v1.31.pdf.bz2
>> + *
>>   *  Other errata and documentation available under NDA.
> 
> 
> I'll apply patches 1-2.
> 
> For this patch #3, two comments:
> 
> * the above URL points to CMD680 docs, not 3114

Oops. :-p

> 
> * I'm unconvinced that long exposition should be in the .c file.  I 
> think its far better to put it into Documentation/DocBook/libata.tmpl. I 
> have some long term plans to add more hardware details to the docbook 
> docs, infact.

Hmmm... I don't know. You know I'm not a big fan of docbook docs mainly 
because of accessibility. It's your call. Anyways, it would be nice to 
keep up-to-date information regarding libata hardware quirks in some 
easily-accessible place (wiki?).

-- 
tejun

Re: [PATCH 3/3] sata_sil: clean up and add errata/workaround documentation

[Jeff Garzik]

Tejun Heo wrote:
> Hmmm... I don't know. You know I'm not a big fan of docbook docs mainly 
> because of accessibility. It's your call. Anyways, it would be nice to 
> keep up-to-date information regarding libata hardware quirks in some 
> easily-accessible place (wiki?).

The docbook docs are a great place to keep such information, since they 
are archived all over the place, available even to disconnected users.

It is quite easy to do "make htmldocs" and get that posted on 
linux-ata.org, if people would like...

	Jeff