Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1516104 > unrolled thread

[PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time

Back to article view | Back to linux.kernel

Contents

  [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time William Wu <wulf@rock-chips.com> - 2016-11-07 14:10 +0100
    Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock  stable time Doug Anderson <dianders@google.com> - 2016-11-09 22:00 +0100
      Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock  stable time wlf <wulf@rock-chips.com> - 2016-11-10 04:00 +0100
        Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time Heiko Stübner <heiko@sntech.de> - 2016-11-10 10:30 +0100
          Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock  stable time wlf <wulf@rock-chips.com> - 2016-11-11 04:30 +0100

#1516104 — [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time

We found that the system crashed due to 480MHz output clock of
USB2 PHY was unstable after clock had been enabled by gpu module.

Theoretically, 1 millisecond is a critical value for 480MHz
output clock stable time, so we try to change the delay time
to 1.2 millisecond to avoid this issue.

Signed-off-by: William Wu <wulf@rock-chips.com>
---
 drivers/phy/phy-rockchip-inno-usb2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/phy-rockchip-inno-usb2.c b/drivers/phy/phy-rockchip-inno-usb2.c
index ecfd7d1..8f2d2b6 100644
--- a/drivers/phy/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/phy-rockchip-inno-usb2.c
@@ -267,7 +267,7 @@ static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw)
 			return ret;
 
 		/* waitting for the clk become stable */
-		mdelay(1);
+		udelay(1200);
 	}
 
 	return 0;
-- 
2.0.0

[toc] | [next] | [standalone]

#1518480 — Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time

Hi,

On Mon, Nov 7, 2016 at 5:00 AM, William Wu <wulf@rock-chips.com> wrote:
> We found that the system crashed due to 480MHz output clock of
> USB2 PHY was unstable after clock had been enabled by gpu module.
>
> Theoretically, 1 millisecond is a critical value for 480MHz
> output clock stable time, so we try to change the delay time
> to 1.2 millisecond to avoid this issue.
>
> Signed-off-by: William Wu <wulf@rock-chips.com>
> ---
>  drivers/phy/phy-rockchip-inno-usb2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/phy-rockchip-inno-usb2.c b/drivers/phy/phy-rockchip-inno-usb2.c
> index ecfd7d1..8f2d2b6 100644
> --- a/drivers/phy/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
> @@ -267,7 +267,7 @@ static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw)
>                         return ret;
>
>                 /* waitting for the clk become stable */
> -               mdelay(1);
> +               udelay(1200);

Several people who have seen this patch have expressed concern that a
1.2 ms delay is pretty long for something that's supposed to be
"atomic" like a clk_enable().  Consider that someone might call
clk_enable() while interrupts are disabled and that a 1.2 ms interrupt
latency is not so great.

It seems like this clock should be moved to be enabled in "prepare"
and the "enable" should be a no-op.  This is a functionality change,
but I don't think there are any real users for this clock at the
moment so it should be fine.

(of course, the 1 ms latency that existed before this patch was still
pretty bad, but ...)

-Doug

[toc] | [prev] | [next] | [standalone]

#1518656 — Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time

Hi Doug,

在 2016年11月10日 04:54, Doug Anderson 写道:
> Hi,
>
> On Mon, Nov 7, 2016 at 5:00 AM, William Wu <wulf@rock-chips.com> wrote:
>> We found that the system crashed due to 480MHz output clock of
>> USB2 PHY was unstable after clock had been enabled by gpu module.
>>
>> Theoretically, 1 millisecond is a critical value for 480MHz
>> output clock stable time, so we try to change the delay time
>> to 1.2 millisecond to avoid this issue.
>>
>> Signed-off-by: William Wu <wulf@rock-chips.com>
>> ---
>>   drivers/phy/phy-rockchip-inno-usb2.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/phy-rockchip-inno-usb2.c b/drivers/phy/phy-rockchip-inno-usb2.c
>> index ecfd7d1..8f2d2b6 100644
>> --- a/drivers/phy/phy-rockchip-inno-usb2.c
>> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
>> @@ -267,7 +267,7 @@ static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw)
>>                          return ret;
>>
>>                  /* waitting for the clk become stable */
>> -               mdelay(1);
>> +               udelay(1200);
> Several people who have seen this patch have expressed concern that a
> 1.2 ms delay is pretty long for something that's supposed to be
> "atomic" like a clk_enable().  Consider that someone might call
> clk_enable() while interrupts are disabled and that a 1.2 ms interrupt
> latency is not so great.
>
> It seems like this clock should be moved to be enabled in "prepare"
> and the "enable" should be a no-op.  This is a functionality change,
> but I don't think there are any real users for this clock at the
> moment so it should be fine.
>
> (of course, the 1 ms latency that existed before this patch was still
> pretty bad, but ...)
Thanks a lot for your suggestion.
I agree with you.  clk_enable() will call spin_lock_irqsave() to disable 
interrupt, and we add
more than 1ms in clk_enable may cause big latency.

And according to clk_prepare() description:
  In a simple case, clk_prepare can be used instead of clk_enable to 
ungate a clk if the
  operation may sleep.  One example is a clk which is accessed over I2c.

So maybe we can remove the clock to clk_prepare.

Hi Heiko, Frank,
        What  do you think of it?

Best regards,
         wulf
>
> -Doug
>
>
>

[toc] | [prev] | [next] | [standalone]

#1518780

