Linux-SCSI Archive on lore.kernel.org
 help / color / Atom feed
From: cang@codeaurora.org
To: Vinod Koul <vkoul@kernel.org>
Cc: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
	asutoshd@codeaurora.org, nguyenb@codeaurora.org,
	Rajendra Nayak <rnayak@codeaurora.org>,
	linux-scsi@vger.kernel.org, kernel-team@android.com,
	saravanak@google.com, salyzyn@google.com,
	Andy Gross <agross@kernel.org>,
	Alim Akhtar <alim.akhtar@samsung.com>,
	Avri Altman <avri.altman@wdc.com>,
	Pedro Sousa <pedrom.sousa@synopsys.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	"open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/7] scsi: ufs-qcom: Add reset control support for host controller
Date: Tue, 17 Dec 2019 18:09:49 +0800
Message-ID: <fc8952a0eee5c010fe14e5f107d89e64@codeaurora.org> (raw)
In-Reply-To: <20191217092433.GN2536@vkoul-mobl>

On 2019-12-17 17:24, Vinod Koul wrote:
> Hi Can,
> 
> On 17-12-19, 15:10, cang@codeaurora.org wrote:
>> On 2019-12-17 12:13, Vinod Koul wrote:
>> > Hi Can,
>> >
>> > On 17-12-19, 08:37, cang@codeaurora.org wrote:
>> > > On 2019-12-17 03:12, Jeffrey Hugo wrote:
>> > > > On Mon, Dec 16, 2019 at 12:05 PM Vinod Koul <vkoul@kernel.org> wrote:
>> > > > >
>> > > > > Hi Can,
>> > > > >
>> > > > > On 14-11-19, 22:09, Can Guo wrote:
>> > > > > > Add reset control for host controller so that host controller can be reset
>> > > > > > as required in its power up sequence.
>> > > > >
>> > > > > I am seeing a regression on UFS on SM8150-mtp with this patch. I think
>> > > > > Jeff is seeing same one lenove laptop on 8998.
>> > > >
>> > > > Confirmed.
>> > > >
>> > > > >
>> > > > > 845 does not seem to have this issue and only thing I can see is
>> > > > > that on
>> > > > > sm8150 and 8998 we define reset as:
>> > > > >
>> > > > >                         resets = <&gcc GCC_UFS_BCR>;
>> > > > >                         reset-names = "rst";
>> > > > >
>> > >
>> > > Hi Jeffrey and Vinod,
>> > >
>> > > Thanks for reporting this. May I know what kind of regression do you
>> > > see on
>> > > 8150 and 8998?
>> > > BTW, do you have reset control for UFS PHY in your DT?
>> > > See 71278b058a9f8752e51030e363b7a7306938f64e.
>> > >
>> > > FYI, we use reset control on all of our platforms and it is
>> > > a must during our power up sequence.
>> >
>> > Yes we do have this and additionally both the DTS describe a 'rst' reset
>> > and this patch tries to use this.
>> >
>> > Can you please tell me which platform this was tested on how the reset
>> > was described in DT
>> >
>> > Thanks
>> 
>> Hi Vinod,
>> 
>> If you are using the 8998's DT present on upstream, you may also need 
>> to
>> enable
>> device reset on your platform. (We usually do a device reset before 
>> call
>> ufshcd_hba_enable())
>> Given that 845 works fine, it may be the difference you have with 845. 
>> 845
>> has device
>> reset support ready in upstream code, you can check sdm845-mtp.dts.
>> It is same for 8150, which is a lack of device reset support in 
>> upstream
>> code base.
> 
> I am using 8150mtp and you can see the DTS at [1]
> with this patch I get phy timeout error
> 
> [    2.532594] qcom-qmp-phy 1d87000.phy: phy initialization timed-out
> 
> If i revert this patch the Timeout goes away. UFS node for this 
> platform
> is enabled in [2] and [3]
> 
> I did add the GPIO as well for testing but that doesnt work, only thing
> that makes it work is rename the reset line to something other that 
> 'rst'
> 
> I found that with this patch the reset is invoked twice, not sure why!
> 
> The 845 does not define a reset 'rst' but both 8150 and 8998 define
> that!
> 
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/log/?h=for-next
> [2]:
> https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=3834a2e92229ef26d30de28acb698b2b23d3e397
> [3]:
> https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=3e5bf28d2c3981f949e848eec8a60e0b9b61189d
>> 
>> To enable UFS device reset, please see
>> 1. 
>> https://lore.kernel.org/linux-arm-msm/20190828191756.24312-4-bjorn.andersson@linaro.org/
>> 2. 53a5372ce326116f3e3d3f1d701113b2542509f4
> 
> Yes both are added for UFS and I am testing with these..
>> 
>> FYI, I tested the patch on 8250 and its family platforms. In my build, 
>> I
>> ported
>> change in #2 to my code base (in your case, make change to
>> drivers/pinctrl/qcom/pinctrl-msm8998.c) and enable the GPIO in DT like
>> sdm845-mtp.dts
> 
> Please see drivers/pinctrl/qcom/pinctrl-sm8150.c upstream
> 
>>         reset-gpios = <&tlmm 150 GPIO_ACTIVE_LOW>;
> 
> Yup, added:
> 
>         reset-gpios = <&tlmm 175 GPIO_ACTIVE_LOW>;

