Re: [PATCH] usb: move usb_calc_bus_time into common code

[Doug Anderson]

John,

On Fri, Feb 19, 2016 at 6:55 PM, John Youn  wrote:
> On 2/19/2016 2:48 PM, Doug Anderson wrote:
>> Hi,
>>
>> On Fri, Feb 19, 2016 at 1:52 PM, Alan Stern  
>> wrote:
>>> On Fri, 19 Feb 2016, Arnd Bergmann wrote:
>>>
 The dwc2 dual-role USB controller driver has started calling
 usb_calc_bus_time, and does so regardless of whether it is
 being built as a host controller or gadget. In case all
>>>
>>> Out of curiosity...  Why does dwc2 need to calculate bus times when it
>>> is in device mode?
>>
>> Hoo boy.  it doesn't.  Nor does it need to properly set the even/odd
>> frame in device mode.
>>
>> Basically dwc2's "core.c" has a whole bunch of stuff in it that's host
>> only and isn't guarded with any #ifdef.  ...yet that file is included
>> in gadget-only builds.  It's a bit of a mess.  Take a gander at all of
>> the "dwc2_hc_xxx" functions sitting in "drivers/usb/dwc2/core.c".
>>
>> Long term that needs to be cleaned up, but such a cleanup is going to
>> be a bit of churn so we'd need to schedule it for a time when not much
>> else is going on (and presumably it should be done by John or in close
>> coordination with him so it can get Acked / landed quickly to avoid
>> conflicts?).  To do this we'd have to figure out why some things are
>> in "core.c" and some in "hcd.c" and if it makes sense to move them all
>> to "hcd.c" or if we need a new "core_hc.c" or something...  All of
>> that design predates me.
>>
>
> Yeah, that stuff has been bugging me for a while. I have a branch that
> started a lot of clean-ups, but it's never been a great time to merge
> it.
>
>> In any case I guess we need a solution for right now, huh?
>>
>> One terribly lame hack would be to just make a dummy no-op
>> "dwc2_hc_set_even_odd_frame()" if host mode is disabled.  That doesn't
>> really fix the root problem of lots of host mode code being compiled
>> in a gadget-only driver.  It also adds an ugly "#ifdef".  ...but it
>> does get around the current compile error.
>>
>>
>> What do folks think?
>>
>
> I think if we can fix it for -next in dwc2 by moving all host code out
> of core then we should do it that way. I'll see if I can revive my
> patches.

It looks like Felipe has dropped my series out of his testing/next to
avoid this issue (thanks Felipe!).  I'm interested in getting this
resolved as soon as possible to get things back in.  Is there anything
you'd like me to do to help?

-Doug
--
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: move usb_calc_bus_time into common code

[John Youn]

On 2/19/2016 2:48 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Feb 19, 2016 at 1:52 PM, Alan Stern  wrote:
>> On Fri, 19 Feb 2016, Arnd Bergmann wrote:
>>
>>> The dwc2 dual-role USB controller driver has started calling
>>> usb_calc_bus_time, and does so regardless of whether it is
>>> being built as a host controller or gadget. In case all
>>
>> Out of curiosity...  Why does dwc2 need to calculate bus times when it
>> is in device mode?
> 
> Hoo boy.  it doesn't.  Nor does it need to properly set the even/odd
> frame in device mode.
> 
> Basically dwc2's "core.c" has a whole bunch of stuff in it that's host
> only and isn't guarded with any #ifdef.  ...yet that file is included
> in gadget-only builds.  It's a bit of a mess.  Take a gander at all of
> the "dwc2_hc_xxx" functions sitting in "drivers/usb/dwc2/core.c".
> 
> Long term that needs to be cleaned up, but such a cleanup is going to
> be a bit of churn so we'd need to schedule it for a time when not much
> else is going on (and presumably it should be done by John or in close
> coordination with him so it can get Acked / landed quickly to avoid
> conflicts?).  To do this we'd have to figure out why some things are
> in "core.c" and some in "hcd.c" and if it makes sense to move them all
> to "hcd.c" or if we need a new "core_hc.c" or something...  All of
> that design predates me.
> 

