[PATCH 1/1] scsi: ufs: make UFS Tx lane1 clock optional

[PATCH 1/1] scsi: ufs: make UFS Tx lane1 clock optional

[Can Guo]

From: Venkat Gopalakrishnan <venkatg@codeaurora.org>

The UFS Tx lane1 clock could be muxed, hence keep it optional by ignoring
it if it is not provided in device tree.

Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 46 ++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 2b38db2..a6e2ed3 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -85,13 +85,10 @@ static int ufs_qcom_host_clk_get(struct device *dev,
 	int err = 0;
 
 	clk = devm_clk_get(dev, name);
-	if (IS_ERR(clk)) {
+	if (IS_ERR(clk))
 		err = PTR_ERR(clk);
-		dev_err(dev, "%s: failed to get %s err %d",
-				__func__, name, err);
-	} else {
+	else
 		*clk_out = clk;
-	}
 
 	return err;
 }
@@ -113,10 +110,10 @@ 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)
+	if (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)
+	if (host->rx_l1_sync_clk)
 		clk_disable_unprepare(host->rx_l1_sync_clk);
 	clk_disable_unprepare(host->rx_l0_sync_clk);
 
@@ -141,24 +138,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) {
+	if (host->rx_l1_sync_clk) {
 		err = ufs_qcom_host_clk_enable(dev, "rx_lane1_sync_clk",
 			host->rx_l1_sync_clk);
 		if (err)
 			goto disable_tx_l0;
-
-		err = ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
-			host->tx_l1_sync_clk);
-		if (err)
-			goto disable_rx_l1;
 	}
 
+	/* The tx lane1 clk could be muxed, hence keep this optional */
+	if (host->tx_l1_sync_clk)
+		ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
+					 host->tx_l1_sync_clk);
+
 	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);
 disable_tx_l0:
 	clk_disable_unprepare(host->tx_l0_sync_clk);
 disable_rx_l0:
@@ -174,23 +168,33 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
 
 	err = ufs_qcom_host_clk_get(dev,
 			"rx_lane0_sync_clk", &host->rx_l0_sync_clk);
-	if (err)
+	if (err) {
+		dev_err(dev, "%s: failed to get rx_lane0_sync_clk, err %d",
+				__func__, err);
 		goto out;
+	}
 
 	err = ufs_qcom_host_clk_get(dev,
 			"tx_lane0_sync_clk", &host->tx_l0_sync_clk);
-	if (err)
+	if (err) {
+		dev_err(dev, "%s: failed to get tx_lane0_sync_clk, err %d",
+				__func__, 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);
-		if (err)
+		if (err) {
+			dev_err(dev, "%s: failed to get rx_lane1_sync_clk, err %d",
+					__func__, err);
 			goto out;
+		}
 
-		err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
-			&host->tx_l1_sync_clk);
+		/* The tx lane1 clk could be muxed, hence keep this optional */
+		ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
+					&host->tx_l1_sync_clk);
 	}
 out:
 	return err;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/1] scsi: ufs: make UFS Tx lane1 clock optional

[Doug Anderson]

Hi,

On Sun, Oct 7, 2018 at 9:34 PM Can Guo <cang@codeaurora.org> wrote:
>
> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>
> The UFS Tx lane1 clock could be muxed, hence keep it optional by ignoring
> it if it is not provided in device tree.

Thanks for the updated commit message.  This makes much more sense now!  :-)


> @@ -113,10 +110,10 @@ 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)
> +       if (host->tx_l1_sync_clk)
>                 clk_disable_unprepare(host->tx_l1_sync_clk);

I don't believe you need the "if".  A NULL clock is by definition OK
to enable / disable which is designed to make optional clocks easier
to deal with.


>         clk_disable_unprepare(host->tx_l0_sync_clk);
> -       if (host->hba->lanes_per_direction > 1)
> +       if (host->rx_l1_sync_clk)
>                 clk_disable_unprepare(host->rx_l1_sync_clk);

optional: Technically this part of the patch wasn't actually needed,
right?  "rx_l1_sync_clk" is not optional so that means that
"rx_l1_sync_clk" should be non-NULL exactly when lanes_per_direction >
1.

...but actually I'm fine with keeping this part of your patch for
symmetry.  Especially since you can leverage the
clk_disable_unprepare(NULL) to simplify your code.  Please mention in
your commit message that this is a cleanup and just for symmetry.
Probably also remove the "if" tests that are guarding
ufs_qcom_host_clk_enable().


>         clk_disable_unprepare(host->rx_l0_sync_clk);
>
> @@ -141,24 +138,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) {
> +       if (host->rx_l1_sync_clk) {

Similar: the above change isn't required but I'm OK if you want to
make this change for symmetry / to cleanup clock handling.  Please
mention in your commit message.


> +       /* The tx lane1 clk could be muxed, hence keep this optional */
> +       if (host->tx_l1_sync_clk)
> +               ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
> +                                        host->tx_l1_sync_clk);

If "host->tx_l1_sync_clk" is provided then you should still check the
return value of ufs_qcom_host_clk_enable(), right?  ...so please leave
the error path here.


> @@ -174,23 +168,33 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
>
>         err = ufs_qcom_host_clk_get(dev,
>                         "rx_lane0_sync_clk", &host->rx_l0_sync_clk);
> -       if (err)
> +       if (err) {
> +               dev_err(dev, "%s: failed to get rx_lane0_sync_clk, err %d",
> +                               __func__, err);

nit: including "__func__" in dev_xxx() calls is discouraged.  The
"dev_xxx" calls already print the device name and the string within a
given device driver should be unique enough so __func__ just adds crap
to the logs.  If you really feel that __func__ adds something for you,
try posting up a patch to make all "dev_err" functions include
__func__.  ...but I think you'd probably be rejected.

suggestion: you'd save a few lines of code and simplify your probe if
you just passed an "optional" bool to the ufs_qcom_host_clk_get() that
controlled the printout.


> -               err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
> -                       &host->tx_l1_sync_clk);
> +               /* The tx lane1 clk could be muxed, hence keep this optional */
> +               ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
> +                                       &host->tx_l1_sync_clk);

To be technically correct you should actually check the error value
returned by ufs_qcom_host_clk_get().  Specifically figure out what the
error value is when "tx_lane1_sync_clk" isn't specified and check for
that.

...one reason this matters is -EPROBE_DEFER.  In theory devm_clk_get()
could return -EPROBE_DEFER.  In such a case you'd want to exit the
probe, not continue on.  It's also just good coding practice to handle
just the error you want though so that if the function returned some
other failing case you'd propagate it down instead of eating it.

If you passed "optional" to ufs_qcom_host_clk_get() as suggested above
you could put this error-code checking in ufs_qcom_host_clk_get() and
return 0 from that function if an optional clock was found to not
exist.

-Doug

Re: [PATCH 1/1] scsi: ufs: make UFS Tx lane1 clock optional

[cang]

Hi Doug,

Really thank you for your review.

On 2018-10-10 05:56, Doug Anderson wrote:
> Hi,
> 
> On Sun, Oct 7, 2018 at 9:34 PM Can Guo <cang@codeaurora.org> wrote:
>> 
>> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>> 
>> The UFS Tx lane1 clock could be muxed, hence keep it optional by 
>> ignoring
>> it if it is not provided in device tree.
> 
> Thanks for the updated commit message.  This makes much more sense now! 
>  :-)
> 
> 

Yeah, sorry for making you guys confused. My bad previously.

>> @@ -113,10 +110,10 @@ 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)
>> +       if (host->tx_l1_sync_clk)
>>                 clk_disable_unprepare(host->tx_l1_sync_clk);
> 
> I don't believe you need the "if".  A NULL clock is by definition OK
> to enable / disable which is designed to make optional clocks easier
> to deal with.
> 
> 

You are right, I checked the implemention, clock enable/disable func 
would
bail out if clk is NULL just as your comments.

>>         clk_disable_unprepare(host->tx_l0_sync_clk);
>> -       if (host->hba->lanes_per_direction > 1)
>> +       if (host->rx_l1_sync_clk)
>>                 clk_disable_unprepare(host->rx_l1_sync_clk);
> 
> optional: Technically this part of the patch wasn't actually needed,
> right?  "rx_l1_sync_clk" is not optional so that means that
> "rx_l1_sync_clk" should be non-NULL exactly when lanes_per_direction >
> 1.
> 
> ...but actually I'm fine with keeping this part of your patch for
> symmetry.  Especially since you can leverage the
> clk_disable_unprepare(NULL) to simplify your code.  Please mention in
> your commit message that this is a cleanup and just for symmetry.
> Probably also remove the "if" tests that are guarding
> ufs_qcom_host_clk_enable().
> 
> 

Sure, got it.

>>         clk_disable_unprepare(host->rx_l0_sync_clk);
>> 
>> @@ -141,24 +138,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) {
>> +       if (host->rx_l1_sync_clk) {
> 
> Similar: the above change isn't required but I'm OK if you want to
> make this change for symmetry / to cleanup clock handling.  Please
> mention in your commit message.
> 
> 

Sure, shall do so.

>> +       /* The tx lane1 clk could be muxed, hence keep this optional 
>> */
>> +       if (host->tx_l1_sync_clk)
>> +               ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
>> +                                        host->tx_l1_sync_clk);
> 
> If "host->tx_l1_sync_clk" is provided then you should still check the
> return value of ufs_qcom_host_clk_enable(), right?  ...so please leave
> the error path here.
> 
> 

Yeah... you are right.

>> @@ -174,23 +168,33 @@ static int ufs_qcom_init_lane_clks(struct 
>> ufs_qcom_host *host)
>> 
>>         err = ufs_qcom_host_clk_get(dev,
>>                         "rx_lane0_sync_clk", &host->rx_l0_sync_clk);
>> -       if (err)
>> +       if (err) {
>> +               dev_err(dev, "%s: failed to get rx_lane0_sync_clk, err 
>> %d",
>> +                               __func__, err);
> 
> nit: including "__func__" in dev_xxx() calls is discouraged.  The
> "dev_xxx" calls already print the device name and the string within a
> given device driver should be unique enough so __func__ just adds crap
> to the logs.  If you really feel that __func__ adds something for you,
> try posting up a patch to make all "dev_err" functions include
> __func__.  ...but I think you'd probably be rejected.
> 
> suggestion: you'd save a few lines of code and simplify your probe if
> you just passed an "optional" bool to the ufs_qcom_host_clk_get() that
> controlled the printout.
> 
> 

Good idea!

>> -               err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
>> -                       &host->tx_l1_sync_clk);
>> +               /* The tx lane1 clk could be muxed, hence keep this 
>> optional */
>> +               ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
>> +                                       &host->tx_l1_sync_clk);
> 
> To be technically correct you should actually check the error value
> returned by ufs_qcom_host_clk_get().  Specifically figure out what the
> error value is when "tx_lane1_sync_clk" isn't specified and check for
> that.
> 
> ...one reason this matters is -EPROBE_DEFER.  In theory devm_clk_get()
> could return -EPROBE_DEFER.  In such a case you'd want to exit the
> probe, not continue on.  It's also just good coding practice to handle
> just the error you want though so that if the function returned some
> other failing case you'd propagate it down instead of eating it.
> 
> If you passed "optional" to ufs_qcom_host_clk_get() as suggested above
> you could put this error-code checking in ufs_qcom_host_clk_get() and
> return 0 from that function if an optional clock was found to not
> exist.
> 
> -Doug

I think I understand it. Thanks a lot for the thorough review and 
suggestions.

-Can Guo