[patch 0/5] iMX51 usb fixes

[patch 0/5] iMX51 usb fixes

[Arnaud Patard (Rtp)]

This patchset aims at fixing several issues I found in the usb support for
iMX51 while working on the Genesis EFIKAMX/EFIKASB systems. The most
"annoying" one is the usb clock patch. I found out that when the clocks are
not properly enabled my system just hangs when trying to read a usb register.
So far, I guess only luck (or different bootloader behaviour) may explain that
nobody got hit by this.

Warning:
It has been tested on both devices against Sascha's imx-for-2.6.38 branch.
It's possible that some of them apply against current mainline but I can't
test such a configuration.

Arnaud

[patch 1/5] ehci-mxc: Enable vbus later

[Arnaud Patard (Rtp)]

An embedded and charset-unspecified text was scrubbed...
Name: ehci_mxc_enable_vbus_later.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101220/3f175bfe/attachment.ksh>

[patch 1/5] ehci-mxc: Enable vbus later

[Sascha Hauer]

On Mon, Dec 20, 2010 at 04:48:54PM +0100, Arnaud Patard wrote:
> vbus should be enabled after usb_add_hcd() otherwise USB is not working
> on my efikamx.

There is really something wrong with the way we handle initialization of
the mxc ehci controller. There are frequently patches posted trying to
change this order, and I think we are at a point where we can't move
anymore without possibly breaking some other board. Should this be
completely board dependend in the end?

Sascha

> 
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Index: tst-usb/drivers/usb/host/ehci-mxc.c
> ===================================================================
> --- tst-usb.orig/drivers/usb/host/ehci-mxc.c	2010-12-20 15:38:41.000000000 +0100
> +++ tst-usb/drivers/usb/host/ehci-mxc.c	2010-12-20 15:38:43.000000000 +0100
> @@ -210,11 +210,6 @@
>  			ret = -ENODEV;
>  			goto err_add;
>  		}
> -		ret = otg_set_vbus(pdata->otg, 1);
> -		if (ret) {
> -			dev_err(dev, "unable to enable vbus on transceiver\n");
> -			goto err_add;
> -		}
>  	}
>  
>  	priv->hcd = hcd;
> @@ -224,6 +219,14 @@
>  	if (ret)
>  		goto err_add;
>  
> +	if (pdata->otg) {
> +		ret = otg_set_vbus(pdata->otg, 1);
> +		if (ret) {
> +			dev_err(dev, "unable to enable vbus on transceiver\n");
> +			goto err_add;
> +		}
> +	}
> +
>  	return 0;
>  
>  err_add:
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[patch 1/5] ehci-mxc: Enable vbus later

[Arnaud Patard (Rtp)]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> On Mon, Dec 20, 2010 at 04:48:54PM +0100, Arnaud Patard wrote:
>> vbus should be enabled after usb_add_hcd() otherwise USB is not working
>> on my efikamx.
>
> There is really something wrong with the way we handle initialization of
> the mxc ehci controller. There are frequently patches posted trying to
> change this order, and I think we are at a point where we can't move
> anymore without possibly breaking some other board. Should this be
> completely board dependend in the end?

I'm not aware of any other bug except the postsc setting one (which was
that any code trying to read the ulpi register without making sure that
portsc is configured for ulpi is buggy). Moreover, the change is
following the fsl bsp. I know it's not necessary the best code of the
world but I believe it has been tested on a lot of imx platforms.


Arnaud

[patch 1/5] ehci-mxc: Enable vbus later

[Sascha Hauer]

On Mon, Dec 20, 2010 at 04:48:54PM +0100, Arnaud Patard wrote:
> vbus should be enabled after usb_add_hcd() otherwise USB is not working
> on my efikamx.

I have tested this on different i.MX27/31/35 based boards, the USB is
still functional, so

Tested-by: Sascha Hauer <s.hauer@pengutronix.de>

> 
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Index: tst-usb/drivers/usb/host/ehci-mxc.c
> ===================================================================
> --- tst-usb.orig/drivers/usb/host/ehci-mxc.c	2010-12-20 15:38:41.000000000 +0100
> +++ tst-usb/drivers/usb/host/ehci-mxc.c	2010-12-20 15:38:43.000000000 +0100
> @@ -210,11 +210,6 @@
>  			ret = -ENODEV;
>  			goto err_add;
>  		}
> -		ret = otg_set_vbus(pdata->otg, 1);
> -		if (ret) {
> -			dev_err(dev, "unable to enable vbus on transceiver\n");
> -			goto err_add;
> -		}
>  	}
>  
>  	priv->hcd = hcd;
> @@ -224,6 +219,14 @@
>  	if (ret)
>  		goto err_add;
>  
> +	if (pdata->otg) {
> +		ret = otg_set_vbus(pdata->otg, 1);
> +		if (ret) {
> +			dev_err(dev, "unable to enable vbus on transceiver\n");
> +			goto err_add;
> +		}
> +	}
> +
>  	return 0;
>  
>  err_add:
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Arnaud Patard (Rtp)]