Yeah, that stuff has been bugging me for a while. I have a branch that
started a lot of clean-ups, but it's never been a great time to merge
it.

> In any case I guess we need a solution for right now, huh?
> 
> One terribly lame hack would be to just make a dummy no-op
> "dwc2_hc_set_even_odd_frame()" if host mode is disabled.  That doesn't
> really fix the root problem of lots of host mode code being compiled
> in a gadget-only driver.  It also adds an ugly "#ifdef".  ...but it
> does get around the current compile error.
> 
> 
> What do folks think?
> 

I think if we can fix it for -next in dwc2 by moving all host code out
of core then we should do it that way. I'll see if I can revive my
patches.

Regards,
John
--
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: move usb_calc_bus_time into common code

[Doug Anderson]

Hi,

On Fri, Feb 19, 2016 at 1:52 PM, Alan Stern  wrote:
> On Fri, 19 Feb 2016, Arnd Bergmann wrote:
>
>> The dwc2 dual-role USB controller driver has started calling
>> usb_calc_bus_time, and does so regardless of whether it is
>> being built as a host controller or gadget. In case all
>
> Out of curiosity...  Why does dwc2 need to calculate bus times when it
> is in device mode?

Hoo boy.  it doesn't.  Nor does it need to properly set the even/odd
frame in device mode.

Basically dwc2's "core.c" has a whole bunch of stuff in it that's host
only and isn't guarded with any #ifdef.  ...yet that file is included
in gadget-only builds.  It's a bit of a mess.  Take a gander at all of
the "dwc2_hc_xxx" functions sitting in "drivers/usb/dwc2/core.c".

Long term that needs to be cleaned up, but such a cleanup is going to
be a bit of churn so we'd need to schedule it for a time when not much
else is going on (and presumably it should be done by John or in close
coordination with him so it can get Acked / landed quickly to avoid
conflicts?).  To do this we'd have to figure out why some things are
in "core.c" and some in "hcd.c" and if it makes sense to move them all
to "hcd.c" or if we need a new "core_hc.c" or something...  All of
that design predates me.

In any case I guess we need a solution for right now, huh?

One terribly lame hack would be to just make a dummy no-op
"dwc2_hc_set_even_odd_frame()" if host mode is disabled.  That doesn't
really fix the root problem of lots of host mode code being compiled
in a gadget-only driver.  It also adds an ugly "#ifdef".  ...but it
does get around the current compile error.


What do folks think?

-Doug
--
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: move usb_calc_bus_time into common code

[Alan Stern]

On Fri, 19 Feb 2016, Arnd Bergmann wrote:

> The dwc2 dual-role USB controller driver has started calling
> usb_calc_bus_time, and does so regardless of whether it is
> being built as a host controller or gadget. In case all

Out of curiosity...  Why does dwc2 need to calculate bus times when it
is in device mode?

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] usb: move usb_calc_bus_time into common code

[Arnd Bergmann]

The dwc2 dual-role USB controller driver has started calling
usb_calc_bus_time, and does so regardless of whether it is
being built as a host controller or gadget. In case all
USB host support is disabled (CONFIG_USB=n), this now causes
a link error:

ERROR: "usb_calc_bus_time" [drivers/usb/dwc2/dwc2.ko] undefined!

Moving the function that is called from the USB HCD specific code
into the shared USB code avoids this problem.

Signed-off-by: Arnd Bergmann 
Fixes: f889904b25df ("usb: dwc2: host: Properly set even/odd frame")
---
I have no idea if this is the correct solution for the problem,
it just seemed like the easiest way to avoid the build failure.

 drivers/usb/common/common.c | 51 +
 drivers/usb/core/hcd.c  | 51 -
 2 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index e3d01619d6b3..654fd37f89d5 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 const char *usb_otg_state_string(enum usb_otg_state state)
