[PATCH 0/4] MediaTek Runtime Power Management Clocks for PLL

[PATCH 0/4] MediaTek Runtime Power Management Clocks for PLL

[Nicolas Frattaroli]

This series refactors all users of mtk-pll, just so we can enable
runtime power management. This will then allow us to have clock
controllers that depend on other clocks to be on for their control
registers to be functional.

The final use is to add this sort of relationship to the MT8196 mfgpll
clocks, which all need the CLK_TOP_MFG_EB to be on before their control
registers can even be read.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Nicolas Frattaroli (4):
      dt-bindings: clock: mediatek: Add clocks for MT8196 mfgpll
      clk: mediatek: Refactor pll registration to pass device
      clk: mediatek: Pass device to clk_hw_register for PLLs
      clk: mediatek: Add rpm clocks to clk-mt8196-mfg

 .../bindings/clock/mediatek,mt8196-sys-clock.yaml  |  28 ++++++
 drivers/clk/mediatek/clk-mt2701.c                  |   2 +-
 drivers/clk/mediatek/clk-mt2712-apmixedsys.c       |   2 +-
 drivers/clk/mediatek/clk-mt6735-apmixedsys.c       |   4 +-
 drivers/clk/mediatek/clk-mt6765.c                  |   2 +-
 drivers/clk/mediatek/clk-mt6779.c                  |   2 +-
 drivers/clk/mediatek/clk-mt6797.c                  |   2 +-
 drivers/clk/mediatek/clk-mt7622-apmixedsys.c       |   2 +-
 drivers/clk/mediatek/clk-mt7629.c                  |   2 +-
 drivers/clk/mediatek/clk-mt7981-apmixed.c          |   2 +-
 drivers/clk/mediatek/clk-mt7986-apmixed.c          |   2 +-
 drivers/clk/mediatek/clk-mt7988-apmixed.c          |   2 +-
 drivers/clk/mediatek/clk-mt8135-apmixedsys.c       |   3 +-
 drivers/clk/mediatek/clk-mt8167-apmixedsys.c       |   2 +-
 drivers/clk/mediatek/clk-mt8183-apmixedsys.c       |   2 +-
 drivers/clk/mediatek/clk-mt8188-apmixedsys.c       |   2 +-
 drivers/clk/mediatek/clk-mt8195-apusys_pll.c       |   3 +-
 drivers/clk/mediatek/clk-mt8196-apmixedsys.c       |   3 +-
 drivers/clk/mediatek/clk-mt8196-mcu.c              |   2 +-
 drivers/clk/mediatek/clk-mt8196-mfg.c              | 104 +++++++++++++++++----
 drivers/clk/mediatek/clk-mt8196-vlpckgen.c         |   2 +-
 drivers/clk/mediatek/clk-mt8365-apmixedsys.c       |   2 +-
 drivers/clk/mediatek/clk-mt8516-apmixedsys.c       |   2 +-
 drivers/clk/mediatek/clk-pll.c                     |  16 ++--
 drivers/clk/mediatek/clk-pll.h                     |  12 ++-
 drivers/clk/mediatek/clk-pllfh.c                   |   2 +-
 26 files changed, 157 insertions(+), 52 deletions(-)
---
base-commit: 905612298ef4f5fa9f85fbc6825af224f40af70f
change-id: 20250929-mtk-pll-rpm-bf28192dd016

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

[PATCH 1/4] dt-bindings: clock: mediatek: Add clocks for MT8196 mfgpll

[Nicolas Frattaroli]

The clock controllers for mfgpll, mfgpll-sc0, and mfgpll-sc1 all need
CLK_TOP_MFG_EB to be on if their clock control registers are touched in
any way.

This was not known at the time this binding was written, as this
dependency only came to light when I started poking at the MFlexGraphics
hardware, where this undocumented peculiarity made itself known through
SErrors being thrown during register reads.

Add a clocks property to the binding to describe this relationship, and
mark it as required for the affected clocks.

Fixes: dd240e95f1be ("dt-bindings: clock: mediatek: Describe MT8196 clock controllers")
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 .../bindings/clock/mediatek,mt8196-sys-clock.yaml  | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
index 660ab64f390d2e722b7d3e25cf057926da318bc0..41aacd8d5f69050eebdf8392f7b652427632f491 100644
--- a/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
+++ b/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
@@ -45,6 +45,9 @@ properties:
   reg:
     maxItems: 1
 
+  clocks:
+    maxItems: 1
+
   '#clock-cells':
     const: 1
 
@@ -90,6 +93,23 @@ required:
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt8196-mfgpll-pll-ctrl
+              - mediatek,mt8196-mfgpll-sc0-pll-ctrl
+              - mediatek,mt8196-mfgpll-sc1-pll-ctrl
+    then:
+      properties:
+        clocks:
+          items:
+            - description: mfg_eb clock
+      required:
+        - clocks
+
 examples:
   - |
     apmixedsys_clk: syscon@10000800 {
@@ -104,4 +124,12 @@ examples:
         mediatek,hardware-voter = <&scp_hwv>;
         #clock-cells = <1>;
     };
+  - |
+    #include <dt-bindings/clock/mediatek,mt8196-clock.h>
 
+    clock-controller@4b810000 {
+        compatible = "mediatek,mt8196-mfgpll-pll-ctrl", "syscon";
+        reg = <0x4b810000 0x400>;
+        clocks = <&topckgen CLK_TOP_MFG_EB>;
+        #clock-cells = <1>;
+    };

-- 
2.51.0

[PATCH 2/4] clk: mediatek: Refactor pll registration to pass device

[Nicolas Frattaroli]

As it stands, mtk_clk_register_plls takes a struct device_node pointer
as its first argument. This is a tragic happenstance, as it's trivial to
get the device_node from a struct device, but the opposite not so much.
The struct device is a much more useful thing to have passed down.

Refactor mtk_clk_register_plls to take a struct device pointer instead
of a struct device_node pointer, and fix up all users of this function.
This will allow us to extend clk-pll with runtime PM things later.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/clk/mediatek/clk-mt2701.c            | 2 +-
 drivers/clk/mediatek/clk-mt2712-apmixedsys.c | 2 +-
 drivers/clk/mediatek/clk-mt6735-apmixedsys.c | 4 ++--
 drivers/clk/mediatek/clk-mt6765.c            | 2 +-
 drivers/clk/mediatek/clk-mt6779.c            | 2 +-
 drivers/clk/mediatek/clk-mt6797.c            | 2 +-
 drivers/clk/mediatek/clk-mt7622-apmixedsys.c | 2 +-
 drivers/clk/mediatek/clk-mt7629.c            | 2 +-
 drivers/clk/mediatek/clk-mt7981-apmixed.c    | 2 +-
 drivers/clk/mediatek/clk-mt7986-apmixed.c    | 2 +-
 drivers/clk/mediatek/clk-mt7988-apmixed.c    | 2 +-
 drivers/clk/mediatek/clk-mt8135-apmixedsys.c | 3 ++-
 drivers/clk/mediatek/clk-mt8167-apmixedsys.c | 2 +-
 drivers/clk/mediatek/clk-mt8183-apmixedsys.c | 2 +-
 drivers/clk/mediatek/clk-mt8188-apmixedsys.c | 2 +-
 drivers/clk/mediatek/clk-mt8195-apusys_pll.c | 3 ++-
 drivers/clk/mediatek/clk-mt8196-apmixedsys.c | 3 ++-
 drivers/clk/mediatek/clk-mt8196-mcu.c        | 2 +-
 drivers/clk/mediatek/clk-mt8196-mfg.c        | 2 +-
 drivers/clk/mediatek/clk-mt8196-vlpckgen.c   | 2 +-
 drivers/clk/mediatek/clk-mt8365-apmixedsys.c | 2 +-
 drivers/clk/mediatek/clk-mt8516-apmixedsys.c | 2 +-
 drivers/clk/mediatek/clk-pll.c               | 7 ++++---
 drivers/clk/mediatek/clk-pll.h               | 6 +++---
 24 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt2701.c b/drivers/clk/mediatek/clk-mt2701.c
index 1e88ad8b93f4485ad40f842e19c68117e00a2fbe..d9f40fda73d1abc56ebc97ab755bb48bd5f0991f 100644
--- a/drivers/clk/mediatek/clk-mt2701.c
+++ b/drivers/clk/mediatek/clk-mt2701.c
@@ -978,7 +978,7 @@ static int mtk_apmixedsys_init(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	mtk_clk_register_plls(node, apmixed_plls, ARRAY_SIZE(apmixed_plls),
+	mtk_clk_register_plls(&pdev->dev, apmixed_plls, ARRAY_SIZE(apmixed_plls),
 								clk_data);
 	mtk_clk_register_factors(apmixed_fixed_divs, ARRAY_SIZE(apmixed_fixed_divs),
 								clk_data);
diff --git a/drivers/clk/mediatek/clk-mt2712-apmixedsys.c b/drivers/clk/mediatek/clk-mt2712-apmixedsys.c
index a60622d251ff30fe8db2e596d87986a88f854e61..54b18e9f83f8f403460c77d8f5d4ea0737316774 100644
--- a/drivers/clk/mediatek/clk-mt2712-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt2712-apmixedsys.c
@@ -119,7 +119,7 @@ static int clk_mt2712_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	r = mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls), clk_data);
 	if (r)
 		goto free_clk_data;
 
diff --git a/drivers/clk/mediatek/clk-mt6735-apmixedsys.c b/drivers/clk/mediatek/clk-mt6735-apmixedsys.c
index e0949911e8f7da7894b204012caefd0404cf8308..9e30c089a2092472bab889ede419c41890c307a0 100644
--- a/drivers/clk/mediatek/clk-mt6735-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt6735-apmixedsys.c
@@ -93,8 +93,8 @@ static int clk_mt6735_apmixed_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, clk_data);
 
-	ret = mtk_clk_register_plls(pdev->dev.of_node, apmixedsys_plls,
-				   ARRAY_SIZE(apmixedsys_plls), clk_data);
+	ret = mtk_clk_register_plls(&pdev->dev, apmixedsys_plls,
+				    ARRAY_SIZE(apmixedsys_plls), clk_data);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register PLLs: %d\n", ret);
 		return ret;
diff --git a/drivers/clk/mediatek/clk-mt6765.c b/drivers/clk/mediatek/clk-mt6765.c
index d53731e7933f46d88ff180e43eb7163e52fb5b1c..60f6f9fa7dcf279631d0fa2eb30a3bcbadef3225 100644
--- a/drivers/clk/mediatek/clk-mt6765.c
+++ b/drivers/clk/mediatek/clk-mt6765.c
@@ -740,7 +740,7 @@ static int clk_mt6765_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls), clk_data);
 
 	mtk_clk_register_gates(&pdev->dev, node, apmixed_clks,
 			       ARRAY_SIZE(apmixed_clks), clk_data);
diff --git a/drivers/clk/mediatek/clk-mt6779.c b/drivers/clk/mediatek/clk-mt6779.c
index 86732f5acf93407a5aa99bc2f386f0728a06bb9b..4b9dcb910b03f1078212dc7089d7171d05de7e7f 100644
--- a/drivers/clk/mediatek/clk-mt6779.c
+++ b/drivers/clk/mediatek/clk-mt6779.c
@@ -1220,7 +1220,7 @@ static int clk_mt6779_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls), clk_data);
 
 	mtk_clk_register_gates(&pdev->dev, node, apmixed_clks,
 			       ARRAY_SIZE(apmixed_clks), clk_data);
diff --git a/drivers/clk/mediatek/clk-mt6797.c b/drivers/clk/mediatek/clk-mt6797.c
index fb59e71af58e32d9419e036e3dbd28cdaa61cac3..ebf850ac57f540f2317e63dfabe94a953db3ae29 100644
--- a/drivers/clk/mediatek/clk-mt6797.c
+++ b/drivers/clk/mediatek/clk-mt6797.c
@@ -655,7 +655,7 @@ static int mtk_apmixedsys_init(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls), clk_data);
 
 	return of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
 }
diff --git a/drivers/clk/mediatek/clk-mt7622-apmixedsys.c b/drivers/clk/mediatek/clk-mt7622-apmixedsys.c
index 2350592d9a934f3ec8efb0cd8197e4c4fee49697..8a29eaab0cfcb7a389e09f8869b572d5886e2eaf 100644
--- a/drivers/clk/mediatek/clk-mt7622-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt7622-apmixedsys.c
@@ -96,7 +96,7 @@ static int clk_mt7622_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	ret = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	ret = mtk_clk_register_plls(dev, plls, ARRAY_SIZE(plls), clk_data);
 	if (ret)
 		return ret;
 
