[PATCH v2 0/3] Init support for RK3576 UFS controller

[PATCH v2 0/3] Init support for RK3576 UFS controller

[Shawn Lin]

This patchset add initial support UFS controller for RK3576 SoC.
Patch 1 export ufshcd_dme_link_startup function for host drivers, and
patch 2&3 add ufs-rockchip driver.


Changes in v2:
- renmae file name
- fix all errors and pass the dt_binding_check:
  make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml
- use dev_probe_err
- remove ufs-phy-config-mode as it's not used
- drop of_match_ptr

Shawn Lin (3):
  scsi: ufs: core: Export ufshcd_dme_link_startup() helper
  dt-bindings: ufs: Document Rockchip UFS host controller
  scsi: ufs: rockchip: init support for UFS

 .../bindings/ufs/rockchip,rk3576-ufs.yaml          |  96 +++++
 drivers/ufs/core/ufshcd.c                          |   4 +-
 drivers/ufs/host/Kconfig                           |  12 +
 drivers/ufs/host/Makefile                          |   1 +
 drivers/ufs/host/ufs-rockchip.c                    | 438 +++++++++++++++++++++
 drivers/ufs/host/ufs-rockchip.h                    |  51 +++
 include/ufs/ufshcd.h                               |   1 +
 7 files changed, 602 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
 create mode 100644 drivers/ufs/host/ufs-rockchip.c
 create mode 100644 drivers/ufs/host/ufs-rockchip.h

-- 
2.7.4

[PATCH v2 2/3] 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 v2:
- renmae file name
- fix all errors and pass the dt_binding_check:
  make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml

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

diff --git a/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml b/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
new file mode 100644
index 0000000..1844fe3
--- /dev/null
+++ b/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ufs/rockchip,ufs.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-ufs
+
+  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
+
+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/rk3576-power.h>
+    #include <dt-bindings/pinctrl/rockchip.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    ufs: ufs@2a2d0000 {
+          compatible = "rockchip,rk3576-ufs";
+          reg = <0x2a2d0000 0x10000>,
+                <0x2b040000 0x10000>,
+                <0x2601f000 0x1000>,
+                <0x2603c000 0x1000>,
+                <0x2a2e0000 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_HIGH>;
+    };
-- 
2.7.4

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

[Rob Herring (Arm)]

On Thu, 08 Aug 2024 11:52:42 +0800, Shawn Lin wrote:
> Document Rockchip UFS host controller for RK3576 SoC.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> Changes in v2:
> - renmae file name
> - fix all errors and pass the dt_binding_check:
>   make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml
> 
>  .../bindings/ufs/rockchip,rk3576-ufs.yaml          | 96 ++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
 	 $id: http://devicetree.org/schemas/ufs/rockchip,ufs.yaml
 	file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.example.dts:24:18: fatal error: dt-bindings/clock/rockchip,rk3576-cru.h: No such file or directory
   24 |         #include <dt-bindings/clock/rockchip,rk3576-cru.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1723089163-28983-3-git-send-email-shawn.lin@rock-chips.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Shawn Lin]

Hi Rob

在 2024/8/8 13:29, Rob Herring (Arm) 写道:
> 
> On Thu, 08 Aug 2024 11:52:42 +0800, Shawn Lin wrote:
>> Document Rockchip UFS host controller for RK3576 SoC.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - renmae file name
>> - fix all errors and pass the dt_binding_check:
>>    make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml
>>
>>   .../bindings/ufs/rockchip,rk3576-ufs.yaml          | 96 ++++++++++++++++++++++
>>   1 file changed, 96 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
>>
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
>   	 $id: http://devicetree.org/schemas/ufs/rockchip,ufs.yaml
>   	file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml

This is already fixed by a resend v2 2/3 patch, a moment ago. Sorry for 
that.

> Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.example.dts:24:18: fatal error: dt-bindings/clock/rockchip,rk3576-cru.h: No such file or directory
>     24 |         #include <dt-bindings/clock/rockchip,rk3576-cru.h>
>        |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.

There are still pending patches from Rockchip in queue for review. This
patchset is based on them. I will wait for more comments and update them
after all under-review patches got merged.

Thanks.

