| Message ID | 1539306736-9519-1-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | [v3,1/1] scsi: ufs: make UFS Tx lane1 clock optional for QCOM platforms | expand |
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
Hi, On 2018-10-12 14:10, Vivek Gautam wrote: > 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 Thank you Vivek. -Can Guo
Hi, On Thu, Oct 11, 2018 at 6:12 PM Can Guo <cang@codeaurora.org> wrote: > + if (err != -EPROBE_DEFER) > + dev_err(dev, "failed to get %s err %d", > + name, err); I wouldn't spin just for this, but if you spin for some other reason you could move the above "dev_err" onto one line now. Sorry: I should have noticed that / done that on my patch... > @@ -141,24 +147,21 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host) Idea for a future patch: now that I look at what's left of this function you're basically re-implementing clk_bulk_prepare_enable() and clk_bulk_disable_unprepare() now. I bet your code would be cleaner / nicer by switching to that. ...possibly you might need to improve the clk_bulk_get() API to allow for some clock to be optional, but that would be a pretty easy patch to post up. In any case I think it's better to do the clk_bulk switch in a future / separate patch, so: Reviewed-by: Douglas Anderson <dianders@chromium.org> Tested-by: Douglas Anderson <dianders@chromium.org> -Doug
On 2018-10-13 00:55, Doug Anderson wrote: > Hi, > > On Thu, Oct 11, 2018 at 6:12 PM Can Guo <cang@codeaurora.org> wrote: >> + if (err != -EPROBE_DEFER) >> + dev_err(dev, "failed to get %s err %d", >> + name, err); > > I wouldn't spin just for this, but if you spin for some other reason > you could move the above "dev_err" onto one line now. Sorry: I should > have noticed that / done that on my patch... > > Yeah... I didn't notice it too. Shall push a new one to address it. >> @@ -141,24 +147,21 @@ static int ufs_qcom_enable_lane_clks(struct >> ufs_qcom_host *host) > > Idea for a future patch: now that I look at what's left of this > function you're basically re-implementing clk_bulk_prepare_enable() > and clk_bulk_disable_unprepare() now. I bet your code would be > cleaner / nicer by switching to that. > > ...possibly you might need to improve the clk_bulk_get() API to allow > for some clock to be optional, but that would be a pretty easy patch > to post up. > > In any case I think it's better to do the clk_bulk switch in a future > / separate patch, so: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Douglas Anderson <dianders@chromium.org> > > > -Doug That is a good idea, I shall follow up after this one is merged. Thank you. -Can Guo
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;