Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933812AbcJRUNy (ORCPT ); Tue, 18 Oct 2016 16:13:54 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:43666 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753615AbcJRUNq (ORCPT ); Tue, 18 Oct 2016 16:13:46 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 18 Oct 2016 13:13:44 -0700 From: Subhash Jadavani To: Vivek Gautam Cc: kishon@ti.com, jejb@linux.vnet.ibm.com, vinholikatti@gmail.com, martin.petersen@oracle.com, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org Subject: Re: [PATCH 08/10] ufs-qcom: phy/hcd: Refactoring phy clock handling In-Reply-To: <1476800897-19898-9-git-send-email-vivek.gautam@codeaurora.org> References: <1476800897-19898-1-git-send-email-vivek.gautam@codeaurora.org> <1476800897-19898-9-git-send-email-vivek.gautam@codeaurora.org> Message-ID: User-Agent: Roundcube Webmail/1.2.1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7812 Lines: 237 On 2016-10-18 07:28, Vivek Gautam wrote: > Add phy clock enable code to phy_power_on/off callbacks, and > remove explicit calls to enable these phy clocks from the > ufs-qcom hcd driver. > > Signed-off-by: Vivek Gautam > --- > > Changes since v1: > - staticized ufs_qcom_phy_enable(/disable)_ref_clk(), > - staticized ufs_qcom_phy_enable(/disable)_iface_clk() > - removed function declaration and export symbol for these APIs. > > drivers/phy/phy-qcom-ufs.c | 36 > ++++++++++++++++++------------------ > drivers/scsi/ufs/ufs-qcom.c | 15 --------------- > include/linux/phy/phy-qcom-ufs.h | 18 ------------------ > 3 files changed, 18 insertions(+), 51 deletions(-) > > diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c > index 144b11f..a425cc2 100644 > --- a/drivers/phy/phy-qcom-ufs.c > +++ b/drivers/phy/phy-qcom-ufs.c > @@ -373,10 +373,9 @@ out: > return ret; > } > > -int ufs_qcom_phy_enable_ref_clk(struct phy *generic_phy) > +static int ufs_qcom_phy_enable_ref_clk(struct ufs_qcom_phy *phy) > { > int ret = 0; > - struct ufs_qcom_phy *phy = get_ufs_qcom_phy(generic_phy); > > if (phy->is_ref_clk_enabled) > goto out; > @@ -423,7 +422,6 @@ out_disable_src: > out: > return ret; > } > -EXPORT_SYMBOL_GPL(ufs_qcom_phy_enable_ref_clk); > > static > int ufs_qcom_phy_disable_vreg(struct device *dev, > @@ -448,10 +446,8 @@ out: > return ret; > } > > -void ufs_qcom_phy_disable_ref_clk(struct phy *generic_phy) > +static void ufs_qcom_phy_disable_ref_clk(struct ufs_qcom_phy *phy) > { > - struct ufs_qcom_phy *phy = get_ufs_qcom_phy(generic_phy); > - > if (phy->is_ref_clk_enabled) { > clk_disable_unprepare(phy->ref_clk); > /* > @@ -464,7 +460,6 @@ void ufs_qcom_phy_disable_ref_clk(struct phy > *generic_phy) > phy->is_ref_clk_enabled = false; > } > } > -EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_ref_clk); > > #define UFS_REF_CLK_EN (1 << 5) > > @@ -517,9 +512,8 @@ void ufs_qcom_phy_disable_dev_ref_clk(struct phy > *generic_phy) > EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_dev_ref_clk); > > /* Turn ON M-PHY RMMI interface clocks */ > -int ufs_qcom_phy_enable_iface_clk(struct phy *generic_phy) > +static int ufs_qcom_phy_enable_iface_clk(struct ufs_qcom_phy *phy) > { > - struct ufs_qcom_phy *phy = get_ufs_qcom_phy(generic_phy); > int ret = 0; > > if (phy->is_iface_clk_enabled) > @@ -543,20 +537,16 @@ int ufs_qcom_phy_enable_iface_clk(struct phy > *generic_phy) > out: > return ret; > } > -EXPORT_SYMBOL_GPL(ufs_qcom_phy_enable_iface_clk); > > /* Turn OFF M-PHY RMMI interface clocks */ > -void ufs_qcom_phy_disable_iface_clk(struct phy *generic_phy) > +void ufs_qcom_phy_disable_iface_clk(struct ufs_qcom_phy *phy) > { > - struct ufs_qcom_phy *phy = get_ufs_qcom_phy(generic_phy); > - > if (phy->is_iface_clk_enabled) { > clk_disable_unprepare(phy->tx_iface_clk); > clk_disable_unprepare(phy->rx_iface_clk); > phy->is_iface_clk_enabled = false; > } > } > -EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_iface_clk); > > int ufs_qcom_phy_start_serdes(struct phy *generic_phy) > { > @@ -674,13 +664,20 @@ int ufs_qcom_phy_power_on(struct phy > *generic_phy) > goto out_disable_phy; > } > > - err = ufs_qcom_phy_enable_ref_clk(generic_phy); > + err = ufs_qcom_phy_enable_iface_clk(phy_common); > if (err) { > - dev_err(dev, "%s enable phy ref clock failed, err=%d\n", > + dev_err(dev, "%s enable phy iface clock failed, err=%d\n", > __func__, err); > goto out_disable_pll; > } > > + err = ufs_qcom_phy_enable_ref_clk(phy_common); > + if (err) { > + dev_err(dev, "%s enable phy ref clock failed, err=%d\n", > + __func__, err); > + goto out_disable_iface_clk; > + } > + > /* enable device PHY ref_clk pad rail */ > if (phy_common->vddp_ref_clk.reg) { > err = ufs_qcom_phy_enable_vreg(dev, > @@ -696,7 +693,9 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy) > goto out; > > out_disable_ref_clk: > - ufs_qcom_phy_disable_ref_clk(generic_phy); > + ufs_qcom_phy_disable_ref_clk(phy_common); > +out_disable_iface_clk: > + ufs_qcom_phy_disable_iface_clk(phy_common); > out_disable_pll: > ufs_qcom_phy_disable_vreg(dev, &phy_common->vdda_pll); > out_disable_phy: > @@ -715,7 +714,8 @@ int ufs_qcom_phy_power_off(struct phy *generic_phy) > if (phy_common->vddp_ref_clk.reg) > ufs_qcom_phy_disable_vreg(phy_common->dev, > &phy_common->vddp_ref_clk); > - ufs_qcom_phy_disable_ref_clk(generic_phy); > + ufs_qcom_phy_disable_ref_clk(phy_common); > + ufs_qcom_phy_disable_iface_clk(phy_common); > > ufs_qcom_phy_disable_vreg(phy_common->dev, &phy_common->vdda_pll); > ufs_qcom_phy_disable_vreg(phy_common->dev, &phy_common->vdda_phy); > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > index 3aedf73..6e4ce5f 100644 > --- a/drivers/scsi/ufs/ufs-qcom.c > +++ b/drivers/scsi/ufs/ufs-qcom.c > @@ -1112,17 +1112,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba > *hba, bool on) > return 0; > > if (on) { > - err = ufs_qcom_phy_enable_iface_clk(host->generic_phy); > - if (err) > - goto out; > - > - err = ufs_qcom_phy_enable_ref_clk(host->generic_phy); > - if (err) { > - dev_err(hba->dev, "%s enable phy ref clock failed, err=%d\n", > - __func__, err); > - ufs_qcom_phy_disable_iface_clk(host->generic_phy); > - goto out; > - } Now that you are moving these ref clk enable/disable to phy_power_on/off and these phy_power_on/off are called only in runtime suspend/resume (3 seconds after last UFS access). Goal is to disable the phy reference clock during aggressive gating (10ms from last UFS access) so shouldn't we call the phy_power_on/off from these setup_clocks() function as well? > /* enable the device ref clock for HS mode*/ > if (ufshcd_is_hs_mode(&hba->pwr_info)) > ufs_qcom_dev_ref_clk_ctrl(host, true); > @@ -1131,9 +1120,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba > *hba, bool on) > ufs_qcom_update_bus_bw_vote(host); > > } else { > - > - /* M-PHY RMMI interface clocks can be turned off */ > - ufs_qcom_phy_disable_iface_clk(host->generic_phy); > if (!ufs_qcom_is_link_active(hba)) > /* disable device ref_clk */ > ufs_qcom_dev_ref_clk_ctrl(host, false); > @@ -1146,7 +1132,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba > *hba, bool on) > dev_err(hba->dev, "%s: set bus vote failed %d\n", > __func__, err); > > -out: > return err; > } > > diff --git a/include/linux/phy/phy-qcom-ufs.h > b/include/linux/phy/phy-qcom-ufs.h > index 9d18e9f..35c070e 100644 > --- a/include/linux/phy/phy-qcom-ufs.h > +++ b/include/linux/phy/phy-qcom-ufs.h > @@ -18,22 +18,6 @@ > #include "phy.h" > > /** > - * ufs_qcom_phy_enable_ref_clk() - Enable the phy > - * ref clock. > - * @phy: reference to a generic phy > - * > - * returns 0 for success, and non-zero for error. > - */ > -int ufs_qcom_phy_enable_ref_clk(struct phy *phy); > - > -/** > - * ufs_qcom_phy_disable_ref_clk() - Disable the phy > - * ref clock. > - * @phy: reference to a generic phy. > - */ > -void ufs_qcom_phy_disable_ref_clk(struct phy *phy); > - > -/** > * ufs_qcom_phy_enable_dev_ref_clk() - Enable the device > * ref clock. > * @phy: reference to a generic phy. > @@ -47,8 +31,6 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy > *phy); > */ > void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy); > > -int ufs_qcom_phy_enable_iface_clk(struct phy *phy); > -void ufs_qcom_phy_disable_iface_clk(struct phy *phy); > int ufs_qcom_phy_start_serdes(struct phy *phy); > int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes); > int ufs_qcom_phy_calibrate_phy(struct phy *phy, bool is_rate_B); -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project