[PATCH v2 0/5] MediaTek UFS Cleanup and MT8196 Enablement

[PATCH v2 0/5] MediaTek UFS Cleanup and MT8196 Enablement

[Nicolas Frattaroli]

In this series, the existing MediaTek UFS binding is expanded and
completed to correctly describe not just the existing compatibles, but
also to introduce a new compatible in the from of the MT8196 SoC.

The resets, which until now were completely absent from both the UFS
host controller binding and the UFS PHY binding, are introduced to both.
This also means the driver's undocumented and, in mainline, unused reset
logic is reworked. In particular, the PHY reset is no longer a reset of
the host controller node, but of the PHY node.

This means the host controller can reset the PHY through the common PHY
framework.

The resets remain optional.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Changes in v2:
- Reorder define in mtk_sip_svc.h
- Use bulk reset APIs in UFS host driver
- Link to v1: https://lore.kernel.org/r/20251014-mt8196-ufs-v1-0-195dceb83bc8@collabora.com

---
Nicolas Frattaroli (5):
      dt-bindings: ufs: mediatek,ufs: Add mt8196-ufshci variant
      dt-bindings: phy: Add mediatek,mt8196-ufsphy variant
      scsi: ufs: mediatek: Move MTK_SIP_UFS_CONTROL to mtk_sip_svc.h
      phy: mediatek: ufs: Add support for resets
      scsi: ufs: mediatek: Rework resets

 .../devicetree/bindings/phy/mediatek,ufs-phy.yaml  |  16 +++
 .../devicetree/bindings/ufs/mediatek,ufs.yaml      | 134 +++++++++++++++++++--
 drivers/phy/mediatek/phy-mtk-ufs.c                 |  71 +++++++++++
 drivers/ufs/host/ufs-mediatek-sip.h                |   9 --
 drivers/ufs/host/ufs-mediatek.c                    |  78 ++++++------
 drivers/ufs/host/ufs-mediatek.h                    |   7 +-
 include/linux/soc/mediatek/mtk_sip_svc.h           |   3 +
 7 files changed, 255 insertions(+), 63 deletions(-)
---
base-commit: 40a3abb0f3e5229996c8ef0498fc8d8a0c2bd64f
change-id: 20251014-mt8196-ufs-cec4b9a97e53

Best regards,
-- 
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 1/5] dt-bindings: ufs: mediatek,ufs: Add mt8196-ufshci variant

[Nicolas Frattaroli]

The MediaTek MT8196 SoC contains the same UFS host controller interface
hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
its list of allowed clocks, as well as give it the previously absent
resets property.

Also add examples for both MT8195 and the new MT8196, so that the
binding can be verified against examples for these two variants.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 .../devicetree/bindings/ufs/mediatek,ufs.yaml      | 134 +++++++++++++++++++--
 1 file changed, 123 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
index 1dec54fb00f3..070ae0982591 100644
--- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
+++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
@@ -11,18 +11,30 @@ maintainers:
 
 properties:
   compatible:
-    enum:
-      - mediatek,mt8183-ufshci
-      - mediatek,mt8192-ufshci
-      - mediatek,mt8195-ufshci
+    oneOf:
+      - enum:
+          - mediatek,mt8183-ufshci
+          - mediatek,mt8195-ufshci
+      - items:
+          - enum:
+              - mediatek,mt8192-ufshci
+          - const: mediatek,mt8183-ufshci
+      - items:
+          - enum:
+              - mediatek,mt8196-ufshci
+          - const: mediatek,mt8195-ufshci
 
   clocks:
     minItems: 1
-    maxItems: 8
+    maxItems: 16
 
   clock-names:
     minItems: 1
-    maxItems: 8
+    maxItems: 16
+
+  freq-table-hz: true
+
+  interrupts: true
 
   phys:
     maxItems: 1
@@ -30,7 +42,15 @@ properties:
   reg:
     maxItems: 1
 
+  resets:
+    maxItems: 3
+
+  reset-names:
+    maxItems: 3
+
   vcc-supply: true
+  vccq-supply: true
+  vccq2-supply: true
 
   mediatek,ufs-disable-mcq:
     $ref: /schemas/types.yaml#/definitions/flag
@@ -44,22 +64,19 @@ required:
   - reg
   - vcc-supply
 
-unevaluatedProperties: false
-
 allOf:
   - $ref: ufs-common.yaml
-
   - if:
       properties:
         compatible:
           contains:
-            enum:
-              - mediatek,mt8195-ufshci
+            const: mediatek,mt8195-ufshci
     then:
       properties:
         clocks:
           minItems: 8
         clock-names:
+          minItems: 8
           items:
             - const: ufs
             - const: ufs_aes
@@ -69,6 +86,19 @@ allOf:
             - const: unipro_mp_bclk
             - const: ufs_tx_symbol
             - const: ufs_mem_sub
+            - const: crypt_mux
+            - const: crypt_lp
+            - const: crypt_perf
+            - const: ufs_sel
+            - const: ufs_sel_min_src
+            - const: ufs_sel_max_src
+            - const: ufs_rx_symbol0
+            - const: ufs_rx_symbol1
+        reset-names:
+          items:
+            - const: unipro_rst
+            - const: crypto_rst
+            - const: hci_rst
     else:
       properties:
         clocks:
@@ -76,6 +106,10 @@ allOf:
         clock-names:
           items:
             - const: ufs
+        resets: false
+        reset-names: false
+
+unevaluatedProperties: false
 
 examples:
   - |
@@ -99,3 +133,81 @@ examples:
             vcc-supply = <&mt_pmic_vemc_ldo_reg>;
         };
     };
+  - |
+    ufshci@11270000 {
+        compatible = "mediatek,mt8195-ufshci";
+        reg = <0x11270000 0x2300>;
+        interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
+        phys = <&ufsphy>;
+        clocks = <&infracfg_ao 63>, <&infracfg_ao 64>, <&infracfg_ao 65>,
+                 <&infracfg_ao 54>, <&infracfg_ao 55>,
+                 <&infracfg_ao 56>, <&infracfg_ao 90>,
+                 <&infracfg_ao 93>;
+        clock-names = "ufs", "ufs_aes", "ufs_tick",
+                      "unipro_sysclk", "unipro_tick",
+                      "unipro_mp_bclk", "ufs_tx_symbol",
+                      "ufs_mem_sub";
+        freq-table-hz = <0 0>, <0 0>, <0 0>,
+                        <0 0>, <0 0>, <0 0>,
+                        <0 0>, <0 0>;
+        vcc-supply = <&mt6359_vemc_1_ldo_reg>;
+        mediatek,ufs-disable-mcq;
+    };
+  - |
+    #include <dt-bindings/reset/mediatek,mt8196-resets.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    ufshci@16810000 {
+        compatible = "mediatek,mt8196-ufshci", "mediatek,mt8195-ufshci";
+        reg = <0x16810000 0x2a00>;
+        interrupts = <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>;
+
+        clocks = <&ufs_ao_clk 6>,
+                 <&ufs_ao_clk 7>,
+                 <&clk26m>,
+                 <&ufs_ao_clk 3>,
+                 <&clk26m>,
+                 <&ufs_ao_clk 4>,
+                 <&ufs_ao_clk 0>,
+                 <&topckgen 7>,
+                 <&topckgen 41>,
+                 <&topckgen 105>,
+                 <&topckgen 83>,
+                 <&topckgen 42>,
+                 <&topckgen 84>,
+                 <&topckgen 102>,
+                 <&ufs_ao_clk 1>,
+                 <&ufs_ao_clk 2>;
+        clock-names = "ufs",
+                      "ufs_aes",
+                      "ufs_tick",
+                      "unipro_sysclk",
+                      "unipro_tick",
+                      "unipro_mp_bclk",
+                      "ufs_tx_symbol",
+                      "ufs_mem_sub",
+                      "crypt_mux",
+                      "crypt_lp",
+                      "crypt_perf",
+                      "ufs_sel",
+                      "ufs_sel_min_src",
+                      "ufs_sel_max_src",
+                      "ufs_rx_symbol0",
+                      "ufs_rx_symbol1";
+
+        freq-table-hz = <273000000 499200000>, <0 0>, <0 0>, <0 0>, <0 0>,
+                        <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>,
+                        <0 0>;
+
+        phys = <&ufsphy>;
+
+        vcc-supply = <&mt6363_vemc>;
+        vccq-supply = <&mt6363_vufs12>;
+        vccq2-supply = <&mt6363_vufs18>;
+
+        resets = <&ufs_ao_clk MT8196_UFSAO_RST1_UFS_UNIPRO>,
+                 <&ufs_ao_clk MT8196_UFSAO_RST1_UFS_CRYPTO>,
+                 <&ufs_ao_clk MT8196_UFSAO_RST1_UFSHCI>;
+        reset-names = "unipro_rst", "crypto_rst", "hci_rst";
+        mediatek,ufs-disable-mcq;
+    };

-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 2/5] dt-bindings: phy: Add mediatek,mt8196-ufsphy variant

[Nicolas Frattaroli]

The MediaTek MT8196 SoC includes an M-PHY compatible with the already
existing mt8183 binding.

However, one omission from the original binding was that all of these
variants may have an optional reset.

Add the new compatible, and also the resets property, with an example.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 .../devicetree/bindings/phy/mediatek,ufs-phy.yaml        | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/mediatek,ufs-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,ufs-phy.yaml
index 3e62b5d4da61..f414aaa18997 100644
--- a/Documentation/devicetree/bindings/phy/mediatek,ufs-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/mediatek,ufs-phy.yaml
@@ -26,6 +26,7 @@ properties:
       - items:
           - enum:
               - mediatek,mt8195-ufsphy
+              - mediatek,mt8196-ufsphy
           - const: mediatek,mt8183-ufsphy
       - const: mediatek,mt8183-ufsphy
 
@@ -42,6 +43,10 @@ properties:
       - const: unipro
       - const: mp
 
