[U-Boot] [PATCH] musb: set MUSB speed based on CONFIG

[Bin Liu]

Hi,

On 07/11/2015 08:04 AM, Hans de Goede wrote:
> Hi,
>
> On 10-07-15 17:31, Bin Liu wrote:
>> Hi,
>>
>> On 07/10/2015 10:12 AM, Heiko Schocher wrote:
>>> Hello Samuel,
>>>
>>> Am 10.07.2015 um 16:50 schrieb Egli, Samuel:
>>>> Hi Hans,
>>>>
>>>>> -----Original Message----- From: Hans de Goede
>>>>> [mailto:hdegoede at redhat.com] Sent: Freitag, 10. Juli 2015 16:37
>>>>> To: Egli, Samuel; marex at denx.de Cc: u-boot at lists.denx.de;
>>>>> trini at konsulko.com; Bin Liu; Meier, Roger; Daniel Mack Subject:
>>>>> Re: [U-Boot] [PATCH] musb: set MUSB speed based on CONFIG
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 10-07-15 16:30, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10-07-15 15:16, Samuel Egli wrote:
>>>>>>> From: Bin Liu <b-liu at ti.com>
>>>>>>>
>>>>>>> Do not config MUSB to highspeed mode if
>>>>>>> CONFIG_USB_GADGET_DUALSPEED is not set, in which case Ether
>>>>>>> gadget only operates in fullspeed.
>>>>>>>
>>>>>>> Note: This patch is necessary for devices like some siemens
>>>>>>> boards that allow only FULL SPEED USB 1.1, e.g. DFU
>>>>>>> download.
>>>>>>>
>>>>>>> Signed-off-by: Bin Liu <b-liu at ti.com> Reviewed-by: Tom Rini
>>>>>>> <trini at konsulko.com> Tested-by: Samuel Egli
>>>>>>> <samuel.egli at siemens.com> CC: Marek Vasut <marex at denx.de> CC:
>>>>>>> Heiko Schocher <hs at denx.de> CC: Daniel Mack
>>>>>>> <zonque at gmail.com> CC: Roger Meier <r.meier at siemens.com>
>>>>>>
>>>>>> Nack this breaks highspeed mode on boards which use the musb in
>>>>>> host mode, and thus do not set CONFIG_USB_GADGET_DUALSPEED.
>>
>> My bad, I had a short thought about this when I was initially working on
>> this patch, but did not really think about it throughly. Thanks for
>> bring it up.
>>
>>>>>
>>>>> p.s.
>>>>>
>>>>> Given that you want to use this as a hack to work around the
>>>>> broken pcb design of your board I suggest adding a new option for
>>>>> this
>>>>
>>>> Well, lets not discuss the "broken" pcb design. It seems that
>>>> wiring protection is not that common. Unfortunately, such a
>>>> protection is too expensive for USB High speed :-(.
>>
>> Agreed, we have seen many cases that have nothing to do with hardware
>> design, but MUSB has to work in full-speed mode.
>>
>>>>
>>>>> titled: CONFIG_USB_MUSB_NO_HIGHSPEED and then do:
>>>>>
>>>>> +#ifndef CONFIG_USB_MUSB_NO_HIGHSPEED | MUSB_POWER_HSENAB
>>>>> +#endif
>>>>>
>>>> This would be good enough. The point is indeed to limit it to full
>>>> speed.
>>>>
>>>>> Using CONFIG_USB_GADGET_DUALSPEED for this seems wrong, since
>>>>> this has nothing to do with enabling dualspeed mode for the
>>>>> gadget code really.
>>>>
>>>> I agree that the name is confusing.
>>>
>>> Yes, I vote for Hans suggestion.
>>
>> *Adding* a new macro CONFIG_USB_MUSB_NO_HIGHSPEED for musb driver causes
>> CONFIG_USB_GADGET_DUALSPEED redundant, because both control for
>> full-speed.
>>
>> I suggest to *rename* CONFIG_USB_GADGET_DUALSPEED to
>> CONFIG_USB_FULLSPEED_ONLY. So that we can use one macro for both gadget
>> and musb drivers. Considering supper-speed exists and future, I think
>> CONFIG_USB_NO_HIGHSPEED is confusing...
>
> Sorry I was too fast. "CONFIG_USB_FULLSPEED_ONLY" is IMHO not acceptable
> as it is not granular enough. sunxi boards have both EHCI/OHCI usb
> controller
> pairs for host ports and an musb controller. One may be limited to
> fullspeed
> while the other is not.
>
> Likewise I can see a case where some ports but not all ports are fullspeed
> only, so we still want to build the gadged code with dual-speed
> descriptors.

Good point.

>
> I really believe that something like:
>
> CONFIG_MUSB_FULLSPEED_ONLY

I am not sure like the idea of using two macros in config.h to control 
one thing. Please review the following patch which uses 
CONFIG_USB_GADGET_DUALSPEED and musb->board_mode instead. Beware that 
this patch is not tested.

diff --git a/drivers/usb/musb-new/musb_core.c 
b/drivers/usb/musb-new/musb_core.c
index eeaacdf..55d1c20 100644
--- a/drivers/usb/musb-new/musb_core.c
+++ b/drivers/usb/musb-new/musb_core.c
@@ -931,6 +931,7 @@ void musb_start(struct musb *musb)
  {
         void __iomem    *regs = musb->mregs;
         u8              devctl = musb_readb(regs, MUSB_DEVCTL);
+       u8              power;

         dev_dbg(musb->controller, "<== devctl %02x\n", devctl);

@@ -942,11 +943,12 @@ void musb_start(struct musb *musb)
         musb_writeb(regs, MUSB_TESTMODE, 0);

         /* put into basic highspeed mode and start session */
-       musb_writeb(regs, MUSB_POWER, MUSB_POWER_ISOUPDATE
-                                               | MUSB_POWER_HSENAB
-                                               /* ENSUSPEND wedges tusb */
-                                               /* | MUSB_POWER_ENSUSPEND */
-                                               );
+       power = MUSB_POWER_ISOUPDATE | MUSB_POWER_HSENAB;
+#ifndef CONFIG_USB_GADGET_DUALSPEED
+       if (musb->board_mode != MUSB_HOST)
+               power &= ~MUSB_POWER_HSENAB;
+#endif
+       musb_writeb(regs, MUSB_POWER, power);

         musb->is_active = 0;
         devctl = musb_readb(regs, MUSB_DEVCTL);
diff --git a/drivers/usb/musb-new/musb_uboot.c 
b/drivers/usb/musb-new/musb_uboot.c
index 85d3027..c240032 100644
--- a/drivers/usb/musb-new/musb_uboot.c
+++ b/drivers/usb/musb-new/musb_uboot.c
@@ -176,7 +176,7 @@ int usb_gadget_register_driver(struct 
usb_gadget_driver *driver)
  {
         int ret;

-       if (!driver || driver->speed < USB_SPEED_HIGH || !driver->bind ||
+       if (!driver || driver->speed < USB_SPEED_FULL || !driver->bind ||
             !driver->setup) {
                 printf("bad parameter.\n");
                 return -EINVAL;

Regards,
-Bin.

>
> Is what we need here, as that sets things at the granularity which we may
> need in some cases.
>
> Regards,
>
> Hans