> make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.example.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
> make: *** [Makefile:240: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1723089163-28983-3-git-send-email-shawn.lin@rock-chips.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 

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

[Krzysztof Kozlowski]

On 08/08/2024 08:10, Shawn Lin wrote:
> Hi Rob
> 
> 在 2024/8/8 13:29, Rob Herring (Arm) 写道:
>>
>> On Thu, 08 Aug 2024 11:52:42 +0800, Shawn Lin wrote:
>>> Document Rockchip UFS host controller for RK3576 SoC.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - renmae file name
>>> - fix all errors and pass the dt_binding_check:
>>>    make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml
>>>
>>>   .../bindings/ufs/rockchip,rk3576-ufs.yaml          | 96 ++++++++++++++++++++++
>>>   1 file changed, 96 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
>>>
>>
>> My bot found errors running 'make dt_binding_check' on your patch:
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
>>   	 $id: http://devicetree.org/schemas/ufs/rockchip,ufs.yaml
>>   	file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
> 
> This is already fixed by a resend v2 2/3 patch, a moment ago. Sorry for 
> that.

If you change patches, it is not a resend. Send a proper new version
with proper changelog.

> 
>> Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.example.dts:24:18: fatal error: dt-bindings/clock/rockchip,rk3576-cru.h: No such file or directory
>>     24 |         #include <dt-bindings/clock/rockchip,rk3576-cru.h>
>>        |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> compilation terminated.
> 
> There are still pending patches from Rockchip in queue for review. This
> patchset is based on them. I will wait for more comments and update them
> after all under-review patches got merged.

You need to document dependencies in changelog (---).

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 08/08/2024 05:52, Shawn Lin wrote:
> Document Rockchip UFS host controller for RK3576 SoC.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> Changes in v2:
> - renmae file name
> - fix all errors and pass the dt_binding_check:
>   make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml


You did much more. Some properties appeared which were here not
before... The way you send patches makes it difficult to review. Look:

b4 diff '<1723089163-28983-3-git-send-email-shawn.lin@rock-chips.com>'
Grabbing thread from
lore.kernel.org/all/1723089163-28983-3-git-send-email-shawn.lin@rock-chips.com/t.mbox.gz
Checking for older revisions
Grabbing search results from lore.kernel.org
  Added from v1: 4 patches
---
Analyzing 18 messages in the thread
WARNING: duplicate messages found at index 2
   Subject 1: dt-bindings: ufs: Document Rockchip UFS host controller
   Subject 2: dt-bindings: ufs: Document Rockchip UFS host controller
  2 is not a reply... assume additional patch
Looking for additional code-review trailers on lore.kernel.org
Analyzing 0 code-review messages
Preparing fake-am for v1: Init support for RK3576 UFS controller
  range: e86f0d80765d..3ae8e722f6ab
Preparing fake-am for v2: scsi: ufs: core: Export
ufshcd_dme_link_startup() helper
ERROR: v2 series incomplete; unable to create a fake-am range
---
Could not create fake-am range for upper series v2


So how can we handle it? Your job is to use standard process so life of
reviewers is not more difficult than it should be.

Provide FULL CHANGELOG with explanation what happened here.

Best regards,
Krzysztof

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

[Shawn Lin]

Hi Krzysztof

在 2024/8/8 18:34, Krzysztof Kozlowski 写道:
> On 08/08/2024 05:52, Shawn Lin wrote:
>> Document Rockchip UFS host controller for RK3576 SoC.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - renmae file name
>> - fix all errors and pass the dt_binding_check:
>>    make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml
> 
> 
> You did much more. Some properties appeared which were here not
> before... The way you send patches makes it difficult to review. Look:
> 

Yes, will update full changelog next version.

> b4 diff '<1723089163-28983-3-git-send-email-shawn.lin@rock-chips.com>'
> Grabbing thread from
> lore.kernel.org/all/1723089163-28983-3-git-send-email-shawn.lin@rock-chips.com/t.mbox.gz
> Checking for older revisions
> Grabbing search results from lore.kernel.org
>    Added from v1: 4 patches
> ---
> Analyzing 18 messages in the thread
> WARNING: duplicate messages found at index 2
>     Subject 1: dt-bindings: ufs: Document Rockchip UFS host controller
>     Subject 2: dt-bindings: ufs: Document Rockchip UFS host controller
>    2 is not a reply... assume additional patch
> Looking for additional code-review trailers on lore.kernel.org
> Analyzing 0 code-review messages
> Preparing fake-am for v1: Init support for RK3576 UFS controller
>    range: e86f0d80765d..3ae8e722f6ab
> Preparing fake-am for v2: scsi: ufs: core: Export
> ufshcd_dme_link_startup() helper
> ERROR: v2 series incomplete; unable to create a fake-am range
> ---
> Could not create fake-am range for upper series v2
> 
> 
> So how can we handle it? Your job is to use standard process so life of
> reviewers is not more difficult than it should be.
> 
> Provide FULL CHANGELOG with explanation what happened here.

Sorry for that. I'll improve them when the dependency is solved.

> 
> Best regards,
> Krzysztof
> 

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

[Shawn Lin]

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

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

---

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

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

diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
index 580c8d0..fafaa33 100644
--- a/drivers/ufs/host/Kconfig
+++ b/drivers/ufs/host/Kconfig
@@ -142,3 +142,15 @@ config SCSI_UFS_SPRD
 
 	  Select this if you have UFS controller on Unisoc chipset.
 	  If unsure, say N.
+
+config SCSI_UFS_ROCKCHIP
+	tristate "Rockchip specific hooks to UFS controller platform driver"
+	depends on SCSI_UFSHCD_PLATFORM && (ARCH_ROCKCHIP || COMPILE_TEST)
+	help
+	  This selects the Rockchip specific additions to UFSHCD platform driver.
+	  UFS host on Rockchip needs some vendor specific configuration before
+	  accessing the hardware which includes PHY configuration and vendor
+	  specific registers.
+
+	  Select this if you have UFS controller on Rockchip chipset.
+	  If unsure, say N.
diff --git a/drivers/ufs/host/Makefile b/drivers/ufs/host/Makefile
index 4573aea..2f97feb 100644
--- a/drivers/ufs/host/Makefile
+++ b/drivers/ufs/host/Makefile
@@ -10,5 +10,6 @@ obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
 obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
 obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
 obj-$(CONFIG_SCSI_UFS_RENESAS) += ufs-renesas.o
+obj-$(CONFIG_SCSI_UFS_ROCKCHIP) += ufs-rockchip.o
 obj-$(CONFIG_SCSI_UFS_SPRD) += ufs-sprd.o
 obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
diff --git a/drivers/ufs/host/ufs-rockchip.c b/drivers/ufs/host/ufs-rockchip.c
new file mode 100644
index 0000000..46c90d6
--- /dev/null
+++ b/drivers/ufs/host/ufs-rockchip.c
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Rockchip UFS Host Controller driver
+ *
+ * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include <ufs/ufshcd.h>
+#include <ufs/unipro.h>
+#include "ufshcd-pltfrm.h"
+#include "ufshcd-dwc.h"
+#include "ufs-rockchip.h"
+
+static inline bool ufshcd_is_device_present(struct ufs_hba *hba)
+{
+	return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & DEVICE_PRESENT;
+}
+
+static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
+					 enum ufs_notify_change_status status)
+{
+	int err = 0;
+
+	if (status == PRE_CHANGE) {
+		int retry_outer = 3;
+		int retry_inner;
+start:
+		if (ufshcd_is_hba_active(hba))
+			/* change controller state to "reset state" */
+			ufshcd_hba_stop(hba);
+
+		/* UniPro link is disabled at this point */
+		ufshcd_set_link_off(hba);
+
+		/* start controller initialization sequence */
+		ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE);
+
+		usleep_range(100, 200);
+
+		/* wait for the host controller to complete initialization */
+		retry_inner = 50;
+		while (!ufshcd_is_hba_active(hba)) {
+			if (retry_inner) {
+				retry_inner--;
+			} else {
+				dev_err(hba->dev,
+					"Controller enable failed\n");
+				if (retry_outer) {
+					retry_outer--;
+					goto start;
+				}
+				return -EIO;
+			}
+			usleep_range(1000, 1100);
+		}
+	} else { /* POST_CHANGE */
+		err = ufshcd_vops_phy_initialization(hba);
+	}
+
+	return err;
+}
+
+static void ufs_rockchip_set_pm_lvl(struct ufs_hba *hba)
+{
+	hba->rpm_lvl = UFS_PM_LVL_1;
+	hba->spm_lvl = UFS_PM_LVL_3;
+}
+
+static int ufs_rockchip_rk3576_phy_init(struct ufs_hba *hba)
+{
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(PA_LOCAL_TX_LCC_ENABLE, 0x0), 0x0);
+	/* enable the mphy DME_SET cfg */
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x200, 0x0), 0x40);
+	for (int i = 0; i < 2; i++) {
+		/* Configuration M-TX */
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xaa, SEL_TX_LANE0 + i), 0x06);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xa9, SEL_TX_LANE0 + i), 0x02);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xad, SEL_TX_LANE0 + i), 0x44);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xac, SEL_TX_LANE0 + i), 0xe6);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xab, SEL_TX_LANE0 + i), 0x07);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x94, SEL_TX_LANE0 + i), 0x93);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x93, SEL_TX_LANE0 + i), 0xc9);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x7f, SEL_TX_LANE0 + i), 0x00);
+		/* Configuration M-RX */
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x12, SEL_RX_LANE0 + i), 0x06);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x11, SEL_RX_LANE0 + i), 0x00);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1d, SEL_RX_LANE0 + i), 0x58);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1c, SEL_RX_LANE0 + i), 0x8c);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1b, SEL_RX_LANE0 + i), 0x02);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x25, SEL_RX_LANE0 + i), 0xf6);
+		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x2f, SEL_RX_LANE0 + i), 0x69);
+	}
+	/* disable the mphy DME_SET cfg */
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x200, 0x0), 0x00);
+
+	ufs_sys_writel(host->mphy_base, 0x80, 0x08C);
+	ufs_sys_writel(host->mphy_base, 0xB5, 0x110);
+	ufs_sys_writel(host->mphy_base, 0xB5, 0x250);
+
+	ufs_sys_writel(host->mphy_base, 0x03, 0x134);
+	ufs_sys_writel(host->mphy_base, 0x03, 0x274);
+
+	ufs_sys_writel(host->mphy_base, 0x38, 0x0E0);
+	ufs_sys_writel(host->mphy_base, 0x38, 0x220);
+
+	ufs_sys_writel(host->mphy_base, 0x50, 0x164);
+	ufs_sys_writel(host->mphy_base, 0x50, 0x2A4);
+
+	ufs_sys_writel(host->mphy_base, 0x80, 0x178);
+	ufs_sys_writel(host->mphy_base, 0x80, 0x2B8);
+
+	ufs_sys_writel(host->mphy_base, 0x18, 0x1B0);
+	ufs_sys_writel(host->mphy_base, 0x18, 0x2F0);
+
+	ufs_sys_writel(host->mphy_base, 0x03, 0x128);
+	ufs_sys_writel(host->mphy_base, 0x03, 0x268);
+
+	ufs_sys_writel(host->mphy_base, 0x20, 0x12C);
+	ufs_sys_writel(host->mphy_base, 0x20, 0x26C);
+
+	ufs_sys_writel(host->mphy_base, 0xC0, 0x120);
+	ufs_sys_writel(host->mphy_base, 0xC0, 0x260);
+
+	ufs_sys_writel(host->mphy_base, 0x03, 0x094);
+
+	ufs_sys_writel(host->mphy_base, 0x03, 0x1B4);
+	ufs_sys_writel(host->mphy_base, 0x03, 0x2F4);
+
+	ufs_sys_writel(host->mphy_base, 0xC0, 0x08C);
+	udelay(1);
+	ufs_sys_writel(host->mphy_base, 0x00, 0x08C);
+
+	udelay(200);
+	/* start link up */
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_TX_ENDIAN, 0), 0x0);
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_RX_ENDIAN, 0), 0x0);
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID, 0), 0x0);
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID_VALID, 0), 0x1);
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_PEERDEVICEID, 0), 0x1);
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_CONNECTIONSTATE, 0), 0x1);
+
+	return 0;
+}
+
+static int ufs_rockchip_common_init(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ufs_rockchip_host *host;
+	int err = 0;
+
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+
+	/* system control register for hci */
+	host->ufs_sys_ctrl = devm_platform_ioremap_resource_byname(pdev, "hci_grf");
+	if (IS_ERR(host->ufs_sys_ctrl))
+		return dev_err_probe(dev, PTR_ERR(host->ufs_sys_ctrl),
+					"cannot ioremap for hci system control register\n");
+
+	/* system control register for mphy */
+	host->ufs_phy_ctrl = devm_platform_ioremap_resource_byname(pdev, "mphy_grf");
+	if (IS_ERR(host->ufs_phy_ctrl))
+		return dev_err_probe(dev, PTR_ERR(host->ufs_phy_ctrl),
+				"cannot ioremap for mphy system control register\n");
+
+	/* mphy base register */
+	host->mphy_base = devm_platform_ioremap_resource_byname(pdev, "mphy");
+	if (IS_ERR(host->mphy_base))
+		return dev_err_probe(dev, PTR_ERR(host->mphy_base),
+					"cannot ioremap for mphy base register\n");
+
+	host->rst = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR(host->rst))
+		return dev_err_probe(dev, PTR_ERR(host->rst), "failed to get reset control\n");
+
+	reset_control_assert(host->rst);
+	udelay(1);
+	reset_control_deassert(host->rst);
+
+	host->ref_out_clk = devm_clk_get(dev, "ref_out");
+	if (IS_ERR(host->ref_out_clk))
+		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk), "ciu-drive not available\n");
+
+	err = clk_prepare_enable(host->ref_out_clk);
+	if (err)
+		return dev_err_probe(dev, err, "failed to enable ref out clock\n");
+
+	host->rst_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(host->rst_gpio)) {
+		dev_err_probe(&pdev->dev, PTR_ERR(host->rst_gpio),
+				"invalid reset-gpios property in node\n");
+		err = PTR_ERR(host->rst_gpio);
+		goto out;
+	}
+	udelay(20);
+	gpiod_set_value_cansleep(host->rst_gpio, 1);
+
+	host->clks[0].id = "core";
+	host->clks[1].id = "pclk";
+	host->clks[2].id = "pclk_mphy";
+	err = devm_clk_bulk_get_optional(dev, UFS_MAX_CLKS, host->clks);
+	if (err) {
+		dev_err_probe(dev, err, "failed to get clocks\n");
+		goto out;
+	}
+
+	err = clk_bulk_prepare_enable(UFS_MAX_CLKS, host->clks);
+	if (err) {
+		dev_err_probe(dev, err, "failed to enable clocks\n");
+		goto out;
+	}
+
+	pm_runtime_set_active(&pdev->dev);
+
+	host->hba = hba;
+	ufs_rockchip_set_pm_lvl(hba);
+
+	ufshcd_set_variant(hba, host);
+
+	return 0;
+out:
+	clk_disable_unprepare(host->ref_out_clk);
+	return err;
+}
+
+static int ufs_rockchip_rk3576_init(struct ufs_hba *hba)
+{
+	int ret = 0;
+	struct device *dev = hba->dev;
+
+	hba->quirks = UFSHCI_QUIRK_BROKEN_HCE | UFSHCD_QUIRK_SKIP_DEF_UNIPRO_TIMEOUT_SETTING;
+
+	/* Enable BKOPS when suspend */
+	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
+	/* Enable putting device into deep sleep */
+	hba->caps |= UFSHCD_CAP_DEEPSLEEP;
+	/* Enable devfreq of UFS */
+	hba->caps |= UFSHCD_CAP_CLK_SCALING;
+	/* Enable WriteBooster */
+	hba->caps |= UFSHCD_CAP_WB_EN;
+
+	ret = ufs_rockchip_common_init(hba);
+	if (ret)
+		return dev_err_probe(dev, ret, "ufs common init fail\n");
+
+	return 0;
+}
+
+static int ufs_rockchip_device_reset(struct ufs_hba *hba)
+{
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+
+	if (!host->rst_gpio)
+		return -EOPNOTSUPP;
+
+	gpiod_set_value_cansleep(host->rst_gpio, 0);
+	udelay(20);
+
+	gpiod_set_value_cansleep(host->rst_gpio, 1);
+	udelay(20);
+
+	return 0;
+}
+
+static const struct ufs_hba_variant_ops ufs_hba_rk3576_vops = {
+	.name = "rk3576",
+	.init = ufs_rockchip_rk3576_init,
+	.device_reset = ufs_rockchip_device_reset,
+	.hce_enable_notify = ufs_rockchip_hce_enable_notify,
+	.phy_initialization = ufs_rockchip_rk3576_phy_init,
+};
+
+static const struct of_device_id ufs_rockchip_of_match[] = {
+	{ .compatible = "rockchip,rk3576-ufs", .data = &ufs_hba_rk3576_vops},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ufs_rockchip_of_match);
+
+static int ufs_rockchip_probe(struct platform_device *pdev)
+{
+	int err = 0;
+	struct device *dev = &pdev->dev;
+	const struct ufs_hba_variant_ops *vops;
+
+	vops = device_get_match_data(dev);
+	err = ufshcd_pltfrm_init(pdev, vops);
+	if (err)
+		dev_err_probe(dev, err, "ufshcd_pltfrm_init failed\n");
+
+	return err;
+}
+
+static void ufs_rockchip_remove(struct platform_device *pdev)
+{
+	struct ufs_hba *hba = platform_get_drvdata(pdev);
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+
+	pm_runtime_forbid(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+	ufshcd_remove(hba);
+	ufshcd_dealloc_host(hba);
+	clk_disable_unprepare(host->ref_out_clk);
+}
+
+static int ufs_rockchip_restore_link(struct ufs_hba *hba, bool is_store)
+{
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+	int err, retry = 3;
+
+	if (is_store) {
+		host->ie = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
+		host->ahit = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
+		return 0;
+	}
+
+	/* Enable controller */
+	err = ufshcd_hba_enable(hba);
+	if (err)
+		return err;
+
+	/* Link startup and wait for DP */
+	do {
+		err = ufshcd_dme_link_startup(hba);
+		if (!err && ufshcd_is_device_present(hba)) {
+			dev_dbg_ratelimited(hba->dev, "rockchip link startup successfully.\n");
+			break;
+		}
+	} while (retry--);
+
+	if (retry < 0) {
+		dev_err(hba->dev, "rockchip link startup failed.\n");
+		return -ENXIO;
+	}
+
+	/* Restore negotiated power mode */
+	err = ufshcd_config_pwr_mode(hba, &(hba->pwr_info));
+	if (err)
+		dev_err(hba->dev, "Failed to restore power mode, err = %d\n", err);
+
+	/* Restore task and transfer list */
+	ufshcd_writel(hba, 0xffffffff, REG_INTERRUPT_STATUS);
+	ufshcd_make_hba_operational(hba);
+
+	/* Restore lost regs */
+	ufshcd_writel(hba, host->ie, REG_INTERRUPT_ENABLE);
+	ufshcd_writel(hba, host->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER);
+	ufshcd_writel(hba, 0x1, REG_UTP_TRANSFER_REQ_LIST_RUN_STOP);
+	ufshcd_writel(hba, 0x1, REG_UTP_TASK_REQ_LIST_RUN_STOP);
+
+	return err;
+}
+
+static int ufs_rockchip_runtime_suspend(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+
+	clk_disable_unprepare(host->ref_out_clk);
+	return ufs_rockchip_restore_link(hba, true);
+}
+
+static int ufs_rockchip_runtime_resume(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
+	int err;
+
+	err = clk_prepare_enable(host->ref_out_clk);
+	if (err) {
+		dev_err(hba->dev, "failed to enable ref out clock %d\n", err);
+		return err;
+	}
+
+	reset_control_assert(host->rst);
+	udelay(1);
+	reset_control_deassert(host->rst);
+
+	return ufs_rockchip_restore_link(hba, false);
+}
+
+static int ufs_rockchip_suspend(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (pm_runtime_suspended(hba->dev))
+		return 0;
+
+	ufs_rockchip_restore_link(hba, true);
+
+	return 0;
+}
+
+static int ufs_rockchip_resume(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (pm_runtime_suspended(hba->dev))
+		return 0;
+
+	/* Reset device if possible */
+	ufs_rockchip_device_reset(hba);
+	ufs_rockchip_restore_link(hba, false);
+
+	return 0;
+}
+
+static const struct dev_pm_ops ufs_rockchip_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, ufs_rockchip_resume)
+	SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	 = ufshcd_resume_complete,
+};
+
+static struct platform_driver ufs_rockchip_pltform = {
+	.probe = ufs_rockchip_probe,
+	.remove = ufs_rockchip_remove,
+	.driver = {
+		.name = "ufshcd-rockchip",
+		.pm = &ufs_rockchip_pm_ops,
+		.of_match_table = ufs_rockchip_of_match,
+	},
+};
+module_platform_driver(ufs_rockchip_pltform);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Rockchip UFS Host Driver");
diff --git a/drivers/ufs/host/ufs-rockchip.h b/drivers/ufs/host/ufs-rockchip.h
new file mode 100644
index 0000000..9eb80e8
--- /dev/null
+++ b/drivers/ufs/host/ufs-rockchip.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Rockchip UFS Host Controller driver
+ *
+ * Copyright (C) 2024 Rockchip Electronics Co.Ltd.
+ */
+
+#ifndef _UFS_ROCKCHIP_H_
+#define _UFS_ROCKCHIP_H_
+
+#define UFS_MAX_CLKS 3
+
+#define SEL_TX_LANE0 0x0
+#define SEL_TX_LANE1 0x1
+#define SEL_TX_LANE2 0x2
+#define SEL_TX_LANE3 0x3
+#define SEL_RX_LANE0 0x4
+#define SEL_RX_LANE1 0x5
+#define SEL_RX_LANE2 0x6
+#define SEL_RX_LANE3 0x7
+
+#define MIB_T_DBG_CPORT_TX_ENDIAN	0xc022
+#define MIB_T_DBG_CPORT_RX_ENDIAN	0xc023
+
+struct ufs_rockchip_host {
+	struct ufs_hba *hba;
+	void __iomem *ufs_phy_ctrl;
+	void __iomem *ufs_sys_ctrl;
+	void __iomem *mphy_base;
+	struct gpio_desc *rst_gpio;
+	struct reset_control *rst;
+	struct clk *ref_out_clk;
+	struct clk_bulk_data clks[UFS_MAX_CLKS];
+	uint64_t caps;
+	bool in_suspend;
+	u32 ie;
+	u32 ahit;
+};
+
+#define ufs_sys_writel(base, val, reg)                                    \
+	writel((val), (base) + (reg))
+#define ufs_sys_readl(base, reg) readl((base) + (reg))
+#define ufs_sys_set_bits(base, mask, reg)                                 \
+	ufs_sys_writel(                                                   \
+		(base), ((mask) | (ufs_sys_readl((base), (reg)))), (reg))
+#define ufs_sys_ctrl_clr_bits(base, mask, reg)                                 \
+	ufs_sys_writel((base),                                            \
+			    ((~(mask)) & (ufs_sys_readl((base), (reg)))), \
+			    (reg))
+
+#endif /* _UFS_ROCKCHIP_H_ */
-- 
2.7.4

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

[Krzysztof Kozlowski]

On 08/08/2024 05:52, Shawn Lin wrote:
> RK3576 contains a UFS controller, add init support fot it.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 

...

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

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

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

Where is your DTS so we can validate it?

Best regards,
Krzysztof

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

[Shawn Lin]

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

Thanks for the feadback.

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


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


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

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

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


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



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

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

> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

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

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

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

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

You mix logical level and line level.

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

Maybe the the line is active low?

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

Best regards,
Krzysztof

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

[Shawn Lin]

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

Got it.

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

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

Thanks.

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

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

[Krzysztof Kozlowski]

On 08/08/2024 05:52, Shawn Lin wrote:
> RK3576 contains a UFS controller, add init support fot it.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 


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

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



Best regards,
Krzysztof

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

[Manivannan Sadhasivam]

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

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

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

No inline in .c file please.

> +{
> +	return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & DEVICE_PRESENT;
> +}
> +
> +static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
> +					 enum ufs_notify_change_status status)
> +{
> +	int err = 0;
> +
> +	if (status == PRE_CHANGE) {
> +		int retry_outer = 3;
> +		int retry_inner;
> +start:
> +		if (ufshcd_is_hba_active(hba))
> +			/* change controller state to "reset state" */
> +			ufshcd_hba_stop(hba);
> +
> +		/* UniPro link is disabled at this point */
> +		ufshcd_set_link_off(hba);
> +
> +		/* start controller initialization sequence */
> +		ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE);
> +
> +		usleep_range(100, 200);
> +
> +		/* wait for the host controller to complete initialization */
> +		retry_inner = 50;
> +		while (!ufshcd_is_hba_active(hba)) {
> +			if (retry_inner) {
> +				retry_inner--;
> +			} else {
> +				dev_err(hba->dev,
> +					"Controller enable failed\n");
> +				if (retry_outer) {
> +					retry_outer--;
> +					goto start;
> +				}
> +				return -EIO;
> +			}
> +			usleep_range(1000, 1100);
> +		}

You just duplicated ufshcd_hba_execute_hce() here. Why? This doesn't make sense.

> +	} else { /* POST_CHANGE */
> +		err = ufshcd_vops_phy_initialization(hba);
> +	}
> +
> +	return err;
> +}
> +
> +static void ufs_rockchip_set_pm_lvl(struct ufs_hba *hba)
> +{
> +	hba->rpm_lvl = UFS_PM_LVL_1;
> +	hba->spm_lvl = UFS_PM_LVL_3;
> +}
> +
> +static int ufs_rockchip_rk3576_phy_init(struct ufs_hba *hba)
> +{
> +	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
> +
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(PA_LOCAL_TX_LCC_ENABLE, 0x0), 0x0);
> +	/* enable the mphy DME_SET cfg */
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x200, 0x0), 0x40);
> +	for (int i = 0; i < 2; i++) {
> +		/* Configuration M-TX */
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xaa, SEL_TX_LANE0 + i), 0x06);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xa9, SEL_TX_LANE0 + i), 0x02);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xad, SEL_TX_LANE0 + i), 0x44);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xac, SEL_TX_LANE0 + i), 0xe6);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xab, SEL_TX_LANE0 + i), 0x07);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x94, SEL_TX_LANE0 + i), 0x93);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x93, SEL_TX_LANE0 + i), 0xc9);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x7f, SEL_TX_LANE0 + i), 0x00);
> +		/* Configuration M-RX */
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x12, SEL_RX_LANE0 + i), 0x06);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x11, SEL_RX_LANE0 + i), 0x00);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1d, SEL_RX_LANE0 + i), 0x58);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1c, SEL_RX_LANE0 + i), 0x8c);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1b, SEL_RX_LANE0 + i), 0x02);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x25, SEL_RX_LANE0 + i), 0xf6);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x2f, SEL_RX_LANE0 + i), 0x69);
> +	}
> +	/* disable the mphy DME_SET cfg */
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x200, 0x0), 0x00);
> +
> +	ufs_sys_writel(host->mphy_base, 0x80, 0x08C);
> +	ufs_sys_writel(host->mphy_base, 0xB5, 0x110);
> +	ufs_sys_writel(host->mphy_base, 0xB5, 0x250);
> +

Why can't you do these settings in a PHY driver?

> +	ufs_sys_writel(host->mphy_base, 0x03, 0x134);
> +	ufs_sys_writel(host->mphy_base, 0x03, 0x274);
> +
> +	ufs_sys_writel(host->mphy_base, 0x38, 0x0E0);
> +	ufs_sys_writel(host->mphy_base, 0x38, 0x220);
> +
> +	ufs_sys_writel(host->mphy_base, 0x50, 0x164);
> +	ufs_sys_writel(host->mphy_base, 0x50, 0x2A4);
> +
> +	ufs_sys_writel(host->mphy_base, 0x80, 0x178);
> +	ufs_sys_writel(host->mphy_base, 0x80, 0x2B8);
> +
> +	ufs_sys_writel(host->mphy_base, 0x18, 0x1B0);
> +	ufs_sys_writel(host->mphy_base, 0x18, 0x2F0);
> +
> +	ufs_sys_writel(host->mphy_base, 0x03, 0x128);
> +	ufs_sys_writel(host->mphy_base, 0x03, 0x268);
> +
> +	ufs_sys_writel(host->mphy_base, 0x20, 0x12C);
> +	ufs_sys_writel(host->mphy_base, 0x20, 0x26C);
> +
> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x120);
> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x260);
> +
> +	ufs_sys_writel(host->mphy_base, 0x03, 0x094);
> +
> +	ufs_sys_writel(host->mphy_base, 0x03, 0x1B4);
> +	ufs_sys_writel(host->mphy_base, 0x03, 0x2F4);
> +
> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x08C);
> +	udelay(1);
> +	ufs_sys_writel(host->mphy_base, 0x00, 0x08C);
> +
> +	udelay(200);
> +	/* start link up */
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_TX_ENDIAN, 0), 0x0);
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_RX_ENDIAN, 0), 0x0);
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID, 0), 0x0);
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID_VALID, 0), 0x1);
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_PEERDEVICEID, 0), 0x1);
> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_CONNECTIONSTATE, 0), 0x1);
> +
> +	return 0;
> +}
> +
> +static int ufs_rockchip_common_init(struct ufs_hba *hba)
> +{
> +	struct device *dev = hba->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ufs_rockchip_host *host;
> +	int err = 0;
> +
> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	/* system control register for hci */
> +	host->ufs_sys_ctrl = devm_platform_ioremap_resource_byname(pdev, "hci_grf");
> +	if (IS_ERR(host->ufs_sys_ctrl))
> +		return dev_err_probe(dev, PTR_ERR(host->ufs_sys_ctrl),
> +					"cannot ioremap for hci system control register\n");
> +
> +	/* system control register for mphy */
> +	host->ufs_phy_ctrl = devm_platform_ioremap_resource_byname(pdev, "mphy_grf");
> +	if (IS_ERR(host->ufs_phy_ctrl))
> +		return dev_err_probe(dev, PTR_ERR(host->ufs_phy_ctrl),
> +				"cannot ioremap for mphy system control register\n");
> +
> +	/* mphy base register */
> +	host->mphy_base = devm_platform_ioremap_resource_byname(pdev, "mphy");
> +	if (IS_ERR(host->mphy_base))
> +		return dev_err_probe(dev, PTR_ERR(host->mphy_base),
> +					"cannot ioremap for mphy base register\n");
> +
> +	host->rst = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(host->rst))
> +		return dev_err_probe(dev, PTR_ERR(host->rst), "failed to get reset control\n");
> +
> +	reset_control_assert(host->rst);
> +	udelay(1);
> +	reset_control_deassert(host->rst);
> +
> +	host->ref_out_clk = devm_clk_get(dev, "ref_out");
> +	if (IS_ERR(host->ref_out_clk))
> +		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk), "ciu-drive not available\n");

What is 'ciu-drive'?

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

Krzysztof already pointed out this.

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

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

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

This is already called in ufshcd_pltfrm_init().

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

s/out/disable_ref_clk

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

Initialization not needed.

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

Also reverse Xmas order for local variables please.

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

Is it possible to hit this condition?

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

Use 'rockchip,rk3576-ufshc'.

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

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

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

Is it OK if vops is NULL?

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

Return err here and return 0 below.

> +
> +	return err;
> +}
> +

[...]

> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, ufs_rockchip_resume)
> +	SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)

Why can't you use ufshcd PM ops as like other vendor drivers?

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

Move bool to the end to avoid holes.

- Mani

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

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

[Shawn Lin]

Hi Mani

Thanks for the comments.

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

Will improve.

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

ok.

> 
>> +{
>> +	return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & DEVICE_PRESENT;
>> +}
>> +
>> +static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
>> +					 enum ufs_notify_change_status status)
>> +{
>> +	int err = 0;
>> +
>> +	if (status == PRE_CHANGE) {
>> +		int retry_outer = 3;
>> +		int retry_inner;
>> +start:
>> +		if (ufshcd_is_hba_active(hba))
>> +			/* change controller state to "reset state" */
>> +			ufshcd_hba_stop(hba);
>> +
>> +		/* UniPro link is disabled at this point */
>> +		ufshcd_set_link_off(hba);
>> +
>> +		/* start controller initialization sequence */
>> +		ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE);
>> +
>> +		usleep_range(100, 200);
>> +
>> +		/* wait for the host controller to complete initialization */
>> +		retry_inner = 50;
>> +		while (!ufshcd_is_hba_active(hba)) {
>> +			if (retry_inner) {
>> +				retry_inner--;
>> +			} else {
>> +				dev_err(hba->dev,
>> +					"Controller enable failed\n");
>> +				if (retry_outer) {
>> +					retry_outer--;
>> +					goto start;
>> +				}
>> +				return -EIO;
>> +			}
>> +			usleep_range(1000, 1100);
>> +		}
> 
> You just duplicated ufshcd_hba_execute_hce() here. Why? This doesn't make sense.

Since we set UFSHCI_QUIRK_BROKEN_HCE, and we also need to do someting
which is very similar to ufshcd_hba_execute_hce(), before calling
ufshcd_dme_reset(). Similar but not totally the same. I'll try to see if
we can export ufshcd_hba_execute_hce() to make full use of it.

> 
>> +	} else { /* POST_CHANGE */
>> +		err = ufshcd_vops_phy_initialization(hba);
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static void ufs_rockchip_set_pm_lvl(struct ufs_hba *hba)
>> +{
>> +	hba->rpm_lvl = UFS_PM_LVL_1;
>> +	hba->spm_lvl = UFS_PM_LVL_3;
>> +}
>> +
>> +static int ufs_rockchip_rk3576_phy_init(struct ufs_hba *hba)
>> +{
>> +	struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>> +
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(PA_LOCAL_TX_LCC_ENABLE, 0x0), 0x0);
>> +	/* enable the mphy DME_SET cfg */
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x200, 0x0), 0x40);
>> +	for (int i = 0; i < 2; i++) {
>> +		/* Configuration M-TX */
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xaa, SEL_TX_LANE0 + i), 0x06);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xa9, SEL_TX_LANE0 + i), 0x02);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xad, SEL_TX_LANE0 + i), 0x44);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xac, SEL_TX_LANE0 + i), 0xe6);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xab, SEL_TX_LANE0 + i), 0x07);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x94, SEL_TX_LANE0 + i), 0x93);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x93, SEL_TX_LANE0 + i), 0xc9);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x7f, SEL_TX_LANE0 + i), 0x00);
>> +		/* Configuration M-RX */
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x12, SEL_RX_LANE0 + i), 0x06);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x11, SEL_RX_LANE0 + i), 0x00);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1d, SEL_RX_LANE0 + i), 0x58);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1c, SEL_RX_LANE0 + i), 0x8c);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x1b, SEL_RX_LANE0 + i), 0x02);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x25, SEL_RX_LANE0 + i), 0xf6);
>> +		ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x2f, SEL_RX_LANE0 + i), 0x69);
>> +	}
>> +	/* disable the mphy DME_SET cfg */
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x200, 0x0), 0x00);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x80, 0x08C);
>> +	ufs_sys_writel(host->mphy_base, 0xB5, 0x110);
>> +	ufs_sys_writel(host->mphy_base, 0xB5, 0x250);
>> +
> 
> Why can't you do these settings in a PHY driver?