An embedded and charset-unspecified text was scrubbed...
Name: ulpi_handle_chrgvbus.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101220/1b8cfa20/attachment.ksh>

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Igor Grinberg]

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@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?

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

-- 
Regards,
Igor.

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Arnaud Patard (Rtp)]

Igor Grinberg <grinberg@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@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.

Arnaud

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Igor Grinberg]

On 12/23/10 22:11, Arnaud Patard (Rtp) wrote:
> Igor Grinberg <grinberg@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@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.

Have you tried without this patch or have you just applied it along with other
patches from the fsl bsp?

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?

> Arnaud
>

-- 
Regards,
Igor.

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Arnaud Patard (Rtp)]

Igor Grinberg <grinberg@compulab.co.il> writes:
Hi,

> On 12/23/10 22:11, Arnaud Patard (Rtp) wrote:
>> Igor Grinberg <grinberg@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@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 ?

>
> 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.

Arnaud

[1] http://gitorious.org/efikamx/linux-kernel/

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Igor Grinberg]

On 01/03/11 13:41, Arnaud Patard (Rtp) wrote:
> Igor Grinberg <grinberg@compulab.co.il> writes:
> Hi,
>> On 12/23/10 22:11, Arnaud Patard (Rtp) wrote:
>>> Igor Grinberg <grinberg@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@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?
Also, what ulpi vendor/product id is reported in ulpi_init()?

> Arnaud
>
> [1] http://gitorious.org/efikamx/linux-kernel/

-- 
Regards,
Igor.

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Arnaud Patard (Rtp)]

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

> On 01/03/11 13:41, Arnaud Patard (Rtp) wrote:
>> Igor Grinberg <grinberg@compulab.co.il> writes:
>> Hi,
>>> On 12/23/10 22:11, Arnaud Patard (Rtp) wrote:
>>>> Igor Grinberg <grinberg@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@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.

> Also, what ulpi vendor/product id is reported in ulpi_init()?

ULPI transceiver vendor/product ID 0x0424/0x0006
Found SMSC USB3319 ULPI transceiver.

Arnaud

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Igor Grinberg]

On 01/03/11 16:04, Arnaud Patard (Rtp) wrote:
> Igor Grinberg <grinberg@compulab.co.il> writes:
>> On 01/03/11 13:41, Arnaud Patard (Rtp) wrote:
>>> Igor Grinberg <grinberg@compulab.co.il> writes:
>>> Hi,
>>>> On 12/23/10 22:11, Arnaud Patard (Rtp) wrote:
>>>>> Igor Grinberg <grinberg@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@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).

-- 
Regards,
Igor.

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Arnaud Patard (Rtp)]

Igor Grinberg <grinberg@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@compulab.co.il> writes:
>>> On 01/03/11 13:41, Arnaud Patard (Rtp) wrote:
>>>> Igor Grinberg <grinberg@compulab.co.il> writes:
>>>> Hi,
>>>>> On 12/23/10 22:11, Arnaud Patard (Rtp) wrote:
>>>>>> Igor Grinberg <grinberg@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@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).

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Matt Sealey]

On Tue, Jan 4, 2011 at 2:00 PM, Arnaud Patard <arnaud.patard@rtp-net.org> wrote:
> Igor Grinberg <grinberg@compulab.co.il> writes:
>
> Adding Matt Sealey in CC:. he's the product development analyst so he
> knows the hardware (unlike me).


>>>> 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).

Hey guys,

On the Smartbook at least both USB host ports (H1 and H2) on the board
(one port each) are connected directly to 4-port USB hubs (SMSC2514).
We don't have anything on there except that connection: the hub should
handle VBUS properly. Both ports use an SMSC3317 (just a 3311 with a
built in 3.3V supply so the id and behavior should be identical).

On the Smarttop H1 is connected to a 4-port USB hub (Terminus FE1.1)
with the same configuration. Same PHY. The DR port is connected
directly to an ASIX ethernet controller. VBUS seems routed to a test
point.

