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 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, 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? > + 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
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;
Re: [PATCH v1] scsi: ufs: add 2 lane support
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(). + 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 } out: return err;
