Re: [PATCH] build some drivers only when compile-testing
On 06/18/2013 06:04 PM, Greg Kroah-Hartman wrote: >> So currently I have what is attached... Comments? > > Looks good to me, want me to queue it up through my char/misc driver > tree for 3.11? If there are no objections... Whoever picks that up, I would be happy 8-). -- js suse labs -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] usb: fix build error without CONFIG_USB_PHY
On Wed, Jun 19, 2013 at 10:11:05AM +0800, Peter Chen wrote:
> on i386:
>
> drivers/built-in.o: In function `ci_hdrc_probe':
> core.c:(.text+0x20446b): undefined reference to `of_usb_get_phy_mode'
>
> Signed-off-by: Peter Chen
> ---
> include/linux/usb/of.h | 14 +-
> 1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> index e460a24..a0aa1c8 100644
> --- a/include/linux/usb/of.h
> +++ b/include/linux/usb/of.h
> @@ -11,18 +11,22 @@
> #include
>
> #ifdef CONFIG_OF
let's use IS_ENABLED() here
> -enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np);
> enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np);
> #else
> -static inline enum usb_phy_interface of_usb_get_phy_mode(struct device_node
> *np)
> +static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
> {
> - return USBPHY_INTERFACE_MODE_UNKNOWN;
> + return USB_DR_MODE_UNKNOWN;
> }
> +#endif
>
> -static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
> +#if defined(CONFIG_OF) && defined(CONFIG_USB_PHY)
and here
> +enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np);
> +#else
> +static inline enum usb_phy_interface of_usb_get_phy_mode(struct device_node
> *np)
> {
> - return USB_DR_MODE_UNKNOWN;
> + return USBPHY_INTERFACE_MODE_UNKNOWN;
> }
> +
> #endif
Also, why can't the same problem happen with of_usb_get_dr_mode() ??
--
balbi
signature.asc
Description: Digital signature
Re: [RFC PATCH v1 6/6] USB: EHCI: support running URB giveback in tasklet context
On Wed, Jun 19, 2013 at 12:55 AM, Alan Stern wrote: > On Tue, 18 Jun 2013, Ming Lei wrote: > >> Both 4 transfers can work well on EHCI HCD after switching to run > > "Both 4 transfers"? The word "both" applies when there are two items, > not more than two. And what are the four transfers you are referring > to? Do you mean all four transfer _types_? Yes, I mean the four transfer types, and "Both 4 transfers" should have been "all four transfer types", will fix it in v2. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
On Wed, Jun 19, 2013 at 12:51 AM, Alan Stern wrote: > On Tue, 18 Jun 2013, Ming Lei wrote: > >> Given interrupt URB will be resubmitted from tasklet context which >> is scheduled by ehci hardware interrupt handler, and commonly only >> one interrupt URB is scheduled on qh, so the qh may be unlinked >> immediately once qh_completions() returns from ehci_irq(), then >> the intr URB to be resubmitted in complete() can only be scheduled >> and linked to hardware until the qh unlink is completed. >> >> This patch applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh) >> state to improve this above situation, and the qh will wait for 5 >> milliseconds before being unlinked from hardware, if one URB is submitted >> during the period, the qh is move out of unlink wait state and the >> interrupt transfer can be scheduled immediately, not like before: the >> transfer is only linked to hardware until previous unlink is completed. >> >> Only enable the improvement in case that HCD supports to run >> giveback of URB in tasklet context. > > The approach used in this patch is wrong. You shouldn't start the > unlink, then wait, then finish the unlink. Consider what would happen What you mentioned above is just what the patch is doing, :-) start_unlink_intr() only sets the qh as QH_STATE_UNLINK_WAIT, puts the qh into one wait list and starts a timer, if it is expired the qh will be started to unlink, otherwise the qh can be recovered to QH_STATE_LINKED immediately if there is one URB submitted. So unlinking intr qh becomes a 3-stage process: - wait(qh return to linked state if URB is submitted during the period, otherwise go to start unlink) - start unlink(do unlink, and wait for end of unlink) - end unlink > if an URB were submitted during the delay: It would have to wait until If an URB were submitted during the delay, the previous wait is canceled immediately, and the qh state is recovered to linked state, please see cancel_unlink_wait_intr() called in intr_submit(). > the QH was completely unlinked. Instead, you should wait first, then > do the entire unlink. Yes, it is just what the patch is doing, :-) > > For example, scan_async() in ehci-q.c doesn't immediately begin to > unlink empty async QHs. It merely marks them as being empty and starts > a timer to check them later. It they are still empty at that point, > then they are unlinked. Yes, the patch starts to use QH_STATE_UNLINK_WAIT state for intr qh, and previously the state is only used by async qh. > > Also, it's silly to check whether or not giveback uses a tasklet. We > know that following the 6/6 patch it will. Even if it doesn't, there's > no harm in waiting a little while before unlinking an empty interrupt > QH. It is still for the test reason, since the patch may cause recursion if HCD_BH isn't set. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] USB: initialize or shutdown PHY when add or remove host controller
On Wed, Jun 19, 2013 at 10:48 AM, Greg KH wrote: > On Tue, Jun 18, 2013 at 10:31:20PM -0400, Chao Xie wrote: >> Some controller need software to initialize PHY before add >> host controller, and shut down PHY after remove host controller. >> Add the generic code for these controllers so they do not need >> do it in its own host controller driver. > > Why? What breaks if we add this patch, and what gets fixed? I'm > guessing you can then remove code? > > What out-of-tree code now works properly? Or gets broken? > > we need more info here please... > The patch does not fix any bug. Some echi-xxx driver will need initialize the phy before it do usb_add_hcd, and shut down phy after do usb_remove_hcd, and i did a patch for ehci-mv.c to do above thing. Alan and Felipe comments on my patch, and they think it is a peice of generic code, and it can be moved to hcd to handle it, so other ehci-xxx will not do the same thing again. So i add the patch to add the generic code in hcd to handle phy initialization and shut down. > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 4/6] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context
On Wed, Jun 19, 2013 at 12:43 AM, Alan Stern wrote: > On Tue, 18 Jun 2013, Ming Lei wrote: > >> If HCD_BH is set for HC driver's flags, URB giveback will be >> done in tasklet context instead of interrupt context, so the >> ehci->lock needn't to be released any more before calling >> usb_hcd_giveback_urb(). > > It is premature to do this now. This should be part of the 6/6 patch, It is fine since HCD_BH isn't enabled. > when it won't be necessary to test hcd_giveback_urb_in_bh(). Keeping it is easy to compare test results between running complete() in tasklet and in hard irq handler. Also it might be helpful for out of tree drivers. But it isn't a big deal, I can merge it to 6/6 if you care. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 3/6] USB: URB documentation: claim complete() may be run with IRQs enabled
On Wed, Jun 19, 2013 at 12:42 AM, Alan Stern wrote: > On Tue, 18 Jun 2013, Ming Lei wrote: > >> There is no good reason to run complete() in hard interrupt >> disabled context. >> >> After running complete() in tasklet, we will enable local IRQs >> when calling complete() since we can do it now. >> >> Even though we still disable IRQs now when calling complete() >> in tasklet, the URB documentation is updated to claim complete() >> may be run in tasklet context and local IRQs may be enabled, so >> that USB drivers can know the change and avoid one deadlock caused >> by: assume IRQs disabled in complete() and call spin_lock() to >> hold lock which might be acquired in interrupt context. >> >> Current spin_lock() usages in drivers' complete() will be cleaned >> up at the same time, and when the cleanup is finished, local IRQs >> will be enabled when calling complete() in tasklet. >> >> Also fix description about type of usb_complete_t. > >> @@ -210,12 +209,19 @@ have transferred successfully before the completion >> was called. >> >> >> NOTE: * WARNING * >> -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called >> -during hardware interrupt processing. If you can, defer substantial >> -work to a tasklet (bottom half) to keep system latencies low. You'll >> -probably need to use spinlocks to protect data structures you manipulate >> -in completion handlers. >> - >> +NEVER SLEEP IN A COMPLETION HANDLER. These are called during hardware >> +interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise >> +called in tasklet context. If you can, defer substantial work to a tasklet >> +(bottom half) to keep system latencies low. You'll probably need to use >> +spinlocks to protect data structures you manipulate in completion handlers. > > You shouldn't go into so much detail here, partly because it doesn't > matter for driver authors and partly because it is subject to change. > Just say that completion handlers are usually called in an atomic > context, so they must not sleep. Right. > > Also, the advice about deferring work to a tasklet seems out of place > now, since many completion handlers will already be running in a > tasklet. Good catch. > >> +Driver can't assume that local IRQs are disabled any more when running >> +complete(), for example, if drivers want to hold a lock which might be >> +acquired in hard interrupt handler, they have to use spin_lock_irqsave() >> +instead of spin_lock() to hold the lock. > > Don't say so much. Just mention that in the current kernel (3.10), > completion handlers always run with local interrupts disabled, but in > the future this is likely to change. Therefore drivers should not make > any assumptions. OK. > >> +In the future, all HCD might set HCD_BH to run complete() in tasklet so >> +that system latencies can be kept low. > > This does not need to be in the documentation. Right, since drivers don't care HCD. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()
On Wed, Jun 19, 2013 at 12:36 AM, Alan Stern wrote:
> On Tue, 18 Jun 2013, Ming Lei wrote:
>
>> We disable local IRQs here in case of running complete() by
>> tasklet to avoid possible deadlock because drivers may call
>> spin_lock() to hold lock which might be acquired in one hard
>> interrupt handler.
>>
>> The local_irq_save()/local_irq_restore() around complete()
>> will be removed if current USB drivers have been cleaned up
>> and no one may trigger the above deadlock situation when
>> running complete() in tasklet.
>
> This should be part of the first patch, not added on separately.
Yes, but I want to highlight the change since that will be removed
after drivers have been cleaned up.
>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index a272968..09a8263 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>>
>> /* pass ownership to the completion handler */
>> urb->status = status;
>> - urb->complete (urb);
>> +
>> + /*
>> + * We disable local IRQs here in case of running complete() by
>> + * tasklet to avoid possible deadlock because drivers may call
>> + * spin_lock() to hold lock which might be acquired in one hard
>> + * interrupt handler.
>> + *
>> + * The local_irq_save()/local_irq_restore() around complete()
>> + * will be removed if current USB drivers have been cleaned up
>> + * and no one may trigger the above deadlock situation when
>> + * running complete() in tasklet.
>> + */
>> + if (hcd_giveback_urb_in_bh(hcd)) {
>> + unsigned long flags;
>> +
>> + local_irq_save(flags);
>> + urb->complete (urb);
>> + local_irq_restore(flags);
>> + } else {
>> + urb->complete (urb);
>> + }
>> +
>
> There's no reason to bother with the test. You might as well always do
> the local_irq_save.
Looks fine.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context
On Wed, Jun 19, 2013 at 12:05 AM, Alan Stern wrote:
> On Tue, 18 Jun 2013, Ming Lei wrote:
>
>> This patch implements the mechanism of giveback of URB in
>> tasklet context, so that hardware interrupt handling time for
>> usb host controller can be saved much, and HCD interrupt handling
>> can be simplified.
>
> Okay. I'd make a few relatively minor changes.
>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 014dc99..a272968 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -699,9 +699,11 @@ error:
>>* Avoiding calls to local_irq_disable/enable makes the code
>>* RT-friendly.
>>*/
>> - spin_unlock(&hcd_root_hub_lock);
>> + if (!hcd_giveback_urb_in_bh(hcd))
>> + spin_unlock(&hcd_root_hub_lock);
>> usb_hcd_giveback_urb(hcd, urb, status);
>> - spin_lock(&hcd_root_hub_lock);
>> + if (!hcd_giveback_urb_in_bh(hcd))
>> + spin_lock(&hcd_root_hub_lock);
>>
>> spin_unlock_irq(&hcd_root_hub_lock);
>> return 0;
>> @@ -742,9 +744,11 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
>> memcpy(urb->transfer_buffer, buffer, length);
>>
>> usb_hcd_unlink_urb_from_ep(hcd, urb);
>> - spin_unlock(&hcd_root_hub_lock);
>> + if (!hcd_giveback_urb_in_bh(hcd))
>> + spin_unlock(&hcd_root_hub_lock);
>> usb_hcd_giveback_urb(hcd, urb, 0);
>> - spin_lock(&hcd_root_hub_lock);
>> + if (!hcd_giveback_urb_in_bh(hcd))
>> + spin_lock(&hcd_root_hub_lock);
>> } else {
>> length = 0;
>> set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags);
>> @@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd,
>> struct urb *urb, int status)
>> hcd->status_urb = NULL;
>> usb_hcd_unlink_urb_from_ep(hcd, urb);
>>
>> - spin_unlock(&hcd_root_hub_lock);
>> + if (!hcd_giveback_urb_in_bh(hcd))
>> + spin_unlock(&hcd_root_hub_lock);
>> usb_hcd_giveback_urb(hcd, urb, status);
>> - spin_lock(&hcd_root_hub_lock);
>> + if (!hcd_giveback_urb_in_bh(hcd))
>> + spin_lock(&hcd_root_hub_lock);
>> }
>> }
>> done:
>
> None of these tests are necessary. Root hubs are different from normal
> devices; their URBs are handled mostly by usbcore. The only part done
> by the HCD is always synchronous. And we know that root-hub URBs
Looks not always synchronous, control transfer is synchronous, and
interrupt transfer is still asynchronous. No drivers(hub, usbfs) depend
on that, and IMO, treating root hub same as hub will simplify HCD core,
and finally we can remove all the above lock releasing & acquiring if
all HCDs set HCD_BH.
Also there is very less roothub transfers and always letting tasklet
handle URB giveback of roothub won't have performance problem, so
how about keeping the above tests?
> always have a very short completion handler. So we may as well
root hub's completion handler is same with hub's, and usb_submit_urb()
is still called to submit new schedule.
> continue to handle root-hub URBs the way we always have.
>
>> @@ -1648,6 +1654,60 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
>>
>>
>> /*-*/
>>
>> +static void __usb_hcd_giveback_urb(struct urb *urb)
>> +{
>> + struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
>> + int status = urb->status;
>> +
>> + urb->hcpriv = NULL;
>> + if (unlikely(urb->unlinked))
>> + status = urb->unlinked;
>
> The way you're storing the status value isn't good. We decided to make
> the status a separate argument because of drivers that abuse the
> urb->status field (they poll it instead of waiting for the completion
> handler to be called). Hopefully there aren't any examples of drivers
> still doing this, but...
>
> This means you shouldn't store anything in urb->status until just
> before calling the completion handler. What you can do instead is
> always store the status value in urb->unlinked.
Good point, will do it.
>
>> + else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
>> + urb->actual_length < urb->transfer_buffer_length &&
>> + !status))
>> + status = -EREMOTEIO;
>> +
>> + unmap_urb_for_dma(hcd, urb);
>> + usbmon_urb_complete(&hcd->self, urb, status);
>> + usb_unanchor_urb(urb);
>> +
>> + /* pass ownership to the completion handler */
>> + urb->status = status;
>> + urb->complete (urb);
>
> You are supposed to disable local interrupts around the call to the
> completion handler.
I did it in the 2nd
Re: [PATCH V2] USB: initialize or shutdown PHY when add or remove host controller
On Tue, Jun 18, 2013 at 10:31:20PM -0400, Chao Xie wrote: > Some controller need software to initialize PHY before add > host controller, and shut down PHY after remove host controller. > Add the generic code for these controllers so they do not need > do it in its own host controller driver. Why? What breaks if we add this patch, and what gets fixed? I'm guessing you can then remove code? What out-of-tree code now works properly? Or gets broken? we need more info here please... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] USB: initialize or shutdown PHY when add or remove host controller
Some controller need software to initialize PHY before add
host controller, and shut down PHY after remove host controller.
Add the generic code for these controllers so they do not need
do it in its own host controller driver.
Signed-off-by: Chao Xie
---
drivers/usb/core/hcd.c | 24 +++-
1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..301c639 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -40,9 +40,11 @@
#include
#include
#include
+#include
#include
#include
+#include
#include "usb.h"
@@ -2531,12 +2533,26 @@ int usb_add_hcd(struct usb_hcd *hcd,
*/
set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
+ /* In case hcd->phy contains the error code. */
+ if (IS_ERR(hcd->phy))
+ hcd->phy = NULL;
+
+ /* Initialize the PHY before other hardware operation. */
+ if (hcd->phy) {
+ retval = usb_phy_init(hcd->phy);
+ if (retval) {
+ dev_err(hcd->self.controller,
+ "can't initialize phy\n");
+ goto err_hcd_driver_setup;
+ }
+ }
+
/* "reset" is misnamed; its role is now one-time init. the controller
* should already have been reset (and boot firmware kicked off etc).
*/
if (hcd->driver->reset && (retval = hcd->driver->reset(hcd)) < 0) {
dev_err(hcd->self.controller, "can't setup\n");
- goto err_hcd_driver_setup;
+ goto err_hcd_driver_init_phy;
}
hcd->rh_pollable = 1;
@@ -2608,6 +2624,9 @@ err_hcd_driver_start:
if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
free_irq(irqnum, hcd);
err_request_irq:
+err_hcd_driver_init_phy:
+ if (hcd->phy)
+ usb_phy_shutdown(hcd->phy);
err_hcd_driver_setup:
err_set_rh_speed:
usb_put_dev(hcd->self.root_hub);
@@ -2674,6 +2693,9 @@ void usb_remove_hcd(struct usb_hcd *hcd)
free_irq(hcd->irq, hcd);
}
+ if (hcd->phy)
+ usb_phy_shutdown(hcd->phy);
+
usb_put_dev(hcd->self.root_hub);
usb_deregister_bus(&hcd->self);
hcd_buffer_destroy(hcd);
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] usb: fix build error without CONFIG_USB_PHY
on i386:
drivers/built-in.o: In function `ci_hdrc_probe':
core.c:(.text+0x20446b): undefined reference to `of_usb_get_phy_mode'
Signed-off-by: Peter Chen
---
include/linux/usb/of.h | 14 +-
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
index e460a24..a0aa1c8 100644
--- a/include/linux/usb/of.h
+++ b/include/linux/usb/of.h
@@ -11,18 +11,22 @@
#include
#ifdef CONFIG_OF
-enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np);
enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np);
#else
-static inline enum usb_phy_interface of_usb_get_phy_mode(struct device_node
*np)
+static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
{
- return USBPHY_INTERFACE_MODE_UNKNOWN;
+ return USB_DR_MODE_UNKNOWN;
}
+#endif
-static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
+#if defined(CONFIG_OF) && defined(CONFIG_USB_PHY)
+enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np);
+#else
+static inline enum usb_phy_interface of_usb_get_phy_mode(struct device_node
*np)
{
- return USB_DR_MODE_UNKNOWN;
+ return USBPHY_INTERFACE_MODE_UNKNOWN;
}
+
#endif
#endif /* __LINUX_USB_OF_H */
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: Tree for Jun 18 usb/chipidea)
On Wed, Jun 19, 2013 at 4:12 AM, Felipe Balbi wrote: > Hi, > > On Tue, Jun 18, 2013 at 11:52:36AM -0700, Randy Dunlap wrote: >> On 06/18/13 00:30, Stephen Rothwell wrote: >> > Hi all, >> > >> > Changes since 20130617: >> > >> >> >> on i386: >> >> # CONFIG_USB_PHY is not set >> >> drivers/built-in.o: In function `ci_hdrc_probe': >> core.c:(.text+0x20446b): undefined reference to `of_usb_get_phy_mode' >> >> >> chipidea needs to depend on or select some kind of USB_PHY support...? > > hmm, looks like a missing stub to me. Alex ? > > -- > balbi Seems i386 chooses CONFIG_OF, but not CONFIG_USB_PHY. I will send a patch to fix it. -- BR, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Bug] USB 2.0 Ports Dont Work on Sony Vaio Laptop
Hi, Recently, there is one bug report from Ubuntu community: USB 2.0 Ports Dont Work on Sony Vaio Laptop and the problem exists on upstream kernel too. The built-in two USB 2.0 devices can be recognized correctly, but external devices can't be recognized when the device is connected to USB 2.0 port. Also, the USB 2.0 bus can be waken up successfully via /sys/bus/devices/.../power/control, but still can't find external connected devices, and no any 'dmesg' log with CONFIG_USB_DEBUG enabled after the device is plugged into 2.0 ports. See detailed log in below link: https://bugs.launchpad.net/bugs/1172908 No such problem on Windows. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi Kishon,
I've noticed there is a little inconsistency between the code and
documentation.
On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote:
+3. Creating the PHY
+
+The PHY driver should create the PHY in order for other peripheral controllers
+to make use of it. The PHY framework provides 2 APIs to create the PHY.
+
+struct phy *phy_create(struct device *dev, int id, const struct phy_ops *ops,
+ void *priv);
+struct phy *devm_phy_create(struct device *dev, int id,
+ const struct phy_ops *ops, void *priv);
The 'label' argument is missing in those function prototypes. It seems the
labels must be unique, I guess this needs to be documented. And do we allow
NULL labels ? If so, then there is probably a check for NULL label needed
in phy_lookup() and if not, then phy_create() should fail when NULL label
is passed ?
+The PHY drivers can use one of the above 2 APIs to create the PHY by passing
+the device pointer, id, phy ops and a driver data.
+phy_ops is a set of function pointers for performing PHY operations such as
+init, exit, power_on and power_off.
+/**
+ * phy_create() - create a new phy
+ * @dev: device that is creating the new phy
+ * @id: id of the phy
+ * @ops: function pointers for performing phy operations
+ * @label: label given to the phy
+ * @priv: private data from phy driver
+ *
+ * Called to create a phy using phy framework.
+ */
+struct phy *phy_create(struct device *dev, u8 id, const struct phy_ops *ops,
+ const char *label, void *priv)
+{
+ int ret;
+ struct phy *phy;
+
+ if (!dev) {
+ dev_WARN(dev, "no device provided for PHY\n");
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ phy = kzalloc(sizeof(*phy), GFP_KERNEL);
+ if (!phy) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ device_initialize(&phy->dev);
+
+ phy->dev.class = phy_class;
+ phy->dev.parent = dev;
+ phy->dev.of_node = dev->of_node;
+ phy->id = id;
+ phy->ops = ops;
+ phy->label = label;
+
+ dev_set_drvdata(&phy->dev, priv);
+
+ ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
+ if (ret)
+ goto err1;
+
+ ret = device_add(&phy->dev);
+ if (ret)
+ goto err1;
+
+ if (pm_runtime_enabled(dev))
+ pm_runtime_enable(&phy->dev);
+
+ return phy;
+
+err1:
+ put_device(&phy->dev);
+ kfree(phy);
+
+err0:
+ return ERR_PTR(ret);
+}
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] xhci: Compute last_ctx from complete set of configured endpoints.
The context entries field of the slot context must be set to one more
than the highest endpoint index currently active. The previous logic
only included the set of endpoints currently being added, meaning that
if an endpoint where dropped then the field would be reset to 1,
deactivating all configured endpoints.
The xHCI spec is decidedly unclear on whether this field includes all
configured endpoints or only those being modified by a configure
endpoint command. My interpretation is the former as when the xHC writes
changes into the output device context array it must know how large it
is. It does not make sense for this field to only refer to the input
context.
---
drivers/usb/host/xhci.c | 52 +
1 file changed, 18 insertions(+), 34 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d8f640b..aa117d1 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1571,12 +1571,10 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
struct xhci_hcd *xhci;
struct xhci_container_ctx *in_ctx, *out_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
- struct xhci_slot_ctx *slot_ctx;
- unsigned int last_ctx;
unsigned int ep_index;
struct xhci_ep_ctx *ep_ctx;
u32 drop_flag;
- u32 new_add_flags, new_drop_flags, new_slot_info;
+ u32 new_add_flags, new_drop_flags;
int ret;
ret = xhci_check_args(hcd, udev, ep, 1, true, __func__);
@@ -1617,24 +1615,13 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
ctrl_ctx->add_flags &= cpu_to_le32(~drop_flag);
new_add_flags = le32_to_cpu(ctrl_ctx->add_flags);
- last_ctx = xhci_last_valid_endpoint(le32_to_cpu(ctrl_ctx->add_flags));
- slot_ctx = xhci_get_slot_ctx(xhci, in_ctx);
- /* Update the last valid endpoint context, if we deleted the last one */
- if ((le32_to_cpu(slot_ctx->dev_info) & LAST_CTX_MASK) >
- LAST_CTX(last_ctx)) {
- slot_ctx->dev_info &= cpu_to_le32(~LAST_CTX_MASK);
- slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(last_ctx));
- }
- new_slot_info = le32_to_cpu(slot_ctx->dev_info);
-
xhci_endpoint_zero(xhci, xhci->devs[udev->slot_id], ep);
- xhci_dbg(xhci, "drop ep 0x%x, slot id %d, new drop flags = %#x, new add
flags = %#x, new slot info = %#x\n",
+ xhci_dbg(xhci, "drop ep 0x%x, slot id %d, new drop flags = %#x, new add
flags = %#x\n",
(unsigned int) ep->desc.bEndpointAddress,
udev->slot_id,
(unsigned int) new_drop_flags,
- (unsigned int) new_add_flags,
- (unsigned int) new_slot_info);
+ (unsigned int) new_add_flags);
return 0;
}
@@ -1657,11 +1644,9 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
struct xhci_hcd *xhci;
struct xhci_container_ctx *in_ctx, *out_ctx;
unsigned int ep_index;
- struct xhci_slot_ctx *slot_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
u32 added_ctxs;
- unsigned int last_ctx;
- u32 new_add_flags, new_drop_flags, new_slot_info;
+ u32 new_add_flags, new_drop_flags;
struct xhci_virt_device *virt_dev;
int ret = 0;
@@ -1676,7 +1661,6 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
return -ENODEV;
added_ctxs = xhci_get_endpoint_flag(&ep->desc);
- last_ctx = xhci_last_valid_endpoint(added_ctxs);
if (added_ctxs == SLOT_FLAG || added_ctxs == EP0_FLAG) {
/* FIXME when we have to issue an evaluate endpoint command to
* deal with ep0 max packet size changing once we get the
@@ -1737,24 +1721,14 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
*/
new_drop_flags = le32_to_cpu(ctrl_ctx->drop_flags);
- slot_ctx = xhci_get_slot_ctx(xhci, in_ctx);
- /* Update the last valid endpoint context, if we just added one past */
- if ((le32_to_cpu(slot_ctx->dev_info) & LAST_CTX_MASK) <
- LAST_CTX(last_ctx)) {
- slot_ctx->dev_info &= cpu_to_le32(~LAST_CTX_MASK);
- slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(last_ctx));
- }
- new_slot_info = le32_to_cpu(slot_ctx->dev_info);
-
/* Store the usb_device pointer for later use */
ep->hcpriv = udev;
- xhci_dbg(xhci, "add ep 0x%x, slot id %d, new drop flags = %#x, new add
flags = %#x, new slot info = %#x\n",
+ xhci_dbg(xhci, "add ep 0x%x, slot id %d, new drop flags = %#x, new add
flags = %#x\n",
(unsigned int) ep->desc.bEndpointAddress,
udev->slot_id,
(unsigned int) new_drop_flags,
- (unsigned int) new_add_flags,
-
Re: linux-next: Tree for Jun 18 usb/chipidea)
Hi, On Tue, Jun 18, 2013 at 11:52:36AM -0700, Randy Dunlap wrote: > On 06/18/13 00:30, Stephen Rothwell wrote: > > Hi all, > > > > Changes since 20130617: > > > > > on i386: > > # CONFIG_USB_PHY is not set > > drivers/built-in.o: In function `ci_hdrc_probe': > core.c:(.text+0x20446b): undefined reference to `of_usb_get_phy_mode' > > > chipidea needs to depend on or select some kind of USB_PHY support...? hmm, looks like a missing stub to me. Alex ? -- balbi signature.asc Description: Digital signature
Re: Crash in xhci_hcd for 3.10-rc5 (old 3.5.0 bug)
On Wed, Jun 12, 2013 at 9:25 PM, Jérôme Poulin wrote: > > That seems to make sense, I don't have my laptop with me but I can say I > didn't try. After checking it out, UAS does not seem to be compiled in nor as a module on my current kernel. A Ubuntu bug was already open on this matter: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1154739 this problem seems to be existing since 3.5 from this bug report. Maybe it only happens with specific equipments. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: Tree for Jun 18 usb/chipidea)
On 06/18/13 00:30, Stephen Rothwell wrote: > Hi all, > > Changes since 20130617: > on i386: # CONFIG_USB_PHY is not set drivers/built-in.o: In function `ci_hdrc_probe': core.c:(.text+0x20446b): undefined reference to `of_usb_get_phy_mode' chipidea needs to depend on or select some kind of USB_PHY support...? -- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] add entries in Documentation/ABI for new wusbhc sysfs attributes
Add documenation for new ABI entries. Signed-off-by: Thomas Pugliese --- Documentation/ABI/testing/sysfs-class-uwb_rc-wusbhc | 19 +++ 1 file changed, 19 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-class-uwb_rc-wusbhc b/Documentation/ABI/testing/sysfs-class-uwb_rc-wusbhc index 25b1e75..5977e28 100644 --- a/Documentation/ABI/testing/sysfs-class-uwb_rc-wusbhc +++ b/Documentation/ABI/testing/sysfs-class-uwb_rc-wusbhc @@ -36,3 +36,22 @@ Description: Refer to [ECMA-368] section 10.3.1.1 for the value to use. + +What: /sys/class/uwb_rc/uwbN/wusbhc/wusb_dnts +Date: June 2013 +KernelVersion: 3.11 +Contact:Thomas Pugliese +Description: +The device notification time slot (DNTS) count and inverval in +milliseconds that the WUSB host should use. This controls how +often the devices will have the opportunity to send +notifications to the host. + +What: /sys/class/uwb_rc/uwbN/wusbhc/wusb_retry_count +Date: June 2013 +KernelVersion: 3.11 +Contact:Thomas Pugliese +Description: +The number of retries that the WUSB host should attempt +before reporting an error for a bus transaction. The range of +valid values is [0..15], where 0 indicates infinite retries. -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] USB: wusbcore: add sysfs attributes for HC parameters
This patch set adds sysfs attributes to the wusbhc for DNTS count and interval and the host controller transaction retry count. Thomas Pugliese (3): add sysfs attribute for DNTS count and interval add sysfs attribute for retry count add entries in Documentation/ABI for new wusbhc sysfs attributes .../ABI/testing/sysfs-class-uwb_rc-wusbhc | 19 ++ drivers/usb/wusbcore/mmc.c |6 +- drivers/usb/wusbcore/wa-rpipe.c|3 +- drivers/usb/wusbcore/wusbhc.c | 65 drivers/usb/wusbcore/wusbhc.h |5 ++ 5 files changed, 93 insertions(+), 5 deletions(-) -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/6] USB: OHCI: make ohci-omap3 a separate driver
On Wed, 12 Jun 2013, Manjunath Goudar wrote:
> Separate the TI OHCI OMAP3 host controller driver from ohci-hcd
> host code so that it can be built as a separate driver module.
> This work is part of enabling multi-platform kernels on ARM;
> it would be nice to have in 3.11.
>
> V2:
> -ohci_setup() removed because it is called in .reset member
> of the ohci_hc_driver structure.
> -The improper multi-line commenting style written in proper way.
> ('*' characters aligned in vertically).
Generally okay, just one problem...
> @@ -185,7 +118,14 @@ static int ohci_hcd_omap3_probe(struct platform_device
> *pdev)
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
>
> - ohci_hcd_init(hcd_to_ohci(hcd));
> + ohci = hcd_to_ohci(hcd);
> + /*
> + * RemoteWakeupConnected has to be set explicitly before
> + * calling ohci_run. The reset value of RWC is 0.
> + */
> + ohci->hc_control = OHCI_CTRL_RWC;
> + writel(OHCI_CTRL_RWC, &ohci->regs->control);
ohci->regs doesn't get set until usb_add_hcd, so this line must not
appear here. You can simply remove it.
> + dev_dbg(hcd->self.controller, "starting OHCI controller\n");
>
> ret = usb_add_hcd(hcd, irq, 0);
> if (ret) {
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] add sysfs attribute for DNTS count and interval
This patch adds a sysfs attribute for the wireless USB host controller
device notification transmit slot(DNTS) count and interval. It also
changes the defaults from 16 slots in every MMC to a more reasonable 4
slots every 2ms.
Signed-off-by: Thomas Pugliese
---
drivers/usb/wusbcore/mmc.c|6 +++---
drivers/usb/wusbcore/wusbhc.c | 34 ++
drivers/usb/wusbcore/wusbhc.h |2 ++
3 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/wusbcore/mmc.c b/drivers/usb/wusbcore/mmc.c
index b8c7258..021467f 100644
--- a/drivers/usb/wusbcore/mmc.c
+++ b/drivers/usb/wusbcore/mmc.c
@@ -214,9 +214,9 @@ int wusbhc_start(struct wusbhc *wusbhc)
dev_err(dev, "error starting security in the HC: %d\n", result);
goto error_sec_start;
}
- /* FIXME: the choice of the DNTS parameters is somewhat
-* arbitrary */
- result = wusbhc->set_num_dnts(wusbhc, 0, 15);
+
+ result = wusbhc->set_num_dnts(wusbhc, wusbhc->dnts_interval,
+ wusbhc->dnts_num_slots);
if (result < 0) {
dev_err(dev, "Cannot set DNTS parameters: %d\n", result);
goto error_set_num_dnts;
diff --git a/drivers/usb/wusbcore/wusbhc.c b/drivers/usb/wusbcore/wusbhc.c
index c35ee43..8759aa6 100644
--- a/drivers/usb/wusbcore/wusbhc.c
+++ b/drivers/usb/wusbcore/wusbhc.c
@@ -175,11 +175,42 @@ static ssize_t wusb_phy_rate_store(struct device *dev,
}
static DEVICE_ATTR(wusb_phy_rate, 0644, wusb_phy_rate_show,
wusb_phy_rate_store);
+static ssize_t wusb_dnts_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct wusbhc *wusbhc = usbhc_dev_to_wusbhc(dev);
+
+ return sprintf(buf, "num slots: %d\ninterval: %dms\n",
+ wusbhc->dnts_num_slots, wusbhc->dnts_interval);
+}
+
+static ssize_t wusb_dnts_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ struct wusbhc *wusbhc = usbhc_dev_to_wusbhc(dev);
+ uint8_t num_slots, interval;
+ ssize_t result;
+
+ result = sscanf(buf, "%hhu %hhu", &num_slots, &interval);
+
+ if (result != 2)
+ return -EINVAL;
+
+ wusbhc->dnts_num_slots = num_slots;
+ wusbhc->dnts_interval = interval;
+
+ return size;
+}
+static DEVICE_ATTR(wusb_dnts, 0644, wusb_dnts_show, wusb_dnts_store);
+
/* Group all the WUSBHC attributes */
static struct attribute *wusbhc_attrs[] = {
&dev_attr_wusb_trust_timeout.attr,
&dev_attr_wusb_chid.attr,
&dev_attr_wusb_phy_rate.attr,
+ &dev_attr_wusb_dnts.attr,
NULL,
};
@@ -205,8 +236,11 @@ int wusbhc_create(struct wusbhc *wusbhc)
{
int result = 0;
+ /* set defaults. These can be overwritten using sysfs attributes. */
wusbhc->trust_timeout = WUSB_TRUST_TIMEOUT_MS;
wusbhc->phy_rate = UWB_PHY_RATE_INVALID - 1;
+ wusbhc->dnts_num_slots = 4;
+ wusbhc->dnts_interval = 2;
mutex_init(&wusbhc->mutex);
result = wusbhc_mmcie_create(wusbhc);
diff --git a/drivers/usb/wusbcore/wusbhc.h b/drivers/usb/wusbcore/wusbhc.h
index b4a4fa7..a7069f4 100644
--- a/drivers/usb/wusbcore/wusbhc.h
+++ b/drivers/usb/wusbcore/wusbhc.h
@@ -252,6 +252,8 @@ struct wusbhc {
unsigned trust_timeout; /* in jiffies */
struct wusb_ckhdid chid;
uint8_t phy_rate;
+ uint8_t dnts_num_slots;
+ uint8_t dnts_interval;
struct wuie_host_info *wuie_host_info;
struct mutex mutex; /* locks everything else */
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] add sysfs attribute for retry count
This patch adds a sysfs attribute for the wireless host controller
transaction retry count. It also changes the default value from 15
retries to infinite retries because the driver currently does not handle
retry errors gracefully.
Signed-off-by: Thomas Pugliese
---
drivers/usb/wusbcore/wa-rpipe.c |3 +--
drivers/usb/wusbcore/wusbhc.c | 31 +++
drivers/usb/wusbcore/wusbhc.h |3 +++
3 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/wusbcore/wa-rpipe.c b/drivers/usb/wusbcore/wa-rpipe.c
index 9429c12..9a595c1 100644
--- a/drivers/usb/wusbcore/wa-rpipe.c
+++ b/drivers/usb/wusbcore/wa-rpipe.c
@@ -367,8 +367,7 @@ static int rpipe_aim(struct wa_rpipe *rpipe, struct wahc
*wa,
rpipe->descr.bmAttribute = (ep->desc.bmAttributes &
USB_ENDPOINT_XFERTYPE_MASK);
/* rpipe->descr.bmCharacteristics RO */
- /* FIXME: bmRetryOptions */
- rpipe->descr.bmRetryOptions = 15;
+ rpipe->descr.bmRetryOptions = (wa->wusb->retry_count & 0xF);
/* FIXME: use for assessing link quality? */
rpipe->descr.wNumTransactionErrors = 0;
result = __rpipe_set_descr(wa, &rpipe->descr,
diff --git a/drivers/usb/wusbcore/wusbhc.c b/drivers/usb/wusbcore/wusbhc.c
index 8759aa6..e712af3 100644
--- a/drivers/usb/wusbcore/wusbhc.c
+++ b/drivers/usb/wusbcore/wusbhc.c
@@ -205,12 +205,42 @@ static ssize_t wusb_dnts_store(struct device *dev,
}
static DEVICE_ATTR(wusb_dnts, 0644, wusb_dnts_show, wusb_dnts_store);
+static ssize_t wusb_retry_count_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct wusbhc *wusbhc = usbhc_dev_to_wusbhc(dev);
+
+ return sprintf(buf, "%d\n", wusbhc->retry_count);
+}
+
+static ssize_t wusb_retry_count_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ struct wusbhc *wusbhc = usbhc_dev_to_wusbhc(dev);
+ uint8_t retry_count;
+ ssize_t result;
+
+ result = sscanf(buf, "%hhu", &retry_count);
+
+ if (result != 1)
+ return -EINVAL;
+
+ wusbhc->retry_count = max_t(uint8_t, retry_count, WUSB_RETRY_COUNT_MAX);
+
+ return size;
+}
+static DEVICE_ATTR(wusb_retry_count, 0644, wusb_retry_count_show,
+ wusb_retry_count_store);
+
/* Group all the WUSBHC attributes */
static struct attribute *wusbhc_attrs[] = {
&dev_attr_wusb_trust_timeout.attr,
&dev_attr_wusb_chid.attr,
&dev_attr_wusb_phy_rate.attr,
&dev_attr_wusb_dnts.attr,
+ &dev_attr_wusb_retry_count.attr,
NULL,
};
@@ -241,6 +271,7 @@ int wusbhc_create(struct wusbhc *wusbhc)
wusbhc->phy_rate = UWB_PHY_RATE_INVALID - 1;
wusbhc->dnts_num_slots = 4;
wusbhc->dnts_interval = 2;
+ wusbhc->retry_count = WUSB_RETRY_COUNT_INFINITE;
mutex_init(&wusbhc->mutex);
result = wusbhc_mmcie_create(wusbhc);
diff --git a/drivers/usb/wusbcore/wusbhc.h b/drivers/usb/wusbcore/wusbhc.h
index a7069f4..711b195 100644
--- a/drivers/usb/wusbcore/wusbhc.h
+++ b/drivers/usb/wusbcore/wusbhc.h
@@ -69,6 +69,8 @@
* zone 0.
*/
#define WUSB_CHANNEL_STOP_DELAY_MS 8
+#define WUSB_RETRY_COUNT_MAX 15
+#define WUSB_RETRY_COUNT_INFINITE 0
/**
* Wireless USB device
@@ -254,6 +256,7 @@ struct wusbhc {
uint8_t phy_rate;
uint8_t dnts_num_slots;
uint8_t dnts_interval;
+ uint8_t retry_count;
struct wuie_host_info *wuie_host_info;
struct mutex mutex; /* locks everything else */
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 4/6] USB: OHCI: make ohci-spear a separate driver
On Wed, 12 Jun 2013, Manjunath Goudar wrote: > Separate the ST OHCI SPEAr host controller driver from ohci-hcd > host code so that it can be built as a separate driver module. > This work is part of enabling multi-platform kernels on ARM; > it would be nice to have in 3.11. > > V2: > -ohci_setup() removed because it is called in .reset member > of the ohci_hc_driver structure. > -debugging stuff isn't needed any more that's what removed. Acked-by: Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/6] USB: OHCI: make ohci-omap a separate driver
On Wed, 12 Jun 2013, Manjunath Goudar wrote:
> Separate the TI OHCI OMAP1/2 host controller driver from ohci-hcd
> host code so that it can be built as a separate driver module.
> This work is part of enabling multi-platform kernels on ARM;
> it would be nice to have in 3.11.
>
> V2:
> -omap_ohci_clock_power(0) called in usb_hcd_omap_remove().
> -Removed ohci_setup() call from usb_hcd_omap_probe() and ohci_omap_reset().
> -host_enabled and host_initialized variables aren't used for anything
> thats what removed.
There's one more thing I just noticed.
> @@ -188,21 +195,21 @@ static void start_hnp(struct ohci_hcd *ohci)
>
> /*-*/
>
> -static int ohci_omap_init(struct usb_hcd *hcd)
> +static int ohci_omap_reset(struct usb_hcd *hcd)
> {
> struct ohci_hcd *ohci = hcd_to_ohci(hcd);
> struct omap_usb_config *config = hcd->self.controller->platform_data;
> int need_transceiver = (config->otg != 0);
> - int ret;
>
> dev_dbg(hcd->self.controller, "starting USB Controller\n");
>
> - if (config->otg) {
> + if (config->otg || config->rwc) {
> ohci_to_hcd(ohci)->self.otg_port = config->otg;
> /* default/minimum OTG power budget: 8 mA */
> ohci_to_hcd(ohci)->power_budget = 8;
> + ohci->hc_control = OHCI_CTRL_RWC;
> + writel(OHCI_CTRL_RWC, &ohci->regs->control);
This last line must not appear here, because ohci->regs doesn't get set
until ohci_setup() calls ohci_init(). Removing it entirely is safe
because ohci_run() does the same thing later on. Or you can move both
of these last two lines after the call to ohci_setup().
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 0/1] Fix xHCI NULL pointer deref
On Mon, Jun 17, 2013 at 08:05:03PM -0700, Sarah Sharp wrote: > On Mon, Jun 17, 2013 at 10:41:56AM -0700, Greg Kroah-Hartman wrote: > > On Mon, Jun 17, 2013 at 09:56:30AM -0700, Sarah Sharp wrote: > > > This patchset breaks out the one real bug fix patch from the Klockwork > > > "security issues" patchset. It should be applied to usb-linus and > > > queued for stable, as it handles a real potential NULL pointer > > > deference. > > > > But, given that if we are under this type of memory pressure, lots of > > things will be dying, it can wait for 3.11-rc1, and is not a regression > > fix that has to go into 3.10. > > > > Do you still want me to pull this, as you marked it "RFC"? > > Sure, it can wait until 3.11. If you want to just pull it, go ahead. > However, the patch is against usb-linus, so I'm not sure how it will > merge. If it doesn't merge, let me know, and I'll send you an updated > pull request. It merged just fine, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Pull Request] xhci: Remove BUG() calls from the driver
On Mon, Jun 17, 2013 at 07:57:52PM -0700, Sarah Sharp wrote: > The following changes since commit 976f8bef9cfb5246bc0e8dc781562daa79cb7aaf: > > Merge tag 'usb-for-v3.11' of > git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb into usb-next > (2013-06-12 14:44:13 -0700) > > are available in the git repository at: > > > git://git.kernel.org/pub/scm/linux/kernel/git/sarah/xhci.git > for-usb-next-2013-06-17 Pulled and pushed out, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/6] USB: OHCI: make ohci-omap a separate driver
On Wed, 12 Jun 2013, Manjunath Goudar wrote:
> Separate the TI OHCI OMAP1/2 host controller driver from ohci-hcd
> host code so that it can be built as a separate driver module.
> This work is part of enabling multi-platform kernels on ARM;
> it would be nice to have in 3.11.
>
> V2:
> -omap_ohci_clock_power(0) called in usb_hcd_omap_remove().
> -Removed ohci_setup() call from usb_hcd_omap_probe() and ohci_omap_reset().
You were supposed to remove the call from usb_hcd_omap_probe but leave
in the call from ohci_omap_reset.
> -host_enabled and host_initialized variables aren't used for anything
> thats what removed.
> @@ -188,21 +195,21 @@ static void start_hnp(struct ohci_hcd *ohci)
>
> /*-*/
>
> -static int ohci_omap_init(struct usb_hcd *hcd)
> +static int ohci_omap_reset(struct usb_hcd *hcd)
> {
> struct ohci_hcd *ohci = hcd_to_ohci(hcd);
> struct omap_usb_config *config = hcd->self.controller->platform_data;
> int need_transceiver = (config->otg != 0);
> - int ret;
>
> dev_dbg(hcd->self.controller, "starting USB Controller\n");
>
> - if (config->otg) {
> + if (config->otg || config->rwc) {
> ohci_to_hcd(ohci)->self.otg_port = config->otg;
> /* default/minimum OTG power budget: 8 mA */
> ohci_to_hcd(ohci)->power_budget = 8;
These three lines are supposed to run only when config->otg is set, not
when config->rwc is set. The next two lines can run when either one is
set.
Also, "ohci_to_hcd(ohci)" should be replaced by "hcd".
> + ohci->hc_control = OHCI_CTRL_RWC;
> + writel(OHCI_CTRL_RWC, &ohci->regs->control);
> }
> -
Don't remove this blank line.
> /* boards can use OTG transceivers in non-OTG modes */
> need_transceiver = need_transceiver
> || machine_is_omap_h2() || machine_is_omap_h3();
> @@ -238,9 +245,6 @@ static int ohci_omap_init(struct usb_hcd *hcd)
> omap_1510_local_bus_init();
> }
>
> - if ((ret = ohci_init(ohci)) < 0)
> - return ret;
> -
This should call ohci_setup instead of ohci_init.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 1/6] USB: OHCI: make ohci-exynos a separate driver
On Wed, 12 Jun 2013, Manjunath Goudar wrote: > Separate the Samsung OHCI EXYNOS host controller driver from ohci-hcd > host code so that it can be built as a separate driver module. > This work is part of enabling multi-platform kernels on ARM; > it would be nice to have in 3.11. > > V2: > -exynos_ohci_hcd structure assignment error fixed. > -Removed multiple usb_create_hcd() from prob funtion. > -platform_set_drvdata() called before exynos_ohci_phy_enable(). > -ohci_setup() removed because it is called in .reset member > of the ohci_hc_driver structure > --- a/drivers/usb/host/ohci-exynos.c > +++ b/drivers/usb/host/ohci-exynos.c > @@ -12,24 +12,39 @@ > */ > > #include > +#include > +#include > +#include > +#include > #include > #include > #include > #include > #include > +#include > +#include > +#include > + > +#include "ohci.h" > + > +#define DRIVER_DESC "OHCI exynos driver" I think the word "EXYNOS" is supposed to be in all capital letters. Apart from that, Acked-by: Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] USB: OMAP1: Tahvo USB transceiver driver
Add Tahvo USB transceiver driver.
Based on old code from linux-omap tree. The original driver was written
by Juha Yrjölä, Tony Lindgren, and Timo Teräs.
Signed-off-by: Aaro Koskinen
---
drivers/usb/phy/Kconfig | 14 ++
drivers/usb/phy/Makefile| 1 +
drivers/usb/phy/phy-tahvo.c | 448
3 files changed, 463 insertions(+)
create mode 100644 drivers/usb/phy/phy-tahvo.c
diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index 14a50bd..a5c7937 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -145,6 +145,20 @@ config OMAP_OTG
This driver can also be built as a module. If so, the module
will be called omap-otg.
+config TAHVO_USB
+ tristate "Tahvo USB transceiver driver"
+ depends on MFD_RETU && EXTCON
+ help
+ Enable this to support USB transceiver on Tahvo. This is used
+ at least on Nokia 770.
+
+config TAHVO_USB_HOST_BY_DEFAULT
+ depends on TAHVO_USB
+ boolean "Device in USB host mode by default"
+ help
+ Say Y here, if you want the device to enter USB host mode
+ by default on bootup.
+
config USB_ISP1301
tristate "NXP ISP1301 USB transceiver support"
depends on USB || USB_GADGET
diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
index c7f391b..f3984d1 100644
--- a/drivers/usb/phy/Makefile
+++ b/drivers/usb/phy/Makefile
@@ -13,6 +13,7 @@ phy-fsl-usb2-objs := phy-fsl-usb.o
phy-fsm-usb.o
obj-$(CONFIG_FSL_USB2_OTG) += phy-fsl-usb2.o
obj-$(CONFIG_ISP1301_OMAP) += phy-isp1301-omap.o
obj-$(CONFIG_MV_U3D_PHY) += phy-mv-u3d-usb.o
+obj-$(CONFIG_TAHVO_USB)+= phy-tahvo.o
obj-$(CONFIG_NOP_USB_XCEIV)+= phy-nop.o
obj-$(CONFIG_OMAP_CONTROL_USB) += phy-omap-control.o
obj-$(CONFIG_OMAP_OTG) += phy-omap-otg.o
diff --git a/drivers/usb/phy/phy-tahvo.c b/drivers/usb/phy/phy-tahvo.c
new file mode 100644
index 000..a2d96a2
--- /dev/null
+++ b/drivers/usb/phy/phy-tahvo.c
@@ -0,0 +1,448 @@
+/*
+ * Tahvo USB transceiver driver
+ *
+ * Copyright (C) 2005-2006 Nokia Corporation
+ *
+ * Parts copied from isp1301_omap.c.
+ * Copyright (C) 2004 Texas Instruments
+ * Copyright (C) 2004 David Brownell
+ *
+ * Original driver written by Juha Yrjölä, Tony Lindgren and Timo Teräs.
+ * Modified for Retu/Tahvo MFD by Aaro Koskinen.
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRIVER_NAME "tahvo-usb"
+
+#define TAHVO_REG_IDSR 0x02
+#define TAHVO_REG_USBR 0x06
+
+#define USBR_SLAVE_CONTROL (1 << 8)
+#define USBR_VPPVIO_SW (1 << 7)
+#define USBR_SPEED (1 << 6)
+#define USBR_REGOUT(1 << 5)
+#define USBR_MASTER_SW2(1 << 4)
+#define USBR_MASTER_SW1(1 << 3)
+#define USBR_SLAVE_SW (1 << 2)
+#define USBR_NSUSPEND (1 << 1)
+#define USBR_SEMODE(1 << 0)
+
+#define TAHVO_MODE_HOST0
+#define TAHVO_MODE_PERIPHERAL 1
+
+struct tahvo_usb {
+ struct platform_device *pt_dev;
+ struct usb_phy phy;
+ int vbus_state;
+ struct mutexserialize;
+ struct clk *ick;
+ int irq;
+ int tahvo_mode;
+ struct extcon_dev extcon;
+};
+
+static const char *tahvo_cable[] = {
+ "USB-HOST",
+ "USB",
+ NULL,
+};
+
+static ssize_t vbus_state_show(struct device *device,
+ struct device_attribute *attr, char *buf)
+{
+ struct tahvo_usb *tu = dev_get_drvdata(device);
+ return sprintf(buf, "%d\n", tu->vbus_state);
+}
+static DEVICE_ATTR(vbus_state, 0444, vbus_state_show, NULL);
+
+static void check_vbus_state(struct tahvo_usb *tu)
+{
+ struct retu_dev *rdev = dev_get_drvdata(tu->pt_dev->dev.parent);
+ int reg, prev_state;
+
+ reg = retu_read(rdev, TAHVO_REG_IDSR);
+ if (reg & TAHVO_STAT_VBUS) {
+ switch (tu->phy.state) {
+ case OTG_STATE_B_IDLE:
+ /* Enable the gadget driver */
+ if (tu->phy.otg->gadget)
+ usb_gadget_vbus_connect(tu->phy.otg->gadget);
+ tu->phy.state = OTG_STATE_B_PERIPHERAL;
+ break;
+ case OTG_STATE_A_IDLE:
+
[PATCH v3 0/4] USB: OMAP1: Tahvo USB support for 770
Hi, These patches add support for Tahvo USB transceiver and allow using both host and peripheral modes on Nokia 770. Tested (peripheral mode, host mode, vbus detection) with 3.10-rc6. History: v3: Delete accidental #include from patch 3. Drop board file changes, already queued to linux-omap. v2: http://marc.info/?l=linux-omap&m=137138976406242&w=2 Use extcon framework to trigger OTG driver mode changes. v1: http://marc.info/?l=linux-omap&m=137081763029385&w=2 Earlier RFC versions: http://marc.info/?l=linux-omap&m=13655371679&w=2 http://marc.info/?l=linux-omap&m=136266725827698&w=2 Aaro Koskinen (4): ARM: OMAP1: USB: move omap_usb_config to platform data USB: OMAP1: add extcon to platform data USB: OMAP1: OTG controller driver USB: OMAP1: Tahvo USB transceiver driver arch/arm/mach-omap1/include/mach/usb.h | 38 +-- drivers/usb/phy/Kconfig | 24 ++ drivers/usb/phy/Makefile| 2 + drivers/usb/phy/phy-omap-otg.c | 169 drivers/usb/phy/phy-tahvo.c | 448 include/linux/platform_data/usb-omap1.h | 53 6 files changed, 697 insertions(+), 37 deletions(-) create mode 100644 drivers/usb/phy/phy-omap-otg.c create mode 100644 drivers/usb/phy/phy-tahvo.c create mode 100644 include/linux/platform_data/usb-omap1.h A. -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/4] ARM: OMAP1: USB: move omap_usb_config to platform data
Move omap_usb_config to platform data, so that OTG driver can include it.
Signed-off-by: Aaro Koskinen
Acked-by: Tony Lindgren
---
arch/arm/mach-omap1/include/mach/usb.h | 38 +---
include/linux/platform_data/usb-omap1.h | 51 +
2 files changed, 52 insertions(+), 37 deletions(-)
create mode 100644 include/linux/platform_data/usb-omap1.h
diff --git a/arch/arm/mach-omap1/include/mach/usb.h
b/arch/arm/mach-omap1/include/mach/usb.h
index 45e5ac7..2c26305 100644
--- a/arch/arm/mach-omap1/include/mach/usb.h
+++ b/arch/arm/mach-omap1/include/mach/usb.h
@@ -8,43 +8,7 @@
#defineis_usb0_device(config) 0
#endif
-struct omap_usb_config {
- /* Configure drivers according to the connectors on your board:
-* - "A" connector (rectagular)
-* ... for host/OHCI use, set "register_host".
-* - "B" connector (squarish) or "Mini-B"
-* ... for device/gadget use, set "register_dev".
-* - "Mini-AB" connector (very similar to Mini-B)
-* ... for OTG use as device OR host, initialize "otg"
-*/
- unsignedregister_host:1;
- unsignedregister_dev:1;
- u8 otg;/* port number, 1-based: usb1 == 2 */
-
- u8 hmc_mode;
-
- /* implicitly true if otg: host supports remote wakeup? */
- u8 rwc;
-
- /* signaling pins used to talk to transceiver on usbN:
-* 0 == usbN unused
-* 2 == usb0-only, using internal transceiver
-* 3 == 3 wire bidirectional
-* 4 == 4 wire bidirectional
-* 6 == 6 wire unidirectional (or TLL)
-*/
- u8 pins[3];
-
- struct platform_device *udc_device;
- struct platform_device *ohci_device;
- struct platform_device *otg_device;
-
- u32 (*usb0_init)(unsigned nwires, unsigned is_device);
- u32 (*usb1_init)(unsigned nwires);
- u32 (*usb2_init)(unsigned nwires, unsigned alt_pingroup);
-
- int (*ocpi_enable)(void);
-};
+#include
void omap_otg_init(struct omap_usb_config *config);
diff --git a/include/linux/platform_data/usb-omap1.h
b/include/linux/platform_data/usb-omap1.h
new file mode 100644
index 000..8c7764d
--- /dev/null
+++ b/include/linux/platform_data/usb-omap1.h
@@ -0,0 +1,51 @@
+/*
+ * Platform data for OMAP1 USB
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive for
+ * more details.
+ */
+#ifndef __LINUX_USB_OMAP1_H
+#define __LINUX_USB_OMAP1_H
+
+#include
+
+struct omap_usb_config {
+ /* Configure drivers according to the connectors on your board:
+* - "A" connector (rectagular)
+* ... for host/OHCI use, set "register_host".
+* - "B" connector (squarish) or "Mini-B"
+* ... for device/gadget use, set "register_dev".
+* - "Mini-AB" connector (very similar to Mini-B)
+* ... for OTG use as device OR host, initialize "otg"
+*/
+ unsignedregister_host:1;
+ unsignedregister_dev:1;
+ u8 otg;/* port number, 1-based: usb1 == 2 */
+
+ u8 hmc_mode;
+
+ /* implicitly true if otg: host supports remote wakeup? */
+ u8 rwc;
+
+ /* signaling pins used to talk to transceiver on usbN:
+* 0 == usbN unused
+* 2 == usb0-only, using internal transceiver
+* 3 == 3 wire bidirectional
+* 4 == 4 wire bidirectional
+* 6 == 6 wire unidirectional (or TLL)
+*/
+ u8 pins[3];
+
+ struct platform_device *udc_device;
+ struct platform_device *ohci_device;
+ struct platform_device *otg_device;
+
+ u32 (*usb0_init)(unsigned nwires, unsigned is_device);
+ u32 (*usb1_init)(unsigned nwires);
+ u32 (*usb2_init)(unsigned nwires, unsigned alt_pingroup);
+
+ int (*ocpi_enable)(void);
+};
+
+#endif /* __LINUX_USB_OMAP1_H */
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/4] USB: OMAP1: OTG controller driver
Transceivers need to manage OTG controller state on OMAP1 to enable
switching between peripheral and host modes. Provide a driver for that.
Signed-off-by: Aaro Koskinen
---
drivers/usb/phy/Kconfig| 10 +++
drivers/usb/phy/Makefile | 1 +
drivers/usb/phy/phy-omap-otg.c | 169 +
3 files changed, 180 insertions(+)
create mode 100644 drivers/usb/phy/phy-omap-otg.c
diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index 7ef3eb8..14a50bd 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -135,6 +135,16 @@ config USB_GPIO_VBUS
optionally control of a D+ pullup GPIO as well as a VBUS
current limit regulator.
+config OMAP_OTG
+ tristate "OMAP USB OTG controller driver"
+ depends on ARCH_OMAP_OTG && EXTCON
+ help
+ Enable this to support some transceivers on OMAP1 platforms. OTG
+ controller is needed to switch between host and peripheral modes.
+
+ This driver can also be built as a module. If so, the module
+ will be called omap-otg.
+
config USB_ISP1301
tristate "NXP ISP1301 USB transceiver support"
depends on USB || USB_GADGET
diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
index a9169cb..c7f391b 100644
--- a/drivers/usb/phy/Makefile
+++ b/drivers/usb/phy/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_ISP1301_OMAP)+= phy-isp1301-omap.o
obj-$(CONFIG_MV_U3D_PHY) += phy-mv-u3d-usb.o
obj-$(CONFIG_NOP_USB_XCEIV)+= phy-nop.o
obj-$(CONFIG_OMAP_CONTROL_USB) += phy-omap-control.o
+obj-$(CONFIG_OMAP_OTG) += phy-omap-otg.o
obj-$(CONFIG_OMAP_USB2)+= phy-omap-usb2.o
obj-$(CONFIG_OMAP_USB3)+= phy-omap-usb3.o
obj-$(CONFIG_SAMSUNG_USBPHY) += phy-samsung-usb.o
diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c
new file mode 100644
index 000..11598cd
--- /dev/null
+++ b/drivers/usb/phy/phy-omap-otg.c
@@ -0,0 +1,169 @@
+/*
+ * OMAP OTG controller driver
+ *
+ * Based on code from tahvo-usb.c and isp1301_omap.c drivers.
+ *
+ * Copyright (C) 2005-2006 Nokia Corporation
+ * Copyright (C) 2004 Texas Instruments
+ * Copyright (C) 2004 David Brownell
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct otg_device {
+ void __iomem*base;
+ boolid;
+ boolvbus;
+ struct extcon_specific_cable_nb vbus_dev;
+ struct extcon_specific_cable_nb id_dev;
+ struct notifier_block vbus_nb;
+ struct notifier_block id_nb;
+};
+
+#define OMAP_OTG_CTRL 0x0c
+#define OMAP_OTG_ASESSVLD (1 << 20)
+#define OMAP_OTG_BSESSEND (1 << 19)
+#define OMAP_OTG_BSESSVLD (1 << 18)
+#define OMAP_OTG_VBUSVLD (1 << 17)
+#define OMAP_OTG_ID(1 << 16)
+#define OMAP_OTG_XCEIV_OUTPUTS \
+ (OMAP_OTG_ASESSVLD | OMAP_OTG_BSESSEND | OMAP_OTG_BSESSVLD | \
+OMAP_OTG_VBUSVLD | OMAP_OTG_ID)
+
+static void omap_otg_ctrl(struct otg_device *otg_dev, u32 outputs)
+{
+ u32 l;
+
+ l = readl(otg_dev->base + OMAP_OTG_CTRL);
+ l &= ~OMAP_OTG_XCEIV_OUTPUTS;
+ l |= outputs;
+ writel(l, otg_dev->base + OMAP_OTG_CTRL);
+}
+
+static void omap_otg_set_mode(struct otg_device *otg_dev)
+{
+ if (!otg_dev->id && otg_dev->vbus)
+ /* Set B-session valid. */
+ omap_otg_ctrl(otg_dev, OMAP_OTG_ID | OMAP_OTG_BSESSVLD);
+ else if (otg_dev->vbus)
+ /* Set A-session valid. */
+ omap_otg_ctrl(otg_dev, OMAP_OTG_ASESSVLD);
+ else if (!otg_dev->id)
+ /* Set B-session end to indicate no VBUS. */
+ omap_otg_ctrl(otg_dev, OMAP_OTG_ID | OMAP_OTG_BSESSEND);
+}
+
+static int omap_otg_id_notifier(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct otg_device *otg_dev = container_of(nb, struct otg_device, id_nb);
+
+ otg_dev->id = event;
+ omap_otg_set_mode(otg_dev);
+
+ return NOTIFY_DONE;
+}
+
+static int omap_otg_vbus_notifier(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct otg_device *otg_dev = container_of(nb, struct otg_device,
+ vbus_nb);
+
+ otg_dev->vbus = event;
+ omap_otg_set_mode(o
[PATCH v3 2/4] USB: OMAP1: add extcon to platform data
Add extcon field to platform data.
Signed-off-by: Aaro Koskinen
---
include/linux/platform_data/usb-omap1.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/platform_data/usb-omap1.h
b/include/linux/platform_data/usb-omap1.h
index 8c7764d..43b5ce1 100644
--- a/include/linux/platform_data/usb-omap1.h
+++ b/include/linux/platform_data/usb-omap1.h
@@ -23,6 +23,8 @@ struct omap_usb_config {
unsignedregister_dev:1;
u8 otg;/* port number, 1-based: usb1 == 2 */
+ const char *extcon;/* extcon device for OTG */
+
u8 hmc_mode;
/* implicitly true if otg: host supports remote wakeup? */
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 1/1] usb: check usb_hub_to_struct_hub() return value
On Tue, 18 Jun 2013, Mathias Nyman wrote: > usb_hub_to_struct_hub() can return NULL in some unlikely cases. > Add checks where appropriate, or pass the hub pointer as an additional > argument if it's known to be valid. > > The places it makes sense to check usb_hub_to_struct_hub() > are picked based on feedback from Alan Stern. Acked-by: Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()
On Tue, 18 Jun 2013, Ming Lei wrote:
> We disable local IRQs here in case of running complete() by
> tasklet to avoid possible deadlock because drivers may call
> spin_lock() to hold lock which might be acquired in one hard
> interrupt handler.
>
> The local_irq_save()/local_irq_restore() around complete()
> will be removed if current USB drivers have been cleaned up
> and no one may trigger the above deadlock situation when
> running complete() in tasklet.
This should be part of the first patch, not added on separately.
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a272968..09a8263 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>
> /* pass ownership to the completion handler */
> urb->status = status;
> - urb->complete (urb);
> +
> + /*
> + * We disable local IRQs here in case of running complete() by
> + * tasklet to avoid possible deadlock because drivers may call
> + * spin_lock() to hold lock which might be acquired in one hard
> + * interrupt handler.
> + *
> + * The local_irq_save()/local_irq_restore() around complete()
> + * will be removed if current USB drivers have been cleaned up
> + * and no one may trigger the above deadlock situation when
> + * running complete() in tasklet.
> + */
> + if (hcd_giveback_urb_in_bh(hcd)) {
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + urb->complete (urb);
> + local_irq_restore(flags);
> + } else {
> + urb->complete (urb);
> + }
> +
There's no reason to bother with the test. You might as well always do
the local_irq_save.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 6/6] USB: EHCI: support running URB giveback in tasklet context
On Tue, 18 Jun 2013, Ming Lei wrote: > Both 4 transfers can work well on EHCI HCD after switching to run "Both 4 transfers"? The word "both" applies when there are two items, not more than two. And what are the four transfers you are referring to? Do you mean all four transfer _types_? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/5] USB: OMAP1: OTG controller driver
Hi, On Tue, Jun 18, 2013 at 11:21:43AM +0300, Felipe Balbi wrote: > On Sun, Jun 16, 2013 at 04:35:10PM +0300, Aaro Koskinen wrote: > > +#include > > no mach/* includes under drivers/usb, sorry. It's a pain to fix those up > later. Sorry, that was left there by a mistake. I will fix. A. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
On Tue, 18 Jun 2013, Ming Lei wrote: > Given interrupt URB will be resubmitted from tasklet context which > is scheduled by ehci hardware interrupt handler, and commonly only > one interrupt URB is scheduled on qh, so the qh may be unlinked > immediately once qh_completions() returns from ehci_irq(), then > the intr URB to be resubmitted in complete() can only be scheduled > and linked to hardware until the qh unlink is completed. > > This patch applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh) > state to improve this above situation, and the qh will wait for 5 > milliseconds before being unlinked from hardware, if one URB is submitted > during the period, the qh is move out of unlink wait state and the > interrupt transfer can be scheduled immediately, not like before: the > transfer is only linked to hardware until previous unlink is completed. > > Only enable the improvement in case that HCD supports to run > giveback of URB in tasklet context. The approach used in this patch is wrong. You shouldn't start the unlink, then wait, then finish the unlink. Consider what would happen if an URB were submitted during the delay: It would have to wait until the QH was completely unlinked. Instead, you should wait first, then do the entire unlink. For example, scan_async() in ehci-q.c doesn't immediately begin to unlink empty async QHs. It merely marks them as being empty and starts a timer to check them later. It they are still empty at that point, then they are unlinked. Also, it's silly to check whether or not giveback uses a tasklet. We know that following the 6/6 patch it will. Even if it doesn't, there's no harm in waiting a little while before unlinking an empty interrupt QH. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 4/6] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context
On Tue, 18 Jun 2013, Ming Lei wrote: > If HCD_BH is set for HC driver's flags, URB giveback will be > done in tasklet context instead of interrupt context, so the > ehci->lock needn't to be released any more before calling > usb_hcd_giveback_urb(). It is premature to do this now. This should be part of the 6/6 patch, when it won't be necessary to test hcd_giveback_urb_in_bh(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 3/6] USB: URB documentation: claim complete() may be run with IRQs enabled
On Tue, 18 Jun 2013, Ming Lei wrote: > There is no good reason to run complete() in hard interrupt > disabled context. > > After running complete() in tasklet, we will enable local IRQs > when calling complete() since we can do it now. > > Even though we still disable IRQs now when calling complete() > in tasklet, the URB documentation is updated to claim complete() > may be run in tasklet context and local IRQs may be enabled, so > that USB drivers can know the change and avoid one deadlock caused > by: assume IRQs disabled in complete() and call spin_lock() to > hold lock which might be acquired in interrupt context. > > Current spin_lock() usages in drivers' complete() will be cleaned > up at the same time, and when the cleanup is finished, local IRQs > will be enabled when calling complete() in tasklet. > > Also fix description about type of usb_complete_t. > @@ -210,12 +209,19 @@ have transferred successfully before the completion was > called. > > > NOTE: * WARNING * > -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called > -during hardware interrupt processing. If you can, defer substantial > -work to a tasklet (bottom half) to keep system latencies low. You'll > -probably need to use spinlocks to protect data structures you manipulate > -in completion handlers. > - > +NEVER SLEEP IN A COMPLETION HANDLER. These are called during hardware > +interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise > +called in tasklet context. If you can, defer substantial work to a tasklet > +(bottom half) to keep system latencies low. You'll probably need to use > +spinlocks to protect data structures you manipulate in completion handlers. You shouldn't go into so much detail here, partly because it doesn't matter for driver authors and partly because it is subject to change. Just say that completion handlers are usually called in an atomic context, so they must not sleep. Also, the advice about deferring work to a tasklet seems out of place now, since many completion handlers will already be running in a tasklet. > +Driver can't assume that local IRQs are disabled any more when running > +complete(), for example, if drivers want to hold a lock which might be > +acquired in hard interrupt handler, they have to use spin_lock_irqsave() > +instead of spin_lock() to hold the lock. Don't say so much. Just mention that in the current kernel (3.10), completion handlers always run with local interrupts disabled, but in the future this is likely to change. Therefore drivers should not make any assumptions. > +In the future, all HCD might set HCD_BH to run complete() in tasklet so > +that system latencies can be kept low. This does not need to be in the documentation. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context
On Tue, 18 Jun 2013, Ming Lei wrote:
> This patch implements the mechanism of giveback of URB in
> tasklet context, so that hardware interrupt handling time for
> usb host controller can be saved much, and HCD interrupt handling
> can be simplified.
Okay. I'd make a few relatively minor changes.
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 014dc99..a272968 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -699,9 +699,11 @@ error:
>* Avoiding calls to local_irq_disable/enable makes the code
>* RT-friendly.
>*/
> - spin_unlock(&hcd_root_hub_lock);
> + if (!hcd_giveback_urb_in_bh(hcd))
> + spin_unlock(&hcd_root_hub_lock);
> usb_hcd_giveback_urb(hcd, urb, status);
> - spin_lock(&hcd_root_hub_lock);
> + if (!hcd_giveback_urb_in_bh(hcd))
> + spin_lock(&hcd_root_hub_lock);
>
> spin_unlock_irq(&hcd_root_hub_lock);
> return 0;
> @@ -742,9 +744,11 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
> memcpy(urb->transfer_buffer, buffer, length);
>
> usb_hcd_unlink_urb_from_ep(hcd, urb);
> - spin_unlock(&hcd_root_hub_lock);
> + if (!hcd_giveback_urb_in_bh(hcd))
> + spin_unlock(&hcd_root_hub_lock);
> usb_hcd_giveback_urb(hcd, urb, 0);
> - spin_lock(&hcd_root_hub_lock);
> + if (!hcd_giveback_urb_in_bh(hcd))
> + spin_lock(&hcd_root_hub_lock);
> } else {
> length = 0;
> set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags);
> @@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd,
> struct urb *urb, int status)
> hcd->status_urb = NULL;
> usb_hcd_unlink_urb_from_ep(hcd, urb);
>
> - spin_unlock(&hcd_root_hub_lock);
> + if (!hcd_giveback_urb_in_bh(hcd))
> + spin_unlock(&hcd_root_hub_lock);
> usb_hcd_giveback_urb(hcd, urb, status);
> - spin_lock(&hcd_root_hub_lock);
> + if (!hcd_giveback_urb_in_bh(hcd))
> + spin_lock(&hcd_root_hub_lock);
> }
> }
> done:
None of these tests are necessary. Root hubs are different from normal
devices; their URBs are handled mostly by usbcore. The only part done
by the HCD is always synchronous. And we know that root-hub URBs
always have a very short completion handler. So we may as well
continue to handle root-hub URBs the way we always have.
> @@ -1648,6 +1654,60 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
>
> /*-*/
>
> +static void __usb_hcd_giveback_urb(struct urb *urb)
> +{
> + struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus);
> + int status = urb->status;
> +
> + urb->hcpriv = NULL;
> + if (unlikely(urb->unlinked))
> + status = urb->unlinked;
The way you're storing the status value isn't good. We decided to make
the status a separate argument because of drivers that abuse the
urb->status field (they poll it instead of waiting for the completion
handler to be called). Hopefully there aren't any examples of drivers
still doing this, but...
This means you shouldn't store anything in urb->status until just
before calling the completion handler. What you can do instead is
always store the status value in urb->unlinked.
> + else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) &&
> + urb->actual_length < urb->transfer_buffer_length &&
> + !status))
> + status = -EREMOTEIO;
> +
> + unmap_urb_for_dma(hcd, urb);
> + usbmon_urb_complete(&hcd->self, urb, status);
> + usb_unanchor_urb(urb);
> +
> + /* pass ownership to the completion handler */
> + urb->status = status;
> + urb->complete (urb);
You are supposed to disable local interrupts around the call to the
completion handler.
> + atomic_dec (&urb->use_count);
> + if (unlikely(atomic_read(&urb->reject)))
> + wake_up (&usb_kill_urb_queue);
> + usb_put_urb (urb);
> +}
> +
> +static void usb_giveback_urb_bh(unsigned long param)
> +{
> + struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param;
> + unsigned long flags;
> + struct list_head local_list;
> +
> + spin_lock_irqsave(&bh->lock, flags);
> + bh->running = 1;
> +restart:
I like to have statement labels indented by a single space character,
so that tools like diff don't think the label marks the start of a new
function.
> @@ -1667,25 +1727,33 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
> */
> void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
> {
> - urb->hcpriv
Re: [PATCH] build some drivers only when compile-testing
On Mon, Jun 17, 2013 at 10:05:19PM +0200, Jiri Slaby wrote: > On 05/23/2013 05:09 AM, Jeff Mahoney wrote: > > On 5/22/13 10:23 PM, Greg Kroah-Hartman wrote: > >> On Wed, May 22, 2013 at 11:18:46AM +0200, Jiri Slaby wrote: > >>> Some drivers can be built on more platforms than they run on. This > >>> causes users and distributors packaging burden when they have to > >>> manually deselect some drivers from their allmodconfigs. Or sometimes > >>> it is even impossible to disable the drivers without patching the > >>> kernel. > >>> > >>> Introduce a new config option COMPILE_TEST and make all those drivers > >>> to depend on the platform they run on, or on the COMPILE_TEST option. > >>> Now, when users/distributors choose COMPILE_TEST=n they will not have > >>> the drivers in their allmodconfig setups, but developers still can > >>> compile-test them with COMPILE_TEST=y. > >> > >> I understand the urge, and it's getting hard for distros to handle these > >> drivers that just don't work on other architectures, but it's really > >> valuable to ensure that they build properly, for those of us that don't > >> have many/any cross compilers set up. > > But this is exactly what COMPILE_TEST will give us when set to "y", or > am I missing something? > > >>> Now the drivers where we use this new option: > >>> * PTP_1588_CLOCK_PCH: The PCH EG20T is only compatible with Intel Atom > >>> processors so it should depend on x86. > >>> * FB_GEODE: Geode is 32-bit only so only enable it for X86_32. > >>> * USB_CHIPIDEA_IMX: The OF_DEVICE dependency will be met on powerpc > >>> systems -- which do not actually support the hardware via that > >>> method. > >> > >> This seems ripe to start to get really messy, really quickly. Shouldn't > >> "default configs" handle if this "should" be enabled for a platform or > >> not, and let the rest of us just build them with no problems? > > > > If every time a new Kconfig option is added, corresponding default > > config updates come with it, sure. I just don't see that happening, > > especially when it can be done much more clearly in the Kconfig while > > the developer is writing the driver. > > > >> What problems is this causing you? Are you running out of space in > >> kernel packages with drivers that will never be actually used? > > > > Wasted build resources. Wasted disk space on /every/ system the kernel > > package is installed on. We're all trying to pare down the kernel > > packages to eliminate wasted space and doing it manually means a bunch > > of research, sometimes with incorrect assumptions about the results, > > needs to be done by someone not usually associated with that code. That > > research gets repeated by people maintaining kernel packages for pretty > > much every distro. > > I second all the above. > > >>> +config COMPILE_TEST > >>> + bool "Compile also drivers which will not load" if EXPERT > >> > >> EXPERT is getting to be the "let's hide it here" option, isn't it... > >> > >> I don't know, if no one else strongly objects, I can be convinced that > >> this is needed, but so far, I don't see why it really is, or what this > >> is going to help with. > > > > I'm not convinced adding a || COMPILE_TEST option to every driver that > > may be arch specific is the best way to go either. Perhaps adding a new > > Kconfig verb called "archdepends on" or something that will evaluate as > > true if COMPILE_TEST is enabled but will evaluate the conditional if > > not. *waves hands* > > Sam Ravnborg (the kconfig ex-maintainer) once wrote that he doesn't want > to extend the kconfig language for this purpose (which I support). That > a config option is fine and sufficient in this case [1]. Except he > called the config option "SHOW_ALL_DRIVERS". Adding the current > maintainer to CCs ;). > > [1] http://comments.gmane.org/gmane.linux.kbuild.devel/9829 > > The last point I inclined to the Greg's argument to remove the EXPERT > dependency. > > So currently I have what is attached... Comments? Looks good to me, want me to queue it up through my char/misc driver tree for 3.11? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller
On Tue, 18 Jun 2013, Felipe Balbi wrote: > yeah, Roger brought up a big problem with OMAP's EHCI depending on the > mode so, at least for now, we should keep phy_get and, in case of EHCI > OMAP, phy_init in the glue :-( > > Roger has all the details, and they're also in the list archives, but > basically, depending on the mode, PHY *must* be initialized at a > particular point. Right. Which means the core shouldn't be involved, since the OMAP PHY initialization has to be done at a non-standard time. (Unless we decide to add a flag for this special case...) > > Given that the glue module has to be responsible for getting the PHY, > > it should also be responsible for error checking. So the code added to > > hcd.c doesn't need to apply an IS_ERR check; it can simply assume that > > if hcd->phy is NULL then either there is no software-controllable PHY > > or else the HCD doesn't want the core to manage it. > > makes sense to me, add the requirement to: > > if (IS_ERR(hcd->phy)) > hcd->phy = NULL; Actually, in the IS_ERR case, most glue drivers just fail the probe. But for any that want to continue on, we would have to add this requirement. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/9] usb: gadget/uvc: stability and usability fixes
Hi Laurent, On Tue, Jun 04, 2013 at 05:08:19PM +0200, Michael Grzeschik wrote: > Hi, > > this series is fixing some stability and usability issues found with the usb > uvc-gadget. It's needed to make the device usable as an v4l2sink with > gstreamer. > > Thanks, > Michael > > Laurent Pinchart (1): > usb: gadget/uvc: Fix error handling in uvc_queue_buffer() > > Michael Grzeschik (8): > usb: gadget/uvc: cancel the video queue if buffers could not be enqueued > usb: gadget/uvc: also handle v4l2 ioctl ENUM_FMT > usb: gadget/uvc: free buffers after streamoff > usb: gadget/uvc: Add monotonic time stamp flag > usb: gadget/uvc: change KERN_INFO to KERN_DEBUG on request shutdown > usb: gadget/uvc: set sizes in queue_setup to 0 when memorytype is USERPTR > usb: gadget/uvc: fix S_FMT always assume sizeimage for MJPEG > usb: gadget/uvc: add uvc suspend resume functions > > drivers/usb/gadget/f_uvc.c | 17 + > drivers/usb/gadget/uvc_queue.c | 9 +++-- > drivers/usb/gadget/uvc_v4l2.c | 33 +++-- > drivers/usb/gadget/uvc_video.c | 5 - > 4 files changed, 59 insertions(+), 5 deletions(-) Ping! Did you find time to look into that series? Regards, Michael -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()
Hello.
On 18-06-2013 19:03, Ming Lei wrote:
We disable local IRQs here in case of running complete() by
tasklet to avoid possible deadlock because drivers may call
spin_lock() to hold lock which might be acquired in one hard
interrupt handler.
The local_irq_save()/local_irq_restore() around complete()
will be removed if current USB drivers have been cleaned up
and no one may trigger the above deadlock situation when
running complete() in tasklet.
Cc: Oliver Neukum
Cc: Alan Stern
Signed-off-by: Ming Lei
---
drivers/usb/core/hcd.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index a272968..09a8263 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
/* pass ownership to the completion handler */
urb->status = status;
- urb->complete (urb);
+
+ /*
+* We disable local IRQs here in case of running complete() by
+* tasklet to avoid possible deadlock because drivers may call
+* spin_lock() to hold lock which might be acquired in one hard
+* interrupt handler.
+*
+* The local_irq_save()/local_irq_restore() around complete()
+* will be removed if current USB drivers have been cleaned up
+* and no one may trigger the above deadlock situation when
+* running complete() in tasklet.
+*/
+ if (hcd_giveback_urb_in_bh(hcd)) {
+ unsigned long flags;
+
+ local_irq_save(flags);
+ urb->complete (urb);
I guess you didn't run the patch thru scripts/checkpatch.pl, did you?
It would complain about the space before (.
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 05/10] USB: OHCI: Properly handle ohci-exynos suspend
On Tue, 18 Jun 2013, Manjunath Goudar wrote:
> After Alan explanation I am writing below code end of ohci_suspend()
> routine.is it correct Alan.
>
>if (do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
> ohci_resume(hcd, false);
> rc = -EBUSY;
> }
I'm glad you asked, because there is an important part I forgot about.
Just before the code above, it is also necessary to add:
synchronize_irq(hcd->irq);
The reason is because a wakeup interrupt might race with the suspend
call. If the suspend finishes first, we need to wait until the
interrupt has been handled before checking whether there is a pending
wakeup. That's what synchronize_irq() does; it waits until all the
outstanding interrupt requests have been handled.
(Also, as Sachin pointed out, you have to return rc instead of 0.)
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink
Given interrupt URB will be resubmitted from tasklet context which
is scheduled by ehci hardware interrupt handler, and commonly only
one interrupt URB is scheduled on qh, so the qh may be unlinked
immediately once qh_completions() returns from ehci_irq(), then
the intr URB to be resubmitted in complete() can only be scheduled
and linked to hardware until the qh unlink is completed.
This patch applied the QH_STATE_UNLINK_WAIT(not used by interrupt qh)
state to improve this above situation, and the qh will wait for 5
milliseconds before being unlinked from hardware, if one URB is submitted
during the period, the qh is move out of unlink wait state and the
interrupt transfer can be scheduled immediately, not like before: the
transfer is only linked to hardware until previous unlink is completed.
Only enable the improvement in case that HCD supports to run
giveback of URB in tasklet context.
Cc: Alan Stern
Signed-off-by: Ming Lei
---
drivers/usb/host/ehci-hcd.c | 16 ---
drivers/usb/host/ehci-hub.c |1 +
drivers/usb/host/ehci-sched.c | 60 ++---
drivers/usb/host/ehci-timer.c | 43 +
drivers/usb/host/ehci.h |3 +++
5 files changed, 115 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 246e124..0ab82de 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -304,7 +304,9 @@ static void end_unlink_async(struct ehci_hcd *ehci);
static void unlink_empty_async(struct ehci_hcd *ehci);
static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
static void ehci_work(struct ehci_hcd *ehci);
-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
+static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
+ bool wait);
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
#include "ehci-timer.c"
@@ -484,6 +486,7 @@ static int ehci_init(struct usb_hcd *hcd)
ehci->periodic_size = DEFAULT_I_TDPS;
INIT_LIST_HEAD(&ehci->async_unlink);
INIT_LIST_HEAD(&ehci->async_idle);
+ INIT_LIST_HEAD(&ehci->intr_unlink_wait);
INIT_LIST_HEAD(&ehci->intr_unlink);
INIT_LIST_HEAD(&ehci->intr_qh_list);
INIT_LIST_HEAD(&ehci->cached_itd_list);
@@ -908,15 +911,20 @@ static int ehci_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
switch (qh->qh_state) {
case QH_STATE_LINKED:
if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT)
- start_unlink_intr(ehci, qh);
+ start_unlink_intr(ehci, qh, false);
else
start_unlink_async(ehci, qh);
break;
case QH_STATE_COMPLETING:
qh->dequeue_during_giveback = 1;
break;
- case QH_STATE_UNLINK:
case QH_STATE_UNLINK_WAIT:
+ if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) {
+ cancel_unlink_wait_intr(ehci, qh);
+ start_unlink_intr(ehci, qh, false);
+ }
+ break;
+ case QH_STATE_UNLINK:
/* already started */
break;
case QH_STATE_IDLE:
@@ -1042,7 +1050,7 @@ ehci_endpoint_reset(struct usb_hcd *hcd, struct
usb_host_endpoint *ep)
if (eptype == USB_ENDPOINT_XFER_BULK)
start_unlink_async(ehci, qh);
else
- start_unlink_intr(ehci, qh);
+ start_unlink_intr(ehci, qh, false);
}
}
spin_unlock_irqrestore(&ehci->lock, flags);
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index b2f6450..4104c66 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -345,6 +345,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
end_unlink_async(ehci);
unlink_empty_async_suspended(ehci);
+ ehci_handle_start_intr_unlinks(ehci);
ehci_handle_intr_unlinks(ehci);
end_free_itds(ehci);
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f80d033..5b9ca31 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -601,10 +601,10 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci,
struct ehci_qh *qh)
list_del(&qh->intr_node);
}
-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
- /* If the QH isn't linked then there's nothing we can do. */
[RFC PATCH v1 4/6] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context
If HCD_BH is set for HC driver's flags, URB giveback will be done in tasklet context instead of interrupt context, so the ehci->lock needn't to be released any more before calling usb_hcd_giveback_urb(). Cc: Alan Stern Signed-off-by: Ming Lei --- drivers/usb/host/ehci-q.c |6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c index d34b399..0387a81 100644 --- a/drivers/usb/host/ehci-q.c +++ b/drivers/usb/host/ehci-q.c @@ -283,9 +283,11 @@ __acquires(ehci->lock) /* complete() can reenter this HCD */ usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb); - spin_unlock (&ehci->lock); + if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci))) + spin_unlock(&ehci->lock); usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); - spin_lock (&ehci->lock); + if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci))) + spin_lock(&ehci->lock); } static int qh_schedule (struct ehci_hcd *ehci, struct ehci_qh *qh); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH v1 0/6] USB: HCD/EHCI: giveback of URB in tasklet context
Hi, The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler(IRQs disabled time) can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb(). But, some drivers may assume IRQs are disabled in complete(), and they may use spin_lock() to hold lock, so deadlock might be caused when the lock is acquired in hard interrupt context. So currently patch 2/6 disables IRQs when calling complete() in tasklet, and at the same time we can start to clean up drivers by converting spin_lock() in complete() to spin_lock_irqsave() if the deadlock situation may exist. After the cleanup is completed, complete() in tasklet will be called with IRQs enabled. Patch 5/6 improves interrupt qh unlink on EHCI HCD when the mechanism is introduced. The patchset enables the mechanism on EHCI HCD. In the commit log of patch 6/6, detailed test result on three machines (ARM A9/A15 dual core, X86) are provided about transfer performance and ehci irq handling time. From the result, no transfer performance loss is found and ehci irq handling time drops much with the patchset. V1: - change percput tasklet into tasklet in HCD to avoid out of order of URB->complete() for same endpoint - disable local IRQs when calling complete() from tasklet to avoid deadlock which is caused by holding lock via spin_lock and the same lock might be acquired in hard irq context - document coming change about calling complete() with irq enabled so that we can start to clean up USB drivers which call spin_lock() in complete() Documentation/usb/URB.txt | 30 +++--- drivers/usb/core/hcd.c| 196 - drivers/usb/host/ehci-fsl.c |2 +- drivers/usb/host/ehci-grlib.c |2 +- drivers/usb/host/ehci-hcd.c | 18 +++- drivers/usb/host/ehci-hub.c |1 + drivers/usb/host/ehci-mv.c|2 +- drivers/usb/host/ehci-octeon.c|2 +- drivers/usb/host/ehci-pmcmsp.c|2 +- drivers/usb/host/ehci-ppc-of.c|2 +- drivers/usb/host/ehci-ps3.c |2 +- drivers/usb/host/ehci-q.c |6 +- drivers/usb/host/ehci-sched.c | 60 +++- drivers/usb/host/ehci-sead3.c |2 +- drivers/usb/host/ehci-sh.c|2 +- drivers/usb/host/ehci-tegra.c |2 +- drivers/usb/host/ehci-tilegx.c|2 +- drivers/usb/host/ehci-timer.c | 43 drivers/usb/host/ehci-w90x900.c |2 +- drivers/usb/host/ehci-xilinx-of.c |2 +- drivers/usb/host/ehci.h |3 + include/linux/usb/hcd.h | 17 22 files changed, 341 insertions(+), 59 deletions(-) Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH v1 1/6] USB: HCD: support giveback of URB in tasklet context
This patch implements the mechanism of giveback of URB in
tasklet context, so that hardware interrupt handling time for
usb host controller can be saved much, and HCD interrupt handling
can be simplified.
Motivations:
1), on some arch(such as ARM), DMA mapping/unmapping is a bit
time-consuming, for example: when accessing usb mass storage
via EHCI on pandaboard, the common length of transfer buffer is 120KB,
the time consumed on DMA unmapping may reach hundreds of microseconds;
even on A15 based box, the time is still about scores of microseconds
2), on some arch, reading DMA coherent memoery is very time-consuming,
the most common example is usb video class driver[1]
3), driver's complete() callback may do much things which is driver
specific, so the time is consumed unnecessarily in hardware irq context.
4), running driver's complete() callback in hardware irq context causes
that host controller driver has to release its lock in interrupt handler,
so reacquiring the lock after return may busy wait a while and increase
interrupt handling time. More seriously, releasing the HCD lock makes
HCD becoming quite complicated to deal with introduced races.
So the patch proposes to run giveback of URB in tasklet context, then
time consumed in HCD irq handling doesn't depend on drivers' complete and
DMA mapping/unmapping any more, also we can simplify HCD since the HCD
lock isn't needed to be released during irq handling.
The patch should be reasonable and doable:
1), for drivers, they don't care if the complete() is called in hard irq
context or softirq context
2), the biggest change is the situation in which usb_submit_urb() is called
in complete() callback, so the introduced tasklet schedule delay might be a
con, but it shouldn't be a big deal:
- control/bulk asynchronous transfer isn't sensitive to schedule
delay
- the patch schedules giveback of periodic URBs using
tasklet_hi_schedule, so the introduced delay should be very
small
- for ISOC transfer, generally, drivers submit several URBs
concurrently to avoid interrupt delay, so it is OK with the
little schedule delay.
- for interrupt transfer, generally, drivers only submit one URB
at the same time, but interrupt transfer is often used in event
report, polling, ... situations, and a little delay should be OK.
Considered that HCDs may optimize on submitting URB in complete(), the
patch may cause the optimization not working, so introduces one flag to mark
if the HCD supports to run giveback URB in tasklet context. When all HCDs
are ready, the flag can be removed.
[1], http://marc.info/?t=136438111600010&r=1&w=2
Cc: Oliver Neukum
Cc: Alan Stern
Signed-off-by: Ming Lei
---
drivers/usb/core/hcd.c | 175 ---
include/linux/usb/hcd.h | 17 +
2 files changed, 169 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 014dc99..a272968 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -699,9 +699,11 @@ error:
* Avoiding calls to local_irq_disable/enable makes the code
* RT-friendly.
*/
- spin_unlock(&hcd_root_hub_lock);
+ if (!hcd_giveback_urb_in_bh(hcd))
+ spin_unlock(&hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(&hcd_root_hub_lock);
+ if (!hcd_giveback_urb_in_bh(hcd))
+ spin_lock(&hcd_root_hub_lock);
spin_unlock_irq(&hcd_root_hub_lock);
return 0;
@@ -742,9 +744,11 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
memcpy(urb->transfer_buffer, buffer, length);
usb_hcd_unlink_urb_from_ep(hcd, urb);
- spin_unlock(&hcd_root_hub_lock);
+ if (!hcd_giveback_urb_in_bh(hcd))
+ spin_unlock(&hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, 0);
- spin_lock(&hcd_root_hub_lock);
+ if (!hcd_giveback_urb_in_bh(hcd))
+ spin_lock(&hcd_root_hub_lock);
} else {
length = 0;
set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags);
@@ -835,9 +839,11 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
hcd->status_urb = NULL;
usb_hcd_unlink_urb_from_ep(hcd, urb);
- spin_unlock(&hcd_root_hub_lock);
+ if (!hcd_giveback_urb_in_bh(hcd))
+ spin_unlock(&hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(&hcd_root_hub_lock);
+ if (!hcd_giveback_urb_in_bh(hcd))
+ spin_lock(&hcd_root_hub_lock);
[RFC PATCH v1 3/6] USB: URB documentation: claim complete() may be run with IRQs enabled
There is no good reason to run complete() in hard interrupt disabled context. After running complete() in tasklet, we will enable local IRQs when calling complete() since we can do it now. Even though we still disable IRQs now when calling complete() in tasklet, the URB documentation is updated to claim complete() may be run in tasklet context and local IRQs may be enabled, so that USB drivers can know the change and avoid one deadlock caused by: assume IRQs disabled in complete() and call spin_lock() to hold lock which might be acquired in interrupt context. Current spin_lock() usages in drivers' complete() will be cleaned up at the same time, and when the cleanup is finished, local IRQs will be enabled when calling complete() in tasklet. Also fix description about type of usb_complete_t. Cc: Oliver Neukum Cc: Alan Stern Signed-off-by: Ming Lei --- Documentation/usb/URB.txt | 30 ++ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/Documentation/usb/URB.txt b/Documentation/usb/URB.txt index 00d2c64..454db87 100644 --- a/Documentation/usb/URB.txt +++ b/Documentation/usb/URB.txt @@ -195,13 +195,12 @@ by the completion handler. The handler is of the following type: - typedef void (*usb_complete_t)(struct urb *, struct pt_regs *) + typedef void (*usb_complete_t)(struct urb *) -I.e., it gets the URB that caused the completion call, plus the -register values at the time of the corresponding interrupt (if any). -In the completion handler, you should have a look at urb->status to -detect any USB errors. Since the context parameter is included in the URB, -you can pass information to the completion handler. +I.e., it gets the URB that caused the completion call. In the completion +handler, you should have a look at urb->status to detect any USB errors. +Since the context parameter is included in the URB, you can pass +information to the completion handler. Note that even when an error (or unlink) is reported, data may have been transferred. That's because USB transfers are packetized; it might take @@ -210,12 +209,19 @@ have transferred successfully before the completion was called. NOTE: * WARNING * -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called -during hardware interrupt processing. If you can, defer substantial -work to a tasklet (bottom half) to keep system latencies low. You'll -probably need to use spinlocks to protect data structures you manipulate -in completion handlers. - +NEVER SLEEP IN A COMPLETION HANDLER. These are called during hardware +interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise +called in tasklet context. If you can, defer substantial work to a tasklet +(bottom half) to keep system latencies low. You'll probably need to use +spinlocks to protect data structures you manipulate in completion handlers. + +Driver can't assume that local IRQs are disabled any more when running +complete(), for example, if drivers want to hold a lock which might be +acquired in hard interrupt handler, they have to use spin_lock_irqsave() +instead of spin_lock() to hold the lock. + +In the future, all HCD might set HCD_BH to run complete() in tasklet so +that system latencies can be kept low. 1.8. How to do isochronous (ISO) transfers? -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH v1 6/6] USB: EHCI: support running URB giveback in tasklet context
Both 4 transfers can work well on EHCI HCD after switching to run
URB giveback in tasklet context, so mark all HCD drivers to support
it.
>From below test results on 3 machines(2 ARM and one x86), time
consumed by EHCI interrupt handler droped much without performance
loss.
1 test description
1.1 mass storage performance test:
- run below command 10 times and compute the average performance
dd if=/dev/sdN iflag=direct of=/dev/null bs=200M count=1
- two usb mass storage device:
A: sandisk extreme USB 3.0 16G(used in test case 1 & case 2)
B: kingston DataTraveler G2 4GB(only used in test case 2)
1.2 uvc function test:
- run one simple capture program in the below link
http://kernel.ubuntu.com/~ming/up/capture.c
- capture format 640*480 and results in High Bandwidth mode on the
uvc device: Z-Star 0x0ac8/0x3450
- on T410(x86) laptop, also use guvcview to watch video capture/playback
1.3 about test2 and test4
- both two devices involved are tested concurrently by above test items
1.4 how to compute irq time(the time consumed by ehci_irq)
- use trace points of irq:irq_handler_entry and irq:irq_handler_exit
1.5 kernel
3.10.0-rc3-next-20130528
1.6 test machines
Pandaboard A1: ARM CortexA9 dural core
Arndale board: ARM CortexA15 dural core
T410: i5 CPU 2.67GHz quad core
2 test result
2.1 test case1: single mass storage device performance test
upstream| patched
perf(MB/s)+irq time(us) | perf(MB/s)+irq time(us)
Pandaboard A1: 25.280(avg:145,max:772) | 25.540(avg:14, max:75)
Arndale board: 29.700(avg:33, max:129) | 29.700(avg:10, max:50)
T410: 34.430(avg:17, max:154*)| 34.660(avg:12, max:155)
-
2.2 test case2: two mass storage devices' performance test
upstream| patched
perf(MB/s)+irq time(us) | perf(MB/s)+irq time(us)
Pandaboard A1: 15.840/15.580(avg:158,max:1216) | 16.500/16.160(avg:15,max:139)
Arndale board: 17.370/16.220(avg:33 max:234) | 17.480/16.200(avg:11, max:91)
T410: 21.180/19.820(avg:18 max:160) | 21.220/19.880(avg:11, max:149)
-
2.3 test case3: one uvc streaming test
- uvc device works well(on x86, luvcview can be used too and has
same result with uvc capture)
upstream| patched
irq time(us)| irq time(us)
Pandaboard A1: (avg:445, max:873) | (avg:33, max:44)
Arndale board: (avg:316, max:630) | (avg:20, max:27)
T410: (avg:39, max:107) | (avg:10, max:65)
-
2.4 test case4: one uvc streaming plus one mass storage device test
upstream| patched
perf(MB/s)+irq time(us) | perf(MB/s)+irq time(us)
Pandaboard A1: 20.340(avg:259,max:1704)| 20.390(avg:24, max:101)
Arndale board: 23.460(avg:124,max:726) | 23.370(avg:15, max:52)
T410: 28.520(avg:27, max:169) | 28.630(avg:13, max:160)
-
* On T410, sometimes read ehci status register in ehci_irq takes more
than 100us, and the problem has been reported on the link:
http://marc.info/?t=13706586731&r=1&w=2
Cc: Alan Stern
Signed-off-by: Ming Lei
---
drivers/usb/host/ehci-fsl.c |2 +-
drivers/usb/host/ehci-grlib.c |2 +-
drivers/usb/host/ehci-hcd.c |2 +-
drivers/usb/host/ehci-mv.c|2 +-
drivers/usb/host/ehci-octeon.c|2 +-
drivers/usb/host/ehci-pmcmsp.c|2 +-
drivers/usb/host/ehci-ppc-of.c|2 +-
drivers/usb/host/ehci-ps3.c |2 +-
drivers/usb/host/ehci-sead3.c |2 +-
drivers/usb/host/ehci-sh.c|2 +-
drivers/usb/host/ehci-tegra.c |2 +-
drivers/usb/host/ehci-tilegx.c|2 +-
drivers/usb/host/ehci-w90x900.c |2 +-
drivers/usb/host/ehci-xilinx-of.c |2 +-
14 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index bd831ec..330274a 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -669,7 +669,7 @@ static const struct hc_driver ehci_fsl_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags = H
[RFC PATCH v1 2/6] USB: disable IRQs deliberately when calling complete()
We disable local IRQs here in case of running complete() by
tasklet to avoid possible deadlock because drivers may call
spin_lock() to hold lock which might be acquired in one hard
interrupt handler.
The local_irq_save()/local_irq_restore() around complete()
will be removed if current USB drivers have been cleaned up
and no one may trigger the above deadlock situation when
running complete() in tasklet.
Cc: Oliver Neukum
Cc: Alan Stern
Signed-off-by: Ming Lei
---
drivers/usb/core/hcd.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index a272968..09a8263 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
/* pass ownership to the completion handler */
urb->status = status;
- urb->complete (urb);
+
+ /*
+* We disable local IRQs here in case of running complete() by
+* tasklet to avoid possible deadlock because drivers may call
+* spin_lock() to hold lock which might be acquired in one hard
+* interrupt handler.
+*
+* The local_irq_save()/local_irq_restore() around complete()
+* will be removed if current USB drivers have been cleaned up
+* and no one may trigger the above deadlock situation when
+* running complete() in tasklet.
+*/
+ if (hcd_giveback_urb_in_bh(hcd)) {
+ unsigned long flags;
+
+ local_irq_save(flags);
+ urb->complete (urb);
+ local_irq_restore(flags);
+ } else {
+ urb->complete (urb);
+ }
+
atomic_dec (&urb->use_count);
if (unlikely(atomic_read(&urb->reject)))
wake_up (&usb_kill_urb_queue);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller
Hi, On Tue, Jun 18, 2013 at 10:53:26AM -0400, Alan Stern wrote: > > > If the controllers don't want HCD core to manage the PHY they can just > > > set it > > > to some error code. > > > > they shouldn't have the choice, otherwise it'll be a bit of a PITA to > > maintain the code. ehci core tries to grab the PHY, if it's not there, > > try to continue anyway. Assume it's not needed. > > Felipe, are all these issues straightened out to your satisfaction? I > can't tell from the email thread. > > Looking through the EHCI glue source files, there appears to be a > variety of different ways of getting the PHY. It's not at all clear > that they can be moved into the ehci-hcd core (or even better, the USB > core -- which is what Chao is trying to do). yeah, Roger brought up a big problem with OMAP's EHCI depending on the mode so, at least for now, we should keep phy_get and, in case of EHCI OMAP, phy_init in the glue :-( Roger has all the details, and they're also in the list archives, but basically, depending on the mode, PHY *must* be initialized at a particular point. > Given that the glue module has to be responsible for getting the PHY, > it should also be responsible for error checking. So the code added to > hcd.c doesn't need to apply an IS_ERR check; it can simply assume that > if hcd->phy is NULL then either there is no software-controllable PHY > or else the HCD doesn't want the core to manage it. makes sense to me, add the requirement to: if (IS_ERR(hcd->phy)) hcd->phy = NULL; -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 5/9] ARM: OMAP: USB: Add phy binding information
Hi, On Tue, Jun 18, 2013 at 08:25:00PM +0530, Kishon Vijay Abraham I wrote: > Hi, > > On Tuesday 18 June 2013 06:05 PM, Felipe Balbi wrote: > >Hi, > > > >On Tue, Jun 18, 2013 at 04:43:44PM +0530, Kishon Vijay Abraham I wrote: > >>>On Tue, Jun 18, 2013 at 03:34:36PM +0530, Kishon Vijay Abraham I wrote: > On Tuesday 18 June 2013 03:14 PM, Felipe Balbi wrote: > >On Thu, Jun 13, 2013 at 02:13:55PM +0530, Kishon Vijay Abraham I wrote: > >>In order for controllers to get PHY in case of non dt boot, the phy > >>binding information (phy device name) should be added in the platform > >>data of the controller. > >> > >>Signed-off-by: Kishon Vijay Abraham I > > > >I would rather not pass strings around, any other way to handle this ? > >Why do you need to pass this string ? > > Our old way of binding the controller and the phy using device name > started creating problems after the devices are created using > PLATFORM_DEVID_AUTO. Infact non-dt boot is broken in mainline for > OMAP3 platforms for which I have posted a RFC series > http://www.serverphorums.com/read.php?12,708632 which also uses > strings. > I'm not sure of any other way to deal with this. > >>> > >>>have you checked how other frameworks handle it ? Regulator has some > >>>sort of binding in board-files, but I guess it passes the regulator > >>>name? > >> > >> From whatever I could make of, regulator has 3 ways to get the > >>regulator one of which is using the binding in board-files (but it > >>also uses device name which could create the same problem that we are > >>facing). > >> > >>1.) from dt data > >>2.) from _supply_ name > >>3.) from binding in board file > >> > >>(referred regulator_dev_lookup() in regulator/core.c) > > > >right, spot on. Which means we don't have a better, more elegant > >solution now. Let's go ahead with this. > > Ok. So I'll drop RFC and resend this patch series > http://www.serverphorums.com/read.php?12,708632 please do :-) -- balbi signature.asc Description: Digital signature
Re: [PATCH] usb: host: Faraday fotg210-hcd driver
On Tue, Jun 18, 2013 at 10:42:09AM +0800, Yuan-Hsin Chen wrote: > Hi, > > On Tue, Jun 18, 2013 at 4:39 AM, Greg KH wrote: > > On Wed, Jun 05, 2013 at 05:15:43PM +, Yuan-Hsin Chen wrote: > >> FOTG210 is an OTG controller which can be configured as an > >> USB2.0 host. FOTG210 host is an ehci-like controller with > >> some differences. First, register layout of FOTG210 is > >> incompatible with EHCI. Furthermore, FOTG210 is lack of > >> siTDs which means iTDs are used for both HS and FS ISO > >> transfer. > >> > >> Signed-off-by: Yuan-Hsin Chen > >> --- > >> drivers/usb/Makefile |1 + > >> drivers/usb/host/Kconfig | 12 + > >> drivers/usb/host/Makefile |1 + > >> drivers/usb/host/fotg210-hcd.c | 5967 > >> > >> drivers/usb/host/fotg210.h | 746 + > >> 5 files changed, 6727 insertions(+), 0 deletions(-) > >> create mode 100644 drivers/usb/host/fotg210-hcd.c > >> create mode 100644 drivers/usb/host/fotg210.h > > > > You obviously didn't even run this through checkpatch.pl, did you? > > > > $ ./scripts/checkpatch.pl --terse ../usb.mbox | tail -n 1 > > total: 138 errors, 618 warnings, 6742 lines checked > > > > Please fix all of these if you wish us to at least start reviewing the > > patch. Your internal Q/A should have caught this first, please be more > > careful in the future. > > > > Actually I did run checkpatch.pl and found that almost all errors and > warnings are from ehci core (ehci-hcd.c, ehci-hub.c and so on) where > my driver borrowed most of code. > > $ ./scripts/checkpatch.pl --terse -f drivers/usb/host/ehci-hub.c | tail -n 1 > total: 18 errors, 69 warnings, 1138 lines checked > > $ ./scripts/checkpatch.pl --terse -f drivers/usb/host/ehci-hcd.c | tail -n 1 > total: 17 errors, 78 warnings, 1403 lines checked > > So you're saying that I should fix them, is that right? At the very least, yes, you should. And those tiny numbers of errors do not line up with the much larger number of errors in your patch submission, so it isn't just a "copy and paste" issue here. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 5/9] ARM: OMAP: USB: Add phy binding information
Hi, On Tuesday 18 June 2013 06:05 PM, Felipe Balbi wrote: Hi, On Tue, Jun 18, 2013 at 04:43:44PM +0530, Kishon Vijay Abraham I wrote: On Tue, Jun 18, 2013 at 03:34:36PM +0530, Kishon Vijay Abraham I wrote: On Tuesday 18 June 2013 03:14 PM, Felipe Balbi wrote: On Thu, Jun 13, 2013 at 02:13:55PM +0530, Kishon Vijay Abraham I wrote: In order for controllers to get PHY in case of non dt boot, the phy binding information (phy device name) should be added in the platform data of the controller. Signed-off-by: Kishon Vijay Abraham I I would rather not pass strings around, any other way to handle this ? Why do you need to pass this string ? Our old way of binding the controller and the phy using device name started creating problems after the devices are created using PLATFORM_DEVID_AUTO. Infact non-dt boot is broken in mainline for OMAP3 platforms for which I have posted a RFC series http://www.serverphorums.com/read.php?12,708632 which also uses strings. I'm not sure of any other way to deal with this. have you checked how other frameworks handle it ? Regulator has some sort of binding in board-files, but I guess it passes the regulator name? From whatever I could make of, regulator has 3 ways to get the regulator one of which is using the binding in board-files (but it also uses device name which could create the same problem that we are facing). 1.) from dt data 2.) from _supply_ name 3.) from binding in board file (referred regulator_dev_lookup() in regulator/core.c) right, spot on. Which means we don't have a better, more elegant solution now. Let's go ahead with this. Ok. So I'll drop RFC and resend this patch series http://www.serverphorums.com/read.php?12,708632 Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller
On Tue, 18 Jun 2013, Felipe Balbi wrote:
> HI,
>
> On Tue, Jun 18, 2013 at 11:23:59AM +0300, Roger Quadros wrote:
> > On 06/18/2013 11:01 AM, Felipe Balbi wrote:
> > > Hi,
> > >
> > > On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> > >> Some controller need software to initialize PHY before add
> > >> host controller, and shut down PHY after remove host controller.
> > >> Add the generic code for these controllers so they do not need
> > >> do it in its own host controller driver.
> > >>
> > >> Signed-off-by: Chao Xie
> > >> ---
> > >> drivers/usb/core/hcd.c | 19 ++-
> > >> 1 files changed, 18 insertions(+), 1 deletions(-)
> > >>
> > >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > >> index d53547d..b26196b 100644
> > >> --- a/drivers/usb/core/hcd.c
> > >> +++ b/drivers/usb/core/hcd.c
> > >> @@ -43,6 +43,7 @@
> > >>
> > >> #include
> > >> #include
> > >> +#include
> > >>
> > >> #include "usb.h"
> > >>
> > >> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
> > >> */
> > >> set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> > >>
> > >> +/* Initialize the PHY before other hardware operation. */
> > >> +if (hcd->phy) {
> > >
> > > this looks wrong for two reasons:
> > >
> > > a) you're not grabbing the PHY here.
> > >
> > > You can't just assume another entity grabbed your PHY for you.
> >
> > Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
>
> right, and what I'm saying is that it should all be re-factored into
> ehci-hcd core :-)
>
> > If the controllers don't want HCD core to manage the PHY they can just set
> > it
> > to some error code.
>
> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> maintain the code. ehci core tries to grab the PHY, if it's not there,
> try to continue anyway. Assume it's not needed.
Felipe, are all these issues straightened out to your satisfaction? I
can't tell from the email thread.
Looking through the EHCI glue source files, there appears to be a
variety of different ways of getting the PHY. It's not at all clear
that they can be moved into the ehci-hcd core (or even better, the USB
core -- which is what Chao is trying to do).
Given that the glue module has to be responsible for getting the PHY,
it should also be responsible for error checking. So the code added to
hcd.c doesn't need to apply an IS_ERR check; it can simply assume that
if hcd->phy is NULL then either there is no software-controllable PHY
or else the HCD doesn't want the core to manage it.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: host: Faraday fotg210-hcd driver
On Tue, 18 Jun 2013, Yuan-Hsin Chen wrote: > > In that case, no, you should be figuring out how to refactor and reuse > > the EHCI code instead of copying it straight into your driver. > > I was trying to use ehci-platform.c, anonymous union/struct, and quirk > flags to avoid copying EHCI code. > But there are too big incompatibilities between fotg210/fusbh200 > controller and EHCI. > That's why Alan agreed that I could create a stand-alone driver for > fusbh200 host controller. > Since fotg210 and fusbh200 have the same issue, fotg210 hcd is > supposed to be stand-alone. > More details please refer to mail sequence > http://www.spinics.net/lists/linux-usb/msg83812.html That's right. The patch's description mentions some of the incompatibilities. In short, the Faraday controllers are a _very_ noncompliant EHCI variant. The changes needed to make ehci-hcd work with them were too invasive IMO. It's a shame, because so much of the code is the same. It makes you want to go back and ask those Faraday engineers what they were thinking of at the time. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/1] Intel xhci: rework EHCI/xHCI port switching
On Mon, 17 Jun 2013, Sarah Sharp wrote: > > Correct me if I'm wrong here. The original idea was to switch > > everything over to xHCI during some early stage (like when the xHCI > > controller is first passed to the pci-quirks.c code) and switch back to > > EHCI at shutdown. As a refinement, you want to switch over to xHCI > > again following system resume, in case the BIOS messed things up. Is > > there anything else? > > Yes, that was the idea. > > > Why does the old code need to be changed? The only problem I see is > > that it may be a little too elaborate. For example, why bother trying > > to do the switch when the EHCI controller resumes? Won't everything > > work okay if you wait until the xHCI controller resumes? > > The code needs to be changed because we don't want to keep adding new > PCI IDs for all the new Intel hosts. Okay. That's simple enough; just remove the product ID checks. > Mathias also didn't like that we > inefficiently walked the PCI bus, looking for the xHCI host in the EHCI > resume routine, so he stored the xHCI pointer. Perhaps this would be > more clear if these two goals were broken up into two patches? Why not just remove all that stuff from the EHCI resume routine? It would be a lot simpler. See below. > > Why does the code check so hard to see whether or not there are any > > switchable ports? Why not just always try to set the switch on any > > Intel controller? > > Not all ports may be exposed on the platform (because there are > different skews). If we don't check which ports to switchover, some > BIOSes freak out on either shutdown, or suspend. I don't remember > which. So we need to pay attention to the port switchover masks. Sorry, I wasn't clear. Yes, by all means, do check the switchover masks. But there's no need to check the product ID; checking just the vendor ID should be good enough. > > Why is it necessary to search through all the PCI devices? Why is it > > necessary to store any sort of list of switchable controllers? > > We don't know which host will resume from suspend first. > > Assume we have a broken BIOS that switches ports back to EHCI on resume. > We don't want the USB core to notice EHCI connects and start servicing > them before xHCI resumes. Therefore, we need to have both the xHCI and > the EHCI resume functions attempt to switch the ports over to xHCI. The USB core won't notice anything, because the hub driver doesn't start running again until after all the devices (including host controllers) have been resumed. In other words, the khubd thread is freezable. (This is not by some random chance; it was an explicit decision that we don't want the hub driver mucking things up while we're in the middle of a big transition like system suspend or resume. You can rely on it.) So it won't matter if a device gets switched over to EHCI for a while. When the xHCI controller resumes, the device will get switched back to it, and usbcore will see only that there was a disconnect/reconnect during suspend. > That means for EHCI, we either need to search the PCI tree for the xHCI > host on resume (because that's where the port switchover mechanism > registers are), or we need to store the xHCI PCI host pointer somewhere. > Mathias chose to store the pointer. The reasoning above shows that there's no need either to search the entire PCI tree or to store any host controller pointers. > > Why should hot-unplug be an issue? Or multiple xHCI controllers? > > This is not a problem right now, I'm perhaps being paranoid. :) For all > current Intel systems, AFAIK, there will only be one xHCI host > controller, and it will be a part of the PCH, so you won't see any PCI > hot unplug events unless someone removes the entire PCH (which I don't > know if we support?). Even if someone in the future decides to put an Intel xHCI controller on a PCIe card, there shouldn't be any trouble if you handle things as decribed above. Agreed? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 0/1] usb: check usb_hub_to_struct_hub() return value
This is a separate patch that addresses the feedback Alan Stern gave on patch "[6/6] usb: check usb_hub_to_struct_hub() return value" from the series "[RFC 0/6] xHCI and USB security bug fixes" Original patch was dropped from later versions of the series Changes since v1:: - removed checks where not needed - pass additional hub pointer argument to usb_hub_set_port_power() Mathias Nyman (1): usb: check usb_hub_to_struct_hub() return value drivers/usb/core/hub.c | 23 --- drivers/usb/core/hub.h |2 +- drivers/usb/core/port.c |4 ++-- 3 files changed, 19 insertions(+), 10 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 1/1] usb: check usb_hub_to_struct_hub() return value
usb_hub_to_struct_hub() can return NULL in some unlikely cases.
Add checks where appropriate, or pass the hub pointer as an additional
argument if it's known to be valid.
The places it makes sense to check usb_hub_to_struct_hub()
are picked based on feedback from Alan Stern.
Signed-off-by: Mathias Nyman
---
drivers/usb/core/hub.c | 23 ---
drivers/usb/core/hub.h |2 +-
drivers/usb/core/port.c |4 ++--
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index feef935..4191db3 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -718,18 +718,18 @@ static void hub_tt_work(struct work_struct *work)
/**
* usb_hub_set_port_power - control hub port's power state
- * @hdev: target hub
+ * @hdev: USB device belonging to the usb hub
+ * @hub: target hub
* @port1: port index
* @set: expected status
*
* call this function to control port's power via setting or
* clearing the port's PORT_POWER feature.
*/
-int usb_hub_set_port_power(struct usb_device *hdev, int port1,
- bool set)
+int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub,
+ int port1, bool set)
{
int ret;
- struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
struct usb_port *port_dev = hub->ports[port1 - 1];
if (set)
@@ -1769,15 +1769,17 @@ hub_ioctl(struct usb_interface *intf, unsigned int
code, void *user_data)
static int find_port_owner(struct usb_device *hdev, unsigned port1,
struct dev_state ***ppowner)
{
+ struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+
if (hdev->state == USB_STATE_NOTATTACHED)
return -ENODEV;
if (port1 == 0 || port1 > hdev->maxchild)
return -EINVAL;
- /* This assumes that devices not managed by the hub driver
+ /* Devices not managed by the hub driver
* will always have maxchild equal to 0.
*/
- *ppowner = &(usb_hub_to_struct_hub(hdev)->ports[port1 - 1]->port_owner);
+ *ppowner = &(hub->ports[port1 - 1]->port_owner);
return 0;
}
@@ -5323,7 +5325,8 @@ void usb_set_hub_port_connect_type(struct usb_device
*hdev, int port1,
{
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
- hub->ports[port1 - 1]->connect_type = type;
+ if (hub)
+ hub->ports[port1 - 1]->connect_type = type;
}
/**
@@ -5339,6 +5342,9 @@ usb_get_hub_port_connect_type(struct usb_device *hdev,
int port1)
{
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ if (!hub)
+ return USB_PORT_CONNECT_TYPE_UNKNOWN;
+
return hub->ports[port1 - 1]->connect_type;
}
@@ -5397,6 +5403,9 @@ acpi_handle usb_get_hub_port_acpi_handle(struct
usb_device *hdev,
{
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ if (!hub)
+ return NULL;
+
return DEVICE_ACPI_HANDLE(&hub->ports[port1 - 1]->dev);
}
#endif
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 80ab9ee..6508e02 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,7 +100,7 @@ extern int usb_hub_create_port_device(struct usb_hub *hub,
int port1);
extern void usb_hub_remove_port_device(struct usb_hub *hub,
int port1);
-extern int usb_hub_set_port_power(struct usb_device *hdev,
+extern int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub,
int port1, bool set);
extern struct usb_hub *usb_hub_to_struct_hub(struct usb_device *hdev);
extern int hub_port_debounce(struct usb_hub *hub, int port1,
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index b8bad29..5fd3fee 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -86,7 +86,7 @@ static int usb_port_runtime_resume(struct device *dev)
usb_autopm_get_interface(intf);
set_bit(port1, hub->busy_bits);
- retval = usb_hub_set_port_power(hdev, port1, true);
+ retval = usb_hub_set_port_power(hdev, hub, port1, true);
if (port_dev->child && !retval) {
/*
* Wait for usb hub port to be reconnected in order to make
@@ -128,7 +128,7 @@ static int usb_port_runtime_suspend(struct device *dev)
usb_autopm_get_interface(intf);
set_bit(port1, hub->busy_bits);
- retval = usb_hub_set_port_power(hdev, port1, false);
+ retval = usb_hub_set_port_power(hdev, hub, port1, false);
usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION);
usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE);
clear_bit(port1, hub->busy_bits);
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 0/4] Remove BUG() calls from xHCI driver
On 06/17/2013 08:25 PM, Alan Stern wrote: On Mon, 17 Jun 2013, Sarah Sharp wrote: The older patchset did have some useful improvements, aside from the misguided patch to make the USB core be more robust about handling NULL pointers from usb_hub_to_struct_hub(). As discussed, that could only happen if khubd binds to a hub with no ports, and there's already a fix in Greg's tree to avoid that case. In my email on May 26, I pointed out a couple of places where the proposed changes to the hub driver would make sense. Mathias, would you like to submit a patch doing what I outlined? Sure, I'll make a separate patch -Mathias -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Detecting start/stop streaming for UVC webcam with bulk transfer mode
Hi Chetan, On Tuesday 18 June 2013 11:17:40 Chetan Nanda wrote: > Hi, > > I am currently working with UVC camera device which send data using > bulk transfer for preview and capture. > I have modified UVC gadget driver to start bulk streaming on receiving > first UVC_VS_COMMIT_CONTROL request from host side. > But currently not able to detect when to stop the streaming. > > I am running Cheese Application on host side to test start/stop streaming. > UVC gadget driver's 'uvc_function_set_alt' function is getting called > when closing the cheese application, but this function is also > (sometime) getting called when starting the cheese application on HOST > side, also some time this function gets called after receiving first > COMMIT_CONTROL. > > So, currently I am not able to find a proper way for starting / > stopping streaming for UVC bulk transfer. > Anyone has successfully implemented the start/stop streaming usecase > for bulk mode? That's in my opinion one of the shortcomings of the UVC specification. There is no explicit way to start streaming on bulk endpoints. One option would be to start streaming when receiving the first IN token on the bulk endpoint, and to stop streaming when no bulk activity has been detected for a given amount of time. A bit hackish, but I'm not sure if there's any other practical way. > Any pointer would be very helpful. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 5/9] ARM: OMAP: USB: Add phy binding information
Hi, On Tue, Jun 18, 2013 at 04:43:44PM +0530, Kishon Vijay Abraham I wrote: > >On Tue, Jun 18, 2013 at 03:34:36PM +0530, Kishon Vijay Abraham I wrote: > >>On Tuesday 18 June 2013 03:14 PM, Felipe Balbi wrote: > >>>On Thu, Jun 13, 2013 at 02:13:55PM +0530, Kishon Vijay Abraham I wrote: > In order for controllers to get PHY in case of non dt boot, the phy > binding information (phy device name) should be added in the platform > data of the controller. > > Signed-off-by: Kishon Vijay Abraham I > >>> > >>>I would rather not pass strings around, any other way to handle this ? > >>>Why do you need to pass this string ? > >> > >>Our old way of binding the controller and the phy using device name > >>started creating problems after the devices are created using > >>PLATFORM_DEVID_AUTO. Infact non-dt boot is broken in mainline for > >>OMAP3 platforms for which I have posted a RFC series > >>http://www.serverphorums.com/read.php?12,708632 which also uses > >>strings. > >>I'm not sure of any other way to deal with this. > > > >have you checked how other frameworks handle it ? Regulator has some > >sort of binding in board-files, but I guess it passes the regulator > >name? > > From whatever I could make of, regulator has 3 ways to get the > regulator one of which is using the binding in board-files (but it > also uses device name which could create the same problem that we are > facing). > > 1.) from dt data > 2.) from _supply_ name > 3.) from binding in board file > > (referred regulator_dev_lookup() in regulator/core.c) right, spot on. Which means we don't have a better, more elegant solution now. Let's go ahead with this. -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 5/9] ARM: OMAP: USB: Add phy binding information
Hi, On Tuesday 18 June 2013 03:57 PM, Felipe Balbi wrote: Hi, On Tue, Jun 18, 2013 at 03:34:36PM +0530, Kishon Vijay Abraham I wrote: On Tuesday 18 June 2013 03:14 PM, Felipe Balbi wrote: On Thu, Jun 13, 2013 at 02:13:55PM +0530, Kishon Vijay Abraham I wrote: In order for controllers to get PHY in case of non dt boot, the phy binding information (phy device name) should be added in the platform data of the controller. Signed-off-by: Kishon Vijay Abraham I I would rather not pass strings around, any other way to handle this ? Why do you need to pass this string ? Our old way of binding the controller and the phy using device name started creating problems after the devices are created using PLATFORM_DEVID_AUTO. Infact non-dt boot is broken in mainline for OMAP3 platforms for which I have posted a RFC series http://www.serverphorums.com/read.php?12,708632 which also uses strings. I'm not sure of any other way to deal with this. have you checked how other frameworks handle it ? Regulator has some sort of binding in board-files, but I guess it passes the regulator name? From whatever I could make of, regulator has 3 ways to get the regulator one of which is using the binding in board-files (but it also uses device name which could create the same problem that we are facing). 1.) from dt data 2.) from _supply_ name 3.) from binding in board file (referred regulator_dev_lookup() in regulator/core.c) Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] USB: serial: make minor allocation dynamic
> > > @@ -123,8 +116,9 @@ static void return_serial(struct usb_ser > > > > > > mutex_lock(&table_lock); > > > for (i = 0; i < serial->num_ports; ++i) > > > - serial_table[serial->minor + i] = NULL; > > > + idr_remove(&serial_minors, serial->port[i]->minor); > > > mutex_unlock(&table_lock); > > > + serial->minors_reserved = 0; > > > > This isn't strictly needed as the serial struct release_serial is only > > called once when the struct is about to be freed. > > Really? Why were we doing this type of thing before with the "not > allocated" flag? It seems that we were protecting some path that I > can't remember at the moment. So to be safe, I'll leave it for now... It was and is only used when releasing the serial struct to check whether minors had been allocated or not at probe and if return_serial (release_minors) should be called. This in done in destroy_serial just before freeing the struct, so clearing the flag is redundant, but doesn't hurt anyone, I guess. ;) Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 5/9] ARM: OMAP: USB: Add phy binding information
Hi, On Tue, Jun 18, 2013 at 03:34:36PM +0530, Kishon Vijay Abraham I wrote: > On Tuesday 18 June 2013 03:14 PM, Felipe Balbi wrote: > >On Thu, Jun 13, 2013 at 02:13:55PM +0530, Kishon Vijay Abraham I wrote: > >>In order for controllers to get PHY in case of non dt boot, the phy > >>binding information (phy device name) should be added in the platform > >>data of the controller. > >> > >>Signed-off-by: Kishon Vijay Abraham I > > > >I would rather not pass strings around, any other way to handle this ? > >Why do you need to pass this string ? > > Our old way of binding the controller and the phy using device name > started creating problems after the devices are created using > PLATFORM_DEVID_AUTO. Infact non-dt boot is broken in mainline for > OMAP3 platforms for which I have posted a RFC series > http://www.serverphorums.com/read.php?12,708632 which also uses > strings. > I'm not sure of any other way to deal with this. have you checked how other frameworks handle it ? Regulator has some sort of binding in board-files, but I guess it passes the regulator name? -- balbi signature.asc Description: Digital signature
Re: [PATCH v2 5/5] ARM: OMAP1: nokia770: enable Tahvo
* Aaro Koskinen [130616 06:41]: > Add platform data for Tahvo. > > Signed-off-by: Aaro Koskinen Thanks, applying into omap-for-v3.11/board. I left out the .extcon entry to remove the dependency to the USB patches. Then the rest of the series can be queued along with other USB patches, and you can do a follow-up patch to patch in .extcon. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 5/9] ARM: OMAP: USB: Add phy binding information
Hi, On Tuesday 18 June 2013 03:14 PM, Felipe Balbi wrote: On Thu, Jun 13, 2013 at 02:13:55PM +0530, Kishon Vijay Abraham I wrote: In order for controllers to get PHY in case of non dt boot, the phy binding information (phy device name) should be added in the platform data of the controller. Signed-off-by: Kishon Vijay Abraham I I would rather not pass strings around, any other way to handle this ? Why do you need to pass this string ? Our old way of binding the controller and the phy using device name started creating problems after the devices are created using PLATFORM_DEVID_AUTO. Infact non-dt boot is broken in mainline for OMAP3 platforms for which I have posted a RFC series http://www.serverphorums.com/read.php?12,708632 which also uses strings. I'm not sure of any other way to deal with this. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/9] usb: phy: omap-usb2: use the new generic PHY framework
Hi, On Tue, Jun 18, 2013 at 03:26:23PM +0530, Kishon Vijay Abraham I wrote: > >>>On Thu, Jun 13, 2013 at 02:13:52PM +0530, Kishon Vijay Abraham I wrote: > @@ -159,6 +191,12 @@ static int omap_usb2_probe(struct platform_device > *pdev) > otg->start_srp = omap_usb_start_srp; > otg->phy= &phy->phy; > > + pm_runtime_enable(phy->dev); > >>> > >>>enabling pm_runtime here has the potential to cause a real big problem. > >>>How have you tested this patch ? > >> > >>performed boot and enumeration testing. What issue do you expect here? > > > >hint: look at drvdata usage around the driver. Where is it initialized ? > >Where is it used ? > > hmm.. since runtime_get calls weren't made very early, I think I dint > see any issues. Thanks for pointing this. right, no problem. BTW, you don't get to_platform_device() -> platform_get_drvdata(). A simple dev_get_drvdata() is sufficient :-) -- balbi signature.asc Description: Digital signature
Re: [PATCH V2 05/10] USB: OHCI: Properly handle ohci-exynos suspend
On 18 June 2013 15:24, Manjunath Goudar wrote:
>
>
> On 14 June 2013 01:22, Alan Stern wrote:
>>
>> On Thu, 13 Jun 2013, Tomasz Figa wrote:
>>
>> > > + rc = ohci_suspend(hcd, do_wakeup);
>> > > + if (rc == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
>> > > + ohci_resume(hcd, false);
>> > > + rc = -EBUSY;
>> > > + }
>> >
>> > I'm not into USB host subsystem, so I might just ask a stupid question.
>> >
>> > Can't we make ohci_suspend check this for us, so the drivers would just
>> > check for error code? It seems like in all your patches this part of
>> > code
>> > is duplicated, looking as a good candidate to be generic.
>>
>> Argh! You're right, of course.
>>
>> I didn't see it, because the only existing place where this check is
>> made is in the PCI glue layer. Pushing it into the HCDs themselves is
>> obviously the right thing to do.
>>
>> Manjanuth, let's do this. You can write a preliminary patch that puts
>> this check at the end of the ohci_suspend() routine, and then resubmit
>> your series.
>>
>
> Alan and Tomaszas you are correct.
>
> Initially I also thought same, but later I analyzed this code is not
> necessary for all bus glue; so I did not write in ohci_suspend() routine.
>
> After Alan explanation I am writing below code end of ohci_suspend()
> routine.is it correct Alan.
>
>if (do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
> ohci_resume(hcd, false);
> rc = -EBUSY;
You probably need to return this error code.
--
With warm regards,
Sachin
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] ARM: OMAP1: USB: move omap_usb_config to platform data
* Aaro Koskinen [130616 06:41]:
> Move omap_usb_config to platform data, so that OTG driver can include it.
>
> Signed-off-by: Aaro Koskinen
This is probably best queued with the USB transceiver:
Acked-by: Tony Lindgren
> ---
> arch/arm/mach-omap1/include/mach/usb.h | 38 +---
> include/linux/platform_data/usb-omap1.h | 51
> +
> 2 files changed, 52 insertions(+), 37 deletions(-)
> create mode 100644 include/linux/platform_data/usb-omap1.h
>
> diff --git a/arch/arm/mach-omap1/include/mach/usb.h
> b/arch/arm/mach-omap1/include/mach/usb.h
> index 45e5ac7..2c26305 100644
> --- a/arch/arm/mach-omap1/include/mach/usb.h
> +++ b/arch/arm/mach-omap1/include/mach/usb.h
> @@ -8,43 +8,7 @@
> #define is_usb0_device(config) 0
> #endif
>
> -struct omap_usb_config {
> - /* Configure drivers according to the connectors on your board:
> - * - "A" connector (rectagular)
> - * ... for host/OHCI use, set "register_host".
> - * - "B" connector (squarish) or "Mini-B"
> - * ... for device/gadget use, set "register_dev".
> - * - "Mini-AB" connector (very similar to Mini-B)
> - * ... for OTG use as device OR host, initialize "otg"
> - */
> - unsignedregister_host:1;
> - unsignedregister_dev:1;
> - u8 otg;/* port number, 1-based: usb1 == 2 */
> -
> - u8 hmc_mode;
> -
> - /* implicitly true if otg: host supports remote wakeup? */
> - u8 rwc;
> -
> - /* signaling pins used to talk to transceiver on usbN:
> - * 0 == usbN unused
> - * 2 == usb0-only, using internal transceiver
> - * 3 == 3 wire bidirectional
> - * 4 == 4 wire bidirectional
> - * 6 == 6 wire unidirectional (or TLL)
> - */
> - u8 pins[3];
> -
> - struct platform_device *udc_device;
> - struct platform_device *ohci_device;
> - struct platform_device *otg_device;
> -
> - u32 (*usb0_init)(unsigned nwires, unsigned is_device);
> - u32 (*usb1_init)(unsigned nwires);
> - u32 (*usb2_init)(unsigned nwires, unsigned alt_pingroup);
> -
> - int (*ocpi_enable)(void);
> -};
> +#include
>
> void omap_otg_init(struct omap_usb_config *config);
>
> diff --git a/include/linux/platform_data/usb-omap1.h
> b/include/linux/platform_data/usb-omap1.h
> new file mode 100644
> index 000..8c7764d
> --- /dev/null
> +++ b/include/linux/platform_data/usb-omap1.h
> @@ -0,0 +1,51 @@
> +/*
> + * Platform data for OMAP1 USB
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file "COPYING" in the main directory of this archive for
> + * more details.
> + */
> +#ifndef __LINUX_USB_OMAP1_H
> +#define __LINUX_USB_OMAP1_H
> +
> +#include
> +
> +struct omap_usb_config {
> + /* Configure drivers according to the connectors on your board:
> + * - "A" connector (rectagular)
> + * ... for host/OHCI use, set "register_host".
> + * - "B" connector (squarish) or "Mini-B"
> + * ... for device/gadget use, set "register_dev".
> + * - "Mini-AB" connector (very similar to Mini-B)
> + * ... for OTG use as device OR host, initialize "otg"
> + */
> + unsignedregister_host:1;
> + unsignedregister_dev:1;
> + u8 otg;/* port number, 1-based: usb1 == 2 */
> +
> + u8 hmc_mode;
> +
> + /* implicitly true if otg: host supports remote wakeup? */
> + u8 rwc;
> +
> + /* signaling pins used to talk to transceiver on usbN:
> + * 0 == usbN unused
> + * 2 == usb0-only, using internal transceiver
> + * 3 == 3 wire bidirectional
> + * 4 == 4 wire bidirectional
> + * 6 == 6 wire unidirectional (or TLL)
> + */
> + u8 pins[3];
> +
> + struct platform_device *udc_device;
> + struct platform_device *ohci_device;
> + struct platform_device *otg_device;
> +
> + u32 (*usb0_init)(unsigned nwires, unsigned is_device);
> + u32 (*usb1_init)(unsigned nwires);
> + u32 (*usb2_init)(unsigned nwires, unsigned alt_pingroup);
> +
> + int (*ocpi_enable)(void);
> +};
> +
> +#endif /* __LINUX_USB_OMAP1_H */
> --
> 1.8.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH -next] usb: gadget: f_ncm: fix missing unlock on error in ncm_alloc()
On Tuesday, June 18, 2013 5:43 AM Wei Yongjun wrote:
> From: Wei Yongjun
>
> Add the missing unlock before return from function ncm_alloc() in the
> error handling case.
>
> Introduced by commit e730660378be92b83288b59b824ccdace5cd2652.
> (usb: gadget: f_ncm: add configfs support)
>
> Signed-off-by: Wei Yongjun
Thank you for pointing this out,
Acked-by: Andrzej Pietrasiewicz
> ---
> drivers/usb/gadget/f_ncm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/f_ncm.c b/drivers/usb/gadget/f_ncm.c index
> 47a5724..952177f 100644
> --- a/drivers/usb/gadget/f_ncm.c
> +++ b/drivers/usb/gadget/f_ncm.c
> @@ -1403,6 +1403,7 @@ struct usb_function *ncm_alloc(struct
> usb_function_instance *fi)
> sizeof(ncm->ethaddr));
> if (status < 12) { /* strlen("01234567890a") */
> kfree(ncm);
> + mutex_unlock(&opts->lock);
> return ERR_PTR(-EINVAL);
> }
> ncm_string_defs[STRING_MAC_IDX].s = ncm->ethaddr;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH -next] usb: gadget: f_subset: fix missing unlock on error in geth_alloc()
On Tuesday, June 18, 2013 5:41 AM Wei Yongjun wrote:
> From: Wei Yongjun
>
> Add the missing unlock before return from function geth_alloc() in the
> error handling case.
>
> Introduced by commit 02832e56f88a981474ee4c7c141f46fc1b4454f4.
> (usb: gadget: f_subset: add configfs support)
>
> Signed-off-by: Wei Yongjun
Thank you for pointing this out,
Acked-by: Andrzej Pietrasiewicz
> ---
> drivers/usb/gadget/f_subset.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/f_subset.c b/drivers/usb/gadget/f_subset.c
> index fbc7a24..5601e1d 100644
> --- a/drivers/usb/gadget/f_subset.c
> +++ b/drivers/usb/gadget/f_subset.c
> @@ -548,6 +548,7 @@ static struct usb_function *geth_alloc(struct
> usb_function_instance *fi)
> sizeof(geth->ethaddr));
> if (status < 12) {
> kfree(geth);
> + mutex_unlock(&opts->lock);
> return ERR_PTR(-EINVAL);
> }
> geth_string_defs[1].s = geth->ethaddr;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH -next] usb: gadget: f_ecm: fix missing unlock on error in ecm_alloc()
On Tuesday, June 18, 2013 6:21 AM Wei Yongjun wrote:
> From: Wei Yongjun
>
> Add the missing unlock before return from function ecm_alloc() in the
> error handling case.
>
> Introduced by commit da92801c647cdebfd45001fd6aaecb8f0be7f56b.
> (usb: gadget: f_ecm: add configfs support)
>
> Signed-off-by: Wei Yongjun
Thank you for pointing this out,
Acked-by: Andrzej Pietrasiewicz
> ---
> drivers/usb/gadget/f_ecm.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/f_ecm.c b/drivers/usb/gadget/f_ecm.c index
> fcafe1a..5d3561e 100644
> --- a/drivers/usb/gadget/f_ecm.c
> +++ b/drivers/usb/gadget/f_ecm.c
> @@ -1012,6 +1012,7 @@ struct usb_function *ecm_alloc(struct
> usb_function_instance *fi)
> sizeof(ecm->ethaddr));
> if (status < 12) {
> kfree(ecm);
> + mutex_unlock(&opts->lock);
> return ERR_PTR(-EINVAL);
> }
> ecm_string_defs[1].s = ecm->ethaddr;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/9] usb: phy: omap-usb2: use the new generic PHY framework
Hi, On Tuesday 18 June 2013 03:20 PM, Felipe Balbi wrote: Hi, On Tue, Jun 18, 2013 at 03:19:03PM +0530, Kishon Vijay Abraham I wrote: Hi, On Tuesday 18 June 2013 03:10 PM, Felipe Balbi wrote: Hi, On Thu, Jun 13, 2013 at 02:13:52PM +0530, Kishon Vijay Abraham I wrote: @@ -159,6 +191,12 @@ static int omap_usb2_probe(struct platform_device *pdev) otg->start_srp = omap_usb_start_srp; otg->phy = &phy->phy; + pm_runtime_enable(phy->dev); enabling pm_runtime here has the potential to cause a real big problem. How have you tested this patch ? performed boot and enumeration testing. What issue do you expect here? hint: look at drvdata usage around the driver. Where is it initialized ? Where is it used ? hmm.. since runtime_get calls weren't made very early, I think I dint see any issues. Thanks for pointing this. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime
Hi,
On Tue, Jun 18, 2013 at 11:24:40AM +0200, Sebastian Andrzej Siewior wrote:
> On 06/18/2013 10:54 AM, Felipe Balbi wrote:
> > Hi,
>
> Hi Felipe,
>
> >> This isn't exactly a child node, is it? There is
> >> of_get_child_count() but this isn't a child, it is one property.
> >
> > is it because we haven't added DTS support for musb core ?
> > Eventually we can/should add and convert this to
> > of_get_child_count() then. For now, we can go ahead with your
> > approach.
>
> mother {
>child1 {
>};
>child1 {
>};
> };
>
> That would be a child imho.
> For counting of phandles (for the number of assigned gpios for instance)
> you would use
> of_count_phandle_with_args(node, "gpios", "#gpio-cell-size");
>
> We don't have an equivalent for "#gpio-cell-size". In our case it would
> be the sum of "#address-cells" and "#size-cells" minus 1 because we
> don't have a phandle and abuse a function :)
> Counting the number of "addresses" is special since bother its
> members (address and size) can be either 32bit or 64bit in size.
>
> > We have such a large amount of function pointers to sort out anyway
> > :-(
>
> If you want get the while() loop replaced by something else I have a
> few suggestions:
> - check for iomem(2). If it is there we have two instances. If not,
> just 1. This includes also the hope that we don't get a third port.
>
> - loop over of_address_to_resource(). This is what of_device_alloc() is
> doing:
>if (of_can_translate_address(np))
> while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> num_reg++;
> So it is different.
let's keep it as it is, I think once we convert musb-core to dt, all
those issues will vanish and we will be able to use of_get_child_count()
anyway. Forget my noise :-)
--
balbi
signature.asc
Description: Digital signature
Re: [Application RFC PATCH] ptp-gadget: move from gadgetfs to functionfs
Hi Michael On Mon, 17 Jun 2013, Michael Grzeschik wrote: > Hi Guennadi, > > On Thu, Jun 06, 2013 at 12:34:39AM +0200, Michael Grzeschik wrote: > > This patch moves the ptp-gadget to use functionfs for the endpoint > > handling. With functionfs, this patch is deleting a lot of control ep0 > > code, which is now handled by the framework. > > Ping! Do you know who is maintaining this code and/or > could be interested in this work? Yes, I'm still - formally - the maintainer of the code. Fortunately the patch rate is quite low, but still - I have little time to take care of them when they arrive, and if they're non-trivial, my expertise level in this area isn't high enough to process patches quickly. Also in this case to really review patch contents I would first have to study the functionfs. Besides I don't have a platform, on which I _easily_ could test the software, I would first have to identify, which one is best suitable. So, if someone wants to take over mainternership of this package, I wouldn't mind. Anatolij? Anyone@DENX? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/9] usb: phy: omap-usb2: use the new generic PHY framework
Hi, On Tue, Jun 18, 2013 at 03:19:03PM +0530, Kishon Vijay Abraham I wrote: > Hi, > > On Tuesday 18 June 2013 03:10 PM, Felipe Balbi wrote: > >Hi, > > > >On Thu, Jun 13, 2013 at 02:13:52PM +0530, Kishon Vijay Abraham I wrote: > >>@@ -159,6 +191,12 @@ static int omap_usb2_probe(struct platform_device > >>*pdev) > >>otg->start_srp = omap_usb_start_srp; > >>otg->phy= &phy->phy; > >> > >>+ pm_runtime_enable(phy->dev); > > > >enabling pm_runtime here has the potential to cause a real big problem. > >How have you tested this patch ? > > performed boot and enumeration testing. What issue do you expect here? hint: look at drvdata usage around the driver. Where is it initialized ? Where is it used ? -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 0/9] Generic PHY Framework
Hi, On Mon, Jun 17, 2013 at 12:16:35PM +0200, Sylwester Nawrocki wrote: > I have already used this API for our MIPI CSI-2/DSIM DPHYs driver, > the RFC patch series can be found at [1]. > > Thanks, > Sylwester > > [1] http://www.spinics.net/lists/arm-kernel/msg251666.html one comment to that series: let's make the phy names standardized, how about phy-exynos-video-mipi.c or phy-mipi-csi-dsim.c ? -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 8/9] usb: phy: omap-usb2: remove *set_suspend* callback from omap-usb2
On Thu, Jun 13, 2013 at 02:13:58PM +0530, Kishon Vijay Abraham I wrote: > Now that omap-usb2 is adapted to the new generic PHY framework, > *set_suspend* ops can be removed from omap-usb2 driver. > > Signed-off-by: Kishon Vijay Abraham I Reviewed-by: Felipe Balbi -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 9/9] usb: phy: twl4030-usb: remove *set_suspend* and *phy_init* ops
On Thu, Jun 13, 2013 at 02:13:59PM +0530, Kishon Vijay Abraham I wrote: > Now that twl4030-usb is adapted to the new generic PHY framework, > *set_suspend* and *phy_init* ops can be removed from twl4030-usb driver. > > Signed-off-by: Kishon Vijay Abraham I Reviewed-by: Felipe Balbi -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 2/9] usb: phy: omap-usb2: use the new generic PHY framework
Hi, On Tuesday 18 June 2013 03:10 PM, Felipe Balbi wrote: Hi, On Thu, Jun 13, 2013 at 02:13:52PM +0530, Kishon Vijay Abraham I wrote: @@ -159,6 +191,12 @@ static int omap_usb2_probe(struct platform_device *pdev) otg->start_srp = omap_usb_start_srp; otg->phy = &phy->phy; + pm_runtime_enable(phy->dev); enabling pm_runtime here has the potential to cause a real big problem. How have you tested this patch ? performed boot and enumeration testing. What issue do you expect here? Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 7/9] usb: musb: omap2430: use the new generic PHY framework
Hi,
On Thu, Jun 13, 2013 at 02:13:57PM +0530, Kishon Vijay Abraham I wrote:
> Use the generic PHY framework API to get the PHY. The usb_phy_set_resume
> and usb_phy_set_suspend is replaced with power_on/get_sync and
> power_off/put_sync to align with the new PHY framework.
>
> musb->xceiv can't be removed as of now because musb core uses xceiv.state and
> xceiv.otg. Once there is a separate state machine to handle otg, these can be
> moved out of xceiv and then we can start using the generic PHY framework.
>
> Signed-off-by: Kishon Vijay Abraham I
> ---
> drivers/usb/musb/musb_core.c |1 +
> drivers/usb/musb/musb_core.h |3 +++
> drivers/usb/musb/omap2430.c | 29 +
> 3 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 37a261a..f732bcc 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1864,6 +1864,7 @@ musb_init_controller(struct device *dev, int nIrq, void
> __iomem *ctrl)
> musb->board_set_power = plat->set_power;
> musb->min_power = plat->min_power;
> musb->ops = plat->platform_ops;
> + musb->phy_label = plat->phy_label;
>
> /* The musb_platform_init() call:
>* - adjusts musb->mregs
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index 7fb4819..498ae21 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -46,6 +46,7 @@
> #include
> #include
> #include
> +#include
>
> struct musb;
> struct musb_hw_ep;
> @@ -357,6 +358,7 @@ struct musb {
> u16 int_tx;
>
> struct usb_phy *xceiv;
> + struct phy *phy;
>
> int nIrq;
> unsignedirq_wake:1;
> @@ -434,6 +436,7 @@ struct musb {
> unsigneddouble_buffer_not_ok:1;
>
> struct musb_hdrc_config *config;
> + const char *phy_label;
>
> #ifdef MUSB_CONFIG_PROC_FS
> struct proc_dir_entry *proc_entry;
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 628b93f..c62a004 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -348,14 +348,24 @@ static int omap2430_musb_init(struct musb *musb)
>* up through ULPI. TWL4030-family PMICs include one,
>* which needs a driver, drivers aren't always needed.
>*/
> - if (dev->parent->of_node)
> + if (dev->parent->of_node) {
> + musb->phy = devm_phy_get(dev->parent, "usb2-phy");
> +
> + /* We can't totally remove musb->xceiv as of now because
> + * musb core uses xceiv.state and xceiv.otg. Once we have
> + * a separate state machine to handle otg, these can be moved
> + * out of xceiv and then we can start using the generic PHY
> + * framework
> + */
> musb->xceiv = devm_usb_get_phy_by_phandle(dev->parent,
> "usb-phy", 0);
> - else
> + } else {
> musb->xceiv = devm_usb_get_phy_dev(dev, 0);
> + musb->phy = devm_phy_get(dev, musb->phy_label);
> + }
>
> - if (IS_ERR(musb->xceiv)) {
> - status = PTR_ERR(musb->xceiv);
> + if (IS_ERR(musb->xceiv) || IS_ERR(musb->phy)) {
> + status = PTR_ERR(musb->xceiv) | PTR_ERR(musb->phy);
>
> if (status == -ENXIO)
> return status;
> @@ -397,9 +407,10 @@ static int omap2430_musb_init(struct musb *musb)
> if (glue->status != OMAP_MUSB_UNKNOWN)
> omap_musb_set_mailbox(glue);
>
> - usb_phy_init(musb->xceiv);
> + phy_init(musb->phy);
>
> pm_runtime_put_noidle(musb->controller);
> + phy_pm_runtime_put(musb->phy);
see, weird unbalanced calls :-)
Make it all explicit:
phy_pm_runtime_get_sync(phy);
phy_init(phy);
phy_pm_runtime_put(phy);
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v7 6/9] ARM: dts: omap: update usb_otg_hs data
On Thu, Jun 13, 2013 at 02:13:56PM +0530, Kishon Vijay Abraham I wrote: > Updated the usb_otg_hs dt data to include the *phy* and *phy-names* > binding in order for the driver to use the new generic PHY framework. > Also updated the Documentation to include the binding information. > The PHY binding information can be found at > Documentation/devicetree/bindings/phy/phy-bindings.txt > > Signed-off-by: Kishon Vijay Abraham I Reviewed-by: Felipe Balbi -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 5/9] ARM: OMAP: USB: Add phy binding information
On Thu, Jun 13, 2013 at 02:13:55PM +0530, Kishon Vijay Abraham I wrote: > In order for controllers to get PHY in case of non dt boot, the phy > binding information (phy device name) should be added in the platform > data of the controller. > > Signed-off-by: Kishon Vijay Abraham I I would rather not pass strings around, any other way to handle this ? Why do you need to pass this string ? -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 4/9] usb: phy: twl4030: twl4030 shouldn't be subsys_initcall
On Thu, Jun 13, 2013 at 02:13:54PM +0530, Kishon Vijay Abraham I wrote: > Changed the inticall from subsys_initcall to module_init for > twl4030-usb. > > Signed-off-by: Kishon Vijay Abraham I not part of the series, should be sent separately. I'll queue this one for v3.12 once v3.11-rc1 is tagged. -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 3/9] usb: phy: twl4030: use the new generic PHY framework
On Thu, Jun 13, 2013 at 02:13:53PM +0530, Kishon Vijay Abraham I wrote: > Used the generic PHY framework API to create the PHY. For powering on > and powering off the PHY, power_on and power_off ops are used. Once the > MUSB OMAP glue is adapted to the new framework, the suspend and resume > ops of usb phy library will be removed. > > However using the old usb phy library cannot be completely removed > because otg is intertwined with phy and moving to the new > framework completely will break otg. Once we have a separate otg state > machine, > we can get rid of the usb phy library. > > Signed-off-by: Kishon Vijay Abraham I Reviewed-by: Felipe Balbi -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 2/9] usb: phy: omap-usb2: use the new generic PHY framework
Hi, On Thu, Jun 13, 2013 at 02:13:52PM +0530, Kishon Vijay Abraham I wrote: > @@ -159,6 +191,12 @@ static int omap_usb2_probe(struct platform_device *pdev) > otg->start_srp = omap_usb_start_srp; > otg->phy= &phy->phy; > > + pm_runtime_enable(phy->dev); enabling pm_runtime here has the potential to cause a real big problem. How have you tested this patch ? -- balbi signature.asc Description: Digital signature
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi,
On Thu, Jun 13, 2013 at 02:13:51PM +0530, Kishon Vijay Abraham I wrote:
> +struct phy_provider *of_phy_provider_register(struct device *dev,
> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> + struct of_phandle_args *args))
I would rename this to __of_phy_provider_register() and in a header
have:
#define of_phy_provider_register(dev, xlate)\
__of_phy_provider_register((dev), THIS_MODULE, (xlate))
then your users don't need to remember always passing THIS_MODULE
argument.
> +struct phy_provider *devm_of_phy_provider_register(struct device *dev,
> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> + struct of_phandle_args *args))
likewise.
> +/**
> + * phy_release() - release the phy
> + * @dev: the dev member within phy
> + *
> + * When the last reference to the device is removed, it is called
> + * from the embedded kobject as release method.
> + */
> +static void phy_release(struct device *dev)
> +{
> + struct phy *phy;
> +
> + phy = container_of(dev, struct phy, dev);
I would have a:
#define to_phy(dev) (container_of((dev), struct phy dev))
somewhere in a header, just for the sake of brevity :-p
> + dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
make it dev_vdbg() instead.
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> new file mode 100644
> index 000..df0c98a
> --- /dev/null
> +++ b/include/linux/phy/phy.h
[ ... ]
> +static inline int phy_init(struct phy *phy)
> +{
> + int ret = -ENOTSUPP;
> +
> + if (!pm_runtime_enabled(&phy->dev))
> + goto no_pm_runtime;
> +
> + ret = pm_runtime_get_sync(&phy->dev);
> + if (ret < 0) {
> + dev_err(&phy->dev, "get sync failed: %d\n", ret);
> + return ret;
> + }
> +
> +no_pm_runtime:
you can avoid this goto if you code as below, note that I'm also solving
a possible unbalanced PM runtime call which would prevent actual
*runtime* suspend of the PHY:
ret = phy_pm_runtime_get_sync(phy);
if (ret < 0 && ret != -ENOTSUPP)
return ret;
if (phy->ops->init) {
ret = phy->ops->init(phy);
if (ret < 0) {
dev_err(&phy->dev, "failed to initialize --> %d\n",
ret);
goto out;
}
}
out:
/*
* In order to avoid unbalanced PM runtime calls, let's make
* sure to disable what we might have enabled when entering this
* function.
*/
phy_pm_runtime_put(phy);
return ret;
> +}
> +
> +static inline int phy_exit(struct phy *phy)
> +{
> + int ret = -ENOTSUPP;
> +
> + if (phy->ops->exit)
> + ret = phy->ops->exit(phy);
> +
> + if (!pm_runtime_enabled(&phy->dev))
> + goto no_pm_runtime;
> +
> + ret = pm_runtime_put_sync(&phy->dev);
move your pm runtime wrappers before these calls and you can use them.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller
On Tue, Jun 18, 2013 at 4:48 PM, Felipe Balbi wrote: > Hi, > > On Tue, Jun 18, 2013 at 11:45:05AM +0300, Roger Quadros wrote: >> > this looks wrong for two reasons: >> > >> > a) you're not grabbing the PHY here. >> > >> > You can't just assume another entity grabbed your PHY for you. >> >> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, >> etc? >> >>> >> >>> right, and what I'm saying is that it should all be re-factored into >> >>> ehci-hcd core :-) >> >>> >> If the controllers don't want HCD core to manage the PHY they can just >> set it >> to some error code. >> >>> >> >>> they shouldn't have the choice, otherwise it'll be a bit of a PITA to >> >>> maintain the code. ehci core tries to grab the PHY, if it's not there, >> >>> try to continue anyway. Assume it's not needed. >> >>> >> >> >> >> OK fine, but ehci-omap is a weird case as it needs a slightly different >> >> sequence as to when PHY is initialized depending on which mode it is. >> >> (Transceiver >> >> or transceiver-less). please see this fix. >> >> http://www.spinics.net/lists/stable/msg12106.html >> >> >> >> All I'm saying as that ehci-omap needs a way to tell hcd core that it >> >> needs PHY >> >> handling for itself. >> > >> > why don't you do that always ? Meaning, why don't you *always* take PHY >> > out of suspend ? If PHY is suspended, you can't wakeup unless you have >> > (in OMAP case) pad wakeup working, right ? >> > >> >> Maybe I wasn't clear before. This is nothing about wakeup and e always take >> PHY out of suspend. >> The problem is when to take it out of suspend relative to when EHCI >> controller starts. >> Let me clarify. >> >> In Transceiver mode we need this. >> >> - bring phy out of reset >> - start EHCI controller >> >> Whereas for Transceiver-less mode we need this. >> >> - start EHCI controller >> - bring phy out of reset >> >> If there is some way to signal this behaviour to the HCD core, it >> should be good enough. > > alright, now I get it. That's quite messed up that it has to be this way > :-p > It depends on the host controler's driver ehci-xxx to get the phy and set it to hcd->phy. If some controller do not want HCD or EHCI-HCD to do the phy initialization and shutdown, just do not set hcd->phy, and it will be NULL. If the host controller driver successlly get the phy, it can set hcd->phy, or it need return false in its probe. > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime
On 06/18/2013 10:54 AM, Felipe Balbi wrote:
> Hi,
Hi Felipe,
>> This isn't exactly a child node, is it? There is
>> of_get_child_count() but this isn't a child, it is one property.
>
> is it because we haven't added DTS support for musb core ?
> Eventually we can/should add and convert this to
> of_get_child_count() then. For now, we can go ahead with your
> approach.
mother {
child1 {
};
child1 {
};
};
That would be a child imho.
For counting of phandles (for the number of assigned gpios for instance)
you would use
of_count_phandle_with_args(node, "gpios", "#gpio-cell-size");
We don't have an equivalent for "#gpio-cell-size". In our case it would
be the sum of "#address-cells" and "#size-cells" minus 1 because we
don't have a phandle and abuse a function :)
Counting the number of "addresses" is special since bother its
members (address and size) can be either 32bit or 64bit in size.
> We have such a large amount of function pointers to sort out anyway
> :-(
If you want get the while() loop replaced by something else I have a
few suggestions:
- check for iomem(2). If it is there we have two instances. If not,
just 1. This includes also the hope that we don't get a third port.
- loop over of_address_to_resource(). This is what of_device_alloc() is
doing:
if (of_can_translate_address(np))
while (of_address_to_resource(np, num_reg, &temp_res) == 0)
num_reg++;
So it is different.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] build some drivers only when compile-testing
On 06/18/2013 10:51 AM, Felipe Balbi wrote:
> right, but my argument is that I rather not have either. Depend on
> PCI if you us PCI, depend on EXTCON if you use extcon, but no
> driver should have an ARCH dependency. Specially since it lets
> people include mach/* and asm/* headers because "it doesn't break
> compilation for anyone".
The argument is elsewhere. If I understand correctly, Kconfig is for
users, not to be hi-jacked by kernel developers. And users should not
really care about our development processes, cross compilations or
whatever bells and whistles we use. They just don't want to see
drivers which they have no way to *use*, they indeed don't care
whether some more compile at all. We do not want every kernel packager
for every distro out in the wild, to go through all the help texts, to
see whether they should compile and package a driver or not. It's a
tedious work and this option would save time to the packagers.
Try to package and maintain a kernel for a distribution, you will find
out what a cool and surprising work that is...
In the best case I would vote for hard dependencies as cross-compilers
are easy to obtain and set up nowadays. But well, we still want to
("cross") compile drivers, so let's add COMPILE_TEST and use that to
save time to automatic builds.
--
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime
Hi,
On Tue, Jun 18, 2013 at 10:43:55AM +0200, Sebastian Andrzej Siewior wrote:
> >> --- a/drivers/usb/musb/musb_dsps.c +++
> >> b/drivers/usb/musb/musb_dsps.c @@ -110,8 +110,6 @@ struct
> >> dsps_musb_wrapper { @@ -646,6 +645,23 @@ static int
> >> dsps_probe(struct platform_device *pdev) }
> >> platform_set_drvdata(pdev, glue);
> >>
> >> + i = 1; +do { + iomem = platform_get_resource(pdev,
> >> IORESOURCE_MEM, i);
> >
> > IIRC this index starts at zero, no ? Meaning that this should be:
> >
> > i = 0 do { iomem = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > if (!iomem) break;
> >
> > i++; } while (true);
> >
> > glue->instances = i + 1;
>
> No. 3x ioadress i.e. 0, 1, 2 means two instances because we have 0 as
> the glue io address and two musb port (1 and 2).
heh, true.
> > Also, why are you getting the resource and doing nothing with it ?
> > Is it just to figure out the amount of instances ? Isn't there a DT
> > helper to
>
> exactly, just to count.
>
> > count how many child nodes a certain node has ?
> This isn't exactly a child node, is it? There is of_get_child_count()
> but this isn't a child, it is one property.
is it because we haven't added DTS support for musb core ? Eventually we
can/should add and convert this to of_get_child_count() then. For now,
we can go ahead with your approach.
We have such a large amount of function pointers to sort out anyway :-(
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] build some drivers only when compile-testing
Hi, On Tue, Jun 18, 2013 at 10:44:52AM +0200, Michal Marek wrote: > > On Tue, Jun 18, 2013 at 10:24:40AM +0200, Jiri Slaby wrote: > > Sam Ravnborg (the kconfig ex-maintainer) once wrote that > > he doesn't want to extend the kconfig language for this > > purpose (which I support). That a config option is fine and > > sufficient in this case [1]. Except he called the config > > option "SHOW_ALL_DRIVERS". Adding the current maintainer to > > CCs ;). > > I agree with Sam. 'depends on XY || COMPILE_TEST' is quite > self-explanatory. And even if it's not, you can look up the > help text for COMPILE_TEST. With "archdepends on" or > "available on", you need to know what to look for to override > the dependency. > >>> > >>> you will still end up with: > >>> > >>> depends on (ARCH_OMAP || ARCH_EXYNOS || ARCH_DAVINCI || > >>> ARCH_PPC || ...) > >>> > >>> And every now and again that particular line will be updated > >>> to add another arch dependency. > >> > >> But that is perfectly fine *when* the driver is supported on > >> those archs only. > >> > >> And come on, how much often will this "every now and again" > >> happen? We don't have that much cases where a driver is augmented > >> to work on another arch or platform. It either works on all of > >> them => doesn't need COMPILE_TEST, or work on one or two arches > >> at most. > > > > MUSB alone has 8 different arch choices. Before, it used to be that > > core driver was dependendent on all of them, so whenever someone > > wanted to build MUSB for another arch, they had to introdude their > > glue code and modify the dependency of the core driver. > > But that you have complex dependencies in some drivers is mostly > orthogonal to the two choices of syntax, isn't it? > > depends on ARCH1 || ARCH2 || || ARCH8 || COMPILE_TEST > > vs. > > archdepends on ARCH1 || ARCH2 || || ARCH8 right, but my argument is that I rather not have either. Depend on PCI if you us PCI, depend on EXTCON if you use extcon, but no driver should have an ARCH dependency. Specially since it lets people include mach/* and asm/* headers because "it doesn't break compilation for anyone". -- balbi signature.asc Description: Digital signature
Re: [PATCH] build some drivers only when compile-testing
Dne 18.6.2013 10:34, Felipe Balbi napsal(a): > Hi, > > On Tue, Jun 18, 2013 at 10:24:40AM +0200, Jiri Slaby wrote: > Sam Ravnborg (the kconfig ex-maintainer) once wrote that > he doesn't want to extend the kconfig language for this > purpose (which I support). That a config option is fine and > sufficient in this case [1]. Except he called the config > option "SHOW_ALL_DRIVERS". Adding the current maintainer to > CCs ;). I agree with Sam. 'depends on XY || COMPILE_TEST' is quite self-explanatory. And even if it's not, you can look up the help text for COMPILE_TEST. With "archdepends on" or "available on", you need to know what to look for to override the dependency. >>> >>> you will still end up with: >>> >>> depends on (ARCH_OMAP || ARCH_EXYNOS || ARCH_DAVINCI || >>> ARCH_PPC || ...) >>> >>> And every now and again that particular line will be updated >>> to add another arch dependency. >> >> But that is perfectly fine *when* the driver is supported on >> those archs only. >> >> And come on, how much often will this "every now and again" >> happen? We don't have that much cases where a driver is augmented >> to work on another arch or platform. It either works on all of >> them => doesn't need COMPILE_TEST, or work on one or two arches >> at most. > > MUSB alone has 8 different arch choices. Before, it used to be that > core driver was dependendent on all of them, so whenever someone > wanted to build MUSB for another arch, they had to introdude their > glue code and modify the dependency of the core driver. But that you have complex dependencies in some drivers is mostly orthogonal to the two choices of syntax, isn't it? depends on ARCH1 || ARCH2 || || ARCH8 || COMPILE_TEST vs. archdepends on ARCH1 || ARCH2 || || ARCH8 Michal -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller
Hi, On Tue, Jun 18, 2013 at 11:45:05AM +0300, Roger Quadros wrote: > > this looks wrong for two reasons: > > > > a) you're not grabbing the PHY here. > > > > You can't just assume another entity grabbed your PHY for you. > > Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, > etc? > >>> > >>> right, and what I'm saying is that it should all be re-factored into > >>> ehci-hcd core :-) > >>> > If the controllers don't want HCD core to manage the PHY they can just > set it > to some error code. > >>> > >>> they shouldn't have the choice, otherwise it'll be a bit of a PITA to > >>> maintain the code. ehci core tries to grab the PHY, if it's not there, > >>> try to continue anyway. Assume it's not needed. > >>> > >> > >> OK fine, but ehci-omap is a weird case as it needs a slightly different > >> sequence as to when PHY is initialized depending on which mode it is. > >> (Transceiver > >> or transceiver-less). please see this fix. > >> http://www.spinics.net/lists/stable/msg12106.html > >> > >> All I'm saying as that ehci-omap needs a way to tell hcd core that it > >> needs PHY > >> handling for itself. > > > > why don't you do that always ? Meaning, why don't you *always* take PHY > > out of suspend ? If PHY is suspended, you can't wakeup unless you have > > (in OMAP case) pad wakeup working, right ? > > > > Maybe I wasn't clear before. This is nothing about wakeup and e always take > PHY out of suspend. > The problem is when to take it out of suspend relative to when EHCI > controller starts. > Let me clarify. > > In Transceiver mode we need this. > > - bring phy out of reset > - start EHCI controller > > Whereas for Transceiver-less mode we need this. > > - start EHCI controller > - bring phy out of reset > > If there is some way to signal this behaviour to the HCD core, it > should be good enough. alright, now I get it. That's quite messed up that it has to be this way :-p -- balbi signature.asc Description: Digital signature
