Re: [PATCH v2 3/3] scsi: ufs: rockchip: init support for UFS

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Mon, Aug 12, 2024 at 09:28:26AM +0800, Shawn Lin wrote:
> JHi Mani,
> 
> 在 2024/8/10 17:28, Manivannan Sadhasivam 写道:
> > On Fri, Aug 09, 2024 at 04:16:41PM +0800, Shawn Lin wrote:
> > 
> > [...]
> > 
> > > > > +static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
> > > > > +					 enum ufs_notify_change_status status)
> > > > > +{
> > > > > +	int err = 0;
> > > > > +
> > > > > +	if (status == PRE_CHANGE) {
> > > > > +		int retry_outer = 3;
> > > > > +		int retry_inner;
> > > > > +start:
> > > > > +		if (ufshcd_is_hba_active(hba))
> > > > > +			/* change controller state to "reset state" */
> > > > > +			ufshcd_hba_stop(hba);
> > > > > +
> > > > > +		/* UniPro link is disabled at this point */
> > > > > +		ufshcd_set_link_off(hba);
> > > > > +
> > > > > +		/* start controller initialization sequence */
> > > > > +		ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE);
> > > > > +
> > > > > +		usleep_range(100, 200);
> > > > > +
> > > > > +		/* wait for the host controller to complete initialization */
> > > > > +		retry_inner = 50;
> > > > > +		while (!ufshcd_is_hba_active(hba)) {
> > > > > +			if (retry_inner) {
> > > > > +				retry_inner--;
> > > > > +			} else {
> > > > > +				dev_err(hba->dev,
> > > > > +					"Controller enable failed\n");
> > > > > +				if (retry_outer) {
> > > > > +					retry_outer--;
> > > > > +					goto start;
> > > > > +				}
> > > > > +				return -EIO;
> > > > > +			}
> > > > > +			usleep_range(1000, 1100);
> > > > > +		}
> > > > 
> > > > You just duplicated ufshcd_hba_execute_hce() here. Why? This doesn't make sense.
> > > 
> > > Since we set UFSHCI_QUIRK_BROKEN_HCE, and we also need to do someting
> > > which is very similar to ufshcd_hba_execute_hce(), before calling
> > > ufshcd_dme_reset(). Similar but not totally the same. I'll try to see if
> > > we can export ufshcd_hba_execute_hce() to make full use of it.
> > > 
> > 
> > But you are starting the controller using REG_CONTROLLER_ENABLE. Isn't that
> > supposed to be broken if you set UFSHCI_QUIRK_BROKEN_HCE? Or I am
> > misunderstanding the quirk?
> > 
> 
> Our controller doesn't work with exiting code, whether setting
> UFSHCI_QUIRK_BROKEN_HCE or not.
> 

Okay. Then this means you do not need this quirk at all.

> 
> For UFSHCI_QUIRK_BROKEN_HCE case, it calls ufshcd_dme_reset（）first,
> but we need to set REG_CONTROLLER_ENABLE first.
> 
> For !UFSHCI_QUIRK_BROKEN_HCE case, namly ufshcd_hba_execute_hce, it
> sets REG_CONTROLLER_ENABLE  first but never send DMA_RESET and calls
> ufshcd_dme_enable.
> 

I don't see where ufshcd_dme_enable() is getting called for
!UFSHCI_QUIRK_BROKEN_HCE case.

> So the closet code path is to go through UFSHCI_QUIRK_BROKEN_HCE case,
> and set REG_CONTROLLER_ENABLE by adding hce_enable_notify hook.
> 

No, that is abusing the quirk. But I'm confused about why your controller wants
resetting the unipro stack _after_ enabling the controller? Why can't it be
reset before?

> > > > 
> > > > > +	} else { /* POST_CHANGE */
> > > > > +		err = ufshcd_vops_phy_initialization(hba);
> > > > > +	}
> > > > > +
> > > > > +	return err;
> > > > > +}
> > > > > +

[...]

> > > > > +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
> > > > > +	SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, ufs_rockchip_resume)
> > > > > +	SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)
> > > > 
> > > > Why can't you use ufshcd PM ops as like other vendor drivers?
> > > 
> > > It doesn't work from the test. We have many use case to power down the
> > > controller and device, so there is no flow to recovery the link. Only
> > > when the first accessing to UFS fails, the ufshcd error handle recovery the
> > > link. This is not what we expect.
> > > 
> > 
> > What tests? The existing UFS controller drivers are used in production devices
> > and they never had a usecase to invent their own PM callbacks. So if your
> > controller is special, then you need to justify it more elaborately. If
> > something is missing in ufshcd callbacks, then we can add them.
> > 
> 
> All the register got lost each time as we power down both controller & PHY
> and devices in suspend.

Which suspend? runtime or system suspend? I believe system suspend.

> So we have to restore the necessary
> registers and link. I didn't see where the code recovery the controller
> settings in ufshcd_resume, except ufshcd_err_handler（）triggers that.
> Am I missing any thing? 

Can you explain what is causing the powerdown of the controller and PHY?
Because, ufshcd_suspend() just turns off the clocks and regulators (if
UFSHCD_CAP_AGGR_POWER_COLLAPSE is set) and spm_lvl 3 set by this driver only
puts the device in sleep mode and link in hibern8 state.

- Mani