diff --git a/drivers/clk/mediatek/clk-mt7629.c b/drivers/clk/mediatek/clk-mt7629.c
index baf94e7bea373c59cb6333fdb483d00240b744c7..e154771b1b8bba7378af8a797c81d0784b626e3b 100644
--- a/drivers/clk/mediatek/clk-mt7629.c
+++ b/drivers/clk/mediatek/clk-mt7629.c
@@ -634,7 +634,7 @@ static int mtk_apmixedsys_init(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls),
+	mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls),
 			      clk_data);
 
 	mtk_clk_register_gates(&pdev->dev, node, apmixed_clks,
diff --git a/drivers/clk/mediatek/clk-mt7981-apmixed.c b/drivers/clk/mediatek/clk-mt7981-apmixed.c
index e8211eb4e09e1a645f7e50a1e5814d29030c1757..6606b54fb376983ec7d49b00c2c0d1690c734058 100644
--- a/drivers/clk/mediatek/clk-mt7981-apmixed.c
+++ b/drivers/clk/mediatek/clk-mt7981-apmixed.c
@@ -76,7 +76,7 @@ static int clk_mt7981_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls), clk_data);
 
 	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
 	if (r) {
diff --git a/drivers/clk/mediatek/clk-mt7986-apmixed.c b/drivers/clk/mediatek/clk-mt7986-apmixed.c
index 93751abe6be89784a102a0e5ac629d363ab3baaf..1c79418d08a77acf25cee914fb6573ac1707163e 100644
--- a/drivers/clk/mediatek/clk-mt7986-apmixed.c
+++ b/drivers/clk/mediatek/clk-mt7986-apmixed.c
@@ -74,7 +74,7 @@ static int clk_mt7986_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls), clk_data);
 
 	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
 	if (r) {
diff --git a/drivers/clk/mediatek/clk-mt7988-apmixed.c b/drivers/clk/mediatek/clk-mt7988-apmixed.c
index 63d33a78cb48805f71aa6a74f8ed6b83f3b4fe22..416a4b88d100bb47bdb07e4f72bc13208c8707a7 100644
--- a/drivers/clk/mediatek/clk-mt7988-apmixed.c
+++ b/drivers/clk/mediatek/clk-mt7988-apmixed.c
@@ -86,7 +86,7 @@ static int clk_mt7988_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	r = mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls), clk_data);
 	if (r)
 		goto free_apmixed_data;
 
diff --git a/drivers/clk/mediatek/clk-mt8135-apmixedsys.c b/drivers/clk/mediatek/clk-mt8135-apmixedsys.c
index bdadc35c64cbd8987061c4442b8ff2f5fe50cc32..19e4ee489ec3905e92674ed0813a9f60f9c28209 100644
--- a/drivers/clk/mediatek/clk-mt8135-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8135-apmixedsys.c
@@ -57,7 +57,8 @@ static int clk_mt8135_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	ret = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	ret = mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls),
+				    clk_data);
 	if (ret)
 		goto free_clk_data;
 
diff --git a/drivers/clk/mediatek/clk-mt8167-apmixedsys.c b/drivers/clk/mediatek/clk-mt8167-apmixedsys.c
index adf576786696e0962dfd5147dfc8897bfaa48054..fb6c21bbeef81a383b56c8fada1799e0680676e5 100644
--- a/drivers/clk/mediatek/clk-mt8167-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8167-apmixedsys.c
@@ -105,7 +105,7 @@ static int clk_mt8167_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	ret = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	ret = mtk_clk_register_plls(dev, plls, ARRAY_SIZE(plls), clk_data);
 	if (ret)
 		return ret;
 
diff --git a/drivers/clk/mediatek/clk-mt8183-apmixedsys.c b/drivers/clk/mediatek/clk-mt8183-apmixedsys.c
index 551adbfd7ac9309bbc4f9beefe4f26230514f062..6242d4f5376e79346b2219b0a35cf0c5ad755e49 100644
--- a/drivers/clk/mediatek/clk-mt8183-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8183-apmixedsys.c
@@ -155,7 +155,7 @@ static int clk_mt8183_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	ret = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	ret = mtk_clk_register_plls(dev, plls, ARRAY_SIZE(plls), clk_data);
 	if (ret)
 		return ret;
 
diff --git a/drivers/clk/mediatek/clk-mt8188-apmixedsys.c b/drivers/clk/mediatek/clk-mt8188-apmixedsys.c
index 21d7a9a2ab1af64cca6962960418d44c81dc664a..a1de596bff9945ca938504391e3e33a4987d3a63 100644
--- a/drivers/clk/mediatek/clk-mt8188-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8188-apmixedsys.c
@@ -106,7 +106,7 @@ static int clk_mt8188_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	r = mtk_clk_register_plls(&pdev->dev, plls, ARRAY_SIZE(plls), clk_data);
 	if (r)
 		goto free_apmixed_data;
 
diff --git a/drivers/clk/mediatek/clk-mt8195-apusys_pll.c b/drivers/clk/mediatek/clk-mt8195-apusys_pll.c
index 8b45a3fad02f18df30e4c2ce2ba5b6338eae321f..a2d98ed58e34866b3d68bd0f85bde339c258d822 100644
--- a/drivers/clk/mediatek/clk-mt8195-apusys_pll.c
+++ b/drivers/clk/mediatek/clk-mt8195-apusys_pll.c
@@ -66,7 +66,8 @@ static int clk_mt8195_apusys_pll_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(node, apusys_plls, ARRAY_SIZE(apusys_plls), clk_data);
+	r = mtk_clk_register_plls(&pdev->dev, apusys_plls,
+				  ARRAY_SIZE(apusys_plls), clk_data);
 	if (r)
 		goto free_apusys_pll_data;
 
diff --git a/drivers/clk/mediatek/clk-mt8196-apmixedsys.c b/drivers/clk/mediatek/clk-mt8196-apmixedsys.c
index 617f5449b88b8bcaf282e8ed8593b52413a233a8..c4ebb0170b82b979fbe7f03925f205325247d55d 100644
--- a/drivers/clk/mediatek/clk-mt8196-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8196-apmixedsys.c
@@ -152,7 +152,8 @@ static int clk_mt8196_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(node, mcd->clks, mcd->num_clks, clk_data);
+	r = mtk_clk_register_plls(&pdev->dev, mcd->clks, mcd->num_clks,
+				  clk_data);
 	if (r)
 		goto free_apmixed_data;
 
diff --git a/drivers/clk/mediatek/clk-mt8196-mcu.c b/drivers/clk/mediatek/clk-mt8196-mcu.c
index 5cbcc411ae734c82b97bf099a645cb6aaa31d9c3..13642fc673c267a66027d1fa7073c9cfed68c682 100644
--- a/drivers/clk/mediatek/clk-mt8196-mcu.c
+++ b/drivers/clk/mediatek/clk-mt8196-mcu.c
@@ -122,7 +122,7 @@ static int clk_mt8196_mcu_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(node, plls, num_plls, clk_data);
+	r = mtk_clk_register_plls(&pdev->dev, plls, num_plls, clk_data);
 	if (r)
 		goto free_clk_data;
 
diff --git a/drivers/clk/mediatek/clk-mt8196-mfg.c b/drivers/clk/mediatek/clk-mt8196-mfg.c
index ae1eb9de79ae2992b10a400c75e2e0324b100f66..8e09c0f7b7548f8e286671cea2dac64530b8ce47 100644
--- a/drivers/clk/mediatek/clk-mt8196-mfg.c
+++ b/drivers/clk/mediatek/clk-mt8196-mfg.c
@@ -105,7 +105,7 @@ static int clk_mt8196_mfg_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(node, plls, num_plls, clk_data);
+	r = mtk_clk_register_plls(&pdev->dev, plls, num_plls, clk_data);
 	if (r)
 		goto free_clk_data;
 
diff --git a/drivers/clk/mediatek/clk-mt8196-vlpckgen.c b/drivers/clk/mediatek/clk-mt8196-vlpckgen.c
index d59a8a9d98550e897d18031d9bb814aa96d3cf57..7dcc164627c578ca93377425c3b21b46da4b4c28 100644
--- a/drivers/clk/mediatek/clk-mt8196-vlpckgen.c
+++ b/drivers/clk/mediatek/clk-mt8196-vlpckgen.c
@@ -664,7 +664,7 @@ static int clk_mt8196_vlp_probe(struct platform_device *pdev)
 	if (r)
 		goto unregister_factors;
 
-	r = mtk_clk_register_plls(node, vlp_plls, ARRAY_SIZE(vlp_plls),
+	r = mtk_clk_register_plls(dev, vlp_plls, ARRAY_SIZE(vlp_plls),
 				  clk_data);
 	if (r)
 		goto unregister_muxes;
diff --git a/drivers/clk/mediatek/clk-mt8365-apmixedsys.c b/drivers/clk/mediatek/clk-mt8365-apmixedsys.c
index f41b991a0178af3067b19a693512ec922af48e07..e331aa28a4bd58baf48a4aae1601cc80fc5661ac 100644
--- a/drivers/clk/mediatek/clk-mt8365-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8365-apmixedsys.c
@@ -133,7 +133,7 @@ static int clk_mt8365_apmixed_probe(struct platform_device *pdev)
 		return PTR_ERR(hw);
 	clk_data->hws[CLK_APMIXED_USB20_EN] = hw;
 
-	ret = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	ret = mtk_clk_register_plls(dev, plls, ARRAY_SIZE(plls), clk_data);
 	if (ret)
 		return ret;
 
diff --git a/drivers/clk/mediatek/clk-mt8516-apmixedsys.c b/drivers/clk/mediatek/clk-mt8516-apmixedsys.c
index edd9174d2f2ff97a0c1198caa2a0b9c1ca40ffd2..2a6206cae2f087ff06fe60a6cf96a0fa3143e567 100644
--- a/drivers/clk/mediatek/clk-mt8516-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8516-apmixedsys.c
@@ -87,7 +87,7 @@ static int clk_mt8516_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	ret = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	ret = mtk_clk_register_plls(dev, plls, ARRAY_SIZE(plls), clk_data);
 	if (ret)
 		return ret;
 
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index cd2b6ce551c6b0333cbe0a4f0d155ba2411f757a..5caf91ae9ddbe4f4d7052864adf0a5a70bda66bc 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -11,6 +11,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 
 #include "clk-pll.h"
@@ -404,7 +405,7 @@ void mtk_clk_unregister_pll(struct clk_hw *hw)
 	kfree(pll);
 }
 
-int mtk_clk_register_plls(struct device_node *node,
+int mtk_clk_register_plls(struct device *dev,
 			  const struct mtk_pll_data *plls, int num_plls,
 			  struct clk_hw_onecell_data *clk_data)
 {
@@ -412,7 +413,7 @@ int mtk_clk_register_plls(struct device_node *node,
 	int i;
 	struct clk_hw *hw;
 
-	base = of_iomap(node, 0);
+	base = of_iomap(dev->of_node, 0);
 	if (!base) {
 		pr_err("%s(): ioremap failed\n", __func__);
 		return -EINVAL;
@@ -423,7 +424,7 @@ int mtk_clk_register_plls(struct device_node *node,
 
 		if (!IS_ERR_OR_NULL(clk_data->hws[pll->id])) {
 			pr_warn("%pOF: Trying to register duplicate clock ID: %d\n",
-				node, pll->id);
+				dev->of_node, pll->id);
 			continue;
 		}
 
diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
index d71c150ce83e4bb2fe78290c2d5570a90084246d..0e2b94b9cd4b56adceee3b04e9ab24c2526471da 100644
--- a/drivers/clk/mediatek/clk-pll.h
+++ b/drivers/clk/mediatek/clk-pll.h
@@ -78,9 +78,9 @@ struct mtk_clk_pll {
 	const struct mtk_pll_data *data;
 };
 
-int mtk_clk_register_plls(struct device_node *node,
-			  const struct mtk_pll_data *plls, int num_plls,
-			  struct clk_hw_onecell_data *clk_data);
+int mtk_clk_register_plls(struct device *dev, const struct mtk_pll_data *plls,
+			  int num_plls, struct clk_hw_onecell_data *clk_data);
+
 void mtk_clk_unregister_plls(const struct mtk_pll_data *plls, int num_plls,
 			     struct clk_hw_onecell_data *clk_data);
 

-- 
2.51.0

[PATCH 3/4] clk: mediatek: Pass device to clk_hw_register for PLLs

[Nicolas Frattaroli]

Passing the struct device pointer to clk_hw_register allows for runtime
power management to work for the registered clocks. However, the
mediatek PLL clocks do not do this.

Change this by adding a struct device pointer argument to
mtk_clk_register_pll, and fix up the only other user of it. Also add a
new member to the struct mtk_clk_pll for the struct device pointer,
which is set by mtk_clk_register_pll and is used by
mtk_clk_register_pll_ops.

If mtk_clk_register_pll is called with a NULL struct device pointer,
then everything still works as expected; the clock core will simply
treat them as previously, i.e. without runtime power management.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/clk/mediatek/clk-pll.c   | 9 ++++++---
 drivers/clk/mediatek/clk-pll.h   | 4 +++-
 drivers/clk/mediatek/clk-pllfh.c | 2 +-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 5caf91ae9ddbe4f4d7052864adf0a5a70bda66bc..c4f9c06e5133dbc5902f261353c197fbde95e54d 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -366,7 +366,7 @@ struct clk_hw *mtk_clk_register_pll_ops(struct mtk_clk_pll *pll,
 		init.parent_names = &parent_name;
 	init.num_parents = 1;
 
-	ret = clk_hw_register(NULL, &pll->hw);
+	ret = clk_hw_register(pll->dev, &pll->hw);
 
 	if (ret)
 		return ERR_PTR(ret);
@@ -374,7 +374,8 @@ struct clk_hw *mtk_clk_register_pll_ops(struct mtk_clk_pll *pll,
 	return &pll->hw;
 }
 
-struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
+struct clk_hw *mtk_clk_register_pll(struct device *dev,
+				    const struct mtk_pll_data *data,
 				    void __iomem *base)
 {
 	struct mtk_clk_pll *pll;
@@ -385,6 +386,8 @@ struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
 	if (!pll)
 		return ERR_PTR(-ENOMEM);
 
+	pll->dev = dev;
+
 	hw = mtk_clk_register_pll_ops(pll, data, base, pll_ops);
 	if (IS_ERR(hw))
 		kfree(pll);
@@ -428,7 +431,7 @@ int mtk_clk_register_plls(struct device *dev,
 			continue;
 		}
 
-		hw = mtk_clk_register_pll(pll, base);
+		hw = mtk_clk_register_pll(dev, pll, base);
 
 		if (IS_ERR(hw)) {
 			pr_err("Failed to register clk %s: %pe\n", pll->name,
diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
index 0e2b94b9cd4b56adceee3b04e9ab24c2526471da..0f2a1d19eea78b7390b221af47016eb9897f3596 100644
--- a/drivers/clk/mediatek/clk-pll.h
+++ b/drivers/clk/mediatek/clk-pll.h
@@ -63,6 +63,7 @@ struct mtk_pll_data {
  */
 
 struct mtk_clk_pll {
+	struct device *dev;
 	struct clk_hw	hw;
 	void __iomem	*base_addr;
 	void __iomem	*pd_addr;
@@ -110,7 +111,8 @@ struct clk_hw *mtk_clk_register_pll_ops(struct mtk_clk_pll *pll,
 					const struct mtk_pll_data *data,
 					void __iomem *base,
 					const struct clk_ops *pll_ops);
-struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
+struct clk_hw *mtk_clk_register_pll(struct device *dev,
+				    const struct mtk_pll_data *data,
 				    void __iomem *base);
 void mtk_clk_unregister_pll(struct clk_hw *hw);
 
diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
index 83630ee07ee976bf980c8cf2dd35ea24c1b40821..62bfe4a480f14a0a742fb094aff0e6d1a79fe0c3 100644
--- a/drivers/clk/mediatek/clk-pllfh.c
+++ b/drivers/clk/mediatek/clk-pllfh.c
@@ -220,7 +220,7 @@ int mtk_clk_register_pllfhs(struct device_node *node,
 		if (use_fhctl)
 			hw = mtk_clk_register_pllfh(pll, pllfh, base);
 		else
-			hw = mtk_clk_register_pll(pll, base);
+			hw = mtk_clk_register_pll(NULL, pll, base);
 
 		if (IS_ERR(hw)) {
 			pr_err("Failed to register %s clk %s: %ld\n",

-- 
2.51.0

[PATCH 4/4] clk: mediatek: Add rpm clocks to clk-mt8196-mfg

[Nicolas Frattaroli]

The mfgpll clocks on mt8196 require that the MFG's EB clock is on if
their control registers are touched in any way at all. Failing to ensure
this results in a pleasant SError interrupt if the EB clock happens to
be off.

To achieve this, leverage the CCF core's runtime power management
support. Define the necessary suspend/resume callbacks, add the
necessary code to get RPM clocks from the DT, and make sure RPM is
enabled before clock registration happens.

For the RPM callbacks to really make much sense at all, we change the
drvdata from clk_data to a new private struct, as is common in drivers
across the Linux kernel.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/clk/mediatek/clk-mt8196-mfg.c | 104 +++++++++++++++++++++++++++-------
 drivers/clk/mediatek/clk-pll.h        |   2 +
 2 files changed, 87 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8196-mfg.c b/drivers/clk/mediatek/clk-mt8196-mfg.c
index 8e09c0f7b7548f8e286671cea2dac64530b8ce47..64cc0c037f62d7eab8d0e7fc00c05d122bf4130c 100644
--- a/drivers/clk/mediatek/clk-mt8196-mfg.c
+++ b/drivers/clk/mediatek/clk-mt8196-mfg.c
@@ -13,6 +13,7 @@
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 
 #include "clk-mtk.h"
 #include "clk-pll.h"
@@ -38,7 +39,7 @@
 	    _flags, _rst_bar_mask,				\
 	    _pd_reg, _pd_shift, _tuner_reg,			\
 	    _tuner_en_reg, _tuner_en_bit,			\
-	    _pcw_reg, _pcw_shift, _pcwbits) {			\
+	    _pcw_reg, _pcw_shift, _pcwbits, _rpm_clks) {	\
 		.id = _id,					\
 		.name = _name,					\
 		.reg = _reg,					\
@@ -58,26 +59,60 @@
 		.pcw_shift = _pcw_shift,			\
 		.pcwbits = _pcwbits,				\
 		.pcwibits = MT8196_INTEGER_BITS,		\
+		.rpm_clk_names = _rpm_clks,			\
+		.num_rpm_clks = ARRAY_SIZE(_rpm_clks),		\
 	}
 
+static const char * const mfgpll_rpm_clk_names[] = {
+	NULL
+};
+
 static const struct mtk_pll_data mfg_ao_plls[] = {
 	PLL(CLK_MFG_AO_MFGPLL, "mfgpll", MFGPLL_CON0, MFGPLL_CON0, 0, 0, 0,
-	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0,
-	    MFGPLL_CON1, 0, 22),
+	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0, MFGPLL_CON1, 0, 22,
+	    mfgpll_rpm_clk_names),
 };
 
 static const struct mtk_pll_data mfgsc0_ao_plls[] = {
 	PLL(CLK_MFGSC0_AO_MFGPLL_SC0, "mfgpll-sc0", MFGPLL_SC0_CON0,
 	    MFGPLL_SC0_CON0, 0, 0, 0, BIT(0), MFGPLL_SC0_CON1, 24, 0, 0, 0,
-	    MFGPLL_SC0_CON1, 0, 22),
+	    MFGPLL_SC0_CON1, 0, 22, mfgpll_rpm_clk_names),
 };
 
 static const struct mtk_pll_data mfgsc1_ao_plls[] = {
 	PLL(CLK_MFGSC1_AO_MFGPLL_SC1, "mfgpll-sc1", MFGPLL_SC1_CON0,
 	    MFGPLL_SC1_CON0, 0, 0, 0, BIT(0), MFGPLL_SC1_CON1, 24, 0, 0, 0,
-	    MFGPLL_SC1_CON1, 0, 22),
+	    MFGPLL_SC1_CON1, 0, 22, mfgpll_rpm_clk_names),
 };
 
+struct clk_mt8196_mfg {
+	struct clk_hw_onecell_data *clk_data;
+	struct clk_bulk_data *rpm_clks;
+	unsigned int num_rpm_clks;
+};
+
+static int __maybe_unused clk_mt8196_mfg_resume(struct device *dev)
+{
+	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
+
+	if (!clk_mfg || !clk_mfg->rpm_clks)
+		return 0;
+
+	return clk_bulk_prepare_enable(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
+}
+
+static int __maybe_unused clk_mt8196_mfg_suspend(struct device *dev)
+{
+	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
+
+	if (!clk_mfg || !clk_mfg->rpm_clks)
+		return 0;
+
+	clk_bulk_disable_unprepare(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
+
+	return 0;
+}
+
 static const struct of_device_id of_match_clk_mt8196_mfg[] = {
 	{ .compatible = "mediatek,mt8196-mfgpll-pll-ctrl",
 	  .data = &mfg_ao_plls },
@@ -92,35 +127,60 @@ MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mfg);
 static int clk_mt8196_mfg_probe(struct platform_device *pdev)
 {
 	const struct mtk_pll_data *plls;
-	struct clk_hw_onecell_data *clk_data;
+	struct clk_mt8196_mfg *clk_mfg;
 	struct device_node *node = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
 	const int num_plls = 1;
-	int r;
+	int r, i;
 
-	plls = of_device_get_match_data(&pdev->dev);
+	plls = of_device_get_match_data(dev);
 	if (!plls)
 		return -EINVAL;
 
-	clk_data = mtk_alloc_clk_data(num_plls);
-	if (!clk_data)
+	clk_mfg = devm_kzalloc(dev, sizeof(*clk_mfg), GFP_KERNEL);
+	if (!clk_mfg)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(&pdev->dev, plls, num_plls, clk_data);
+	clk_mfg->num_rpm_clks = plls[0].num_rpm_clks;
+
+	if (clk_mfg->num_rpm_clks) {
+		clk_mfg->rpm_clks = devm_kcalloc(dev, clk_mfg->num_rpm_clks,
+						 sizeof(*clk_mfg->rpm_clks),
+						 GFP_KERNEL);
+		if (!clk_mfg->rpm_clks)
+			return -ENOMEM;
+
+		for (i = 0; i < clk_mfg->num_rpm_clks; i++)
+			clk_mfg->rpm_clks->id = plls[0].rpm_clk_names[i];
+
+		r = devm_clk_bulk_get(dev, clk_mfg->num_rpm_clks,
+				      clk_mfg->rpm_clks);
+		if (r)
+			return r;
+	}
+
+	clk_mfg->clk_data = mtk_alloc_clk_data(num_plls);
+	if (!clk_mfg->clk_data)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, clk_mfg);
+	pm_runtime_enable(dev);
+
+	r = mtk_clk_register_plls(dev, plls, num_plls, clk_mfg->clk_data);
 	if (r)
 		goto free_clk_data;
 
-	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
+	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
+				   clk_mfg->clk_data);
 	if (r)
 		goto unregister_plls;
 
-	platform_set_drvdata(pdev, clk_data);
-
 	return r;
 
 unregister_plls:
-	mtk_clk_unregister_plls(plls, num_plls, clk_data);
+	mtk_clk_unregister_plls(plls, num_plls, clk_mfg->clk_data);
 free_clk_data:
-	mtk_free_clk_data(clk_data);
+	mtk_free_clk_data(clk_mfg->clk_data);
 
 	return r;
 }
@@ -128,20 +188,26 @@ static int clk_mt8196_mfg_probe(struct platform_device *pdev)
 static void clk_mt8196_mfg_remove(struct platform_device *pdev)
 {
 	const struct mtk_pll_data *plls = of_device_get_match_data(&pdev->dev);
-	struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
+	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(&pdev->dev);
 	struct device_node *node = pdev->dev.of_node;
 
 	of_clk_del_provider(node);
-	mtk_clk_unregister_plls(plls, 1, clk_data);
-	mtk_free_clk_data(clk_data);
+	mtk_clk_unregister_plls(plls, 1, clk_mfg->clk_data);
+	mtk_free_clk_data(clk_mfg->clk_data);
 }
 
+static DEFINE_RUNTIME_DEV_PM_OPS(clk_mt8196_mfg_pm_ops,
+				 clk_mt8196_mfg_suspend,
+				 clk_mt8196_mfg_resume,
+				 NULL);
+
 static struct platform_driver clk_mt8196_mfg_drv = {
 	.probe = clk_mt8196_mfg_probe,
 	.remove = clk_mt8196_mfg_remove,
 	.driver = {
 		.name = "clk-mt8196-mfg",
 		.of_match_table = of_match_clk_mt8196_mfg,
+		.pm = pm_ptr(&clk_mt8196_mfg_pm_ops),
 	},
 };
 module_platform_driver(clk_mt8196_mfg_drv);
diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
index 0f2a1d19eea78b7390b221af47016eb9897f3596..82b86b849a67359d8f23d828f50422081c2747e3 100644
--- a/drivers/clk/mediatek/clk-pll.h
+++ b/drivers/clk/mediatek/clk-pll.h
@@ -53,6 +53,8 @@ struct mtk_pll_data {
 	u8 pll_en_bit; /* Assume 0, indicates BIT(0) by default */
 	u8 pcw_chg_bit;
 	u8 fenc_sta_bit;
+	const char * const *rpm_clk_names;
+	unsigned int num_rpm_clks;
 };
 
 /*

-- 
2.51.0

Re: [PATCH 1/4] dt-bindings: clock: mediatek: Add clocks for MT8196 mfgpll

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

On Mon, Sep 29, 2025 at 02:13:20PM +0200, Nicolas Frattaroli wrote:
> The clock controllers for mfgpll, mfgpll-sc0, and mfgpll-sc1 all need
> CLK_TOP_MFG_EB to be on if their clock control registers are touched in
> any way.
> 
> This was not known at the time this binding was written, as this
> dependency only came to light when I started poking at the MFlexGraphics
> hardware, where this undocumented peculiarity made itself known through
> SErrors being thrown during register reads.
> 
> Add a clocks property to the binding to describe this relationship, and
> mark it as required for the affected clocks.
> 
> Fixes: dd240e95f1be ("dt-bindings: clock: mediatek: Describe MT8196 clock controllers")
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  .../bindings/clock/mediatek,mt8196-sys-clock.yaml  | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> index 660ab64f390d2e722b7d3e25cf057926da318bc0..41aacd8d5f69050eebdf8392f7b652427632f491 100644
> --- a/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> @@ -45,6 +45,9 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  clocks:
> +    maxItems: 1
> +
>    '#clock-cells':
>      const: 1
>  
> @@ -90,6 +93,23 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - mediatek,mt8196-mfgpll-pll-ctrl
> +              - mediatek,mt8196-mfgpll-sc0-pll-ctrl
> +              - mediatek,mt8196-mfgpll-sc1-pll-ctrl
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: mfg_eb clock
> +      required:
> +        - clocks

Don't you want an else: properties: clocks: false here?

> +
>  examples:
>    - |
>      apmixedsys_clk: syscon@10000800 {
> @@ -104,4 +124,12 @@ examples:
>          mediatek,hardware-voter = <&scp_hwv>;
>          #clock-cells = <1>;
>      };
> +  - |
> +    #include <dt-bindings/clock/mediatek,mt8196-clock.h>
>  
> +    clock-controller@4b810000 {
> +        compatible = "mediatek,mt8196-mfgpll-pll-ctrl", "syscon";
> +        reg = <0x4b810000 0x400>;
> +        clocks = <&topckgen CLK_TOP_MFG_EB>;
> +        #clock-cells = <1>;
> +    };
> 
> -- 
> 2.51.0
> 

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

Re: [PATCH 1/4] dt-bindings: clock: mediatek: Add clocks for MT8196 mfgpll

[Nicolas Frattaroli]

On Monday, 29 September 2025 19:31:36 Central European Summer Time Conor Dooley wrote:
> On Mon, Sep 29, 2025 at 02:13:20PM +0200, Nicolas Frattaroli wrote:
> > The clock controllers for mfgpll, mfgpll-sc0, and mfgpll-sc1 all need
> > CLK_TOP_MFG_EB to be on if their clock control registers are touched in
> > any way.
> > 
> > This was not known at the time this binding was written, as this
> > dependency only came to light when I started poking at the MFlexGraphics
> > hardware, where this undocumented peculiarity made itself known through
> > SErrors being thrown during register reads.
> > 
> > Add a clocks property to the binding to describe this relationship, and
> > mark it as required for the affected clocks.
> > 
> > Fixes: dd240e95f1be ("dt-bindings: clock: mediatek: Describe MT8196 clock controllers")
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  .../bindings/clock/mediatek,mt8196-sys-clock.yaml  | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> > index 660ab64f390d2e722b7d3e25cf057926da318bc0..41aacd8d5f69050eebdf8392f7b652427632f491 100644
> > --- a/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> > +++ b/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> > @@ -45,6 +45,9 @@ properties:
> >    reg:
> >      maxItems: 1
> >  
> > +  clocks:
> > +    maxItems: 1
> > +
> >    '#clock-cells':
> >      const: 1
> >  
> > @@ -90,6 +93,23 @@ required:
> >  
> >  additionalProperties: false
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - mediatek,mt8196-mfgpll-pll-ctrl
> > +              - mediatek,mt8196-mfgpll-sc0-pll-ctrl
> > +              - mediatek,mt8196-mfgpll-sc1-pll-ctrl
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: mfg_eb clock
> > +      required:
> > +        - clocks
> 
> Don't you want an else: properties: clocks: false here?

Possibly. I'm never quite sure how strict bindings should be when
it comes to stuff like this. On the one hand, none of the other
compatibles described in it use any clocks that we know of
right now.

On the other, if we have a second set of compatibles that also
needs clocks, but in a different way, would we repeat that for
each such if/then condition? Or would be reformulate this as
some oneOf/anyOf construct specifically for the clock property?

Kind regards,
Nicolas Frattaroli

> 
> > +
> >  examples:
> >    - |
> >      apmixedsys_clk: syscon@10000800 {
> > @@ -104,4 +124,12 @@ examples:
> >          mediatek,hardware-voter = <&scp_hwv>;
> >          #clock-cells = <1>;
> >      };
> > +  - |
> > +    #include <dt-bindings/clock/mediatek,mt8196-clock.h>
> >  
> > +    clock-controller@4b810000 {
> > +        compatible = "mediatek,mt8196-mfgpll-pll-ctrl", "syscon";
> > +        reg = <0x4b810000 0x400>;
> > +        clocks = <&topckgen CLK_TOP_MFG_EB>;
> > +        #clock-cells = <1>;
> > +    };
> > 
> 

Re: [PATCH 1/4] dt-bindings: clock: mediatek: Add clocks for MT8196 mfgpll

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3329 bytes --]

On Tue, Sep 30, 2025 at 05:57:00PM +0200, Nicolas Frattaroli wrote:
> On Monday, 29 September 2025 19:31:36 Central European Summer Time Conor Dooley wrote:
> > On Mon, Sep 29, 2025 at 02:13:20PM +0200, Nicolas Frattaroli wrote:
> > > The clock controllers for mfgpll, mfgpll-sc0, and mfgpll-sc1 all need
> > > CLK_TOP_MFG_EB to be on if their clock control registers are touched in
> > > any way.
> > > 
> > > This was not known at the time this binding was written, as this
> > > dependency only came to light when I started poking at the MFlexGraphics
> > > hardware, where this undocumented peculiarity made itself known through
> > > SErrors being thrown during register reads.
> > > 
> > > Add a clocks property to the binding to describe this relationship, and
> > > mark it as required for the affected clocks.
> > > 
> > > Fixes: dd240e95f1be ("dt-bindings: clock: mediatek: Describe MT8196 clock controllers")
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > >  .../bindings/clock/mediatek,mt8196-sys-clock.yaml  | 28 ++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> > > index 660ab64f390d2e722b7d3e25cf057926da318bc0..41aacd8d5f69050eebdf8392f7b652427632f491 100644
> > > --- a/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> > > +++ b/Documentation/devicetree/bindings/clock/mediatek,mt8196-sys-clock.yaml
> > > @@ -45,6 +45,9 @@ properties:
> > >    reg:
> > >      maxItems: 1
> > >  
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > >    '#clock-cells':
> > >      const: 1
> > >  
> > > @@ -90,6 +93,23 @@ required:
> > >  
> > >  additionalProperties: false
> > >  
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - mediatek,mt8196-mfgpll-pll-ctrl
> > > +              - mediatek,mt8196-mfgpll-sc0-pll-ctrl
> > > +              - mediatek,mt8196-mfgpll-sc1-pll-ctrl
> > > +    then:
> > > +      properties:
> > > +        clocks:
> > > +          items:
> > > +            - description: mfg_eb clock
> > > +      required:
> > > +        - clocks
> > 
> > Don't you want an else: properties: clocks: false here?
> 
> Possibly. I'm never quite sure how strict bindings should be when
> it comes to stuff like this. On the one hand, none of the other
> compatibles described in it use any clocks that we know of
> right now.

It's a bit case-by-case I suppose, but anything that is invalid and can
be easily prevented should be. Particularly in cases where information
about what's correct is harder to find.

> On the other, if we have a second set of compatibles that also
> needs clocks, but in a different way, would we repeat that for
> each such if/then condition? Or would be reformulate this as
> some oneOf/anyOf construct specifically for the clock property?

Depends on what's simpler to do. Sometimes it's better to have if/then
on a per property basis and sometimes separate entries under the allOf
per platform is. Usually depends on how unique each platform is.

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

Re: [PATCH 3/4] clk: mediatek: Pass device to clk_hw_register for PLLs

[AngeloGioacchino Del Regno]

Il 29/09/25 14:13, Nicolas Frattaroli ha scritto:
> Passing the struct device pointer to clk_hw_register allows for runtime
> power management to work for the registered clocks. However, the
> mediatek PLL clocks do not do this.
> 
> Change this by adding a struct device pointer argument to
> mtk_clk_register_pll, and fix up the only other user of it. Also add a
> new member to the struct mtk_clk_pll for the struct device pointer,
> which is set by mtk_clk_register_pll and is used by
> mtk_clk_register_pll_ops.
> 
> If mtk_clk_register_pll is called with a NULL struct device pointer,
> then everything still works as expected; the clock core will simply
> treat them as previously, i.e. without runtime power management.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>   drivers/clk/mediatek/clk-pll.c   | 9 ++++++---
>   drivers/clk/mediatek/clk-pll.h   | 4 +++-
>   drivers/clk/mediatek/clk-pllfh.c | 2 +-
>   3 files changed, 10 insertions(+), 5 deletions(-)
> 

..snip..

> diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
> index 83630ee07ee976bf980c8cf2dd35ea24c1b40821..62bfe4a480f14a0a742fb094aff0e6d1a79fe0c3 100644
> --- a/drivers/clk/mediatek/clk-pllfh.c
> +++ b/drivers/clk/mediatek/clk-pllfh.c
> @@ -220,7 +220,7 @@ int mtk_clk_register_pllfhs(struct device_node *node,
>   		if (use_fhctl)
>   			hw = mtk_clk_register_pllfh(pll, pllfh, base);
>   		else
> -			hw = mtk_clk_register_pll(pll, base);
> +			hw = mtk_clk_register_pll(NULL, pll, base);
>   

Seriously, you've done all that work in patch [2/4], and now you're doing that
just only with PLLFH?

Nicolas, c'mon. It's just 5 minutes ahead. :-D

Cheers,
Angelo

>   		if (IS_ERR(hw)) {
>   			pr_err("Failed to register %s clk %s: %ld\n",
> 

Re: [PATCH 2/4] clk: mediatek: Refactor pll registration to pass device

[AngeloGioacchino Del Regno]

Il 29/09/25 14:13, Nicolas Frattaroli ha scritto:
> As it stands, mtk_clk_register_plls takes a struct device_node pointer
> as its first argument. This is a tragic happenstance, as it's trivial to
> get the device_node from a struct device, but the opposite not so much.
> The struct device is a much more useful thing to have passed down.
> 
> Refactor mtk_clk_register_plls to take a struct device pointer instead
> of a struct device_node pointer, and fix up all users of this function.
> This will allow us to extend clk-pll with runtime PM things later.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Thanks for doing that - this exact thing has been in my backlog for a
very long time now, and you've done it before I could, and without even
knowing that this was in my plans.

Perfect.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 4/4] clk: mediatek: Add rpm clocks to clk-mt8196-mfg

[AngeloGioacchino Del Regno]

Il 29/09/25 14:13, Nicolas Frattaroli ha scritto:
> The mfgpll clocks on mt8196 require that the MFG's EB clock is on if
> their control registers are touched in any way at all.

....so, the MFGPLL clocks are children of EB?

Why are you using such a convoluted way of adding a parent clock to the MFGPLL
instead of just
---->   `.parent_name = "mfg_eb"`  <-----

???????

Cheers,
Angelo

> Failing to ensure
> this results in a pleasant SError interrupt if the EB clock happens to
> be off.
> 
> To achieve this, leverage the CCF core's runtime power management
> support. Define the necessary suspend/resume callbacks, add the
> necessary code to get RPM clocks from the DT, and make sure RPM is
> enabled before clock registration happens.
> 
> For the RPM callbacks to really make much sense at all, we change the
> drvdata from clk_data to a new private struct, as is common in drivers
> across the Linux kernel.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>   drivers/clk/mediatek/clk-mt8196-mfg.c | 104 +++++++++++++++++++++++++++-------
>   drivers/clk/mediatek/clk-pll.h        |   2 +
>   2 files changed, 87 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8196-mfg.c b/drivers/clk/mediatek/clk-mt8196-mfg.c
> index 8e09c0f7b7548f8e286671cea2dac64530b8ce47..64cc0c037f62d7eab8d0e7fc00c05d122bf4130c 100644
> --- a/drivers/clk/mediatek/clk-mt8196-mfg.c
> +++ b/drivers/clk/mediatek/clk-mt8196-mfg.c
> @@ -13,6 +13,7 @@
>   #include <linux/of_address.h>
>   #include <linux/of_device.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>   
>   #include "clk-mtk.h"
>   #include "clk-pll.h"
> @@ -38,7 +39,7 @@
>   	    _flags, _rst_bar_mask,				\
>   	    _pd_reg, _pd_shift, _tuner_reg,			\
>   	    _tuner_en_reg, _tuner_en_bit,			\
> -	    _pcw_reg, _pcw_shift, _pcwbits) {			\
> +	    _pcw_reg, _pcw_shift, _pcwbits, _rpm_clks) {	\
>   		.id = _id,					\
>   		.name = _name,					\
>   		.reg = _reg,					\
> @@ -58,26 +59,60 @@
>   		.pcw_shift = _pcw_shift,			\
>   		.pcwbits = _pcwbits,				\
>   		.pcwibits = MT8196_INTEGER_BITS,		\
> +		.rpm_clk_names = _rpm_clks,			\
> +		.num_rpm_clks = ARRAY_SIZE(_rpm_clks),		\
>   	}
>   
> +static const char * const mfgpll_rpm_clk_names[] = {
> +	NULL
> +};
> +
>   static const struct mtk_pll_data mfg_ao_plls[] = {
>   	PLL(CLK_MFG_AO_MFGPLL, "mfgpll", MFGPLL_CON0, MFGPLL_CON0, 0, 0, 0,
> -	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0,
> -	    MFGPLL_CON1, 0, 22),
> +	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0, MFGPLL_CON1, 0, 22,
> +	    mfgpll_rpm_clk_names),
>   };
>   
>   static const struct mtk_pll_data mfgsc0_ao_plls[] = {
>   	PLL(CLK_MFGSC0_AO_MFGPLL_SC0, "mfgpll-sc0", MFGPLL_SC0_CON0,
>   	    MFGPLL_SC0_CON0, 0, 0, 0, BIT(0), MFGPLL_SC0_CON1, 24, 0, 0, 0,
> -	    MFGPLL_SC0_CON1, 0, 22),
> +	    MFGPLL_SC0_CON1, 0, 22, mfgpll_rpm_clk_names),
>   };
>   
>   static const struct mtk_pll_data mfgsc1_ao_plls[] = {
>   	PLL(CLK_MFGSC1_AO_MFGPLL_SC1, "mfgpll-sc1", MFGPLL_SC1_CON0,
>   	    MFGPLL_SC1_CON0, 0, 0, 0, BIT(0), MFGPLL_SC1_CON1, 24, 0, 0, 0,
> -	    MFGPLL_SC1_CON1, 0, 22),
> +	    MFGPLL_SC1_CON1, 0, 22, mfgpll_rpm_clk_names),
>   };
>   
> +struct clk_mt8196_mfg {
> +	struct clk_hw_onecell_data *clk_data;
> +	struct clk_bulk_data *rpm_clks;
> +	unsigned int num_rpm_clks;
> +};
> +
> +static int __maybe_unused clk_mt8196_mfg_resume(struct device *dev)
> +{
> +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
> +
> +	if (!clk_mfg || !clk_mfg->rpm_clks)
> +		return 0;
> +
> +	return clk_bulk_prepare_enable(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
> +}
> +
> +static int __maybe_unused clk_mt8196_mfg_suspend(struct device *dev)
> +{
> +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
> +
> +	if (!clk_mfg || !clk_mfg->rpm_clks)
> +		return 0;
> +
> +	clk_bulk_disable_unprepare(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
> +
> +	return 0;
> +}
> +
>   static const struct of_device_id of_match_clk_mt8196_mfg[] = {
>   	{ .compatible = "mediatek,mt8196-mfgpll-pll-ctrl",
>   	  .data = &mfg_ao_plls },
> @@ -92,35 +127,60 @@ MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mfg);
>   static int clk_mt8196_mfg_probe(struct platform_device *pdev)
>   {
>   	const struct mtk_pll_data *plls;
> -	struct clk_hw_onecell_data *clk_data;
> +	struct clk_mt8196_mfg *clk_mfg;
>   	struct device_node *node = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
>   	const int num_plls = 1;
> -	int r;
> +	int r, i;
>   
> -	plls = of_device_get_match_data(&pdev->dev);
> +	plls = of_device_get_match_data(dev);
>   	if (!plls)
>   		return -EINVAL;
>   
> -	clk_data = mtk_alloc_clk_data(num_plls);
> -	if (!clk_data)
> +	clk_mfg = devm_kzalloc(dev, sizeof(*clk_mfg), GFP_KERNEL);
> +	if (!clk_mfg)
>   		return -ENOMEM;
>   
> -	r = mtk_clk_register_plls(&pdev->dev, plls, num_plls, clk_data);
> +	clk_mfg->num_rpm_clks = plls[0].num_rpm_clks;
> +
> +	if (clk_mfg->num_rpm_clks) {
> +		clk_mfg->rpm_clks = devm_kcalloc(dev, clk_mfg->num_rpm_clks,
> +						 sizeof(*clk_mfg->rpm_clks),
> +						 GFP_KERNEL);
> +		if (!clk_mfg->rpm_clks)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < clk_mfg->num_rpm_clks; i++)
> +			clk_mfg->rpm_clks->id = plls[0].rpm_clk_names[i];
> +
> +		r = devm_clk_bulk_get(dev, clk_mfg->num_rpm_clks,
> +				      clk_mfg->rpm_clks);
> +		if (r)
> +			return r;
> +	}
> +
> +	clk_mfg->clk_data = mtk_alloc_clk_data(num_plls);
> +	if (!clk_mfg->clk_data)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, clk_mfg);
> +	pm_runtime_enable(dev);
> +
> +	r = mtk_clk_register_plls(dev, plls, num_plls, clk_mfg->clk_data);
>   	if (r)
>   		goto free_clk_data;
>   
> -	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
> +	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> +				   clk_mfg->clk_data);
>   	if (r)
>   		goto unregister_plls;
>   
> -	platform_set_drvdata(pdev, clk_data);
> -
>   	return r;
>   
>   unregister_plls:
> -	mtk_clk_unregister_plls(plls, num_plls, clk_data);
> +	mtk_clk_unregister_plls(plls, num_plls, clk_mfg->clk_data);
>   free_clk_data:
> -	mtk_free_clk_data(clk_data);
> +	mtk_free_clk_data(clk_mfg->clk_data);
>   
>   	return r;
>   }
> @@ -128,20 +188,26 @@ static int clk_mt8196_mfg_probe(struct platform_device *pdev)
>   static void clk_mt8196_mfg_remove(struct platform_device *pdev)
>   {
>   	const struct mtk_pll_data *plls = of_device_get_match_data(&pdev->dev);
> -	struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
> +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(&pdev->dev);
>   	struct device_node *node = pdev->dev.of_node;
>   
>   	of_clk_del_provider(node);
> -	mtk_clk_unregister_plls(plls, 1, clk_data);
> -	mtk_free_clk_data(clk_data);
> +	mtk_clk_unregister_plls(plls, 1, clk_mfg->clk_data);
> +	mtk_free_clk_data(clk_mfg->clk_data);
>   }
>   
> +static DEFINE_RUNTIME_DEV_PM_OPS(clk_mt8196_mfg_pm_ops,
> +				 clk_mt8196_mfg_suspend,
> +				 clk_mt8196_mfg_resume,
> +				 NULL);
> +
>   static struct platform_driver clk_mt8196_mfg_drv = {
>   	.probe = clk_mt8196_mfg_probe,
>   	.remove = clk_mt8196_mfg_remove,
>   	.driver = {
>   		.name = "clk-mt8196-mfg",
>   		.of_match_table = of_match_clk_mt8196_mfg,
> +		.pm = pm_ptr(&clk_mt8196_mfg_pm_ops),
>   	},
>   };
>   module_platform_driver(clk_mt8196_mfg_drv);
> diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
> index 0f2a1d19eea78b7390b221af47016eb9897f3596..82b86b849a67359d8f23d828f50422081c2747e3 100644
> --- a/drivers/clk/mediatek/clk-pll.h
> +++ b/drivers/clk/mediatek/clk-pll.h
> @@ -53,6 +53,8 @@ struct mtk_pll_data {
>   	u8 pll_en_bit; /* Assume 0, indicates BIT(0) by default */
>   	u8 pcw_chg_bit;
>   	u8 fenc_sta_bit;
> +	const char * const *rpm_clk_names;
> +	unsigned int num_rpm_clks;
>   };
>   
>   /*
> 

Re: [PATCH 4/4] clk: mediatek: Add rpm clocks to clk-mt8196-mfg

[Nicolas Frattaroli]

On Wednesday, 1 October 2025 13:49:54 Central European Summer Time AngeloGioacchino Del Regno wrote:
> Il 29/09/25 14:13, Nicolas Frattaroli ha scritto:
> > The mfgpll clocks on mt8196 require that the MFG's EB clock is on if
> > their control registers are touched in any way at all.
> 
> ....so, the MFGPLL clocks are children of EB?
> 
> Why are you using such a convoluted way of adding a parent clock to the MFGPLL
> instead of just
> ---->   `.parent_name = "mfg_eb"`  <-----
> 
> ???????

They're not children. A child would mean that they derive their
clock rate from the parent clock. This isn't the relationship here
though, their clock signal is completely independent of mfg_eb,
but the registers they access for clock control depend on mfg_eb to
be on. This means that even if mfgpll is off, and something wants to
check if they're on or not, the mfg_eb needs to be on for that
register access to happen. Similarly, when reconfiguring the mfgpll
rate, the clock rate of mfg_eb plays no role. It just needs to be on
for the register access.

mfg_eb here is a clock that drives a register, the register just
happens to be part of a clock controller.

Kind regards,
Nicolas Frattaroli

> 
> Cheers,
> Angelo
> 
> > Failing to ensure
> > this results in a pleasant SError interrupt if the EB clock happens to
> > be off.
> > 
> > To achieve this, leverage the CCF core's runtime power management
> > support. Define the necessary suspend/resume callbacks, add the
> > necessary code to get RPM clocks from the DT, and make sure RPM is
> > enabled before clock registration happens.
> > 
> > For the RPM callbacks to really make much sense at all, we change the
> > drvdata from clk_data to a new private struct, as is common in drivers
> > across the Linux kernel.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >   drivers/clk/mediatek/clk-mt8196-mfg.c | 104 +++++++++++++++++++++++++++-------
> >   drivers/clk/mediatek/clk-pll.h        |   2 +
> >   2 files changed, 87 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/clk/mediatek/clk-mt8196-mfg.c b/drivers/clk/mediatek/clk-mt8196-mfg.c
> > index 8e09c0f7b7548f8e286671cea2dac64530b8ce47..64cc0c037f62d7eab8d0e7fc00c05d122bf4130c 100644
> > --- a/drivers/clk/mediatek/clk-mt8196-mfg.c
> > +++ b/drivers/clk/mediatek/clk-mt8196-mfg.c
> > @@ -13,6 +13,7 @@
> >   #include <linux/of_address.h>
> >   #include <linux/of_device.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >   
> >   #include "clk-mtk.h"
> >   #include "clk-pll.h"
> > @@ -38,7 +39,7 @@
> >   	    _flags, _rst_bar_mask,				\
> >   	    _pd_reg, _pd_shift, _tuner_reg,			\
> >   	    _tuner_en_reg, _tuner_en_bit,			\
> > -	    _pcw_reg, _pcw_shift, _pcwbits) {			\
> > +	    _pcw_reg, _pcw_shift, _pcwbits, _rpm_clks) {	\
> >   		.id = _id,					\
> >   		.name = _name,					\
> >   		.reg = _reg,					\
> > @@ -58,26 +59,60 @@
> >   		.pcw_shift = _pcw_shift,			\
> >   		.pcwbits = _pcwbits,				\
> >   		.pcwibits = MT8196_INTEGER_BITS,		\
> > +		.rpm_clk_names = _rpm_clks,			\
> > +		.num_rpm_clks = ARRAY_SIZE(_rpm_clks),		\
> >   	}
> >   
> > +static const char * const mfgpll_rpm_clk_names[] = {
> > +	NULL
> > +};
> > +
> >   static const struct mtk_pll_data mfg_ao_plls[] = {
> >   	PLL(CLK_MFG_AO_MFGPLL, "mfgpll", MFGPLL_CON0, MFGPLL_CON0, 0, 0, 0,
> > -	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0,
> > -	    MFGPLL_CON1, 0, 22),
> > +	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0, MFGPLL_CON1, 0, 22,
> > +	    mfgpll_rpm_clk_names),
> >   };
> >   
> >   static const struct mtk_pll_data mfgsc0_ao_plls[] = {
> >   	PLL(CLK_MFGSC0_AO_MFGPLL_SC0, "mfgpll-sc0", MFGPLL_SC0_CON0,
> >   	    MFGPLL_SC0_CON0, 0, 0, 0, BIT(0), MFGPLL_SC0_CON1, 24, 0, 0, 0,
> > -	    MFGPLL_SC0_CON1, 0, 22),
> > +	    MFGPLL_SC0_CON1, 0, 22, mfgpll_rpm_clk_names),
> >   };
> >   
> >   static const struct mtk_pll_data mfgsc1_ao_plls[] = {
> >   	PLL(CLK_MFGSC1_AO_MFGPLL_SC1, "mfgpll-sc1", MFGPLL_SC1_CON0,
> >   	    MFGPLL_SC1_CON0, 0, 0, 0, BIT(0), MFGPLL_SC1_CON1, 24, 0, 0, 0,
> > -	    MFGPLL_SC1_CON1, 0, 22),
> > +	    MFGPLL_SC1_CON1, 0, 22, mfgpll_rpm_clk_names),
> >   };
> >   
> > +struct clk_mt8196_mfg {
> > +	struct clk_hw_onecell_data *clk_data;
> > +	struct clk_bulk_data *rpm_clks;
> > +	unsigned int num_rpm_clks;
> > +};
> > +
> > +static int __maybe_unused clk_mt8196_mfg_resume(struct device *dev)
> > +{
> > +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
> > +
> > +	if (!clk_mfg || !clk_mfg->rpm_clks)
> > +		return 0;
> > +
> > +	return clk_bulk_prepare_enable(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
> > +}
> > +
> > +static int __maybe_unused clk_mt8196_mfg_suspend(struct device *dev)
> > +{
> > +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
> > +
> > +	if (!clk_mfg || !clk_mfg->rpm_clks)
> > +		return 0;
> > +
> > +	clk_bulk_disable_unprepare(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
> > +
> > +	return 0;
> > +}
> > +
> >   static const struct of_device_id of_match_clk_mt8196_mfg[] = {
> >   	{ .compatible = "mediatek,mt8196-mfgpll-pll-ctrl",
> >   	  .data = &mfg_ao_plls },
> > @@ -92,35 +127,60 @@ MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mfg);
> >   static int clk_mt8196_mfg_probe(struct platform_device *pdev)
> >   {
> >   	const struct mtk_pll_data *plls;
> > -	struct clk_hw_onecell_data *clk_data;
> > +	struct clk_mt8196_mfg *clk_mfg;
> >   	struct device_node *node = pdev->dev.of_node;
> > +	struct device *dev = &pdev->dev;
> >   	const int num_plls = 1;
> > -	int r;
> > +	int r, i;
> >   
> > -	plls = of_device_get_match_data(&pdev->dev);
> > +	plls = of_device_get_match_data(dev);
> >   	if (!plls)
> >   		return -EINVAL;
> >   
> > -	clk_data = mtk_alloc_clk_data(num_plls);
> > -	if (!clk_data)
> > +	clk_mfg = devm_kzalloc(dev, sizeof(*clk_mfg), GFP_KERNEL);
> > +	if (!clk_mfg)
> >   		return -ENOMEM;
> >   
> > -	r = mtk_clk_register_plls(&pdev->dev, plls, num_plls, clk_data);
> > +	clk_mfg->num_rpm_clks = plls[0].num_rpm_clks;
> > +
> > +	if (clk_mfg->num_rpm_clks) {
> > +		clk_mfg->rpm_clks = devm_kcalloc(dev, clk_mfg->num_rpm_clks,
> > +						 sizeof(*clk_mfg->rpm_clks),
> > +						 GFP_KERNEL);
> > +		if (!clk_mfg->rpm_clks)
> > +			return -ENOMEM;
> > +
> > +		for (i = 0; i < clk_mfg->num_rpm_clks; i++)
> > +			clk_mfg->rpm_clks->id = plls[0].rpm_clk_names[i];
> > +
> > +		r = devm_clk_bulk_get(dev, clk_mfg->num_rpm_clks,
> > +				      clk_mfg->rpm_clks);
> > +		if (r)
> > +			return r;
> > +	}
> > +
> > +	clk_mfg->clk_data = mtk_alloc_clk_data(num_plls);
> > +	if (!clk_mfg->clk_data)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, clk_mfg);
> > +	pm_runtime_enable(dev);
> > +
> > +	r = mtk_clk_register_plls(dev, plls, num_plls, clk_mfg->clk_data);
> >   	if (r)
> >   		goto free_clk_data;
> >   
> > -	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
> > +	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> > +				   clk_mfg->clk_data);
> >   	if (r)
> >   		goto unregister_plls;
> >   
> > -	platform_set_drvdata(pdev, clk_data);
> > -
> >   	return r;
> >   
> >   unregister_plls:
> > -	mtk_clk_unregister_plls(plls, num_plls, clk_data);
> > +	mtk_clk_unregister_plls(plls, num_plls, clk_mfg->clk_data);
> >   free_clk_data:
> > -	mtk_free_clk_data(clk_data);
> > +	mtk_free_clk_data(clk_mfg->clk_data);
> >   
> >   	return r;
> >   }
> > @@ -128,20 +188,26 @@ static int clk_mt8196_mfg_probe(struct platform_device *pdev)
> >   static void clk_mt8196_mfg_remove(struct platform_device *pdev)
> >   {
> >   	const struct mtk_pll_data *plls = of_device_get_match_data(&pdev->dev);
> > -	struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
> > +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(&pdev->dev);
> >   	struct device_node *node = pdev->dev.of_node;
> >   
> >   	of_clk_del_provider(node);
> > -	mtk_clk_unregister_plls(plls, 1, clk_data);
> > -	mtk_free_clk_data(clk_data);
> > +	mtk_clk_unregister_plls(plls, 1, clk_mfg->clk_data);
> > +	mtk_free_clk_data(clk_mfg->clk_data);
> >   }
> >   
> > +static DEFINE_RUNTIME_DEV_PM_OPS(clk_mt8196_mfg_pm_ops,
> > +				 clk_mt8196_mfg_suspend,
> > +				 clk_mt8196_mfg_resume,
> > +				 NULL);
> > +
> >   static struct platform_driver clk_mt8196_mfg_drv = {
> >   	.probe = clk_mt8196_mfg_probe,
> >   	.remove = clk_mt8196_mfg_remove,
> >   	.driver = {
> >   		.name = "clk-mt8196-mfg",
> >   		.of_match_table = of_match_clk_mt8196_mfg,
> > +		.pm = pm_ptr(&clk_mt8196_mfg_pm_ops),
> >   	},
> >   };
> >   module_platform_driver(clk_mt8196_mfg_drv);
> > diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
> > index 0f2a1d19eea78b7390b221af47016eb9897f3596..82b86b849a67359d8f23d828f50422081c2747e3 100644
> > --- a/drivers/clk/mediatek/clk-pll.h
> > +++ b/drivers/clk/mediatek/clk-pll.h
> > @@ -53,6 +53,8 @@ struct mtk_pll_data {
> >   	u8 pll_en_bit; /* Assume 0, indicates BIT(0) by default */
> >   	u8 pcw_chg_bit;
> >   	u8 fenc_sta_bit;
> > +	const char * const *rpm_clk_names;
> > +	unsigned int num_rpm_clks;
> >   };
> >   
> >   /*
> > 
> 
> 
> 

Re: [PATCH 4/4] clk: mediatek: Add rpm clocks to clk-mt8196-mfg

[Nicolas Frattaroli]

On Wednesday, 1 October 2025 15:17:13 Central European Summer Time Nicolas Frattaroli wrote:
> On Wednesday, 1 October 2025 13:49:54 Central European Summer Time AngeloGioacchino Del Regno wrote:
> > Il 29/09/25 14:13, Nicolas Frattaroli ha scritto:
> > > The mfgpll clocks on mt8196 require that the MFG's EB clock is on if
> > > their control registers are touched in any way at all.
> > 
> > ....so, the MFGPLL clocks are children of EB?
> > 
> > Why are you using such a convoluted way of adding a parent clock to the MFGPLL
> > instead of just
> > ---->   `.parent_name = "mfg_eb"`  <-----
> > 
> > ???????
> 
> They're not children. A child would mean that they derive their
> clock rate from the parent clock. This isn't the relationship here
> though, their clock signal is completely independent of mfg_eb,
> but the registers they access for clock control depend on mfg_eb to
> be on. This means that even if mfgpll is off, and something wants to
> check if they're on or not, the mfg_eb needs to be on for that
> register access to happen. Similarly, when reconfiguring the mfgpll
> rate, the clock rate of mfg_eb plays no role. It just needs to be on
> for the register access.
> 
> mfg_eb here is a clock that drives a register, the register just
> happens to be part of a clock controller.

As a follow-up to this, I've done some experiments now, as Angelo
let me know that the only way we can know for sure is by observing
mfgpll working while mfg_eb is off.

With horrid hacks, I've managed to manufacture a scenario in which
EB was definitely off, the mfgplls were still on, the GPUEB didn't
try to ruin the party by fiddling with the mfgpll clock registers,
and what I got in return was panthor printing errors about its jobs
timing out, before an SError splat after several timeouts.

This makes me think that mfg_eb really is a parent of mfgpll, as
evidently mfgpll didn't seem to be ticking anymore and caused the
timeouts to happen before something touched unclocked registers
and caused an SError. Put in glmark2 terms, the horse was no longer
spinning around its axis, causing much grief.

The other possibility is, and I think this is quite likely: we are
modeling the mfgpll power up/down wrong anyway, and always rely on
the GPUEB to actually do this properly. I've diffed the mfgpll clock
registers with the GPUEB running and with it not running, here are
the results in addr u32 u32 u32 u32 format:

$ diff -u mfgplldump-not-running.txt mfgplldump-running.txt 
--- mfgplldump-not-running.txt  2025-10-06 20:03:11.902125545 +0200
+++ mfgplldump-running.txt      2025-10-06 20:03:08.457020814 +0200
@@ -1,5 +1,5 @@
-4b810000  00000000 0001fc23  00084444 831a0000
-4b810010  001a0000 00000002  00000048 00000200
+4b810000  00000000 0001fc23  00084445 031a0000
+4b810010  001a0000 80000002  00000048 00000200
 4b810020  00000000 00000000  00000000 00000000
 4b810030  00000000 00000000  00000000 00000000
 4b810040  00018000 01ff1a00  00000000 00000000
@@ -62,8 +62,8 @@
 4b8103d0  00000000 00000000  00000000 00000000
 4b8103e0  00000000 00000000  00000000 00000000
 4b8103f0  00000000 00000000  00000000 00000000
-4b810400  00000000 0001fc23  00084444 831a0000
-4b810410  001a0000 00000002  00000048 00000200
+4b810400  00000000 0001fc23  00084445 031a0000
+4b810410  001a0000 80000002  00000048 00000200
 4b810420  00000000 00000000  00000000 00000000
 4b810430  00000000 00000000  00000000 00000000
 4b810440  00018000 01ff1a00  00000000 00000000
@@ -126,8 +126,8 @@
 4b8107d0  00000000 00000000  00000000 00000000
 4b8107e0  00000000 00000000  00000000 00000000
 4b8107f0  00000000 00000000  00000000 00000000
-4b810800  00000000 0001fc23  00084444 831a0000
-4b810810  001a0000 00000002  00000048 00000200
+4b810800  00000000 0001fc23  00084445 031a0000
+4b810810  001a0000 80000002  00000048 00000200
 4b810820  00000000 00000000  00000000 00000000
 4b810830  00000000 00000000  00000000 00000000
 4b810840  00018000 01ff1a00  00000000 00000000

Note how bit 31 of MFGPLL*CON3 gets flipped to 1 in the registers
when the GPUEB is running. Nothing in our code would model it to be
like this.

I'll rework the series to just model MFG_EB as a parent. I'll drop the
bindings patch, but will keep the refactor since it's still useful.
Then I'll drop the mfgpll RPM patch and make it a fixes patch to add
the EB parent instead.

However, I'm not happy about this, because I am quite certain that
the code here is not all that correct, but we do not have access to
the documentation to show us how it would be correct, and downstream
kernels are not of any help either.

I also can't just adjust the mfgpll stuff to fit the bit writes I
actually see happening from the EB, because powering off the EB so
it doesn't try to touch them and then setting those registers right
again will still leave a gap wherein the register is not correct
and who knows what happens. Ditto for the regulators. Similarly,
I can't just try disabling MFG_EB while the EB is running, because
it will touch the control registers of the clocks and depends on
MFG_EB for sure, and I can't observe whether MFGPLL is ticking in
any other way other than looking at whether the GPU is working. Or
maybe I can and there's some monitor register somewhere, but not
anywhere I have documentation for.

I've checked the remaining memory area of the mfgpll clock controllers,
and found no suspiciously changing values in there that could indicate
a monitor register that contains the actual pll frequency.

Kind regards,
Nicolas Frattaroli

> 
> Kind regards,
> Nicolas Frattaroli
> 
> > 
> > Cheers,
> > Angelo
> > 
> > > Failing to ensure
> > > this results in a pleasant SError interrupt if the EB clock happens to
> > > be off.
> > > 
> > > To achieve this, leverage the CCF core's runtime power management
> > > support. Define the necessary suspend/resume callbacks, add the
> > > necessary code to get RPM clocks from the DT, and make sure RPM is
> > > enabled before clock registration happens.
> > > 
> > > For the RPM callbacks to really make much sense at all, we change the
> > > drvdata from clk_data to a new private struct, as is common in drivers
> > > across the Linux kernel.
> > > 
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > >   drivers/clk/mediatek/clk-mt8196-mfg.c | 104 +++++++++++++++++++++++++++-------
> > >   drivers/clk/mediatek/clk-pll.h        |   2 +
> > >   2 files changed, 87 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/clk/mediatek/clk-mt8196-mfg.c b/drivers/clk/mediatek/clk-mt8196-mfg.c
> > > index 8e09c0f7b7548f8e286671cea2dac64530b8ce47..64cc0c037f62d7eab8d0e7fc00c05d122bf4130c 100644
> > > --- a/drivers/clk/mediatek/clk-mt8196-mfg.c
> > > +++ b/drivers/clk/mediatek/clk-mt8196-mfg.c
> > > @@ -13,6 +13,7 @@
> > >   #include <linux/of_address.h>
> > >   #include <linux/of_device.h>
> > >   #include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > >   
> > >   #include "clk-mtk.h"
> > >   #include "clk-pll.h"
> > > @@ -38,7 +39,7 @@
> > >   	    _flags, _rst_bar_mask,				\
> > >   	    _pd_reg, _pd_shift, _tuner_reg,			\
> > >   	    _tuner_en_reg, _tuner_en_bit,			\
> > > -	    _pcw_reg, _pcw_shift, _pcwbits) {			\
> > > +	    _pcw_reg, _pcw_shift, _pcwbits, _rpm_clks) {	\
> > >   		.id = _id,					\
> > >   		.name = _name,					\
> > >   		.reg = _reg,					\
> > > @@ -58,26 +59,60 @@
> > >   		.pcw_shift = _pcw_shift,			\
> > >   		.pcwbits = _pcwbits,				\
> > >   		.pcwibits = MT8196_INTEGER_BITS,		\
> > > +		.rpm_clk_names = _rpm_clks,			\
> > > +		.num_rpm_clks = ARRAY_SIZE(_rpm_clks),		\
> > >   	}
> > >   
> > > +static const char * const mfgpll_rpm_clk_names[] = {
> > > +	NULL
> > > +};
> > > +
> > >   static const struct mtk_pll_data mfg_ao_plls[] = {
> > >   	PLL(CLK_MFG_AO_MFGPLL, "mfgpll", MFGPLL_CON0, MFGPLL_CON0, 0, 0, 0,
> > > -	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0,
> > > -	    MFGPLL_CON1, 0, 22),
> > > +	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0, MFGPLL_CON1, 0, 22,
> > > +	    mfgpll_rpm_clk_names),
> > >   };
> > >   
> > >   static const struct mtk_pll_data mfgsc0_ao_plls[] = {
> > >   	PLL(CLK_MFGSC0_AO_MFGPLL_SC0, "mfgpll-sc0", MFGPLL_SC0_CON0,
> > >   	    MFGPLL_SC0_CON0, 0, 0, 0, BIT(0), MFGPLL_SC0_CON1, 24, 0, 0, 0,
> > > -	    MFGPLL_SC0_CON1, 0, 22),
> > > +	    MFGPLL_SC0_CON1, 0, 22, mfgpll_rpm_clk_names),
> > >   };
> > >   
> > >   static const struct mtk_pll_data mfgsc1_ao_plls[] = {
> > >   	PLL(CLK_MFGSC1_AO_MFGPLL_SC1, "mfgpll-sc1", MFGPLL_SC1_CON0,
> > >   	    MFGPLL_SC1_CON0, 0, 0, 0, BIT(0), MFGPLL_SC1_CON1, 24, 0, 0, 0,
> > > -	    MFGPLL_SC1_CON1, 0, 22),
> > > +	    MFGPLL_SC1_CON1, 0, 22, mfgpll_rpm_clk_names),
> > >   };
> > >   
> > > +struct clk_mt8196_mfg {
> > > +	struct clk_hw_onecell_data *clk_data;
> > > +	struct clk_bulk_data *rpm_clks;
> > > +	unsigned int num_rpm_clks;
> > > +};
> > > +
> > > +static int __maybe_unused clk_mt8196_mfg_resume(struct device *dev)
> > > +{
> > > +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
> > > +
> > > +	if (!clk_mfg || !clk_mfg->rpm_clks)
> > > +		return 0;
> > > +
> > > +	return clk_bulk_prepare_enable(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
> > > +}
> > > +
> > > +static int __maybe_unused clk_mt8196_mfg_suspend(struct device *dev)
> > > +{
> > > +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
> > > +
> > > +	if (!clk_mfg || !clk_mfg->rpm_clks)
> > > +		return 0;
> > > +
> > > +	clk_bulk_disable_unprepare(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static const struct of_device_id of_match_clk_mt8196_mfg[] = {
> > >   	{ .compatible = "mediatek,mt8196-mfgpll-pll-ctrl",
> > >   	  .data = &mfg_ao_plls },
> > > @@ -92,35 +127,60 @@ MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mfg);
> > >   static int clk_mt8196_mfg_probe(struct platform_device *pdev)
> > >   {
> > >   	const struct mtk_pll_data *plls;
> > > -	struct clk_hw_onecell_data *clk_data;
> > > +	struct clk_mt8196_mfg *clk_mfg;
> > >   	struct device_node *node = pdev->dev.of_node;
> > > +	struct device *dev = &pdev->dev;
> > >   	const int num_plls = 1;
> > > -	int r;
> > > +	int r, i;
> > >   
> > > -	plls = of_device_get_match_data(&pdev->dev);
> > > +	plls = of_device_get_match_data(dev);
> > >   	if (!plls)
> > >   		return -EINVAL;
> > >   
> > > -	clk_data = mtk_alloc_clk_data(num_plls);
> > > -	if (!clk_data)
> > > +	clk_mfg = devm_kzalloc(dev, sizeof(*clk_mfg), GFP_KERNEL);
> > > +	if (!clk_mfg)
> > >   		return -ENOMEM;
> > >   
> > > -	r = mtk_clk_register_plls(&pdev->dev, plls, num_plls, clk_data);
> > > +	clk_mfg->num_rpm_clks = plls[0].num_rpm_clks;
> > > +
> > > +	if (clk_mfg->num_rpm_clks) {
> > > +		clk_mfg->rpm_clks = devm_kcalloc(dev, clk_mfg->num_rpm_clks,
> > > +						 sizeof(*clk_mfg->rpm_clks),
> > > +						 GFP_KERNEL);
> > > +		if (!clk_mfg->rpm_clks)
> > > +			return -ENOMEM;
> > > +
> > > +		for (i = 0; i < clk_mfg->num_rpm_clks; i++)
> > > +			clk_mfg->rpm_clks->id = plls[0].rpm_clk_names[i];
> > > +
> > > +		r = devm_clk_bulk_get(dev, clk_mfg->num_rpm_clks,
> > > +				      clk_mfg->rpm_clks);
> > > +		if (r)
> > > +			return r;
> > > +	}
> > > +
> > > +	clk_mfg->clk_data = mtk_alloc_clk_data(num_plls);
> > > +	if (!clk_mfg->clk_data)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_set_drvdata(dev, clk_mfg);
> > > +	pm_runtime_enable(dev);
> > > +
> > > +	r = mtk_clk_register_plls(dev, plls, num_plls, clk_mfg->clk_data);
> > >   	if (r)
> > >   		goto free_clk_data;
> > >   
> > > -	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
> > > +	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> > > +				   clk_mfg->clk_data);
> > >   	if (r)
> > >   		goto unregister_plls;
> > >   
> > > -	platform_set_drvdata(pdev, clk_data);
> > > -
> > >   	return r;
> > >   
> > >   unregister_plls:
> > > -	mtk_clk_unregister_plls(plls, num_plls, clk_data);
> > > +	mtk_clk_unregister_plls(plls, num_plls, clk_mfg->clk_data);
> > >   free_clk_data:
> > > -	mtk_free_clk_data(clk_data);
> > > +	mtk_free_clk_data(clk_mfg->clk_data);
> > >   
> > >   	return r;
> > >   }
> > > @@ -128,20 +188,26 @@ static int clk_mt8196_mfg_probe(struct platform_device *pdev)
> > >   static void clk_mt8196_mfg_remove(struct platform_device *pdev)
> > >   {
> > >   	const struct mtk_pll_data *plls = of_device_get_match_data(&pdev->dev);
> > > -	struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
> > > +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(&pdev->dev);
> > >   	struct device_node *node = pdev->dev.of_node;
> > >   
> > >   	of_clk_del_provider(node);
> > > -	mtk_clk_unregister_plls(plls, 1, clk_data);
> > > -	mtk_free_clk_data(clk_data);
> > > +	mtk_clk_unregister_plls(plls, 1, clk_mfg->clk_data);
> > > +	mtk_free_clk_data(clk_mfg->clk_data);
> > >   }
> > >   
> > > +static DEFINE_RUNTIME_DEV_PM_OPS(clk_mt8196_mfg_pm_ops,
> > > +				 clk_mt8196_mfg_suspend,
> > > +				 clk_mt8196_mfg_resume,
> > > +				 NULL);
> > > +
> > >   static struct platform_driver clk_mt8196_mfg_drv = {
> > >   	.probe = clk_mt8196_mfg_probe,
> > >   	.remove = clk_mt8196_mfg_remove,
> > >   	.driver = {
> > >   		.name = "clk-mt8196-mfg",
> > >   		.of_match_table = of_match_clk_mt8196_mfg,
> > > +		.pm = pm_ptr(&clk_mt8196_mfg_pm_ops),
> > >   	},
> > >   };
> > >   module_platform_driver(clk_mt8196_mfg_drv);
> > > diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
> > > index 0f2a1d19eea78b7390b221af47016eb9897f3596..82b86b849a67359d8f23d828f50422081c2747e3 100644
> > > --- a/drivers/clk/mediatek/clk-pll.h
> > > +++ b/drivers/clk/mediatek/clk-pll.h
> > > @@ -53,6 +53,8 @@ struct mtk_pll_data {
> > >   	u8 pll_en_bit; /* Assume 0, indicates BIT(0) by default */
> > >   	u8 pcw_chg_bit;
> > >   	u8 fenc_sta_bit;
> > > +	const char * const *rpm_clk_names;
> > > +	unsigned int num_rpm_clks;
> > >   };
> > >   
> > >   /*
> > > 
> > 
> > 
> > 
> 
> 

Re: [PATCH 4/4] clk: mediatek: Add rpm clocks to clk-mt8196-mfg

[AngeloGioacchino Del Regno]

Il 06/10/25 21:01, Nicolas Frattaroli ha scritto:
> On Wednesday, 1 October 2025 15:17:13 Central European Summer Time Nicolas Frattaroli wrote:
>> On Wednesday, 1 October 2025 13:49:54 Central European Summer Time AngeloGioacchino Del Regno wrote:
>>> Il 29/09/25 14:13, Nicolas Frattaroli ha scritto:
>>>> The mfgpll clocks on mt8196 require that the MFG's EB clock is on if
>>>> their control registers are touched in any way at all.
>>>
>>> ....so, the MFGPLL clocks are children of EB?
>>>
>>> Why are you using such a convoluted way of adding a parent clock to the MFGPLL
>>> instead of just
>>> ---->   `.parent_name = "mfg_eb"`  <-----
>>>
>>> ???????
>>
>> They're not children. A child would mean that they derive their
>> clock rate from the parent clock. This isn't the relationship here
>> though, their clock signal is completely independent of mfg_eb,
>> but the registers they access for clock control depend on mfg_eb to
>> be on. This means that even if mfgpll is off, and something wants to
>> check if they're on or not, the mfg_eb needs to be on for that
>> register access to happen. Similarly, when reconfiguring the mfgpll
>> rate, the clock rate of mfg_eb plays no role. It just needs to be on
>> for the register access.
>>
>> mfg_eb here is a clock that drives a register, the register just
>> happens to be part of a clock controller.
> 
> As a follow-up to this, I've done some experiments now, as Angelo
> let me know that the only way we can know for sure is by observing
> mfgpll working while mfg_eb is off.
> 
> With horrid hacks, I've managed to manufacture a scenario in which
> EB was definitely off, the mfgplls were still on, the GPUEB didn't
> try to ruin the party by fiddling with the mfgpll clock registers,
> and what I got in return was panthor printing errors about its jobs
> timing out, before an SError splat after several timeouts.
> 
> This makes me think that mfg_eb really is a parent of mfgpll, as
> evidently mfgpll didn't seem to be ticking anymore and caused the
> timeouts to happen before something touched unclocked registers
> and caused an SError. Put in glmark2 terms, the horse was no longer
> spinning around its axis, causing much grief.
> 
> The other possibility is, and I think this is quite likely: we are
> modeling the mfgpll power up/down wrong anyway, and always rely on
> the GPUEB to actually do this properly. I've diffed the mfgpll clock
> registers with the GPUEB running and with it not running, here are
> the results in addr u32 u32 u32 u32 format:
> 
> $ diff -u mfgplldump-not-running.txt mfgplldump-running.txt
> --- mfgplldump-not-running.txt  2025-10-06 20:03:11.902125545 +0200
> +++ mfgplldump-running.txt      2025-10-06 20:03:08.457020814 +0200
> @@ -1,5 +1,5 @@
> -4b810000  00000000 0001fc23  00084444 831a0000
> -4b810010  001a0000 00000002  00000048 00000200
> +4b810000  00000000 0001fc23  00084445 031a0000
> +4b810010  001a0000 80000002  00000048 00000200
>   4b810020  00000000 00000000  00000000 00000000
>   4b810030  00000000 00000000  00000000 00000000
>   4b810040  00018000 01ff1a00  00000000 00000000
> @@ -62,8 +62,8 @@
>   4b8103d0  00000000 00000000  00000000 00000000
>   4b8103e0  00000000 00000000  00000000 00000000
>   4b8103f0  00000000 00000000  00000000 00000000
> -4b810400  00000000 0001fc23  00084444 831a0000
> -4b810410  001a0000 00000002  00000048 00000200
> +4b810400  00000000 0001fc23  00084445 031a0000
> +4b810410  001a0000 80000002  00000048 00000200
>   4b810420  00000000 00000000  00000000 00000000
>   4b810430  00000000 00000000  00000000 00000000
>   4b810440  00018000 01ff1a00  00000000 00000000
> @@ -126,8 +126,8 @@
>   4b8107d0  00000000 00000000  00000000 00000000
>   4b8107e0  00000000 00000000  00000000 00000000
>   4b8107f0  00000000 00000000  00000000 00000000
> -4b810800  00000000 0001fc23  00084444 831a0000
> -4b810810  001a0000 00000002  00000048 00000200
> +4b810800  00000000 0001fc23  00084445 031a0000
> +4b810810  001a0000 80000002  00000048 00000200
>   4b810820  00000000 00000000  00000000 00000000
>   4b810830  00000000 00000000  00000000 00000000
>   4b810840  00018000 01ff1a00  00000000 00000000
> 
> Note how bit 31 of MFGPLL*CON3 gets flipped to 1 in the registers
> when the GPUEB is running. Nothing in our code would model it to be
> like this.
> 
> I'll rework the series to just model MFG_EB as a parent. I'll drop the
> bindings patch, but will keep the refactor since it's still useful.
> Then I'll drop the mfgpll RPM patch and make it a fixes patch to add
> the EB parent instead.
> 
> However, I'm not happy about this, because I am quite certain that
> the code here is not all that correct, but we do not have access to
> the documentation to show us how it would be correct, and downstream
> kernels are not of any help either.
> 
> I also can't just adjust the mfgpll stuff to fit the bit writes I
> actually see happening from the EB, because powering off the EB so
> it doesn't try to touch them and then setting those registers right
> again will still leave a gap wherein the register is not correct
> and who knows what happens. Ditto for the regulators. Similarly,
> I can't just try disabling MFG_EB while the EB is running, because
> it will touch the control registers of the clocks and depends on
> MFG_EB for sure, and I can't observe whether MFGPLL is ticking in
> any other way other than looking at whether the GPU is working. Or
> maybe I can and there's some monitor register somewhere, but not
> anywhere I have documentation for.
> 
> I've checked the remaining memory area of the mfgpll clock controllers,
> and found no suspiciously changing values in there that could indicate
> a monitor register that contains the actual pll frequency.
> 

It's possible that there is a specific mode for firmware-control for the PLL, which
makes it "useless" without the EB parent being on - but then since that is not
described in the datasheets we can't really reinvent the wheel here on hypotetical
variations.

This SoC has EB (as much as some other old ones like MT8188/92/95, but since there
we never enabled the GPUEB, we don't really-really-really know) and from your test
the EB is clearly a parent of the MFGPLL... so... like it or not, it is what it is.

Btw, regarding the possibilty of powering on the MFGPLL in some wrong way...

No.

The poweron sequence is correct - as per the datasheets (and matches with other MTK
PLL on/off strategies).

Thanks for doing this crazy test, and thanks for confirming my suspect!

Cheers,
Angelo


>>>
>>>> Failing to ensure
>>>> this results in a pleasant SError interrupt if the EB clock happens to
>>>> be off.
>>>>
>>>> To achieve this, leverage the CCF core's runtime power management
>>>> support. Define the necessary suspend/resume callbacks, add the
>>>> necessary code to get RPM clocks from the DT, and make sure RPM is
>>>> enabled before clock registration happens.
>>>>
>>>> For the RPM callbacks to really make much sense at all, we change the
>>>> drvdata from clk_data to a new private struct, as is common in drivers
>>>> across the Linux kernel.
>>>>
>>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>>>> ---
>>>>    drivers/clk/mediatek/clk-mt8196-mfg.c | 104 +++++++++++++++++++++++++++-------
>>>>    drivers/clk/mediatek/clk-pll.h        |   2 +
>>>>    2 files changed, 87 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/mediatek/clk-mt8196-mfg.c b/drivers/clk/mediatek/clk-mt8196-mfg.c
>>>> index 8e09c0f7b7548f8e286671cea2dac64530b8ce47..64cc0c037f62d7eab8d0e7fc00c05d122bf4130c 100644
>>>> --- a/drivers/clk/mediatek/clk-mt8196-mfg.c
>>>> +++ b/drivers/clk/mediatek/clk-mt8196-mfg.c
>>>> @@ -13,6 +13,7 @@
>>>>    #include <linux/of_address.h>
>>>>    #include <linux/of_device.h>
>>>>    #include <linux/platform_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>>    
>>>>    #include "clk-mtk.h"
>>>>    #include "clk-pll.h"
>>>> @@ -38,7 +39,7 @@
>>>>    	    _flags, _rst_bar_mask,				\
>>>>    	    _pd_reg, _pd_shift, _tuner_reg,			\
>>>>    	    _tuner_en_reg, _tuner_en_bit,			\
>>>> -	    _pcw_reg, _pcw_shift, _pcwbits) {			\
>>>> +	    _pcw_reg, _pcw_shift, _pcwbits, _rpm_clks) {	\
>>>>    		.id = _id,					\
>>>>    		.name = _name,					\
>>>>    		.reg = _reg,					\
>>>> @@ -58,26 +59,60 @@
>>>>    		.pcw_shift = _pcw_shift,			\
>>>>    		.pcwbits = _pcwbits,				\
>>>>    		.pcwibits = MT8196_INTEGER_BITS,		\
>>>> +		.rpm_clk_names = _rpm_clks,			\
>>>> +		.num_rpm_clks = ARRAY_SIZE(_rpm_clks),		\
>>>>    	}
>>>>    
>>>> +static const char * const mfgpll_rpm_clk_names[] = {
>>>> +	NULL
>>>> +};
>>>> +
>>>>    static const struct mtk_pll_data mfg_ao_plls[] = {
>>>>    	PLL(CLK_MFG_AO_MFGPLL, "mfgpll", MFGPLL_CON0, MFGPLL_CON0, 0, 0, 0,
>>>> -	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0,
>>>> -	    MFGPLL_CON1, 0, 22),
>>>> +	    BIT(0), MFGPLL_CON1, 24, 0, 0, 0, MFGPLL_CON1, 0, 22,
>>>> +	    mfgpll_rpm_clk_names),
>>>>    };
>>>>    
>>>>    static const struct mtk_pll_data mfgsc0_ao_plls[] = {
>>>>    	PLL(CLK_MFGSC0_AO_MFGPLL_SC0, "mfgpll-sc0", MFGPLL_SC0_CON0,
>>>>    	    MFGPLL_SC0_CON0, 0, 0, 0, BIT(0), MFGPLL_SC0_CON1, 24, 0, 0, 0,
>>>> -	    MFGPLL_SC0_CON1, 0, 22),
>>>> +	    MFGPLL_SC0_CON1, 0, 22, mfgpll_rpm_clk_names),
>>>>    };
>>>>    
>>>>    static const struct mtk_pll_data mfgsc1_ao_plls[] = {
>>>>    	PLL(CLK_MFGSC1_AO_MFGPLL_SC1, "mfgpll-sc1", MFGPLL_SC1_CON0,
>>>>    	    MFGPLL_SC1_CON0, 0, 0, 0, BIT(0), MFGPLL_SC1_CON1, 24, 0, 0, 0,
>>>> -	    MFGPLL_SC1_CON1, 0, 22),
>>>> +	    MFGPLL_SC1_CON1, 0, 22, mfgpll_rpm_clk_names),
>>>>    };
>>>>    
>>>> +struct clk_mt8196_mfg {
>>>> +	struct clk_hw_onecell_data *clk_data;
>>>> +	struct clk_bulk_data *rpm_clks;
>>>> +	unsigned int num_rpm_clks;
>>>> +};
>>>> +
>>>> +static int __maybe_unused clk_mt8196_mfg_resume(struct device *dev)
>>>> +{
>>>> +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
>>>> +
>>>> +	if (!clk_mfg || !clk_mfg->rpm_clks)
>>>> +		return 0;
>>>> +
>>>> +	return clk_bulk_prepare_enable(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
>>>> +}
>>>> +
>>>> +static int __maybe_unused clk_mt8196_mfg_suspend(struct device *dev)
>>>> +{
>>>> +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(dev);
>>>> +
>>>> +	if (!clk_mfg || !clk_mfg->rpm_clks)
>>>> +		return 0;
>>>> +
>>>> +	clk_bulk_disable_unprepare(clk_mfg->num_rpm_clks, clk_mfg->rpm_clks);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static const struct of_device_id of_match_clk_mt8196_mfg[] = {
>>>>    	{ .compatible = "mediatek,mt8196-mfgpll-pll-ctrl",
>>>>    	  .data = &mfg_ao_plls },
>>>> @@ -92,35 +127,60 @@ MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mfg);
>>>>    static int clk_mt8196_mfg_probe(struct platform_device *pdev)
>>>>    {
>>>>    	const struct mtk_pll_data *plls;
>>>> -	struct clk_hw_onecell_data *clk_data;
>>>> +	struct clk_mt8196_mfg *clk_mfg;
>>>>    	struct device_node *node = pdev->dev.of_node;
>>>> +	struct device *dev = &pdev->dev;
>>>>    	const int num_plls = 1;
>>>> -	int r;
>>>> +	int r, i;
>>>>    
>>>> -	plls = of_device_get_match_data(&pdev->dev);
>>>> +	plls = of_device_get_match_data(dev);
>>>>    	if (!plls)
>>>>    		return -EINVAL;
>>>>    
>>>> -	clk_data = mtk_alloc_clk_data(num_plls);
>>>> -	if (!clk_data)
>>>> +	clk_mfg = devm_kzalloc(dev, sizeof(*clk_mfg), GFP_KERNEL);
>>>> +	if (!clk_mfg)
>>>>    		return -ENOMEM;
>>>>    
>>>> -	r = mtk_clk_register_plls(&pdev->dev, plls, num_plls, clk_data);
>>>> +	clk_mfg->num_rpm_clks = plls[0].num_rpm_clks;
>>>> +
>>>> +	if (clk_mfg->num_rpm_clks) {
>>>> +		clk_mfg->rpm_clks = devm_kcalloc(dev, clk_mfg->num_rpm_clks,
>>>> +						 sizeof(*clk_mfg->rpm_clks),
>>>> +						 GFP_KERNEL);
>>>> +		if (!clk_mfg->rpm_clks)
>>>> +			return -ENOMEM;
>>>> +
>>>> +		for (i = 0; i < clk_mfg->num_rpm_clks; i++)
>>>> +			clk_mfg->rpm_clks->id = plls[0].rpm_clk_names[i];
>>>> +
>>>> +		r = devm_clk_bulk_get(dev, clk_mfg->num_rpm_clks,
>>>> +				      clk_mfg->rpm_clks);
>>>> +		if (r)
>>>> +			return r;
>>>> +	}
>>>> +
>>>> +	clk_mfg->clk_data = mtk_alloc_clk_data(num_plls);
>>>> +	if (!clk_mfg->clk_data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	dev_set_drvdata(dev, clk_mfg);
>>>> +	pm_runtime_enable(dev);
>>>> +
>>>> +	r = mtk_clk_register_plls(dev, plls, num_plls, clk_mfg->clk_data);
>>>>    	if (r)
>>>>    		goto free_clk_data;
>>>>    
>>>> -	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
>>>> +	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
>>>> +				   clk_mfg->clk_data);
>>>>    	if (r)
>>>>    		goto unregister_plls;
>>>>    
>>>> -	platform_set_drvdata(pdev, clk_data);
>>>> -
>>>>    	return r;
>>>>    
>>>>    unregister_plls:
>>>> -	mtk_clk_unregister_plls(plls, num_plls, clk_data);
>>>> +	mtk_clk_unregister_plls(plls, num_plls, clk_mfg->clk_data);
>>>>    free_clk_data:
>>>> -	mtk_free_clk_data(clk_data);
>>>> +	mtk_free_clk_data(clk_mfg->clk_data);
>>>>    
>>>>    	return r;
>>>>    }
>>>> @@ -128,20 +188,26 @@ static int clk_mt8196_mfg_probe(struct platform_device *pdev)
>>>>    static void clk_mt8196_mfg_remove(struct platform_device *pdev)
>>>>    {
>>>>    	const struct mtk_pll_data *plls = of_device_get_match_data(&pdev->dev);
>>>> -	struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
>>>> +	struct clk_mt8196_mfg *clk_mfg = dev_get_drvdata(&pdev->dev);
>>>>    	struct device_node *node = pdev->dev.of_node;
>>>>    
>>>>    	of_clk_del_provider(node);
>>>> -	mtk_clk_unregister_plls(plls, 1, clk_data);
>>>> -	mtk_free_clk_data(clk_data);
>>>> +	mtk_clk_unregister_plls(plls, 1, clk_mfg->clk_data);
>>>> +	mtk_free_clk_data(clk_mfg->clk_data);
>>>>    }
>>>>    
>>>> +static DEFINE_RUNTIME_DEV_PM_OPS(clk_mt8196_mfg_pm_ops,
>>>> +				 clk_mt8196_mfg_suspend,
>>>> +				 clk_mt8196_mfg_resume,
>>>> +				 NULL);
>>>> +
>>>>    static struct platform_driver clk_mt8196_mfg_drv = {
>>>>    	.probe = clk_mt8196_mfg_probe,
>>>>    	.remove = clk_mt8196_mfg_remove,
>>>>    	.driver = {
>>>>    		.name = "clk-mt8196-mfg",
>>>>    		.of_match_table = of_match_clk_mt8196_mfg,
>>>> +		.pm = pm_ptr(&clk_mt8196_mfg_pm_ops),
>>>>    	},
>>>>    };
>>>>    module_platform_driver(clk_mt8196_mfg_drv);
>>>> diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
>>>> index 0f2a1d19eea78b7390b221af47016eb9897f3596..82b86b849a67359d8f23d828f50422081c2747e3 100644
>>>> --- a/drivers/clk/mediatek/clk-pll.h
>>>> +++ b/drivers/clk/mediatek/clk-pll.h
>>>> @@ -53,6 +53,8 @@ struct mtk_pll_data {
>>>>    	u8 pll_en_bit; /* Assume 0, indicates BIT(0) by default */
>>>>    	u8 pcw_chg_bit;
>>>>    	u8 fenc_sta_bit;
>>>> +	const char * const *rpm_clk_names;
>>>> +	unsigned int num_rpm_clks;
>>>>    };
>>>>    
>>>>    /*
>>>>
>>>
>>>
>>>
>>
>>
> 
> 
> 
>