[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[Michael Trimarchi]

This patch fix the omap access to the transceiver
configuration registers using the ulpi bus. As reported by
the documentation the bit31 is used only to check if the
transaction is done or still running and the reading and
writing operation have different offset and have different
values. What we need to do at the end of a transaction is
leave the bus in done state. Anyway an error using the ulpi
omap register is not recoverable so any error give out the
usage of this interface.

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>
---
- changes for V3
  Fix patch subject
- changes for V2
  Fix commit message
---
 drivers/usb/ulpi/omap-ulpi-viewport.c |   40 +++++++--------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/ulpi/omap-ulpi-viewport.c b/drivers/usb/ulpi/omap-ulpi-viewport.c
index 3c1ea1a..2a42033 100644
--- a/drivers/usb/ulpi/omap-ulpi-viewport.c
+++ b/drivers/usb/ulpi/omap-ulpi-viewport.c
@@ -22,18 +22,19 @@
 #include <asm/io.h>
 #include <usb/ulpi.h>
 
-#define OMAP_ULPI_WR_OPSEL	(3 << 21)
-#define OMAP_ULPI_ACCESS	(1 << 31)
+#define OMAP_ULPI_WR_OPSEL	(2 << 22)
+#define OMAP_ULPI_RD_OPSEL	(3 << 22)
+#define OMAP_ULPI_START		(1 << 31)
 
 /*
- * Wait for the ULPI Access to complete
+ * Wait for having ulpi in done state
  */
 static int ulpi_wait(struct ulpi_viewport *ulpi_vp, u32 mask)
 {
 	int timeout = CONFIG_USB_ULPI_TIMEOUT;
 
 	while (--timeout) {
-		if ((readl(ulpi_vp->viewport_addr) & mask))
+		if (!(readl(ulpi_vp->viewport_addr) & mask))
 			return 0;
 
 		udelay(1);
@@ -43,40 +44,15 @@ static int ulpi_wait(struct ulpi_viewport *ulpi_vp, u32 mask)
 }
 
 /*
- * Wake the ULPI PHY up for communication
- *
- * returns 0 on success.
- */
-static int ulpi_wakeup(struct ulpi_viewport *ulpi_vp)
-{
-	int err;
-
-	if (readl(ulpi_vp->viewport_addr) & OMAP_ULPI_ACCESS)
-		return 0; /* already awake */
-
-	writel(OMAP_ULPI_ACCESS, ulpi_vp->viewport_addr);
-
-	err = ulpi_wait(ulpi_vp, OMAP_ULPI_ACCESS);
-	if (err)
-		debug("ULPI wakeup timed out\n");
-
-	return err;
-}
-
-/*
  * Issue a ULPI read/write request
  */
 static int ulpi_request(struct ulpi_viewport *ulpi_vp, u32 value)
 {
 	int err;
 
-	err = ulpi_wakeup(ulpi_vp);
-	if (err)
-		return err;
-
 	writel(value, ulpi_vp->viewport_addr);
 
-	err = ulpi_wait(ulpi_vp, OMAP_ULPI_ACCESS);
+	err = ulpi_wait(ulpi_vp, OMAP_ULPI_START);
 	if (err)
 		debug("ULPI request timed out\n");
 
@@ -85,7 +61,7 @@ static int ulpi_request(struct ulpi_viewport *ulpi_vp, u32 value)
 
 int ulpi_write(struct ulpi_viewport *ulpi_vp, u8 *reg, u32 value)
 {
-	u32 val = ((ulpi_vp->port_num & 0xf) << 24) |
+	u32 val = (OMAP_ULPI_START | (ulpi_vp->port_num & 0xf) << 24) |
 			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);
 
 	return ulpi_request(ulpi_vp, val);
@@ -95,7 +71,7 @@ u32 ulpi_read(struct ulpi_viewport *ulpi_vp, u8 *reg)
 {
 	int err;
 	u32 val = ((ulpi_vp->port_num & 0xf) << 24) |
-			 OMAP_ULPI_WR_OPSEL | ((u32)reg << 16);
+			 OMAP_ULPI_RD_OPSEL | ((u32)reg << 16);
 
 	err = ulpi_request(ulpi_vp, val);
 	if (err)
-- 
1.7.9.5

[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[Marek Vasut]

Dear Michael Trimarchi,

> This patch fix the omap access to the transceiver
> configuration registers using the ulpi bus. As reported by
> the documentation the bit31 is used only to check if the
> transaction is done or still running and the reading and
> writing operation have different offset and have different
> values. What we need to do at the end of a transaction is
> leave the bus in done state. Anyway an error using the ulpi
> omap register is not recoverable so any error give out the
> usage of this interface.
> 
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>

Tom, can you ACK/NAK this ? I have no omap board.

Best regards,
Marek Vasut

[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[Michael Trimarchi]

Dear Marek

On 06/09/2013 10:05 PM, Marek Vasut wrote:
> Dear Michael Trimarchi,
> 
>> This patch fix the omap access to the transceiver
>> configuration registers using the ulpi bus. As reported by
>> the documentation the bit31 is used only to check if the
>> transaction is done or still running and the reading and
>> writing operation have different offset and have different
>> values. What we need to do at the end of a transaction is
>> leave the bus in done state. Anyway an error using the ulpi
>> omap register is not recoverable so any error give out the
>> usage of this interface.
>>
>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>> Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>
> 
> Tom, can you ACK/NAK this ? I have no omap board.
> 

I don't understand the point, the old code was wrong and you can
check omap3/omap4 documentation. If you revert it you still have a wrong
code so it's better to drop omap3/4 viewport. 

You can take a look at this patch

http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=2a18e1248588c326f0a63c5bce4a611d709130a8

that is used to fix this errata

http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=a0dd0ee69578e32f1469596b8fd3a6c8ef172d42

I'm using this ulpi code in one of our device. I have fixed the u-boot viewport code because
I have seen it wrong. Sorry for the late response but I was busy for a Wedding ;), I can try
to test it tomorrow on an omap3 device but I think that is more easy for Stefano because he
has already a platform with a recent uboot

> Best regards,
> Marek Vasut
> 

Michael

[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[Marek Vasut]

Dear Michael Trimarchi,

> Dear Marek
> 
> On 06/09/2013 10:05 PM, Marek Vasut wrote:
> > Dear Michael Trimarchi,
> > 
> >> This patch fix the omap access to the transceiver
> >> configuration registers using the ulpi bus. As reported by
> >> the documentation the bit31 is used only to check if the
> >> transaction is done or still running and the reading and
> >> writing operation have different offset and have different
> >> values. What we need to do at the end of a transaction is
> >> leave the bus in done state. Anyway an error using the ulpi
> >> omap register is not recoverable so any error give out the
> >> usage of this interface.
> >> 
> >> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> >> Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>
> > 
> > Tom, can you ACK/NAK this ? I have no omap board.
> 
> I don't understand the point, the old code was wrong and you can
> check omap3/omap4 documentation. If you revert it you still have a wrong
> code so it's better to drop omap3/4 viewport.
> 
> You can take a look at this patch
> 
> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=2a18e1248588c326f
> 0a63c5bce4a611d709130a8
> 
> that is used to fix this errata
> 
> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=a0dd0ee69578e32f1
> 469596b8fd3a6c8ef172d42
> 
> I'm using this ulpi code in one of our device. I have fixed the u-boot
> viewport code because I have seen it wrong. Sorry for the late response
> but I was busy for a Wedding ;)

Fear not! I'm busy having no life, it's really hard task! I will actually be 
busy with that until sometimes mid-next-week.

> I can try to test it tomorrow on an omap3
> device but I think that is more easy for Stefano because he has already a
> platform with a recent uboot

I don't care who tests it, I'd just like to make sure it's tested on more 
devices than one ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[Michael Trimarchi]

Hi Marek

On 06/09/2013 11:09 PM, Marek Vasut wrote:
> Dear Michael Trimarchi,
> 
>> Dear Marek
>>
>> On 06/09/2013 10:05 PM, Marek Vasut wrote:
>>> Dear Michael Trimarchi,
>>>
>>>> This patch fix the omap access to the transceiver
>>>> configuration registers using the ulpi bus. As reported by
>>>> the documentation the bit31 is used only to check if the
>>>> transaction is done or still running and the reading and
>>>> writing operation have different offset and have different
>>>> values. What we need to do at the end of a transaction is
>>>> leave the bus in done state. Anyway an error using the ulpi
>>>> omap register is not recoverable so any error give out the
>>>> usage of this interface.
>>>>
>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>>>> Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>
>>>
>>> Tom, can you ACK/NAK this ? I have no omap board.
>>
>> I don't understand the point, the old code was wrong and you can
>> check omap3/omap4 documentation. If you revert it you still have a wrong
>> code so it's better to drop omap3/4 viewport.
>>
>> You can take a look at this patch
>>
>> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=2a18e1248588c326f
>> 0a63c5bce4a611d709130a8
>>
>> that is used to fix this errata
>>
>> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=a0dd0ee69578e32f1
>> 469596b8fd3a6c8ef172d42
>>
>> I'm using this ulpi code in one of our device. I have fixed the u-boot
>> viewport code because I have seen it wrong. Sorry for the late response
>> but I was busy for a Wedding ;)
> 
> Fear not! I'm busy having no life, it's really hard task! I will actually be 
> busy with that until sometimes mid-next-week.
> 
>> I can try to test it tomorrow on an omap3
>> device but I think that is more easy for Stefano because he has already a
>> platform with a recent uboot
> 
> I don't care who tests it, I'd just like to make sure it's tested on more 
> devices than one ;-)

I think that I have already understand the problem. 
The port_num is used starting from 0 in omap-ehci, so if this is correct my patch need a fix

from

u32 val = (OMAP_ULPI_START | (ulpi_vp->port_num & 0xf) << 24) |
 			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);

to

u32 val = (OMAP_ULPI_START | ((ulpi_vp->port_num + 1) & 0xf) << 24) |
 			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);

Michael

> 
> Best regards,
> Marek Vasut
> 

[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[Marek Vasut]

Dear Michael Trimarchi,

> Hi Marek
> 
> On 06/09/2013 11:09 PM, Marek Vasut wrote:
> > Dear Michael Trimarchi,
> > 
> >> Dear Marek
> >> 
> >> On 06/09/2013 10:05 PM, Marek Vasut wrote:
> >>> Dear Michael Trimarchi,
> >>> 
> >>>> This patch fix the omap access to the transceiver
> >>>> configuration registers using the ulpi bus. As reported by
> >>>> the documentation the bit31 is used only to check if the
> >>>> transaction is done or still running and the reading and
> >>>> writing operation have different offset and have different
> >>>> values. What we need to do at the end of a transaction is
> >>>> leave the bus in done state. Anyway an error using the ulpi
> >>>> omap register is not recoverable so any error give out the
> >>>> usage of this interface.
> >>>> 
> >>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> >>>> Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>
> >>> 
> >>> Tom, can you ACK/NAK this ? I have no omap board.
> >> 
> >> I don't understand the point, the old code was wrong and you can
> >> check omap3/omap4 documentation. If you revert it you still have a wrong
> >> code so it's better to drop omap3/4 viewport.
> >> 
> >> You can take a look at this patch
> >> 
> >> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=2a18e1248588c3
> >> 26f 0a63c5bce4a611d709130a8
> >> 
> >> that is used to fix this errata
> >> 
> >> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=a0dd0ee69578e3
> >> 2f1 469596b8fd3a6c8ef172d42
> >> 
> >> I'm using this ulpi code in one of our device. I have fixed the u-boot
> >> viewport code because I have seen it wrong. Sorry for the late response
> >> but I was busy for a Wedding ;)
> > 
> > Fear not! I'm busy having no life, it's really hard task! I will actually
> > be busy with that until sometimes mid-next-week.
> > 
> >> I can try to test it tomorrow on an omap3
> >> device but I think that is more easy for Stefano because he has already
> >> a platform with a recent uboot
> > 
> > I don't care who tests it, I'd just like to make sure it's tested on more
> > devices than one ;-)
> 
> I think that I have already understand the problem.
> The port_num is used starting from 0 in omap-ehci, so if this is correct my
> patch need a fix
> 
> from
> 
> u32 val = (OMAP_ULPI_START | (ulpi_vp->port_num & 0xf) << 24) |
>  			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);
> 
> to
> 
> u32 val = (OMAP_ULPI_START | ((ulpi_vp->port_num + 1) & 0xf) << 24) |
>  			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);
> 
> Michael

