Re: [PATCH] scsi: ufs-qcom: add number of lanes per direction
On 2018-02-09 10:29, Rob Herring wrote: On Mon, Feb 05, 2018 at 08:02:07PM +0800, Can Guo wrote: From: Gilad Broner Different platforms may have different number of lanes for the UFS link. Add parameter to device tree specifying how many lanes should be configured for the UFS link. And don't print err message for clocks that are optional, this leads to unnecessary confusion about failure. Signed-off-by: Gilad Broner Signed-off-by: Subhash Jadavani Signed-off-by: Can Guo diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt index 5357919..4cee3f9 100644 --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt @@ -31,6 +31,9 @@ Optional properties: defined or a value in the array is "0" then it is assumed that the frequency is set by the parent clock or a fixed rate clock source. +- lanes-per-direction: number of lanes available per direction - either 1 or 2. + Note that it is assume same number of lanes is used both directions at once. Seems reasonable until someone does not make things symmetrical. We should design for that case. You are right, I will make changes like using lanes-tx and lanes-rx for Tx/Rx links for asymmetrial senarios and upload V2 patch + If not specified, default is 2 lanes per direction. Note: If above properties are not defined it can be assumed that the supply regulators or clocks are always on.
Re: [PATCH v1] scsi: ufs: add 2 lane support
On 2018-10-04 02:34, Evan Green wrote: Hi, On Wed, Oct 3, 2018 at 11:19 AM Can Guo wrote: From: Venkat Gopalakrishnan Qcom ufs controller v3.1.0 supports 2 lanes, add support to configure 2 lanes during phy initialization. I'm reviving this old chestnut, sorry I missed it initially. This description is a little terse, and I'm actually confused about it. The description makes it sounds like this patch is adding support for 2-lane UFS controllers, but the patch itself appears to only make the UFS controller tolerant of a missing lane (or more specifically, a missing lane clock). Can you describe a little more about what's going on here, and perhaps fix the description? I notice that the global clock controller has clocks for TX symbol 0, and RX symbol 1, but seems to be missing GCC_UFS_PHY_TX_SYMBOL_1_CLK. Was that an oversight, or is it really not there? Signed-off-by: Venkat Gopalakrishnan Signed-off-by: Subhash Jadavani Signed-off-by: Can Guo --- drivers/scsi/ufs/ufs-qcom.c | 20 +--- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index 2b38db2..51889ad 100644 --- a/drivers/scsi/ufs/ufs-qcom.c +++ b/drivers/scsi/ufs/ufs-qcom.c @@ -113,10 +113,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); @@ -147,18 +147,15 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host) 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 */ I'm confused by this comment. What do you mean the clock could be muxed? Hi Evan, sorry for the late response, I totally missed this mail. Here it means Tx Lane1 clock can be muxed with Tx Lane0 clock. As by design, Tx Lane1 clock derives from Tx Lane0 clock. I pushed a new change of it as I change the commit name. Please help review the new patch. Sorry for the inconveience. + 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: @@ -189,8 +186,9 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host) if (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;
Re: [PATCH v1] scsi: ufs: add 2 lane support
Hi Evan, On 2018-10-04 02:34, Evan Green wrote: Hi, On Wed, Oct 3, 2018 at 11:19 AM Can Guo wrote: From: Venkat Gopalakrishnan Qcom ufs controller v3.1.0 supports 2 lanes, add support to configure 2 lanes during phy initialization. I'm reviving this old chestnut, sorry I missed it initially. This description is a little terse, and I'm actually confused about it. The description makes it sounds like this patch is adding support for 2-lane UFS controllers, but the patch itself appears to only make the UFS controller tolerant of a missing lane (or more specifically, a missing lane clock). Can you describe a little more about what's going on here, and perhaps fix the description? I notice that the global clock controller has clocks for TX symbol 0, and RX symbol 1, but seems to be missing GCC_UFS_PHY_TX_SYMBOL_1_CLK. Was that an oversight, or is it really not there? You are right. The name and commit message are not representing itself correctly as most of the original commit has been upstreamed already. I uploaded a new patch to address it. As per Qcom's design Tx Lane1 clock derives from Tx Lane0. So only one Tx Lane0 clock would make 2 Tx lanes work. Signed-off-by: Venkat Gopalakrishnan Signed-off-by: Subhash Jadavani Signed-off-by: Can Guo --- drivers/scsi/ufs/ufs-qcom.c | 20 +--- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index 2b38db2..51889ad 100644 --- a/drivers/scsi/ufs/ufs-qcom.c +++ b/drivers/scsi/ufs/ufs-qcom.c @@ -113,10 +113,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); @@ -147,18 +147,15 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host) 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 */ I'm confused by this comment. What do you mean the clock could be muxed? + 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: @@ -189,8 +186,9 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host) if (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;
Re: [PATCH 1/1] scsi: ufs: make UFS Tx lane1 clock optional
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 wrote:
From: Venkat Gopalakrishnan
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
Re: [PATCH v2 1/1] scsi: ufs: make UFS Tx lane1 clock optional for QCOM platforms
Hi Doug,
On 2018-10-12 04:21, Doug Anderson wrote:
Hi,
On Thu, Oct 11, 2018 at 5:33 AM Can Guo wrote:
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)) {
+ if (clk == ERR_PTR(-EPROBE_DEFER)) {
+ err = -EPROBE_DEFER;
+ dev_warn(dev, "required clock %s hasn't probed yet,
err %d\n",
+ name, err);
+ } else if (IS_ERR(clk)) {
+ if (optional)
+ return 0;
Change this function to:
static int ufs_qcom_host_clk_get(struct device *dev,
const char *name, struct clk **clk_out, bool optional)
{
struct clk *clk;
int err;
clk = devm_clk_get(dev, name);
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;
}
Specifically:
* Typically it just spams the log to print something when you see an
-EPROBE_DEFER.
* If a clock is optional you should only consider things a success
only if "-ENOENT" was returned. All other errors should be considered
fatal.
* If a clock is optional it's slightly cleaner to set *clk_out to
NULL. I know you're initting global data that happened to already be
NULL by default, but it still feels nice for the abstraction of the
function to do this.
* Please don't pass __func__ to your error messages.
Thank you for the detailed instructions. Shall make the change like so.
err = PTR_ERR(clk);
dev_err(dev, "%s: failed to get %s err %d",
__func__, name, err);
@@ -113,10 +119,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)
Remove this "if". Always call clk_disable_unprepare() which will be a
no-op if "host->tx_l1_sync_clk" is NULL. clk_disable_unprepare() is a
no-op for NULL clocks by design specifically to make code like this
cleaner.
Shall do.
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)
Remove this "if". Always call clk_disable_unprepare() which will be a
no-op if "host->rx_l1_sync_clk" is NULL. clk_disable_unprepare() is a
no-op for NULL clocks by design specifically to make code like this
cleaner.
Shall do.
clk_disable_unprepare(host->rx_l1_sync_clk);
clk_disable_unprepare(host->rx_l0_sync_clk);
@@ -141,12 +147,14 @@ 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) {
Remove this "if". Always call ufs_qcom_host_clk_enable(). The
clk_prepare_enable() inside ufs_qcom_host_clk_enable() will be a no-op
if "host->rx_l1_sync_clk" is NULL and will return 0 (no error).
Shall do so.
err = ufs_qcom_host_clk_enable(dev,
"rx_lane1_sync_clk",
host->rx_l1_sync_clk);
if (err)
goto disable_tx_l0;
+ }
+ if (host->tx_l1_sync_clk) {
Remove this "if". Always call ufs_qcom_host_clk_enable(). The
clk_prepare_enable() inside ufs_qcom_host_clk_enable() will be a no-op
if "host->tx_l1_sync_clk" is NULL and will return 0 (no error).
-Doug
Shall do so. Thank you.
-Can Guo
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
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
Signed-off-by: Subhash Jadavani
Signed-off-by: Can Guo
---
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, t
Re: [PATCH v3 1/1] scsi: ufs: make UFS Tx lane1 clock optional for QCOM platforms
On 2018-10-13 00:55, Doug Anderson wrote: Hi, On Thu, Oct 11, 2018 at 6:12 PM Can Guo 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 Tested-by: Douglas Anderson -Doug That is a good idea, I shall follow up after this one is merged. Thank you. -Can Guo
Re: [PATCH v5 1/1] scsi: ufs: make UFS Tx lane1 clock optional for QCOM platforms
Hi Martin, On 2018-10-16 10:56, Martin K. Petersen wrote: Can, 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. Applied to 4.20/scsi-queue, thanks! Thanks a lot! -Can Guo
Re: [PATCH v1] scsi: ufs: add 2 lane support
On 2018-04-02 18:00, Vivek Gautam wrote: Hi Can, On 3/2/2018 1:48 PM, Can Guo wrote: From: Venkat Gopalakrishnan Qcom ufs controller v3.1.0 supports 2 lanes, add support to configure 2 lanes during phy initialization. Signed-off-by: Venkat Gopalakrishnan Signed-off-by: Subhash Jadavani Signed-off-by: Can Guo --- drivers/scsi/ufs/ufs-qcom.c | 20 +--- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index 2b38db2..51889ad 100644 --- a/drivers/scsi/ufs/ufs-qcom.c +++ b/drivers/scsi/ufs/ufs-qcom.c @@ -113,10 +113,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); @@ -147,18 +147,15 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host) 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 */ You need a similar change for "rx_l1_sync_clk" also. And also get rid of 'lanes_per_direction' flag as well for ufs_qcom_enable_lane_clks() and ufs_qcom_init_lane_clks() too, as you are doing in ufs_qcom_disable_lane_clks(). Sure, got it. Thanks Can + 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: @@ -189,8 +186,9 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host) if (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); same here. Best regards Vivek Shall do as well. Thanks Can } out: return err;