+  resets:
+    items:
+      - description: Optional UFS M-PHY reset.
+
   "#phy-cells":
     const: 0
 
@@ -65,5 +70,16 @@ examples:
         clock-names = "unipro", "mp";
         #phy-cells = <0>;
     };
+  - |
+    #include <dt-bindings/reset/mediatek,mt8196-resets.h>
+    ufs-phy@16800000 {
+        compatible = "mediatek,mt8196-ufsphy", "mediatek,mt8183-ufsphy";
+        reg = <0x16800000 0x10000>;
+        clocks = <&ufs_ao_clk 3>,
+                 <&ufs_ao_clk 5>;
+        clock-names = "unipro", "mp";
+        resets = <&ufs_ao_clk MT8196_UFSAO_RST0_UFS_MPHY>;
+        #phy-cells = <0>;
+    };
 
 ...

-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 3/5] scsi: ufs: mediatek: Move MTK_SIP_UFS_CONTROL to mtk_sip_svc.h

[Nicolas Frattaroli]

SMC commands used by multiple drivers need to live in a shared header
file somewhere to avoid code duplication. In order to rework the MPHY
reset control to be in the phy-mtk-ufs.c driver, both ufs-mediatek and
the phy driver need access to this command.

Move it to mtk_sip_svc.h, where other such command definitions already
live.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/ufs/host/ufs-mediatek-sip.h      | 1 -
 include/linux/soc/mediatek/mtk_sip_svc.h | 3 +++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-mediatek-sip.h b/drivers/ufs/host/ufs-mediatek-sip.h
index 7d17aedf6fb8..d627dfb4a766 100644
--- a/drivers/ufs/host/ufs-mediatek-sip.h
+++ b/drivers/ufs/host/ufs-mediatek-sip.h
@@ -11,7 +11,6 @@
 /*
  * SiP (Slicon Partner) commands
  */
-#define MTK_SIP_UFS_CONTROL               MTK_SIP_SMC_CMD(0x276)
 #define UFS_MTK_SIP_VA09_PWR_CTRL         BIT(0)
 #define UFS_MTK_SIP_DEVICE_RESET          BIT(1)
 #define UFS_MTK_SIP_CRYPTO_CTRL           BIT(2)
diff --git a/include/linux/soc/mediatek/mtk_sip_svc.h b/include/linux/soc/mediatek/mtk_sip_svc.h
index abe24a73ee19..7265ff2a6e2a 100644
--- a/include/linux/soc/mediatek/mtk_sip_svc.h
+++ b/include/linux/soc/mediatek/mtk_sip_svc.h
@@ -22,6 +22,9 @@
 	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, MTK_SIP_SMC_CONVENTION, \
 			   ARM_SMCCC_OWNER_SIP, fn_id)
 
+/* UFS related SMC call */
+#define MTK_SIP_UFS_CONTROL		MTK_SIP_SMC_CMD(0x276)
+
 /* DVFSRC SMC calls */
 #define MTK_SIP_DVFSRC_VCOREFS_CONTROL	MTK_SIP_SMC_CMD(0x506)
 

-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 4/5] phy: mediatek: ufs: Add support for resets

[Nicolas Frattaroli]

The MediaTek UFS PHY supports PHY resets. Until now, they've been
implemented in the UFS host driver. Since they were never documented in
the UFS HCI node's DT bindings, and no mainline DT uses it, it's fine if
it's moved to the correct location, which is the PHY driver.

Implement the MPHY reset logic in this driver and expose it through the
phy subsystem's reset op. The reset itself is optional, as judging by
other mainline devices that use this hardware, it's not required for the
device to function.

If no reset is present, the reset op returns -EOPNOTSUPP, which means
that the ufshci driver can detect it's present and not double sleep in
its own reset function, where it will call the phy reset.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-ufs.c | 71 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/phy/mediatek/phy-mtk-ufs.c b/drivers/phy/mediatek/phy-mtk-ufs.c
index 0cb5a25b1b7a..d77ba689ebc8 100644
--- a/drivers/phy/mediatek/phy-mtk-ufs.c
+++ b/drivers/phy/mediatek/phy-mtk-ufs.c
@@ -4,6 +4,7 @@
  * Author: Stanley Chu <stanley.chu@mediatek.com>
  */
 
+#include <linux/arm-smccc.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/io.h>
@@ -11,6 +12,8 @@
 #include <linux/module.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/soc/mediatek/mtk_sip_svc.h>
 
 #include "phy-mtk-io.h"
 
@@ -36,9 +39,17 @@
 
 #define UFSPHY_CLKS_CNT    2
 
+#define UFS_MTK_SIP_MPHY_CTRL       BIT(8)
+
+enum ufs_mtk_mphy_op {
+	UFS_MPHY_BACKUP = 0,
+	UFS_MPHY_RESTORE
+};
+
 struct ufs_mtk_phy {
 	struct device *dev;
 	void __iomem *mmio;
+	struct reset_control *reset;
 	struct clk_bulk_data clks[UFSPHY_CLKS_CNT];
 };
 
@@ -141,9 +152,59 @@ static int ufs_mtk_phy_power_off(struct phy *generic_phy)
 	return 0;
 }
 
+static int ufs_mtk_phy_ctrl(struct ufs_mtk_phy *phy, enum ufs_mtk_mphy_op op)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(MTK_SIP_UFS_CONTROL, UFS_MTK_SIP_MPHY_CTRL, op,
+		      0, 0, 0, 0, 0, &res);
+
+	switch (res.a0) {
+	case SMCCC_RET_NOT_SUPPORTED:
+		return -EOPNOTSUPP;
+	case SMCCC_RET_INVALID_PARAMETER:
+		return -EINVAL;
+	default:
+		return 0;
+	}
+}
+
+static int ufs_mtk_phy_reset(struct phy *generic_phy)
+{
+	struct ufs_mtk_phy *phy = get_ufs_mtk_phy(generic_phy);
+	int ret;
+
+	if (!phy->reset)
+		return -EOPNOTSUPP;
+
+	ret = reset_control_assert(phy->reset);
+	if (ret)
+		return ret;
+
+	usleep_range(100, 110);
+
+	ret = reset_control_deassert(phy->reset);
+	if (ret)
+		return ret;
+
+	/*
+	 * To avoid double-sleep and other unintended side-effects in the ufshci
+	 * driver, don't return the phy_ctrl retval here, but just return -EPROTO.
+	 */
+	ret = ufs_mtk_phy_ctrl(phy, UFS_MPHY_RESTORE);
+	if (ret) {
+		dev_err(phy->dev, "UFS_MPHY_RESTORE SMC command failed: %pe\n",
+			ERR_PTR(ret));
+		return -EPROTO;
+	}
+
+	return 0;
+}
+
 static const struct phy_ops ufs_mtk_phy_ops = {
 	.power_on       = ufs_mtk_phy_power_on,
 	.power_off      = ufs_mtk_phy_power_off,
+	.reset          = ufs_mtk_phy_reset,
 	.owner          = THIS_MODULE,
 };
 
@@ -163,8 +224,18 @@ static int ufs_mtk_phy_probe(struct platform_device *pdev)
 	if (IS_ERR(phy->mmio))
 		return PTR_ERR(phy->mmio);
 
+	phy->reset = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(phy->reset))
+		return dev_err_probe(dev, PTR_ERR(phy->reset), "Failed to get reset\n");
+
 	phy->dev = dev;
 
+	if (phy->reset) {
+		ret = ufs_mtk_phy_ctrl(phy, UFS_MPHY_BACKUP);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to back up MPHY\n");
+	}
+
 	ret = ufs_mtk_phy_clk_init(phy);
 	if (ret)
 		return ret;

-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 5/5] scsi: ufs: mediatek: Rework resets

[Nicolas Frattaroli]

Rework the reset control getting in the driver's probe function to use
the bulk reset APIs. Use the optional variant instead of defaulting to
NULL if the resets fail, so that absent resets can be distinguished from
erroneous resets.

Also move all remnants of the MPHY reset ever having lived in this
driver.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/ufs/host/ufs-mediatek-sip.h |  8 ----
 drivers/ufs/host/ufs-mediatek.c     | 78 ++++++++++++++++++-------------------
 drivers/ufs/host/ufs-mediatek.h     |  7 ++--
 3 files changed, 42 insertions(+), 51 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek-sip.h b/drivers/ufs/host/ufs-mediatek-sip.h
index d627dfb4a766..256598cc3b5b 100644
--- a/drivers/ufs/host/ufs-mediatek-sip.h
+++ b/drivers/ufs/host/ufs-mediatek-sip.h
@@ -31,11 +31,6 @@ enum ufs_mtk_vcc_num {
 	UFS_VCC_MAX
 };
 
-enum ufs_mtk_mphy_op {
-	UFS_MPHY_BACKUP = 0,
-	UFS_MPHY_RESTORE
-};
-
 /*
  * SMC call wrapper function
  */
@@ -84,9 +79,6 @@ static inline void _ufs_mtk_smc(struct ufs_mtk_smc_arg s)
 #define ufs_mtk_device_pwr_ctrl(on, ufs_version, res) \
 	ufs_mtk_smc(UFS_MTK_SIP_DEVICE_PWR_CTRL, &(res), on, ufs_version)
 
-#define ufs_mtk_mphy_ctrl(op, res) \
-	ufs_mtk_smc(UFS_MTK_SIP_MPHY_CTRL, &(res), op)
-
 #define ufs_mtk_mtcmos_ctrl(op, res) \
 	ufs_mtk_smc(UFS_MTK_SIP_MTCMOS_CTRL, &(res), op)
 
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 758a393a9de1..5239ca4a428b 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -94,6 +94,12 @@ static const char *const ufs_uic_dl_err_str[] = {
 	"PA_INIT"
 };
 