> Below is the dump we get if using
> SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume).
> It can work as ufshcd_err_handler () will fix the link, but we have to
> suffer from getting the error log each time. Moreover, we need to gate
> 26MHz refclk for device when RPM is called. So our own rpm callback is
> needed.
> 
> [   14.318444] <<GTP-INF>>[gt1x_wakeup_sleep:964] Wakeup by poweron
> [   14.439723] ufshcd-rockchip 2a2d0000.ufs: Controller not ready to accept
> UIC commands
> [   14.439730] ufshcd-rockchip 2a2d0000.ufs: pwr ctrl cmd 0x18 with mode 0x0
> uic error -5
> [   14.439736] ufshcd-rockchip 2a2d0000.ufs: UFS Host state=1
> [   14.439740] ufshcd-rockchip 2a2d0000.ufs: outstanding reqs=0x0 tasks=0x0
> [   14.439744] ufshcd-rockchip 2a2d0000.ufs: saved_err=0x0,
> saved_uic_err=0x0
> [   14.439748] ufshcd-rockchip 2a2d0000.ufs: Device power mode=2, UIC link
> state=2
> [   14.439753] ufshcd-rockchip 2a2d0000.ufs: PM in progress=1, sys.
> suspended=1
> [   14.439758] ufshcd-rockchip 2a2d0000.ufs: Auto BKOPS=0, Host self-block=0
> [   14.439762] ufshcd-rockchip 2a2d0000.ufs: Clk gate=1
> [   14.439766] ufshcd-rockchip 2a2d0000.ufs: last_hibern8_exit_tstamp at 0
> us, hibern8_exit_cnt=0
> [   14.439770] ufshcd-rockchip 2a2d0000.ufs: last intr at 10807625 us, last
> intr status=0x440
> [   14.439775] ufshcd-rockchip 2a2d0000.ufs: error handling flags=0x0, req.
> abort count=0
> [   14.439779] ufshcd-rockchip 2a2d0000.ufs: hba->ufs_version=0x200, Host
> capabilities=0x187011f, caps=0x48c
> [   14.439785] ufshcd-rockchip 2a2d0000.ufs: quirks=0x2100, dev. quirks=0xc4
> [   14.439790] ufshcd-rockchip 2a2d0000.ufs: UFS dev info: SAMSUNG
> KLUDG2R1DE-B0F1  rev 0100
> [   14.439796] ufshcd-rockchip 2a2d0000.ufs: clk: core, rate: 50000000
> [   14.439822] host_regs: 00000000: 0187011f 00000000 00000200 00000000
> [   14.439827] host_regs: 00000010: 00000000 000005e6 00000000 00000000
> [   14.439831] host_regs: 00000020: 00000000 00000000 00000000 00000000
> [   14.439835] host_regs: 00000030: 00000000 00000000 00000000 00000000
> [   14.439839] host_regs: 00000040: 00000000 00000000 00000000 00000000
> [   14.439843] host_regs: 00000050: 00000000 00000000 00000000 00000000
> [   14.439847] host_regs: 00000060: 00000000 00000000 00000000 00000000
> [   14.439851] host_regs: 00000070: 00000000 00000000 00000000 00000000
> [   14.439855] host_regs: 00000080: 00000000 00000000 00000000 00000000
> [   14.439859] host_regs: 00000090: 00000000 00000000 00000000 00000000
> [   14.439863] ufshcd-rockchip 2a2d0000.ufs: No record of pa_err
> [   14.439867] ufshcd-rockchip 2a2d0000.ufs: No record of dl_err
> [   14.439871] ufshcd-rockchip 2a2d0000.ufs: No record of nl_err
> [   14.439876] ufshcd-rockchip 2a2d0000.ufs: No record of tl_err
> [   14.439880] ufshcd-rockchip 2a2d0000.ufs: No record of dme_err
> [   14.439884] ufshcd-rockchip 2a2d0000.ufs: No record of auto_hibern8_err
> [   14.439888] ufshcd-rockchip 2a2d0000.ufs: No record of fatal_err
> [   14.439892] ufshcd-rockchip 2a2d0000.ufs: No record of link_startup_fail
> [   14.439896] ufshcd-rockchip 2a2d0000.ufs: No record of resume_fail
> [   14.439900] ufshcd-rockchip 2a2d0000.ufs: No record of suspend_fail
> [   14.439905] ufshcd-rockchip 2a2d0000.ufs: dev_reset[0] = 0x0 at 1418763
> us
> [   14.439910] ufshcd-rockchip 2a2d0000.ufs: dev_reset: total cnt=1
> [   14.439914] ufshcd-rockchip 2a2d0000.ufs: No record of host_reset
> [   14.439918] ufshcd-rockchip 2a2d0000.ufs: No record of task_abort
> [   14.439930] ufshcd-rockchip 2a2d0000.ufs: ufshcd_uic_hibern8_exit:
> hibern8 exit failed. ret = -5
> [   14.439935] ufshcd-rockchip 2a2d0000.ufs: __ufshcd_wl_resume: hibern8
> exit failed -5
> [   14.439944] ufs_device_wlun 0:0:0:49488: ufshcd_wl_resume failed: -5
> [   14.439950] ufs_device_wlun 0:0:0:49488: PM: dpm_run_callback():
> scsi_bus_resume+0x0/0xa8 returns -5
> [   14.440003] ufshcd-rockchip 2a2d0000.ufs: ufshcd_err_handler started; HBA
> state eh_fatal; powered 1; shutting down 0; saved_err = 0; saved_uic_err =
> 0; force_reset = 0; link is broken
> [   14.440017] ufs_device_wlun 0:0:0:49488: PM: failed to resume async:
> error -5
> 
> > - Mani
> > 

-- 
மணிவண்ணன் சதாசிவம்