Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751618AbdI0GVy (ORCPT ); Wed, 27 Sep 2017 02:21:54 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:45920 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751055AbdI0GVv (ORCPT ); Wed, 27 Sep 2017 02:21:51 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 8300660723 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=vivek.gautam@codeaurora.org X-Google-Smtp-Source: AOwi7QCGGYRKiV1EVGpn3x18XyvirfCKODa++9q59QV1Qt6IdVKy306y4H5aBUjxlJjwAL/jxKKwhO53Q5Khl3jRCTA= MIME-Version: 1.0 In-Reply-To: References: <1501829332-5183-1-git-send-email-vivek.gautam@codeaurora.org> <1501829332-5183-6-git-send-email-vivek.gautam@codeaurora.org> From: Vivek Gautam Date: Wed, 27 Sep 2017 11:51:48 +0530 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 5/5] ufs/phy: qcom: Refactor to use phy_init call To: Subhash Jadavani Cc: kishon , "Martin K. Petersen" , linux-scsi@vger.kernel.org, "linux-kernel@vger.kernel.org" , "robh+dt" , linux-arm-msm , Bjorn Andersson , "devicetree@vger.kernel.org" , jejb@linux.vnet.ibm.com, Vinayak Holikatti Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8222 Lines: 220 Hi Subhash, On Wed, Sep 27, 2017 at 4:43 AM, Subhash Jadavani wrote: > Hi Vivek, > > Please find one comment inline below, rest look good. > > Regards, > Subhash > > > On 2017-08-03 23:48, Vivek Gautam wrote: >> >> Refactor ufs_qcom_power_up_sequence() to get rid of ugly >> exported phy APIs and use the phy_init() and phy_power_on() >> to do the phy initialization. >> >> Signed-off-by: Vivek Gautam >> --- >> drivers/phy/qualcomm/phy-qcom-ufs-i.h | 2 -- >> drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c | 9 +++++-- >> drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c | 9 +++++-- >> drivers/phy/qualcomm/phy-qcom-ufs.c | 38 >> ++++++++-------------------- >> drivers/scsi/ufs/ufs-qcom.c | 36 >> ++++++++++---------------- >> include/linux/phy/phy-qcom-ufs.h | 3 --- >> 6 files changed, 38 insertions(+), 59 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-i.h >> b/drivers/phy/qualcomm/phy-qcom-ufs-i.h >> index 94326ed107c3..495fd5941231 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-ufs-i.h >> +++ b/drivers/phy/qualcomm/phy-qcom-ufs-i.h >> @@ -123,7 +123,6 @@ struct ufs_qcom_phy { >> * struct ufs_qcom_phy_specific_ops - set of pointers to functions which >> have a >> * specific implementation per phy. Each UFS phy, should implement >> * those functions according to its spec and requirements >> - * @calibrate_phy: pointer to a function that calibrate the phy >> * @start_serdes: pointer to a function that starts the serdes >> * @is_physical_coding_sublayer_ready: pointer to a function that >> * checks pcs readiness. returns 0 for success and non-zero for error. >> @@ -132,7 +131,6 @@ struct ufs_qcom_phy { >> * and writes to QSERDES_RX_SIGDET_CNTRL attribute >> */ >> struct ufs_qcom_phy_specific_ops { >> - int (*calibrate_phy)(struct ufs_qcom_phy *phy, bool is_rate_B); >> void (*start_serdes)(struct ufs_qcom_phy *phy); >> int (*is_physical_coding_sublayer_ready)(struct ufs_qcom_phy >> *phy); >> void (*set_tx_lane_enable)(struct ufs_qcom_phy *phy, u32 val); >> diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c >> b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c >> index af65785230b5..c39440b56b6d 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c >> +++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c >> @@ -44,7 +44,13 @@ void ufs_qcom_phy_qmp_14nm_advertise_quirks(struct >> ufs_qcom_phy *phy_common) >> >> static int ufs_qcom_phy_qmp_14nm_init(struct phy *generic_phy) >> { >> - return 0; >> + struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy); >> + bool is_rate_B = false; >> + >> + if (phy_common->mode == PHY_MODE_UFS_HS_B) >> + is_rate_B = true; >> + >> + return ufs_qcom_phy_qmp_14nm_phy_calibrate(phy_common, is_rate_B); >> } >> >> static int ufs_qcom_phy_qmp_14nm_exit(struct phy *generic_phy) >> @@ -120,7 +126,6 @@ static int >> ufs_qcom_phy_qmp_14nm_is_pcs_ready(struct ufs_qcom_phy *phy_common) >> }; >> >> static struct ufs_qcom_phy_specific_ops phy_14nm_ops = { >> - .calibrate_phy = ufs_qcom_phy_qmp_14nm_phy_calibrate, >> .start_serdes = ufs_qcom_phy_qmp_14nm_start_serdes, >> .is_physical_coding_sublayer_ready = >> ufs_qcom_phy_qmp_14nm_is_pcs_ready, >> .set_tx_lane_enable = >> ufs_qcom_phy_qmp_14nm_set_tx_lane_enable, >> diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c >> b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c >> index 5c18c41dbdb4..5705a2d4c6d2 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c >> +++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c >> @@ -63,7 +63,13 @@ void ufs_qcom_phy_qmp_20nm_advertise_quirks(struct >> ufs_qcom_phy *phy_common) >> >> static int ufs_qcom_phy_qmp_20nm_init(struct phy *generic_phy) >> { >> - return 0; >> + struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy); >> + bool is_rate_B = false; >> + >> + if (phy_common->mode == PHY_MODE_UFS_HS_B) >> + is_rate_B = true; >> + >> + return ufs_qcom_phy_qmp_20nm_phy_calibrate(phy_common, is_rate_B); >> } >> >> static int ufs_qcom_phy_qmp_20nm_exit(struct phy *generic_phy) >> @@ -178,7 +184,6 @@ static int >> ufs_qcom_phy_qmp_20nm_is_pcs_ready(struct ufs_qcom_phy *phy_common) >> }; >> >> static struct ufs_qcom_phy_specific_ops phy_20nm_ops = { >> - .calibrate_phy = ufs_qcom_phy_qmp_20nm_phy_calibrate, >> .start_serdes = ufs_qcom_phy_qmp_20nm_start_serdes, >> .is_physical_coding_sublayer_ready = >> ufs_qcom_phy_qmp_20nm_is_pcs_ready, >> .set_tx_lane_enable = >> ufs_qcom_phy_qmp_20nm_set_tx_lane_enable, >> diff --git a/drivers/phy/qualcomm/phy-qcom-ufs.c >> b/drivers/phy/qualcomm/phy-qcom-ufs.c >> index 43865ef340e2..1febe3294fe3 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-ufs.c >> +++ b/drivers/phy/qualcomm/phy-qcom-ufs.c >> @@ -518,9 +518,8 @@ void ufs_qcom_phy_disable_iface_clk(struct >> ufs_qcom_phy *phy) >> } >> } >> >> -int ufs_qcom_phy_start_serdes(struct phy *generic_phy) >> +static int ufs_qcom_phy_start_serdes(struct ufs_qcom_phy *ufs_qcom_phy) >> { >> - struct ufs_qcom_phy *ufs_qcom_phy = get_ufs_qcom_phy(generic_phy); >> int ret = 0; >> >> if (!ufs_qcom_phy->phy_spec_ops->start_serdes) { >> @@ -533,7 +532,6 @@ int ufs_qcom_phy_start_serdes(struct phy *generic_phy) >> >> return ret; >> } >> -EXPORT_SYMBOL_GPL(ufs_qcom_phy_start_serdes); >> >> int ufs_qcom_phy_set_tx_lane_enable(struct phy *generic_phy, u32 >> tx_lanes) >> { >> @@ -564,31 +562,8 @@ void ufs_qcom_phy_save_controller_version(struct >> phy *generic_phy, >> } >> EXPORT_SYMBOL_GPL(ufs_qcom_phy_save_controller_version); >> >> -int ufs_qcom_phy_calibrate_phy(struct phy *generic_phy, bool is_rate_B) >> -{ >> - struct ufs_qcom_phy *ufs_qcom_phy = get_ufs_qcom_phy(generic_phy); >> - int ret = 0; >> - >> - if (!ufs_qcom_phy->phy_spec_ops->calibrate_phy) { >> - dev_err(ufs_qcom_phy->dev, "%s: calibrate_phy() callback >> is not supported\n", >> - __func__); >> - ret = -ENOTSUPP; >> - } else { >> - ret = ufs_qcom_phy->phy_spec_ops-> >> - calibrate_phy(ufs_qcom_phy, is_rate_B); >> - if (ret) >> - dev_err(ufs_qcom_phy->dev, "%s: calibrate_phy() >> failed %d\n", >> - __func__, ret); >> - } >> - >> - return ret; >> -} >> -EXPORT_SYMBOL_GPL(ufs_qcom_phy_calibrate_phy); >> - >> -int ufs_qcom_phy_is_pcs_ready(struct phy *generic_phy) >> +static int ufs_qcom_phy_is_pcs_ready(struct ufs_qcom_phy *ufs_qcom_phy) >> { >> - struct ufs_qcom_phy *ufs_qcom_phy = get_ufs_qcom_phy(generic_phy); >> - >> if >> (!ufs_qcom_phy->phy_spec_ops->is_physical_coding_sublayer_ready) { >> dev_err(ufs_qcom_phy->dev, "%s: >> is_physical_coding_sublayer_ready() >> callback is not supported\n", >> __func__); >> @@ -598,7 +573,6 @@ int ufs_qcom_phy_is_pcs_ready(struct phy *generic_phy) >> return ufs_qcom_phy->phy_spec_ops-> >> is_physical_coding_sublayer_ready(ufs_qcom_phy); >> } >> -EXPORT_SYMBOL_GPL(ufs_qcom_phy_is_pcs_ready); >> >> int ufs_qcom_phy_power_on(struct phy *generic_phy) >> { >> @@ -609,6 +583,14 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy) >> if (phy_common->is_powered_on) >> return 0; >> >> + err = ufs_qcom_phy_start_serdes(phy_common); >> + if (err) >> + return err; >> + >> + err = ufs_qcom_phy_is_pcs_ready(phy_common); >> + if (err) >> + return err; >> + > > > Now that we are doing serdes start (and checking the PCS ready), I am not > sure we can call phy_power_on() from ufs_qcom_resume(). Please check. > Right. Thanks for catching this. I will check this further. BRs Vivek [snip] -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation