Messages in this thread |  | | Date | Thu, 05 Dec 2013 12:15:14 +0800 | From | Chris Ruehl <> | Subject | Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support |
| |
On Wednesday, December 04, 2013 05:49 PM, Heikki Krogerus wrote: > Hi Chris, > > On Wed, Dec 04, 2013 at 03:16:21PM +0800, Chris Ruehl wrote: >> On Tuesday, December 03, 2013 04:15 PM, Heikki Krogerus wrote: >>> On Mon, Dec 02, 2013 at 03:05:19PM +0800, Chris Ruehl wrote: >>>> @@ -154,6 +164,27 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_gen_xceiv *nop, >>>> { >>>> int err; >>>> >>>> + if (nop->ulpi_vbus> 0) { >>>> + unsigned int flags = 0; >>>> + >>>> + if (nop->ulpi_vbus& 0x1) >>>> + flags |= ULPI_OTG_DRVVBUS; >>>> + if (nop->ulpi_vbus& 0x2) >>>> + flags |= ULPI_OTG_DRVVBUS_EXT; >>>> + if (nop->ulpi_vbus& 0x4) >>>> + flags |= ULPI_OTG_EXTVBUSIND; >>>> + if (nop->ulpi_vbus& 0x8) >>>> + flags |= ULPI_OTG_CHRGVBUS; >>>> + >>>> + nop->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, flags); >>>> + if (!nop->ulpi) { >>>> + dev_err(dev, "Failed create ULPI Phy\n"); >>>> + return -ENOMEM; >>>> + } >>>> + dev_dbg(dev, "Create ULPI Phy\n"); >>>> + nop->ulpi->io_priv = nop->viewport; >>>> + } >>> >>> This is so wrong. You are registering one kind of usb phy driver from >>> an other. Change drivers/usb/phy/ulpi.c to be a platform device. The >>> whole flag system in it is pretty horrid. While you are at it, change >>> that so it sets the values based on boolean flags from OF properties >>> or platform data. >>> >>> NAK for the whole set. >>> >>> >> >> Heikki, >> >> Thanks for your comments, even not much positive to me.. any how. >> My intention on the "horrid" path was to reduce kernel code where >> one of_read32 vs. four of_boolean. And mentioned logic is simple. >> But that's history. > > I should probable explain why I have problems with them. First of all, > things like driving the vbus should be a function that can be called > from upper layers. struct usb_otg has the set_vbus hook for that. You > can call it for example from your host controller's init routine. I'm > assuming you have a host controller since you are driving vbus.
My platform is Freescale imx27 and the host controller the ChipIdea, where I have already send some patches for. I uses the set_vbus it in the wrong place nop->ulpi->otg->set_vbus(nop->ulpi->otg,true); (phy-generic:usb_gen_phy_init())
and now I start to understand where is the issue. I must tell chipidea to init the vbus using the platform....
> > You don't need to set the ULPI_OTG_CHRGVBUS. It's used for the VBUS > pulsing of SRP, which btw. is not anymore supported in OTG&EH2.0 spec, > so just don't use that bit even if you want to start SRP.
OK, got it. Test it right away, yes my USB still works great even I omit the flag. The reason I introduced it was the fact that plat-mxc/isp1504xc.c of the 2.6.22 with the freescale patches set this flag.
> > The only of_booleans you should need are for the DRV_VBUS_EXT and > USE_EXT_VBUS_IND. In my case I could not use even those. My controller > provides it's own control for them, so even if I set them to my ULPI > phy, the controller would simply override the values. > > Secondly, why those silly flags in the first place. Those flags are > just bits in the registers. It would have been much easier and cleaner > to deliver a small struct with default values for the registers > instead. > >> On my way to find a solution for my board I'd look around and found using of >> phy-ulpi.c functions in phy-tegra-usb.c and don't mind to use them too. > > OK, IC. I have not followed what is happening with USB in linux for a > while. > > The whole otg_ulpi_create() thing, and the flags with it, were > originally planned to be used from platform code. It's evil and it > should have never been accepted into upstream kernel. The time it was > introduced I was on vacation and nobody else seemed to care :(. All I > was able to do was to protest afterwards. >
Checked!
>> I accept your NAK and will work on a patch to make phy-ulpi.c >> working as platform device. >> >> Last question to you. What you don't like on the patch to support >> chip-select gpio of my patch-set.. I ask because you NAK the whole >> set. >> I really need the ChipSelect function to make my hardware work! > > OK, I did not explain my problem with that patch. I'm sorry about > that. It also looks like I made wrong assumption with it. I thought > that your phy (is was ISP1504 right) is just like isp1704 that I have > worked with. On isp1704 you only have the chip_sel pin (no reset pin), > so I thought you can not have any reason to add handler for an other > gpio to this driver. After a quick look at isp1504 data sheet, it > looks like you have both reset and chip_sel pins on it, which I guess > are both connected to gpios on your platform. > Yes 1504, and my hardware guys make otg using the chipselect with gpio and the usbh2 is fixed selected via pull down resistor.
> So I don't have a problem with that. Though I'm not sure is this > driver the right place to handle things like these gpios, which are > pretty phy specific, in the first place. The phy-generic was > originally meant to be pure NOP phy driver. May then change the meaning back to "generic" when support generic requirements like chip-select(1704+1504) reset(1504). If the 1504 missed a proper reset its ends up in weird errors ..
> > One comment about how to handle the gpios. You should move to the new > gpio descriptor API. The legacy gpio API is now just a wrapper on top > of it. > Back to the Manuals.. :-) OK its on the list.
> >> Chris >> > > Thanks, >
I thank you! Chris
-- GTSYS Limited RFID Technology A01 24/F Gold King Industrial Bld 35-41 Tai Lin Pai Road, Kwai Chung, Hong Kong Fax (852) 8167 4060 - Tel (852) 3598 9488
Disclaimer: http://www.gtsys.com.hk/email/classified.html
|  |