+static const char *const ufs_reset_names[] = {
+	"unipro_rst",
+	"crypto_rst",
+	"hci_rst",
+};
+
 static bool ufs_mtk_is_boost_crypt_enabled(struct ufs_hba *hba)
 {
 	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
@@ -204,49 +210,45 @@ static void ufs_mtk_crypto_enable(struct ufs_hba *hba)
 static void ufs_mtk_host_reset(struct ufs_hba *hba)
 {
 	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
-	struct arm_smccc_res res;
-
-	reset_control_assert(host->hci_reset);
-	reset_control_assert(host->crypto_reset);
-	reset_control_assert(host->unipro_reset);
-	reset_control_assert(host->mphy_reset);
-
-	usleep_range(100, 110);
+	int ret;
 
-	reset_control_deassert(host->unipro_reset);
-	reset_control_deassert(host->crypto_reset);
-	reset_control_deassert(host->hci_reset);
-	reset_control_deassert(host->mphy_reset);
+	ret = reset_control_bulk_assert(MTK_UFS_NUM_RESETS, host->resets);
+	if (ret)
+		dev_warn(hba->dev, "Host reset assert failed: %pe\n", ERR_PTR(ret));
 
-	/* restore mphy setting aftre mphy reset */
-	if (host->mphy_reset)
-		ufs_mtk_mphy_ctrl(UFS_MPHY_RESTORE, res);
-}
+	ret = phy_reset(host->mphy);
 
-static void ufs_mtk_init_reset_control(struct ufs_hba *hba,
-				       struct reset_control **rc,
-				       char *str)
-{
-	*rc = devm_reset_control_get(hba->dev, str);
-	if (IS_ERR(*rc)) {
-		dev_info(hba->dev, "Failed to get reset control %s: %ld\n",
-			 str, PTR_ERR(*rc));
-		*rc = NULL;
+	/*
+	 * Only sleep if MPHY doesn't have a reset implemented (which already
+	 * sleeps) or the PHY reset function failed somehow, just to be safe
+	 */
+	if (ret) {
+		usleep_range(100, 110);
+		if (ret != -EOPNOTSUPP)
+			dev_warn(hba->dev, "PHY reset failed: %pe\n", ERR_PTR(ret));
 	}
+
+	ret = reset_control_bulk_deassert(MTK_UFS_NUM_RESETS, host->resets);
+	if (ret)
+		dev_warn(hba->dev, "Host reset deassert failed: %pe\n", ERR_PTR(ret));
 }
 
-static void ufs_mtk_init_reset(struct ufs_hba *hba)
+static int ufs_mtk_init_reset(struct ufs_hba *hba)
 {
 	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+	int ret, i;
+
+	for (i = 0; i < MTK_UFS_NUM_RESETS; i++)
+		host->resets[i].id = ufs_reset_names[i];
 
-	ufs_mtk_init_reset_control(hba, &host->hci_reset,
-				   "hci_rst");
-	ufs_mtk_init_reset_control(hba, &host->unipro_reset,
-				   "unipro_rst");
-	ufs_mtk_init_reset_control(hba, &host->crypto_reset,
-				   "crypto_rst");
-	ufs_mtk_init_reset_control(hba, &host->mphy_reset,
-				   "mphy_rst");
+	ret = devm_reset_control_bulk_get_optional_exclusive(hba->dev, MTK_UFS_NUM_RESETS,
+							     host->resets);
+	if (ret) {
+		dev_err(hba->dev, "Failed to get resets: %pe\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	return 0;
 }
 
 static int ufs_mtk_hce_enable_notify(struct ufs_hba *hba,
@@ -1238,11 +1240,9 @@ static int ufs_mtk_init(struct ufs_hba *hba)
 	if (err)
 		goto out_variant_clear;
 
-	ufs_mtk_init_reset(hba);
-
-	/* backup mphy setting if mphy can reset */
-	if (host->mphy_reset)
-		ufs_mtk_mphy_ctrl(UFS_MPHY_BACKUP, res);
+	err = ufs_mtk_init_reset(hba);
+	if (err)
+		goto out_variant_clear;
 
 	/* Enable runtime autosuspend */
 	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-mediatek.h
index dfbf78bd8664..c020fe04fe9e 100644
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -7,12 +7,14 @@
 #define _UFS_MEDIATEK_H
 
 #include <linux/bitops.h>
+#include <linux/reset.h>
 
 /*
  * MCQ define and struct
  */
 #define UFSHCD_MAX_Q_NR 8
 #define MTK_MCQ_INVALID_IRQ	0xFFFF
+#define MTK_UFS_NUM_RESETS 3
 
 /* REG_UFS_MMIO_OPT_CTRL_0 160h */
 #define EHS_EN                  BIT(0)
@@ -171,10 +173,7 @@ struct ufs_mtk_mcq_intr_info {
 struct ufs_mtk_host {
 	struct phy *mphy;
 	struct regulator *reg_va09;
-	struct reset_control *hci_reset;
-	struct reset_control *unipro_reset;
-	struct reset_control *crypto_reset;
-	struct reset_control *mphy_reset;
+	struct reset_control_bulk_data resets[MTK_UFS_NUM_RESETS];
 	struct ufs_hba *hba;
 	struct ufs_mtk_crypt_cfg *crypt;
 	struct ufs_mtk_clk mclk;

-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 5/5] scsi: ufs: mediatek: Rework resets

[Philipp Zabel]

On Do, 2025-10-16 at 14:06 +0200, Nicolas Frattaroli wrote:
> Rework the reset control getting in the driver's probe function to use
> the bulk reset APIs. Use the optional variant instead of defaulting to
> NULL if the resets fail, so that absent resets can be distinguished from
> erroneous resets.
> 
> Also move all remnants of the MPHY reset ever having lived in this
> driver.
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/5] dt-bindings: phy: Add mediatek,mt8196-ufsphy variant

[Peter Wang (王信友)]

On Thu, 2025-10-16 at 14:06 +0200, Nicolas Frattaroli wrote:
> The MediaTek MT8196 SoC includes an M-PHY compatible with the already
> existing mt8183 binding.
> 
> However, one omission from the original binding was that all of these
> variants may have an optional reset.
> 
> Add the new compatible, and also the resets property, with an
> example.
> 
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>
-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 3/5] scsi: ufs: mediatek: Move MTK_SIP_UFS_CONTROL to mtk_sip_svc.h

[Peter Wang (王信友)]

On Thu, 2025-10-16 at 14:06 +0200, Nicolas Frattaroli wrote:
> SMC commands used by multiple drivers need to live in a shared header
> file somewhere to avoid code duplication. In order to rework the MPHY
> reset control to be in the phy-mtk-ufs.c driver, both ufs-mediatek
> and
> the phy driver need access to this command.
> 
> Move it to mtk_sip_svc.h, where other such command definitions
> already
> live.
> 
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Peter Wang <peter.wang@mediatek.com>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 4/5] phy: mediatek: ufs: Add support for resets

[Peter Wang (王信友)]

On Thu, 2025-10-16 at 14:06 +0200, Nicolas Frattaroli wrote
> 
> The MediaTek UFS PHY supports PHY resets. Until now, they've been
> implemented in the UFS host driver. Since they were never documented
> in
> the UFS HCI node's DT bindings, and no mainline DT uses it, it's fine
> if
> it's moved to the correct location, which is the PHY driver.
> 
> Implement the MPHY reset logic in this driver and expose it through
> the
> phy subsystem's reset op. The reset itself is optional, as judging by
> other mainline devices that use this hardware, it's not required for
> the
> device to function.
> 
> If no reset is present, the reset op returns -EOPNOTSUPP, which means
> that the ufshci driver can detect it's present and not double sleep
> in
> its own reset function, where it will call the phy reset.
> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Peter Wang <peter.wang@mediatek.com>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 5/5] scsi: ufs: mediatek: Rework resets

[Peter Wang (王信友)]

On Thu, 2025-10-16 at 14:06 +0200, Nicolas Frattaroli wrote:
> Rework the reset control getting in the driver's probe function to
> use
> the bulk reset APIs. Use the optional variant instead of defaulting
> to
> NULL if the resets fail, so that absent resets can be distinguished
> from
> erroneous resets.
> 
> Also move all remnants of the MPHY reset ever having lived in this
> driver.
> 
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/5] dt-bindings: ufs: mediatek,ufs: Add mt8196-ufshci variant

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 873 bytes --]

On Thu, Oct 16, 2025 at 02:06:43PM +0200, Nicolas Frattaroli wrote:
> The MediaTek MT8196 SoC contains the same UFS host controller interface
> hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> its list of allowed clocks, as well as give it the previously absent
> resets property.
> 
> Also add examples for both MT8195 and the new MT8196, so that the
> binding can be verified against examples for these two variants.
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

I provided a review on v1 of this series yesterday, although I think
after this v2 was posted.
https://lore.kernel.org/all/20251016-kettle-clobber-2558d9c709de@spud/
I believe all of my comments still apply.

pw-bot: changes-requested

Thanks,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/5] dt-bindings: ufs: mediatek,ufs: Add mt8196-ufshci variant

[Nicolas Frattaroli]

On Friday, 17 October 2025 17:42:10 Central European Summer Time Conor Dooley wrote:
> On Thu, Oct 16, 2025 at 02:06:43PM +0200, Nicolas Frattaroli wrote:
> > The MediaTek MT8196 SoC contains the same UFS host controller interface
> > hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> > its list of allowed clocks, as well as give it the previously absent
> > resets property.
> > 
> > Also add examples for both MT8195 and the new MT8196, so that the
> > binding can be verified against examples for these two variants.
> > 
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 
> I provided a review on v1 of this series yesterday, although I think
> after this v2 was posted.
> https://lore.kernel.org/all/20251016-kettle-clobber-2558d9c709de@spud/
> I believe all of my comments still apply.

Hi Conor,

thanks for your review, I'll respond to the comments of those here
to avoid needlessly bumping the v1 thread.

