| Message ID | 1430667220-23477-2-git-send-email-stripathi@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 5/3/2015 6:33 PM, Suman Tripathi wrote: > This patch adds the support to handle HOST_IRQ_STAT as edge trigger > latch. > Signed-off-by: Suman Tripathi <stripathi@apm.com> > --- > drivers/ata/ahci.h | 2 ++ > drivers/ata/libahci.c | 19 +++++++++++++++++++ > 2 files changed, 21 insertions(+) > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index 71262e0..2df2237 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h [...] > +++ b/drivers/ata/libahci.c > @@ -1879,6 +1879,25 @@ static irqreturn_t ahci_single_irq_intr(int irq, void *dev_instance) > */ > writel(irq_stat, mmio + HOST_IRQ_STAT); > > + /* > + * HOST_IRQ_STAT behaves as edge trigger latch. When HOST_IRQ_STAT > + * detects a egde from PORT_IRQ_STAT, it happens to loose interrupts s/loose/lose/. MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 03, 2015 at 09:03:39PM +0530, Suman Tripathi wrote: > This patch adds the support to handle HOST_IRQ_STAT as edge trigger > latch. ... > + /* > + * HOST_IRQ_STAT behaves as edge trigger latch. When HOST_IRQ_STAT > + * detects a egde from PORT_IRQ_STAT, it happens to loose interrupts > + * when interrupts are triggered from both ports. So handling of > + * the residual interrupt is required. > + */ > + if (hpriv->flags & AHCI_HFLAG_EDGE_TRIG_IRQ) { > + for (i = 0; i < host->n_ports; i++) { > + struct ata_port *ap; > + > + ap = host->ports[i]; > + if (ap) { > + ahci_port_intr(ap); > + VPRINTK("Residual irq from port %u\n", i); > + } > + handled = 1; > + } > + } This is kinda gross. The right thing do is clearing irq stat registers before handling the events, right? That shouldn't be too difficult to implement. Create a separate set of irq functions which clear irqs before processing rather than after. Thanks.
On Mon, May 4, 2015 at 9:17 PM, Tejun Heo <tj@kernel.org> wrote: > On Sun, May 03, 2015 at 09:03:39PM +0530, Suman Tripathi wrote: >> This patch adds the support to handle HOST_IRQ_STAT as edge trigger >> latch. > ... >> + /* >> + * HOST_IRQ_STAT behaves as edge trigger latch. When HOST_IRQ_STAT >> + * detects a egde from PORT_IRQ_STAT, it happens to loose interrupts >> + * when interrupts are triggered from both ports. So handling of >> + * the residual interrupt is required. >> + */ >> + if (hpriv->flags & AHCI_HFLAG_EDGE_TRIG_IRQ) { >> + for (i = 0; i < host->n_ports; i++) { >> + struct ata_port *ap; >> + >> + ap = host->ports[i]; >> + if (ap) { >> + ahci_port_intr(ap); >> + VPRINTK("Residual irq from port %u\n", i); >> + } >> + handled = 1; >> + } >> + } > > This is kinda gross. The right thing do is clearing irq stat > registers before handling the events, right? That shouldn't be too > difficult to implement. Create a separate set of irq functions which > clear irqs before processing rather than after. AFAIK clearing host_irq_stat means we have handled port interrupts . Now for our case we still have interrupts left because it didn't get detected on first ahci_port_intr. So you mean to handle that residual irq in the next cycle (i mean next call intr handler ) ?? > > Thanks. > > -- > tejun
Hello, On Mon, May 04, 2015 at 10:13:11PM +0530, Suman Tripathi wrote: > AFAIK clearing host_irq_stat means we have handled port interrupts . > Now for our case we still have interrupts left because it didn't get > detected on > first ahci_port_intr. So you mean to handle that residual irq in the > next cycle (i mean next call intr handler ) ?? Heh, I think we're talking past each other. For level triggered IRQs, the latched IRQ bits should be cleared after handling the events; otherwise, the latched bits are gonna get set immediately as the events are still pending. Also, this doesn't lose any events as they're level triggered latches - if any event is pending, the IRQ is gonna be raised again. Edge triggered latches are the other way around. You should clear the latches before actually handling and clearing the events. The pending events won't trigger the latches again as it's edge-triggered and the events which happens after this irq handling starts won't get lost as they'll be latched for the next round. Thanks.
Hi Tejun, On Mon, May 4, 2015 at 11:56 PM, Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Mon, May 04, 2015 at 10:13:11PM +0530, Suman Tripathi wrote: >> AFAIK clearing host_irq_stat means we have handled port interrupts . >> Now for our case we still have interrupts left because it didn't get >> detected on >> first ahci_port_intr. So you mean to handle that residual irq in the >> next cycle (i mean next call intr handler ) ?? > > Heh, I think we're talking past each other. For level triggered IRQs, > the latched IRQ bits should be cleared after handling the events; > otherwise, the latched bits are gonna get set immediately as the > events are still pending. Also, this doesn't lose any events as > they're level triggered latches - if any event is pending, the IRQ is > gonna be raised again. > > Edge triggered latches are the other way around. You should clear the > latches before actually handling and clearing the events. The pending > events won't trigger the latches again as it's edge-triggered and the > events which happens after this irq handling starts won't get lost as > they'll be latched for the next round. Yeah it's very simple. The clearing of irq_stat needs to move up. That's it. tried and it worked. Thanks a lot !! Just a quick question : Why edge trigger handling is not yet present in libahci yet? I mean no one has used it yet ?? > > Thanks. > > -- > tejun
On Tue, May 05, 2015 at 11:25:14AM +0530, Suman Tripathi wrote: > Yeah it's very simple. The clearing of irq_stat needs to move up. > That's it. tried and it worked. Thanks a lot !! > Just a quick question : Why edge trigger handling is not yet present > in libahci yet? I mean no one has used it yet ?? Yeah, no other ahci controller uses edge triggered latches. I think ahci spec says irq is supposed to be level triggered. Thanks.
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h index 71262e0..2df2237 100644 --- a/drivers/ata/ahci.h +++ b/drivers/ata/ahci.h @@ -238,6 +238,8 @@ enum { AHCI_HFLAG_MULTI_MSI = (1 << 16), /* multiple PCI MSIs */ AHCI_HFLAG_NO_DEVSLP = (1 << 17), /* no device sleep */ AHCI_HFLAG_NO_FBS = (1 << 18), /* no FBS */ + AHCI_HFLAG_EDGE_TRIG_IRQ = (1 << 19), /* HOST_IRQ_STAT behaves as + Edge Triggered */ /* ap->flags bits */ diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index 61a9c07..0e7e8b4 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -1879,6 +1879,25 @@ static irqreturn_t ahci_single_irq_intr(int irq, void *dev_instance) */ writel(irq_stat, mmio + HOST_IRQ_STAT); + /* + * HOST_IRQ_STAT behaves as edge trigger latch. When HOST_IRQ_STAT + * detects a egde from PORT_IRQ_STAT, it happens to loose interrupts + * when interrupts are triggered from both ports. So handling of + * the residual interrupt is required. + */ + if (hpriv->flags & AHCI_HFLAG_EDGE_TRIG_IRQ) { + for (i = 0; i < host->n_ports; i++) { + struct ata_port *ap; + + ap = host->ports[i]; + if (ap) { + ahci_port_intr(ap); + VPRINTK("Residual irq from port %u\n", i); + } + handled = 1; + } + } + spin_unlock(&host->lock); VPRINTK("EXIT\n");
This patch adds the support to handle HOST_IRQ_STAT as edge trigger latch. Signed-off-by: Suman Tripathi <stripathi@apm.com> --- drivers/ata/ahci.h | 2 ++ drivers/ata/libahci.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) -- 1.8.2.1 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html