[PATCH 0/4] Add SMI clamp and reset

[PATCH 0/4] Add SMI clamp and reset

[friday.yang]

Based on tag: next-20240821, linux-next/master

Refer to the discussion in the following link:
https://lore.kernel.org/all/CAFGrd9qZhObQXvm2_abqaX83xMLqxjQETB2=wXpobDWU1CnvkA@mail.gmail.com/
https://lore.kernel.org/all/CAPDyKFpokXV2gJDgowbixTvOH_5VL3B5H8eyhP+KJ5Fasm2rFg@mail.gmail.com/
SMI clamp and reset operations should be implemented in SMI driver
instead of PM driver.

When we enable/disable power domain, the SMI LARBs linked to this power
domain could be affected by the bus glitch. To avoid this issue, SMI
need to apply clamp and reset opereations.

These patches mainly add these functions:
1) Register genpd callback for SMI LARBs and handle this power domain
   status change into SMI driver.
2) Add SMI reset control driver to implement SMI reset opereations.
3) Add bindings for describing the reset control regmap and SMI Sub Common.

friday.yang (4):
  dt-bindings: memory: mediatek: Add mt8188 SMI reset control binding
  dt-bindings: memory: mediatek: Add smi-sub-common property for reset
  memory: mtk-smi: mt8188: Add SMI clamp function
  reset: mediatek: Add reset control driver for SMI

 .../mediatek,smi-common.yaml                  |   2 +
 .../memory-controllers/mediatek,smi-larb.yaml |  22 +++
 .../bindings/reset/mediatek,smi-reset.yaml    |  46 ++++++
 drivers/memory/mtk-smi.c                      | 148 ++++++++++++++++-
 drivers/reset/Kconfig                         |   9 ++
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-mediatek-smi.c            | 152 ++++++++++++++++++
 include/dt-bindings/reset/mt8188-resets.h     |  11 ++
 8 files changed, 389 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml
 create mode 100644 drivers/reset/reset-mediatek-smi.c

--
2.46.0

[PATCH 1/4] dt-bindings: memory: mediatek: Add mt8188 SMI reset control binding

[friday.yang]

To support SMI clamp and reset operation in genpd callback, add
SMI LARB reset register offset and mask related information in
the bindings. Add index in mt8188-resets.h to query the register
offset and mask in the SMI reset control driver.

Signed-off-by: friday.yang <friday.yang@mediatek.com>
---
 .../bindings/reset/mediatek,smi-reset.yaml    | 46 +++++++++++++++++++
 include/dt-bindings/reset/mt8188-resets.h     | 11 +++++
 2 files changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml

diff --git a/Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml b/Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml
new file mode 100644
index 000000000000..66ac121d2396
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2024 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/mediatek,smi-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek SMI Reset Controller
+
+maintainers:
+  - Friday Yang <friday.yang@mediatek.com>
+
+description: |
+  This reset controller node is used to perform reset management
+  of SMI larbs on MediaTek platform. It is used to implement various
+  reset functions required when SMI larbs apply clamp operation.
+
+  For list of all valid reset indices see
+    <dt-bindings/reset/mt8188-resets.h> for MT8188.
+
+properties:
+  compatible:
+    enum:
+      - mediatek,smi-reset-mt8188
+
+  "#reset-cells":
+    const: 1
+
+  mediatek,larb-rst-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the SMI larb's reset controller syscon.
+
+required:
+  - compatible
+  - "#reset-cells"
+  - mediatek,larb-rst-syscon
+
+additionalProperties: false
+
+examples:
+  - |
+    imgsys1_dip_top_rst: reset-controller {
+          compatible = "mediatek,smi-reset-mt8188";
+          #reset-cells = <1>;
+          mediatek,larb-rst-syscon = <&imgsys1_dip_top>;
+    };
diff --git a/include/dt-bindings/reset/mt8188-resets.h b/include/dt-bindings/reset/mt8188-resets.h
index 5a58c54e7d20..387a4beac688 100644
--- a/include/dt-bindings/reset/mt8188-resets.h
+++ b/include/dt-bindings/reset/mt8188-resets.h
@@ -113,4 +113,15 @@
 #define MT8188_VDO1_RST_HDR_GFX_FE1_DL_ASYNC	52
 #define MT8188_VDO1_RST_HDR_VDO_BE_DL_ASYNC	53
 
+#define MT8188_SMI_RST_LARB10			0
+#define MT8188_SMI_RST_LARB11A			1
+#define MT8188_SMI_RST_LARB11C			2
+#define MT8188_SMI_RST_LARB12			3
+#define MT8188_SMI_RST_LARB11B			4
+#define MT8188_SMI_RST_LARB15			5
+#define MT8188_SMI_RST_LARB16B			6
+#define MT8188_SMI_RST_LARB17B			7
+#define MT8188_SMI_RST_LARB16A			8
+#define MT8188_SMI_RST_LARB17A			9
+
 #endif  /* _DT_BINDINGS_RESET_CONTROLLER_MT8188 */
-- 
2.46.0

[PATCH 2/4] dt-bindings: memory: mediatek: Add smi-sub-common property for reset

[friday.yang]

On the MediaTek platform, some SMI LARBs are directly linked to SMI
common. While some SMI LARBs are linked to SMI sub common, then SMI
sub common is linked to SMI common. Add 'mediatek,smi-sub-comm' and
'mediatek,smi-sub-comm-in-portid' properties here. The SMI reset
driver could query which port of the SMI sub common the current LARB
is linked to through the two properties. The hardware block diagram
could be described as below.

             SMI Common(Smart Multimedia Interface Common)
                 |
         +----------------+-------
         |                |
         |                |
         |                |
         |                |
         |                |
       larb0       SMI Sub Common
                   |      |     |
                  larb1  larb2 larb3

Signed-off-by: friday.yang <friday.yang@mediatek.com>
---
 .../mediatek,smi-common.yaml                  |  2 ++
 .../memory-controllers/mediatek,smi-larb.yaml | 22 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
index 2f36ac23604c..4392d349878c 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
@@ -39,6 +39,7 @@ properties:
           - mediatek,mt8186-smi-common
           - mediatek,mt8188-smi-common-vdo
           - mediatek,mt8188-smi-common-vpp
+          - mediatek,mt8188-smi-sub-common
           - mediatek,mt8192-smi-common
           - mediatek,mt8195-smi-common-vdo
           - mediatek,mt8195-smi-common-vpp
@@ -107,6 +108,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - mediatek,mt8188-smi-sub-common
               - mediatek,mt8195-smi-sub-common
     then:
       required:
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
index 2381660b324c..5f162bb360db 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
@@ -69,6 +69,16 @@ properties:
     description: the hardware id of this larb. It's only required when this
       hardware id is not consecutive from its M4U point of view.
 
+  mediatek,smi-sub-comm:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: a phandle of smi_sub_common that the larb is linked to.
+
+  mediatek,smi-sub-comm-in-portid:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 7
+    description: which port of smi_sub_common that the larb is linked to.
+
 required:
   - compatible
   - reg
@@ -125,6 +135,18 @@ allOf:
       required:
         - mediatek,larb-id
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt8188-smi-larb
+
+    then:
+      required:
+        - mediatek,smi-sub-comm
+        - mediatek,smi-sub-comm-in-portid
+
 additionalProperties: false
 
 examples:
-- 
2.46.0

[PATCH 3/4] memory: mtk-smi: mt8188: Add SMI clamp function

[friday.yang]

In order to avoid handling glitch signal when MTCMOS on/off, SMI need
clamp and reset operation. Parse power reset settings for LARBs which
need to reset. Register genpd callback for SMI LARBs and apply reset
operations in the callback.

Signed-off-by: friday.yang <friday.yang@mediatek.com>
---
 drivers/memory/mtk-smi.c | 148 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index fbe52ecc0eca..1ccd62a17b1d 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -10,11 +10,16 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/reset-controller.h>
 #include <linux/soc/mediatek/mtk_sip_svc.h>
 #include <soc/mediatek/smi.h>
 #include <dt-bindings/memory/mt2701-larb-port.h>
@@ -36,6 +41,13 @@
 #define SMI_DCM				0x300
 #define SMI_DUMMY			0x444
 
+#define SMI_COMMON_CLAMP_EN		0x3c0
+#define SMI_COMMON_CLAMP_EN_SET		0x3c4
+#define SMI_COMMON_CLAMP_EN_CLR		0x3c8
+#define SMI_COMMON_CLAMP_MASK(inport)	BIT(inport)
+
+#define SMI_SUB_COMM_INPORT_NR		(8)
+
 /* SMI LARB */
 #define SMI_LARB_SLP_CON                0xc
 #define SLP_PROT_EN                     BIT(0)
@@ -150,6 +162,7 @@ struct mtk_smi {
 };
 
 struct mtk_smi_larb { /* larb: local arbiter */
+	struct device			*dev;
 	struct mtk_smi			smi;
 	void __iomem			*base;
 	struct device			*smi_common_dev; /* common or sub-common dev */
@@ -157,6 +170,10 @@ struct mtk_smi_larb { /* larb: local arbiter */
 	int				larbid;
 	u32				*mmu;
 	unsigned char			*bank;
+	struct regmap			*sub_comm_syscon;
+	int				sub_comm_inport;
+	struct notifier_block		nb;
+	struct reset_control		*rst_con;
 };
 
 static int
@@ -472,6 +489,60 @@ static void mtk_smi_larb_sleep_ctrl_disable(struct mtk_smi_larb *larb)
 	writel_relaxed(0, larb->base + SMI_LARB_SLP_CON);
 }
 
