[PATCH] OMAP3: USBHOST: Fix prcm interrupt handler

[PATCH] OMAP3: USBHOST: Fix prcm interrupt handler

[Vikram Pandita]

For clearing the PM_WKST of USBHOST domain, two fclocks need
to be enabled: HOST1 and HOST2

Current code just enables HOST1 fclock and thus not clearing the
Wake status of usbhost domain correctly

Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
---
 arch/arm/mach-omap2/pm34xx.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index e80d59f..4cbeff1 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -264,8 +264,10 @@ static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
 					       CM_FCLKEN);
 			cm_set_mod_reg_bits(wkst, OMAP3430ES2_USBHOST_MOD,
 					    CM_ICLKEN);
-			cm_set_mod_reg_bits(wkst, OMAP3430ES2_USBHOST_MOD,
-					    CM_FCLKEN);
+			cm_set_mod_reg_bits((1<<OMAP3430ES2_EN_USBHOST2_SHIFT)|
+					(1<<OMAP3430ES2_EN_USBHOST1_SHIFT),
+					OMAP3430ES2_USBHOST_MOD,
+					CM_FCLKEN);
 			prm_write_mod_reg(wkst, OMAP3430ES2_USBHOST_MOD,
 					  PM_WKST);
 			while (prm_read_mod_reg(OMAP3430ES2_USBHOST_MOD,
-- 
1.6.3.3.334.g916e1

Re: [PATCH] OMAP3: USBHOST: Fix prcm interrupt handler

[Kevin Hilman]

Vikram Pandita <vikram.pandita@ti.com> writes:

> For clearing the PM_WKST of USBHOST domain, two fclocks need
> to be enabled: HOST1 and HOST2
>
> Current code just enables HOST1 fclock and thus not clearing the
> Wake status of usbhost domain correctly

I think the changelog needs a little more detail.  Something like:

USBHOST module has 2 fclocks (for HOST1 and HOST2), only one iclock
and only a single bit in the WKST register to indicate a wakeup event.

Because of the single WKST bit, we cannot know whether a wakeup event
was on HOST1 or HOST2, so enable both fclocks before clearing the
wakeup event to ensure both hosts can properly clear the event.

And shortlog (subject) should be:

OMAP3: PM: USBHOST: clear wakeup events on both hosts

> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index e80d59f..4cbeff1 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -264,8 +264,10 @@ static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>  					       CM_FCLKEN);
>  			cm_set_mod_reg_bits(wkst, OMAP3430ES2_USBHOST_MOD,
>  					    CM_ICLKEN);
> -			cm_set_mod_reg_bits(wkst, OMAP3430ES2_USBHOST_MOD,
> -					    CM_FCLKEN);
> +			cm_set_mod_reg_bits((1<<OMAP3430ES2_EN_USBHOST2_SHIFT)|
> +					(1<<OMAP3430ES2_EN_USBHOST1_SHIFT),
> +					OMAP3430ES2_USBHOST_MOD,
> +					CM_FCLKEN);

Instead how about keeping original code, but just add something like:

        /* We don't know whether HOST1 or HOST2 woke us up, so
         * enable both clocks. */
        clken = wkst | (1 << OMAP3430ES2_EN_USBHOST2_SHIFT);

Then write 'clken' to the FCLKEN reg and original 'wkst' to the WKST
reg...

>  			prm_write_mod_reg(wkst, OMAP3430ES2_USBHOST_MOD,
>  					  PM_WKST);

otherwise you're writing undefined bits here.

Kevin

RE: [PATCH] OMAP3: USBHOST: Fix prcm interrupt handler

[Pandita, Vikram]

>-----Original Message-----
>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>Sent: Friday, July 17, 2009 6:32 PM
>To: Pandita, Vikram
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCH] OMAP3: USBHOST: Fix prcm interrupt handler
>
>Vikram Pandita <vikram.pandita@ti.com> writes:
>
>> For clearing the PM_WKST of USBHOST domain, two fclocks need
>> to be enabled: HOST1 and HOST2
>>
>> Current code just enables HOST1 fclock and thus not clearing the
>> Wake status of usbhost domain correctly
>
>I think the changelog needs a little more detail.  Something like:
>
>USBHOST module has 2 fclocks (for HOST1 and HOST2), only one iclock
>and only a single bit in the WKST register to indicate a wakeup event.
>
>Because of the single WKST bit, we cannot know whether a wakeup event
>was on HOST1 or HOST2, so enable both fclocks before clearing the
>wakeup event to ensure both hosts can properly clear the event.
>
>And shortlog (subject) should be:
>
>OMAP3: PM: USBHOST: clear wakeup events on both hosts

Ok will send v2 patch with this short log and comments.

>
>> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
>> ---
>>  arch/arm/mach-omap2/pm34xx.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index e80d59f..4cbeff1 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -264,8 +264,10 @@ static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>>  					       CM_FCLKEN);
>>  			cm_set_mod_reg_bits(wkst, OMAP3430ES2_USBHOST_MOD,
>>  					    CM_ICLKEN);
>> -			cm_set_mod_reg_bits(wkst, OMAP3430ES2_USBHOST_MOD,
>> -					    CM_FCLKEN);
>> +			cm_set_mod_reg_bits((1<<OMAP3430ES2_EN_USBHOST2_SHIFT)|
>> +					(1<<OMAP3430ES2_EN_USBHOST1_SHIFT),
>> +					OMAP3430ES2_USBHOST_MOD,
>> +					CM_FCLKEN);
>
>Instead how about keeping original code, but just add something like:
>
>        /* We don't know whether HOST1 or HOST2 woke us up, so
>         * enable both clocks. */
>        clken = wkst | (1 << OMAP3430ES2_EN_USBHOST2_SHIFT);

Ok looks cleaner. Will have this in v2 patch.

>
>Then write 'clken' to the FCLKEN reg and original 'wkst' to the WKST
>reg...
>
>>  			prm_write_mod_reg(wkst, OMAP3430ES2_USBHOST_MOD,
>>  					  PM_WKST);
>
>otherwise you're writing undefined bits here.

No. Original patch does not write any undefined bits. 
Wkst is 0x1 always.

>
>Kevin
>