On Thursday, 16 October 2025 18:53:01 Central European Summer Time Conor Dooley wrote:
> Hey,
> 
> On Tue, Oct 14, 2025 at 05:10:05PM +0200, Nicolas Frattaroli wrote:
> > The MediaTek MT8196 SoC contains the same UFS host controller interface
> > hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> > its list of allowed clocks, as well as give it the previously absent
> > resets property.
> > 
> > Also add examples for both MT8195 and the new MT8196, so that the
> > binding can be verified against examples for these two variants.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  .../devicetree/bindings/ufs/mediatek,ufs.yaml      | 134 +++++++++++++++++++--
> >  1 file changed, 123 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > index 1dec54fb00f3..070ae0982591 100644
> > --- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > +++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > @@ -11,18 +11,30 @@ maintainers:
> >  
> >  properties:
> >    compatible:
> > -    enum:
> > -      - mediatek,mt8183-ufshci
> > -      - mediatek,mt8192-ufshci
> > -      - mediatek,mt8195-ufshci
> > +    oneOf:
> > +      - enum:
> > +          - mediatek,mt8183-ufshci
> > +          - mediatek,mt8195-ufshci
> > +      - items:
> > +          - enum:
> > +              - mediatek,mt8192-ufshci
> > +          - const: mediatek,mt8183-ufshci
> 
> It's hard to follow what's going on in this commit.
> Firstly, this seems to be some sort of unrelated change that isn't
> mentioned in the commit message.

Sorry about that. Basically, the binding is currently wildly
incomplete, and this was my attempt at making it at least partly
useful for mainline DTs. You'll note that currently, no complete
(i.e. not just SoC dtsi) device tree uses it, and if they did,
all would most definitely generate warnings if they used it in a way
that actually worked, or silently relied on incomplete descriptions
that just happened to work out in practice.

So I'm being a bit heavy-handed here at untangling things. The
compatible changes here are to stop pretending that the mt8195
can use the mt8183 as a fallback, as the binding itself would
not really agree with that AFAIU. The mt8183 sets the maximum
number of clocks to 1, whereas mt8195 sets the minimum to 8.

> > +      - items:
> > +          - enum:
> > +              - mediatek,mt8196-ufshci
> > +          - const: mediatek,mt8195-ufshci
> >  
> >    clocks:
> >      minItems: 1
> > -    maxItems: 8
> > +    maxItems: 16
> >  
> >    clock-names:
> >      minItems: 1
> > -    maxItems: 8
> > +    maxItems: 16
> 
> Then all devices grow 8 more permitted clocks, despite the wording in
> the commit message being 8195 specific. (Hint: you missed maxItems: 8 in
> the else)

Right, thanks, I'll add maxItems to the clock-names property in the else.
Though that's already missing.

> > +
> > +  freq-table-hz: true
> 
> Then you add this deprecated property, which isn't mentioned in the
> commit message and I don't see why a deprecated property is needed.

I'll rework it to use operating-points-v2 instead. It needs one of
the two, or else on mt8196 at least, the hardware locks up.

I'll still add operating-points-v2 for all SoCs though, if that's
okay with you.

> > +
> > +  interrupts: true
> >  
> >    phys:
> >      maxItems: 1
> > @@ -30,7 +42,15 @@ properties:
> >    reg:
> >      maxItems: 1
> >  
> > +  resets:
> > +    maxItems: 3
> > +
> > +  reset-names:
> > +    maxItems: 3
> 
> You cannot use reset-names if you don't define what the names are.
> Please provide a items list with descriptions in resets and some
> names in reset-names.

Will do.

> 
> >    vcc-supply: true
> > +  vccq-supply: true
> > +  vccq2-supply: true
> 
> And then two new supplies that are not mentioned in the commit message,
> and again are allowed for all variants. The commit message talks about
> extended 8195 features, so this is starting to look like there was some
> sort of squashing accident.

No squashing accident, just me trying to get around having to justify
things I cannot justify. I've just checked the MT8195 and MT8196
datasheets for their pins, and see that MT8195 has a 1.8V and two 1.2V
supplies.

MT8196 on the other hand has seemingly no 1.8V UFS supply, but two
1.2V supplies and two 0.9V supplies.

I think MT8195 can use vcc-supply for 1.8V (with the vcc-supply-1p8
flag) and vccq-supply/vccq2-supply for 1.2V.

I suppose MT8196 then gets two 0.9v supply properties to play with in
its driver, I'm open to name suqqestions. Vendor uses va09-supply, but
that only covers one of the two possible supplies.

Interestingly, MT8183 has all of 1.8V, 1.2V and 0.9V supplies as
well. No duplicates though.

MT8192 has 1.8V, and two different 1.2V supplies for UFS.

It's also entirely possible that some other supply rail is used
as well for 1.8V operation on MT8196, but I'm not privy to this
kind of information.

So yeah, I'll fix the supply situation, maybe by splitting
this into a few separate commits.
 
> >  
> >    mediatek,ufs-disable-mcq:
> >      $ref: /schemas/types.yaml#/definitions/flag
> > @@ -44,22 +64,19 @@ required:
> >    - reg
> >    - vcc-supply
> >  
> > -unevaluatedProperties: false
> > -
> >  allOf:
> >    - $ref: ufs-common.yaml
> > -
> >    - if:
> >        properties:
> >          compatible:
> >            contains:
> > -            enum:
> > -              - mediatek,mt8195-ufshci
> > +            const: mediatek,mt8195-ufshci
> 
> The commit message says:
> | hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> | its list of allowed clocks, as well as give it the previously absent
> | resets property.
> 
> I don't know if that's meant to mean that only the new device has 16 and
> the 8195 only has 8, or if the 8195 should have had 16 possible clocks
> too.

It looks like MT8195 has crypt_mux, crypt_lp, crypt_perf and ufs_rx_symbol0
and ufs_rx_symbol1. I haven't found any of ufs_sel/ufs_sel_min_src/
ufs_sel_max_src analogues yet.

Should I in this case order those three last, and then set the minimum
number of clocks to 13? Should I not make mt8195 a fallback for mt8196
at all?

> >      then:
> >        properties:
> >          clocks:
> >            minItems: 8
> >          clock-names:
> > +          minItems: 8
> >            items:
> >              - const: ufs
> >              - const: ufs_aes
> > @@ -69,6 +86,19 @@ allOf:
> >              - const: unipro_mp_bclk
> >              - const: ufs_tx_symbol
> >              - const: ufs_mem_sub
> > +            - const: crypt_mux
> > +            - const: crypt_lp
> > +            - const: crypt_perf
> > +            - const: ufs_sel
> > +            - const: ufs_sel_min_src
> > +            - const: ufs_sel_max_src
> > +            - const: ufs_rx_symbol0
> > +            - const: ufs_rx_symbol1
> > +        reset-names:
> > +          items:
> > +            - const: unipro_rst
> > +            - const: crypto_rst
> > +            - const: hci_rst
> >      else:
> >        properties:
> >          clocks:
> > @@ -76,6 +106,10 @@ allOf:
> >          clock-names:
> >            items:
> >              - const: ufs
> > +        resets: false
> > +        reset-names: false
> > +
> > +unevaluatedProperties: false
> >  
> >  examples:
> >    - |
> > @@ -99,3 +133,81 @@ examples:
> >              vcc-supply = <&mt_pmic_vemc_ldo_reg>;
> >          };
> >      };
> > +  - |
> > +    ufshci@11270000 {
> > +        compatible = "mediatek,mt8195-ufshci";
> > +        reg = <0x11270000 0x2300>;
> > +        interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
> > +        phys = <&ufsphy>;
> > +        clocks = <&infracfg_ao 63>, <&infracfg_ao 64>, <&infracfg_ao 65>,
> > +                 <&infracfg_ao 54>, <&infracfg_ao 55>,
> > +                 <&infracfg_ao 56>, <&infracfg_ao 90>,
> > +                 <&infracfg_ao 93>;
> > +        clock-names = "ufs", "ufs_aes", "ufs_tick",
> > +                      "unipro_sysclk", "unipro_tick",
> > +                      "unipro_mp_bclk", "ufs_tx_symbol",
> > +                      "ufs_mem_sub";
> > +        freq-table-hz = <0 0>, <0 0>, <0 0>,
> > +                        <0 0>, <0 0>, <0 0>,
> > +                        <0 0>, <0 0>;
> > +        vcc-supply = <&mt6359_vemc_1_ldo_reg>;
> > +        mediatek,ufs-disable-mcq;
> > +    };
> > +  - |
> > +    #include <dt-bindings/reset/mediatek,mt8196-resets.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    ufshci@16810000 {
> > +        compatible = "mediatek,mt8196-ufshci", "mediatek,mt8195-ufshci";
> > +        reg = <0x16810000 0x2a00>;
> > +        interrupts = <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +        clocks = <&ufs_ao_clk 6>,
> > +                 <&ufs_ao_clk 7>,
> > +                 <&clk26m>,
> > +                 <&ufs_ao_clk 3>,
> > +                 <&clk26m>,
> > +                 <&ufs_ao_clk 4>,
> > +                 <&ufs_ao_clk 0>,
> > +                 <&topckgen 7>,
> > +                 <&topckgen 41>,
> > +                 <&topckgen 105>,
> > +                 <&topckgen 83>,
> > +                 <&topckgen 42>,
> > +                 <&topckgen 84>,
> > +                 <&topckgen 102>,
> > +                 <&ufs_ao_clk 1>,
> > +                 <&ufs_ao_clk 2>;
> > +        clock-names = "ufs",
> > +                      "ufs_aes",
> > +                      "ufs_tick",
> > +                      "unipro_sysclk",
> > +                      "unipro_tick",
> > +                      "unipro_mp_bclk",
> > +                      "ufs_tx_symbol",
> > +                      "ufs_mem_sub",
> > +                      "crypt_mux",
> > +                      "crypt_lp",
> > +                      "crypt_perf",
> > +                      "ufs_sel",
> > +                      "ufs_sel_min_src",
> > +                      "ufs_sel_max_src",
> > +                      "ufs_rx_symbol0",
> > +                      "ufs_rx_symbol1";
> > +
> > +        freq-table-hz = <273000000 499200000>, <0 0>, <0 0>, <0 0>, <0 0>,
> > +                        <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>,
> > +                        <0 0>;
> > +
> > +        phys = <&ufsphy>;
> > +
> > +        vcc-supply = <&mt6363_vemc>;
> > +        vccq-supply = <&mt6363_vufs12>;
> > +        vccq2-supply = <&mt6363_vufs18>;
> > +
> > +        resets = <&ufs_ao_clk MT8196_UFSAO_RST1_UFS_UNIPRO>,
> > +                 <&ufs_ao_clk MT8196_UFSAO_RST1_UFS_CRYPTO>,
> > +                 <&ufs_ao_clk MT8196_UFSAO_RST1_UFSHCI>;
> > +        reset-names = "unipro_rst", "crypto_rst", "hci_rst";
> 
> Putting _rst in the name of a reset is redundant.
> 
> pw-bot: changes-requested
> 
> Thanks,
> Conor.
> > +        mediatek,ufs-disable-mcq;
> > +    };
> > 
> 