I'm curious exactly what the real problem here is: that VBUS is
basically not being handled correctly? It should be driven or not? I'm
not entirely familiar with the specification.

-- 
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Igor Grinberg]

On 01/04/11 22:03, Matt Sealey wrote:
> On Tue, Jan 4, 2011 at 2:00 PM, Arnaud Patard <arnaud.patard@rtp-net.org> wrote:
>> Igor Grinberg <grinberg@compulab.co.il> writes:
>>
>> Adding Matt Sealey in CC:. he's the product development analyst so he
>> knows the hardware (unlike me).
>>>>> 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).
> Hey guys,

Hi

Thanks for the information. Now I am finally getting the picture of what's is
going on there...

> On the Smartbook at least both USB host ports (H1 and H2) on the board
> (one port each) are connected directly to 4-port USB hubs (SMSC2514).
> We don't have anything on there except that connection: the hub should
> handle VBUS properly. Both ports use an SMSC3317 (just a 3311 with a
> built in 3.3V supply so the id and behavior should be identical).

OK, so the SMSC331x ulpi transceiver is connected to the SMSC2514 usb hub,
so all the peripheral devices (ethernet/wifi/bt/hid) are connected to the
downstream ports of that hub and thus get their Vbus as expected. This is fine.

> On the Smarttop H1 is connected to a 4-port USB hub (Terminus FE1.1)
> with the same configuration. Same PHY. The DR port is connected
> directly to an ASIX ethernet controller. VBUS seems routed to a test
> point.
>
> I'm curious exactly what the real problem here is: that VBUS is
> basically not being handled correctly? It should be driven or not? I'm
> not entirely familiar with the specification.

SMSC2514 usb hub will not provide power to its D+ and D- pull-up resistors
until it detects a Vbus enabled on the upstream port. This is totally fine.

SMSC331x ulpi transceiver does not have either an integrated Vbus switch or
an integrated charge pump - this means that it cannot provide a Vbus to the hub.

The hub in its turn does not power the pull-up resistors and peripheral devices
are not being connected to the usb subsystem.

With this patch applied, SMSC331x ulpi transceiver issues an SRP pulses on the Vbus
and hub senses that there is something that looks like Vbus and then enables the
pull-up resistors -> peripheral devices are being connected to the usb subsystem.
This behavior violates the ULPI (and mostly certain OTG and may be also USB 2.0)
specification and SMSC2514 usb hub datasheet.

According to the SMSC2514 usb hub datasheet, VBUS_DET pin has to be connected
to a valid Vbus from the upstream port, but SMSC331x does not provide this Vbus.
This means that you should have add a Vbus switch or a charge pump to the
VBUS_DET pin of the usb hub and provide means (like GPIO) to enable/disable that
switch or charge pump.
This is h/w design bug we are dealing with.

The best solution to this would be to add a missing h/w component.
Now, I understand that it can be kind a problematic ;)
But, we cannot violate the ULPI spec and the generic driver to workaround
some h/w problem that is existing in some specific configuration and hopefully will
be fixed in the next h/w revisions. Therefore, as I said before, this patch is NAK.

What we can do is:
1) implement the int (*start_srp)(struct otg_transceiver *otg);
method as defined by the ULPI spec.

2) and then add a call (along with huge comment explaining this workaround)
to otg_start_srp(). I'd recommend to restrict this call to that specific board somehow,
but it is up to Sascha to decide where to put it.

-- 
Regards,
Igor.

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Arnaud Patard (Rtp)]

Hi

