Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support

[Chris Ruehl <chris.ruehl@gtsys.com.hk>]

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