Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966347AbcKKDYE (ORCPT ); Thu, 10 Nov 2016 22:24:04 -0500 Received: from regular1.263xmail.com ([211.150.99.134]:37900 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964823AbcKKDYB (ORCPT ); Thu, 10 Nov 2016 22:24:01 -0500 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-RL-SENDER: wulf@rock-chips.com X-FST-TO: linux-clk@vger.kernel.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: wulf@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time To: =?UTF-8?Q?Heiko_St=c3=bcbner?= References: <1478523658-9400-1-git-send-email-wulf@rock-chips.com> <9eeb3d84-f850-4b76-ce6a-67cc92d6de3b@rock-chips.com> <2283070.Hkeafucara@diego> Cc: Doug Anderson , Kishon Vijay Abraham I , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Rockchip SoC..." , "devicetree@vger.kernel.org" , Rob Herring , Frank Wang , =?UTF-8?B?6buE5rab?= , Brian Norris , Guenter Roeck , Matthias Kaehlcke , linux-clk From: wlf Message-ID: Date: Fri, 11 Nov 2016 11:23:49 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <2283070.Hkeafucara@diego> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2712 Lines: 77 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 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 >>>> --- >>>> >>>> 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 > > >