Igor Grinberg <grinberg@compulab.co.il> writes:
[...]
> Thanks for the information. Now I am finally getting the picture of what's is
> going on there...
>
>> On the Smartbook at least both USB host ports (H1 and H2) on the board
>> (one port each) are connected directly to 4-port USB hubs (SMSC2514).
>> We don't have anything on there except that connection: the hub should
>> handle VBUS properly. Both ports use an SMSC3317 (just a 3311 with a
>> built in 3.3V supply so the id and behavior should be identical).
>
> OK, so the SMSC331x ulpi transceiver is connected to the SMSC2514 usb hub,
> so all the peripheral devices (ethernet/wifi/bt/hid) are connected to the
> downstream ports of that hub and thus get their Vbus as expected. This is fine.
>
>> On the Smarttop H1 is connected to a 4-port USB hub (Terminus FE1.1)
>> with the same configuration. Same PHY. The DR port is connected
>> directly to an ASIX ethernet controller. VBUS seems routed to a test
>> point.
>>
>> I'm curious exactly what the real problem here is: that VBUS is
>> basically not being handled correctly? It should be driven or not? I'm
>> not entirely familiar with the specification.
>
> SMSC2514 usb hub will not provide power to its D+ and D- pull-up resistors
> until it detects a Vbus enabled on the upstream port. This is totally fine.
>
> SMSC331x ulpi transceiver does not have either an integrated Vbus switch or
> an integrated charge pump - this means that it cannot provide a Vbus to the hub.
>
> The hub in its turn does not power the pull-up resistors and peripheral devices
> are not being connected to the usb subsystem.
>
> With this patch applied, SMSC331x ulpi transceiver issues an SRP pulses on the Vbus
> and hub senses that there is something that looks like Vbus and then enables the
> pull-up resistors -> peripheral devices are being connected to the usb subsystem.
> This behavior violates the ULPI (and mostly certain OTG and may be also USB 2.0)
> specification and SMSC2514 usb hub datasheet.
>
> According to the SMSC2514 usb hub datasheet, VBUS_DET pin has to be connected
> to a valid Vbus from the upstream port, but SMSC331x does not provide this Vbus.
> This means that you should have add a Vbus switch or a charge pump to the
> VBUS_DET pin of the usb hub and provide means (like GPIO) to enable/disable that
> switch or charge pump.
> This is h/w design bug we are dealing with.
>
> The best solution to this would be to add a missing h/w component.
> Now, I understand that it can be kind a problematic ;)
> But, we cannot violate the ULPI spec and the generic driver to workaround
> some h/w problem that is existing in some specific configuration and hopefully will
> be fixed in the next h/w revisions. Therefore, as I said before, this patch is NAK.
>
> What we can do is:
> 1) implement the int (*start_srp)(struct otg_transceiver *otg);
> method as defined by the ULPI spec.
>
> 2) and then add a call (along with huge comment explaining this workaround)
> to otg_start_srp(). I'd recommend to restrict this call to that specific board somehow,
> but it is up to Sascha to decide where to put it.

What about the following patch ?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ulpi_chrgvbus2.patch
Type: text/x-diff
Size: 1867 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110107/dad4f389/attachment-0001.bin>

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Igor Grinberg]

Hi Arnaud,


Can you, please, send patches inline and not as attachments next time?
It is very inconvenient to review and comment on attached patches.
Comments below.

On 01/07/11 11:02, Arnaud Patard (Rtp) wrote:
> Hi
>
> Igor Grinberg <grinberg@compulab.co.il> writes:
> [...]
>> Thanks for the information. Now I am finally getting the picture of what's is
>> going on there...
>>
>>> On the Smartbook at least both USB host ports (H1 and H2) on the board
>>> (one port each) are connected directly to 4-port USB hubs (SMSC2514).
>>> We don't have anything on there except that connection: the hub should
>>> handle VBUS properly. Both ports use an SMSC3317 (just a 3311 with a
>>> built in 3.3V supply so the id and behavior should be identical).
>> OK, so the SMSC331x ulpi transceiver is connected to the SMSC2514 usb hub,
>> so all the peripheral devices (ethernet/wifi/bt/hid) are connected to the
>> downstream ports of that hub and thus get their Vbus as expected. This is fine.
>>
>>> On the Smarttop H1 is connected to a 4-port USB hub (Terminus FE1.1)
>>> with the same configuration. Same PHY. The DR port is connected
>>> directly to an ASIX ethernet controller. VBUS seems routed to a test
>>> point.
>>>
>>> I'm curious exactly what the real problem here is: that VBUS is
>>> basically not being handled correctly? It should be driven or not? I'm
>>> not entirely familiar with the specification.
>> SMSC2514 usb hub will not provide power to its D+ and D- pull-up resistors
>> until it detects a Vbus enabled on the upstream port. This is totally fine.
>>
>> SMSC331x ulpi transceiver does not have either an integrated Vbus switch or
>> an integrated charge pump - this means that it cannot provide a Vbus to the hub.
>>
>> The hub in its turn does not power the pull-up resistors and peripheral devices
>> are not being connected to the usb subsystem.
>>
>> With this patch applied, SMSC331x ulpi transceiver issues an SRP pulses on the Vbus
>> and hub senses that there is something that looks like Vbus and then enables the
>> pull-up resistors -> peripheral devices are being connected to the usb subsystem.
>> This behavior violates the ULPI (and mostly certain OTG and may be also USB 2.0)
>> specification and SMSC2514 usb hub datasheet.
>>
>> According to the SMSC2514 usb hub datasheet, VBUS_DET pin has to be connected
>> to a valid Vbus from the upstream port, but SMSC331x does not provide this Vbus.
>> This means that you should have add a Vbus switch or a charge pump to the
>> VBUS_DET pin of the usb hub and provide means (like GPIO) to enable/disable that
>> switch or charge pump.
>> This is h/w design bug we are dealing with.
>>
>> The best solution to this would be to add a missing h/w component.
>> Now, I understand that it can be kind a problematic ;)
>> But, we cannot violate the ULPI spec and the generic driver to workaround
>> some h/w problem that is existing in some specific configuration and hopefully will
>> be fixed in the next h/w revisions. Therefore, as I said before, this patch is NAK.
>>
>> What we can do is:
>> 1) implement the int (*start_srp)(struct otg_transceiver *otg);
>> method as defined by the ULPI spec.
>>
>> 2) and then add a call (along with huge comment explaining this workaround)
>> to otg_start_srp(). I'd recommend to restrict this call to that specific board somehow,
>> but it is up to Sascha to decide where to put it.
> What about the following patch ?
> ulpi: provide start_srp
>
> Add support for setting CHRGVBUS thanks to start_srp and use it
> to workaround a hardware bug on efika mx/sb boards.
> See http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/037341.html
>
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Index: linux-2.6-submit/drivers/usb/otg/ulpi.c
> ===================================================================
> --- linux-2.6-submit.orig/drivers/usb/otg/ulpi.c	2011-01-04 17:12:26.000000000 +0100
> +++ linux-2.6-submit/drivers/usb/otg/ulpi.c	2011-01-06 14:44:50.000000000 +0100
> @@ -247,6 +247,14 @@
>  	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>  }
>
> +static int ulpi_start_srp(struct otg_transceiver *otg, bool on)
> +{
> +	unsigned int flags = otg_io_read(otg, ULPI_OTG_CTRL);
> +
> +	flags |= ULPI_OTG_CTRL_CHRGVBUS;
> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
> +}
> +

