* [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).