RE: xHCI ISR be registered twice
> On Thu, Aug 08, 2013 at 03:38:53AM +, 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: xHCI ISR be registered twice
On Thu, Aug 08, 2013 at 03:38:53AM +, 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 will send you an updated patch to test shortly. 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: 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/drivers/usb/host/xhci.c
> >
> > index d8f640b..b43f722 100644
> >
> > --- a/drivers/usb/host/xhci.c
> >
> > +++ b/drivers/usb/host/xhci.c
> >
> > @@ -372,6 +372,10 @@ static int xhci_try_enable_msi(struct usb_hcd
> > *hcd)
> >
> > }
> >
> >
> >
> > legacy_irq:
> >
> > + /* The leacy irq was already registered in USB core */
> >
> > + if (hcd->irq)
> >
> > + return 0;
> >
> > +
>
> Let's take a look at the function you're patching:
>
> static int xhci_try_enable_msi(struct usb_hcd *hcd) {
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> struct pci_d
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.
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.
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-2043a643d048c341b2a52044fd72d852aac87fef
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.
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/drivers/usb/host/xhci.c
>
> index d8f640b..b43f722 100644
>
> --- a/drivers/usb/host/xhci.c
>
> +++ b/drivers/usb/host/xhci.c
>
> @@ -372,6 +372,10 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
>
> }
>
>
>
> legacy_irq:
>
> + /* The leacy irq was already registered in USB core */
>
> + if (hcd->irq)
>
> + return 0;
>
> +
Let's take a look at the function you're patching:
static int xhci_try_enable_msi(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
int ret;
/*
* Some Fresco Logic host controllers advertise MSI, but fail to
* generate interrupts. Don't even try to enable MSI.
*/
if (xhci->quirks & XHCI_BROKEN_MSI)
goto legacy_irq;
/* unregister the legacy interrupt */
if (hcd->irq)
free_irq(hcd->irq, hcd);
hcd->irq = 0;
ret = xhci_setup_msix(xhci);
if (ret)
/* fall back to msi*/
ret = xhci_setup_msi(xhci);
if (!ret)
/* hcd->irq is 0, we have MSI */
return 0;
if (!pdev->irq) {
xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n");
return -EINVAL;
}
legac
