diff mbox

Nokia N900: musb is in wrong state after boot

Message ID 201601231357.32629@pali
State New, archived
Headers show

Commit Message

Pali Rohár Jan. 23, 2016, 12:57 p.m. UTC
On Thursday 21 January 2016 20:21:13 Tony Lindgren wrote:
> * joerg Reisenweber <joerg@openmoko.org> [160121 10:45]:
> > On Thu 21 January 2016 09:41:46 Tony Lindgren wrote:
> > > Then for supporting the USB host mode.. We should add regulator
> > > support to the USB PHY driver so if the ID pin is grounded, the
> > > PHY driver enables the VBUS regulator. That too seems to need
> > > some coordination between the drivers/phy/phy-twl4030-usb.c and
> > > 1707 driver if the ID pin interrupt is only detected in
> > > drivers/phy/phy-twl4030-usb.c.
> > 
> > Note that, while this is probably a good thing to do, it needs to
> > be sufficiently loose coupling to allow user to 'intercept' this
> > VBOOS regulator enabling and instead allow device charging while
> > in externally powered hostmode. There's even a spec for this in
> > USB-docs-foo iirc, something along a certain resistor value on ID
> > to GND - alas I guess the twl4030 is not capable to detect such
> > sophisticated signaling, and anyway it's always desirable to allow
> > user to manually override the VBOOST and enable VBUS-charging
> > while in hostmode.
> 
> OK, I think this is what's happening with the Motorola LapDock BTW.
> It always feeds the VBUS, well most of the time. Do you have some
> pointer to the "certain resistor value on ID to GND" spec? Is it
> maybe part of the carkit related parts of the USB spec?
> 
> > On N900 the situation is even more complex since the 1707 doesn't
> > support genuine ID detection, neither does it support emulated ID
> > grounding. And there's no other method than a ID=GND message from
> > PHY to musb core to make the musb core state engine transfer into
> > proper hostmode. Thus my H-E-N hostmode botch abuses debug flags
> > to force the musb core into a "emulated" hostmode and this mode
> > doesn't support USB speed detection. Thus speed settings are
> > forced onto musb core and PHY by software, and the musb core speed
> > bits are only effective before session enabled.
> > Bottom line: you need VBUS to try and negotiate speed with the
> > attached device in hostmode, but to actually set this speed you
> > detected by software means, you need to disable and discharge VBUS
> > again, or musb core won't care about the speed you set. To be
> > utterly clear: unconditional enabling of VBUS in ID=GND won't
> > work.
> > 
> > This is quite complex and it's questionable if it could get handled
> > reasonably in kernel space. *Very* N900 specific niche solution,
> > I'd not think it's suited for upstreaming.
> 
> Yeah OK. I think we should be able to support the aux VBUS regulator
> part with mainline kernel though.
> 
> Regards,
> 
> Tony

Hello, attached patch for musb debugfs adds option to force both 
hostmode with speed. It is just example, I tested only compilation.

Something like that will be needed for usb host mode on Nokia N900.

Comments

Pali Rohár May 29, 2016, 10:38 a.m. UTC | #1
On Saturday 23 January 2016 13:57:32 Pali Rohár wrote:
> On Thursday 21 January 2016 20:21:13 Tony Lindgren wrote:
> > * joerg Reisenweber <joerg@openmoko.org> [160121 10:45]:
> > > On Thu 21 January 2016 09:41:46 Tony Lindgren wrote:
> > > > Then for supporting the USB host mode.. We should add regulator
> > > > support to the USB PHY driver so if the ID pin is grounded, the
> > > > PHY driver enables the VBUS regulator. That too seems to need
> > > > some coordination between the drivers/phy/phy-twl4030-usb.c and
> > > > 1707 driver if the ID pin interrupt is only detected in
> > > > drivers/phy/phy-twl4030-usb.c.
> > > 
> > > Note that, while this is probably a good thing to do, it needs to
> > > be sufficiently loose coupling to allow user to 'intercept' this
> > > VBOOS regulator enabling and instead allow device charging while
> > > in externally powered hostmode. There's even a spec for this in
> > > USB-docs-foo iirc, something along a certain resistor value on ID
> > > to GND - alas I guess the twl4030 is not capable to detect such
> > > sophisticated signaling, and anyway it's always desirable to allow
> > > user to manually override the VBOOST and enable VBUS-charging
> > > while in hostmode.
> > 
> > OK, I think this is what's happening with the Motorola LapDock BTW.
> > It always feeds the VBUS, well most of the time. Do you have some
> > pointer to the "certain resistor value on ID to GND" spec? Is it
> > maybe part of the carkit related parts of the USB spec?
> > 
> > > On N900 the situation is even more complex since the 1707 doesn't
> > > support genuine ID detection, neither does it support emulated ID
> > > grounding. And there's no other method than a ID=GND message from
> > > PHY to musb core to make the musb core state engine transfer into
> > > proper hostmode. Thus my H-E-N hostmode botch abuses debug flags
> > > to force the musb core into a "emulated" hostmode and this mode
> > > doesn't support USB speed detection. Thus speed settings are
> > > forced onto musb core and PHY by software, and the musb core speed
> > > bits are only effective before session enabled.
> > > Bottom line: you need VBUS to try and negotiate speed with the
> > > attached device in hostmode, but to actually set this speed you
> > > detected by software means, you need to disable and discharge VBUS
> > > again, or musb core won't care about the speed you set. To be
> > > utterly clear: unconditional enabling of VBUS in ID=GND won't
> > > work.
> > > 
> > > This is quite complex and it's questionable if it could get handled
> > > reasonably in kernel space. *Very* N900 specific niche solution,
> > > I'd not think it's suited for upstreaming.
> > 
> > Yeah OK. I think we should be able to support the aux VBUS regulator
> > part with mainline kernel though.
> > 
> > Regards,
> > 
> > Tony
> 
> Hello, attached patch for musb debugfs adds option to force both 
> hostmode with speed. It is just example, I tested only compilation.
> 
> Something like that will be needed for usb host mode on Nokia N900.
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com

> From fd67b58e3538c0732750ecad915cde736da099dc Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali.rohar@gmail.com>
> Date: Sat, 9 Jan 2016 16:57:59 +0100
> Subject: [PATCH] musb: debugfs: Add support in testmode for forcing host mode
>  together with speed
> 
> ---
>  drivers/usb/musb/musb_debugfs.c |   44 +++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c
> index 9b22d94..62c13a2 100644
> --- a/drivers/usb/musb/musb_debugfs.c
> +++ b/drivers/usb/musb/musb_debugfs.c
> @@ -147,28 +147,34 @@ static int musb_test_mode_show(struct seq_file *s, void *unused)
>  
>  	test = musb_readb(musb->mregs, MUSB_TESTMODE);
>  
> -	if (test & MUSB_TEST_FORCE_HOST)
> +	if (test & (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS))
> +		seq_printf(s, "force host full-speed\n");
> +
> +	else if (test & (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS))
> +		seq_printf(s, "force host high-speed\n");
> +
> +	else if (test & MUSB_TEST_FORCE_HOST)
>  		seq_printf(s, "force host\n");
>  
> -	if (test & MUSB_TEST_FIFO_ACCESS)
> +	else if (test & MUSB_TEST_FIFO_ACCESS)
>  		seq_printf(s, "fifo access\n");
>  
> -	if (test & MUSB_TEST_FORCE_FS)
> +	else if (test & MUSB_TEST_FORCE_FS)
>  		seq_printf(s, "force full-speed\n");
>  
> -	if (test & MUSB_TEST_FORCE_HS)
> +	else if (test & MUSB_TEST_FORCE_HS)
>  		seq_printf(s, "force high-speed\n");
>  
> -	if (test & MUSB_TEST_PACKET)
> +	else if (test & MUSB_TEST_PACKET)
>  		seq_printf(s, "test packet\n");
>  
> -	if (test & MUSB_TEST_K)
> +	else if (test & MUSB_TEST_K)
>  		seq_printf(s, "test K\n");
>  
> -	if (test & MUSB_TEST_J)
> +	else if (test & MUSB_TEST_J)
>  		seq_printf(s, "test J\n");
>  
> -	if (test & MUSB_TEST_SE0_NAK)
> +	else if (test & MUSB_TEST_SE0_NAK)
>  		seq_printf(s, "test SE0 NAK\n");
>  
>  	return 0;
> @@ -206,30 +212,36 @@ static ssize_t musb_test_mode_write(struct file *file,
>  	if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
>  		return -EFAULT;
>  
> -	if (strstarts(buf, "force host"))
> +	if (strstarts(buf, "force host full-speed"))
> +		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS;
> +
> +	else if (strstarts(buf, "force host high-speed"))
> +		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS;
> +
> +	else if (strstarts(buf, "force host"))
>  		test = MUSB_TEST_FORCE_HOST;
>  
> -	if (strstarts(buf, "fifo access"))
> +	else if (strstarts(buf, "fifo access"))
>  		test = MUSB_TEST_FIFO_ACCESS;
>  
> -	if (strstarts(buf, "force full-speed"))
> +	else if (strstarts(buf, "force full-speed"))
>  		test = MUSB_TEST_FORCE_FS;
>  
> -	if (strstarts(buf, "force high-speed"))
> +	else if (strstarts(buf, "force high-speed"))
>  		test = MUSB_TEST_FORCE_HS;
>  
> -	if (strstarts(buf, "test packet")) {
> +	else if (strstarts(buf, "test packet")) {
>  		test = MUSB_TEST_PACKET;
>  		musb_load_testpacket(musb);
>  	}
>  
> -	if (strstarts(buf, "test K"))
> +	else if (strstarts(buf, "test K"))
>  		test = MUSB_TEST_K;
>  
> -	if (strstarts(buf, "test J"))
> +	else if (strstarts(buf, "test J"))
>  		test = MUSB_TEST_J;
>  
> -	if (strstarts(buf, "test SE0 NAK"))
> +	else if (strstarts(buf, "test SE0 NAK"))
>  		test = MUSB_TEST_SE0_NAK;
>  
>  	musb_writeb(musb->mregs, MUSB_TESTMODE, test);
> -- 
> 1.7.9.5
> 

Tony, what do you think about that patch?
Pali Rohár June 7, 2016, 12:50 p.m. UTC | #2
On Sunday 29 May 2016 12:38:24 Pali Rohár wrote:
> > From fd67b58e3538c0732750ecad915cde736da099dc Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali.rohar@gmail.com>
> > Date: Sat, 9 Jan 2016 16:57:59 +0100
> > Subject: [PATCH] musb: debugfs: Add support in testmode for forcing host mode
> >  together with speed
> > 
> > ---
> >  drivers/usb/musb/musb_debugfs.c |   44 +++++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c
> > index 9b22d94..62c13a2 100644
> > --- a/drivers/usb/musb/musb_debugfs.c
> > +++ b/drivers/usb/musb/musb_debugfs.c
> > @@ -147,28 +147,34 @@ static int musb_test_mode_show(struct seq_file *s, void *unused)
> >  
> >  	test = musb_readb(musb->mregs, MUSB_TESTMODE);
> >  
> > -	if (test & MUSB_TEST_FORCE_HOST)
> > +	if (test & (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS))
> > +		seq_printf(s, "force host full-speed\n");
> > +
> > +	else if (test & (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS))
> > +		seq_printf(s, "force host high-speed\n");
> > +
> > +	else if (test & MUSB_TEST_FORCE_HOST)
> >  		seq_printf(s, "force host\n");
> >  
> > -	if (test & MUSB_TEST_FIFO_ACCESS)
> > +	else if (test & MUSB_TEST_FIFO_ACCESS)
> >  		seq_printf(s, "fifo access\n");
> >  
> > -	if (test & MUSB_TEST_FORCE_FS)
> > +	else if (test & MUSB_TEST_FORCE_FS)
> >  		seq_printf(s, "force full-speed\n");
> >  
> > -	if (test & MUSB_TEST_FORCE_HS)
> > +	else if (test & MUSB_TEST_FORCE_HS)
> >  		seq_printf(s, "force high-speed\n");
> >  
> > -	if (test & MUSB_TEST_PACKET)
> > +	else if (test & MUSB_TEST_PACKET)
> >  		seq_printf(s, "test packet\n");
> >  
> > -	if (test & MUSB_TEST_K)
> > +	else if (test & MUSB_TEST_K)
> >  		seq_printf(s, "test K\n");
> >  
> > -	if (test & MUSB_TEST_J)
> > +	else if (test & MUSB_TEST_J)
> >  		seq_printf(s, "test J\n");
> >  
> > -	if (test & MUSB_TEST_SE0_NAK)
> > +	else if (test & MUSB_TEST_SE0_NAK)
> >  		seq_printf(s, "test SE0 NAK\n");
> >  
> >  	return 0;
> > @@ -206,30 +212,36 @@ static ssize_t musb_test_mode_write(struct file *file,
> >  	if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> >  		return -EFAULT;
> >  
> > -	if (strstarts(buf, "force host"))
> > +	if (strstarts(buf, "force host full-speed"))
> > +		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS;
> > +
> > +	else if (strstarts(buf, "force host high-speed"))
> > +		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS;
> > +
> > +	else if (strstarts(buf, "force host"))
> >  		test = MUSB_TEST_FORCE_HOST;
> >  
> > -	if (strstarts(buf, "fifo access"))
> > +	else if (strstarts(buf, "fifo access"))
> >  		test = MUSB_TEST_FIFO_ACCESS;
> >  
> > -	if (strstarts(buf, "force full-speed"))
> > +	else if (strstarts(buf, "force full-speed"))
> >  		test = MUSB_TEST_FORCE_FS;
> >  
> > -	if (strstarts(buf, "force high-speed"))
> > +	else if (strstarts(buf, "force high-speed"))
> >  		test = MUSB_TEST_FORCE_HS;
> >  
> > -	if (strstarts(buf, "test packet")) {
> > +	else if (strstarts(buf, "test packet")) {
> >  		test = MUSB_TEST_PACKET;
> >  		musb_load_testpacket(musb);
> >  	}
> >  
> > -	if (strstarts(buf, "test K"))
> > +	else if (strstarts(buf, "test K"))
> >  		test = MUSB_TEST_K;
> >  
> > -	if (strstarts(buf, "test J"))
> > +	else if (strstarts(buf, "test J"))
> >  		test = MUSB_TEST_J;
> >  
> > -	if (strstarts(buf, "test SE0 NAK"))
> > +	else if (strstarts(buf, "test SE0 NAK"))
> >  		test = MUSB_TEST_SE0_NAK;
> >  
> >  	musb_writeb(musb->mregs, MUSB_TESTMODE, test);
> > -- 
> > 1.7.9.5
> > 
> 
> Tony, what do you think about that patch?
> 

Tony, PING
Tony Lindgren June 8, 2016, 9:53 a.m. UTC | #3
* Pali Rohár <pali.rohar@gmail.com> [160607 05:53]:
> > Tony, what do you think about that patch?
> > 
> 
> Tony, PING

Yeah I don't know, AFAIK we don't have a generic way to
force MUSB to change mode without ID pin. If you have figured
something generic for that which does not actually tinker with
the PHY registers directly, that should be the generic
musb_set_mode() that we've been wondering about for years.

Maybe split the patches to the mode change and then the
sysfs entry fixes and repost to linux-usb and linux-omap
and make sure Bin as the new musb maintainer is included :)

The custom sysfs flags we should maintain support for naturally.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi June 8, 2016, 10:02 a.m. UTC | #4
Hi,

Tony Lindgren <tony@atomide.com> writes:
> * Pali Rohár <pali.rohar@gmail.com> [160607 05:53]:
>> > Tony, what do you think about that patch?
>> > 
>> 
>> Tony, PING
>
> Yeah I don't know, AFAIK we don't have a generic way to
> force MUSB to change mode without ID pin. If you have figured
> something generic for that which does not actually tinker with
> the PHY registers directly, that should be the generic
> musb_set_mode() that we've been wondering about for years.

#define MUSB_TEST_FORCE_HOST	0x80

Can someone confirm on MUSB's docs (and actual running system) that this
does what's supposed to do?
Pali Rohár June 8, 2016, 10:19 a.m. UTC | #5
On Wednesday 08 June 2016 12:02:00 Felipe Balbi wrote:
> Hi,
> 
> Tony Lindgren <tony@atomide.com> writes:
> > * Pali Rohár <pali.rohar@gmail.com> [160607 05:53]:
> >> > Tony, what do you think about that patch?
> >> 
> >> Tony, PING
> > 
> > Yeah I don't know, AFAIK we don't have a generic way to
> > force MUSB to change mode without ID pin. If you have figured
> > something generic for that which does not actually tinker with
> > the PHY registers directly, that should be the generic
> > musb_set_mode() that we've been wondering about for years.
> 
> #define MUSB_TEST_FORCE_HOST	0x80
> 
> Can someone confirm on MUSB's docs (and actual running system) that
> this does what's supposed to do?

Maybe this can help you:

https://garage.maemo.org/plugins/ggit/browse.php/?p=kernel-power;a=blob;f=kernel-power-2.6.28/debian/patches/usbhostmode.diff

Patch for 2.6.28 omap1 kernel which adds usb host mode support for Nokia
N900. Look for MUSB_TESTMODE, MUSB_TEST_FORCE_HOST is called together
with MUSB_TEST_FORCE_FS or MUSB_TEST_FORCE_HS (line 355).
joerg Reisenweber June 8, 2016, 10:20 a.m. UTC | #6
On Wed 08 June 2016 13:02:00 Felipe Balbi wrote:
> Hi,
> 
> Tony Lindgren <tony@atomide.com> writes:
> > Yeah I don't know, AFAIK we don't have a generic way to
> > force MUSB to change mode without ID pin. If you have figured
> #define MUSB_TEST_FORCE_HOST	0x80
> 
> Can someone confirm on MUSB's docs (and actual running system) that this
> does what's supposed to do?



(disclaimer: all IIRC) Alas it doesn't, at least for the MentorGraphics MUSB 
in OMAP3.  
We use MUSB_TEST_FORCE_HOST in the H-E-N N900 hostmode hack 
(http://talk.maemo.org/showthread.php?p=696115) and it's really just a test 
thing that doesn't support full hostmode as we know it. 

There is definitely no other way to force the state machine in MUSB core into 
hostmode, than the "ID pin grounded" message from PHY via ULPI bus up to the 
MUSB, whether it's real or *sw-triggered by a command to PHY*.
Shame on the MUSB core design for this.


/jOERG
Sergei Shtylyov June 8, 2016, 12:04 p.m. UTC | #7
On 6/8/2016 1:02 PM, Felipe Balbi wrote:

>> * Pali Rohár <pali.rohar@gmail.com> [160607 05:53]:
>>>> Tony, what do you think about that patch?
>>>>
>>>
>>> Tony, PING
>>
>> Yeah I don't know, AFAIK we don't have a generic way to
>> force MUSB to change mode without ID pin. If you have figured
>> something generic for that which does not actually tinker with
>> the PHY registers directly, that should be the generic
>> musb_set_mode() that we've been wondering about for years.
>
> #define MUSB_TEST_FORCE_HOST	0x80
>
> Can someone confirm on MUSB's docs (and actual running system) that this
> does what's supposed to do?

    The MUSB programmer's guide says the CID (sic) input is ignored when the 
Force_Host bit is set. The host mode is entered when the Session bit is set.
But I don't have a MUSB hardware readily available to confirm.

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
joerg Reisenweber June 8, 2016, 12:18 p.m. UTC | #8
On Wed 08 June 2016 15:04:02 Sergei Shtylyov wrote:
> On 6/8/2016 1:02 PM, Felipe Balbi wrote:
> >> * Pali Rohár <pali.rohar@gmail.com> [160607 05:53]:
> >>>> Tony, what do you think about that patch?
> >>> 
> >>> Tony, PING
> >> 
> >> Yeah I don't know, AFAIK we don't have a generic way to
> >> force MUSB to change mode without ID pin. If you have figured
> >> something generic for that which does not actually tinker with
> >> the PHY registers directly, that should be the generic
> >> musb_set_mode() that we've been wondering about for years.
> > 
> > #define MUSB_TEST_FORCE_HOST	0x80
> > 
> > Can someone confirm on MUSB's docs (and actual running system) that this
> > does what's supposed to do?
> 
>     The MUSB programmer's guide says the CID (sic) input is ignored when the
> Force_Host bit is set. The host mode is entered when the Session bit is
> set. But I don't have a MUSB hardware readily available to confirm.
> 
> MBR, Sergei

also says:

While the FORCE_HOST bit remains set, the core will enter Host mode when the 
Session bit is set and **remain in Host mode until the Session bit is cleared 
even if a connected device is disconnected during the session.** 
The operating speed while in this mode **is determined for the setting of the 
FORCE_HS and FORCE_FS bits of the Testmode register** in Section 23.1.4.11.

see http://talk.maemo.org/showthread.php?p=685367

It's not any fully operational hostmode.

/j
Sergei Shtylyov June 8, 2016, 12:32 p.m. UTC | #9
On 6/8/2016 3:18 PM, joerg Reisenweber wrote:

>>>>>> Tony, what do you think about that patch?
>>>>>
>>>>> Tony, PING
>>>>
>>>> Yeah I don't know, AFAIK we don't have a generic way to
>>>> force MUSB to change mode without ID pin. If you have figured
>>>> something generic for that which does not actually tinker with
>>>> the PHY registers directly, that should be the generic
>>>> musb_set_mode() that we've been wondering about for years.
>>>
>>> #define MUSB_TEST_FORCE_HOST	0x80
>>>
>>> Can someone confirm on MUSB's docs (and actual running system) that this
>>> does what's supposed to do?
>>
>>     The MUSB programmer's guide says the CID (sic) input is ignored when the
>> Force_Host bit is set. The host mode is entered when the Session bit is
>> set. But I don't have a MUSB hardware readily available to confirm.
>>
>> MBR, Sergei
>
> also says:
>
> While the FORCE_HOST bit remains set, the core will enter Host mode when the
> Session bit is set and **remain in Host mode until the Session bit is cleared
> even if a connected device is disconnected during the session.**
> The operating speed while in this mode **is determined for the setting of the
> FORCE_HS and FORCE_FS bits of the Testmode register** in Section 23.1.4.11.

    Yeah, the MUSB PG says the same.

[...]

> /j

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bin Liu June 9, 2016, 8:58 p.m. UTC | #10
Hi,

On Sun, May 29, 2016 at 12:38:24PM +0200, Pali Rohár wrote:
> > Hello, attached patch for musb debugfs adds option to force both 
> > hostmode with speed. It is just example, I tested only compilation.
> > 
> > Something like that will be needed for usb host mode on Nokia N900.
> > 
> > -- 
> > Pali Rohár
> > pali.rohar@gmail.com
> 
> > From fd67b58e3538c0732750ecad915cde736da099dc Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali.rohar@gmail.com>
> > Date: Sat, 9 Jan 2016 16:57:59 +0100
> > Subject: [PATCH] musb: debugfs: Add support in testmode for forcing host mode
> >  together with speed
> > 
> > ---
> >  drivers/usb/musb/musb_debugfs.c |   44 +++++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c
> > index 9b22d94..62c13a2 100644
> > --- a/drivers/usb/musb/musb_debugfs.c
> > +++ b/drivers/usb/musb/musb_debugfs.c
> > @@ -147,28 +147,34 @@ static int musb_test_mode_show(struct seq_file *s, void *unused)
> >  
> >  	test = musb_readb(musb->mregs, MUSB_TESTMODE);
> >  
> > -	if (test & MUSB_TEST_FORCE_HOST)
> > +	if (test & (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS))
> > +		seq_printf(s, "force host full-speed\n");
> > +
> > +	else if (test & (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS))
> > +		seq_printf(s, "force host high-speed\n");
> > +
> > +	else if (test & MUSB_TEST_FORCE_HOST)
> >  		seq_printf(s, "force host\n");
> >  
> > -	if (test & MUSB_TEST_FIFO_ACCESS)
> > +	else if (test & MUSB_TEST_FIFO_ACCESS)
> >  		seq_printf(s, "fifo access\n");
> >  
> > -	if (test & MUSB_TEST_FORCE_FS)
> > +	else if (test & MUSB_TEST_FORCE_FS)
> >  		seq_printf(s, "force full-speed\n");
> >  
> > -	if (test & MUSB_TEST_FORCE_HS)
> > +	else if (test & MUSB_TEST_FORCE_HS)
> >  		seq_printf(s, "force high-speed\n");
> >  
> > -	if (test & MUSB_TEST_PACKET)
> > +	else if (test & MUSB_TEST_PACKET)
> >  		seq_printf(s, "test packet\n");
> >  
> > -	if (test & MUSB_TEST_K)
> > +	else if (test & MUSB_TEST_K)
> >  		seq_printf(s, "test K\n");
> >  
> > -	if (test & MUSB_TEST_J)
> > +	else if (test & MUSB_TEST_J)
> >  		seq_printf(s, "test J\n");
> >  
> > -	if (test & MUSB_TEST_SE0_NAK)
> > +	else if (test & MUSB_TEST_SE0_NAK)
> >  		seq_printf(s, "test SE0 NAK\n");
> >  
> >  	return 0;
> > @@ -206,30 +212,36 @@ static ssize_t musb_test_mode_write(struct file *file,
> >  	if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> >  		return -EFAULT;
> >  
> > -	if (strstarts(buf, "force host"))
> > +	if (strstarts(buf, "force host full-speed"))
> > +		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS;
> > +
> > +	else if (strstarts(buf, "force host high-speed"))
> > +		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS;
> > +
> > +	else if (strstarts(buf, "force host"))
> >  		test = MUSB_TEST_FORCE_HOST;
> >  
> > -	if (strstarts(buf, "fifo access"))
> > +	else if (strstarts(buf, "fifo access"))
> >  		test = MUSB_TEST_FIFO_ACCESS;
> >  
> > -	if (strstarts(buf, "force full-speed"))
> > +	else if (strstarts(buf, "force full-speed"))
> >  		test = MUSB_TEST_FORCE_FS;
> >  
> > -	if (strstarts(buf, "force high-speed"))
> > +	else if (strstarts(buf, "force high-speed"))
> >  		test = MUSB_TEST_FORCE_HS;
> >  
> > -	if (strstarts(buf, "test packet")) {
> > +	else if (strstarts(buf, "test packet")) {
> >  		test = MUSB_TEST_PACKET;
> >  		musb_load_testpacket(musb);
> >  	}
> >  
> > -	if (strstarts(buf, "test K"))
> > +	else if (strstarts(buf, "test K"))
> >  		test = MUSB_TEST_K;
> >  
> > -	if (strstarts(buf, "test J"))
> > +	else if (strstarts(buf, "test J"))
> >  		test = MUSB_TEST_J;
> >  
> > -	if (strstarts(buf, "test SE0 NAK"))
> > +	else if (strstarts(buf, "test SE0 NAK"))
> >  		test = MUSB_TEST_SE0_NAK;
> >  
> >  	musb_writeb(musb->mregs, MUSB_TESTMODE, test);
> > -- 
> > 1.7.9.5
> > 
> 
> Tony, what do you think about that patch?

Based on the musb ug, force_host bit is allowed to be set along with
force_hs or force_fs bit. So please resend this patch with subject
changed to the following, and I will take it. The current suject makes
is sound like a new feature rather than a bugfix.

"usb: musb: debugfs: allow forcing host mode together with speed in testmode"

But I am not sure how this will fix n900 host mode problem, since
testmode is not used in normal operation.

Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bin Liu June 9, 2016, 9:09 p.m. UTC | #11
Hi,

On Wed, Jun 08, 2016 at 01:02:00PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Tony Lindgren <tony@atomide.com> writes:
> > * Pali Rohár <pali.rohar@gmail.com> [160607 05:53]:
> >> > Tony, what do you think about that patch?
> >> > 
> >> 
> >> Tony, PING
> >
> > Yeah I don't know, AFAIK we don't have a generic way to
> > force MUSB to change mode without ID pin. If you have figured
> > something generic for that which does not actually tinker with
> > the PHY registers directly, that should be the generic
> > musb_set_mode() that we've been wondering about for years.
> 
> #define MUSB_TEST_FORCE_HOST	0x80
> 
> Can someone confirm on MUSB's docs (and actual running system) that this
> does what's supposed to do?

I don't know what to expect while setting this bit. I am never able to get musb
to enumerate anything or does anything which a host suppose to do.

Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
joerg Reisenweber June 9, 2016, 9:25 p.m. UTC | #12
On Thu 09 June 2016 15:58:56 Bin Liu wrote:
> But I am not sure how this will fix n900 host mode problem, since
> testmode is not used in normal operation.

Please see http://talk.maemo.org/showthread.php?p=685367 and 
http://maemo.org/packages/view/hostmode-gui/ which indeed is _no_ normal 
hostmode but an implementation of a workaround that exploits the force host 
test mode (among others)

/jOERG
Bin Liu June 10, 2016, 3:08 p.m. UTC | #13
Hi,

On Thu, Jun 09, 2016 at 11:25:57PM +0200, joerg Reisenweber wrote:
> On Thu 09 June 2016 15:58:56 Bin Liu wrote:
> > But I am not sure how this will fix n900 host mode problem, since
> > testmode is not used in normal operation.
> 
> Please see http://talk.maemo.org/showthread.php?p=685367 and 
> http://maemo.org/packages/view/hostmode-gui/ which indeed is _no_ normal 
> hostmode but an implementation of a workaround that exploits the force host 
> test mode (among others)

What I meant was that it is okay for hobby, but you don't want to
something that the IP vender does not support in real products.

Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
joerg Reisenweber June 10, 2016, 3:23 p.m. UTC | #14
On Fri 10 June 2016 10:08:08 Bin Liu wrote:
> Hi,
> 
> On Thu, Jun 09, 2016 at 11:25:57PM +0200, joerg Reisenweber wrote:
> > On Thu 09 June 2016 15:58:56 Bin Liu wrote:
> > > But I am not sure how this will fix n900 host mode problem, since
> > > testmode is not used in normal operation.
> > 
> > Please see http://talk.maemo.org/showthread.php?p=685367 and
> > http://maemo.org/packages/view/hostmode-gui/ which indeed is _no_ normal
> > hostmode but an implementation of a workaround that exploits the force
> > host
> > test mode (among others)
> 
> What I meant was that it is okay for hobby, but you don't want to
> something that the IP vender does not support in real products.


You lost me. 
> > > But I am not sure how this will fix n900 host mode problem
http://maemo.org/packages/view/hostmode-gui/ and the associated kernel patches 
for FORCE_HOSTMODE _is_ how we 'fix' the N900 hostmode problem (see 
https://www.youtube.com/watch?v=fkCDyUO0sKQ for a proof)

/j
Bin Liu June 10, 2016, 3:59 p.m. UTC | #15
Hi,

On Fri, Jun 10, 2016 at 05:23:11PM +0200, joerg Reisenweber wrote:
> On Fri 10 June 2016 10:08:08 Bin Liu wrote:
> > Hi,
> > 
> > On Thu, Jun 09, 2016 at 11:25:57PM +0200, joerg Reisenweber wrote:
> > > On Thu 09 June 2016 15:58:56 Bin Liu wrote:
> > > > But I am not sure how this will fix n900 host mode problem, since
> > > > testmode is not used in normal operation.
> > > 
> > > Please see http://talk.maemo.org/showthread.php?p=685367 and
> > > http://maemo.org/packages/view/hostmode-gui/ which indeed is _no_ normal
> > > hostmode but an implementation of a workaround that exploits the force
> > > host
> > > test mode (among others)
> > 
> > What I meant was that it is okay for hobby, but you don't want to
> > something that the IP vender does not support in real products.
> 
> 
> You lost me.

Sorry.

The musb ug says the testmde is not used in normal operation, so my
opinion is force_host should not be used for hacking n900 host mode if
this is for real product development or support.

Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
joerg Reisenweber June 10, 2016, 4:15 p.m. UTC | #16
On Fri 10 June 2016 10:59:40 Bin Liu wrote:
> The musb ug says the testmde is not used in normal operation, so my
> opinion is force_host should not be used for hacking n900 host mode if
> this is for real product development or support.

You're aware N900 OS aka maemo is a) FOSS, and b) EOL at least from Nokia's 
POV? So there's neither "product development" nor any _'official'_ support 
involved.
And c) we (community) already _did_ use it since it was the only chance to 
make hostmode sort of work for N900, it's not like we could redesign N900 
hardware to support regular hostmode, we need to work with what RL gave us. 
It evades me why you discourage resp reject this established solution. 
Just Nokia not supporting hostmode evidently doesn't mean we can't get 
anything done, and I don't see why we should refrain from doing so.

/j
Nishanth Menon June 10, 2016, 5:04 p.m. UTC | #17
On 06/10/2016 11:15 AM, joerg Reisenweber wrote:

Sorry for butting in...

> On Fri 10 June 2016 10:59:40 Bin Liu wrote:
>> The musb ug says the testmde is not used in normal operation, so my
>> opinion is force_host should not be used for hacking n900 host mode if
>> this is for real product development or support.
> 
> You're aware N900 OS aka maemo is a) FOSS, and b) EOL at least from Nokia's 
> POV? So there's neither "product development" nor any _'official'_ support 
> involved.
> And c) we (community) already _did_ use it since it was the only chance to 
> make hostmode sort of work for N900, it's not like we could redesign N900 
> hardware to support regular hostmode, we need to work with what RL gave us. 
> It evades me why you discourage resp reject this established solution. 
> Just Nokia not supporting hostmode evidently doesn't mean we can't get 
> anything done, and I don't see why we should refrain from doing so.

I think there was some unfortunately choice of words used in the
thread. It is TI intention to support community effort and we are very
appreciative of the work and effort done by the N900 community. Please
do not misunderstand that we dont care for FOSS community, in fact, we
are part of the FOSS community as well and a significant investment is
done to ensure that "upstream first" approach is taken to benefit
everyone.

Hopefully with that out of the way, on this specific topic, based on a
quick chat with Bin, I think Bin meant to indicate that as per Mentor
vendor documentation, the option is a test mode meant for silicon
validation purposes - typically many vendor hardware blocks have these
"test mode" bits and options meant to help silicon validation
software, unfortunately these modes do not tend to be well tested and
the typical "official disclaimer" is "Not for 'production device
usage' and 'user might be on  his/her own' " - That does not mean it
cannot work, but it may not always be working OR can have reliability
issues/open up unknown silicon issues that has not been well covered
by SoC and/or IP vendor. In this case specifically, I think Bin's
experience of having had tried to get this working in AM335x and had
failed makes him a little more skeptical.

I think Bin has accepted this patch, but anyways, it is always good to
highlight potential risk. I assume Bin can elaborate more as needed.

Post Note: We all do appreciate all the creative ways folks do use TI
SoCs, it is important we try and continue do that to leverage every
single transistor that the SoC has, but we should also just keep a
watch for any potential risks we might have to face with these options
we exploit.
joerg Reisenweber June 10, 2016, 5:21 p.m. UTC | #18
On Fri 10 June 2016 12:04:43 Nishanth Menon wrote:
> I think there was some unfortunately choice of words used in the
> thread. It is TI intention to support community effort
[...]
> software, unfortunately these modes do not tend to be well tested and
> the typical "official disclaimer" is "Not for 'production device
> usage' and 'user might be on  his/her own' " 

Understood, agreed and appreciated, many thanks! :-)
I didn't mean to suggest this as the recommended way to implement hostmode on 
arbitrary devices, we just had to find a way to fix a hw-flaw in N900 platform. 
Odds are this N900 specific H-E-N botch won't work on any other platform ever.

Best Regards
jOERG
Bin Liu June 10, 2016, 5:37 p.m. UTC | #19
Hi,

On Fri, Jun 10, 2016 at 06:15:26PM +0200, joerg Reisenweber wrote:
> On Fri 10 June 2016 10:59:40 Bin Liu wrote:
> > The musb ug says the testmde is not used in normal operation, so my
> > opinion is force_host should not be used for hacking n900 host mode if
> > this is for real product development or support.
> 
> You're aware N900 OS aka maemo is a) FOSS, and b) EOL at least from Nokia's 
> POV? So there's neither "product development" nor any _'official'_ support 
> involved.
> And c) we (community) already _did_ use it since it was the only chance to 
> make hostmode sort of work for N900, it's not like we could redesign N900 
> hardware to support regular hostmode, we need to work with what RL gave us. 

Well, misunderstanding does happening as Nishanth warned me offline :(

> It evades me why you discourage resp reject this established solution. 

Not sure how you concluded I reject this. Instead I am taking patches.
The only thing I did was warning that if you use force_host testmode in
normal operation in your 'products', you would get liability problem.

> Just Nokia not supporting hostmode evidently doesn't mean we can't get 
> anything done, and I don't see why we should refrain from doing so.

Regards,
-Bin.

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

Patch

From fd67b58e3538c0732750ecad915cde736da099dc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pali=20Roh=C3=A1r?= <pali.rohar@gmail.com>
Date: Sat, 9 Jan 2016 16:57:59 +0100
Subject: [PATCH] musb: debugfs: Add support in testmode for forcing host mode
 together with speed

---
 drivers/usb/musb/musb_debugfs.c |   44 +++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c
index 9b22d94..62c13a2 100644
--- a/drivers/usb/musb/musb_debugfs.c
+++ b/drivers/usb/musb/musb_debugfs.c
@@ -147,28 +147,34 @@  static int musb_test_mode_show(struct seq_file *s, void *unused)
 
 	test = musb_readb(musb->mregs, MUSB_TESTMODE);
 
-	if (test & MUSB_TEST_FORCE_HOST)
+	if (test & (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS))
+		seq_printf(s, "force host full-speed\n");
+
+	else if (test & (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS))
+		seq_printf(s, "force host high-speed\n");
+
+	else if (test & MUSB_TEST_FORCE_HOST)
 		seq_printf(s, "force host\n");
 
-	if (test & MUSB_TEST_FIFO_ACCESS)
+	else if (test & MUSB_TEST_FIFO_ACCESS)
 		seq_printf(s, "fifo access\n");
 
-	if (test & MUSB_TEST_FORCE_FS)
+	else if (test & MUSB_TEST_FORCE_FS)
 		seq_printf(s, "force full-speed\n");
 
-	if (test & MUSB_TEST_FORCE_HS)
+	else if (test & MUSB_TEST_FORCE_HS)
 		seq_printf(s, "force high-speed\n");
 
-	if (test & MUSB_TEST_PACKET)
+	else if (test & MUSB_TEST_PACKET)
 		seq_printf(s, "test packet\n");
 
-	if (test & MUSB_TEST_K)
+	else if (test & MUSB_TEST_K)
 		seq_printf(s, "test K\n");
 
-	if (test & MUSB_TEST_J)
+	else if (test & MUSB_TEST_J)
 		seq_printf(s, "test J\n");
 
-	if (test & MUSB_TEST_SE0_NAK)
+	else if (test & MUSB_TEST_SE0_NAK)
 		seq_printf(s, "test SE0 NAK\n");
 
 	return 0;
@@ -206,30 +212,36 @@  static ssize_t musb_test_mode_write(struct file *file,
 	if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
 		return -EFAULT;
 
-	if (strstarts(buf, "force host"))
+	if (strstarts(buf, "force host full-speed"))
+		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS;
+
+	else if (strstarts(buf, "force host high-speed"))
+		test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS;
+
+	else if (strstarts(buf, "force host"))
 		test = MUSB_TEST_FORCE_HOST;
 
-	if (strstarts(buf, "fifo access"))
+	else if (strstarts(buf, "fifo access"))
 		test = MUSB_TEST_FIFO_ACCESS;
 
-	if (strstarts(buf, "force full-speed"))
+	else if (strstarts(buf, "force full-speed"))
 		test = MUSB_TEST_FORCE_FS;
 
-	if (strstarts(buf, "force high-speed"))
+	else if (strstarts(buf, "force high-speed"))
 		test = MUSB_TEST_FORCE_HS;
 
-	if (strstarts(buf, "test packet")) {
+	else if (strstarts(buf, "test packet")) {
 		test = MUSB_TEST_PACKET;
 		musb_load_testpacket(musb);
 	}
 
-	if (strstarts(buf, "test K"))
+	else if (strstarts(buf, "test K"))
 		test = MUSB_TEST_K;
 
-	if (strstarts(buf, "test J"))
+	else if (strstarts(buf, "test J"))
 		test = MUSB_TEST_J;
 
-	if (strstarts(buf, "test SE0 NAK"))
+	else if (strstarts(buf, "test SE0 NAK"))
 		test = MUSB_TEST_SE0_NAK;
 
 	musb_writeb(musb->mregs, MUSB_TESTMODE, test);
-- 
1.7.9.5