Make sure you base this stuff on u-boot-usb/master too.

Best regards,
Marek Vasut

[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[Igor Grinberg]

On 06/10/13 00:49, Marek Vasut wrote:
> Dear Michael Trimarchi,
> 
>> Hi Marek
>>
>> On 06/09/2013 11:09 PM, Marek Vasut wrote:
>>> Dear Michael Trimarchi,
>>>
>>>> Dear Marek
>>>>
>>>> On 06/09/2013 10:05 PM, Marek Vasut wrote:
>>>>> Dear Michael Trimarchi,
>>>>>
>>>>>> This patch fix the omap access to the transceiver
>>>>>> configuration registers using the ulpi bus. As reported by
>>>>>> the documentation the bit31 is used only to check if the
>>>>>> transaction is done or still running and the reading and
>>>>>> writing operation have different offset and have different
>>>>>> values. What we need to do at the end of a transaction is
>>>>>> leave the bus in done state. Anyway an error using the ulpi
>>>>>> omap register is not recoverable so any error give out the
>>>>>> usage of this interface.
>>>>>>
>>>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>>>>>> Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>
>>>>>
>>>>> Tom, can you ACK/NAK this ? I have no omap board.
>>>>
>>>> I don't understand the point, the old code was wrong and you can
>>>> check omap3/omap4 documentation. If you revert it you still have a wrong
>>>> code so it's better to drop omap3/4 viewport.
>>>>
>>>> You can take a look at this patch
>>>>
>>>> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=2a18e1248588c3
>>>> 26f 0a63c5bce4a611d709130a8
>>>>
>>>> that is used to fix this errata
>>>>
>>>> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=a0dd0ee69578e3
>>>> 2f1 469596b8fd3a6c8ef172d42
>>>>
>>>> I'm using this ulpi code in one of our device. I have fixed the u-boot
>>>> viewport code because I have seen it wrong. Sorry for the late response
>>>> but I was busy for a Wedding ;)
>>>
>>> Fear not! I'm busy having no life, it's really hard task! I will actually
>>> be busy with that until sometimes mid-next-week.
>>>
>>>> I can try to test it tomorrow on an omap3
>>>> device but I think that is more easy for Stefano because he has already
>>>> a platform with a recent uboot
>>>
>>> I don't care who tests it, I'd just like to make sure it's tested on more
>>> devices than one ;-)
>>
>> I think that I have already understand the problem.
>> The port_num is used starting from 0 in omap-ehci, so if this is correct my
>> patch need a fix
>>
>> from
>>
>> u32 val = (OMAP_ULPI_START | (ulpi_vp->port_num & 0xf) << 24) |
>>  			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);
>>
>> to
>>
>> u32 val = (OMAP_ULPI_START | ((ulpi_vp->port_num + 1) & 0xf) << 24) |
>>  			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);
>>
>> Michael
> 
> Make sure you base this stuff on u-boot-usb/master too.

