diff mbox series

[v2,1/6] dt-bindings: clock: imx8m: document nominal/overdrive properties

Message ID 20250106-imx8m-clk-v2-1-6aaeadac65fe@pengutronix.de (mailing list archive)
State Superseded, archived
Headers show
Series arm64: dts: freescale: imx8mp-skov: switch to nominal drive mode | expand

Commit Message

Ahmad Fatoum Jan. 6, 2025, 2:21 p.m. UTC 
The imx8m-clock.yaml binding covers the clock controller inside all
of the i.MX8M Q/M/N/P SoCs. All of them have in common that they
support two operating modes: nominal and overdrive mode.

While the overdrive mode allows for higher frequencies for many IPs,
the nominal mode needs a lower SoC voltage, thereby reducing
heat generation and power usage.

As increasing clock rates beyond the maximum permitted by the supplied
SoC voltage can lead to difficult to debug issues, device tree consumers
would benefit from knowing what mode is active to enforce the clock rate
limits that come with it.

To facilitate this, extend the clock controller bindings with two mutually
exclusive optional properties. Allowing for absence of both properties is
needed, because there is no default suitable for all boards:

For i.MX8M Mini and Nano, the kernel SoC DTSIs has assigned-clock-rates
that are all achievable in nominal mode. For i.MX8MP, there are some
rates only validated for overdrive mode.

But even for the i.MX8M Mini/Nano boards, we don't know what rates they
may configure at runtime, so it has not been possible so far to infer from
just the device tree what the mode is.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 Documentation/devicetree/bindings/clock/imx8m-clock.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Frank Li Jan. 6, 2025, 4:41 p.m. UTC | #1 
On Mon, Jan 06, 2025 at 03:21:42PM +0100, Ahmad Fatoum wrote:
> The imx8m-clock.yaml binding covers the clock controller inside all
> of the i.MX8M Q/M/N/P SoCs. All of them have in common that they
> support two operating modes: nominal and overdrive mode.
>
> While the overdrive mode allows for higher frequencies for many IPs,
> the nominal mode needs a lower SoC voltage, thereby reducing
> heat generation and power usage.
>
> As increasing clock rates beyond the maximum permitted by the supplied
> SoC voltage can lead to difficult to debug issues, device tree consumers
> would benefit from knowing what mode is active to enforce the clock rate
> limits that come with it.
>
> To facilitate this, extend the clock controller bindings with two mutually
> exclusive optional properties. Allowing for absence of both properties is
> needed, because there is no default suitable for all boards:
>
> For i.MX8M Mini and Nano, the kernel SoC DTSIs has assigned-clock-rates
> that are all achievable in nominal mode. For i.MX8MP, there are some
> rates only validated for overdrive mode.
>
> But even for the i.MX8M Mini/Nano boards, we don't know what rates they
> may configure at runtime, so it has not been possible so far to infer from
> just the device tree what the mode is.
>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/clock/imx8m-clock.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> index c643d4a814786a1fc7e559140fe58911990f71bb..a6ae5257ef531f3fa8506d515d202f5b7842c4fa 100644
> --- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
> @@ -43,6 +43,14 @@ properties:
>        ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h
>        for the full list of i.MX8M clock IDs.
>
> +  fsl,nominal-mode:
> +    description: Set if SoC is operated in nominal mode
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +  fsl,overdrive-mode:
> +    description: Set if SoC is operated in overdrive mode
> +    $ref: /schemas/types.yaml#/definitions/flag
> +

why need two flags? suppose "not set fsl,overdrive-mode" is equal to
fsl,nominal-mode

Frank

>  required:
>    - compatible
>    - reg
> @@ -95,6 +103,12 @@ allOf:
>              - const: clk_ext2
>              - const: clk_ext3
>              - const: clk_ext4
> +  - if:
> +      required:
> +        - fsl,overdrive-mode
> +    then:
> +      properties:
> +        fsl,nominal-mode: false
>
>  additionalProperties: false
>
>
> --
> 2.39.5
>
Ahmad Fatoum Jan. 6, 2025, 7:25 p.m. UTC | #2 
Hello Frank,

On 06.01.25 17:41, Frank Li wrote:
> On Mon, Jan 06, 2025 at 03:21:42PM +0100, Ahmad Fatoum wrote:

[snip]

>> Allowing for absence of both properties is
>> needed, because there is no default suitable for all boards:
>>
>> For i.MX8M Mini and Nano, the kernel SoC DTSIs has assigned-clock-rates
>> that are all achievable in nominal mode. For i.MX8MP, there are some
>> rates only validated for overdrive mode.
>>
>> But even for the i.MX8M Mini/Nano boards, we don't know what rates they
>> may configure at runtime, so it has not been possible so far to infer from
>> just the device tree what the mode is.

[snip]

>> +  fsl,nominal-mode:
>> +    description: Set if SoC is operated in nominal mode
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +
>> +  fsl,overdrive-mode:
>> +    description: Set if SoC is operated in overdrive mode
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +
> 
> why need two flags? suppose "not set fsl,overdrive-mode" is equal to
> fsl,nominal-mode

Conor had the same question on v1[1] and I have addressed this in the commit
message of v2, see above.

In short, there is unfortunately no universally applicable default.

[1]: https://lore.kernel.org/all/4e2250b3-5170-4e88-aa0a-dd796b81e78b@pengutronix.de/

Thanks,
Ahmad


> 
> Frank
> 
>>  required:
>>    - compatible
>>    - reg
>> @@ -95,6 +103,12 @@ allOf:
>>              - const: clk_ext2
>>              - const: clk_ext3
>>              - const: clk_ext4
>> +  - if:
>> +      required:
>> +        - fsl,overdrive-mode
>> +    then:
>> +      properties:
>> +        fsl,nominal-mode: false
>>
>>  additionalProperties: false
>>
>>
>> --
>> 2.39.5
>>
>
Frank Li Jan. 6, 2025, 7:42 p.m. UTC | #3 
On Mon, Jan 06, 2025 at 08:25:38PM +0100, Ahmad Fatoum wrote:
> Hello Frank,
>
> On 06.01.25 17:41, Frank Li wrote:
> > On Mon, Jan 06, 2025 at 03:21:42PM +0100, Ahmad Fatoum wrote:
>
> [snip]
>
> >> Allowing for absence of both properties is
> >> needed, because there is no default suitable for all boards:
> >>
> >> For i.MX8M Mini and Nano, the kernel SoC DTSIs has assigned-clock-rates
> >> that are all achievable in nominal mode. For i.MX8MP, there are some
> >> rates only validated for overdrive mode.
> >>
> >> But even for the i.MX8M Mini/Nano boards, we don't know what rates they
> >> may configure at runtime, so it has not been possible so far to infer from
> >> just the device tree what the mode is.
>
> [snip]
>
> >> +  fsl,nominal-mode:
> >> +    description: Set if SoC is operated in nominal mode
> >> +    $ref: /schemas/types.yaml#/definitions/flag
> >> +
> >> +  fsl,overdrive-mode:
> >> +    description: Set if SoC is operated in overdrive mode
> >> +    $ref: /schemas/types.yaml#/definitions/flag
> >> +
> >
> > why need two flags? suppose "not set fsl,overdrive-mode" is equal to
> > fsl,nominal-mode
>
> Conor had the same question on v1[1] and I have addressed this in the commit
> message of v2, see above.
>
> In short, there is unfortunately no universally applicable default.
>
> [1]: https://lore.kernel.org/all/4e2250b3-5170-4e88-aa0a-dd796b81e78b@pengutronix.de/
>

But there are problem if use dts overlay because dts overlay can't support
delete property.

If one dts file use "fsl,overdrive-mode", dts overlay file can't delete
this property. "mutually exclusive optional properties" was not preferred.

mode = ["normal" | "overdrive"]

Frank

> Thanks,
> Ahmad
>
>
> >
> > Frank
> >
> >>  required:
> >>    - compatible
> >>    - reg
> >> @@ -95,6 +103,12 @@ allOf:
> >>              - const: clk_ext2
> >>              - const: clk_ext3
> >>              - const: clk_ext4
> >> +  - if:
> >> +      required:
> >> +        - fsl,overdrive-mode
> >> +    then:
> >> +      properties:
> >> +        fsl,nominal-mode: false
> >>
> >>  additionalProperties: false
> >>
> >>
> >> --
> >> 2.39.5
> >>
> >
>
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Ahmad Fatoum Jan. 6, 2025, 7:55 p.m. UTC | #4 
Hello Frank,

