Re: [PATCH v3 1/1] scsi: ufs: make UFS Tx lane1 clock optional for QCOM platforms

cang
2018-10-12T06:46:59Z

Hi,

On 2018-10-12 14:10, Vivek Gautam wrote:
> On 10/12/2018 6:42 AM, Can Guo wrote:
>> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>>
>> Per Qcom's UFS host controller HW design, the UFS Tx lane1 clock could
>> be
>> muxed with Tx lane0 clock, hence keep Tx lane1 clock optional by
>> ignoring
>> it if it is not provided in device tree. This change also performs
>> some
>> cleanup to lanes per direction checks when enable/disable lane clocks
>> just
>> for symmetry.
>>
>> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>> Changes since v2:
>> - Incorporated review comments from Doug.
>>
>> Changes since v1:
>> - Incorporated review comments from Doug.
>> - Update the commit title and commit message.
>>
>> drivers/scsi/ufs/ufs-qcom.c | 55
>> ++++++++++++++++++++++++---------------------
>> 1 file changed, 29 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index 2b38db2..dbc84cb 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -79,20 +79,28 @@ static int ufs_qcom_get_connected_tx_lanes(struct
>> ufs_hba *hba, u32 *tx_lanes)
>> }
>> static int ufs_qcom_host_clk_get(struct device *dev,
>> - const char *name, struct clk **clk_out)
>> + const char *name, struct clk **clk_out, bool optional)
>> {
>> struct clk *clk;
>> int err = 0;
>> clk = devm_clk_get(dev, name);
>> - if (IS_ERR(clk)) {
>> - err = PTR_ERR(clk);
>> - dev_err(dev, "%s: failed to get %s err %d",
>> - __func__, name, err);
>> - } else {
>> + if (!IS_ERR(clk)) {
>> *clk_out = clk;
>> + return 0;
>> }
>> + err = PTR_ERR(clk);
>> +
>> + if (optional && err == -ENOENT) {
>> + *clk_out = NULL;
>> + return 0;
>> + }
>> +
>> + if (err != -EPROBE_DEFER)
>> + dev_err(dev, "failed to get %s err %d",
>> + name, err);
>> +
>> return err;
>> }
>> @@ -113,11 +121,9 @@ static void ufs_qcom_disable_lane_clks(struct
>> ufs_qcom_host *host)
>> if (!host->is_lane_clks_enabled)
>> return;
>> - if (host->hba->lanes_per_direction > 1)
>> - clk_disable_unprepare(host->tx_l1_sync_clk);
>> + clk_disable_unprepare(host->tx_l1_sync_clk);
>> clk_disable_unprepare(host->tx_l0_sync_clk);
>> - if (host->hba->lanes_per_direction > 1)
>> - clk_disable_unprepare(host->rx_l1_sync_clk);
>> + clk_disable_unprepare(host->rx_l1_sync_clk);
>> clk_disable_unprepare(host->rx_l0_sync_clk);
>> host->is_lane_clks_enabled = false;
>> @@ -141,24 +147,21 @@ static int ufs_qcom_enable_lane_clks(struct
>> ufs_qcom_host *host)
>> if (err)
>> goto disable_rx_l0;
>> - if (host->hba->lanes_per_direction > 1) {
>> - err = ufs_qcom_host_clk_enable(dev, "rx_lane1_sync_clk",
>> + err = ufs_qcom_host_clk_enable(dev, "rx_lane1_sync_clk",
>> host->rx_l1_sync_clk);
>> - if (err)
>> - goto disable_tx_l0;
>> + if (err)
>> + goto disable_tx_l0;
>> - err = ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
>> + err = ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
>> host->tx_l1_sync_clk);
>> - if (err)
>> - goto disable_rx_l1;
>> - }
>> + if (err)
>> + goto disable_rx_l1;
>> host->is_lane_clks_enabled = true;
>> goto out;
>> disable_rx_l1:
>> - if (host->hba->lanes_per_direction > 1)
>> - clk_disable_unprepare(host->rx_l1_sync_clk);
>> + clk_disable_unprepare(host->rx_l1_sync_clk);
>> disable_tx_l0:
>> clk_disable_unprepare(host->tx_l0_sync_clk);
>> disable_rx_l0:
>> @@ -172,25 +175,25 @@ static int ufs_qcom_init_lane_clks(struct
>> ufs_qcom_host *host)
>> int err = 0;
>> struct device *dev = host->hba->dev;
>> - err = ufs_qcom_host_clk_get(dev,
>> - "rx_lane0_sync_clk", &host->rx_l0_sync_clk);
>> + err = ufs_qcom_host_clk_get(dev, "rx_lane0_sync_clk",
>> + &host->rx_l0_sync_clk, false);
>> if (err)
>> goto out;
>> - err = ufs_qcom_host_clk_get(dev,
>> - "tx_lane0_sync_clk", &host->tx_l0_sync_clk);
>> + err = ufs_qcom_host_clk_get(dev, "tx_lane0_sync_clk",
>> + &host->tx_l0_sync_clk, false);
>> if (err)
>> goto out;
>> /* In case of single lane per direction, don't read lane1 clocks
>> */
>> if (host->hba->lanes_per_direction > 1) {
>> err = ufs_qcom_host_clk_get(dev, "rx_lane1_sync_clk",
>> - &host->rx_l1_sync_clk);
>> + &host->rx_l1_sync_clk, false);
>> if (err)
>> goto out;
>> err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
>> - &host->tx_l1_sync_clk);
>> + &host->tx_l1_sync_clk, true);
>> }
>> out:
>> return err;
>
> Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>
> Best regards
> Vivek

Thank you Vivek.

-Can Guo