| Message ID | 1538973275-5961-1-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] scsi: ufs: make UFS Tx lane1 clock optional | expand |
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
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
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;