[PATCH v6 0/3] Add SMI reset and clamp for MediaTek MT8188 SoC

[PATCH v6 0/3] Add SMI reset and clamp for MediaTek MT8188 SoC

[Friday Yang]

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

On the MediaTek MT8188 SoC platform, we encountered power-off failures
and SMI bus hang issues during camera stress tests. The issue arises
because bus glitches are sometimes produced when MTCMOS powers on or
off. While this is fairly normal, the software must handle these
glitches to avoid mistaking them for transaction signals. What's
more, this issue emerged only after the initial upstreaming of SMI
driver.

The software solutions can be summarized as follows:

1. Use CLAMP to disable the SMI sub-common port after turning off the
   LARB CG and before turning off the LARB MTCMOS.
2. Use CLAMP to disable/enable the SMI sub-common port.
3. Implement an AXI reset for SMI LARBs.

Changes v6:
- Fix code comments style.
- Add another patch to replace 'pm_runtime_enable' with
  'devm_pm_runtime_enable'.

v5:
https://lore.kernel.org/lkml/20250311122327.20685-2-friday.yang@mediatek.com/
https://lore.kernel.org/lkml/20250311122327.20685-3-friday.yang@mediatek.com/

Friday Yang (3):
  dt-bindings: memory: mediatek: Add SMI reset and clamp for MT8188
  memory: mtk-smi: mt8188: Add SMI reset and clamp for MT8188
  memory: mtk-smi: mt8188: Use devm_pm_runtime_enable

 .../mediatek,smi-common.yaml                  |   2 +
 .../memory-controllers/mediatek,smi-larb.yaml |  19 +++
 drivers/memory/mtk-smi.c                      | 150 +++++++++++++++++-
 3 files changed, 164 insertions(+), 7 deletions(-)

--
2.46.0

[PATCH v6 1/3] dt-bindings: memory: mediatek: Add SMI reset and clamp for MT8188

[Friday Yang]

From: "Friday Yang" <friday.yang@mediatek.com>

Add 'resets' and 'reset-names' properties for SMI LARBs to support
SMI reset operations.
On the MediaTek platform, some SMI LARBs are directly connected to
the SMI Common, while others are connected to the SMI Sub-Common,
which in turn is connected to the SMI Common. The hardware block
diagram can be described as follows.

             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>
---
 .../mediatek,smi-common.yaml                  |  2 ++
 .../memory-controllers/mediatek,smi-larb.yaml | 19 +++++++++++++++++++
 2 files changed, 21 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..f4f0ed0f1fd9 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
@@ -69,6 +69,12 @@ 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
+
+  reset-names:
+    const: larb
+
 required:
   - compatible
   - reg
@@ -125,6 +131,19 @@ allOf:
       required:
         - mediatek,larb-id

+  - if:  # only for image, camera and ipe subsys
+      properties:
+        compatible:
+          const: mediatek,mt8188-smi-larb
+        mediatek,larb-id:
+          enum:
+            [ 9, 10, 11, 12, 13, 16, 17, 18, 19, 20 ]
+
+    then:
+      required:
+        - resets
+        - reset-names
+
 additionalProperties: false

 examples:
--
2.46.0

[PATCH v6 2/3] memory: mtk-smi: mt8188: Add SMI reset and clamp for MT8188

[Friday Yang]

From: "Friday Yang" <friday.yang@mediatek.com>

To prevent handling glitch signals during MTCMOS on/off transitions,
SMI requires clamp and reset operations. Parse the reset settings for
SMI LARBs and the clamp settings for the SMI Sub-Common. Register
genpd callback for the SMI LARBs located in image, camera and IPE
subsystems, and apply reset and clamp operations within the callback.

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

diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index a8f5467d6b31..f25d46d2ef33 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -10,11 +10,15 @@
 #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/soc/mediatek/mtk_sip_svc.h>
 #include <soc/mediatek/smi.h>
 #include <dt-bindings/memory/mt2701-larb-port.h>
@@ -36,6 +40,12 @@
 #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_SUB_COMM_INPORT_NR		(8)
+
 /* SMI LARB */
 #define SMI_LARB_SLP_CON                0xc
 #define SLP_PROT_EN                     BIT(0)
@@ -134,6 +144,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 +161,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 +169,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
@@ -409,6 +425,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[MTK_LARB_NR_MAX] = {
+	[9]	= BIT(1), /* larb10 */
+	[10]	= BIT(2), /* larb11a */
+	[11]	= BIT(2), /* larb11b */
+	[12]	= BIT(3), /* larb11c */
+	[13]	= BIT(0), /* larb12 */
+	[16]	= BIT(1), /* larb15 */
+	[17]	= BIT(2), /* larb16a */
+	[18]	= BIT(2), /* larb16b */
+	[19]	= BIT(3), /* larb17a */
+	[20]	= BIT(3), /* larb17b */
+};
+
 static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = {
 	.port_in_larb = {
 		LARB0_PORT_OFFSET, LARB1_PORT_OFFSET,
@@ -455,6 +484,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 = {
@@ -505,6 +535,46 @@ 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, 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,
+			   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;
+}
+
+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, true);
+	} else if (flags == GENPD_NOTIFY_ON) {
+		/* enable related SMI sub-common port */
+		reset_control_reset(larb->rst_con);
+		mtk_smi_larb_clamp_protect_enable(dev, false);
+	}
+
+	return NOTIFY_OK;
+}
+
 static int mtk_smi_device_link_common(struct device *dev, struct device **com_dev)
 {
 	struct platform_device *smi_com_pdev;
@@ -561,6 +631,53 @@ static int mtk_smi_dts_clk_init(struct device *dev, struct mtk_smi *smi,
 	return ret;
 }

+static int mtk_smi_larb_parse_clamp_optional(struct mtk_smi_larb *larb)
+{
+	struct device *dev = larb->dev;
+	const struct mtk_smi_larb_gen *larb_gen = larb->larb_gen;
+	u32 larb_id;
+	int ret;
+
+	/*
+	 * Only SMI LARBs in camera, image and IPE subsys need to
+	 * apply clamp and reset operations, others can be skipped.
+	 */
+	ret = of_property_read_u32(dev->of_node, "mediatek,larb-id", &larb_id);
+	if (ret)
+		return -EINVAL;
+	if (!larb_gen->clamp_port || !larb_gen->clamp_port[larb_id])
+		return 0;
+
+	larb->sub_comm_inport = larb_gen->clamp_port[larb_id];
+	larb->sub_comm_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
+								"mediatek,smi");
+	if (IS_ERR(larb->sub_comm_syscon)) {
+		larb->sub_comm_syscon = NULL;
+		return dev_err_probe(dev, -EINVAL,
+				     "Unknown clamp port for larb %d\n", larb_id);
+	}
+
+	return 0;
+}
+
+static int mtk_smi_larb_parse_reset_optional(struct mtk_smi_larb *larb)
+{
+	struct device *dev = larb->dev;
+	int ret;
+
+	larb->rst_con = devm_reset_control_get_optional_exclusive(dev, "larb");
+	if (!larb->rst_con)
+		return 0;
+
+	larb->nb.notifier_call = mtk_smi_genpd_callback;
+	ret = dev_pm_genpd_add_notifier(dev, &larb->nb);
+	if (ret)
+		return dev_err_probe(dev, -EINVAL,
+				     "Failed to add genpd callback %d\n", ret);
+
+	return 0;
+}
+
 static int mtk_smi_larb_probe(struct platform_device *pdev)
 {
 	struct mtk_smi_larb *larb;
@@ -571,6 +688,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))
@@ -587,15 +705,25 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;

+	ret = mtk_smi_larb_parse_clamp_optional(larb);
+	if (ret)
+		goto err_link_remove;
+
+	ret = mtk_smi_larb_parse_reset_optional(larb);
+	if (ret)
+		goto err_link_remove;
+
 	pm_runtime_enable(dev);
 	platform_set_drvdata(pdev, larb);
 	ret = component_add(dev, &mtk_smi_larb_component_ops);
 	if (ret)
 		goto err_pm_disable;
 	return 0;

 err_pm_disable:
 	pm_runtime_disable(dev);
+err_link_remove:
 	device_link_remove(dev, larb->smi_common_dev);
 	return ret;
 }
@@ -719,6 +847,11 @@ 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,
+	.has_gals = true,
+};
+
 static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = {
 	.type     = MTK_SMI_GEN2,
 	.has_gals = true,
@@ -762,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

[PATCH v6 3/3] memory: mtk-smi: mt8188: Use devm_pm_runtime_enable

[Friday Yang]

Replace pm_runtime_enable with the devres-enabled version which
can trigger pm_runtime_disable.

Signed-off-by: Friday Yang <friday.yang@mediatek.com>
---
 drivers/memory/mtk-smi.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index f25d46d2ef33..daef6d350419 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -713,16 +713,17 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_link_remove;

-	pm_runtime_enable(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		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;

 	return 0;

-err_pm_disable:
-	pm_runtime_disable(dev);
 err_link_remove:
 	device_link_remove(dev, larb->smi_common_dev);
 	return ret;
@@ -733,7 +734,6 @@ static void mtk_smi_larb_remove(struct platform_device *pdev)
 	struct mtk_smi_larb *larb = platform_get_drvdata(pdev);

 	device_link_remove(&pdev->dev, larb->smi_common_dev);
-	pm_runtime_disable(&pdev->dev);
 	component_del(&pdev->dev, &mtk_smi_larb_component_ops);
 }

@@ -954,7 +954,10 @@ static int mtk_smi_common_probe(struct platform_device *pdev)
 			return ret;
 	}

-	pm_runtime_enable(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
 	platform_set_drvdata(pdev, common);
 	return 0;
 }
@@ -965,7 +968,6 @@ static void mtk_smi_common_remove(struct platform_device *pdev)

 	if (common->plat->type == MTK_SMI_GEN2_SUB_COMM)
 		device_link_remove(&pdev->dev, common->smi_common_dev);
-	pm_runtime_disable(&pdev->dev);
 }

 static int __maybe_unused mtk_smi_common_resume(struct device *dev)
--
2.46.0

Re: [PATCH v6 1/3] dt-bindings: memory: mediatek: Add SMI reset and clamp for MT8188

[Krzysztof Kozlowski]

On Tue, Apr 08, 2025 at 11:31:54AM GMT, Friday Yang wrote:
> From: "Friday Yang" <friday.yang@mediatek.com>
> 
> Add 'resets' and 'reset-names' properties for SMI LARBs to support
> SMI reset operations.
> On the MediaTek platform, some SMI LARBs are directly connected to
> the SMI Common, while others are connected to the SMI Sub-Common,
> which in turn is connected to the SMI Common. The hardware block
> diagram can be described as follows.
> 
>              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>
> ---
>  .../mediatek,smi-common.yaml                  |  2 ++
>  .../memory-controllers/mediatek,smi-larb.yaml | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 

<form letter>
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions of patchset, under or above your Signed-off-by tag, unless
patch changed significantly (e.g. new properties added to the DT
bindings). Tag is "received", when provided in a message replied to you
on the mailing list. Tools like b4 can help here. However, there's no
need to repost patches *only* to add the tags. The upstream maintainer
will do that for tags received on the version they apply.

Please read:
https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.
</form letter>

Best regards,
Krzysztof

Re: [PATCH v6 3/3] memory: mtk-smi: mt8188: Use devm_pm_runtime_enable

[Krzysztof Kozlowski]

On Tue, Apr 08, 2025 at 11:31:56AM GMT, Friday Yang wrote:
> Replace pm_runtime_enable with the devres-enabled version which
> can trigger pm_runtime_disable.
> 
> Signed-off-by: Friday Yang <friday.yang@mediatek.com>
> ---
>  drivers/memory/mtk-smi.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index f25d46d2ef33..daef6d350419 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -713,16 +713,17 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_link_remove;
> 
> -	pm_runtime_enable(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		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;
> 
>  	return 0;
> 
> -err_pm_disable:
> -	pm_runtime_disable(dev);

You now broke/changed the order of cleanup without any explanation.

Best regards,
Krzysztof

Re: [PATCH v6 3/3] memory: mtk-smi: mt8188: Use devm_pm_runtime_enable

[AngeloGioacchino Del Regno]

Il 08/04/25 08:29, Krzysztof Kozlowski ha scritto:
> On Tue, Apr 08, 2025 at 11:31:56AM GMT, Friday Yang wrote:
>> Replace pm_runtime_enable with the devres-enabled version which
>> can trigger pm_runtime_disable.
>>
>> Signed-off-by: Friday Yang <friday.yang@mediatek.com>
>> ---
>>   drivers/memory/mtk-smi.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
>> index f25d46d2ef33..daef6d350419 100644
>> --- a/drivers/memory/mtk-smi.c
>> +++ b/drivers/memory/mtk-smi.c
>> @@ -713,16 +713,17 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto err_link_remove;
>>
>> -	pm_runtime_enable(dev);
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		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;
>>
>>   	return 0;
>>
>> -err_pm_disable:
>> -	pm_runtime_disable(dev);
> 
> You now broke/changed the order of cleanup without any explanation.
> 
> Best regards,
> Krzysztof
> 

I agree some comment in the commit description saying that the cleanup reordering
doesn't matter in this specific case would've been nice to have, but anyway IMO
it's not a big deal - he didn't break anything, anyway :-)

