[PATCH] sh7786: modify usb setup timeout judgment bug.

[PATCH] sh7786: modify usb setup timeout judgment bug.

[Kuninori Morimoto]

Timeout counter "i" can reach -1 if USB setup isn't done.
And "i" can be 0 if USB setup is done when last possible moment.
Then it is judged to fail though setup is done.
This patch modify this problem.
Special thanks to Mr. Juha Leppanen for nice advice.

Reported-by: "Juha Leppanen" <juha_motorsportcom@luukku.com>
Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
---
> Paul
Can you test this patch on Urquell board please ?

 arch/sh/kernel/cpu/sh4a/setup-sh7786.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7786.c b/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
index 5a47e1c..ee30e68 100644
--- a/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
+++ b/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
@@ -127,6 +127,7 @@ void __init sh7786_usb_use_exclock(void)
 static void __init sh7786_usb_setup(void)
 {
 	int i = 1000000;
+	u32 val;
 
 	/*
 	 * USB initial settings
@@ -143,11 +144,14 @@ static void __init sh7786_usb_setup(void)
 	 * Set the PHY and PLL enable bit
 	 */
 	__raw_writel(PHY_ENB | PLL_ENB, USBPCTL1);
-	while (i-- &&
-	       ((__raw_readl(USBST) & ACT_PLL_STATUS) != ACT_PLL_STATUS))
+	while (i--) {
+		val = __raw_readl(USBST) & ACT_PLL_STATUS;
+		if (ACT_PLL_STATUS = val)
+			break;
 		cpu_relax();
+	}
 
-	if (i) {
+	if (ACT_PLL_STATUS = val) {
 		/* Set the PHY RST bit */
 		__raw_writel(PHY_ENB | PLL_ENB | PHY_RST, USBPCTL1);
 		printk(KERN_INFO "sh7786 usb setup done\n");
-- 
1.5.6.3

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Nobuhiro Iwamatsu]

Kuninori Morimoto wrote:
> Timeout counter "i" can reach -1 if USB setup isn't done.
> And "i" can be 0 if USB setup is done when last possible moment.
> Then it is judged to fail though setup is done.
> This patch modify this problem.
> Special thanks to Mr. Juha Leppanen for nice advice.
> 
> Reported-by: "Juha Leppanen" <juha_motorsportcom@luukku.com>
> Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
> ---
>> Paul
> Can you test this patch on Urquell board please ?
> 
>  arch/sh/kernel/cpu/sh4a/setup-sh7786.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7786.c b/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
> index 5a47e1c..ee30e68 100644
> --- a/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
> +++ b/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
> @@ -127,6 +127,7 @@ void __init sh7786_usb_use_exclock(void)
>  static void __init sh7786_usb_setup(void)
>  {
>  	int i = 1000000;
> +	u32 val;
>  
>  	/*
>  	 * USB initial settings
> @@ -143,11 +144,14 @@ static void __init sh7786_usb_setup(void)
>  	 * Set the PHY and PLL enable bit
>  	 */
>  	__raw_writel(PHY_ENB | PLL_ENB, USBPCTL1);
> -	while (i-- &&
> -	       ((__raw_readl(USBST) & ACT_PLL_STATUS) != ACT_PLL_STATUS))
> +	while (i--) {
> +		val = __raw_readl(USBST) & ACT_PLL_STATUS;
> +		if (ACT_PLL_STATUS = val)
> +			break;
>  		cpu_relax();
> +	}
>  
> -	if (i) {
> +	if (ACT_PLL_STATUS = val) {
>  		/* Set the PHY RST bit */
>  		__raw_writel(PHY_ENB | PLL_ENB | PHY_RST, USBPCTL1);
>  		printk(KERN_INFO "sh7786 usb setup done\n");

I think this line to be able to be in while.

Best regards,
  Nobuhiro

-----
diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7786.c b/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
index 5a47e1c..08dcec5 100644
--- a/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
+++ b/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
@@ -127,6 +127,7 @@ void __init sh7786_usb_use_exclock(void)
  static void __init sh7786_usb_setup(void)
  {
  	int i = 1000000;
+	u32 val;

  	/*
  	 * USB initial settings
@@ -143,14 +144,15 @@ static void __init sh7786_usb_setup(void)
  	 * Set the PHY and PLL enable bit
  	 */
  	__raw_writel(PHY_ENB | PLL_ENB, USBPCTL1);
-	while (i-- &&
-	       ((__raw_readl(USBST) & ACT_PLL_STATUS) != ACT_PLL_STATUS))
+	while (i--) {
+		val = __raw_readl(USBST) & ACT_PLL_STATUS;
+		if (ACT_PLL_STATUS = val) {
+			/* Set the PHY RST bit */
+			__raw_writel(PHY_ENB | PLL_ENB | PHY_RST, USBPCTL1);
+			printk(KERN_INFO "sh7786 usb setup done\n");
+			break;
+		}
  		cpu_relax();
-
-	if (i) {
-		/* Set the PHY RST bit */
-		__raw_writel(PHY_ENB | PLL_ENB | PHY_RST, USBPCTL1);
-		printk(KERN_INFO "sh7786 usb setup done\n");
  	}
  }

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Michael Trimarchi]

Hi,

Kuninori Morimoto wrote:
> Timeout counter "i" can reach -1 if USB setup isn't done.
> And "i" can be 0 if USB setup is done when last possible moment.
> Then it is judged to fail though setup is done.
> This patch modify this problem.
> Special thanks to Mr. Juha Leppanen for nice advice.
>
> Reported-by: "Juha Leppanen" <juha_motorsportcom@luukku.com>
> Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
> ---
>   
>> Paul
>>     
> Can you test this patch on Urquell board please ?
>
>  arch/sh/kernel/cpu/sh4a/setup-sh7786.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7786.c b/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
> index 5a47e1c..ee30e68 100644
> --- a/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
> +++ b/arch/sh/kernel/cpu/sh4a/setup-sh7786.c
> @@ -127,6 +127,7 @@ void __init sh7786_usb_use_exclock(void)
>  static void __init sh7786_usb_setup(void)
>  {
>  	int i = 1000000;
> +	u32 val;
>  
>  	/*
>  	 * USB initial settings
> @@ -143,11 +144,14 @@ static void __init sh7786_usb_setup(void)
>  	 * Set the PHY and PLL enable bit
>  	 */
>  	__raw_writel(PHY_ENB | PLL_ENB, USBPCTL1);
> -	while (i-- &&
> -	       ((__raw_readl(USBST) & ACT_PLL_STATUS) != ACT_PLL_STATUS))
> +	while (i--) {
> +		val = __raw_readl(USBST) & ACT_PLL_STATUS;
> +		if (ACT_PLL_STATUS = val)
> +			break;
>  		cpu_relax();
> +	}
>  
> -	if (i) {
> +	if (ACT_PLL_STATUS = val) {
>  		/* Set the PHY RST bit */
>  		__raw_writel(PHY_ENB | PLL_ENB | PHY_RST, USBPCTL1);
>  		printk(KERN_INFO "sh7786 usb setup done\n");
>   
I think that this initialization function maybe pass to the platform 
data of the
usb driver. Give a change to go ahead and deferrable the inizialization 
of the system.
So during the bootup we can avoid the "while loop". What is exaclty 100000?

Michael

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[morimoto.kuninori]

Dear Michael

Thank you for advice.

> I think that this initialization function maybe pass to the platform 
> data of the
> usb driver. Give a change to go ahead and deferrable the inizialization 
> of the system.

Hmm. I can agree with you.

This usb setup register have not been mapped as USB module.
And, (I think) "current" usb driver (sh_ohci/r8a66597)
can not call peculiar init function.

I think following step is best.

1) Change usb driver to be able to call peculiar init function.
2) setup-sh7786 set peculiar init funtion to usb driver.
3) usb driver call it when start

How about this idea ?

Best regards
--
Kuninori Morimoto
 

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Michael Trimarchi]

Hi,

morimoto.kuninori@renesas.com wrote:
> Dear Michael
>
> Thank you for advice.
>
>   
>> I think that this initialization function maybe pass to the platform 
>> data of the
>> usb driver. Give a change to go ahead and deferrable the inizialization 
>> of the system.
>>     
>
> Hmm. I can agree with you.
>
> This usb setup register have not been mapped as USB module.
> And, (I think) "current" usb driver (sh_ohci/r8a66597)
> can not call peculiar init function.
>
> I think following step is best.
>
> 1) Change usb driver to be able to call peculiar init function.
> 2) setup-sh7786 set peculiar init funtion to usb driver.
> 3) usb driver call it when start
>
> How about this idea ?
>   
I think that is a good approch. If it is possible, before the setup, the 
best
thing is to test if the bootloader has just enable it and skip the setup 
part in that
case.
Michael
> Best regards
> --
> Kuninori Morimoto
>  
>
>   

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[morimoto.kuninori]

Dear Michael

Thank you for comment

> > 1) Change usb driver to be able to call peculiar init function.
> > 2) setup-sh7786 set peculiar init funtion to usb driver.
> > 3) usb driver call it when start
> >
> > How about this idea ?
> >   
> I think that is a good approch. If it is possible, before the setup, the 
> best thing is to test if the bootloader has just
> enable it and skip the setup part in that case.

