[PATCH v2 0/2] Add SMI clamp in MediaTek SMI driver

[PATCH v2 0/2] Add SMI clamp in MediaTek SMI driver

[Friday Yang]

Based on tag: next-20241119, 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.

This patch mainly add these functions:
1) Register genpd callback for SMI LARBs and handle this power domain
   status change in SMI driver.
2) Add bindings to support SMI Sub Common for MT8188.
3) Add bindings to support SMI larbs reset opearation

Changes v2:
- According to previous discussions in v1, divided these four
  patches into two topic separately
- Modify the description for 'resets' in binding
- Add const value 'larb' for 'reset-names' in binding
- Modify requirement for 'resets' and 'reset-names' in binding
- Delete 'mediatek,smi-sub-comm' in binding
- Delete 'mediatek,smi-sub-comm-in-portid' in binding
- Modify the example in binding
- Add 'mtk_smi_larb_clamp_port_mt8188' definition in SMI driver
- Change the way to parse the 'resets' in driver
- Change label from 'err_pm_disable' to 'err_link_remove'

v1:
https://patchwork.kernel.org/project/linux-mediatek/patch/20240821082845.11792-3-friday.yang@mediatek.com/
https://patchwork.kernel.org/project/linux-mediatek/patch/20240821082845.11792-4-friday.yang@mediatek.com/

friday.yang (2):
  dt-bindings: reset: mediatek: Add mt8188 SMI reset control binding
  reset: mediatek: Add reset control driver for SMI

 .../bindings/reset/mediatek,smi-reset.yaml    |  53 ++++++
 drivers/reset/Kconfig                         |   9 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-mediatek-smi.c            | 156 ++++++++++++++++++
 include/dt-bindings/reset/mt8188-resets.h     |  11 ++
 5 files changed, 230 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/mediatek,smi-reset.yaml
 create mode 100644 drivers/reset/reset-mediatek-smi.c

--
2.46.0

[PATCH v2 1/2] dt-bindings: memory: mediatek: Add SMI reset and clamp related property

