linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller
@ 2012-09-04 11:08 Shaohui Xie
  2012-09-04 12:00 ` Jenkins, Clive
  2012-09-04 14:51 ` David Laight
  0 siblings, 2 replies; 4+ messages in thread
From: Shaohui Xie @ 2012-09-04 11:08 UTC (permalink / raw)
  To: jgarzik, linux-ide; +Cc: linuxppc-dev, linux-kernel, Shaohui Xie, Anju Bhartiya

The freescale V2 SATA controller checks if the received data length matches
the programmed length 'ttl', if not, it assumes that this is an error.
In ATAPI, the 'ttl' is based on max allocation length and not the actual
data transfer length, controller will raise 'DLM' (Data length Mismatch)
error bit in Hstatus register. Along with 'DLM', DE (Device error) and
FE (fatal Error) bits are also set in Hstatus register, 'E' (Internal Error)
bit is set in Serror register and CE (Command Error) and DE (Device error)
registers have the corresponding bit set. In this condition, we need to
clear errors in following way: in the service routine, based on 'DLM' flag,
HCONTROL[27] operation clears Hstatus, CE and DE registers, clear Serror
register.

Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
Signed-off-by: Anju Bhartiya <Anju.Bhartiya@freescale.com>
---
 drivers/ata/sata_fsl.c |   48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index d6577b9..00a00cc 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -143,6 +143,7 @@ enum {
 	    FATAL_ERR_CRC_ERR_RX |
 	    FATAL_ERR_FIFO_OVRFL_TX | FATAL_ERR_FIFO_OVRFL_RX,
 
+	INT_ON_DATA_LENGTH_MISMATCH = (1 << 12),
 	INT_ON_FATAL_ERR = (1 << 5),
 	INT_ON_PHYRDY_CHG = (1 << 4),
 
@@ -283,6 +284,8 @@ struct sata_fsl_host_priv {
 	int irq;
 	int data_snoop;
 	struct device_attribute intr_coalescing;
+	u32 quirks;
+#define SATA_FSL_QUIRK_V2_ERRATA        (1 << 1)
 };
 
 static void fsl_sata_set_irq_coalescing(struct ata_host *host,
@@ -1180,26 +1183,58 @@ static void sata_fsl_host_intr(struct ata_port *ap)
 	void __iomem *hcr_base = host_priv->hcr_base;
 	u32 hstatus, done_mask = 0;
 	struct ata_queued_cmd *qc;
-	u32 SError;
+	u32 SError, tag, atapi_flag = 0;
+	u32 status_mask = INT_ON_ERROR;
 
 	hstatus = ioread32(hcr_base + HSTATUS);
 
 	sata_fsl_scr_read(&ap->link, SCR_ERROR, &SError);
 
+	/* Read command completed register */
+	done_mask = ioread32(hcr_base + CC);
+
+	if (host_priv->quirks & SATA_FSL_QUIRK_V2_ERRATA) {
+		if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
+			for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
+				qc = ata_qc_from_tag(ap, tag);
+				if (qc && ata_is_atapi(qc->tf.protocol)) {
+					atapi_flag = 1;
+					break;
+				}
+			}
+		}
+	}
+
+	/* Workaround for data length mismatch errata */
+	if (atapi_flag) {
+		u32 Hcontrol;
+#define HCONTROL_CLEAR_ERROR	(1 << 27)
+		/* Set HControl[27] to clear error registers */
+		Hcontrol = ioread32(hcr_base + HCONTROL);
+		iowrite32(Hcontrol | HCONTROL_CLEAR_ERROR, hcr_base + HCONTROL);
+
+		/* Clear HControl[27] */
+		iowrite32(Hcontrol & (~HCONTROL_CLEAR_ERROR),
+						hcr_base + HCONTROL);
+
+		/* Clear SError[E] bit */
+		sata_fsl_scr_write(&ap->link, SCR_ERROR, SError);
+
+		/* Ignore fatal error and device error */
+		status_mask &= ~(INT_ON_SINGL_DEVICE_ERR | INT_ON_FATAL_ERR);
+	}
+
 	if (unlikely(SError & 0xFFFF0000)) {
 		DPRINTK("serror @host_intr : 0x%x\n", SError);
 		sata_fsl_error_intr(ap);
 	}
 