wow !!
how cool idea !!

> shimoda san

Can you change use driver to be able to call peculiar init function ?
Or Can I do that ?

Best regards
--
Kuninori Morimoto
 

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Michael Trimarchi]

Hi,

morimoto.kuninori@renesas.com wrote:
> Dear Michael
>
> Thank you for comment
>
>   
>>> 1) Change usb driver to be able to call peculiar init function.
>>> 2) setup-sh7786 set peculiar init funtion to usb driver.
>>> 3) usb driver call it when start
>>>
>>> How about this idea ?
>>>   
>>>       
>> I think that is a good approch. If it is possible, before the setup, the 
>> best thing is to test if the bootloader has just
>> enable it and skip the setup part in that case.
>>     
>
> wow !!
> how cool idea !!
>
>   
>> shimoda san
>>     
>
> Can you change use driver to be able to call peculiar init function ?
> Or Can I do that ?
>   
I will try to provide a patch if you want on 14 of April, or if you 
prefer just start and
send a patch to the reviewer :)

Michael

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[morimoto.kuninori]

Dear Michael

> >> shimoda san
> >
> > Can you change use driver to be able to call peculiar init function ?
> > Or Can I do that ?
> >   
> I will try to provide a patch if you want on 14 of April, or if you 
> prefer just start and
> send a patch to the reviewer :)