+static int mtk_smi_larb_clamp_protect_enable(struct device *dev)
+{
+	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
+	int ret;
+
+	/* sub_comm_syscon could be NULL if larb directly linked to SMI common */
+	if (!larb->sub_comm_syscon)
+		return -EINVAL;
+
+	ret = regmap_write(larb->sub_comm_syscon, SMI_COMMON_CLAMP_EN_SET,
+			   SMI_COMMON_CLAMP_MASK(larb->sub_comm_inport));
+	if (ret)
+		dev_err(dev, "Unable to enable clamp, inport %d, ret %d\n",
+			larb->sub_comm_inport, ret);
+
+	return ret;
+}
+
+static int mtk_smi_larb_clamp_protect_disble(struct device *dev)
+{
+	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
+	int ret;
+
+	/* sub_comm_syscon could be NULL if larb directly linked to SMI common */
+	if (!larb->sub_comm_syscon)
+		return -EINVAL;
+
+	ret = regmap_write(larb->sub_comm_syscon, SMI_COMMON_CLAMP_EN_CLR,
+			   SMI_COMMON_CLAMP_MASK(larb->sub_comm_inport));
+	if (ret)
+		dev_err(dev, "Unable to disable clamp, inport %d, ret %d\n",
+			larb->sub_comm_inport, ret);
+
+	return ret;
+}
+
+static int mtk_smi_genpd_callback(struct notifier_block *nb,
+				  unsigned long flags, void *data)
+{
+	struct mtk_smi_larb *larb = container_of(nb, struct mtk_smi_larb, nb);
+	struct device *dev = larb->dev;
+
+	if (flags == GENPD_NOTIFY_PRE_ON || flags == GENPD_NOTIFY_PRE_OFF) {
+		/* disable related SMI sub-common port */
+		mtk_smi_larb_clamp_protect_enable(dev);
+	} else if (flags == GENPD_NOTIFY_ON) {
+		/* enable related SMI sub-common port */
+		reset_control_reset(larb->rst_con);
+		mtk_smi_larb_clamp_protect_disble(dev);
+	}
+
+	return NOTIFY_OK;
+}
+
 static int mtk_smi_device_link_common(struct device *dev, struct device **com_dev)
 {
 	struct platform_device *smi_com_pdev;
@@ -528,6 +599,59 @@ static int mtk_smi_dts_clk_init(struct device *dev, struct mtk_smi *smi,
 	return ret;
 }
 
+static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb *larb)
+{
+	struct device *dev = larb->dev;
+	struct device_node *smi_node;
+	int ret;
+
+	/* smi_node could be NULL if larb directly linked to SMI common */
+	smi_node = of_parse_phandle(dev->of_node, "mediatek,smi-sub-comm", 0);
+	if (!smi_node)
+		return 0;
+
+	larb->sub_comm_syscon = device_node_to_regmap(smi_node);
+	of_node_put(smi_node);
+	ret = of_property_read_u32(dev->of_node,
+				   "mediatek,smi-sub-comm-in-portid",
+				   &larb->sub_comm_inport);
+
+	if (IS_ERR(larb->sub_comm_syscon) || ret ||
+	    larb->sub_comm_inport >= SMI_SUB_COMM_INPORT_NR) {
+		larb->sub_comm_syscon = NULL;
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb *larb)
+{
+	struct device_node *reset_node;
+	struct device *dev = larb->dev;
+	int ret;
+
+	/* only larb with "resets" need to get reset setting */
+	reset_node = of_parse_phandle(dev->of_node, "resets", 0);
+	if (!reset_node)
+		return 0;
+	of_node_put(reset_node);
+
+	larb->rst_con = devm_reset_control_get(dev, "larb_rst");
+	if (IS_ERR(larb->rst_con))
+		return dev_err_probe(dev, PTR_ERR(larb->rst_con),
+				     "cannot get larb reset controller\n");
+
+	larb->nb.notifier_call = mtk_smi_genpd_callback;
+	ret = dev_pm_genpd_add_notifier(dev, &larb->nb);
+	if (ret) {
+		dev_err(dev, "Failed to add genpd callback %d\n", ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int mtk_smi_larb_probe(struct platform_device *pdev)
 {
 	struct mtk_smi_larb *larb;
@@ -538,6 +662,7 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
 	if (!larb)
 		return -ENOMEM;
 
+	larb->dev = dev;
 	larb->larb_gen = of_device_get_match_data(dev);
 	larb->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(larb->base))
@@ -554,15 +679,29 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	pm_runtime_enable(dev);
+	/* find sub common to clamp larb for ISP software reset */
+	ret = mtk_smi_larb_parse_clamp_info(larb);
+	if (ret) {
+		dev_err(dev, "Failed to get clamp setting for larb\n");
+		goto err_pm_disable;
+	}
+
+	ret = mtk_smi_larb_parse_reset_info(larb);
+	if (ret) {
+		dev_err(dev, "Failed to get power setting for larb\n");
+		goto err_pm_disable;
+	}
+
 	platform_set_drvdata(pdev, larb);
 	ret = component_add(dev, &mtk_smi_larb_component_ops);
 	if (ret)
 		goto err_pm_disable;
+
+	pm_runtime_enable(dev);
+
 	return 0;
 
 err_pm_disable:
-	pm_runtime_disable(dev);
 	device_link_remove(dev, larb->smi_common_dev);
 	return ret;
 }
@@ -686,6 +825,10 @@ static const struct mtk_smi_common_plat mtk_smi_common_mt8188_vpp = {
 	.init     = mtk_smi_common_mt8195_init,
 };
 
+static const struct mtk_smi_common_plat mtk_smi_sub_common_mt8188 = {
+	.type     = MTK_SMI_GEN2_SUB_COMM,
+};
+
 static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = {
 	.type     = MTK_SMI_GEN2,
 	.has_gals = true,
@@ -729,6 +872,7 @@ static const struct of_device_id mtk_smi_common_of_ids[] = {
 	{.compatible = "mediatek,mt8186-smi-common", .data = &mtk_smi_common_mt8186},
 	{.compatible = "mediatek,mt8188-smi-common-vdo", .data = &mtk_smi_common_mt8188_vdo},
 	{.compatible = "mediatek,mt8188-smi-common-vpp", .data = &mtk_smi_common_mt8188_vpp},
+	{.compatible = "mediatek,mt8188-smi-sub-common", .data = &mtk_smi_sub_common_mt8188},
 	{.compatible = "mediatek,mt8192-smi-common", .data = &mtk_smi_common_mt8192},
 	{.compatible = "mediatek,mt8195-smi-common-vdo", .data = &mtk_smi_common_mt8195_vdo},
 	{.compatible = "mediatek,mt8195-smi-common-vpp", .data = &mtk_smi_common_mt8195_vpp},
-- 
2.46.0

[PATCH 4/4] reset: mediatek: Add reset control driver for SMI

[friday.yang]

Add a reset-controller driver for performing reset management of
SMI LARBs on MediaTek platform. This driver uses the regmap
frameworks to actually implement the various reset functions
needed when SMI LARBs apply clamp operations.

Signed-off-by: friday.yang <friday.yang@mediatek.com>
---
 drivers/reset/Kconfig              |   9 ++
 drivers/reset/Makefile             |   1 +
 drivers/reset/reset-mediatek-smi.c | 152 +++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+)
 create mode 100644 drivers/reset/reset-mediatek-smi.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 67bce340a87e..e984a5a332f1 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -154,6 +154,15 @@ config RESET_MESON_AUDIO_ARB
 	  This enables the reset driver for Audio Memory Arbiter of
 	  Amlogic's A113 based SoCs
 
+config RESET_MTK_SMI
+	bool "MediaTek SMI Reset Driver"
+	depends on MTK_SMI
+	help
+	  This option enables the reset controller driver for MediaTek SMI.
+	  This reset driver is responsible for managing the reset signals
+	  for SMI larbs. Say Y if you want to control reset signals for
+	  MediaTek SMI larbs. Otherwise, say N.
+
 config RESET_NPCM
 	bool "NPCM BMC Reset Driver" if COMPILE_TEST
 	default ARCH_NPCM
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 27b0bbdfcc04..241777485b40 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
 obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
 obj-$(CONFIG_RESET_MESON) += reset-meson.o
 obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
+obj-$(CONFIG_RESET_MTK_SMI) += reset-mediatek-smi.o
 obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
 obj-$(CONFIG_RESET_NUVOTON_MA35D1) += reset-ma35d1.o
 obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
diff --git a/drivers/reset/reset-mediatek-smi.c b/drivers/reset/reset-mediatek-smi.c
new file mode 100644
index 000000000000..ead747e80ad5
--- /dev/null
+++ b/drivers/reset/reset-mediatek-smi.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Reset driver for MediaTek SMI module
+ *
+ * Copyright (C) 2024 MediaTek Inc.
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+#include <dt-bindings/reset/mt8188-resets.h>
+
+#define to_mtk_smi_reset_data(_rcdev)	\
+	container_of(_rcdev, struct mtk_smi_reset_data, rcdev)
+
+struct mtk_smi_larb_reset {
+	unsigned int offset;
+	unsigned int value;
+};
+
+static const struct mtk_smi_larb_reset rst_signal_mt8188[] = {
+	[MT8188_SMI_RST_LARB10]		= { 0xC, BIT(0) }, /* larb10 */
+	[MT8188_SMI_RST_LARB11A]	= { 0xC, BIT(0) }, /* larb11a */
+	[MT8188_SMI_RST_LARB11C]	= { 0xC, BIT(0) }, /* larb11c */
+	[MT8188_SMI_RST_LARB12]		= { 0xC, BIT(8) }, /* larb12 */
+	[MT8188_SMI_RST_LARB11B]	= { 0xC, BIT(0) }, /* larb11b */
+	[MT8188_SMI_RST_LARB15]		= { 0xC, BIT(0) }, /* larb15 */
+	[MT8188_SMI_RST_LARB16B]	= { 0xA0, BIT(4) }, /* larb16b */
+	[MT8188_SMI_RST_LARB17B]	= { 0xA0, BIT(4) }, /* larb17b */
+	[MT8188_SMI_RST_LARB16A]	= { 0xA0, BIT(4) }, /* larb16a */
+	[MT8188_SMI_RST_LARB17A]	= { 0xA0, BIT(4) }, /* larb17a */
+};
+
+struct mtk_smi_larb_plat {
+	const struct mtk_smi_larb_reset		*reset_signal;
+	const unsigned int			larb_reset_nr;
+};
+
+struct mtk_smi_reset_data {
+	const struct mtk_smi_larb_plat *larb_plat;
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+};
+
+static const struct mtk_smi_larb_plat mtk_smi_larb_mt8188 = {
+	.reset_signal = rst_signal_mt8188,
+	.larb_reset_nr = ARRAY_SIZE(rst_signal_mt8188),
+};
+
+static int mtk_smi_larb_reset(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
+	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
+	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
+	int ret;
+
+	ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst->value);
+	if (ret)
+		return ret;
+	ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst->value);
+
+	return ret;
+}
+
+static int mtk_smi_larb_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
+	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
+	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
+	int ret;
+
+	ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst->value);
+	if (ret)
+		dev_err(rcdev->dev, "[%s] Failed to shutdown larb %d\n", __func__, ret);
+
+	return ret;
+}
+
+static int mtk_smi_larb_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
+	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
+	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
+	int ret;
+
+	ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst->value);
+	if (ret)
+		dev_err(rcdev->dev, "[%s] Failed to reopen larb %d\n", __func__, ret);
+
+	return ret;
+}
+
+static const struct reset_control_ops mtk_smi_reset_ops = {
+	.reset		= mtk_smi_larb_reset,
+	.assert		= mtk_smi_larb_reset_assert,
+	.deassert	= mtk_smi_larb_reset_deassert,
+};
+
+static int mtk_smi_reset_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct mtk_smi_larb_plat *larb_plat = of_device_get_match_data(dev);
+	struct device_node *np = dev->of_node, *reset_node;
+	struct mtk_smi_reset_data *data;
+	struct regmap *regmap;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	reset_node = of_parse_phandle(np, "mediatek,larb-rst-syscon", 0);
+	if (!reset_node)
+		return -EINVAL;
+
+	regmap = device_node_to_regmap(reset_node);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	data->larb_plat = larb_plat;
+	data->regmap = regmap;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.ops = &mtk_smi_reset_ops;
+	data->rcdev.of_node = np;
+	data->rcdev.nr_resets = larb_plat->larb_reset_nr;
+	data->rcdev.dev = dev;
+	platform_set_drvdata(pdev, data);
+
+	return devm_reset_controller_register(dev, &data->rcdev);
+}
+
+static const struct of_device_id mtk_smi_larb_reset_of_match[] = {
+	{ .compatible = "mediatek,smi-reset-mt8188", .data = &mtk_smi_larb_mt8188 },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mtk_smi_larb_reset_of_match);
+
+static struct platform_driver mtk_smi_reset_driver = {
+	.probe = mtk_smi_reset_probe,
+	.driver = {
+		.name = "mediatek-smi-reset",
+		.of_match_table = mtk_smi_larb_reset_of_match,
+	},
+};
+module_platform_driver(mtk_smi_reset_driver);
+
+MODULE_AUTHOR("Friday.Yang@mediatek.com");
+MODULE_DESCRIPTION("MediaTek SMI Reset Driver");
+MODULE_LICENSE("GPL");
-- 
2.46.0

Re: [PATCH 1/4] dt-bindings: memory: mediatek: Add mt8188 SMI reset control binding

[Krzysztof Kozlowski]

On 21/08/2024 10:26, friday.yang wrote:
> To support SMI clamp and reset operation in genpd callback, add
> SMI LARB reset register offset and mask related information in
> the bindings. Add index in mt8188-resets.h to query the register
> offset and mask in the SMI reset control driver.
> 
> Signed-off-by: friday.yang <friday.yang@mediatek.com>

User proper full name instead of login.


Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> ---
>  .../bindings/reset/mediatek,smi-reset.yaml    | 46 +++++++++++++++++++
>  include/dt-bindings/reset/mt8188-resets.h     | 11 +++++
>  2 files changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml b/Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml
> new file mode 100644
> index 000000000000..66ac121d2396
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2024 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/mediatek,smi-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek SMI Reset Controller
> +
> +maintainers:
> +  - Friday Yang <friday.yang@mediatek.com>
> +
> +description: |
> +  This reset controller node is used to perform reset management
> +  of SMI larbs on MediaTek platform. It is used to implement various
> +  reset functions required when SMI larbs apply clamp operation.
> +
> +  For list of all valid reset indices see
> +    <dt-bindings/reset/mt8188-resets.h> for MT8188.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,smi-reset-mt8188

Wrong placement of soc. It's mediatek,mt8189-whatever

> +
> +  "#reset-cells":
> +    const: 1
> +
> +  mediatek,larb-rst-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of the SMI larb's reset controller syscon.

Explain what is it used for.

> +
> +required:
> +  - compatible
> +  - "#reset-cells"
> +  - mediatek,larb-rst-syscon
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    imgsys1_dip_top_rst: reset-controller {

Drop label

> +          compatible = "mediatek,smi-reset-mt8188";

Use 4 spaces for example indentation.

> +          #reset-cells = <1>;
> +          mediatek,larb-rst-syscon = <&imgsys1_dip_top>;
> +    };


Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: memory: mediatek: Add smi-sub-common property for reset

[Krzysztof Kozlowski]

On 21/08/2024 10:26, friday.yang wrote:
> On the MediaTek platform, some SMI LARBs are directly linked to SMI
> common. While some SMI LARBs are linked to SMI sub common, then SMI
> sub common is linked to SMI common. Add 'mediatek,smi-sub-comm' and
> 'mediatek,smi-sub-comm-in-portid' properties here. The SMI reset
> driver could query which port of the SMI sub common the current LARB
> is linked to through the two properties. The hardware block diagram
> could be described as below.
> 
>              SMI Common(Smart Multimedia Interface Common)
>                  |
>          +----------------+-------
>          |                |
>          |                |
>          |                |
>          |                |
>          |                |
>        larb0       SMI Sub Common
>                    |      |     |
>                   larb1  larb2 larb3
> 
> Signed-off-by: friday.yang <friday.yang@mediatek.com>
> ---
>  .../mediatek,smi-common.yaml                  |  2 ++
>  .../memory-controllers/mediatek,smi-larb.yaml | 22 +++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> index 2f36ac23604c..4392d349878c 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> @@ -39,6 +39,7 @@ properties:
>            - mediatek,mt8186-smi-common
>            - mediatek,mt8188-smi-common-vdo
>            - mediatek,mt8188-smi-common-vpp
> +          - mediatek,mt8188-smi-sub-common
>            - mediatek,mt8192-smi-common
>            - mediatek,mt8195-smi-common-vdo
>            - mediatek,mt8195-smi-common-vpp
> @@ -107,6 +108,7 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - mediatek,mt8188-smi-sub-common
>                - mediatek,mt8195-smi-sub-common
>      then:
>        required:
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> index 2381660b324c..5f162bb360db 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> @@ -69,6 +69,16 @@ properties:
>      description: the hardware id of this larb. It's only required when this
>        hardware id is not consecutive from its M4U point of view.
>  
> +  mediatek,smi-sub-comm:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: a phandle of smi_sub_common that the larb is linked to.

Why do you have to smi phandle properties per each node?

> +
> +  mediatek,smi-sub-comm-in-portid:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 7
> +    description: which port of smi_sub_common that the larb is linked to.

Merge it into phandle.

> +
>  required:
>    - compatible
>    - reg
> @@ -125,6 +135,18 @@ allOf:
>        required:
>          - mediatek,larb-id
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - mediatek,mt8188-smi-larb
> +
> +    then:
> +      required:
> +        - mediatek,smi-sub-comm
> +        - mediatek,smi-sub-comm-in-portid
> +

and add it to the example (since you claim it is valid for every device).

Best regards,
Krzysztof

Re: [PATCH 4/4] reset: mediatek: Add reset control driver for SMI

[Krzysztof Kozlowski]

On 21/08/2024 10:26, friday.yang wrote:
> Add a reset-controller driver for performing reset management of
> SMI LARBs on MediaTek platform. This driver uses the regmap
> frameworks to actually implement the various reset functions
> needed when SMI LARBs apply clamp operations.

How does this depend on memory controller patches? Why is this grouped
in one patchset?

> 
> Signed-off-by: friday.yang <friday.yang@mediatek.com>
> ---
>  drivers/reset/Kconfig              |   9 ++
>  drivers/reset/Makefile             |   1 +
>  drivers/reset/reset-mediatek-smi.c | 152 +++++++++++++++++++++++++++++
>  3 files changed, 162 insertions(+)
>  create mode 100644 drivers/reset/reset-mediatek-smi.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 67bce340a87e..e984a5a332f1 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -154,6 +154,15 @@ config RESET_MESON_AUDIO_ARB
>  	  This enables the reset driver for Audio Memory Arbiter of
>  	  Amlogic's A113 based SoCs
>  
> +config RESET_MTK_SMI
> +	bool "MediaTek SMI Reset Driver"
> +	depends on MTK_SMI

compile test

> +	help
> +	  This option enables the reset controller driver for MediaTek SMI.
> +	  This reset driver is responsible for managing the reset signals
> +	  for SMI larbs. Say Y if you want to control reset signals for
> +	  MediaTek SMI larbs. Otherwise, say N.
> +
>  config RESET_NPCM
>  	bool "NPCM BMC Reset Driver" if COMPILE_TEST
>  	default ARCH_NPCM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 27b0bbdfcc04..241777485b40 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
>  obj-$(CONFIG_RESET_MESON) += reset-meson.o
>  obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
> +obj-$(CONFIG_RESET_MTK_SMI) += reset-mediatek-smi.o
>  obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
>  obj-$(CONFIG_RESET_NUVOTON_MA35D1) += reset-ma35d1.o
>  obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> diff --git a/drivers/reset/reset-mediatek-smi.c b/drivers/reset/reset-mediatek-smi.c
> new file mode 100644
> index 000000000000..ead747e80ad5
> --- /dev/null
> +++ b/drivers/reset/reset-mediatek-smi.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Reset driver for MediaTek SMI module
> + *
> + * Copyright (C) 2024 MediaTek Inc.
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#include <dt-bindings/reset/mt8188-resets.h>
> +
> +#define to_mtk_smi_reset_data(_rcdev)	\
> +	container_of(_rcdev, struct mtk_smi_reset_data, rcdev)
> +
> +struct mtk_smi_larb_reset {
> +	unsigned int offset;
> +	unsigned int value;
> +};
> +
> +static const struct mtk_smi_larb_reset rst_signal_mt8188[] = {
> +	[MT8188_SMI_RST_LARB10]		= { 0xC, BIT(0) }, /* larb10 */
> +	[MT8188_SMI_RST_LARB11A]	= { 0xC, BIT(0) }, /* larb11a */
> +	[MT8188_SMI_RST_LARB11C]	= { 0xC, BIT(0) }, /* larb11c */
> +	[MT8188_SMI_RST_LARB12]		= { 0xC, BIT(8) }, /* larb12 */
> +	[MT8188_SMI_RST_LARB11B]	= { 0xC, BIT(0) }, /* larb11b */
> +	[MT8188_SMI_RST_LARB15]		= { 0xC, BIT(0) }, /* larb15 */
> +	[MT8188_SMI_RST_LARB16B]	= { 0xA0, BIT(4) }, /* larb16b */
> +	[MT8188_SMI_RST_LARB17B]	= { 0xA0, BIT(4) }, /* larb17b */
> +	[MT8188_SMI_RST_LARB16A]	= { 0xA0, BIT(4) }, /* larb16a */
> +	[MT8188_SMI_RST_LARB17A]	= { 0xA0, BIT(4) }, /* larb17a */
> +};
> +
> +struct mtk_smi_larb_plat {
> +	const struct mtk_smi_larb_reset		*reset_signal;
> +	const unsigned int			larb_reset_nr;
> +};
> +
> +struct mtk_smi_reset_data {
> +	const struct mtk_smi_larb_plat *larb_plat;
> +	struct reset_controller_dev rcdev;
> +	struct regmap *regmap;
> +};
> +
> +static const struct mtk_smi_larb_plat mtk_smi_larb_mt8188 = {
> +	.reset_signal = rst_signal_mt8188,
> +	.larb_reset_nr = ARRAY_SIZE(rst_signal_mt8188),
> +};
> +
> +static int mtk_smi_larb_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> +	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> +	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
> +	int ret;
> +
> +	ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst->value);
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst->value);
> +
> +	return ret;
> +}
> +
> +static int mtk_smi_larb_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> +	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> +	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
> +	int ret;
> +
> +	ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst->value);
> +	if (ret)
> +		dev_err(rcdev->dev, "[%s] Failed to shutdown larb %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int mtk_smi_larb_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> +	const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> +	const struct mtk_smi_larb_reset *larb_rst = larb_plat->reset_signal + id;
> +	int ret;
> +
> +	ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst->value);
> +	if (ret)
> +		dev_err(rcdev->dev, "[%s] Failed to reopen larb %d\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static const struct reset_control_ops mtk_smi_reset_ops = {
> +	.reset		= mtk_smi_larb_reset,
> +	.assert		= mtk_smi_larb_reset_assert,
> +	.deassert	= mtk_smi_larb_reset_deassert,
> +};
> +
> +static int mtk_smi_reset_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct mtk_smi_larb_plat *larb_plat = of_device_get_match_data(dev);
> +	struct device_node *np = dev->of_node, *reset_node;
> +	struct mtk_smi_reset_data *data;
> +	struct regmap *regmap;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	reset_node = of_parse_phandle(np, "mediatek,larb-rst-syscon", 0);
> +	if (!reset_node)

This looks just wrong. This looks like a child of whatever phandle
points here.

Why do you create MMIO-using node as not MMIO?

Best regards,
Krzysztof

Re: [PATCH 3/4] memory: mtk-smi: mt8188: Add SMI clamp function

[Krzysztof Kozlowski]

On 21/08/2024 10:26, friday.yang wrote:
> In order to avoid handling glitch signal when MTCMOS on/off, SMI need
> clamp and reset operation. Parse power reset settings for LARBs which
> need to reset. Register genpd callback for SMI LARBs and apply reset
> operations in the callback.
> 
> Signed-off-by: friday.yang <friday.yang@mediatek.com>
> ---
>  drivers/memory/mtk-smi.c | 148 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 146 insertions(+), 2 deletions(-)
> 

...

