[PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

[PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

[Chris Ruehl]

usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

* init the sts flag to 0 (missed)
* set the sts flag only if not 0

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---
 drivers/usb/chipidea/core.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 5075407..1a6010e 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -245,6 +245,8 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
 {
 	u32 portsc, lpm, sts = 0;
 
 	switch (ci->platdata->phy_mode) {
 	case USBPHY_INTERFACE_MODE_UTMI:
 		portsc = PORTSC_PTS(PTS_UTMI);
@@ -273,10 +275,12 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
 
 	if (ci->hw_bank.lpm) {
 		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
-		hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
+		if (sts)
+			hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
 	} else {
 		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
-		hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
+		if ( sts )
+			hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
 	}
 }
 
-- 
1.7.10.4

RE: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

[Peter Chen]

Best regards,
Peter Chen


> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of Chris Ruehl
> Sent: Friday, November 29, 2013 3:20 PM
> To: alexander.shishkin@linux.intel.com; gregkh@linuxfoundation.org
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Chris Ruehl
> Subject: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM
> related to STS flag
> 
> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
> 
> * init the sts flag to 0 (missed)
> * set the sts flag only if not 0
> 
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> ---
>  drivers/usb/chipidea/core.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 5075407..1a6010e 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -245,6 +245,8 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>  {
>  	u32 portsc, lpm, sts = 0;
> 
>  	switch (ci->platdata->phy_mode) {
>  	case USBPHY_INTERFACE_MODE_UTMI:
>  		portsc = PORTSC_PTS(PTS_UTMI);
> @@ -273,10 +275,12 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
> 
>  	if (ci->hw_bank.lpm) {
>  		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
> -		hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
> +		if (sts)
> +			hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>  	} else {
>  		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
> -		hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
> +		if ( sts )
> +			hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
>  	}
>  }
> 
> --

Acked-by: Peter Chen <peter.chen@freescale.com>

Peter

Re: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

[Michael Grzeschik]

On Fri, Nov 29, 2013 at 03:19:45PM +0800, Chris Ruehl wrote:
> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
> 
> * init the sts flag to 0 (missed)
> * set the sts flag only if not 0
> 
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> ---
>  drivers/usb/chipidea/core.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 5075407..1a6010e 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -245,6 +245,8 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>  {
>  	u32 portsc, lpm, sts = 0;
>  
>  	switch (ci->platdata->phy_mode) {
>  	case USBPHY_INTERFACE_MODE_UTMI:
>  		portsc = PORTSC_PTS(PTS_UTMI);
> @@ -273,10 +275,12 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>  
>  	if (ci->hw_bank.lpm) {
>  		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
> -		hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
> +		if (sts)
> +			hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>  	} else {
>  		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
> -		hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
> +		if ( sts )
> +			hw_write(ci, OP_PORTSC, PORTSC_STS, sts);

The conditions coding style is broken.

>  	}
>  }

Still don't get why a system with ehci compliant PORTSC register
should not want to have the sts bit _explicitly_ set to 0 if
we don't use serial phy mode. So NACK!

Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

[Chris Ruehl]

On Saturday, November 30, 2013 02:28 AM, Michael Grzeschik wrote:
> On Fri, Nov 29, 2013 at 03:19:45PM +0800, Chris Ruehl wrote:
>> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
>>
>> * init the sts flag to 0 (missed)
>> * set the sts flag only if not 0
>>
>> Signed-off-by: Chris Ruehl<chris.ruehl@gtsys.com.hk>
>> ---
>>   drivers/usb/chipidea/core.c |    8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index 5075407..1a6010e 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -245,6 +245,8 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>>   {
>>   	u32 portsc, lpm, sts = 0;
>>
>>   	switch (ci->platdata->phy_mode) {
>>   	case USBPHY_INTERFACE_MODE_UTMI:
>>   		portsc = PORTSC_PTS(PTS_UTMI);
>> @@ -273,10 +275,12 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>>
>>   	if (ci->hw_bank.lpm) {
>>   		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
>> -		hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>> +		if (sts)
>> +			hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>>   	} else {
>>   		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
>> -		hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
>> +		if ( sts )
>> +			hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
>
> The conditions coding style is broken.
>
>>   	}
>>   }
>
> Still don't get why a system with ehci compliant PORTSC register
> should not want to have the sts bit _explicitly_ set to 0 if
> we don't use serial phy mode. So NACK!
>
> Michael

Michael,

I agree that this patch is not sufficient. I had a look in the reference manual 
for the imx27 and here:

Serial Transceiver Select—Read/Write. This register bit is used in conjunction 
with the configuration constant
VUSB_HS_PHY_SERIAL to control whether the parallel or serial transceiver 
interface is selected for FS and LS
operation. The Serial Interface Engine can be used in combination with the UTMI+ 
or ULPI physical interface to
provide FS/LS signaling instead of the parallel interface. If VUSB_HS_PHY_SERIAL 
is set for 0 or 1 then this
bit is read only. If VUSB_HS_PHY_SERIAL is 3 or 4 then this bit is read/write.
This bit has no effect unless Parallel Transceiver Select is set to UTMI+ or 
ULPI. The Serial/1.1 physical interface
will use the Serial Interface Engine for FS/LS signaling regardless of this bit 
value.
Note: This bit is reserved for future operation and is a placeholder adding 
dynamic use of the serial engine in
accord with UMTI+ and ULPI characterization logic.
This bit is not defined in the EHCI specification.


To make it short. In some VUSB_HS_PHY_SERIAL configurations the STS is a READ 
ONLY and should not be written. The current code (without my wrong patch) always 
write to the STS!
With the patch there was no write and no oops with Internal 808 ARM exception..

Let me rework it.

Regards
Chris

Re: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

[Chris Ruehl]

On Saturday, November 30, 2013 02:28 AM, Michael Grzeschik wrote:
> On Fri, Nov 29, 2013 at 03:19:45PM +0800, Chris Ruehl wrote:
>> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
>>
>> * init the sts flag to 0 (missed)
>> * set the sts flag only if not 0
>>
>> Signed-off-by: Chris Ruehl<chris.ruehl@gtsys.com.hk>
>> ---
>>   drivers/usb/chipidea/core.c |    8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index 5075407..1a6010e 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -245,6 +245,8 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>>   {
>>   	u32 portsc, lpm, sts = 0;
>>
>>   	switch (ci->platdata->phy_mode) {
>>   	case USBPHY_INTERFACE_MODE_UTMI:
>>   		portsc = PORTSC_PTS(PTS_UTMI);
>> @@ -273,10 +275,12 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>>
>>   	if (ci->hw_bank.lpm) {
>>   		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
>> -		hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>> +		if (sts)
>> +			hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>>   	} else {
>>   		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
>> -		hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
>> +		if ( sts )
>> +			hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
>
> The conditions coding style is broken.
>
>>   	}
>>   }
>
> Still don't get why a system with ehci compliant PORTSC register
> should not want to have the sts bit _explicitly_ set to 0 if
> we don't use serial phy mode. So NACK!
>
> Michael

That's happen when I remove the if() statements and unconditional write the sts flag

[    1.128482] ehci-mxc: Freescale On-Chip EHCI Host driver
[    1.136702] usbcore: registered new interface driver usb-storage
[    1.147594] imx_usb 10024000.usb: dummy supplies not allowed
[    1.154803] Unhandled fault: external abort on non-linefetch (0x808) at 
0xf4424184
[    1.162424] Internal error: : 808 [#1] ARM
[    1.166548] Modules linked in:
[    1.169670] CPU: 0 PID: 1 Comm: swapper Not tainted 
3.13.0-rc1-next-20131125-dirty #44
[    1.177629] task: cf850000 ti: cf852000 task.ti: cf852000
[    1.183086] PC is at ci_hdrc_probe+0x250/0x630
[    1.187582] LR is at console_unlock+0x2d4/0x30c
[    1.192165] pc : [<c025f7a8>]    lr : [<c003ec50>]    psr: 60000013


regards
Chris

-- 
GTSYS Limited RFID Technology
A01 24/F Gold King Industrial Bld
35-41 Tai Lin Pai Road, Kwai Chung, Hong Kong
Fax (852) 8167 4060 - Tel (852) 3598 9488

Disclaimer: http://www.gtsys.com.hk/email/classified.html

Re: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

[Chris Ruehl]

On Saturday, November 30, 2013 02:28 AM, Michael Grzeschik wrote:
> On Fri, Nov 29, 2013 at 03:19:45PM +0800, Chris Ruehl wrote:
>> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
>>
>> * init the sts flag to 0 (missed)
>> * set the sts flag only if not 0
>>
>> Signed-off-by: Chris Ruehl<chris.ruehl@gtsys.com.hk>
>> ---
>>   drivers/usb/chipidea/core.c |    8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index 5075407..1a6010e 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -245,6 +245,8 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>>   {
>>   	u32 portsc, lpm, sts = 0;
>>
>>   	switch (ci->platdata->phy_mode) {
>>   	case USBPHY_INTERFACE_MODE_UTMI:
>>   		portsc = PORTSC_PTS(PTS_UTMI);
>> @@ -273,10 +275,12 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
>>
>>   	if (ci->hw_bank.lpm) {
>>   		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
>> -		hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>> +		if (sts)
>> +			hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>>   	} else {
>>   		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
>> -		hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
>> +		if ( sts )
>> +			hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
>
> The conditions coding style is broken.
>
>>   	}
>>   }
>
> Still don't get why a system with ehci compliant PORTSC register
> should not want to have the sts bit _explicitly_ set to 0 if
> we don't use serial phy mode. So NACK!
>
> Michael
>

Guys,,, I must be blind

hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
sts has a wrong value, its should be (sts << 29) when sts = 1
same for lpm value (lpm << 28)

Still needs to check the value of PTS, if PTS > 1  or sts flag is read only.


         if (ci->hw_bank.lpm) {
                 hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
                 if ( sts )
                         hw_write(ci, OP_DEVLC, DEVLC_STS, BIT(28));
                 else
                         hw_write(ci, OP_DEVLC, DEVLC_STS, ~BIT(28));
         } else {
                 hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
                 if (((portsc >> 30) & 0x3) > 1) {   /* check if STS is read only */
                         if (sts) {
                                 hw_write(ci, OP_PORTSC, PORTSC_STS, BIT(29));
                         }
                         else {
                                 portsc = (ioread32(ci->hw_bank.regmap[OP_PORTSC])
                                                 & BIT(29));
                                 if (portsc)  /* sts needs reset */
                                         hw_write(ci, OP_PORTSC, PORTSC_STS,
                                                         ~BIT(29));
                         }
                 }
         }


With a write to an already sts = 0 my kernel oops 808. so check if the sts is 0 
and if 0 then no write.

Can someone have a look for the lpm stuff ?


Michael, thanks for the NOACK ;-) otherwise

Regards
Chris

RE: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag

[Peter Chen]

 
> >  {
> >  	u32 portsc, lpm, sts = 0;
> >
> >  	switch (ci->platdata->phy_mode) {
> >  	case USBPHY_INTERFACE_MODE_UTMI:
> >  		portsc = PORTSC_PTS(PTS_UTMI);
> > @@ -273,10 +275,12 @@ static void hw_phymode_configure(struct ci_hdrc
> *ci)
> >
> >  	if (ci->hw_bank.lpm) {
> >  		hw_write(ci, OP_DEVLC, DEVLC_PTS(7) | DEVLC_PTW, lpm);
> > -		hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
> > +		if (sts)
> > +			hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
> >  	} else {
> >  		hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
> > -		hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
> > +		if ( sts )
> > +			hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
> 
> The conditions coding style is broken.
> 
> >  	}
> >  }
> 
> Still don't get why a system with ehci compliant PORTSC register
> should not want to have the sts bit _explicitly_ set to 0 if
> we don't use serial phy mode. So NACK!
> 

At current code the PORTSC_STS is set to unknown value due to sts is
uninitialized, and it causes ARM internal error at Chris's imx27 platform.

This patch just gives sts an initialized value, and only set PORTSC_STS
for serial phy, it follows the code original logic mostly.

Until now, no one reports problem due to the wrong value of PORTSC_STS, it means
both 1 and 0 are working for most of platforms. So, just keep its current logic
until someone blames.

The chipidea datasheet said "This bit has no effect unless Parallel Transceiver Select is set to UTMI+.",
my imx6 uses UTIM+, but the controller works fine whether PORTSC_STS is 0 or 1.
My original idea is just delete the code about PORTSC_STS, but chris
mentions it is used by serial phy at current logic, then, just keep current logic.
 
Peter

> Michael
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 | http://www.pengutronix.de/
> |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555
> |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html