Ooops. sorry.
My request is to "shimoda san" who created usb driver for SH :)

Best regards
--
Kuninori Morimoto
 

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Yoshihiro Shimoda]

Hi Morimoto-san,

morimoto.kuninori@renesas.com wrote:
> Dear Michael
> 
>>>> shimoda san
>>> Can you change use driver to be able to call peculiar init function ?
>>> Or Can I do that ?
>>>   
>> I will try to provide a patch if you want on 14 of April, or if you 
>> prefer just start and
>> send a patch to the reviewer :)
> 
> Ooops. sorry.
> My request is to "shimoda san" who created usb driver for SH :)
> 

I think that it is after next month if I try it.
Because linux-2.6.30 merge window was just closed recently :)

Thanks,
Yoshihiro Shimoda

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Michael Trimarchi]

Hi,

Yoshihiro Shimoda wrote:
> Hi Morimoto-san,
>
> morimoto.kuninori@renesas.com wrote:
>> Dear Michael
>>
>>>>> shimoda san
>>>> Can you change use driver to be able to call peculiar init function ?
>>>> Or Can I do that ?
>>>>   
>>> I will try to provide a patch if you want on 14 of April, or if you 
>>> prefer just start and
>>> send a patch to the reviewer :)
>> Ooops. sorry.
>> My request is to "shimoda san" who created usb driver for SH :)
>>
>
> I think that it is after next month if I try it.
> Because linux-2.6.30 merge window was just closed recently :)
>
I don't have all the data. Look at the code there only the init function 
to be
deferrable. Introduce a platform data it can be usefull but I would like to
know if there are other info that can be passed with the platform data.
Is that the only cpu that needs a specific initialization? Before a patch
maybe we can ask how can we use it?
Just to know:
what is this function sh7786_usb_use_exclock?