-	if (unlikely(hstatus & INT_ON_ERROR)) {
+	if (unlikely(hstatus & status_mask)) {
 		DPRINTK("error interrupt!!\n");
 		sata_fsl_error_intr(ap);
 		return;
 	}
 
-	/* Read command completed register */
-	done_mask = ioread32(hcr_base + CC);
-
 	VPRINTK("Status of all queues :\n");
 	VPRINTK("done_mask/CC = 0x%x, CA = 0x%x, CE=0x%x,CQ=0x%x,apqa=0x%x\n",
 		done_mask,
@@ -1437,6 +1472,9 @@ static int sata_fsl_probe(struct platform_device *ofdev)
 	else
 		host_priv->data_snoop = DATA_SNOOP_ENABLE_V1;
 
+	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,pq-sata-v2"))
+		host_priv->quirks |= SATA_FSL_QUIRK_V2_ERRATA;
+
 	/* allocate host structure */
 	host = ata_host_alloc_pinfo(&ofdev->dev, ppi, SATA_FSL_MAX_PORTS);
 	if (!host) {
-- 
1.6.4



^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller
  2012-09-04 11:08 [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller Shaohui Xie
@ 2012-09-04 12:00 ` Jenkins, Clive
  2012-09-04 14:51 ` David Laight
  1 sibling, 0 replies; 4+ messages in thread
From: Jenkins, Clive @ 2012-09-04 12:00 UTC (permalink / raw)
  To: Shaohui Xie, jgarzik, linux-ide; +Cc: linuxppc-dev, linux-kernel, Anju Bhartiya

> The freescale V2 SATA controller checks
> if the received data length matches
> the programmed length 'ttl', if not,
> it assumes that this is an error.
...

Can you tell us exactly what
"The freescale V2 SATA controller" is,
and what versions of what devices contain it?

Thanks,
Clive

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller
  2012-09-04 11:08 [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller Shaohui Xie
  2012-09-04 12:00 ` Jenkins, Clive
@ 2012-09-04 14:51 ` David Laight
  2012-09-05  3:20   ` Xie Shaohui-B21989
  1 sibling, 1 reply; 4+ messages in thread
From: David Laight @ 2012-09-04 14:51 UTC (permalink / raw)
  To: Shaohui Xie, jgarzik, linux-ide; +Cc: linuxppc-dev, linux-kernel, Anju Bhartiya

> +	/* Read command completed register */
> +	done_mask = ioread32(hcr_base + CC);
> +
> +	if (host_priv->quirks & SATA_FSL_QUIRK_V2_ERRATA) {
> +		if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
> +			for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
> +				qc = ata_qc_from_tag(ap, tag);
> +				if (qc && ata_is_atapi(qc->tf.protocol))
{
> +					atapi_flag = 1;
> +					break;
> +				}
> +			}
> +		}
> +	}
> +
> +	/* Workaround for data length mismatch errata */
> +	if (atapi_flag) {

Seems to me like the conditionals for this code are all
in the wrong order - adding code to the normal path.

The whole lot should probably be inside:
	if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
and the 'atapi_flag' boolean removed.

I also wonder it this is worthy of an actual quirk?
Might be worth doing anyway.

	David




^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller
  2012-09-04 14:51 ` David Laight
@ 2012-09-05  3:20   ` Xie Shaohui-B21989
  0 siblings, 0 replies; 4+ messages in thread
From: Xie Shaohui-B21989 @ 2012-09-05  3:20 UTC (permalink / raw)
  To: David Laight, jgarzik, linux-ide
  Cc: linuxppc-dev, linux-kernel, Bhartiya Anju-B07263

> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Tuesday, September 04, 2012 10:51 PM
> To: Xie Shaohui-B21989; jgarzik@pobox.com; linux-ide@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Bhartiya
> Anju-B07263
> Subject: RE: [PATCH] sata_fsl: add workaround for data length mismatch on
> freescale V2 controller
> 
> > +	/* Read command completed register */
> > +	done_mask = ioread32(hcr_base + CC);
> > +
> > +	if (host_priv->quirks & SATA_FSL_QUIRK_V2_ERRATA) {
> > +		if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
> > +			for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
> > +				qc = ata_qc_from_tag(ap, tag);
> > +				if (qc && ata_is_atapi(qc->tf.protocol))
> {
> > +					atapi_flag = 1;
> > +					break;
> > +				}
> > +			}
> > +		}
> > +	}
> > +
> > +	/* Workaround for data length mismatch errata */
> > +	if (atapi_flag) {
> 
> Seems to me like the conditionals for this code are all in the wrong
> order - adding code to the normal path.
> 
> The whole lot should probably be inside:
> 	if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) { and the
> 'atapi_flag' boolean removed.
[S.H] OK. But I need to move the "done_mask = ioread32(hcr_base + CC);" before these codes,
 because these codes will clean command completed register.

> 
> I also wonder it this is worthy of an actual quirk?
> Might be worth doing anyway.
[S.H] The quirk is useful for our internal use(there is another errata but got fixed by silicon upgrade),
but you are right it's worth doing anyway in upstream, since the upstream code only handles this errata.

Best Regards, 
Shaohui Xie


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2012-09-05  3:20 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-04 11:08 [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller Shaohui Xie
2012-09-04 12:00 ` Jenkins, Clive
2012-09-04 14:51 ` David Laight
2012-09-05  3:20   ` Xie Shaohui-B21989

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).