Thanks for the review, I'll try to get a v3 out next week that
addresses these issues and also makes the required adjustments
to the drivers.

Kind regards,
Nicolas Frattaroli



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/5] dt-bindings: ufs: mediatek,ufs: Add mt8196-ufshci variant

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 17383 bytes --]

On Fri, Oct 17, 2025 at 09:02:07PM +0200, Nicolas Frattaroli wrote:
> On Friday, 17 October 2025 17:42:10 Central European Summer Time Conor Dooley wrote:
> > On Thu, Oct 16, 2025 at 02:06:43PM +0200, Nicolas Frattaroli wrote:
> > > The MediaTek MT8196 SoC contains the same UFS host controller interface
> > > hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> > > its list of allowed clocks, as well as give it the previously absent
> > > resets property.
> > > 
> > > Also add examples for both MT8195 and the new MT8196, so that the
> > > binding can be verified against examples for these two variants.
> > > 
> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > 
> > I provided a review on v1 of this series yesterday, although I think
> > after this v2 was posted.
> > https://lore.kernel.org/all/20251016-kettle-clobber-2558d9c709de@spud/
> > I believe all of my comments still apply.
> 
> thanks for your review, I'll respond to the comments of those here
> to avoid needlessly bumping the v1 thread.

Cool, good idea.

> On Thursday, 16 October 2025 18:53:01 Central European Summer Time Conor Dooley wrote:
> > Hey,
> > 
> > On Tue, Oct 14, 2025 at 05:10:05PM +0200, Nicolas Frattaroli wrote:
> > > The MediaTek MT8196 SoC contains the same UFS host controller interface
> > > hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> > > its list of allowed clocks, as well as give it the previously absent
> > > resets property.
> > > 
> > > Also add examples for both MT8195 and the new MT8196, so that the
> > > binding can be verified against examples for these two variants.
> > > 
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > >  .../devicetree/bindings/ufs/mediatek,ufs.yaml      | 134 +++++++++++++++++++--
> > >  1 file changed, 123 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > index 1dec54fb00f3..070ae0982591 100644
> > > --- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > +++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > @@ -11,18 +11,30 @@ maintainers:
> > >  
> > >  properties:
> > >    compatible:
> > > -    enum:
> > > -      - mediatek,mt8183-ufshci
> > > -      - mediatek,mt8192-ufshci
> > > -      - mediatek,mt8195-ufshci
> > > +    oneOf:
> > > +      - enum:
> > > +          - mediatek,mt8183-ufshci
> > > +          - mediatek,mt8195-ufshci
> > > +      - items:
> > > +          - enum:
> > > +              - mediatek,mt8192-ufshci
> > > +          - const: mediatek,mt8183-ufshci
> > 
> > It's hard to follow what's going on in this commit.
> > Firstly, this seems to be some sort of unrelated change that isn't
> > mentioned in the commit message.
> 
> Sorry about that. Basically, the binding is currently wildly
> incomplete, and this was my attempt at making it at least partly
> useful for mainline DTs. You'll note that currently, no complete
> (i.e. not just SoC dtsi) device tree uses it, and if they did,
> all would most definitely generate warnings if they used it in a way
> that actually worked, or silently relied on incomplete descriptions
> that just happened to work out in practice.
> 
> So I'm being a bit heavy-handed here at untangling things. The
> compatible changes here are to stop pretending that the mt8195
> can use the mt8183 as a fallback, as the binding itself would
> not really agree with that AFAIU. The mt8183 sets the maximum
> number of clocks to 1, whereas mt8195 sets the minimum to 8.

The binding doesn't allow the 8183 as a fallback for the 8195, so that
tracks ;)

Really the problem is that the commit subject says that this is adding
8196, but that's not what the body of the commit actually is. I know you
mention splitting further down the mail, but what I'd really like to see
done is one commit that corrects the property situation for the 8195,
providing whatever justifications you have for the changes - it's okay
if you don't necessarily have explanations for backed by stuff from docs
or whatever, if all you have is based on what platforms with the 8195 are
doing just mention that as why you need to have x-supply or whatever.
Then add one commit that adds the 8196, which is probably a fairly
minimal change once the 8195 is corrected.

> > > +      - items:
> > > +          - enum:
> > > +              - mediatek,mt8196-ufshci
> > > +          - const: mediatek,mt8195-ufshci
> > >  
> > >    clocks:
> > >      minItems: 1
> > > -    maxItems: 8
> > > +    maxItems: 16
> > >  
> > >    clock-names:
> > >      minItems: 1
> > > -    maxItems: 8
> > > +    maxItems: 16
> > 
> > Then all devices grow 8 more permitted clocks, despite the wording in
> > the commit message being 8195 specific. (Hint: you missed maxItems: 8 in
> > the else)
> 
> Right, thanks, I'll add maxItems to the clock-names property in the else.
> Though that's already missing.
> 
> > > +
> > > +  freq-table-hz: true
> > 
> > Then you add this deprecated property, which isn't mentioned in the
> > commit message and I don't see why a deprecated property is needed.
> 
> I'll rework it to use operating-points-v2 instead. It needs one of
> the two, or else on mt8196 at least, the hardware locks up.
> 
> I'll still add operating-points-v2 for all SoCs though, if that's
> okay with you.

Right. I'd accept freq-table-hz if the other devices here have been
using it all along, but if this is something new - then please use the
operating-points-v2 property. Looking at the binding example, it looks
like it does indeed use freq-table-hz, so that's probably justification
enough to keep doing so.

> > > +
> > > +  interrupts: true
> > >  
> > >    phys:
> > >      maxItems: 1
> > > @@ -30,7 +42,15 @@ properties:
> > >    reg:
> > >      maxItems: 1
> > >  
> > > +  resets:
> > > +    maxItems: 3
> > > +
> > > +  reset-names:
> > > +    maxItems: 3
> > 
> > You cannot use reset-names if you don't define what the names are.
> > Please provide a items list with descriptions in resets and some
> > names in reset-names.
> 
> Will do.
> 
> > 
> > >    vcc-supply: true
> > > +  vccq-supply: true
> > > +  vccq2-supply: true
> > 
> > And then two new supplies that are not mentioned in the commit message,
> > and again are allowed for all variants. The commit message talks about
> > extended 8195 features, so this is starting to look like there was some
> > sort of squashing accident.
> 
> No squashing accident, just me trying to get around having to justify
> things I cannot justify. I've just checked the MT8195 and MT8196
> datasheets for their pins, and see that MT8195 has a 1.8V and two 1.2V
> supplies.

As I said above, if you don't have some documented justification, just
cite usage or w/e, that's better than not mentioning it at all. If you
don't work for the vendor (and sometimes, sadly, when you do) it's not
possible to get complete info.

> MT8196 on the other hand has seemingly no 1.8V UFS supply, but two
> 1.2V supplies and two 0.9V supplies.
> 
> I think MT8195 can use vcc-supply for 1.8V (with the vcc-supply-1p8
> flag) and vccq-supply/vccq2-supply for 1.2V.
> 
> I suppose MT8196 then gets two 0.9v supply properties to play with in
> its driver, I'm open to name suqqestions. Vendor uses va09-supply, but
> that only covers one of the two possible supplies.
> 
> Interestingly, MT8183 has all of 1.8V, 1.2V and 0.9V supplies as
> well. No duplicates though.
> 
> MT8192 has 1.8V, and two different 1.2V supplies for UFS.
> 
> It's also entirely possible that some other supply rail is used
> as well for 1.8V operation on MT8196, but I'm not privy to this
> kind of information.
> 
> So yeah, I'll fix the supply situation, maybe by splitting
> this into a few separate commits.

My personal opinion is that the best way to do supplies is to match as
close to possible as the datasheet names for the supply. If that means
the supply names have to be different between devices, I think the
more complex binding is better than trying to get the names to somehow
fit for all devices.

> > >    mediatek,ufs-disable-mcq:
> > >      $ref: /schemas/types.yaml#/definitions/flag
> > > @@ -44,22 +64,19 @@ required:
> > >    - reg
> > >    - vcc-supply
> > >  
> > > -unevaluatedProperties: false
> > > -
> > >  allOf:
> > >    - $ref: ufs-common.yaml
> > > -
> > >    - if:
> > >        properties:
> > >          compatible:
> > >            contains:
> > > -            enum:
> > > -              - mediatek,mt8195-ufshci
> > > +            const: mediatek,mt8195-ufshci
> > 
> > The commit message says:
> > | hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> > | its list of allowed clocks, as well as give it the previously absent
> > | resets property.
> > 
> > I don't know if that's meant to mean that only the new device has 16 and
> > the 8195 only has 8, or if the 8195 should have had 16 possible clocks
> > too.
> 
> It looks like MT8195 has crypt_mux, crypt_lp, crypt_perf and ufs_rx_symbol0
> and ufs_rx_symbol1. I haven't found any of ufs_sel/ufs_sel_min_src/
> ufs_sel_max_src analogues yet.

