[PATCH v3 0/5] Initial support for RK3576 UFS controller

[PATCH v3 0/5] Initial support for RK3576 UFS controller

[Shawn Lin]

This patchset adds initial UFS controller supprt for RK3576 SoC.
Patch 1 adds new quirk and patch 2 is the dt-bindings.
patch 3/4 deal with rpm and spm support in advanced. Final atch 5 is the
driver added.


Changes in v3:
- rename the file to rockchip,rk3576-ufshc.yaml
- add description for reset-gpios
- reword Kconfig description
- elaborate more about controller in commit msg
- use rockchip,rk3576-ufshc for compatible
- remove useless header file
- remove inline for ufshcd_is_device_present
- use usleep_range instead
- remove initialization, reverse Xmas order
- remove useless varibles
- check vops for null
- other small fixes for err path
- remove pm_runtime_set_active
- fix the active and inactive reset-gpios logic
- fix rpm_lvl and spm_lvl to 5 and move to end of probe path
- remove unnecessary system PM callbacks
- use UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE instead
  of UFSHCI_QUIRK_BROKEN_HCE

Changes in v2:
- rename the file
- add reset-gpios

Shawn Lin (5):
  scsi: ufs: core: Add UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE
  dt-bindings: ufs: Document Rockchip UFS host controller
  soc: rockchip: add header for suspend mode SIP interface
  soc: rockchip: power-domain: Add GENPD_FLAG_RPM_ALWAYS_ON support
  scsi: ufs: rockchip: initial support for UFS

 .../bindings/ufs/rockchip,rk3576-ufshc.yaml        | 103 ++++++
 drivers/pmdomain/rockchip/pm-domains.c             |   3 +
 drivers/ufs/core/ufshcd.c                          |  17 +
 drivers/ufs/host/Kconfig                           |  12 +
 drivers/ufs/host/Makefile                          |   1 +
 drivers/ufs/host/ufs-rockchip.c                    | 346 +++++++++++++++++++++
 drivers/ufs/host/ufs-rockchip.h                    |  51 +++
 include/soc/rockchip/rockchip_sip.h                |   3 +
 include/ufs/ufshcd.h                               |   6 +
 9 files changed, 542 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufshc.yaml
 create mode 100644 drivers/ufs/host/ufs-rockchip.c
 create mode 100644 drivers/ufs/host/ufs-rockchip.h

-- 
2.7.4

[PATCH v3 1/5] scsi: ufs: core: Add UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE

[Shawn Lin]

HCE on Rockchip SoC is different from both of ufshcd_hba_execute_hce()
and UFSHCI_QUIRK_BROKEN_HCE case. It need to do dme_reset and dme_enable
after enabling HCE. So in order not to abuse UFSHCI_QUIRK_BROKEN_HCE, add
a new quirk UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE, to deal with that
limitation.

Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v3: None
Changes in v2: None

 drivers/ufs/core/ufshcd.c | 17 +++++++++++++++++
 include/ufs/ufshcd.h      |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7cab1031..4868099 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4819,6 +4819,7 @@ static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
 {
 	int retry_outer = 3;
 	int retry_inner;
+	int ret;
 
 start:
 	if (ufshcd_is_hba_active(hba))
@@ -4865,6 +4866,22 @@ static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
 	/* enable UIC related interrupts */
 	ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
 
+	/*
+	 * Do dme_reset and dme_enable if a UFS host controller need
+	 * this procedure to actually finish HCE.
+	 */
+	if (hba->quirks & UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE) {
+		ret = ufshcd_dme_reset(hba);
+		if (!ret) {
+			ret = ufshcd_dme_enable(hba);
+			if (ret)
+				dev_err(hba->dev,
+					"Failed to do dme_enable after HCE.\n");
+		} else {
+			dev_err(hba->dev, "Failed to do dme_reset after HCE.\n");
+		}
+	}
+
 	ufshcd_vops_hce_enable_notify(hba, POST_CHANGE);
 
 	return 0;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a95282b..73b888f 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -685,6 +685,12 @@ enum ufshcd_quirks {
 	 * single doorbell mode.
 	 */
 	UFSHCD_QUIRK_BROKEN_LSDBS_CAP			= 1 << 25,
+
+	/*
+	 * This quirks needs to be enabled if host controller need to
+	 * do dme_reset and dme_enable after hce.
+	 */
+	UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE		= 1 << 26,
 };
 
 enum ufshcd_caps {
-- 
2.7.4

Re: [PATCH v3 1/5] scsi: ufs: core: Add UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE

[Bart Van Assche]

On 10/7/24 11:15 PM, Shawn Lin wrote:
> +	/*
> +	 * Do dme_reset and dme_enable if a UFS host controller need
> +	 * this procedure to actually finish HCE.
> +	 */

need -> needs

> +
> +	/*
> +	 * This quirks needs to be enabled if host controller need to
> +	 * do dme_reset and dme_enable after hce.
> +	 */

quirks -> quirk

Thanks,

Bart.

[PATCH v3 2/5] dt-bindings: ufs: Document Rockchip UFS host controller

[Shawn Lin]

Document Rockchip UFS host controller for RK3576 SoC.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v3:
- rename the file to rockchip,rk3576-ufshc.yaml
- add description for reset-gpios
- use rockchip,rk3576-ufshc as compatible

Changes in v2:
- rename the file
- add reset-gpios

 .../bindings/ufs/rockchip,rk3576-ufshc.yaml        | 103 +++++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufshc.yaml

diff --git a/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufshc.yaml b/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufshc.yaml
new file mode 100644
index 0000000..1c78ea5
--- /dev/null
+++ b/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufshc.yaml
@@ -0,0 +1,103 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ufs/rockchip,rk3576-ufshc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip UFS Host Controller
+
+maintainers:
+  - Shawn Lin <shawn.lin@rock-chips.com>
+
+allOf:
+  - $ref: ufs-common.yaml
+
+properties:
+  compatible:
+    const: rockchip,rk3576-ufshc
+
+  reg:
+    maxItems: 5
+
+  reg-names:
+    items:
+      - const: hci
+      - const: mphy
+      - const: hci_grf
+      - const: mphy_grf
+      - const: hci_apb
+
+  clocks:
+    maxItems: 4
+
+  clock-names:
+    items:
+      - const: core
+      - const: pclk
+      - const: pclk_mphy
+      - const: ref_out
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 4
+
+  reset-names:
+    items:
+      - const: biu
+      - const: sys
+      - const: ufs
+      - const: grf
+
+  reset-gpios:
+    maxItems: 1
+    description: |
+      GPIO specifiers for host to reset the device.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+  - power-domains
+  - resets
+  - reset-names
+  - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rockchip,rk3576-cru.h>
+    #include <dt-bindings/reset/rockchip,rk3576-cru.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/rockchip,rk3576-power.h>
+    #include <dt-bindings/pinctrl/rockchip.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        ufs: ufs@2a2d0000 {
+              compatible = "rockchip,rk3576-ufshc";
+              reg = <0x0 0x2a2d0000 0x0 0x10000>,
+                    <0x0 0x2b040000 0x0 0x10000>,
+                    <0x0 0x2601f000 0x0 0x1000>,
+                    <0x0 0x2603c000 0x0 0x1000>,
+                    <0x0 0x2a2e0000 0x0 0x10000>;
+              reg-names = "hci", "mphy", "hci_grf", "mphy_grf", "hci_apb";
+              clocks = <&cru ACLK_UFS_SYS>, <&cru PCLK_USB_ROOT>, <&cru PCLK_MPHY>,
+                      <&cru CLK_REF_UFS_CLKOUT>;
+              clock-names = "core", "pclk", "pclk_mphy", "ref_out";
+              interrupts = <GIC_SPI 361 IRQ_TYPE_LEVEL_HIGH>;
+              power-domains = <&power RK3576_PD_USB>;
+              resets = <&cru SRST_A_UFS_BIU>, <&cru SRST_A_UFS_SYS>, <&cru SRST_A_UFS>,
+                        <&cru SRST_P_UFS_GRF>;
+              reset-names = "biu", "sys", "ufs", "grf";
+              reset-gpios = <&gpio4 RK_PD0 GPIO_ACTIVE_LOW>;
+        };
+    };
-- 
2.7.4

Re: [PATCH v3 2/5] dt-bindings: ufs: Document Rockchip UFS host controller

[Krzysztof Kozlowski]

On Tue, Oct 08, 2024 at 02:15:27PM +0800, Shawn Lin wrote:
> Document Rockchip UFS host controller for RK3576 SoC.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> Changes in v3:
> - rename the file to rockchip,rk3576-ufshc.yaml
> - add description for reset-gpios
> - use rockchip,rk3576-ufshc as compatible

... 

> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 4
> +
> +  reset-names:
> +    items:
> +      - const: biu
> +      - const: sys
> +      - const: ufs
> +      - const: grf
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: |
> +      GPIO specifiers for host to reset the device.

Redundant description. I don't get why did you add it.... maybe we were
confused by duplicating resets and GPIO, but then say here something
useful not what the property name is already saying.

E.g. Which pin is this? Active low/high? Resets which device - phy?
memory?

With proper description:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v3 3/5] soc: rockchip: add header for suspend mode SIP interface

[Shawn Lin]

Add ROCKCHIP_SIP_SUSPEND_MODE to pass down parameters to Trusted Firmware
in order to decide suspend mode. Currently only add ROCKCHIP_SLEEP_PD_CONFIG
which teaches firmware to power down controllers or not.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v3: None
Changes in v2: None

 include/soc/rockchip/rockchip_sip.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/soc/rockchip/rockchip_sip.h b/include/soc/rockchip/rockchip_sip.h