As we have ->phy_initialization in struct ufs_hba_variant_ops,
which asks the host driver to use it to initialize phys. It doesn't
seem to need to create a whole new file to just add some smalls fixed
parameters. :)


> 
>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x134);
>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x274);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x38, 0x0E0);
>> +	ufs_sys_writel(host->mphy_base, 0x38, 0x220);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x50, 0x164);
>> +	ufs_sys_writel(host->mphy_base, 0x50, 0x2A4);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x80, 0x178);
>> +	ufs_sys_writel(host->mphy_base, 0x80, 0x2B8);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x18, 0x1B0);
>> +	ufs_sys_writel(host->mphy_base, 0x18, 0x2F0);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x128);
>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x268);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x20, 0x12C);
>> +	ufs_sys_writel(host->mphy_base, 0x20, 0x26C);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x120);
>> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x260);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x094);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x1B4);
>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x2F4);
>> +
>> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x08C);
>> +	udelay(1);
>> +	ufs_sys_writel(host->mphy_base, 0x00, 0x08C);
>> +
>> +	udelay(200);
>> +	/* start link up */
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_TX_ENDIAN, 0), 0x0);
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_RX_ENDIAN, 0), 0x0);
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID, 0), 0x0);
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID_VALID, 0), 0x1);
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_PEERDEVICEID, 0), 0x1);
>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_CONNECTIONSTATE, 0), 0x1);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ufs_rockchip_common_init(struct ufs_hba *hba)
>> +{
>> +	struct device *dev = hba->dev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ufs_rockchip_host *host;
>> +	int err = 0;
>> +
>> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
>> +	if (!host)
>> +		return -ENOMEM;
>> +
>> +	/* system control register for hci */
>> +	host->ufs_sys_ctrl = devm_platform_ioremap_resource_byname(pdev, "hci_grf");
>> +	if (IS_ERR(host->ufs_sys_ctrl))
>> +		return dev_err_probe(dev, PTR_ERR(host->ufs_sys_ctrl),
>> +					"cannot ioremap for hci system control register\n");
>> +
>> +	/* system control register for mphy */
>> +	host->ufs_phy_ctrl = devm_platform_ioremap_resource_byname(pdev, "mphy_grf");
>> +	if (IS_ERR(host->ufs_phy_ctrl))
>> +		return dev_err_probe(dev, PTR_ERR(host->ufs_phy_ctrl),
>> +				"cannot ioremap for mphy system control register\n");
>> +
>> +	/* mphy base register */
>> +	host->mphy_base = devm_platform_ioremap_resource_byname(pdev, "mphy");
>> +	if (IS_ERR(host->mphy_base))
>> +		return dev_err_probe(dev, PTR_ERR(host->mphy_base),
>> +					"cannot ioremap for mphy base register\n");
>> +
>> +	host->rst = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(host->rst))
>> +		return dev_err_probe(dev, PTR_ERR(host->rst), "failed to get reset control\n");
>> +
>> +	reset_control_assert(host->rst);
>> +	udelay(1);
>> +	reset_control_deassert(host->rst);
>> +
>> +	host->ref_out_clk = devm_clk_get(dev, "ref_out");
>> +	if (IS_ERR(host->ref_out_clk))
>> +		return dev_err_probe(dev, PTR_ERR(host->ref_out_clk), "ciu-drive not available\n");
> 
> What is 'ciu-drive'?

Will fix.

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

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

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

Ok.

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

Will fix.

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

Will improve, as blow the same.

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

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

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

ok.

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

Will check.

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

Ok.

>> +
>> +	return err;
>> +}
>> +
> 
> [...]
> 
>> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, ufs_rockchip_resume)
>> +	SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)
> 
> Why can't you use ufshcd PM ops as like other vendor drivers?

It doesn't work from the test. We have many use case to power down the
controller and device, so there is no flow to recovery the link. Only
when the first accessing to UFS fails, the ufshcd error handle recovery 
the link. This is not what we expect.


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

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

[Manivannan Sadhasivam]

On Fri, Aug 09, 2024 at 04:16:41PM +0800, Shawn Lin wrote:

[...]

> > > +static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
> > > +					 enum ufs_notify_change_status status)
> > > +{
> > > +	int err = 0;
> > > +
> > > +	if (status == PRE_CHANGE) {
> > > +		int retry_outer = 3;
> > > +		int retry_inner;
> > > +start:
> > > +		if (ufshcd_is_hba_active(hba))
> > > +			/* change controller state to "reset state" */
> > > +			ufshcd_hba_stop(hba);
> > > +
> > > +		/* UniPro link is disabled at this point */
> > > +		ufshcd_set_link_off(hba);
> > > +
> > > +		/* start controller initialization sequence */
> > > +		ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE);
> > > +
> > > +		usleep_range(100, 200);
> > > +
> > > +		/* wait for the host controller to complete initialization */
> > > +		retry_inner = 50;
> > > +		while (!ufshcd_is_hba_active(hba)) {
> > > +			if (retry_inner) {
> > > +				retry_inner--;
> > > +			} else {
> > > +				dev_err(hba->dev,
> > > +					"Controller enable failed\n");
> > > +				if (retry_outer) {
> > > +					retry_outer--;
> > > +					goto start;
> > > +				}
> > > +				return -EIO;
> > > +			}
> > > +			usleep_range(1000, 1100);
> > > +		}
> > 
> > You just duplicated ufshcd_hba_execute_hce() here. Why? This doesn't make sense.
> 
> Since we set UFSHCI_QUIRK_BROKEN_HCE, and we also need to do someting
> which is very similar to ufshcd_hba_execute_hce(), before calling
> ufshcd_dme_reset(). Similar but not totally the same. I'll try to see if
> we can export ufshcd_hba_execute_hce() to make full use of it.
> 

But you are starting the controller using REG_CONTROLLER_ENABLE. Isn't that
supposed to be broken if you set UFSHCI_QUIRK_BROKEN_HCE? Or I am
misunderstanding the quirk?

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

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

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

[...]

> > > +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
> > > +	SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, ufs_rockchip_resume)
> > > +	SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)
> > 
> > Why can't you use ufshcd PM ops as like other vendor drivers?
> 
> It doesn't work from the test. We have many use case to power down the
> controller and device, so there is no flow to recovery the link. Only
> when the first accessing to UFS fails, the ufshcd error handle recovery the
> link. This is not what we expect.
> 

What tests? The existing UFS controller drivers are used in production devices
and they never had a usecase to invent their own PM callbacks. So if your
controller is special, then you need to justify it more elaborately. If
something is missing in ufshcd callbacks, then we can add them.

- Mani

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

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

[Shawn Lin]

JHi Mani,

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

Our controller doesn't work with exiting code, whether setting
UFSHCI_QUIRK_BROKEN_HCE or not.


For UFSHCI_QUIRK_BROKEN_HCE case, it calls ufshcd_dme_reset（）first,
but we need to set REG_CONTROLLER_ENABLE first.

For !UFSHCI_QUIRK_BROKEN_HCE case, namly ufshcd_hba_execute_hce, it
sets REG_CONTROLLER_ENABLE  first but never send DMA_RESET and calls
ufshcd_dme_enable.

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

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

yes, it needn't.