I went and looked at the driver, and the list of clocks it looks up
don't even match what the binding permits for the 8195 (or any other
device):

	if (ufs_mtk_init_host_clk(hba, "crypt_mux",
				  &cfg->clk_crypt_mux))
		goto disable_caps;

	if (ufs_mtk_init_host_clk(hba, "crypt_lp",
				  &cfg->clk_crypt_lp))
		goto disable_caps;

	if (ufs_mtk_init_host_clk(hba, "crypt_perf",
				  &cfg->clk_crypt_perf))
		goto disable_caps;

+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt8195-ufshci
+    then:
+      properties:
+        clocks:
+          minItems: 8
+        clock-names:
+          items:
+            - const: ufs
+            - const: ufs_aes
+            - const: ufs_tick
+            - const: unipro_sysclk
+            - const: unipro_tick
+            - const: unipro_mp_bclk
+            - const: ufs_tx_symbol
+            - const: ufs_mem_sub

Looks like that series should never have actually been accepted,
particularly given some of the commentary on it. It's worth noting, that
there is no ack etc from a devicetree binding maintainer. Telling I
suppose.

You don't mention this mismatch in your commit message, but you should.

Also makes me wonder why the driver doesn't bother to enable all of
these clocks either, since the IP must need them for something, right?
Does the driver even work for 8195?

> Should I in this case order those three last, and then set the minimum
> number of clocks to 13? Should I not make mt8195 a fallback for mt8196
> at all?

For fallbacks, mostly think of it as "if the driver probed thinking that
this was a 8195, would it work correctly"? If you have to make changes
to a driver written for a 8195 to support clocks that are required on a
8196, then the devices are not compatible. If the 8196 will function if
the extra clocks are not enabled, then sure have a fallback. But if you
need to turn them on, then the devices are not compatible.
In other words, if the fallback doesn't implement a viable subset of
features on the new device, then it's not suitable.

As for the 13 v 16 etc, sure order them last. You'll have to be careful
with how you set up the conditional portion of the binding so that it
doesn't create impossible constraints, if you decide that a fallback is
suitable. Obviously, if there's no fallback, cos the extra 3 clocks are
mandatory, then it'll be much easier to set the conditions up.

> > >      then:
> > >        properties:
> > >          clocks:
> > >            minItems: 8
> > >          clock-names:
> > > +          minItems: 8
> > >            items:
> > >              - const: ufs
> > >              - const: ufs_aes
> > > @@ -69,6 +86,19 @@ allOf:
> > >              - const: unipro_mp_bclk
> > >              - const: ufs_tx_symbol
> > >              - const: ufs_mem_sub
> > > +            - const: crypt_mux
> > > +            - const: crypt_lp
> > > +            - const: crypt_perf
> > > +            - const: ufs_sel
> > > +            - const: ufs_sel_min_src
> > > +            - const: ufs_sel_max_src
> > > +            - const: ufs_rx_symbol0
> > > +            - const: ufs_rx_symbol1
> > > +        reset-names:
> > > +          items:
> > > +            - const: unipro_rst
> > > +            - const: crypto_rst
> > > +            - const: hci_rst
> > >      else:
> > >        properties:
> > >          clocks:
> > > @@ -76,6 +106,10 @@ allOf:
> > >          clock-names:
> > >            items:
> > >              - const: ufs
> > > +        resets: false
> > > +        reset-names: false
> > > +
> > > +unevaluatedProperties: false
> > >  
> > >  examples:
> > >    - |
> > > @@ -99,3 +133,81 @@ examples:
> > >              vcc-supply = <&mt_pmic_vemc_ldo_reg>;
> > >          };
> > >      };
> > > +  - |
> > > +    ufshci@11270000 {
> > > +        compatible = "mediatek,mt8195-ufshci";
> > > +        reg = <0x11270000 0x2300>;
> > > +        interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
> > > +        phys = <&ufsphy>;
> > > +        clocks = <&infracfg_ao 63>, <&infracfg_ao 64>, <&infracfg_ao 65>,
> > > +                 <&infracfg_ao 54>, <&infracfg_ao 55>,
> > > +                 <&infracfg_ao 56>, <&infracfg_ao 90>,
> > > +                 <&infracfg_ao 93>;
> > > +        clock-names = "ufs", "ufs_aes", "ufs_tick",
> > > +                      "unipro_sysclk", "unipro_tick",
> > > +                      "unipro_mp_bclk", "ufs_tx_symbol",
> > > +                      "ufs_mem_sub";
> > > +        freq-table-hz = <0 0>, <0 0>, <0 0>,
> > > +                        <0 0>, <0 0>, <0 0>,
> > > +                        <0 0>, <0 0>;
> > > +        vcc-supply = <&mt6359_vemc_1_ldo_reg>;
> > > +        mediatek,ufs-disable-mcq;
> > > +    };
> > > +  - |
> > > +    #include <dt-bindings/reset/mediatek,mt8196-resets.h>
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +
> > > +    ufshci@16810000 {
> > > +        compatible = "mediatek,mt8196-ufshci", "mediatek,mt8195-ufshci";
> > > +        reg = <0x16810000 0x2a00>;
> > > +        interrupts = <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>;
> > > +
> > > +        clocks = <&ufs_ao_clk 6>,
> > > +                 <&ufs_ao_clk 7>,
> > > +                 <&clk26m>,
> > > +                 <&ufs_ao_clk 3>,
> > > +                 <&clk26m>,
> > > +                 <&ufs_ao_clk 4>,
> > > +                 <&ufs_ao_clk 0>,
> > > +                 <&topckgen 7>,
> > > +                 <&topckgen 41>,
> > > +                 <&topckgen 105>,
> > > +                 <&topckgen 83>,
> > > +                 <&topckgen 42>,
> > > +                 <&topckgen 84>,
> > > +                 <&topckgen 102>,
> > > +                 <&ufs_ao_clk 1>,
> > > +                 <&ufs_ao_clk 2>;
> > > +        clock-names = "ufs",
> > > +                      "ufs_aes",
> > > +                      "ufs_tick",
> > > +                      "unipro_sysclk",
> > > +                      "unipro_tick",
> > > +                      "unipro_mp_bclk",
> > > +                      "ufs_tx_symbol",
> > > +                      "ufs_mem_sub",
> > > +                      "crypt_mux",
> > > +                      "crypt_lp",
> > > +                      "crypt_perf",
> > > +                      "ufs_sel",
> > > +                      "ufs_sel_min_src",
> > > +                      "ufs_sel_max_src",
> > > +                      "ufs_rx_symbol0",
> > > +                      "ufs_rx_symbol1";
> > > +
> > > +        freq-table-hz = <273000000 499200000>, <0 0>, <0 0>, <0 0>, <0 0>,
> > > +                        <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>,
> > > +                        <0 0>;
> > > +
> > > +        phys = <&ufsphy>;
> > > +
> > > +        vcc-supply = <&mt6363_vemc>;
> > > +        vccq-supply = <&mt6363_vufs12>;
> > > +        vccq2-supply = <&mt6363_vufs18>;
> > > +
> > > +        resets = <&ufs_ao_clk MT8196_UFSAO_RST1_UFS_UNIPRO>,
> > > +                 <&ufs_ao_clk MT8196_UFSAO_RST1_UFS_CRYPTO>,
> > > +                 <&ufs_ao_clk MT8196_UFSAO_RST1_UFSHCI>;
> > > +        reset-names = "unipro_rst", "crypto_rst", "hci_rst";
> > 
> > Putting _rst in the name of a reset is redundant.
> > 
> > pw-bot: changes-requested
> > 
> > Thanks,
> > Conor.
> > > +        mediatek,ufs-disable-mcq;
> > > +    };
> > > 
> > 
> 
> Thanks for the review, I'll try to get a v3 out next week that
> addresses these issues and also makes the required adjustments
> to the drivers.

Cool. TL;DR, just be clear about where things are coming from and please
try to sort out the 8195 mess in a different patch to the 8196.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/5] dt-bindings: ufs: mediatek,ufs: Add mt8196-ufshci variant

[Nicolas Frattaroli]

