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

Commit Message

Shawn Lin Aug. 8, 2024, 3:52 a.m. UTC

RK3576 contains a UFS controller, add init support fot it.

Signed-off-by: Shawn Lin <[e-mail]>

---

Changes in v2:
- use dev_probe_err
- remove ufs-phy-config-mode as it's not used
- drop of_match_ptr

 drivers/ufs/host/Kconfig        |  12 ++
 drivers/ufs/host/Makefile       |   1 +
 drivers/ufs/host/ufs-rockchip.c | 438 ++++++++++++++++++++++++++++++++++++++++
 drivers/ufs/host/ufs-rockchip.h |  51 +++++
 4 files changed, 502 insertions(+)
 create mode 100644 drivers/ufs/host/ufs-rockchip.c
 create mode 100644 drivers/ufs/host/ufs-rockchip.h

Comments

Krzysztof Kozlowski Aug. 8, 2024, 10:36 a.m. UTC | #1

On 08/08/2024 05:52, Shawn Lin wrote:
> RK3576 contains a UFS controller, add init support fot it.
> 
> Signed-off-by: Shawn Lin <[e-mail]>
> 

...

> +	err = clk_prepare_enable(host->ref_out_clk);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
> +
> +	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(host->rst_gpio)) {
> +		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
> +				"invalid reset-gpios property in node\n");
> +		err = PTR_ERR(host->rst_gpio);

No. Look at your code above - you have return dev_err_probe, so logical
is that the syntax is err = dev_err_probe. Don't over-complicate the code.

Anyway, this is suspicious. You already have resets with four resets
(!!!) and you claim you have fifth reset - GPIO? This looks just wrong,
like you represent some properties which do not belong here.

Where is your DTS so we can validate it?

Best regards,
Krzysztof

Krzysztof Kozlowski Aug. 8, 2024, 10:39 a.m. UTC | #2

On 08/08/2024 05:52, Shawn Lin wrote:
> RK3576 contains a UFS controller, add init support fot it.
> 
> Signed-off-by: Shawn Lin <[e-mail]>
> 


> +	reset_control_assert(host->rst);
> +	udelay(1);
> +	reset_control_deassert(host->rst);
> +
> +	host->ref_out_clk = devm_clk_get(dev, "ref_out");
> +	if (IS_ERR(host->ref_out_clk))
> +		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk), "ciu-drive not available\n");
> +
> +	err = clk_prepare_enable(host->ref_out_clk);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
> +
> +	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(host->rst_gpio)) {
> +		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
> +				"invalid reset-gpios property in node\n");
> +		err = PTR_ERR(host->rst_gpio);
> +		goto out;
> +	}
> +	udelay(20);
> +	gpiod_set_value_cansleep(host->rst_gpio, 1);

Also, why do you leave the device in the reset state? Logical one means
- reset is asserted. This applies to ufs_rockchip_device_reset() as well
- that's just wrong code.



Best regards,
Krzysztof

Shawn Lin Aug. 9, 2024, 12:53 a.m. UTC | #3

在 2024/8/8 18:36, Krzysztof Kozlowski 写道:
> On 08/08/2024 05:52, Shawn Lin wrote:
>> RK3576 contains a UFS controller, add init support fot it.
>>
>> Signed-off-by: Shawn Lin <[e-mail]>
>>
> 
> ...
> 
>> +	err = clk_prepare_enable(host->ref_out_clk);
>> +	if (err)
>> +		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
>> +
>> +	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(host->rst_gpio)) {
>> +		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
>> +				"invalid reset-gpios property in node\n");
>> +		err = PTR_ERR(host->rst_gpio);
> 
> No. Look at your code above - you have return dev_err_probe, so logical
> is that the syntax is err = dev_err_probe. Don't over-complicate the code.
> 
> Anyway, this is suspicious. You already have resets with four resets
> (!!!) and you claim you have fifth reset - GPIO? This looks just wrong,
> like you represent some properties which do not belong here.
> 

Thanks for the feadback.

Yes, we have 4 resets for controller, and one gpio to reset the
device. It happened to be called reset-gpios in DT but can be
any name if you like it to be. I added reset-gpios as a required one
listed in dt-bindings file, in patch 2.


 > Also, why do you leave the device in the reset state? Logical one
means
 > - reset is asserted. This applies to ufs_rockchip_device_reset() as > 
well
 > - that's just wrong code.


No, I tested it in linux-net and the output is HIGH, and leave the
device in active state.

In dts, we add:
reset-gpios = <&gpio4 RK_PD0 GPIO_ACTIVE_HIGH>;

so gpiod_set_value_cansleep(host->rst_gpio, 1) -> output HIGH for
gpio4_d0. Based on your comment, we should change the dts to use
GPIO_ACTIVE_LOW and then gpiod_set_value_cansleep（host->rst_gpio, 0）


Both can work, however IMO, isn't logical one means HIGH level in line 
more human readable?



> Where is your DTS so we can validate it?
> 

Will add it in the next version, as now the rk3576.dtsi is under
reviewed and UFS node was not added. So I was afraid to interfere
with that patch and just wanted to add incremental patch once
rk3576.dtsi got merged.

> Best regards,
> Krzysztof
>

Krzysztof Kozlowski Aug. 9, 2024, 5:44 a.m. UTC | #4

On 09/08/2024 02:53, Shawn Lin wrote:
> 在 2024/8/8 18:36, Krzysztof Kozlowski 写道:
>> On 08/08/2024 05:52, Shawn Lin wrote:
>>> RK3576 contains a UFS controller, add init support fot it.
>>>
>>> Signed-off-by: Shawn Lin <[e-mail]>
>>>
>>
>> ...
>>
>>> +	err = clk_prepare_enable(host->ref_out_clk);
>>> +	if (err)
>>> +		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
>>> +
>>> +	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
>>> +	if (IS_ERR(host->rst_gpio)) {
>>> +		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
>>> +				"invalid reset-gpios property in node\n");
>>> +		err = PTR_ERR(host->rst_gpio);
>>
>> No. Look at your code above - you have return dev_err_probe, so logical
>> is that the syntax is err = dev_err_probe. Don't over-complicate the code.
>>
>> Anyway, this is suspicious. You already have resets with four resets
>> (!!!) and you claim you have fifth reset - GPIO? This looks just wrong,
>> like you represent some properties which do not belong here.
>>
> 
> Thanks for the feadback.
> 
> Yes, we have 4 resets for controller, and one gpio to reset the
> device. It happened to be called reset-gpios in DT but can be
> any name if you like it to be. I added reset-gpios as a required one
> listed in dt-bindings file, in patch 2.

Then explain in the bindings what this reset-gpios is for.
> 
> 
>  > Also, why do you leave the device in the reset state? Logical one
> means
>  > - reset is asserted. This applies to ufs_rockchip_device_reset() as > 
> well
>  > - that's just wrong code.
> 
> 
> No, I tested it in linux-net and the output is HIGH, and leave the
> device in active state.
> 
> In dts, we add:
> reset-gpios = <&gpio4 RK_PD0 GPIO_ACTIVE_HIGH>;

? Don't use random values, what is type of the reset line?

> 
> so gpiod_set_value_cansleep(host->rst_gpio, 1) -> output HIGH for
> gpio4_d0. Based on your comment, we should change the dts to use
> GPIO_ACTIVE_LOW and then gpiod_set_value_cansleep（host->rst_gpio, 0）
> 
> 
> Both can work, however IMO, isn't logical one means HIGH level in line 
> more human readable?

You mix logical level and line level.

If this is reset GPIO, then 1 means asserted so your code is bogus.

Maybe the the line is active low?