index c46a9ae..501ad1f 100644
--- a/include/soc/rockchip/rockchip_sip.h
+++ b/include/soc/rockchip/rockchip_sip.h
@@ -6,6 +6,9 @@
 #ifndef __SOC_ROCKCHIP_SIP_H
 #define __SOC_ROCKCHIP_SIP_H
 
+#define ROCKCHIP_SIP_SUSPEND_MODE		0x82000003
+#define ROCKCHIP_SLEEP_PD_CONFIG		0xff
+
 #define ROCKCHIP_SIP_DRAM_FREQ			0x82000008
 #define ROCKCHIP_SIP_CONFIG_DRAM_INIT		0x00
 #define ROCKCHIP_SIP_CONFIG_DRAM_SET_RATE	0x01
-- 
2.7.4

[PATCH v3 4/5] soc: rockchip: power-domain: Add GENPD_FLAG_RPM_ALWAYS_ON support

[Shawn Lin]

If low level driver claims to keep its own power domain always on,
let pd driver respect the flag of GENPD_FLAG_RPM_ALWAYS_ON.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v3: None
Changes in v2: None

 drivers/pmdomain/rockchip/pm-domains.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index cb0f938..b2bb458 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -621,6 +621,9 @@ static int rockchip_pd_power_off(struct generic_pm_domain *domain)
 {
 	struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
 
+	if (pd->genpd.flags & GENPD_FLAG_RPM_ALWAYS_ON)
+		return 0;
+
 	return rockchip_pd_power(pd, false);
 }
 
-- 
2.7.4

Re: [PATCH v3 4/5] soc: rockchip: power-domain: Add GENPD_FLAG_RPM_ALWAYS_ON support

[Ulf Hansson]

On Tue, 8 Oct 2024 at 08:16, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> If low level driver claims to keep its own power domain always on,
> let pd driver respect the flag of GENPD_FLAG_RPM_ALWAYS_ON.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/pmdomain/rockchip/pm-domains.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index cb0f938..b2bb458 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
> @@ -621,6 +621,9 @@ static int rockchip_pd_power_off(struct generic_pm_domain *domain)
>  {
>         struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
>
> +       if (pd->genpd.flags & GENPD_FLAG_RPM_ALWAYS_ON)
> +               return 0;

During system suspend, genpd may try to power off the PM domains that
have the GENPD_FLAG_RPM_ALWAYS_ON being set.

It seems like you need to prevent the PM domains from being power off
during system suspend too, right? In that case, why not use
GENPD_FLAG_ALWAYS_ON instead?

Or maybe the use case is different, let's continue to discuss patch5 first.

> +
>         return rockchip_pd_power(pd, false);
>  }
>
> --
> 2.7.4
>

Kind regards
Uffe

[PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Shawn Lin]

RK3576 SoC contains a UFS controller, add initial support fot it.
The features are:
(1) support UFS 2.0 features
(2) High speed up to HS-G3
(3) 2RX-2TX lanes
(4) auto H8 entry and exit

Software limitation:
(1) HCE procedure: enbale controller->enbale intr->dme_reset->dme_enable
(2) disable unipro timeout values before power mode change

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v3:
- reword Kconfig description
- elaborate more about controller in commit msg
- use rockchip,rk3576-ufshc for compatible
- remove useless header file
- remove inline for ufshcd_is_device_present
- use usleep_range instead
- remove initialization, reverse Xmas order
- remove useless varibles
- check vops for null
- other small fixes for err path
- remove pm_runtime_set_active
- fix the active and inactive reset-gpios logic
- fix rpm_lvl and spm_lvl to 5 and move to end of probe path
- remove unnecessary system PM callbacks
- use UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE instead
  of UFSHCI_QUIRK_BROKEN_HCE

Changes in v2: None

 drivers/ufs/host/Kconfig        |  12 ++
 drivers/ufs/host/Makefile       |   1 +
 drivers/ufs/host/ufs-rockchip.c | 346 ++++++++++++++++++++++++++++++++++++++++
 drivers/ufs/host/ufs-rockchip.h |  51 ++++++
 4 files changed, 410 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..191fbd7 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 UFS host controller 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..98bd646
--- /dev/null
+++ b/drivers/ufs/host/ufs-rockchip.c
@@ -0,0 +1,346 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Rockchip UFS Host Controller driver
+ *
+ * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <soc/rockchip/rockchip_sip.h>
+
+#include <ufs/ufshcd.h>
+#include <ufs/unipro.h>
+#include "ufshcd-pltfrm.h"
+#include "ufs-rockchip.h"
+
+static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
+					 enum ufs_notify_change_status status)
+{
+	int err = 0;
+
+	if (status == 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_5;
+	hba->spm_lvl = UFS_PM_LVL_5;
+}
+
+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);
+	usleep_range(1, 2);
+	ufs_sys_writel(host->mphy_base, 0x00, 0x08C);
+
+	usleep_range(200, 250);
+	/* 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;
+
+	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);
+	usleep_range(1, 2);
+	reset_control_deassert(host->rst);
+
+	host->ref_out_clk = devm_clk_get_enabled(dev, "ref_out");
+	if (IS_ERR(host->ref_out_clk))
+		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk),
+				"ref_out unavailable\n");
+
+	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(host->rst_gpio))
+		return dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
+				"invalid reset-gpios property in node\n");
+
+	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)
+		return dev_err_probe(dev, err, "failed to get clocks\n");
+
+	err = clk_bulk_prepare_enable(UFS_MAX_CLKS, host->clks);
+	if (err)
+		return dev_err_probe(dev, err, "failed to enable clocks\n");
+
+	host->hba = hba;
+
+	ufshcd_set_variant(hba, host);
+
+	return 0;
+}
+
+static int ufs_rockchip_rk3576_init(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+	int ret;
+
+	hba->quirks = UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING |
+		      UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE;
+
+	/* 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;
+
+	host->pd_id = RK3576_PD_UFS;
+
+	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);
+
+	/* Active the reset-gpios */
+	gpiod_set_value_cansleep(host->rst_gpio, 1);
+	usleep_range(20, 25);
+
+	/* Inactive the reset-gpios */
+	gpiod_set_value_cansleep(host->rst_gpio, 0);
+	usleep_range(20, 25);
+
+	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-ufshc", .data = &ufs_hba_rk3576_vops},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ufs_rockchip_of_match);
+
+static int ufs_rockchip_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct ufs_hba_variant_ops *vops;
+	struct ufs_hba *hba;
+	int err;
+
+	vops = device_get_match_data(dev);
+	if (!vops)
+		return dev_err_probe(dev, -EINVAL, "ufs_hba_variant_ops not defined.\n");
+
+	err = ufshcd_pltfrm_init(pdev, vops);
+	if (err)
+		return dev_err_probe(dev, err, "ufshcd_pltfrm_init failed\n");
+
+	hba = platform_get_drvdata(pdev);
+	/* Set the default desired pm level in case no users set via sysfs */
+	ufs_rockchip_set_pm_lvl(hba);
+
+	return 0;
+}
+
+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_runtime_suspend(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+
+	clk_disable_unprepare(host->ref_out_clk);
+
+	/*
+	 * Shouldn't power down if rpm_lvl is less than level 5.
+	 * This flag will be passed down to platform power-domain driver
+	 * which has the final decision.
+	 */
+	if (hba->rpm_lvl < UFS_PM_LVL_5)
+		genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
+	else
+		genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
+
+	return ufshcd_runtime_suspend(dev);
+}
+
+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);
+	usleep_range(1, 2);
+	reset_control_deassert(host->rst);
+
+	return ufshcd_runtime_resume(dev);
+}
+
+static int ufs_rockchip_system_suspend(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+
+	/* Pass down desired spm_lvl to Firmware */
+	arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
+			host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
+
+	return ufshcd_system_suspend(dev);
+}
+
+static const struct dev_pm_ops ufs_rockchip_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_system_suspend, ufshcd_system_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..37c45a5
--- /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
+
+#define RK3576_PD_UFS			0x7
+
+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;
+	uint64_t pd_id;
+};
+
+#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_ */
-- 
2.7.4

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Ulf Hansson]

[...]

> +
> +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);
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);

pd_to_genpd() isn't safe to use like this. It's solely to be used by
genpd provider drivers.

> +
> +       clk_disable_unprepare(host->ref_out_clk);
> +
> +       /*
> +        * Shouldn't power down if rpm_lvl is less than level 5.

Can you elaborate on why we must not power-off the power-domain when
level is less than 5?

What happens if we power-off anyway when the level is less than 5?

> +        * This flag will be passed down to platform power-domain driver
> +        * which has the final decision.
> +        */
> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
> +       else
> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;

The genpd->flags is not supposed to be changed like this - and
especially not from a genpd consumer driver.

I am trying to understand a bit more of the use case here. Let's see
if that helps me to potentially suggest an alternative approach.

> +
> +       return ufshcd_runtime_suspend(dev);
> +}
> +
> +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);
> +       usleep_range(1, 2);
> +       reset_control_deassert(host->rst);
> +
> +       return ufshcd_runtime_resume(dev);
> +}
> +
> +static int ufs_rockchip_system_suspend(struct device *dev)
> +{
> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> +
> +       /* Pass down desired spm_lvl to Firmware */
> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);

Can you please elaborate on what goes on here? Is this turning off the
power-domain that the dev is attached to - or what is actually
happening?

> +
> +       return ufshcd_system_suspend(dev);
> +}
> +
> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_system_suspend, ufshcd_system_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);
> +

[...]

Kind regards
Uffe

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Shawn Lin]

Hi Ulf

在 2024/10/9 21:15, Ulf Hansson 写道:
> [...]
> 
>> +
>> +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);
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> 
> pd_to_genpd() isn't safe to use like this. It's solely to be used by
> genpd provider drivers.
> 
>> +
>> +       clk_disable_unprepare(host->ref_out_clk);
>> +
>> +       /*
>> +        * Shouldn't power down if rpm_lvl is less than level 5.
> 
> Can you elaborate on why we must not power-off the power-domain when
> level is less than 5?
> 

Because ufshcd driver assume the controller is active and the link is on
if level is less than 5. So the default resume policy will not try to
recover the registers until the first error happened. Otherwise if the
level is >=5, it assumes the controller is off and the link is down,
then it will restore the registers and link.

And the level is changeable via sysfs.

> What happens if we power-off anyway when the level is less than 5?
> 
>> +        * This flag will be passed down to platform power-domain driver
>> +        * which has the final decision.
>> +        */
>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
>> +       else
>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
> 
> The genpd->flags is not supposed to be changed like this - and
> especially not from a genpd consumer driver.
> 
> I am trying to understand a bit more of the use case here. Let's see
> if that helps me to potentially suggest an alternative approach.
> 

I was not familiar with the genpd part, so I haven't come up with 
another solution. It would be great if you can guide me to the right
way.

>> +
>> +       return ufshcd_runtime_suspend(dev);
>> +}
>> +
>> +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);
>> +       usleep_range(1, 2);
>> +       reset_control_deassert(host->rst);
>> +
>> +       return ufshcd_runtime_resume(dev);
>> +}
>> +
>> +static int ufs_rockchip_system_suspend(struct device *dev)
>> +{
>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>> +
>> +       /* Pass down desired spm_lvl to Firmware */
>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
> 
> Can you please elaborate on what goes on here? Is this turning off the
> power-domain that the dev is attached to - or what is actually
> happening?
> 

This smc call is trying to ask firmware not to turn off the power-domian
that the UFS is attached to and also not to turn off the power of UFS
conntroller.

Per your comment at patch 4, should I use GENPD_FLAG_ALWAYS_ON +
arm_smccc_smc here in system suspend?

>> +
>> +       return ufshcd_system_suspend(dev);
>> +}
>> +
>> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_system_suspend, ufshcd_system_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);
>> +
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Ulf Hansson]

On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Ulf
>
> 在 2024/10/9 21:15, Ulf Hansson 写道:
> > [...]
> >
> >> +
> >> +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);
> >> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> >
> > pd_to_genpd() isn't safe to use like this. It's solely to be used by
> > genpd provider drivers.
> >
> >> +
> >> +       clk_disable_unprepare(host->ref_out_clk);
> >> +
> >> +       /*
> >> +        * Shouldn't power down if rpm_lvl is less than level 5.
> >
> > Can you elaborate on why we must not power-off the power-domain when
> > level is less than 5?
> >
>
> Because ufshcd driver assume the controller is active and the link is on
> if level is less than 5. So the default resume policy will not try to
> recover the registers until the first error happened. Otherwise if the
> level is >=5, it assumes the controller is off and the link is down,
> then it will restore the registers and link.
>
> And the level is changeable via sysfs.

Okay, thanks for clarifying.

>
> > What happens if we power-off anyway when the level is less than 5?
> >
> >> +        * This flag will be passed down to platform power-domain driver
> >> +        * which has the final decision.
> >> +        */
> >> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
> >> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
> >> +       else
> >> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
> >
> > The genpd->flags is not supposed to be changed like this - and
> > especially not from a genpd consumer driver.
> >
> > I am trying to understand a bit more of the use case here. Let's see
> > if that helps me to potentially suggest an alternative approach.
> >
>
> I was not familiar with the genpd part, so I haven't come up with
> another solution. It would be great if you can guide me to the right
> way.

I have been playing with the existing infrastructure we have at hand
to support this, but I need a few more days to be able to propose
something for you.

>
> >> +
> >> +       return ufshcd_runtime_suspend(dev);
> >> +}
> >> +
> >> +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);
> >> +       usleep_range(1, 2);
> >> +       reset_control_deassert(host->rst);
> >> +
> >> +       return ufshcd_runtime_resume(dev);
> >> +}
> >> +
> >> +static int ufs_rockchip_system_suspend(struct device *dev)
> >> +{
> >> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> >> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> >> +
> >> +       /* Pass down desired spm_lvl to Firmware */
> >> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
> >> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
> >
> > Can you please elaborate on what goes on here? Is this turning off the
> > power-domain that the dev is attached to - or what is actually
> > happening?
> >
>
> This smc call is trying to ask firmware not to turn off the power-domian
> that the UFS is attached to and also not to turn off the power of UFS
> conntroller.

Okay, thanks for clarifying!

A follow up question, don't you need to make a corresponding smc call
to inform the FW that it's okay to turn off the power-domain at some
point?

>
> Per your comment at patch 4, should I use GENPD_FLAG_ALWAYS_ON +
> arm_smccc_smc here in system suspend?
>
> >> +
> >> +       return ufshcd_system_suspend(dev);
> >> +}
> >> +
> >> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
> >> +       SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_system_suspend, ufshcd_system_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);
> >> +
> >
> > [...]

Kind regards
Uffe

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Shawn Lin]

Hi Ulf,

在 2024/10/18 17:07, Ulf Hansson 写道:
> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Ulf
>>
>> 在 2024/10/9 21:15, Ulf Hansson 写道:
>>> [...]
>>>
>>>> +
>>>> +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);
>>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>>>
>>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
>>> genpd provider drivers.
>>>
>>>> +
>>>> +       clk_disable_unprepare(host->ref_out_clk);
>>>> +
>>>> +       /*
>>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
>>>
>>> Can you elaborate on why we must not power-off the power-domain when
>>> level is less than 5?
>>>
>>
>> Because ufshcd driver assume the controller is active and the link is on
>> if level is less than 5. So the default resume policy will not try to
>> recover the registers until the first error happened. Otherwise if the
>> level is >=5, it assumes the controller is off and the link is down,
>> then it will restore the registers and link.
>>
>> And the level is changeable via sysfs.
> 
> Okay, thanks for clarifying.
> 
>>
>>> What happens if we power-off anyway when the level is less than 5?
>>>
>>>> +        * This flag will be passed down to platform power-domain driver
>>>> +        * which has the final decision.
>>>> +        */
>>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
>>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
>>>> +       else
>>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
>>>
>>> The genpd->flags is not supposed to be changed like this - and
>>> especially not from a genpd consumer driver.
>>>
>>> I am trying to understand a bit more of the use case here. Let's see
>>> if that helps me to potentially suggest an alternative approach.
>>>
>>
>> I was not familiar with the genpd part, so I haven't come up with
>> another solution. It would be great if you can guide me to the right
>> way.
> 
> I have been playing with the existing infrastructure we have at hand
> to support this, but I need a few more days to be able to propose
> something for you.
> 

Much appreciate.

>>
>>>> +
>>>> +       return ufshcd_runtime_suspend(dev);
>>>> +}
>>>> +
>>>> +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);
>>>> +       usleep_range(1, 2);
>>>> +       reset_control_deassert(host->rst);
>>>> +
>>>> +       return ufshcd_runtime_resume(dev);
>>>> +}
>>>> +
>>>> +static int ufs_rockchip_system_suspend(struct device *dev)
>>>> +{
>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>>>> +
>>>> +       /* Pass down desired spm_lvl to Firmware */
>>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
>>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
>>>
>>> Can you please elaborate on what goes on here? Is this turning off the
>>> power-domain that the dev is attached to - or what is actually
>>> happening?
>>>
>>
>> This smc call is trying to ask firmware not to turn off the power-domian
>> that the UFS is attached to and also not to turn off the power of UFS
>> conntroller.
> 
> Okay, thanks for clarifying!
> 
> A follow up question, don't you need to make a corresponding smc call
> to inform the FW that it's okay to turn off the power-domain at some
> point?
> 

Yes. Each time entering sleep, we teach FW if it need to turn off or 
keep power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
off and 1 means on.

>>
>> Per your comment at patch 4, should I use GENPD_FLAG_ALWAYS_ON +
>> arm_smccc_smc here in system suspend?
>>
>>>> +
>>>> +       return ufshcd_system_suspend(dev);
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
>>>> +       SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_system_suspend, ufshcd_system_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);
>>>> +
>>>
>>> [...]
> 
> Kind regards
> Uffe
> 

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Ulf Hansson]

On Fri, 18 Oct 2024 at 11:20, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Ulf,
>
> 在 2024/10/18 17:07, Ulf Hansson 写道:
> > On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> Hi Ulf
> >>
> >> 在 2024/10/9 21:15, Ulf Hansson 写道:
> >>> [...]
> >>>
> >>>> +
> >>>> +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);
> >>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> >>>
> >>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
> >>> genpd provider drivers.
> >>>
> >>>> +
> >>>> +       clk_disable_unprepare(host->ref_out_clk);
> >>>> +
> >>>> +       /*
> >>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
> >>>
> >>> Can you elaborate on why we must not power-off the power-domain when
> >>> level is less than 5?
> >>>
> >>
> >> Because ufshcd driver assume the controller is active and the link is on
> >> if level is less than 5. So the default resume policy will not try to
> >> recover the registers until the first error happened. Otherwise if the
> >> level is >=5, it assumes the controller is off and the link is down,
> >> then it will restore the registers and link.
> >>
> >> And the level is changeable via sysfs.
> >
> > Okay, thanks for clarifying.
> >
> >>
> >>> What happens if we power-off anyway when the level is less than 5?
> >>>
> >>>> +        * This flag will be passed down to platform power-domain driver
> >>>> +        * which has the final decision.
> >>>> +        */
> >>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
> >>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
> >>>> +       else
> >>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
> >>>
> >>> The genpd->flags is not supposed to be changed like this - and
> >>> especially not from a genpd consumer driver.
> >>>
> >>> I am trying to understand a bit more of the use case here. Let's see
> >>> if that helps me to potentially suggest an alternative approach.
> >>>
> >>
> >> I was not familiar with the genpd part, so I haven't come up with
> >> another solution. It would be great if you can guide me to the right
> >> way.
> >
> > I have been playing with the existing infrastructure we have at hand
> > to support this, but I need a few more days to be able to propose
> > something for you.
> >
>
> Much appreciate.
>
> >>
> >>>> +
> >>>> +       return ufshcd_runtime_suspend(dev);
> >>>> +}
> >>>> +
> >>>> +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);
> >>>> +       usleep_range(1, 2);
> >>>> +       reset_control_deassert(host->rst);
> >>>> +
> >>>> +       return ufshcd_runtime_resume(dev);
> >>>> +}
> >>>> +
> >>>> +static int ufs_rockchip_system_suspend(struct device *dev)
> >>>> +{
> >>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> >>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> >>>> +
> >>>> +       /* Pass down desired spm_lvl to Firmware */
> >>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
> >>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
> >>>
> >>> Can you please elaborate on what goes on here? Is this turning off the
> >>> power-domain that the dev is attached to - or what is actually
> >>> happening?
> >>>
> >>
> >> This smc call is trying to ask firmware not to turn off the power-domian
> >> that the UFS is attached to and also not to turn off the power of UFS
> >> conntroller.
> >
> > Okay, thanks for clarifying!
> >
> > A follow up question, don't you need to make a corresponding smc call
> > to inform the FW that it's okay to turn off the power-domain at some
> > point?
> >
>
> Yes. Each time entering sleep, we teach FW if it need to turn off or
> keep power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
> off and 1 means on.

I see. So you need to make the call each time when entering the system suspend?

Or would it be okay to just make it once, when the spm_lvl is changed?

Another way to deal with it, would be to make the smc call each time
the power-domain is turned-on, based on spm_lvl too of course.

Would that work?

[...]

Kind regards
Uffe

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Shawn Lin]

在 2024/10/18 18:03, Ulf Hansson 写道:
> On Fri, 18 Oct 2024 at 11:20, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Ulf,
>>
>> 在 2024/10/18 17:07, Ulf Hansson 写道:
>>> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>
>>>> Hi Ulf
>>>>
>>>> 在 2024/10/9 21:15, Ulf Hansson 写道:
>>>>> [...]
>>>>>
>>>>>> +
>>>>>> +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);
>>>>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>>>>>
>>>>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
>>>>> genpd provider drivers.
>>>>>
>>>>>> +
>>>>>> +       clk_disable_unprepare(host->ref_out_clk);
>>>>>> +
>>>>>> +       /*
>>>>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
>>>>>
>>>>> Can you elaborate on why we must not power-off the power-domain when
>>>>> level is less than 5?
>>>>>
>>>>
>>>> Because ufshcd driver assume the controller is active and the link is on
>>>> if level is less than 5. So the default resume policy will not try to
>>>> recover the registers until the first error happened. Otherwise if the
>>>> level is >=5, it assumes the controller is off and the link is down,
>>>> then it will restore the registers and link.
>>>>
>>>> And the level is changeable via sysfs.
>>>
>>> Okay, thanks for clarifying.
>>>
>>>>
>>>>> What happens if we power-off anyway when the level is less than 5?
>>>>>
>>>>>> +        * This flag will be passed down to platform power-domain driver
>>>>>> +        * which has the final decision.
>>>>>> +        */
>>>>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
>>>>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
>>>>>> +       else
>>>>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
>>>>>
>>>>> The genpd->flags is not supposed to be changed like this - and
>>>>> especially not from a genpd consumer driver.
>>>>>
>>>>> I am trying to understand a bit more of the use case here. Let's see
>>>>> if that helps me to potentially suggest an alternative approach.
>>>>>
>>>>
>>>> I was not familiar with the genpd part, so I haven't come up with
>>>> another solution. It would be great if you can guide me to the right
>>>> way.
>>>
>>> I have been playing with the existing infrastructure we have at hand
>>> to support this, but I need a few more days to be able to propose
>>> something for you.
>>>
>>
>> Much appreciate.
>>
>>>>
>>>>>> +
>>>>>> +       return ufshcd_runtime_suspend(dev);
>>>>>> +}
>>>>>> +
>>>>>> +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);
>>>>>> +       usleep_range(1, 2);
>>>>>> +       reset_control_deassert(host->rst);
>>>>>> +
>>>>>> +       return ufshcd_runtime_resume(dev);
>>>>>> +}
>>>>>> +
>>>>>> +static int ufs_rockchip_system_suspend(struct device *dev)
>>>>>> +{
>>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>>>>>> +
>>>>>> +       /* Pass down desired spm_lvl to Firmware */
>>>>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
>>>>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
>>>>>
>>>>> Can you please elaborate on what goes on here? Is this turning off the
>>>>> power-domain that the dev is attached to - or what is actually
>>>>> happening?
>>>>>
>>>>
>>>> This smc call is trying to ask firmware not to turn off the power-domian
>>>> that the UFS is attached to and also not to turn off the power of UFS
>>>> conntroller.
>>>
>>> Okay, thanks for clarifying!
>>>
>>> A follow up question, don't you need to make a corresponding smc call
>>> to inform the FW that it's okay to turn off the power-domain at some
>>> point?
>>>
>>
>> Yes. Each time entering sleep, we teach FW if it need to turn off or
>> keep power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
>> off and 1 means on.
> 
> I see. So you need to make the call each time when entering the system suspend?
> 
> Or would it be okay to just make it once, when the spm_lvl is changed?

Thers is no nofity when changing spm_lvl.

> 
> Another way to deal with it, would be to make the smc call each time
> the power-domain is turned-on, based on spm_lvl too of course.
> 
> Would that work?

Yes, that works. Another option is to cache power-domain states and
check spm_lvl locally. If it doesn't change, we skip smc call.

> 
> [...]
> 
> Kind regards
> Uffe
> 

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Ulf Hansson]