On Saturday, 18 October 2025 23:30:17 Central European Summer Time Conor Dooley wrote:
> On Fri, Oct 17, 2025 at 09:02:07PM +0200, Nicolas Frattaroli wrote:
> > On Friday, 17 October 2025 17:42:10 Central European Summer Time Conor Dooley wrote:
> > > On Thu, Oct 16, 2025 at 02:06:43PM +0200, Nicolas Frattaroli wrote:
> > > > The MediaTek MT8196 SoC contains the same UFS host controller interface
> > > > hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> > > > its list of allowed clocks, as well as give it the previously absent
> > > > resets property.
> > > > 
> > > > Also add examples for both MT8195 and the new MT8196, so that the
> > > > binding can be verified against examples for these two variants.
> > > > 
> > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > 
> > > I provided a review on v1 of this series yesterday, although I think
> > > after this v2 was posted.
> > > https://lore.kernel.org/all/20251016-kettle-clobber-2558d9c709de@spud/
> > > I believe all of my comments still apply.
> > 
> > thanks for your review, I'll respond to the comments of those here
> > to avoid needlessly bumping the v1 thread.
> 
> Cool, good idea.
> 
> > On Thursday, 16 October 2025 18:53:01 Central European Summer Time Conor Dooley wrote:
> > > Hey,
> > > 
> > > On Tue, Oct 14, 2025 at 05:10:05PM +0200, Nicolas Frattaroli wrote:
> > > > The MediaTek MT8196 SoC contains the same UFS host controller interface
> > > > hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> > > > its list of allowed clocks, as well as give it the previously absent
> > > > resets property.
> > > > 
> > > > Also add examples for both MT8195 and the new MT8196, so that the
> > > > binding can be verified against examples for these two variants.
> > > > 
> > > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > > ---
> > > >  .../devicetree/bindings/ufs/mediatek,ufs.yaml      | 134 +++++++++++++++++++--
> > > >  1 file changed, 123 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > > index 1dec54fb00f3..070ae0982591 100644
> > > > --- a/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > > +++ b/Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
> > > > @@ -11,18 +11,30 @@ maintainers:
> > > >  
> > > >  properties:
> > > >    compatible:
> > > > -    enum:
> > > > -      - mediatek,mt8183-ufshci
> > > > -      - mediatek,mt8192-ufshci
> > > > -      - mediatek,mt8195-ufshci
> > > > +    oneOf:
> > > > +      - enum:
> > > > +          - mediatek,mt8183-ufshci
> > > > +          - mediatek,mt8195-ufshci
> > > > +      - items:
> > > > +          - enum:
> > > > +              - mediatek,mt8192-ufshci
> > > > +          - const: mediatek,mt8183-ufshci
> > > 
> > > It's hard to follow what's going on in this commit.
> > > Firstly, this seems to be some sort of unrelated change that isn't
> > > mentioned in the commit message.
> > 
> > Sorry about that. Basically, the binding is currently wildly
> > incomplete, and this was my attempt at making it at least partly
> > useful for mainline DTs. You'll note that currently, no complete
> > (i.e. not just SoC dtsi) device tree uses it, and if they did,
> > all would most definitely generate warnings if they used it in a way
> > that actually worked, or silently relied on incomplete descriptions
> > that just happened to work out in practice.
> > 
> > So I'm being a bit heavy-handed here at untangling things. The
> > compatible changes here are to stop pretending that the mt8195
> > can use the mt8183 as a fallback, as the binding itself would
> > not really agree with that AFAIU. The mt8183 sets the maximum
> > number of clocks to 1, whereas mt8195 sets the minimum to 8.
> 
> The binding doesn't allow the 8183 as a fallback for the 8195, so that
> tracks ;)
> 
> Really the problem is that the commit subject says that this is adding
> 8196, but that's not what the body of the commit actually is. I know you
> mention splitting further down the mail, but what I'd really like to see
> done is one commit that corrects the property situation for the 8195,
> providing whatever justifications you have for the changes - it's okay
> if you don't necessarily have explanations for backed by stuff from docs
> or whatever, if all you have is based on what platforms with the 8195 are
> doing just mention that as why you need to have x-supply or whatever.
> Then add one commit that adds the 8196, which is probably a fairly
> minimal change once the 8195 is corrected.

Alright, I think part of the confusion is me rebasing across a
midstream branch that has some unrelated bandaid fixes in it
which have never been submitted or were last submitted in 2024.
I'll simply drop those from my boot-testing branch and do my own
thing as I see fit, because things that were never submitted
upstream aren't real and can't hurt me.

> 
> > > > +      - items:
> > > > +          - enum:
> > > > +              - mediatek,mt8196-ufshci
> > > > +          - const: mediatek,mt8195-ufshci
> > > >  
> > > >    clocks:
> > > >      minItems: 1
> > > > -    maxItems: 8
> > > > +    maxItems: 16
> > > >  
> > > >    clock-names:
> > > >      minItems: 1
> > > > -    maxItems: 8
> > > > +    maxItems: 16
> > > 
> > > Then all devices grow 8 more permitted clocks, despite the wording in
> > > the commit message being 8195 specific. (Hint: you missed maxItems: 8 in
> > > the else)
> > 
> > Right, thanks, I'll add maxItems to the clock-names property in the else.
> > Though that's already missing.
> > 
> > > > +
> > > > +  freq-table-hz: true
> > > 
> > > Then you add this deprecated property, which isn't mentioned in the
> > > commit message and I don't see why a deprecated property is needed.
> > 
> > I'll rework it to use operating-points-v2 instead. It needs one of
> > the two, or else on mt8196 at least, the hardware locks up.
> > 
> > I'll still add operating-points-v2 for all SoCs though, if that's
> > okay with you.
> 
> Right. I'd accept freq-table-hz if the other devices here have been
> using it all along, but if this is something new - then please use the
> operating-points-v2 property. Looking at the binding example, it looks
> like it does indeed use freq-table-hz, so that's probably justification
> enough to keep doing so.

Turns out the only usage of freq-table-hz is in the examples I've added.
Mainline does not at all have any nodes in the DT right now that would
use this property.

Ergo, I think I will go for operating-points-v2. We might as well clean
this up now instead of ossifying a deprecated property in a new binding
for the sake of downstream compatibility (which should never be a concern)
that I am already breaking. The added benefit is that if we ever do get
better OPP definitions than just two clock states, then we can actually
add the OPP bandwidth properties so implementations can make informed
decisions.

> > > > +
> > > > +  interrupts: true
> > > >  
> > > >    phys:
> > > >      maxItems: 1
> > > > @@ -30,7 +42,15 @@ properties:
> > > >    reg:
> > > >      maxItems: 1
> > > >  
> > > > +  resets:
> > > > +    maxItems: 3
> > > > +
> > > > +  reset-names:
> > > > +    maxItems: 3
> > > 
> > > You cannot use reset-names if you don't define what the names are.
> > > Please provide a items list with descriptions in resets and some
> > > names in reset-names.
> > 
> > Will do.
> > 
> > > 
> > > >    vcc-supply: true
> > > > +  vccq-supply: true
> > > > +  vccq2-supply: true
> > > 
> > > And then two new supplies that are not mentioned in the commit message,
> > > and again are allowed for all variants. The commit message talks about
> > > extended 8195 features, so this is starting to look like there was some
> > > sort of squashing accident.
> > 
> > No squashing accident, just me trying to get around having to justify
> > things I cannot justify. I've just checked the MT8195 and MT8196
> > datasheets for their pins, and see that MT8195 has a 1.8V and two 1.2V
> > supplies.
> 
> As I said above, if you don't have some documented justification, just
> cite usage or w/e, that's better than not mentioning it at all. If you
> don't work for the vendor (and sometimes, sadly, when you do) it's not
> possible to get complete info.
> 
> > MT8196 on the other hand has seemingly no 1.8V UFS supply, but two
> > 1.2V supplies and two 0.9V supplies.
> > 
> > I think MT8195 can use vcc-supply for 1.8V (with the vcc-supply-1p8
> > flag) and vccq-supply/vccq2-supply for 1.2V.
> > 
> > I suppose MT8196 then gets two 0.9v supply properties to play with in
> > its driver, I'm open to name suqqestions. Vendor uses va09-supply, but
> > that only covers one of the two possible supplies.
> > 
> > Interestingly, MT8183 has all of 1.8V, 1.2V and 0.9V supplies as
> > well. No duplicates though.
> > 
> > MT8192 has 1.8V, and two different 1.2V supplies for UFS.
> > 
> > It's also entirely possible that some other supply rail is used
> > as well for 1.8V operation on MT8196, but I'm not privy to this
> > kind of information.
> > 
> > So yeah, I'll fix the supply situation, maybe by splitting
> > this into a few separate commits.
> 
> My personal opinion is that the best way to do supplies is to match as
> close to possible as the datasheet names for the supply. If that means
> the supply names have to be different between devices, I think the
> more complex binding is better than trying to get the names to somehow
> fit for all devices.

Alright. I think I agree with this. I've tried to use the ufs common
supply names because I thought that was the way to go, but using the
supply names that are in the datasheets seems much more clear and
reasonable to me, as we're no longer introducing different meanings
depending on the used compatible.

> 
> > > >    mediatek,ufs-disable-mcq:
> > > >      $ref: /schemas/types.yaml#/definitions/flag
> > > > @@ -44,22 +64,19 @@ required:
> > > >    - reg
> > > >    - vcc-supply
> > > >  
> > > > -unevaluatedProperties: false
> > > > -
> > > >  allOf:
> > > >    - $ref: ufs-common.yaml
> > > > -
> > > >    - if:
> > > >        properties:
> > > >          compatible:
> > > >            contains:
> > > > -            enum:
> > > > -              - mediatek,mt8195-ufshci
> > > > +            const: mediatek,mt8195-ufshci
> > > 
> > > The commit message says:
> > > | hardware as the MT8195 SoC. Add it as a variant of MT8195, and extend
> > > | its list of allowed clocks, as well as give it the previously absent
> > > | resets property.
> > > 
> > > I don't know if that's meant to mean that only the new device has 16 and
> > > the 8195 only has 8, or if the 8195 should have had 16 possible clocks
> > > too.
> > 
> > It looks like MT8195 has crypt_mux, crypt_lp, crypt_perf and ufs_rx_symbol0
> > and ufs_rx_symbol1. I haven't found any of ufs_sel/ufs_sel_min_src/
> > ufs_sel_max_src analogues yet.
> 
> I went and looked at the driver, and the list of clocks it looks up
> don't even match what the binding permits for the 8195 (or any other
> device):
> 
> 	if (ufs_mtk_init_host_clk(hba, "crypt_mux",
> 				  &cfg->clk_crypt_mux))
> 		goto disable_caps;
> 
> 	if (ufs_mtk_init_host_clk(hba, "crypt_lp",
> 				  &cfg->clk_crypt_lp))
> 		goto disable_caps;
> 
> 	if (ufs_mtk_init_host_clk(hba, "crypt_perf",
> 				  &cfg->clk_crypt_perf))
> 		goto disable_caps;
> 
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - mediatek,mt8195-ufshci
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 8
> +        clock-names:
> +          items:
> +            - const: ufs
> +            - const: ufs_aes
> +            - const: ufs_tick
> +            - const: unipro_sysclk
> +            - const: unipro_tick
> +            - const: unipro_mp_bclk
> +            - const: ufs_tx_symbol
> +            - const: ufs_mem_sub
> 
> Looks like that series should never have actually been accepted,
> particularly given some of the commentary on it. It's worth noting, that
> there is no ack etc from a devicetree binding maintainer. Telling I
> suppose.