> 
> 
> 
>> Where is your DTS so we can validate it?
>>
> 
> Will add it in the next version, as now the rk3576.dtsi is under
> reviewed and UFS node was not added. So I was afraid to interfere
> with that patch and just wanted to add incremental patch once
> rk3576.dtsi got merged.
> 
>> Best regards,
>> Krzysztof
>>

Best regards,
Krzysztof

Shawn Lin Aug. 9, 2024, 6:11 a.m. UTC | #5

在 2024/8/9 13:44, Krzysztof Kozlowski 写道:
> On 09/08/2024 02:53, Shawn Lin wrote:
>> 在 2024/8/8 18:36, Krzysztof Kozlowski 写道:
>>> On 08/08/2024 05:52, Shawn Lin wrote:
>>>> RK3576 contains a UFS controller, add init support fot it.
>>>>
>>>> Signed-off-by: Shawn Lin <[e-mail]>
>>>>
>>>
>>> ...
>>>
>>>> +	err = clk_prepare_enable(host->ref_out_clk);
>>>> +	if (err)
>>>> +		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
>>>> +
>>>> +	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
>>>> +	if (IS_ERR(host->rst_gpio)) {
>>>> +		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
>>>> +				"invalid reset-gpios property in node\n");
>>>> +		err = PTR_ERR(host->rst_gpio);
>>>
>>> No. Look at your code above - you have return dev_err_probe, so logical
>>> is that the syntax is err = dev_err_probe. Don't over-complicate the code.
>>>
>>> Anyway, this is suspicious. You already have resets with four resets
>>> (!!!) and you claim you have fifth reset - GPIO? This looks just wrong,
>>> like you represent some properties which do not belong here.
>>>
>>
>> Thanks for the feadback.
>>
>> Yes, we have 4 resets for controller, and one gpio to reset the
>> device. It happened to be called reset-gpios in DT but can be
>> any name if you like it to be. I added reset-gpios as a required one
>> listed in dt-bindings file, in patch 2.
> 
> Then explain in the bindings what this reset-gpios is for.

Got it.

>>
>>
>>   > Also, why do you leave the device in the reset state? Logical one
>> means
>>   > - reset is asserted. This applies to ufs_rockchip_device_reset() as >
>> well
>>   > - that's just wrong code.
>>
>>
>> No, I tested it in linux-net and the output is HIGH, and leave the
>> device in active state.
>>
>> In dts, we add:
>> reset-gpios = <&gpio4 RK_PD0 GPIO_ACTIVE_HIGH>;
> 
> ? Don't use random values, what is type of the reset line?
> 
>>
>> so gpiod_set_value_cansleep(host->rst_gpio, 1) -> output HIGH for
>> gpio4_d0. Based on your comment, we should change the dts to use
>> GPIO_ACTIVE_LOW and then gpiod_set_value_cansleep（host->rst_gpio, 0）
>>
>>
>> Both can work, however IMO, isn't logical one means HIGH level in line
>> more human readable?
> 
> You mix logical level and line level.
> 
> If this is reset GPIO, then 1 means asserted so your code is bogus.
> 
> Maybe the the line is active low?
> 

Yes, I mix the them. Will change to use active-low.

Thanks.

>>
>>
>>
>>> Where is your DTS so we can validate it?
>>>
>>
>> Will add it in the next version, as now the rk3576.dtsi is under
>> reviewed and UFS node was not added. So I was afraid to interfere
>> with that patch and just wanted to add incremental patch once
>> rk3576.dtsi got merged.
>>
>>> Best regards,
>>> Krzysztof
>>>
> 
> Best regards,
> Krzysztof
>

Manivannan Sadhasivam Aug. 9, 2024, 6:28 a.m. UTC | #6

On Thu, Aug 08, 2024 at 11:52:43AM +0800, Shawn Lin wrote:
> RK3576 contains a UFS controller, add init support fot it.
> 

This description is very simple. Please add more info like the UFSHCD version,
lane config, quirks and any other vendor specific difference.

> Signed-off-by: Shawn Lin <[e-mail]>
> 
> ---
> 
> Changes in v2:
> - use dev_probe_err
> - remove ufs-phy-config-mode as it's not used
> - drop of_match_ptr
> 
>  drivers/ufs/host/Kconfig        |  12 ++
>  drivers/ufs/host/Makefile       |   1 +
>  drivers/ufs/host/ufs-rockchip.c | 438 ++++++++++++++++++++++++++++++++++++++++
>  drivers/ufs/host/ufs-rockchip.h |  51 +++++
>  4 files changed, 502 insertions(+)
>  create mode 100644 drivers/ufs/host/ufs-rockchip.c
>  create mode 100644 drivers/ufs/host/ufs-rockchip.h
> 
> diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
> index 580c8d0..fafaa33 100644
> --- a/drivers/ufs/host/Kconfig
> +++ b/drivers/ufs/host/Kconfig
> @@ -142,3 +142,15 @@ config SCSI_UFS_SPRD
>  
>  	  Select this if you have UFS controller on Unisoc chipset.
>  	  If unsure, say N.
> +
> +config SCSI_UFS_ROCKCHIP
> +	tristate "Rockchip specific hooks to UFS controller platform driver"
> +	depends on SCSI_UFSHCD_PLATFORM && (ARCH_ROCKCHIP || COMPILE_TEST)
> +	help
> +	  This selects the Rockchip specific additions to UFSHCD platform driver.
> +	  UFS host on Rockchip needs some vendor specific configuration before
> +	  accessing the hardware which includes PHY configuration and vendor
> +	  specific registers.
> +
> +	  Select this if you have UFS controller on Rockchip chipset.
> +	  If unsure, say N.
> diff --git a/drivers/ufs/host/Makefile b/drivers/ufs/host/Makefile
> index 4573aea..2f97feb 100644
> --- a/drivers/ufs/host/Makefile
> +++ b/drivers/ufs/host/Makefile
> @@ -10,5 +10,6 @@ obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
>  obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
>  obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
>  obj-$(CONFIG_SCSI_UFS_RENESAS) += ufs-renesas.o
> +obj-$(CONFIG_SCSI_UFS_ROCKCHIP) += ufs-rockchip.o
>  obj-$(CONFIG_SCSI_UFS_SPRD) += ufs-sprd.o
>  obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
> diff --git a/drivers/ufs/host/ufs-rockchip.c b/drivers/ufs/host/ufs-rockchip.c
> new file mode 100644
> index 0000000..46c90d6
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-rockchip.c
> @@ -0,0 +1,438 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Rockchip UFS Host Controller driver
> + *
> + * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +#include <ufs/ufshcd.h>
> +#include <ufs/unipro.h>
> +#include "ufshcd-pltfrm.h"
> +#include "ufshcd-dwc.h"
> +#include "ufs-rockchip.h"
> +
> +static inline bool ufshcd_is_device_present(struct ufs_hba *hba)

No inline in .c file please.

> +{
> +	return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & DEVICE_PRESENT;
> +}
> +
> +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.

> +	} 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?