> +
> +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb *larb)
> +{
> +	struct device_node *reset_node;
> +	struct device *dev = larb->dev;
> +	int ret;
> +
> +	/* only larb with "resets" need to get reset setting */
> +	reset_node = of_parse_phandle(dev->of_node, "resets", 0);

Nope, you do not parse rasets.

> +	if (!reset_node)
> +		return 0;
> +	of_node_put(reset_node);
> +
> +	larb->rst_con = devm_reset_control_get(dev, "larb_rst");

Where are the bindings? Why do you add undocumented properties? How
possible this passes dtbs_check???


> +	if (IS_ERR(larb->rst_con))
> +		return dev_err_probe(dev, PTR_ERR(larb->rst_con),
> +				     "cannot get larb reset controller\n");
> +
> +	larb->nb.notifier_call = mtk_smi_genpd_callback;
> +	ret = dev_pm_genpd_add_notifier(dev, &larb->nb);
> +	if (ret) {
> +		dev_err(dev, "Failed to add genpd callback %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mtk_smi_larb_probe(struct platform_device *pdev)
>  {
>  	struct mtk_smi_larb *larb;
> @@ -538,6 +662,7 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>  	if (!larb)
>  		return -ENOMEM;
>  
> +	larb->dev = dev;
>  	larb->larb_gen = of_device_get_match_data(dev);
>  	larb->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(larb->base))
> @@ -554,15 +679,29 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	pm_runtime_enable(dev);
> +	/* find sub common to clamp larb for ISP software reset */
> +	ret = mtk_smi_larb_parse_clamp_info(larb);
> +	if (ret) {
> +		dev_err(dev, "Failed to get clamp setting for larb\n");
> +		goto err_pm_disable;
> +	}
> +
> +	ret = mtk_smi_larb_parse_reset_info(larb);
> +	if (ret) {
> +		dev_err(dev, "Failed to get power setting for larb\n");
> +		goto err_pm_disable;
> +	}
> +
>  	platform_set_drvdata(pdev, larb);
>  	ret = component_add(dev, &mtk_smi_larb_component_ops);
>  	if (ret)
>  		goto err_pm_disable;
> +
> +	pm_runtime_enable(dev);
> +
>  	return 0;
>  
>  err_pm_disable:
> -	pm_runtime_disable(dev);
>  	device_link_remove(dev, larb->smi_common_dev);

Label asls pm disable. Where is the pm disable?

>  	return ret;
>  }
> @@ -686,6 +825,10 @@ static const struct mtk_smi_common_plat mtk_smi_common_mt8188_vpp = {
>  	.init     = mtk_smi_common_mt8195_init,
>  };

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: memory: mediatek: Add mt8188 SMI reset control binding

[Krzysztof Kozlowski]

On 21/08/2024 10:26, friday.yang wrote:
> To support SMI clamp and reset operation in genpd callback, add
> SMI LARB reset register offset and mask related information in
> the bindings. Add index in mt8188-resets.h to query the register
> offset and mask in the SMI reset control driver.
> 
> Signed-off-by: friday.yang <friday.yang@mediatek.com>
> ---
>  .../bindings/reset/mediatek,smi-reset.yaml    | 46 +++++++++++++++++++
>  include/dt-bindings/reset/mt8188-resets.h     | 11 +++++

Also, your patches did not reach DT patchwork, so something is odd.
Maybe they got flagged as spam? Please investigate with your IT
department. In case it keeps missing patchwork, they won't be tested by
automation and I will generally ignore them (not apply). :(

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: memory: mediatek: Add mt8188 SMI reset control binding

[Krzysztof Kozlowski]

On Wed, Aug 21, 2024 at 11:28:17AM +0200, Krzysztof Kozlowski wrote:
> On 21/08/2024 10:26, friday.yang wrote:
> > To support SMI clamp and reset operation in genpd callback, add
> > SMI LARB reset register offset and mask related information in
> > the bindings. Add index in mt8188-resets.h to query the register
> > offset and mask in the SMI reset control driver.
> > 
> > Signed-off-by: friday.yang <friday.yang@mediatek.com>
> > ---
> >  .../bindings/reset/mediatek,smi-reset.yaml    | 46 +++++++++++++++++++
> >  include/dt-bindings/reset/mt8188-resets.h     | 11 +++++
> 
> Also, your patches did not reach DT patchwork, so something is odd.
> Maybe they got flagged as spam? Please investigate with your IT
> department. In case it keeps missing patchwork, they won't be tested by
> automation and I will generally ignore them (not apply). :(

Update: they reached now, so maybe this was some Patchwork issue. You
still can double check that you send patches correctly.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: memory: mediatek: Add mt8188 SMI reset control binding

[Friday Yang (杨阳)]

On Wed, 2024-08-21 at 10:54 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 21/08/2024 10:26, friday.yang wrote:
> > To support SMI clamp and reset operation in genpd callback, add
> > SMI LARB reset register offset and mask related information in
> > the bindings. Add index in mt8188-resets.h to query the register
> > offset and mask in the SMI reset control driver.
> > 
> > Signed-off-by: friday.yang <friday.yang@mediatek.com>
> 
> User proper full name instead of login.
> 

Thanks for your comments and sorry for replying so late.
I will fix it to Friday Yang <friday.yang@mediatek.com>.

> 
> Please use subject prefixes matching the subsystem. You can get them
> for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> 
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 

I have already used this command `git log --oneline Documentation/devicetree/bindings/reset/`If you think it is inappropriate, how about this one?
'dt-bindings: reset: mediatek,mt8188-smi: Add SMI reset control binding
for MT8188'

> > ---
> >  .../bindings/reset/mediatek,smi-reset.yaml    | 46
> +++++++++++++++++++
> >  include/dt-bindings/reset/mt8188-resets.h     | 11 +++++
> >  2 files changed, 57 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/reset/mediatek,smi-
> reset.yaml b/Documentation/devicetree/bindings/reset/mediatek,smi-
> reset.yaml
> > new file mode 100644
> > index 000000000000..66ac121d2396
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reset/mediatek,smi-
> reset.yaml
> > @@ -0,0 +1,46 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2024 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/reset/mediatek,smi-reset.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek SMI Reset Controller
> > +
> > +maintainers:
> > +  - Friday Yang <friday.yang@mediatek.com>
> > +
> > +description: |
> > +  This reset controller node is used to perform reset management
> > +  of SMI larbs on MediaTek platform. It is used to implement
> various
> > +  reset functions required when SMI larbs apply clamp operation.
> > +
> > +  For list of all valid reset indices see
> > +    <dt-bindings/reset/mt8188-resets.h> for MT8188.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,smi-reset-mt8188
> 
> Wrong placement of soc. It's mediatek,mt8189-whatever
> 

Thanks, I will fix it to 'mediatek,mt8188-smi-reset'.

> > +
> > +  "#reset-cells":
> > +    const: 1
> > +
> > +  mediatek,larb-rst-syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle of the SMI larb's reset controller
> syscon.
> 
> Explain what is it used for.
> 

I will add the descriton like this, is this clear for you?
When SMI larb power on/off, we need larb clamp and reset to avoid bus
glitch, this is to improve bus protect. 
The reset controller node is used to update regmap to implement larb
reset function.

> > +
> > +required:
> > +  - compatible
> > +  - "#reset-cells"
> > +  - mediatek,larb-rst-syscon
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    imgsys1_dip_top_rst: reset-controller {
> 
> Drop label

I see marvell,berlin2-reset.yaml, which also have a label.
Do you mean we should remove imgsys1_dip_top_rst, just use
'reset-controller' here.

> 
> > +          compatible = "mediatek,smi-reset-mt8188";
> 
> Use 4 spaces for example indentation.
> 

OK, I will use spaces instead of tab.

> > +          #reset-cells = <1>;
> > +          mediatek,larb-rst-syscon = <&imgsys1_dip_top>;
> > +    };
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/4] dt-bindings: memory: mediatek: Add smi-sub-common property for reset

[Friday Yang (杨阳)]

On Wed, 2024-08-21 at 10:55 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 21/08/2024 10:26, friday.yang wrote:
> > On the MediaTek platform, some SMI LARBs are directly linked to SMI
> > common. While some SMI LARBs are linked to SMI sub common, then SMI
> > sub common is linked to SMI common. Add 'mediatek,smi-sub-comm' and
> > 'mediatek,smi-sub-comm-in-portid' properties here. The SMI reset
> > driver could query which port of the SMI sub common the current
> LARB
> > is linked to through the two properties. The hardware block diagram
> > could be described as below.
> > 
> >              SMI Common(Smart Multimedia Interface Common)
> >                  |
> >          +----------------+-------
> >          |                |
> >          |                |
> >          |                |
> >          |                |
> >          |                |
> >        larb0       SMI Sub Common
> >                    |      |     |
> >                   larb1  larb2 larb3
> > 
> > Signed-off-by: friday.yang <friday.yang@mediatek.com>
> > ---
> >  .../mediatek,smi-common.yaml                  |  2 ++
> >  .../memory-controllers/mediatek,smi-larb.yaml | 22
> +++++++++++++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-common.yaml
> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
> common.yaml
> > index 2f36ac23604c..4392d349878c 100644
> > --- a/Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-common.yaml
> > +++ b/Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-common.yaml
> > @@ -39,6 +39,7 @@ properties:
> >            - mediatek,mt8186-smi-common
> >            - mediatek,mt8188-smi-common-vdo
> >            - mediatek,mt8188-smi-common-vpp
> > +          - mediatek,mt8188-smi-sub-common
> >            - mediatek,mt8192-smi-common
> >            - mediatek,mt8195-smi-common-vdo
> >            - mediatek,mt8195-smi-common-vpp
> > @@ -107,6 +108,7 @@ allOf:
> >          compatible:
> >            contains:
> >              enum:
> > +              - mediatek,mt8188-smi-sub-common
> >                - mediatek,mt8195-smi-sub-common
> >      then:
> >        required:
> > diff --git a/Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-larb.yaml
> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
> larb.yaml
> > index 2381660b324c..5f162bb360db 100644
> > --- a/Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-larb.yaml
> > +++ b/Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-larb.yaml
> > @@ -69,6 +69,16 @@ properties:
> >      description: the hardware id of this larb. It's only required
> when this
> >        hardware id is not consecutive from its M4U point of view.
> >  
> > +  mediatek,smi-sub-comm:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: a phandle of smi_sub_common that the larb is
> linked to.
> 
> Why do you have to smi phandle properties per each node?
> 

As shown in the picture from the commit message, we have multipule smi-
sub-common, each SMI larb may link to one of the smi-sub-common. So we
need the 'mediatek,smi-sub-comm' to describe which smi-sub-common the
larb is linked to.
In next version, I will add two smi-sub-common to the diagram in the
commit message.

> > +
> > +  mediatek,smi-sub-comm-in-portid:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 7
> > +    description: which port of smi_sub_common that the larb is
> linked to.
> 
> Merge it into phandle.
> 

Just confirm,
Do you mean merge these two into one property, like:
mediatek,smi-sub-comm = <&phandle port-id>;

> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -125,6 +135,18 @@ allOf:
> >        required:
> >          - mediatek,larb-id
> >  
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - mediatek,mt8188-smi-larb
> > +
> > +    then:
> > +      required:
> > +        - mediatek,smi-sub-comm
> > +        - mediatek,smi-sub-comm-in-portid
> > +
> 
> and add it to the example (since you claim it is valid for every
> device).
> 

OK, I will add this to the example.

> Best regards,
> Krzysztof
> 

Re: [PATCH 3/4] memory: mtk-smi: mt8188: Add SMI clamp function

[Friday Yang (杨阳)]

On Wed, 2024-08-21 at 11:00 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 21/08/2024 10:26, friday.yang wrote:
> > In order to avoid handling glitch signal when MTCMOS on/off, SMI
> need
> > clamp and reset operation. Parse power reset settings for LARBs
> which
> > need to reset. Register genpd callback for SMI LARBs and apply
> reset
> > operations in the callback.
> > 
> > Signed-off-by: friday.yang <friday.yang@mediatek.com>
> > ---
> >  drivers/memory/mtk-smi.c | 148
> ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 146 insertions(+), 2 deletions(-)
> > 
> 
> ...
> 
> > +
> > +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb
> *larb)
> > +{
> > +struct device_node *reset_node;
> > +struct device *dev = larb->dev;
> > +int ret;
> > +
> > +/* only larb with "resets" need to get reset setting */
> > +reset_node = of_parse_phandle(dev->of_node, "resets", 0);
> 
> Nope, you do not parse rasets.

1.If I need to use the Linux reset control framework, 'resets' is the
required property.
2.'reset-names' give the list of reset signal name strings sorted in
the same order as the 'resets' property. SMI driver will use 'reset-
names' to match reset signal names with reset specifiers.
3.Not all SMI larbs need to apply reset operations, only SMI larbs
which may have bus glitch issues need this. Just to confirm if this
larb support reset function.

> 
> > +if (!reset_node)
> > +return 0;
> > +of_node_put(reset_node);
> > +
> > +larb->rst_con = devm_reset_control_get(dev, "larb_rst");
> 
> Where are the bindings? Why do you add undocumented properties? How
> possible this passes dtbs_check???
> 

This is not the new added property in SMI larb DT node.
It is the reset signal name which is inclued in 'reset-names'.
Just like this:

resets = <&imgsys1_dip_nr_rst MT8188_SIM_RST_LARB15>;
reset-name = 'larb_rst';

Maybe I can add this name to the 'reset-name' property binding
description, like this, is this clear for you?

reset-name:
    const: larb_rst
    description: the name of reset signal. SMI driver need to obtain 		
 the reset controller based on this.

> 
> > +if (IS_ERR(larb->rst_con))
> > +return dev_err_probe(dev, PTR_ERR(larb->rst_con),
> > +     "cannot get larb reset controller\n");
> > +
> > +larb->nb.notifier_call = mtk_smi_genpd_callback;
> > +ret = dev_pm_genpd_add_notifier(dev, &larb->nb);
> > +if (ret) {
> > +dev_err(dev, "Failed to add genpd callback %d\n", ret);
> > +return -EINVAL;
> > +}
> > +
> > +return 0;
> > +}
> > +
> >  static int mtk_smi_larb_probe(struct platform_device *pdev)
> >  {
> >  struct mtk_smi_larb *larb;
> > @@ -538,6 +662,7 @@ static int mtk_smi_larb_probe(struct
> platform_device *pdev)
> >  if (!larb)
> >  return -ENOMEM;
> >  
> > +larb->dev = dev;
> >  larb->larb_gen = of_device_get_match_data(dev);
> >  larb->base = devm_platform_ioremap_resource(pdev, 0);
> >  if (IS_ERR(larb->base))
> > @@ -554,15 +679,29 @@ static int mtk_smi_larb_probe(struct
> platform_device *pdev)
> >  if (ret < 0)
> >  return ret;
> >  
> > -pm_runtime_enable(dev);
> > +/* find sub common to clamp larb for ISP software reset */
> > +ret = mtk_smi_larb_parse_clamp_info(larb);
> > +if (ret) {
> > +dev_err(dev, "Failed to get clamp setting for larb\n");
> > +goto err_pm_disable;
> > +}
> > +
> > +ret = mtk_smi_larb_parse_reset_info(larb);
> > +if (ret) {
> > +dev_err(dev, "Failed to get power setting for larb\n");
> > +goto err_pm_disable;
> > +}
> > +
> >  platform_set_drvdata(pdev, larb);
> >  ret = component_add(dev, &mtk_smi_larb_component_ops);
> >  if (ret)
> >  goto err_pm_disable;
> > +
> > +pm_runtime_enable(dev);
> > +
> >  return 0;
> >  
> >  err_pm_disable:
> > -pm_runtime_disable(dev);
> >  device_link_remove(dev, larb->smi_common_dev);
> 
> Label asls pm disable. Where is the pm disable?
> 

Thanks, I will fix it to 'err_link_remove'.

> >  return ret;
> >  }
> > @@ -686,6 +825,10 @@ static const struct mtk_smi_common_plat
> mtk_smi_common_mt8188_vpp = {
> >  .init     = mtk_smi_common_mt8195_init,
> >  };
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 4/4] reset: mediatek: Add reset control driver for SMI

[Friday Yang (杨阳)]

On Wed, 2024-08-21 at 10:58 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 21/08/2024 10:26, friday.yang wrote:
> > Add a reset-controller driver for performing reset management of
> > SMI LARBs on MediaTek platform. This driver uses the regmap
> > frameworks to actually implement the various reset functions
> > needed when SMI LARBs apply clamp operations.
> 
> How does this depend on memory controller patches? Why is this
> grouped
> in one patchset?
> 

How about changing it like this,
patchset1:
(1)SMI reset control driver
(2)SMI reset bindings
patchset2
(1)SMI driver
(2)SMI bindings

> > 
> > Signed-off-by: friday.yang <friday.yang@mediatek.com>
> > ---
> >  drivers/reset/Kconfig              |   9 ++
> >  drivers/reset/Makefile             |   1 +
> >  drivers/reset/reset-mediatek-smi.c | 152
> +++++++++++++++++++++++++++++
> >  3 files changed, 162 insertions(+)
> >  create mode 100644 drivers/reset/reset-mediatek-smi.c
> > 
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 67bce340a87e..e984a5a332f1 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -154,6 +154,15 @@ config RESET_MESON_AUDIO_ARB
> >    This enables the reset driver for Audio Memory Arbiter of
> >    Amlogic's A113 based SoCs
> >  
> > +config RESET_MTK_SMI
> > +bool "MediaTek SMI Reset Driver"
> > +depends on MTK_SMI
> 
> compile test

Thanks, I will fix it to 'depends on MTK_SMI || COMPILE_TEST'

> 
> > +help
> > +  This option enables the reset controller driver for MediaTek
> SMI.
> > +  This reset driver is responsible for managing the reset signals
> > +  for SMI larbs. Say Y if you want to control reset signals for
> > +  MediaTek SMI larbs. Otherwise, say N.
> > +
> >  config RESET_NPCM
> >  bool "NPCM BMC Reset Driver" if COMPILE_TEST
> >  default ARCH_NPCM
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index 27b0bbdfcc04..241777485b40 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -22,6 +22,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
> >  obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
> >  obj-$(CONFIG_RESET_MESON) += reset-meson.o
> >  obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
> > +obj-$(CONFIG_RESET_MTK_SMI) += reset-mediatek-smi.o
> >  obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
> >  obj-$(CONFIG_RESET_NUVOTON_MA35D1) += reset-ma35d1.o
> >  obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
> > diff --git a/drivers/reset/reset-mediatek-smi.c
> b/drivers/reset/reset-mediatek-smi.c
> > new file mode 100644
> > index 000000000000..ead747e80ad5
> > --- /dev/null
> > +++ b/drivers/reset/reset-mediatek-smi.c
> > @@ -0,0 +1,152 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Reset driver for MediaTek SMI module
> > + *
> > + * Copyright (C) 2024 MediaTek Inc.
> > + */
> > +
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> > +
> > +#include <dt-bindings/reset/mt8188-resets.h>
> > +
> > +#define to_mtk_smi_reset_data(_rcdev)\
> > +container_of(_rcdev, struct mtk_smi_reset_data, rcdev)
> > +
> > +struct mtk_smi_larb_reset {
> > +unsigned int offset;
> > +unsigned int value;
> > +};
> > +
> > +static const struct mtk_smi_larb_reset rst_signal_mt8188[] = {
> > +[MT8188_SMI_RST_LARB10]= { 0xC, BIT(0) }, /* larb10 */
> > +[MT8188_SMI_RST_LARB11A]= { 0xC, BIT(0) }, /* larb11a */
> > +[MT8188_SMI_RST_LARB11C]= { 0xC, BIT(0) }, /* larb11c */
> > +[MT8188_SMI_RST_LARB12]= { 0xC, BIT(8) }, /* larb12 */
> > +[MT8188_SMI_RST_LARB11B]= { 0xC, BIT(0) }, /* larb11b */
> > +[MT8188_SMI_RST_LARB15]= { 0xC, BIT(0) }, /* larb15 */
> > +[MT8188_SMI_RST_LARB16B]= { 0xA0, BIT(4) }, /* larb16b */
> > +[MT8188_SMI_RST_LARB17B]= { 0xA0, BIT(4) }, /* larb17b */
> > +[MT8188_SMI_RST_LARB16A]= { 0xA0, BIT(4) }, /* larb16a */
> > +[MT8188_SMI_RST_LARB17A]= { 0xA0, BIT(4) }, /* larb17a */
> > +};
> > +
> > +struct mtk_smi_larb_plat {
> > +const struct mtk_smi_larb_reset*reset_signal;
> > +const unsigned intlarb_reset_nr;
> > +};
> > +
> > +struct mtk_smi_reset_data {
> > +const struct mtk_smi_larb_plat *larb_plat;
> > +struct reset_controller_dev rcdev;
> > +struct regmap *regmap;
> > +};
> > +
> > +static const struct mtk_smi_larb_plat mtk_smi_larb_mt8188 = {
> > +.reset_signal = rst_signal_mt8188,
> > +.larb_reset_nr = ARRAY_SIZE(rst_signal_mt8188),
> > +};
> > +
> > +static int mtk_smi_larb_reset(struct reset_controller_dev *rcdev,
> unsigned long id)
> > +{
> > +struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> > +const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> > +const struct mtk_smi_larb_reset *larb_rst = larb_plat-
> >reset_signal + id;
> > +int ret;
> > +
> > +ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst-
> >value);
> > +if (ret)
> > +return ret;
> > +ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst-
> >value);
> > +
> > +return ret;
> > +}
> > +
> > +static int mtk_smi_larb_reset_assert(struct reset_controller_dev
> *rcdev, unsigned long id)
> > +{
> > +struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> > +const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> > +const struct mtk_smi_larb_reset *larb_rst = larb_plat-
> >reset_signal + id;
> > +int ret;
> > +
> > +ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst-
> >value);
> > +if (ret)
> > +dev_err(rcdev->dev, "[%s] Failed to shutdown larb %d\n", __func__,
> ret);
> > +
> > +return ret;
> > +}
> > +
> > +static int mtk_smi_larb_reset_deassert(struct reset_controller_dev
> *rcdev, unsigned long id)
> > +{
> > +struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
> > +const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
> > +const struct mtk_smi_larb_reset *larb_rst = larb_plat-
> >reset_signal + id;
> > +int ret;
> > +
> > +ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst-
> >value);
> > +if (ret)
> > +dev_err(rcdev->dev, "[%s] Failed to reopen larb %d\n", __func__,
> ret);
> > +
> > +return ret;
> > +}
> > +
> > +static const struct reset_control_ops mtk_smi_reset_ops = {
> > +.reset= mtk_smi_larb_reset,
> > +.assert= mtk_smi_larb_reset_assert,
> > +.deassert= mtk_smi_larb_reset_deassert,
> > +};
> > +
> > +static int mtk_smi_reset_probe(struct platform_device *pdev)
> > +{
> > +struct device *dev = &pdev->dev;
> > +const struct mtk_smi_larb_plat *larb_plat =
> of_device_get_match_data(dev);
> > +struct device_node *np = dev->of_node, *reset_node;
> > +struct mtk_smi_reset_data *data;
> > +struct regmap *regmap;
> > +
> > +data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +if (!data)
> > +return -ENOMEM;
> > +
> > +reset_node = of_parse_phandle(np, "mediatek,larb-rst-syscon", 0);
> > +if (!reset_node)
> 
> This looks just wrong. This looks like a child of whatever phandle
> points here.
> 
> Why do you create MMIO-using node as not MMIO?
> 

This is the definition for imgsys1_dip_top and imgsys1_dip_top_rst.
SMI reset controller driver parse the 'mediatek,larb-rst-syscon'
to get the 'imgsys1_dip_top' device node and regmap.
Then SMI driver could read and write the register by the regmap
to apply reset operations here.

	imgsys1_dip_top: clock-controller@15110000 {
		compatible = "mediatek,mt8188-imgsys1-dip-top";
		reg = <0 0x15110000 0 0x1000>;
		#clock-cells = <1>;
	};

	imgsys1_dip_top_rst: reset-controller0 {
		compatible = "mediatek,smi-reset-mt8188";
		#reset-cells = <1>;
		mediatek,larb-rst-syscon = <&imgsys1_dip_top>;
	};
	
	larb10: smi@15120000{
		resets = <&imgsys1_dip_top_rst MT8188_SMI_RST_LARB10>;
		reset-names = "larb_rst";
	};

I am not so sure the meaning "MMIO-using" here. 
Cureently I use regmap_set_bits and regmap_clear_bits to update the
larb reset register. These registers locate in each subsys and are used
by respective drivers. So SMI add 'imgsys1_dip_top_rst' node to get the
regmap to avoid affecting subsys driver.

From my point of view, there are two methods:
(1)use of_iomap to get MMIO register region, and use writel/readl to
wirte/read register.
(2)use device_node_regmap to get the regmap, and use regmap_set_bits
and regmap_clear_bits to set register

And you prefer option (1), is this correct?

What is more, you prefer to put the imgsys1_dip_top_rst node into the
imgsys1_dip_top node as a child node, like this?

	imgsys1_dip_top: clock-controller@15110000 {
		compatible = "mediatek,mt8188-imgsys1-dip-top";
		reg = <0 0x15110000 0 0x1000>;
		#clock-cells = <1>;

		imgsys1_dip_top_rst: reset-controller0 {
			compatible = "mediatek,smi-reset-mt8188";
			#reset-cells = <1>;
		};
	};

> Best regards,
> Krzysztof
> 

Re: [PATCH 2/4] dt-bindings: memory: mediatek: Add smi-sub-common property for reset

[Krzysztof Kozlowski]

On 24/10/2024 03:28, Friday Yang (杨阳) wrote:
> On Wed, 2024-08-21 at 10:55 +0200, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 21/08/2024 10:26, friday.yang wrote:
>>> On the MediaTek platform, some SMI LARBs are directly linked to SMI
>>> common. While some SMI LARBs are linked to SMI sub common, then SMI
>>> sub common is linked to SMI common. Add 'mediatek,smi-sub-comm' and
>>> 'mediatek,smi-sub-comm-in-portid' properties here. The SMI reset
>>> driver could query which port of the SMI sub common the current
>> LARB
>>> is linked to through the two properties. The hardware block diagram
>>> could be described as below.
>>>
>>>              SMI Common(Smart Multimedia Interface Common)
>>>                  |
>>>          +----------------+-------
>>>          |                |
>>>          |                |
>>>          |                |
>>>          |                |
>>>          |                |
>>>        larb0       SMI Sub Common
>>>                    |      |     |
>>>                   larb1  larb2 larb3
>>>
>>> Signed-off-by: friday.yang <friday.yang@mediatek.com>
>>> ---
>>>  .../mediatek,smi-common.yaml                  |  2 ++
>>>  .../memory-controllers/mediatek,smi-larb.yaml | 22
>> +++++++++++++++++++
>>>  2 files changed, 24 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-common.yaml
>> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
>> common.yaml
>>> index 2f36ac23604c..4392d349878c 100644
>>> --- a/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-common.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-common.yaml
>>> @@ -39,6 +39,7 @@ properties:
>>>            - mediatek,mt8186-smi-common
>>>            - mediatek,mt8188-smi-common-vdo
>>>            - mediatek,mt8188-smi-common-vpp
>>> +          - mediatek,mt8188-smi-sub-common
>>>            - mediatek,mt8192-smi-common
>>>            - mediatek,mt8195-smi-common-vdo
>>>            - mediatek,mt8195-smi-common-vpp
>>> @@ -107,6 +108,7 @@ allOf:
>>>          compatible:
>>>            contains:
>>>              enum:
>>> +              - mediatek,mt8188-smi-sub-common
>>>                - mediatek,mt8195-smi-sub-common
>>>      then:
>>>        required:
>>> diff --git a/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-larb.yaml
>> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
>> larb.yaml
>>> index 2381660b324c..5f162bb360db 100644
>>> --- a/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-larb.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-larb.yaml
>>> @@ -69,6 +69,16 @@ properties:
>>>      description: the hardware id of this larb. It's only required
>> when this
>>>        hardware id is not consecutive from its M4U point of view.
>>>  
>>> +  mediatek,smi-sub-comm:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: a phandle of smi_sub_common that the larb is
>> linked to.
>>
>> Why do you have to smi phandle properties per each node?
>>
> 
> As shown in the picture from the commit message, we have multipule smi-
> sub-common, each SMI larb may link to one of the smi-sub-common. So we
> need the 'mediatek,smi-sub-comm' to describe which smi-sub-common the
> larb is linked to.
> In next version, I will add two smi-sub-common to the diagram in the
> commit message.

You respond two months after... That email conversation is not even in
my mailbox anymore.

Anyway, sub-common are subnodes, no?

> 
>>> +
>>> +  mediatek,smi-sub-comm-in-portid:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 0
>>> +    maximum: 7
>>> +    description: which port of smi_sub_common that the larb is
>> linked to.
>>
>> Merge it into phandle.
>>
> 
> Just confirm,
> Do you mean merge these two into one property, like:
> mediatek,smi-sub-comm = <&phandle port-id>;

Yes


Best regards,
Krzysztof

Re: [PATCH 3/4] memory: mtk-smi: mt8188: Add SMI clamp function

[AngeloGioacchino Del Regno]

Il 24/10/24 03:29, Friday Yang (杨阳) ha scritto:
> On Wed, 2024-08-21 at 11:00 +0200, Krzysztof Kozlowski wrote:
>>   	
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>   On 21/08/2024 10:26, friday.yang wrote:
>>> In order to avoid handling glitch signal when MTCMOS on/off, SMI
>> need
>>> clamp and reset operation. Parse power reset settings for LARBs
>> which
>>> need to reset. Register genpd callback for SMI LARBs and apply
>> reset
>>> operations in the callback.
>>>
>>> Signed-off-by: friday.yang <friday.yang@mediatek.com>
>>> ---
>>>   drivers/memory/mtk-smi.c | 148
>> ++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 146 insertions(+), 2 deletions(-)
>>>
>>
>> ...
>>
>>> +
>>> +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb
>> *larb)
>>> +{
>>> +struct device_node *reset_node;
>>> +struct device *dev = larb->dev;
>>> +int ret;
>>> +
>>> +/* only larb with "resets" need to get reset setting */
>>> +reset_node = of_parse_phandle(dev->of_node, "resets", 0);
>>
>> Nope, you do not parse rasets.
> 
> 1.If I need to use the Linux reset control framework, 'resets' is the
> required property.

Leave that to the reset API, don't manually parse the resets phandle here,
that's what Krzysztof was meaning.

> 2.'reset-names' give the list of reset signal name strings sorted in
> the same order as the 'resets' property. SMI driver will use 'reset-
> names' to match reset signal names with reset specifiers.
> 3.Not all SMI larbs need to apply reset operations, only SMI larbs
> which may have bus glitch issues need this. Just to confirm if this
> larb support reset function.
> 
>>
>>> +if (!reset_node)
>>> +return 0;
>>> +of_node_put(reset_node);
>>> +
>>> +larb->rst_con = devm_reset_control_get(dev, "larb_rst");

"larb" is just fine as a name: it's clear that this is a reset, as
it's specified as `reset-names = "name"`.

>>
>> Where are the bindings? Why do you add undocumented properties? How
>> possible this passes dtbs_check???
>>
> 
> This is not the new added property in SMI larb DT node.
> It is the reset signal name which is inclued in 'reset-names'.
> Just like this:
> 
> resets = <&imgsys1_dip_nr_rst MT8188_SIM_RST_LARB15>;
> reset-name = 'larb_rst';
> 
> Maybe I can add this name to the 'reset-name' property binding
> description, like this, is this clear for you?
> 
> reset-name:

It's "reset-names" btw.

>      const: larb_rst
>      description: the name of reset signal. SMI driver need to obtain 		
>   the reset controller based on this.
> 
>>
>>> +if (IS_ERR(larb->rst_con))
>>> +return dev_err_probe(dev, PTR_ERR(larb->rst_con),
>>> +     "cannot get larb reset controller\n");
>>> +
>>> +larb->nb.notifier_call = mtk_smi_genpd_callback;
>>> +ret = dev_pm_genpd_add_notifier(dev, &larb->nb);
>>> +if (ret) {
>>> +dev_err(dev, "Failed to add genpd callback %d\n", ret);
>>> +return -EINVAL;
>>> +}
>>> +
>>> +return 0;
>>> +}
>>> +
>>>   static int mtk_smi_larb_probe(struct platform_device *pdev)
>>>   {
>>>   struct mtk_smi_larb *larb;
>>> @@ -538,6 +662,7 @@ static int mtk_smi_larb_probe(struct
>> platform_device *pdev)
>>>   if (!larb)
>>>   return -ENOMEM;
>>>   
>>> +larb->dev = dev;
>>>   larb->larb_gen = of_device_get_match_data(dev);
>>>   larb->base = devm_platform_ioremap_resource(pdev, 0);
>>>   if (IS_ERR(larb->base))
>>> @@ -554,15 +679,29 @@ static int mtk_smi_larb_probe(struct
>> platform_device *pdev)
>>>   if (ret < 0)
>>>   return ret;
>>>   
>>> -pm_runtime_enable(dev);
>>> +/* find sub common to clamp larb for ISP software reset */
>>> +ret = mtk_smi_larb_parse_clamp_info(larb);
>>> +if (ret) {
>>> +dev_err(dev, "Failed to get clamp setting for larb\n");
>>> +goto err_pm_disable;
>>> +}
>>> +
>>> +ret = mtk_smi_larb_parse_reset_info(larb);
>>> +if (ret) {
>>> +dev_err(dev, "Failed to get power setting for larb\n");
>>> +goto err_pm_disable;
>>> +}
>>> +
>>>   platform_set_drvdata(pdev, larb);
>>>   ret = component_add(dev, &mtk_smi_larb_component_ops);
>>>   if (ret)
>>>   goto err_pm_disable;
>>> +
>>> +pm_runtime_enable(dev);
>>> +
>>>   return 0;
>>>   
>>>   err_pm_disable:
>>> -pm_runtime_disable(dev);
>>>   device_link_remove(dev, larb->smi_common_dev);
>>
>> Label asls pm disable. Where is the pm disable?
>>
> 
> Thanks, I will fix it to 'err_link_remove'.
> 

...or you can just use devm_pm_runtime_enable() instead, and not worry
about disabling it on your own.

Regards,
Angelo

Re: [PATCH 2/4] dt-bindings: memory: mediatek: Add smi-sub-common property for reset

[AngeloGioacchino Del Regno]

Il 24/10/24 03:28, Friday Yang (杨阳) ha scritto:
> On Wed, 2024-08-21 at 10:55 +0200, Krzysztof Kozlowski wrote:
>>   	
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>   On 21/08/2024 10:26, friday.yang wrote:
>>> On the MediaTek platform, some SMI LARBs are directly linked to SMI
>>> common. While some SMI LARBs are linked to SMI sub common, then SMI
>>> sub common is linked to SMI common. Add 'mediatek,smi-sub-comm' and
>>> 'mediatek,smi-sub-comm-in-portid' properties here. The SMI reset
>>> driver could query which port of the SMI sub common the current
>> LARB
>>> is linked to through the two properties. The hardware block diagram
>>> could be described as below.
>>>
>>>               SMI Common(Smart Multimedia Interface Common)
>>>                   |
>>>           +----------------+-------
>>>           |                |
>>>           |                |
>>>           |                |
>>>           |                |
>>>           |                |
>>>         larb0       SMI Sub Common
>>>                     |      |     |
>>>                    larb1  larb2 larb3
>>>
>>> Signed-off-by: friday.yang <friday.yang@mediatek.com>
>>> ---
>>>   .../mediatek,smi-common.yaml                  |  2 ++
>>>   .../memory-controllers/mediatek,smi-larb.yaml | 22
>> +++++++++++++++++++
>>>   2 files changed, 24 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-common.yaml
>> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
>> common.yaml
>>> index 2f36ac23604c..4392d349878c 100644
>>> --- a/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-common.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-common.yaml
>>> @@ -39,6 +39,7 @@ properties:
>>>             - mediatek,mt8186-smi-common
>>>             - mediatek,mt8188-smi-common-vdo
>>>             - mediatek,mt8188-smi-common-vpp
>>> +          - mediatek,mt8188-smi-sub-common
>>>             - mediatek,mt8192-smi-common
>>>             - mediatek,mt8195-smi-common-vdo
>>>             - mediatek,mt8195-smi-common-vpp
>>> @@ -107,6 +108,7 @@ allOf:
>>>           compatible:
>>>             contains:
>>>               enum:
>>> +              - mediatek,mt8188-smi-sub-common
>>>                 - mediatek,mt8195-smi-sub-common
>>>       then:
>>>         required:
>>> diff --git a/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-larb.yaml
>> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
>> larb.yaml
>>> index 2381660b324c..5f162bb360db 100644
>>> --- a/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-larb.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-
>> controllers/mediatek,smi-larb.yaml
>>> @@ -69,6 +69,16 @@ properties:
>>>       description: the hardware id of this larb. It's only required
>> when this
>>>         hardware id is not consecutive from its M4U point of view.
>>>   
>>> +  mediatek,smi-sub-comm:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: a phandle of smi_sub_common that the larb is
>> linked to.
>>
>> Why do you have to smi phandle properties per each node?
>>
> 
> As shown in the picture from the commit message, we have multipule smi-
> sub-common, each SMI larb may link to one of the smi-sub-common. So we
> need the 'mediatek,smi-sub-comm' to describe which smi-sub-common the
> larb is linked to.
> In next version, I will add two smi-sub-common to the diagram in the
> commit message.
> 
>>> +
>>> +  mediatek,smi-sub-comm-in-portid:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 0
>>> +    maximum: 7
>>> +    description: which port of smi_sub_common that the larb is
>> linked to.
>>
>> Merge it into phandle.
>>
> 
> Just confirm,
> Do you mean merge these two into one property, like:
> mediatek,smi-sub-comm = <&phandle port-id>;
> 
>>> +
>>>   required:
>>>     - compatible
>>>     - reg
>>> @@ -125,6 +135,18 @@ allOf:
>>>         required:
>>>           - mediatek,larb-id
>>>   
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - mediatek,mt8188-smi-larb
>>> +
>>> +    then:
>>> +      required:
>>> +        - mediatek,smi-sub-comm
>>> +        - mediatek,smi-sub-comm-in-portid
>>> +
>>
>> and add it to the example (since you claim it is valid for every
>> device).
>>

It's valid only for the Local Arbiters that have a sub-common port, which anyway
are only the ones that are used by CAMSYS if I'm not wrong....

Regardless of that, not all of the mt8188-smi-larb *require* smi-sub-comm.

Besides, if the larb is anyway already linked to a sub-common, can't we just grab
that from walking back?
Or is this property's purpose to actually add a link to a sub-common?

Regards,
Angelo

> 
> OK, I will add this to the example.
> 
>> Best regards,
>> Krzysztof
>>

Re: [PATCH 2/4] dt-bindings: memory: mediatek: Add smi-sub-common property for reset

[Friday Yang (杨阳)]

On Thu, 2024-10-24 at 08:38 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 24/10/2024 03:28, Friday Yang (杨阳) wrote:
> > On Wed, 2024-08-21 at 10:55 +0200, Krzysztof Kozlowski wrote:
> >>   
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>  On 21/08/2024 10:26, friday.yang wrote:
> >>> On the MediaTek platform, some SMI LARBs are directly linked to
> SMI
> >>> common. While some SMI LARBs are linked to SMI sub common, then
> SMI
> >>> sub common is linked to SMI common. Add 'mediatek,smi-sub-comm'
> and
> >>> 'mediatek,smi-sub-comm-in-portid' properties here. The SMI reset
> >>> driver could query which port of the SMI sub common the current
> >> LARB
> >>> is linked to through the two properties. The hardware block
> diagram
> >>> could be described as below.
> >>>
> >>>              SMI Common(Smart Multimedia Interface Common)
> >>>                  |
> >>>          +----------------+-------
> >>>          |                |
> >>>          |                |
> >>>          |                |
> >>>          |                |
> >>>          |                |
> >>>        larb0       SMI Sub Common
> >>>                    |      |     |
> >>>                   larb1  larb2 larb3
> >>>
> >>> Signed-off-by: friday.yang <friday.yang@mediatek.com>
> >>> ---
> >>>  .../mediatek,smi-common.yaml                  |  2 ++
> >>>  .../memory-controllers/mediatek,smi-larb.yaml | 22
> >> +++++++++++++++++++
> >>>  2 files changed, 24 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/memory-
> >> controllers/mediatek,smi-common.yaml
> >> b/Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-
> >> common.yaml
> >>> index 2f36ac23604c..4392d349878c 100644
> >>> --- a/Documentation/devicetree/bindings/memory-
> >> controllers/mediatek,smi-common.yaml
> >>> +++ b/Documentation/devicetree/bindings/memory-
> >> controllers/mediatek,smi-common.yaml
> >>> @@ -39,6 +39,7 @@ properties:
> >>>            - mediatek,mt8186-smi-common
> >>>            - mediatek,mt8188-smi-common-vdo
> >>>            - mediatek,mt8188-smi-common-vpp
> >>> +          - mediatek,mt8188-smi-sub-common
> >>>            - mediatek,mt8192-smi-common
> >>>            - mediatek,mt8195-smi-common-vdo
> >>>            - mediatek,mt8195-smi-common-vpp
> >>> @@ -107,6 +108,7 @@ allOf:
> >>>          compatible:
> >>>            contains:
> >>>              enum:
> >>> +              - mediatek,mt8188-smi-sub-common
> >>>                - mediatek,mt8195-smi-sub-common
> >>>      then:
> >>>        required:
> >>> diff --git a/Documentation/devicetree/bindings/memory-
> >> controllers/mediatek,smi-larb.yaml
> >> b/Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-
> >> larb.yaml
> >>> index 2381660b324c..5f162bb360db 100644
> >>> --- a/Documentation/devicetree/bindings/memory-
> >> controllers/mediatek,smi-larb.yaml
> >>> +++ b/Documentation/devicetree/bindings/memory-
> >> controllers/mediatek,smi-larb.yaml
> >>> @@ -69,6 +69,16 @@ properties:
> >>>      description: the hardware id of this larb. It's only
> required
> >> when this
> >>>        hardware id is not consecutive from its M4U point of view.
> >>>  
> >>> +  mediatek,smi-sub-comm:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: a phandle of smi_sub_common that the larb is
> >> linked to.
> >>
> >> Why do you have to smi phandle properties per each node?
> >>
> > 
> > As shown in the picture from the commit message, we have multipule
> smi-
> > sub-common, each SMI larb may link to one of the smi-sub-common. So
> we
> > need the 'mediatek,smi-sub-comm' to describe which smi-sub-common
> the
> > larb is linked to.
> > In next version, I will add two smi-sub-common to the diagram in
> the
> > commit message.
> 
> You respond two months after... That email conversation is not even
> in
> my mailbox anymore.
> 
> Anyway, sub-common are subnodes, no?
> 


                         IOMMU
                           |
                     SMI Common(Smart Multimedia Interface Common)
                           |
          +----------------+------------------------+
          |                |                        |
          |                |                        |
          |                |                        |
          |                |                        |
          |                |                        |
       larb0       SMI-Sub-Common0          SMI-Sub-Common1
                   |       |     |          |             |
                  larb2   larb4  larb5   larb1         larb7

This is the new version diagram.
In MediaTek platform, some larbs are linked to SMI Common, while some
other larbs are linked to smi-sub-common. Different platforms have
different hardware structures.

In the device tree, smi sub common is not a child node, but is at the
same level as smi common. 'sub' is just a internal name rule.
And 'mediatek,smi' is used to describe this connection. 
Example as below...

> > 
> >>> +
> >>> +  mediatek,smi-sub-comm-in-portid:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    minimum: 0
> >>> +    maximum: 7
> >>> +    description: which port of smi_sub_common that the larb is
> >> linked to.
> >>
> >> Merge it into phandle.
> >>
> > 
> > Just confirm,
> > Do you mean merge these two into one property, like:
> > mediatek,smi-sub-comm = <&phandle port-id>;
> 
> Yes
> 

We will remove 'mediatek,smi-sub-comm', which should be redundant.
Merge this 'mediatek,smi-sub-comm-in-portid' into 'mediatek,smi', is
this OK for you?

		smi_common_vdo: smi@1c024000 {
			compatible = "mediatek,mt8188-smi-common-vdo";
			reg = <0 0x1c024000 0 0x1000>;
			...
		};
		
		smi_sub_common_img0_4x1: smi@15002000 {
			compatible = "mediatek,mt8188-smi-sub-common";
			reg = <0 0x15002000 0 0x1000>;
			mediatek,smi = <&smi_common_vpp 5>;
			....
		};

		larb10: larb@15120000 {
			compatible = "mediatek,mt8188-smi-larb";
			reg = <0 0x15120000 0 0x1000>;
			mediatek,smi = <&smi_sub_common_img0_4x1 1>;
			...
		};

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/4] dt-bindings: memory: mediatek: Add smi-sub-common property for reset

[Friday Yang (杨阳)]

On Thu, 2024-10-24 at 13:59 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/10/24 03:28, Friday Yang (杨阳) ha scritto:
> > On Wed, 2024-08-21 at 10:55 +0200, Krzysztof Kozlowski wrote:
> > >   	
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >   On 21/08/2024 10:26, friday.yang wrote:
> > > > On the MediaTek platform, some SMI LARBs are directly linked to
> > > > SMI
> > > > common. While some SMI LARBs are linked to SMI sub common, then
> > > > SMI
> > > > sub common is linked to SMI common. Add 'mediatek,smi-sub-comm' 
> > > > and
> > > > 'mediatek,smi-sub-comm-in-portid' properties here. The SMI
> > > > reset
> > > > driver could query which port of the SMI sub common the current
> > > 
> > > LARB
> > > > is linked to through the two properties. The hardware block
> > > > diagram
> > > > could be described as below.
> > > > 
> > > >               SMI Common(Smart Multimedia Interface Common)
> > > >                   |
> > > >           +----------------+-------
> > > >           |                |
> > > >           |                |
> > > >           |                |
> > > >           |                |
> > > >           |                |
> > > >         larb0       SMI Sub Common
> > > >                     |      |     |
> > > >                    larb1  larb2 larb3
> > > > 
> > > > Signed-off-by: friday.yang <friday.yang@mediatek.com>
> > > > ---
> > > >   .../mediatek,smi-common.yaml                  |  2 ++
> > > >   .../memory-controllers/mediatek,smi-larb.yaml | 22
> > > 
> > > +++++++++++++++++++
> > > >   2 files changed, 24 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/memory-
> > > 
> > > controllers/mediatek,smi-common.yaml
> > > b/Documentation/devicetree/bindings/memory-
> > > controllers/mediatek,smi-
> > > common.yaml
> > > > index 2f36ac23604c..4392d349878c 100644
> > > > --- a/Documentation/devicetree/bindings/memory-
> > > 
> > > controllers/mediatek,smi-common.yaml
> > > > +++ b/Documentation/devicetree/bindings/memory-
> > > 
> > > controllers/mediatek,smi-common.yaml
> > > > @@ -39,6 +39,7 @@ properties:
> > > >             - mediatek,mt8186-smi-common
> > > >             - mediatek,mt8188-smi-common-vdo
> > > >             - mediatek,mt8188-smi-common-vpp
> > > > +          - mediatek,mt8188-smi-sub-common
> > > >             - mediatek,mt8192-smi-common
> > > >             - mediatek,mt8195-smi-common-vdo
> > > >             - mediatek,mt8195-smi-common-vpp
> > > > @@ -107,6 +108,7 @@ allOf:
> > > >           compatible:
> > > >             contains:
> > > >               enum:
> > > > +              - mediatek,mt8188-smi-sub-common
> > > >                 - mediatek,mt8195-smi-sub-common
> > > >       then:
> > > >         required:
> > > > diff --git a/Documentation/devicetree/bindings/memory-
> > > 
> > > controllers/mediatek,smi-larb.yaml
> > > b/Documentation/devicetree/bindings/memory-
> > > controllers/mediatek,smi-
> > > larb.yaml
> > > > index 2381660b324c..5f162bb360db 100644
> > > > --- a/Documentation/devicetree/bindings/memory-
> > > 
> > > controllers/mediatek,smi-larb.yaml
> > > > +++ b/Documentation/devicetree/bindings/memory-
> > > 
> > > controllers/mediatek,smi-larb.yaml
> > > > @@ -69,6 +69,16 @@ properties:
> > > >       description: the hardware id of this larb. It's only
> > > > required
> > > 
> > > when this
> > > >         hardware id is not consecutive from its M4U point of
> > > > view.
> > > >   
> > > > +  mediatek,smi-sub-comm:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description: a phandle of smi_sub_common that the larb is
> > > 
> > > linked to.
> > > 
> > > Why do you have to smi phandle properties per each node?
> > > 
> > 
> > As shown in the picture from the commit message, we have multipule
> > smi-
> > sub-common, each SMI larb may link to one of the smi-sub-common. So
> > we
> > need the 'mediatek,smi-sub-comm' to describe which smi-sub-common
> > the
> > larb is linked to.
> > In next version, I will add two smi-sub-common to the diagram in
> > the
> > commit message.
> > 
> > > > +
> > > > +  mediatek,smi-sub-comm-in-portid:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    minimum: 0
> > > > +    maximum: 7
> > > > +    description: which port of smi_sub_common that the larb is
> > > 
> > > linked to.
> > > 
> > > Merge it into phandle.
> > > 
> > 
> > Just confirm,
> > Do you mean merge these two into one property, like:
> > mediatek,smi-sub-comm = <&phandle port-id>;
> > 
> > > > +
> > > >   required:
> > > >     - compatible
> > > >     - reg
> > > > @@ -125,6 +135,18 @@ allOf:
> > > >         required:
> > > >           - mediatek,larb-id
> > > >   
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            enum:
> > > > +              - mediatek,mt8188-smi-larb
> > > > +
> > > > +    then:
> > > > +      required:
> > > > +        - mediatek,smi-sub-comm
> > > > +        - mediatek,smi-sub-comm-in-portid
> > > > +
> > > 
> > > and add it to the example (since you claim it is valid for every
> > > device).
> > > 
> 
> It's valid only for the Local Arbiters that have a sub-common port,
> which anyway
> are only the ones that are used by CAMSYS if I'm not wrong....
> 
> Regardless of that, not all of the mt8188-smi-larb *require* smi-sub-
> comm.
> 
> Besides, if the larb is anyway already linked to a sub-common, can't
> we just grab
> that from walking back?
> Or is this property's purpose to actually add a link to a sub-common?
> 
> Regards,
> Angelo
> 
> > 
> > OK, I will add this to the example.
> > 
> > > Best regards,
> > > Krzysztof
> > > 
> 

Not only camerasys has smi-sub-common, other subsys also has smi-sub-
common. And this property's purpose is to add a link to a sub-common,
this is right.
We will remove 'mediatek,smi-sub-comm', just use 'mediatek,smi'.
Merge 'mediatek,smi-sub-comm-in-portid' into 'mediatek,smi', SMI
driver should be changed to adapt this. Is this ok for you?

> 

Re: [PATCH 3/4] memory: mtk-smi: mt8188: Add SMI clamp function

[Friday Yang (杨阳)]

On Thu, 2024-10-24 at 13:56 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/10/24 03:29, Friday Yang (杨阳) ha scritto:
> > On Wed, 2024-08-21 at 11:00 +0200, Krzysztof Kozlowski wrote:
> > >   	
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >   On 21/08/2024 10:26, friday.yang wrote:
> > > > In order to avoid handling glitch signal when MTCMOS on/off,
> > > > SMI
> > > 
> > > need
> > > > clamp and reset operation. Parse power reset settings for LARBs
> > > 
> > > which
> > > > need to reset. Register genpd callback for SMI LARBs and apply
> > > 
> > > reset
> > > > operations in the callback.
> > > > 
> > > > Signed-off-by: friday.yang <friday.yang@mediatek.com>
> > > > ---
> > > >   drivers/memory/mtk-smi.c | 148
> > > 
> > > ++++++++++++++++++++++++++++++++++++++-
> > > >   1 file changed, 146 insertions(+), 2 deletions(-)
> > > > 
> > > 
> > > ...
> > > 
> > > > +
> > > > +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb
> > > 
> > > *larb)
> > > > +{
> > > > +struct device_node *reset_node;
> > > > +struct device *dev = larb->dev;
> > > > +int ret;
> > > > +
> > > > +/* only larb with "resets" need to get reset setting */
> > > > +reset_node = of_parse_phandle(dev->of_node, "resets", 0);
> > > 
> > > Nope, you do not parse rasets.
> > 
> > 1.If I need to use the Linux reset control framework, 'resets' is
> > the
> > required property.
> 
> Leave that to the reset API, don't manually parse the resets phandle
> here,
> that's what Krzysztof was meaning.

OK, I will just use 'larb->rst_con' to judge whether this larb need
reset or not, not parse 'resets' here.

> 
> > 2.'reset-names' give the list of reset signal name strings sorted
> > in
> > the same order as the 'resets' property. SMI driver will use
> > 'reset-
> > names' to match reset signal names with reset specifiers.
> > 3.Not all SMI larbs need to apply reset operations, only SMI larbs
> > which may have bus glitch issues need this. Just to confirm if this
> > larb support reset function.
> > 
> > > 
> > > > +if (!reset_node)
> > > > +return 0;
> > > > +of_node_put(reset_node);
> > > > +
> > > > +larb->rst_con = devm_reset_control_get(dev, "larb_rst");
> 
> "larb" is just fine as a name: it's clear that this is a reset, as
> it's specified as `reset-names = "name"`.

Thanks, I will fix it to 'larb'.

> 
> > > 
> > > Where are the bindings? Why do you add undocumented properties?
> > > How
> > > possible this passes dtbs_check???
> > > 
> > 
> > This is not the new added property in SMI larb DT node.
> > It is the reset signal name which is inclued in 'reset-names'.
> > Just like this:
> > 
> > resets = <&imgsys1_dip_nr_rst MT8188_SIM_RST_LARB15>;
> > reset-name = 'larb_rst';
> > 
> > Maybe I can add this name to the 'reset-name' property binding
> > description, like this, is this clear for you?
> > 
> > reset-name:
> 
> It's "reset-names" btw.

Yes, you are right.

> 
> >      const: larb_rst
> >      description: the name of reset signal. SMI driver need to
> > obtain 		
> >   the reset controller based on this.
> > 
> > > 
> > > > +if (IS_ERR(larb->rst_con))
> > > > +return dev_err_probe(dev, PTR_ERR(larb->rst_con),
> > > > +     "cannot get larb reset controller\n");
> > > > +
> > > > +larb->nb.notifier_call = mtk_smi_genpd_callback;
> > > > +ret = dev_pm_genpd_add_notifier(dev, &larb->nb);
> > > > +if (ret) {
> > > > +dev_err(dev, "Failed to add genpd callback %d\n", ret);
> > > > +return -EINVAL;
> > > > +}
> > > > +
> > > > +return 0;
> > > > +}
> > > > +
> > > >   static int mtk_smi_larb_probe(struct platform_device *pdev)
> > > >   {
> > > >   struct mtk_smi_larb *larb;
> > > > @@ -538,6 +662,7 @@ static int mtk_smi_larb_probe(struct
> > > 
> > > platform_device *pdev)
> > > >   if (!larb)
> > > >   return -ENOMEM;
> > > >   
> > > > +larb->dev = dev;
> > > >   larb->larb_gen = of_device_get_match_data(dev);
> > > >   larb->base = devm_platform_ioremap_resource(pdev, 0);
> > > >   if (IS_ERR(larb->base))
> > > > @@ -554,15 +679,29 @@ static int mtk_smi_larb_probe(struct
> > > 
> > > platform_device *pdev)
> > > >   if (ret < 0)
> > > >   return ret;
> > > >   
> > > > -pm_runtime_enable(dev);
> > > > +/* find sub common to clamp larb for ISP software reset */
> > > > +ret = mtk_smi_larb_parse_clamp_info(larb);
> > > > +if (ret) {
> > > > +dev_err(dev, "Failed to get clamp setting for larb\n");
> > > > +goto err_pm_disable;
> > > > +}
> > > > +
> > > > +ret = mtk_smi_larb_parse_reset_info(larb);
> > > > +if (ret) {
> > > > +dev_err(dev, "Failed to get power setting for larb\n");
> > > > +goto err_pm_disable;
> > > > +}
> > > > +
> > > >   platform_set_drvdata(pdev, larb);
> > > >   ret = component_add(dev, &mtk_smi_larb_component_ops);
> > > >   if (ret)
> > > >   goto err_pm_disable;
> > > > +
> > > > +pm_runtime_enable(dev);
> > > > +
> > > >   return 0;
> > > >   
> > > >   err_pm_disable:
> > > > -pm_runtime_disable(dev);
> > > >   device_link_remove(dev, larb->smi_common_dev);
> > > 
> > > Label asls pm disable. Where is the pm disable?
> > > 
> > 
> > Thanks, I will fix it to 'err_link_remove'.
> > 
> 
> ...or you can just use devm_pm_runtime_enable() instead, and not
> worry
> about disabling it on your own.
> 

This is a good choice, but in this smi clamp patchset, I will just
modify the label as 'err_link_remove'.

> Regards,
> Angelo

Re: [PATCH 3/4] memory: mtk-smi: mt8188: Add SMI clamp function

[Krzysztof Kozlowski]

On 24/10/2024 03:29, Friday Yang (杨阳) wrote:
> On Wed, 2024-08-21 at 11:00 +0200, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 21/08/2024 10:26, friday.yang wrote:
>>> In order to avoid handling glitch signal when MTCMOS on/off, SMI
>> need
>>> clamp and reset operation. Parse power reset settings for LARBs
>> which
>>> need to reset. Register genpd callback for SMI LARBs and apply
>> reset
>>> operations in the callback.
>>>
>>> Signed-off-by: friday.yang <friday.yang@mediatek.com>
>>> ---
>>>  drivers/memory/mtk-smi.c | 148
>> ++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 146 insertions(+), 2 deletions(-)
>>>
>>
>> ...
>>
>>> +
>>> +static int mtk_smi_larb_parse_reset_info(struct mtk_smi_larb
>> *larb)
>>> +{
>>> +struct device_node *reset_node;
>>> +struct device *dev = larb->dev;
>>> +int ret;
>>> +
>>> +/* only larb with "resets" need to get reset setting */
>>> +reset_node = of_parse_phandle(dev->of_node, "resets", 0);
>>
>> Nope, you do not parse rasets.
> 
> 1.If I need to use the Linux reset control framework, 'resets' is the
> required property.

And you never parse it directly. Find me existing examples of this, if
you disagree.

> 2.'reset-names' give the list of reset signal name strings sorted in
> the same order as the 'resets' property. SMI driver will use 'reset-
> names' to match reset signal names with reset specifiers.

?

> 3.Not all SMI larbs need to apply reset operations, only SMI larbs
> which may have bus glitch issues need this. Just to confirm if this
> larb support reset function.

?

Really, read kernel API about reset framework first.

> 
>>
>>> +if (!reset_node)
>>> +return 0;
>>> +of_node_put(reset_node);
>>> +
>>> +larb->rst_con = devm_reset_control_get(dev, "larb_rst");
>>
>> Where are the bindings? Why do you add undocumented properties? How
>> possible this passes dtbs_check???
>>
> 
> This is not the new added property in SMI larb DT node.
> It is the reset signal name which is inclued in 'reset-names'.
> Just like this:

$ git grep larb_rst
No such property, so how this could be "not the new added"?

If you keep responding with some random or irrelevant responses, this
won't work. This is really poor way to upstream. I suggest first perform
extensive internal review which would point out all such trivial issues.



Best regards,
Krzysztof

Re: [PATCH 4/4] reset: mediatek: Add reset control driver for SMI

[Krzysztof Kozlowski]

On 24/10/2024 03:29, Friday Yang (杨阳) wrote:
> On Wed, 2024-08-21 at 10:58 +0200, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 21/08/2024 10:26, friday.yang wrote:
>>> Add a reset-controller driver for performing reset management of
>>> SMI LARBs on MediaTek platform. This driver uses the regmap
>>> frameworks to actually implement the various reset functions
>>> needed when SMI LARBs apply clamp operations.
>>
>> How does this depend on memory controller patches? Why is this
>> grouped
>> in one patchset?
>>
> 
> How about changing it like this,
> patchset1:
> (1)SMI reset control driver
> (2)SMI reset bindings
> patchset2
> (1)SMI driver
> (2)SMI bindings
> 
>>>
>>> Signed-off-by: friday.yang <friday.yang@mediatek.com>
>>> ---
>>>  drivers/reset/Kconfig              |   9 ++
>>>  drivers/reset/Makefile             |   1 +
>>>  drivers/reset/reset-mediatek-smi.c | 152
>> +++++++++++++++++++++++++++++
>>>  3 files changed, 162 insertions(+)
>>>  create mode 100644 drivers/reset/reset-mediatek-smi.c
>>>
>>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>>> index 67bce340a87e..e984a5a332f1 100644
>>> --- a/drivers/reset/Kconfig
>>> +++ b/drivers/reset/Kconfig
>>> @@ -154,6 +154,15 @@ config RESET_MESON_AUDIO_ARB
>>>    This enables the reset driver for Audio Memory Arbiter of
>>>    Amlogic's A113 based SoCs
>>>  
>>> +config RESET_MTK_SMI
>>> +bool "MediaTek SMI Reset Driver"
>>> +depends on MTK_SMI
>>
>> compile test
> 
> Thanks, I will fix it to 'depends on MTK_SMI || COMPILE_TEST'
> 
>>
>>> +help
>>> +  This option enables the reset controller driver for MediaTek
>> SMI.
>>> +  This reset driver is responsible for managing the reset signals
>>> +  for SMI larbs. Say Y if you want to control reset signals for
>>> +  MediaTek SMI larbs. Otherwise, say N.
>>> +
>>>  config RESET_NPCM
>>>  bool "NPCM BMC Reset Driver" if COMPILE_TEST
>>>  default ARCH_NPCM
>>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>>> index 27b0bbdfcc04..241777485b40 100644
>>> --- a/drivers/reset/Makefile
>>> +++ b/drivers/reset/Makefile
>>> @@ -22,6 +22,7 @@ obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
>>>  obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
>>>  obj-$(CONFIG_RESET_MESON) += reset-meson.o
>>>  obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
>>> +obj-$(CONFIG_RESET_MTK_SMI) += reset-mediatek-smi.o
>>>  obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
>>>  obj-$(CONFIG_RESET_NUVOTON_MA35D1) += reset-ma35d1.o
>>>  obj-$(CONFIG_RESET_PISTACHIO) += reset-pistachio.o
>>> diff --git a/drivers/reset/reset-mediatek-smi.c
>> b/drivers/reset/reset-mediatek-smi.c
>>> new file mode 100644
>>> index 000000000000..ead747e80ad5
>>> --- /dev/null
>>> +++ b/drivers/reset/reset-mediatek-smi.c
>>> @@ -0,0 +1,152 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Reset driver for MediaTek SMI module
>>> + *
>>> + * Copyright (C) 2024 MediaTek Inc.
>>> + */
>>> +
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/reset-controller.h>
>>> +
>>> +#include <dt-bindings/reset/mt8188-resets.h>
>>> +
>>> +#define to_mtk_smi_reset_data(_rcdev)\
>>> +container_of(_rcdev, struct mtk_smi_reset_data, rcdev)
>>> +
>>> +struct mtk_smi_larb_reset {
>>> +unsigned int offset;
>>> +unsigned int value;
>>> +};
>>> +
>>> +static const struct mtk_smi_larb_reset rst_signal_mt8188[] = {
>>> +[MT8188_SMI_RST_LARB10]= { 0xC, BIT(0) }, /* larb10 */
>>> +[MT8188_SMI_RST_LARB11A]= { 0xC, BIT(0) }, /* larb11a */
>>> +[MT8188_SMI_RST_LARB11C]= { 0xC, BIT(0) }, /* larb11c */
>>> +[MT8188_SMI_RST_LARB12]= { 0xC, BIT(8) }, /* larb12 */
>>> +[MT8188_SMI_RST_LARB11B]= { 0xC, BIT(0) }, /* larb11b */
>>> +[MT8188_SMI_RST_LARB15]= { 0xC, BIT(0) }, /* larb15 */
>>> +[MT8188_SMI_RST_LARB16B]= { 0xA0, BIT(4) }, /* larb16b */
>>> +[MT8188_SMI_RST_LARB17B]= { 0xA0, BIT(4) }, /* larb17b */
>>> +[MT8188_SMI_RST_LARB16A]= { 0xA0, BIT(4) }, /* larb16a */
>>> +[MT8188_SMI_RST_LARB17A]= { 0xA0, BIT(4) }, /* larb17a */
>>> +};
>>> +
>>> +struct mtk_smi_larb_plat {
>>> +const struct mtk_smi_larb_reset*reset_signal;
>>> +const unsigned intlarb_reset_nr;
>>> +};
>>> +
>>> +struct mtk_smi_reset_data {
>>> +const struct mtk_smi_larb_plat *larb_plat;
>>> +struct reset_controller_dev rcdev;
>>> +struct regmap *regmap;
>>> +};
>>> +
>>> +static const struct mtk_smi_larb_plat mtk_smi_larb_mt8188 = {
>>> +.reset_signal = rst_signal_mt8188,
>>> +.larb_reset_nr = ARRAY_SIZE(rst_signal_mt8188),
>>> +};
>>> +
>>> +static int mtk_smi_larb_reset(struct reset_controller_dev *rcdev,
>> unsigned long id)
>>> +{
>>> +struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
>>> +const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
>>> +const struct mtk_smi_larb_reset *larb_rst = larb_plat-
>>> reset_signal + id;
>>> +int ret;
>>> +
>>> +ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst-
>>> value);
>>> +if (ret)
>>> +return ret;
>>> +ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst-
>>> value);
>>> +
>>> +return ret;
>>> +}
>>> +
>>> +static int mtk_smi_larb_reset_assert(struct reset_controller_dev
>> *rcdev, unsigned long id)
>>> +{
>>> +struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
>>> +const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
>>> +const struct mtk_smi_larb_reset *larb_rst = larb_plat-
>>> reset_signal + id;
>>> +int ret;
>>> +
>>> +ret = regmap_set_bits(data->regmap, larb_rst->offset, larb_rst-
>>> value);
>>> +if (ret)
>>> +dev_err(rcdev->dev, "[%s] Failed to shutdown larb %d\n", __func__,
>> ret);
>>> +
>>> +return ret;
>>> +}
>>> +
>>> +static int mtk_smi_larb_reset_deassert(struct reset_controller_dev
>> *rcdev, unsigned long id)
>>> +{
>>> +struct mtk_smi_reset_data *data = to_mtk_smi_reset_data(rcdev);
>>> +const struct mtk_smi_larb_plat *larb_plat = data->larb_plat;
>>> +const struct mtk_smi_larb_reset *larb_rst = larb_plat-
>>> reset_signal + id;
>>> +int ret;
>>> +
>>> +ret = regmap_clear_bits(data->regmap, larb_rst->offset, larb_rst-
>>> value);
>>> +if (ret)
>>> +dev_err(rcdev->dev, "[%s] Failed to reopen larb %d\n", __func__,
>> ret);
>>> +
>>> +return ret;
>>> +}
>>> +
>>> +static const struct reset_control_ops mtk_smi_reset_ops = {
>>> +.reset= mtk_smi_larb_reset,
>>> +.assert= mtk_smi_larb_reset_assert,
>>> +.deassert= mtk_smi_larb_reset_deassert,
>>> +};
>>> +
>>> +static int mtk_smi_reset_probe(struct platform_device *pdev)
>>> +{
>>> +struct device *dev = &pdev->dev;
>>> +const struct mtk_smi_larb_plat *larb_plat =
>> of_device_get_match_data(dev);
>>> +struct device_node *np = dev->of_node, *reset_node;
>>> +struct mtk_smi_reset_data *data;
>>> +struct regmap *regmap;
>>> +
>>> +data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> +if (!data)
>>> +return -ENOMEM;
>>> +
>>> +reset_node = of_parse_phandle(np, "mediatek,larb-rst-syscon", 0);
>>> +if (!reset_node)
>>
>> This looks just wrong. This looks like a child of whatever phandle
>> points here.
>>
>> Why do you create MMIO-using node as not MMIO?
>>
> 
> This is the definition for imgsys1_dip_top and imgsys1_dip_top_rst.
> SMI reset controller driver parse the 'mediatek,larb-rst-syscon'
> to get the 'imgsys1_dip_top' device node and regmap.
> Then SMI driver could read and write the register by the regmap
> to apply reset operations here.
> 
> 	imgsys1_dip_top: clock-controller@15110000 {
> 		compatible = "mediatek,mt8188-imgsys1-dip-top";
> 		reg = <0 0x15110000 0 0x1000>;
> 		#clock-cells = <1>;
> 	};
> 
> 	imgsys1_dip_top_rst: reset-controller0 {
> 		compatible = "mediatek,smi-reset-mt8188";
> 		#reset-cells = <1>;
> 		mediatek,larb-rst-syscon = <&imgsys1_dip_top>;

This is obviously incorrect DTS code. Run standard checks on your DTS
code first.


> 	};
> 	
> 	larb10: smi@15120000{
> 		resets = <&imgsys1_dip_top_rst MT8188_SMI_RST_LARB10>;
> 		reset-names = "larb_rst";
> 	};
> 
> I am not so sure the meaning "MMIO-using" here. 

You pretend that something is MMIO but you do not use it as MMIO.

Anyway, this was 2 months ago, I lost all the context, all the emails
and I am not going back.

Respond to feedback in reasonable time, if you want to keep discussion
going.

All this is so far: NAK

Best regards,
Krzysztof