Hi Vinod,

What do you mean by the reset is invoked twice?

Renaming 'rst' to something else equals disabling this patch.

You said 845 has not this problem, I thought you tested the patch on 845 
with
the same 'rst' defined on 8998 and 8150. If 'rst' is not present in 
845's DT,
it means this patch has no impact on 845.

Actually, in our code base, we are not using phy-qcom-qmp.c. Instead,
we are using phy-qcom-ufs.c. phy-qcom-ufs.c is the one we use in all of 
our
mobile projects. Although both have the same functionality,
but in phy-qcom-ufs.c, the PCS ready bit polling timeout is 1000000us,
while in phy-qcom-qmp.c the PCS ready bit polling timeout is 1000us.
Would you mind give below change a try?

FYI, I tried the opposite change on my board (decrease the PCS polling 
timeout
used in phy-qcom-ufs.c), I did see PCS polling timeout, which is the 
same failure
you encountered.

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c 
b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 39e8deb..0ee9149 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -66,7 +66,7 @@
  /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
  #define CLAMP_EN                               BIT(0) /* enables i/o 
clamp_n */

-#define PHY_INIT_COMPLETE_TIMEOUT              1000
+#define PHY_INIT_COMPLETE_TIMEOUT              1000000
  #define POWER_DOWN_DELAY_US_MIN                        10
  #define POWER_DOWN_DELAY_US_MAX                        11

On 845, if there is 'rst'

Thanks.

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-15  6:09 [PATCH v5 0/7] UFS driver general fixes bundle 3 Can Guo
2019-11-15  6:09 ` [PATCH v5 1/7] scsi: ufs: Add device reset in link recovery path Can Guo
2019-11-15  6:25   ` Stanley Chu
2019-11-15  6:09 ` [PATCH v5 2/7] scsi: ufs-qcom: Add reset control support for host controller Can Guo
2019-11-15 10:08   ` Avri Altman
2019-12-16 19:04   ` Vinod Koul
2019-12-16 19:12     ` Jeffrey Hugo
2019-12-17  0:37       ` cang
2019-12-17  4:13         ` Vinod Koul
2019-12-17  7:10           ` cang
2019-12-17  9:24             ` Vinod Koul
2019-12-17 10:09               ` cang [this message]
2019-12-17 15:08                 ` Vinod Koul
2019-12-17 16:00                   ` Jeffrey Hugo
2019-12-17 18:44                     ` cang
2019-12-18  4:12                       ` Vinod Koul
2019-12-19  7:12                         ` cang
2019-12-19 14:21                           ` Vinod Koul
2019-12-19 14:25                             ` Jeffrey Hugo
2019-12-19 14:52                               ` Vinod Koul
2019-12-20  0:30                                 ` cang
2019-11-15  6:09 ` [PATCH v5 3/7] scsi: ufs: Fix up auto hibern8 enablement Can Guo
2019-11-15  6:35   ` Stanley Chu
2019-11-15  7:03     ` Can Guo
2019-11-15  7:18       ` Stanley Chu
2019-11-15 12:27         ` Can Guo
2019-11-15 13:46           ` Stanley Chu
2019-11-15  6:09 ` [PATCH v5 4/7] scsi: ufs: Fix register dump caused sleep in atomic context Can Guo
2019-11-15  6:09 ` [PATCH v5 5/7] scsi: ufs: Fix irq return code Can Guo
2019-11-15  6:09 ` [PATCH v5 6/7] scsi: ufs: Abort gating if clock on request is pending Can Guo
2019-11-15  6:09 ` [PATCH v5 7/7] scsi: ufs: Fix error handing during hibern8 enter Can Guo
2019-11-15 10:12   ` Avri Altman
2019-11-17 16:36   ` [EXT] " Bean Huo (beanhuo)
2019-11-19  4:28 ` [PATCH v5 0/7] UFS driver general fixes bundle 3 Martin K. Petersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fc8952a0eee5c010fe14e5f107d89e64@codeaurora.org \
    --to=cang@codeaurora.org \
    --cc=agross@kernel.org \
    --cc=alim.akhtar@samsung.com \
    --cc=asutoshd@codeaurora.org \
    --cc=avri.altman@wdc.com \
    --cc=jeffrey.l.hugo@gmail.com \
    --cc=jejb@linux.ibm.com \
    --cc=kernel-team@android.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=nguyenb@codeaurora.org \
    --cc=pedrom.sousa@synopsys.com \
    --cc=rnayak@codeaurora.org \
    --cc=salyzyn@google.com \
    --cc=saravanak@google.com \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-SCSI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-scsi/0 linux-scsi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-scsi linux-scsi/ https://lore.kernel.org/linux-scsi \
		linux-scsi@vger.kernel.org
	public-inbox-index linux-scsi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-scsi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git