* [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
@ 2013-11-30 3:51 Chris Ruehl
2013-11-30 10:20 ` Peter Chen
2013-11-30 17:28 ` Sergei Shtylyov
0 siblings, 2 replies; 7+ messages in thread
From: Chris Ruehl @ 2013-11-30 3:51 UTC (permalink / raw)
To: alexander.shishkin, gregkh; +Cc: linux-usb, linux-kernel, Chris Ruehl
usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
* init the sts flag to 0 (missed)
* Set PORTCS_STS only if VUSB_HS_PHY_TYPE > 1
otherwise the register is ReadOnly
* Set/Reset correct BIT(28)/BIT(29) for STS
Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---
drivers/usb/chipidea/core.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 5075407..2c634c1 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -243,7 +243,7 @@ static int hw_device_init(struct ci_hdrc *ci, void __iomem *base)
static void hw_phymode_configure(struct ci_hdrc *ci)
{
- u32 portsc, lpm, sts;
+ u32 portsc, lpm, sts = 0;
switch (ci->platdata->phy_mode) {
case USBPHY_INTERFACE_MODE_UTMI:
@@ -273,10 +273,24 @@ 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, BIT(28));
+ else
+ hw_write(ci, OP_DEVLC, DEVLC_STS, ~BIT(28));
} else {
hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
- hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
+ if (((portsc >> 30) & 0x3) > 1) {
+ if (sts) {
+ hw_write(ci, OP_PORTSC, PORTSC_STS, BIT(29));
+ }
+ else {
+ portsc = (ioread32(ci->hw_bank.regmap[OP_PORTSC])
+ & PORTSC_STS);
+ if (portsc)
+ hw_write(ci, OP_PORTSC, PORTSC_STS,
+ ~BIT(29));
+ }
+ }
}
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-11-30 3:51 [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag Chris Ruehl
@ 2013-11-30 10:20 ` Peter Chen
2013-12-02 1:49 ` Chris Ruehl
2013-11-30 17:28 ` Sergei Shtylyov
1 sibling, 1 reply; 7+ messages in thread
From: Peter Chen @ 2013-11-30 10:20 UTC (permalink / raw)
To: Chris Ruehl, alexander.shishkin@linux.intel.com,
gregkh@linuxfoundation.org
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
>
> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
>
> * init the sts flag to 0 (missed)
> * Set PORTCS_STS only if VUSB_HS_PHY_TYPE > 1
> otherwise the register is ReadOnly
> * Set/Reset correct BIT(28)/BIT(29) for STS
>
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> ---
> drivers/usb/chipidea/core.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 5075407..2c634c1 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -243,7 +243,7 @@ static int hw_device_init(struct ci_hdrc *ci, void
> __iomem *base)
>
> static void hw_phymode_configure(struct ci_hdrc *ci)
> {
> - u32 portsc, lpm, sts;
> + u32 portsc, lpm, sts = 0;
>
> switch (ci->platdata->phy_mode) {
> case USBPHY_INTERFACE_MODE_UTMI:
> @@ -273,10 +273,24 @@ 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, BIT(28));
> + else
> + hw_write(ci, OP_DEVLC, DEVLC_STS, ~BIT(28));
> } else {
> hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
> - hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
> + if (((portsc >> 30) & 0x3) > 1) {
> + if (sts) {
> + hw_write(ci, OP_PORTSC, PORTSC_STS, BIT(29));
> + }
> + else {
> + portsc = (ioread32(ci->hw_bank.regmap[OP_PORTSC])
> + & PORTSC_STS);
> + if (portsc)
> + hw_write(ci, OP_PORTSC, PORTSC_STS,
> + ~BIT(29));
> + }
> + }
> }
> }
>
> --
At my chipidea datasheet, the VUSB_HS_PHY_SERIAL is at HWGENERAL (bit[10..11]),
We are still not sure the portsc_sts is needed to set or clear, and how to do
it. My suggestion is just use v2 patch (except fixing one code style problem)
Peter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-11-30 3:51 [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag Chris Ruehl
2013-11-30 10:20 ` Peter Chen
@ 2013-11-30 17:28 ` Sergei Shtylyov
2013-12-02 1:57 ` Chris Ruehl
1 sibling, 1 reply; 7+ messages in thread
From: Sergei Shtylyov @ 2013-11-30 17:28 UTC (permalink / raw)
To: Chris Ruehl, alexander.shishkin, gregkh; +Cc: linux-usb, linux-kernel
Hello.
On 30-11-2013 7:51, Chris Ruehl wrote:
> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
> * init the sts flag to 0 (missed)
> * Set PORTCS_STS only if VUSB_HS_PHY_TYPE > 1
> otherwise the register is ReadOnly
> * Set/Reset correct BIT(28)/BIT(29) for STS
> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
The coding style is still wrong at places...
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 5075407..2c634c1 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
[...]
> @@ -273,10 +273,24 @@ 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 )
Remove spaces around 'sts', please.
> + 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);
> - hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
> + if (((portsc >> 30) & 0x3) > 1) {
> + if (sts) {
> + hw_write(ci, OP_PORTSC, PORTSC_STS, BIT(29));
> + }
> + else {
} else {
> + portsc = (ioread32(ci->hw_bank.regmap[OP_PORTSC])
> + & PORTSC_STS);
No need for outer (). And it's preferred that an operator is left at the
end of a first line, not starts the continuation line.
WBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-11-30 10:20 ` Peter Chen
@ 2013-12-02 1:49 ` Chris Ruehl
2013-12-02 5:10 ` Peter Chen
0 siblings, 1 reply; 7+ messages in thread
From: Chris Ruehl @ 2013-12-02 1:49 UTC (permalink / raw)
To: Peter Chen
Cc: alexander.shishkin@linux.intel.com, gregkh@linuxfoundation.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
On Saturday, November 30, 2013 06:20 PM, Peter Chen wrote:
>
>>
>> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
>>
>> * init the sts flag to 0 (missed)
>> * Set PORTCS_STS only if VUSB_HS_PHY_TYPE> 1
>> otherwise the register is ReadOnly
>> * Set/Reset correct BIT(28)/BIT(29) for STS
>>
>> Signed-off-by: Chris Ruehl<chris.ruehl@gtsys.com.hk>
>> ---
>> drivers/usb/chipidea/core.c | 20 +++++++++++++++++---
>> 1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index 5075407..2c634c1 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -243,7 +243,7 @@ static int hw_device_init(struct ci_hdrc *ci, void
>> __iomem *base)
>>
>> static void hw_phymode_configure(struct ci_hdrc *ci)
>> {
>> - u32 portsc, lpm, sts;
>> + u32 portsc, lpm, sts = 0;
>>
>> switch (ci->platdata->phy_mode) {
>> case USBPHY_INTERFACE_MODE_UTMI:
>> @@ -273,10 +273,24 @@ 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, BIT(28));
>> + else
>> + hw_write(ci, OP_DEVLC, DEVLC_STS, ~BIT(28));
>> } else {
>> hw_write(ci, OP_PORTSC, PORTSC_PTS(7) | PORTSC_PTW, portsc);
>> - hw_write(ci, OP_PORTSC, PORTSC_STS, sts);
>> + if (((portsc>> 30)& 0x3)> 1) {
>> + if (sts) {
>> + hw_write(ci, OP_PORTSC, PORTSC_STS, BIT(29));
>> + }
>> + else {
>> + portsc = (ioread32(ci->hw_bank.regmap[OP_PORTSC])
>> + & PORTSC_STS);
>> + if (portsc)
>> + hw_write(ci, OP_PORTSC, PORTSC_STS,
>> + ~BIT(29));
>> + }
>> + }
>> }
>> }
>>
>> --
>
> At my chipidea datasheet, the VUSB_HS_PHY_SERIAL is at HWGENERAL (bit[10..11]),
> We are still not sure the portsc_sts is needed to set or clear, and how to do
> it. My suggestion is just use v2 patch (except fixing one code style problem)
>
> Peter
>
Peter,
Thanks you for your comments.
If you have a look into the function hw_write() you will see that there is no
effect if hw_write(...,sts) is called with sts=0/1, because the mask will cut
off all bits beside BIT(29).
I used BIT(29) rather then PORTCS_STS to make it more clear what going on.
A write to PORTCS will always be "0" for the STS Register no matter if sts is 1
or 0 within Patch v2. Patch v3 will take care of the registers bit position and
set 1 or 0 to PORTCS_STS.
I used the imx27 reference manual Capital 30.8.1.5.12 PORTSCx.
Please comment.
Chris
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-11-30 17:28 ` Sergei Shtylyov
@ 2013-12-02 1:57 ` Chris Ruehl
0 siblings, 0 replies; 7+ messages in thread
From: Chris Ruehl @ 2013-12-02 1:57 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-kernel
Hi
On Sunday, December 01, 2013 01:28 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 30-11-2013 7:51, Chris Ruehl wrote:
>
>> usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
>
>> * init the sts flag to 0 (missed)
>> * Set PORTCS_STS only if VUSB_HS_PHY_TYPE > 1
>> otherwise the register is ReadOnly
>> * Set/Reset correct BIT(28)/BIT(29) for STS
>
>> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
>
> The coding style is still wrong at places...
>
..
>> - hw_write(ci, OP_DEVLC, DEVLC_STS, sts);
>> + if ( sts )
>
> Remove spaces around 'sts', please.
...
>> + portsc = (ioread32(ci->hw_bank.regmap[OP_PORTSC])
>> + & PORTSC_STS);
>
> No need for outer (). And it's preferred that an operator is left at the
> end of a first line, not starts the continuation line.
>
> WBR, Sergei
>
Sergei,
Thanks, I will take care of and have a question about style
portsc = ioread32(ci->hw_bank.regmap[OP_PORTSC] & PORTSC_STS;
the line in the if/else alignment extent the 80char barrier and should be
splitted in this case '&' is at 81 .. how serious I should follow this one?
portsc = ioread32(ci->hw_bank.regmap[OP_PORTSC] &
PORTSC_STS;
I keep this mail private don't want make too much noise on the list.
regards
Chris
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-12-02 1:49 ` Chris Ruehl
@ 2013-12-02 5:10 ` Peter Chen
2013-12-02 5:17 ` Chris Ruehl
0 siblings, 1 reply; 7+ messages in thread
From: Peter Chen @ 2013-12-02 5:10 UTC (permalink / raw)
To: Chris Ruehl
Cc: alexander.shishkin@linux.intel.com, gregkh@linuxfoundation.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
>
> If you have a look into the function hw_write() you will see that there
> is no
> effect if hw_write(...,sts) is called with sts=0/1, because the mask will
> cut
> off all bits beside BIT(29).
Yes, it is my careless. I thought sts is PORTCS_STS.
> I used BIT(29) rather then PORTCS_STS to make it more clear what going on.
It is not a good coding style, you do need use MACRO to instead of raw number directly.
> A write to PORTCS will always be "0" for the STS Register no matter if
> sts is 1
> or 0 within Patch v2. Patch v3 will take care of the registers bit
> position and
> set 1 or 0 to PORTCS_STS.
>
My suggestion like below:
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, DEVLC_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, PORTSC_STS);
}
I think we just need to fix the original bug, and do not add any new fixes
since we don't know which one is correct for every platform. My proposal is
just set PORTSC_STS (DEVLC_STS is lpm) if it is serial PHY. For any other PHYS, just keep
the reset value.
Peter
> I used the imx27 reference manual Capital 30.8.1.5.12 PORTSCx.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag
2013-12-02 5:10 ` Peter Chen
@ 2013-12-02 5:17 ` Chris Ruehl
0 siblings, 0 replies; 7+ messages in thread
From: Chris Ruehl @ 2013-12-02 5:17 UTC (permalink / raw)
To: Peter Chen
Cc: alexander.shishkin@linux.intel.com, gregkh@linuxfoundation.org,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
On Monday, December 02, 2013 01:10 PM, Peter Chen wrote:
>
>>
>> If you have a look into the function hw_write() you will see that there
>> is no
>> effect if hw_write(...,sts) is called with sts=0/1, because the mask will
>> cut
>> off all bits beside BIT(29).
>
> Yes, it is my careless. I thought sts is PORTCS_STS.
>
>> I used BIT(29) rather then PORTCS_STS to make it more clear what going on.
>
> It is not a good coding style, you do need use MACRO to instead of raw number directly.
>
>> A write to PORTCS will always be "0" for the STS Register no matter if
>> sts is 1
>> or 0 within Patch v2. Patch v3 will take care of the registers bit
>> position and
>> set 1 or 0 to PORTCS_STS.
>>
>
> My suggestion like below:
>
> 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, DEVLC_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, PORTSC_STS);
> }
>
>
> I think we just need to fix the original bug, and do not add any new fixes
> since we don't know which one is correct for every platform. My proposal is
> just set PORTSC_STS (DEVLC_STS is lpm) if it is serial PHY. For any other PHYS, just keep
> the reset value.
>
> Peter
>
>> I used the imx27 reference manual Capital 30.8.1.5.12 PORTSCx.
>>
I can follow your arguments, ACK.
I prepare a patch set with this solution if not other people have better
ideas.
Chris
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-12-02 5:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-30 3:51 [PATCH 2/3 v3] usb: chipidea: Fix Internal error: : 808 [#1] ARM related to STS flag Chris Ruehl
2013-11-30 10:20 ` Peter Chen
2013-12-02 1:49 ` Chris Ruehl
2013-12-02 5:10 ` Peter Chen
2013-12-02 5:17 ` Chris Ruehl
2013-11-30 17:28 ` Sergei Shtylyov
2013-12-02 1:57 ` Chris Ruehl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox