Re: About wakeup IRQ support in USB core
On Mon, Nov 17, 2014 at 10:46:25AM +0100, Oliver Neukum wrote: On Mon, 2014-11-17 at 13:51 +0800, Wang, Yu wrote: Hi Roger, I saw one old thread about your patch for wakeup IRQ support in USB core. We need it for Intel platform. And it is work well on intel platform. May I know why this patch haven't get merged so far? I saw everything was smooth during the discussion, and you had already fixed all comments from Alan. But somehow the discussion is stop. Old thread link: http://lkml.iu.edu//hypermail/linux/kernel/1307.1/01392.html Maybe this is just me, but this looks hackish. Why not clear the IRQ and queue a work resuming the HC and calling the handler? The controller is in suspended state which is power gated. How to access its registers? We have to resume it first before clear IRQ. Thanks Yu Regards Oliver -- 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 -- 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: About wakeup IRQ support in USB core
On Mon, Nov 17, 2014 at 11:20:01AM +0100, Oliver Neukum wrote: On Mon, 2014-11-17 at 17:47 +0800, Wang, Yu wrote: On Mon, Nov 17, 2014 at 10:46:25AM +0100, Oliver Neukum wrote: On Mon, 2014-11-17 at 13:51 +0800, Wang, Yu wrote: Hi Roger, I saw one old thread about your patch for wakeup IRQ support in USB core. We need it for Intel platform. And it is work well on intel platform. May I know why this patch haven't get merged so far? I saw everything was smooth during the discussion, and you had already fixed all comments from Alan. But somehow the discussion is stop. Old thread link: http://lkml.iu.edu//hypermail/linux/kernel/1307.1/01392.html Maybe this is just me, but this looks hackish. Why not clear the IRQ and queue a work resuming the HC and calling the handler? The controller is in suspended state which is power gated. How to access its registers? We have to resume it first before clear IRQ. Sorry block was meant. You cannot clear the interrupt in a sleeping HC. But why depend on the resume() handler to redo the interrupt? We know there was an interrupt. We can call the handler. Due to we can't clear the IRQ before controller resume. So the IRQ will always trigger until we handled it. It will cause CPU aways be interrupted by this IRQ during. After get wakeup IRQ, until HC get resumed. There have a little bit long latency. 1, get wakeup IRQ. 2, call usb_hcd_resume_root_hub which will schedule the workqueue. 3, In the usb_remote_wakeup, it will call pm_runtime_get_sync. For step 2 and 3, they are all cost progresses scheduling latency. That is why roger disable it in the ISR, and re-enable it after controller get resumed. Thanks Yu Regards Oliver -- 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: About wakeup IRQ support in USB core
On Mon, Nov 17, 2014 at 01:36:19PM +0200, Roger Quadros wrote: Hi Roger, Hi Yu, On 11/17/2014 07:51 AM, Wang, Yu wrote: Hi Roger, I saw one old thread about your patch for wakeup IRQ support in USB core. We need it for Intel platform. And it is work well on intel platform. May I know why this patch haven't get merged so far? I saw everything was smooth during the discussion, and you had already fixed all comments from Alan. But somehow the discussion is stop. Old thread link: http://lkml.iu.edu//hypermail/linux/kernel/1307.1/01392.html Sorry I gave up on that entire series for a while. Can you please pick that one patch which addresses your issue and send it again? I think it can go in independent of the others. Thanks. Sure, I will try to port it to the latest code base and re-sbumit it. Thanks Yu cheers, -roger -- 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 -- 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
About wakeup IRQ support in USB core
Hi Roger, I saw one old thread about your patch for wakeup IRQ support in USB core. We need it for Intel platform. And it is work well on intel platform. May I know why this patch haven't get merged so far? I saw everything was smooth during the discussion, and you had already fixed all comments from Alan. But somehow the discussion is stop. Old thread link: http://lkml.iu.edu//hypermail/linux/kernel/1307.1/01392.html Thanks Yu -- 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
[RFC] xhci: free legacy irq in xhci driver.
From: Wang, Yu yu.y.w...@intel.com
By current xhci ISR request/release flow, the IRQ request be done by
xhci driver(xhci_run) whatever it is legacy irq or MSI. But the IRQ
release flow is a little mess, MSI release be handled by xhci
driver(xhci_stop), and legacy IRQ release be handled by USB core when
hcd is removing(usb_remove_hcd).
There have one bug if XHCI_BROKEN_MSI quirk be set and meet STS_SRE
error during xhci resuming(xhci_resume), it will cause the xhci ISR be
requested more times.
The reason is xhci driver will try to re-initialize all registers and
related structures to rescue the restore operation failure. And it will
call xhci_cleanup_msix to release the ISR and call xhci_run to
re-request it. In this flow, hcd will not be remove so USB core will not
to free the legacy xhci ISR. For MSI case, it should be fine due to they
will be free in xhci_cleanup_msix. But if XHCI_BROKEN_MSI quirk be set,
the xhci will use legacy irq then reproduce this issue.
This patch will unify the xhci irq request/release flow in xhci driver
to avoid involve USB core.
Change-Id: I85d589b9c7849c471595da89e5b63347bc3a2939
Signed-off-by: Wang, Yu yu.y.w...@intel.com
---
drivers/usb/host/xhci.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 402265d..2a31e92e3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -265,18 +265,23 @@ static int xhci_setup_msi(struct xhci_hcd *xhci)
*/
static void xhci_free_irq(struct xhci_hcd *xhci)
{
- struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)-self.controller);
+ struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct pci_dev *pdev = to_pci_dev(hcd-self.controller);
int ret;
- /* return if using legacy interrupt */
- if (xhci_to_hcd(xhci)-irq 0)
- return;
+ /* free_irq directly if using legacy interrupt */
+ if (hcd-irq 0)
+ goto legacy_irq;
ret = xhci_free_msi(xhci);
if (!ret)
return;
- if (pdev-irq 0)
- free_irq(pdev-irq, xhci_to_hcd(xhci));
+
+legacy_irq:
+ if (pdev-irq 0) {
+ free_irq(pdev-irq, hcd);
+ hcd-irq = 0;
+ }
return;
}
@@ -351,6 +356,9 @@ static void xhci_cleanup_msix(struct xhci_hcd *xhci)
xhci_free_irq(xhci);
+ if (xhci-quirks XHCI_BROKEN_MSI)
+ return;
+
if (xhci-msix_entries) {
pci_disable_msix(pdev);
kfree(xhci-msix_entries);
--
1.7.9.5
--
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: [RFC] xhci: free legacy irq in xhci driver.
On Fri, Aug 01, 2014 at 11:00:08AM +0800, Wang, Yu wrote: From: Wang, Yu yu.y.w...@intel.com By current xhci ISR request/release flow, the IRQ request be done by xhci driver(xhci_run) whatever it is legacy irq or MSI. But the IRQ release flow is a little mess, MSI release be handled by xhci driver(xhci_stop), and legacy IRQ release be handled by USB core when hcd is removing(usb_remove_hcd). There have one bug if XHCI_BROKEN_MSI quirk be set and meet STS_SRE error during xhci resuming(xhci_resume), it will cause the xhci ISR be requested more times. The reason is xhci driver will try to re-initialize all registers and related structures to rescue the restore operation failure. And it will call xhci_cleanup_msix to release the ISR and call xhci_run to re-request it. In this flow, hcd will not be remove so USB core will not to free the legacy xhci ISR. For MSI case, it should be fine due to they will be free in xhci_cleanup_msix. But if XHCI_BROKEN_MSI quirk be set, the xhci will use legacy irq then reproduce this issue. This patch will unify the xhci irq request/release flow in xhci driver to avoid involve USB core. Change-Id: I85d589b9c7849c471595da89e5b63347bc3a2939 Why is this line here? Please don't do that... [Yu:] Oh. Sorry. It is some intel internal tool add it. I will remove it. Thanks Yu -- 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
[RFC] xhci: free legacy irq in xhci driver.
From: Wang, Yu yu.y.w...@intel.com
By current xhci ISR request/release flow, the IRQ request be done by
xhci driver(xhci_run) whatever it is legacy irq or MSI. But the IRQ
release flow is a little mess, MSI release be handled by xhci
driver(xhci_stop), and legacy IRQ release be handled by USB core when
hcd is removing(usb_remove_hcd).
There have one bug if XHCI_BROKEN_MSI quirk be set and meet STS_SRE
error during xhci resuming(xhci_resume), it will cause the xhci ISR be
requested more times.
The reason is xhci driver will try to re-initialize all registers and
related structures to rescue the restore operation failure. And it will
call xhci_cleanup_msix to release the ISR and call xhci_run to
re-request it. In this flow, hcd will not be remove so USB core will not
to free the legacy xhci ISR. For MSI case, it should be fine due to they
will be free in xhci_cleanup_msix. But if XHCI_BROKEN_MSI quirk be set,
the xhci will use legacy irq then reproduce this issue.
This patch will unify the xhci irq request/release flow in xhci driver
to avoid involve USB core.
Signed-off-by: Wang, Yu yu.y.w...@intel.com
---
drivers/usb/host/xhci.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 7436d5f..e77cf6b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -243,18 +243,23 @@ static int xhci_setup_msi(struct xhci_hcd *xhci)
*/
static void xhci_free_irq(struct xhci_hcd *xhci)
{
- struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)-self.controller);
+ struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct pci_dev *pdev = to_pci_dev(hcd-self.controller);
int ret;
- /* return if using legacy interrupt */
- if (xhci_to_hcd(xhci)-irq 0)
- return;
+ /* free_irq directly if using legacy interrupt */
+ if (hcd-irq 0)
+ goto legacy_irq;
ret = xhci_free_msi(xhci);
if (!ret)
return;
- if (pdev-irq 0)
- free_irq(pdev-irq, xhci_to_hcd(xhci));
+
+legacy_irq:
+ if (pdev-irq 0) {
+ free_irq(pdev-irq, hcd);
+ hcd-irq = 0;
+ }
return;
}
@@ -330,6 +335,9 @@ static void xhci_cleanup_msix(struct xhci_hcd *xhci)
xhci_free_irq(xhci);
+ if (xhci-quirks XHCI_BROKEN_MSI)
+ return;
+
if (xhci-msix_entries) {
pci_disable_msix(pdev);
kfree(xhci-msix_entries);
--
1.7.9.5
--
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
[RFC] xhci: Fix xhci block system enter system suspend state.
From: Wang, Yu yu.y.w...@intel.com
The system suspend flow as following:
1, Freeze all user processes and kenrel threads.
2, Trying to suspend all devices.
2.1, If pci device under RPM suspended state, then pci driver will try
to resume it to RPM active state in prepare stage.
2.2, xhci_resume function will call usb_hcd_resume_root_hub to queue two
workqueue items to resume usb2usb3 roothub devices.
2.3, Call suspend callbacks of devices.
2.3.1, All suspend callbacks be called of all hcd's children included
roothub devices.
2.3.2, Fianlly, hcd_pci_suspend callback be called.
Due to workqueue threads were already frozen in step 1. So the workqueue
items can't be scheduled which will cause roothub devices can't be
resumed in this flow. Then HCD_FLAG_WAKEUP_PENDING flags will not be
clear which set in usb_hcd_resume_root_hub function. Finally,
hcd_pci_suspend will return -EBUSY, and system suspend is failed.
The reason why this issue doesn't show up very often, it is due to
choose_wakeup will be called in step step 2.3.1. In step 2.3.1
udev-do_remote_wakeup is not equal device_may_wakeup(udev-dev), then
udev will be resume to RPM active for change the wakeup settings. This
is one be resumed to RPM active to change lucky hit which hides this
issue.
For some special xHCI controllers which have no USB2 port, then roothub
will not match hub driver due to probe failed. Then its
do_remote_wakeup will be set to zero, then will not hit above lucky.
Actually, xhci driver needn't to resume roothub devices everytime like
above case. It's only needed when there are pending event TRBs.
Signed-off-by: Wang, Yu yu.y.w...@intel.com
---
drivers/usb/host/xhci.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 3008369..4285e94 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -932,7 +932,7 @@ int xhci_suspend(struct xhci_hcd *xhci)
*/
int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
{
- u32 command, temp = 0;
+ u32 command, temp = 0, status;
struct usb_hcd *hcd = xhci_to_hcd(xhci);
struct usb_hcd *secondary_hcd;
int retval = 0;
@@ -1050,8 +1050,12 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
done:
if (retval == 0) {
- usb_hcd_resume_root_hub(hcd);
- usb_hcd_resume_root_hub(xhci-shared_hcd);
+ /* Resume root hubs only when have pending events. */
+ status = xhci_readl(xhci, xhci-op_regs-status);
+ if (status STS_EINT) {
+ usb_hcd_resume_root_hub(hcd);
+ usb_hcd_resume_root_hub(xhci-shared_hcd);
+ }
}
/*
--
1.7.9.5
--
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: [Intel-linux-usb] RE: One question about Linux xHCI driver
On Thu, 19 Jun 2014, Wang, Yu Y wrote: I'm not sure of the right way to solve this problem. Probably xhci_resume() should check the root-hub statuses to see if either root hub really needs to be resumed before calling usb_hcd_resume_root_hub(); I think that will work. [Yu:] I understand your point. From the big picture, to my understanding, there have three scenarios. The first one is USB device trigger remote wakeup. The second one is user space trying to resume xHC which will queue URBs. The third one is PCI driver try to resume pci device during S3 entering if the device under suspended state. Your patch is focus on the first case. Avoid xHCI re-enter RPM suspended in the gap between HCD resumed and activate the IRQ, right? Right. But your patch will cause this BUG for the third case. And the second case should be fine. So my understanding is to find one way to distinguish the first one and second one. We need to confirm if RH need to resume before trigger resume in xhci_resume. During xhci_resume function, after set USBCMD.R/S bit, then the controller can get IRQ. So we can check USBSTS.EINT or USBSTS.PCD bit to determine if need to resume the root hubs to handle these events. Yes, that is what I recommended above. [Yu:] Get it. I will prepare one patch. Thanks Yu Alan Stern -- 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: [Intel-linux-usb] RE: One question about Linux xHCI driver
On Wed, 18 Jun 2014, Mathias Nyman wrote: On 06/17/2014 05:17 AM, Wang, Yu Y wrote: Hi All, I have one question about Linux xHCI driver. Need your help to introduce more backgrounds. About the S3 flow: 1, Freeze all user processes. 2, Freeze all kernel threads (including workqueue thread). 3, Trying to suspend all devices. 3.1, if pci device in RPM suspended, then try to resume it to D0. Call pm_runtime_resume API in prepare stage. 3.2, xhci_resume callback will call usb_hcd_resume_root_hub to queue two workqueue items to resume USB2USB3 roothub devices. 3.3, call suspend callback of devices. 3.3.1, All suspend callback be called of hcd's children included roothub. 3.3.2, Finally, hcd_pci_suspend callback will be called. For above S3 enter flow. Due to workqueue thread were already frozen in step 2. So the two workqueue items can't be scheduled in step 3.2. But step 3.2 will set HCD_FLAG_WAKEUP_PENDING flag to hcd and shared_hcd which expecting clear in bus_resume which will not be executed in this scenario. It will cause step 3.3.2 return -EBUSY due to HCD_FLAG_WAKEUP_PENDING flag is set. Finally, S3 enter failed. As my debugging, I see sometimes, the HCD_FLAG_WAKEUP_PENDING flag will be clear in step 3.3.1 due to udev-do_remote_wakeup is not equal device_may_wakeup(udev-dev) in choose_wakeup function. Is it by design behavior? Or it is just one lucky hit? It is a bug. And you are right; the only reason it doesn't show up very often is because of luck. [Yu:] Thanks for help confirm. Another question, step 3.2, why xhci_resume try to positive to resume its roothub devices? Hi This is a good question, I'm not that familiar with the internal details of pm framework or the usb core power management. The powermanagemnt workqueue (pm_wq) which is used by the PM framework is used here as well to resume the root hub by calling hcd_bus_resume() which clears WAKEUP_PENDING flags. I have a hard time believing this workqueue can't be scheduled in the middle of PM transitions activity. It can't, and for a very good reason: We don't want workqueue actions interfering with a system suspend by waking devices up after the PM core has powered them down. I think that it should be possible to suspend a device already in runtime suspend without resuming it first. This would at least make sure the runtime resume won't cause the system suspend to fail. It may or may not be possible, depending on the circumstances. Generally, if a device is in runtime suspend then it is enabled for wakeup (if it supports wakeup). But during system sleep, a device is supposed to be enabled for wakeup only if the user wants it to be. Root hubs in particular usually aren't wakeup-enabled during system sleep. You can imagine the trouble that would result if unplugging a USB mouse from a sleeping computer caused the computer to wake up. This means that for root hubs and various other devices, the wakeup settings have to be changed if the device is already in runtime suspend when a system sleep starts. And in general, you can't change a device's wakeup settings while it is powered down; you have to resume it first. That's why the PCI core does a runtime resume on every PCI device at the start of system sleep. It's overkill, but it allows wakeup settings to be adjusted properly. The problem here is that xhci_resume() assumes that it gets called _only_ when the root hubs are about to be resumed -- either because the whole system is waking up from sleep or because a USB device has sent a runtime wakeup request. But this isn't true during the initial stages of system suspend. When the PCI core does its runtime resume of the controller at this time, this does not have to cause the root hubs to resume. I'm not sure of the right way to solve this problem. Probably xhci_resume() should check the root-hub statuses to see if either root hub really needs to be resumed before calling usb_hcd_resume_root_hub(); I think that will work. [Yu:] I understand your point. From the big picture, to my understanding, there have three scenarios. The first one is USB device trigger remote wakeup. The second one is user space trying to resume xHC which will queue URBs. The third one is PCI driver try to resume pci device during S3 entering if the device under suspended state. Your patch is focus on the first case. Avoid xHCI re-enter RPM suspended in the gap between HCD resumed and activate the IRQ, right? But your patch will cause this BUG for the third case. And the second case should be fine. So my understanding is to find one way to distinguish the first one and second one. We need to confirm if RH need to resume before trigger resume in xhci_resume. During xhci_resume function, after set USBCMD.R/S bit, then the controller can get IRQ. So we can check USBSTS.EINT
Re: About the DRD mode implementation of DWC3 driver]
On Fri, Apr 11, 2014 at 06:19:57PM -0500, Felipe Balbi wrote: Hi, On Fri, Apr 11, 2014 at 10:23:38AM +0800, Wang, Yu wrote: Hi Balbi, At first, thank you to help give the response in patience. Hi, On Wed, Apr 09, 2014 at 02:08:32PM +0800, Wang, Yu wrote: Glad to see the OTG mode is prepare to support in your dwc3-role-switch branch. But it is not fit for intel Merrfield/Moorfield platforms. :( that's not what I heard when I asked some friends to test that branch on different platforms. It's certainly not perfect yet and that's why the code isn't merged in mainline, but claiming that it's not fit for Merrifield/Moorfield is just a lie. Can I get your friends' email address if they are in Intel? I would like to You already have that. Just look at Cc list. contact them to make the branch work on my Merrifield/Moorefield board too. Currently I can't see any link change event with the branch. how did you test it ? According to [1] Merrifield won't work for more than 20s or so with v3.14 and, since there is no resolution on the thread, I assume today's mainline won't work either. Anyway, if you really did test, test again with enabling verbose debug on dwc3 and show me the logs, I'm interested in what the IP is doing. Yes. As you said, the v3.14 haven't get stable so far on Merrifield platform. So I tried to back port your dwc3-role-switch branch solution to our v3.10 base and verified. I will waiting v3.14 get ready and do the test again to double confirm. I will let you know the result. Sorry cause the misunderstanding for you. The reason is we implemented DRD mode instead of OTG mode. So the GCTL.PortCapDir will be set as 01 for host mode, and 10 for device mode. from a SW perspective there is *no* difference. The only reason for using PortCapDir is to cope with platforms which want to switch roles but have screwed up ID pin reporting. And since most of the platforms actually have tha problem, it's just easier to implement it that way. Ok. Just confirm with you that you think it's not a matter to use GCTL.PortCapDir or OCTL.PeriMode to switch role, right? As of today, I don't see a difference. Things *can* change though. We can find out more details about the HW which forces us to use one of the other. I will help to try both DRD/OTG solution with your design on Merrifield when v3.14 get stable on it. If no cable connected, we will trigger D0i3cold which will power gate every clock/power used by dwc3 controller. that's not in mainline though, why should I care ? Show me code adding support for D0i3cold for dwc3 then we can talk. Until then, I'll continue assuming there's no such PM state and continue with happy hacking sessions. And entering D0i3hot with hibernation mode when acting as host mode (micro A cable connected), also during device mode(micro B cable connected to PC host). Current driver also doesn't support any runtime PM, so why should I care about your out-of-tree changes ? If you have any code which is not in mainline and I change something in mainline you have to deal to with it, not me. Get it. So we can treat this solution is working w/o PM implementation so far. And maybe we need to re-design DRD/OTG when we consider to support PM, right? We probably don't need to redesign anything when adding PM. What we need is that when we start to add PM support to this driver, in case there is any regression, we find out why there is a regression. Then we solve it properly, after looking at logs and fully understanding what's breaking the driver. In any case, that's something that won't happen in mainline for at least a couple more major releases since there are still quite a few changes pending. Get your point. Thanks I just cannot spend time imagining all twisted scenarios Vendors (including my employer, for that matter) want to support with whatever hacked up version of mainline drivers they might have. My plan is to *not* add a lot of PM support until I know all other features are functional. When I'm happy with those features and know they have been thoroughly tested on *all* users of dwc3 then we can all get together in a conference (maybe have a DWC3 mini-summit or whatever) and discuss how we're gonna implement PM *together*. Since the driver is used by many different vendors, it would be unfair for me to treat Intel (or any vendor, really) differently just because someone dropped me an email commenting about all the out-of-tree changes they have. Understand. What are the other features you mean that need thorough testing before adding PM support? DRD for one. Then there is a redesign of parts of the gadget API that I want to do before PM too. Then there is the whole Link Power Management (which has *nothing* to do with Linux PM - system or runtime; we can put the USB bus in low power mode even though we don't gate any of the USB IP's
Re: About the DRD mode implementation of DWC3 driver]
On Tue, Apr 15, 2014 at 09:53:49AM -0500, Felipe Balbi wrote: Hi, On Tue, Apr 15, 2014 at 06:29:20PM +0800, Wang, Yu wrote: On Fri, Apr 11, 2014 at 06:19:57PM -0500, Felipe Balbi wrote: Hi, On Fri, Apr 11, 2014 at 10:23:38AM +0800, Wang, Yu wrote: Hi Balbi, At first, thank you to help give the response in patience. Hi, On Wed, Apr 09, 2014 at 02:08:32PM +0800, Wang, Yu wrote: Glad to see the OTG mode is prepare to support in your dwc3-role-switch branch. But it is not fit for intel Merrfield/Moorfield platforms. :( that's not what I heard when I asked some friends to test that branch on different platforms. It's certainly not perfect yet and that's why the code isn't merged in mainline, but claiming that it's not fit for Merrifield/Moorfield is just a lie. Can I get your friends' email address if they are in Intel? I would like to You already have that. Just look at Cc list. contact them to make the branch work on my Merrifield/Moorefield board too. Currently I can't see any link change event with the branch. how did you test it ? According to [1] Merrifield won't work for more than 20s or so with v3.14 and, since there is no resolution on the thread, I assume today's mainline won't work either. Anyway, if you really did test, test again with enabling verbose debug on dwc3 and show me the logs, I'm interested in what the IP is doing. Yes. As you said, the v3.14 haven't get stable so far on Merrifield platform. So I tried to back port your dwc3-role-switch branch solution to our v3.10 base and verified. That's no the same. What if you missed something ? What if it didn't work because you broke it while backporting ? I don't know that because you never showed me your backported version, also did you test on v3.10 vanilla or v3.10 + Intel's patches + dwc3-role-switch backport ? Yes. That is why I will try v3.14 original dwc3-role-switch code to double confirm. You can ignore this result first. I will share the v3.14 result to you after the stability issue fixed. I will waiting v3.14 get ready and do the test again to double confirm. I will let you know the result. Sorry cause the misunderstanding for you. ok, just next time make sure to be extra clear about your setup. If I didn't have reports from one of your colleagues that the patches were working, I could've spent time debugging something that doesn't exist. Understood. Sorry for the mistake made by the new comer. I will provide the result with extra clear environment for you in the future. The reason is we implemented DRD mode instead of OTG mode. So the GCTL.PortCapDir will be set as 01 for host mode, and 10 for device mode. from a SW perspective there is *no* difference. The only reason for using PortCapDir is to cope with platforms which want to switch roles but have screwed up ID pin reporting. And since most of the platforms actually have tha problem, it's just easier to implement it that way. Ok. Just confirm with you that you think it's not a matter to use GCTL.PortCapDir or OCTL.PeriMode to switch role, right? As of today, I don't see a difference. Things *can* change though. We can find out more details about the HW which forces us to use one of the other. I will help to try both DRD/OTG solution with your design on Merrifield when v3.14 get stable on it. ok, but you probably want to use v3.15-rc1 instead of v3.14. I just cannot spend time imagining all twisted scenarios Vendors (including my employer, for that matter) want to support with whatever hacked up version of mainline drivers they might have. My plan is to *not* add a lot of PM support until I know all other features are functional. When I'm happy with those features and know they have been thoroughly tested on *all* users of dwc3 then we can all get together in a conference (maybe have a DWC3 mini-summit or whatever) and discuss how we're gonna implement PM *together*. Since the driver is used by many different vendors, it would be unfair for me to treat Intel (or any vendor, really) differently just because someone dropped me an email commenting about all the out-of-tree changes they have. Understand. What are the other features you mean that need thorough testing before adding PM support? DRD for one. Then there is a redesign of parts of the gadget API that I want to do before PM too. Then there is the whole Link Power Management (which has *nothing* to do with Linux PM - system or runtime; we can put the USB bus in low power mode even though we don't gate any of the USB IP's clocks or power rails). Get it. I think your mean is the U1/U2, right? yup And also for OTG hibernation mode. If we power gate the USB PHY during OTG hibernation mode, and using PMIC to do ID/VBUS detection. I don't know if there have any issues. Until we get there, we won't know. So
About the DRD mode implementation of DWC3 driver]
Hi Balbi, At first, thank you to help give the response in patience. Hi, On Wed, Apr 09, 2014 at 02:08:32PM +0800, Wang, Yu wrote: Glad to see the OTG mode is prepare to support in your dwc3-role-switch branch. But it is not fit for intel Merrfield/Moorfield platforms. :( that's not what I heard when I asked some friends to test that branch on different platforms. It's certainly not perfect yet and that's why the code isn't merged in mainline, but claiming that it's not fit for Merrifield/Moorfield is just a lie. Can I get your friends' email address if they are in Intel? I would like to contact them to make the branch work on my Merrifield/Moorefield board too. Currently I can't see any link change event with the branch. The reason is we implemented DRD mode instead of OTG mode. So the GCTL.PortCapDir will be set as 01 for host mode, and 10 for device mode. from a SW perspective there is *no* difference. The only reason for using PortCapDir is to cope with platforms which want to switch roles but have screwed up ID pin reporting. And since most of the platforms actually have tha problem, it's just easier to implement it that way. Ok. Just confirm with you that you think it's not a matter to use GCTL.PortCapDir or OCTL.PeriMode to switch role, right? And the most important thing is we implemented two low power states for dwc3 controller. D0i3hot and D0i3cold. what does that have to do with role switching ? Power management and role switching are two completely different problems. You can have full DRD without PM the same way you can have full PM without DRD. Agree. They can be discussed separately. If no cable connected, we will trigger D0i3cold which will power gate every clock/power used by dwc3 controller. that's not in mainline though, why should I care ? Show me code adding support for D0i3cold for dwc3 then we can talk. Until then, I'll continue assuming there's no such PM state and continue with happy hacking sessions. And entering D0i3hot with hibernation mode when acting as host mode (micro A cable connected), also during device mode(micro B cable connected to PC host). Current driver also doesn't support any runtime PM, so why should I care about your out-of-tree changes ? If you have any code which is not in mainline and I change something in mainline you have to deal to with it, not me. Get it. So we can treat this solution is working w/o PM implementation so far. And maybe we need to re-design DRD/OTG when we consider to support PM, right? I just cannot spend time imagining all twisted scenarios Vendors (including my employer, for that matter) want to support with whatever hacked up version of mainline drivers they might have. My plan is to *not* add a lot of PM support until I know all other features are functional. When I'm happy with those features and know they have been thoroughly tested on *all* users of dwc3 then we can all get together in a conference (maybe have a DWC3 mini-summit or whatever) and discuss how we're gonna implement PM *together*. Since the driver is used by many different vendors, it would be unfair for me to treat Intel (or any vendor, really) differently just because someone dropped me an email commenting about all the out-of-tree changes they have. Understand. What are the other features you mean that need thorough testing before adding PM support? For ID/VBus detection, we are using PMIC to do detect. So we will also power gate the USB PHY for D0i3cold. yeah, everybody does that. Everybody I've seen so far have moved the analog comparators for VBUS and ID out of the PHY and into the PMIC. We even have a very nice framework to cope with that (see drivers/extcon/extcon-palmas.c for an example). Thanks for pointing it out. We will use it too. When we plug in micro A/B cable, the PMIC will report the ID/VBus change event, then driver will force controller resume to D0 from D0i3cold. Due to we haven't do any backup before entering D0i3cold, so we have to re-initialize all host/device portion registers with setting GCTL.PortCapDir to 01 or 10. yeah, but this is just how *you* decided to implement this for your employer and, guess what, all of that is out-of-tree and, by all practical means wrt mainline kernel, it's all non-existent. This means I'm free to implement all of this any way I feel it's best considering the diverse user-base we have on this driver. Quite frankly, resetting the IP is only necessary iff all power rails are cut, in that case, sure, we will reset the IP and start all over, but we don't want to cut all power while we're attached to host, which means we don't need to reset the IP, a simple context restore is enough in most cases. Also, when we implement hibernation (dwc3's hibernation, that is) we won't need to reset the IP even if we go all the way down to D0i3cold because the IP keeps all context in a scratch buffer we
About the DRD mode implementation of DWC3 driver
Hi Balbi, Glad to see the OTG mode is prepare to support in your dwc3-role-switch branch. But it is not fit for intel Merrfield/Moorfield platforms. :( The reason is we implemented DRD mode instead of OTG mode. So the GCTL.PortCapDir will be set as 01 for host mode, and 10 for device mode. And the most important thing is we implemented two low power states for dwc3 controller. D0i3hot and D0i3cold. If no cable connected, we will trigger D0i3cold which will power gate every clock/power used by dwc3 controller. And entering D0i3hot with hibernation mode when acting as host mode (micro A cable connected), also during device mode(micro B cable connected to PC host). For ID/VBus detection, we are using PMIC to do detect. So we will also power gate the USB PHY for D0i3cold. When we plug in micro A/B cable, the PMIC will report the ID/VBus change event, then driver will force controller resume to D0 from D0i3cold. Due to we haven't do any backup before entering D0i3cold, so we have to re-initialize all host/device portion registers with setting GCTL.PortCapDir to 01 or 10. So with this PM design and DRD mode, we can't use your OTG mode. :( I want to get your suggestions for DRD mode of dwc3 driver. We add DRD support based on your otg.c or create new file drd.c? Thanks Yu -- 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: [PATCH 4/4] xhci-plat: Don't enable legacy PCI interrupts.
On Thu, Aug 15, 2013 at 06:43:59PM -0700, Sarah Sharp wrote: The xHCI platform driver calls into usb_add_hcd to register the irq for its platform device. It does not want the xHCI generic driver to register an interrupt for it at all. The original code did that by setting the XHCI_BROKEN_MSI quirk, which tells the xHCI driver to not enable MSI or MSI-X for a PCI host. Unfortunately, if CONFIG_PCI is enabled, and CONFIG_USB_DW3 is enabled, the xHCI generic driver will attempt to register a legacy PCI interrupt for the xHCI platform device in xhci_try_enable_msi(). This will result in a bogus irq being registered, since the underlying device is a platform_device, not a pci_device, and thus the pci_device-irq pointer will be bogus. What shipping hardware has this problem today? In other words, doesn't seem like late -rc material to me. thanks, greg k-h [Yu:] I met this issue on Intel mobile SOC(Merrifield). But to my understanding, this issue should not depend on hardware. This is one software issue. When system enabled CONFIG_PCI and xHCI driver registered as platform device driver. Then xHCI-plat driver will be met initialization failed. -- 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 -- 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: About the hibernation feature implementation in dwc3 driver
From: Wang, Yu Y [mailto:yu.y.w...@intel.com] Sent: Wednesday, August 14, 2013 8:09 PM From: Felipe Balbi [mailto:ba...@ti.com] Sent: Tuesday, August 13, 2013 1:30 PM On Tue, Aug 13, 2013 at 08:04:26PM +, Paul Zimmerman wrote: From: Felipe Balbi Sent: Tuesday, August 13, 2013 12:20 PM On Mon, Aug 05, 2013 at 03:41:57PM +, Wang, Yu Y wrote: Hi Balbi, Please check the attached logs. The kernel base one kernel3.10. you didn't take the regdump after xHCI :-( I need to check if erst_base is being mirrored to DEPCMDPAR* Hi Felipe, There seems to be some basic misunderstanding here. ALL versions of the Synopsys core share the internal RAM between device and host modes. So the only supported way of switching modes is to shut down the driver for the mode you are leaving, then start up the driver for the mode you are entering and re-initialize (most of) the registers. This is described in the databook in the OTG - Software Flow and OTG - Programming Model chapters. sure. So whether a particular set of RAM-based registers is mirrored between modes does not matter. fair enough. And I don't see what this has to do with hibernation? I have lost track of the conversation, probably. but I believe Yu mentioned resetting the IP everytime when coming out of hibernation and, for whatever reason, I confused myself with the other problem. OK :) By the way, I wanted to tell Yu that you (Felipe) are correct about not resetting the core when coming out of hibernation. That is definitely not required, and would probably break the resume from hibernation. I think we (Synopsys) should update the databook to mention that explicitly, since it is different from the normal initialization flow. -- Paul [Yu:] One question. If follow baili's design. When nothing connected, both host and device Should under hibernation mode, right? That is mean U2PMU/U3PMU should be save *two* copies of backup data respective for host and device mode, right? I don't think the U2PMU/U3PMU have this capability. No. One of the modes will be disabled, depending on the state of the ID pin. So only the mode that is active will enter hibernation. And, if nothing is connected, there is no need to save the state data anyway. [Yu:] Correct. This is my design in Intel platforms. So during switch, whatever if Enabled hibernation mode, we need to re-initialization corresponding mode. Right? -- Paul -- 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: [PATCH 4/4] xhci-plat: Don't enable legacy PCI interrupts.
On Fri, Aug 16, 2013 at 02:22:43AM +, Wang, Yu Y wrote: On Thu, Aug 15, 2013 at 06:43:59PM -0700, Sarah Sharp wrote: The xHCI platform driver calls into usb_add_hcd to register the irq for its platform device. It does not want the xHCI generic driver to register an interrupt for it at all. The original code did that by setting the XHCI_BROKEN_MSI quirk, which tells the xHCI driver to not enable MSI or MSI-X for a PCI host. Unfortunately, if CONFIG_PCI is enabled, and CONFIG_USB_DW3 is enabled, the xHCI generic driver will attempt to register a legacy PCI interrupt for the xHCI platform device in xhci_try_enable_msi(). This will result in a bogus irq being registered, since the underlying device is a platform_device, not a pci_device, and thus the pci_device-irq pointer will be bogus. What shipping hardware has this problem today? In other words, doesn't seem like late -rc material to me. thanks, greg k-h [Yu:] I met this issue on Intel mobile SOC(Merrifield). But to my understanding, this issue should not depend on hardware. This is one software issue. When system enabled CONFIG_PCI and xHCI driver registered as platform device driver. Then xHCI-plat driver will be met initialization failed. Yes, but what normal system ever registers the xHCI driver as a platform driver? Does this happen today with systems? Which ones? [Yu:] Yes. I think there is no scenario for xHCI driver register as a platform driver. Then can't reproduce on normal system. And is Merrifield shipping in devices today (sorry, I don't know the Intel mobile codenames anymore, it's been almost a year since I saw my last Intel powerpoint presentation... [Yu:] It should not only on Merrifield. But should be also met for ARM SOC which use dwc3 controller as xHCI controller. Because DWC3 driver register xHCI as platform driver by default. So this issue can be reproduce. thanks, greg k-h -- 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: [PATCH 4/4] xhci-plat: Don't enable legacy PCI interrupts.
On Fri, Aug 16, 2013 at 04:01:50AM +, Wang, Yu Y wrote: On Fri, Aug 16, 2013 at 02:22:43AM +, Wang, Yu Y wrote: On Thu, Aug 15, 2013 at 06:43:59PM -0700, Sarah Sharp wrote: The xHCI platform driver calls into usb_add_hcd to register the irq for its platform device. It does not want the xHCI generic driver to register an interrupt for it at all. The original code did that by setting the XHCI_BROKEN_MSI quirk, which tells the xHCI driver to not enable MSI or MSI-X for a PCI host. Unfortunately, if CONFIG_PCI is enabled, and CONFIG_USB_DW3 is enabled, the xHCI generic driver will attempt to register a legacy PCI interrupt for the xHCI platform device in xhci_try_enable_msi(). This will result in a bogus irq being registered, since the underlying device is a platform_device, not a pci_device, and thus the pci_device-irq pointer will be bogus. What shipping hardware has this problem today? In other words, doesn't seem like late -rc material to me. thanks, greg k-h [Yu:] I met this issue on Intel mobile SOC(Merrifield). But to my understanding, this issue should not depend on hardware. This is one software issue. When system enabled CONFIG_PCI and xHCI driver registered as platform device driver. Then xHCI-plat driver will be met initialization failed. Yes, but what normal system ever registers the xHCI driver as a platform driver? Does this happen today with systems? Which ones? [Yu:] Yes. I think there is no scenario for xHCI driver register as a platform driver. Then can't reproduce on normal system. Thanks for confirming. Then you have no objection for this waiting until 3.12-rc1 to be merged? [Yu:] Ok for me. But not sure if ok with Sarah and Baili. :-) And is Merrifield shipping in devices today (sorry, I don't know the Intel mobile codenames anymore, it's been almost a year since I saw my last Intel powerpoint presentation... [Yu:] It should not only on Merrifield. Are any Merrifield systems in the wild yet? Can I buy one? [Yu:] I am not sure. But I don't think it can be find in market so far. But should be also met for ARM SOC which use dwc3 controller as xHCI controller. Because DWC3 driver register xHCI as platform driver by default. So this issue can be reproduce. Which system can I reproduce this on? Do you know of a board with a dwc3 controller running in xHCI mode that I can get to test with (I would _love_ to get an ARM board running in xHCI mode for testing.) thanks, greg k-h [Yu:] I think some boards with TI OMAP SOC can be reproduce it. Need check with Balbi~ :-) Thanks, Yu -- 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: About the hibernation feature implementation in dwc3 driver
From: Felipe Balbi [mailto:ba...@ti.com] Sent: Tuesday, August 13, 2013 1:30 PM On Tue, Aug 13, 2013 at 08:04:26PM +, Paul Zimmerman wrote: From: Felipe Balbi Sent: Tuesday, August 13, 2013 12:20 PM On Mon, Aug 05, 2013 at 03:41:57PM +, Wang, Yu Y wrote: Hi Balbi, Please check the attached logs. The kernel base one kernel3.10. you didn't take the regdump after xHCI :-( I need to check if erst_base is being mirrored to DEPCMDPAR* Hi Felipe, There seems to be some basic misunderstanding here. ALL versions of the Synopsys core share the internal RAM between device and host modes. So the only supported way of switching modes is to shut down the driver for the mode you are leaving, then start up the driver for the mode you are entering and re-initialize (most of) the registers. This is described in the databook in the OTG - Software Flow and OTG - Programming Model chapters. sure. So whether a particular set of RAM-based registers is mirrored between modes does not matter. fair enough. And I don't see what this has to do with hibernation? I have lost track of the conversation, probably. but I believe Yu mentioned resetting the IP everytime when coming out of hibernation and, for whatever reason, I confused myself with the other problem. OK :) By the way, I wanted to tell Yu that you (Felipe) are correct about not resetting the core when coming out of hibernation. That is definitely not required, and would probably break the resume from hibernation. I think we (Synopsys) should update the databook to mention that explicitly, since it is different from the normal initialization flow. -- Paul [Yu:] One question. If follow baili's design. When nothing connected, both host and device Should under hibernation mode, right? That is mean U2PMU/U3PMU should be save *two* copies of backup data respective for host and device mode, right? I don't think the U2PMU/U3PMU have this capability. With balbi's design. If no role switch, and no disconnect case. It should be working for hibernation. But with role switch, we have to re-initialization. -- 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 -- 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: About the hibernation feature implementation in dwc3 driver
Hi, On Tue, Aug 13, 2013 at 08:04:26PM +, Paul Zimmerman wrote: From: Felipe Balbi Sent: Tuesday, August 13, 2013 12:20 PM On Mon, Aug 05, 2013 at 03:41:57PM +, Wang, Yu Y wrote: Hi Balbi, Please check the attached logs. The kernel base one kernel3.10. you didn't take the regdump after xHCI :-( I need to check if erst_base is being mirrored to DEPCMDPAR* Hi Felipe, There seems to be some basic misunderstanding here. ALL versions of the Synopsys core share the internal RAM between device and host modes. So the only supported way of switching modes is to shut down the driver for the mode you are leaving, then start up the driver for the mode you are entering and re-initialize (most of) the registers. This is described in the databook in the OTG - Software Flow and OTG - Programming Model chapters. sure. So whether a particular set of RAM-based registers is mirrored between modes does not matter. fair enough. And I don't see what this has to do with hibernation? [Yu:] Sorry. Miss this question. For hibernation mode, if not met disconnect case, then needn't do soft-reset. But for disconnect case, spec said need to re-initialization. Because when controller enter hibernation mode, all backup data will be store in the hiber-storage inside of U2PMU/U3PMU. But for disconnect case, the spec said need to do Device Power-On or Soft Reset(2.50a spec 12.2.3.5). The reset will reset the PMU along with vaux_reset. Then all backup data will be lost. So driver have to re-initialization all registers. I have lost track of the conversation, probably. but I believe Yu mentioned resetting the IP everytime when coming out of hibernation and, for whatever reason, I confused myself with the other problem. [Yu:] Correct. We have to re-initialization device and host stack before switch to corresponding mode. And we will planning to upstream some patches in the near future. Include one common DWC3 OTG driver which maintain one OTG state machine, support USB charger detection and role switch. And it need one hardware ops structure registered by platform dependent driver to implemented. And also have separate drivers for host/otg/udc implementation for Intel mobile platforms. If they can be merged. We will submit patches for hibernation feature base on them. We are still preparing clear up our patches. OTG and host almost done. The device mode patches still doing now. After all things done, I will send them to review. Thanks Balbi. -- balbi -- 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 role switch cause IRQ request failed
On Fri, Aug 09, 2013 at 03:16:20PM +, Wang, Yu Y wrote: On Fri, Aug 09, 2013 at 01:34:09PM +, Wang, Yu Y wrote: On Wed, Aug 07, 2013 at 12:03:34PM +, Wang, Yu Y wrote: Hi Balbi, Because dwc3 driver request_threaded_irq with flags IRQF_ONESHOT and IRQF_SHARED. But xHCI driver will not set IRQF_ONESHOT. Then will met IRQ request failed if use same IRQ number. 4[1.019248] Call Trace: 4[1.019270] [c18c9f0f] dump_stack+0x16/0x18 4[1.019292] [c18c723c] __schedule_bug+0x65/0x77 4[1.019313] [c18cf48d] __schedule+0x7ad/0x830 4[1.019335] [c18b8080] ? rest_init+0xc0/0xc0 4[1.019358] [c1204b02] ? printk_address+0x32/0x40 4[1.019380] [c1293ae0] ? __module_text_address+0x10/0x60 4[1.019402] [c12991fb] ? is_module_text_address+0x2b/0x50 4[1.019424] [c125707f] ? __kernel_text_address+0x4f/0x70 4[1.019444] [c18cf583] schedule+0x23/0x60 4[1.019466] [c18cc765] schedule_timeout+0x125/0x1f0 4[1.019488] [c18ce66b] ? wait_for_completion+0x2b/0xe0 4[1.019509] [c18ce6d8] ? wait_for_completion+0x98/0xe0 4[1.019530] [c18ce6df] wait_for_completion+0x9f/0xe0 4[1.019551] [c12699f0] ? try_to_wake_up+0x280/0x280 4[1.019572] [c1259fb1] kthread_stop+0x41/0x100 4[1.019595] [c12a8256] __setup_irq+0xd6/0x460 4[1.019617] [c1641520] ? dwc3_gadget_set_selfpowered+0x60/0x60 4[1.019639] [c12a8682] request_threaded_irq+0xa2/0x120 4[1.019660] [c16437f0] ? dwc3_gadget_reset_interrupt+0x1a0/0x1a0 4[1.019681] [c1642c54] dwc3_gadget_start+0x1a4/0x1f0 4[1.019703] [c166ff47] udc_bind_to_driver+0x57/0x100 4[1.019724] [c1670481] usb_gadget_probe_driver+0xa1/0xe0 4[1.019745] [c153c618] ? device_create_file+0x38/0xb0 4[1.019766] [c167142f] usb_composite_probe+0x6f/0x90 4[1.019788] [c1c762c5] init+0x147/0x19f 4[1.019809] [c1c7617e] ? acmmod_init+0xf/0xf 4[1.019829] [c12001aa] do_one_initcall+0xba/0x170 4[1.019853] [c1c3fb32] kernel_init_freeable+0x119/0x1b8 4[1.019875] [c1c3f4dc] ? do_early_param+0x7a/0x7a 4[1.019896] [c18b8090] kernel_init+0x10/0xd0 4[1.019917] [c18d17f7] ret_from_kernel_thread+0x1b/0x28 4[1.019937] [c18b8080] ? rest_init+0xc0/0xc0 3[1.020082] dwc3 dwc3.0.auto: failed to request irq #34 -- -16 This is one known issue or new issue? I have already removed IRQF_ONESHOT, it'll be on v3.12 [Yu:] Thanks balbi. Can you please share the patch link? http://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h= nextid=e8 adfc30ff9282a728fd8b666b6418308164c415 as usual, it's in my 'next' branch. you really need to fix your mailer btw, I got many copies of this same message. [Yu:] Thanks balbi. Don't know the reason. I send the email to you, and always get send failed response. So I tried more times. Sorry about that. BTW, Where can I find your git server link? I want to clone it to get latest code base. on git.kernel.org, in the link above, click on 'summary' and scroll down to the end of the page. [Yu:] Get it. Thanks. -- balbi -- 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 role switch cause IRQ request failed
On Fri, Aug 09, 2013 at 01:34:09PM +, Wang, Yu Y wrote: On Wed, Aug 07, 2013 at 12:03:34PM +, Wang, Yu Y wrote: Hi Balbi, Because dwc3 driver request_threaded_irq with flags IRQF_ONESHOT and IRQF_SHARED. But xHCI driver will not set IRQF_ONESHOT. Then will met IRQ request failed if use same IRQ number. 4[1.019248] Call Trace: 4[1.019270] [c18c9f0f] dump_stack+0x16/0x18 4[1.019292] [c18c723c] __schedule_bug+0x65/0x77 4[1.019313] [c18cf48d] __schedule+0x7ad/0x830 4[1.019335] [c18b8080] ? rest_init+0xc0/0xc0 4[1.019358] [c1204b02] ? printk_address+0x32/0x40 4[1.019380] [c1293ae0] ? __module_text_address+0x10/0x60 4[1.019402] [c12991fb] ? is_module_text_address+0x2b/0x50 4[1.019424] [c125707f] ? __kernel_text_address+0x4f/0x70 4[1.019444] [c18cf583] schedule+0x23/0x60 4[1.019466] [c18cc765] schedule_timeout+0x125/0x1f0 4[1.019488] [c18ce66b] ? wait_for_completion+0x2b/0xe0 4[1.019509] [c18ce6d8] ? wait_for_completion+0x98/0xe0 4[1.019530] [c18ce6df] wait_for_completion+0x9f/0xe0 4[1.019551] [c12699f0] ? try_to_wake_up+0x280/0x280 4[1.019572] [c1259fb1] kthread_stop+0x41/0x100 4[1.019595] [c12a8256] __setup_irq+0xd6/0x460 4[1.019617] [c1641520] ? dwc3_gadget_set_selfpowered+0x60/0x60 4[1.019639] [c12a8682] request_threaded_irq+0xa2/0x120 4[1.019660] [c16437f0] ? dwc3_gadget_reset_interrupt+0x1a0/0x1a0 4[1.019681] [c1642c54] dwc3_gadget_start+0x1a4/0x1f0 4[1.019703] [c166ff47] udc_bind_to_driver+0x57/0x100 4[1.019724] [c1670481] usb_gadget_probe_driver+0xa1/0xe0 4[1.019745] [c153c618] ? device_create_file+0x38/0xb0 4[1.019766] [c167142f] usb_composite_probe+0x6f/0x90 4[1.019788] [c1c762c5] init+0x147/0x19f 4[1.019809] [c1c7617e] ? acmmod_init+0xf/0xf 4[1.019829] [c12001aa] do_one_initcall+0xba/0x170 4[1.019853] [c1c3fb32] kernel_init_freeable+0x119/0x1b8 4[1.019875] [c1c3f4dc] ? do_early_param+0x7a/0x7a 4[1.019896] [c18b8090] kernel_init+0x10/0xd0 4[1.019917] [c18d17f7] ret_from_kernel_thread+0x1b/0x28 4[1.019937] [c18b8080] ? rest_init+0xc0/0xc0 3[1.020082] dwc3 dwc3.0.auto: failed to request irq #34 -- -16 This is one known issue or new issue? I have already removed IRQF_ONESHOT, it'll be on v3.12 [Yu:] Thanks balbi. Can you please share the patch link? http://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=nextid=e8 adfc30ff9282a728fd8b666b6418308164c415 as usual, it's in my 'next' branch. you really need to fix your mailer btw, I got many copies of this same message. [Yu:] Thanks balbi. Don't know the reason. I send the email to you, and always get send failed response. So I tried more times. Sorry about that. BTW, Where can I find your git server link? I want to clone it to get latest code base. -- balbi -- 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: xHCI ISR be registered twice
Hi Felipe,
This patch brings up an interesting point: will a dwc3 PCI host use the xhci
platform driver instead of the xhci PCI driver?
If so, that seems less than ideal. Won't it not use the standard xHCI PCI
suspend and resume functions if it's registered as a platform device? Also,
it
seems registering it that way means XHCI_BROKEN_MSI is set unconditionally.
That leads to the xhci driver not enabling MSI or MSI-X for the PCI host,
which
will impact performance.
[Yu:] The upstream dwc3 driver haven't enable power management for the host
mode until now. Actually, this is another big changes. In Intel intern tree, I
have to
wrote another separate file to re-implemented the PM callback to support
platform
device design. Maybe we need consider to add one new file(hcd-plat.c) which is
familiar
with hcd-pci.c?
Hi Yu,
Thanks for taking this bug down. I have some process-oriented issues that
need to be addressed, and some comments that need to be addressed in the
next version of your patch.
[Yu:] Ok. Thanks for your review. Sorry, I am newcomer of the community.
I will setup environment followed the guide and re-submit one new patch.
First, please use this email address to send me patches:
Sarah Sharp sarah.a.sh...@linux.intel.com I use the linux.intel.com
email address to handle all patches and traffic to the Linux mailing lists.
My
intel.com email address is for Intel internal communications only.
In order for us to apply patches, you need to send them inline in the email,
without your introduction. The subject line must be the subject line of the
patch. Basically, you need to use either `git send-email` or `git
format-patch`
and mutt to send the patch.
Please see this tutorial for more information on sending proper patches:
http://kernelnewbies.org/OPWfirstpatch#head-2043a643d048c341b2a52044fd
72d852aac87fef
If you still need to introduce a patch, you can put this scissors line
between
your introduction and the patch description:
8-8
However, in general, your patch description should contain all the information
necessary for us to decide whether we need to apply the patch. If the bug
was only hit with a specific configuration, that needs to be included in the
patch
description, so that distros know whether they need to apply the patch.
[Yu:] Get it. I will provide more details in the comment.
Comments on the patch below.
On Tue, Aug 06, 2013 at 11:08:04PM -0700, Wang, Yu Y wrote:
Hi Balbi, Sarah,
I found that when CONFIG_PCI is set, and xHCI driver register as
platform device driver.
Then xhci ISR will be register twice.
The first time is in usb_add_hcd, the second time is in xhci_run
(xhci_try_enable_msi).
This issue should be reproduce when dwc3 driver registered as PCI
device driver.
And CONFIG_USB_DWC3_DUAL_ROLE is set.
This information should all be in your patch description. It's important to
document how to reproduce the bug you're fixing in the patch that fixes it.
Fixed patch please help review:
Hopefully when you use `git send-email` or mutt you won't get these extra
newlines. Don't send patches through outlook, it completely mangles them.
You'll need to set up esmtp on a Linux system in order to be able to send
mail to
the Intel exchange servers with `git send-email` or mutt.
From 8b437ac59be296cd7c0fa189344f3b30281fb3f1 Mon Sep 17 00:00:00
2001
From: Wang, Yu yu.y.w...@intel.com
Date: Wed, 7 Aug 2013 13:26:30 +0800
Subject: [PATCH 5/6] xhci: xHCI ISR be registers twice.
This is one xHCI driver BUG. If CONFIG_PCI be set and xHCI driver
registers as platform driver. Then xHCI ISR will be registers twice,
the
first time is during usb_add_hcd. The second time is in xhci_run.
You'll need a newline between your description and the Signed-off-by line. I
can't tell if you currently have one because of the mangled patch.
Signed-off-by: Wang, Yu yu.y.w...@intel.com
Change-Id: Ibf86bca8f099586a0f379ee843b95c4d93b88d67
---
drivers/usb/host/xhci.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d8f640b..b43f722 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -372,6 +372,10 @@ static int xhci_try_enable_msi(struct usb_hcd
*hcd)
}
legacy_irq:
+ /* The leacy irq was already registered in USB core */
+ if (hcd-irq)
+ return 0;
+
Let's take a look at the function you're patching:
static int xhci_try_enable_msi(struct usb_hcd *hcd) {
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct pci_dev *pdev =
to_pci_dev(xhci_to_hcd(xhci)-self.controller);
int ret;
/*
* Some Fresco Logic host controllers advertise MSI
RE: About the hibernation feature implementation in dwc3 driver
Hi Balbi,
Please check the attached logs. The kernel base one kernel3.10.
Thanks,
Yu
On Fri, Aug 02, 2013 at 03:42:20PM +, Wang, Yu Y wrote:
Hi,
On Fri, Aug 02, 2013 at 10:54:18AM +, Wang, Yu Y wrote:
Check my comments as follows.
weird, you sent plain text email, but there are no quotation
marks... it makes it very difficult to read. Please go through our
netiquette, there is a copy of that at [1] and also lots of good
information under Documentation, including tips on how to
configure many
email client.
[Yu:] Thanks for your help. Now I changed the configuration to add
quotation marks :)
np
On Fri, Aug 02, 2013 at 09:25:39AM +, Wang, Yu Y wrote:
Thanks the quick response.
Actually, our solution is familiar with your thinking.
Due to we have to do Power-On or Soft Reset for disconnect
and re-connect case, so driver need re-initialized all hardware
registers.
you don't need a full Soft-reset when disconnecting the cable.
Read the driver and you'll we don't soft-reset the IP at every
disconnect.
[Yu] I don't know what version of your spec. My spec version is
Version 2.10a January 2012, in section 12.2.3.4.
Configure the core as described in Device
Power-On or Soft Reset. Spec require the core
do soft-reset.
As described in that, but it doesn't mean we need to do a full SW
reset of
the IP.
It just means we need to setup DCTL and all those other registers again.
[Yu:] Correct. So we have to re-initialize all relevant registers
for this requirement. And we want to maximize to re-use original
functions. But we find that we have to modify some code to achieve
this purposes.
hmm.. weird. We already system suspend/resume working and I
re-factored all code needed exactly to avoid duplication. Are you
reading v3.11-rc3 code or some older tree ?
[Yu:] No. Haven't check latest code.
I will check 3.11 code for these new changes. Thanks.
That's why we want to separate register initialization
operation from software part (like sw structure init and
endpoint
initialization)
register initialization is also SW, so I don't know what you
want to split
here.
[Yu] We just want to split the SW initialization and hardware
settings.
and what do you mean by that ? What are you calling SW initialization
and what do you consider to be hardware settings ?
[Yu:] The SW initialization is mean that no relevant hardware
registers
access.
The hardware settings is mean set some configurations via write
controller registers.
alright, that's all factored out. If you read v3.11-rc3 there are two
situations where we have memory allocation and register initialization
together, but if you look into my 'next' branch in kernel.org, you'll
see that I already have patches fixing those up.
[Yu:] Great. I will check your new patches for these changes.
However, we find that the hardware registers initialization is
located in dwc3_gadget_init and 'dwc-gadget_driver'
initialization is handled in dwc3_gadget_start.() So we can't
full re-use these two
functions.
this is wrong. dwc3_gadget_init() does nothing more than
allocate
memory.
[Yu] In kernel3.10. In dwc3_gadget_init(), We find some hardware
setting.
For example, set clear suspend enable bit of GUSB2PHYCFG and
GUSB3PIPECFG registers. Maybe we need to check the new version?
hmm, that code wouldn't even run in version 2.10a of the core,
it's for versions before 1.94a as you can see below:
| int dwc3_gadget_init(struct dwc3 *dwc) {
| u32 reg;
| int ret;
[ ... ]
| /* Enable USB2 LPM and automatic phy suspend only on recent
versions */
| if (dwc-revision = DWC3_REVISION_194A) {
| dwc3_gadget_usb2_phy_suspend(dwc, false);
| dwc3_gadget_usb3_phy_suspend(dwc, false);
| }
[ ... ]
| }
and, recently, I've dropped support for manual PHY control, see [2].
[Yu:]This patch is good. But for hibernation mode, still need to set
these two PHY registers. Anyway, we can consider that to use other
implementations for them
But hey, starting in version 1.94a of the core, the core itself will
manage the PHY automatically. We shouldn't have to do anything with
them. Besides, even if we _do_ have to manuall suspend the PHY when in
hibernation, that's a power optimization thing, it shouldn't break
anything if we leave the PHYs resumed ;-)
Which means that this can be tackled later :-)
[Yu:] Correct. But with 2.10a RTL. There have some silicon BUGs. You can check
the section
RE: About the hibernation feature implementation in dwc3 driver
Hi,
On Fri, Aug 02, 2013 at 03:42:20PM +, Wang, Yu Y wrote:
Hi,
On Fri, Aug 02, 2013 at 10:54:18AM +, Wang, Yu Y wrote:
Check my comments as follows.
weird, you sent plain text email, but there are no quotation
marks... it makes it very difficult to read. Please go through our
netiquette, there is a copy of that at [1] and also lots of good
information under Documentation, including tips on how to configure many
email client.
[Yu:] Thanks for your help. Now I changed the configuration to add
quotation marks :)
np
On Fri, Aug 02, 2013 at 09:25:39AM +, Wang, Yu Y wrote:
Thanks the quick response.
Actually, our solution is familiar with your thinking.
Due to we have to do Power-On or Soft Reset for disconnect and
re-connect case, so driver need re-initialized all hardware registers.
you don't need a full Soft-reset when disconnecting the cable.
Read the driver and you'll we don't soft-reset the IP at every
disconnect.
[Yu] I don't know what version of your spec. My spec version is
Version 2.10a January 2012, in section 12.2.3.4.
Configure the core as described in Device
Power-On or Soft Reset. Spec require the core
do soft-reset.
As described in that, but it doesn't mean we need to do a full SW reset of
the IP.
It just means we need to setup DCTL and all those other registers again.
[Yu:] Correct. So we have to re-initialize all relevant registers for
this requirement. And we want to maximize to re-use original
functions. But we find that we have to modify some code to achieve
this purposes.
hmm.. weird. We already system suspend/resume working and I re-factored all
code needed exactly to avoid duplication. Are you reading v3.11-rc3 code or
some older tree ?
[Yu:] No. Haven't check latest code.
I will check 3.11 code for these new changes. Thanks.
That's why we want to separate register initialization operation
from software part (like sw structure init and endpoint
initialization)
register initialization is also SW, so I don't know what you want to
split
here.
[Yu] We just want to split the SW initialization and hardware settings.
and what do you mean by that ? What are you calling SW initialization
and what do you consider to be hardware settings ?
[Yu:] The SW initialization is mean that no relevant hardware registers
access.
The hardware settings is mean set some configurations via write
controller registers.
alright, that's all factored out. If you read v3.11-rc3 there are two
situations
where we have memory allocation and register initialization together, but if
you
look into my 'next' branch in kernel.org, you'll see that I already have
patches
fixing those up.
[Yu:] Great. I will check your new patches for these changes.
However, we find that the hardware registers initialization is
located in dwc3_gadget_init and 'dwc-gadget_driver'
initialization is handled in dwc3_gadget_start.() So we can't
full re-use these two
functions.
this is wrong. dwc3_gadget_init() does nothing more than allocate
memory.
[Yu] In kernel3.10. In dwc3_gadget_init(), We find some hardware
setting.
For example, set clear suspend enable bit of GUSB2PHYCFG and
GUSB3PIPECFG registers. Maybe we need to check the new version?
hmm, that code wouldn't even run in version 2.10a of the core, it's
for versions before 1.94a as you can see below:
| int dwc3_gadget_init(struct dwc3 *dwc) {
| u32 reg;
| int ret;
[ ... ]
| /* Enable USB2 LPM and automatic phy suspend only on recent versions */
| if (dwc-revision = DWC3_REVISION_194A) {
| dwc3_gadget_usb2_phy_suspend(dwc, false);
| dwc3_gadget_usb3_phy_suspend(dwc, false);
| }
[ ... ]
| }
and, recently, I've dropped support for manual PHY control, see [2].
[Yu:]This patch is good. But for hibernation mode, still need to set
these two PHY registers. Anyway, we can consider that to use other
implementations for them
But hey, starting in version 1.94a of the core, the core itself will manage
the
PHY automatically. We shouldn't have to do anything with them. Besides, even
if we _do_ have to manuall suspend the PHY when in hibernation, that's a
power optimization thing, it shouldn't break anything if we leave the PHYs
resumed ;-)
Which means that this can be tackled later :-)
[Yu:] Correct. But with 2.10a RTL. There have some silicon BUGs. You can check
the
section 1.9 of 2.10a spec. For some situation, we have to clear suspend enable
bit of GUSB2PHYCFG register. And for hibernation mode, we have to enable them
during suspend flow.
All device and host hardware initialization will be done in these API.
So
RE: About the hibernation feature implementation in dwc3 driver
Hi Balbi, Thanks the quick response. Actually, our solution is familiar with your thinking. Due to we have to do Power-On or Soft Reset for disconnect and re-connect case, so driver need re-initialized all hardware registers. That's why we want to separate register initialization operation from software part (like sw structure init and endpoint initialization) However, we find that the hardware registers initialization is located in dwc3_gadget_init and 'dwc-gadget_driver' initialization is handled in dwc3_gadget_start.() So we can't full re-use these two functions. If all the hardware initialization can be extract into a new function which call by udc_start callback and also can be called for hibernation disconnect case, then it is fine. And for disconnect, our driver need to do some hardware clear work. For example, disable irq, clear events which haven't handled, and disable endpoints and so on. BTW: Besides that. In our design, we also support host mode, and also have one OTG driver to maintain the role switch and USB charger detection. For role switch, our implementation is not only set GCTL.PORTCAP. Also we implemented some API for OTG driver, for example: start_host/stop_host and start_peripheral/stop_peripheral. All device and host hardware initialization will be done in these API. So for connect/disconnect case, the driver will re-initialization the host or device stack. I saw your design is do all hardware initialization during kernel boot. And just use GCTL.PORTCAP to do the role switch. I haven't try this solution on intel platform. And not sure if is working with hibernation feature. Thanks, Regards, Yu -Original Message- From: Felipe Balbi [mailto:ba...@ti.com] Sent: Friday, August 02, 2013 3:07 PM To: Wang, Yu Y Cc: ba...@ti.com; Li, Jiebing; Linux USB Mailing List Subject: Re: About the hibernation feature implementation in dwc3 driver Hi, +linux-usb Always add linux-usb On Fri, Aug 02, 2013 at 05:33:32AM +, Wang, Yu Y wrote: We are the USB3 driver developers from Intel. And also base on Synopsys DWC USB3 OTG controller. good to see Intel is also using my driver. We base on your driver to enabled hibernation feature for runtime pm suspend both for device and host mode. We just want to check with you, is there any plan to implement hibernation mode in the future? TI's SoCs don't support hibernation, none of them. It's not configured in the IP, so I'd have no way how to test it. Because our changes was a little big for the hibernation mode feature with your driver. It shouldn't. Based on what I read in the spec and the original Hack from Paul Zimmerman, hibernation should be very, very simple. There are two conditions, one is enter suspended with cable connected, another one is enter suspended without cable connected. no, there is only one condition. When there is no cable attached, there's nothing in any register to be initialized, so you don't need to save any context, just call the the setup functions directly and it should work next time you connect the cable. For the first case, we can enter hibernation mode which will backup internal states to u2pmu/u3pmu, and also can restore during the resume flow. There will be few change for this case. right, just a few. But for the second case, followed spec. The controller should not enter hibernation mode. Then there will be no any backup to u2pmu/u3pmu, so we have to re-initialization whole driver stack. right(-ish), but that's already all factored-out into reusable functions. And we are consider to use add/remove the platform device to trigger probe/remove to achieve this purpose. But all data structures will be free and re-alloc. This will be make impacts for the performance. don't do that, it's unnecessary. So our major changes is divide the probe/remove and udc_start/udc_stop to two part. All hardware settings will be move to another special functions which will be called after cable connect/disconnect. heh, gotta revert that part, it's totally unnecessary. So hope you can provide your hibernation solutions as the reference. Please continue this discussion in linux-usb@vger.kernel.org -- balbi -- 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: About the hibernation feature implementation in dwc3 driver
Hi Balibi, Check my comments as follows. Thanks, Regards, Yu -Original Message- From: Felipe Balbi [mailto:ba...@ti.com] Sent: Friday, August 02, 2013 6:34 PM To: Wang, Yu Y Cc: ba...@ti.com; Li, Jiebing; Linux USB Mailing List; Zhuang, Jin Can; Wu, Hao; Yuan, Hang Subject: Re: About the hibernation feature implementation in dwc3 driver Hi, no top-posting please, limit your lines to 80-characters On Fri, Aug 02, 2013 at 09:25:39AM +, Wang, Yu Y wrote: Thanks the quick response. Actually, our solution is familiar with your thinking. Due to we have to do Power-On or Soft Reset for disconnect and re-connect case, so driver need re-initialized all hardware registers. you don't need a full Soft-reset when disconnecting the cable. Read the driver and you'll we don't soft-reset the IP at every disconnect. [Yu] I don't know what version of your spec. My spec version is Version 2.10a January 2012, in section 12.2.3.4. Configure the core as described in Device Power-On or Soft Reset. Spec require the core do soft-reset. That's why we want to separate register initialization operation from software part (like sw structure init and endpoint initialization) register initialization is also SW, so I don't know what you want to split here. [Yu] We just want to split the SW initialization and hardware settings. However, we find that the hardware registers initialization is located in dwc3_gadget_init and 'dwc-gadget_driver' initialization is handled in dwc3_gadget_start.() So we can't full re-use these two functions. this is wrong. dwc3_gadget_init() does nothing more than allocate memory. [Yu] In kernel3.10. In dwc3_gadget_init(), We find some hardware setting. For example, set clear suspend enable bit of GUSB2PHYCFG and GUSB3PIPECFG registers. Maybe we need to check the new version? If all the hardware initialization can be extract into a new function which call by udc_start callback and also can be called for hibernation disconnect case, then it is fine. that's already how it works. Just read the code. And for disconnect, our driver need to do some hardware clear work. For example, disable irq, clear events which haven't handled, and disable endpoints and so on. that's already how it works. Just read the code. BTW: Besides that. In our design, we also support host mode, and also have one OTG driver to maintain the role switch and USB charger detection. right, Ido from codeaurora had some design to support but the work was lost since he stopped sending newer versions. For role switch, our implementation is not only set GCTL.PORTCAP. Also we implemented some API for OTG driver, for example: start_host/stop_host and start_peripheral/stop_peripheral. alright, now go ahead and remove those since they're likely not needed. All device and host hardware initialization will be done in these API. So for connect/disconnect case, the driver will re-initialization the host or device stack. this wastes too much time and is usually unnecessary. [Yu] Base on this design, the core will be reset during role switching. Not only controller, also for the PHY. We will try our design to confirm if working. I saw your design is do all hardware initialization during kernel boot. And just use GCTL.PORTCAP to do the role switch. I haven't try this solution on intel platform. And not sure if is working with hibernation feature. right, then test and let me know the results. Also, instead of spending so much time telling me what kind of changes Intel has in their own hidden tree, why don't you go ahead and send patches for review ? That would be so much easier for both of us... [Yu] Actually. The hibernation feature waste us lots of time. And we will try your design to confirm if hibernation mode still working. And we will also try to change our drivers to better upstream. After that, We will provide the patch to review. -- balbi -- 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: About the hibernation feature implementation in dwc3 driver
Hi,
On Fri, Aug 02, 2013 at 10:54:18AM +, Wang, Yu Y wrote:
Check my comments as follows.
weird, you sent plain text email, but there are no quotation marks... it
makes it
very difficult to read. Please go through our netiquette, there is a copy of
that
at [1] and also lots of good information under Documentation, including tips
on
how to configure many email client.
[Yu:] Thanks for your help. Now I changed the configuration to add quotation
marks :)
On Fri, Aug 02, 2013 at 09:25:39AM +, Wang, Yu Y wrote:
Thanks the quick response.
Actually, our solution is familiar with your thinking.
Due to we have to do Power-On or Soft Reset for disconnect and
re-connect case, so driver need re-initialized all hardware registers.
you don't need a full Soft-reset when disconnecting the cable. Read
the driver and you'll we don't soft-reset the IP at every disconnect.
[Yu] I don't know what version of your spec. My spec version is
Version 2.10a January 2012, in section 12.2.3.4.
Configure the core as described in Device
Power-On or Soft Reset. Spec require the core
do soft-reset.
As described in that, but it doesn't mean we need to do a full SW reset of
the IP.
It just means we need to setup DCTL and all those other registers again.
[Yu:] Correct. So we have to re-initialize all relevant registers for this
requirement.
And we want to maximize to re-use original functions. But we find that we have
to
modify some code to achieve this purposes.
That's why we want to separate register initialization operation
from software part (like sw structure init and endpoint
initialization)
register initialization is also SW, so I don't know what you want to split
here.
[Yu] We just want to split the SW initialization and hardware settings.
and what do you mean by that ? What are you calling SW initialization
and what do you consider to be hardware settings ?
[Yu:] The SW initialization is mean that no relevant hardware registers access.
The hardware settings is mean set some configurations via write controller
registers.
However, we find that the hardware registers initialization is
located in dwc3_gadget_init and 'dwc-gadget_driver' initialization
is handled in dwc3_gadget_start.() So we can't full re-use these two
functions.
this is wrong. dwc3_gadget_init() does nothing more than allocate memory.
[Yu] In kernel3.10. In dwc3_gadget_init(), We find some hardware setting.
For example, set clear suspend enable bit of GUSB2PHYCFG and
GUSB3PIPECFG registers. Maybe we need to check the new version?
hmm, that code wouldn't even run in version 2.10a of the core, it's for
versions
before 1.94a as you can see below:
| int dwc3_gadget_init(struct dwc3 *dwc) {
| u32 reg;
| int ret;
[ ... ]
| /* Enable USB2 LPM and automatic phy suspend only on recent versions */
| if (dwc-revision = DWC3_REVISION_194A) {
| dwc3_gadget_usb2_phy_suspend(dwc, false);
| dwc3_gadget_usb3_phy_suspend(dwc, false);
| }
[ ... ]
| }
and, recently, I've dropped support for manual PHY control, see [2].
[Yu:]This patch is good. But for hibernation mode, still need to set these
two PHY registers. Anyway, we can consider that to use other implementations
for them
.
All device and host hardware initialization will be done in these API.
So for connect/disconnect case, the driver will re-initialization
the host or device stack.
this wastes too much time and is usually unnecessary.
[Yu] Base on this design, the core will be reset during role
switching. Not only controller, also for the PHY. We will
try our design to confirm if working.
that's way too much time wasted. You *really* don't need to reset the whole
thing just to change roles. I doubt that's how Synopsys designed the IP, if
they
did then nobody will ever pass the tight timing requirements imposed by OTG
specification.
The only problem I know about in that area is that the IP mirrors some device
registers to important xHCI register. We have a case with Synopsys to track
that but I'd say all versions suffer from that bug all the way to 2.50a, IIRC
2.60a
has that sorted out.
BTW, it might very well be that because of this problem you're suggesting that
we Soft-Reset the IP, perhaps Synopsys didn't give you the full gory details
? But
in summary, writing to some of the DEPCMDPAR registers, mirrors the same
value into the xHCI's Ring registers, thus destroying any functionality when
switching from device to host and back.
We need to *really* think how we want to support these older versions of the
Synopsys IP since that will be a *really* invasive change on both
dwc3 and xhci drivers. My *strong* suggestion is to focus on stabilizing role
switching before trying