Yeah :(

> 
> You don't mention this mismatch in your commit message, but you should.
> 
> Also makes me wonder why the driver doesn't bother to enable all of
> these clocks either, since the IP must need them for something, right?
> Does the driver even work for 8195?

It's possible that it's relying on clock states from some magic MCU
that fiddles with things. It should definitely be fixed if that's the
case.

> 
> > Should I in this case order those three last, and then set the minimum
> > number of clocks to 13? Should I not make mt8195 a fallback for mt8196
> > at all?
> 
> For fallbacks, mostly think of it as "if the driver probed thinking that
> this was a 8195, would it work correctly"? If you have to make changes
> to a driver written for a 8195 to support clocks that are required on a
> 8196, then the devices are not compatible. If the 8196 will function if
> the extra clocks are not enabled, then sure have a fallback. But if you
> need to turn them on, then the devices are not compatible.
> In other words, if the fallback doesn't implement a viable subset of
> features on the new device, then it's not suitable.

I've just tried MT8196 with only the 13 clocks MT8195 will have once
I get to work, and it seems to probe fine an read from UFS fine.
Doesn't necessarily have to mean anything because again, a magic
MCU may be fiddling with the clock gates.

However, since the supplies of MT8195 won't be a subset of the
supplies of MT8196, I won't make it a fallback relationship.
E.g. an MT8195 device could be allowed to just have the 1.8V
supply and an MT8196 device could be allowed to just have the
1.2V/0.9V supplies, which means an implementation written against
the MT8195 binding would not work for the MT8196.

> 
> As for the 13 v 16 etc, sure order them last. You'll have to be careful
> with how you set up the conditional portion of the binding so that it
> doesn't create impossible constraints, if you decide that a fallback is
> suitable. Obviously, if there's no fallback, cos the extra 3 clocks are
> mandatory, then it'll be much easier to set the conditions up.
> 
> > > >      then:
> > > >        properties:
> > > >          clocks:
> > > >            minItems: 8
> > > >          clock-names:
> > > > +          minItems: 8
> > > >            items:
> > > >              - const: ufs
> > > >              - const: ufs_aes
> > > > @@ -69,6 +86,19 @@ allOf:
> > > >              - const: unipro_mp_bclk
> > > >              - const: ufs_tx_symbol
> > > >              - const: ufs_mem_sub
> > > > +            - const: crypt_mux
> > > > +            - const: crypt_lp
> > > > +            - const: crypt_perf
> > > > +            - const: ufs_sel
> > > > +            - const: ufs_sel_min_src
> > > > +            - const: ufs_sel_max_src
> > > > +            - const: ufs_rx_symbol0
> > > > +            - const: ufs_rx_symbol1
> > > > +        reset-names:
> > > > +          items:
> > > > +            - const: unipro_rst
> > > > +            - const: crypto_rst
> > > > +            - const: hci_rst
> > > >      else:
> > > >        properties:
> > > >          clocks:
> > > > @@ -76,6 +106,10 @@ allOf:
> > > >          clock-names:
> > > >            items:
> > > >              - const: ufs
> > > > +        resets: false
> > > > +        reset-names: false
> > > > +
> > > > +unevaluatedProperties: false
> > > >  
> > > >  examples:
> > > >    - |
> > > > @@ -99,3 +133,81 @@ examples:
> > > >              vcc-supply = <&mt_pmic_vemc_ldo_reg>;
> > > >          };
> > > >      };
> > > > +  - |
> > > > +    ufshci@11270000 {
> > > > +        compatible = "mediatek,mt8195-ufshci";
> > > > +        reg = <0x11270000 0x2300>;
> > > > +        interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
> > > > +        phys = <&ufsphy>;
> > > > +        clocks = <&infracfg_ao 63>, <&infracfg_ao 64>, <&infracfg_ao 65>,
> > > > +                 <&infracfg_ao 54>, <&infracfg_ao 55>,
> > > > +                 <&infracfg_ao 56>, <&infracfg_ao 90>,
> > > > +                 <&infracfg_ao 93>;
> > > > +        clock-names = "ufs", "ufs_aes", "ufs_tick",
> > > > +                      "unipro_sysclk", "unipro_tick",
> > > > +                      "unipro_mp_bclk", "ufs_tx_symbol",
> > > > +                      "ufs_mem_sub";
> > > > +        freq-table-hz = <0 0>, <0 0>, <0 0>,
> > > > +                        <0 0>, <0 0>, <0 0>,
> > > > +                        <0 0>, <0 0>;
> > > > +        vcc-supply = <&mt6359_vemc_1_ldo_reg>;
> > > > +        mediatek,ufs-disable-mcq;
> > > > +    };
> > > > +  - |
> > > > +    #include <dt-bindings/reset/mediatek,mt8196-resets.h>
> > > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > +
> > > > +    ufshci@16810000 {
> > > > +        compatible = "mediatek,mt8196-ufshci", "mediatek,mt8195-ufshci";
> > > > +        reg = <0x16810000 0x2a00>;
> > > > +        interrupts = <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>;
> > > > +
> > > > +        clocks = <&ufs_ao_clk 6>,
> > > > +                 <&ufs_ao_clk 7>,
> > > > +                 <&clk26m>,
> > > > +                 <&ufs_ao_clk 3>,
> > > > +                 <&clk26m>,
> > > > +                 <&ufs_ao_clk 4>,
> > > > +                 <&ufs_ao_clk 0>,
> > > > +                 <&topckgen 7>,
> > > > +                 <&topckgen 41>,
> > > > +                 <&topckgen 105>,
> > > > +                 <&topckgen 83>,
> > > > +                 <&topckgen 42>,
> > > > +                 <&topckgen 84>,
> > > > +                 <&topckgen 102>,
> > > > +                 <&ufs_ao_clk 1>,
> > > > +                 <&ufs_ao_clk 2>;
> > > > +        clock-names = "ufs",
> > > > +                      "ufs_aes",
> > > > +                      "ufs_tick",
> > > > +                      "unipro_sysclk",
> > > > +                      "unipro_tick",
> > > > +                      "unipro_mp_bclk",
> > > > +                      "ufs_tx_symbol",
> > > > +                      "ufs_mem_sub",
> > > > +                      "crypt_mux",
> > > > +                      "crypt_lp",
> > > > +                      "crypt_perf",
> > > > +                      "ufs_sel",
> > > > +                      "ufs_sel_min_src",
> > > > +                      "ufs_sel_max_src",
> > > > +                      "ufs_rx_symbol0",
> > > > +                      "ufs_rx_symbol1";
> > > > +
> > > > +        freq-table-hz = <273000000 499200000>, <0 0>, <0 0>, <0 0>, <0 0>,
> > > > +                        <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>, <0 0>,
> > > > +                        <0 0>;
> > > > +
> > > > +        phys = <&ufsphy>;
> > > > +
> > > > +        vcc-supply = <&mt6363_vemc>;
> > > > +        vccq-supply = <&mt6363_vufs12>;
> > > > +        vccq2-supply = <&mt6363_vufs18>;
> > > > +
> > > > +        resets = <&ufs_ao_clk MT8196_UFSAO_RST1_UFS_UNIPRO>,
> > > > +                 <&ufs_ao_clk MT8196_UFSAO_RST1_UFS_CRYPTO>,
> > > > +                 <&ufs_ao_clk MT8196_UFSAO_RST1_UFSHCI>;
> > > > +        reset-names = "unipro_rst", "crypto_rst", "hci_rst";
> > > 
> > > Putting _rst in the name of a reset is redundant.
> > > 
> > > pw-bot: changes-requested
> > > 
> > > Thanks,
> > > Conor.
> > > > +        mediatek,ufs-disable-mcq;
> > > > +    };
> > > > 
> > > 
> > 
> > Thanks for the review, I'll try to get a v3 out next week that
> > addresses these issues and also makes the required adjustments
> > to the drivers.
> 
> Cool. TL;DR, just be clear about where things are coming from and please
> try to sort out the 8195 mess in a different patch to the 8196.
> 
> Cheers,
> Conor.
> 

Thank you for helping to clear this whole mess up. Turns out this
was way more work than initially expected, but I reckon if I come
in like a wrecking ball right now then that will make the job easier
for everyone contributing to mainline in the future.

Kind regards,
Nicolas Frattaroli





-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/5] dt-bindings: ufs: mediatek,ufs: Add mt8196-ufshci variant

[Nicolas Frattaroli]

On Monday, 20 October 2025 15:27:40 Central European Summer Time Nicolas Frattaroli wrote:
> On Saturday, 18 October 2025 23:30:17 Central European Summer Time Conor Dooley wrote:
> > On Fri, Oct 17, 2025 at 09:02:07PM +0200, Nicolas Frattaroli wrote:
> > > On Friday, 17 October 2025 17:42:10 Central European Summer Time Conor Dooley wrote:
> > > > On Thu, Oct 16, 2025 at 02:06:43PM +0200, Nicolas Frattaroli wrote:
> > > 
> > > > > +
> > > > > +  freq-table-hz: true
> > > > 
> > > > Then you add this deprecated property, which isn't mentioned in the
> > > > commit message and I don't see why a deprecated property is needed.
> > > 
> > > I'll rework it to use operating-points-v2 instead. It needs one of
> > > the two, or else on mt8196 at least, the hardware locks up.
> > > 
> > > I'll still add operating-points-v2 for all SoCs though, if that's
> > > okay with you.
> > 
> > Right. I'd accept freq-table-hz if the other devices here have been
> > using it all along, but if this is something new - then please use the
> > operating-points-v2 property. Looking at the binding example, it looks
> > like it does indeed use freq-table-hz, so that's probably justification
> > enough to keep doing so.
> 
> Turns out the only usage of freq-table-hz is in the examples I've added.
> Mainline does not at all have any nodes in the DT right now that would
> use this property.
> 
> Ergo, I think I will go for operating-points-v2. We might as well clean
> this up now instead of ossifying a deprecated property in a new binding
> for the sake of downstream compatibility (which should never be a concern)
> that I am already breaking. The added benefit is that if we ever do get
> better OPP definitions than just two clock states, then we can actually
> add the OPP bandwidth properties so implementations can make informed
> decisions.
> 

Nevermind, I see the existing binding had it in the example for 8183,
just not in the binding itself. So freq-table-hz it is.



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy