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

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

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?

> > 
> > > +	} else { /* POST_CHANGE */
> > > +		err = ufshcd_vops_phy_initialization(hba);
> > > +	}
> > > +
> > > +	return err;
> > > +}
> > > +
> > > +static void ufs_rockchip_set_pm_lvl(struct ufs_hba *hba)
> > > +{
> > > +	hba->rpm_lvl = UFS_PM_LVL_1;
> > > +	hba->spm_lvl = UFS_PM_LVL_3;
> > > +}
> > > +
> > > +static int ufs_rockchip_rk3576_phy_init(struct ufs_hba *hba)
> > > +{
> > > +	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> > > +
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(PA_LOCAL_TX_LCC_ENABLE, 0x0), 0x0);
> > > +	/* enable the mphy DME_SET cfg */
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x200, 0x0), 0x40);
> > > +	for (int i = 0; i < 2; i++) {
> > > +		/* Configuration M-TX */
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xaa, SEL_TX_LANE0 + i), 0x06);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xa9, SEL_TX_LANE0 + i), 0x02);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xad, SEL_TX_LANE0 + i), 0x44);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xac, SEL_TX_LANE0 + i), 0xe6);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xab, SEL_TX_LANE0 + i), 0x07);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x94, SEL_TX_LANE0 + i), 0x93);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x93, SEL_TX_LANE0 + i), 0xc9);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x7f, SEL_TX_LANE0 + i), 0x00);
> > > +		/* Configuration M-RX */
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x12, SEL_RX_LANE0 + i), 0x06);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x11, SEL_RX_LANE0 + i), 0x00);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1d, SEL_RX_LANE0 + i), 0x58);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1c, SEL_RX_LANE0 + i), 0x8c);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1b, SEL_RX_LANE0 + i), 0x02);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x25, SEL_RX_LANE0 + i), 0xf6);
> > > +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x2f, SEL_RX_LANE0 + i), 0x69);
> > > +	}
> > > +	/* disable the mphy DME_SET cfg */
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x200, 0x0), 0x00);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x80, 0x08C);
> > > +	ufs_sys_writel(host->mphy_base, 0xB5, 0x110);
> > > +	ufs_sys_writel(host->mphy_base, 0xB5, 0x250);
> > > +
> > 
> > Why can't you do these settings in a PHY driver?
> 
> As we have ->phy_initialization in struct ufs_hba_variant_ops,
> which asks the host driver to use it to initialize phys. It doesn't
> seem to need to create a whole new file to just add some smalls fixed
> parameters. :)
> 

So the PHY doesn't need any resources (clocks, regulators, etc...) other than
programming these sequences? If so, it is fine with me.

> 
> > 
> > > +	ufs_sys_writel(host->mphy_base, 0x03, 0x134);
> > > +	ufs_sys_writel(host->mphy_base, 0x03, 0x274);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x38, 0x0E0);
> > > +	ufs_sys_writel(host->mphy_base, 0x38, 0x220);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x50, 0x164);
> > > +	ufs_sys_writel(host->mphy_base, 0x50, 0x2A4);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x80, 0x178);
> > > +	ufs_sys_writel(host->mphy_base, 0x80, 0x2B8);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x18, 0x1B0);
> > > +	ufs_sys_writel(host->mphy_base, 0x18, 0x2F0);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x03, 0x128);
> > > +	ufs_sys_writel(host->mphy_base, 0x03, 0x268);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x20, 0x12C);
> > > +	ufs_sys_writel(host->mphy_base, 0x20, 0x26C);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0xC0, 0x120);
> > > +	ufs_sys_writel(host->mphy_base, 0xC0, 0x260);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x03, 0x094);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0x03, 0x1B4);
> > > +	ufs_sys_writel(host->mphy_base, 0x03, 0x2F4);
> > > +
> > > +	ufs_sys_writel(host->mphy_base, 0xC0, 0x08C);
> > > +	udelay(1);
> > > +	ufs_sys_writel(host->mphy_base, 0x00, 0x08C);
> > > +
> > > +	udelay(200);
> > > +	/* start link up */
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_TX_ENDIAN, 0), 0x0);
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_RX_ENDIAN, 0), 0x0);
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID, 0), 0x0);
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID_VALID, 0), 0x1);
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_PEERDEVICEID, 0), 0x1);
> > > +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_CONNECTIONSTATE, 0), 0x1);
> > > +
> > > +	return 0;
> > > +}
> > > +

[...]

> > > +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.

- Mani

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