From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Sun, 19 Jul 2015 13:01:54 +0200 Subject: [U-Boot] [PATCH] musb: set MUSB speed based on CONFIG In-Reply-To: <55A3C853.3030307@ti.com> References: <1436534204-26853-1-git-send-email-samuel.egli@siemens.com> <1436534204-26853-2-git-send-email-samuel.egli@siemens.com> <559FD6FC.4010606@redhat.com> <559FD87C.7060905@redhat.com> <27E9275BC1C8554E840881B19B62BE421A9ED3@DENBGAT9EI1MSX.ww902.siemens.net> <559FE0CF.60202@denx.de> <559FE55F.90605@ti.com> <55A1144E.6000707@redhat.com> <55A3C853.3030307@ti.com> Message-ID: <55AB83A2.1050506@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 >>>>>>>> >>>>>>>> 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 Reviewed-by: Tom Rini >>>>>>>> Tested-by: Samuel Egli >>>>>>>> CC: Marek Vasut CC: >>>>>>>> Heiko Schocher CC: Daniel Mack >>>>>>>> CC: Roger Meier >>>>>>> >>>>>>> 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