[2/7] dt-bindings: devfreq: Add bindings for generic imx buses

Commit Message

Leonard Crestez Aug. 12, 2019, 6:49 p.m. UTC

Add initial dt bindings for the interconnects inside i.MX chips.
Multiple external IPs are involved but SOC integration means the
software controllable interfaces are very similar.

This is initially only for imx8mm but add an "fsl,imx8m-nic" fallback
similar to exynos-bus.

Signed-off-by: Leonard Crestez <[e-mail]>
---
 .../devicetree/bindings/devfreq/imx.yaml      | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml

Comments

Rob Herring Aug. 12, 2019, 7:46 p.m. UTC | #1

On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez
<[e-mail]> wrote:
>
> Add initial dt bindings for the interconnects inside i.MX chips.
> Multiple external IPs are involved but SOC integration means the
> software controllable interfaces are very similar.
>
> This is initially only for imx8mm but add an "fsl,imx8m-nic" fallback
> similar to exynos-bus.
>
> Signed-off-by: Leonard Crestez <[e-mail]>
> ---
>  .../devicetree/bindings/devfreq/imx.yaml      | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml
>
> diff --git a/Documentation/devicetree/bindings/devfreq/imx.yaml b/Documentation/devicetree/bindings/devfreq/imx.yaml
> new file mode 100644
> index 000000000000..0e2ee3a5205e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/imx.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/devfreq/imx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic i.MX bus frequency device
> +
> +maintainers:
> +  - Leonard Crestez <[e-mail]>
> +
> +description: |
> +  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
> +  voltage) can be adjusted.
> +
> +  Some of those buses expose register areas mentioned in the memory maps as GPV
> +  ("Global Programmers View") but not all. Access to this area might be denied for
> +  normal world.
> +
> +  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
> +  FlexNOC but DT bindings are specific to the integration of these bus
> +  interconnect IPs into imx SOCs.

No need to use the interconnect binding?

> +
> +properties:
> +  compatible:
> +    contains:
> +      enum:
> +       - fsl,imx8m-noc
> +       - fsl,imx8m-nic

This means any combination of these 2 strings is valid. I suspect you
want a given node to have only one of them, so drop 'contains'

> +
> +  reg:
> +    maxItems: 1
> +    description: GPV area
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks

reg?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8mm-clock.h>
> +    noc: noc@32700000 {
> +            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";

Doesn't match the schema. (Well, it does with 'contains', but
fsl,imx8mm-noc is not documented.)

> +            reg = <0x32700000 0x100000>;
> +            clocks = <&clk IMX8MM_CLK_NOC>;
> +            operating-points-v2 = <&noc_opp_table>;

Not documented.

> +    };
> --
> 2.17.1
>

Leonard Crestez Aug. 13, 2019, 1:32 a.m. UTC | #2

On 8/12/2019 10:47 PM, Rob Herring wrote:
> On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez <[e-mail]> wrote:

>> Add initial dt bindings for the interconnects inside i.MX chips.
>> Multiple external IPs are involved but SOC integration means the
>> software controllable interfaces are very similar.
>>
>> +description: |
>> +  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
>> +  voltage) can be adjusted.
>> +
>> +  Some of those buses expose register areas mentioned in the memory maps as GPV
>> +  ("Global Programmers View") but not all. Access to this area might be denied for
>> +  normal world.
>> +
>> +  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
>> +  FlexNOC but DT bindings are specific to the integration of these bus
>> +  interconnect IPs into imx SOCs.
> 
> No need to use the interconnect binding?

Separate RFC: https://patchwork.kernel.org/patch/11078673/

The interconnect is represented by a separate "virtual" node which might 
not be OK. There was also a recent RFC from samsung which turns devfreq 
nodes into interconnect providers:
     https://patchwork.kernel.org/cover/11054417/

Is that preferable?

>> +required:
>> +  - compatible
>> +  - clocks
> 
> reg?

This is deliberately optional: for some NICs the GPV register area is 
not exposed in the memory map. This is unusual but an accurate 
description of the hardware.

The current driver doesn't even attempt to map registers, it only 
adjusts the clock.

>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/imx8mm-clock.h>
>> +    noc: noc@32700000 {
>> +            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
> 
> Doesn't match the schema. (Well, it does with 'contains', but
> fsl,imx8mm-noc is not documented.)

I'm confused about how per-SOC compatible strings works with validation. 
There is a rule that every SOC dtsi needs to add soc prefix to all 
device nodes but of_device_id in driver code doesn't need to be updated.

Without using "contains" on the "compatible" property then all 
SOC-specific compatible strings would need to be mentioned in every yaml 
files. Unless I'm missing something this means updating update every 
binding file for each new SOC?

I guess it can be useful because it also validates the compatible 
sequence itself.

For this current example something like this seems to work:

   compatible:
     oneOf:
       - items:
         - enum:
           - fsl,imx8mm-nic
           - fsl,imx8mq-nic
         - const: fsl,imx8m-nic
       - items:
         - enum:
           - fsl,imx8mm-noc
           - fsl,imx8mq-noc
         - const: fsl,imx8m-noc

--
Regards,
Leonard

MyungJoo Ham Aug. 13, 2019, 2:25 a.m. UTC | #3

>Add initial dt bindings for the interconnects inside i.MX chips.
>Multiple external IPs are involved but SOC integration means the
>software controllable interfaces are very similar.
>
>This is initially only for imx8mm but add an "fsl,imx8m-nic" fallback
>similar to exynos-bus.
>
>Signed-off-by: Leonard Crestez <[e-mail]>
>---
> .../devicetree/bindings/devfreq/imx.yaml      | 50 +++++++++++++++++++
> 1 file changed, 50 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/devfreq/imx.yaml

Acked-by: MyungJoo Ham <[e-mail]>

Rob Herring Aug. 13, 2019, 2:06 p.m. UTC | #4

On Mon, Aug 12, 2019 at 7:32 PM Leonard Crestez <[e-mail]> wrote:
>
> On 8/12/2019 10:47 PM, Rob Herring wrote:
> > On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez <[e-mail]> wrote:
>
> >> Add initial dt bindings for the interconnects inside i.MX chips.
> >> Multiple external IPs are involved but SOC integration means the
> >> software controllable interfaces are very similar.
> >>
> >> +description: |
> >> +  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
> >> +  voltage) can be adjusted.
> >> +
> >> +  Some of those buses expose register areas mentioned in the memory maps as GPV
> >> +  ("Global Programmers View") but not all. Access to this area might be denied for
> >> +  normal world.
> >> +
> >> +  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
> >> +  FlexNOC but DT bindings are specific to the integration of these bus
> >> +  interconnect IPs into imx SOCs.
> >
> > No need to use the interconnect binding?
>
> Separate RFC: https://patchwork.kernel.org/patch/11078673/
>
> The interconnect is represented by a separate "virtual" node which might
> not be OK. There was also a recent RFC from samsung which turns devfreq
> nodes into interconnect providers:
>      https://patchwork.kernel.org/cover/11054417/
>
> Is that preferable?

Virtual nodes are not OK.

>
> >> +required:
> >> +  - compatible
> >> +  - clocks
> >
> > reg?
>
> This is deliberately optional: for some NICs the GPV register area is
> not exposed in the memory map. This is unusual but an accurate
> description of the hardware.

Different h/w blocks should have different compatibles. GPV is an Arm
thing and I'd expect FlexNOC to be different.

> The current driver doesn't even attempt to map registers, it only
> adjusts the clock.

Irrelevant to the binding...

>
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/clock/imx8mm-clock.h>
> >> +    noc: noc@32700000 {
> >> +            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
> >
> > Doesn't match the schema. (Well, it does with 'contains', but
> > fsl,imx8mm-noc is not documented.)
>
> I'm confused about how per-SOC compatible strings works with validation.
> There is a rule that every SOC dtsi needs to add soc prefix to all
> device nodes but of_device_id in driver code doesn't need to be updated.
>
> Without using "contains" on the "compatible" property then all
> SOC-specific compatible strings would need to be mentioned in every yaml
> files. Unless I'm missing something this means updating update every
> binding file for each new SOC?

Yes. The main exception is if various SoCs are just packaging,
binning, or fuse differences.

>
> I guess it can be useful because it also validates the compatible
> sequence itself.

Right. Order matters.

>
> For this current example something like this seems to work:
>
>    compatible:
>      oneOf:
>        - items:
>          - enum:
>            - fsl,imx8mm-nic
>            - fsl,imx8mq-nic
>          - const: fsl,imx8m-nic
>        - items:
>          - enum:
>            - fsl,imx8mm-noc
>            - fsl,imx8mq-noc
>          - const: fsl,imx8m-noc

Looks correct.

Rob

Leonard Crestez Aug. 13, 2019, 2:59 p.m. UTC | #5

On 13.08.2019 17:06, Rob Herring wrote:
> On Mon, Aug 12, 2019 at 7:32 PM Leonard Crestez <[e-mail]> wrote:
>> On 8/12/2019 10:47 PM, Rob Herring wrote:
>>> On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez <[e-mail]> wrote:

>>>> Add initial dt bindings for the interconnects inside i.MX chips.
>>>> Multiple external IPs are involved but SOC integration means the
>>>> software controllable interfaces are very similar.
>>>>
>>>> +description: |
>>>> +  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
>>>> +  voltage) can be adjusted.
>>>> +
>>>> +  Some of those buses expose register areas mentioned in the memory maps as GPV
>>>> +  ("Global Programmers View") but not all. Access to this area might be denied for
>>>> +  normal world.
>>>> +
>>>> +  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
>>>> +  FlexNOC but DT bindings are specific to the integration of these bus
>>>> +  interconnect IPs into imx SOCs.
>>>
>>> No need to use the interconnect binding?
>>
>> The interconnect is represented by a separate "virtual" node which might
>> not be OK. There was also a recent RFC from samsung which turns devfreq
>> nodes into interconnect providers:
>>
>> Is that preferable?
> 
> Virtual nodes are not OK.

Then I'll try to make the "interconnect" device probe from a soc driver 
and turn devfreq nodes into interconnect providers backed by this same 
singleton device.

Still separate from this series.

>>>> +required:
>>>> +  - compatible
>>>> +  - clocks
>>>
>>> reg?
>>
>> This is deliberately optional: for some NICs the GPV register area is
>> not exposed in the memory map. This is unusual but an accurate
>> description of the hardware.
> 
> Different h/w blocks should have different compatibles. GPV is an Arm
> thing and I'd expect FlexNOC to be different.

The imx reference manuals call them both "GPV" though layout is indeed 
quite different (and for FlexNoC it's not even documented).

The h/w blocks do have different compat strings (imx8m-nic and 
imx8m-noc). They have a single binding document because didn't want to 
create two nearly-identical bindings, I assume it would be fine to split 
later if needed.

--
Regards,
Leonard

Patch

diff --git a/Documentation/devicetree/bindings/devfreq/imx.yaml b/Documentation/devicetree/bindings/devfreq/imx.yaml
new file mode 100644
index 000000000000..0e2ee3a5205e
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/imx.yaml
@@ -0,0 +1,50 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/devfreq/imx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <[e-mail]>
+
+description: |
+  The i.MX SoC family has multiple buses for which clock frequency (and sometimes
+  voltage) can be adjusted.
+
+  Some of those buses expose register areas mentioned in the memory maps as GPV
+  ("Global Programmers View") but not all. Access to this area might be denied for
+  normal world.
+
+  The buses are based on externally licensed IPs such as ARM NIC-301 and Arteris
+  FlexNOC but DT bindings are specific to the integration of these bus
+  interconnect IPs into imx SOCs.
+ 
+properties:
+  compatible:
+    contains:
+      enum:
+       - fsl,imx8m-noc
+       - fsl,imx8m-nic
+
+  reg:
+    maxItems: 1
+    description: GPV area
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+    noc: noc@32700000 {
+            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
+            reg = <0x32700000 0x100000>;
+            clocks = <&clk IMX8MM_CLK_NOC>;
+            operating-points-v2 = <&noc_opp_table>;
+    };