I went through the OTG spec. and found that there are much more things
should be done for the start_srp() function to be implemented properly.
For example, it says that we should make sure that the Vbus is discharged
(means, the previous session has finished) before attempting to start a new
session, but that is only a half of the problem. The real problem for you, is that
the duration of the SRP should be no more then 100ms.
This means that my proposal under 1) is not good for you, sorry...

>  struct otg_transceiver *
>  otg_ulpi_create(struct otg_io_access_ops *ops,
>  		unsigned int flags)
> @@ -263,6 +271,7 @@
>  	otg->init	= ulpi_init;
>  	otg->set_host	= ulpi_set_host;
>  	otg->set_vbus	= ulpi_set_vbus;
> +	otg->start_srp	= ulpi_start_srp;
>
>  	return otg;
>  }
> Index: linux-2.6-submit/drivers/usb/host/ehci-mxc.c
> ===================================================================
> --- linux-2.6-submit.orig/drivers/usb/host/ehci-mxc.c	2011-01-06 14:45:14.000000000 +0100
> +++ linux-2.6-submit/drivers/usb/host/ehci-mxc.c	2011-01-06 15:48:16.000000000 +0100
> @@ -25,6 +25,8 @@
>
>  #include <mach/mxc_ehci.h>
>
> +#include <asm/mach-types.h>
> +
>  #define ULPI_VIEWPORT_OFFSET	0x170
>
>  struct ehci_mxc_priv {
> @@ -239,6 +241,13 @@
>  			dev_err(dev, "unable to enable vbus on transceiver\n");
>  			goto err_add;
>  		}
> +		if (machine_is_mx51_efikamx() || machine_is_mx51_efikasb()) {
> +			ret = otg_start_srp(pdata->otg);
> +			if (ret) {
> +				dev_err(dev, "unable to start srp\n");
> +				goto err_add;
> +			}
> +		}

In light of the problem described above, here you will need to write directly
to the view port instead of calling the otg_start_srp() function.

I'd recommend to move this to the machine (board) specific code.
May be to platform init (pdata->init) call back?

IMHO, the explanation of the h/w bug workaround in the commit message
is not enough - in-code comment regarding the bug workaround should be added.

-- 
Regards,
Igor.

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Arnaud Patard (Rtp)]

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

> Hi Arnaud,

Hi,
>
>
> Can you, please, send patches inline and not as attachments next time?
> It is very inconvenient to review and comment on attached patches.

