Messages in this thread | | | Date | Tue, 05 Jan 2021 18:06:12 +0800 | From | Can Guo <> | Subject | Re: [PATCH RFC v4 1/1] scsi: ufs: Fix ufs power down/on specs violation |
| |
On 2021-01-05 15:33, Adrian Hunter wrote: > On 5/01/21 9:28 am, Can Guo wrote: >> On 2021-01-05 15:16, Adrian Hunter wrote: >>> On 4/01/21 8:55 pm, Bjorn Andersson wrote: >>>> On Mon 04 Jan 03:15 CST 2021, Adrian Hunter wrote: >>>> >>>>> On 22/12/20 3:49 pm, Ziqi Chen wrote: >>>>>> As per specs, e.g, JESD220E chapter 7.2, while powering >>>>>> off/on the ufs device, RST_N signal and REF_CLK signal >>>>>> should be between VSS(Ground) and VCCQ/VCCQ2. >>>>>> >>>>>> To flexibly control device reset line, refactor the function >>>>>> ufschd_vops_device_reset(sturct ufs_hba *hba) to ufshcd_ >>>>>> vops_device_reset(sturct ufs_hba *hba, bool asserted). The >>>>>> new parameter "bool asserted" is used to separate device reset >>>>>> line pulling down from pulling up. >>>>> >>>>> This patch assumes the power is controlled by voltage regulators, >>>>> but >>>>> for us >>>>> it is controlled by firmware (ACPI), so it is not correct to change >>>>> RST_n >>>>> for all host controllers as you are doing. >>>>> >>>>> Also we might need to use a firmware interface for device reset, in >>>>> which >>>>> case the 'asserted' value doe not make sense. >>>>> >>>> >>>> Are you saying that the entire flip-flop-the-reset is a single >>>> firmware >>>> operation in your case? >>> >>> Yes >>> >>>> If you look at the Mediatek driver, the >>>> implementation of ufs_mtk_device_reset_ctrl() is a jump to firmware. >>>> >>>> >>>> But perhaps "asserted" isn't the appropriate English word for saying >>>> "the reset is in the resetting state"? >>>> >>>> I just wanted to avoid the use of "high"/"lo" as if you look at the >>>> Mediatek code they pass the expected line-level to the firmware, >>>> while >>>> in the Qualcomm code we pass the logical state to the GPIO code >>>> which is >>>> setup up as "active low" and thereby flip the meaning before hitting >>>> the >>>> pad. >>>> >>>>> Can we leave the device reset callback alone, and instead introduce >>>>> a new >>>>> variant operation for setting RST_n to match voltage regulator >>>>> power >>>>> changes? >>>> >>>> Wouldn't this new function just have to look like the proposed >>>> patches? >>>> In which case for existing platforms we'd have both? >>>> >>>> How would you implement this, or would you simply skip implementing >>>> this? >>> >>> Functionally, doing a device reset is not the same as adjusting >>> signal >>> levels to meet power up/off ramp requirements. However, the issue is >>> that >>> we do not use regulators, so the power is not necessarily being >>> changed at >>> those points, and we definitely do not want to reset instead of >>> entering >>> DeepSleep for example. >>> >>> Off the top of my head, I imagine something like a callback called >>> ufshcd_vops_prepare_power_ramp(hba, bool on) which is called only if >>> hba->vreg_info->vcc is not NULL. >> >> Hi Adrian, >> >> I don't see you have the vops device_reset() implemented anywhere in >> current code base, how is this change impacting you? Do I miss >> anything >> or are you planning to push a change which implements device_reset() >> soon? > > At some point, yes.
OK, then we don't even have to add a new vops, just go back to version #1 to use ufshcd_vops_suspend() to control the device_reset. We took the hard way because we wanted to fix it for all users.
| |