From: Peter Chen <hzpeterchen@gmail.com> To: pawell@cadence.com Cc: balbi@kernel.org, devicetree@vger.kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org, rogerq@ti.com, lkml <linux-kernel@vger.kernel.org>, adouglas@cadence.com, jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com, pjez@cadence.com, kurahul@cadence.com Subject: Re: [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver Date: Wed, 12 Dec 2018 10:04:25 +0800 [thread overview] Message-ID: <CAL411-ryK2LLZVg5_sLkCYOuhwdHLVm5XavSyC9nbEXZah_43w@mail.gmail.com> (raw) In-Reply-To: <BYAPR07MB47095110AF28CC11B4C9E144DDA60@BYAPR07MB4709.namprd07.prod.outlook.com> > >> + tmode = le16_to_cpu(ctrl->wIndex); > >> + > >> + if (!set || (tmode & 0xff) != 0) > >> + return -EINVAL; > >> + > >> + switch (tmode >> 8) { > >> + case TEST_J: > >> + case TEST_K: > >> + case TEST_SE0_NAK: > >> + case TEST_PACKET: > >> + cdns3_set_register_bit(&priv_dev->regs->usb_cmd, > >> + USB_CMD_STMODE | > >> + USB_STS_TMODE_SEL(tmode - 1)); > > > >I'm 90% sure this won't work. There's a reason why we only enter the > >requested test mode from status stage. How have you tested this? > What's the reason? It can work although the code is a little different with above, I tested it using USBxHSETT tool at Windows. > From USB spec: > "The transition to test mode must be complete no later than 3 ms after the completion of the status stage of the > request." > But I don't remember any issues with this test on other ours drivers. Maybe status stage > is armed in this case by controller. I have to confirm how it works with hardware team. > Driver doesn't know when status stage was completed. We don't have > any event on status stage completion. I haven't checked it yet with tester on this driver. > > >> + irqreturn_t ret = IRQ_NONE; > >> + unsigned long flags; > >> + u32 reg; > >> + > >> + priv_dev = cdns->gadget_dev; > >> + spin_lock_irqsave(&priv_dev->lock, flags); > > > >you're already running in hardirq context. Why do you need this lock at > >all? I would be better to use the hardirq handler to mask your > >interrupts, so they don't fire again, then used the top-half (softirq) > >handler to actually handle the interrupts. > This controller may be ran at SMP environment, register and flag access needs to be protected among CPUs running. > Yes, spin_lock_irqsave is not necessary here. > > Do you mean replacing devm_request_irq with a request_threaded_irq ? > I have single interrupt line shared between Host, Driver, DRD/OTG. > I'm not sure if it will work more efficiently. > > > > >> + /* check USB device interrupt */ > >> + reg = readl(&priv_dev->regs->usb_ists); > >> + writel(reg, &priv_dev->regs->usb_ists); > >> + > >> + if (reg) { > >> + dev_dbg(priv_dev->dev, "IRQ: usb_ists: %08X\n", reg); > > > >I strongly advise against using dev_dbg() for debugging. Even more so > >inside your IRQ handler. Felipe, I use Dynamic Debug for debugging, and show debug messages with "dmesg" after testing/debugging. I see dwc3 using trace, any benefits for switching to trace? > Ok, It's not necessary in this place, especially, that there is invoked trace point > inside cdns3_check_usb_interrupt_proceed which makes the same. > Peter
WARNING: multiple messages have this Message-ID (diff)
From: Peter Chen <hzpeterchen@gmail.com> To: pawell@cadence.com Cc: balbi@kernel.org, devicetree@vger.kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org, rogerq@ti.com, lkml <linux-kernel@vger.kernel.org>, adouglas@cadence.com, jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com, pjez@cadence.com, kurahul@cadence.com Subject: [v1,2/2] usb:cdns3 Add Cadence USB3 DRD Driver Date: Wed, 12 Dec 2018 10:04:25 +0800 [thread overview] Message-ID: <CAL411-ryK2LLZVg5_sLkCYOuhwdHLVm5XavSyC9nbEXZah_43w@mail.gmail.com> (raw) > >> + tmode = le16_to_cpu(ctrl->wIndex); > >> + > >> + if (!set || (tmode & 0xff) != 0) > >> + return -EINVAL; > >> + > >> + switch (tmode >> 8) { > >> + case TEST_J: > >> + case TEST_K: > >> + case TEST_SE0_NAK: > >> + case TEST_PACKET: > >> + cdns3_set_register_bit(&priv_dev->regs->usb_cmd, > >> + USB_CMD_STMODE | > >> + USB_STS_TMODE_SEL(tmode - 1)); > > > >I'm 90% sure this won't work. There's a reason why we only enter the > >requested test mode from status stage. How have you tested this? > What's the reason? It can work although the code is a little different with above, I tested it using USBxHSETT tool at Windows. > From USB spec: > "The transition to test mode must be complete no later than 3 ms after the completion of the status stage of the > request." > But I don't remember any issues with this test on other ours drivers. Maybe status stage > is armed in this case by controller. I have to confirm how it works with hardware team. > Driver doesn't know when status stage was completed. We don't have > any event on status stage completion. I haven't checked it yet with tester on this driver. > > >> + irqreturn_t ret = IRQ_NONE; > >> + unsigned long flags; > >> + u32 reg; > >> + > >> + priv_dev = cdns->gadget_dev; > >> + spin_lock_irqsave(&priv_dev->lock, flags); > > > >you're already running in hardirq context. Why do you need this lock at > >all? I would be better to use the hardirq handler to mask your > >interrupts, so they don't fire again, then used the top-half (softirq) > >handler to actually handle the interrupts. > This controller may be ran at SMP environment, register and flag access needs to be protected among CPUs running. > Yes, spin_lock_irqsave is not necessary here. > > Do you mean replacing devm_request_irq with a request_threaded_irq ? > I have single interrupt line shared between Host, Driver, DRD/OTG. > I'm not sure if it will work more efficiently. > > > > >> + /* check USB device interrupt */ > >> + reg = readl(&priv_dev->regs->usb_ists); > >> + writel(reg, &priv_dev->regs->usb_ists); > >> + > >> + if (reg) { > >> + dev_dbg(priv_dev->dev, "IRQ: usb_ists: %08X\n", reg); > > > >I strongly advise against using dev_dbg() for debugging. Even more so > >inside your IRQ handler. Felipe, I use Dynamic Debug for debugging, and show debug messages with "dmesg" after testing/debugging. I see dwc3 using trace, any benefits for switching to trace? > Ok, It's not necessary in this place, especially, that there is invoked trace point > inside cdns3_check_usb_interrupt_proceed which makes the same. > Peter
next prev parent reply other threads:[~2018-12-12 2:04 UTC|newest] Thread overview: 79+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-10 12:39 [PATCH v1 0/2] Introduced new Cadence USBSS DRD Driver Pawel Laszczak 2018-12-10 12:39 ` Pawel Laszczak 2018-12-10 12:39 ` [PATCH v1 1/2] dt-bindings: add binding for USBSS-DRD controller Pawel Laszczak 2018-12-10 12:39 ` [v1,1/2] " Pawel Laszczak 2018-12-10 12:39 ` [PATCH v1 1/2] " Pawel Laszczak 2018-12-11 10:16 ` Roger Quadros 2018-12-11 10:16 ` [v1,1/2] " Roger Quadros 2018-12-11 10:16 ` [PATCH v1 1/2] " Roger Quadros 2018-12-13 9:20 ` Peter Chen 2018-12-13 9:20 ` [v1,1/2] " Peter Chen 2018-12-13 9:25 ` [PATCH v1 1/2] " Pawel Laszczak 2018-12-13 9:25 ` [v1,1/2] " Pawel Laszczak 2018-12-20 20:01 ` [PATCH v1 1/2] " Rob Herring 2018-12-20 20:01 ` [v1,1/2] " Rob Herring 2018-12-22 22:24 ` [PATCH v1 1/2] " Pawel Laszczak 2018-12-22 22:24 ` [v1,1/2] " Pawel Laszczak 2018-12-27 21:01 ` [PATCH v1 1/2] " Rob Herring 2018-12-27 21:01 ` [v1,1/2] " Rob Herring 2018-12-31 5:35 ` [PATCH v1 1/2] " Pawel Laszczak 2018-12-31 5:35 ` [v1,1/2] " Pawel Laszczak 2018-12-10 12:39 ` [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver Pawel Laszczak 2018-12-10 12:39 ` [v1,2/2] " Pawel Laszczak 2018-12-10 12:39 ` [PATCH v1 2/2] " Pawel Laszczak 2018-12-11 9:39 ` Roger Quadros 2018-12-11 9:39 ` [v1,2/2] " Roger Quadros 2018-12-11 9:39 ` [PATCH v1 2/2] " Roger Quadros 2018-12-11 10:01 ` Pawel Laszczak 2018-12-11 10:01 ` [v1,2/2] " Pawel Laszczak 2018-12-11 12:15 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-11 12:15 ` [v1,2/2] " Felipe Balbi 2018-12-11 12:15 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-11 11:46 ` Felipe Balbi 2018-12-11 11:46 ` [v1,2/2] " Felipe Balbi 2018-12-11 12:14 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-11 12:14 ` [v1,2/2] " Felipe Balbi 2018-12-11 12:14 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-11 19:04 ` Pawel Laszczak 2018-12-11 19:04 ` [v1,2/2] " Pawel Laszczak 2018-12-11 19:04 ` [PATCH v1 2/2] " Pawel Laszczak 2018-12-12 2:04 ` Peter Chen [this message] 2018-12-12 2:04 ` [v1,2/2] " Peter Chen 2018-12-12 6:55 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-12 6:55 ` [v1,2/2] " Felipe Balbi 2018-12-12 7:38 ` [PATCH v1 2/2] " Peter Chen 2018-12-12 7:38 ` [v1,2/2] " Peter Chen 2018-12-12 8:34 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-12 8:34 ` [v1,2/2] " Felipe Balbi 2018-12-12 8:34 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-12 9:24 ` Peter Chen 2018-12-12 9:24 ` [v1,2/2] " Peter Chen 2018-12-12 15:53 ` [PATCH v1 2/2] " Bin Liu 2018-12-12 15:53 ` [v1,2/2] " Bin Liu 2018-12-12 15:53 ` [PATCH v1 2/2] " Bin Liu 2018-12-13 1:21 ` Peter Chen 2018-12-13 1:21 ` [v1,2/2] " Peter Chen 2018-12-12 6:52 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-12 6:52 ` [v1,2/2] " Felipe Balbi 2018-12-12 6:52 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-14 3:46 ` Kishon Vijay Abraham I 2018-12-14 3:46 ` [v1,2/2] " Kishon Vijay Abraham I 2018-12-17 5:46 ` [PATCH v1 2/2] " Pawel Laszczak 2018-12-17 5:46 ` [v1,2/2] " Pawel Laszczak 2018-12-17 11:25 ` [PATCH v1 2/2] " Pawel Laszczak 2018-12-17 11:25 ` [v1,2/2] " Pawel Laszczak 2018-12-17 11:34 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-17 11:34 ` [v1,2/2] " Felipe Balbi 2018-12-17 11:34 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-17 11:51 ` Pawel Laszczak 2018-12-17 11:51 ` [v1,2/2] " Pawel Laszczak 2018-12-17 11:56 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-17 11:56 ` [v1,2/2] " Felipe Balbi 2018-12-17 11:56 ` [PATCH v1 2/2] " Felipe Balbi 2018-12-13 9:35 ` Peter Chen 2018-12-13 9:35 ` [v1,2/2] " Peter Chen 2018-12-16 13:01 ` [PATCH v1 2/2] " Pawel Laszczak 2018-12-16 13:01 ` [v1,2/2] " Pawel Laszczak 2018-12-14 22:56 ` [PATCH v1 2/2] " kbuild test robot 2018-12-14 22:56 ` [v1,2/2] " kbuild test robot 2018-12-14 22:56 ` [PATCH v1 2/2] " kbuild test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAL411-ryK2LLZVg5_sLkCYOuhwdHLVm5XavSyC9nbEXZah_43w@mail.gmail.com \ --to=hzpeterchen@gmail.com \ --cc=adouglas@cadence.com \ --cc=balbi@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=jbergsagel@ti.com \ --cc=kurahul@cadence.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=nm@ti.com \ --cc=nsekhar@ti.com \ --cc=pawell@cadence.com \ --cc=peter.chen@nxp.com \ --cc=pjez@cadence.com \ --cc=rogerq@ti.com \ --cc=sureshp@cadence.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.