On 06.01.25 20:42, Frank Li wrote:
> On Mon, Jan 06, 2025 at 08:25:38PM +0100, Ahmad Fatoum wrote:
>> On 06.01.25 17:41, Frank Li wrote:
>>> On Mon, Jan 06, 2025 at 03:21:42PM +0100, Ahmad Fatoum wrote:
>>>> Allowing for absence of both properties is
>>>> needed, because there is no default suitable for all boards:
>>>>
>>>> For i.MX8M Mini and Nano, the kernel SoC DTSIs has assigned-clock-rates
>>>> that are all achievable in nominal mode. For i.MX8MP, there are some
>>>> rates only validated for overdrive mode.
>>>>
>>>> But even for the i.MX8M Mini/Nano boards, we don't know what rates they
>>>> may configure at runtime, so it has not been possible so far to infer from
>>>> just the device tree what the mode is.
>>
>> [snip]
>>
>>>> +  fsl,nominal-mode:
>>>> +    description: Set if SoC is operated in nominal mode
>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>> +
>>>> +  fsl,overdrive-mode:
>>>> +    description: Set if SoC is operated in overdrive mode
>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>> +
>>>
>>> why need two flags? suppose "not set fsl,overdrive-mode" is equal to
>>> fsl,nominal-mode
>>
>> Conor had the same question on v1[1] and I have addressed this in the commit
>> message of v2, see above.
>>
>> In short, there is unfortunately no universally applicable default.
>>
>> [1]: https://lore.kernel.org/all/4e2250b3-5170-4e88-aa0a-dd796b81e78b@pengutronix.de/
>>
> 
> But there are problem if use dts overlay because dts overlay can't support
> delete property.
> 
> If one dts file use "fsl,overdrive-mode", dts overlay file can't delete
> this property. "mutually exclusive optional properties" was not preferred.
> 
> mode = ["normal" | "overdrive"]

I don't see myself setting this via overlay, but if you say it's useful, we can
adjust the binding.

I am not sure what's the preference is though, should we have

  fsl,operating-mode = "normal" / "overdrive"

or 

  fsl,operating-mode = <IMX_OPMODE_NORMAL> / <IMX_OPMODE_OVERDRIVE>


Any idea? Conor?

Cheers,
Ahmad

> 
> Frank
> 
>> Thanks,
>> Ahmad
>>
>>
>>>
>>> Frank
>>>
>>>>  required:
>>>>    - compatible
>>>>    - reg
>>>> @@ -95,6 +103,12 @@ allOf:
>>>>              - const: clk_ext2
>>>>              - const: clk_ext3
>>>>              - const: clk_ext4
>>>> +  - if:
>>>> +      required:
>>>> +        - fsl,overdrive-mode
>>>> +    then:
>>>> +      properties:
>>>> +        fsl,nominal-mode: false
>>>>
>>>>  additionalProperties: false
>>>>
>>>>
>>>> --
>>>> 2.39.5
>>>>
>>>
>>
>>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
Krzysztof Kozlowski Jan. 7, 2025, 6:52 a.m. UTC | #5 
On Mon, Jan 06, 2025 at 08:55:08PM +0100, Ahmad Fatoum wrote:
> >>> why need two flags? suppose "not set fsl,overdrive-mode" is equal to
> >>> fsl,nominal-mode
> >>
> >> Conor had the same question on v1[1] and I have addressed this in the commit
> >> message of v2, see above.
> >>
> >> In short, there is unfortunately no universally applicable default.
> >>
> >> [1]: https://lore.kernel.org/all/4e2250b3-5170-4e88-aa0a-dd796b81e78b@pengutronix.de/
> >>
> > 
> > But there are problem if use dts overlay because dts overlay can't support
> > delete property.
> > 
> > If one dts file use "fsl,overdrive-mode", dts overlay file can't delete
> > this property. "mutually exclusive optional properties" was not preferred.
> > 
> > mode = ["normal" | "overdrive"]
> 
> I don't see myself setting this via overlay, but if you say it's useful, we can
> adjust the binding.
> 
> I am not sure what's the preference is though, should we have
> 
>   fsl,operating-mode = "normal" / "overdrive"
> 
> or 
> 
>   fsl,operating-mode = <IMX_OPMODE_NORMAL> / <IMX_OPMODE_OVERDRIVE>
> 
> 
> Any idea? Conor?

If these are two exclusive modes (plus default), then as Frank
suggested: use enum of strings. Imagine encoding enum of 10 values with
bools as you did: that would be enourmous oneOf condition or if:then:.

Preferred is strings, but numerical values are also accepted.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
index c643d4a814786a1fc7e559140fe58911990f71bb..a6ae5257ef531f3fa8506d515d202f5b7842c4fa 100644
--- a/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
+++ b/Documentation/devicetree/bindings/clock/imx8m-clock.yaml
@@ -43,6 +43,14 @@  properties:
       ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8m-clock.h
       for the full list of i.MX8M clock IDs.
 
+  fsl,nominal-mode:
+    description: Set if SoC is operated in nominal mode
+    $ref: /schemas/types.yaml#/definitions/flag
+
+  fsl,overdrive-mode:
+    description: Set if SoC is operated in overdrive mode
+    $ref: /schemas/types.yaml#/definitions/flag
+
 required:
   - compatible
   - reg
@@ -95,6 +103,12 @@  allOf:
             - const: clk_ext2
             - const: clk_ext3
             - const: clk_ext4
+  - if:
+      required:
+        - fsl,overdrive-mode
+    then:
+      properties:
+        fsl,nominal-mode: false
 
 additionalProperties: false