On Mon, 21 Oct 2024 at 02:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> 在 2024/10/18 18:03, Ulf Hansson 写道:
> > On Fri, 18 Oct 2024 at 11:20, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> Hi Ulf,
> >>
> >> 在 2024/10/18 17:07, Ulf Hansson 写道:
> >>> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>>>
> >>>> Hi Ulf
> >>>>
> >>>> 在 2024/10/9 21:15, Ulf Hansson 写道:
> >>>>> [...]
> >>>>>
> >>>>>> +
> >>>>>> +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);
> >>>>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> >>>>>
> >>>>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
> >>>>> genpd provider drivers.
> >>>>>
> >>>>>> +
> >>>>>> +       clk_disable_unprepare(host->ref_out_clk);
> >>>>>> +
> >>>>>> +       /*
> >>>>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
> >>>>>
> >>>>> Can you elaborate on why we must not power-off the power-domain when
> >>>>> level is less than 5?
> >>>>>
> >>>>
> >>>> Because ufshcd driver assume the controller is active and the link is on
> >>>> if level is less than 5. So the default resume policy will not try to
> >>>> recover the registers until the first error happened. Otherwise if the
> >>>> level is >=5, it assumes the controller is off and the link is down,
> >>>> then it will restore the registers and link.
> >>>>
> >>>> And the level is changeable via sysfs.
> >>>
> >>> Okay, thanks for clarifying.
> >>>
> >>>>
> >>>>> What happens if we power-off anyway when the level is less than 5?
> >>>>>
> >>>>>> +        * This flag will be passed down to platform power-domain driver
> >>>>>> +        * which has the final decision.
> >>>>>> +        */
> >>>>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
> >>>>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
> >>>>>> +       else
> >>>>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
> >>>>>
> >>>>> The genpd->flags is not supposed to be changed like this - and
> >>>>> especially not from a genpd consumer driver.
> >>>>>
> >>>>> I am trying to understand a bit more of the use case here. Let's see
> >>>>> if that helps me to potentially suggest an alternative approach.
> >>>>>
> >>>>
> >>>> I was not familiar with the genpd part, so I haven't come up with
> >>>> another solution. It would be great if you can guide me to the right
> >>>> way.
> >>>
> >>> I have been playing with the existing infrastructure we have at hand
> >>> to support this, but I need a few more days to be able to propose
> >>> something for you.
> >>>
> >>
> >> Much appreciate.
> >>
> >>>>
> >>>>>> +
> >>>>>> +       return ufshcd_runtime_suspend(dev);
> >>>>>> +}
> >>>>>> +
> >>>>>> +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);
> >>>>>> +       usleep_range(1, 2);
> >>>>>> +       reset_control_deassert(host->rst);
> >>>>>> +
> >>>>>> +       return ufshcd_runtime_resume(dev);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int ufs_rockchip_system_suspend(struct device *dev)
> >>>>>> +{
> >>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> >>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> >>>>>> +
> >>>>>> +       /* Pass down desired spm_lvl to Firmware */
> >>>>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
> >>>>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
> >>>>>
> >>>>> Can you please elaborate on what goes on here? Is this turning off the
> >>>>> power-domain that the dev is attached to - or what is actually
> >>>>> happening?
> >>>>>
> >>>>
> >>>> This smc call is trying to ask firmware not to turn off the power-domian
> >>>> that the UFS is attached to and also not to turn off the power of UFS
> >>>> conntroller.
> >>>
> >>> Okay, thanks for clarifying!
> >>>
> >>> A follow up question, don't you need to make a corresponding smc call
> >>> to inform the FW that it's okay to turn off the power-domain at some
> >>> point?
> >>>
> >>
> >> Yes. Each time entering sleep, we teach FW if it need to turn off or
> >> keep power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
> >> off and 1 means on.
> >
> > I see. So you need to make the call each time when entering the system suspend?
> >
> > Or would it be okay to just make it once, when the spm_lvl is changed?
>
> Thers is no nofity when changing spm_lvl.
>
> >
> > Another way to deal with it, would be to make the smc call each time
> > the power-domain is turned-on, based on spm_lvl too of course.
> >
> > Would that work?
>
> Yes, that works. Another option is to cache power-domain states and
> check spm_lvl locally. If it doesn't change, we skip smc call.

Apologize for the delay! I needed to think a bit more carefully about
how to suggest moving this forward.

My conclusion is that we need to extend the PM domain infrastructure
(genpd in particular), to allow drivers to dynamically inform whether
it's okay to turn on/off the PM domain in runtime.

There is a similar thing already available, which is to use dev PM qos
along with the genpd governor, but that would not work in this case
because it may prevent runtime suspend for the device in question too.
I have therefore cooked a patch for genpd, see below. I think you can
fold it into your next version of the series. See also additional
suggestions below the patch.

From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Fri, 1 Nov 2024 15:55:56 +0100
Subject: [PATCH] pmdomain: core: Introduce dev_pm_genpd_rpm_always_on()

For some usecases a consumer driver requires its device to remain power-on
from the PM domain perspective during runtime. Using dev PM qos along with
the genpd governors, doesn't work for this case as would potentially
prevent the device from being runtime suspended too.

To support these usecases, let's introduce dev_pm_genpd_rpm_always_on() to
allow consumers drivers to dynamically control the behaviour in genpd for a
device that is attached to it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/pmdomain/core.c   | 34 ++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h |  7 +++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index a6c8b85dd024..e86e270b7eb9 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -697,6 +697,36 @@ bool dev_pm_genpd_get_hwmode(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_genpd_get_hwmode);

+/**
+ * dev_pm_genpd_rpm_always_on() - Control if the PM domain can be powered off.
+ *
+ * @dev: Device for which the PM domain may need to stay on for.
+ * @on: Value to set or unset for the condition.
+ *
+ * For some usecases a consumer driver requires its device to remain power-on
+ * from the PM domain perspective during runtime. This function allows the
+ * behaviour to be dynamically controlled for a device attached to a genpd.
+ *
+ * It is assumed that the users guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ *
+ * Return: Returns 0 on success and negative error values on failures.
+ */
+int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
+{
+       struct generic_pm_domain *genpd;
+
+       genpd = dev_to_genpd_safe(dev);
+       if (!genpd)
+               return -ENODEV;
+
+       genpd_lock(genpd);
+       dev_gpd_data(dev)->rpm_always_on = on;
+       genpd_unlock(genpd);
+
+       return 0;
+}
+
 static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
        unsigned int state_idx = genpd->state_idx;
@@ -868,6 +898,10 @@ static int genpd_power_off(struct
generic_pm_domain *genpd, bool one_dev_on,
                if (!pm_runtime_suspended(pdd->dev) ||
                        irq_safe_dev_in_sleep_domain(pdd->dev, genpd))
                        not_suspended++;
+
+               /* The device may need its PM domain to stay powered on. */
+               if (to_gpd_data(pdd)->rpm_always_on)
+                       return -EBUSY;
        }

        if (not_suspended > 1 || (not_suspended == 1 && !one_dev_on))
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 45646bfcaf1a..d4c4a7cf34bd 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -260,6 +260,7 @@ struct generic_pm_domain_data {
        unsigned int rpm_pstate;
        unsigned int opp_token;
        bool hw_mode;
+       bool rpm_always_on;
        void *data;
 };

@@ -292,6 +293,7 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
 void dev_pm_genpd_synced_poweroff(struct device *dev);
 int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
 bool dev_pm_genpd_get_hwmode(struct device *dev);
+int dev_pm_genpd_rpm_always_on(struct device *dev, bool on);

 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -375,6 +377,11 @@ static inline bool dev_pm_genpd_get_hwmode(struct
device *dev)
        return false;
 }

+static inline int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
+{
+       return -EOPNOTSUPP;
+}
+
 #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
 #define pm_domain_always_on_gov                (*(struct
dev_power_governor *)(NULL))
 #endif
-- 
2.43.0

Furthermore, I believe the way forward could be like this:

*) The arm smc call to inform the FW whether it's okay to turn off (or
not turn off) the PM domain for the UFS controller really belongs in
the genpd provider. More precisely, from the corresponding genpd's
->power_on() callback, we should send the smc call that prevents
power-off and in the ->power_off() callback we should send the smc
call that allows power-off again. No matter of what the spm|rpm_level
is set to. If you think caching of the state is important, I suggest
looking into that as an improvement on top.

*) In the UFS controller driver, we should call the new
dev_pm_genpd_rpm_always_on() to control whether the PM domain should
remain on during runtime or is allowed to be turned off.

*) In the system suspend callback, based on the spm|rpm_level (I guess
only one of those levels is really needed?), we may call
device_awake_path(). This can prevent genpd from turning off the PM
domain during system suspend in those cases when that is needed. See
genpd_finish_suspend() more details. Also note that, the genpd in
question also needs the GENPD_FLAG_ACTIVE_WAKEUP bit set for it, if
not already.

Kind regards
Uffe

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Shawn Lin]