This patch a quick rewrite&test. It was more meant to understand if the
direction was right rather than a formal submission. (I use quilt to
send patches and not my mailer). There are some comment in the code
needed and I've not put them.

> Comments below.
>
> On 01/07/11 11:02, Arnaud Patard (Rtp) wrote:
>> Hi
>>
>> Igor Grinberg <grinberg@compulab.co.il> writes:
>> [...]
>>> Thanks for the information. Now I am finally getting the picture of what's is
>>> going on there...
>>>
>>>> On the Smartbook at least both USB host ports (H1 and H2) on the board
>>>> (one port each) are connected directly to 4-port USB hubs (SMSC2514).
>>>> We don't have anything on there except that connection: the hub should
>>>> handle VBUS properly. Both ports use an SMSC3317 (just a 3311 with a
>>>> built in 3.3V supply so the id and behavior should be identical).
>>> OK, so the SMSC331x ulpi transceiver is connected to the SMSC2514 usb hub,
>>> so all the peripheral devices (ethernet/wifi/bt/hid) are connected to the
>>> downstream ports of that hub and thus get their Vbus as expected. This is fine.
>>>
>>>> On the Smarttop H1 is connected to a 4-port USB hub (Terminus FE1.1)
>>>> with the same configuration. Same PHY. The DR port is connected
>>>> directly to an ASIX ethernet controller. VBUS seems routed to a test
>>>> point.
>>>>
>>>> I'm curious exactly what the real problem here is: that VBUS is
>>>> basically not being handled correctly? It should be driven or not? I'm
>>>> not entirely familiar with the specification.
>>> SMSC2514 usb hub will not provide power to its D+ and D- pull-up resistors
>>> until it detects a Vbus enabled on the upstream port. This is totally fine.
>>>
>>> SMSC331x ulpi transceiver does not have either an integrated Vbus switch or
>>> an integrated charge pump - this means that it cannot provide a Vbus to the hub.
>>>
>>> The hub in its turn does not power the pull-up resistors and peripheral devices
>>> are not being connected to the usb subsystem.
>>>
>>> With this patch applied, SMSC331x ulpi transceiver issues an SRP pulses on the Vbus
>>> and hub senses that there is something that looks like Vbus and then enables the
>>> pull-up resistors -> peripheral devices are being connected to the usb subsystem.
>>> This behavior violates the ULPI (and mostly certain OTG and may be also USB 2.0)
>>> specification and SMSC2514 usb hub datasheet.
>>>
>>> According to the SMSC2514 usb hub datasheet, VBUS_DET pin has to be connected
>>> to a valid Vbus from the upstream port, but SMSC331x does not provide this Vbus.
>>> This means that you should have add a Vbus switch or a charge pump to the
>>> VBUS_DET pin of the usb hub and provide means (like GPIO) to enable/disable that
>>> switch or charge pump.
>>> This is h/w design bug we are dealing with.
>>>
>>> The best solution to this would be to add a missing h/w component.
>>> Now, I understand that it can be kind a problematic ;)
>>> But, we cannot violate the ULPI spec and the generic driver to workaround
>>> some h/w problem that is existing in some specific configuration and hopefully will
>>> be fixed in the next h/w revisions. Therefore, as I said before, this patch is NAK.
>>>
>>> What we can do is:
>>> 1) implement the int (*start_srp)(struct otg_transceiver *otg);
>>> method as defined by the ULPI spec.
>>>
>>> 2) and then add a call (along with huge comment explaining this workaround)
>>> to otg_start_srp(). I'd recommend to restrict this call to that specific board somehow,
>>> but it is up to Sascha to decide where to put it.
>> What about the following patch ?
>> ulpi: provide start_srp
>>
>> Add support for setting CHRGVBUS thanks to start_srp and use it
>> to workaround a hardware bug on efika mx/sb boards.
>> See http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/037341.html
>>
>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>> Index: linux-2.6-submit/drivers/usb/otg/ulpi.c
>> ===================================================================
>> --- linux-2.6-submit.orig/drivers/usb/otg/ulpi.c	2011-01-04 17:12:26.000000000 +0100
>> +++ linux-2.6-submit/drivers/usb/otg/ulpi.c	2011-01-06 14:44:50.000000000 +0100
>> @@ -247,6 +247,14 @@
>>  	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>>  }
>>
>> +static int ulpi_start_srp(struct otg_transceiver *otg, bool on)
>> +{
>> +	unsigned int flags = otg_io_read(otg, ULPI_OTG_CTRL);
>> +
>> +	flags |= ULPI_OTG_CTRL_CHRGVBUS;
>> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>> +}
>> +
>
> I went through the OTG spec. and found that there are much more things
> should be done for the start_srp() function to be implemented properly.
> For example, it says that we should make sure that the Vbus is discharged
> (means, the previous session has finished) before attempting to start a new
> session, but that is only a half of the problem. The real problem for you, is that
> the duration of the SRP should be no more then 100ms.