> 
>>
>>>
>>>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x134);
>>>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x274);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0x38, 0x0E0);
>>>> +	ufs_sys_writel(host->mphy_base, 0x38, 0x220);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0x50, 0x164);
>>>> +	ufs_sys_writel(host->mphy_base, 0x50, 0x2A4);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0x80, 0x178);
>>>> +	ufs_sys_writel(host->mphy_base, 0x80, 0x2B8);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0x18, 0x1B0);
>>>> +	ufs_sys_writel(host->mphy_base, 0x18, 0x2F0);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x128);
>>>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x268);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0x20, 0x12C);
>>>> +	ufs_sys_writel(host->mphy_base, 0x20, 0x26C);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x120);
>>>> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x260);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x094);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x1B4);
>>>> +	ufs_sys_writel(host->mphy_base, 0x03, 0x2F4);
>>>> +
>>>> +	ufs_sys_writel(host->mphy_base, 0xC0, 0x08C);
>>>> +	udelay(1);
>>>> +	ufs_sys_writel(host->mphy_base, 0x00, 0x08C);
>>>> +
>>>> +	udelay(200);
>>>> +	/* start link up */
>>>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_TX_ENDIAN, 0), 0x0);
>>>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(MIB_T_DBG_CPORT_RX_ENDIAN, 0), 0x0);
>>>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID, 0), 0x0);
>>>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(N_DEVICEID_VALID, 0), 0x1);
>>>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_PEERDEVICEID, 0), 0x1);
>>>> +	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(T_CONNECTIONSTATE, 0), 0x1);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
> 
> [...]
> 
>>>> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
>>>> +	SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, ufs_rockchip_resume)
>>>> +	SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)
>>>
>>> Why can't you use ufshcd PM ops as like other vendor drivers?
>>
>> It doesn't work from the test. We have many use case to power down the
>> controller and device, so there is no flow to recovery the link. Only
>> when the first accessing to UFS fails, the ufshcd error handle recovery the
>> link. This is not what we expect.
>>
> 
> What tests? The existing UFS controller drivers are used in production devices
> and they never had a usecase to invent their own PM callbacks. So if your
> controller is special, then you need to justify it more elaborately. If
> something is missing in ufshcd callbacks, then we can add them.
> 

All the register got lost each time as we power down both controller & 
PHY and devices in suspend. So we have to restore the necessary
registers and link. I didn't see where the code recovery the controller
settings in ufshcd_resume, except ufshcd_err_handler（）triggers that.
Am I missing any thing? Below is the dump we get if using
SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume).
It can work as ufshcd_err_handler () will fix the link, but we have to
suffer from getting the error log each time. Moreover, we need to gate
26MHz refclk for device when RPM is called. So our own rpm callback is
needed.

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

> - Mani
> 

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

[Manivannan Sadhasivam]

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

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

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

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

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

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

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

[...]

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

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

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

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

- Mani

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

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

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

[Shawn Lin]

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

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

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

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

Both.

> 
>> So we have to restore the necessary
>> registers and link. I didn't see where the code recovery the controller
>> settings in ufshcd_resume, except ufshcd_err_handler（）triggers that.
>> Am I missing any thing?
> 
> Can you explain what is causing the powerdown of the controller and PHY?
> Because, ufshcd_suspend() just turns off the clocks and regulators (if
> UFSHCD_CAP_AGGR_POWER_COLLAPSE is set) and spm_lvl 3 set by this driver only
> puts the device in sleep mode and link in hibern8 state.
> 

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

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

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

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

[Manivannan Sadhasivam]

On Mon, Aug 12, 2024 at 02:24:31PM +0800, Shawn Lin wrote:
> 在 2024/8/12 12:10, Manivannan Sadhasivam 写道:
> > On Mon, Aug 12, 2024 at 09:28:26AM +0800, Shawn Lin wrote:
> > > JHi Mani,
> > > 
> > > 在 2024/8/10 17:28, Manivannan Sadhasivam 写道:
> > > > On Fri, Aug 09, 2024 at 04:16:41PM +0800, Shawn Lin wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > > +static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
> > > > > > > +					 enum ufs_notify_change_status status)
> > > > > > > +{
> > > > > > > +	int err = 0;
> > > > > > > +
> > > > > > > +	if (status == PRE_CHANGE) {
> > > > > > > +		int retry_outer = 3;
> > > > > > > +		int retry_inner;
> > > > > > > +start:
> > > > > > > +		if (ufshcd_is_hba_active(hba))
> > > > > > > +			/* change controller state to "reset state" */
> > > > > > > +			ufshcd_hba_stop(hba);
> > > > > > > +
> > > > > > > +		/* UniPro link is disabled at this point */
> > > > > > > +		ufshcd_set_link_off(hba);
> > > > > > > +
> > > > > > > +		/* start controller initialization sequence */
> > > > > > > +		ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE);
> > > > > > > +
> > > > > > > +		usleep_range(100, 200);
> > > > > > > +
> > > > > > > +		/* wait for the host controller to complete initialization */
> > > > > > > +		retry_inner = 50;
> > > > > > > +		while (!ufshcd_is_hba_active(hba)) {
> > > > > > > +			if (retry_inner) {
> > > > > > > +				retry_inner--;
> > > > > > > +			} else {
> > > > > > > +				dev_err(hba->dev,
> > > > > > > +					"Controller enable failed\n");
> > > > > > > +				if (retry_outer) {
> > > > > > > +					retry_outer--;
> > > > > > > +					goto start;
> > > > > > > +				}
> > > > > > > +				return -EIO;
> > > > > > > +			}
> > > > > > > +			usleep_range(1000, 1100);
> > > > > > > +		}
> > > > > > 
> > > > > > You just duplicated ufshcd_hba_execute_hce() here. Why? This doesn't make sense.
> > > > > 
> > > > > Since we set UFSHCI_QUIRK_BROKEN_HCE, and we also need to do someting
> > > > > which is very similar to ufshcd_hba_execute_hce(), before calling
> > > > > ufshcd_dme_reset(). Similar but not totally the same. I'll try to see if
> > > > > we can export ufshcd_hba_execute_hce() to make full use of it.
> > > > > 
> > > > 
> > > > But you are starting the controller using REG_CONTROLLER_ENABLE. Isn't that
> > > > supposed to be broken if you set UFSHCI_QUIRK_BROKEN_HCE? Or I am
> > > > misunderstanding the quirk?
> > > > 
> > > 
> > > Our controller doesn't work with exiting code, whether setting
> > > UFSHCI_QUIRK_BROKEN_HCE or not.
> > > 
> > 
> > Okay. Then this means you do not need this quirk at all.
> > 
> > > 
> > > For UFSHCI_QUIRK_BROKEN_HCE case, it calls ufshcd_dme_reset（）first,
> > > but we need to set REG_CONTROLLER_ENABLE first.
> > > 
> > > For !UFSHCI_QUIRK_BROKEN_HCE case, namly ufshcd_hba_execute_hce, it
> > > sets REG_CONTROLLER_ENABLE  first but never send DMA_RESET and calls
> > > ufshcd_dme_enable.
> > > 
> > 
> > I don't see where ufshcd_dme_enable() is getting called for
> > !UFSHCI_QUIRK_BROKEN_HCE case.
> > 
> > > So the closet code path is to go through UFSHCI_QUIRK_BROKEN_HCE case,
> > > and set REG_CONTROLLER_ENABLE by adding hce_enable_notify hook.
> > > 
> > 
> > No, that is abusing the quirk. But I'm confused about why your controller wants
> > resetting the unipro stack _after_ enabling the controller? Why can't it be
> > reset before?
> > 
> 
> It can't be. The DME_RESET to reset the unipro stack will be failed
> without enabling REG_CONTROLLER_ENABLE. And the controller does want us
> to reset the unipro stack before other coming UICs.
> 
> So I considered it's a kind of broken HCE case as well. Should I add a
> new quirk or add a new hba_enable hook in ufs_hba_variant_ops? Or just
> use UFSHCI_QUIRK_BROKEN_HCE ?
> 

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

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

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

> > 
> > > So we have to restore the necessary
> > > registers and link. I didn't see where the code recovery the controller
> > > settings in ufshcd_resume, except ufshcd_err_handler（）triggers that.
> > > Am I missing any thing?
> > 
> > Can you explain what is causing the powerdown of the controller and PHY?
> > Because, ufshcd_suspend() just turns off the clocks and regulators (if
> > UFSHCD_CAP_AGGR_POWER_COLLAPSE is set) and spm_lvl 3 set by this driver only
> > puts the device in sleep mode and link in hibern8 state.
> > 
> 
> For runtime PM case, it's the power-domain driver will power down the
> controller and PHY if UFS stack is not active any more（autosuspend）.
> 
> For system PM case, it's the SoC's firmware to cutting of all the power
> for controller/PHY and device.
> 

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

- Mani

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

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

[Shawn Lin]

Hi Mani,

在 2024/8/13 0:55, Manivannan Sadhasivam 写道:
> On Mon, Aug 12, 2024 at 02:24:31PM +0800, Shawn Lin wrote:
>> 在 2024/8/12 12:10, Manivannan Sadhasivam 写道:
>>> On Mon, Aug 12, 2024 at 09:28:26AM +0800, Shawn Lin wrote:
>>>> JHi Mani,
>>>>
>>>> 在 2024/8/10 17:28, Manivannan Sadhasivam 写道:
>>>>> On Fri, Aug 09, 2024 at 04:16:41PM +0800, Shawn Lin wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> +static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
>>>>>>>> +					 enum ufs_notify_change_status status)
>>>>>>>> +{
>>>>>>>> +	int err = 0;
>>>>>>>> +
>>>>>>>> +	if (status == PRE_CHANGE) {
>>>>>>>> +		int retry_outer = 3;
>>>>>>>> +		int retry_inner;
>>>>>>>> +start:
>>>>>>>> +		if (ufshcd_is_hba_active(hba))
>>>>>>>> +			/* change controller state to "reset state" */
>>>>>>>> +			ufshcd_hba_stop(hba);
>>>>>>>> +
>>>>>>>> +		/* UniPro link is disabled at this point */
>>>>>>>> +		ufshcd_set_link_off(hba);
>>>>>>>> +
>>>>>>>> +		/* start controller initialization sequence */
>>>>>>>> +		ufshcd_writel(hba, CONTROLLER_ENABLE, REG_CONTROLLER_ENABLE);
>>>>>>>> +
>>>>>>>> +		usleep_range(100, 200);
>>>>>>>> +
>>>>>>>> +		/* wait for the host controller to complete initialization */
>>>>>>>> +		retry_inner = 50;
>>>>>>>> +		while (!ufshcd_is_hba_active(hba)) {
>>>>>>>> +			if (retry_inner) {
>>>>>>>> +				retry_inner--;
>>>>>>>> +			} else {
>>>>>>>> +				dev_err(hba->dev,
>>>>>>>> +					"Controller enable failed\n");
>>>>>>>> +				if (retry_outer) {
>>>>>>>> +					retry_outer--;
>>>>>>>> +					goto start;
>>>>>>>> +				}
>>>>>>>> +				return -EIO;
>>>>>>>> +			}
>>>>>>>> +			usleep_range(1000, 1100);
>>>>>>>> +		}
>>>>>>>
>>>>>>> You just duplicated ufshcd_hba_execute_hce() here. Why? This doesn't make sense.
>>>>>>
>>>>>> Since we set UFSHCI_QUIRK_BROKEN_HCE, and we also need to do someting
>>>>>> which is very similar to ufshcd_hba_execute_hce(), before calling
>>>>>> ufshcd_dme_reset(). Similar but not totally the same. I'll try to see if
>>>>>> we can export ufshcd_hba_execute_hce() to make full use of it.
>>>>>>
>>>>>
>>>>> But you are starting the controller using REG_CONTROLLER_ENABLE. Isn't that
>>>>> supposed to be broken if you set UFSHCI_QUIRK_BROKEN_HCE? Or I am
>>>>> misunderstanding the quirk?
>>>>>
>>>>
>>>> Our controller doesn't work with exiting code, whether setting
>>>> UFSHCI_QUIRK_BROKEN_HCE or not.
>>>>
>>>
>>> Okay. Then this means you do not need this quirk at all.
>>>
>>>>
>>>> For UFSHCI_QUIRK_BROKEN_HCE case, it calls ufshcd_dme_reset（）first,
>>>> but we need to set REG_CONTROLLER_ENABLE first.
>>>>
>>>> For !UFSHCI_QUIRK_BROKEN_HCE case, namly ufshcd_hba_execute_hce, it
>>>> sets REG_CONTROLLER_ENABLE  first but never send DMA_RESET and calls
>>>> ufshcd_dme_enable.
>>>>
>>>
>>> I don't see where ufshcd_dme_enable() is getting called for
>>> !UFSHCI_QUIRK_BROKEN_HCE case.
>>>
>>>> So the closet code path is to go through UFSHCI_QUIRK_BROKEN_HCE case,
>>>> and set REG_CONTROLLER_ENABLE by adding hce_enable_notify hook.
>>>>
>>>
>>> No, that is abusing the quirk. But I'm confused about why your controller wants
>>> resetting the unipro stack _after_ enabling the controller? Why can't it be
>>> reset before?
>>>
>>
>> It can't be. The DME_RESET to reset the unipro stack will be failed
>> without enabling REG_CONTROLLER_ENABLE. And the controller does want us
>> to reset the unipro stack before other coming UICs.
>>
>> So I considered it's a kind of broken HCE case as well. Should I add a
>> new quirk or add a new hba_enable hook in ufs_hba_variant_ops? Or just
>> use UFSHCI_QUIRK_BROKEN_HCE ?
>>
> 
> IMO, you should add a new quirk and use it directly in ufshcd_hba_execute_hce().
> But you need to pick the quirk name as per the actual quirky behavior of the
> controller.
> 

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

>>>>>>>
>>>>>>>> +	} else { /* POST_CHANGE */
>>>>>>>> +		err = ufshcd_vops_phy_initialization(hba);
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	return err;
>>>>>>>> +}
>>>>>>>> +
>>>
>>> [...]
>>>
>>>>>>>> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
>>>>>>>> +	SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, ufs_rockchip_resume)
>>>>>>>> +	SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)
>>>>>>>
>>>>>>> Why can't you use ufshcd PM ops as like other vendor drivers?
>>>>>>
>>>>>> It doesn't work from the test. We have many use case to power down the
>>>>>> controller and device, so there is no flow to recovery the link. Only
>>>>>> when the first accessing to UFS fails, the ufshcd error handle recovery the
>>>>>> link. This is not what we expect.
>>>>>>
>>>>>
>>>>> What tests? The existing UFS controller drivers are used in production devices
>>>>> and they never had a usecase to invent their own PM callbacks. So if your
>>>>> controller is special, then you need to justify it more elaborately. If
>>>>> something is missing in ufshcd callbacks, then we can add them.
>>>>>
>>>>
>>>> All the register got lost each time as we power down both controller & PHY
>>>> and devices in suspend.
>>>
>>> Which suspend? runtime or system suspend? I believe system suspend.
>>
>> Both.
>>
> 
> With {rpm/spm}_lvl = 3, you should not power down the controller.
> 
>>>
>>>> So we have to restore the necessary
>>>> registers and link. I didn't see where the code recovery the controller
>>>> settings in ufshcd_resume, except ufshcd_err_handler（）triggers that.
>>>> Am I missing any thing?
>>>
>>> Can you explain what is causing the powerdown of the controller and PHY?
>>> Because, ufshcd_suspend() just turns off the clocks and regulators (if
>>> UFSHCD_CAP_AGGR_POWER_COLLAPSE is set) and spm_lvl 3 set by this driver only
>>> puts the device in sleep mode and link in hibern8 state.
>>>
>>
>> For runtime PM case, it's the power-domain driver will power down the
>> controller and PHY if UFS stack is not active any more（autosuspend）.
>>
>> For system PM case, it's the SoC's firmware to cutting of all the power
>> for controller/PHY and device.
>>
> 
> Both cases are not matching the expectations of {rpm/spm}_lvl. So the platform
> (power domain or the firmware) should be fixed. What if the user sets the
> {rpm/spm}_lvl to 1? Will the platform power down the controller even then? If
> so, then I'd say that the platform is broken and should be fixed.

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

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


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