在 2024/11/1 23:12, Ulf Hansson 写道:
> On Mon, 21 Oct 2024 at 02:43, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> 在 2024/10/18 18:03, Ulf Hansson 写道:
>>> On Fri, 18 Oct 2024 at 11:20, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>
>>>> Hi Ulf,
>>>>
>>>> 在 2024/10/18 17:07, Ulf Hansson 写道:
>>>>> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>>>
>>>>>> Hi Ulf
>>>>>>
>>>>>> 在 2024/10/9 21:15, Ulf Hansson 写道:
>>>>>>> [...]
>>>>>>>
>>>>>>>> +
>>>>>>>> +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);
>>>>>>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>>>>>>>
>>>>>>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
>>>>>>> genpd provider drivers.
>>>>>>>
>>>>>>>> +
>>>>>>>> +       clk_disable_unprepare(host->ref_out_clk);
>>>>>>>> +
>>>>>>>> +       /*
>>>>>>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
>>>>>>>
>>>>>>> Can you elaborate on why we must not power-off the power-domain when
>>>>>>> level is less than 5?
>>>>>>>
>>>>>>
>>>>>> Because ufshcd driver assume the controller is active and the link is on
>>>>>> if level is less than 5. So the default resume policy will not try to
>>>>>> recover the registers until the first error happened. Otherwise if the
>>>>>> level is >=5, it assumes the controller is off and the link is down,
>>>>>> then it will restore the registers and link.
>>>>>>
>>>>>> And the level is changeable via sysfs.
>>>>>
>>>>> Okay, thanks for clarifying.
>>>>>
>>>>>>
>>>>>>> What happens if we power-off anyway when the level is less than 5?
>>>>>>>
>>>>>>>> +        * This flag will be passed down to platform power-domain driver
>>>>>>>> +        * which has the final decision.
>>>>>>>> +        */
>>>>>>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
>>>>>>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
>>>>>>>> +       else
>>>>>>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
>>>>>>>
>>>>>>> The genpd->flags is not supposed to be changed like this - and
>>>>>>> especially not from a genpd consumer driver.
>>>>>>>
>>>>>>> I am trying to understand a bit more of the use case here. Let's see
>>>>>>> if that helps me to potentially suggest an alternative approach.
>>>>>>>
>>>>>>
>>>>>> I was not familiar with the genpd part, so I haven't come up with
>>>>>> another solution. It would be great if you can guide me to the right
>>>>>> way.
>>>>>
>>>>> I have been playing with the existing infrastructure we have at hand
>>>>> to support this, but I need a few more days to be able to propose
>>>>> something for you.
>>>>>
>>>>
>>>> Much appreciate.
>>>>
>>>>>>
>>>>>>>> +
>>>>>>>> +       return ufshcd_runtime_suspend(dev);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +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);
>>>>>>>> +       usleep_range(1, 2);
>>>>>>>> +       reset_control_deassert(host->rst);
>>>>>>>> +
>>>>>>>> +       return ufshcd_runtime_resume(dev);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int ufs_rockchip_system_suspend(struct device *dev)
>>>>>>>> +{
>>>>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>>>>>>>> +
>>>>>>>> +       /* Pass down desired spm_lvl to Firmware */
>>>>>>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
>>>>>>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
>>>>>>>
>>>>>>> Can you please elaborate on what goes on here? Is this turning off the
>>>>>>> power-domain that the dev is attached to - or what is actually
>>>>>>> happening?
>>>>>>>
>>>>>>
>>>>>> This smc call is trying to ask firmware not to turn off the power-domian
>>>>>> that the UFS is attached to and also not to turn off the power of UFS
>>>>>> conntroller.
>>>>>
>>>>> Okay, thanks for clarifying!
>>>>>
>>>>> A follow up question, don't you need to make a corresponding smc call
>>>>> to inform the FW that it's okay to turn off the power-domain at some
>>>>> point?
>>>>>
>>>>
>>>> Yes. Each time entering sleep, we teach FW if it need to turn off or
>>>> keep power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
>>>> off and 1 means on.
>>>
>>> I see. So you need to make the call each time when entering the system suspend?
>>>
>>> Or would it be okay to just make it once, when the spm_lvl is changed?
>>
>> Thers is no nofity when changing spm_lvl.
>>
>>>
>>> Another way to deal with it, would be to make the smc call each time
>>> the power-domain is turned-on, based on spm_lvl too of course.
>>>
>>> Would that work?
>>
>> Yes, that works. Another option is to cache power-domain states and
>> check spm_lvl locally. If it doesn't change, we skip smc call.
> 
> Apologize for the delay! I needed to think a bit more carefully about
> how to suggest moving this forward.
> 
> My conclusion is that we need to extend the PM domain infrastructure
> (genpd in particular), to allow drivers to dynamically inform whether
> it's okay to turn on/off the PM domain in runtime.
> 
> There is a similar thing already available, which is to use dev PM qos
> along with the genpd governor, but that would not work in this case
> because it may prevent runtime suspend for the device in question too.
> I have therefore cooked a patch for genpd, see below. I think you can
> fold it into your next version of the series. See also additional
> suggestions below the patch.

Thanks, Ulf.  I'll fold it into my v4 series and fix the code in UFS 
driver and genpd provider according to your suggestions.

> 
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Date: Fri, 1 Nov 2024 15:55:56 +0100
> Subject: [PATCH] pmdomain: core: Introduce dev_pm_genpd_rpm_always_on()
> 
> For some usecases a consumer driver requires its device to remain power-on
> from the PM domain perspective during runtime. Using dev PM qos along with
> the genpd governors, doesn't work for this case as would potentially
> prevent the device from being runtime suspended too.
> 
> To support these usecases, let's introduce dev_pm_genpd_rpm_always_on() to
> allow consumers drivers to dynamically control the behaviour in genpd for a
> device that is attached to it.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/pmdomain/core.c   | 34 ++++++++++++++++++++++++++++++++++
>   include/linux/pm_domain.h |  7 +++++++
>   2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index a6c8b85dd024..e86e270b7eb9 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -697,6 +697,36 @@ bool dev_pm_genpd_get_hwmode(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(dev_pm_genpd_get_hwmode);
> 
> +/**
> + * dev_pm_genpd_rpm_always_on() - Control if the PM domain can be powered off.
> + *
> + * @dev: Device for which the PM domain may need to stay on for.
> + * @on: Value to set or unset for the condition.
> + *
> + * For some usecases a consumer driver requires its device to remain power-on
> + * from the PM domain perspective during runtime. This function allows the
> + * behaviour to be dynamically controlled for a device attached to a genpd.
> + *
> + * It is assumed that the users guarantee that the genpd wouldn't be detached
> + * while this routine is getting called.
> + *
> + * Return: Returns 0 on success and negative error values on failures.
> + */
> +int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
> +{
> +       struct generic_pm_domain *genpd;
> +
> +       genpd = dev_to_genpd_safe(dev);
> +       if (!genpd)
> +               return -ENODEV;
> +
> +       genpd_lock(genpd);
> +       dev_gpd_data(dev)->rpm_always_on = on;
> +       genpd_unlock(genpd);
> +
> +       return 0;
> +}
> +
>   static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>   {
>          unsigned int state_idx = genpd->state_idx;
> @@ -868,6 +898,10 @@ static int genpd_power_off(struct
> generic_pm_domain *genpd, bool one_dev_on,
>                  if (!pm_runtime_suspended(pdd->dev) ||
>                          irq_safe_dev_in_sleep_domain(pdd->dev, genpd))
>                          not_suspended++;
> +
> +               /* The device may need its PM domain to stay powered on. */
> +               if (to_gpd_data(pdd)->rpm_always_on)
> +                       return -EBUSY;
>          }
> 
>          if (not_suspended > 1 || (not_suspended == 1 && !one_dev_on))
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 45646bfcaf1a..d4c4a7cf34bd 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -260,6 +260,7 @@ struct generic_pm_domain_data {
>          unsigned int rpm_pstate;
>          unsigned int opp_token;
>          bool hw_mode;
> +       bool rpm_always_on;
>          void *data;
>   };
> 
> @@ -292,6 +293,7 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
>   void dev_pm_genpd_synced_poweroff(struct device *dev);
>   int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
>   bool dev_pm_genpd_get_hwmode(struct device *dev);
> +int dev_pm_genpd_rpm_always_on(struct device *dev, bool on);
> 
>   extern struct dev_power_governor simple_qos_governor;
>   extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -375,6 +377,11 @@ static inline bool dev_pm_genpd_get_hwmode(struct
> device *dev)
>          return false;
>   }
> 
> +static inline int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>   #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
>   #define pm_domain_always_on_gov                (*(struct
> dev_power_governor *)(NULL))
>   #endif

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Manivannan Sadhasivam]

On Fri, Oct 18, 2024 at 05:20:08PM +0800, Shawn Lin wrote:
> Hi Ulf,
> 
> 在 2024/10/18 17:07, Ulf Hansson 写道:
> > On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> > > 
> > > Hi Ulf
> > > 
> > > 在 2024/10/9 21:15, Ulf Hansson 写道:
> > > > [...]
> > > > 
> > > > > +
> > > > > +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);
> > > > > +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> > > > 
> > > > pd_to_genpd() isn't safe to use like this. It's solely to be used by
> > > > genpd provider drivers.
> > > > 
> > > > > +
> > > > > +       clk_disable_unprepare(host->ref_out_clk);
> > > > > +
> > > > > +       /*
> > > > > +        * Shouldn't power down if rpm_lvl is less than level 5.
> > > > 
> > > > Can you elaborate on why we must not power-off the power-domain when
> > > > level is less than 5?
> > > > 
> > > 
> > > Because ufshcd driver assume the controller is active and the link is on
> > > if level is less than 5. So the default resume policy will not try to
> > > recover the registers until the first error happened. Otherwise if the
> > > level is >=5, it assumes the controller is off and the link is down,
> > > then it will restore the registers and link.
> > > 
> > > And the level is changeable via sysfs.
> > 
> > Okay, thanks for clarifying.
> > 
> > > 
> > > > What happens if we power-off anyway when the level is less than 5?
> > > > 
> > > > > +        * This flag will be passed down to platform power-domain driver
> > > > > +        * which has the final decision.
> > > > > +        */
> > > > > +       if (hba->rpm_lvl < UFS_PM_LVL_5)
> > > > > +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
> > > > > +       else
> > > > > +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
> > > > 
> > > > The genpd->flags is not supposed to be changed like this - and
> > > > especially not from a genpd consumer driver.
> > > > 
> > > > I am trying to understand a bit more of the use case here. Let's see
> > > > if that helps me to potentially suggest an alternative approach.
> > > > 
> > > 
> > > I was not familiar with the genpd part, so I haven't come up with
> > > another solution. It would be great if you can guide me to the right
> > > way.
> > 
> > I have been playing with the existing infrastructure we have at hand
> > to support this, but I need a few more days to be able to propose
> > something for you.
> > 
> 
> Much appreciate.
> 
> > > 
> > > > > +
> > > > > +       return ufshcd_runtime_suspend(dev);
> > > > > +}
> > > > > +
> > > > > +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);
> > > > > +       usleep_range(1, 2);
> > > > > +       reset_control_deassert(host->rst);
> > > > > +
> > > > > +       return ufshcd_runtime_resume(dev);
> > > > > +}
> > > > > +
> > > > > +static int ufs_rockchip_system_suspend(struct device *dev)
> > > > > +{
> > > > > +       struct ufs_hba *hba = dev_get_drvdata(dev);
> > > > > +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> > > > > +
> > > > > +       /* Pass down desired spm_lvl to Firmware */
> > > > > +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
> > > > > +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
> > > > 
> > > > Can you please elaborate on what goes on here? Is this turning off the
> > > > power-domain that the dev is attached to - or what is actually
> > > > happening?
> > > > 
> > > 
> > > This smc call is trying to ask firmware not to turn off the power-domian
> > > that the UFS is attached to and also not to turn off the power of UFS
> > > conntroller.
> > 
> > Okay, thanks for clarifying!
> > 
> > A follow up question, don't you need to make a corresponding smc call
> > to inform the FW that it's okay to turn off the power-domain at some
> > point?
> > 
> 
> Yes. Each time entering sleep, we teach FW if it need to turn off or keep
> power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
> off and 1 means on.
> 

We had a requirement to notify the genpd provider from consumer to not turn off
the power domain during system suspend. So Ulf came up with an API for
consumers, device_set_wakeup_path() setting the 'dev->power.wakeup_path' which
will be honored by the genpd core. Will that work for you?

PS: The API naming suggests that the device will be used in wakeup path, which
may not be true here but the end result will be the same.

- Mani

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

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Shawn Lin]

