* [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba
@ 2005-12-14 20:10 Dan Williams
2005-12-18 9:19 ` Jeremy Higdon
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2005-12-14 20:10 UTC (permalink / raw)
To: linux-ide; +Cc: jeremy
* libata does not care about error interrupts, so handle them locally
* the interrupts that are ignored only appear to happen at init time
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/scsi/sata_vsc.c | 30 +++++++++++++++++++++++++++++-
1 files changed, 29 insertions(+), 1 deletions(-)
applies-to: 90d8277c8775f3c09655df3b48c37fe07504d162
710b992dc354832bb854b9fb05c8fc73c15664d8
diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
index cf94e01..7dcc486 100644
--- a/drivers/scsi/sata_vsc.c
+++ b/drivers/scsi/sata_vsc.c
@@ -81,6 +81,19 @@
/* Port stride */
#define VSC_SATA_PORT_OFFSET 0x200
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_E_OFFSET 2
+#define VSC_SATA_INT_ERROR_P_OFFSET 4
+#define VSC_SATA_INT_ERROR_T_OFFSET 5
+#define VSC_SATA_INT_ERROR_M_OFFSET 1
+#define is_vsc_sata_int_err(port_idx, int_status) \
+ (int_status & ((1 << (VSC_SATA_INT_ERROR_E_OFFSET + (8 * port_idx))) | \
+ (1 << (VSC_SATA_INT_ERROR_P_OFFSET + (8 * port_idx))) | \
+ (1 << (VSC_SATA_INT_ERROR_T_OFFSET + (8 * port_idx))) | \
+ (1 << (VSC_SATA_INT_ERROR_M_OFFSET + (8 * port_idx))) \
+ )\
+ )
+
static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
{
@@ -193,13 +206,28 @@ static irqreturn_t vsc_sata_interrupt (i
struct ata_port *ap;
ap = host_set->ports[i];
+
+ if (is_vsc_sata_int_err(i, int_status)) {
+ u32 err_status;
+ printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
+ err_status = ap ? vsc_sata_scr_read(ap, SCR_ERROR) : 0;
+ vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+ handled++;
+ }
+
if (ap && !(ap->flags &
(ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
struct ata_queued_cmd *qc;
qc = ata_qc_from_tag(ap, ap->active_tag);
- if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+ if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
handled += ata_host_intr(ap, qc);
+ } else {
+ printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
+ ata_chk_status(ap);
+ handled++;
+ }
+
}
}
}
---
0.99.9a
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba
2005-12-14 20:10 [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba Dan Williams
@ 2005-12-18 9:19 ` Jeremy Higdon
2005-12-27 19:27 ` Dan Williams
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Higdon @ 2005-12-18 9:19 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-ide
On Wed, Dec 14, 2005 at 01:10:49PM -0700, Dan Williams wrote:
> * libata does not care about error interrupts, so handle them locally
> * the interrupts that are ignored only appear to happen at init time
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Okay, I finally had some time to look at this. A few comments . . . .
> ---
>
> drivers/scsi/sata_vsc.c | 30 +++++++++++++++++++++++++++++-
> 1 files changed, 29 insertions(+), 1 deletions(-)
>
> applies-to: 90d8277c8775f3c09655df3b48c37fe07504d162
> 710b992dc354832bb854b9fb05c8fc73c15664d8
> diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
> index cf94e01..7dcc486 100644
> --- a/drivers/scsi/sata_vsc.c
> +++ b/drivers/scsi/sata_vsc.c
> @@ -81,6 +81,19 @@
> /* Port stride */
> #define VSC_SATA_PORT_OFFSET 0x200
>
> +/* Error interrupt status bit offsets */
> +#define VSC_SATA_INT_ERROR_E_OFFSET 2
> +#define VSC_SATA_INT_ERROR_P_OFFSET 4
> +#define VSC_SATA_INT_ERROR_T_OFFSET 5
> +#define VSC_SATA_INT_ERROR_M_OFFSET 1
> +#define is_vsc_sata_int_err(port_idx, int_status) \
> + (int_status & ((1 << (VSC_SATA_INT_ERROR_E_OFFSET + (8 * port_idx))) | \
> + (1 << (VSC_SATA_INT_ERROR_P_OFFSET + (8 * port_idx))) | \
> + (1 << (VSC_SATA_INT_ERROR_T_OFFSET + (8 * port_idx))) | \
> + (1 << (VSC_SATA_INT_ERROR_M_OFFSET + (8 * port_idx))) \
> + )\
> + )
> +
I just want to be sure I'm getting this right.
Here you're specifying an interest in the "Data Integrity", "Unrecognized FIS
Reception", "FIFO" errors, and "PHY went from Not-Ready to Ready" bits (text
is using names in VSC7174 manual). Are these the four bits that have caused
you trouble, and you haven't had problems with the "CRC Error" and "RERR Received",
and "PHY Ready Change-Of-State" bits?
> static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
> {
> @@ -193,13 +206,28 @@ static irqreturn_t vsc_sata_interrupt (i
> struct ata_port *ap;
>
> ap = host_set->ports[i];
> +
> + if (is_vsc_sata_int_err(i, int_status)) {
> + u32 err_status;
> + printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
> + err_status = ap ? vsc_sata_scr_read(ap, SCR_ERROR) : 0;
> + vsc_sata_scr_write(ap, SCR_ERROR, err_status);
> + handled++;
> + }
> +
This is short circuiting the normal LIBATA interrupt handling, right?
Later, if LIBATA becomes interested in these bits, they'll already be
cleared.
Also, if ap actually is NULL, the vsc_sata_scr_write() call will fail
when it tries to dereference ap.
> if (ap && !(ap->flags &
> (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
> struct ata_queued_cmd *qc;
>
> qc = ata_qc_from_tag(ap, ap->active_tag);
> - if (qc && (!(qc->tf.ctl & ATA_NIEN)))
> + if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
> handled += ata_host_intr(ap, qc);
> + } else {
> + printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
> + ata_chk_status(ap);
> + handled++;
> + }
> +
I don't think I fully understand the problem you're trying to solve, but
the gist seems to be that you're taking endless interrupts even when
ATA_NIEN is set. So you add the dummy ata_chk_status call to clear
interrupts that the libata core isn't interested in. But ata_chk_status
doesn't actually clear it unless you first write to the SCR_ERROR
register above?
FWIW, on my Vitesse chip, the int_status and SCR_ERROR bits that are set and
when this code is called this code are 0x83 and 0x50002, respectively. But
not clearing them doesn't seem to cause a problem . . . this must be different
behavior in your chip (presumably the Intel branch).
Can you try this redo of your patch? I get this printed with it:
vsc_sata_interrupt: clearing interrupt,status 8300; sata err status 50002
jeremy
--- linux-2.6.15-rc5/drivers/scsi/sata_vsc.c 2005-12-03 21:10:42.000000000 -0800
+++ linux-2.6.15-rc5-new/drivers/scsi/sata_vsc.c 2005-12-18 00:41:29.000000000 -0800
@@ -81,6 +81,19 @@
/* Port stride */
#define VSC_SATA_PORT_OFFSET 0x200
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_E_OFFSET 2
+#define VSC_SATA_INT_ERROR_P_OFFSET 4
+#define VSC_SATA_INT_ERROR_T_OFFSET 5
+#define VSC_SATA_INT_ERROR_M_OFFSET 1
+#define is_vsc_sata_int_err(port_idx, int_status) \
+ (int_status & ((1 << (VSC_SATA_INT_ERROR_E_OFFSET + (8 * port_idx))) | \
+ (1 << (VSC_SATA_INT_ERROR_P_OFFSET + (8 * port_idx))) | \
+ (1 << (VSC_SATA_INT_ERROR_T_OFFSET + (8 * port_idx))) | \
+ (1 << (VSC_SATA_INT_ERROR_M_OFFSET + (8 * port_idx))) \
+ )\
+ )
+
static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
{
@@ -206,8 +219,25 @@
struct ata_queued_cmd *qc;
qc = ata_qc_from_tag(ap, ap->active_tag);
- if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+ if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
handled += ata_host_intr(ap, qc);
+ } else if (is_vsc_sata_int_err(i, int_status)) {
+ /*
+ * On some chips, there will be an endless
+ * interrupt loop if the SCR_ERROR interrupts
+ * are not cleared.
+ */
+ u32 err_status;
+ err_status = vsc_sata_scr_read(ap, SCR_ERROR);
+ printk(KERN_DEBUG "%s: clearing interrupt,"
+ "status %x; sata err status %x\n",
+ __FUNCTION__,
+ int_status, err_status);
+ vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+ /* Clear interrupt status */
+ ata_chk_status(ap);
+ handled++;
+ }
}
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba
2005-12-18 9:19 ` Jeremy Higdon
@ 2005-12-27 19:27 ` Dan Williams
2006-01-05 7:57 ` Jeremy Higdon
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2005-12-27 19:27 UTC (permalink / raw)
To: Jeremy Higdon; +Cc: linux-ide
[-- Attachment #1: Type: text/plain, Size: 6666 bytes --]
On 12/18/05, Jeremy Higdon <jeremy@sgi.com> wrote:
> On Wed, Dec 14, 2005 at 01:10:49PM -0700, Dan Williams wrote:
> > * libata does not care about error interrupts, so handle them locally
> > * the interrupts that are ignored only appear to happen at init time
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Okay, I finally had some time to look at this. A few comments . . . .
>
> > ---
> >
> > drivers/scsi/sata_vsc.c | 30 +++++++++++++++++++++++++++++-
> > 1 files changed, 29 insertions(+), 1 deletions(-)
> >
> > applies-to: 90d8277c8775f3c09655df3b48c37fe07504d162
> > 710b992dc354832bb854b9fb05c8fc73c15664d8
> > diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
> > index cf94e01..7dcc486 100644
> > --- a/drivers/scsi/sata_vsc.c
> > +++ b/drivers/scsi/sata_vsc.c
> > @@ -81,6 +81,19 @@
> > /* Port stride */
> > #define VSC_SATA_PORT_OFFSET 0x200
> >
> > +/* Error interrupt status bit offsets */
> > +#define VSC_SATA_INT_ERROR_E_OFFSET 2
> > +#define VSC_SATA_INT_ERROR_P_OFFSET 4
> > +#define VSC_SATA_INT_ERROR_T_OFFSET 5
> > +#define VSC_SATA_INT_ERROR_M_OFFSET 1
> > +#define is_vsc_sata_int_err(port_idx, int_status) \
> > + (int_status & ((1 << (VSC_SATA_INT_ERROR_E_OFFSET + (8 * port_idx))) | \
> > + (1 << (VSC_SATA_INT_ERROR_P_OFFSET + (8 * port_idx))) | \
> > + (1 << (VSC_SATA_INT_ERROR_T_OFFSET + (8 * port_idx))) | \
> > + (1 << (VSC_SATA_INT_ERROR_M_OFFSET + (8 * port_idx))) \
> > + )\
> > + )
> > +
>
> I just want to be sure I'm getting this right.
> Here you're specifying an interest in the "Data Integrity", "Unrecognized FIS
> Reception", "FIFO" errors, and "PHY went from Not-Ready to Ready" bits (text
> is using names in VSC7174 manual). Are these the four bits that have caused
> you trouble, and you haven't had problems with the "CRC Error" and "RERR Received",
> and "PHY Ready Change-Of-State" bits?
In the Intel 31244 manual (pg 222
http://www.intel.com/design/storage/manuals/27360306.pdf) it
classifies some errors as diagnostic. My initial thought was to just
check for and clear non-diagnostic errors, but after working with your
patch I discovered this was not sufficient.
> > static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
> > {
> > @@ -193,13 +206,28 @@ static irqreturn_t vsc_sata_interrupt (i
> > struct ata_port *ap;
> >
> > ap = host_set->ports[i];
> > +
> > + if (is_vsc_sata_int_err(i, int_status)) {
> > + u32 err_status;
> > + printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
> > + err_status = ap ? vsc_sata_scr_read(ap, SCR_ERROR) : 0;
> > + vsc_sata_scr_write(ap, SCR_ERROR, err_status);
> > + handled++;
> > + }
> > +
>
> This is short circuiting the normal LIBATA interrupt handling, right?
> Later, if LIBATA becomes interested in these bits, they'll already be
> cleared.
See below, it turns out that clobbering these bits is not necessary,
but it would negatively impact performance to leave them set.
> Also, if ap actually is NULL, the vsc_sata_scr_write() call will fail
> when it tries to dereference ap.
Ok.
>
> > if (ap && !(ap->flags &
> > (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
> > struct ata_queued_cmd *qc;
> >
> > qc = ata_qc_from_tag(ap, ap->active_tag);
> > - if (qc && (!(qc->tf.ctl & ATA_NIEN)))
> > + if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
> > handled += ata_host_intr(ap, qc);
> > + } else {
> > + printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
> > + ata_chk_status(ap);
> > + handled++;
> > + }
> > +
>
> I don't think I fully understand the problem you're trying to solve, but
> the gist seems to be that you're taking endless interrupts even when
> ATA_NIEN is set.
Yes...
> So you add the dummy ata_chk_status call to clear
> interrupts that the libata core isn't interested in. But ata_chk_status
> doesn't actually clear it unless you first write to the SCR_ERROR
> register above?
Actually I added the dummy ata_chk_status because just clearing the
SCR_ERROR bits did not stop the stream of interrupts. Upon further
investigation it appears that clearing the SCR_ERRORS is not
_required_ and all that is necessary is the ata_chk_status() call.
however not clearing the SCR_ERROR results in the error interrupt
recurring everytime the disk is accessed.
For example:
vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002
Adding 136512k swap on /dev/sda1. Priority:-1 extents:1 across:136512k
vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002
Adding 125456k swap on /dev/sdb1. Priority:-2 extents:1 across:125456k
vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002
Adding 136512k swap on /dev/sdc1. Priority:-3 extents:1 across:136512k
> FWIW, on my Vitesse chip, the int_status and SCR_ERROR bits that are set and
> when this code is called this code are 0x83 and 0x50002, respectively. But
> not clearing them doesn't seem to cause a problem . . . this must be different
> behavior in your chip (presumably the Intel branch).
Yes, with your patch I get the same output, perhaps the Vitesse part
does not tie these bits to the error interrupt, or they are masked by
default?
I like your new organization of the code. I went ahead and revised
the patch further to make is_vsc_sata_int_err() less expensive, and
better document the need for the extra error handling code. I also
updated the Kconfig entry to make it obvious that the Intel part can
use the same driver. My apologies for attaching the patch I am still
working on the best way to get list traffic to a sane MTA on my linux
box at work.
Dan
[-- Attachment #2: sata_vsc_intel-2.6.15-rc.patch --]
[-- Type: text/x-patch, Size: 3262 bytes --]
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 20dd85a..5bde03b 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -586,10 +586,10 @@ config SCSI_SATA_VIA
If unsure, say N.
config SCSI_SATA_VITESSE
- tristate "VITESSE VSC-7174 SATA support"
+ tristate "VITESSE VSC-7174 / INTEL 31244 SATA support"
depends on SCSI_SATA && PCI
help
- This option enables support for Vitesse VSC7174 Serial ATA.
+ This option enables support for Vitesse VSC7174 and Intel 31244 Serial ATA.
If unsure, say N.
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 2282c04..72ddba9 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -2044,7 +2044,7 @@ static int atapi_qc_complete(struct ata_
else {
u8 *scsicmd = cmd->cmnd;
- if ((scsicmd[0] == INQUIRY) && ((scsicmd[1] & 0x03) == 0)) {
+ if (scsicmd[0] == INQUIRY) {
u8 *buf = NULL;
unsigned int buflen;
@@ -2058,6 +2058,9 @@ static int atapi_qc_complete(struct ata_
* device. 2) Ensure response data format / ATAPI information
* are always correct.
*/
+ /* FIXME: do we ever override EVPD pages and the like, with
+ * this code?
+ */
if (buf[2] == 0) {
buf[2] = 0x5;
buf[3] = 0x32;
diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
index fcfa486..149ed83 100644
--- a/drivers/scsi/sata_vsc.c
+++ b/drivers/scsi/sata_vsc.c
@@ -81,6 +81,22 @@
/* Port stride */
#define VSC_SATA_PORT_OFFSET 0x200
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_CRC (1 << 6)
+#define VSC_SATA_INT_ERROR_T (1 << 5)
+#define VSC_SATA_INT_ERROR_P (1 << 4)
+#define VSC_SATA_INT_ERROR_R (1 << 3)
+#define VSC_SATA_INT_ERROR_E (1 << 2)
+#define VSC_SATA_INT_ERROR_M (1 << 1)
+#define VSC_SATA_INT_PHY_CHANGE (1 << 0)
+#define VSC_SATA_INT_ERROR (VSC_SATA_INT_ERROR_CRC + VSC_SATA_INT_ERROR_T + \
+ VSC_SATA_INT_ERROR_P + VSC_SATA_INT_ERROR_R + \
+ VSC_SATA_INT_ERROR_E + VSC_SATA_INT_ERROR_M + \
+ VSC_SATA_INT_PHY_CHANGE)
+
+#define is_vsc_sata_int_err(port_idx, int_status) \
+ (int_status & (VSC_SATA_INT_ERROR << (8 * port_idx)))
+
static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
{
@@ -206,8 +222,28 @@ static irqreturn_t vsc_sata_interrupt (i
struct ata_queued_cmd *qc;
qc = ata_qc_from_tag(ap, ap->active_tag);
- if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+ if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
handled += ata_host_intr(ap, qc);
+ } else if (is_vsc_sata_int_err(i, int_status)) {
+ /*
+ * On some chips (i.e. Intel 31244), an error
+ * interrupt will sneak in at initialization
+ * time (phy state changes). Clearing the SCR
+ * error register is not required, but it prevents
+ * the phy state change interrupts from recurring
+ * later.
+ */
+ u32 err_status;
+ err_status = vsc_sata_scr_read(ap, SCR_ERROR);
+ printk(KERN_DEBUG "%s: clearing interrupt, "
+ "status %x; sata err status %x\n",
+ __FUNCTION__,
+ int_status, err_status);
+ vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+ /* Clear interrupt status */
+ ata_chk_status(ap);
+ handled++;
+ }
}
}
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba
2005-12-27 19:27 ` Dan Williams
@ 2006-01-05 7:57 ` Jeremy Higdon
2006-01-09 23:30 ` Dan Williams
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Higdon @ 2006-01-05 7:57 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-ide
On Tue, Dec 27, 2005 at 12:27:57PM -0700, Dan Williams wrote:
> On 12/18/05, Jeremy Higdon <jeremy@sgi.com> wrote:
> >
> > I don't think I fully understand the problem you're trying to solve, but
> > the gist seems to be that you're taking endless interrupts even when
> > ATA_NIEN is set.
> Yes...
>
> > So you add the dummy ata_chk_status call to clear
> > interrupts that the libata core isn't interested in. But ata_chk_status
> > doesn't actually clear it unless you first write to the SCR_ERROR
> > register above?
>
> Actually I added the dummy ata_chk_status because just clearing the
> SCR_ERROR bits did not stop the stream of interrupts. Upon further
> investigation it appears that clearing the SCR_ERRORS is not
> _required_ and all that is necessary is the ata_chk_status() call.
> however not clearing the SCR_ERROR results in the error interrupt
> recurring everytime the disk is accessed.
>
> For example:
> vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002
> vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002
> Adding 136512k swap on /dev/sda1. Priority:-1 extents:1 across:136512k
> vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002
> vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002
> Adding 125456k swap on /dev/sdb1. Priority:-2 extents:1 across:125456k
> vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002
> vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002
> Adding 136512k swap on /dev/sdc1. Priority:-3 extents:1 across:136512k
>
> > FWIW, on my Vitesse chip, the int_status and SCR_ERROR bits that are set and
> > when this code is called this code are 0x83 and 0x50002, respectively. But
> > not clearing them doesn't seem to cause a problem . . . this must be different
> > behavior in your chip (presumably the Intel branch).
>
> Yes, with your patch I get the same output, perhaps the Vitesse part
> does not tie these bits to the error interrupt, or they are masked by
> default?
Dan, please trim trailing whitespace from your patch, if you would. Other
than that, your patch to sata_vsc.c and Kconfig looks fine to me. I'm not sure
about the patch to libata-scsi.c, but jgarzik can review that one.
I checked both the Intel and Vitesse documentation, and they both indicate
that the error interrupts are masked by default. Perhaps your PROM/BIOS
software is enabling them. Ours does not, and the sata_vsc driver doesn't
either. Perhaps it should -- though since we're just ignoring them, that
doesn't make much sense. Maybe we should explicitly mask them off? Or
explicitly enable them, rather than leaving the chip in whatever state
the PROM/BIOS leaves it. Perhaps this patch is all you really need.
Can you try it?
--- linux-2.6.15/drivers/scsi/sata_vsc.c 2006-01-02 19:21:10.000000000 -0800
+++ linux-2.6.15-mask/drivers/scsi/sata_vsc.c 2006-01-04 23:55:16.000000000 -0800
@@ -108,9 +108,9 @@
VSC_SATA_INT_MASK_OFFSET + ap->port_no;
mask = readb(mask_addr);
if (ctl & ATA_NIEN)
- mask |= 0x80;
+ mask = 0x80;
else
- mask &= 0x7F;
+ mask = 0;
writeb(mask, mask_addr);
}
jeremy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba
2006-01-05 7:57 ` Jeremy Higdon
@ 2006-01-09 23:30 ` Dan Williams
2006-01-11 9:39 ` Jeremy Higdon
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2006-01-09 23:30 UTC (permalink / raw)
To: Jeremy Higdon; +Cc: linux-ide
[-- Attachment #1: Type: text/plain, Size: 3507 bytes --]
> Dan, please trim trailing whitespace from your patch, if you would. Other
> than that, your patch to sata_vsc.c and Kconfig looks fine to me. I'm not sure
> about the patch to libata-scsi.c, but jgarzik can review that one.
>
> I checked both the Intel and Vitesse documentation, and they both indicate
> that the error interrupts are masked by default. Perhaps your PROM/BIOS
> software is enabling them. Ours does not, and the sata_vsc driver doesn't
> either. Perhaps it should -- though since we're just ignoring them, that
> doesn't make much sense. Maybe we should explicitly mask them off? Or
> explicitly enable them, rather than leaving the chip in whatever state
> the PROM/BIOS leaves it. Perhaps this patch is all you really need.
> Can you try it?
>
> --- linux-2.6.15/drivers/scsi/sata_vsc.c 2006-01-02 19:21:10.000000000 -0800
> +++ linux-2.6.15-mask/drivers/scsi/sata_vsc.c 2006-01-04 23:55:16.000000000 -0800
> @@ -108,9 +108,9 @@
> VSC_SATA_INT_MASK_OFFSET + ap->port_no;
> mask = readb(mask_addr);
> if (ctl & ATA_NIEN)
> - mask |= 0x80;
> + mask = 0x80;
> else
> - mask &= 0x7F;
> + mask = 0;
> writeb(mask, mask_addr);
> }
>
Surprisingly, I still get the endless error interrupts with this patch applied.
I integrated this patch with the previous explicit clear patch, and
added debug prints to see when vsc_intr_mask_update is called relative
to the interrupt handler. Below is the debug output, and attached is
the integrated patch with the interrupt mask changes (trimmed for
white space against 2.6.15).
Dan
libata version 1.20 loaded.
sata_vsc 0000:00:01.0: version 1.1
ata1: SATA max UDMA/133 cmd 0xEE0A0200 ctl 0xEE0A0229 bmdma 0xEE0A0270 irq 25
ata2: SATA max UDMA/133 cmd 0xEE0A0400 ctl 0xEE0A0429 bmdma 0xEE0A0470 irq 25
ata3: SATA max UDMA/133 cmd 0xEE0A0600 ctl 0xEE0A0629 bmdma 0xEE0A0670 irq 25
ata4: SATA max UDMA/133 cmd 0xEE0A0800 ctl 0xEE0A0829 bmdma 0xEE0A0870 irq 25
entering vsc_intr_mask_update
exiting vsc_intr_mask_update
vsc_sata_interrupt: clearing interrupt, status 83; sata err status 50002
ata1: dev 0 cfg 49:2f00 82:74eb 83:7f63 84:4003 85:74e9 86:3c43 87:4003 88:007f
ata1: dev 0 ATA-6, max UDMA/133, 145226112 sectors: LBA48
ata1: dev 0 configured for UDMA/133
scsi0 : sata_vsc
entering vsc_intr_mask_update
exiting vsc_intr_mask_update
vsc_sata_interrupt: clearing interrupt, status 8300; sata err status 50002
ata2: dev 0 cfg 49:2f00 82:74eb 83:7f63 84:4003 85:74e9 86:3c43 87:4003 88:007f
ata2: dev 0 ATA-6, max UDMA/133, 145226112 sectors: LBA48
ata2: dev 0 configured for UDMA/133
scsi1 : sata_vsc
entering vsc_intr_mask_update
exiting vsc_intr_mask_update
vsc_sata_interrupt: clearing interrupt, status 830000; sata err status 50002
ata3: dev 0 cfg 49:2f00 82:74eb 83:7f63 84:4003 85:74e9 86:3c43 87:4003 88:007f
ata3: dev 0 ATA-6, max UDMA/133, 145226112 sectors: LBA48
ata3: dev 0 configured for UDMA/133
scsi2 : sata_vsc
ata4: no device found (phy stat 00000000)
scsi3 : sata_vsc
Vendor: ATA Model: WDC WD740GD-00FL Rev: 27.0
Type: Direct-Access ANSI SCSI revision: 05
Vendor: ATA Model: WDC WD740GD-00FL Rev: 27.0
Type: Direct-Access ANSI SCSI revision: 05
Vendor: ATA Model: WDC WD740GD-00FL Rev: 27.0
Type: Direct-Access ANSI SCSI revision: 05
[-- Attachment #2: sata_vsc_intel-2.6.15.patch --]
[-- Type: text/x-patch, Size: 3098 bytes --]
[PATCH] Clear Intel 31244 error interrupts in the sata_vsc driver
* This patch represents the result of the following thread:
http://marc.theaimsgroup.com/?t=113459121300006&r=1&w=2
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
--- linux-2.6.15/drivers/scsi/sata_vsc.c 2006-01-09 16:07:17.000000000 -0700
+++ linux-2.6/drivers/scsi/sata_vsc.c 2006-01-09 16:12:48.000000000 -0700
@@ -81,6 +81,22 @@
/* Port stride */
#define VSC_SATA_PORT_OFFSET 0x200
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_CRC (1 << 6)
+#define VSC_SATA_INT_ERROR_T (1 << 5)
+#define VSC_SATA_INT_ERROR_P (1 << 4)
+#define VSC_SATA_INT_ERROR_R (1 << 3)
+#define VSC_SATA_INT_ERROR_E (1 << 2)
+#define VSC_SATA_INT_ERROR_M (1 << 1)
+#define VSC_SATA_INT_PHY_CHANGE (1 << 0)
+#define VSC_SATA_INT_ERROR (VSC_SATA_INT_ERROR_CRC + VSC_SATA_INT_ERROR_T + \
+ VSC_SATA_INT_ERROR_P + VSC_SATA_INT_ERROR_R + \
+ VSC_SATA_INT_ERROR_E + VSC_SATA_INT_ERROR_M + \
+ VSC_SATA_INT_PHY_CHANGE)
+
+#define is_vsc_sata_int_err(port_idx, int_status) \
+ (int_status & (VSC_SATA_INT_ERROR << (8 * port_idx)))
+
static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
{
@@ -102,15 +118,15 @@
static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
{
void __iomem *mask_addr;
- u8 mask;
-
+ u8 mask;
+
mask_addr = ap->host_set->mmio_base +
VSC_SATA_INT_MASK_OFFSET + ap->port_no;
mask = readb(mask_addr);
if (ctl & ATA_NIEN)
- mask |= 0x80;
+ mask = 0x80;
else
- mask &= 0x7F;
+ mask = 0;
writeb(mask, mask_addr);
}
@@ -206,8 +222,28 @@
struct ata_queued_cmd *qc;
qc = ata_qc_from_tag(ap, ap->active_tag);
- if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+ if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
handled += ata_host_intr(ap, qc);
+ } else if (is_vsc_sata_int_err(i, int_status)) {
+ /*
+ * On some chips (i.e. Intel 31244), an error
+ * interrupt will sneak in at initialization
+ * time (phy state changes). Clearing the SCR
+ * error register is not required, but it prevents
+ * the phy state change interrupts from recurring
+ * later.
+ */
+ u32 err_status;
+ err_status = vsc_sata_scr_read(ap, SCR_ERROR);
+ printk(KERN_DEBUG "%s: clearing interrupt, "
+ "status %x; sata err status %x\n",
+ __FUNCTION__,
+ int_status, err_status);
+ vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+ /* Clear interrupt status */
+ ata_chk_status(ap);
+ handled++;
+ }
}
}
}
--- linux-2.6.15/drivers/scsi/Kconfig 2006-01-09 16:07:14.000000000 -0700
+++ linux-2.6/drivers/scsi/Kconfig 2005-12-29 11:05:06.000000000 -0700
@@ -586,10 +586,10 @@
If unsure, say N.
config SCSI_SATA_VITESSE
- tristate "VITESSE VSC-7174 SATA support"
+ tristate "VITESSE VSC-7174 / INTEL 31244 SATA support"
depends on SCSI_SATA && PCI
help
- This option enables support for Vitesse VSC7174 Serial ATA.
+ This option enables support for Vitesse VSC7174 and Intel 31244 Serial ATA.
If unsure, say N.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba
2006-01-09 23:30 ` Dan Williams
@ 2006-01-11 9:39 ` Jeremy Higdon
0 siblings, 0 replies; 6+ messages in thread
From: Jeremy Higdon @ 2006-01-11 9:39 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-ide
On Mon, Jan 09, 2006 at 04:30:56PM -0700, Dan Williams wrote:
>
> Surprisingly, I still get the endless error interrupts with this patch applied.
Do you get the endless interrupts even after the mask is cleared?
I'm thinking that maybe you're getting them before that point, in
which case, we could move the clearing earlier.
If you get error interrupts even after the mask is cleared, then
that's very strange.
jeremy
> I integrated this patch with the previous explicit clear patch, and
> added debug prints to see when vsc_intr_mask_update is called relative
> to the interrupt handler. Below is the debug output, and attached is
> the integrated patch with the interrupt mask changes (trimmed for
> white space against 2.6.15).
>
> Dan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-01-11 9:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-12-14 20:10 [PATCH 2.6.14.3] Necessary evil to get sata_vsc to initialize with Intel iq3124h hba Dan Williams
2005-12-18 9:19 ` Jeremy Higdon
2005-12-27 19:27 ` Dan Williams
2006-01-05 7:57 ` Jeremy Higdon
2006-01-09 23:30 ` Dan Williams
2006-01-11 9:39 ` Jeremy Higdon
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).