Hi Sergei, > -----Original Message----- > From: Sergei Shtylyov [mailto:sshtylyov@xxxxxxxxxx] > Sent: 2011年11月29日 19:36 > To: Neil Zhang > Cc: balbi@xxxxxx; stern@xxxxxxxxxxxxxxxxxxx; gregkh@xxxxxxx; linux- > usb@xxxxxxxxxxxxxxx; Chao Xie > Subject: Re: [PATCH 4/7 v1] usb: gadget: mv_udc: disable ISR when > stopped > > Hello. > > On 28-11-2011 15:40, Neil Zhang wrote: > > >>>>> When device is stopped, there is no need to handle ISR. > >>>>> Especially when otg switch to HOST mode. > > [...] > > >>>>> Signed-off-by: Neil Zhang<zhangwm@xxxxxxxxxxx> > >>>>> --- > >>>>> drivers/usb/gadget/mv_udc.h | 3 ++- > >>>>> drivers/usb/gadget/mv_udc_core.c | 8 ++++++++ > >>>>> 2 files changed, 10 insertions(+), 1 deletions(-) > >> > >>>>> diff --git a/drivers/usb/gadget/mv_udc_core.c > >>>> b/drivers/usb/gadget/mv_udc_core.c > >>>>> index c7c0bfa..e852f9d 100644 > >>>>> --- a/drivers/usb/gadget/mv_udc_core.c > >>>>> +++ b/drivers/usb/gadget/mv_udc_core.c > >>>>> @@ -1056,6 +1056,8 @@ static void udc_stop(struct mv_udc *udc) > >>>>> USBINTR_PORT_CHANGE_DETECT_EN | USBINTR_RESET_EN); > >>>>> writel(tmp,&udc->op_regs->usbintr); > >>>>> > >>>>> + udc->stopped = 1; > >>>>> + > >>>>> /* Reset the Run the bit in the command register to stop > VUSB */ > >>>>> tmp = readl(&udc->op_regs->usbcmd); > >>>>> tmp&= ~USBCMD_RUN_STOP; > >>>>> @@ -1072,6 +1074,8 @@ static void udc_start(struct mv_udc *udc) > >>>>> /* Enable interrupts */ > >>>>> writel(usbintr,&udc->op_regs->usbintr); > >>>>> > >>>>> + udc->stopped = 0; > >>>>> + > >>>>> /* Set the Run bit in the command register */ > >>>>> writel(USBCMD_RUN_STOP,&udc->op_regs->usbcmd); > >>>>> } > >>>>> @@ -2040,6 +2044,10 @@ static irqreturn_t mv_udc_irq(int irq, > void *dev) > >>>>> struct mv_udc *udc = (struct mv_udc *)dev; > >>>>> u32 status, intr; > >>>>> > >>>>> + /* Disable ISR when stopped bit is set */ > >>>>> + if (udc->stopped) > >>>>> + return IRQ_NONE; > > >>>> Can't this lead to the interrupt storm and masking the > >>>> interrupt as a result? > >>>> You'd better mask off interrupts via your hardware's interrupt > mask > >>>> register. > > >>> Yes, we will mask the interrupt when stop the gadget. But this irq > line > >>> is shared between gadget and host driver, if the host is not active, > there > >>> will be no interrupt here, so I think it Should be OK here to > return IRQ_NONE. > > >> And if the host is active? Aren't you checking the gadget > interrupt > >> status > >> register in order to know if it's gadget's interrupt? You should if > not, > >> and > >> so your new code seems to have no sense at all. > > > If we don't add this code, when otg works in host mode, the udc_irq > is still called > > because of PORT CHANGE event when we plug in an usb key. > > Is that event mapped to the gadget's interrupt register too? If not, > the > mask register will be zero, and the code will just return IRQ_NONE. Yes, PORT CHANGE event mapped to the gadget's interrupt register. > > > But it should not be called when otg is in host mode. > > Why not, if gadget interrupts are masked? You return IRQ_NONE in > that case > anyway. > > > The code if (udc->stopped) here means that the controller exits the > gadget mode, > > And udc_irq should not be called. > > Not sure why your gadget driver is located separately from the host > driver > if this is OTG controller... shouldn't it be in the same place, like > drivers/usb/musb/? Yes, our gadget driver is separated from the host driver for this OTG controller. So I have to ignore the event in gadget driver when works as host. > > Best Regards, > > Neil Zhang > > WBR, Sergei Best Regards, Neil Zhang ?韬{.n?????%??檩??w?{.n???{炳???骅w* jg????????G??⒏ ⒎?:+v???????? ????＂??????