Does that sound feasible to you?


Thanks.

> 
> - Mani
> 

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

[Shawn Lin]

在 2024/8/13 11:52, Shawn Lin 写道:
> Hi Mani,
> 
> 在 2024/8/13 0:55, Manivannan Sadhasivam 写道:
>> On Mon, Aug 12, 2024 at 02:24:31PM +0800, Shawn Lin wrote:
>>> 在 2024/8/12 12:10, Manivannan Sadhasivam 写道:
>>>> On Mon, Aug 12, 2024 at 09:28:26AM +0800, Shawn Lin wrote:
>>>>> JHi Mani,
>>>>>
>>>>> 在 2024/8/10 17:28, Manivannan Sadhasivam 写道:
>>>>>> On Fri, Aug 09, 2024 at 04:16:41PM +0800, Shawn Lin wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> +static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
>>>>>>>>> +                     enum ufs_notify_change_status status)
>>>>>>>>> +{
>>>>>>>>> +    int err = 0;
>>>>>>>>> +
>>>>>>>>> +    if (status == PRE_CHANGE) {
>>>>>>>>> +        int retry_outer = 3;
>>>>>>>>> +        int retry_inner;
>>>>>>>>> +start:
>>>>>>>>> +        if (ufshcd_is_hba_active(hba))
>>>>>>>>> +            /* change controller state to "reset state" */
>>>>>>>>> +            ufshcd_hba_stop(hba);
>>>>>>>>> +
>>>>>>>>> +        /* UniPro link is disabled at this point */
>>>>>>>>> +        ufshcd_set_link_off(hba);
>>>>>>>>> +
>>>>>>>>> +        /* start controller initialization sequence */
>>>>>>>>> +        ufshcd_writel(hba, CONTROLLER_ENABLE, 
>>>>>>>>> REG_CONTROLLER_ENABLE);
>>>>>>>>> +
>>>>>>>>> +        usleep_range(100, 200);
>>>>>>>>> +
>>>>>>>>> +        /* wait for the host controller to complete 
>>>>>>>>> initialization */
>>>>>>>>> +        retry_inner = 50;
>>>>>>>>> +        while (!ufshcd_is_hba_active(hba)) {
>>>>>>>>> +            if (retry_inner) {
>>>>>>>>> +                retry_inner--;
>>>>>>>>> +            } else {
>>>>>>>>> +                dev_err(hba->dev,
>>>>>>>>> +                    "Controller enable failed\n");
>>>>>>>>> +                if (retry_outer) {
>>>>>>>>> +                    retry_outer--;
>>>>>>>>> +                    goto start;
>>>>>>>>> +                }
>>>>>>>>> +                return -EIO;
>>>>>>>>> +            }
>>>>>>>>> +            usleep_range(1000, 1100);
>>>>>>>>> +        }
>>>>>>>>
>>>>>>>> You just duplicated ufshcd_hba_execute_hce() here. Why? This 
>>>>>>>> doesn't make sense.
>>>>>>>
>>>>>>> Since we set UFSHCI_QUIRK_BROKEN_HCE, and we also need to do 
>>>>>>> someting
>>>>>>> which is very similar to ufshcd_hba_execute_hce(), before calling
>>>>>>> ufshcd_dme_reset(). Similar but not totally the same. I'll try to 
>>>>>>> see if
>>>>>>> we can export ufshcd_hba_execute_hce() to make full use of it.
>>>>>>>
>>>>>>
>>>>>> But you are starting the controller using REG_CONTROLLER_ENABLE. 
>>>>>> Isn't that
>>>>>> supposed to be broken if you set UFSHCI_QUIRK_BROKEN_HCE? Or I am
>>>>>> misunderstanding the quirk?
>>>>>>
>>>>>
>>>>> Our controller doesn't work with exiting code, whether setting
>>>>> UFSHCI_QUIRK_BROKEN_HCE or not.
>>>>>
>>>>
>>>> Okay. Then this means you do not need this quirk at all.
>>>>
>>>>>
>>>>> For UFSHCI_QUIRK_BROKEN_HCE case, it calls ufshcd_dme_reset（）first,
>>>>> but we need to set REG_CONTROLLER_ENABLE first.
>>>>>
>>>>> For !UFSHCI_QUIRK_BROKEN_HCE case, namly ufshcd_hba_execute_hce, it
>>>>> sets REG_CONTROLLER_ENABLE  first but never send DMA_RESET and calls
>>>>> ufshcd_dme_enable.
>>>>>
>>>>
>>>> I don't see where ufshcd_dme_enable() is getting called for
>>>> !UFSHCI_QUIRK_BROKEN_HCE case.
>>>>
>>>>> So the closet code path is to go through UFSHCI_QUIRK_BROKEN_HCE case,
>>>>> and set REG_CONTROLLER_ENABLE by adding hce_enable_notify hook.
>>>>>
>>>>
>>>> No, that is abusing the quirk. But I'm confused about why your 
>>>> controller wants
>>>> resetting the unipro stack _after_ enabling the controller? Why 
>>>> can't it be
>>>> reset before?
>>>>
>>>
>>> It can't be. The DME_RESET to reset the unipro stack will be failed
>>> without enabling REG_CONTROLLER_ENABLE. And the controller does want us
>>> to reset the unipro stack before other coming UICs.
>>>
>>> So I considered it's a kind of broken HCE case as well. Should I add a
>>> new quirk or add a new hba_enable hook in ufs_hba_variant_ops? Or just
>>> use UFSHCI_QUIRK_BROKEN_HCE ?
>>>
>>
>> IMO, you should add a new quirk and use it directly in 
>> ufshcd_hba_execute_hce().
>> But you need to pick the quirk name as per the actual quirky behavior 
>> of the
>> controller.
>>
> 
> Thanks, Main. I'll add a new quirk for
> ufshcd_hba_execute_hce() as per the actual quirky behavour.
> 
>>>>>>>>
>>>>>>>>> +    } else { /* POST_CHANGE */
>>>>>>>>> +        err = ufshcd_vops_phy_initialization(hba);
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    return err;
>>>>>>>>> +}
>>>>>>>>> +
>>>>
>>>> [...]
>>>>
>>>>>>>>> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
>>>>>>>>> +    SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_suspend, 
>>>>>>>>> ufs_rockchip_resume)
>>>>>>>>> +    SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, 
>>>>>>>>> ufs_rockchip_runtime_resume, NULL)
>>>>>>>>
>>>>>>>> Why can't you use ufshcd PM ops as like other vendor drivers?
>>>>>>>
>>>>>>> It doesn't work from the test. We have many use case to power 
>>>>>>> down the
>>>>>>> controller and device, so there is no flow to recovery the link. 
>>>>>>> Only
>>>>>>> when the first accessing to UFS fails, the ufshcd error handle 
>>>>>>> recovery the
>>>>>>> link. This is not what we expect.
>>>>>>>
>>>>>>
>>>>>> What tests? The existing UFS controller drivers are used in 
>>>>>> production devices
>>>>>> and they never had a usecase to invent their own PM callbacks. So 
>>>>>> if your
>>>>>> controller is special, then you need to justify it more 
>>>>>> elaborately. If
>>>>>> something is missing in ufshcd callbacks, then we can add them.
>>>>>>
>>>>>
>>>>> All the register got lost each time as we power down both 
>>>>> controller & PHY
>>>>> and devices in suspend.
>>>>
>>>> Which suspend? runtime or system suspend? I believe system suspend.
>>>
>>> Both.
>>>
>>
>> With {rpm/spm}_lvl = 3, you should not power down the controller.
>>
>>>>
>>>>> So we have to restore the necessary
>>>>> registers and link. I didn't see where the code recovery the 
>>>>> controller
>>>>> settings in ufshcd_resume, except ufshcd_err_handler（）triggers that.
>>>>> Am I missing any thing?
>>>>
>>>> Can you explain what is causing the powerdown of the controller and 
>>>> PHY?
>>>> Because, ufshcd_suspend() just turns off the clocks and regulators (if
>>>> UFSHCD_CAP_AGGR_POWER_COLLAPSE is set) and spm_lvl 3 set by this 
>>>> driver only
>>>> puts the device in sleep mode and link in hibern8 state.
>>>>
>>>
>>> For runtime PM case, it's the power-domain driver will power down the
>>> controller and PHY if UFS stack is not active any more（autosuspend）.
>>>
>>> For system PM case, it's the SoC's firmware to cutting of all the power
>>> for controller/PHY and device.
>>>
>>
>> Both cases are not matching the expectations of {rpm/spm}_lvl. So the 
>> platform
>> (power domain or the firmware) should be fixed. What if the user sets the
>> {rpm/spm}_lvl to 1? Will the platform power down the controller even 
>> then? If
>> so, then I'd say that the platform is broken and should be fixed.
> 
> Ok, it seems I need to set {rpm/spm}_lvl = 6 if I want platform to power
> down the controller for ultra power-saving. But I still need to add my
> own system PM callback in that case to recovery the link first. Do I
> misunderstand it?
> 
> And for the user who sets the rpm/spm level via
> ufs_sysfs_pm_lvl_store(), I think there is no way to block it currently,
> except that we need to fix the power-domain driver and Firmware to
> respect the level and choose correct policy.
> 
> 
> So in summary for what the next step I should to:
> (1) Set {rpm/spm}_lvl = 6 in host driver to reflect the expectation
> (2) Add own PM callbacks to recovery the link to meet the expectation
> (3) Fix the broken behaviour of PD or Firmware to respect the actual
> desired pm level if user changes the pm level.
> 
> 

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

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

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