for example the s3c2410 define and use a platform data, for port flags or
power control or overun current situation. What can we put in a platform 
data?

Michael



-
> Thanks,
> Yoshihiro Shimoda
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Yoshihiro Shimoda]

Dear Michael,

Michael Trimarchi wrote:
> Hi,
> 
> Yoshihiro Shimoda wrote:
>> Hi Morimoto-san,
>>
>> morimoto.kuninori@renesas.com wrote:
>>> Dear Michael
>>>
>>>>>> shimoda san
>>>>> Can you change use driver to be able to call peculiar init function ?
>>>>> Or Can I do that ?
>>>>>   
>>>> I will try to provide a patch if you want on 14 of April, or if you 
>>>> prefer just start and
>>>> send a patch to the reviewer :)
>>> Ooops. sorry.
>>> My request is to "shimoda san" who created usb driver for SH :)
>>>
>> I think that it is after next month if I try it.
>> Because linux-2.6.30 merge window was just closed recently :)
>>
> I don't have all the data. Look at the code there only the init function 
> to be
> deferrable. Introduce a platform data it can be usefull but I would like to
> know if there are other info that can be passed with the platform data.

Thank you for your comment! I understood it.

> Is that the only cpu that needs a specific initialization? Before a patch
> maybe we can ask how can we use it?
> Just to know:
> what is this function sh7786_usb_use_exclock?

SH7763's OHCI need initialization too
(but differ proccess: arch/sh/board/mach-sh7763rdp/setup.c: line 172).

I'm sorry, I don't know detail of this function.
Morimoto-san, please tell me this information?

> for example the s3c2410 define and use a platform data, for port flags or
> power control or overun current situation. What can we put in a platform 
> data?

The Renesas usb host controllers are:
 - OHCI (drivers/usb/host/ohci-sh.c)
 - r8a66597 (drivers/usb/host/r8a66597-hcd.c)

In OHCI, we can put extend initialize function in a platform data only at the moment.
However, in r8a66597, we can put all module_param data.

Thanks,
Yoshihiro Shimoda

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Paul Mundt]

On Thu, Apr 09, 2009 at 08:08:21PM +0900, Yoshihiro Shimoda wrote:
> Michael Trimarchi wrote:
> > Is that the only cpu that needs a specific initialization? Before a patch
> > maybe we can ask how can we use it?
> > Just to know:
> > what is this function sh7786_usb_use_exclock?
> 
> SH7763's OHCI need initialization too
> (but differ proccess: arch/sh/board/mach-sh7763rdp/setup.c: line 172).
> 
> I'm sorry, I don't know detail of this function.
> Morimoto-san, please tell me this information?
> 
> > for example the s3c2410 define and use a platform data, for port flags or
> > power control or overun current situation. What can we put in a platform 
> > data?
> 
> The Renesas usb host controllers are:
>  - OHCI (drivers/usb/host/ohci-sh.c)
>  - r8a66597 (drivers/usb/host/r8a66597-hcd.c)
> 
> In OHCI, we can put extend initialize function in a platform data only at the moment.
> However, in r8a66597, we can put all module_param data.
> 
I don't see any reason why we can't accomodate the r7a66597 platform data
change within -rc2, so if that is the way you want to fix this, then that
should be fine. Of course if it is too invasive, we will just use the
original fix in the board, but there is not much point in doing a
stop-gap fix if we already have some plan on how to fix it properly.

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[morimoto.kuninori]

Dear all

