otg/ulpi.c : fix register write

PATCH Superseded Eric Bénard 3 participants 11 messages Jun 23, 2010 – Jul 21, 2010 USB

Share & tools

Eric Bénard

author

2010-06-23 16:43 UTC #0 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write to the PHY's registers, which means we can only set bits in the PHY's register and not clear them.
By directly using the address of the register without any offset, we now get the expected behaviour for these functions.

Signed-off-by Eric B?nard <[email protected]>

Cc Daniel Mack <[email protected]>

Cc linux-usb at vger.kernel.org

Cc linux-arm-kernel at lists.infradead.org

Cc Sascha Hauer <[email protected]>

---
v2 :
drop reg/val swap which is handled in a better way by Igor's patch here :
http://comments.gmane.org/gmane.linux.usb.general/32092

 drivers/usb/otg/ulpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/usb/otg/ulpi.c +2 -2

index b1b3469..086853b 100644
@@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
 		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 static int ulpi_init(struct otg_transceiver *otg)

@@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
 	}
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 struct otg_transceiver *

Signature · 1 line

1.6.3.3

Igor Grinberg

2010-06-24 14:04 UTC #1 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

Quoted 24 lines · Eric B?nard

ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write to the PHY's registers, which means we can only set bits in the PHY's register and not clear them.
By directly using the address of the register without any offset, we now get the expected behaviour for these functions.
Signed-off-by Eric B?nard <[email protected]>
Cc Daniel Mack <[email protected]>
Cc linux-usb at vger.kernel.org
Cc linux-arm-kernel at lists.infradead.org
Cc Sascha Hauer <[email protected]>
---
v2 :
drop reg/val swap which is handled in a better way by Igor's patch here :
http://comments.gmane.org/gmane.linux.usb.general/32092
 drivers/usb/otg/ulpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/usb/otg/ulpi.c +0 -0
index b1b3469..086853b 100644

drivers/usb/otg/ulpi.c static int ulpi_set_flags(struct otg_transceiver *otg) · line 59

Quoted 7 lines

drivers/usb/otg/ulpi.c +1 -1

@@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
 		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }

ulpi_set_flags is used to set the OTG flags and not to clear them, also it is a static function and is not a part of struct otg_transceiver, so it cannot be called from outside the ulpi.c, thus after the ulpi_create, it is never called.
I think, currently, this should stay as it is.

drivers/usb/otg/ulpi.c static int ulpi_set_vbus(struct otg_transceiver *otg, bool on) · line 95

Quoted 9 lines

drivers/usb/otg/ulpi.c +1 -1

 static int ulpi_init(struct otg_transceiver *otg)

@@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
 	}
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }

This one seems ok.

Quoted 3 lines

drivers/usb/otg/ulpi.c +0 -0
 struct otg_transceiver *

Signature · 2 lines

Regards,
Igor.

Eric Bénard

author

2010-06-24 14:37 UTC #2 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

Le 24/06/2010 16:04, Igor Grinberg a ?crit :

Quoted 6 lines

ulpi_set_flags is used to set the OTG flags and not to clear them, also it is a static function and is not a part of struct otg_transceiver, so it cannot be called from outside the ulpi.c, thus after the ulpi_create, it is never called.
I think, currently, this should stay as it is.

OK, the problem that when one flag is set, you can't clear it without resetting the PHY.

Eric

Igor Grinberg

2010-06-27 07:42 UTC #3 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

Quoted 11 lines · Eric B?nard

Le 24/06/2010 16:04, Igor Grinberg a ?crit :
Quoted 8 lines
ulpi_set_flags is used to set the OTG flags and not to clear them, also it is a static function and is not a part of struct otg_transceiver,
so it cannot be called from outside the ulpi.c, thus after the ulpi_create,
it is never called.
I think, currently, this should stay as it is.
OK, the problem that when one flag is set, you can't clear it without resetting the PHY.

This is correct, but current ulpi driver is not designed for writing multiple times
into the ULPI_OTG_CTRL register, only once when ulpi_init function is called.
Do you call the ulpi_init function for a couple of times? Looks like a hack.

Quoted 3 lines

Eric

Signature · 2 lines

Regards,
Igor.

Eric Bénard

author

2010-06-27 14:00 UTC #4 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

Le 27/06/2010 09:42, Igor Grinberg a ?crit :

Quoted 11 lines · Eric B?nard

Quoted 9 lines
Le 24/06/2010 16:04, Igor Grinberg a ?crit :
Quoted 8 lines
ulpi_set_flags is used to set the OTG flags and not to clear them, also it is a static function and is not a part of struct otg_transceiver,
so it cannot be called from outside the ulpi.c, thus after the ulpi_create,
it is never called.
I think, currently, this should stay as it is.

Quoted 9 lines

Quoted 2 lines
OK, the problem that when one flag is set, you can't clear it without resetting the PHY.
This is correct, but current ulpi driver is not designed for writing multiple times
into the ULPI_OTG_CTRL register, only once when ulpi_init function is called.
Do you call the ulpi_init function for a couple of times? Looks like a hack.

no hack, as I was fixing the other function, I though this one could also be fixed :)
I'll send a patch with only the other fix.

Eric

Igor Grinberg

2010-07-06 10:31 UTC #5 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

Quoted 22 lines · Eric B?nard

Le 27/06/2010 09:42, Igor Grinberg a ?crit :
Quoted 14 lines · Eric B?nard
Quoted 11 lines
Le 24/06/2010 16:04, Igor Grinberg a ?crit :
Quoted 8 lines
ulpi_set_flags is used to set the OTG flags and not to clear them, also it is a static function and is not a part of struct otg_transceiver,
so it cannot be called from outside the ulpi.c, thus after the ulpi_create,
it is never called.
I think, currently, this should stay as it is.
OK, the problem that when one flag is set, you can't clear it without resetting the PHY.
Quoted 7 lines
This is correct, but current ulpi driver is not designed for writing multiple times
into the ULPI_OTG_CTRL register, only once when ulpi_init function is called.
Do you call the ulpi_init function for a couple of times? Looks like a hack.

Quoted 6 lines

no hack, as I was fixing the other function, I though this one could also be fixed :)
I'll send a patch with only the other fix.
Eric

I've found in the ulpi specification that there are some bits in the OTG Control Register that are set by default after power-on. Current ulpi driver can only set those bits and not clear them, making it impossible for the platform to clear them even at initialization time.
So, after all v2 of your patch is correct.
Sorry, for not seeing this at first time :( , so

Acked-by Igor Grinberg <[email protected]>

Signature · 2 lines

Regards,
Igor.

Igor Grinberg

2010-07-14 14:22 UTC #6 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

In reply to Eric Bénard

Greg,

I have a patch set, I want to submit in the very near future and it relies on this one.
Will this patch get to your tree?

Quoted 42 lines

ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write to the PHY's registers, which means we can only set bits in the PHY's register and not clear them.
By directly using the address of the register without any offset, we now get the expected behaviour for these functions.
Signed-off-by Eric B?nard <[email protected]>
Cc Daniel Mack <[email protected]>
Cc linux-usb at vger.kernel.org
Cc linux-arm-kernel at lists.infradead.org
Cc Sascha Hauer <[email protected]>
---
v2 :
drop reg/val swap which is handled in a better way by Igor's patch here :
http://comments.gmane.org/gmane.linux.usb.general/32092
 drivers/usb/otg/ulpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/usb/otg/ulpi.c +2 -2
index b1b3469..086853b 100644
@@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
 		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 static int ulpi_init(struct otg_transceiver *otg)
@@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
 	}
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 struct otg_transceiver *

Signature · 2 lines

Regards,
Igor.

Greg KH

2010-07-14 20:47 UTC #7 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

Quoted 5 lines · Igor Grinberg

Greg,
I have a patch set, I want to submit in the very near future and it relies on this one.
Will this patch get to your tree?

Odd, I don't see it in my "to-apply" queue. Did you cc: me on it? Care to resend it if you want me to apply it?

thanks,

greg k-h

Igor Grinberg

2010-07-15 05:26 UTC #8 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

Quoted 13 lines · Igor Grinberg

Quoted 7 lines
Greg,
I have a patch set, I want to submit in the very near future and it relies on this one.
Will this patch get to your tree?
Odd, I don't see it in my "to-apply" queue. Did you cc: me on it? Care to resend it if you want me to apply it?

Well, you were in "To:", when Eric sent it the first time. Nevertheless, Eric does not seem to care resending it. I can send it along with my patch set (with Eric's Signed-off). Also, this patch will make more sense then.

Quoted 5 lines

thanks,
greg k-h

Signature · 2 lines

Regards,
Igor.

Eric Bénard

author

2010-07-15 07:20 UTC #9 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

In reply to Greg KH

Signed-off-by Eric B?nard <[email protected]>

Cc Daniel Mack <[email protected]>

Cc linux-usb at vger.kernel.org

Cc linux-arm-kernel at lists.infradead.org

Cc Sascha Hauer <[email protected]>

---
v2 :
drop reg/val swap which is handled in a better way by Igor's patch here :
http://comments.gmane.org/gmane.linux.usb.general/32092

 drivers/usb/otg/ulpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/usb/otg/ulpi.c +2 -2

index b1b3469..086853b 100644
@@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
 		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 static int ulpi_init(struct otg_transceiver *otg)

@@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
 	}
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 struct otg_transceiver *

Signature · 1 line

1.6.3.3

Igor Grinberg

2010-07-21 14:46 UTC #10 lore raw

PATCH v2 1/1 otg/ulpi.c : fix register write

Greg,

This was a resend, you've asked...
Can you please update us on its status?

Thanks

Quoted 42 lines

ulpi_set_vbus and ulpi_set_flags are using ULPI_SET(register) to write to the PHY's registers, which means we can only set bits in the PHY's register and not clear them.
By directly using the address of the register without any offset, we now get the expected behaviour for these functions.
Signed-off-by Eric B?nard <[email protected]>
Cc Daniel Mack <[email protected]>
Cc linux-usb at vger.kernel.org
Cc linux-arm-kernel at lists.infradead.org
Cc Sascha Hauer <[email protected]>
---
v2 :
drop reg/val swap which is handled in a better way by Igor's patch here :
http://comments.gmane.org/gmane.linux.usb.general/32092
 drivers/usb/otg/ulpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/usb/otg/ulpi.c +2 -2
index b1b3469..086853b 100644
@@ -54,7 +54,7 @@ static int ulpi_set_flags(struct otg_transceiver *otg)
 	if (otg->flags & USB_OTG_EXT_VBUS_INDICATOR)
 		flags |= ULPI_OTG_CTRL_EXTVBUSIND;
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 static int ulpi_init(struct otg_transceiver *otg)
@@ -90,7 +90,7 @@ static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
 	}
 
-	return otg_io_write(otg, flags, ULPI_SET(ULPI_OTG_CTRL));
+	return otg_io_write(otg, flags, ULPI_OTG_CTRL);
 }
 
 struct otg_transceiver *

Signature · 2 lines

Regards,
Igor.