Date:   Sat, 9 Feb 2019 14:12:26 +0530
From:   Alim Akhtar <alim.akhtar@...sung.com>
To:     Jeffrey Hugo <jhugo@...eaurora.org>,
        Marc Gonzalez <marc.w.gonzalez@...e.fr>,
        MSM <linux-arm-msm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Evan Green <evgreen@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Avri Altman <avri.altman@....com>,
        Pedro Sousa <pedrom.sousa@...opsys.com>,
        Subhash Jadavani <subhashj@...eaurora.org>,
        Bart Van Assche <bvanassche@....org>,
        SCSI <linux-scsi@...r.kernel.org>, Hannes Reinecke <hare@...e.de>
Subject: Re: [PATCH v3 5/5] Revert
 "scsi: ufs: disable vccq if it's not needed by UFS device"



On 08/02/19 8:29 PM, Jeffrey Hugo wrote:
> On 2/8/2019 2:09 AM, Alim Akhtar wrote:
>> Hi Jeffrey,
>>
>> On 07/02/19 8:22 PM, Jeffrey Hugo wrote:
>>> On 2/7/2019 1:50 AM, Alim Akhtar wrote:
>>>> Hi Marc,
>>>>
>>>> On 06/02/19 9:22 PM, Marc Gonzalez wrote:
>>>>> On 06/02/2019 16:27, Alim Akhtar wrote:
>>>>>
>>>>>> On 06/02/19 8:29 PM, Marc Gonzalez wrote:
>>>>>>
>>>>>>> [    2.405734] regulator_disable: ENTER vdd_l26
>>>>>>> [    2.405958] regulator_disable: EXIT vdd_l26
>>>>>>> [    2.406032]   regulator_set_load: vdd_l26 = 0 uA
>>>>>>> [    3.930447] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode
>>>>>>> 0x04 for idn 13 failed, index 0, err = -11
>>>>>>> [    5.434358] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode
>>>>>>> 0x04 for idn 13 failed, index 0, err = -11
>>>>>>> [    6.938318] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode
>>>>>>> 0x04 for idn 13 failed, index 0, err = -11
>>>>>>> [    6.938414] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr_retry:
>>>>>>> query attribute, idn 13, failed with error -11 after 3 retires
>>>>>>> [    6.946959] ufshcd-qcom 1da4000.ufshc:
>>>>>>> ufshcd_disable_auto_bkops: failed to enable exception event -11
>>>>>>> [    6.958523] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id
>>>>>>> 0x1587 failed 3 retries
>>>>>>> [    6.967730] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id
>>>>>>> 0x1586 failed 3 retries
>>>>>>> [    6.975576] ufshcd-qcom 1da4000.ufshc: ufshcd_get_max_pwr_mode:
>>>>>>> invalid max pwm tx gear read = 0
>>>>>>> [    6.983306] ufshcd-qcom 1da4000.ufshc: ufshcd_probe_hba: Failed
>>>>>>> getting max supported power mode
>>>>>>> [    8.506314] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag:
>>>>>>> Sending flag query for idn 3 failed, err = -11
>>>>>>> [   10.010352] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag:
>>>>>>> Sending flag query for idn 3 failed, err = -11
>>>>>>> [   11.514313] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag:
>>>>>>> Sending flag query for idn 3 failed, err = -11
>>>>>>> [   11.514412] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag_retry:
>>>>>>> query attribute, opcode 5, idn 3, failed with error -11 after 3
>>>>>>> retires
>>>>>>> [   13.050354] ufshcd-qcom 1da4000.ufshc:
>>>>>>> __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0,
>>>>>>> err = -11
>>>>>>> [   14.554313] ufshcd-qcom 1da4000.ufshc:
>>>>>>> __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0,
>>>>>>> err = -11
>>>>>>> [   16.058313] ufshcd-qcom 1da4000.ufshc:
>>>>>>> __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0,
>>>>>>> err = -11
>>>>>>> [   16.058421] ufshcd-qcom 1da4000.ufshc: ufshcd_read_desc_param:
>>>>>>> Failed reading descriptor. desc_id 8, desc_index 0, param_offset 0,
>>>>>>> ret -11
>>>>>>> [   16.067654] ufshcd-qcom 1da4000.ufshc: ufshcd_init_icc_levels:
>>>>>>> Failed reading power descriptor.len = 98 ret = -11
>>>>>>> [   37.074334] ufshcd-qcom 1da4000.ufshc: link startup failed 1
>>>>>>
>>>>>> Can you check if your UFS device RESET_N is asserted correctly. It
>>>>>> might
>>>>>> be connected to some regulator and may be you can try keeping that
>>>>>> regulator as "regulator-always-on" from your DT node.
>>>>>
>>>>> How do I check RESET_N? In software or hardware?
>>>>>
>>>> RST_N is the reset logic for UFS device core logic and it is input to
>>>> the device from UFS host controller.So, in your platform please 
>>>> check if
>>>> this line somehow connected to (pulled up) a PMIC supply. If that is 
>>>> the
>>>> case, please keep that regulator ON and see if this issue is resolved.
>>>
>>> The reset line is routed though the global clock controller (GCC), and
>>> must be explicitly asserted within the GCC to trigger a reset.  As far
>>> as I am aware, Linux is not touching this.
>>>
>>> Additionally, I fail to see how if this was a reset issue, reverting
>>> 60f0187031c0 would have any impact (which doing so addresses our issue)
>>>
>> OK, that's again implementation dependent and your platform used that
>> way. My point was to make sure that reset part is ok, if reset/power is
>> not proper to the UFS device core logic this kind of issues comes.
> 
> We are following the Hardware Programming Guide written by the platform 
> designers with regard to UFS, including the reset logic.  I really don't 
> think the reset logic is an issue here.
> 
>>
>>>>> Do you think it is not a good idea to revert
>>>>> 60f0187031c05e04cbadffb62f557d0ff3564490 ?
>>>>>
>>>> Please hold on till we understand the real cause of this issue. Or we
>>>> have a consensuses for reverting the said commit.
>>>> Thanks!
>>>
>>> Did you see https://lkml.org/lkml/2019/2/5/659 where I indicated VCCQ
>>> powers components within the host controller, and by not setting load on
>>> the regulator properly, we are likely undervolting those components due
>>> to the current draw?
>>>
>> In theory may be true. But looks like we dont have a solid evidence yet
>> (correct me if I am wrong or misunderstood anything here
> The evidence seems simple.  We have properly described in DT all the 
> regulators that are consumed by the UFS host controller, and by 
> extension, the UFS storage chip as well.
> 
> By default, with no kernel changes, UFS does not work.
> 
> Marc and I debugged the issue, and found that the VCCQ regulator was not 
> being handled properly, and reverting the change we are discussing fixes 
> the the VCCQ regulator issue, and as a result UFS works.
> 
Ok, fair, before we revert this patch, Marc can you try below patch, or 
let me know if you have already tried this and share your 
result/observation:

diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi 
b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
index 50e9033aa7f6..b08e8d1ea0f3 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
@@ -212,6 +212,7 @@
                 vreg_l26a_1p2: l26 {
                         regulator-min-microvolt = <1200000>;
                         regulator-max-microvolt = <1200000>;
+                       regulator-always-on;
                 };
                 vreg_l28_3p0: l28 {
                         regulator-min-microvolt = <3008000>;
---

I believe "vreg_l26a_1p2" is supply for ufs's vccq.

check the result of this patch /wo reverting 
60f0187031c05e04cbadffb62f557d0ff3564490

> Again, despite the fact that we may have a Samsung UFS storage chip, 
> which triggers the quirk, the UFS host controller which talks to that 
> chip requires VCCQ, therefore this quirk breaks us.
> 
>> So that means its some short of hardware/board quirk, right?
> 
> No
> 
>> Can you please recheck the schematic and see what Bjorn is telling
>> (about having right entries in the DT for regulator) resolve your issue?
> 
> Already done.  The schematic defines vcc, vccq, and vccq2.  All of those 
> are listed in DT, and have been since Marc and I have been trying to 
> utilize UFS.
> 
>>
>> Marc, Can you disabled pmic on that board (hope your board boots with
>> default PMIC supply) and see if this issue still occurs?
> 
> The PMIC is required the boot the board.  I doubt the board will be 
> functional with the PMIC disabled.
> 
>> I am just trying to understand and see what is the real cause.
> 
> Our analysis is that VCCQ is required and 
> 60f0187031c05e04cbadffb62f557d0ff3564490 prevents the proper 
> configuration of VCCQ, thus a required resource (VCCQ) is not in the 
> proper state.
> 
Not in proper state or vccq regulator is disabled?
>>
>> @Yaniv Gardi, will you be able to comment on reason for adding
>> 60f0187031c05e04cbadffb62f557d0ff3564490 (any issue faced)?
>>
> 
> 

Powered by blists - more mailing lists