Re: DWC3: Event Interrupt Mask issue
Hi, On Tue, Jul 16, 2013 at 10:25:09AM +0800, Huang Rui wrote: sorry for the delay, but I just tested my 'testing' branch on OMAP5 uEVM board and it's working fine, then I merged my dwc3-no-oneshot branch on top and I still have dwc3 working. So this patch which I sent you is alright. I'll queue it for v3.12. Thank you, I saw that patch. But I just knew my HW board would be received in early September. Apology for that. When I get the board, I will test it at once. BTW, I have another small fix which sent last month: http://marc.info/?l=linux-usbm=137105158909488w=2 Could you kindly also queue it in your test branch? :) now done. -- balbi signature.asc Description: Digital signature
Re: DWC3: Event Interrupt Mask issue
On Fri, Jun 14, 2013 at 09:37:54AM +0800, Huang Rui wrote: On Thu, Jun 13, 2013 at 10:20:53PM +0800, Felipe Balbi wrote: Hi, On Thu, Jun 13, 2013 at 08:26:12PM +0800, Huang Rui wrote: I was reading dwc3 codes and found that during the process of configuring event buffer (dwc3_event_buffers_setup), it only write the size of the buffer and doesn't write interrupt mask bit into GEVNTSIZ register like below, dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), evt-length 0x); But in spec, it suggests that write this bit to prevent the interrupt from being generated in an event buffer configuration. So need we set where does it say that ? I just re-read details about that bit and all it says is that it can be used to mask the interrupt, but events will still be queued. Maybe I'm missing some part. Which revision of the databook are you reading ? Thanks a lot to look back into this issue. I read version 2.50a, in section 8.2.2, the step 3 to configure an Event Buffer describes: Writes the size of the buffer and interrupt mask into GEVNTSIZn. Depending on your system interrupt latency, enough Event Buffer space must be allocated to avoid lost interrupts or reduced performance. If I misunderstood, please correct me. but we are writing Interrupt mask bit, just always writing 0 :-) You're right. :) Anyway, we don't really need that bit right now because linux will only enable the IRQ line after request_*irq() has been called and we're setting up our event buffers before calling that. Yeah, when the event buffers are set up, it must not encounter any interrupts. right. OTOH, we could use that bit as means to get rid of IRQF_ONESHOT from DWC3 driver. Thanks to teach me. The IRQF_ONESHOT interrupt should also prevent the other interrupts until the current thread has been run, just like the function of interrupt mask bit, am I right? that's correct. IRQ subsystem makes sure to keep IRQs masked until thread has finished running. Can you test a patch for me ? I don't have access to HW right now. I assume the patch below works fine, does it ? Sorry, I haven't got the board yet. Your patch looks good, and I will test it as soon as I get HW. alright, so it's likely that I'll get access to my stuff back before. Anyway, if you happen to have time, I'd be glad to see a Tested-by, always good to test on more than a single platform. With my pleasure if I get board at that time. sorry for the delay, but I just tested my 'testing' branch on OMAP5 uEVM board and it's working fine, then I merged my dwc3-no-oneshot branch on top and I still have dwc3 working. So this patch which I sent you is alright. I'll queue it for v3.12. -- balbi signature.asc Description: Digital signature
Re: DWC3: Event Interrupt Mask issue
Hi Felipe, On Mon, Jul 15, 2013 at 11:48:01PM +0800, Felipe Balbi wrote: On Fri, Jun 14, 2013 at 09:37:54AM +0800, Huang Rui wrote: On Thu, Jun 13, 2013 at 10:20:53PM +0800, Felipe Balbi wrote: Hi, On Thu, Jun 13, 2013 at 08:26:12PM +0800, Huang Rui wrote: I was reading dwc3 codes and found that during the process of configuring event buffer (dwc3_event_buffers_setup), it only write the size of the buffer and doesn't write interrupt mask bit into GEVNTSIZ register like below, dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), evt-length 0x); But in spec, it suggests that write this bit to prevent the interrupt from being generated in an event buffer configuration. So need we set where does it say that ? I just re-read details about that bit and all it says is that it can be used to mask the interrupt, but events will still be queued. Maybe I'm missing some part. Which revision of the databook are you reading ? Thanks a lot to look back into this issue. I read version 2.50a, in section 8.2.2, the step 3 to configure an Event Buffer describes: Writes the size of the buffer and interrupt mask into GEVNTSIZn. Depending on your system interrupt latency, enough Event Buffer space must be allocated to avoid lost interrupts or reduced performance. If I misunderstood, please correct me. but we are writing Interrupt mask bit, just always writing 0 :-) You're right. :) Anyway, we don't really need that bit right now because linux will only enable the IRQ line after request_*irq() has been called and we're setting up our event buffers before calling that. Yeah, when the event buffers are set up, it must not encounter any interrupts. right. OTOH, we could use that bit as means to get rid of IRQF_ONESHOT from DWC3 driver. Thanks to teach me. The IRQF_ONESHOT interrupt should also prevent the other interrupts until the current thread has been run, just like the function of interrupt mask bit, am I right? that's correct. IRQ subsystem makes sure to keep IRQs masked until thread has finished running. Can you test a patch for me ? I don't have access to HW right now. I assume the patch below works fine, does it ? Sorry, I haven't got the board yet. Your patch looks good, and I will test it as soon as I get HW. alright, so it's likely that I'll get access to my stuff back before. Anyway, if you happen to have time, I'd be glad to see a Tested-by, always good to test on more than a single platform. With my pleasure if I get board at that time. sorry for the delay, but I just tested my 'testing' branch on OMAP5 uEVM board and it's working fine, then I merged my dwc3-no-oneshot branch on top and I still have dwc3 working. So this patch which I sent you is alright. I'll queue it for v3.12. Thank you, I saw that patch. But I just knew my HW board would be received in early September. Apology for that. When I get the board, I will test it at once. BTW, I have another small fix which sent last month: http://marc.info/?l=linux-usbm=137105158909488w=2 Could you kindly also queue it in your test branch? :) Thanks, Rui -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: DWC3: Event Interrupt Mask issue
Hi Felipe,
On Thu, Jun 13, 2013 at 02:01:04AM +0800, Felipe Balbi wrote:
Hi,
On Fri, Jun 07, 2013 at 03:50:17PM +0800, Huang Rui wrote:
Hi Felipe,
I was reading dwc3 codes and found that during the process of
configuring event buffer (dwc3_event_buffers_setup), it only write the
size of the buffer and doesn't write interrupt mask bit into GEVNTSIZ
register like below,
dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n),
evt-length 0x);
But in spec, it suggests that write this bit to prevent the interrupt
from being generated in an event buffer configuration. So need we set
where does it say that ? I just re-read details about that bit and all
it says is that it can be used to mask the interrupt, but events will
still be queued. Maybe I'm missing some part. Which revision of the
databook are you reading ?
Thanks a lot to look back into this issue. I read version 2.50a, in
section 8.2.2, the step 3 to configure an Event Buffer describes:
Writes the size of the buffer and interrupt mask into GEVNTSIZn.
Depending on your system interrupt latency, enough Event Buffer space
must be allocated to avoid lost interrupts or reduced performance.
If I misunderstood, please correct me.
Anyway, we don't really need that bit right now because linux will only
enable the IRQ line after request_*irq() has been called and we're
setting up our event buffers before calling that.
Yeah, when the event buffers are set up, it must not encounter any
interrupts.
OTOH, we could use that bit as means to get rid of IRQF_ONESHOT from
DWC3 driver.
Thanks to teach me. The IRQF_ONESHOT interrupt should also prevent the
other interrupts until the current thread has been run, just like the
function of interrupt mask bit, am I right?
Can you test a patch for me ? I don't have access to HW right now. I
assume the patch below works fine, does it ?
Sorry, I haven't got the board yet. Your patch looks good, and I will
test it as soon as I get HW.
(it's not final yet, I will still break it down into proper patches)
8--- cut here
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c35d49d..be74df6 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -236,7 +236,7 @@ static int dwc3_event_buffers_setup(struct dwc3 *dwc)
dwc3_writel(dwc-regs, DWC3_GEVNTADRHI(n),
upper_32_bits(evt-dma));
dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n),
- evt-length 0x);
+ DWC3_GEVNTSIZ_SIZE(evt-length));
dwc3_writel(dwc-regs, DWC3_GEVNTCOUNT(n), 0);
}
@@ -255,7 +255,8 @@ static void dwc3_event_buffers_cleanup(struct dwc3 *dwc)
dwc3_writel(dwc-regs, DWC3_GEVNTADRLO(n), 0);
dwc3_writel(dwc-regs, DWC3_GEVNTADRHI(n), 0);
- dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), 0);
+ dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), DWC3_GEVNTSIZ_INTMASK
+ | DWC3_GEVNTSIZ_SIZE(0));
dwc3_writel(dwc-regs, DWC3_GEVNTCOUNT(n), 0);
}
}
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b69d322..d2ceb82 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -194,6 +194,10 @@
#define DWC3_GTXFIFOSIZ_TXFDEF(n)((n) 0x)
#define DWC3_GTXFIFOSIZ_TXFSTADDR(n) ((n) 0x)
+/* Global Event Size Registers */
+#define DWC3_GEVNTSIZ_INTMASK(1 31)
+#define DWC3_GEVNTSIZ_SIZE(n)((n) 0x)
+
/* Global HWPARAMS1 Register */
#define DWC3_GHWPARAMS1_EN_PWROPT(n) (((n) (3 24)) 24)
#define DWC3_GHWPARAMS1_EN_PWROPT_NO 0
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2b6e7e0..dc7ee3d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1567,7 +1567,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
irq = platform_get_irq(to_platform_device(dwc-dev), 0);
ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
- IRQF_SHARED | IRQF_ONESHOT, dwc3, dwc);
+ IRQF_SHARED, dwc3, dwc);
if (ret) {
dev_err(dwc-dev, failed to request irq #%d -- %d\n,
irq, ret);
@@ -2491,6 +2491,7 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void
*_dwc)
struct dwc3 *dwc = _dwc;
unsigned long flags;
irqreturn_t ret = IRQ_NONE;
+ u32 reg;
int i;
spin_lock_irqsave(dwc-lock, flags);
@@ -2530,6 +2531,11 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void
*_dwc)
evt-count = 0;
evt-flags = ~DWC3_EVENT_PENDING;
ret = IRQ_HANDLED;
+
+ /* Unask interrupt */
+ reg = dwc3_readl(dwc-regs, DWC3_GEVNTSIZ(i));
+
Re: DWC3: Event Interrupt Mask issue
Hi, On Thu, Jun 13, 2013 at 08:26:12PM +0800, Huang Rui wrote: I was reading dwc3 codes and found that during the process of configuring event buffer (dwc3_event_buffers_setup), it only write the size of the buffer and doesn't write interrupt mask bit into GEVNTSIZ register like below, dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), evt-length 0x); But in spec, it suggests that write this bit to prevent the interrupt from being generated in an event buffer configuration. So need we set where does it say that ? I just re-read details about that bit and all it says is that it can be used to mask the interrupt, but events will still be queued. Maybe I'm missing some part. Which revision of the databook are you reading ? Thanks a lot to look back into this issue. I read version 2.50a, in section 8.2.2, the step 3 to configure an Event Buffer describes: Writes the size of the buffer and interrupt mask into GEVNTSIZn. Depending on your system interrupt latency, enough Event Buffer space must be allocated to avoid lost interrupts or reduced performance. If I misunderstood, please correct me. but we are writing Interrupt mask bit, just always writing 0 :-) Anyway, we don't really need that bit right now because linux will only enable the IRQ line after request_*irq() has been called and we're setting up our event buffers before calling that. Yeah, when the event buffers are set up, it must not encounter any interrupts. right. OTOH, we could use that bit as means to get rid of IRQF_ONESHOT from DWC3 driver. Thanks to teach me. The IRQF_ONESHOT interrupt should also prevent the other interrupts until the current thread has been run, just like the function of interrupt mask bit, am I right? that's correct. IRQ subsystem makes sure to keep IRQs masked until thread has finished running. Can you test a patch for me ? I don't have access to HW right now. I assume the patch below works fine, does it ? Sorry, I haven't got the board yet. Your patch looks good, and I will test it as soon as I get HW. alright, so it's likely that I'll get access to my stuff back before. Anyway, if you happen to have time, I'd be glad to see a Tested-by, always good to test on more than a single platform. -- balbi signature.asc Description: Digital signature
Re: DWC3: Event Interrupt Mask issue
On Thu, Jun 13, 2013 at 10:20:53PM +0800, Felipe Balbi wrote: Hi, On Thu, Jun 13, 2013 at 08:26:12PM +0800, Huang Rui wrote: I was reading dwc3 codes and found that during the process of configuring event buffer (dwc3_event_buffers_setup), it only write the size of the buffer and doesn't write interrupt mask bit into GEVNTSIZ register like below, dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), evt-length 0x); But in spec, it suggests that write this bit to prevent the interrupt from being generated in an event buffer configuration. So need we set where does it say that ? I just re-read details about that bit and all it says is that it can be used to mask the interrupt, but events will still be queued. Maybe I'm missing some part. Which revision of the databook are you reading ? Thanks a lot to look back into this issue. I read version 2.50a, in section 8.2.2, the step 3 to configure an Event Buffer describes: Writes the size of the buffer and interrupt mask into GEVNTSIZn. Depending on your system interrupt latency, enough Event Buffer space must be allocated to avoid lost interrupts or reduced performance. If I misunderstood, please correct me. but we are writing Interrupt mask bit, just always writing 0 :-) You're right. :) Anyway, we don't really need that bit right now because linux will only enable the IRQ line after request_*irq() has been called and we're setting up our event buffers before calling that. Yeah, when the event buffers are set up, it must not encounter any interrupts. right. OTOH, we could use that bit as means to get rid of IRQF_ONESHOT from DWC3 driver. Thanks to teach me. The IRQF_ONESHOT interrupt should also prevent the other interrupts until the current thread has been run, just like the function of interrupt mask bit, am I right? that's correct. IRQ subsystem makes sure to keep IRQs masked until thread has finished running. Can you test a patch for me ? I don't have access to HW right now. I assume the patch below works fine, does it ? Sorry, I haven't got the board yet. Your patch looks good, and I will test it as soon as I get HW. alright, so it's likely that I'll get access to my stuff back before. Anyway, if you happen to have time, I'd be glad to see a Tested-by, always good to test on more than a single platform. With my pleasure if I get board at that time. Best Regards, Rui -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: DWC3: Event Interrupt Mask issue
Hi,
On Fri, Jun 07, 2013 at 03:50:17PM +0800, Huang Rui wrote:
Hi Felipe,
I was reading dwc3 codes and found that during the process of
configuring event buffer (dwc3_event_buffers_setup), it only write the
size of the buffer and doesn't write interrupt mask bit into GEVNTSIZ
register like below,
dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n),
evt-length 0x);
But in spec, it suggests that write this bit to prevent the interrupt
from being generated in an event buffer configuration. So need we set
where does it say that ? I just re-read details about that bit and all
it says is that it can be used to mask the interrupt, but events will
still be queued. Maybe I'm missing some part. Which revision of the
databook are you reading ?
Anyway, we don't really need that bit right now because linux will only
enable the IRQ line after request_*irq() has been called and we're
setting up our event buffers before calling that.
OTOH, we could use that bit as means to get rid of IRQF_ONESHOT from
DWC3 driver.
Can you test a patch for me ? I don't have access to HW right now. I
assume the patch below works fine, does it ?
(it's not final yet, I will still break it down into proper patches)
8--- cut here
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c35d49d..be74df6 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -236,7 +236,7 @@ static int dwc3_event_buffers_setup(struct dwc3 *dwc)
dwc3_writel(dwc-regs, DWC3_GEVNTADRHI(n),
upper_32_bits(evt-dma));
dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n),
- evt-length 0x);
+ DWC3_GEVNTSIZ_SIZE(evt-length));
dwc3_writel(dwc-regs, DWC3_GEVNTCOUNT(n), 0);
}
@@ -255,7 +255,8 @@ static void dwc3_event_buffers_cleanup(struct dwc3 *dwc)
dwc3_writel(dwc-regs, DWC3_GEVNTADRLO(n), 0);
dwc3_writel(dwc-regs, DWC3_GEVNTADRHI(n), 0);
- dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), 0);
+ dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), DWC3_GEVNTSIZ_INTMASK
+ | DWC3_GEVNTSIZ_SIZE(0));
dwc3_writel(dwc-regs, DWC3_GEVNTCOUNT(n), 0);
}
}
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b69d322..d2ceb82 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -194,6 +194,10 @@
#define DWC3_GTXFIFOSIZ_TXFDEF(n) ((n) 0x)
#define DWC3_GTXFIFOSIZ_TXFSTADDR(n) ((n) 0x)
+/* Global Event Size Registers */
+#define DWC3_GEVNTSIZ_INTMASK (1 31)
+#define DWC3_GEVNTSIZ_SIZE(n) ((n) 0x)
+
/* Global HWPARAMS1 Register */
#define DWC3_GHWPARAMS1_EN_PWROPT(n) (((n) (3 24)) 24)
#define DWC3_GHWPARAMS1_EN_PWROPT_NO 0
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2b6e7e0..dc7ee3d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1567,7 +1567,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
irq = platform_get_irq(to_platform_device(dwc-dev), 0);
ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
- IRQF_SHARED | IRQF_ONESHOT, dwc3, dwc);
+ IRQF_SHARED, dwc3, dwc);
if (ret) {
dev_err(dwc-dev, failed to request irq #%d -- %d\n,
irq, ret);
@@ -2491,6 +2491,7 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void
*_dwc)
struct dwc3 *dwc = _dwc;
unsigned long flags;
irqreturn_t ret = IRQ_NONE;
+ u32 reg;
int i;
spin_lock_irqsave(dwc-lock, flags);
@@ -2530,6 +2531,11 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void
*_dwc)
evt-count = 0;
evt-flags = ~DWC3_EVENT_PENDING;
ret = IRQ_HANDLED;
+
+ /* Unask interrupt */
+ reg = dwc3_readl(dwc-regs, DWC3_GEVNTSIZ(i));
+ reg = ~DWC3_GEVNTSIZ_INTMASK;
+ dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(i), reg);
}
spin_unlock_irqrestore(dwc-lock, flags);
@@ -2541,6 +2547,7 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3
*dwc, u32 buf)
{
struct dwc3_event_buffer *evt;
u32 count;
+ u32 reg;
evt = dwc-ev_buffs[buf];
@@ -2552,6 +2559,11 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3
*dwc, u32 buf)
evt-count = count;
evt-flags |= DWC3_EVENT_PENDING;
+ /* Mask interrupt */
+ reg = dwc3_readl(dwc-regs, DWC3_GEVNTSIZ(buf));
+ reg |= DWC3_GEVNTSIZ_INTMASK;
+ dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(buf), reg);
+
return IRQ_WAKE_THREAD;
}
--
balbi
signature.asc
Description: Digital signature
DWC3: Event Interrupt Mask issue
Hi Felipe, I was reading dwc3 codes and found that during the process of configuring event buffer (dwc3_event_buffers_setup), it only write the size of the buffer and doesn't write interrupt mask bit into GEVNTSIZ register like below, dwc3_writel(dwc-regs, DWC3_GEVNTSIZ(n), evt-length 0x); But in spec, it suggests that write this bit to prevent the interrupt from being generated in an event buffer configuration. So need we set this bit? If I was wrong, please correct me. Thanks, Rui -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