> > SH7763's OHCI need initialization too
> > (but differ proccess: arch/sh/board/mach-sh7763rdp/setup.c: line 172).
> > 
> > I'm sorry, I don't know detail of this function.
> > Morimoto-san, please tell me this information?

Sorry, I don't know either...

> deferrable. Introduce a platform data it can be usefull but I would like to
> know if there are other info that can be passed with the platform data.
> Is that the only cpu that needs a specific initialization? Before a patch
> maybe we can ask how can we use it?

SH7724 CPU (new CPU) need specific initialization too.
It use r8a66597.

> Just to know:
> what is this function sh7786_usb_use_exclock?

sh7786 can select usb clock mode.
- crystal resonator (default)
- external clock

So, if board use external clock,
we have to set it to register.
This function for it.

Best regards
--
Kuninori Morimoto
 

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Yoshihiro Shimoda]

Hi Paul,

Paul Mundt wrote:
> On Thu, Apr 09, 2009 at 08:08:21PM +0900, Yoshihiro Shimoda wrote:
>> Michael Trimarchi wrote:
>>> Is that the only cpu that needs a specific initialization? Before a patch
>>> maybe we can ask how can we use it?
>>> Just to know:
>>> what is this function sh7786_usb_use_exclock?
>> SH7763's OHCI need initialization too
>> (but differ proccess: arch/sh/board/mach-sh7763rdp/setup.c: line 172).
>>
>> I'm sorry, I don't know detail of this function.
>> Morimoto-san, please tell me this information?
>>
>>> for example the s3c2410 define and use a platform data, for port flags or
>>> power control or overun current situation. What can we put in a platform 
>>> data?
>> The Renesas usb host controllers are:
>>  - OHCI (drivers/usb/host/ohci-sh.c)
>>  - r8a66597 (drivers/usb/host/r8a66597-hcd.c)
>>
>> In OHCI, we can put extend initialize function in a platform data only at the moment.
>> However, in r8a66597, we can put all module_param data.
>>
> I don't see any reason why we can't accomodate the r7a66597 platform data
> change within -rc2, so if that is the way you want to fix this, then that
> should be fine. Of course if it is too invasive, we will just use the
> original fix in the board, but there is not much point in doing a
> stop-gap fix if we already have some plan on how to fix it properly.

Thank you for your comment!
I will try this changing properly. But I may need much time.

Thanks,
Yoshihiro Shimoda

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[morimoto.kuninori]

Dear Paul

> > I don't see any reason why we can't accomodate the r7a66597 platform data
> > change within -rc2, so if that is the way you want to fix this, then that
> > should be fine. Of course if it is too invasive, we will just use the
> > original fix in the board, but there is not much point in doing a
> > stop-gap fix if we already have some plan on how to fix it properly.
> 
> Thank you for your comment!
> I will try this changing properly. But I may need much time.

So, what is the best way about this problem ?

Can you agree to use this (niche) patch until
shimoda-san's work will be completed ?
If you can agree, I would like to send v2 patch.

Or should I wait for shimoda-san's work ?

Best regards
--
Kuninori Morimoto
 

Re: [PATCH] sh7786: modify usb setup timeout judgment bug.

[Paul Mundt]

On Fri, Apr 10, 2009 at 01:56:09PM +0900, morimoto.kuninori@renesas.com wrote:
> > > I don't see any reason why we can't accomodate the r7a66597 platform data
> > > change within -rc2, so if that is the way you want to fix this, then that
> > > should be fine. Of course if it is too invasive, we will just use the
> > > original fix in the board, but there is not much point in doing a
> > > stop-gap fix if we already have some plan on how to fix it properly.
> > 
> > Thank you for your comment!
> > I will try this changing properly. But I may need much time.
> 
> So, what is the best way about this problem ?
> 
> Can you agree to use this (niche) patch until
> shimoda-san's work will be completed ?
> If you can agree, I would like to send v2 patch.
> 
> Or should I wait for shimoda-san's work ?
> 
You can send a v2 patch. If Shimoda-san does not believe he will be able
to submit a more complete patch within the -rc2 timeframe, then we will
use your v2 patch instead.