* Re: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-11-29 18:28 ` Michael Grzeschik
@ 2013-11-30 1:18 ` Chris Ruehl
2013-11-30 1:33 ` Chris Ruehl
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Chris Ruehl @ 2013-11-30 1:18 UTC (permalink / raw)
To: Michael Grzeschik; +Cc: alexander.shishkin, gregkh, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-11-29 18:28 ` Michael Grzeschik
2013-11-30 1:18 ` Chris Ruehl
@ 2013-11-30 1:33 ` Chris Ruehl
2013-11-30 3:04 ` Chris Ruehl
2013-11-30 10:14 ` Peter Chen
3 siblings, 0 replies; 7+ messages in thread
From: Chris Ruehl @ 2013-11-30 1:33 UTC (permalink / raw)
To: Michael Grzeschik; +Cc: alexander.shishkin, gregkh, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-11-29 18:28 ` Michael Grzeschik
2013-11-30 1:18 ` Chris Ruehl
2013-11-30 1:33 ` Chris Ruehl
@ 2013-11-30 3:04 ` Chris Ruehl
2013-11-30 10:14 ` Peter Chen
3 siblings, 0 replies; 7+ messages in thread
From: Chris Ruehl @ 2013-11-30 3:04 UTC (permalink / raw)
To: Michael Grzeschik; +Cc: alexander.shishkin, gregkh, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH 2/3 v2] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-11-29 18:28 ` Michael Grzeschik
` (2 preceding siblings ...)
2013-11-30 3:04 ` Chris Ruehl
@ 2013-11-30 10:14 ` Peter Chen
3 siblings, 0 replies; 7+ messages in thread
From: Peter Chen @ 2013-11-30 10:14 UTC (permalink / raw)
To: Michael Grzeschik, Chris Ruehl
Cc: alexander.shishkin@linux.intel.com, gregkh@linuxfoundation.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
> > {
> > 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
^ permalink raw reply [flat|nested] 7+ messages in thread