Instead, the big deal is that Friday forgot to retain Reviewed-by and Acked-by tags
that were released to him on those patches....... :-)

Cheers,
Angelo

Re: [PATCH v6 3/3] memory: mtk-smi: mt8188: Use devm_pm_runtime_enable

[Krzysztof Kozlowski]

On 09/04/2025 10:26, AngeloGioacchino Del Regno wrote:
> Il 08/04/25 08:29, Krzysztof Kozlowski ha scritto:
>> On Tue, Apr 08, 2025 at 11:31:56AM GMT, Friday Yang wrote:
>>> Replace pm_runtime_enable with the devres-enabled version which
>>> can trigger pm_runtime_disable.
>>>
>>> Signed-off-by: Friday Yang <friday.yang@mediatek.com>
>>> ---
>>>   drivers/memory/mtk-smi.c | 16 +++++++++-------
>>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
>>> index f25d46d2ef33..daef6d350419 100644
>>> --- a/drivers/memory/mtk-smi.c
>>> +++ b/drivers/memory/mtk-smi.c
>>> @@ -713,16 +713,17 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>>>   	if (ret)
>>>   		goto err_link_remove;
>>>
>>> -	pm_runtime_enable(dev);
>>> +	ret = devm_pm_runtime_enable(dev);
>>> +	if (ret)
>>> +		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;
>>>
>>>   	return 0;
>>>
>>> -err_pm_disable:
>>> -	pm_runtime_disable(dev);
>>
>> You now broke/changed the order of cleanup without any explanation.
>>
>> Best regards,
>> Krzysztof
>>
> 
> I agree some comment in the commit description saying that the cleanup reordering
> doesn't matter in this specific case would've been nice to have, but anyway IMO
> it's not a big deal - he didn't break anything, anyway :-)

Cleanup orderings are tricky, so are you sure nothing got here called in
incorrect moment? I see that runtime PM will be disabled much later and
what certainty you have that device won't get resumed that time?

Best regards,
Krzysztof

Re: [PATCH v6 3/3] memory: mtk-smi: mt8188: Use devm_pm_runtime_enable

[AngeloGioacchino Del Regno]

Il 09/04/25 11:56, Krzysztof Kozlowski ha scritto:
> On 09/04/2025 10:26, AngeloGioacchino Del Regno wrote:
>> Il 08/04/25 08:29, Krzysztof Kozlowski ha scritto:
>>> On Tue, Apr 08, 2025 at 11:31:56AM GMT, Friday Yang wrote:
>>>> Replace pm_runtime_enable with the devres-enabled version which
>>>> can trigger pm_runtime_disable.
>>>>
>>>> Signed-off-by: Friday Yang <friday.yang@mediatek.com>
>>>> ---
>>>>    drivers/memory/mtk-smi.c | 16 +++++++++-------
>>>>    1 file changed, 9 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
>>>> index f25d46d2ef33..daef6d350419 100644
>>>> --- a/drivers/memory/mtk-smi.c
>>>> +++ b/drivers/memory/mtk-smi.c
>>>> @@ -713,16 +713,17 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>>>>    	if (ret)
>>>>    		goto err_link_remove;
>>>>
>>>> -	pm_runtime_enable(dev);
>>>> +	ret = devm_pm_runtime_enable(dev);
>>>> +	if (ret)
>>>> +		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;
>>>>
>>>>    	return 0;
>>>>
>>>> -err_pm_disable:
>>>> -	pm_runtime_disable(dev);
>>>
>>> You now broke/changed the order of cleanup without any explanation.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> I agree some comment in the commit description saying that the cleanup reordering
>> doesn't matter in this specific case would've been nice to have, but anyway IMO
>> it's not a big deal - he didn't break anything, anyway :-)
> 
> Cleanup orderings are tricky, so are you sure nothing got here called in
> incorrect moment?

