MediaTek Runtime Power Management Clocks for PLL

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

Posted by Nicolas Frattaroli 2 days, 10 hours ago

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

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

Posted by Conor Dooley 2 days, 5 hours ago

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
>

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

Posted by Nicolas Frattaroli 1 day, 6 hours ago

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

Posted by Conor Dooley 1 day, 4 hours ago

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.

[PATCH 1/4] dt-bindings: clock: mediatek: Add clocks for MT8196 mfgpll
[PATCH 2/4] clk: mediatek: Refactor pll registration to pass device
[PATCH 3/4] clk: mediatek: Pass device to clk_hw_register for PLLs
[PATCH 4/4] clk: mediatek: Add rpm clocks to clk-mt8196-mfg