oops. That's really annoying. (fwiw, I think that original code from fsl
doesn't take care of that 100ms delay otherwise I guess I would have
detected it too).

> This means that my proposal under 1) is not good for you, sorry...
>
>>  struct otg_transceiver *
>>  otg_ulpi_create(struct otg_io_access_ops *ops,
>>  		unsigned int flags)
>> @@ -263,6 +271,7 @@
>>  	otg->init	= ulpi_init;
>>  	otg->set_host	= ulpi_set_host;
>>  	otg->set_vbus	= ulpi_set_vbus;
>> +	otg->start_srp	= ulpi_start_srp;
>>
>>  	return otg;
>>  }
>> Index: linux-2.6-submit/drivers/usb/host/ehci-mxc.c
>> ===================================================================
>> --- linux-2.6-submit.orig/drivers/usb/host/ehci-mxc.c	2011-01-06 14:45:14.000000000 +0100
>> +++ linux-2.6-submit/drivers/usb/host/ehci-mxc.c	2011-01-06 15:48:16.000000000 +0100
>> @@ -25,6 +25,8 @@
>>
>>  #include <mach/mxc_ehci.h>
>>
>> +#include <asm/mach-types.h>
>> +
>>  #define ULPI_VIEWPORT_OFFSET	0x170
>>
>>  struct ehci_mxc_priv {
>> @@ -239,6 +241,13 @@
>>  			dev_err(dev, "unable to enable vbus on transceiver\n");
>>  			goto err_add;
>>  		}
>> +		if (machine_is_mx51_efikamx() || machine_is_mx51_efikasb()) {
>> +			ret = otg_start_srp(pdata->otg);
>> +			if (ret) {
>> +				dev_err(dev, "unable to start srp\n");
>> +				goto err_add;
>> +			}
>> +		}
>
> In light of the problem described above, here you will need to write directly
> to the view port instead of calling the otg_start_srp() function.

ok. I don't think it's a problem. Looks otg_io_(read|write) are
available here.

>
> I'd recommend to move this to the machine (board) specific code.
> May be to platform init (pdata->init) call back?

oh, no. I've put this call in this place for some good reasons. This
trick is working only if it's done _after_ add_hcd. Any other place
won't work. For instance, pdata->init is meant to be called before
mxc_initialise_usb_hw iirc which is _before_ add_hcd.

>
> IMHO, the explanation of the h/w bug workaround in the commit message
> is not enough - in-code comment regarding the bug workaround should be added.

I know. As I said this patch was not the final submission. It was
probably not clear enough.

Arnaud

[patch 2/5] ulpi: handle ULPI_OTG_CTRL_CHRGVBUS

[Igor Grinberg]

On 01/10/11 16:22, Arnaud Patard (Rtp) wrote:

> Igor Grinberg <grinberg@compulab.co.il> writes:
>> Can you, please, send patches inline and not as attachments next time?
>> It is very inconvenient to review and comment on attached patches.
> This patch a quick rewrite&test. It was more meant to understand if the
> direction was right rather than a formal submission. (I use quilt to
> send patches and not my mailer). There are some comment in the code
> needed and I've not put them.

Well, I also have got all other patches in the series as attachments...
Why not use git send-email?

