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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6d24a84708f5682df0540d852f60314d@codeaurora.org>
Date:   Fri, 12 Oct 2018 14:46:55 +0800
From:   cang@...eaurora.org
To:     Vivek Gautam <vivek.gautam@...eaurora.org>
Cc:     dianders@...omium.org, subhashj@...eaurora.org,
        asutoshd@...eaurora.org, evgreen@...omium.org,
        rnayak@...eaurora.org, vinholikatti@...il.com,
        jejb@...ux.vnet.ibm.com, martin.petersen@...cle.com,
        linux-scsi@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        Venkat Gopalakrishnan <venkatg@...eaurora.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/1] scsi: ufs: make UFS Tx lane1 clock optional for
 QCOM platforms

Hi,

On 2018-10-12 14:10, Vivek Gautam wrote:
> On 10/12/2018 6:42 AM, Can Guo wrote:
>> From: Venkat Gopalakrishnan <venkatg@...eaurora.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@...eaurora.org>
>> Signed-off-by: Subhash Jadavani <subhashj@...eaurora.org>
>> Signed-off-by: Can Guo <cang@...eaurora.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@...eaurora.org>
> 
> Best regards
> Vivek

Thank you Vivek.

-Can Guo