Re: [PATCH 2/2] usb: host: ehci-msm: Use posted data writes on AHB
On Fri, 6 Nov 2015, Georgi Djakov wrote: > On 11/06/2015 08:04 AM, Andy Gross wrote: > > This patch sets the AHBMODE to allow for posted data writes. This > > results in higher performance. > > > > Signed-off-by: Andy Gross > > With these patches I see significant improvement in throughput > on my db410c board. > > Tested-by: Georgi Djakov > > > --- > > drivers/usb/host/ehci-msm.c |4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c > > index c4f84c8..c23e285 100644 > > --- a/drivers/usb/host/ehci-msm.c > > +++ b/drivers/usb/host/ehci-msm.c > > @@ -57,8 +57,8 @@ static int ehci_msm_reset(struct usb_hcd *hcd) > > > > /* bursts of unspecified length. */ > > writel(0, USB_AHBBURST); > > - /* Use the AHB transactor */ > > - writel(0, USB_AHBMODE); > > + /* Use the AHB transactor, allow posted data writes */ > > + writel(0x8, USB_AHBMODE); > > /* Disable streaming mode and select host mode */ > > writel(0x13, USB_USBMODE); Acked-by: Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Revert "usb: host: ehci-msm: Use devm_ioremap_resource instead of devm_ioremap"
On Mon, 27 Apr 2015, Ivan T. Ivanov wrote:
> This reverts commit 70843f623b58 ("usb: host: ehci-msm: Use
> devm_ioremap_resource instead of devm_ioremap") and commit
> e507bf577e5a ("host: ehci-msm: remove duplicate check on resource"),
> because msm_otg and this driver are using same address space to
> access AHB mode and USB command registers.
>
> Cc: Vivek Gautam
> Signed-off-by: Ivan T. Ivanov
> ---
>
> Changes since v0:
>
> * Add note to patch description that also commit e507bf577e5a is reverted.
>
> drivers/usb/host/ehci-msm.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> index 9db74ca..275c92e 100644
> --- a/drivers/usb/host/ehci-msm.c
> +++ b/drivers/usb/host/ehci-msm.c
> @@ -88,13 +88,20 @@ static int ehci_msm_probe(struct platform_device *pdev)
> }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - hcd->regs = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(hcd->regs)) {
> - ret = PTR_ERR(hcd->regs);
> + if (!res) {
> + dev_err(&pdev->dev, "Unable to get memory resource\n");
> + ret = -ENODEV;
> goto put_hcd;
> }
> +
> hcd->rsrc_start = res->start;
> hcd->rsrc_len = resource_size(res);
> + hcd->regs = devm_ioremap(&pdev->dev, hcd->rsrc_start, hcd->rsrc_len);
> + if (!hcd->regs) {
> + dev_err(&pdev->dev, "ioremap failed\n");
> + ret = -ENOMEM;
> + goto put_hcd;
> + }
>
> /*
>* OTG driver takes care of PHY initialization, clock management,
> --
> 1.9.1
Acked-by: Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert "usb: host: ehci-msm: Use devm_ioremap_resource instead of devm_ioremap"
On Tue, 21 Apr 2015, Ivan T. Ivanov wrote:
>
> On Tue, 2015-04-21 at 11:04 -0400, Alan Stern wrote:
> > On Tue, 21 Apr 2015, Ivan T. Ivanov wrote:
> >
> > > This reverts commit 70843f623b58 ("usb: host: ehci-msm: Use
> > > devm_ioremap_resource instead of devm_ioremap"), because msm_otg
> > > and this driver are using same address space to access AHB mode
> > > and USB command registers.
> >
> > Um, this patch is in fact _not_ a reversion of 70843f623b58. That
> > commit removed 4 lines of code and added 3. If this were truly a
> > reversion, it would remove 3 lines and add 4. Instead it adds 10.
> >
> > Please make this a true reversion.
>
> Right, but I will have to revert 2 commits then, is this ok?
Yes, that's okay. I hadn't noticed that e507bf577e5a touched the same
code. Just mention them both in the patch description.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert "usb: host: ehci-msm: Use devm_ioremap_resource instead of devm_ioremap"
On Tue, 21 Apr 2015, Ivan T. Ivanov wrote:
> This reverts commit 70843f623b58 ("usb: host: ehci-msm: Use
> devm_ioremap_resource instead of devm_ioremap"), because msm_otg
> and this driver are using same address space to access AHB mode
> and USB command registers.
Um, this patch is in fact _not_ a reversion of 70843f623b58. That
commit removed 4 lines of code and added 3. If this were truly a
reversion, it would remove 3 lines and add 4. Instead it adds 10.
Please make this a true reversion.
Alan Stern
> Cc: Vivek Gautam
> Signed-off-by: Ivan T. Ivanov
> ---
> drivers/usb/host/ehci-msm.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> index 9db74ca..275c92e 100644
> --- a/drivers/usb/host/ehci-msm.c
> +++ b/drivers/usb/host/ehci-msm.c
> @@ -88,13 +88,20 @@ static int ehci_msm_probe(struct platform_device *pdev)
> }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - hcd->regs = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(hcd->regs)) {
> - ret = PTR_ERR(hcd->regs);
> + if (!res) {
> + dev_err(&pdev->dev, "Unable to get memory resource\n");
> + ret = -ENODEV;
> goto put_hcd;
> }
> +
> hcd->rsrc_start = res->start;
> hcd->rsrc_len = resource_size(res);
> + hcd->regs = devm_ioremap(&pdev->dev, hcd->rsrc_start, hcd->rsrc_len);
> + if (!hcd->regs) {
> + dev_err(&pdev->dev, "ioremap failed\n");
> + ret = -ENOMEM;
> + goto put_hcd;
> + }
>
> /*
>* OTG driver takes care of PHY initialization, clock management,
> --
> 1.9.1
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: ehci-msm: Don't ioremap configuration space exclusively
On Mon, 20 Apr 2015, Ivan T. Ivanov wrote: > > On Mon, 2015-04-20 at 10:14 -0400, Alan Stern wrote: > > On Mon, 20 Apr 2015, Ivan T. Ivanov wrote: > > > > > Hi Alan, > > > > > > Perhaps I have to resend this patch with updated commit > > > message? Are they any other obstacles? > > > > Instead of submitting this new patch, would it be okay to revert commit > > 70843f623b58? That would be simpler. > > Sure, wherever is working better for you. > > > > > Also, I'd like to get an Acked-by from Vivek before accepting this. > > > > Alan Stern > > > > Do you expect something from my side? Just submit a new version of the patch (reverting that commit) and CC: Vivek on the submission. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: ehci-msm: Don't ioremap configuration space exclusively
On Mon, 20 Apr 2015, Ivan T. Ivanov wrote: > Hi Alan, > > Perhaps I have to resend this patch with updated commit > message? Are they any other obstacles? Instead of submitting this new patch, would it be okay to revert commit 70843f623b58? That would be simpler. Also, I'd like to get an Acked-by from Vivek before accepting this. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: ehci-msm: Don't ioremap configuration space exclusively
On Thu, 9 Apr 2015, Ivan T. Ivanov wrote:
> This allow same IO space to be shared between HCD and Device
> controller driver. Which can be loaded simultaneously and
> started/stopped on demand by USB OTG PHY driver.
You really should CC the person who wrote the code you are changing.
This is almost exactly the same as reverting commit 70843f623b58 (usb:
host: ehci-msm: Use devm_ioremap_resource instead of devm_ioremap).
Vivek, what do you think?
Alan Stern
> Signed-off-by: Ivan T. Ivanov
> ---
> drivers/usb/host/ehci-msm.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> index 9db74ca..f059e15 100644
> --- a/drivers/usb/host/ehci-msm.c
> +++ b/drivers/usb/host/ehci-msm.c
> @@ -88,13 +88,17 @@ static int ehci_msm_probe(struct platform_device *pdev)
> }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - hcd->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (!res)
> + return -ENODEV;
> +
> + hcd->rsrc_start = res->start;
> + hcd->rsrc_len = resource_size(res);
> +
> + hcd->regs = devm_ioremap(&pdev->dev, hcd->rsrc_start, hcd->rsrc_len);
> if (IS_ERR(hcd->regs)) {
> ret = PTR_ERR(hcd->regs);
> goto put_hcd;
> }
> - hcd->rsrc_start = res->start;
> - hcd->rsrc_len = resource_size(res);
>
> /*
>* OTG driver takes care of PHY initialization, clock management,
> --
> 1.9.1
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] usb: Kconfig: make EHCI_MSM selectable for QCOM SOCs
On Mon, 30 Jun 2014, Felipe Balbi wrote: > On Mon, Jun 30, 2014 at 06:29:34PM +0100, Srinivas Kandagatla wrote: > > This patch makes the msm ehci driver available to use on QCOM SOCs, > > which have the same IP. > > > > Signed-off-by: Srinivas Kandagatla > > Alan, this is ok to me: > > Acked-by: Felipe Balbi Greg, it's okay with me too. Acked-by: Alan Stern > > --- > > drivers/usb/host/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > > index 61b7817..03314f8 100644 > > --- a/drivers/usb/host/Kconfig > > +++ b/drivers/usb/host/Kconfig > > @@ -176,7 +176,7 @@ config USB_EHCI_HCD_AT91 > > > > config USB_EHCI_MSM > > tristate "Support for Qualcomm QSD/MSM on-chip EHCI USB controller" > > - depends on ARCH_MSM > > + depends on ARCH_MSM || ARCH_QCOM > > select USB_EHCI_ROOT_HUB_TT > > ---help--- > > Enables support for the USB Host controller present on the > > -- > > 1.9.1 > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
On Tue, 13 Aug 2013, Jack Pham wrote: > > > In addition, we may want some of the work (though at this point I'm > > > not still clear on exactly what parts) to be moved into usbcore. > > +1. I am open to suggestions on how best to achieve this. I have a > another patch in the for doing the same 15-second delay in xHCI but am > hesitant to submit it just yet, pending ideas on a better way to do it > in the common HCD core. Well, here's one possibility, not fully thought out. Suppose there was a way to tell the HCD to carry out _just_ the Setup stage of a control transfer. Or to omit the Setup stage and go directly to the Data stage. Then the SINGLE_STEP_GET_DEVICE_DESCRIPTOR test could be implemented very simply by submitting two URBs with a delay between. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
On Tue, 13 Aug 2013, Felipe Balbi wrote:
> > That's not what I meant. Yes, the test-device driver knows what test
> > it wants to run, based on the VID/PID. I was asking how it would
> > communicate this knowledge to the HCD.
> >
> > For example, it doesn't make sense to have a callback that means
> > "Insert a 15-second delay into the next URB that I submit", because the
> > HCD doesn't know where URBs come from.
>
> static int ehci_test_mode(struct usb_hcd *hcd, unsigned int test)
> {
> struct ehci_hcd *ehci = to_ehci(hcd);
>
>
>
> switch (test) {
> case USB_TEST_SINGLE_STEP_GET_DESC:
> start_test();
> wait_15_seconds();
> finish_test();
> break;
> ...
>
> default:
> return -ENOTSUP;
> }
>
> return ret;
> }
>
> ...
>
> static struct hc_driver ehci_hcd_driver = {
>
>
> .test_mode = ehci_test_mode,
>
> ...
> };
This is almost exactly the same as the way it is done in the newly
merged code. The only difference is that it uses a special test_mode
callback instead of a special selector value.
Didn't you say you wanted a large part of the support moved into
usbcore? For example, the routines that create and enqueue this "URB
with a delay in the middle" are little more than copies of the core
code.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
On Mon, 12 Aug 2013, Felipe Balbi wrote: > > > maybe a single callback for supporting 'testmodes' ? which receives the > > > test mode as argument ? > > > > I don't have a clear picture of how you would apply such an approach to > > this case. There would have to be a way to tell the HCD to insert a > > 15-second delay between the Setup and Data stages of a particular > > control URB. How would you do that? Whatever method you choose, > > each test is started after enumerating a known Vid/Pid pair. Based on > that, you *know* which test to run. That's not what I meant. Yes, the test-device driver knows what test it wants to run, based on the VID/PID. I was asking how it would communicate this knowledge to the HCD. For example, it doesn't make sense to have a callback that means "Insert a 15-second delay into the next URB that I submit", because the HCD doesn't know where URBs come from. > > What other test modes would you want to support? > > anything that USB[23]0CV supports today. There are even link layer tests > for USB3 and a bunch of others. This SINGLE_STEP_SET_FEATURE is but one > of a large(-ish) test suite which needs to be supported. That's what I'm trying to find out. What are the special features that we would need to implement in order to support the entire test suite? > > Is it worth adding this support to the standard host controller > > drivers, or should there be a special version (a Kconfig option like > > CONFIG_RCU_TORTURE_TEST) to enable it? Putting a lot of testing code > > in distribution kernels where it will never be used seems like a big > > waste. > > right, I think it should be hidden by Kconfig, not arguing against that. Therefore we both agree the $SUBJECT patch should not be accepted in its current form. At the very least, the new code in ehci-hcd should be conditional on a CONFIG_USB_HCD_TEST_MODE option. In addition, we may want some of the work (though at this point I'm not still clear on exactly what parts) to be moved into usbcore. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
On Fri, 9 Aug 2013, Felipe Balbi wrote: > > > heh, it doesn't need to be entirely in the core. Core could have the > > > generic calls and HCDs could implement some callbacks, but I think quite > > > a bit of the code will be similar if we implement the same thing on all > > > HCDs. > > > > What generic calls and callbacks would you suggest? I assume you want > > enough to cover not just this one test but the entire USB-CV suite. > > maybe a single callback for supporting 'testmodes' ? which receives the > test mode as argument ? I don't have a clear picture of how you would apply such an approach to this case. There would have to be a way to tell the HCD to insert a 15-second delay between the Setup and Data stages of a particular control URB. How would you do that? Whatever method you choose, implementing it in every HCD would be a huge amount of work. What other test modes would you want to support? Is it worth adding this support to the standard host controller drivers, or should there be a special version (a Kconfig option like CONFIG_RCU_TORTURE_TEST) to enable it? Putting a lot of testing code in distribution kernels where it will never be used seems like a big waste. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
On Fri, 9 Aug 2013, Felipe Balbi wrote: > > > Wait a minute, didn't we discuss a while back that these test features > > > should be built into usbcore so that we could have a usbcv clone for > > > linux ? > > > > There's no way this can be built into the core. This test requires the > > behavior of the host controller to be modified at a low level > > (introducing a 15-second delay between the Setup and Data stages of a > > control transfer). Only the HCD can do that. > > heh, it doesn't need to be entirely in the core. Core could have the > generic calls and HCDs could implement some callbacks, but I think quite > a bit of the code will be similar if we implement the same thing on all > HCDs. What generic calls and callbacks would you suggest? I assume you want enough to cover not just this one test but the entire USB-CV suite. > > Besides, doesn't USB-CV install its own version of Windows' EHCI driver > > while it runs its tests? > > IIRC only USB20CV, the new USB30CV (which also tests USB2) doesn't do > that. I don't have more than the foggiest notion of how these things really work under Windows. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
On Thu, 25 Jul 2013, Felipe Balbi wrote: > On Thu, Jul 25, 2013 at 03:44:20PM -0400, Alan Stern wrote: > > On Thu, 25 Jul 2013, Greg KH wrote: > > > > > On Tue, Jul 02, 2013 at 08:13:52PM -0700, Jack Pham wrote: > > > > From: Manu Gautam > > > > > > > > The USB Embedded High-speed Host Electrical Test (EHSET) defines the > > > > SINGLE_STEP_SET_FEATURE test as follows: > > > > > > > > 1) The host enumerates the test device with VID:0x1A0A, PID:0x0108 > > > > 2) The host sends the SETUP stage of a GetDescriptor(Device) > > > > 3) The device ACKs the request > > > > 4) The host issues SOFs for 15 seconds allowing the test operator to > > > >raise the scope trigger just above the SOF voltage level > > > > 5) The host sends the IN packet > > > > 6) The device sends data in response, triggering the scope > > > > 7) The host sends an ACK in response to the data > > > > > > > > This patch adds additional handling to the EHCI hub driver and allows > > > > the EHSET driver to initiate this test mode by issuing a a SetFeature > > > > request to the root hub with a Test Selector value of 0x06. From there > > > > it mimics ehci_urb_enqueue() but separately submits QTDs for the > > > > SETUP and DATA/STATUS stages in order to insert a delay in between. > > > > > > > > Signed-off-by: Manu Gautam > > > > Signed-off-by: Jack Pham > > > > > > Alan, any thoughts about this patch? > > > > Sorry, this slipped my mind. > > > > It looks okay. I haven't tested it yet (and it's so specialized that > > it probably will never receive very much testing). It is somewhat > > fragile, in that it copies part of usbcore into ehci-hcd; updates to > > the core will have to be mirrored in the driver. > > > > On the other hand, there's no real reason to reject it, and it could > > end up helping people who want to test new USB devices. So... > > > > Acked-by: Alan Stern > > Wait a minute, didn't we discuss a while back that these test features > should be built into usbcore so that we could have a usbcv clone for > linux ? There's no way this can be built into the core. This test requires the behavior of the host controller to be modified at a low level (introducing a 15-second delay between the Setup and Data stages of a control transfer). Only the HCD can do that. Besides, doesn't USB-CV install its own version of Windows' EHCI driver while it runs its tests? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
On Thu, 25 Jul 2013, Greg KH wrote: > On Tue, Jul 02, 2013 at 08:13:52PM -0700, Jack Pham wrote: > > From: Manu Gautam > > > > The USB Embedded High-speed Host Electrical Test (EHSET) defines the > > SINGLE_STEP_SET_FEATURE test as follows: > > > > 1) The host enumerates the test device with VID:0x1A0A, PID:0x0108 > > 2) The host sends the SETUP stage of a GetDescriptor(Device) > > 3) The device ACKs the request > > 4) The host issues SOFs for 15 seconds allowing the test operator to > >raise the scope trigger just above the SOF voltage level > > 5) The host sends the IN packet > > 6) The device sends data in response, triggering the scope > > 7) The host sends an ACK in response to the data > > > > This patch adds additional handling to the EHCI hub driver and allows > > the EHSET driver to initiate this test mode by issuing a a SetFeature > > request to the root hub with a Test Selector value of 0x06. From there > > it mimics ehci_urb_enqueue() but separately submits QTDs for the > > SETUP and DATA/STATUS stages in order to insert a delay in between. > > > > Signed-off-by: Manu Gautam > > Signed-off-by: Jack Pham > > Alan, any thoughts about this patch? Sorry, this slipped my mind. It looks okay. I haven't tested it yet (and it's so specialized that it probably will never receive very much testing). It is somewhat fragile, in that it copies part of usbcore into ehci-hcd; updates to the core will have to be mirrored in the driver. On the other hand, there's no real reason to reject it, and it could end up helping people who want to test new USB devices. So... Acked-by: Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] EHCI: Update qTD next pointer in QH overlay region during unlink
On Fri, 7 Sep 2012, Alan Stern wrote:
> On Fri, 7 Sep 2012, Pavankumar Kondeti wrote:
>
> > There is a possibility of QH overlay region having reference to a stale
> > qTD pointer during unlink.
> >
> > Consider an endpoint having two pending qTD before unlink process begins.
> > The endpoint's QH queue looks like this.
> >
> > qTD1 --> qTD2 --> Dummy
> >
> > To unlink qTD2, QH is removed from asynchronous list and Asynchronous
> > Advance Doorbell is programmed. The qTD1's next qTD pointer is set to
> > qTD2'2 next qTD pointer and qTD2 is retired upon controller's doorbell
> > interrupt. If QH's current qTD pointer points to qTD1, transfer overlay
> > region still have reference to qTD2. But qtD2 is just unlinked and freed.
> > This may cause EHCI system error. Fix this by updating qTD next pointer
> > in QH overlay region with the qTD next pointer of the current qTD.
> >
> > Signed-off-by: Pavankumar Kondeti
> > ---
> > drivers/usb/host/ehci-q.c | 12 ++--
> > 1 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> > index 9bc39ca..4b66374 100644
> > --- a/drivers/usb/host/ehci-q.c
> > +++ b/drivers/usb/host/ehci-q.c
> > @@ -128,9 +128,17 @@ qh_refresh (struct ehci_hcd *ehci, struct ehci_qh *qh)
> > else {
> > qtd = list_entry (qh->qtd_list.next,
> > struct ehci_qtd, qtd_list);
> > - /* first qtd may already be partially processed */
> > - if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current)
> > + /*
> > +* first qtd may already be partially processed.
> > +* If we come here during unlink, the QH overlay region
> > +* might have reference to the just unlinked qtd. The
> > +* qtd is updated in qh_completions(). Update the QH
> > +* overlay here.
> > +*/
> > + if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current) {
> > + qh->hw->hw_qtd_next = qtd->hw_next;
> > qtd = NULL;
> > + }
> > }
> >
> > if (qtd)
>
> Acked-by: Alan Stern
I forgot to mention: This patch should be included in the next 3.6-rc
release and marked for -stable.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] EHCI: Update qTD next pointer in QH overlay region during unlink
On Fri, 7 Sep 2012, Pavankumar Kondeti wrote:
> There is a possibility of QH overlay region having reference to a stale
> qTD pointer during unlink.
>
> Consider an endpoint having two pending qTD before unlink process begins.
> The endpoint's QH queue looks like this.
>
> qTD1 --> qTD2 --> Dummy
>
> To unlink qTD2, QH is removed from asynchronous list and Asynchronous
> Advance Doorbell is programmed. The qTD1's next qTD pointer is set to
> qTD2'2 next qTD pointer and qTD2 is retired upon controller's doorbell
> interrupt. If QH's current qTD pointer points to qTD1, transfer overlay
> region still have reference to qTD2. But qtD2 is just unlinked and freed.
> This may cause EHCI system error. Fix this by updating qTD next pointer
> in QH overlay region with the qTD next pointer of the current qTD.
>
> Signed-off-by: Pavankumar Kondeti
> ---
> drivers/usb/host/ehci-q.c | 12 ++--
> 1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 9bc39ca..4b66374 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -128,9 +128,17 @@ qh_refresh (struct ehci_hcd *ehci, struct ehci_qh *qh)
> else {
> qtd = list_entry (qh->qtd_list.next,
> struct ehci_qtd, qtd_list);
> - /* first qtd may already be partially processed */
> - if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current)
> + /*
> + * first qtd may already be partially processed.
> + * If we come here during unlink, the QH overlay region
> + * might have reference to the just unlinked qtd. The
> + * qtd is updated in qh_completions(). Update the QH
> + * overlay here.
> + */
> + if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current) {
> + qh->hw->hw_qtd_next = qtd->hw_next;
> qtd = NULL;
> + }
> }
>
> if (qtd)
Acked-by: Alan Stern
Have you been able to determine that this eliminates your host system
errors?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: question on memory barriers
On Tue, 26 Jul 2011, Oliver Neukum wrote: > Hi, > > this code made me think: > > /* Give this link TRB to the hardware */ > wmb(); > next->link.control ^= cpu_to_le32(TRB_CYCLE); > > Can you do this or may there be a CPU that speculatively writes before > it reads next->link.control ? CPUs never do speculative writes. And even if they did, the wmb() would prevent the speculative write from preceding any statement that comes before the memory barrier. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] usb: Add support for streams alloc/dealloc to devio.c
On Fri, 17 Jun 2011 ab...@codeaurora.org wrote: > My name is Amit Blay, I'm working with Tatyana Brokhman, I originally > created this patch. I want to make sure I understand the solution: > > We will create a new IOCTL, i.e., USBDEVFS_SUBMITURB_SS. This IOCTL will > pass new URB structure, usbdevfs_urb_ss, which will hold the "legacy" URB > and in addition the stream_id. libusb will be able to use the new ABI > whenever SuperSpeed streams are required. > > Is this correct? This probably should be USBDEVFS_SUBMITURB_STREAM, since it is necesary only for URBs that use bulk streaming rather than for all SuperSpeed URBs. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/5] usb:dummy_hcd: connect/disconnect test support
On Thu, 16 Jun 2011, Alan Stern wrote: > On Thu, 16 Jun 2011, Felipe Balbi wrote: > > > Hi, > > > > On Thu, Jun 16, 2011 at 11:06:47AM -0400, Alan Stern wrote: > > > On Thu, 16 Jun 2011, Tatyana Brokhman wrote: > > > > > > > This implementation adds a new proprietary device control requests (to > > > > be > > > > handled by the dummy_hcd) that initiates a connect/disconnect sequence. > > > > The bRequest value of the new control request is 0x52. > > > > It is used by the user-space Unit testing application. > > > > > > This is not a bad idea. On the other hand, it is slightly peculiar -- > > > it you're testing with real UDC hardware, you would do the > > > disconnect/reconnect sequence by hand (unplug and replug the USB > > > cable), not in software. > > > > actually, this is quite useful. Specially for the controllers which > > _can_ do soft-connect by toggling data pullups. I would rather have > > these sort of thing maybe in composite.c, so that we can build tests > > with all gadget drivers/controllers, not only dummy_hcd. > > It sounds like you don't understand the point of this patch. It allows > the _host_ to control the pullup settings. Sorry, the misunderstanding was mine -- I didn't grasp what you were suggesting. Yes, something like this could be useful in composite.c. It could be defined as a vendor-specific request with the device as the recipient. One problem with making it special for dummy-hcd is that it would prevent people from testing gadget drivers that want to use the same bRequest value to mean something different. Putting it in composite.c would force all function drivers to know about it. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/5] usb:dummy_hcd: connect/disconnect test support
On Thu, 16 Jun 2011, Felipe Balbi wrote: > Hi, > > On Thu, Jun 16, 2011 at 11:06:47AM -0400, Alan Stern wrote: > > On Thu, 16 Jun 2011, Tatyana Brokhman wrote: > > > > > This implementation adds a new proprietary device control requests (to be > > > handled by the dummy_hcd) that initiates a connect/disconnect sequence. > > > The bRequest value of the new control request is 0x52. > > > It is used by the user-space Unit testing application. > > > > This is not a bad idea. On the other hand, it is slightly peculiar -- > > it you're testing with real UDC hardware, you would do the > > disconnect/reconnect sequence by hand (unplug and replug the USB > > cable), not in software. > > actually, this is quite useful. Specially for the controllers which > _can_ do soft-connect by toggling data pullups. I would rather have > these sort of thing maybe in composite.c, so that we can build tests > with all gadget drivers/controllers, not only dummy_hcd. It sounds like you don't understand the point of this patch. It allows the _host_ to control the pullup settings. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 5/5] usb: Add support for streams alloc/dealloc to devio.c
On Thu, 16 Jun 2011, Tatyana Brokhman wrote:
> Allow user space applications such as LIBUSB, to request
> streams alloc/dealloc from HCD that implements XHCI.
>
> Signed-off-by: Amit Blay
> Signed-off-by: Tatyana Brokhman
...
> diff --git a/include/linux/usbdevice_fs.h b/include/linux/usbdevice_fs.h
> index 15591d2..133c216 100644
> --- a/include/linux/usbdevice_fs.h
> +++ b/include/linux/usbdevice_fs.h
> @@ -108,6 +108,7 @@ struct usbdevfs_urb {
> or 0 if none should be sent. */
> void __user *usercontext;
> struct usbdevfs_iso_packet_desc iso_frame_desc[0];
> + unsigned int stream_id;
> };
>
> /* ioctls for talking directly to drivers */
> @@ -165,6 +166,7 @@ struct usbdevfs_urb32 {
> compat_uint_t signr;
> compat_caddr_t usercontext; /* unused */
> struct usbdevfs_iso_packet_desc iso_frame_desc[0];
> + compat_uint_t stream_id;
> };
It would be nice if we could do this, but we can't. We are not allowed
to change an established ABI. Every copy of libusb would have to be
rebuilt.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/5] usb:dummy_hcd: connect/disconnect test support
On Thu, 16 Jun 2011, Tatyana Brokhman wrote: > This implementation adds a new proprietary device control requests (to be > handled by the dummy_hcd) that initiates a connect/disconnect sequence. > The bRequest value of the new control request is 0x52. > It is used by the user-space Unit testing application. > > Signed-off-by: Tatyana Linder > @@ -1534,6 +1572,10 @@ static int handle_control_request(struct dummy_hcd > *dum_hcd, struct urb *urb, > *status = 0; > } > break; > + case 0x52: /* UT: Connect/disconnect the device */ > + schedule_work(&dum_hcd->conn_disc_work); > + ret_val = 0; > + break; > } > return ret_val; > } I forgot to mention... You need to test setup->bRequestType. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 4/5] usb:dummy_hcd: Disable single-request fifo in dummy hcd
On Thu, 16 Jun 2011, Tatyana Brokhman wrote: > This is not the actual behavior of the udc, thus it's removed. I don't understand. What UDC are you talking about? This _is_ the actual behavior of the net2280 (depending on which endpoint you're looking at and how the device is configured). > This patch is needed for several of UAS test to pass. Then the tests are broken. NAK. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/5] usb:dummy_hcd: connect/disconnect test support
On Thu, 16 Jun 2011, Tatyana Brokhman wrote:
> This implementation adds a new proprietary device control requests (to be
> handled by the dummy_hcd) that initiates a connect/disconnect sequence.
> The bRequest value of the new control request is 0x52.
> It is used by the user-space Unit testing application.
This is not a bad idea. On the other hand, it is slightly peculiar --
it you're testing with real UDC hardware, you would do the
disconnect/reconnect sequence by hand (unplug and replug the USB
cable), not in software.
> Signed-off-by: Tatyana Linder
>
> ---
> drivers/usb/gadget/dummy_hcd.c | 48
>
> 1 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index 74b7655..7d0a6fe 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -179,6 +179,7 @@ struct dummy_hcd {
> unsignedactive:1;
> unsignedold_active:1;
> unsignedresuming:1;
> + struct work_struct conn_disc_work;
> };
>
> struct dummy {
> @@ -1362,6 +1363,43 @@ static struct dummy_ep *find_endpoint (struct dummy
> *dum, u8 address)
> #define Ep_Request (USB_TYPE_STANDARD | USB_RECIP_ENDPOINT)
> #define Ep_InRequest (Ep_Request | USB_DIR_IN)
>
> +/**
> + * dummy_hcd_conn_disc_work() - performs a disconnect/connect sequence.
> + * @data: pointer to the sceduled work_struct
Spelling error.
> + */
> +static void dummy_hcd_conn_disc_work(struct work_struct *data)
> +{
> + struct dummy_hcd *dum_hcd =
> + container_of(data, struct dummy_hcd, conn_disc_work);
> + int ret_val;
> +
> + if (!dum_hcd->dum->driver) {
> + dev_err(dummy_dev(dum_hcd),
> + "dummy_hcd_conn_disc_work called without connected "
> + "device\n");
> + return;
> + }
> +
> + dev_info(dummy_dev(dum_hcd), "disconnecting device...\n");
> + ret_val = dummy_pullup(&dum_hcd->dum->gadget, 0);
> + if (ret_val) {
> + dev_err(dummy_dev(dum_hcd), "dummy_hcd_conn_disc_work:"
> + " couldn't disconnect device:"
> + " ret_val=%d\n",
> + ret_val);
> + return;
> + }
> +
> + dev_info(dummy_dev(dum_hcd), "re-connecting device...\n");
> + /* We have to let the hub task to update the device disconnect state */
> + msleep_interruptible(1000);
These two lines should come before the "re-connecting" message.
> + ret_val = dummy_pullup(&dum_hcd->dum->gadget, 1);
> + if (ret_val)
> + dev_err(dummy_dev(dum_hcd), "dummy_hcd_conn_disc_work:"
> + " couldn't re-connect device:"
> + " ret_val=%d\n",
> + ret_val);
> +}
>
> /**
> * handle_control_request() - handles all control transfers
> @@ -1534,6 +1572,10 @@ static int handle_control_request(struct dummy_hcd
> *dum_hcd, struct urb *urb,
> *status = 0;
> }
> break;
> + case 0x52: /* UT: Connect/disconnect the device */
Please define a symbolic constant for this. Also, nobody is going to
know what "UT:" is supposed to mean.
The rest is okay.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] usb:gadget: use min_t() macro instead of min()
On Mon, 13 Jun 2011, Tanya Brokhman wrote: > > > > The change I suggested involved replacing two typecasts with a single > > min_t. All (or almost all) the places this patch touches currently > > contain only one typecast, so the motivation for changing them is a lot > > weaker. > > > You're right. So what's the final call on this one? Do you think it can be > merged or you prefer not change anything? I personally think the code looks > nicer using min_t instead of min with casting but that's just my opinion and > of course there are arguments against this patch. I don't care either way. It's up to you and Felipe. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] usb:gadget: use min_t() macro instead of min()
On Mon, 13 Jun 2011, Tanya Brokhman wrote: > > > > On Sun, Jun 12, 2011 at 02:14:46PM +0300, Tatyana Brokhman wrote: > > > Signed-off-by: Tatyana Brokhman > > > > I need a sensible commit log for this. Why do we need to change all > > min() to min_t() ? > > > > Actually, Alan asked me to make this change in one place in dummy_hcd. I > wasn't aware of the min_t macro before that. So when I searched the code for > other places I found quite a few and just thought that it would be "nicer" > to use min_t() instead of min() with casting. > So we don't "need" to make this change. Everything works as is. This patch > only makes the code look nicer, nothing more. > I can elaborate the above in the commit log if you want. The change I suggested involved replacing two typecasts with a single min_t. All (or almost all) the places this patch touches currently contain only one typecast, so the motivation for changing them is a lot weaker. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb:dummy_hcd: Force FS device connection according to module parameter
On Fri, 10 Jun 2011, Tatyana Brokhman wrote: > This patch adds a new module parameter to dummy_hcd: is_high_speed > When set to false the connected device will be forced to operate in FS > mode. The default of this parameter is true. > > Signed-off-by: Tatyana Brokhman Acked-by: Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] usb:dummy_hcd: Force FS device connection according to module parameter
On Thu, 9 Jun 2011, Tanya Brokhman wrote: > > > + if (!mod_data.is_high_speed && mod_data.is_super_speed) > > > + return -EINVAL; > > > > Print an error message in the log so that the user will know why the > > failure occurred. > > > > But when the module fails to load the message sais that it's invalid > parameter (or something like that). That's why I thought it will be enough. Oh yes, that's true. > You mean to add something that explains WHY these values are wrong? That's what I had in mind. But "invalid parameter" is probably good enough, since these are the _only_ parameters. :-) So this part is okay as it stands. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb:dummy_hcd: Force FS device connection according to module parameter
On Thu, 9 Jun 2011, Tatyana Brokhman wrote:
> This patch adds a new module parameter to dummy_hcd: is_high_speed
> When set to false the connected device will be forced to operate in FS
> mode. The default of this parameter is true.
...
> @@ -904,12 +908,15 @@ usb_gadget_probe_driver(struct usb_gadget_driver
> *driver,
> dum->gadget.ep0 = &dum->ep [0].ep;
> if (mod_data.is_super_speed)
> dum->gadget.speed = driver->speed;
> - else
> + else if (mod_data.is_high_speed)
> dum->gadget.speed = min((u8)USB_SPEED_HIGH, (u8)driver->speed);
Use min_t().
> + else
> + dum->gadget.speed = USB_SPEED_FULL;
> if (dum->gadget.speed < driver->speed)
> - dev_dbg(udc_dev(dum), "This device can perform faster if"
> - " you connect it to a "
> - "SupeSpeed port...\n");
> + dev_dbg(udc_dev(dum), "This device can perform faster"
> + " if you connect it to a %s port...\n",
> + (driver->speed == USB_SPEED_SUPER ?
> + "SuperSpeed" : "HighSpeed"));
>
> if (dum->gadget.speed == USB_SPEED_SUPER) {
> for (i = 0; i < DUMMY_ENDPOINTS; i++)
> @@ -2417,6 +2424,9 @@ static int __init init (void)
> if (usb_disabled ())
> return -ENODEV;
>
> + if (!mod_data.is_high_speed && mod_data.is_super_speed)
> + return -EINVAL;
Print an error message in the log so that the user will know why the
failure occurred.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v15 10/10] usb:dummy_hcd: Force FS device connection according to module parameter
On Tue, 7 Jun 2011, Tanya Brokhman wrote: > > > I thought about that as well. Even added it but removed at the last > > minute > > > :) I encountered quite a few places in the code where some error > > message to > > > the user is really needed but is missing > > > > What other places? There is very little the user has to do with > > dummy-hcd -- really nothing more than setting the module parameters. > > I'm not talking about dummy_hcd. It was a general comment about the gadget > code. I can't give you an example from the top of my head... Oh. Well, if you're writing new code and you come across a place where an error message really is needed, you should add the error message. > > IMO the driver should print an error message and fail to load if there > > are contradictory module parameters. > > > > Well, I can add the error message but I think that as far as dummy_hcd is > concerned failing it's loading is a bit harsh, isn't it? I mean, this is a > type of user error that can be fixed so why not fix it and just notify the > user of what was done? But if the parameters are contradictory, you don't know _how_ to fix it. If is_super_speed is True and is_high_speed is False, did the user want to add SuperSpeed support or to remove high-speed support? Besides, the best (i.e., easiest and most likely to be correct) way of fixing the error is for the user to issue the modprobe command again, with the correct parameters this time. Therefore the driver should fail to load, so that the modprobe can be reissued with no need to do rmmod first. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v15 10/10] usb:dummy_hcd: Force FS device connection according to module parameter
On Tue, 7 Jun 2011, Tanya Brokhman wrote: > > You might want to print out some kind of error message if the user > > specifies is_super_speed = True and is_high_speed = False. > > > > I thought about that as well. Even added it but removed at the last minute > :) I encountered quite a few places in the code where some error message to > the user is really needed but is missing What other places? There is very little the user has to do with dummy-hcd -- really nothing more than setting the module parameters. > so my impression was that it's best > to keep the printks to a minimum. Isn't that the general approach? IMO, in > this particular case informing the user isn't a "must". If you feel strongly > about it I can add the message. IMO the driver should print an error message and fail to load if there are contradictory module parameters. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v15 10/10] usb:dummy_hcd: Force FS device connection according to module parameter
On Mon, 6 Jun 2011, Tatyana Brokhman wrote: > This patch adds a new module parameter to dummy_hcd: is_high_speed > When set to false the connected device will be forced to operate in FS > mode. The default of this parameter is true. You might want to print out some kind of error message if the user specifies is_super_speed = True and is_high_speed = False. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 4/9] usb: Add max_speed to usb_composite_driver structure
On Mon, 6 Jun 2011, Felipe Balbi wrote: > But we might still want to force HighSpeed even though the HW has > SuperSpeed support. That would be a useful module parameter for a SuperSpeed UDC driver. Like the parameter Tanya added to dummy-hcd. > The real solution, for the long run would be to > always start with pullups disabled (iow, don't connect to host > immediately) and only connect after the gadget driver is all > initialized, then we will know the gadget speed before connecting, and > we can have the gadget driver "request" for a particular speed from the > controller. Unfortunately this would mean changing a bunch of UDC drivers. But I agree, it makes sense for the gadget driver to specifically ask for the pullups to be enabled when it is ready. Don't some of the existing UDC drivers fail to implement the pullup method at all? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 9/9] usb: Adding SuperSpeed support to dummy_hcd
On Tue, 31 May 2011, Brokhman Tatyana wrote: > Hi Alan, > > > If is_super_speed is false, shouldn't you avoid registering the > > SuperSpeed hub? Or would that add too many complications? > > It woun't add too many complications but I would prefere not to do that. > My goal was to simulate as much as posible "real" USB connection so the > is_super_speed flag simulates the USB port you connect to, HS/SS. The terms "HS port" and "SS port" don't really mean anything. A physical port connected to a USB-3 controller will run at either HS or SS depending on the cable and device plugged into it. > When > working with a real host if you connect a HS device to a SS port it > connects to a HS root hub but the SS root hub is functional and > registered. The correct way to express this is: If you connect a HS device to a USB-3 controller then it connects to the HS root hub, but the SS root hub is functional and present. That's true, but it misses the point of my question. Does the "is_super_speed" module parameter refer to dummy-hcd's emulated _host_ controller or to the emulated _device_ controller? (Or does it perhaps refer to both?) If it refers to the emulated host controller then there should be no SS root hub. But if it refers only to the emulated device controller then there should be. Let's put it another way. If the "is_super_speed" module parameter is set to False then we know that the emulated connection will never run at SS. Therefore there's no point in registering a SS root hub. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 9/9] usb: Adding SuperSpeed support to dummy_hcd
On Tue, 31 May 2011, Tatyana Brokhman wrote: > This patch adds SS support to the dummy hcd module. It may be used to test > SS device when no (SS) HW is available. > USB 3.0 hub includes 2 hubs - HS and SS ones. > This patch adds support for a SS hub in the dummy_hcd module. Thus, when > dummy_hcd enabled it will register 2 root hubs (SS and HS). > A new module parameter was added to simulate a SuperSpeed connection. > When a new device is connected it will enumerate via the HS root hub by > default. In order to simulate SuperSpeed connection set the is_super_speed > module > parameter to true. If is_super_speed is false, shouldn't you avoid registering the SuperSpeed hub? Or would that add too many complications? > Signed-off-by: Tatyana Brokhman > > --- > drivers/usb/gadget/Kconfig |1 + > drivers/usb/gadget/dummy_hcd.c | 556 > +--- > 2 files changed, 469 insertions(+), 88 deletions(-) > > diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig > index 5927541..d5f0af7 100644 > --- a/drivers/usb/gadget/Kconfig > +++ b/drivers/usb/gadget/Kconfig > @@ -626,6 +626,7 @@ config USB_DUMMY_HCD > depends on USB_GADGET_DUMMY_HCD > default USB_GADGET > select USB_GADGET_SELECTED > + select USB_GADGET_SUPERSPEED This belongs under USB_GADGET_DUMMY_HCD, next to the "select USB_GADGET_DUALSPEED" line. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Sat, 28 May 2011, Tanya Brokhman wrote: > I was making the same change for SS and I think that if you're about to send > this patch to Greg, it might need a small addition: > At the moment dummy_hcd supports high_speed so I think if this option is no > longer configurable by user you should set this flag when dummy_hcd is > selected: > > config USB_DUMMY_HCD > tristate > depends on USB_GADGET_DUMMY_HCD > default USB_GADGET > select USB_GADGET_SELECTED > select USB_GADGET_DUALSPEED Right now USB_GADGET_DUALSPEED is selected under USB_GADGET_DUMMY_HCD, which is the right place for it, I think. Same as all the other UDC drivers. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Thu, 26 May 2011, Tanya Brokhman wrote: > > > Right, but at this point there is no way of knowing what function > > drivers > > > will bind to it and what descriptors they will provide. Most of the > > > function drivers allocate their descriptors at bind() that occurs > > after > > > the UDC already registers. > > > > Well, there must be an appropriate spot to do this. > > Unfortunately in the current implementation there isn't. Sure there is. You describe a good possibility later on in your email... > > It looks like you need to add a "max_speed" field to struct > > usb_composite_driver. Each driver will initialize this to the highest > > speed it supports, and it must guarantee that the necessary descriptors > > are available. > > I also thought of that. This can be done... > Just to make sure we're on the same page: > We can add a "max_sup_speed" field to struct usb_composite_driver. Each of > the Gadget drivers will set this field prior to calling > usb_composite_probe(). Then, in usb_composite_probe(), we'll update the > composite_driver.speed as follows: > > composite_driver.speed = min(composite_driver.speed, > driver->max_sup_speed); Just call it max_speed. You can mention in the kerneldoc that it is the maximum speed supported by the driver. > Of course for SS we'll update the composite_driver.speed @ static struct > usb_gadget_driver composite_driver, as we agreed with Felipe. > > How does that sound? Felipe - it seems to me that these should cover your > hesitations about updating the driver speed :) > > Now that I think about it, the above will be true for HS as well. I mean the > current speed of composite_driver is set always to HS but if there is a > function driver that supports only FS then the above will update > composite_driver.speed to FS. That's right. The same solution will work for both cases. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Wed, 25 May 2011, Brokhman Tatyana wrote: > > composite_driver.speed to USB_SPEED_SUPER. > >> > >> Not sure how to verify this. I need to know whether the driver that is > >> registered with the UDC is SS or not. This is before the function > >> drivers > >> are binded to it. So how can I verify at that point that the function > >> drivers that will bind to this driver will provide SS descriptors? > >> (I'm sorry, I don't have the ability to view the code at the moment and > >> due to the time differences between us I don't want to leave this > >> question > >> for tomorrow and loose another day...) > > > > I'm not sure about this either. I have never used the composite > > framework so I'm not familiar with its details. This has to be decided > > before the composite gadget driver registers with the UDC driver ... > Right, but at this point there is no way of knowing what function drivers > will bind to it and what descriptors they will provide. Most of the > function drivers allocate their descriptors at bind() that occurs after > the UDC already registers. Well, there must be an appropriate spot to do this. It looks like you need to add a "max_speed" field to struct usb_composite_driver. Each driver will initialize this to the highest speed it supports, and it must guarantee that the necessary descriptors are available. Since these structures are passed to usb_composite_probe() before the gadget driver is registered, the necessary calculations can be done there. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Wed, 25 May 2011, Brokhman Tatyana wrote: > Hi Alan, > > > I have looked this over more carefully. It turns out that both of you > > have misunderstood the purpose of CONFIG_USB_GADGET_DUALSPEED (and by > > extension, CONFIG_USB_GADGET_SUPERSPEED). In fact, the existing > > Kconfig file is also wrong. > > > > The _only_ reason for CONFIG_USB_GADGET_DUALSPEED is so that gadget > > drivers can use conditional compilation to avoid including the > > high-speed descriptors when the UDC doesn't support high-speed > > operation. That's all. This means that the > > CONFIG_USB_GAGDET_DUALSPEED option does not need to be > > user-controllable in Kconfig. It should default to N, and UDC drivers > > that support high speed should select it. > > > > The same should be true of CONFIG_USB_GADGET_HIGHSPEED. It should not > > be user-controllable. It should default to N, and UDC drivers that > > support SuperSpeed operation (after these patches, only dummy-hcd) > > should select it. > > Ok, agreed. Thanks for the clarification. I saw your patch, I'll update > mine in the same way. Good. If Felipe doesn't object to my patch, I'll send it to Greg. > > There remains the other question, about whether composite_driver.speed > > should be set to USB_SPEED_SUPER. I think the matter can be settled at > > runtime. Iterate through all the function drivers; if all of them > > support SuperSpeed and CONFIG_USB_GADGET_SUPERSPEED is enabled then set > > composite_driver.speed to USB_SPEED_SUPER. > > Not sure how to verify this. I need to know whether the driver that is > registered with the UDC is SS or not. This is before the function drivers > are binded to it. So how can I verify at that point that the function > drivers that will bind to this driver will provide SS descriptors? > (I'm sorry, I don't have the ability to view the code at the moment and > due to the time differences between us I don't want to leave this question > for tomorrow and loose another day...) I'm not sure about this either. I have never used the composite framework so I'm not familiar with its details. This has to be decided before the composite gadget driver registers with the UDC driver ... and surely that can't happen until all the function drivers have registered with the composite driver? After all, the gadget can't be enumerated until all the function drivers are registered. Maybe this logic can be put in usb_add_function()? That routine already deals with issues involving device speed and available descriptors. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Wed, 25 May 2011, Alan Stern wrote: > I have looked this over more carefully. It turns out that both of you > have misunderstood the purpose of CONFIG_USB_GADGET_DUALSPEED (and by > extension, CONFIG_USB_GADGET_SUPERSPEED). In fact, the existing > Kconfig file is also wrong. > > The _only_ reason for CONFIG_USB_GADGET_DUALSPEED is so that gadget > drivers can use conditional compilation to avoid including the > high-speed descriptors when the UDC doesn't support high-speed > operation. That's all. This means that the > CONFIG_USB_GAGDET_DUALSPEED option does not need to be > user-controllable in Kconfig. It should default to N, and UDC drivers > that support high speed should select it. In other words, we should merge the following patch and the new SuperSpeed support should follow the same pattern. Alan Stern - This patch (as1468) changes the Kconfig definition for USB_GADGET_DUALSPEED. This option is determined entirely by which device controller drivers are to be built, through Select statements; it does not need to be (and should not be) configurable by the user. Also, the "default n" line is superfluous -- everything defaults to N. Signed-off-by: Alan Stern --- drivers/usb/gadget/Kconfig |5 + 1 file changed, 1 insertion(+), 4 deletions(-) Index: usb-2.6/drivers/usb/gadget/Kconfig === --- usb-2.6.orig/drivers/usb/gadget/Kconfig +++ usb-2.6/drivers/usb/gadget/Kconfig @@ -597,13 +597,10 @@ config USB_DUMMY_HCD endchoice +# Selected by UDC drivers that support high-speed operation. config USB_GADGET_DUALSPEED bool depends on USB_GADGET - default n - help - Means that gadget drivers should include extra descriptors - and code to handle dual-speed controllers. # # USB Gadget Drivers -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Wed, 25 May 2011, Tanya Brokhman wrote:
> Ok so just to make sure I understand you correctly:
> You want me to remove the modification made to usb_composite_probe() and
> instead add:
> static struct usb_gadget_driver composite_driver = {
> +#ifdef CONFIG_USB_GADGET_SUPERSPEED
> + .speed = USB_SPEE_SUPER,
> +#else
> .speed = USB_SPEED_HIGH,
> +#endif
> .unbind = composite_unbind,
>
> And then you'll be ok with change? Or is there anything else?
>
> If this is it then I'm relieved :) and of course will update the code.
I have looked this over more carefully. It turns out that both of you
have misunderstood the purpose of CONFIG_USB_GADGET_DUALSPEED (and by
extension, CONFIG_USB_GADGET_SUPERSPEED). In fact, the existing
Kconfig file is also wrong.
The _only_ reason for CONFIG_USB_GADGET_DUALSPEED is so that gadget
drivers can use conditional compilation to avoid including the
high-speed descriptors when the UDC doesn't support high-speed
operation. That's all. This means that the
CONFIG_USB_GAGDET_DUALSPEED option does not need to be
user-controllable in Kconfig. It should default to N, and UDC drivers
that support high speed should select it.
The same should be true of CONFIG_USB_GADGET_HIGHSPEED. It should not
be user-controllable. It should default to N, and UDC drivers that
support SuperSpeed operation (after these patches, only dummy-hcd)
should select it.
There remains the other question, about whether composite_driver.speed
should be set to USB_SPEED_SUPER. I think the matter can be settled at
runtime. Iterate through all the function drivers; if all of them
support SuperSpeed and CONFIG_USB_GADGET_SUPERSPEED is enabled then set
composite_driver.speed to USB_SPEED_SUPER. Otherwise, if all of them
support high speed and CONFIG_USB_GADGET_DUALSPEED is enabled then set
composite_driver.speed to USB_SPEED_HIGH. Otherwise set it to
USB_SPEED_FULL.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Tue, 24 May 2011, Tanya Brokhman wrote: > Hi Felipe > > > > Yes :) The driver->speed is updated in usb_composite_probe() if > > > CONFIG_USB_GADGET_SUPERSPEED is defined. > > > > > > So, are we ok with this solution? The module parameter I mean? > > > Are you going to try the v13 in your branch? Please let me know how > > it > > > goes and of course if you have any comments. > > > > I think it still gives the possibility for failure. I would rather not > > take that until all gadget drivers are fixed. We can help you doing > > that and we only change driver->speed after all gadget drivers have > > their "sensible defaults" SuperSpeed descriptors. > > By "until all gadget drivers are fixed" you mean until all gadget drivers > provide SS descriptors? This will take for ever... > I wasn't about to modify all gadget drivers and to add SS descriptors for > them. I can add default values (as generate_ss_descriptors() did if you > remember) but I don't think this is the right approach because as you said - > different gadget drivers might have different SS descriptors and I don't > feel confident enough to set these values. Nor do I have the ability to test > each of the gadget drivers the way I would like to after this change. > The only gadget driver I felt confident adding SS descriptors for is UASP, > which I tested properly. > > Actually if the CONFIG_USB_GADGET_SUPERSPEED is turned off, which is the > default of it, the speed won't be updated and all these series won't be > functional so I don't see any possibilities for failure in such > configuration. Or am I missing something? dummy_hcd should work when CONFIG_USB_GADGET_SUPERSPEED is enabled, even if the usb_gadget_driver structure is initialized with the speed field set to USB_SPEED_HIGH. This will be true for all the standalone gadget drivers until they are updated. Which leaves a question about the composite gadget framework. Should it be updated with SS support? Probably not until the various function drivers have all been updated. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Tue, 24 May 2011, Felipe Balbi wrote: > > It would be more accurate to say the module parameter will be used to > > force the connection to run at a lower speed than the maximum possible. > > This is kind of like what happens when you plug in a SuperSpeed device > > using a USB-2 cable -- the connection runs at a lower speed than it > > could have. > > if it's something like that, for sure we can have the module parameter. But > plugging a USB2 gadget/function to a USB3-capable UDC/HCD should > work fine. > > With Tatyana's patches, if we load a USB2 g_zero to dummy_hcd, enumeration > will fail where it shouldn't. This has been my whole point ;-) Maybe I wasn't > clear enough. I guess not. I thought Tatyana said she was working to fix that bug... > >> What about the case where SuperSpeed enumeration > >> fails and you have to fall back to high speed? > > > > If SuperSpeed enumeration fails, say because the device doesn't have > > any SuperSpeed descriptors, xhci-hcd doesn't fall back to high speed, > > does it? dummy-hcd should behave the same way. > > it should at least. Isn't that what happens between EHCI/OHCI ? HS Chirp > sequencing fails, then we fall back to FullSpeed. That's a failure in initialization, not a failure in enumeration. There are two reasons why the HS chirp might fail: the device doesn't support high speed operation or hardware errors prevent the chirp from working. With dummy-hcd there are no hardware errors (because there's no hardware). As for whether or not the device supports high-speed or SuperSpeed operation, that's determined by the usb_gadget_driver->speed field. If the field doesn't specify SuperSpeed then dummy-hcd should connect the gadget to the USB-2 root hub rather than the USB-3 root hub. Isn't that what Tatyana's patch does? It contains a line saying: dum->gadget.speed = driver->speed; > >> It seems like you really > >> need to handle both speeds and the speed fall back parameter in the same > >> driver. Isn't there some other gadget driver that has a fall back to > >> full or low speed when high speed enumeration fails? > > > > That's a property of the gadget driver, not the UDC driver. dummy-hcd > > is a UDC driver (and an HCD too). > > USB3.0 dummy_hcd should still enumerate USB2.0 gadget drivers. Yes, certainly it should. If it doesn't, that's a bug, not a design error. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Mon, 23 May 2011, Sarah Sharp wrote: > > 2. whether the device HW supports SS protocol. In our scenario it can since > > SS support is enabled in our udc. (We haven't released it yet.) > > What is a UDC? USB Device Controller. It's the device-side analog of a host controller. > > Since in dummy_hcd all of this is much simpler I think that the device speed > > should be determined by driver->speed and "which type of cable the > > connection was made over - SS or HS". The "cable type" is exactly what the > > module parameter is. > > I really don't understand this. You're going to have a module parameter > for what type of cable is plugged in? It would be more accurate to say the module parameter will be used to force the connection to run at a lower speed than the maximum possible. This is kind of like what happens when you plug in a SuperSpeed device using a USB-2 cable -- the connection runs at a lower speed than it could have. > How can you tell which one the > user is going to use? This isn't about actual cables or devices. dummy-hcd is an emulator; it presents as a host controller and a device controller both on the same system. This allows people to test gadget drivers without having any UDC hardware. > What about the case where SuperSpeed enumeration > fails and you have to fall back to high speed? If SuperSpeed enumeration fails, say because the device doesn't have any SuperSpeed descriptors, xhci-hcd doesn't fall back to high speed, does it? dummy-hcd should behave the same way. > It seems like you really > need to handle both speeds and the speed fall back parameter in the same > driver. Isn't there some other gadget driver that has a fall back to > full or low speed when high speed enumeration fails? That's a property of the gadget driver, not the UDC driver. dummy-hcd is a UDC driver (and an HCD too). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Mon, 23 May 2011, Tanya Brokhman wrote: > I ran some more tests with xhci and I think (or hope :) ) I figured this > out: > When connecting a gadget driver that is marked as SS device (the flag > CONFIG_USB_GADGET_IS_SUPER_SPEED = true) to a SS port over SS cable - > the enumeration fails if that gadget driver doesn't provide SS descriptors. > BUT: if I connect the same device via HS cable to SS port - the enumeration > is successful. I think that this is the case where xhci-ring handles the > device over to the HS hcd :) (By the way, I think that in xhci the > shared_hcd is SS and the main_hcd is HS) > > In conclusion it seems to me that the device speed is determined by 2 > things: > 1. the cable used > 2. whether the device HW supports SS protocol. In our scenario it can since > SS support is enabled in our udc. (We haven't released it yet.) > So when a HS device is connected to a SS port, the xHCI checks it's speed > and if necessary handles it over to the SS root hub. But this is done prior > to the enumeration phase so if the device speed is SS but it has no SS > descriptors - the enumeration will fail. The enumeration itself occurs not > in xhci but in hub.c so the xhci isn't aware of the fact that it failed and > doesn't handle this. > > Since in dummy_hcd all of this is much simpler I think that the device speed > should be determined by driver->speed and "which type of cable the > connection was made over - SS or HS". The "cable type" is exactly what the > module parameter is. > > My familiarity with the Linux host isn't as good as I would like it to be > (still working on that) so I might be wrong with my conclusions... Your analysis is basically correct. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Mon, 23 May 2011, Felipe Balbi wrote: > > > > +module_param_named(is_super_speed, mod_data.is_super_speed, bool, > > > > +S_IRUGO); MODULE_PARM_DESC(is_super_speed, "true to simulate > > > > +SuperSpeed connection"); > > > > > > you shouldn't need this. You should always enable SuperSpeed for this > > > driver. > > > > You mean I don't need the module parameter? IMO it's the best way to enable > > HS connection. If driver->speed=USB_SPEED_SUPER than dummy_hcd will try to > > enumerate the device on the SS root hub and if the gadget didn't provide SS > > descriptors - it will fail. Just as it happened before. Finding out from > > then it should hand the device over to the hs_hcd ;-) Meaning it would > disconnect the device, switch to hs_hcd and reconnect :-) No. That doesn't happen with real devices, so there's no reason dummy-hcd should do it. > > dummy_hcd that the enumeration failed is very complicated (if even possible) > > and I'm not sure that is the right thing to do... If you connect a real > > device over SS port to xHCI and the device doesn't provide SS descriptors - > > the enumeration fails and it's ok. But if you connect the same device to a > > HS port - it should work properly. This is what I tried to simulate with > > this parameter. > > it doesn't just fails, it gives the device over to the shared_hcd :-) It does not. xhci-hcd does not keep track of whether or not enumeration succeeds, and it doesn't do anything special when enumeration fails. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
On Mon, 23 May 2011, Felipe Balbi wrote:
> > diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> > index bf7981d..c2731d3 100644
> > --- a/drivers/usb/gadget/dummy_hcd.c
> > +++ b/drivers/usb/gadget/dummy_hcd.c
> > @@ -70,6 +70,15 @@ MODULE_DESCRIPTION (DRIVER_DESC);
> > MODULE_AUTHOR ("David Brownell");
> > MODULE_LICENSE ("GPL");
> >
> > +struct dummy_hcd_module_parameters {
> > + bool is_super_speed;
> > +};
> > +
> > +static struct dummy_hcd_module_parameters mod_data = {
> > + .is_super_speed = false
> > +};
> > +module_param_named(is_super_speed, mod_data.is_super_speed, bool, S_IRUGO);
> > +MODULE_PARM_DESC(is_super_speed, "true to simulate SuperSpeed connection");
>
> you shouldn't need this. You should always enable SuperSpeed for this
> driver.
I would say the opposite and go even farther. It's good to have a way
to force a SuperSpeed gadget to run at high speed, and it would also be
good to have a way to force a high-speed gadget to run at full speed.
A module parameter seems to be the simplest way of doing this.
> > + /*
> > +* We're connected and not reseted (reset occured now),
>
> 'reseted' isn't proper spelling :-)
Neither is "occured".
> > @@ -1371,6 +1577,10 @@ static void dummy_timer(unsigned long _dum_hcd)
> > case USB_SPEED_HIGH:
> > total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
> > break;
> > + case USB_SPEED_SUPER:
> > + /* Bus speed is 500000 bytes/ms, so use a little less */
>
> isn't it 50 bits/ms ?
SuperSpeed is 5 billion bits/s or 5 million bits/ms. The physical
layer encodes each data byte using 10 bits, therefore the bus speed is
50 bytes/ms.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 6/7] usb:dummy_hcd: use the shared_hcd infrustructure
On Thu, 19 May 2011, Tatyana Brokhman wrote:
> This patch is a preparation for adding SuperSpeed support to dummy hcd.
> It takes the master side fields out of the struct dummy to a separate
> structure. The init process was also modified to resemble the way it is
> done by xHCI.
>
> Signed-off-by: Tatyana Brokhman
I have not checked this in detail, but right off the bat there are a
few stylistic issues.
BTW, splitting this up into two patches as Felipe suggested was a very
good idea.
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -152,6 +152,24 @@ enum dummy_rh_state {
> DUMMY_RH_RUNNING
> };
>
> +struct dummy_hcd {
> + struct usb_hcd *hcd;
What is this pointer for? It doesn't get used by the
dummy_hcd_to_hcd() routine, which means it shouldn't get used at all.
> +
> + struct dummy*dum;
> + enum dummy_rh_state rh_state;
> + struct timer_list timer;
> + u32 port_status;
> + u32 old_status;
> + unsignedresuming:1;
It's silly to have a bitfield in isolation like this -- you end up
wasting a bunch of space. Move this next to the other bitfields.
> + unsigned long re_timeout;
> +
> + struct usb_device *udev;
> + struct list_headurbp_list;
> +
> + unsignedactive:1;
> + unsignedold_active:1;
> +};
> +
> struct dummy {
> spinlock_t lock;
>
> @@ -167,36 +185,26 @@ struct dummy {
> u16 devstatus;
> unsignedudc_suspended:1;
> unsignedpullup:1;
> - unsignedactive:1;
> - unsignedold_active:1;
>
> /*
>* MASTER/HOST side support
>*/
> - enum dummy_rh_state rh_state;
> - struct timer_list timer;
> - u32 port_status;
> - u32 old_status;
> - unsignedresuming:1;
> - unsigned long re_timeout;
> -
> - struct usb_device *udev;
> - struct list_headurbp_list;
> + struct dummy_hcd *hs_hcd;
You should use tabs to line up the columns with the existing entries.
Doesn't this look strange to you -- a single item in the middle of the
line by itself like that?
> };
>
> -static inline struct dummy *hcd_to_dummy (struct usb_hcd *hcd)
> +static inline struct dummy_hcd *hcd_to_dummy_hcd(struct usb_hcd *hcd)
> {
> - return (struct dummy *) (hcd->hcd_priv);
> + return (struct dummy_hcd *) (hcd->hcd_priv);
> }
>
> -static inline struct usb_hcd *dummy_to_hcd (struct dummy *dum)
> +static inline struct usb_hcd *dummy_hcd_to_hcd(struct dummy_hcd *dum)
> {
> return container_of((void *) dum, struct usb_hcd, hcd_priv);
> }
In several places you open-coded dummy_hcd_to_hcd(). Please change
them to use the inline routine instead.
> -static inline struct device *dummy_dev (struct dummy *dum)
> +static inline struct device *dummy_dev(struct dummy_hcd *dum)
> {
> - return dummy_to_hcd(dum)->self.controller;
> + return dummy_hcd_to_hcd(dum)->self.controller;
> }
You also open-coded this routine. It doesn't matter so much, but for
the sake of style you should use this inline routine too.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 7/7] usb: Adding SuperSpeed support to dummy_hcd
On Thu, 12 May 2011, Tanya Brokhman wrote: > > >+ retval = usb_add_hcd(hs_hcd, 0, IRQF_DISABLED | IRQF_SHARED); > > Why IRQF_DISABLED | IRQF_SHARED? you don't interrupts at all. > > I used xhci as reference for this patch. This is the way xhci driver adds > the hcds. You can find it in xhci_pci_probe(). > I'm not very familiar with how this two hcds (main and shared shcd) will > work together so I thought these flags are needed for their co-operation. > Was my assumption mistaken? What flags should I use? You don't need any flags at all, because dummy-hcd does not handle any hardware interrupts. > Also, Sergei pointed out that IRQF_DISABLED is deprecated so it seems to me > that xhci needs an update as well Plus every other source file that uses IRQF_DISABLED... Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH v3 2/5] uas: MS UAS Gadget driver - Infrastructure
On Thu, 28 Apr 2011, Tanya Brokhman wrote: > Thank you very much for you inputs and help! I'm not familiar with the SCSI > Target framework Christoph referred me too so I postponed looking into that > until the driver is stable. Well, I guess that time has come so I'll switch > over to studying that and applying it to the UAS gadget driver. I'm not familiar with that framework either, so if you find any good introductory documentation (other than the files in Documentation/target) please post a link to it. Nicholas Bellinger is very willing to answer questions. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v3 2/5] uas: MS UAS Gadget driver - Infrastructure
On Tue, 26 Apr 2011, Sarah Sharp wrote: > I meant that during enumeration, configuration 1 would be installed, and > because the USB core doesn't try to install a particular alternate > interface setting, alt setting 0 would be active by default. Unless the driver changed it. > How would the usb-storage driver reject a bind by the USB core? By > returning an error from the probe function? Would the USB core go and > search for the next driver after the BOT driver rejected the bind? It > looks like usb_probe_interface will just return an error if the first > driver's probe function fails. If a driver's or subsystem's probe routine returns an error then the driver core continues looking for another driver. If the error code is -ENODEV or -ENXIO then it doesn't even put a warning in the kernel log; the driver core takes this to mean that the driver detected it couldn't handle the device. See drivers/base/dd.c:really_probe(). > > Which reminds me... Fallbacks are always a good idea. If usb-storage > > did decide not to bind to combined BOT/UAS devices, we should have a > > mechanism for overriding this choice (i.e., forcing usb-storage to bind > > regardless). > > Sure, maybe a module parameter like "own_uas"? Or do we want something > fancier, like a way to specify a list of VID:PIDs that the usb-storage > driver should own? (I think the list parsing might be a bit hard to > implement though.) The VID:PID thing would work; usb-storage already has a "quirks" parameter that accepts such things. We could add a quirk for binding to a BOT/UAS interface. > > Likewise, Sarah, you should consider adding a mechanism to xhci-hcd for > > forcing individual root-hub ports not to run at SuperSpeed (rather like > > the "companion" attribute file in ehci-hcd, although I'm sure you can > > come up with a better name). > > I'm not entirely sure I can force a port down to USB 2.0 speeds, because > I'm not sure I can disable the port or turn off SuperSpeed terminations > from the xHCI driver. I'd have to look into it. This sounds like the problem we encountered while trying to disable a SuperSpeed port. It merely forced the device to switch over to the USB-2 bus. That wasn't what we wanted then, but it is what we're interested in now. Getting things to switch back might be harder -- re-enabling the SuperSpeed terminations won't force the device to stop using the USB-2 bus. It might be necessary to reset the USB-2 port. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v3 2/5] uas: MS UAS Gadget driver - Infrastructure
m IDs, and each thread > can handle one SCSI command. Or if you want each thread to be able to > handle C number of commands, you could set bMaxStreams value to C*X. > > Right now the UAS driver attempts to allocate 65536 streams (which > translates to allowing the SCSI core to submit up to 65536 commands > asynchronously), but the xHCI driver will only let it allocate the > minimum number of streams that each of the three endpoints advertise. > Probably the UAS driver needs to update the .can_queue value for the > SCSI host after looking at the device descriptors. I'm not sure if that > means the SCSI core can submit up to .can_queue commands for each LUN, > or up to .can_queue commands for all LUNs. For all LUNs. Since Tanya is writing the gadget driver, she gets to decide how many streams it will support (limited by the device controller hardware). In this setting I would use one thread per stream, since a thread can't have more than one VFS read or write request outstanding at any time. Except that, as Christoph has mentioned a few times, it would be better to abandon this driver design entirely and use the SCSI Target framework instead. Then presumably threads would not be an issue. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v6 1/5] usb: Add streams support to the gadget framework
On Thu, 14 Apr 2011, Tatyana Brokhman wrote:
> This patch defines necessary fields to support streaming for USB3.0.
> It implements a new function (usb_ep_autoconfig_ss) to be used instead of
> the existing usb_ep_autoconfig when working in SS mode and there is a
> need to search for an endpoint according to the number of required
> streams.
Suggestions for some small improvements...
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -34,6 +34,8 @@ struct usb_ep;
> * by adding a zero length packet as needed;
> * @short_not_ok: When reading data, makes short packets be
> * treated as errors (queue stops advancing till cleanup).
> + * @reserved: reserved bits
> + * @stream_id: the stream id
If you move stream_id to the start of the bitfields then you won't need
to reserve any bits.
Also, the comment should explain a little more. For example:
* @stream_id: The stream id, when USB-3.0 bulk streams are being used.
> @@ -131,6 +136,8 @@ struct usb_ep_ops {
> * @maxpacket:The maximum packet size used on this endpoint. The initial
> * value can sometimes be reduced (hardware allowing), according to
> * the endpoint descriptor used to configure the endpoint.
> + * @num_supported_strms:The number of maximum streams supported
> + * by this EP (0 - 16, actual number is 2^n)
Ugh! Call this max_streams instead. The comment should say "maximum
number of streams", not "number of maximum streams".
> * @mult: multiplier, 'mult' value for SS Isoc EPs
> * @maxburst: the maximum number of bursts supported by this EP (for usb3)
> * @driver_data:for use by the gadget driver.
> @@ -152,6 +159,7 @@ struct usb_ep {
> const struct usb_ep_ops *ops;
> struct list_headep_list;
> unsignedmaxpacket:16;
> + int num_supported_strms;
You might also want to make this a 16-bit unsigned field, instead of
sticking an integer between two bitfields.
> unsignedmult:2;
> unsignedpad:1;
> unsignedmaxburst:4;
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v3 2/5] uas: MS UAS Gadget driver - Infrastructure
On Thu, 14 Apr 2011, Tatyana Brokhman wrote: > This patch implements the infrastructure for the UAS gadget driver. > The UAS gadget driver registers as a second configuration of the MS > gadet driver. > > A new module parameter was added to the mass_storage module: > bool use_uasp. (default = 0) > If this parameter is set to true, the mass_storage module will register > with the UAS configuration as the devices first configuration and > operate according to the UAS protocol. As I understand it, drives typically have only one configuration and one interface. They implement Bulk-Only Transport on altsetting 0 and UAS on altsetting 1. On the other hand, I don't think there's anything that says you _have_ to do it that way... Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: merge the two dummy_hcds into one
On Thu, 14 Apr 2011, Sebastian Andrzej Siewior wrote: > This patch merges the two HCDs so there is one for both. It is too premature for this. In fact, the support for USB-3 HCDs is currently in flux. Sarah Sharp has submitted a whole bunch of patches to allow for the creation of primary and secondary HCDs, representing the USB-2 and USB-3 root hubs of an xHCI controller. dummy-hcd should use the same mechanism. This will means creating only one host-side platform device, and then creating two usb_hcd structures below it. Tanya's design should be based on Sarah's work. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UAS gadget driver & UAS host driver [was: Re: [RFC/PATCH 0/4] UASP device driver]
On Mon, 21 Feb 2011, Sarah Sharp wrote: > On Mon, Feb 21, 2011 at 05:35:47PM +0200, Tanya Brokhman wrote: > > Hi Sarah > > > > > That particular error is from the host side. It means the xHCI driver > > > tried to move the ring dequeue pointer past the last part of your > > > command (probably the status phase), and the host refused to do so, > > > because that was the "active" stream ID. Can you please send me the > > > full dmesg for the stalled request with CONFIG_USB_XHCI_HCD_DEBUGGING > > > turned on? > > > > Np. I've already overcame this issue but I'll roll back and send you the > > dmesg output. When I tried enabling this flag the dmesg was swamped with > > xhci messages and the messages I was looking for weren't there already. > > I read that in order to increase the dmesg buffer size one needs to update > > the #define of LOG_BUF_LEN in linux/kernel/printk.c. I haven't tried that > > yet. Is this correct? The appropriate way to change this value is by updating the kernel configuration. The value is set by CONFIG_LOG_BUF_SHIFT (General setup / Kernel log buffer size, if you use "make menuconfig"). > There will be a lot of messages, but you can capture them by running > this command line: > > tail -f /var/log/kern.log | tee logfile.txt This is not as reliable as using dmesg. When a lot of messages are generated very quickly, the log daemons often drop some of them. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/5] usb: Adding SuperSpeed support to dummy_hcd
On Thu, 10 Feb 2011, Tanya Brokhman wrote: > > > + if (dum->gadget.speed < USB_SPEED_SUPER) > > > + max &= 0x3ff; > > > > Strictly speaking, both the old and the new code are wrong. For all > > devices, regardless of speed, we should say: > > > > max = le16_to_cpu(desc->wMaxPacketSize) & 0x7ff; > > > > In other words, the speed is contained in bits 0..10 (not 0..9). > > > > You're right about HS/FS/LS devices but I see something different in the > USB30 spec for SS devices. Could you please refer me to what errata this was > mentioned in? I looked through them and according to errata from 01/15/09 > the definition of wMaxPacketSize field in the SS endpoint descriptor is: > > "Maximum packet size this endpoint is capable of sending or receiving when > this configuration is selected. For control endpoints this field shall be > set to 512. For bulk endpoint types this field shall be set to 1024." > > So according to the above, for SS devices, the whole 2 bytes contain the > maxpacketsize and not only bits 0..10. > Am I missing something? No, you are right. However, it is still true even with SS that the value is contained in bits 0..10, since the value can never be larger than 2047. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/5] usb: Adding SuperSpeed support to dummy_hcd
On Mon, 31 Jan 2011, Tanya Brokhman wrote: > > > This patch adds SS support to the dummy hcd module. It may be used to > > test > > > SS device when no (SS) HW is available. > > > USB 3.0 hub includes 2 hubs - HS and SS ones. > > > This patch adds support for a SS hub in the dummy_hcd module. Thus, > > when > > > dummy_hcd enabled it will register 2 root hubs (SS and HS). > > > > Getting there. A few spots could still use some attention. > > > > Could you please elaborate? I _did_ elaborate in the original email message. Here's a repeat of what I said: > @@ -343,7 +431,13 @@ dummy_enable (struct usb_ep *_ep, const struct > usb_endpoint_descriptor *desc) > dum = ep_to_dummy (ep); > if (!dum->driver || !is_enabled (dum)) > return -ESHUTDOWN; > - max = le16_to_cpu(desc->wMaxPacketSize) & 0x3ff; > + max = le16_to_cpu(desc->wMaxPacketSize) ; > + /* > + * For HS/FS devices only bits 0..9 of the wMaxPacketSize represent the > + * maximum packet size > + */ > + if (dum->gadget.speed < USB_SPEED_SUPER) > + max &= 0x3ff; Strictly speaking, both the old and the new code are wrong. For all devices, regardless of speed, we should say: max = le16_to_cpu(desc->wMaxPacketSize) & 0x7ff; In other words, the speed is contained in bits 0..10 (not 0..9). > @@ -1352,6 +1559,10 @@ static void dummy_timer (unsigned long _dum) > case USB_SPEED_HIGH: > total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/; > break; > + case USB_SPEED_SUPER: > + /* 400MBps = 400*(2^20)/1000 bytes per milisec */ > + total = (400 << 20) / 1000; > + break; Ugh. For one thing, SuperSpeed runs at 500 million bytes/s, not 400 million. For another, 400 << 20 isn't equal to 400 million. Don't try to be too clever. Realizing that this is only an approximation anyway, just say: /* Bus speed is 50 bytes/ms, so use a little less */ total = 49; > > Also, you might try running this patch through checkpatch.pl and fixing > > any complaints it makes. > > > > Actually I always do that. It came out clean. If you see something wrong > with the coding style please let me know. There are a few places where a terminating ';' is preceded by an unnecessary ' ' character. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/5] usb: Adding SuperSpeed support to dummy_hcd
On Thu, 27 Jan 2011, Tatyana Brokhman wrote: > This patch adds SS support to the dummy hcd module. It may be used to test > SS device when no (SS) HW is available. > USB 3.0 hub includes 2 hubs - HS and SS ones. > This patch adds support for a SS hub in the dummy_hcd module. Thus, when > dummy_hcd enabled it will register 2 root hubs (SS and HS). Getting there. A few spots could still use some attention. Also, you might try running this patch through checkpatch.pl and fixing any complaints it makes. > @@ -343,7 +431,13 @@ dummy_enable (struct usb_ep *_ep, const struct > usb_endpoint_descriptor *desc) > dum = ep_to_dummy (ep); > if (!dum->driver || !is_enabled (dum)) > return -ESHUTDOWN; > - max = le16_to_cpu(desc->wMaxPacketSize) & 0x3ff; > + max = le16_to_cpu(desc->wMaxPacketSize) ; > + /* > + * For HS/FS devices only bits 0..9 of the wMaxPacketSize represent the > + * maximum packet size > + */ > + if (dum->gadget.speed < USB_SPEED_SUPER) > + max &= 0x3ff; Strictly speaking, both the old and the new code are wrong. For all devices, regardless of speed, we should say: max = le16_to_cpu(desc->wMaxPacketSize) & 0x7ff; In other words, the speed is contained in bits 0..10 (not 0..9). > @@ -1352,6 +1559,10 @@ static void dummy_timer (unsigned long _dum) > case USB_SPEED_HIGH: > total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/; > break; > + case USB_SPEED_SUPER: > + /* 400MBps = 400*(2^20)/1000 bytes per milisec */ > + total = (400 << 20) / 1000; > + break; Ugh. For one thing, SuperSpeed runs at 500 million bytes/s, not 400 million. For another, 400 << 20 isn't equal to 400 million. Don't try to be too clever. Realizing that this is only an approximation anyway, just say: /* Bus speed is 50 bytes/ms, so use a little less */ total = 49; Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/4] UASP device driver
On Fri, 21 Jan 2011, Tatyana Brokhman wrote: > This patch series implements the UASP device driver. > The implementation is incomplete. We would like to receive comments > on the design. I haven't read the code, and I won't have time to read it for quite a while. Nevertheless, the title and the first sentence in the patch description contain two errors. First, there is no such thing as UASP. What you're talking about is UAS -- USB-Attached SCSI. Second, the patch set implements a gadget (or function) driver, not a device driver. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 1/4] USB: core: OTG Supplement Revision 2.0 updates
On Thu, 16 Dec 2010, Pavankumar Kondeti wrote:
> OTG supplement revision 2.0 spec introduces Attach Detection Protocol
> (ADP) for detecting peripheral connection without applying power on
> VBUS. ADP is optional and is included in the OTG descriptor along with
> SRP and HNP.
>
> HNP polling is introduced for peripheral to notify its wish to become
> host. Host polls (GET_STATUS on DEVICE) peripheral for host_request
> and suspend the bus when peripheral returns host_request TRUE. The spec
> insists the polling frequency to be in 1-2 sec range and bus should be
> suspended within 2 sec from host_request is set.
>
> a_alt_hnp_support feature is obsolete and a_hnp_support feature is limited
> to only legacy OTG B-device. The newly introduced bcdOTG field in the OTG
> descriptor is used for identifying the 2.0 compliant B-device.
This combination of things doesn't make sense:
> index b9278a1..baada06 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1270,6 +1282,47 @@ static int usb_resume_both(struct usb_device *udev,
> pm_message_t msg)
> return status;
> }
>
> +#ifdef CONFIG_USB_OTG
> +void usb_hnp_polling_work(struct work_struct *work)
> +{
...
> +}
> +#endif
usb_hnp_polling_work() is defined only when CONFIG_USB_OTG is set.
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index e70aeaf..5dd59e9 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -869,6 +869,7 @@ static void usb_bus_init (struct usb_bus *bus)
> bus->bandwidth_isoc_reqs = 0;
>
> INIT_LIST_HEAD (&bus->bus_list);
> + INIT_DELAYED_WORK(&bus->hnp_polling, usb_hnp_polling_work);
> }
But its address is taken regardless of CONFIG_USB_OTG.
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index b975450..ffa16e1 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -72,6 +72,14 @@ static inline int usb_port_resume(struct usb_device *udev,
> pm_message_t msg)
>
> #endif
>
> +#ifdef CONFIG_USB_OTG
> +extern void usb_hnp_polling_work(struct work_struct *work);
> +#else
> +static inline void usb_hnp_polling_work(struct work_struct *work)
> +{
> +}
> +#endif
Otherwise it is an empty inline routine.
But taking the address of an inline routine forces the compiler to
produce an out-of-line version. Therefore you might as well explicitly
define usb_hnp_polling_work() as an empty function when CONFIG_USB_OTG
isn't set.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates
On Wed, 15 Dec 2010, Felipe Balbi wrote:
> >+{
> >+int ret;
> >+struct usb_bus *bus =
> >+container_of(work, struct usb_bus, hnp_polling.work);
> >+struct usb_device *udev = bus->root_hub->children[bus->otg_port - 1];
> >+u8 *status = kmalloc(sizeof(*status), GFP_KERNEL);
>
> how about:
>
> u8 status;
>
> and...
>
> >+if (!status)
> >+return;
> >+
> >+ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> >+USB_REQ_GET_STATUS, USB_DIR_IN | USB_RECIP_DEVICE,
> >+0, OTG_STATUS_SELECTOR, status, sizeof(*status),
>
> 0, OTG_STATUS_SELECTOR, &status, sizeof(status);
> ??
You mustn't do DMA to addresses on the stack.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/5] USB core changes for supporting OTG on MSM SoC
On Wed, 15 Dec 2010, Pavankumar Kondeti wrote: > This patch series adds OTG 2.0 enhancements and misc changes > required for supporting SRP & HNP in MSM OTG driver. As these > patches modify the USB core code, I would like to receive > feedback before posting driver patches. The driver patches are > available at > https://www.codeaurora.org/gitweb/quic/la/?p=kernel/msm.git;a=commitdiff;h=05535a995e9eb34c3c41d874d09955a443008cd5 All those "#ifdef CONFIG_USB_OTG" lines you are sprinkling throughout the core are very annoying. A few simple inline routines should allow you to eliminate many of them. Also, in the 5/5 patch in hub_activate(), all those if (hdev->bus->is_b_host && type == HUB_INIT) tests are distracting. You can make the test once, at the start of the function, and store the result in a local variable. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates
On Wed, 15 Dec 2010, Pavankumar Kondeti wrote:
> OTG supplement revision 2.0 spec introduces Attach Detection Protocol
> (ADP) for detecting peripheral connection without applying power on
> VBUS. ADP is optional and is included in the OTG descriptor along with
> SRP and HNP.
>
> HNP polling is introduced for peripheral to notify its wish to become
> host. Host polls (GET_STATUS on DEVICE) peripheral for host_request
> and suspend the bus when peripheral returns host_request TRUE. The spec
> insists the polling frequency to be in 1-2 sec range and bus should be
> suspended with in 2 sec from host_request is set.
>
> a_alt_hnp_support feature is obsolete and a_hnp_support feature is limited
> to only legacy OTG B-device. The newly introduced bcdOTG field in the OTG
> descriptor is used for identifying the 2.0 compliant B-device.
>
> Signed-off-by: Pavankumar Kondeti
> ---
> drivers/usb/core/driver.c | 50 +++
> drivers/usb/core/hcd.c|3 ++
> drivers/usb/core/hub.c| 71 ++--
> drivers/usb/core/usb.h|4 ++
> include/linux/usb.h |2 +
> include/linux/usb/ch9.h | 11 ++-
> 6 files changed, 129 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index b9278a1..38885b6 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1270,6 +1283,43 @@ static int usb_resume_both(struct usb_device *udev,
> pm_message_t msg)
> return status;
> }
>
> +#ifdef CONFIG_USB_OTG
> +void usb_hnp_polling_work(struct work_struct *work)
> +{
> + int ret;
> + struct usb_bus *bus =
> + container_of(work, struct usb_bus, hnp_polling.work);
> + struct usb_device *udev = bus->root_hub->children[bus->otg_port - 1];
> + u8 *status = kmalloc(sizeof(*status), GFP_KERNEL);
> +
> + if (!status)
> + return;
Shouldn't you reschedule the delayed work? A memory allocation failure
is likely to be temporary.
> +
> + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> + USB_REQ_GET_STATUS, USB_DIR_IN | USB_RECIP_DEVICE,
> + 0, OTG_STATUS_SELECTOR, status, sizeof(*status),
> + USB_CTRL_GET_TIMEOUT);
> + if (ret < 0) {
> + /* Peripheral may not be supporting HNP polling */
> + dev_vdbg(&udev->dev, "HNP polling failed. status %d\n", ret);
> + goto out;
> + }
> +
> + /* Spec says host must suspend the bus with in 2 sec. */
> + if (*status & (1 << HOST_REQUEST_FLAG)) {
> + do_unbind_rebind(udev, DO_UNBIND);
You forget to set udev->do_remote_wakeup to 0.
> + ret = usb_suspend_both(udev, PMSG_USER_SUSPEND);
> + if (ret)
> + dev_info(&udev->dev, "suspend failed\n");
> + } else {
> + schedule_delayed_work(&bus->hnp_polling,
> + msecs_to_jiffies(THOST_REQ_POLL));
> + }
> +out:
> + kfree(status);
> +}
> +#endif
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] usb: Adding SuperSpeed support to dummy_hcd
On Thu, 18 Nov 2010, Tatyana Brokhman wrote: > USB 3.0 hub includes 2 hubs - HS and SS ones. > Thus, when dummy_hcd enabled it will register 2 root hubs (SS and HS). Hmm. A quick comparison with the original source shows that you neglected to add code for SuperSpeed transfers to periodic_bytes() and show_urb(). > @@ -1352,6 +1544,9 @@ static void dummy_timer (unsigned long _dum) > case USB_SPEED_HIGH: > total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/; > break; > + case USB_SPEED_SUPER: > + total = 400 << 20; /* 400MB = 400*(2^20) bytes */ > + break; Somehow I doubt that SuperSpeed USB, quick though it is, can transfer 400 MB in one millisecond. Where did you get that number from? I can't find the appropriate table in the USB-3.0 spec (it might not be there at all), but with a bus transfer rate of 500 million bytes/s it seems clear that you can't transfer more than 50 bytes per frame. The actual limit must be lower than that, maybe something like 1024 bytes/packet * 60 packets/uframe * 8 uframes/frame = 491520 bytes/frame. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] usb: dummy_hcd code simplification
On Tue, 19 Oct 2010, Tatyana Brokhman wrote:
> Take handling of the control requests out from dummy_timer to a different
> function.
>
> Signed-off-by: Tatyana Brokhman
> ---
> drivers/usb/gadget/dummy_hcd.c | 250
> +---
> 1 files changed, 134 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index dc65462..ff93fea 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -1195,6 +1195,138 @@ static struct dummy_ep *find_endpoint (struct dummy
> *dum, u8 address)
> #define Ep_Request (USB_TYPE_STANDARD | USB_RECIP_ENDPOINT)
> #define Ep_InRequest (Ep_Request | USB_DIR_IN)
>
> +
> +/**
> + * handle_control_request() - handles all control transferes
> + *
There should not be a blank line here.
> + * @dum - pointer to dummy (the_controller)
> + * @urb - the urb request to handle
Where's the line for "setup"?
> + * @status - pointer to request handling status
In each of these there should be a ':' following the variable name,
not a space and a '-'. Honestly, I can't see why you have so much
trouble copying the format of existing kerneldoc entries.
> + *
> + * Return 0 - if the request was handled
> + * 1 - if the request wasn't handles
> + * error code on error
> + */
> +static int handle_control_request(struct dummy *dum, struct urb *urb,
> + struct usb_ctrlrequest setup,
Don't pass "setup" by value! Pass it by reference, i.e., pass a
pointer to the "setup" variable in dummy_timer().
> + int *status)
> +{
The rest of this is okay.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 3/3] usb: Adding SuperSpeed support to dummy_hcd
On Tue, 19 Oct 2010 tlin...@codeaurora.org wrote:
> >> +/**
> >> + * dummy_address_device() - Assign an address for the connected
> >> + * device
> >> + * @param hcd - host controller of the device
> >> + * @param udev - device to address
> >> + *
> >> + * @return int - 0 on success, or an error code (refer to
> >> + * errno-base.h for details)
> >> + *
> >> + * Issue an Address Device command (which will issue a
> >> + * SetAddress request to the device). We should be protected by
> >> + * the usb_address0_mutex in khubd's hub_port_init, so we should
> >> + * only issue and wait on one address command at the same time.
> >> + *
> >> + * This function is used only for SS hcd drivers.
> >> + */
> >> +static int dummy_address_device(struct usb_hcd *hcd, struct usb_device
> >> *udev)
> >> +{
> >> + udev->devnum = 3;
> >> + return usb_control_msg(udev, (PIPE_CONTROL << 30),
> >> + USB_REQ_SET_ADDRESS, 0, udev->devnum, 0,
> >> + NULL, 0, USB_CTRL_SET_TIMEOUT);
> >> +}
> >
> > This looks very suspicious. Why have this function if it's only going
> > to do the same thing that usbcore would do if it weren't present?
>
> Upon new device connection the host addresses the device from
> hub_set_address() that if the address_device cb was provided for the hcd -
> calls it. This function begins with a verification that either this cb was
> supplied or the usbcore already addresses the device (meaning udev->devnum
> > 1).
> usbcore addresses the device in choose_address() that is called from
> hub_port_connect_change but only if it's not a SuperSpeed hcd:
> if (!(hcd->driver->flags & HCD_USB3)) {
> /* set the address */
> choose_address(udev);
> ...
> }
> Since our hcd is SuperSpeed, choose_address() isn't called and
> udev->devnum remains 0.
> Due to the above in order for this implementation to work properly we have
> to provide the address_device cb for a SuperSpeed hcd.
> Perhaps this was already fixed but I'm not familiar with such patch. I
> would be happy to pick it up if you could refer me to it.
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-07-usb/usb-core-use-kernel-assigned-address-for-devices-under-xhci.patch
> > Also, the address_device routine should not change udev->devnum. The
> > code that does this in the xhci driver is being removed, because it
> > causes bugs.
>
> A better solution might be to call choose_address() for SuperSpeed hcd as
> well. If that sounds good to everyone I can make the change.
The patch listed above already contains these changes.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 3/3] usb: Adding SuperSpeed support to dummy_hcd
On Mon, 18 Oct 2010, Tatyana Brokhman wrote:
> USB 3.0 hub includes 2 hubs - HS and SS ones.
> Thus, when dummy_hcd enabled it will register 2 root hubs (SS and HS).
All of your new kerneldoc comments are invalid. The patch cannot be
accepted until you fix them.
> @@ -750,16 +865,24 @@ static DEVICE_ATTR (function, S_IRUGO, show_function,
> NULL);
> int
> usb_gadget_register_driver (struct usb_gadget_driver *driver)
> {
> - struct dummy*dum = the_controller;
> + struct dummy*dum;
> int retval, i;
> + enum usb_device_speed speed = USB_SPEED_UNKNOWN;
> +
> + if (!driver->bind || !driver->setup
> + || driver->speed == USB_SPEED_UNKNOWN)
> + return -EINVAL;
> +
> + speed = driver->speed;
What do you need "speed" for? Can't you use driver->speed?
> +static int dummy_ss_udc_probe(struct platform_device *pdev)
> +{
> + struct dummy*dum = the_ss_controller;
> + int rc;
> +
> + dum->gadget.name = gadget_name;
> + dum->gadget.ops = &dummy_ops;
> + dum->gadget.is_dualspeed = 1;
Is this setting appropriate? The "is_dualspeed" field indicates that
the gadget runs at both full speed and high speed. But that's not what
you want -- you want to indicate that the gadget runs at SuperSpeed.
> @@ -1384,6 +1559,9 @@ static void dummy_timer (unsigned long _dum)
> case USB_SPEED_HIGH:
> total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
> break;
> + case USB_SPEED_SUPER:
> + total = 1024/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
> + break;
I don't know what the transfer parameters for SuperSpeed are, but I do
know that these are wrong. With over 6 bytes per uframe there is
certainly room for more than thirteen 1024-byte packets.
> +/**
> + * dummy_address_device() - Assign an address for the connected
> + * device
> + * @param hcd - host controller of the device
> + * @param udev - device to address
> + *
> + * @return int - 0 on success, or an error code (refer to
> + * errno-base.h for details)
> + *
> + * Issue an Address Device command (which will issue a
> + * SetAddress request to the device). We should be protected by
> + * the usb_address0_mutex in khubd's hub_port_init, so we should
> + * only issue and wait on one address command at the same time.
> + *
> + * This function is used only for SS hcd drivers.
> + */
> +static int dummy_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> + udev->devnum = 3;
> + return usb_control_msg(udev, (PIPE_CONTROL << 30),
> + USB_REQ_SET_ADDRESS, 0, udev->devnum, 0,
> + NULL, 0, USB_CTRL_SET_TIMEOUT);
> +}
This looks very suspicious. Why have this function if it's only going
to do the same thing that usbcore would do if it weren't present?
Also, the address_device routine should not change udev->devnum. The
code that does this in the xhci driver is being removed, because it
causes bugs.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 2/3] usb: dummy_hcd code simplification
On Mon, 18 Oct 2010, Tatyana Brokhman wrote:
> Take handling of the control requests out from dummy_timer to a different
> function.
This is basically okay but it has a few problems.
>
> Signed-off-by: Tatyana Brokhman
> ---
> drivers/usb/gadget/dummy_hcd.c | 284
>
> 1 files changed, 168 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index dc65462..7640e06 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -1195,6 +1195,173 @@ static struct dummy_ep *find_endpoint (struct dummy
> *dum, u8 address)
> #define Ep_Request (USB_TYPE_STANDARD | USB_RECIP_ENDPOINT)
> #define Ep_InRequest (Ep_Request | USB_DIR_IN)
>
> +
> +/**
> + * This function handles all control transferes
> + *
> + * @param dum - pointer to dummy (the_controller)
> + * @param urb - the urb request to handle
> + * @param status - pointer to request handling status
> + * @param master - pointer to the dummy master this request was
> + *received for
> + */
The kerneldoc format is still wrong. Please fix it. What is all this
"@param" stuff? And what is "master"?
> +static int handle_control_request(struct dummy *dum, struct urb *urb,
> + int *status)
> +{
> + struct usb_ctrlrequest setup;
> + struct dummy_ep *ep2;
> + int ret_val = 1;
> + unsignedw_index;
> + unsignedw_value;
> +
> + setup = *(struct usb_ctrlrequest *) urb->setup_packet;
You should pass the "setup" variable as a pointer instead of making it
a local variable.
> + w_index = le16_to_cpu(setup.wIndex);
> + w_value = le16_to_cpu(setup.wValue);
> + switch (setup.bRequest) {
> + case USB_REQ_SET_ADDRESS:
> + if (setup.bRequestType != Dev_Request)
> + break;
> + dum->address = w_value;
> + *status = 0;
> + dev_dbg(udc_dev(dum), "set_address = %d\n",
> + w_value);
> + ret_val = 0;
> + break;
> + case USB_REQ_SET_FEATURE:
> + if (setup.bRequestType == Dev_Request) {
> + ret_val = 0;
> + switch (w_value) {
> + case USB_DEVICE_REMOTE_WAKEUP:
> + break;
> + case USB_DEVICE_B_HNP_ENABLE:
> + dum->gadget.b_hnp_enable = 1;
> + break;
> + case USB_DEVICE_A_HNP_SUPPORT:
> + dum->gadget.a_hnp_support = 1;
> + break;
> + case USB_DEVICE_A_ALT_HNP_SUPPORT:
> + dum->gadget.a_alt_hnp_support = 1;
> + break;
> + case USB_DEVICE_U1_ENABLE:
> + if (dum->gadget.speed == USB_SPEED_SUPER)
> + w_value = USB_DEV_STAT_U1_ENABLED;
> + else
> + ret_val = -EOPNOTSUPP;
> + break;
> + case USB_DEVICE_U2_ENABLE:
> + if (dum->gadget.speed == USB_SPEED_SUPER)
> + w_value = USB_DEV_STAT_U2_ENABLED;
> + else
> + ret_val = -EOPNOTSUPP;
> + break;
> + case USB_DEVICE_LTM_ENABLE:
> + if (dum->gadget.speed == USB_SPEED_SUPER)
> + w_value = USB_DEV_STAT_LTM_ENABLED;
> + else
> + ret_val = -EOPNOTSUPP;
> + break;
Where did these last three cases come from? They aren't in the current
version of dummy-hcd. The same is true for the cases under
USB_REQ_CLEAR_FEATURE.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] USB: Add MSM USB Device Controller driver
On Wed, 1 Sep 2010, Pavankumar Kondeti wrote:
> This patch adds the basic support for the USB Device Controller on Qualcomm
> MSM family of SOCs. The controller supports upto 16 endpoints including the
> default endpoint (ep0). All the data transfers are driven by DMA.
>
> VBUS line is also connected to PMIC chip. The module controlling PMIC chip
> notifies about cable connect/disconnect events. Hence, PHY comparators
> are turned off in low power mode.
>
> This driver was originally developed by Google and is available at
> http://android.git.kernel.org/?p=kernel/experimental.git.
...
> +static const struct usb_ep_ops msm72k_ep_ops = {
> + .enable = msm72k_enable,
> + .disable= msm72k_disable,
> +
> + .alloc_request = msm72k_alloc_request,
> + .free_request = msm72k_free_request,
> +
> + .queue = msm72k_queue,
> + .dequeue= msm72k_dequeue,
> +
> + .set_halt = msm72k_set_halt,
> + .fifo_flush = msm72k_fifo_flush,
> +};
The .set_wedge method is missing.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
