Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 83991600E6
Subject: Re: [PATCH v3 1/1] scsi: ufs: make UFS Tx lane1 clock optional for
 QCOM platforms
To:     Can Guo <cang@codeaurora.org>, dianders@chromium.org,
        subhashj@codeaurora.org, asutoshd@codeaurora.org,
        evgreen@chromium.org, rnayak@codeaurora.org,
        vinholikatti@gmail.com, jejb@linux.vnet.ibm.com,
        martin.petersen@oracle.com
Cc:     linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        Venkat Gopalakrishnan <venkatg@codeaurora.org>,
        open list <linux-kernel@vger.kernel.org>
References: <1539306736-9519-1-git-send-email-cang@codeaurora.org>
From:   Vivek Gautam <vivek.gautam@codeaurora.org>
Message-ID: <462df3c4-582b-ccf1-8ced-e0c084bca0f2@codeaurora.org>
Date:   Fri, 12 Oct 2018 11:40:32 +0530
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <1539306736-9519-1-git-send-email-cang@codeaurora.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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