[PATCH 0/2] USB: twl4030-usb: fix isp1707 xceiver powering

[PATCH 0/2] USB: twl4030-usb: fix isp1707 xceiver powering

[Kalle Jokiniemi]

These two patches introduce phy_power calls form board files
to twl4030-usb. This fixes a problem in Nokia N900, where the
ISP1707 serial tranceiver did not get disabled during phy
power down.

Based on patches used on n900 maemo kernel, mainly from Heikki
Krogerus. Comments&testing appreciated.

Basic test done on MeeGo + linux usb that the ISP powering
happens correctly. Also simple data transfer tests done with
backported patches on MeeGo n900 kernel.

Cross posting to linux-omap, but I propose that we'd push these
to linux-usb. Patches based on linux-usb master.

Kalle Jokiniemi (2):
  USB: twl4030-usb: do board specific phy_power up/down
  OMAP3: rx51: specify phy_power for usb tranceiver

 arch/arm/mach-omap2/board-rx51-peripherals.c |   32 ++++++++++++++++++++++++++
 drivers/usb/otg/twl4030-usb.c                |    9 ++++++-
 2 files changed, 40 insertions(+), 1 deletions(-)

[PATCH 1/2] USB: twl4030-usb: do board specific phy_power up/down

[Kalle Jokiniemi]

In case some board has special powering sequences for
the USB tranceiver, call those during __twl4030_phy_power
calls.

This is a preparation patch to allow powering down the
ISP1707 USB serial tranceiver in Nokia N900.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
---
 drivers/usb/otg/twl4030-usb.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index 6ca505f..dea99b1 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -348,13 +348,20 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
 
 static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
 {
-	u8 pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
+	u8 pwr;
+	struct twl4030_usb_data *board = twl->dev->platform_data;
+
+	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
 
 	if (on)
 		pwr &= ~PHY_PWR_PHYPWD;
 	else
 		pwr |= PHY_PWR_PHYPWD;
 
+	/* do board specific power up/down, if available */
+	if (board->phy_power)
+		board->phy_power(twl->dev, 0, on);
+
 	WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
 }
 
-- 
1.7.1

[PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[Kalle Jokiniemi]

This patch allows the ISP1707 USB tranceiver on Nokia
N900 to be disabled when usb cable is disconnected.
This saves approximately 14mA of battery current.

Patch based on work done by Heikki Krogerus on N900
maemo kernel.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
Cc: Heikki Krogerus <heikki.krogerus@nokia.com>
---
 arch/arm/mach-omap2/board-rx51-peripherals.c |   32 ++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index e75e240..9dd22bc 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -41,6 +41,7 @@
 
 #include "mux.h"
 #include "hsmmc.h"
+#include "cm2xxx_3xxx.h"
 
 #define SYSTEM_REV_B_USES_VAUX3	0x1699
 #define SYSTEM_REV_S_USES_VAUX3 0x8
@@ -48,6 +49,8 @@
 #define RX51_WL1251_POWER_GPIO		87
 #define RX51_WL1251_IRQ_GPIO		42
 
+#define RX51_USB_TRANSCEIVER_RST_GPIO	67
+
 /* list all spi devices here */
 enum {
 	RX51_SPI_WL1251,
@@ -534,8 +537,36 @@ static struct twl4030_gpio_platform_data rx51_gpio_data = {
 	.setup			= rx51_twlgpio_setup,
 };
 
+static void __init rx51_xceiv_init(void)
+{
+	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
+		BUG();
+	gpio_direction_output(RX51_USB_TRANSCEIVER_RST_GPIO, 1);
+}
+
+static int rx51_xceiv_power(struct device *dev, int iD, int on)
+{
+	unsigned long	timeout;
+
+	if (!on) {
+		/* Let musb go stdby before powering down the transceiver */
+		timeout = jiffies + msecs_to_jiffies(100);
+		while (!time_after(jiffies, timeout))
+			if (omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
+				& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK)
+				break;
+		if (!(omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
+			& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK))
+			WARN(1, "could not put musb to sleep\n");
+	}
+	gpio_set_value(RX51_USB_TRANSCEIVER_RST_GPIO, on);
+
+	return 0;
+}
+
 static struct twl4030_usb_data rx51_usb_data = {
 	.usb_mode		= T2_USB_MODE_ULPI,
+	.phy_power		= rx51_xceiv_power,
 };
 
 static struct twl4030_ins sleep_on_seq[] __initdata = {
@@ -917,6 +948,7 @@ error:
 void __init rx51_peripherals_init(void)
 {
 	rx51_i2c_init();
+	rx51_xceiv_init();
 	gpmc_onenand_init(board_onenand_data);
 	board_smc91x_init();
 	rx51_add_gpio_keys();
-- 
1.7.1

Re: [PATCH 1/2] USB: twl4030-usb: do board specific phy_power up/down

[Sergei Shtylyov]

Hello.

Kalle Jokiniemi wrote:

> In case some board has special powering sequences for
> the USB tranceiver, call those during __twl4030_phy_power
> calls.

> This is a preparation patch to allow powering down the
> ISP1707 USB serial tranceiver in Nokia N900.

> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/otg/twl4030-usb.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)

> diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
> index 6ca505f..dea99b1 100644
> --- a/drivers/usb/otg/twl4030-usb.c
> +++ b/drivers/usb/otg/twl4030-usb.c
> @@ -348,13 +348,20 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
>  
>  static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
>  {
> -	u8 pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
> +	u8 pwr;

    Why change this line? Also, some prefer that initialized variables precede 
uninitialized ones...

> +	struct twl4030_usb_data *board = twl->dev->platform_data;
> +
> +	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
>  
>  	if (on)
>  		pwr &= ~PHY_PWR_PHYPWD;
>  	else
>  		pwr |= PHY_PWR_PHYPWD;
>  
> +	/* do board specific power up/down, if available */
> +	if (board->phy_power)
> +		board->phy_power(twl->dev, 0, on);
> +
>  	WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
>  }

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[Sergei Shtylyov]

Hello.

Kalle Jokiniemi wrote:

> This patch allows the ISP1707 USB tranceiver on Nokia
> N900 to be disabled when usb cable is disconnected.
> This saves approximately 14mA of battery current.

> Patch based on work done by Heikki Krogerus on N900
> maemo kernel.

> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> Cc: Heikki Krogerus <heikki.krogerus-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/arm/mach-omap2/board-rx51-peripherals.c |   32 ++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)

> diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
> index e75e240..9dd22bc 100644
> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
[...]
> @@ -534,8 +537,36 @@ static struct twl4030_gpio_platform_data rx51_gpio_data = {
>  	.setup			= rx51_twlgpio_setup,
>  };
>  
> +static void __init rx51_xceiv_init(void)
> +{
> +	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
> +		BUG();

    Why not:

	BUG_ON(gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0);

WBR, Sergei

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

Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[Aaro Koskinen]

Hi,

On Mon, 21 Mar 2011, Sergei Shtylyov wrote:
>> This patch allows the ISP1707 USB tranceiver on Nokia
>> N900 to be disabled when usb cable is disconnected.
>> This saves approximately 14mA of battery current.
>
>> Patch based on work done by Heikki Krogerus on N900
>> maemo kernel.
>
>> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>> Cc: Heikki Krogerus <heikki.krogerus-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>> ---
>>  arch/arm/mach-omap2/board-rx51-peripherals.c |   32 
>> ++++++++++++++++++++++++++
>>  1 files changed, 32 insertions(+), 0 deletions(-)
>
>> diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c 
>> b/arch/arm/mach-omap2/board-rx51-peripherals.c
>> index e75e240..9dd22bc 100644
>> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
>> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
> [...]
>> @@ -534,8 +537,36 @@ static struct twl4030_gpio_platform_data 
>> rx51_gpio_data = {
>>  	.setup			= rx51_twlgpio_setup,
>>  };
>>  +static void __init rx51_xceiv_init(void)
>> +{
>> +	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
>> +		BUG();
>
>   Why not:
>
> 	BUG_ON(gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0);

Well, there should not be no BUG() there in the first place. If the GPIO
cannot be requested there should be an error log and phy_power should
be set to NULL.

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

Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[Jarkko Nikula]

On Mon, 21 Mar 2011 15:50:20 +0200
Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:

> This patch allows the ISP1707 USB tranceiver on Nokia
> N900 to be disabled when usb cable is disconnected.
> This saves approximately 14mA of battery current.
> 
I can measure that these two patches save about 14 mA.

> +static void __init rx51_xceiv_init(void)
> +{
> +	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
> +		BUG();
> +	gpio_direction_output(RX51_USB_TRANSCEIVER_RST_GPIO, 1);
> +}

gpio_request_one makes this function needless.

> +static int rx51_xceiv_power(struct device *dev, int iD, int on)
> +{
> +	unsigned long	timeout;
> +
> +	if (!on) {
> +		/* Let musb go stdby before powering down the transceiver */
> +		timeout = jiffies + msecs_to_jiffies(100);
> +		while (!time_after(jiffies, timeout))
> +			if (omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
> +				& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK)
> +				break;
> +		if (!(omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
> +			& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK))
> +			WARN(1, "could not put musb to sleep\n");
> +	}
> +	gpio_set_value(RX51_USB_TRANSCEIVER_RST_GPIO, on);
> +

Hmm... 100 ms busy loop. I wonder are there better ways in usb to
achieve this gpio control than this loop here and playing with these
registers? I believe musb/phy/etc has knowledge when they are idle and
can do some board specific stuff?

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

Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[Greg KH]

On Mon, Mar 21, 2011 at 04:40:26PM +0200, Aaro Koskinen wrote:
> Hi,
> 
> On Mon, 21 Mar 2011, Sergei Shtylyov wrote:
> >>This patch allows the ISP1707 USB tranceiver on Nokia
> >>N900 to be disabled when usb cable is disconnected.
> >>This saves approximately 14mA of battery current.
> >
> >>Patch based on work done by Heikki Krogerus on N900
> >>maemo kernel.
> >
> >>Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> >>Cc: Heikki Krogerus <heikki.krogerus-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> >>---
> >> arch/arm/mach-omap2/board-rx51-peripherals.c |   32
> >>++++++++++++++++++++++++++
> >> 1 files changed, 32 insertions(+), 0 deletions(-)
> >
> >>diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c
> >>b/arch/arm/mach-omap2/board-rx51-peripherals.c
> >>index e75e240..9dd22bc 100644
> >>--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
> >>+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
> >[...]
> >>@@ -534,8 +537,36 @@ static struct twl4030_gpio_platform_data
> >>rx51_gpio_data = {
> >> 	.setup			= rx51_twlgpio_setup,
> >> };
> >> +static void __init rx51_xceiv_init(void)
> >>+{
> >>+	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
> >>+		BUG();
> >
> >  Why not:
> >
> >	BUG_ON(gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0);
> 
> Well, there should not be no BUG() there in the first place. If the GPIO
> cannot be requested there should be an error log and phy_power should
> be set to NULL.

Yes, NEVER add a BUG() call in a driver, you just crashed the whole
machine, which is not nice at all to anyone involved.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[Felipe Balbi]

On Mon, Mar 21, 2011 at 09:21:48AM -0700, Greg KH wrote:
> On Mon, Mar 21, 2011 at 04:40:26PM +0200, Aaro Koskinen wrote:
> > Hi,
> > 
> > On Mon, 21 Mar 2011, Sergei Shtylyov wrote:
> > >>This patch allows the ISP1707 USB tranceiver on Nokia
> > >>N900 to be disabled when usb cable is disconnected.
> > >>This saves approximately 14mA of battery current.
> > >
> > >>Patch based on work done by Heikki Krogerus on N900
> > >>maemo kernel.
> > >
> > >>Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > >>Cc: Heikki Krogerus <heikki.krogerus-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > >>---
> > >> arch/arm/mach-omap2/board-rx51-peripherals.c |   32
> > >>++++++++++++++++++++++++++
> > >> 1 files changed, 32 insertions(+), 0 deletions(-)
> > >
> > >>diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c
> > >>b/arch/arm/mach-omap2/board-rx51-peripherals.c
> > >>index e75e240..9dd22bc 100644
> > >>--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
> > >>+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
> > >[...]
> > >>@@ -534,8 +537,36 @@ static struct twl4030_gpio_platform_data
> > >>rx51_gpio_data = {
> > >> 	.setup			= rx51_twlgpio_setup,
> > >> };
> > >> +static void __init rx51_xceiv_init(void)
> > >>+{
> > >>+	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
> > >>+		BUG();
> > >
> > >  Why not:
> > >
> > >	BUG_ON(gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0);
> > 
> > Well, there should not be no BUG() there in the first place. If the GPIO
> > cannot be requested there should be an error log and phy_power should
> > be set to NULL.
> 
> Yes, NEVER add a BUG() call in a driver, you just crashed the whole
> machine, which is not nice at all to anyone involved.

Yeah, add a WARN(), though to make it a big fat error report.

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

Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[Heikki Krogerus]

Hi,

On Mon, Mar 21, 2011 at 03:50:20PM +0200, Kalle Jokiniemi wrote:
> This patch allows the ISP1707 USB tranceiver on Nokia
> N900 to be disabled when usb cable is disconnected.
> This saves approximately 14mA of battery current.
> 
> Patch based on work done by Heikki Krogerus on N900
> maemo kernel.
> 
> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
> Cc: Heikki Krogerus <heikki.krogerus@nokia.com>
> ---
>  arch/arm/mach-omap2/board-rx51-peripherals.c |   32 ++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)

<snip>

> +static void __init rx51_xceiv_init(void)
> +{
> +	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
> +		BUG();
> +	gpio_direction_output(RX51_USB_TRANSCEIVER_RST_GPIO, 1);
> +}
> +
> +static int rx51_xceiv_power(struct device *dev, int iD, int on)
> +{
> +	unsigned long	timeout;
> +
> +	if (!on) {
> +		/* Let musb go stdby before powering down the transceiver */
> +		timeout = jiffies + msecs_to_jiffies(100);
> +		while (!time_after(jiffies, timeout))
> +			if (omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
> +				& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK)
> +				break;
> +		if (!(omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
> +			& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK))
> +			WARN(1, "could not put musb to sleep\n");
> +	}
> +	gpio_set_value(RX51_USB_TRANSCEIVER_RST_GPIO, on);
> +
> +	return 0;
> +}

The busy loop is not needed, and not what we want. We need to be able
to toggle the CHIP_SEL even if the USB block is not IDLE or STDBY.

The guys have already pointed out some issues with this code, so
please rework these functions.

>  static struct twl4030_usb_data rx51_usb_data = {
>  	.usb_mode		= T2_USB_MODE_ULPI,
> +	.phy_power		= rx51_xceiv_power,
>  };

This is not the right place for this. The gpio controls isp1707, not
the twl4030_usb. You have the rx51_charger_device platform device for
isp1707. Add them there.

-- 
heikki

Re: [PATCH 1/2] USB: twl4030-usb: do board specific phy_power up/down

[Heikki Krogerus]

Hi,

On Mon, Mar 21, 2011 at 03:50:19PM +0200, Kalle Jokiniemi wrote:
> In case some board has special powering sequences for
> the USB tranceiver, call those during __twl4030_phy_power
> calls.
> 
> This is a preparation patch to allow powering down the
> ISP1707 USB serial tranceiver in Nokia N900.
> 
> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
> ---
>  drivers/usb/otg/twl4030-usb.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
> index 6ca505f..dea99b1 100644
> --- a/drivers/usb/otg/twl4030-usb.c
> +++ b/drivers/usb/otg/twl4030-usb.c
> @@ -348,13 +348,20 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
>  
>  static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
>  {
> -	u8 pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
> +	u8 pwr;
> +	struct twl4030_usb_data *board = twl->dev->platform_data;
> +
> +	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
>  
>  	if (on)
>  		pwr &= ~PHY_PWR_PHYPWD;
>  	else
>  		pwr |= PHY_PWR_PHYPWD;
>  
> +	/* do board specific power up/down, if available */
> +	if (board->phy_power)
> +		board->phy_power(twl->dev, 0, on);
> +
>  	WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
>  }

Wrong driver. We are controlling isp1707. The driver is
drivers/power/isp1704_charger.c.

-- 
heikki

Re: [PATCH 0/2] USB: twl4030-usb: fix isp1707 xceiver powering

[Heikki Krogerus]

Hi Kalle,

Missed the cover letter so I already commented the patches.

On Mon, Mar 21, 2011 at 03:50:18PM +0200, Kalle Jokiniemi wrote:
> These two patches introduce phy_power calls form board files
> to twl4030-usb. This fixes a problem in Nokia N900, where the
> ISP1707 serial tranceiver did not get disabled during phy
> power down.
> 
> Based on patches used on n900 maemo kernel, mainly from Heikki
> Krogerus. Comments&testing appreciated.
> 
> Basic test done on MeeGo + linux usb that the ISP powering
> happens correctly. Also simple data transfer tests done with
> backported patches on MeeGo n900 kernel.
> 
> Cross posting to linux-omap, but I propose that we'd push these
> to linux-usb. Patches based on linux-usb master.
> 
> Kalle Jokiniemi (2):
>   USB: twl4030-usb: do board specific phy_power up/down
>   OMAP3: rx51: specify phy_power for usb tranceiver
> 
>  arch/arm/mach-omap2/board-rx51-peripherals.c |   32 ++++++++++++++++++++++++++
>  drivers/usb/otg/twl4030-usb.c                |    9 ++++++-
>  2 files changed, 40 insertions(+), 1 deletions(-)

I do like the idea of getting the support for power down mode of
isp1707 in RX51 but...

If I understood correctly, meego has the isp1704_charger driver from
mainline, so we don't need to involve twl4030-usb when powering the
isp1707 like we did in maemo kernel.

-- 
heikki

RE: [PATCH 1/2] USB: twl4030-usb: do board specific phy_power up/down

[kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w]

Hi,

 > -----Original Message-----
 > From: ext Sergei Shtylyov [mailto:sshtylyov-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org]
 > Sent: 21. maaliskuuta 2011 16:21
 > To: Jokiniemi Kalle (Nokia-MS/Tampere)
 > Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; balbi-l0cyMroinI0@public.gmane.org;
 > tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org; Krogerus Heikki (Nokia-MS/Helsinki);
 > jhnikula-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; Koskinen Ilkka (Nokia-MS/Tampere)
 > Subject: Re: [PATCH 1/2] USB: twl4030-usb: do board specific phy_power
 > up/down
 > 
 > Hello.
 > 
 > Kalle Jokiniemi wrote:
 > 
 > > In case some board has special powering sequences for
 > > the USB tranceiver, call those during __twl4030_phy_power
 > > calls.
 > 
 > > This is a preparation patch to allow powering down the
 > > ISP1707 USB serial tranceiver in Nokia N900.
 > 
 > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
 > > ---
 > >  drivers/usb/otg/twl4030-usb.c |    9 ++++++++-
 > >  1 files changed, 8 insertions(+), 1 deletions(-)
 > 
 > > diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
 > > index 6ca505f..dea99b1 100644
 > > --- a/drivers/usb/otg/twl4030-usb.c
 > > +++ b/drivers/usb/otg/twl4030-usb.c
 > > @@ -348,13 +348,20 @@ static void twl4030_i2c_access(struct twl4030_usb
 > *twl, int on)
 > >
 > >  static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
 > >  {
 > > -	u8 pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
 > > +	u8 pwr;
 > 
 >     Why change this line? Also, some prefer that initialized variables precede
 > uninitialized ones...

One likes the apple, one likes the orange. I'll change that back, no problem.

- Kalle

 > 
 > > +	struct twl4030_usb_data *board = twl->dev->platform_data;
 > > +
 > > +	pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
 > >
 > >  	if (on)
 > >  		pwr &= ~PHY_PWR_PHYPWD;
 > >  	else
 > >  		pwr |= PHY_PWR_PHYPWD;
 > >
 > > +	/* do board specific power up/down, if available */
 > > +	if (board->phy_power)
 > > +		board->phy_power(twl->dev, 0, on);
 > > +
 > >  	WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0);
 > >  }
 > 
 > WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w]

Hi,

 > -----Original Message-----
 > From: linux-omap-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-omap-
 > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of ext Sergei Shtylyov
 > Sent: 21. maaliskuuta 2011 16:29
 > To: Jokiniemi Kalle (Nokia-MS/Tampere)
 > Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; balbi-l0cyMroinI0@public.gmane.org;
 > tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org; Krogerus Heikki (Nokia-MS/Helsinki);
 > jhnikula-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; Koskinen Ilkka (Nokia-MS/Tampere)
 > Subject: Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver
 > 
 > Hello.
 > 
 > Kalle Jokiniemi wrote:
 > 
 > > This patch allows the ISP1707 USB tranceiver on Nokia
 > > N900 to be disabled when usb cable is disconnected.
 > > This saves approximately 14mA of battery current.
 > 
 > > Patch based on work done by Heikki Krogerus on N900
 > > maemo kernel.
 > 
 > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
 > > Cc: Heikki Krogerus <heikki.krogerus-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
 > > ---
 > >  arch/arm/mach-omap2/board-rx51-peripherals.c |   32
 > ++++++++++++++++++++++++++
 > >  1 files changed, 32 insertions(+), 0 deletions(-)
 > 
 > > diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c
 > b/arch/arm/mach-omap2/board-rx51-peripherals.c
 > > index e75e240..9dd22bc 100644
 > > --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
 > > +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
 > [...]
 > > @@ -534,8 +537,36 @@ static struct twl4030_gpio_platform_data
 > rx51_gpio_data = {
 > >  	.setup			= rx51_twlgpio_setup,
 > >  };
 > >
 > > +static void __init rx51_xceiv_init(void)
 > > +{
 > > +	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
 > > +		BUG();
 > 
 >     Why not:
 > 
 > 	BUG_ON(gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) <
 > 0);

Makes sense, will change that. Thanks for the comments.

- Kalle

 > 
 > WBR, Sergei
 > 
 > --
 > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
 > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/2] USB: twl4030-usb: fix isp1707 xceiver powering

[kalle.jokiniemi]

Hi,

 > -----Original Message-----
 > From: Heikki Krogerus [mailto:heikki.krogerus@nokia.com]
 > Sent: 22. maaliskuuta 2011 11:46
 > To: Jokiniemi Kalle (Nokia-MS/Tampere)
 > Cc: linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; balbi@ti.com;
 > tony@atomide.com; jhnikula@gmail.com; Koskinen Ilkka (Nokia-MS/Tampere)
 > Subject: Re: [PATCH 0/2] USB: twl4030-usb: fix isp1707 xceiver powering
 > 
 > Hi Kalle,
 > 
 > Missed the cover letter so I already commented the patches.
 > 
 > On Mon, Mar 21, 2011 at 03:50:18PM +0200, Kalle Jokiniemi wrote:
 > > These two patches introduce phy_power calls form board files
 > > to twl4030-usb. This fixes a problem in Nokia N900, where the
 > > ISP1707 serial tranceiver did not get disabled during phy
 > > power down.
 > >
 > > Based on patches used on n900 maemo kernel, mainly from Heikki
 > > Krogerus. Comments&testing appreciated.
 > >
 > > Basic test done on MeeGo + linux usb that the ISP powering
 > > happens correctly. Also simple data transfer tests done with
 > > backported patches on MeeGo n900 kernel.
 > >
 > > Cross posting to linux-omap, but I propose that we'd push these
 > > to linux-usb. Patches based on linux-usb master.
 > >
 > > Kalle Jokiniemi (2):
 > >   USB: twl4030-usb: do board specific phy_power up/down
 > >   OMAP3: rx51: specify phy_power for usb tranceiver
 > >
 > >  arch/arm/mach-omap2/board-rx51-peripherals.c |   32
 > ++++++++++++++++++++++++++
 > >  drivers/usb/otg/twl4030-usb.c                |    9 ++++++-
 > >  2 files changed, 40 insertions(+), 1 deletions(-)
 > 
 > I do like the idea of getting the support for power down mode of
 > isp1707 in RX51 but...
 > 
 > If I understood correctly, meego has the isp1704_charger driver from
 > mainline, so we don't need to involve twl4030-usb when powering the
 > isp1707 like we did in maemo kernel.

Thanks for all the comments. I'll rework the driver to use the isp charger driver instead.

- Kalle



 > 
 > --
 > heikki

RE: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[kalle.jokiniemi]

Hi,

 > -----Original Message-----
 > From: Heikki Krogerus [mailto:heikki.krogerus@nokia.com]
 > Sent: 22. maaliskuuta 2011 11:13
 > To: Jokiniemi Kalle (Nokia-MS/Tampere)
 > Cc: linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; balbi@ti.com;
 > tony@atomide.com; jhnikula@gmail.com; Koskinen Ilkka (Nokia-MS/Tampere)
 > Subject: Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver
 > 
 > Hi,
 > 
 > On Mon, Mar 21, 2011 at 03:50:20PM +0200, Kalle Jokiniemi wrote:
 > > This patch allows the ISP1707 USB tranceiver on Nokia
 > > N900 to be disabled when usb cable is disconnected.
 > > This saves approximately 14mA of battery current.
 > >
 > > Patch based on work done by Heikki Krogerus on N900
 > > maemo kernel.
 > >
 > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
 > > Cc: Heikki Krogerus <heikki.krogerus@nokia.com>
 > > ---
 > >  arch/arm/mach-omap2/board-rx51-peripherals.c |   32
 > ++++++++++++++++++++++++++
 > >  1 files changed, 32 insertions(+), 0 deletions(-)
 > 
 > <snip>
 > 
 > > +static void __init rx51_xceiv_init(void)
 > > +{
 > > +	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
 > > +		BUG();
 > > +	gpio_direction_output(RX51_USB_TRANSCEIVER_RST_GPIO, 1);
 > > +}
 > > +
 > > +static int rx51_xceiv_power(struct device *dev, int iD, int on)
 > > +{
 > > +	unsigned long	timeout;
 > > +
 > > +	if (!on) {
 > > +		/* Let musb go stdby before powering down the transceiver */
 > > +		timeout = jiffies + msecs_to_jiffies(100);
 > > +		while (!time_after(jiffies, timeout))
 > > +			if (omap2_cm_read_mod_reg(CORE_MOD,
 > CM_IDLEST1)
 > > +				&
 > OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK)
 > > +				break;
 > > +		if (!(omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
 > > +			& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK))
 > > +			WARN(1, "could not put musb to sleep\n");
 > > +	}
 > > +	gpio_set_value(RX51_USB_TRANSCEIVER_RST_GPIO, on);
 > > +
 > > +	return 0;
 > > +}
 > 
 > The busy loop is not needed, and not what we want. We need to be able
 > to toggle the CHIP_SEL even if the USB block is not IDLE or STDBY.

I basically just took what was in the maemo kernel. Was there some reason originally to include the busyloop? Do I get it now correctly that the ISP is only needed active when we are charging?

 > 
 > The guys have already pointed out some issues with this code, so
 > please rework these functions.

Will do.

 > 
 > >  static struct twl4030_usb_data rx51_usb_data = {
 > >  	.usb_mode		= T2_USB_MODE_ULPI,
 > > +	.phy_power		= rx51_xceiv_power,
 > >  };
 > 
 > This is not the right place for this. The gpio controls isp1707, not
 > the twl4030_usb. You have the rx51_charger_device platform device for
 > isp1707. Add them there.

Will fix.

- Kalle


 > 
 > --
 > heikki

RE: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w]

Hi,

 > -----Original Message-----
 > From: ext Jarkko Nikula [mailto:jhnikula-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
 > Sent: 21. maaliskuuta 2011 17:21
 > To: Jokiniemi Kalle (Nokia-MS/Tampere)
 > Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; balbi-l0cyMroinI0@public.gmane.org;
 > tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org; Krogerus Heikki (Nokia-MS/Helsinki); Koskinen Ilkka
 > (Nokia-MS/Tampere)
 > Subject: Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver
 > 
 > On Mon, 21 Mar 2011 15:50:20 +0200
 > Kalle Jokiniemi <kalle.jokiniemi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
 > 
 > > This patch allows the ISP1707 USB tranceiver on Nokia
 > > N900 to be disabled when usb cable is disconnected.
 > > This saves approximately 14mA of battery current.
 > >
 > I can measure that these two patches save about 14 mA.
 > 
 > > +static void __init rx51_xceiv_init(void)
 > > +{
 > > +	if (gpio_request(RX51_USB_TRANSCEIVER_RST_GPIO, NULL) < 0)
 > > +		BUG();
 > > +	gpio_direction_output(RX51_USB_TRANSCEIVER_RST_GPIO, 1);
 > > +}
 > 
 > gpio_request_one makes this function needless.

Thanks for the pointer, I'll look that up. Though, I want to still keep a function for the init for readability to clearly point out what is being initialized.

 > 
 > > +static int rx51_xceiv_power(struct device *dev, int iD, int on)
 > > +{
 > > +	unsigned long	timeout;
 > > +
 > > +	if (!on) {
 > > +		/* Let musb go stdby before powering down the transceiver */
 > > +		timeout = jiffies + msecs_to_jiffies(100);
 > > +		while (!time_after(jiffies, timeout))
 > > +			if (omap2_cm_read_mod_reg(CORE_MOD,
 > CM_IDLEST1)
 > > +				&
 > OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK)
 > > +				break;
 > > +		if (!(omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
 > > +			& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK))
 > > +			WARN(1, "could not put musb to sleep\n");
 > > +	}
 > > +	gpio_set_value(RX51_USB_TRANSCEIVER_RST_GPIO, on);
 > > +
 > 
 > Hmm... 100 ms busy loop. I wonder are there better ways in usb to
 > achieve this gpio control than this loop here and playing with these
 > registers? I believe musb/phy/etc has knowledge when they are idle and
 > can do some board specific stuff?

Will clean this up. Have to look at that isp1704_charger to see if it can be responsible of the correct state checks...

- Kalle

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

Re: [PATCH 2/2] OMAP3: rx51: specify phy_power for usb tranceiver

[Heikki Krogerus]

Hi,

On Tue, Mar 22, 2011 at 12:24:27PM +0200, Jokiniemi Kalle (Nokia-MS/Tampere) wrote:
>  > > +static int rx51_xceiv_power(struct device *dev, int iD, int on)
>  > > +{
>  > > +	unsigned long	timeout;
>  > > +
>  > > +	if (!on) {
>  > > +		/* Let musb go stdby before powering down the transceiver */
>  > > +		timeout = jiffies + msecs_to_jiffies(100);
>  > > +		while (!time_after(jiffies, timeout))
>  > > +			if (omap2_cm_read_mod_reg(CORE_MOD,
>  > CM_IDLEST1)
>  > > +				&
>  > OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK)
>  > > +				break;
>  > > +		if (!(omap2_cm_read_mod_reg(CORE_MOD, CM_IDLEST1)
>  > > +			& OMAP3430ES2_ST_HSOTGUSB_STDBY_MASK))
>  > > +			WARN(1, "could not put musb to sleep\n");
>  > > +	}
>  > > +	gpio_set_value(RX51_USB_TRANSCEIVER_RST_GPIO, on);
>  > > +
>  > > +	return 0;
>  > > +}
>  > 
>  > The busy loop is not needed, and not what we want. We need to be able
>  > to toggle the CHIP_SEL even if the USB block is not IDLE or STDBY.
> 
> I basically just took what was in the maemo kernel. Was there some reason
> originally to include the busyloop? Do I get it now correctly that the ISP is
> only needed active when we are charging?

OK, my comment was incorrect. The gpio is clearly set regardless of the
outcome of the loop. To answer your question, we only need the ISP to
be active when _detecting_ the charger and obviously when USB is in
use.

We had problems hitting off-mode if we switched the transceiver off
before the controller had entered STDBY. I think this needs to be
tested again. The code in drivers/usb/musb/omap2430.c is quite
different then what we had in maemo kernel. It could be that there is
no need for the loop anymore. If you want to play it safe, fix it and
leave it there.

-- 
heikki