Am Donnerstag, 10. November 2016, 10:54:49 schrieb wlf:
> Hi Doug,
> 
> 在 2016年11月10日 04:54, Doug Anderson 写道:
> > Hi,
> > 
> > On Mon, Nov 7, 2016 at 5:00 AM, William Wu <wulf@rock-chips.com> wrote:
> >> We found that the system crashed due to 480MHz output clock of
> >> USB2 PHY was unstable after clock had been enabled by gpu module.
> >> 
> >> Theoretically, 1 millisecond is a critical value for 480MHz
> >> output clock stable time, so we try to change the delay time
> >> to 1.2 millisecond to avoid this issue.
> >> 
> >> Signed-off-by: William Wu <wulf@rock-chips.com>
> >> ---
> >> 
> >>   drivers/phy/phy-rockchip-inno-usb2.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/phy/phy-rockchip-inno-usb2.c
> >> b/drivers/phy/phy-rockchip-inno-usb2.c index ecfd7d1..8f2d2b6 100644
> >> --- a/drivers/phy/phy-rockchip-inno-usb2.c
> >> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
> >> @@ -267,7 +267,7 @@ static int rockchip_usb2phy_clk480m_enable(struct
> >> clk_hw *hw)>> 
> >>                          return ret;
> >>                  
> >>                  /* waitting for the clk become stable */
> >> 
> >> -               mdelay(1);
> >> +               udelay(1200);
> > 
> > Several people who have seen this patch have expressed concern that a
> > 1.2 ms delay is pretty long for something that's supposed to be
> > "atomic" like a clk_enable().  Consider that someone might call
> > clk_enable() while interrupts are disabled and that a 1.2 ms interrupt
> > latency is not so great.
> > 
> > It seems like this clock should be moved to be enabled in "prepare"
> > and the "enable" should be a no-op.  This is a functionality change,
> > but I don't think there are any real users for this clock at the
> > moment so it should be fine.
> > 
> > (of course, the 1 ms latency that existed before this patch was still
> > pretty bad, but ...)
> 
> Thanks a lot for your suggestion.
> I agree with you.  clk_enable() will call spin_lock_irqsave() to disable
> interrupt, and we add
> more than 1ms in clk_enable may cause big latency.
> 
> And according to clk_prepare() description:
>   In a simple case, clk_prepare can be used instead of clk_enable to
> ungate a clk if the
>   operation may sleep.  One example is a clk which is accessed over I2c.
> 
> So maybe we can remove the clock to clk_prepare.
> 
> Hi Heiko, Frank,
>         What  do you think of it?

moving to clk_prepare sounds sensible. That way you can switch from delay to 
sleep functions as well.


Heiko

[toc] | [prev] | [next] | [standalone]

#1519538 — Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time

Hi Heiko，

在 2016年11月10日 17:21, Heiko Stübner 写道:
> Am Donnerstag, 10. November 2016, 10:54:49 schrieb wlf:
>> Hi Doug,
>>
>> 在 2016年11月10日 04:54, Doug Anderson 写道:
>>> Hi,
>>>
>>> On Mon, Nov 7, 2016 at 5:00 AM, William Wu <wulf@rock-chips.com> wrote:
>>>> We found that the system crashed due to 480MHz output clock of
>>>> USB2 PHY was unstable after clock had been enabled by gpu module.
>>>>
>>>> Theoretically, 1 millisecond is a critical value for 480MHz
>>>> output clock stable time, so we try to change the delay time
>>>> to 1.2 millisecond to avoid this issue.
>>>>
>>>> Signed-off-by: William Wu <wulf@rock-chips.com>
>>>> ---
>>>>
>>>>    drivers/phy/phy-rockchip-inno-usb2.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/phy/phy-rockchip-inno-usb2.c
>>>> b/drivers/phy/phy-rockchip-inno-usb2.c index ecfd7d1..8f2d2b6 100644
>>>> --- a/drivers/phy/phy-rockchip-inno-usb2.c
>>>> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
>>>> @@ -267,7 +267,7 @@ static int rockchip_usb2phy_clk480m_enable(struct
>>>> clk_hw *hw)>>
>>>>                           return ret;
>>>>                   
>>>>                   /* waitting for the clk become stable */
>>>>
>>>> -               mdelay(1);
>>>> +               udelay(1200);
>>> Several people who have seen this patch have expressed concern that a
>>> 1.2 ms delay is pretty long for something that's supposed to be
>>> "atomic" like a clk_enable().  Consider that someone might call
>>> clk_enable() while interrupts are disabled and that a 1.2 ms interrupt
>>> latency is not so great.
>>>
>>> It seems like this clock should be moved to be enabled in "prepare"
>>> and the "enable" should be a no-op.  This is a functionality change,
>>> but I don't think there are any real users for this clock at the
>>> moment so it should be fine.
>>>
>>> (of course, the 1 ms latency that existed before this patch was still
>>> pretty bad, but ...)
>> Thanks a lot for your suggestion.
>> I agree with you.  clk_enable() will call spin_lock_irqsave() to disable
>> interrupt, and we add
>> more than 1ms in clk_enable may cause big latency.
>>
>> And according to clk_prepare() description:
>>    In a simple case, clk_prepare can be used instead of clk_enable to
>> ungate a clk if the
>>    operation may sleep.  One example is a clk which is accessed over I2c.
>>
>> So maybe we can remove the clock to clk_prepare.
>>
>> Hi Heiko, Frank,
>>          What  do you think of it?
> moving to clk_prepare sounds sensible. That way you can switch from delay to
> sleep functions as well.
Thanks very much.
I will try to update a new patch.

Best regards,
          Wulf

>
>
> Heiko
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web