[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. The hardware block diagram could
be described as below.
Add 'resets' and 'reset-names' for SMI LARBs to support SMI reset
and clamp operation. The SMI reset driver could get the reset signal
through the two properties.

             SMI-Common(Smart Multimedia Interface Common)
                          |
         +----------------+------------------+
         |                |                  |
         |                |                  |
         |                |                  |
         |                |                  |
         |                |                  |
       larb0       SMI-Sub-Common0     SMI-Sub-Common1
                   |      |     |      |             |
                  larb1  larb2 larb3  larb7       larb9

Signed-off-by: Friday Yang <friday.yang@mediatek.com>
---

Although this can pass the dtbs_check, maybe there is a better way
to describe the requirements for 'resets' and 'reset-names' in bindings.
But I don't find a better way to describe it that only SMI larbs located
in camera and image subsys requires the 'resets' and 'reset-names'.
I would appreciate it if you could give some suggestions.

.../mediatek,smi-common.yaml                  |  2 +
 .../memory-controllers/mediatek,smi-larb.yaml | 53 +++++++++++++++----
 2 files changed, 44 insertions(+), 11 deletions(-)

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..302c0f93b49d 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
@@ -69,6 +69,18 @@ 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.

+  resets:
+    maxItems: 1
+    description: This contains a phandle to the reset controller node and an index
+      to a reset signal. SMI larbs need to get the reset controller by the node.
+      SMI could get the reset signal by the index number defined in the header
+      include/dt-bindings/reset/mt8188-resets.h.
+
+  reset-names:
+    const: larb
+    description: The name of reset controller. SMI driver need to obtain the
+      reset controller based on this.
+
 required:
   - compatible
   - reg
@@ -125,19 +137,38 @@ allOf:
       required:
         - mediatek,larb-id

+  - if:  # only for camera and image subsys
+      properties:
+        mediatek,smi:
+            contains:
+              enum:
+                - smi_sub_common_img0_4x1
+                - smi_sub_common_img1_4x1
+                - smi_sub_common_cam_5x1
+                - smi_sub_common_cam_8x1
+
+    then:
+      required:
+        - resets
+        - reset-names
+
 additionalProperties: false

 examples:
   - |+
-    #include <dt-bindings/clock/mt8173-clk.h>
-    #include <dt-bindings/power/mt8173-power.h>
-
-    larb1: larb@16010000 {
-      compatible = "mediatek,mt8173-smi-larb";
-      reg = <0x16010000 0x1000>;
-      mediatek,smi = <&smi_common>;
-      power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
-      clocks = <&vdecsys CLK_VDEC_CKEN>,
-               <&vdecsys CLK_VDEC_LARB_CKEN>;
-      clock-names = "apb", "smi";
+    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
+    #include <dt-bindings/power/mediatek,mt8188-power.h>
+    #include <dt-bindings/reset/mt8188-resets.h>
+
+    larb10: smi@15120000 {
+        compatible = "mediatek,mt8188-smi-larb";
+        reg = <0 0x15120000 0 0x1000>;
+        clocks = <&imgsys CLK_IMGSYS_MAIN_DIP0>,
+                 <&imgsys1_dip_top CLK_IMGSYS1_DIP_TOP_LARB10>;
+        clock-names = "apb", "smi";
+        power-domains = <&spm MT8188_POWER_DOMAIN_DIP>;
+        resets = <&imgsys1_dip_nr_rst MT8188_SMI_RST_LARB10>;
+        reset-names = "larb";
+        mediatek,larb-id = <10>;
+        mediatek,smi = <&smi_sub_common_img0_4x1>;
     };
--
2.46.0

[PATCH v2 2/2] 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 | 175 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 171 insertions(+), 4 deletions(-)

diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index 2bc034dff691..c7119f655350 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -10,15 +10,21 @@
 #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>
 #include <dt-bindings/memory/mtk-memory-port.h>
+#include <dt-bindings/reset/mt8188-resets.h>
 
 /* SMI COMMON */
 #define SMI_L1LEN			0x100
@@ -36,6 +42,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)
@@ -134,6 +147,7 @@ struct mtk_smi_larb_gen {
 	unsigned int			larb_direct_to_common_mask;
 	unsigned int			flags_general;
 	const u8			(*ostd)[SMI_LARB_PORT_NR_MAX];
+	const u8			*clamp_port;
 };
 
 struct mtk_smi {
@@ -150,6 +164,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 +172,10 @@ struct mtk_smi_larb { /* larb: local arbiter */
 	int				larbid;
 	u32				*mmu;
 	unsigned char			*bank;
+	struct regmap			*sub_comm_syscon;
+	u8				sub_comm_inport;
+	struct notifier_block		nb;
+	struct reset_control		*rst_con;
 };
 
 static int
@@ -377,6 +396,19 @@ static const u8 mtk_smi_larb_mt8195_ostd[][SMI_LARB_PORT_NR_MAX] = {
 	[28] = {0x1a, 0x0e, 0x0a, 0x0a, 0x0c, 0x0e, 0x10,},
 };
 
+static const u8 mtk_smi_larb_clamp_port_mt8188[] = {
+	[MT8188_SMI_RST_LARB10]		= 1,
+	[MT8188_SMI_RST_LARB11A]	= 2,
+	[MT8188_SMI_RST_LARB11C]	= 3,
+	[MT8188_SMI_RST_LARB12]		= 0,
+	[MT8188_SMI_RST_LARB11B]	= 2,
+	[MT8188_SMI_RST_LARB15]		= 1,
+	[MT8188_SMI_RST_LARB16B]	= 2,
+	[MT8188_SMI_RST_LARB17B]	= 3,
+	[MT8188_SMI_RST_LARB16A]	= 2,
+	[MT8188_SMI_RST_LARB17A]	= 3,
+};
+
 static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = {
 	.port_in_larb = {
 		LARB0_PORT_OFFSET, LARB1_PORT_OFFSET,
@@ -423,6 +455,7 @@ static const struct mtk_smi_larb_gen mtk_smi_larb_mt8188 = {
 	.flags_general	            = MTK_SMI_FLAG_THRT_UPDATE | MTK_SMI_FLAG_SW_FLAG |
 				      MTK_SMI_FLAG_SLEEP_CTL | MTK_SMI_FLAG_CFG_PORT_SEC_CTL,
 	.ostd		            = mtk_smi_larb_mt8188_ostd,
+	.clamp_port                 = mtk_smi_larb_clamp_port_mt8188,
 };
 
 static const struct mtk_smi_larb_gen mtk_smi_larb_mt8192 = {
@@ -472,6 +505,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 +615,66 @@ 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;
+	const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
+	struct device_node *smi_node;
+	struct of_phandle_args args;
+	int ret, index;
+
+	/* Only SMI LARBs located in camera and image subsys need to
+	 * apply clamp and reset operation, others can be skipped.
+	 */
+	ret = of_parse_phandle_with_fixed_args(dev->of_node,
+					       "resets", 1, 0, &args);
+	if (ret)
+		return 0;
+
+	smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0);
+	if (!smi_node)
+		return -EINVAL;
+
+	index = args.args[0];
+	larb->sub_comm_inport = larb_gen->clamp_port[index];
+	larb->sub_comm_syscon = device_node_to_regmap(smi_node);
+	of_node_put(smi_node);
+
+	if (IS_ERR(larb->sub_comm_syscon) ||
+	    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 *dev = larb->dev;
+	int ret;
+
+	/* Only SMI LARBs located in camera and image subsys need to
+	 * apply clamp and reset operation, others can be skipped.
+	 */
+	if (!of_find_property(dev->of_node, "resets", NULL))
+		return 0;
+
+	larb->rst_con = devm_reset_control_get(dev, "larb");
+	if (IS_ERR(larb->rst_con))
+		return dev_err_probe(dev, PTR_ERR(larb->rst_con),
+				     "Can not 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 +685,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 +702,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_link_remove;
+	}
+
+	ret = mtk_smi_larb_parse_reset_info(larb);
+	if (ret) {
+		dev_err(dev, "Failed to get power setting for larb\n");
+		goto err_link_remove;
+	}
+
 	platform_set_drvdata(pdev, larb);
 	ret = component_add(dev, &mtk_smi_larb_component_ops);
 	if (ret)
-		goto err_pm_disable;
+		goto err_link_remove;
+
+	pm_runtime_enable(dev);
+
 	return 0;
 
-err_pm_disable:
-	pm_runtime_disable(dev);
+err_link_remove:
 	device_link_remove(dev, larb->smi_common_dev);
 	return ret;
 }
@@ -686,6 +848,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 +895,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

Re: [PATCH v2 1/2] dt-bindings: memory: mediatek: Add SMI reset and clamp related property

[Rob Herring (Arm)]

On Wed, 20 Nov 2024 14:36:38 +0800, 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. The hardware block diagram could
> be described as below.
> Add 'resets' and 'reset-names' for SMI LARBs to support SMI reset
> and clamp operation. The SMI reset driver could get the reset signal
> through the two properties.
> 
>              SMI-Common(Smart Multimedia Interface Common)
>                           |
>          +----------------+------------------+
>          |                |                  |
>          |                |                  |
>          |                |                  |
>          |                |                  |
>          |                |                  |
>        larb0       SMI-Sub-Common0     SMI-Sub-Common1
>                    |      |     |      |             |
>                   larb1  larb2 larb3  larb7       larb9
> 
> Signed-off-by: Friday Yang <friday.yang@mediatek.com>
> ---
> 
> Although this can pass the dtbs_check, maybe there is a better way
> to describe the requirements for 'resets' and 'reset-names' in bindings.
> But I don't find a better way to describe it that only SMI larbs located
> in camera and image subsys requires the 'resets' and 'reset-names'.
> I would appreciate it if you could give some suggestions.
> 
> .../mediatek,smi-common.yaml                  |  2 +
>  .../memory-controllers/mediatek,smi-larb.yaml | 53 +++++++++++++++----
>  2 files changed, 44 insertions(+), 11 deletions(-)
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml:143:13: [warning] wrong indentation: expected 10 but found 12 (indentation)

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.example.dts:29.43-44 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:129: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1442: dt_binding_check] Error 2
make: *** [Makefile:224: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241120063701.8194-2-friday.yang@mediatek.com

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

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

pip3 install dtschema --upgrade

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

Re: [PATCH v2 1/2] dt-bindings: memory: mediatek: Add SMI reset and clamp related property

[Krzysztof Kozlowski]

On Wed, Nov 20, 2024 at 02:36:38PM +0800, Friday Yang wrote:
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> index 2381660b324c..302c0f93b49d 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> @@ -69,6 +69,18 @@ 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.
> 
> +  resets:
> +    maxItems: 1
> +    description: This contains a phandle to the reset controller node and an index
> +      to a reset signal. SMI larbs need to get the reset controller by the node.

First sentence is 100% redundant. Arguments depend on the reset-cells,
not this binding.

> +      SMI could get the reset signal by the index number defined in the header
> +      include/dt-bindings/reset/mt8188-resets.h.

What? How this can depend on consumer? Drop entire description, it is
useless.

> +
> +  reset-names:
> +    const: larb
> +    description: The name of reset controller. SMI driver need to obtain the
> +      reset controller based on this.

Drop description, useless.

> +
>  required:
>    - compatible
>    - reg
> @@ -125,19 +137,38 @@ allOf:
>        required:
>          - mediatek,larb-id
> 
> +  - if:  # only for camera and image subsys
> +      properties:
> +        mediatek,smi:
> +            contains:

Never tested.

> +              enum:
> +                - smi_sub_common_img0_4x1
> +                - smi_sub_common_img1_4x1
> +                - smi_sub_common_cam_5x1
> +                - smi_sub_common_cam_8x1

Does not work. Test your code before you send it. No clue what you want
to achieve, so not sure how to help.


> +
> +    then:
> +      required:
> +        - resets
> +        - reset-names
> +
>  additionalProperties: false
> 
>  examples:
>    - |+
> -    #include <dt-bindings/clock/mt8173-clk.h>
> -    #include <dt-bindings/power/mt8173-power.h>
> -
> -    larb1: larb@16010000 {
> -      compatible = "mediatek,mt8173-smi-larb";
> -      reg = <0x16010000 0x1000>;
> -      mediatek,smi = <&smi_common>;
> -      power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
> -      clocks = <&vdecsys CLK_VDEC_CKEN>,
> -               <&vdecsys CLK_VDEC_LARB_CKEN>;
> -      clock-names = "apb", "smi";
> +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
> +    #include <dt-bindings/power/mediatek,mt8188-power.h>
> +    #include <dt-bindings/reset/mt8188-resets.h>

This is some total mess. Never tested, not correct. Sorry, run it
internally through some sort of review or internal checklist which will
ask you to test the code before sending.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] memory: mtk-smi: mt8188: Add SMI clamp function

[Krzysztof Kozlowski]

On 20/11/2024 07:36, 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 | 175 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 171 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index 2bc034dff691..c7119f655350 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -10,15 +10,21 @@
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> +#include <linux/mfd/syscon.h>

Where do you use it?

>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>

Where do you use it?

>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>

Where do you use it?

> +#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>
>  #include <dt-bindings/memory/mtk-memory-port.h>
> +#include <dt-bindings/reset/mt8188-resets.h>
>  

...

>  
> +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb *larb)
> +{
> +	struct device *dev = larb->dev;
> +	const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> +	struct device_node *smi_node;
> +	struct of_phandle_args args;
> +	int ret, index;
> +
> +	/* Only SMI LARBs located in camera and image subsys need to

Use Linux coding style.

> +	 * apply clamp and reset operation, others can be skipped.
> +	 */
> +	ret = of_parse_phandle_with_fixed_args(dev->of_node,
> +					       "resets", 1, 0, &args);

NAK

> +	if (ret)
> +		return 0;
> +
> +	smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0);
> +	if (!smi_node)
> +		return -EINVAL;
> +
> +	index = args.args[0];

That's reset, not clamp port. NAK.

> +	larb->sub_comm_inport = larb_gen->clamp_port[index];
> +	larb->sub_comm_syscon = device_node_to_regmap(smi_node);
> +	of_node_put(smi_node);
> +
> +	if (IS_ERR(larb->sub_comm_syscon) ||
> +	    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 *dev = larb->dev;
> +	int ret;
> +
> +	/* Only SMI LARBs located in camera and image subsys need to

Use Linux coding style. This applies to all your patches and all places
in this patch.

> +	 * apply clamp and reset operation, others can be skipped.
> +	 */
> +	if (!of_find_property(dev->of_node, "resets", NULL))

That's not how you use reset framework API. Use proper optional API.

> +		return 0;
> +
> +	larb->rst_con = devm_reset_control_get(dev, "larb");
> +	if (IS_ERR(larb->rst_con))
> +		return dev_err_probe(dev, PTR_ERR(larb->rst_con),
> +				     "Can not 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 +685,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 +702,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_link_remove;
> +	}
> +
> +	ret = mtk_smi_larb_parse_reset_info(larb);
> +	if (ret) {
> +		dev_err(dev, "Failed to get power setting for larb\n");
> +		goto err_link_remove;
> +	}
> +
>  	platform_set_drvdata(pdev, larb);
>  	ret = component_add(dev, &mtk_smi_larb_component_ops);
>  	if (ret)
> -		goto err_pm_disable;
> +		goto err_link_remove;
> +
> +	pm_runtime_enable(dev);
> +
>  	return 0;
>  
> -err_pm_disable:
> -	pm_runtime_disable(dev);
> +err_link_remove:
>  	device_link_remove(dev, larb->smi_common_dev);
>  	return ret;
>  }
> @@ -686,6 +848,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 +895,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},


Best regards,
Krzysztof

Re: [PATCH v2 2/2] memory: mtk-smi: mt8188: Add SMI clamp function

[AngeloGioacchino Del Regno]

Il 20/11/24 07:36, Friday Yang ha scritto:
> 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 | 175 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 171 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index 2bc034dff691..c7119f655350 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -10,15 +10,21 @@
>   #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>
>   #include <dt-bindings/memory/mtk-memory-port.h>
> +#include <dt-bindings/reset/mt8188-resets.h>
>   
>   /* SMI COMMON */
>   #define SMI_L1LEN			0x100
> @@ -36,6 +42,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)
> @@ -134,6 +147,7 @@ struct mtk_smi_larb_gen {
>   	unsigned int			larb_direct_to_common_mask;
>   	unsigned int			flags_general;
>   	const u8			(*ostd)[SMI_LARB_PORT_NR_MAX];
> +	const u8			*clamp_port;
>   };
>   
>   struct mtk_smi {
> @@ -150,6 +164,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 +172,10 @@ struct mtk_smi_larb { /* larb: local arbiter */
>   	int				larbid;
>   	u32				*mmu;
>   	unsigned char			*bank;
> +	struct regmap			*sub_comm_syscon;
> +	u8				sub_comm_inport;
> +	struct notifier_block		nb;
> +	struct reset_control		*rst_con;
>   };
>   
>   static int
> @@ -377,6 +396,19 @@ static const u8 mtk_smi_larb_mt8195_ostd[][SMI_LARB_PORT_NR_MAX] = {
>   	[28] = {0x1a, 0x0e, 0x0a, 0x0a, 0x0c, 0x0e, 0x10,},
>   };
>   
> +static const u8 mtk_smi_larb_clamp_port_mt8188[] = {

You can just set these to BIT(x) directly, like:

	[MT8188_SMI_RST_LARB10] = BIT(0),

so that you can check if there's actually a supported/valid clamp port in function
mtk_smi_larb_parse_clamp_info() - check below for comments in that function.

Note that you are declaring SMI_SUB_COMM_INPORT_NR = 8 and this means that there
are a maximum of 8 ports, with each port having its own BIT, starting from BIT 0
and ending at BIT 7 (so [7:0]).

This means that we have a maximum of 8 bits here, so even if you assign BIT(x) it
all still fits in a u8! :-)

> +	[MT8188_SMI_RST_LARB10]		= 1,
> +	[MT8188_SMI_RST_LARB11A]	= 2,
> +	[MT8188_SMI_RST_LARB11C]	= 3,
> +	[MT8188_SMI_RST_LARB12]		= 0,
> +	[MT8188_SMI_RST_LARB11B]	= 2,
> +	[MT8188_SMI_RST_LARB15]		= 1,
> +	[MT8188_SMI_RST_LARB16B]	= 2,
> +	[MT8188_SMI_RST_LARB17B]	= 3,
> +	[MT8188_SMI_RST_LARB16A]	= 2,
> +	[MT8188_SMI_RST_LARB17A]	= 3,
> +};
> +
>   static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = {
>   	.port_in_larb = {
>   		LARB0_PORT_OFFSET, LARB1_PORT_OFFSET,
> @@ -423,6 +455,7 @@ static const struct mtk_smi_larb_gen mtk_smi_larb_mt8188 = {
>   	.flags_general	            = MTK_SMI_FLAG_THRT_UPDATE | MTK_SMI_FLAG_SW_FLAG |
>   				      MTK_SMI_FLAG_SLEEP_CTL | MTK_SMI_FLAG_CFG_PORT_SEC_CTL,
>   	.ostd		            = mtk_smi_larb_mt8188_ostd,
> +	.clamp_port                 = mtk_smi_larb_clamp_port_mt8188,
>   };
>   
>   static const struct mtk_smi_larb_gen mtk_smi_larb_mt8192 = {
> @@ -472,6 +505,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)

typo: mtk_smi_larb_clamp_protect_disable

> +{
> +	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;
> +}

...but anyway, I would rather do it like that:

static int mtk_smi_larb_clamp_protect_enable(struct device *dev, bool enable)
{
	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
	u32 reg;
	int ret;

	/* sub_comm_syscon could be NULL if larb directly linked to SMI common */
	if (!larb->sub_comm_syscon)
		return -EINVAL;

	reg = enable ? SMI_COMMON_CLAMP_EN_SET : SMI_COMMON_CLAMP_EN_CLR;

	ret = regmap_write(larb->sub_comm_syscon, reg,
			   SMI_COMMON_CLAMP_MASK(larb->sub_comm_inport));
	if (ret) {
		dev_err(dev, "Unable to %s clamp for input port %d: %d\n",
			enable ? "enable" : "disable",
			larb->sub_comm_inport, ret);
		return ret;
	}

	return 0;
}

...and then call it like
ret = mtk_smi_larb_clamp_protect_enable(dev, true);

or

ret = mtk_smi_larb_clamp_protect_enable(dev, false);

> +
> +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 +615,66 @@ 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;
> +	const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> +	struct device_node *smi_node;
> +	struct of_phandle_args args;
> +	int ret, index;
> +
> +	/* Only SMI LARBs located in camera and image subsys need to
> +	 * apply clamp and reset operation, others can be skipped.
> +	 */
> +	ret = of_parse_phandle_with_fixed_args(dev->of_node,
> +					       "resets", 1, 0, &args);
> +	if (ret)
> +		return 0;
> +
> +	smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0);
> +	if (!smi_node)
> +		return -EINVAL;
> +
> +	index = args.args[0];
> +	larb->sub_comm_inport = larb_gen->clamp_port[index];
> +	larb->sub_comm_syscon = device_node_to_regmap(smi_node);
> +	of_node_put(smi_node);

If you declare BIT(x) as clamp_ports, or if your clamp_ports start from 1,
anything that is not more than 0 is something that was not declared, and
this means that you can then error check:

if (!larb->sub_comm_inport)
	return dev_err_probe(dev, -EINVAL, "Unknown clamp port for larb %d\n", index);

> +
> +	if (IS_ERR(larb->sub_comm_syscon) ||
> +	    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 *dev = larb->dev;
> +	int ret;
> +
> +	/* Only SMI LARBs located in camera and image subsys need to
> +	 * apply clamp and reset operation, others can be skipped.
> +	 */
> +	if (!of_find_property(dev->of_node, "resets", NULL))
> +		return 0;

You don't have to manually check whether 'resets' exists or not - that's already
done (devm_)reset_control_get in a way, as that will return -ENOENT if there is
no 'reset-names' property. Check the comments down there....

> +
> +	larb->rst_con = devm_reset_control_get(dev, "larb");
> +	if (IS_ERR(larb->rst_con))
> +		return dev_err_probe(dev, PTR_ERR(larb->rst_con),
> +				     "Can not 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 +685,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 +702,29 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		return ret;
>   
> -	pm_runtime_enable(dev);

	ret = mtk_smi_larb_parse_reset_optional(larb);

Choose between:

	if (ret == 0) {
		ret = mtk_smi_larb_parse_smi_clamp(larb);
		if (ret) {
			dev_err_probe(dev, ret, "Failed to get SMI clamp\n");
			goto err_link_remove;
		}
	} else if (ret != -ENOENT) {
		dev_err_probe(dev, ret, "Cannot get larb resets\n");
		goto err_link_remove;
	} else {
		/*
		 * Only SMI LARBs located in camera and image subsys need to apply
		 * clamp and reset operation. For the others, resets are optional.
		 */
		ret = 0;
	}

and...

	if (ret && ret != -ENOENT) {
		dev_err_probe .....
	} else if (ret) {
		/* only smi larbs .... */
		ret = 0;
	} else {
		ret = mtk_smi_larb_parse_smi_clamp .....
	}

whatever you like the most.

> +	/* 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_link_remove;
> +	}
> +
> +	ret = mtk_smi_larb_parse_reset_info(larb);
> +	if (ret) {
> +		dev_err(dev, "Failed to get power setting for larb\n");
> +		goto err_link_remove;
> +	}
> +
>   	platform_set_drvdata(pdev, larb);
>   	ret = component_add(dev, &mtk_smi_larb_component_ops);
>   	if (ret)
> -		goto err_pm_disable;
> +		goto err_link_remove;
> +
> +	pm_runtime_enable(dev);
> +
>   	return 0;
>   
> -err_pm_disable:
> -	pm_runtime_disable(dev);
> +err_link_remove:
>   	device_link_remove(dev, larb->smi_common_dev);
>   	return ret;
>   }
> @@ -686,6 +848,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,

...no gals in MT8188?!

Cheers,
Angelo

> +};
> +
>   static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = {
>   	.type     = MTK_SMI_GEN2,
>   	.has_gals = true,
> @@ -729,6 +895,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},

Re: [PATCH v2 1/2] dt-bindings: memory: mediatek: Add SMI reset and clamp related property

[Friday Yang (杨阳)]

On Wed, 2024-11-20 at 01:43 -0600, Rob Herring (Arm) wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, 20 Nov 2024 14:36:38 +0800, 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. The hardware block diagram
> > could
> > be described as below.
> > Add 'resets' and 'reset-names' for SMI LARBs to support SMI reset
> > and clamp operation. The SMI reset driver could get the reset
> > signal
> > through the two properties.
> > 
> >              SMI-Common(Smart Multimedia Interface Common)
> >                           |
> >          +----------------+------------------+
> >          |                |                  |
> >          |                |                  |
> >          |                |                  |
> >          |                |                  |
> >          |                |                  |
> >        larb0       SMI-Sub-Common0     SMI-Sub-Common1
> >                    |      |     |      |             |
> >                   larb1  larb2 larb3  larb7       larb9
> > 
> > Signed-off-by: Friday Yang <friday.yang@mediatek.com>
> > ---
> > 
> > Although this can pass the dtbs_check, maybe there is a better way
> > to describe the requirements for 'resets' and 'reset-names' in
> > bindings.
> > But I don't find a better way to describe it that only SMI larbs
> > located
> > in camera and image subsys requires the 'resets' and 'reset-names'.
> > I would appreciate it if you could give some suggestions.
> > 
> > .../mediatek,smi-common.yaml                  |  2 +
> >  .../memory-controllers/mediatek,smi-larb.yaml | 53
> > +++++++++++++++----
> >  2 files changed, 44 insertions(+), 11 deletions(-)
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
> larb.yaml:143:13: [warning] wrong indentation: expected 10 but found
> 12 (indentation)
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/memory-
> controllers/mediatek,smi-larb.example.dts:29.43-44 syntax error
> FATAL ERROR: Unable to parse input tree
> make[2]: *** [scripts/Makefile.dtbs:129:
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
> larb.example.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1442:
> dt_binding_check] Error 2
> make: *** [Makefile:224: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See 
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241120063701.8194-2-friday.yang@mediatek.com
> 
> The base for the series is generally the latest rc1. A different
> dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up
> to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself.
> Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up
> checking
> your schema. However, it must be unset to test all examples with your
> schema.
> 

Thanks for your comments, I used to use this cmd:

make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/memory-
controllers/mediatek,smi-larb.yaml

And I will upgrade dtschema and change to use 'make dt_binding_check'
again.

Re: [PATCH v2 2/2] memory: mtk-smi: mt8188: Add SMI clamp function

[Friday Yang (杨阳)]

On Wed, 2024-11-20 at 12:49 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 20/11/24 07:36, Friday Yang ha scritto:
> > 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 | 175
> > ++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 171 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > index 2bc034dff691..c7119f655350 100644
> > --- a/drivers/memory/mtk-smi.c
> > +++ b/drivers/memory/mtk-smi.c
> > @@ -10,15 +10,21 @@
> >   #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>
> >   #include <dt-bindings/memory/mtk-memory-port.h>
> > +#include <dt-bindings/reset/mt8188-resets.h>
> > 
> >   /* SMI COMMON */
> >   #define SMI_L1LEN                   0x100
> > @@ -36,6 +42,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)
> > @@ -134,6 +147,7 @@ struct mtk_smi_larb_gen {
> >       unsigned int                    larb_direct_to_common_mask;
> >       unsigned int                    flags_general;
> >       const
> > u8                        (*ostd)[SMI_LARB_PORT_NR_MAX];
> > +     const u8                        *clamp_port;
> >   };
> > 
> >   struct mtk_smi {
> > @@ -150,6 +164,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 +172,10 @@ struct mtk_smi_larb { /* larb: local arbiter
> > */
> >       int                             larbid;
> >       u32                             *mmu;
> >       unsigned char                   *bank;
> > +     struct regmap                   *sub_comm_syscon;
> > +     u8                              sub_comm_inport;
> > +     struct notifier_block           nb;
> > +     struct reset_control            *rst_con;
> >   };
> > 
> >   static int
> > @@ -377,6 +396,19 @@ static const u8
> > mtk_smi_larb_mt8195_ostd[][SMI_LARB_PORT_NR_MAX] = {
> >       [28] = {0x1a, 0x0e, 0x0a, 0x0a, 0x0c, 0x0e, 0x10,},
> >   };
> > 
> > +static const u8 mtk_smi_larb_clamp_port_mt8188[] = {
> 
> You can just set these to BIT(x) directly, like:
> 
>         [MT8188_SMI_RST_LARB10] = BIT(0),
> 
> so that you can check if there's actually a supported/valid clamp
> port in function
> mtk_smi_larb_parse_clamp_info() - check below for comments in that
> function.
> 
> Note that you are declaring SMI_SUB_COMM_INPORT_NR = 8 and this means
> that there
> are a maximum of 8 ports, with each port having its own BIT, starting
> from BIT 0
> and ending at BIT 7 (so [7:0]).
> 
> This means that we have a maximum of 8 bits here, so even if you
> assign BIT(x) it
> all still fits in a u8! :-)

Thanks for your comments, I will modify the definition for
mtk_smi_larb_clamp_port_mt8188, and macro 'SMI_COMMON_CLAMP_MASK'
could be removed.

> 
> > +     [MT8188_SMI_RST_LARB10]         = 1,
> > +     [MT8188_SMI_RST_LARB11A]        = 2,
> > +     [MT8188_SMI_RST_LARB11C]        = 3,
> > +     [MT8188_SMI_RST_LARB12]         = 0,
> > +     [MT8188_SMI_RST_LARB11B]        = 2,
> > +     [MT8188_SMI_RST_LARB15]         = 1,
> > +     [MT8188_SMI_RST_LARB16B]        = 2,
> > +     [MT8188_SMI_RST_LARB17B]        = 3,
> > +     [MT8188_SMI_RST_LARB16A]        = 2,
> > +     [MT8188_SMI_RST_LARB17A]        = 3,
> > +};
> > +
> >   static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = {
> >       .port_in_larb = {
> >               LARB0_PORT_OFFSET, LARB1_PORT_OFFSET,
> > @@ -423,6 +455,7 @@ static const struct mtk_smi_larb_gen
> > mtk_smi_larb_mt8188 = {
> >       .flags_general              = MTK_SMI_FLAG_THRT_UPDATE |
> > MTK_SMI_FLAG_SW_FLAG |
> >                                     MTK_SMI_FLAG_SLEEP_CTL |
> > MTK_SMI_FLAG_CFG_PORT_SEC_CTL,
> >       .ostd                       = mtk_smi_larb_mt8188_ostd,
> > +     .clamp_port                 = mtk_smi_larb_clamp_port_mt8188,
> >   };
> > 
> >   static const struct mtk_smi_larb_gen mtk_smi_larb_mt8192 = {
> > @@ -472,6 +505,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)
> 
> typo: mtk_smi_larb_clamp_protect_disable
> 
> > +{
> > +     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;
> > +}
> 
> ...but anyway, I would rather do it like that:
> 
> static int mtk_smi_larb_clamp_protect_enable(struct device *dev, bool
> enable)
> {
>         struct mtk_smi_larb *larb = dev_get_drvdata(dev);
>         u32 reg;
>         int ret;
> 
>         /* sub_comm_syscon could be NULL if larb directly linked to
> SMI common */
>         if (!larb->sub_comm_syscon)
>                 return -EINVAL;
> 
>         reg = enable ? SMI_COMMON_CLAMP_EN_SET :
> SMI_COMMON_CLAMP_EN_CLR;
> 
>         ret = regmap_write(larb->sub_comm_syscon, reg,
>                            SMI_COMMON_CLAMP_MASK(larb-
> >sub_comm_inport));
>         if (ret) {
>                 dev_err(dev, "Unable to %s clamp for input port %d:
> %d\n",
>                         enable ? "enable" : "disable",
>                         larb->sub_comm_inport, ret);
>                 return ret;
>         }
> 
>         return 0;
> }
> 
> ...and then call it like
> ret = mtk_smi_larb_clamp_protect_enable(dev, true);
> 
> or
> 
> ret = mtk_smi_larb_clamp_protect_enable(dev, false);
> 

Thanks for your comments, I will merge these two functions
into one. This looks better.

> > +
> > +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 +615,66 @@ 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;
> > +     const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> > +     struct device_node *smi_node;
> > +     struct of_phandle_args args;
> > +     int ret, index;
> > +
> > +     /* Only SMI LARBs located in camera and image subsys need to
> > +      * apply clamp and reset operation, others can be skipped.
> > +      */
> > +     ret = of_parse_phandle_with_fixed_args(dev->of_node,
> > +                                            "resets", 1, 0,
> > &args);
> > +     if (ret)
> > +             return 0;
> > +
> > +     smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0);
> > +     if (!smi_node)
> > +             return -EINVAL;
> > +
> > +     index = args.args[0];
> > +     larb->sub_comm_inport = larb_gen->clamp_port[index];
> > +     larb->sub_comm_syscon = device_node_to_regmap(smi_node);
> > +     of_node_put(smi_node);
> 
> If you declare BIT(x) as clamp_ports, or if your clamp_ports start
> from 1,
> anything that is not more than 0 is something that was not declared,
> and
> this means that you can then error check:
> 
> if (!larb->sub_comm_inport)
>         return dev_err_probe(dev, -EINVAL, "Unknown clamp port for
> larb %d\n", index);
> 

Yes, you are right. I will fix it as this way.

> > +
> > +     if (IS_ERR(larb->sub_comm_syscon) ||
> > +         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 *dev = larb->dev;
> > +     int ret;
> > +
> > +     /* Only SMI LARBs located in camera and image subsys need to
> > +      * apply clamp and reset operation, others can be skipped.
> > +      */
> > +     if (!of_find_property(dev->of_node, "resets", NULL))
> > +             return 0;
> 
> You don't have to manually check whether 'resets' exists or not -
> that's already
> done (devm_)reset_control_get in a way, as that will return -ENOENT
> if there is
> no 'reset-names' property. Check the comments down there....
> 
> > +
> > +     larb->rst_con = devm_reset_control_get(dev, "larb");
> > +     if (IS_ERR(larb->rst_con))
> > +             return dev_err_probe(dev, PTR_ERR(larb->rst_con),
> > +                                  "Can not 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;
> > +}
> > +

OK, I will just check the return value of devm_reset_control_get
instead of parsing 'resets' here. This looks better.

> >   static int mtk_smi_larb_probe(struct platform_device *pdev)
> >   {
> >       struct mtk_smi_larb *larb;
> > @@ -538,6 +685,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 +702,29 @@ static int mtk_smi_larb_probe(struct
> > platform_device *pdev)
> >       if (ret < 0)
> >               return ret;
> > 
> > -     pm_runtime_enable(dev);
> 
>         ret = mtk_smi_larb_parse_reset_optional(larb);
> 
> Choose between:
> 
>         if (ret == 0) {
>                 ret = mtk_smi_larb_parse_smi_clamp(larb);
>                 if (ret) {
>                         dev_err_probe(dev, ret, "Failed to get SMI
> clamp\n");
>                         goto err_link_remove;
>                 }
>         } else if (ret != -ENOENT) {
>                 dev_err_probe(dev, ret, "Cannot get larb resets\n");
>                 goto err_link_remove;
>         } else {
>                 /*
>                  * Only SMI LARBs located in camera and image subsys
> need to apply
>                  * clamp and reset operation. For the others, resets
> are optional.
>                  */
>                 ret = 0;
>         }
> 
> and...
> 
>         if (ret && ret != -ENOENT) {
>                 dev_err_probe .....
>         } else if (ret) {
>                 /* only smi larbs .... */
>                 ret = 0;
>         } else {
>                 ret = mtk_smi_larb_parse_smi_clamp .....
>         }
> 
> whatever you like the most.

Thanks for your comments, I will check the return value as this way.

> 
> > +     /* 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_link_remove;
> > +     }
> > +
> > +     ret = mtk_smi_larb_parse_reset_info(larb);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to get power setting for
> > larb\n");
> > +             goto err_link_remove;
> > +     }
> > +
> >       platform_set_drvdata(pdev, larb);
> >       ret = component_add(dev, &mtk_smi_larb_component_ops);
> >       if (ret)
> > -             goto err_pm_disable;
> > +             goto err_link_remove;
> > +
> > +     pm_runtime_enable(dev);
> > +
> >       return 0;
> > 
> > -err_pm_disable:
> > -     pm_runtime_disable(dev);
> > +err_link_remove:
> >       device_link_remove(dev, larb->smi_common_dev);
> >       return ret;
> >   }
> > @@ -686,6 +848,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,
> 
> ...no gals in MT8188?!
> 

Yes, there is gals in MT8188 hardware design. And the 'gals' clock 
may be the same as the 'apb' and 'smi' clock.
I will add this in the next version, thanks for your comments.

> Cheers,
> Angelo
> 
> > +};
> > +
> >   static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = {
> >       .type     = MTK_SMI_GEN2,
> >       .has_gals = true,
> > @@ -729,6 +895,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},
> 
> 

Re: [PATCH v2 1/2] dt-bindings: memory: mediatek: Add SMI reset and clamp related property

[Friday Yang (杨阳)]

On Wed, 2024-11-20 at 08:45 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Nov 20, 2024 at 02:36:38PM +0800, Friday Yang wrote:
> > diff --git a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > index 2381660b324c..302c0f93b49d 100644
> > --- a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > +++ b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > @@ -69,6 +69,18 @@ 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.
> > 
> > +  resets:
> > +    maxItems: 1
> > +    description: This contains a phandle to the reset controller
> > node and an index
> > +      to a reset signal. SMI larbs need to get the reset
> > controller by the node.
> 
> First sentence is 100% redundant. Arguments depend on the reset-
> cells,
> not this binding.
> 
> > +      SMI could get the reset signal by the index number defined
> > in the header
> > +      include/dt-bindings/reset/mt8188-resets.h.
> 
> What? How this can depend on consumer? Drop entire description, it is
> useless.
> 

Thanks for your comments, I will remove the entire description.

> > +
> > +  reset-names:
> > +    const: larb
> > +    description: The name of reset controller. SMI driver need to
> > obtain the
> > +      reset controller based on this.
> 
> Drop description, useless.

OK, I will remove the entire description.

> 
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -125,19 +137,38 @@ allOf:
> >        required:
> >          - mediatek,larb-id
> > 
> > +  - if:  # only for camera and image subsys
> > +      properties:
> > +        mediatek,smi:
> > +            contains:
> 
> Never tested.

OK, I will fix the indentation.

> 
> > +              enum:
> > +                - smi_sub_common_img0_4x1
> > +                - smi_sub_common_img1_4x1
> > +                - smi_sub_common_cam_5x1
> > +                - smi_sub_common_cam_8x1
> 
> Does not work. Test your code before you send it. No clue what you
> want
> to achieve, so not sure how to help.
> 

As I mentioned in SMI driver, only SMI larbs located in
camera and image subsys need to apply clamp and reset operation,
others can be skipped.
So I want to know if this description method meets your expectations
here(smi_sub_common_img0_4x1, smi_sub_common_img1_4x1 ...).

> 
> > +
> > +    then:
> > +      required:
> > +        - resets
> > +        - reset-names
> > +
> >  additionalProperties: false
> > 
> >  examples:
> >    - |+
> > -    #include <dt-bindings/clock/mt8173-clk.h>
> > -    #include <dt-bindings/power/mt8173-power.h>
> > -
> > -    larb1: larb@16010000 {
> > -      compatible = "mediatek,mt8173-smi-larb";
> > -      reg = <0x16010000 0x1000>;
> > -      mediatek,smi = <&smi_common>;
> > -      power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
> > -      clocks = <&vdecsys CLK_VDEC_CKEN>,
> > -               <&vdecsys CLK_VDEC_LARB_CKEN>;
> > -      clock-names = "apb", "smi";
> > +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
> > +    #include <dt-bindings/power/mediatek,mt8188-power.h>
> > +    #include <dt-bindings/reset/mt8188-resets.h>
> 
> This is some total mess. Never tested, not correct. Sorry, run it
> internally through some sort of review or internal checklist which
> will
> ask you to test the code before sending.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/2] memory: mtk-smi: mt8188: Add SMI clamp function

[Friday Yang (杨阳)]

On Wed, 2024-11-20 at 08:49 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 20/11/2024 07:36, 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 | 175
> > ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 171 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > index 2bc034dff691..c7119f655350 100644
> > --- a/drivers/memory/mtk-smi.c
> > +++ b/drivers/memory/mtk-smi.c
> > @@ -10,15 +10,21 @@
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/mfd/syscon.h>
> 
> Where do you use it?

device_node_to_regmap need this header file.

> 
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> 
> Where do you use it?

dev_pm_genpd_add_notifier need this header file.

> 
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> 
> Where do you use it?

regmap_write need this header file.

> 
> > +#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>
> >  #include <dt-bindings/memory/mtk-memory-port.h>
> > +#include <dt-bindings/reset/mt8188-resets.h>
> > 
> 
> ...

reset_control_reset/devm_reset_control_get need reset.h
But reset-controller.h could be removed.
MT8188_SMI_RST_LARB10 and other index need mt8188-resets.h 

> 
> > 
> > +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb
> > *larb)
> > +{
> > +     struct device *dev = larb->dev;
> > +     const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
> > +     struct device_node *smi_node;
> > +     struct of_phandle_args args;
> > +     int ret, index;
> > +
> > +     /* Only SMI LARBs located in camera and image subsys need to
> 
> Use Linux coding style.

Sorry for the mistake, I will fix it.

> 
> > +      * apply clamp and reset operation, others can be skipped.
> > +      */
> > +     ret = of_parse_phandle_with_fixed_args(dev->of_node,
> > +                                            "resets", 1, 0,
> > &args);
> 
> NAK
> 
> > +     if (ret)
> > +             return 0;
> > +
> > +     smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0);
> > +     if (!smi_node)
> > +             return -EINVAL;
> > +
> > +     index = args.args[0];
> 
> That's reset, not clamp port. NAK.

I could change from 'clamp_port' to 'reset_port'.
This index is used for getting the port id from the array
'mtk_smi_larb_clamp_port_mt8188 ' in SMI driver.
It could also be used for getting the reset signal in
SMI reset controller driver.


> 
> > +     larb->sub_comm_inport = larb_gen->clamp_port[index];
> > +     larb->sub_comm_syscon = device_node_to_regmap(smi_node);
> > +     of_node_put(smi_node);
> > +
> > +     if (IS_ERR(larb->sub_comm_syscon) ||
> > +         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 *dev = larb->dev;
> > +     int ret;
> > +
> > +     /* Only SMI LARBs located in camera and image subsys need to
> 
> Use Linux coding style. This applies to all your patches and all
> places
> in this patch.

OK, I will fix it.

> 
> > +      * apply clamp and reset operation, others can be skipped.
> > +      */
> > +     if (!of_find_property(dev->of_node, "resets", NULL))
> 
> That's not how you use reset framework API. Use proper optional API.

Thanks, I will check the return value for devm_reset_control_get
instead of parsing 'resets' here.

> 
> > +             return 0;
> > +
> > +     larb->rst_con = devm_reset_control_get(dev, "larb");
> > +     if (IS_ERR(larb->rst_con))
> > +             return dev_err_probe(dev, PTR_ERR(larb->rst_con),
> > +                                  "Can not 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 +685,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 +702,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_link_remove;
> > +     }
> > +
> > +     ret = mtk_smi_larb_parse_reset_info(larb);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to get power setting for
> > larb\n");
> > +             goto err_link_remove;
> > +     }
> > +
> >       platform_set_drvdata(pdev, larb);
> >       ret = component_add(dev, &mtk_smi_larb_component_ops);
> >       if (ret)
> > -             goto err_pm_disable;
> > +             goto err_link_remove;
> > +
> > +     pm_runtime_enable(dev);
> > +
> >       return 0;
> > 
> > -err_pm_disable:
> > -     pm_runtime_disable(dev);
> > +err_link_remove:
> >       device_link_remove(dev, larb->smi_common_dev);
> >       return ret;
> >  }
> > @@ -686,6 +848,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 +895,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},
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v2 2/2] memory: mtk-smi: mt8188: Add SMI clamp function

[Krzysztof Kozlowski]

On 22/11/2024 10:41, Friday Yang (杨阳) wrote:
> On Wed, 2024-11-20 at 08:49 +0100, Krzysztof Kozlowski wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On 20/11/2024 07:36, 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 | 175
>>> ++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 171 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
>>> index 2bc034dff691..c7119f655350 100644
>>> --- a/drivers/memory/mtk-smi.c
>>> +++ b/drivers/memory/mtk-smi.c
>>> @@ -10,15 +10,21 @@
>>>  #include <linux/err.h>
>>>  #include <linux/io.h>
>>>  #include <linux/iopoll.h>
>>> +#include <linux/mfd/syscon.h>
>>
>> Where do you use it?
> 
> device_node_to_regmap need this header file.

Ah, indeed, but then I wonder why you parse phandle instead of using
standard syscon API: syscon_regmap_lookup_by_phandle().


> 
>>
>>>  #include <linux/module.h>
>>>  #include <linux/of.h>
>>>  #include <linux/of_platform.h>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/pm_domain.h>
>>
>> Where do you use it?
> 
> dev_pm_genpd_add_notifier need this header file.

ack

> 
>>
>>>  #include <linux/pm_runtime.h>
>>> +#include <linux/regmap.h>
>>
>> Where do you use it?
> 
> regmap_write need this header file.

ack

> 
>>
>>> +#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>
>>>  #include <dt-bindings/memory/mtk-memory-port.h>
>>> +#include <dt-bindings/reset/mt8188-resets.h>
>>>
>>
>> ...
> 
> reset_control_reset/devm_reset_control_get need reset.h
> But reset-controller.h could be removed.
> MT8188_SMI_RST_LARB10 and other index need mt8188-resets.h 
> 
>>
>>>
>>> +static int mtk_smi_larb_parse_clamp_info(struct mtk_smi_larb
>>> *larb)
>>> +{
>>> +     struct device *dev = larb->dev;
>>> +     const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
>>> +     struct device_node *smi_node;
>>> +     struct of_phandle_args args;
>>> +     int ret, index;
>>> +
>>> +     /* Only SMI LARBs located in camera and image subsys need to
>>
>> Use Linux coding style.
> 
> Sorry for the mistake, I will fix it.
> 
>>
>>> +      * apply clamp and reset operation, others can be skipped.
>>> +      */
>>> +     ret = of_parse_phandle_with_fixed_args(dev->of_node,
>>> +                                            "resets", 1, 0,
>>> &args);
>>
>> NAK
>>
>>> +     if (ret)
>>> +             return 0;
>>> +
>>> +     smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0);
>>> +     if (!smi_node)
>>> +             return -EINVAL;
>>> +
>>> +     index = args.args[0];
>>
>> That's reset, not clamp port. NAK.
> 
> I could change from 'clamp_port' to 'reset_port'.
> This index is used for getting the port id from the array
> 'mtk_smi_larb_clamp_port_mt8188 ' in SMI driver.

Index is for reset, not for port ID.

> It could also be used for getting the reset signal in
> SMI reset controller driver.
> 

Look at my comments from previous version - they were not under reset
property. The argument for reset is for reset provider, not this reset
consumer.

BTW, when you link to previous versions of patchset, link to lore, not
patchwork. Or just use b4.

Best regards,
Krzysztof