> +	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 int ufs_rockchip_common_init(struct ufs_hba *hba)
> +{
> +	struct device *dev = hba->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ufs_rockchip_host *host;
> +	int err = 0;
> +
> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	/* system control register for hci */
> +	host->ufs_sys_ctrl = devm_platform_ioremap_resource_byname(pdev, "hci_grf");
> +	if (IS_ERR(host->ufs_sys_ctrl))
> +		return dev_err_probe(dev, PTR_ERR(host->ufs_sys_ctrl),
> +					"cannot ioremap for hci system control register\n");
> +
> +	/* system control register for mphy */
> +	host->ufs_phy_ctrl = devm_platform_ioremap_resource_byname(pdev, "mphy_grf");
> +	if (IS_ERR(host->ufs_phy_ctrl))
> +		return dev_err_probe(dev, PTR_ERR(host->ufs_phy_ctrl),
> +				"cannot ioremap for mphy system control register\n");
> +
> +	/* mphy base register */
> +	host->mphy_base = devm_platform_ioremap_resource_byname(pdev, "mphy");
> +	if (IS_ERR(host->mphy_base))
> +		return dev_err_probe(dev, PTR_ERR(host->mphy_base),
> +					"cannot ioremap for mphy base register\n");
> +
> +	host->rst = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(host->rst))
> +		return dev_err_probe(dev, PTR_ERR(host->rst), "failed to get reset control\n");
> +
> +	reset_control_assert(host->rst);
> +	udelay(1);
> +	reset_control_deassert(host->rst);
> +
> +	host->ref_out_clk = devm_clk_get(dev, "ref_out");
> +	if (IS_ERR(host->ref_out_clk))
> +		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk), "ciu-drive not available\n");

What is 'ciu-drive'?

> +
> +	err = clk_prepare_enable(host->ref_out_clk);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
> +
> +	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(host->rst_gpio)) {
> +		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
> +				"invalid reset-gpios property in node\n");
> +		err = PTR_ERR(host->rst_gpio);

Krzysztof already pointed out this.

> +		goto out;
> +	}
> +	udelay(20);
> +	gpiod_set_value_cansleep(host->rst_gpio, 1);

Why do you need to assert device reset here? ufshcd driver will do it anyway.

> +
> +	host->clks[0].id = "core";
> +	host->clks[1].id = "pclk";
> +	host->clks[2].id = "pclk_mphy";
> +	err = devm_clk_bulk_get_optional(dev, UFS_MAX_CLKS, host->clks);
> +	if (err) {
> +		dev_err_probe(dev, err, "failed to get clocks\n");
> +		goto out;
> +	}
> +
> +	err = clk_bulk_prepare_enable(UFS_MAX_CLKS, host->clks);
> +	if (err) {
> +		dev_err_probe(dev, err, "failed to enable clocks\n");
> +		goto out;
> +	}
> +
> +	pm_runtime_set_active(&pdev->dev);

This is already called in ufshcd_pltfrm_init().

> +
> +	host->hba = hba;
> +	ufs_rockchip_set_pm_lvl(hba);
> +
> +	ufshcd_set_variant(hba, host);
> +
> +	return 0;
> +out:

s/out/disable_ref_clk

> +	clk_disable_unprepare(host->ref_out_clk);
> +	return err;
> +}
> +
> +static int ufs_rockchip_rk3576_init(struct ufs_hba *hba)
> +{
> +	int ret = 0;

Initialization not needed.

> +	struct device *dev = hba->dev;
> +

Also reverse Xmas order for local variables please.

> +	hba->quirks = UFSHCI_QUIRK_BROKEN_HCE | UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING;
> +
> +	/* Enable BKOPS when suspend */
> +	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
> +	/* Enable putting device into deep sleep */
> +	hba->caps |= UFSHCD_CAP_DEEPSLEEP;
> +	/* Enable devfreq of UFS */
> +	hba->caps |= UFSHCD_CAP_CLK_SCALING;
> +	/* Enable WriteBooster */
> +	hba->caps |= UFSHCD_CAP_WB_EN;
> +
> +	ret = ufs_rockchip_common_init(hba);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "ufs common init fail\n");
> +
> +	return 0;
> +}
> +
> +static int ufs_rockchip_device_reset(struct ufs_hba *hba)
> +{
> +	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> +
> +	if (!host->rst_gpio)
> +		return -EOPNOTSUPP;

Is it possible to hit this condition?

> +
> +	gpiod_set_value_cansleep(host->rst_gpio, 0);
> +	udelay(20);
> +
> +	gpiod_set_value_cansleep(host->rst_gpio, 1);
> +	udelay(20);
> +
> +	return 0;
> +}
> +
> +static const struct ufs_hba_variant_ops ufs_hba_rk3576_vops = {
> +	.name = "rk3576",
> +	.init = ufs_rockchip_rk3576_init,
> +	.device_reset = ufs_rockchip_device_reset,
> +	.hce_enable_notify = ufs_rockchip_hce_enable_notify,
> +	.phy_initialization = ufs_rockchip_rk3576_phy_init,
> +};
> +
> +static const struct of_device_id ufs_rockchip_of_match[] = {
> +	{ .compatible = "rockchip,rk3576-ufs", .data = &ufs_hba_rk3576_vops},

Use 'rockchip,rk3576-ufshc'.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ufs_rockchip_of_match);
> +
> +static int ufs_rockchip_probe(struct platform_device *pdev)
> +{
> +	int err = 0;

Again no init needed and use reverse Xmas order (everywhere).

> +	struct device *dev = &pdev->dev;
> +	const struct ufs_hba_variant_ops *vops;
> +
> +	vops = device_get_match_data(dev);

Is it OK if vops is NULL?

> +	err = ufshcd_pltfrm_init(pdev, vops);
> +	if (err)
> +		dev_err_probe(dev, err, "ufshcd_pltfrm_init failed\n");

Return err here and return 0 below.

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

> +	.prepare	 = ufshcd_suspend_prepare,
> +	.complete	 = ufshcd_resume_complete,
> +};
> +
> +static struct platform_driver ufs_rockchip_pltform = {
> +	.probe = ufs_rockchip_probe,
> +	.remove = ufs_rockchip_remove,
> +	.driver = {
> +		.name = "ufshcd-rockchip",
> +		.pm = &ufs_rockchip_pm_ops,
> +		.of_match_table = ufs_rockchip_of_match,
> +	},
> +};
> +module_platform_driver(ufs_rockchip_pltform);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Rockchip UFS Host Driver");
> diff --git a/drivers/ufs/host/ufs-rockchip.h b/drivers/ufs/host/ufs-rockchip.h
> new file mode 100644
> index 0000000..9eb80e8
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-rockchip.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Rockchip UFS Host Controller driver
> + *
> + * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
> + */
> +
> +#ifndef _UFS_ROCKCHIP_H_
> +#define _UFS_ROCKCHIP_H_
> +
> +#define UFS_MAX_CLKS 3
> +
> +#define SEL_TX_LANE0 0x0
> +#define SEL_TX_LANE1 0x1
> +#define SEL_TX_LANE2 0x2
> +#define SEL_TX_LANE3 0x3
> +#define SEL_RX_LANE0 0x4
> +#define SEL_RX_LANE1 0x5
> +#define SEL_RX_LANE2 0x6
> +#define SEL_RX_LANE3 0x7
> +
> +#define MIB_T_DBG_CPORT_TX_ENDIAN	0xc022
> +#define MIB_T_DBG_CPORT_RX_ENDIAN	0xc023
> +
> +struct ufs_rockchip_host {
> +	struct ufs_hba *hba;
> +	void __iomem *ufs_phy_ctrl;
> +	void __iomem *ufs_sys_ctrl;
> +	void __iomem *mphy_base;
> +	struct gpio_desc *rst_gpio;
> +	struct reset_control *rst;
> +	struct clk *ref_out_clk;
> +	struct clk_bulk_data clks[UFS_MAX_CLKS];
> +	uint64_t caps;
> +	bool in_suspend;

Move bool to the end to avoid holes.

- Mani

Shawn Lin Aug. 9, 2024, 8:16 a.m. UTC | #7

Hi Mani

Thanks for the comments.

在 2024/8/9 14:28, Manivannan Sadhasivam 写道:
> On Thu, Aug 08, 2024 at 11:52:43AM +0800, Shawn Lin wrote:
>> RK3576 contains a UFS controller, add init support fot it.
>>
> 
> This description is very simple. Please add more info like the UFSHCD version,
> lane config, quirks and any other vendor specific difference.
> 

Will improve.

>> Signed-off-by: Shawn Lin <[e-mail]>
>>
>> ---
>>
>> Changes in v2:
>> - use dev_probe_err
>> - remove ufs-phy-config-mode as it's not used
>> - drop of_match_ptr
>>
>>   drivers/ufs/host/Kconfig        |  12 ++
>>   drivers/ufs/host/Makefile       |   1 +
>>   drivers/ufs/host/ufs-rockchip.c | 438 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/ufs/host/ufs-rockchip.h |  51 +++++
>>   4 files changed, 502 insertions(+)
>>   create mode 100644 drivers/ufs/host/ufs-rockchip.c
>>   create mode 100644 drivers/ufs/host/ufs-rockchip.h
>>
>> diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
>> index 580c8d0..fafaa33 100644
>> --- a/drivers/ufs/host/Kconfig
>> +++ b/drivers/ufs/host/Kconfig
>> @@ -142,3 +142,15 @@ config SCSI_UFS_SPRD
>>   
>>   	  Select this if you have UFS controller on Unisoc chipset.
>>   	  If unsure, say N.
>> +
>> +config SCSI_UFS_ROCKCHIP
>> +	tristate "Rockchip specific hooks to UFS controller platform driver"
>> +	depends on SCSI_UFSHCD_PLATFORM && (ARCH_ROCKCHIP || COMPILE_TEST)
>> +	help
>> +	  This selects the Rockchip specific additions to UFSHCD platform driver.
>> +	  UFS host on Rockchip needs some vendor specific configuration before
>> +	  accessing the hardware which includes PHY configuration and vendor
>> +	  specific registers.
>> +
>> +	  Select this if you have UFS controller on Rockchip chipset.
>> +	  If unsure, say N.
>> diff --git a/drivers/ufs/host/Makefile b/drivers/ufs/host/Makefile
>> index 4573aea..2f97feb 100644
>> --- a/drivers/ufs/host/Makefile
>> +++ b/drivers/ufs/host/Makefile
>> @@ -10,5 +10,6 @@ obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
>>   obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
>>   obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
>>   obj-$(CONFIG_SCSI_UFS_RENESAS) += ufs-renesas.o
>> +obj-$(CONFIG_SCSI_UFS_ROCKCHIP) += ufs-rockchip.o
>>   obj-$(CONFIG_SCSI_UFS_SPRD) += ufs-sprd.o
>>   obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
>> diff --git a/drivers/ufs/host/ufs-rockchip.c b/drivers/ufs/host/ufs-rockchip.c
>> new file mode 100644
>> index 0000000..46c90d6
>> --- /dev/null
>> +++ b/drivers/ufs/host/ufs-rockchip.c
>> @@ -0,0 +1,438 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Rockchip UFS Host Controller driver
>> + *
>> + * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/gpio.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +#include <ufs/ufshcd.h>
>> +#include <ufs/unipro.h>
>> +#include "ufshcd-pltfrm.h"
>> +#include "ufshcd-dwc.h"
>> +#include "ufs-rockchip.h"
>> +
>> +static inline bool ufshcd_is_device_present(struct ufs_hba *hba)
> 
> No inline in .c file please.

ok.

> 
>> +{
>> +	return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & DEVICE_PRESENT;
>> +}
>> +
>> +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.

> 
>> +	} 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. :)


