scsi: ufs: make UFS Tx lane1 clock optional
PATCH 1/1 scsi: ufs: make UFS Tx lane1 clock optional
From: Venkat Gopalakrishnan <[email protected]>
The UFS Tx lane1 clock could be muxed, hence keep it optional by ignoring it if it is not provided in device tree.
--- drivers/scsi/ufs/ufs-qcom.c | 46 ++++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 21 deletions(-)
drivers/scsi/ufs/ufs-qcom.c +25 -21
@@ -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;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi,
Quoted 5 lines · Can Guo
From: Venkat Gopalakrishnan <[email protected]>
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! :-)
drivers/scsi/ufs/ufs-qcom.c static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host) · line 114
Quoted 7 lines
drivers/scsi/ufs/ufs-qcom.c +1
-1@@ -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.
drivers/scsi/ufs/ufs-qcom.c static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host) · line 117
Quoted 4 lines
drivers/scsi/ufs/ufs-qcom.c +1
-1clk_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().
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host) · line 141
Quoted 8 lines
drivers/scsi/ufs/ufs-qcom.c +1
-1clk_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.
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host) · line 151
Quoted 4 lines
drivers/scsi/ufs/ufs-qcom.c +4
-0+/* 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.
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host) · line 173
Quoted 8 lines
drivers/scsi/ufs/ufs-qcom.c +3
-1@@ -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.
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host) · line 197
Quoted 5 lines
drivers/scsi/ufs/ufs-qcom.c +3
-2- 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
Hi Doug,
Really thank you for your review.
Quoted 4 lines · Doug Anderson
Hi,
Quoted 1 line · Can Guo
Quoted 9 lines
Quoted 4 lines
From: Venkat Gopalakrishnan <[email protected]>
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.
Quoted 5 lines
Quoted 5 lines
drivers/scsi/ufs/ufs-qcom.c +0
-0@@ -113,10 +110,10 @@ static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)if (!host->is_lane_clks_enabled)return ;
Quoted 9 lines
drivers/scsi/ufs/ufs-qcom.c static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host) · line 114
Quoted 3 lines
drivers/scsi/ufs/ufs-qcom.c +1
-1-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.
Quoted 10 lines
drivers/scsi/ufs/ufs-qcom.c static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host) · line 117
Quoted 4 lines
drivers/scsi/ufs/ufs-qcom.c +1
-1clk_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 whenlanes_per_direction> 1.
Quoted 8 lines
...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 guardingufs_qcom_host_clk_enable().
Sure, got it.
Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufs-qcom.c +0
-0clk_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;
Quoted 8 lines
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host) · line 141
Quoted 2 lines
drivers/scsi/ufs/ufs-qcom.c +1
-1-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.
Quoted 11 lines
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host) · line 151
Quoted 5 lines
drivers/scsi/ufs/ufs-qcom.c +4
-0+/* 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 ofufs_qcom_host_clk_enable(), right? ...so please leave the error path here.
Yeah... you are right.
Quoted 18 lines
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_init_lane_clks(struct · line 175
Quoted 10 lines
drivers/scsi/ufs/ufs-qcom.c +3
-1@@ -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__" indev_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.
Quoted 5 lines
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!
Quoted 18 lines
drivers/scsi/ufs/ufs-qcom.c static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host) · line 197
Quoted 6 lines
drivers/scsi/ufs/ufs-qcom.c +1
-2- 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 */
drivers/scsi/ufs/ufs-qcom.c +2
-0+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.
Quoted 6 lines
If you passed "optional" to
ufs_qcom_host_clk_get()as suggested above you could put this error-code checking inufs_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