>> On 01/07/11 11:02, Arnaud Patard (Rtp) wrote:
>>> What about the following patch ?
>>> ulpi: provide start_srp
>>>
>>> Add support for setting CHRGVBUS thanks to start_srp and use it
>>> to workaround a hardware bug on efika mx/sb boards.
>>> See http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/037341.html
>>>
>>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>>> Index: linux-2.6-submit/drivers/usb/otg/ulpi.c
>>> ===================================================================
>>> --- linux-2.6-submit.orig/drivers/usb/otg/ulpi.c	2011-01-04 17:12:26.000000000 +0100
>>> +++ linux-2.6-submit/drivers/usb/otg/ulpi.c	2011-01-06 14:44:50.000000000 +0100
>>> @@ -247,6 +247,14 @@
>>>  	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>>>  }
>>>
>>> +static int ulpi_start_srp(struct otg_transceiver *otg, bool on)
>>> +{
>>> +	unsigned int flags = otg_io_read(otg, ULPI_OTG_CTRL);
>>> +
>>> +	flags |= ULPI_OTG_CTRL_CHRGVBUS;
>>> +	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
>>> +}
>>> +
>> I went through the OTG spec. and found that there are much more things
>> should be done for the start_srp() function to be implemented properly.
>> For example, it says that we should make sure that the Vbus is discharged
>> (means, the previous session has finished) before attempting to start a new
>> session, but that is only a half of the problem. The real problem for you, is that
>> the duration of the SRP should be no more then 100ms.
> oops. That's really annoying. (fwiw, I think that original code from fsl
> doesn't take care of that 100ms delay otherwise I guess I would have
> detected it too).
>>>  struct otg_transceiver *
>>>  otg_ulpi_create(struct otg_io_access_ops *ops,
>>>  		unsigned int flags)
>>> @@ -263,6 +271,7 @@
>>>  	otg->init	= ulpi_init;
>>>  	otg->set_host	= ulpi_set_host;
>>>  	otg->set_vbus	= ulpi_set_vbus;
>>> +	otg->start_srp	= ulpi_start_srp;
>>>
>>>  	return otg;
>>>  }
>>> Index: linux-2.6-submit/drivers/usb/host/ehci-mxc.c
>>> ===================================================================
>>> --- linux-2.6-submit.orig/drivers/usb/host/ehci-mxc.c	2011-01-06 14:45:14.000000000 +0100
>>> +++ linux-2.6-submit/drivers/usb/host/ehci-mxc.c	2011-01-06 15:48:16.000000000 +0100
>>> @@ -25,6 +25,8 @@
>>>
>>>  #include <mach/mxc_ehci.h>
>>>
>>> +#include <asm/mach-types.h>
>>> +
>>>  #define ULPI_VIEWPORT_OFFSET	0x170
>>>
>>>  struct ehci_mxc_priv {
>>> @@ -239,6 +241,13 @@
>>>  			dev_err(dev, "unable to enable vbus on transceiver\n");
>>>  			goto err_add;
>>>  		}
>>> +		if (machine_is_mx51_efikamx() || machine_is_mx51_efikasb()) {
>>> +			ret = otg_start_srp(pdata->otg);
>>> +			if (ret) {
>>> +				dev_err(dev, "unable to start srp\n");
>>> +				goto err_add;
>>> +			}
>>> +		}
>> In light of the problem described above, here you will need to write directly
>> to the view port instead of calling the otg_start_srp() function.
> ok. I don't think it's a problem. Looks otg_io_(read|write) are
> available here.

yeah, otg_io_(read|write) should do.

>> I'd recommend to move this to the machine (board) specific code.
>> May be to platform init (pdata->init) call back?
> oh, no. I've put this call in this place for some good reasons. This
> trick is working only if it's done _after_ add_hcd. Any other place
> won't work. For instance, pdata->init is meant to be called before
> mxc_initialise_usb_hw iirc which is _before_ add_hcd.

I see. The reason, I'd like to move it to the board code is a possibility that h/w will
change (hopefully even fixed) and then you will not have to violate the spec
anymore, but to deal with the h/w revision checking. ehci-mxc.c doesn't look
like a good place to deal with h/w revision checking of the board, so may be it is
worth to introduce some kind of another platform hook, like pdata->late_init?

I don't insist, it is up to you and Sascha to decide what's the best choice here.

>> IMHO, the explanation of the h/w bug workaround in the commit message
>> is not enough - in-code comment regarding the bug workaround should be added.
> I know. As I said this patch was not the final submission. It was
> probably not clear enough.
>
> Arnaud

-- 
Regards,
Igor.

[patch 3/5] arch/arm/plat-mxc/ehci.c: fix errors/typos

[Arnaud Patard (Rtp)]

An embedded and charset-unspecified text was scrubbed...
Name: plat_mxc_ehci_fix_flags.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101220/7c798ce9/attachment.ksh>

[patch 4/5] MX51: Add support for usb host 2

[Arnaud Patard (Rtp)]

An embedded and charset-unspecified text was scrubbed...
Name: mx51_usbhost2.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101220/9f12177e/attachment.ksh>

[patch 5/5] mx51: fix usb clock support

[Arnaud Patard (Rtp)]

An embedded and charset-unspecified text was scrubbed...
Name: mx51_usb_clk.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101220/2b65e1b4/attachment.ksh>