Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754650AbcKJCzB (ORCPT ); Wed, 9 Nov 2016 21:55:01 -0500 Received: from regular1.263xmail.com ([211.150.99.131]:38735 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753021AbcKJCy7 (ORCPT ); Wed, 9 Nov 2016 21:54:59 -0500 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED4: 1 X-RL-SENDER: wulf@rock-chips.com X-FST-TO: linux-clk@vger.kernel.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: wulf@rock-chips.com X-UNIQUE-TAG: <6dde5c20a5104a6d379b041f1358fbf9> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time To: Doug Anderson , =?UTF-8?Q?Heiko_St=c3=bcbner?= References: <1478523658-9400-1-git-send-email-wulf@rock-chips.com> Cc: 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: <9eeb3d84-f850-4b76-ce6a-67cc92d6de3b@rock-chips.com> Date: Thu, 10 Nov 2016 10:54: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: 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: 2263 Lines: 64 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? Best regards, wulf > > -Doug > > >