Re: [PATCH v3 5/5] Revert "scsi: ufs: disable vccq if it's not needed by UFS device"

[Alim Akhtar]

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)?
>>
> 
>