I can see the v3 of this patch is already in Tom's repo and he has sent a
pull request with it.
I think currently it would be better to just do an incremental patch fixing
the port number to avoid any collisions, no? Tom?


-- 
Regards,
Igor.

[U-Boot] [PATCH v3] usb: omap: ulpi: fix ulpi transceiver access

[Michael Trimarchi]

Hi

On 06/10/2013 03:31 PM, Igor Grinberg wrote:
> On 06/10/13 00:49, Marek Vasut wrote:
>> Dear Michael Trimarchi,
>>
>>> Hi Marek
>>>
>>> On 06/09/2013 11:09 PM, Marek Vasut wrote:
>>>> Dear Michael Trimarchi,
>>>>
>>>>> Dear Marek
>>>>>
>>>>> On 06/09/2013 10:05 PM, Marek Vasut wrote:
>>>>>> Dear Michael Trimarchi,
>>>>>>
>>>>>>> This patch fix the omap access to the transceiver
>>>>>>> configuration registers using the ulpi bus. As reported by
>>>>>>> the documentation the bit31 is used only to check if the
>>>>>>> transaction is done or still running and the reading and
>>>>>>> writing operation have different offset and have different
>>>>>>> values. What we need to do at the end of a transaction is
>>>>>>> leave the bus in done state. Anyway an error using the ulpi
>>>>>>> omap register is not recoverable so any error give out the
>>>>>>> usage of this interface.
>>>>>>>
>>>>>>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>>>>>>> Reviewed-by: Igor Grinberg <grinberg@compulab.co.il>
>>>>>>
>>>>>> Tom, can you ACK/NAK this ? I have no omap board.
>>>>>
>>>>> I don't understand the point, the old code was wrong and you can
>>>>> check omap3/omap4 documentation. If you revert it you still have a wrong
>>>>> code so it's better to drop omap3/4 viewport.
>>>>>
>>>>> You can take a look at this patch
>>>>>
>>>>> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=2a18e1248588c3
>>>>> 26f 0a63c5bce4a611d709130a8
>>>>>
>>>>> that is used to fix this errata
>>>>>
>>>>> http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=a0dd0ee69578e3
>>>>> 2f1 469596b8fd3a6c8ef172d42
>>>>>
>>>>> I'm using this ulpi code in one of our device. I have fixed the u-boot
>>>>> viewport code because I have seen it wrong. Sorry for the late response
>>>>> but I was busy for a Wedding ;)
>>>>
>>>> Fear not! I'm busy having no life, it's really hard task! I will actually
>>>> be busy with that until sometimes mid-next-week.
>>>>
>>>>> I can try to test it tomorrow on an omap3
>>>>> device but I think that is more easy for Stefano because he has already
>>>>> a platform with a recent uboot
>>>>
>>>> I don't care who tests it, I'd just like to make sure it's tested on more
>>>> devices than one ;-)
>>>
>>> I think that I have already understand the problem.
>>> The port_num is used starting from 0 in omap-ehci, so if this is correct my
>>> patch need a fix
>>>
>>> from
>>>
>>> u32 val = (OMAP_ULPI_START | (ulpi_vp->port_num & 0xf) << 24) |
>>>  			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);
>>>
>>> to
>>>
>>> u32 val = (OMAP_ULPI_START | ((ulpi_vp->port_num + 1) & 0xf) << 24) |
>>>  			OMAP_ULPI_WR_OPSEL | ((u32)reg << 16) | (value & 0xff);
>>>
>>> Michael
>>
>> Make sure you base this stuff on u-boot-usb/master too.
> 
> I can see the v3 of this patch is already in Tom's repo and he has sent a
> pull request with it.
> I think currently it would be better to just do an incremental patch fixing
> the port number to avoid any collisions, no? Tom?
> 
> 
Ok, I will clone Tom's repo and prepare a patch

Michael