在 2024/11/3 20:02, Manivannan Sadhasivam 写道:
> On Fri, Oct 18, 2024 at 05:20:08PM +0800, Shawn Lin wrote:
>> Hi Ulf,
>>
>> 在 2024/10/18 17:07, Ulf Hansson 写道:
>>> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>
>>>> Hi Ulf
>>>>
>>>> 在 2024/10/9 21:15, Ulf Hansson 写道:
>>>>> [...]
>>>>>
>>>>>> +
>>>>>> +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);
>>>>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>>>>>
>>>>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
>>>>> genpd provider drivers.
>>>>>
>>>>>> +
>>>>>> +       clk_disable_unprepare(host->ref_out_clk);
>>>>>> +
>>>>>> +       /*
>>>>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
>>>>>
>>>>> Can you elaborate on why we must not power-off the power-domain when
>>>>> level is less than 5?
>>>>>
>>>>
>>>> Because ufshcd driver assume the controller is active and the link is on
>>>> if level is less than 5. So the default resume policy will not try to
>>>> recover the registers until the first error happened. Otherwise if the
>>>> level is >=5, it assumes the controller is off and the link is down,
>>>> then it will restore the registers and link.
>>>>
>>>> And the level is changeable via sysfs.
>>>
>>> Okay, thanks for clarifying.
>>>
>>>>
>>>>> What happens if we power-off anyway when the level is less than 5?
>>>>>
>>>>>> +        * This flag will be passed down to platform power-domain driver
>>>>>> +        * which has the final decision.
>>>>>> +        */
>>>>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
>>>>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
>>>>>> +       else
>>>>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
>>>>>
>>>>> The genpd->flags is not supposed to be changed like this - and
>>>>> especially not from a genpd consumer driver.
>>>>>
>>>>> I am trying to understand a bit more of the use case here. Let's see
>>>>> if that helps me to potentially suggest an alternative approach.
>>>>>
>>>>
>>>> I was not familiar with the genpd part, so I haven't come up with
>>>> another solution. It would be great if you can guide me to the right
>>>> way.
>>>
>>> I have been playing with the existing infrastructure we have at hand
>>> to support this, but I need a few more days to be able to propose
>>> something for you.
>>>
>>
>> Much appreciate.
>>
>>>>
>>>>>> +
>>>>>> +       return ufshcd_runtime_suspend(dev);
>>>>>> +}
>>>>>> +
>>>>>> +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);
>>>>>> +       usleep_range(1, 2);
>>>>>> +       reset_control_deassert(host->rst);
>>>>>> +
>>>>>> +       return ufshcd_runtime_resume(dev);
>>>>>> +}
>>>>>> +
>>>>>> +static int ufs_rockchip_system_suspend(struct device *dev)
>>>>>> +{
>>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>>>>>> +
>>>>>> +       /* Pass down desired spm_lvl to Firmware */
>>>>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
>>>>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
>>>>>
>>>>> Can you please elaborate on what goes on here? Is this turning off the
>>>>> power-domain that the dev is attached to - or what is actually
>>>>> happening?
>>>>>
>>>>
>>>> This smc call is trying to ask firmware not to turn off the power-domian
>>>> that the UFS is attached to and also not to turn off the power of UFS
>>>> conntroller.
>>>
>>> Okay, thanks for clarifying!
>>>
>>> A follow up question, don't you need to make a corresponding smc call
>>> to inform the FW that it's okay to turn off the power-domain at some
>>> point?
>>>
>>
>> Yes. Each time entering sleep, we teach FW if it need to turn off or keep
>> power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
>> off and 1 means on.
>>
> 
> We had a requirement to notify the genpd provider from consumer to not turn off
> the power domain during system suspend. So Ulf came up with an API for
> consumers, device_set_wakeup_path() setting the 'dev->power.wakeup_path' which
> will be honored by the genpd core. Will that work for you?

Yes, that works. And we may need a symmetrical call, for instance,
device_clr_wakeup_path() to allow genpd provider to turn off the power
domain as well.

> 
> PS: The API naming suggests that the device will be used in wakeup path, which
> may not be true here but the end result will be the same.
> 
> - Mani
> 

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Ulf Hansson]

On Mon, 4 Nov 2024 at 07:38, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> 在 2024/11/3 20:02, Manivannan Sadhasivam 写道:
> > On Fri, Oct 18, 2024 at 05:20:08PM +0800, Shawn Lin wrote:
> >> Hi Ulf,
> >>
> >> 在 2024/10/18 17:07, Ulf Hansson 写道:
> >>> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>>>
> >>>> Hi Ulf
> >>>>
> >>>> 在 2024/10/9 21:15, Ulf Hansson 写道:
> >>>>> [...]
> >>>>>
> >>>>>> +
> >>>>>> +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);
> >>>>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> >>>>>
> >>>>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
> >>>>> genpd provider drivers.
> >>>>>
> >>>>>> +
> >>>>>> +       clk_disable_unprepare(host->ref_out_clk);
> >>>>>> +
> >>>>>> +       /*
> >>>>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
> >>>>>
> >>>>> Can you elaborate on why we must not power-off the power-domain when
> >>>>> level is less than 5?
> >>>>>
> >>>>
> >>>> Because ufshcd driver assume the controller is active and the link is on
> >>>> if level is less than 5. So the default resume policy will not try to
> >>>> recover the registers until the first error happened. Otherwise if the
> >>>> level is >=5, it assumes the controller is off and the link is down,
> >>>> then it will restore the registers and link.
> >>>>
> >>>> And the level is changeable via sysfs.
> >>>
> >>> Okay, thanks for clarifying.
> >>>
> >>>>
> >>>>> What happens if we power-off anyway when the level is less than 5?
> >>>>>
> >>>>>> +        * This flag will be passed down to platform power-domain driver
> >>>>>> +        * which has the final decision.
> >>>>>> +        */
> >>>>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
> >>>>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
> >>>>>> +       else
> >>>>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
> >>>>>
> >>>>> The genpd->flags is not supposed to be changed like this - and
> >>>>> especially not from a genpd consumer driver.
> >>>>>
> >>>>> I am trying to understand a bit more of the use case here. Let's see
> >>>>> if that helps me to potentially suggest an alternative approach.
> >>>>>
> >>>>
> >>>> I was not familiar with the genpd part, so I haven't come up with
> >>>> another solution. It would be great if you can guide me to the right
> >>>> way.
> >>>
> >>> I have been playing with the existing infrastructure we have at hand
> >>> to support this, but I need a few more days to be able to propose
> >>> something for you.
> >>>
> >>
> >> Much appreciate.
> >>
> >>>>
> >>>>>> +
> >>>>>> +       return ufshcd_runtime_suspend(dev);
> >>>>>> +}
> >>>>>> +
> >>>>>> +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);
> >>>>>> +       usleep_range(1, 2);
> >>>>>> +       reset_control_deassert(host->rst);
> >>>>>> +
> >>>>>> +       return ufshcd_runtime_resume(dev);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int ufs_rockchip_system_suspend(struct device *dev)
> >>>>>> +{
> >>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> >>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> >>>>>> +
> >>>>>> +       /* Pass down desired spm_lvl to Firmware */
> >>>>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
> >>>>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
> >>>>>
> >>>>> Can you please elaborate on what goes on here? Is this turning off the
> >>>>> power-domain that the dev is attached to - or what is actually
> >>>>> happening?
> >>>>>
> >>>>
> >>>> This smc call is trying to ask firmware not to turn off the power-domian
> >>>> that the UFS is attached to and also not to turn off the power of UFS
> >>>> conntroller.
> >>>
> >>> Okay, thanks for clarifying!
> >>>
> >>> A follow up question, don't you need to make a corresponding smc call
> >>> to inform the FW that it's okay to turn off the power-domain at some
> >>> point?
> >>>
> >>
> >> Yes. Each time entering sleep, we teach FW if it need to turn off or keep
> >> power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
> >> off and 1 means on.
> >>
> >
> > We had a requirement to notify the genpd provider from consumer to not turn off
> > the power domain during system suspend. So Ulf came up with an API for
> > consumers, device_set_wakeup_path() setting the 'dev->power.wakeup_path' which
> > will be honored by the genpd core. Will that work for you?
>
> Yes, that works. And we may need a symmetrical call, for instance,
> device_clr_wakeup_path() to allow genpd provider to turn off the power
> domain as well.

The PM core clears the flag in device_prepare(). The flag is typically
supposed to be set from a ->suspend() callback, so there should be no
need for an additional function that clears the flag, I think.

[...]

Kind regards
Uffe

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Manivannan Sadhasivam]

On Mon, Nov 04, 2024 at 10:51:45AM +0100, Ulf Hansson wrote:
> On Mon, 4 Nov 2024 at 07:38, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >
> > 在 2024/11/3 20:02, Manivannan Sadhasivam 写道:
> > > On Fri, Oct 18, 2024 at 05:20:08PM +0800, Shawn Lin wrote:
> > >> Hi Ulf,
> > >>
> > >> 在 2024/10/18 17:07, Ulf Hansson 写道:
> > >>> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> > >>>>
> > >>>> Hi Ulf
> > >>>>
> > >>>> 在 2024/10/9 21:15, Ulf Hansson 写道:
> > >>>>> [...]
> > >>>>>
> > >>>>>> +
> > >>>>>> +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);
> > >>>>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> > >>>>>
> > >>>>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
> > >>>>> genpd provider drivers.
> > >>>>>
> > >>>>>> +
> > >>>>>> +       clk_disable_unprepare(host->ref_out_clk);
> > >>>>>> +
> > >>>>>> +       /*
> > >>>>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
> > >>>>>
> > >>>>> Can you elaborate on why we must not power-off the power-domain when
> > >>>>> level is less than 5?
> > >>>>>
> > >>>>
> > >>>> Because ufshcd driver assume the controller is active and the link is on
> > >>>> if level is less than 5. So the default resume policy will not try to
> > >>>> recover the registers until the first error happened. Otherwise if the
> > >>>> level is >=5, it assumes the controller is off and the link is down,
> > >>>> then it will restore the registers and link.
> > >>>>
> > >>>> And the level is changeable via sysfs.
> > >>>
> > >>> Okay, thanks for clarifying.
> > >>>
> > >>>>
> > >>>>> What happens if we power-off anyway when the level is less than 5?
> > >>>>>
> > >>>>>> +        * This flag will be passed down to platform power-domain driver
> > >>>>>> +        * which has the final decision.
> > >>>>>> +        */
> > >>>>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
> > >>>>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
> > >>>>>> +       else
> > >>>>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
> > >>>>>
> > >>>>> The genpd->flags is not supposed to be changed like this - and
> > >>>>> especially not from a genpd consumer driver.
> > >>>>>
> > >>>>> I am trying to understand a bit more of the use case here. Let's see
> > >>>>> if that helps me to potentially suggest an alternative approach.
> > >>>>>
> > >>>>
> > >>>> I was not familiar with the genpd part, so I haven't come up with
> > >>>> another solution. It would be great if you can guide me to the right
> > >>>> way.
> > >>>
> > >>> I have been playing with the existing infrastructure we have at hand
> > >>> to support this, but I need a few more days to be able to propose
> > >>> something for you.
> > >>>
> > >>
> > >> Much appreciate.
> > >>
> > >>>>
> > >>>>>> +
> > >>>>>> +       return ufshcd_runtime_suspend(dev);
> > >>>>>> +}
> > >>>>>> +
> > >>>>>> +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);
> > >>>>>> +       usleep_range(1, 2);
> > >>>>>> +       reset_control_deassert(host->rst);
> > >>>>>> +
> > >>>>>> +       return ufshcd_runtime_resume(dev);
> > >>>>>> +}
> > >>>>>> +
> > >>>>>> +static int ufs_rockchip_system_suspend(struct device *dev)
> > >>>>>> +{
> > >>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
> > >>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> > >>>>>> +
> > >>>>>> +       /* Pass down desired spm_lvl to Firmware */
> > >>>>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
> > >>>>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
> > >>>>>
> > >>>>> Can you please elaborate on what goes on here? Is this turning off the
> > >>>>> power-domain that the dev is attached to - or what is actually
> > >>>>> happening?
> > >>>>>
> > >>>>
> > >>>> This smc call is trying to ask firmware not to turn off the power-domian
> > >>>> that the UFS is attached to and also not to turn off the power of UFS
> > >>>> conntroller.
> > >>>
> > >>> Okay, thanks for clarifying!
> > >>>
> > >>> A follow up question, don't you need to make a corresponding smc call
> > >>> to inform the FW that it's okay to turn off the power-domain at some
> > >>> point?
> > >>>
> > >>
> > >> Yes. Each time entering sleep, we teach FW if it need to turn off or keep
> > >> power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
> > >> off and 1 means on.
> > >>
> > >
> > > We had a requirement to notify the genpd provider from consumer to not turn off
> > > the power domain during system suspend. So Ulf came up with an API for
> > > consumers, device_set_wakeup_path() setting the 'dev->power.wakeup_path' which
> > > will be honored by the genpd core. Will that work for you?
> >
> > Yes, that works. And we may need a symmetrical call, for instance,
> > device_clr_wakeup_path() to allow genpd provider to turn off the power
> > domain as well.
> 
> The PM core clears the flag in device_prepare(). The flag is typically
> supposed to be set from a ->suspend() callback, so there should be no
> need for an additional function that clears the flag, I think.
> 

Yeah, that's my understanding as well though I didn't look into PM core deeply.

- Mani

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

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[kernel test robot]

Hi Shawn,

kernel test robot noticed the following build errors:

[auto build test ERROR on jejb-scsi/for-next]
[also build test ERROR on mkp-scsi/for-next robh/for-next linus/master v6.12-rc2 next-20241009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shawn-Lin/scsi-ufs-core-Add-UFSHCI_QUIRK_DME_RESET_ENABLE_AFTER_HCE/20241009-042350
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
patch link:    https://lore.kernel.org/r/1728368130-37213-6-git-send-email-shawn.lin%40rock-chips.com
patch subject: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS
config: csky-randconfig-r073-20241010 (https://download.01.org/0day-ci/archive/20241010/202410100321.D89hHCPV-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241010/202410100321.D89hHCPV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410100321.D89hHCPV-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/ufs/host/ufs-rockchip.c: In function 'ufs_rockchip_runtime_suspend':
>> drivers/ufs/host/ufs-rockchip.c:293:16: error: implicit declaration of function 'ufshcd_runtime_suspend'; did you mean 'pm_runtime_suspend'? [-Wimplicit-function-declaration]
     293 |         return ufshcd_runtime_suspend(dev);
         |                ^~~~~~~~~~~~~~~~~~~~~~
         |                pm_runtime_suspend
   drivers/ufs/host/ufs-rockchip.c: In function 'ufs_rockchip_runtime_resume':
>> drivers/ufs/host/ufs-rockchip.c:312:16: error: implicit declaration of function 'ufshcd_runtime_resume'; did you mean 'pm_runtime_resume'? [-Wimplicit-function-declaration]
     312 |         return ufshcd_runtime_resume(dev);
         |                ^~~~~~~~~~~~~~~~~~~~~
         |                pm_runtime_resume
   drivers/ufs/host/ufs-rockchip.c: In function 'ufs_rockchip_system_suspend':
>> drivers/ufs/host/ufs-rockchip.c:324:16: error: implicit declaration of function 'ufshcd_system_suspend'; did you mean 'ufs_rockchip_system_suspend'? [-Wimplicit-function-declaration]
     324 |         return ufshcd_system_suspend(dev);
         |                ^~~~~~~~~~~~~~~~~~~~~
         |                ufs_rockchip_system_suspend
   drivers/ufs/host/ufs-rockchip.c: At top level:
>> drivers/ufs/host/ufs-rockchip.c:315:12: warning: 'ufs_rockchip_system_suspend' defined but not used [-Wunused-function]
     315 | static int ufs_rockchip_system_suspend(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/ufs/host/ufs-rockchip.c:296:12: warning: 'ufs_rockchip_runtime_resume' defined but not used [-Wunused-function]
     296 | static int ufs_rockchip_runtime_resume(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/ufs/host/ufs-rockchip.c:275:12: warning: 'ufs_rockchip_runtime_suspend' defined but not used [-Wunused-function]
     275 | static int ufs_rockchip_runtime_suspend(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +293 drivers/ufs/host/ufs-rockchip.c

   274	
 > 275	static int ufs_rockchip_runtime_suspend(struct device *dev)
   276	{
   277		struct ufs_hba *hba = dev_get_drvdata(dev);
   278		struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
   279		struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
   280	
   281		clk_disable_unprepare(host->ref_out_clk);
   282	
   283		/*
   284		 * Shouldn't power down if rpm_lvl is less than level 5.
   285		 * This flag will be passed down to platform power-domain driver
   286		 * which has the final decision.
   287		 */
   288		if (hba->rpm_lvl < UFS_PM_LVL_5)
   289			genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
   290		else
   291			genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
   292	
 > 293		return ufshcd_runtime_suspend(dev);
   294	}
   295	
 > 296	static int ufs_rockchip_runtime_resume(struct device *dev)
   297	{
   298		struct ufs_hba *hba = dev_get_drvdata(dev);
   299		struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
   300		int err;
   301	
   302		err = clk_prepare_enable(host->ref_out_clk);
   303		if (err) {
   304			dev_err(hba->dev, "failed to enable ref out clock %d\n", err);
   305			return err;
   306		}
   307	
   308		reset_control_assert(host->rst);
   309		usleep_range(1, 2);
   310		reset_control_deassert(host->rst);
   311	
 > 312		return ufshcd_runtime_resume(dev);
   313	}
   314	
 > 315	static int ufs_rockchip_system_suspend(struct device *dev)
   316	{
   317		struct ufs_hba *hba = dev_get_drvdata(dev);
   318		struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
   319	
   320		/* Pass down desired spm_lvl to Firmware */
   321		arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
   322				host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
   323	
 > 324		return ufshcd_system_suspend(dev);
   325	}
   326	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Christophe JAILLET]

Le 08/10/2024 à 08:15, Shawn Lin a écrit :
> RK3576 SoC contains a UFS controller, add initial support fot it.

s/fot/for/

> The features are:
> (1) support UFS 2.0 features
> (2) High speed up to HS-G3
> (3) 2RX-2TX lanes
> (4) auto H8 entry and exit
> 
> Software limitation:
> (1) HCE procedure: enbale controller->enbale intr->dme_reset->dme_enable

s/enbale/enable/ 	x 2

> (2) disable unipro timeout values before power mode change
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 

No need for an extra empty line

> ---

...

> +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;
> +
> +	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);
> +	usleep_range(1, 2);
> +	reset_control_deassert(host->rst);
> +
> +	host->ref_out_clk = devm_clk_get_enabled(dev, "ref_out");
> +	if (IS_ERR(host->ref_out_clk))
> +		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk),
> +				"ref_out unavailable\n");
> +
> +	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(host->rst_gpio))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
> +				"invalid reset-gpios property in node\n");
> +
> +	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)
> +		return dev_err_probe(dev, err, "failed to get clocks\n");
> +
> +	err = clk_bulk_prepare_enable(UFS_MAX_CLKS, host->clks);

This has to be undone somewhere, likely at the end of ufs_rockchip_remove().

> +	if (err)
> +		return dev_err_probe(dev, err, "failed to enable clocks\n");
> +
> +	host->hba = hba;
> +
> +	ufshcd_set_variant(hba, host);
> +
> +	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-ufshc", .data = &ufs_hba_rk3576_vops},

Missing space before ending }

> +	{},

No need for an extra , here.

> +};
> +MODULE_DEVICE_TABLE(of, ufs_rockchip_of_match);

...

> +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);

No need for clk_disable_unprepare(), ref_out_clk comes from 
devm_clk_get_enabled(), so the framework will already call 
clk_disable_unprepare().

> +}

...

CJ