[Manivannan Sadhasivam]

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

[...]

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

Ok.

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

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

- Mani

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

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

[Bart Van Assche]

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

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

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

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

> +#include "ufshcd-dwc.h"

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

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

Why udelay() instead of usleep_range()?

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

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

Thanks,

Bart.

[RESEND PATCH v2 2/3] 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 v2:
- renmae file name
- fix all errors and pass the dt_binding_check:
  make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml

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

diff --git a/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml b/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
new file mode 100644
index 0000000..1844fe3
--- /dev/null
+++ b/Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ufs/rockchip,rk3576-ufs.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-ufs
+
+  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
+
+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/rk3576-power.h>
+    #include <dt-bindings/pinctrl/rockchip.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    ufs: ufs@2a2d0000 {
+          compatible = "rockchip,rk3576-ufs";
+          reg = <0x2a2d0000 0x10000>,
+                <0x2b040000 0x10000>,
+                <0x2601f000 0x1000>,
+                <0x2603c000 0x1000>,
+                <0x2a2e0000 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_HIGH>;
+    };
-- 
2.7.4

Re: [RESEND PATCH v2 2/3] dt-bindings: ufs: Document Rockchip UFS host controller

[Rob Herring (Arm)]

On Thu, 08 Aug 2024 12:27:00 +0800, Shawn Lin wrote:
> Document Rockchip UFS host controller for RK3576 SoC.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> Changes in v2:
> - renmae file name
> - fix all errors and pass the dt_binding_check:
>   make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml
> 
>  .../bindings/ufs/rockchip,rk3576-ufs.yaml          | 96 ++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.example.dts:24:18: fatal error: dt-bindings/clock/rockchip,rk3576-cru.h: No such file or directory
   24 |         #include <dt-bindings/clock/rockchip,rk3576-cru.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1723091220-29291-1-git-send-email-shawn.lin@rock-chips.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [RESEND PATCH v2 2/3] dt-bindings: ufs: Document Rockchip UFS host controller

[Krzysztof Kozlowski]

On 08/08/2024 11:30, Rob Herring (Arm) wrote:
> 
> On Thu, 08 Aug 2024 12:27:00 +0800, Shawn Lin wrote:
>> Document Rockchip UFS host controller for RK3576 SoC.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - renmae file name
>> - fix all errors and pass the dt_binding_check:
>>   make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml
>>
>>  .../bindings/ufs/rockchip,rk3576-ufs.yaml          | 96 ++++++++++++++++++++++
>>  1 file changed, 96 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.yaml
>>
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/ufs/rockchip,rk3576-ufs.example.dts:24:18: fatal error: dt-bindings/clock/rockchip,rk3576-cru.h: No such file or directory
>    24 |         #include <dt-bindings/clock/rockchip,rk3576-cru.h>
>       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.

Nothing improved... stop sending new versions so fast but allow people
to respond to your patches and then implement the feedback.

Best regards,
Krzysztof

Re: [RESEND PATCH v2 2/3] dt-bindings: ufs: Document Rockchip UFS host controller

[Krzysztof Kozlowski]

On 08/08/2024 06:27, Shawn Lin wrote:
> Document Rockchip UFS host controller for RK3576 SoC.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> Changes in v2:
> - renmae file name
> - fix all errors and pass the dt_binding_check:
>   make dt_binding_check DT_SCHEMA_FILES=rockchip,rk3576-ufs.yaml

That's not a correct way of sending patches. You already sent a v2, so
how tools should process this?

Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.

Also, still dependency is not explained.

Because of that dependency, it is not possible to apply it, so no. Drop
the dependency first.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] scsi: ufs: core: Export ufshcd_dme_link_startup() helper

[Bart Van Assche]

On 8/7/24 8:52 PM, Shawn Lin wrote:
> Export it for host drivers.

That's a very short patch description. Please explain in the patch
description why your host driver is the only host driver that needs to
call this function.

Thanks,

Bart.