@@ -126,6 +127,56 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(usb_get_dr_mode);
 
+/*-*/
+
+/**
+ * usb_calc_bus_time - approximate periodic transaction time in nanoseconds
+ * @speed: from dev->speed; USB_SPEED_{LOW,FULL,HIGH}
+ * @is_input: true iff the transaction sends data to the host
+ * @isoc: true for isochronous transactions, false for interrupt ones
+ * @bytecount: how many bytes in the transaction.
+ *
+ * Return: Approximate bus time in nanoseconds for a periodic transaction.
+ *
+ * Note:
+ * See USB 2.0 spec section 5.11.3; only periodic transfers need to be
+ * scheduled in software, this function is only used for such scheduling.
+ */
+long usb_calc_bus_time (int speed, int is_input, int isoc, int bytecount)
+{
+   unsigned long   tmp;
+
+   switch (speed) {
+   case USB_SPEED_LOW: /* INTR only */
+   if (is_input) {
+   tmp = (67667L * (31L + 10L * BitTime (bytecount))) / 
1000L;
+   return 64060L + (2 * BW_HUB_LS_SETUP) + BW_HOST_DELAY + 
tmp;
+   } else {
+   tmp = (66700L * (31L + 10L * BitTime (bytecount))) / 
1000L;
+   return 64107L + (2 * BW_HUB_LS_SETUP) + BW_HOST_DELAY + 
tmp;
+   }
+   case USB_SPEED_FULL:/* ISOC or INTR */
+   if (isoc) {
+   tmp = (8354L * (31L + 10L * BitTime (bytecount))) / 
1000L;
+   return ((is_input) ? 7268L : 6265L) + BW_HOST_DELAY + 
tmp;
+   } else {
+   tmp = (8354L * (31L + 10L * BitTime (bytecount))) / 
1000L;
+   return 9107L + BW_HOST_DELAY + tmp;
+   }
+   case USB_SPEED_HIGH:/* ISOC or INTR */
+   /* FIXME adjust for input vs output */
+   if (isoc)
+   tmp = HS_NSECS_ISO (bytecount);
+   else
+   tmp = HS_NSECS (bytecount);
+   return tmp;
+   default:
+   pr_debug ("%s: bogus device speed!\n", __func__);
+   return -1;
+   }
+}
+EXPORT_SYMBOL_GPL(usb_calc_bus_time);
+
 #ifdef CONFIG_OF
 /**
  * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 0b8a91004737..cbaa78043793 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1170,57 +1170,6 @@ EXPORT_SYMBOL_GPL(usb_hcd_end_port_resume);
 
 /*-*/
 
-/**
- * usb_calc_bus_time - approximate periodic transaction time in nanoseconds
- * @speed: from dev->speed; USB_SPEED_{LOW,FULL,HIGH}
- * @is_input: true iff the transaction sends data to the host
- * @isoc: true for isochronous transactions, false for interrupt ones
- * @bytecount: how many bytes in the transaction.
- *
- * Return: Approximate bus time in nanoseconds for a periodic transaction.
- *
- * Note:
- * See USB 2.0 spec section 5.11.3; only periodic transfers need to be
- * scheduled in software, this function is only used for such scheduling.
- */
-long usb_calc_bus_time (int speed, int is_input, int isoc, int bytecount)
-{
-   unsigned long   tmp;
-
-   switch (speed) {
-   case USB_SPEED_LOW: /* INTR only */
-   if (is_input) {
-   tmp = (67667L * (31L + 10L * BitTime (bytecount))) / 
1000L;
-   return 64060L + (2 * BW_HUB_LS_SETUP) + BW_HOST_DELAY + 
tmp;
-   } else {
-   tmp = (66700L * (31L + 10L * BitTime (bytecount))) / 
1000L;
-   return 64107L + (2 * BW_HUB_LS_SETUP) +