From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752536AbcFTDdD (ORCPT ); Sun, 19 Jun 2016 23:33:03 -0400 Received: from regular1.263xmail.com ([211.150.99.130]:41953 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752261AbcFTDcx (ORCPT ); Sun, 19 Jun 2016 23:32:53 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: frank.wang@rock-chips.com X-FST-TO: frank.wang@rock-chips.com X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: frank.wang@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v6 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy To: Guenter Roeck , Guenter Roeck , =?UTF-8?Q?Heiko_St=c3=bcbner?= References: <1466129353-48063-1-git-send-email-frank.wang@rock-chips.com> <1466129353-48063-3-git-send-email-frank.wang@rock-chips.com> <576383C6.3050005@roeck-us.net> <98bc69af-c597-d1ee-e83d-c7b5918e9ef4@rock-chips.com> <5763F92D.80102@roeck-us.net> Cc: Douglas Anderson , Guenter Roeck , jwerner@chromium.org, Kishon Vijay Abraham I , robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, Kumar Gala , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-usb@vger.kernel.org, linux-rockchip@lists.infradead.org, Ziyuan Xu , Kever Yang , Tao Huang , william.wu@rock-chips.com, frank.wang@rock-chips.com From: Frank Wang Message-ID: <3b65b363-72a8-7985-7da1-9045583c90b8@rock-chips.com> Date: Mon, 20 Jun 2016 11:32:35 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 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 Hi Heiko & Guenter, On 2016/6/20 11:00, Guenter Roeck wrote: > On Sun, Jun 19, 2016 at 6:27 PM, Frank Wang wrote: >> Hi Guenter, >> >> >> On 2016/6/17 21:20, Guenter Roeck wrote: >>> Hi Frank, >>> >>> On 06/16/2016 11:43 PM, Frank Wang wrote: >>>> Hi Guenter, >>>> >>>> On 2016/6/17 12:59, Guenter Roeck wrote: >>>>> On 06/16/2016 07:09 PM, Frank Wang wrote: >>>>>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block >>>>>> than rk3288 and before, and most of phy-related registers are also >>>>>> different from the past, so a new phy driver is required necessarily. >>>>>> >>>>>> Signed-off-by: Frank Wang >>>>>> Suggested-by: Guenter Roeck >>>>>> Suggested-by: Doug Anderson >>>>>> Reviewed-by: Heiko Stuebner >>>>>> Tested-by: Heiko Stuebner >>>>>> --- >>>>> >>>>> [ ... ] >>>>> >>>>>> + >>>>>> +static int rockchip_usb2phy_resume(struct phy *phy) >>>>>> +{ >>>>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy); >>>>>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent); >>>>>> + int ret; >>>>>> + >>>>>> + dev_dbg(&rport->phy->dev, "port resume\n"); >>>>>> + >>>>>> + ret = clk_prepare_enable(rphy->clk480m); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>> If suspend can be called multiple times, resume can be called >>>>> multiple times as well. Doesn't this cause a clock imbalance >>>>> if you call clk_prepare_enable() multiple times on resume, >>>>> but clk_disable_unprepare() only once on suspend ? >>>>> >>>> Well, what you said is reasonable, How does something like below? >>>> >>>> @@ -307,6 +307,9 @@ static int rockchip_usb2phy_resume(struct phy *phy) >>>> >>>> dev_dbg(&rport->phy->dev, "port resume\n"); >>>> >>>> + if (!rport->suspended) >>>> + return 0; >>>> + >>>> ret = clk_prepare_enable(rphy->clk480m); >>>> if (ret) >>>> return ret; >>>> @@ -327,12 +330,16 @@ static int rockchip_usb2phy_suspend(struct phy >>>> *phy) >>>> >>>> dev_dbg(&rport->phy->dev, "port suspend\n"); >>>> >>>> + if (rport->suspended) >>>> + return 0; >>>> + >>>> ret = property_enable(rphy, &rport->port_cfg->phy_sus, true); >>>> if (ret) >>>> return ret; >>>> >>>> rport->suspended = true; >>>> clk_disable_unprepare(rphy->clk480m); >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -485,6 +492,7 @@ static int rockchip_usb2phy_host_port_init(struct >>>> rockchip_usb2phy *rphy, >>>> >>>> rport->port_id = USB2PHY_PORT_HOST; >>>> rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST]; >>>> + rport->suspended = true; >>>> >>> Why does it start in suspended mode ? That seems odd. >>> >> This is an initialization. Using above design which make 'suspended' as a >> condition both in *_usb2phy_resume and *_usb2phy_suspend, I believe if it is >> not initialized as suspended mode, the first resume process will be skipped. > I had to re-read the entire patch. > > Turns out my problem was one of terminology. Using "suspend" and > "resume" to me suggested the common use of suspend and resume > functions. That is not the case here. After mentally replacing > "suspend" with "power_off" and "resume" with "power_on", you are > right, no problem exists. Sorry for the noise. > > Maybe it would be useful to replace "resume" with "power_on" and > "suspend" with "power_off" in the function and variable names to > reduce confusion and misunderstandings. > > Thanks, > Guenter Well, it does have a bits confusion, however, the phy-port always just goes to suspend and resume mode (Not power off and power on) in a fact. So must it be renamed? @Heiko Stübner. Hey Heiko, what is your unique perceptions? ;-) BR. Frank > >> Theoretically, the phy-port in suspended mode make sense when it is at start >> time, then the upper layer controller will invoke phy_power_on (See >> phy-core.c), and it further call back *_usb2phy_resume to make phy-port work >> properly. >> >> So could you tell me what make you feeling odd or would you like to give >> another appropriate way please? :-) >> >> BR. >> Frank >> >> >>>> mutex_init(&rport->mutex); >>>> INIT_DELAYED_WORK(&rport->sm_work, rockchip_usb2phy_sm_work); >>>> >>