[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Arnaud Patard (Rtp)]

Igor Grinberg <grinberg at compulab.co.il> writes:

Adding Matt Sealey in CC:. he's the product development analyst so he
knows the hardware (unlike me).

> On 01/03/11 16:04, Arnaud Patard (Rtp) wrote:
>> Igor Grinberg <grinberg at compulab.co.il> writes:
>>> On 01/03/11 13:41, Arnaud Patard (Rtp) wrote:
>>>> Igor Grinberg <grinberg at compulab.co.il> writes:
>>>> Hi,
>>>>> On 12/23/10 22:11, Arnaud Patard (Rtp) wrote:
>>>>>> Igor Grinberg <grinberg at compulab.co.il> writes:
>>>>>>> Hi Arnaud,
>>>>>>>
>>>>>>> On 12/20/10 17:48, Arnaud Patard (Rtp) wrote:
>>>>>>>> Current code doesn't handle setting CHRGVBUS when enabling vbus.
>>>>>>>> Add support for it
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Arnaud Patard <arnaud.patard at rtp-net.org>
>>>>>>>> Index: tst-usb/drivers/usb/otg/ulpi.c
>>>>>>>> ===================================================================
>>>>>>>> --- tst-usb.orig/drivers/usb/otg/ulpi.c	2010-12-20 15:38:41.000000000 +0100
>>>>>>>> +++ tst-usb/drivers/usb/otg/ulpi.c	2010-12-20 15:38:57.000000000 +0100
>>>>>>>> @@ -234,7 +234,8 @@
>>>>>>>>  {
>>>>>>>>  	unsigned int flags = otg_io_read(otg, ULPI_OTG_CTRL);
>>>>>>>>  
>>>>>>>> -	flags &= ~(ULPI_OTG_CTRL_DRVVBUS | ULPI_OTG_CTRL_DRVVBUS_EXT);
>>>>>>>> +	flags &= ~(ULPI_OTG_CTRL_DRVVBUS | ULPI_OTG_CTRL_DRVVBUS_EXT |
>>>>>>>> +			ULPI_OTG_CTRL_CHRGVBUS);
>>>>>>>>  
>>>>>>>>  	if (on) {
>>>>>>>>  		if (otg->flags & ULPI_OTG_DRVVBUS)
>>>>>>>> @@ -242,6 +243,9 @@
>>>>>>>>  
>>>>>>>>  		if (otg->flags & ULPI_OTG_DRVVBUS_EXT)
>>>>>>>>  			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
>>>>>>>> +
>>>>>>>> +		if (otg->flags & ULPI_OTG_CHRGVBUS)
>>>>>>>> +			flags |= ULPI_OTG_CTRL_CHRGVBUS;
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>>  	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>>>>>>> I think this is a wrong place to set the ChrgVbus bit.
>>>>>>> As for ULPI spec. 1.1:
>>>>>>> "3.8.7.1 Session Request Protocol (SRP)
>>>>>>> ULPI provides full SRP support. The Link uses the ChrgVbus and DischrgVbus bits
>>>>>>> in the OTG Control register to begin and end a session."
>>>>>>>
>>>>>>> So it is used for SRP.
>>>>>>> May be it is better to implement
>>>>>>> int    (*start_srp)(struct otg_transceiver *otg);
>>>>>>> method for setting this bit?
>>>>>>>
>>>>>> I was not sure on where to put this so I took the same approach as the
>>>>>> fsl bsp which was to set it in this function and to call this function
>>>>>> _after_ usb_add_hcd() [ see my previous patch ]. Indeed, it fixed my
>>>>>> issue so I believe it not so bad given that there has already been some
>>>>>> troubles on the ehci-mxc init.
>>>>> Well, the problem is that this is supposed to be a generic driver and it should
>>>>> somehow follow the ULPI spec.
>>>>> This patch makes it more like mxc specific.
>>>>>
>>>>> As far as I understand, Session Request Protocol (SRP) allows a B-device (Peripheral)
>>>>> to nudge an A-device (Host) to turn on the USB's Vbus.
>>>>> This patch enables SRP along with Vbus, which seems incorrect completely.
>>>>> ulpi_set_vbus() should set the Vbus (as its name says) and that's it.
>>>> so, if I add a srp hook as you're suggesting, which part of the driver
>>>> should call it ?
>>> It should be called from outside the ulpi driver by the OTG logic
>>> (just like ulpi_set_vbus() is called).
>>> But, again, SRP should be set by the B-device (peripheral) and Vbus by the A-device (Host).
>>> Usually, the A-device and B-device are on the opposite sides of the USB cable.
>>>
>>>>> Have you tried without this patch or have you just applied it along with other
>>>>> patches from the fsl bsp?
>>>> I already tried without this patch and without it, things are not
>>>> working on my systems.
>>>>> Also, if this specific patch (2/5) makes your USB (Host as I understand) work,
>>>>> it makes me think that there could be some problem with your Vbus supply.
>>>>> Have you checked that?
>>>> I don't have any schematics and I've access only to the source code of
>>>> the kernel running on the efika nettop and smartbook [1]. I've not seen
>>>> anything (even remotely) related to vbus supply except in the ulpi code
>>>> and from what I've heard, it's unlikely that there's something to
>>>> control it on theses systems. Of course, I'll be happy to be proven
>>>> wrong. Without usb theses systems are useless so anything the can reduce
>>>> the number of patches needed to get the systems working with mainline is
>>>> welcome.
>>> Well, I was certain you are trying to use that port in Host mode (A-device), but
>>> now I'm confused...
>>> You say "things are not working" - what are those things?
>>> Can you, please, describe what are you trying to do?
>>> What are you trying to connect to this USB port? What cable do you
>>> use?
>> I thought it was clear that "things" was everything connected on the usb
>> ports. The problem is that this also means that the nettop/smartbook are
>> kind of not working too as ethernet/wifi/bt/hid devs are all connected
>> on usb [ they're all on the pcb ]. So, even if you boot on the mmc aka
>> the only storage available which doesn't need usb, you won't be able to
>> login.
>>
>
> Well, "everything connected on the usb ports" can be confusing, because
> you can connect devices (usb disk drives, keyboards, mice, etc..) and hosts
> (desktop PCs, mobile computers, etc..) to the OTG port.
> That's why I wanted to be sure, what devices do you connect to that port.
> Now it is clear, that your port should be in the Host mode and you should _not_
> issue an SRP.
>
> "ethernet/wifi/bt/hid devs are all connected on usb" - this means,
> that they are either connected to several (different) usb ports or there is a usb hub
> connected to that port and those devices are connected to it.
> Can you confirm how those devices are connected?
>
>>> Also, what ulpi vendor/product id is reported in ulpi_init()?
>> ULPI transceiver vendor/product ID 0x0424/0x0006
>> Found SMSC USB3319 ULPI transceiver.
>
> SMSC USB3319 does not have either an integrated Vbus switch or Charge Pump,
> For the device connected to that transceiver could work properly,
> there is a need in _external Vbus switch_ , that should be enabled using
> some kind of CPEN pin (can be GPIO).
> This means, that you don't even need to call ulpi_set_vbus().
>
> Either way, this patch is NAK.
> I think you need to check your hardware (in particular Vbus supply).