> 
>> +	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 int ufs_rockchip_common_init(struct ufs_hba *hba)
>> +{
>> +	struct device *dev = hba->dev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ufs_rockchip_host *host;
>> +	int err = 0;
>> +
>> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
>> +	if (!host)
>> +		return -ENOMEM;
>> +
>> +	/* system control register for hci */
>> +	host->ufs_sys_ctrl = devm_platform_ioremap_resource_byname(pdev, "hci_grf");
>> +	if (IS_ERR(host->ufs_sys_ctrl))
>> +		return dev_err_probe(dev, PTR_ERR(host->ufs_sys_ctrl),
>> +					"cannot ioremap for hci system control register\n");
>> +
>> +	/* system control register for mphy */
>> +	host->ufs_phy_ctrl = devm_platform_ioremap_resource_byname(pdev, "mphy_grf");
>> +	if (IS_ERR(host->ufs_phy_ctrl))
>> +		return dev_err_probe(dev, PTR_ERR(host->ufs_phy_ctrl),
>> +				"cannot ioremap for mphy system control register\n");
>> +
>> +	/* mphy base register */
>> +	host->mphy_base = devm_platform_ioremap_resource_byname(pdev, "mphy");
>> +	if (IS_ERR(host->mphy_base))
>> +		return dev_err_probe(dev, PTR_ERR(host->mphy_base),
>> +					"cannot ioremap for mphy base register\n");
>> +
>> +	host->rst = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(host->rst))
>> +		return dev_err_probe(dev, PTR_ERR(host->rst), "failed to get reset control\n");
>> +
>> +	reset_control_assert(host->rst);
>> +	udelay(1);
>> +	reset_control_deassert(host->rst);
>> +
>> +	host->ref_out_clk = devm_clk_get(dev, "ref_out");
>> +	if (IS_ERR(host->ref_out_clk))
>> +		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk), "ciu-drive not available\n");
> 
> What is 'ciu-drive'?

Will fix.

> 
>> +
>> +	err = clk_prepare_enable(host->ref_out_clk);
>> +	if (err)
>> +		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
>> +
>> +	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(host->rst_gpio)) {
>> +		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
>> +				"invalid reset-gpios property in node\n");
>> +		err = PTR_ERR(host->rst_gpio);
> 
> Krzysztof already pointed out this.
> 
>> +		goto out;
>> +	}
>> +	udelay(20);
>> +	gpiod_set_value_cansleep(host->rst_gpio, 1);
> 
> Why do you need to assert device reset here? ufshcd driver will do it anyway.
> 

Yes, I see ufshcd_init（）will do that now. Will improve.

>> +
>> +	host->clks[0].id = "core";
>> +	host->clks[1].id = "pclk";
>> +	host->clks[2].id = "pclk_mphy";
>> +	err = devm_clk_bulk_get_optional(dev, UFS_MAX_CLKS, host->clks);
>> +	if (err) {
>> +		dev_err_probe(dev, err, "failed to get clocks\n");
>> +		goto out;
>> +	}
>> +
>> +	err = clk_bulk_prepare_enable(UFS_MAX_CLKS, host->clks);
>> +	if (err) {
>> +		dev_err_probe(dev, err, "failed to enable clocks\n");
>> +		goto out;
>> +	}
>> +
>> +	pm_runtime_set_active(&pdev->dev);
> 
> This is already called in ufshcd_pltfrm_init().

Ok.

> 
>> +
>> +	host->hba = hba;
>> +	ufs_rockchip_set_pm_lvl(hba);
>> +
>> +	ufshcd_set_variant(hba, host);
>> +
>> +	return 0;
>> +out:
> 
> s/out/disable_ref_clk

Will fix.

> 
>> +	clk_disable_unprepare(host->ref_out_clk);
>> +	return err;
>> +}
>> +
>> +static int ufs_rockchip_rk3576_init(struct ufs_hba *hba)
>> +{
>> +	int ret = 0;
> 
> Initialization not needed.
> 
>> +	struct device *dev = hba->dev;
>> +
> 
> Also reverse Xmas order for local variables please.

Will improve, as blow the same.

> 
>> +	hba->quirks = UFSHCI_QUIRK_BROKEN_HCE | UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING;
>> +
>> +	/* Enable BKOPS when suspend */
>> +	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>> +	/* Enable putting device into deep sleep */
>> +	hba->caps |= UFSHCD_CAP_DEEPSLEEP;
>> +	/* Enable devfreq of UFS */
>> +	hba->caps |= UFSHCD_CAP_CLK_SCALING;
>> +	/* Enable WriteBooster */
>> +	hba->caps |= UFSHCD_CAP_WB_EN;
>> +
>> +	ret = ufs_rockchip_common_init(hba);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "ufs common init fail\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int ufs_rockchip_device_reset(struct ufs_hba *hba)
>> +{
>> +	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>> +
>> +	if (!host->rst_gpio)
>> +		return -EOPNOTSUPP;
> 
> Is it possible to hit this condition?
> 

No. We ask the BSP to provide reset pin as a must one. Will remove.

>> +
>> +	gpiod_set_value_cansleep(host->rst_gpio, 0);
>> +	udelay(20);
>> +
>> +	gpiod_set_value_cansleep(host->rst_gpio, 1);
>> +	udelay(20);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct ufs_hba_variant_ops ufs_hba_rk3576_vops = {
>> +	.name = "rk3576",
>> +	.init = ufs_rockchip_rk3576_init,
>> +	.device_reset = ufs_rockchip_device_reset,
>> +	.hce_enable_notify = ufs_rockchip_hce_enable_notify,
>> +	.phy_initialization = ufs_rockchip_rk3576_phy_init,
>> +};
>> +
>> +static const struct of_device_id ufs_rockchip_of_match[] = {
>> +	{ .compatible = "rockchip,rk3576-ufs", .data = &ufs_hba_rk3576_vops},
> 
> Use 'rockchip,rk3576-ufshc'.

ok.

> 
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ufs_rockchip_of_match);
>> +
>> +static int ufs_rockchip_probe(struct platform_device *pdev)
>> +{
>> +	int err = 0;
> 
> Again no init needed and use reverse Xmas order (everywhere).
> 
>> +	struct device *dev = &pdev->dev;
>> +	const struct ufs_hba_variant_ops *vops;
>> +
>> +	vops = device_get_match_data(dev);
> 
> Is it OK if vops is NULL?

Will check.

> 
>> +	err = ufshcd_pltfrm_init(pdev, vops);
>> +	if (err)
>> +		dev_err_probe(dev, err, "ufshcd_pltfrm_init failed\n");
> 
> Return err here and return 0 below.
> 

Ok.

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


> 
>> +	.prepare	 = ufshcd_suspend_prepare,
>> +	.complete	 = ufshcd_resume_complete,
>> +};
>> +
>> +static struct platform_driver ufs_rockchip_pltform = {
>> +	.probe = ufs_rockchip_probe,
>> +	.remove = ufs_rockchip_remove,
>> +	.driver = {
>> +		.name = "ufshcd-rockchip",
>> +		.pm = &ufs_rockchip_pm_ops,
>> +		.of_match_table = ufs_rockchip_of_match,
>> +	},
>> +};
>> +module_platform_driver(ufs_rockchip_pltform);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Rockchip UFS Host Driver");
>> diff --git a/drivers/ufs/host/ufs-rockchip.h b/drivers/ufs/host/ufs-rockchip.h
>> new file mode 100644
>> index 0000000..9eb80e8
>> --- /dev/null
>> +++ b/drivers/ufs/host/ufs-rockchip.h
>> @@ -0,0 +1,51 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Rockchip UFS Host Controller driver
>> + *
>> + * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
>> + */
>> +
>> +#ifndef _UFS_ROCKCHIP_H_
>> +#define _UFS_ROCKCHIP_H_
>> +
>> +#define UFS_MAX_CLKS 3
>> +
>> +#define SEL_TX_LANE0 0x0
>> +#define SEL_TX_LANE1 0x1
>> +#define SEL_TX_LANE2 0x2
>> +#define SEL_TX_LANE3 0x3
>> +#define SEL_RX_LANE0 0x4
>> +#define SEL_RX_LANE1 0x5
>> +#define SEL_RX_LANE2 0x6
>> +#define SEL_RX_LANE3 0x7
>> +
>> +#define MIB_T_DBG_CPORT_TX_ENDIAN	0xc022
>> +#define MIB_T_DBG_CPORT_RX_ENDIAN	0xc023
>> +
>> +struct ufs_rockchip_host {
>> +	struct ufs_hba *hba;
>> +	void __iomem *ufs_phy_ctrl;
>> +	void __iomem *ufs_sys_ctrl;
>> +	void __iomem *mphy_base;
>> +	struct gpio_desc *rst_gpio;
>> +	struct reset_control *rst;
>> +	struct clk *ref_out_clk;
>> +	struct clk_bulk_data clks[UFS_MAX_CLKS];
>> +	uint64_t caps;
>> +	bool in_suspend;
> 
> Move bool to the end to avoid holes.
> 
> - Mani
>

Bart Van Assche Aug. 9, 2024, 7:06 p.m. UTC | #8

On 8/7/24 8:52 PM, Shawn Lin wrote:
> RK3576 contains a UFS controller, add init support fot it.
					^^^^	     ^^^
                                         initial      for

Again a very short patch description. What is "RK3576"? Please explain.

> +config SCSI_UFS_ROCKCHIP
> +	tristate "Rockchip specific hooks to UFS controller platform driver"

A better description would be: "Rockchip UFS host controller driver"

> +#include "ufshcd-dwc.h"

No, you should not include the ufshcd-dwc.h header file. That is a 
header file for the Designware UFS host controller.

> +	reset_control_assert(host->rst);
> +	udelay(1);
> +	reset_control_deassert(host->rst);

Why udelay() instead of usleep_range()?

> +static int ufs_rockchip_device_reset(struct ufs_hba *hba)
> +{
> +	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> +
> +	if (!host->rst_gpio)
> +		return -EOPNOTSUPP;
> +
> +	gpiod_set_value_cansleep(host->rst_gpio, 0);
> +	udelay(20);
> +
> +	gpiod_set_value_cansleep(host->rst_gpio, 1);
> +	udelay(20);
> +
> +	return 0;
> +}

Same question here: why udelay() instead of usleep_range()?

Thanks,

Bart.

Manivannan Sadhasivam Aug. 10, 2024, 9:28 a.m. UTC | #9

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

Shawn Lin Aug. 12, 2024, 1:28 a.m. UTC | #10

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.


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.

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.

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

yes, it needn't.

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

All the register got lost each time as we power down both controller & 
PHY and devices in 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? 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
>

Manivannan Sadhasivam Aug. 12, 2024, 4:10 a.m. UTC | #11

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

Shawn Lin Aug. 12, 2024, 6:24 a.m. UTC | #12

在 2024/8/12 12:10, Manivannan Sadhasivam 写道:
> 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?
> 

It can't be. The DME_RESET to reset the unipro stack will be failed
without enabling REG_CONTROLLER_ENABLE. And the controller does want us
to reset the unipro stack before other coming UICs.

So I considered it's a kind of broken HCE case as well. Should I add a
new quirk or add a new hba_enable hook in ufs_hba_variant_ops? Or just
use UFSHCI_QUIRK_BROKEN_HCE ?

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

Both.

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

For runtime PM case, it's the power-domain driver will power down the
controller and PHY if UFS stack is not active any more（autosuspend）.

For system PM case, it's the SoC's firmware to cutting of all the power
for controller/PHY and device.

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

Manivannan Sadhasivam Aug. 12, 2024, 4:55 p.m. UTC | #13

On Mon, Aug 12, 2024 at 02:24:31PM +0800, Shawn Lin wrote:
> 在 2024/8/12 12:10, Manivannan Sadhasivam 写道:
> > 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?
> > 
> 
> It can't be. The DME_RESET to reset the unipro stack will be failed
> without enabling REG_CONTROLLER_ENABLE. And the controller does want us
> to reset the unipro stack before other coming UICs.
> 
> So I considered it's a kind of broken HCE case as well. Should I add a
> new quirk or add a new hba_enable hook in ufs_hba_variant_ops? Or just
> use UFSHCI_QUIRK_BROKEN_HCE ?
> 

IMO, you should add a new quirk and use it directly in ufshcd_hba_execute_hce().
But you need to pick the quirk name as per the actual quirky behavior of the
controller.

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

With {rpm/spm}_lvl = 3, you should not power down the controller.

> > 
> > > 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.
> > 
> 
> For runtime PM case, it's the power-domain driver will power down the
> controller and PHY if UFS stack is not active any more（autosuspend）.
> 
> For system PM case, it's the SoC's firmware to cutting of all the power
> for controller/PHY and device.
> 

Both cases are not matching the expectations of {rpm/spm}_lvl. So the platform
(power domain or the firmware) should be fixed. What if the user sets the
{rpm/spm}_lvl to 1? Will the platform power down the controller even then? If
so, then I'd say that the platform is broken and should be fixed.

- Mani

Shawn Lin Aug. 13, 2024, 3:52 a.m. UTC | #14

Hi Mani,

在 2024/8/13 0:55, Manivannan Sadhasivam 写道:
> On Mon, Aug 12, 2024 at 02:24:31PM +0800, Shawn Lin wrote:
>> 在 2024/8/12 12:10, Manivannan Sadhasivam 写道:
>>> 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?
>>>
>>
>> It can't be. The DME_RESET to reset the unipro stack will be failed
>> without enabling REG_CONTROLLER_ENABLE. And the controller does want us
>> to reset the unipro stack before other coming UICs.
>>
>> So I considered it's a kind of broken HCE case as well. Should I add a
>> new quirk or add a new hba_enable hook in ufs_hba_variant_ops? Or just
>> use UFSHCI_QUIRK_BROKEN_HCE ?
>>
> 
> IMO, you should add a new quirk and use it directly in ufshcd_hba_execute_hce().
> But you need to pick the quirk name as per the actual quirky behavior of the
> controller.
> 

Thanks, Main. I'll add a new quirk for
ufshcd_hba_execute_hce() as per the actual quirky behavour.

>>>>>>>
>>>>>>>> +	} 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.
>>
>> Both.
>>
> 
> With {rpm/spm}_lvl = 3, you should not power down the controller.
> 
>>>
>>>> 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.
>>>
>>
>> For runtime PM case, it's the power-domain driver will power down the
>> controller and PHY if UFS stack is not active any more（autosuspend）.
>>
>> For system PM case, it's the SoC's firmware to cutting of all the power
>> for controller/PHY and device.
>>
> 
> Both cases are not matching the expectations of {rpm/spm}_lvl. So the platform
> (power domain or the firmware) should be fixed. What if the user sets the
> {rpm/spm}_lvl to 1? Will the platform power down the controller even then? If
> so, then I'd say that the platform is broken and should be fixed.

Ok, it seems I need to set {rpm/spm}_lvl = 6 if I want platform to power
down the controller for ultra power-saving. But I still need to add my
own system PM callback in that case to recovery the link first. Do I
misunderstand it?

And for the user who sets the rpm/spm level via
ufs_sysfs_pm_lvl_store(), I think there is no way to block it currently,
except that we need to fix the power-domain driver and Firmware to
respect the level and choose correct policy.


So in summary for what the next step I should to:
(1) Set {rpm/spm}_lvl = 6 in host driver to reflect the expectation
(2) Add own PM callbacks to recovery the link to meet the expectation
(3) Fix the broken behaviour of PD or Firmware to respect the actual
desired pm level if user changes the pm level.


Does that sound feasible to you?


Thanks.

> 
> - Mani
>

Shawn Lin Aug. 13, 2024, 7:22 a.m. UTC | #15

在 2024/8/13 11:52, Shawn Lin 写道:
> Hi Mani,
> 
> 在 2024/8/13 0:55, Manivannan Sadhasivam 写道:
>> On Mon, Aug 12, 2024 at 02:24:31PM +0800, Shawn Lin wrote:
>>> 在 2024/8/12 12:10, Manivannan Sadhasivam 写道:
>>>> 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?
>>>>
>>>
>>> It can't be. The DME_RESET to reset the unipro stack will be failed
>>> without enabling REG_CONTROLLER_ENABLE. And the controller does want us
>>> to reset the unipro stack before other coming UICs.
>>>
>>> So I considered it's a kind of broken HCE case as well. Should I add a
>>> new quirk or add a new hba_enable hook in ufs_hba_variant_ops? Or just
>>> use UFSHCI_QUIRK_BROKEN_HCE ?
>>>
>>
>> IMO, you should add a new quirk and use it directly in 
>> ufshcd_hba_execute_hce().
>> But you need to pick the quirk name as per the actual quirky behavior 
>> of the
>> controller.
>>
> 
> Thanks, Main. I'll add a new quirk for
> ufshcd_hba_execute_hce() as per the actual quirky behavour.
> 
>>>>>>>>
>>>>>>>>> +    } 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.
>>>
>>> Both.
>>>
>>
>> With {rpm/spm}_lvl = 3, you should not power down the controller.
>>
>>>>
>>>>> 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.
>>>>
>>>
>>> For runtime PM case, it's the power-domain driver will power down the
>>> controller and PHY if UFS stack is not active any more（autosuspend）.
>>>
>>> For system PM case, it's the SoC's firmware to cutting of all the power
>>> for controller/PHY and device.
>>>
>>
>> Both cases are not matching the expectations of {rpm/spm}_lvl. So the 
>> platform
>> (power domain or the firmware) should be fixed. What if the user sets the
>> {rpm/spm}_lvl to 1? Will the platform power down the controller even 
>> then? If
>> so, then I'd say that the platform is broken and should be fixed.
> 
> Ok, it seems I need to set {rpm/spm}_lvl = 6 if I want platform to power
> down the controller for ultra power-saving. But I still need to add my
> own system PM callback in that case to recovery the link first. Do I
> misunderstand it?
> 
> And for the user who sets the rpm/spm level via
> ufs_sysfs_pm_lvl_store(), I think there is no way to block it currently,
> except that we need to fix the power-domain driver and Firmware to
> respect the level and choose correct policy.
> 
> 
> So in summary for what the next step I should to:
> (1) Set {rpm/spm}_lvl = 6 in host driver to reflect the expectation
> (2) Add own PM callbacks to recovery the link to meet the expectation
> (3) Fix the broken behaviour of PD or Firmware to respect the actual
> desired pm level if user changes the pm level.
> 
> 

Sorry, I misunderstood your comment, so the action should be
(1) Set {rpm/spm}_lvl = 5 in host driver to reflect the expectation
(2) Use ufshcd_system_suspend/resume, but keep our own runtime PM
callbacks as we need a extra step to gate refclk.
(3) Fix the broken behaviour of PD or Firmware to respect the actual
desired pm level if user changes the pm level.

> Does that sound feasible to you?
> 
> 
> Thanks.
> 
>>
>> - Mani
>>

Manivannan Sadhasivam Aug. 16, 2024, 7:52 a.m. UTC | #16

On Tue, Aug 13, 2024 at 03:22:45PM +0800, Shawn Lin wrote:

[...]

> > > > For runtime PM case, it's the power-domain driver will power down the
> > > > controller and PHY if UFS stack is not active any more（autosuspend）.
> > > > 
> > > > For system PM case, it's the SoC's firmware to cutting of all the power
> > > > for controller/PHY and device.
> > > > 
> > > 
> > > Both cases are not matching the expectations of {rpm/spm}_lvl. So
> > > the platform
> > > (power domain or the firmware) should be fixed. What if the user sets the
> > > {rpm/spm}_lvl to 1? Will the platform power down the controller even
> > > then? If
> > > so, then I'd say that the platform is broken and should be fixed.
> > 
> > Ok, it seems I need to set {rpm/spm}_lvl = 6 if I want platform to power
> > down the controller for ultra power-saving. But I still need to add my
> > own system PM callback in that case to recovery the link first. Do I
> > misunderstand it?
> > 
> > And for the user who sets the rpm/spm level via
> > ufs_sysfs_pm_lvl_store(), I think there is no way to block it currently,
> > except that we need to fix the power-domain driver and Firmware to
> > respect the level and choose correct policy.
> > 
> > 
> > So in summary for what the next step I should to:
> > (1) Set {rpm/spm}_lvl = 6 in host driver to reflect the expectation
> > (2) Add own PM callbacks to recovery the link to meet the expectation
> > (3) Fix the broken behaviour of PD or Firmware to respect the actual
> > desired pm level if user changes the pm level.
> > 
> > 
> 
> Sorry, I misunderstood your comment, so the action should be
> (1) Set {rpm/spm}_lvl = 5 in host driver to reflect the expectation
> (2) Use ufshcd_system_suspend/resume, but keep our own runtime PM
> callbacks as we need a extra step to gate refclk.

Ok.

> (3) Fix the broken behaviour of PD or Firmware to respect the actual
> desired pm level if user changes the pm level.

If you do this, then you don't need (1), don't you?

- Mani

Patch

diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
index 580c8d0..fafaa33 100644
--- a/drivers/ufs/host/Kconfig
+++ b/drivers/ufs/host/Kconfig
@@ -142,3 +142,15 @@  config SCSI_UFS_SPRD
 
 	  Select this if you have UFS controller on Unisoc chipset.
 	  If unsure, say N.
+
+config SCSI_UFS_ROCKCHIP
+	tristate "Rockchip specific hooks to UFS controller platform driver"
+	depends on SCSI_UFSHCD_PLATFORM && (ARCH_ROCKCHIP || COMPILE_TEST)
+	help
+	  This selects the Rockchip specific additions to UFSHCD platform driver.
+	  UFS host on Rockchip needs some vendor specific configuration before
+	  accessing the hardware which includes PHY configuration and vendor
+	  specific registers.
+
+	  Select this if you have UFS controller on Rockchip chipset.
+	  If unsure, say N.
diff --git a/drivers/ufs/host/Makefile b/drivers/ufs/host/Makefile
index 4573aea..2f97feb 100644
--- a/drivers/ufs/host/Makefile
+++ b/drivers/ufs/host/Makefile
@@ -10,5 +10,6 @@  obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
 obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
 obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
 obj-$(CONFIG_SCSI_UFS_RENESAS) += ufs-renesas.o
+obj-$(CONFIG_SCSI_UFS_ROCKCHIP) += ufs-rockchip.o
 obj-$(CONFIG_SCSI_UFS_SPRD) += ufs-sprd.o
 obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
diff --git a/drivers/ufs/host/ufs-rockchip.c b/drivers/ufs/host/ufs-rockchip.c
new file mode 100644
index 0000000..46c90d6
--- /dev/null
+++ b/drivers/ufs/host/ufs-rockchip.c
@@ -0,0 +1,438 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Rockchip UFS Host Controller driver
+ *
+ * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include <ufs/ufshcd.h>
+#include <ufs/unipro.h>
+#include "ufshcd-pltfrm.h"
+#include "ufshcd-dwc.h"
+#include "ufs-rockchip.h"
+
+static inline bool ufshcd_is_device_present(struct ufs_hba *hba)
+{
+	return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & DEVICE_PRESENT;
+}
+
+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);
+		}
+	} 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);
+
+	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 int ufs_rockchip_common_init(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ufs_rockchip_host *host;
+	int err = 0;
+
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+
+	/* system control register for hci */
+	host->ufs_sys_ctrl = devm_platform_ioremap_resource_byname(pdev, "hci_grf");
+	if (IS_ERR(host->ufs_sys_ctrl))
+		return dev_err_probe(dev, PTR_ERR(host->ufs_sys_ctrl),
+					"cannot ioremap for hci system control register\n");
+
+	/* system control register for mphy */
+	host->ufs_phy_ctrl = devm_platform_ioremap_resource_byname(pdev, "mphy_grf");
+	if (IS_ERR(host->ufs_phy_ctrl))
+		return dev_err_probe(dev, PTR_ERR(host->ufs_phy_ctrl),
+				"cannot ioremap for mphy system control register\n");
+
+	/* mphy base register */
+	host->mphy_base = devm_platform_ioremap_resource_byname(pdev, "mphy");
+	if (IS_ERR(host->mphy_base))
+		return dev_err_probe(dev, PTR_ERR(host->mphy_base),
+					"cannot ioremap for mphy base register\n");
+
+	host->rst = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR(host->rst))
+		return dev_err_probe(dev, PTR_ERR(host->rst), "failed to get reset control\n");
+
+	reset_control_assert(host->rst);
+	udelay(1);
+	reset_control_deassert(host->rst);
+
+	host->ref_out_clk = devm_clk_get(dev, "ref_out");
+	if (IS_ERR(host->ref_out_clk))
+		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk), "ciu-drive not available\n");
+
+	err = clk_prepare_enable(host->ref_out_clk);
+	if (err)
+		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
+
+	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(host->rst_gpio)) {
+		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
+				"invalid reset-gpios property in node\n");
+		err = PTR_ERR(host->rst_gpio);
+		goto out;
+	}
+	udelay(20);
+	gpiod_set_value_cansleep(host->rst_gpio, 1);
+
+	host->clks[0].id = "core";
+	host->clks[1].id = "pclk";
+	host->clks[2].id = "pclk_mphy";
+	err = devm_clk_bulk_get_optional(dev, UFS_MAX_CLKS, host->clks);
+	if (err) {
+		dev_err_probe(dev, err, "failed to get clocks\n");
+		goto out;
+	}
+
+	err = clk_bulk_prepare_enable(UFS_MAX_CLKS, host->clks);
+	if (err) {
+		dev_err_probe(dev, err, "failed to enable clocks\n");
+		goto out;
+	}
+
+	pm_runtime_set_active(&pdev->dev);
+
+	host->hba = hba;
+	ufs_rockchip_set_pm_lvl(hba);
+
+	ufshcd_set_variant(hba, host);
+
+	return 0;
+out:
+	clk_disable_unprepare(host->ref_out_clk);
+	return err;
+}
+
+static int ufs_rockchip_rk3576_init(struct ufs_hba *hba)
+{
+	int ret = 0;
+	struct device *dev = hba->dev;
+
+	hba->quirks = UFSHCI_QUIRK_BROKEN_HCE | UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING;
+
+	/* Enable BKOPS when suspend */
+	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
+	/* Enable putting device into deep sleep */
+	hba->caps |= UFSHCD_CAP_DEEPSLEEP;
+	/* Enable devfreq of UFS */
+	hba->caps |= UFSHCD_CAP_CLK_SCALING;
+	/* Enable WriteBooster */
+	hba->caps |= UFSHCD_CAP_WB_EN;
+
+	ret = ufs_rockchip_common_init(hba);
+	if (ret)
+		return dev_err_probe(dev, ret, "ufs common init fail\n");
+
+	return 0;
+}
+
+static int ufs_rockchip_device_reset(struct ufs_hba *hba)
+{
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+
+	if (!host->rst_gpio)
+		return -EOPNOTSUPP;
+
+	gpiod_set_value_cansleep(host->rst_gpio, 0);
+	udelay(20);
+
+	gpiod_set_value_cansleep(host->rst_gpio, 1);
+	udelay(20);
+
+	return 0;
+}
+
+static const struct ufs_hba_variant_ops ufs_hba_rk3576_vops = {
+	.name = "rk3576",
+	.init = ufs_rockchip_rk3576_init,
+	.device_reset = ufs_rockchip_device_reset,
+	.hce_enable_notify = ufs_rockchip_hce_enable_notify,
+	.phy_initialization = ufs_rockchip_rk3576_phy_init,
+};
+
+static const struct of_device_id ufs_rockchip_of_match[] = {
+	{ .compatible = "rockchip,rk3576-ufs", .data = &ufs_hba_rk3576_vops},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ufs_rockchip_of_match);
+
+static int ufs_rockchip_probe(struct platform_device *pdev)
+{
+	int err = 0;
+	struct device *dev = &pdev->dev;
+	const struct ufs_hba_variant_ops *vops;
+
+	vops = device_get_match_data(dev);
+	err = ufshcd_pltfrm_init(pdev, vops);
+	if (err)
+		dev_err_probe(dev, err, "ufshcd_pltfrm_init failed\n");
+
+	return err;
+}
+
+static void ufs_rockchip_remove(struct platform_device *pdev)
+{
+	struct ufs_hba *hba = platform_get_drvdata(pdev);
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+
+	pm_runtime_forbid(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+	ufshcd_remove(hba);
+	ufshcd_dealloc_host(hba);
+	clk_disable_unprepare(host->ref_out_clk);
+}
+
+static int ufs_rockchip_restore_link(struct ufs_hba *hba, bool is_store)
+{
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+	int err, retry = 3;
+
+	if (is_store) {
+		host->ie = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
+		host->ahit = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
+		return 0;
+	}
+
+	/* Enable controller */
+	err = ufshcd_hba_enable(hba);
+	if (err)
+		return err;
+
+	/* Link startup and wait for DP */
+	do {
+		err = ufshcd_dme_link_startup(hba);
+		if (!err && ufshcd_is_device_present(hba)) {
+			dev_dbg_ratelimited(hba->dev, "rockchip link startup successfully.\n");
+			break;
+		}
+	} while (retry--);
+
+	if (retry < 0) {
+		dev_err(hba->dev, "rockchip link startup failed.\n");
+		return -ENXIO;
+	}
+
+	/* Restore negotiated power mode */
+	err = ufshcd_config_pwr_mode(hba, &(hba->pwr_info));
+	if (err)
+		dev_err(hba->dev, "Failed to restore power mode, err = %d\n", err);
+
+	/* Restore task and transfer list */
+	ufshcd_writel(hba, 0xffffffff, REG_INTERRUPT_STATUS);
+	ufshcd_make_hba_operational(hba);
+
+	/* Restore lost regs */
+	ufshcd_writel(hba, host->ie, REG_INTERRUPT_ENABLE);
+	ufshcd_writel(hba, host->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER);
+	ufshcd_writel(hba, 0x1, REG_UTP_TRANSFER_REQ_LIST_RUN_STOP);
+	ufshcd_writel(hba, 0x1, REG_UTP_TASK_REQ_LIST_RUN_STOP);
+
+	return err;
+}
+
+static int ufs_rockchip_runtime_suspend(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+
+	clk_disable_unprepare(host->ref_out_clk);
+	return ufs_rockchip_restore_link(hba, true);
+}
+
+static int ufs_rockchip_runtime_resume(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+	int err;
+
+	err = clk_prepare_enable(host->ref_out_clk);
+	if (err) {
+		dev_err(hba->dev, "failed to enable ref out clock %d\n", err);
+		return err;
+	}
+
+	reset_control_assert(host->rst);
+	udelay(1);
+	reset_control_deassert(host->rst);
+
+	return ufs_rockchip_restore_link(hba, false);
+}
+
+static int ufs_rockchip_suspend(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (pm_runtime_suspended(hba->dev))
+		return 0;
+
+	ufs_rockchip_restore_link(hba, true);
+
+	return 0;
+}
+
+static int ufs_rockchip_resume(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (pm_runtime_suspended(hba->dev))
+		return 0;
+
+	/* Reset device if possible */
+	ufs_rockchip_device_reset(hba);
+	ufs_rockchip_restore_link(hba, false);
+
+	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)
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	 = ufshcd_resume_complete,
+};
+
+static struct platform_driver ufs_rockchip_pltform = {
+	.probe = ufs_rockchip_probe,
+	.remove = ufs_rockchip_remove,
+	.driver = {
+		.name = "ufshcd-rockchip",
+		.pm = &ufs_rockchip_pm_ops,
+		.of_match_table = ufs_rockchip_of_match,
+	},
+};
+module_platform_driver(ufs_rockchip_pltform);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Rockchip UFS Host Driver");
diff --git a/drivers/ufs/host/ufs-rockchip.h b/drivers/ufs/host/ufs-rockchip.h
new file mode 100644
index 0000000..9eb80e8
--- /dev/null
+++ b/drivers/ufs/host/ufs-rockchip.h
@@ -0,0 +1,51 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Rockchip UFS Host Controller driver
+ *
+ * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
+ */
+
+#ifndef _UFS_ROCKCHIP_H_
+#define _UFS_ROCKCHIP_H_
+
+#define UFS_MAX_CLKS 3
+
+#define SEL_TX_LANE0 0x0
+#define SEL_TX_LANE1 0x1
+#define SEL_TX_LANE2 0x2
+#define SEL_TX_LANE3 0x3
+#define SEL_RX_LANE0 0x4
+#define SEL_RX_LANE1 0x5
+#define SEL_RX_LANE2 0x6
+#define SEL_RX_LANE3 0x7
+
+#define MIB_T_DBG_CPORT_TX_ENDIAN	0xc022
+#define MIB_T_DBG_CPORT_RX_ENDIAN	0xc023
+
+struct ufs_rockchip_host {
+	struct ufs_hba *hba;
+	void __iomem *ufs_phy_ctrl;
+	void __iomem *ufs_sys_ctrl;
+	void __iomem *mphy_base;
+	struct gpio_desc *rst_gpio;
+	struct reset_control *rst;
+	struct clk *ref_out_clk;
+	struct clk_bulk_data clks[UFS_MAX_CLKS];
+	uint64_t caps;
+	bool in_suspend;
+	u32 ie;
+	u32 ahit;
+};
+
+#define ufs_sys_writel(base, val, reg)                                    \
+	writel((val), (base) + (reg))
+#define ufs_sys_readl(base, reg) readl((base) + (reg))
+#define ufs_sys_set_bits(base, mask, reg)                                 \
+	ufs_sys_writel(                                                   \
+		(base), ((mask) | (ufs_sys_readl((base), (reg)))), (reg))
+#define ufs_sys_ctrl_clr_bits(base, mask, reg)                                 \
+	ufs_sys_writel((base),                                            \
+			    ((~(mask)) & (ufs_sys_readl((base), (reg)))), \
+			    (reg))
+
+#endif /* _UFS_ROCKCHIP_H_ */