| Message ID | 55A3C853.3030307@ti.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi, On 13-07-15 16:16, Bin Liu wrote: > 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@redhat.com] Sent: Freitag, 10. Juli 2015 16:37 >>>>>> To: Egli, Samuel; marex@denx.de Cc: u-boot@lists.denx.de; >>>>>> trini@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@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@ti.com> Reviewed-by: Tom Rini >>>>>>>> <trini@konsulko.com> Tested-by: Samuel Egli >>>>>>>> <samuel.egli@siemens.com> CC: Marek Vasut <marex@denx.de> CC: >>>>>>>> Heiko Schocher <hs@denx.de> CC: Daniel Mack >>>>>>>> <zonque@gmail.com> CC: Roger Meier <r.meier@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. The 2 macros do not control one thing, it is for example perfectly possible to have multiple otg controllers, one which can only do fullspeed and one which can do both high/full speed in this case one will want to build the gadget code in dualspeed mode even though one of the usb controllers is fullspeed only. Regards, Hans > > 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
Hi, On 07/19/2015 06:01 AM, Hans de Goede wrote: > Hi, > > On 13-07-15 16:16, Bin Liu wrote: >> 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@redhat.com] Sent: Freitag, 10. Juli 2015 16:37 >>>>>>> To: Egli, Samuel; marex@denx.de Cc: u-boot@lists.denx.de; >>>>>>> trini@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@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@ti.com> Reviewed-by: Tom Rini >>>>>>>>> <trini@konsulko.com> Tested-by: Samuel Egli >>>>>>>>> <samuel.egli@siemens.com> CC: Marek Vasut <marex@denx.de> CC: >>>>>>>>> Heiko Schocher <hs@denx.de> CC: Daniel Mack >>>>>>>>> <zonque@gmail.com> CC: Roger Meier <r.meier@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. > > The 2 macros do not control one thing, it is for example perfectly possible > to have multiple otg controllers, one which can only do fullspeed and one > which can do both high/full speed in this case one will want to build > the gadget code in dualspeed mode even though one of the usb controllers > is fullspeed only. This case is a different problem, which requires a separate patch to solve, maybe CONFIG_MUSB_NO_HIGHSPEED can play here. My original and 2nd patch were trying to solve the issue that MUSB controller was not set consistently with the gadget driver. Unless the patch breaks something else (as did the original patch), nothing else needs to be added in. Regards, -Bin. > > Regards, > > Hans > > > > > > >> >> 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
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");