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
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 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
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"
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
---
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
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" > > > > 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"
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
---
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
[RFC] xhci: Fix xhci block system enter system suspend state.
From: "Wang, Yu"
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 usb2&usb3 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
---
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 USB2&USB3 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.
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 >> >
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 >> > featu
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
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 Fri, Aug 16, 2013 at 04:01:50AM +0000, Wang, Yu Y wrote: > > > On Fri, Aug 16, 2013 at 02:22:43AM +0000, 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: [PATCH 4/4] xhci-plat: Don't enable legacy PCI interrupts.
> On Fri, Aug 16, 2013 at 02:22:43AM +0000, 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: 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 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: 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: 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? > > 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 +0000, Wang, Yu Y wrote: > > > On Fri, Aug 09, 2013 at 01:34:09PM +0000, 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] [] dump_stack+0x16/0x18 > > > > > > <4>[1.019292] [] __schedule_bug+0x65/0x77 > > > > > > <4>[1.019313] [] __schedule+0x7ad/0x830 > > > > > > <4>[1.019335] [] ? rest_init+0xc0/0xc0 > > > > > > <4>[1.019358] [] ? printk_address+0x32/0x40 > > > > > > <4>[1.019380] [] ? > __module_text_address+0x10/0x60 > > > > > > <4>[1.019402] [] ? > is_module_text_address+0x2b/0x50 > > > > > > <4>[1.019424] [] ? > __kernel_text_address+0x4f/0x70 > > > > > > <4>[1.019444] [] schedule+0x23/0x60 > > > > > > <4>[1.019466] [] schedule_timeout+0x125/0x1f0 > > > > > > <4>[1.019488] [] ? wait_for_completion+0x2b/0xe0 > > > > > > <4>[1.019509] [] ? wait_for_completion+0x98/0xe0 > > > > > > <4>[1.019530] [] wait_for_completion+0x9f/0xe0 > > > > > > <4>[1.019551] [] ? try_to_wake_up+0x280/0x280 > > > > > > <4>[1.019572] [] kthread_stop+0x41/0x100 > > > > > > <4>[1.019595] [] __setup_irq+0xd6/0x460 > > > > > > <4>[1.019617] [] ? > > > > > dwc3_gadget_set_selfpowered+0x60/0x60 > > > > > > <4>[1.019639] [] > request_threaded_irq+0xa2/0x120 > > > > > > <4>[1.019660] [] ? > > > > > dwc3_gadget_reset_interrupt+0x1a0/0x1a0 > > > > > > <4>[1.019681] [] dwc3_gadget_start+0x1a4/0x1f0 > > > > > > <4>[1.019703] [] udc_bind_to_driver+0x57/0x100 > > > > > > <4>[1.019724] [] > usb_gadget_probe_driver+0xa1/0xe0 > > > > > > <4>[1.019745] [] ? device_create_file+0x38/0xb0 > > > > > > <4>[1.019766] [] usb_composite_probe+0x6f/0x90 > > > > > > <4>[1.019788] [] init+0x147/0x19f > > > > > > <4>[1.019809] [] ? acmmod_init+0xf/0xf > > > > > > <4>[1.019829] [] do_one_initcall+0xba/0x170 > > > > > > <4>[1.019853] [] kernel_init_freeable+0x119/0x1b8 > > > > > > <4>[1.019875] [] ? do_early_param+0x7a/0x7a > > > > > > <4>[1.019896] [] kernel_init+0x10/0xd0 > > > > > > <4>[1.019917] [] > ret_from_kernel_thread+0x1b/0x28 > > > > > > <4>[1.019937] [] ? 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= > > > next&id=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 +0000, Wang, Yu Y wrote: > > > On Wed, Aug 07, 2013 at 12:03:34PM +0000, 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] [] dump_stack+0x16/0x18 > > > > <4>[1.019292] [] __schedule_bug+0x65/0x77 > > > > <4>[1.019313] [] __schedule+0x7ad/0x830 > > > > <4>[1.019335] [] ? rest_init+0xc0/0xc0 > > > > <4>[1.019358] [] ? printk_address+0x32/0x40 > > > > <4>[1.019380] [] ? __module_text_address+0x10/0x60 > > > > <4>[1.019402] [] ? is_module_text_address+0x2b/0x50 > > > > <4>[1.019424] [] ? __kernel_text_address+0x4f/0x70 > > > > <4>[1.019444] [] schedule+0x23/0x60 > > > > <4>[1.019466] [] schedule_timeout+0x125/0x1f0 > > > > <4>[1.019488] [] ? wait_for_completion+0x2b/0xe0 > > > > <4>[1.019509] [] ? wait_for_completion+0x98/0xe0 > > > > <4>[1.019530] [] wait_for_completion+0x9f/0xe0 > > > > <4>[1.019551] [] ? try_to_wake_up+0x280/0x280 > > > > <4>[1.019572] [] kthread_stop+0x41/0x100 > > > > <4>[1.019595] [] __setup_irq+0xd6/0x460 > > > > <4>[1.019617] [] ? > > > dwc3_gadget_set_selfpowered+0x60/0x60 > > > > <4>[1.019639] [] request_threaded_irq+0xa2/0x120 > > > > <4>[1.019660] [] ? > > > dwc3_gadget_reset_interrupt+0x1a0/0x1a0 > > > > <4>[1.019681] [] dwc3_gadget_start+0x1a4/0x1f0 > > > > <4>[1.019703] [] udc_bind_to_driver+0x57/0x100 > > > > <4>[1.019724] [] usb_gadget_probe_driver+0xa1/0xe0 > > > > <4>[1.019745] [] ? device_create_file+0x38/0xb0 > > > > <4>[1.019766] [] usb_composite_probe+0x6f/0x90 > > > > <4>[1.019788] [] init+0x147/0x19f > > > > <4>[1.019809] [] ? acmmod_init+0xf/0xf > > > > <4>[1.019829] [] do_one_initcall+0xba/0x170 > > > > <4>[1.019853] [] kernel_init_freeable+0x119/0x1b8 > > > > <4>[1.019875] [] ? do_early_param+0x7a/0x7a > > > > <4>[1.019896] [] kernel_init+0x10/0xd0 > > > > <4>[1.019917] [] ret_from_kernel_thread+0x1b/0x28 > > > > <4>[1.019937] [] ? 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=next&id=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: DWC3 role switch cause IRQ request failed
> On Wed, Aug 07, 2013 at 12:03:34PM +0000, 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] [] dump_stack+0x16/0x18 > > <4>[1.019292] [] __schedule_bug+0x65/0x77 > > <4>[1.019313] [] __schedule+0x7ad/0x830 > > <4>[1.019335] [] ? rest_init+0xc0/0xc0 > > <4>[1.019358] [] ? printk_address+0x32/0x40 > > <4>[1.019380] [] ? __module_text_address+0x10/0x60 > > <4>[1.019402] [] ? is_module_text_address+0x2b/0x50 > > <4>[1.019424] [] ? __kernel_text_address+0x4f/0x70 > > <4>[1.019444] [] schedule+0x23/0x60 > > <4>[1.019466] [] schedule_timeout+0x125/0x1f0 > > <4>[1.019488] [] ? wait_for_completion+0x2b/0xe0 > > <4>[1.019509] [] ? wait_for_completion+0x98/0xe0 > > <4>[1.019530] [] wait_for_completion+0x9f/0xe0 > > <4>[1.019551] [] ? try_to_wake_up+0x280/0x280 > > <4>[1.019572] [] kthread_stop+0x41/0x100 > > <4>[1.019595] [] __setup_irq+0xd6/0x460 > > <4>[1.019617] [] ? > dwc3_gadget_set_selfpowered+0x60/0x60 > > <4>[1.019639] [] request_threaded_irq+0xa2/0x120 > > <4>[1.019660] [] ? > dwc3_gadget_reset_interrupt+0x1a0/0x1a0 > > <4>[1.019681] [] dwc3_gadget_start+0x1a4/0x1f0 > > <4>[1.019703] [] udc_bind_to_driver+0x57/0x100 > > <4>[1.019724] [] usb_gadget_probe_driver+0xa1/0xe0 > > <4>[1.019745] [] ? device_create_file+0x38/0xb0 > > <4>[1.019766] [] usb_composite_probe+0x6f/0x90 > > <4>[1.019788] [] init+0x147/0x19f > > <4>[1.019809] [] ? acmmod_init+0xf/0xf > > <4>[1.019829] [] do_one_initcall+0xba/0x170 > > <4>[1.019853] [] kernel_init_freeable+0x119/0x1b8 > > <4>[1.019875] [] ? do_early_param+0x7a/0x7a > > <4>[1.019896] [] kernel_init+0x10/0xd0 > > <4>[1.019917] [] ret_from_kernel_thread+0x1b/0x28 > > <4>[1.019937] [] ? 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? > -- > 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
> On Thu, Aug 08, 2013 at 03:38:53AM +0000, Wang, Yu Y wrote: > > > 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? > > Felipe, what would be the best approach to add PM support for xhci-plat > devices? [Yu:] I talked with Felipe before. The DWC3 controller used in TI SOC, haven't integrate the hibernation mode feature. So their IP can't support PM. But the DWC3 controller used by Intel have enabled the hibernation mode. We are consider to upstream it. But the our DWC3 design have a big difference with DWC3 original driver.For example: we have to do soft-reset every time before start host or device mode. So we have to reinitialize host/device hardware register every switch time. This is requirement from dwc3 spec. And Felipe's design is not do soft-reset. Then the host/device driver just need to initialization at first time. And use one register to do host/device mode switch. This design is not working in Intel mobile platforms. So if upstream the hibernation feature. I need Felipe approve to make dwc3 driver as two solutions for role switch design. Another concern is we can't re-use xHCI standard PM interfaces. Because they are all base on PCI device design(hcd-pci.c ). > > Sarah Sharp -- 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: [RFT] xhci-plat: Don't enable legacy PCI interrupts.
> Hi Yu,
>
> Please test this patch, and make sure that interrupts aren't registered twice.
> I think this approach is better, since it creates a new quirk specifically
> for xhci
> platform devices, so we can tell them apart from PCI devices.
[Yu:] Yes. This patch is working for me. This is one clear solution. Thanks for
help
solute this issue.
>
> Sarah Sharp
>
> >8---8<
>
> 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.
>
> Add a new quirk, XHCI_PLAT, so that the xHCI generic driver can distinguish
> between a PCI device that can't handle MSI or MSI-X, and a platform device
> that should not have its interrupts touched at all.
> This quirk may be useful in the future, in case other corner cases like this
> arise.
>
> This patch should be backported to kernels as old as 3.9, that contain the
> commit 00eed9c814cb8f281be6f0f5d8f45025dc0a97eb "USB: xhci:
> correctly enable interrupts".
>
> Signed-off-by: Sarah Sharp
> Reported-by: Yu Y Wang
> Cc: sta...@vger.kernel.org
> ---
> drivers/usb/host/xhci-plat.c | 2 +-
> drivers/usb/host/xhci.c | 7 ++-
> drivers/usb/host/xhci.h | 1 +
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index
> 51e22bf..6eca5a5 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -24,7 +24,7 @@ static void xhci_plat_quirks(struct device *dev, struct
> xhci_hcd *xhci)
>* here that the generic code does not try to make a pci_dev from our
>* dev struct in order to setup MSI
>*/
> - xhci->quirks |= XHCI_BROKEN_MSI;
> + xhci->quirks |= XHCI_PLAT;
> }
>
> /* called during probe() after chip reset completes */ diff --git
> a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 9478caa..ead3555
> 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -343,9 +343,14 @@ static void __maybe_unused
> xhci_msix_sync_irqs(struct xhci_hcd *xhci) 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);
> + struct pci_dev *pdev;
> int ret;
>
> + /* The xhci platform device has set up IRQs through usb_add_hcd. */
> + if (xhci->quirks & XHCI_PLAT)
> + return 0;
> +
> + pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> /*
>* Some Fresco Logic host controllers advertise MSI, but fail to
>* generate interrupts. Don't even try to enable MSI.
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index
> c338741..6ab1e60 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1542,6 +1542,7 @@ struct xhci_hcd {
> #define XHCI_SPURIOUS_REBOOT (1 << 13)
> #define XHCI_COMP_MODE_QUIRK (1 << 14)
> #define XHCI_AVOID_BEI (1 << 15)
> +#define XHCI_PLAT(1 << 16)
> unsigned intnum_active_eps;
> unsigned intlimit_active_eps;
> /* There are two roothubs to keep track of bus suspend info for */
> --
> 1.8.3.3
--
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 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 > > > > 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 > > > > > > > > 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/dri
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 +0000, 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. > >
RE: About the hibernation feature implementation in dwc3 driver
>
> Hi,
>
> On Fri, Aug 02, 2013 at 03:42:20PM +0000, 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;
> > >
> > > [ ...
RE: About the hibernation feature implementation in dwc3 driver
> Hi,
>
> On Fri, Aug 02, 2013 at 10:54:18AM +0000, 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 wit
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 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