Yes.

 >> I see that runtime PM will be disabled much later and
> what certainty you have that device won't get resumed that time?
> 
How can a device that failed to probe be resumed?! Who's going to resume it?! :-)

Also, in the remove phase, all users get removed first, there's no ISR (implies
that there's no isr that will resume this device inadvertently, and other than
no isr - there's no kthread/queue/this/that that could do this), and no nothing.

Moreover, SMI-LARB cannot be removed unless all of the components are unbound;
SMI-Common (be it a common or a sub-common) cannot be removed if SMI-LARB is still
using it.

No I don't see anything that can resume it before devm does its job.

If you do see something though, I'm curious to understand what I'm missing here :-)

Cheers!
Angelo

Re: [PATCH v6 3/3] memory: mtk-smi: mt8188: Use devm_pm_runtime_enable

[Krzysztof Kozlowski]

On 09/04/2025 17:50, AngeloGioacchino Del Regno wrote:
> Il 09/04/25 11:56, Krzysztof Kozlowski ha scritto:
>> On 09/04/2025 10:26, AngeloGioacchino Del Regno wrote:
>>> Il 08/04/25 08:29, Krzysztof Kozlowski ha scritto:
>>>> On Tue, Apr 08, 2025 at 11:31:56AM GMT, Friday Yang wrote:
>>>>> Replace pm_runtime_enable with the devres-enabled version which
>>>>> can trigger pm_runtime_disable.
>>>>>
>>>>> Signed-off-by: Friday Yang <friday.yang@mediatek.com>
>>>>> ---
>>>>>    drivers/memory/mtk-smi.c | 16 +++++++++-------
>>>>>    1 file changed, 9 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
>>>>> index f25d46d2ef33..daef6d350419 100644
>>>>> --- a/drivers/memory/mtk-smi.c
>>>>> +++ b/drivers/memory/mtk-smi.c
>>>>> @@ -713,16 +713,17 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>>>>>    	if (ret)
>>>>>    		goto err_link_remove;
>>>>>
>>>>> -	pm_runtime_enable(dev);
>>>>> +	ret = devm_pm_runtime_enable(dev);
>>>>> +	if (ret)
>>>>> +		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;
>>>>>
>>>>>    	return 0;
>>>>>
>>>>> -err_pm_disable:
>>>>> -	pm_runtime_disable(dev);
>>>>
>>>> You now broke/changed the order of cleanup without any explanation.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> I agree some comment in the commit description saying that the cleanup reordering
>>> doesn't matter in this specific case would've been nice to have, but anyway IMO
>>> it's not a big deal - he didn't break anything, anyway :-)
>>
>> Cleanup orderings are tricky, so are you sure nothing got here called in
>> incorrect moment?
> 
> Yes.
> 
>  >> I see that runtime PM will be disabled much later and
>> what certainty you have that device won't get resumed that time?
>>
> How can a device that failed to probe be resumed?! Who's going to resume it?! :-)

That's unbind path.

> 
> Also, in the remove phase, all users get removed first, there's no ISR (implies
> that there's no isr that will resume this device inadvertently, and other than
> no isr - there's no kthread/queue/this/that that could do this), and no nothing.
> 
> Moreover, SMI-LARB cannot be removed unless all of the components are unbound;
> SMI-Common (be it a common or a sub-common) cannot be removed if SMI-LARB is still
> using it.
> 
> No I don't see anything that can resume it before devm does its job.

so this should be in commit msg... I doubt that author did any
investigation but instead just blindly converted to devm.

> 
> If you do see something though, I'm curious to understand what I'm missing here :-)

You change the order of cleanup and this is known to introduce errors.
Real bugs during probe error paths or removal. Some are tricky to
trigger, but some are obvious and really happening. The easiest to
trigger issues is for devices sharing interrupts (there is even CONFIG
for that). That's why generic recommendation is don't use devm with
shared interrupts. Even more generic recommendation is don't mix devm
with non-devm, but just choose one.

Best regards,
Krzysztof