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:

quoted

On 2021-01-05 15:16, Adrian Hunter wrote:

quoted

On 4/01/21 8:55 pm, Bjorn Andersson wrote:

quoted

On Mon 04 Jan 03:15 CST 2021, Adrian Hunter wrote:

quoted

On 22/12/20 3:49 pm, Ziqi Chen wrote:

quoted

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

quoted

                        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.

quoted

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.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek