[PATCH 0/2] Add support for Unisoc UFS host controller

[PATCH 0/2] Add support for Unisoc UFS host controller

[Zhe Wang]

From: Zhe Wang <zhe.wang1@unisoc.com>

Add this patchset to support the Unisoc UFS host controller.

Zhe Wang (2):
  dt-bindings: ufs: Add document for Unisoc UFS host controller
  scsi: ufs-unisoc: Add support for Unisoc UFS host controller

 .../devicetree/bindings/ufs/sprd,ufs.yaml     |  72 +++
 drivers/ufs/host/Kconfig                      |  12 +
 drivers/ufs/host/Makefile                     |   1 +
 drivers/ufs/host/ufs-sprd.c                   | 445 ++++++++++++++++++
 drivers/ufs/host/ufs-sprd.h                   | 125 +++++
 5 files changed, 655 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
 create mode 100644 drivers/ufs/host/ufs-sprd.c
 create mode 100644 drivers/ufs/host/ufs-sprd.h

-- 
2.17.1

[PATCH 1/2] dt-bindings: ufs: Add document for Unisoc UFS host controller

[Zhe Wang]

From: Zhe Wang <zhe.wang1@unisoc.com>

Add Unisoc ums9620 ufs host controller devicetree document.

Signed-off-by: Zhe Wang <zhe.wang1@unisoc.com>
---
 .../devicetree/bindings/ufs/sprd,ufs.yaml     | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ufs/sprd,ufs.yaml

diff --git a/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml b/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
new file mode 100644
index 000000000000..88f2c670b0a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ufs/sprd,ufs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Unisoc Universal Flash Storage (UFS) Controller
+
+maintainers:
+  - Zhe Wang <zhe.wang1@unisoc.com>
+
+allOf:
+  - $ref: ufs-common.yaml
+
+properties:
+  compatible:
+    enum:
+      - sprd,ums9620-ufs
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: hclk
+      - const: hclk_source
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: ufs_soft_rst
+      - const: ufsdev_soft_rst
+
+  sprd,ufs-anly-reg-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of syscon used to control ufs analog reg.
+
+  sprd,aon-apb-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of syscon used to control always-on reg.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    ufs: ufs@22000000 {
+        compatible = "sprd,ums9620-ufs";
+        reg = <0x22000000 0x3000>;
+        interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
+        vcc-supply = <&vddemmcore>;
+        vdd-mphy-supply = <&vddufs1v2>;
+        clocks-name = "ufs_eb", "ufs_cfg_eb",
+            "ufs_hclk", "ufs_hclk_source";
+        clocks = <&apahb_gate CLK_UFS_EB>, <&apahb_gate CLK_UFS_CFG_EB>,
+            <&onapb_clk CLK_UFS_AON>, <&g51_pll CLK_TGPLL_256M>;
+        freq-table-hz = <0 0>, <0 0>, <0 0>, <0 0>;
+        reset-names = "ufs_soft_rst", "ufsdev_soft_rst";
+        resets = <&apahb_gate RESET_AP_AHB_UFS_SOFT_RST>,
+            <&aonapb_gate RESET_AON_APB_UFSDEV_SOFT_RST>;
+        sprd,ufs-anly-reg-syscon = <&anly_phy_g12_regs>;
+        sprd,aon-apb-syscon = <&aon_apb_regs>;
+        status = "disable";
+    };
-- 
2.17.1

[PATCH 2/2] scsi: ufs-unisoc: Add support for Unisoc UFS host controller

[Zhe Wang]

From: Zhe Wang <zhe.wang1@unisoc.com>

Add driver code for Unisoc ufs host controller, along with ufs
initialization.

Signed-off-by: Zhe Wang <zhe.wang1@unisoc.com>
---
 drivers/ufs/host/Kconfig    |  12 +
 drivers/ufs/host/Makefile   |   1 +
 drivers/ufs/host/ufs-sprd.c | 445 ++++++++++++++++++++++++++++++++++++
 drivers/ufs/host/ufs-sprd.h | 125 ++++++++++
 4 files changed, 583 insertions(+)
 create mode 100644 drivers/ufs/host/ufs-sprd.c
 create mode 100644 drivers/ufs/host/ufs-sprd.h

diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
index 4cc2dbd79ed0..90a7142ec846 100644
--- a/drivers/ufs/host/Kconfig
+++ b/drivers/ufs/host/Kconfig
@@ -124,3 +124,15 @@ config SCSI_UFS_EXYNOS
 
 	  Select this if you have UFS host controller on Samsung Exynos SoC.
 	  If unsure, say N.
+
+config SCSI_UFS_SPRD
+	tristate "Unisoc specific hooks to UFS controller platform driver"
+	depends on SCSI_UFSHCD_PLATFORM && (ARCH_SPRD || COMPILE_TEST)
+	help
+	  This selects the Unisoc specific additions to UFSHCD platform driver.
+	  UFS host on Unisoc 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 Unisoc chipset.
+	  If unsure, say N.
diff --git a/drivers/ufs/host/Makefile b/drivers/ufs/host/Makefile
index 7717ca93e7d5..a946c3b35c9d 100644
--- a/drivers/ufs/host/Makefile
+++ b/drivers/ufs/host/Makefile
@@ -13,3 +13,4 @@ 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_TI_J721E) += ti-j721e-ufs.o
+obj-$(CONFIG_SCSI_UFS_SPRD) += ufs-sprd.o
diff --git a/drivers/ufs/host/ufs-sprd.c b/drivers/ufs/host/ufs-sprd.c
new file mode 100644
index 000000000000..6530219c0fdf
--- /dev/null
+++ b/drivers/ufs/host/ufs-sprd.c
@@ -0,0 +1,445 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * UNISOC UFS Host Controller driver
+ *
+ * Copyright (C) 2022 Unisoc, Inc.
+ * Author: Zhe Wang <zhe.wang1@unisoc.com>
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/clk.h>
+#include <linux/regulator/consumer.h>
+
+#include <ufs/ufshcd.h>
+#include "ufshcd-pltfrm.h"
+#include "ufs-sprd.h"
+
+static const struct of_device_id ufs_sprd_of_match[];
+
+static int ufs_sprd_get_clk(struct device *dev, struct ufs_sprd_clk *clki)
+{
+	clki->clk = devm_clk_get(dev, clki->name);
+	if (IS_ERR(clki->clk)) {
+		dev_err(dev, "failed to get clk:%s\n", clki->name);
+		return PTR_ERR(clki->clk);
+	}
+
+	return 0;
+}
+
+static int ufs_sprd_get_reset_ctrl(struct device *dev, struct ufs_sprd_rst *rci)
+{
+	rci->rc = devm_reset_control_get(dev, rci->name);
+	if (IS_ERR(rci->rc)) {
+		dev_err(dev, "failed to get reset ctrl:%s\n", rci->name);
+		return PTR_ERR(rci->rc);
+	}
+
+	return 0;
+}
+
+static int ufs_sprd_get_syscon_reg(struct device *dev, struct ufs_sprd_syscon *sysci)
+{
+	sysci->regmap = syscon_regmap_lookup_by_phandle(dev->of_node, sysci->name);
+	if (IS_ERR(sysci->regmap)) {
+		dev_err(dev, "failed to get ufs syscon:%s\n", sysci->name);
+		return PTR_ERR(sysci->regmap);
+	}
+
+	return 0;
+}
+
+static int ufs_sprd_get_vreg(struct device *dev, struct ufs_sprd_vreg *vregi)
+{
+	vregi->vreg = devm_regulator_get(dev, vregi->name);
+	if (IS_ERR(vregi->vreg)) {
+		dev_err(dev, "failed to get vreg:%s\n", vregi->name);
+		return PTR_ERR(vregi->vreg);
+	}
+
+	return 0;
+}
+
+static int ufs_sprd_parse_dt(struct device *dev, struct ufs_hba *hba, struct ufs_sprd_host *host)
+{
+	u32 i;
+	struct ufs_sprd_priv *priv = host->priv;
+	int ret = 0;
+
+	/* Parse UFS clk info */
+	for (i = 0; i < SPRD_UFS_CLK_MAX; i++) {
+		if (!priv->clki[i].name)
+			continue;
+		ret = ufs_sprd_get_clk(dev, &priv->clki[i]);
+		if (ret)
+			goto out;
+	}
+
+	/* Parse UFS reset ctrl info */
+	for (i = 0; i < SPRD_UFS_RST_MAX; i++) {
+		if (!priv->rci[i].name)
+			continue;
+		ret = ufs_sprd_get_reset_ctrl(dev, &priv->rci[i]);
+		if (ret)
+			goto out;
+	}
+
+	/* Parse UFS syscon reg info */
+	for (i = 0; i < SPRD_UFS_SYSCON_MAX; i++) {
+		if (!priv->sysci[i].name)
+			continue;
+		ret = ufs_sprd_get_syscon_reg(dev, &priv->sysci[i]);
+		if (ret)
+			goto out;
+	}
+
+	/* Parse UFS vreg info */
+	for (i = 0; i < SPRD_UFS_VREG_MAX; i++) {
+		if (!priv->vregi[i].name)
+			continue;
+		ret = ufs_sprd_get_vreg(dev, &priv->vregi[i]);
+		if (ret)
+			goto out;
+	}
+
+out:
+	return ret;
+}
+
+static int ufs_sprd_common_init(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	struct ufs_sprd_host *host;
+	struct platform_device *pdev = to_platform_device(dev);
+	const struct of_device_id *of_id;
+	int ret = 0;
+
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+
+	of_id = of_match_node(ufs_sprd_of_match, pdev->dev.of_node);
+	if (of_id->data != NULL)
+		host->priv = container_of(of_id->data, struct ufs_sprd_priv,
+					  ufs_hba_sprd_vops);
+
+	host->hba = hba;
+	ufshcd_set_variant(hba, host);
+
+	hba->caps |= UFSHCD_CAP_CLK_GATING |
+		UFSHCD_CAP_CRYPTO |
+		UFSHCD_CAP_WB_EN;
+	hba->quirks |= UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS;
+
+	ret = ufs_sprd_parse_dt(dev, hba, host);
+
+	return ret;
+}
+
+static int sprd_ufs_pwr_change_notify(struct ufs_hba *hba,
+				      enum ufs_notify_change_status status,
+				      struct ufs_pa_layer_attr *dev_max_params,
+				      struct ufs_pa_layer_attr *dev_req_params)
+{
+	struct ufs_sprd_host *host = ufshcd_get_variant(hba);
+
+	if (status == PRE_CHANGE) {
+		memcpy(dev_req_params, dev_max_params,
+			sizeof(struct ufs_pa_layer_attr));
+		if (host->unipro_ver >= UFS_UNIPRO_VER_1_8)
+			ufshcd_dme_configure_adapt(hba, dev_req_params->gear_tx,
+						   PA_INITIAL_ADAPT);
+	}
+
+	return 0;
+}
+
+static int ufs_sprd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
+			    enum ufs_notify_change_status status)
+{
+	unsigned long flags;
+
+	if (status == PRE_CHANGE) {
+		if (ufshcd_is_auto_hibern8_supported(hba)) {
+			spin_lock_irqsave(hba->host->host_lock, flags);
+			ufshcd_writel(hba, 0, REG_AUTO_HIBERNATE_IDLE_TIMER);
+			spin_unlock_irqrestore(hba->host->host_lock, flags);
+		}
+	}
+
+	return 0;
+}
+
+static void ufs_sprd_n6_host_reset(struct ufs_hba *hba)
+{
+	struct ufs_sprd_priv *priv = ufs_sprd_get_priv_data(hba);
+
+	dev_info(hba->dev, "ufs host reset!\n");
+
+	reset_control_assert(priv->rci[SPRD_UFSHCI_SOFT_RST].rc);
+	usleep_range(1000, 1100);
+	reset_control_deassert(priv->rci[SPRD_UFSHCI_SOFT_RST].rc);
+}
+
+static int ufs_sprd_n6_device_reset(struct ufs_hba *hba)
+{
+	struct ufs_sprd_priv *priv = ufs_sprd_get_priv_data(hba);
+
+	dev_info(hba->dev, "ufs device reset!\n");
+
+	reset_control_assert(priv->rci[SPRD_UFS_DEV_RST].rc);
+	usleep_range(1000, 1100);
+	reset_control_deassert(priv->rci[SPRD_UFS_DEV_RST].rc);
+
+	return 0;
+}
+
+static void ufs_sprd_n6_key_acc_enable(struct ufs_hba *hba)
+{
+	u32 val;
+	u32 retry = 10;
+	struct arm_smccc_res res;
+
+check_hce:
+	/* Key access only can be enabled under HCE enable */
+	val = ufshcd_readl(hba, REG_CONTROLLER_ENABLE);
+	if (!(val & CONTROLLER_ENABLE)) {
+		ufs_sprd_n6_host_reset(hba);
+		val |= CONTROLLER_ENABLE;
+		ufshcd_writel(hba, val, REG_CONTROLLER_ENABLE);
+		usleep_range(1000, 1100);
+		if (retry) {
+			retry--;
+			goto check_hce;
+		}
+		goto disable_crypto;
+	}
+
+	arm_smccc_smc(SPRD_SIP_SVC_STORAGE_UFS_CRYPTO_ENABLE,
+		      0, 0, 0, 0, 0, 0, 0, &res);
+	if (!res.a0)
+		return;
+
+disable_crypto:
+	dev_err(hba->dev, "key reg access enable fail, disable crypto\n");
+	hba->caps &= ~UFSHCD_CAP_CRYPTO;
+}
+
+static int ufs_sprd_n6_init(struct ufs_hba *hba)
+{
+	struct ufs_sprd_priv *priv;
+	int ret = 0;
+
+	ret = ufs_sprd_common_init(hba);
+	if (ret != 0)
+		return ret;
+
+	priv = ufs_sprd_get_priv_data(hba);
+
+	clk_set_parent(priv->clki[SPRD_UFS_HCLK].clk,
+		       priv->clki[SPRD_UFS_HCLK_SOURCE].clk);
+
+	ret = regulator_enable(priv->vregi[SPRD_UFS_VDD_MPHY].vreg);
+	if (ret)
+		return -ENODEV;
+
+	if (hba->caps & UFSHCD_CAP_CRYPTO)
+		ufs_sprd_n6_key_acc_enable(hba);
+
+	return 0;
+}
+
+static int ufs_sprd_n6_phy_init(struct ufs_hba *hba)
+{
+	int ret = 0;
+	uint32_t val = 0;
+	uint32_t retry = 10;
+	uint32_t offset;
+	struct ufs_sprd_priv *priv = ufs_sprd_get_priv_data(hba);
+
+	ufshcd_dme_set(hba, UIC_ARG_MIB(0x8132), 0x90);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(0x811F), 0x01);
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x8009,
+				UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x01);
+	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0x8009,
+				UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x01);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(0xD085), 0x01);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(0x8114), 0x01);
+
+	do {
+		/* phy_sram_init_done */
+		ufs_sprd_regmap_read(priv, SPRD_UFS_ANLG_REG, 0xc, &val);
+		if ((val & 0x1) == 0x1) {
+			for (offset = 0x40; offset < 0x42; offset++) {
+				/* Lane afe calibration */
+				ufshcd_dme_set(hba, UIC_ARG_MIB(0x8116), 0x1c);
+				ufshcd_dme_set(hba, UIC_ARG_MIB(0x8117), offset);
+				ufshcd_dme_set(hba, UIC_ARG_MIB(0x8118), 0x04);
+				ufshcd_dme_set(hba, UIC_ARG_MIB(0x8119), 0x00);
+				ufshcd_dme_set(hba, UIC_ARG_MIB(0x811C), 0x01);
+				ufshcd_dme_set(hba, UIC_ARG_MIB(0xD085), 0x01);
+			}
+
+			goto update_phy;
+		}
+		udelay(1000);
+		retry--;
+	} while (retry > 0);
+
+	ret = -ETIMEDOUT;
+	goto out;
+
+update_phy:
+	/* phy_sram_ext_ld_done */
+	ufs_sprd_regmap_update(priv, SPRD_UFS_ANLG_REG, 0xc, 0x2, 0);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(0xD085), 0x01);
+	ufshcd_dme_set(hba, UIC_ARG_MIB(0xD0C1), 0x0);
+out:
+	return ret;
+}
+
+
+static int sprd_ufs_n6_hce_enable_notify(struct ufs_hba *hba,
+					 enum ufs_notify_change_status status)
+{
+	int err = 0;
+	struct ufs_sprd_priv *priv = ufs_sprd_get_priv_data(hba);
+
+	if (status == PRE_CHANGE) {
+		/* phy_sram_ext_ld_done */
+		ufs_sprd_regmap_update(priv, SPRD_UFS_ANLG_REG, 0xc, 0x2, 0x2);
+		/* phy_sram_bypass */
+		ufs_sprd_regmap_update(priv, SPRD_UFS_ANLG_REG, 0xc, 0x4, 0x4);
+
+		ufs_sprd_n6_host_reset(hba);
+
+		if (hba->caps & UFSHCD_CAP_CRYPTO)
+			ufs_sprd_n6_key_acc_enable(hba);
+	}
+
+	if (status == POST_CHANGE) {
+		err = ufs_sprd_n6_phy_init(hba);
+		if (err) {
+			dev_err(hba->dev, "Phy setup failed (%d)\n", err);
+			goto out;
+		}
+
+		ufs_sprd_get_unipro_ver(hba);
+	}
+out:
+	return err;
+}
+
+static void sprd_ufs_n6_h8_notify(struct ufs_hba *hba,
+				  enum uic_cmd_dme cmd,
+				  enum ufs_notify_change_status status)
+{
+	struct ufs_sprd_priv *priv = ufs_sprd_get_priv_data(hba);
+
+	if (status == PRE_CHANGE) {
+		if (cmd == UIC_CMD_DME_HIBER_ENTER)
+			/*
+			 * Disable UIC COMPL INTR to prevent access to UFSHCI after
+			 * checking HCS.UPMCRS
+			 */
+			ufs_sprd_ctrl_uic_compl(hba, false);
+
+		if (cmd == UIC_CMD_DME_HIBER_EXIT) {
+			ufs_sprd_regmap_update(priv, SPRD_UFS_AON_APB, APB_UFSDEV_REG,
+				APB_UFSDEV_REFCLK_EN, APB_UFSDEV_REFCLK_EN);
+			ufs_sprd_regmap_update(priv, SPRD_UFS_AON_APB, APB_USB31PLL_CTRL,
+				APB_USB31PLLV_REF2MPHY, APB_USB31PLLV_REF2MPHY);
+		}
+	}
+
+	if (status == POST_CHANGE) {
+		if (cmd == UIC_CMD_DME_HIBER_EXIT)
+			ufs_sprd_ctrl_uic_compl(hba, true);
+
+		if (cmd == UIC_CMD_DME_HIBER_ENTER) {
+			ufs_sprd_regmap_update(priv, SPRD_UFS_AON_APB, APB_UFSDEV_REG,
+				APB_UFSDEV_REFCLK_EN, 0);
+			ufs_sprd_regmap_update(priv, SPRD_UFS_AON_APB, APB_USB31PLL_CTRL,
+				APB_USB31PLLV_REF2MPHY, 0);
+		}
+	}
+}
+
+static struct ufs_sprd_priv n6_ufs = {
+	.clki[SPRD_UFS_HCLK] = { .name = "ufs_hclk", },
+	.clki[SPRD_UFS_HCLK_SOURCE] = { .name = "ufs_hclk_source", },
+
+	.rci[SPRD_UFSHCI_SOFT_RST] = { .name = "ufs_soft_rst", },
+	.rci[SPRD_UFS_DEV_RST] = { .name = "ufsdev_soft_rst", },
+
+	.sysci[SPRD_UFS_ANLG_REG] = { .name = "sprd,ufs-anly-reg-syscon", },
+	.sysci[SPRD_UFS_AON_APB] = { .name = "sprd,aon-apb-syscon", },
+
+	.vregi[SPRD_UFS_VDD_MPHY] = { .name = "vdd-mphy", },
+
+	.ufs_hba_sprd_vops = {
+		.name = "sprd,ums9620-ufs",
+		.init = ufs_sprd_n6_init,
+		.hce_enable_notify = sprd_ufs_n6_hce_enable_notify,
+		.pwr_change_notify = sprd_ufs_pwr_change_notify,
+		.hibern8_notify = sprd_ufs_n6_h8_notify,
+		.device_reset = ufs_sprd_n6_device_reset,
+		.suspend = ufs_sprd_suspend,
+	},
+};
+
+static const struct of_device_id ufs_sprd_of_match[] = {
+	{ .compatible = "sprd,ums9620-ufs", .data = &n6_ufs.ufs_hba_sprd_vops},
+	{},
+};
+
+static int ufs_sprd_probe(struct platform_device *pdev)
+{
+	int err;
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id;
+
+	of_id = of_match_node(ufs_sprd_of_match, dev->of_node);
+	err = ufshcd_pltfrm_init(pdev, of_id->data);
+	if (err)
+		dev_err(dev, "ufshcd_pltfrm_init() failed %d\n", err);
+
+	return err;
+}
+
+static int ufs_sprd_remove(struct platform_device *pdev)
+{
+	struct ufs_hba *hba =  platform_get_drvdata(pdev);
+
+	pm_runtime_get_sync(&(pdev)->dev);
+	ufshcd_remove(hba);
+	return 0;
+}
+
+static const struct dev_pm_ops ufs_sprd_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	 = ufshcd_resume_complete,
+};
+
+static struct platform_driver ufs_sprd_pltform = {
+	.probe = ufs_sprd_probe,
+	.remove = ufs_sprd_remove,
+	.shutdown = ufshcd_pltfrm_shutdown,
+	.driver = {
+		.name = "ufshcd-sprd",
+		.pm = &ufs_sprd_pm_ops,
+		.of_match_table = of_match_ptr(ufs_sprd_of_match),
+	},
+};
+module_platform_driver(ufs_sprd_pltform);
+
+MODULE_AUTHOR("Zhe Wang <zhe.wang1@unisoc.com>");
+MODULE_DESCRIPTION("Unisoc UFS Host Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/ufs/host/ufs-sprd.h b/drivers/ufs/host/ufs-sprd.h
new file mode 100644
index 000000000000..215e7483d1e8
--- /dev/null
+++ b/drivers/ufs/host/ufs-sprd.h
@@ -0,0 +1,125 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * UNISOC UFS Host Controller driver
+ *
+ * Copyright (C) 2022 Unisoc, Inc.
+ * Author: Zhe Wang <zhe.wang1@unisoc.com>
+ */
+
+#ifndef _UFS_SPRD_H_
+#define _UFS_SPRD_H_
+
+#define APB_UFSDEV_REG		0xCE8
+#define APB_UFSDEV_REFCLK_EN	0x2
+#define APB_USB31PLL_CTRL	0xCFC
+#define APB_USB31PLLV_REF2MPHY	0x1
+
+#define SPRD_SIP_SVC_STORAGE_UFS_CRYPTO_ENABLE				\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_SIP,				\
+			   0x0301)
+
+enum SPRD_UFS_CLK_INDEX {
+	SPRD_UFS_HCLK,
+	SPRD_UFS_HCLK_SOURCE,
+
+	SPRD_UFS_CLK_MAX
+};
+
+enum SPRD_UFS_RST_INDEX {
+	SPRD_UFSHCI_SOFT_RST,
+	SPRD_UFS_DEV_RST,
+
+	SPRD_UFS_RST_MAX
+};
+
+enum SPRD_UFS_SYSCON_INDEX {
+	SPRD_UFS_ANLG_REG,
+	SPRD_UFS_AON_APB,
+
+	SPRD_UFS_SYSCON_MAX
+};
+
+enum SPRD_UFS_VREG_INDEX {
+	SPRD_UFS_VDD_MPHY,
+
+	SPRD_UFS_VREG_MAX
+};
+
+struct ufs_sprd_clk {
+	const char *name;
+	struct clk *clk;
+};
+
+struct ufs_sprd_rst {
+	const char *name;
+	struct reset_control *rc;
+};
+
+struct ufs_sprd_syscon {
+	const char *name;
+	struct regmap *regmap;
+};
+
+struct ufs_sprd_vreg {
+	const char *name;
+	struct regulator *vreg;
+};
+
+struct ufs_sprd_priv {
+	struct ufs_sprd_clk clki[SPRD_UFS_CLK_MAX];
+	struct ufs_sprd_rst rci[SPRD_UFS_RST_MAX];
+	struct ufs_sprd_syscon sysci[SPRD_UFS_SYSCON_MAX];
+	struct ufs_sprd_vreg vregi[SPRD_UFS_VREG_MAX];
+	const struct ufs_hba_variant_ops ufs_hba_sprd_vops;
+};
+
+struct ufs_sprd_host {
+	struct ufs_hba *hba;
+	struct ufs_sprd_priv *priv;
+	void __iomem *ufs_dbg_mmio;
+
+	enum ufs_unipro_ver unipro_ver;
+};
+
+static inline struct ufs_sprd_priv *ufs_sprd_get_priv_data(struct ufs_hba *hba)
+{
+	struct ufs_sprd_host *host = ufshcd_get_variant(hba);
+
+	WARN_ON(!host->priv);
+	return host->priv;
+}
+
+static inline void ufs_sprd_regmap_update(struct ufs_sprd_priv *priv, unsigned int index,
+		unsigned int reg, unsigned int bits,  unsigned int val)
+{
+	regmap_update_bits(priv->sysci[index].regmap, reg, bits, val);
+}
+
+static inline void ufs_sprd_regmap_read(struct ufs_sprd_priv *priv, unsigned int index,
+		unsigned int reg, unsigned int *val)
+{
+	regmap_read(priv->sysci[index].regmap, reg, val);
+}
+
+static inline void ufs_sprd_get_unipro_ver(struct ufs_hba *hba)
+{
+	struct ufs_sprd_host *host = ufshcd_get_variant(hba);
+
+	if (ufshcd_dme_get(hba, UIC_ARG_MIB(PA_LOCALVERINFO), &host->unipro_ver))
+		host->unipro_ver = 0;
+}
+
+static inline void ufs_sprd_ctrl_uic_compl(struct ufs_hba *hba, bool enable)
+{
+	u32 set = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
+
+	if (enable == true)
+		set |= UIC_COMMAND_COMPL;
+	else
+		set &= ~UIC_COMMAND_COMPL;
+	ufshcd_writel(hba, set, REG_INTERRUPT_ENABLE);
+}
+
+#endif /* _UFS_SPRD_H_ */
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: ufs: Add document for Unisoc UFS host controller

[Krzysztof Kozlowski]

On 10/11/2022 14:36, Zhe Wang wrote:
> From: Zhe Wang <zhe.wang1@unisoc.com>
> 
> Add Unisoc ums9620 ufs host controller devicetree document.
> 
> Signed-off-by: Zhe Wang <zhe.wang1@unisoc.com>
> ---
>  .../devicetree/bindings/ufs/sprd,ufs.yaml     | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
> 
> diff --git a/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml b/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
> new file mode 100644
> index 000000000000..88f2c670b0a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml


Filename matching the compatible, so sprd,ums9620-ufs.yaml, unless you
expect this to grow already? If so, can you post the rest?

> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/ufs/sprd,ufs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Unisoc Universal Flash Storage (UFS) Controller
> +
> +maintainers:
> +  - Zhe Wang <zhe.wang1@unisoc.com>
> +
> +allOf:
> +  - $ref: ufs-common.yaml
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sprd,ums9620-ufs
> +
> +  clocks:
> +    maxItems: 2
> +
> +  clock-names:
> +    items:
> +      - const: hclk
> +      - const: hclk_source

Can you make these descriptive? "clk" is redundant, so basically you are
saying name is "h" and "h_source"?

> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: ufs_soft_rst
> +      - const: ufsdev_soft_rst

Drop "_rst" from both.

> +
> +  sprd,ufs-anly-reg-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of syscon used to control ufs analog reg.

It's a reg? Then such syntax is expected:
https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42


> +  sprd,aon-apb-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of syscon used to control always-on reg.

It's a reg? Then such syntax is expected:
https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - resets
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    ufs: ufs@22000000 {
> +        compatible = "sprd,ums9620-ufs";
> +        reg = <0x22000000 0x3000>;
> +        interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
> +        vcc-supply = <&vddemmcore>;
> +        vdd-mphy-supply = <&vddufs1v2>;
> +        clocks-name = "ufs_eb", "ufs_cfg_eb",
> +            "ufs_hclk", "ufs_hclk_source";

Align the lines.

> +        clocks = <&apahb_gate CLK_UFS_EB>, <&apahb_gate CLK_UFS_CFG_EB>,
> +            <&onapb_clk CLK_UFS_AON>, <&g51_pll CLK_TGPLL_256M>;
> +        freq-table-hz = <0 0>, <0 0>, <0 0>, <0 0>;

Why this is empty? What's the use of empty table?

> +        reset-names = "ufs_soft_rst", "ufsdev_soft_rst";
> +        resets = <&apahb_gate RESET_AP_AHB_UFS_SOFT_RST>,
> +            <&aonapb_gate RESET_AON_APB_UFSDEV_SOFT_RST>;
> +        sprd,ufs-anly-reg-syscon = <&anly_phy_g12_regs>;
> +        sprd,aon-apb-syscon = <&aon_apb_regs>;
> +        status = "disable";

Drop status.

> +    };

Best regards,
Krzysztof

Re: [PATCH 2/2] scsi: ufs-unisoc: Add support for Unisoc UFS host controller

[Krzysztof Kozlowski]

On 10/11/2022 14:36, Zhe Wang wrote:
> From: Zhe Wang <zhe.wang1@unisoc.com>
> 
> Add driver code for Unisoc ufs host controller, along with ufs
> initialization.

(...)

> +
> +static struct platform_driver ufs_sprd_pltform = {
> +	.probe = ufs_sprd_probe,
> +	.remove = ufs_sprd_remove,
> +	.shutdown = ufshcd_pltfrm_shutdown,
> +	.driver = {
> +		.name = "ufshcd-sprd",
> +		.pm = &ufs_sprd_pm_ops,
> +		.of_match_table = of_match_ptr(ufs_sprd_of_match),

Drop of_match_ptr

> +	},
> +};
> +module_platform_driver(ufs_sprd_pltform);
> +
> +MODULE_AUTHOR("Zhe Wang <zhe.wang1@unisoc.com>");
> +MODULE_DESCRIPTION("Unisoc UFS Host Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/ufs/host/ufs-sprd.h b/drivers/ufs/host/ufs-sprd.h
> new file mode 100644
> index 000000000000..215e7483d1e8
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-sprd.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * UNISOC UFS Host Controller driver
> + *
> + * Copyright (C) 2022 Unisoc, Inc.
> + * Author: Zhe Wang <zhe.wang1@unisoc.com>
> + */
> +
> +#ifndef _UFS_SPRD_H_
> +#define _UFS_SPRD_H_
> +
> +#define APB_UFSDEV_REG		0xCE8
> +#define APB_UFSDEV_REFCLK_EN	0x2
> +#define APB_USB31PLL_CTRL	0xCFC
> +#define APB_USB31PLLV_REF2MPHY	0x1
> +
> +#define SPRD_SIP_SVC_STORAGE_UFS_CRYPTO_ENABLE				\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
> +			   ARM_SMCCC_SMC_32,				\
> +			   ARM_SMCCC_OWNER_SIP,				\
> +			   0x0301)
> +
> +enum SPRD_UFS_CLK_INDEX {
> +	SPRD_UFS_HCLK,
> +	SPRD_UFS_HCLK_SOURCE,
> +
> +	SPRD_UFS_CLK_MAX
> +};
> +
> +enum SPRD_UFS_RST_INDEX {
> +	SPRD_UFSHCI_SOFT_RST,
> +	SPRD_UFS_DEV_RST,
> +
> +	SPRD_UFS_RST_MAX
> +};
> +
> +enum SPRD_UFS_SYSCON_INDEX {
> +	SPRD_UFS_ANLG_REG,
> +	SPRD_UFS_AON_APB,
> +
> +	SPRD_UFS_SYSCON_MAX
> +};
> +
> +enum SPRD_UFS_VREG_INDEX {
> +	SPRD_UFS_VDD_MPHY,
> +
> +	SPRD_UFS_VREG_MAX
> +};
> +
> +struct ufs_sprd_clk {
> +	const char *name;
> +	struct clk *clk;
> +};
> +
> +struct ufs_sprd_rst {
> +	const char *name;
> +	struct reset_control *rc;
> +};
> +
> +struct ufs_sprd_syscon {
> +	const char *name;
> +	struct regmap *regmap;
> +};
> +
> +struct ufs_sprd_vreg {
> +	const char *name;
> +	struct regulator *vreg;
> +};
> +
> +struct ufs_sprd_priv {
> +	struct ufs_sprd_clk clki[SPRD_UFS_CLK_MAX];
> +	struct ufs_sprd_rst rci[SPRD_UFS_RST_MAX];
> +	struct ufs_sprd_syscon sysci[SPRD_UFS_SYSCON_MAX];
> +	struct ufs_sprd_vreg vregi[SPRD_UFS_VREG_MAX];
> +	const struct ufs_hba_variant_ops ufs_hba_sprd_vops;
> +};
> +
> +struct ufs_sprd_host {
> +	struct ufs_hba *hba;
> +	struct ufs_sprd_priv *priv;
> +	void __iomem *ufs_dbg_mmio;
> +
> +	enum ufs_unipro_ver unipro_ver;
> +};
> +
> +static inline struct ufs_sprd_priv *ufs_sprd_get_priv_data(struct ufs_hba *hba)
> +{
> +	struct ufs_sprd_host *host = ufshcd_get_variant(hba);
> +
> +	WARN_ON(!host->priv);
> +	return host->priv;
> +}
> +
> +static inline void ufs_sprd_regmap_update(struct ufs_sprd_priv *priv, unsigned int index,
> +		unsigned int reg, unsigned int bits,  unsigned int val)
> +{
> +	regmap_update_bits(priv->sysci[index].regmap, reg, bits, val);
> +}
> +
> +static inline void ufs_sprd_regmap_read(struct ufs_sprd_priv *priv, unsigned int index,
> +		unsigned int reg, unsigned int *val)
> +{
> +	regmap_read(priv->sysci[index].regmap, reg, val);
> +}
> +
> +static inline void ufs_sprd_get_unipro_ver(struct ufs_hba *hba)
> +{
> +	struct ufs_sprd_host *host = ufshcd_get_variant(hba);
> +
> +	if (ufshcd_dme_get(hba, UIC_ARG_MIB(PA_LOCALVERINFO), &host->unipro_ver))
> +		host->unipro_ver = 0;
> +}
> +
> +static inline void ufs_sprd_ctrl_uic_compl(struct ufs_hba *hba, bool enable)
> +{
> +	u32 set = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> +
> +	if (enable == true)
> +		set |= UIC_COMMAND_COMPL;
> +	else
> +		set &= ~UIC_COMMAND_COMPL;
> +	ufshcd_writel(hba, set, REG_INTERRUPT_ENABLE);
> +}

Drop functions from headers. These are not stubs and your task is not to
micro-optimize code.

> +
> +#endif /* _UFS_SPRD_H_ */

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: ufs: Add document for Unisoc UFS host controller

[Rob Herring]

On Thu, 10 Nov 2022 21:36:39 +0800, Zhe Wang wrote:
> From: Zhe Wang <zhe.wang1@unisoc.com>
> 
> Add Unisoc ums9620 ufs host controller devicetree document.
> 
> Signed-off-by: Zhe Wang <zhe.wang1@unisoc.com>
> ---
>  .../devicetree/bindings/ufs/sprd,ufs.yaml     | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/ufs/sprd,ufs.example.dts:24.27-28 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/ufs/sprd,ufs.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.

Re: [PATCH 2/2] scsi: ufs-unisoc: Add support for Unisoc UFS host controller

[Zhe Wang]

Hi Krzysztof,

Thank you for your review!

On Thu, Nov 10, 2022 at 10:31 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 10/11/2022 14:36, Zhe Wang wrote:
> > From: Zhe Wang <zhe.wang1@unisoc.com>
> >
> > Add driver code for Unisoc ufs host controller, along with ufs
> > initialization.
>
> (...)
>
> > +
> > +static struct platform_driver ufs_sprd_pltform = {
> > +     .probe = ufs_sprd_probe,
> > +     .remove = ufs_sprd_remove,
> > +     .shutdown = ufshcd_pltfrm_shutdown,
> > +     .driver = {
> > +             .name = "ufshcd-sprd",
> > +             .pm = &ufs_sprd_pm_ops,
> > +             .of_match_table = of_match_ptr(ufs_sprd_of_match),
>
> Drop of_match_ptr
>

I'll drop this.

> > +     },
> > +};
> > +module_platform_driver(ufs_sprd_pltform);
> > +
> > +MODULE_AUTHOR("Zhe Wang <zhe.wang1@unisoc.com>");
> > +MODULE_DESCRIPTION("Unisoc UFS Host Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/ufs/host/ufs-sprd.h b/drivers/ufs/host/ufs-sprd.h
> > new file mode 100644
> > index 000000000000..215e7483d1e8
> > --- /dev/null
> > +++ b/drivers/ufs/host/ufs-sprd.h
> > @@ -0,0 +1,125 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * UNISOC UFS Host Controller driver
> > + *
> > + * Copyright (C) 2022 Unisoc, Inc.
> > + * Author: Zhe Wang <zhe.wang1@unisoc.com>
> > + */
> > +
> > +#ifndef _UFS_SPRD_H_
> > +#define _UFS_SPRD_H_
> > +
> > +#define APB_UFSDEV_REG               0xCE8
> > +#define APB_UFSDEV_REFCLK_EN 0x2
> > +#define APB_USB31PLL_CTRL    0xCFC
> > +#define APB_USB31PLLV_REF2MPHY       0x1
> > +
> > +#define SPRD_SIP_SVC_STORAGE_UFS_CRYPTO_ENABLE                               \
> > +     ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                         \
> > +                        ARM_SMCCC_SMC_32,                            \
> > +                        ARM_SMCCC_OWNER_SIP,                         \
> > +                        0x0301)
> > +
> > +enum SPRD_UFS_CLK_INDEX {
> > +     SPRD_UFS_HCLK,
> > +     SPRD_UFS_HCLK_SOURCE,
> > +
> > +     SPRD_UFS_CLK_MAX
> > +};
> > +
> > +enum SPRD_UFS_RST_INDEX {
> > +     SPRD_UFSHCI_SOFT_RST,
> > +     SPRD_UFS_DEV_RST,
> > +
> > +     SPRD_UFS_RST_MAX
> > +};
> > +
> > +enum SPRD_UFS_SYSCON_INDEX {
> > +     SPRD_UFS_ANLG_REG,
> > +     SPRD_UFS_AON_APB,
> > +
> > +     SPRD_UFS_SYSCON_MAX
> > +};
> > +
> > +enum SPRD_UFS_VREG_INDEX {
> > +     SPRD_UFS_VDD_MPHY,
> > +
> > +     SPRD_UFS_VREG_MAX
> > +};
> > +
> > +struct ufs_sprd_clk {
> > +     const char *name;
> > +     struct clk *clk;
> > +};
> > +
> > +struct ufs_sprd_rst {
> > +     const char *name;
> > +     struct reset_control *rc;
> > +};
> > +
> > +struct ufs_sprd_syscon {
> > +     const char *name;
> > +     struct regmap *regmap;
> > +};
> > +
> > +struct ufs_sprd_vreg {
> > +     const char *name;
> > +     struct regulator *vreg;
> > +};
> > +
> > +struct ufs_sprd_priv {
> > +     struct ufs_sprd_clk clki[SPRD_UFS_CLK_MAX];
> > +     struct ufs_sprd_rst rci[SPRD_UFS_RST_MAX];
> > +     struct ufs_sprd_syscon sysci[SPRD_UFS_SYSCON_MAX];
> > +     struct ufs_sprd_vreg vregi[SPRD_UFS_VREG_MAX];
> > +     const struct ufs_hba_variant_ops ufs_hba_sprd_vops;
> > +};
> > +
> > +struct ufs_sprd_host {
> > +     struct ufs_hba *hba;
> > +     struct ufs_sprd_priv *priv;
> > +     void __iomem *ufs_dbg_mmio;
> > +
> > +     enum ufs_unipro_ver unipro_ver;
> > +};
> > +
> > +static inline struct ufs_sprd_priv *ufs_sprd_get_priv_data(struct ufs_hba *hba)
> > +{
> > +     struct ufs_sprd_host *host = ufshcd_get_variant(hba);
> > +
> > +     WARN_ON(!host->priv);
> > +     return host->priv;
> > +}
> > +
> > +static inline void ufs_sprd_regmap_update(struct ufs_sprd_priv *priv, unsigned int index,
> > +             unsigned int reg, unsigned int bits,  unsigned int val)
> > +{
> > +     regmap_update_bits(priv->sysci[index].regmap, reg, bits, val);
> > +}
> > +
> > +static inline void ufs_sprd_regmap_read(struct ufs_sprd_priv *priv, unsigned int index,
> > +             unsigned int reg, unsigned int *val)
> > +{
> > +     regmap_read(priv->sysci[index].regmap, reg, val);
> > +}
> > +
> > +static inline void ufs_sprd_get_unipro_ver(struct ufs_hba *hba)
> > +{
> > +     struct ufs_sprd_host *host = ufshcd_get_variant(hba);
> > +
> > +     if (ufshcd_dme_get(hba, UIC_ARG_MIB(PA_LOCALVERINFO), &host->unipro_ver))
> > +             host->unipro_ver = 0;
> > +}
> > +
> > +static inline void ufs_sprd_ctrl_uic_compl(struct ufs_hba *hba, bool enable)
> > +{
> > +     u32 set = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> > +
> > +     if (enable == true)
> > +             set |= UIC_COMMAND_COMPL;
> > +     else
> > +             set &= ~UIC_COMMAND_COMPL;
> > +     ufshcd_writel(hba, set, REG_INTERRUPT_ENABLE);
> > +}
>
> Drop functions from headers. These are not stubs and your task is not to
> micro-optimize code.
>

Will move these functions to c file.

Best regards,
Zhe Wang

> > +
> > +#endif /* _UFS_SPRD_H_ */
>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/2] dt-bindings: ufs: Add document for Unisoc UFS host controller

[Zhe Wang]

Hi Krzysztof,

Thank you for your review!

On Thu, Nov 10, 2022 at 10:28 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 10/11/2022 14:36, Zhe Wang wrote:
> > From: Zhe Wang <zhe.wang1@unisoc.com>
> >
> > Add Unisoc ums9620 ufs host controller devicetree document.
> >
> > Signed-off-by: Zhe Wang <zhe.wang1@unisoc.com>
> > ---
> >  .../devicetree/bindings/ufs/sprd,ufs.yaml     | 72 +++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml b/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
> > new file mode 100644
> > index 000000000000..88f2c670b0a4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ufs/sprd,ufs.yaml
>
>
> Filename matching the compatible, so sprd,ums9620-ufs.yaml, unless you
> expect this to grow already? If so, can you post the rest?
>

Currently only ums9620 will be uploaded, I'll fix it.

> > @@ -0,0 +1,72 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/ufs/sprd,ufs.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Unisoc Universal Flash Storage (UFS) Controller
> > +
> > +maintainers:
> > +  - Zhe Wang <zhe.wang1@unisoc.com>
> > +
> > +allOf:
> > +  - $ref: ufs-common.yaml
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sprd,ums9620-ufs
> > +
> > +  clocks:
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    items:
> > +      - const: hclk
> > +      - const: hclk_source
>
> Can you make these descriptive? "clk" is redundant, so basically you are
> saying name is "h" and "h_source"?
>

I'll fix it.

> > +
> > +  resets:
> > +    maxItems: 2
> > +
> > +  reset-names:
> > +    items:
> > +      - const: ufs_soft_rst
> > +      - const: ufsdev_soft_rst
>
> Drop "_rst" from both.
>

Will remove this.

> > +
> > +  sprd,ufs-anly-reg-syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle of syscon used to control ufs analog reg.
>
> It's a reg? Then such syntax is expected:
> https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42
>
>

I will modify it based on this example.

> > +  sprd,aon-apb-syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle of syscon used to control always-on reg.
>
> It's a reg? Then such syntax is expected:
> https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42
>

I will modify it based on this example.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    ufs: ufs@22000000 {
> > +        compatible = "sprd,ums9620-ufs";
> > +        reg = <0x22000000 0x3000>;
> > +        interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
> > +        vcc-supply = <&vddemmcore>;
> > +        vdd-mphy-supply = <&vddufs1v2>;
> > +        clocks-name = "ufs_eb", "ufs_cfg_eb",
> > +            "ufs_hclk", "ufs_hclk_source";
>
> Align the lines.
>

I'll fix it.

> > +        clocks = <&apahb_gate CLK_UFS_EB>, <&apahb_gate CLK_UFS_CFG_EB>,
> > +            <&onapb_clk CLK_UFS_AON>, <&g51_pll CLK_TGPLL_256M>;
> > +        freq-table-hz = <0 0>, <0 0>, <0 0>, <0 0>;
>
> Why this is empty? What's the use of empty table?
>

freq-table-hz is used to configure the maximum frequency and minimum
frequency of clk, and an empty table means that no scaling up\down
operation is requiredfor the frequency of these clks.

> > +        reset-names = "ufs_soft_rst", "ufsdev_soft_rst";
> > +        resets = <&apahb_gate RESET_AP_AHB_UFS_SOFT_RST>,
> > +            <&aonapb_gate RESET_AON_APB_UFSDEV_SOFT_RST>;
> > +        sprd,ufs-anly-reg-syscon = <&anly_phy_g12_regs>;
> > +        sprd,aon-apb-syscon = <&aon_apb_regs>;
> > +        status = "disable";
>
> Drop status.
>

Will remove this.

Best regards,
Zhe Wang

> > +    };
>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/2] dt-bindings: ufs: Add document for Unisoc UFS host controller

[Krzysztof Kozlowski]

On 11/11/2022 06:34, Zhe Wang wrote:
>>
> 
> I'll fix it.
> 
>>> +        clocks = <&apahb_gate CLK_UFS_EB>, <&apahb_gate CLK_UFS_CFG_EB>,
>>> +            <&onapb_clk CLK_UFS_AON>, <&g51_pll CLK_TGPLL_256M>;
>>> +        freq-table-hz = <0 0>, <0 0>, <0 0>, <0 0>;
>>
>> Why this is empty? What's the use of empty table?
>>
> 
> freq-table-hz is used to configure the maximum frequency and minimum
> frequency of clk, and an empty table means that no scaling up\down
> operation is requiredfor the frequency of these clks.

No, to indicate lack of scaling you skip freq-table-hz entirely, not
provide empty one.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: ufs: Add document for Unisoc UFS host controller

[Zhe Wang]

Hi Krzysztof,

On Fri, Nov 11, 2022 at 3:48 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/11/2022 06:34, Zhe Wang wrote:
> >>
> >
> > I'll fix it.
> >
> >>> +        clocks = <&apahb_gate CLK_UFS_EB>, <&apahb_gate CLK_UFS_CFG_EB>,
> >>> +            <&onapb_clk CLK_UFS_AON>, <&g51_pll CLK_TGPLL_256M>;
> >>> +        freq-table-hz = <0 0>, <0 0>, <0 0>, <0 0>;
> >>
> >> Why this is empty? What's the use of empty table?
> >>
> >
> > freq-table-hz is used to configure the maximum frequency and minimum
> > frequency of clk, and an empty table means that no scaling up\down
> > operation is requiredfor the frequency of these clks.
>
> No, to indicate lack of scaling you skip freq-table-hz entirely, not
> provide empty one.
>
>

In the ufshcd-pltfrm.c file, the clock information is parsed by
executing the function ufshcd_parse_clock_info, if the number of
"freq-table-hz" is zero or if the number of "clock-names" and
"freq-table-hz" does not match, the UFS CLK information in dts will
not be obtained. Although we don't need to scaling freq, we also need
the CLK information for the CLK GATE operations. So we cannot delete
this freq-table here.

> Best regards,
> Krzysztof
>

According to the local test results just now, I would like to ask a
question about the previous revisions.
> > +
> > +  sprd,ufs-anly-reg-syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle of syscon used to control ufs analog reg.
>
> It's a reg? Then such syntax is expected:
> https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42
>

In the syntax of this example, reg is represented by phandle and
offset, but I only need the information of phandle in this place，So in
this scenario, whether my original syntax is fine？ just describe the
pandlle.

Best regards,
Zhe Wang

Re: [PATCH 1/2] dt-bindings: ufs: Add document for Unisoc UFS host controller

[Krzysztof Kozlowski]

On 11/11/2022 10:34, Zhe Wang wrote:
> Hi Krzysztof,
> 
> On Fri, Nov 11, 2022 at 3:48 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/11/2022 06:34, Zhe Wang wrote:
>>>>
>>>
>>> I'll fix it.
>>>
>>>>> +        clocks = <&apahb_gate CLK_UFS_EB>, <&apahb_gate CLK_UFS_CFG_EB>,
>>>>> +            <&onapb_clk CLK_UFS_AON>, <&g51_pll CLK_TGPLL_256M>;
>>>>> +        freq-table-hz = <0 0>, <0 0>, <0 0>, <0 0>;
>>>>
>>>> Why this is empty? What's the use of empty table?
>>>>
>>>
>>> freq-table-hz is used to configure the maximum frequency and minimum
>>> frequency of clk, and an empty table means that no scaling up\down
>>> operation is requiredfor the frequency of these clks.
>>
>> No, to indicate lack of scaling you skip freq-table-hz entirely, not
>> provide empty one.
>>
>>
> 
> In the ufshcd-pltfrm.c file, the clock information is parsed by
> executing the function ufshcd_parse_clock_info, if the number of
> "freq-table-hz" is zero or if the number of "clock-names" and
> "freq-table-hz" does not match, the UFS CLK information in dts will
> not be obtained. Although we don't need to scaling freq, we also need
> the CLK information for the CLK GATE operations. So we cannot delete
> this freq-table here.

I did not check the driver implementation, but if that's the case it
does not match bindings. Before adding empty useless tables, please
either fix bindings or driver. I think the latter - the driver - becasue
clocks are not depending logically on freq-table-hz.

> 
>> Best regards,
>> Krzysztof
>>
> 
> According to the local test results just now, I would like to ask a
> question about the previous revisions.
>>> +
>>> +  sprd,ufs-anly-reg-syscon:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: phandle of syscon used to control ufs analog reg.
>>
>> It's a reg? Then such syntax is expected:
>> https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42
>>
> 
> In the syntax of this example, reg is represented by phandle and
> offset, but I only need the information of phandle in this place，

No. You wrote "analog reg". So one reg. Not regs.

> So in
> this scenario, whether my original syntax is fine？ just describe the
> pandlle.

No, because your description said one reg.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: ufs: Add document for Unisoc UFS host controller

[Zhe Wang]

Hi Krzysztof,

On Fri, Nov 11, 2022 at 5:51 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/11/2022 10:34, Zhe Wang wrote:
> > Hi Krzysztof,
> >
> > On Fri, Nov 11, 2022 at 3:48 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/11/2022 06:34, Zhe Wang wrote:
> >>>>
> >>>
> >>> I'll fix it.
> >>>
> >>>>> +        clocks = <&apahb_gate CLK_UFS_EB>, <&apahb_gate CLK_UFS_CFG_EB>,
> >>>>> +            <&onapb_clk CLK_UFS_AON>, <&g51_pll CLK_TGPLL_256M>;
> >>>>> +        freq-table-hz = <0 0>, <0 0>, <0 0>, <0 0>;
> >>>>
> >>>> Why this is empty? What's the use of empty table?
> >>>>
> >>>
> >>> freq-table-hz is used to configure the maximum frequency and minimum
> >>> frequency of clk, and an empty table means that no scaling up\down
> >>> operation is requiredfor the frequency of these clks.
> >>
> >> No, to indicate lack of scaling you skip freq-table-hz entirely, not
> >> provide empty one.
> >>
> >>
> >
> > In the ufshcd-pltfrm.c file, the clock information is parsed by
> > executing the function ufshcd_parse_clock_info, if the number of
> > "freq-table-hz" is zero or if the number of "clock-names" and
> > "freq-table-hz" does not match, the UFS CLK information in dts will
> > not be obtained. Although we don't need to scaling freq, we also need
> > the CLK information for the CLK GATE operations. So we cannot delete
> > this freq-table here.
>
> I did not check the driver implementation, but if that's the case it
> does not match bindings. Before adding empty useless tables, please
> either fix bindings or driver. I think the latter - the driver - becasue
> clocks are not depending logically on freq-table-hz.
>

OK, I will think about how to solve this problem.

> >
> >> Best regards,
> >> Krzysztof
> >>
> >
> > According to the local test results just now, I would like to ask a
> > question about the previous revisions.
> >>> +
> >>> +  sprd,ufs-anly-reg-syscon:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: phandle of syscon used to control ufs analog reg.
> >>
> >> It's a reg? Then such syntax is expected:
> >> https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42
> >>
> >
> > In the syntax of this example, reg is represented by phandle and
> > offset, but I only need the information of phandle in this place，
>
> No. You wrote "analog reg". So one reg. Not regs.
>
> > So in
> > this scenario, whether my original syntax is fine？ just describe the
> > pandlle.
>
> No, because your description said one reg.
>

This is my mistake, the description is wrong, it's regs.
Thank you for your review!

Best regards,
Zhe Wang

> Best regards,
> Krzysztof
>