interconnect: Add imx support via devfreq

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.

Main NOC node acts as interconnect provider if #interconnect-cells is present.

Multiple interconnects can be present, each with their own OPP table.

Signed-off-by Leonard Crestez <[email protected]>

---
 .../bindings/interconnect/fsl,imx8m-noc.yaml  | 104 ++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +42 -0

new file mode 100644
index 000000000000..5cd94185fec3
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: https://protect2.fireeye.com/url?k=0c13f3e0-51df3f45-0c1278af-0cc47a30d446-77e809543b673ffd&u=http://devicetree.org/schemas/interconnect/fsl,imx8m-noc.yaml#
+$schema: https://protect2.fireeye.com/url?k=87c672dc-da0abe79-87c7f993-0cc47a30d446-414d3b4d0127419a&u=http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <[email protected]>
+
+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 (non-secure) 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:
+    oneOf:
+      - items:
+        - enum:
+          - fsl,imx8mn-nic
+          - fsl,imx8mm-nic
+          - fsl,imx8mq-nic
+        - const: fsl,imx8m-nic
+      - items:
+        - enum:
+          - fsl,imx8mn-noc
+          - fsl,imx8mm-noc
+          - fsl,imx8mq-noc
+        - const: fsl,imx8m-noc
+
+  reg:
+    maxItems: 1
+

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +18 -0

+      passive governor.
+
+  '#interconnect-cells':
+    description:
+      If specified then also act as an interconnect provider. Should only be
+      set once per soc on main noc.
+    const: 1
+
+  fsl,scalable-node-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of node ids for scalable nodes. Uses same numeric identifier
+      namespace as the consumer "interconnects" binding.
+
+  fsl,scalable-nodes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Array of phandles to scalable nodes. Must be of same length as

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +24 -0

+    #include <dt-bindings/clock/imx8mq-clock.h>
+    #include <dt-bindings/interconnect/imx8mq.h>
+    noc: interconnect@32700000 {
+            compatible = "fsl,imx8mq-noc", "fsl,imx8m-noc";
+            reg = <0x32700000 0x100000>;
+            clocks = <&clk IMX8MQ_CLK_NOC>;
+            #interconnect-cells = <1>;
+            fsl,scalable-node-ids = <IMX8MQ_ICN_NOC>,
+                                    <IMX8MQ_ICS_DRAM>;
+            fsl,scalable-nodes = <&noc>,
+                                 <&ddrc>;
+            operating-points-v2 = <&noc_opp_table>;
+
+            noc_opp_table: opp-table {
+                    compatible = "operating-points-v2";
+
+                    opp-133M {
+                            opp-hz = /bits/ 64 <133333333>;
+                    };
+                    opp-800M {
+                            opp-hz = /bits/ 64 <800000000>;
+                    };
+            };
+    };

Quoted 67 lines · Leonard Crestez

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.

Main NOC node acts as interconnect provider if #interconnect-cells is present.

Multiple interconnects can be present, each with their own OPP table.

Signed-off-by Leonard Crestez <[email protected]>

---
 .../bindings/interconnect/fsl,imx8m-noc.yaml  | 104 ++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +46 -0

new file mode 100644
index 000000000000..5cd94185fec3
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: https://protect2.fireeye.com/url?k=0c13f3e0-51df3f45-0c1278af-0cc47a30d446-77e809543b673ffd&u=http://devicetree.org/schemas/interconnect/fsl,imx8m-noc.yaml#
+$schema: https://protect2.fireeye.com/url?k=87c672dc-da0abe79-87c7f993-0cc47a30d446-414d3b4d0127419a&u=http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <[email protected]>
+
+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 (non-secure) 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:
+    oneOf:
+      - items:
+        - enum:
+          - fsl,imx8mn-nic
+          - fsl,imx8mm-nic
+          - fsl,imx8mq-nic
+        - const: fsl,imx8m-nic
+      - items:
+        - enum:
+          - fsl,imx8mn-noc
+          - fsl,imx8mm-noc
+          - fsl,imx8mq-noc
+        - const: fsl,imx8m-noc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  operating-points-v2: true

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 52

Quoted 6 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +6 -0

+  opp-table: true
+
+  devfreq:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description:
+      Phandle to another devfreq device to match OPPs with by using the

Better to use 'parent' instead of 'another' word for improving the understanding.

Quoted 19 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +18 -0

+      passive governor.
+
+  '#interconnect-cells':
+    description:
+      If specified then also act as an interconnect provider. Should only be
+      set once per soc on main noc.
+    const: 1
+
+  fsl,scalable-node-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of node ids for scalable nodes. Uses same numeric identifier
+      namespace as the consumer "interconnects" binding.
+
+  fsl,scalable-nodes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Array of phandles to scalable nodes. Must be of same length as

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 80

Quoted 10 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +10 -0

+      fsl,scalable-node-ids.
+
+required:
+  - compatible
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |

Is it enough example to understand the relation between imx8m-ddrc.c, imx-devfreq.c and imx interconnect driver?

In my case, if possible, hope to show the more detailed example. This example seems that don't contain the ddrc dt node (imx8m-ddrc.c).

Quoted 25 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +24 -0

+    #include <dt-bindings/clock/imx8mq-clock.h>
+    #include <dt-bindings/interconnect/imx8mq.h>
+    noc: interconnect@32700000 {
+            compatible = "fsl,imx8mq-noc", "fsl,imx8m-noc";
+            reg = <0x32700000 0x100000>;
+            clocks = <&clk IMX8MQ_CLK_NOC>;
+            #interconnect-cells = <1>;
+            fsl,scalable-node-ids = <IMX8MQ_ICN_NOC>,
+                                    <IMX8MQ_ICS_DRAM>;
+            fsl,scalable-nodes = <&noc>,
+                                 <&ddrc>;
+            operating-points-v2 = <&noc_opp_table>;
+
+            noc_opp_table: opp-table {
+                    compatible = "operating-points-v2";
+
+                    opp-133M {
+                            opp-hz = /bits/ 64 <133333333>;
+                    };
+                    opp-800M {
+                            opp-hz = /bits/ 64 <800000000>;
+                    };
+            };
+    };

Hi,

Quoted 68 lines · Chanwoo Choi

Quoted 67 lines · Leonard Crestez

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.

Main NOC node acts as interconnect provider if #interconnect-cells is present.

Multiple interconnects can be present, each with their own OPP table.

Signed-off-by Leonard Crestez <[email protected]>

---
  .../bindings/interconnect/fsl,imx8m-noc.yaml  | 104 ++++++++++++++++++
  1 file changed, 104 insertions(+)

create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +46 -0

new file mode 100644
index 000000000000..5cd94185fec3
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3D0c13f3e0-51df3f45-0c1278af-0cc47a30d446-77e809543b673ffd%26u%3Dhttp%3A%2F%2Fdevicetree.org%2Fschemas%2Finterconnect%2Ffsl%2Cimx8m-noc.yaml%23&data=02%7C01%7Cleonard.crestez%40nxp.com%7C2d1f1868afa140702a6b08d781d6ab68%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637120631307418544&sdata=H2q5nQlKYyLIivkBYUTaRD1Nu3WcnphPJny3k%2BK%2BGFE%3D&reserved=0
+$schema: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3D87c672dc-da0abe79-87c7f993-0cc47a30d446-414d3b4d0127419a%26u%3Dhttp%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=02%7C01%7Cleonard.crestez%40nxp.com%7C2d1f1868afa140702a6b08d781d6ab68%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637120631307418544&sdata=T6PgQ1DWI4OLOx3gifRRt%2FNImdVrgDUoswZ%2FNKw3oR8%3D&reserved=0
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <[email protected]>
+
+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 (non-secure) 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:
+    oneOf:
+      - items:
+        - enum:
+          - fsl,imx8mn-nic
+          - fsl,imx8mm-nic
+          - fsl,imx8mq-nic
+        - const: fsl,imx8m-nic
+      - items:
+        - enum:
+          - fsl,imx8mn-noc
+          - fsl,imx8mm-noc
+          - fsl,imx8mq-noc
+        - const: fsl,imx8m-noc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  operating-points-v2: true

Quoted 8 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 52

Quoted 6 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +6 -0

+  opp-table: true
+
+  devfreq:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description:
+      Phandle to another devfreq device to match OPPs with by using the

Better to use 'parent' instead of 'another' word for improving the understanding.

I think that 'devfreq' is not proper way to get the parent node
in devicetree. Because 'devfreq' name is linuxium. The property name didn't indicate the any h/w device. So, I'll make 'devfreq' property deprecated.

So, you better to make the specific property for this device driver like as following: and use devfreq_get_devfreq_by_node() function which is developed by you in order to get the devfreq instance node.

	fsl,parent-device = <&parent devfreq device>;

[1] [PATCH RFC v5 04/10] PM / devfreq: Add devfreq_get_devfreq_by_node

Quoted 36 lines

Quoted 19 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +18 -0

+      passive governor.
+
+  '#interconnect-cells':
+    description:
+      If specified then also act as an interconnect provider. Should only be
+      set once per soc on main noc.
+    const: 1
+
+  fsl,scalable-node-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of node ids for scalable nodes. Uses same numeric identifier
+      namespace as the consumer "interconnects" binding.
+
+  fsl,scalable-nodes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Array of phandles to scalable nodes. Must be of same length as

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 80

Quoted 10 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +10 -0

+      fsl,scalable-node-ids.
+
+required:
+  - compatible
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |

Is it enough example to understand the relation between imx8m-ddrc.c, imx-devfreq.c and imx interconnect driver?

In my case, if possible, hope to show the more detailed example. This example seems that don't contain the ddrc dt node (imx8m-ddrc.c).

Quoted 17 lines

Quoted 17 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +16 -0

+    #include <dt-bindings/clock/imx8mq-clock.h>
+    #include <dt-bindings/interconnect/imx8mq.h>
+    noc: interconnect@32700000 {
+            compatible = "fsl,imx8mq-noc", "fsl,imx8m-noc";
+            reg = <0x32700000 0x100000>;
+            clocks = <&clk IMX8MQ_CLK_NOC>;
+            #interconnect-cells = <1>;
+            fsl,scalable-node-ids = <IMX8MQ_ICN_NOC>,
+                                    <IMX8MQ_ICS_DRAM>;
+            fsl,scalable-nodes = <&noc>,
+                                 <&ddrc>;
+            operating-points-v2 = <&noc_opp_table>;
+
+            noc_opp_table: opp-table {
+                    compatible = "operating-points-v2";
+

Quoted 8 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 104

Quoted 8 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +8 -0

+                    opp-133M {
+                            opp-hz = /bits/ 64 <133333333>;
+                    };
+                    opp-800M {
+                            opp-hz = /bits/ 64 <800000000>;
+                    };
+            };
+    };

Quoted 84 lines · Chanwoo Choi

Hi,

Quoted 68 lines · Chanwoo Choi

Quoted 67 lines · Leonard Crestez

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.

Main NOC node acts as interconnect provider if #interconnect-cells is present.

Multiple interconnects can be present, each with their own OPP table.

Signed-off-by Leonard Crestez <[email protected]>

---
  .../bindings/interconnect/fsl,imx8m-noc.yaml  | 104 ++++++++++++++++++
  1 file changed, 104 insertions(+)

create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +46 -0

new file mode 100644
index 000000000000..5cd94185fec3
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: https://protect2.fireeye.com/url?k=8570eb5a-d8a45732-85716015-0cc47a3356b2-92a5b92cc514d07e&u=https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3D0c13f3e0-51df3f45-0c1278af-0cc47a30d446-77e809543b673ffd%26u%3Dhttp%3A%2F%2Fdevicetree.org%2Fschemas%2Finterconnect%2Ffsl%2Cimx8m-noc.yaml%23&data=02%7C01%7Cleonard.crestez%40nxp.com%7C2d1f1868afa140702a6b08d781d6ab68%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637120631307418544&sdata=H2q5nQlKYyLIivkBYUTaRD1Nu3WcnphPJny3k%2BK%2BGFE%3D&reserved=0
+$schema: https://protect2.fireeye.com/url?k=f7cec483-aa1a78eb-f7cf4fcc-0cc47a3356b2-4154a3c43886f5ed&u=https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3D87c672dc-da0abe79-87c7f993-0cc47a30d446-414d3b4d0127419a%26u%3Dhttp%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=02%7C01%7Cleonard.crestez%40nxp.com%7C2d1f1868afa140702a6b08d781d6ab68%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637120631307418544&sdata=T6PgQ1DWI4OLOx3gifRRt%2FNImdVrgDUoswZ%2FNKw3oR8%3D&reserved=0
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <[email protected]>
+
+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 (non-secure) 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:
+    oneOf:
+      - items:
+        - enum:
+          - fsl,imx8mn-nic
+          - fsl,imx8mm-nic
+          - fsl,imx8mq-nic
+        - const: fsl,imx8m-nic
+      - items:
+        - enum:
+          - fsl,imx8mn-noc
+          - fsl,imx8mm-noc
+          - fsl,imx8mq-noc
+        - const: fsl,imx8m-noc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  operating-points-v2: true

Quoted 8 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 52

Quoted 6 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +6 -0

+  opp-table: true
+
+  devfreq:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description:
+      Phandle to another devfreq device to match OPPs with by using the

Better to use 'parent' instead of 'another' word for improving the understanding.

I think that 'devfreq' is not proper way to get the parent node
in devicetree. Because 'devfreq' name is linuxium. The property name didn't indicate the any h/w device. So, I'll make 'devfreq' property deprecated.

Quoted 5 lines

So, you better to make the specific property for this device driver like as following: and use devfreq_get_devfreq_by_node() function which is developed by you in order to get the devfreq instance node.

	fsl,parent-device = <&parent devfreq device>;

This is only a "parent" in the sense that it's assigned to devfreq_passive.data.parent. The "devfreq" name is indeed too generic.

The DDRC can measure bandwith usage and I want to use the passive governor to make the main NOC match OPPs.

But at the bus level DDRC only has AXI and APB slave ports.

Buses on imx don't have a parent/child relationship; in fact there are even a few cycles.

Quoted 37 lines

[1] [PATCH RFC v5 04/10] PM / devfreq: Add devfreq_get_devfreq_by_node

Quoted 33 lines

Quoted 19 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +18 -0

+      passive governor.
+
+  '#interconnect-cells':
+    description:
+      If specified then also act as an interconnect provider. Should only be
+      set once per soc on main noc.
+    const: 1
+
+  fsl,scalable-node-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of node ids for scalable nodes. Uses same numeric identifier
+      namespace as the consumer "interconnects" binding.
+
+  fsl,scalable-nodes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Array of phandles to scalable nodes. Must be of same length as

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 80

Quoted 10 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +10 -0

+      fsl,scalable-node-ids.
+
+required:
+  - compatible
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |

Is it enough example to understand the relation between imx8m-ddrc.c, imx-devfreq.c and imx interconnect driver?

Quoted 3 lines

Quoted 3 lines

In my case, if possible, hope to show the more detailed example. This example seems that don't contain the ddrc dt node (imx8m-ddrc.c).

OK, I'll elaborate explanation on noc binding.

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +24 -0

+    #include <dt-bindings/clock/imx8mq-clock.h>
+    #include <dt-bindings/interconnect/imx8mq.h>
+    noc: interconnect@32700000 {
+            compatible = "fsl,imx8mq-noc", "fsl,imx8m-noc";
+            reg = <0x32700000 0x100000>;
+            clocks = <&clk IMX8MQ_CLK_NOC>;
+            #interconnect-cells = <1>;
+            fsl,scalable-node-ids = <IMX8MQ_ICN_NOC>,
+                                    <IMX8MQ_ICS_DRAM>;
+            fsl,scalable-nodes = <&noc>,
+                                 <&ddrc>;
+            operating-points-v2 = <&noc_opp_table>;
+
+            noc_opp_table: opp-table {
+                    compatible = "operating-points-v2";
+
+                    opp-133M {
+                            opp-hz = /bits/ 64 <133333333>;
+                    };
+                    opp-800M {
+                            opp-hz = /bits/ 64 <800000000>;
+                    };
+            };
+    };

Quoted 85 lines · Leonard Crestez

Quoted 84 lines · Chanwoo Choi

Hi,

Quoted 68 lines · Chanwoo Choi

Quoted 67 lines · Leonard Crestez

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.

Main NOC node acts as interconnect provider if #interconnect-cells is present.

Multiple interconnects can be present, each with their own OPP table.

Signed-off-by Leonard Crestez <[email protected]>

---
   .../bindings/interconnect/fsl,imx8m-noc.yaml  | 104 ++++++++++++++++++
   1 file changed, 104 insertions(+)

create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +46 -0

new file mode 100644
index 000000000000..5cd94185fec3
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3D8570eb5a-d8a45732-85716015-0cc47a3356b2-92a5b92cc514d07e%26u%3Dhttps%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Fprotect2.fireeye.com%252Furl%253Fk%253D0c13f3e0-51df3f45-0c1278af-0cc47a30d446-77e809543b673ffd%2526u%253Dhttp%253A%252F%252Fdevicetree.org%252Fschemas%252Finterconnect%252Ffsl%252Cimx8m-noc.yaml%2523%26amp%3Bdata%3D02%257C01%257Cleonard.crestez%2540nxp.com%257C2d1f1868afa140702a6b08d781d6ab68%257C686ea1d3bc2b4c6fa92cd99c5c301635%257C0%257C0%257C637120631307418544%26amp%3Bsdata%3DH2q5nQlKYyLIivkBYUTaRD1Nu3WcnphPJny3k%252BK%252BGFE%253D%26amp%3Breserved%3D0&data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&sdata=HYMJJHWyiKRhf7GDjKoOwjDpcZuYqlFlmRrDZnIRx5w%3D&reserved=0
+$schema: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3Df7cec483-aa1a78eb-f7cf4fcc-0cc47a3356b2-4154a3c43886f5ed%26u%3Dhttps%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Fprotect2.fireeye.com%252Furl%253Fk%253D87c672dc-da0abe79-87c7f993-0cc47a30d446-414d3b4d0127419a%2526u%253Dhttp%253A%252F%252Fdevicetree.org%252Fmeta-schemas%252Fcore.yaml%2523%26amp%3Bdata%3D02%257C01%257Cleonard.crestez%2540nxp.com%257C2d1f1868afa140702a6b08d781d6ab68%257C686ea1d3bc2b4c6fa92cd99c5c301635%257C0%257C0%257C637120631307418544%26amp%3Bsdata%3DT6PgQ1DWI4OLOx3gifRRt%252FNImdVrgDUoswZ%252FNKw3oR8%253D%26amp%3Breserved%3D0&data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&sdata=fIbrUUOUtZ5nt%2FH1tm3dzaI1J%2FGn5Gc54ms93HnBnQg%3D&reserved=0
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <[email protected]>
+
+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 (non-secure) 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:
+    oneOf:
+      - items:
+        - enum:
+          - fsl,imx8mn-nic
+          - fsl,imx8mm-nic
+          - fsl,imx8mq-nic
+        - const: fsl,imx8m-nic
+      - items:
+        - enum:
+          - fsl,imx8mn-noc
+          - fsl,imx8mm-noc
+          - fsl,imx8mq-noc
+        - const: fsl,imx8m-noc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  operating-points-v2: true

Quoted 8 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 52

Quoted 6 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +6 -0

+  opp-table: true
+
+  devfreq:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description:
+      Phandle to another devfreq device to match OPPs with by using the

Better to use 'parent' instead of 'another' word for improving the understanding.

I think that 'devfreq' is not proper way to get the parent node
in devicetree. Because 'devfreq' name is linuxium. The property name didn't indicate the any h/w device. So, I'll make 'devfreq' property deprecated.

Quoted 8 lines

Quoted 5 lines

So, you better to make the specific property for this device driver like as following: and use devfreq_get_devfreq_by_node() function which is developed by you in order to get the devfreq instance node.

	fsl,parent-device = <&parent devfreq device>;

This is only a "parent" in the sense that it's assigned to devfreq_passive.data.parent. The "devfreq" name is indeed too generic.

I thought that 'devfreq' property name is generic. But, it's not proper for DT binding because DT file show the h/w and the relation of h/w. 'devfreq' property name has not meant h/w.

So that devfreq core doesn't force to use the specific property name to get the devfreq parent instance on DT. Just, devfreq core will provide devfreq_get_devfreq_by_node() function.

I developed it on devfre-testing branch[2]. Before I'm sending the these patches, you can check them.

[1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fchanwoo%2Flinux.git%2Fcommit%2F%3Fh%3Ddevfreq-testing%26id%3Df3678b4e6b75dccfe8bb87d005da2d68c70fdeab&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&amp;sdata=R21Tv1QgofBvMYb2VaxFjKSerwQ3tl8kakcYRyALmgM%3D&amp;reserved=0
[2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fchanwoo%2Flinux.git%2Flog%2F%3Fh%3Ddevfreq-testing&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&amp;sdata=uH0d9LvrbCHgZJdrPJNJ8c8w45J7x1QyM7t5I3j%2BpSw%3D&amp;reserved=0

Quoted 3 lines

The DDRC can measure bandwith usage and I want to use the passive governor to make the main NOC match OPPs.

which one use the passive governor? And which one is the parent devfreq device for devfreq device using passive governor?

In my case, it is difficult to catch the relationship between devices. I'd like you to explain the detailed relationship on binding document for user.

Quoted 1 line

But at the bus level DDRC only has AXI and APB slave ports.

'DDRC' indicates the 'drivers/devfreq/imx8m-ddrc.c?

Quoted 1 line

Quoted 2 lines

Buses on imx don't have a parent/child relationship; in fact there are even a few cycles.

You mentioned that 'imx don't have a parent/child relationship', Why do you use 'passive' governor? It is difficult to understand the hierarchy of imx.

Quoted 35 lines

Quoted 35 lines

[1] [PATCH RFC v5 04/10] PM / devfreq: Add devfreq_get_devfreq_by_node

Quoted 33 lines

Quoted 19 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +18 -0

+      passive governor.
+
+  '#interconnect-cells':
+    description:
+      If specified then also act as an interconnect provider. Should only be
+      set once per soc on main noc.
+    const: 1
+
+  fsl,scalable-node-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of node ids for scalable nodes. Uses same numeric identifier
+      namespace as the consumer "interconnects" binding.
+
+  fsl,scalable-nodes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Array of phandles to scalable nodes. Must be of same length as

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 80

Quoted 10 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +10 -0

+      fsl,scalable-node-ids.
+
+required:
+  - compatible
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |

Is it enough example to understand the relation between imx8m-ddrc.c, imx-devfreq.c and imx interconnect driver?

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

In my case, if possible, hope to show the more detailed example. This example seems that don't contain the ddrc dt node (imx8m-ddrc.c).

OK, I'll elaborate explanation on noc binding.

Thanks. If possible, you better to add almost example cases.

Quoted 28 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +24 -0

+    #include <dt-bindings/clock/imx8mq-clock.h>
+    #include <dt-bindings/interconnect/imx8mq.h>
+    noc: interconnect@32700000 {
+            compatible = "fsl,imx8mq-noc", "fsl,imx8m-noc";
+            reg = <0x32700000 0x100000>;
+            clocks = <&clk IMX8MQ_CLK_NOC>;
+            #interconnect-cells = <1>;
+            fsl,scalable-node-ids = <IMX8MQ_ICN_NOC>,
+                                    <IMX8MQ_ICS_DRAM>;
+            fsl,scalable-nodes = <&noc>,
+                                 <&ddrc>;
+            operating-points-v2 = <&noc_opp_table>;
+
+            noc_opp_table: opp-table {
+                    compatible = "operating-points-v2";
+
+                    opp-133M {
+                            opp-hz = /bits/ 64 <133333333>;
+                    };
+                    opp-800M {
+                            opp-hz = /bits/ 64 <800000000>;
+                    };
+            };
+    };

Quoted 129 lines · Leonard Crestez

Quoted 85 lines · Chanwoo Choi

Quoted 84 lines

Hi,

Quoted 68 lines · Chanwoo Choi

Quoted 67 lines · Leonard Crestez

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.

Main NOC node acts as interconnect provider if #interconnect-cells is present.

Multiple interconnects can be present, each with their own OPP table.

Signed-off-by Leonard Crestez <[email protected]>

---
   .../bindings/interconnect/fsl,imx8m-noc.yaml  | 104 ++++++++++++++++++
   1 file changed, 104 insertions(+)

create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +46 -0

new file mode 100644
index 000000000000..5cd94185fec3
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3D8570eb5a-d8a45732-85716015-0cc47a3356b2-92a5b92cc514d07e%26u%3Dhttps%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Fprotect2.fireeye.com%252Furl%253Fk%253D0c13f3e0-51df3f45-0c1278af-0cc47a30d446-77e809543b673ffd%2526u%253Dhttp%253A%252F%252Fdevicetree.org%252Fschemas%252Finterconnect%252Ffsl%252Cimx8m-noc.yaml%2523%26amp%3Bdata%3D02%257C01%257Cleonard.crestez%2540nxp.com%257C2d1f1868afa140702a6b08d781d6ab68%257C686ea1d3bc2b4c6fa92cd99c5c301635%257C0%257C0%257C637120631307418544%26amp%3Bsdata%3DH2q5nQlKYyLIivkBYUTaRD1Nu3WcnphPJny3k%252BK%252BGFE%253D%26amp%3Breserved%3D0&data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&sdata=HYMJJHWyiKRhf7GDjKoOwjDpcZuYqlFlmRrDZnIRx5w%3D&reserved=0
+$schema: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3Df7cec483-aa1a78eb-f7cf4fcc-0cc47a3356b2-4154a3c43886f5ed%26u%3Dhttps%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Fprotect2.fireeye.com%252Furl%253Fk%253D87c672dc-da0abe79-87c7f993-0cc47a30d446-414d3b4d0127419a%2526u%253Dhttp%253A%252F%252Fdevicetree.org%252Fmeta-schemas%252Fcore.yaml%2523%26amp%3Bdata%3D02%257C01%257Cleonard.crestez%2540nxp.com%257C2d1f1868afa140702a6b08d781d6ab68%257C686ea1d3bc2b4c6fa92cd99c5c301635%257C0%257C0%257C637120631307418544%26amp%3Bsdata%3DT6PgQ1DWI4OLOx3gifRRt%252FNImdVrgDUoswZ%252FNKw3oR8%253D%26amp%3Breserved%3D0&data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&sdata=fIbrUUOUtZ5nt%2FH1tm3dzaI1J%2FGn5Gc54ms93HnBnQg%3D&reserved=0
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <[email protected]>
+
+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 (non-secure) 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:
+    oneOf:
+      - items:
+        - enum:
+          - fsl,imx8mn-nic
+          - fsl,imx8mm-nic
+          - fsl,imx8mq-nic
+        - const: fsl,imx8m-nic
+      - items:
+        - enum:
+          - fsl,imx8mn-noc
+          - fsl,imx8mm-noc
+          - fsl,imx8mq-noc
+        - const: fsl,imx8m-noc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  operating-points-v2: true

Quoted 8 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 52

Quoted 6 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +6 -0

+  opp-table: true
+
+  devfreq:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description:
+      Phandle to another devfreq device to match OPPs with by using the

Better to use 'parent' instead of 'another' word for improving the understanding.

I think that 'devfreq' is not proper way to get the parent node
in devicetree. Because 'devfreq' name is linuxium. The property name didn't indicate the any h/w device. So, I'll make 'devfreq' property deprecated.

Quoted 8 lines

Quoted 5 lines

So, you better to make the specific property for this device driver like as following: and use devfreq_get_devfreq_by_node() function which is developed by you in order to get the devfreq instance node.

    fsl,parent-device = <&parent devfreq device>;

This is only a "parent" in the sense that it's assigned to devfreq_passive.data.parent. The "devfreq" name is indeed too generic.

I thought that 'devfreq' property name is generic. But, it's not proper for DT binding because DT file show the h/w and the relation of h/w. 'devfreq' property name has not meant h/w.

So that devfreq core doesn't force to use the specific property name to get the devfreq parent instance on DT. Just, devfreq core will provide devfreq_get_devfreq_by_node() function.

I developed it on devfre-testing branch[2]. Before I'm sending the these patches, you can check them.

[1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fchanwoo%2Flinux.git%2Fcommit%2F%3Fh%3Ddevfreq-testing%26id%3Df3678b4e6b75dccfe8bb87d005da2d68c70fdeab&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&amp;sdata=R21Tv1QgofBvMYb2VaxFjKSerwQ3tl8kakcYRyALmgM%3D&amp;reserved=0
[2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fchanwoo%2Flinux.git%2Flog%2F%3Fh%3Ddevfreq-testing&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&amp;sdata=uH0d9LvrbCHgZJdrPJNJ8c8w45J7x1QyM7t5I3j%2BpSw%3D&amp;reserved=0

Quoted 3 lines

The DDRC can measure bandwith usage and I want to use the passive governor to make the main NOC match OPPs.

which one use the passive governor? And which one is the parent devfreq device for devfreq device using passive governor?

In my case, it is difficult to catch the relationship between devices. I'd like you to explain the detailed relationship on binding document for user.

Quoted 1 line

But at the bus level DDRC only has AXI and APB slave ports.

'DDRC' indicates the 'drivers/devfreq/imx8m-ddrc.c?

Quoted 3 lines

Buses on imx don't have a parent/child relationship; in fact there are even a few cycles.

Quoted 3 lines

You mentioned that 'imx don't have a parent/child relationship', Why do you use 'passive' governor? It is difficult to understand the hierarchy of imx.

The imx8m has a main NOC in the center between the CPU and the DDRC (dram controller) with many other peripheral buses around the NOC.

Here's /sys/kernel/debug/interconnect/interconnect_graph on imx8mm:
https://gist.github.com/cdleonard/84d103dafc9131fcb8ca9a494822a131#file-imx8mm-svg

A lot of stuff is omitted, it mostly just includes high-performance bus masters.

DDRC has a performance monitor attached which can measure current bandwith and feed it to an ondemand governor. I want to use passive governor on the NOC so that it matches frequencies with DDRC and scales

proportionally, otherwise if NOC is at low frequency then dynamically scaling up the DDRC might be ineffective.

Perhaps you could explain how parent/child relationships work on exynos?

Quoted 74 lines

Quoted 36 lines

Quoted 36 lines

[1] [PATCH RFC v5 04/10] PM / devfreq: Add devfreq_get_devfreq_by_node

Quoted 33 lines

Quoted 19 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +18 -0

+      passive governor.
+
+  '#interconnect-cells':
+    description:
+      If specified then also act as an interconnect provider. Should only be
+      set once per soc on main noc.
+    const: 1
+
+  fsl,scalable-node-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of node ids for scalable nodes. Uses same numeric identifier
+      namespace as the consumer "interconnects" binding.
+
+  fsl,scalable-nodes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Array of phandles to scalable nodes. Must be of same length as

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 80

Quoted 10 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +10 -0

+      fsl,scalable-node-ids.
+
+required:
+  - compatible
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |

Is it enough example to understand the relation between imx8m-ddrc.c, imx-devfreq.c and imx interconnect driver?

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

In my case, if possible, hope to show the more detailed example. This example seems that don't contain the ddrc dt node (imx8m-ddrc.c).

OK, I'll elaborate explanation on noc binding.

Thanks. If possible, you better to add almost example cases.

Quoted 28 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +24 -0

+    #include <dt-bindings/clock/imx8mq-clock.h>
+    #include <dt-bindings/interconnect/imx8mq.h>
+    noc: interconnect@32700000 {
+            compatible = "fsl,imx8mq-noc", "fsl,imx8m-noc";
+            reg = <0x32700000 0x100000>;
+            clocks = <&clk IMX8MQ_CLK_NOC>;
+            #interconnect-cells = <1>;
+            fsl,scalable-node-ids = <IMX8MQ_ICN_NOC>,
+                                    <IMX8MQ_ICS_DRAM>;
+            fsl,scalable-nodes = <&noc>,
+                                 <&ddrc>;
+            operating-points-v2 = <&noc_opp_table>;
+
+            noc_opp_table: opp-table {
+                    compatible = "operating-points-v2";
+
+                    opp-133M {
+                            opp-hz = /bits/ 64 <133333333>;
+                    };
+                    opp-800M {
+                            opp-hz = /bits/ 64 <800000000>;
+                    };
+            };
+    };

2019년 12월 19일 (목) 오후 11:33, Leonard Crestez <[email protected]>님이 작성:

Quoted 131 lines · Chanwoo Choi

Quoted 129 lines · Leonard Crestez

Quoted 85 lines · Chanwoo Choi

Quoted 84 lines

Hi,

Quoted 68 lines · Chanwoo Choi

Quoted 67 lines · Leonard Crestez

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.

Main NOC node acts as interconnect provider if #interconnect-cells is present.

Multiple interconnects can be present, each with their own OPP table.

Signed-off-by Leonard Crestez <[email protected]>

---
    .../bindings/interconnect/fsl,imx8m-noc.yaml  | 104 ++++++++++++++++++
    1 file changed, 104 insertions(+)

create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +46 -0

new file mode 100644
index 000000000000..5cd94185fec3
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3D8570eb5a-d8a45732-85716015-0cc47a3356b2-92a5b92cc514d07e%26u%3Dhttps%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Fprotect2.fireeye.com%252Furl%253Fk%253D0c13f3e0-51df3f45-0c1278af-0cc47a30d446-77e809543b673ffd%2526u%253Dhttp%253A%252F%252Fdevicetree.org%252Fschemas%252Finterconnect%252Ffsl%252Cimx8m-noc.yaml%2523%26amp%3Bdata%3D02%257C01%257Cleonard.crestez%2540nxp.com%257C2d1f1868afa140702a6b08d781d6ab68%257C686ea1d3bc2b4c6fa92cd99c5c301635%257C0%257C0%257C637120631307418544%26amp%3Bsdata%3DH2q5nQlKYyLIivkBYUTaRD1Nu3WcnphPJny3k%252BK%252BGFE%253D%26amp%3Breserved%3D0&data=02%7C01%7Cleonard.crestez%40nxp.com%7C40406bfd0f134eeddb3208d7849c0968%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637123678022512623&sdata=IYSYlNtA0fYKLkTr4OjyHkKa6FAi2m8UFr4nXZ9pr7s%3D&reserved=0
+$schema: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Furl%3Fk%3Df7cec483-aa1a78eb-f7cf4fcc-0cc47a3356b2-4154a3c43886f5ed%26u%3Dhttps%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Fprotect2.fireeye.com%252Furl%253Fk%253D87c672dc-da0abe79-87c7f993-0cc47a30d446-414d3b4d0127419a%2526u%253Dhttp%253A%252F%252Fdevicetree.org%252Fmeta-schemas%252Fcore.yaml%2523%26amp%3Bdata%3D02%257C01%257Cleonard.crestez%2540nxp.com%257C2d1f1868afa140702a6b08d781d6ab68%257C686ea1d3bc2b4c6fa92cd99c5c301635%257C0%257C0%257C637120631307418544%26amp%3Bsdata%3DT6PgQ1DWI4OLOx3gifRRt%252FNImdVrgDUoswZ%252FNKw3oR8%253D%26amp%3Breserved%3D0&data=02%7C01%7Cleonard.crestez%40nxp.com%7C40406bfd0f134eeddb3208d7849c0968%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637123678022512623&sdata=nrbPGpgBci5DDm6qRROeFa78hARR9UCXeP59TnPtCuI%3D&reserved=0
+
+title: Generic i.MX bus frequency device
+
+maintainers:
+  - Leonard Crestez <[email protected]>
+
+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 (non-secure) 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:
+    oneOf:
+      - items:
+        - enum:
+          - fsl,imx8mn-nic
+          - fsl,imx8mm-nic
+          - fsl,imx8mq-nic
+        - const: fsl,imx8m-nic
+      - items:
+        - enum:
+          - fsl,imx8mn-noc
+          - fsl,imx8mm-noc
+          - fsl,imx8mq-noc
+        - const: fsl,imx8m-noc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  operating-points-v2: true

Quoted 8 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 52

Quoted 6 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +6 -0

+  opp-table: true
+
+  devfreq:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description:
+      Phandle to another devfreq device to match OPPs with by using the

Better to use 'parent' instead of 'another' word for improving the understanding.

I think that 'devfreq' is not proper way to get the parent node
in devicetree. Because 'devfreq' name is linuxium. The property name didn't indicate the any h/w device. So, I'll make 'devfreq' property deprecated.

Quoted 8 lines

Quoted 5 lines

So, you better to make the specific property for this device driver like as following: and use devfreq_get_devfreq_by_node() function which is developed by you in order to get the devfreq instance node.

     fsl,parent-device = <&parent devfreq device>;

This is only a "parent" in the sense that it's assigned to devfreq_passive.data.parent. The "devfreq" name is indeed too generic.

I thought that 'devfreq' property name is generic. But, it's not proper for DT binding because DT file show the h/w and the relation of h/w. 'devfreq' property name has not meant h/w.

So that devfreq core doesn't force to use the specific property name to get the devfreq parent instance on DT. Just, devfreq core will provide devfreq_get_devfreq_by_node() function.

I developed it on devfre-testing branch[2]. Before I'm sending the these patches, you can check them.

[1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fchanwoo%2Flinux.git%2Fcommit%2F%3Fh%3Ddevfreq-testing%26id%3Df3678b4e6b75dccfe8bb87d005da2d68c70fdeab&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C40406bfd0f134eeddb3208d7849c0968%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637123678022512623&amp;sdata=AhMMx8MZB7HIvYaChWFdSu9JMFGPHyz%2B8W%2Fk7pevofY%3D&amp;reserved=0
[2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fchanwoo%2Flinux.git%2Flog%2F%3Fh%3Ddevfreq-testing&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C40406bfd0f134eeddb3208d7849c0968%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637123678022522615&amp;sdata=29FP1He7PT%2B2NdYeyBIjAk2reqCeOmM5aJgnXotFQzI%3D&amp;reserved=0

Quoted 3 lines

The DDRC can measure bandwith usage and I want to use the passive governor to make the main NOC match OPPs.

which one use the passive governor? And which one is the parent devfreq device for devfreq device using passive governor?

In my case, it is difficult to catch the relationship between devices. I'd like you to explain the detailed relationship on binding document for user.

Quoted 1 line

But at the bus level DDRC only has AXI and APB slave ports.

'DDRC' indicates the 'drivers/devfreq/imx8m-ddrc.c?

Quoted 3 lines

Buses on imx don't have a parent/child relationship; in fact there are even a few cycles.

Quoted 6 lines

Quoted 3 lines

You mentioned that 'imx don't have a parent/child relationship', Why do you use 'passive' governor? It is difficult to understand the hierarchy of imx.

The imx8m has a main NOC in the center between the CPU and the DDRC (dram controller) with many other peripheral buses around the NOC.

Actually, if this dt binding document contains the relationship between ddrc(imx8m-ddrc.c)
, imx-bus.c(bus) and interconnect node, it is more easy to understand the hierarchy
of bus/ddrc. In my case, it is difficult because the binding document doesn't include the enough example. But, I'll expect them as you mentioned on your reply.

Quoted 3 lines

Here's /sys/kernel/debug/interconnect/interconnect_graph on imx8mm:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgist.github.com%2Fcdleonard%2F84d103dafc9131fcb8ca9a494822a131%23file-imx8mm-svg&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C40406bfd0f134eeddb3208d7849c0968%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637123678022522615&amp;sdata=anAPK7CpjdRtE7S%2FRPo0TY08OgC2EWmuHk58nBfZ1og%3D&amp;reserved=0

It is the interconnect node graph. I hope to know the relationship when bus(imx-bus.c)
uses the 'passive governor' and which is the

Quoted 7 lines

A lot of stuff is omitted, it mostly just includes high-performance bus masters.

DDRC has a performance monitor attached which can measure current bandwith and feed it to an ondemand governor. I want to use passive governor on the NOC so that it matches frequencies with DDRC and scales

I have the following questions. If you explain the more detailed descritpion and add multiple dt bidning example, I'll expect that I can understand the following my questions.
- Which one will use the 'passive governor'?
- DDRC is parent devfreq device for imx bus(imx-bus.c) using passive governor?
- Why imx bus(imx-bus.c) use the userspace governor?
- Which the relationship between imx bus(imx-bus.c) using userspace governor and imx bus(imx-bus.c) using passive governor?

Quoted 4 lines

proportionally, otherwise if NOC is at low frequency then dynamically scaling up the DDRC might be ineffective.

Perhaps you could explain how parent/child relationships work on exynos?

Right. So, I wrote[1] why exynos-bus.c have to use the passive governor and how to make the hierarchy/relationship between exynos-bus(parent devfreq device
using devfreq governor except for passive governor)
and exynos-bus (child devfreq device using only passive governor).

[1] Documentation/devicetree/bindings/devfreq/exynos-bus.txt

Quoted 43 lines

Quoted 43 lines

Quoted 36 lines

Quoted 36 lines

[1] [PATCH RFC v5 04/10] PM / devfreq: Add devfreq_get_devfreq_by_node

Quoted 33 lines

Quoted 19 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +18 -0

+      passive governor.
+
+  '#interconnect-cells':
+    description:
+      If specified then also act as an interconnect provider. Should only be
+      set once per soc on main noc.
+    const: 1
+
+  fsl,scalable-node-ids:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      Array of node ids for scalable nodes. Uses same numeric identifier
+      namespace as the consumer "interconnects" binding.
+
+  fsl,scalable-nodes:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Array of phandles to scalable nodes. Must be of same length as

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml line 80

Quoted 10 lines

Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml new file +10 -0

+      fsl,scalable-node-ids.
+
+required:
+  - compatible
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |

Is it enough example to understand the relation between imx8m-ddrc.c, imx-devfreq.c and imx interconnect driver?

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

In my case, if possible, hope to show the more detailed example. This example seems that don't contain the ddrc dt node (imx8m-ddrc.c).

OK, I'll elaborate explanation on noc binding.

Thanks. If possible, you better to add almost example cases.

Add initial support for dynamic frequency switching on pieces of the imx interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Signed-off-by Leonard Crestez <[email protected]>

---
 drivers/devfreq/Kconfig       |   9 ++
 drivers/devfreq/Makefile      |   1 +
 drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/devfreq/imx-devfreq.c

drivers/devfreq/Kconfig +0 -0

index 923a6132e741..fef5ce831e90 100644
@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
 	select DEVFREQ_GOV_USERSPACE
 	help
 	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It 

allows

drivers/devfreq/Kconfig +9 -0

 	  adjusting DRAM frequency.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
 config ARM_TEGRA_DEVFREQ
 	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
 	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
 		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
 		ARCH_TEGRA_210_SOC || \

drivers/devfreq/Makefile +0 -0

index 3eb4d5e6635c..61d0edee16f7 100644
@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= 

governor_userspace.o
obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o

# DEVFREQ Drivers
obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o

drivers/devfreq/Makefile +1 -0

+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
 obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
 # DEVFREQ Event Drivers

drivers/devfreq/imx-devfreq.c new file +42 -0

new file mode 100644
index 000000000000..620b344e87aa
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)
+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long 

*freq)

drivers/devfreq/imx-devfreq.c new file +108 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)
+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static void imx_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;
+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));
+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}
+
+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+
+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),
+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
+MODULE_AUTHOR("Leonard Crestez <[email protected]>");
+MODULE_LICENSE("GPL v2");

Hi Leonard,

Quoted 4 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx
interconnect fabric.

Quoted 3 lines

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Is this working with mainline ATF or does it still need to be used with your modified ATF code ?

Quoted 7 lines · Angus Ainslie

Hi Leonard,

Quoted 4 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx
interconnect fabric.

Quoted 6 lines

Quoted 3 lines

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Is this working with mainline ATF or does it still need to be used with your modified ATF code ?

This series doesn't perform SMC calls, that's done by the imx8m-ddrc
driver: https://patchwork.kernel.org/cover/11244283/

This particular patch allows switching NOC frequency but that's just clk_set_rate.

DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200 patches) otherwise you will get probe failures. Source for imx atf is published here: https://source.codeaurora.org/external/imx/imx-atf/

For your particular 8mq B0 case slightly different setpoints are used and the fix is not in any public release yet so you need this:

https://github.com/cdleonard/arm-trusted-firmware/commits/imx_2.0.y_busfreq

Is "mainline ATF" an important criteria for Purism?

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 7 lines · Angus Ainslie

Hi Leonard,

Quoted 4 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx
interconnect fabric.

Quoted 6 lines

Quoted 3 lines

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Is this working with mainline ATF or does it still need to be used with your modified ATF code ?

This series doesn't perform SMC calls, that's done by the imx8m-ddrc
driver: https://patchwork.kernel.org/cover/11244283/

This particular patch allows switching NOC frequency but that's just clk_set_rate.

DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200 patches) otherwise you will get probe failures. Source for imx atf is published here: https://source.codeaurora.org/external/imx/imx-atf/

For your particular 8mq B0 case slightly different setpoints are used and the fix is not in any public release yet so you need this:

https://github.com/cdleonard/arm-trusted-firmware/commits/imx_2.0.y_busfreq

Is "mainline ATF" an important criteria for Purism?

--
Regards,
Leonard

Hi Leonard,

Quoted 21 lines · Leonard Crestez

Quoted 7 lines · Angus Ainslie

Hi Leonard,

Quoted 4 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx
interconnect fabric.

Quoted 6 lines

Quoted 3 lines

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Is this working with mainline ATF or does it still need to be used with your modified ATF code ?

This series doesn't perform SMC calls, that's done by the imx8m-ddrc
driver: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11244283%2F&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732915017&amp;sdata=ER10Ts4hk2Ft7%2FWBZ7r8lyFkB6un1VRwk0rSvRMm3ew%3D&amp;reserved=0

This particular patch allows switching NOC frequency but that's just clk_set_rate.

Quoted 3 lines

DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200
patches) otherwise you will get probe failures. Source for imx atf is
published here: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Fimx-atf%2F&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732915017&amp;sdata=KcdzTQaW4xxualeRUU%2B9LjBeq99wUtzDBxrHWLVbkDo%3D&amp;reserved=0

Ok I was under the impression that the imx_2.0.y_busfreq branch below was based on this. Shouldn't those patches be added to the imx ATF ?

Quoted 5 lines

For your particular 8mq B0 case slightly different setpoints are used and the fix is not in any public release yet so you need this:

https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcdleonard%2Farm-trusted-firmware%2Fcommits%2Fimx_2.0.y_busfreq&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732925012&amp;sdata=Ape5T7xfiR0yfSuO1Lv9OQhmK5p3f0ROWpzJiAMw1VA%3D&amp;reserved=0

We also have 2n14w ( is that B1 ? ) imx8mq's that we are working with.

Quoted 1 line

Is "mainline ATF" an important criteria for Purism?

Yes we intend to bring all of our patches to mainline and were hoping that NXP would be doing the same. Shouldn't a mainline kernel run on a mainline ATF ?

Quoted 6 lines · Angus Ainslie

Hi Leonard,

Quoted 2 lines · Leonard Crestez

On 20.11.2019 16:08, Angus Ainslie wrote:
Is "mainline ATF" an important criteria for Purism?

Quoted 3 lines

Yes we intend to bring all of our patches to mainline and were hoping that NXP would be doing the same. Shouldn't a mainline kernel run on a mainline ATF ?

You can still use mainline ATF (tested right now) but the imx8m-ddrc driver won't probe.

The ability to mix and match different branches of firmware and kernel is very useful for testing. There might be slight incompatibilities but in theory if a feature depends on both firmware and kernel support then it should gracefully degrade rather than crash or hang.

ATF support for this feature will be mainlined eventually, I picked the linux side first because review is more challenging and changes are much larger relative to what we have in our internal tree.

--
Regards,
Leonard

Quoted 7 lines · Leonard Crestez

Quoted 6 lines · Angus Ainslie

Hi Leonard,

Quoted 2 lines · Leonard Crestez

On 20.11.2019 16:08, Angus Ainslie wrote:
Is "mainline ATF" an important criteria for Purism?

Quoted 6 lines

Quoted 3 lines

Yes we intend to bring all of our patches to mainline and were hoping that NXP would be doing the same. Shouldn't a mainline kernel run on a mainline ATF ?

You can still use mainline ATF (tested right now) but the imx8m-ddrc driver won't probe.

Sorry I was talking about the DDR frequency scaling specifically. >

Quoted 4 lines

The ability to mix and match different branches of firmware and kernel is very useful for testing. There might be slight incompatibilities but in theory if a feature depends on both firmware and kernel support then it should gracefully degrade rather than crash or hang.

I saw the check you put in for the correct ATF version and that's very helpful thanks.

Quoted 3 lines

ATF support for this feature will be mainlined eventually, I picked the linux side first because review is more challenging and changes are much larger relative to what we have in our internal tree.

Do you have a patch against mainline ATF that we can test this feature with ?

Quoted 28 lines · Angus Ainslie

Quoted 7 lines · Leonard Crestez

Quoted 6 lines · Angus Ainslie

Hi Leonard,

Quoted 2 lines · Leonard Crestez

On 20.11.2019 16:08, Angus Ainslie wrote:
Is "mainline ATF" an important criteria for Purism?

Quoted 6 lines

Quoted 3 lines

Yes we intend to bring all of our patches to mainline and were hoping that NXP would be doing the same. Shouldn't a mainline kernel run on a mainline ATF ?

You can still use mainline ATF (tested right now) but the imx8m-ddrc driver won't probe.

Sorry I was talking about the DDR frequency scaling specifically. >

Quoted 4 lines

The ability to mix and match different branches of firmware and kernel is very useful for testing. There might be slight incompatibilities but in theory if a feature depends on both firmware and kernel support then it should gracefully degrade rather than crash or hang.

I saw the check you put in for the correct ATF version and that's very helpful thanks.

Quoted 3 lines

ATF support for this feature will be mainlined eventually, I picked the linux side first because review is more challenging and changes are much larger relative to what we have in our internal tree.

Quoted 2 lines

Do you have a patch against mainline ATF that we can test this feature with ?

Not right now, and imx atf is based on a slightly older version so some porting effort might be required.

Quoted 29 lines · Leonard Crestez

Quoted 28 lines · Angus Ainslie

Quoted 7 lines · Leonard Crestez

Quoted 6 lines · Angus Ainslie

Hi Leonard,

Quoted 2 lines · Leonard Crestez

On 20.11.2019 16:08, Angus Ainslie wrote:
Is "mainline ATF" an important criteria for Purism?

Quoted 6 lines

Quoted 3 lines

Yes we intend to bring all of our patches to mainline and were hoping that NXP would be doing the same. Shouldn't a mainline kernel run on a mainline ATF ?

You can still use mainline ATF (tested right now) but the imx8m-ddrc driver won't probe.

Sorry I was talking about the DDR frequency scaling specifically. >

Quoted 4 lines

The ability to mix and match different branches of firmware and kernel is very useful for testing. There might be slight incompatibilities but in theory if a feature depends on both firmware and kernel support then it should gracefully degrade rather than crash or hang.

I saw the check you put in for the correct ATF version and that's very helpful thanks.

Quoted 3 lines

ATF support for this feature will be mainlined eventually, I picked the linux side first because review is more challenging and changes are much larger relative to what we have in our internal tree.

Quoted 6 lines

Quoted 2 lines

Do you have a patch against mainline ATF that we can test this feature with ?

Not right now, and imx atf is based on a slightly older version so some porting effort might be required.

Hi Leonard,

Have you, by chance, looked at the mainline ATF side for your devfreq work in the meantime? I'd happily test. (btw. the linux driver fails "nicely" there.)

Add initial support for dynamic frequency switching on pieces of the imx interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Signed-off-by Leonard Crestez <[email protected]>

---
 drivers/devfreq/Kconfig       |   9 ++
 drivers/devfreq/Makefile      |   1 +
 drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/devfreq/imx-devfreq.c

drivers/devfreq/Kconfig +9 -0

index 923a6132e741..fef5ce831e90 100644
@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
 	select DEVFREQ_GOV_USERSPACE
 	help
 	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
 	  adjusting DRAM frequency.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
 config ARM_TEGRA_DEVFREQ
 	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
 	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
 		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
 		ARCH_TEGRA_210_SOC || \

drivers/devfreq/Makefile +1 -0

index 3eb4d5e6635c..61d0edee16f7 100644
@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
 obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
 # DEVFREQ Drivers
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
 obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
 obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
 # DEVFREQ Event Drivers

drivers/devfreq/imx-devfreq.c new file +13 -0

new file mode 100644
index 000000000000..620b344e87aa
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>

drivers/devfreq/imx-devfreq.c new file +19 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)

drivers/devfreq/imx-devfreq.c new file +9 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}

drivers/devfreq/imx-devfreq.c new file +25 -0

+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));

drivers/devfreq/imx-devfreq.c new file +23 -0

+
+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+

drivers/devfreq/imx-devfreq.c new file +2 -0

+MODULE_AUTHOR("Leonard Crestez <[email protected]>");
+MODULE_LICENSE("GPL v2");

Hi,

Quoted 73 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Signed-off-by Leonard Crestez <[email protected]>

---
 drivers/devfreq/Kconfig       |   9 ++
 drivers/devfreq/Makefile      |   1 +
 drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/devfreq/imx-devfreq.c

drivers/devfreq/Kconfig +9 -0

index 923a6132e741..fef5ce831e90 100644
@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
 	select DEVFREQ_GOV_USERSPACE
 	help
 	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
 	  adjusting DRAM frequency.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
 config ARM_TEGRA_DEVFREQ
 	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
 	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
 		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
 		ARCH_TEGRA_210_SOC || \

drivers/devfreq/Makefile +1 -0

index 3eb4d5e6635c..61d0edee16f7 100644
@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
 obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
 # DEVFREQ Drivers
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
 obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
 obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
 # DEVFREQ Event Drivers

drivers/devfreq/imx-devfreq.c new file +13 -0

new file mode 100644
index 000000000000..620b344e87aa
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>

drivers/devfreq/imx-devfreq.c line 23

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)

Don't use space for the indentation. Please use only tab.

Quoted 19 lines

drivers/devfreq/imx-devfreq.c new file +19 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)

drivers/devfreq/imx-devfreq.c line 52

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)

ditto. Please use tab for the indentation.

Quoted 9 lines

drivers/devfreq/imx-devfreq.c new file +9 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}

drivers/devfreq/imx-devfreq.c line 71

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+
+static void imx_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;

How about changing the variable name 'priv' to 'imx' or 'imx_data'? because it is not easy to catch the role of 'priv' from variable name.

drivers/devfreq/imx-devfreq.c line 73

Quoted 2 lines

drivers/devfreq/imx-devfreq.c new file +2 -0

+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;

How about changing the variable name 'govdata' to 'gov_data'?
- govdata -> gov_data

drivers/devfreq/imx-devfreq.c line 80

Quoted 7 lines

drivers/devfreq/imx-devfreq.c new file +7 -0

+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);

nitpick: because the clock-name is not mandatory. Don't need to specify the clock name to inform the role of clock of other developer/user?

For example, "ddr", "bus" and so on.

Quoted 26 lines

drivers/devfreq/imx-devfreq.c new file +25 -0

+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));

drivers/devfreq/imx-devfreq.c line 115

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}

You better to change the exception handling as following: It is more simple.

	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
		goto err;
	} else {
		ret = PTR_ERR(priv->passive_data.parent);
		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
		goto err;
	}

Quoted 23 lines

drivers/devfreq/imx-devfreq.c new file +23 -0

+
+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+

drivers/devfreq/imx-devfreq.c line 148

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),
+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");

If this driver is for bus frequency, you better to use 'bus' for the clock-name for the readability.

Quoted 3 lines

drivers/devfreq/imx-devfreq.c new file +2 -0

+MODULE_AUTHOR("Leonard Crestez <[email protected]>");
+MODULE_LICENSE("GPL v2");

Quoted 162 lines · Chanwoo Choi

Hi,

Quoted 73 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Signed-off-by Leonard Crestez <[email protected]>

---
 drivers/devfreq/Kconfig       |   9 ++
 drivers/devfreq/Makefile      |   1 +
 drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/devfreq/imx-devfreq.c

drivers/devfreq/Kconfig +9 -0

index 923a6132e741..fef5ce831e90 100644
@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
 	select DEVFREQ_GOV_USERSPACE
 	help
 	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
 	  adjusting DRAM frequency.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
 config ARM_TEGRA_DEVFREQ
 	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
 	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
 		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
 		ARCH_TEGRA_210_SOC || \

drivers/devfreq/Makefile +1 -0

index 3eb4d5e6635c..61d0edee16f7 100644
@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
 obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
 # DEVFREQ Drivers
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
 obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
 obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
 # DEVFREQ Event Drivers

drivers/devfreq/imx-devfreq.c new file +13 -0

new file mode 100644
index 000000000000..620b344e87aa
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>

drivers/devfreq/imx-devfreq.c line 23

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)

Don't use space for the indentation. Please use only tab.

Quoted 19 lines

drivers/devfreq/imx-devfreq.c new file +19 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)

drivers/devfreq/imx-devfreq.c line 52

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)

ditto. Please use tab for the indentation.

Quoted 9 lines

drivers/devfreq/imx-devfreq.c new file +9 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}

drivers/devfreq/imx-devfreq.c line 71

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+
+static void imx_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;

How about changing the variable name 'priv' to 'imx' or 'imx_data'? because it is not easy to catch the role of 'priv' from variable name.

drivers/devfreq/imx-devfreq.c line 73

Quoted 2 lines

drivers/devfreq/imx-devfreq.c new file +2 -0

+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;

How about changing the variable name 'govdata' to 'gov_data'?
- govdata -> gov_data

drivers/devfreq/imx-devfreq.c line 80

Quoted 7 lines

drivers/devfreq/imx-devfreq.c new file +7 -0

+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);

nitpick: because the clock-name is not mandatory. Don't need to specify the clock name to inform the role of clock of other developer/user?

Quoted 1 line

For example, "ddr", "bus" and so on.

And, this driver doesn't include the 'clk_prepare_enable'. how to enable the clock?

Quoted 90 lines

Quoted 26 lines

drivers/devfreq/imx-devfreq.c new file +25 -0

+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));

drivers/devfreq/imx-devfreq.c line 115

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}

You better to change the exception handling as following: It is more simple.

	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
		goto err;
	} else {
		ret = PTR_ERR(priv->passive_data.parent);
		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
		goto err;
	}

Quoted 23 lines

drivers/devfreq/imx-devfreq.c new file +23 -0

+
+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+

drivers/devfreq/imx-devfreq.c line 148

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),
+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");

If this driver is for bus frequency, you better to use 'bus' for the clock-name for the readability.

Quoted 3 lines

drivers/devfreq/imx-devfreq.c new file +2 -0

+MODULE_AUTHOR("Leonard Crestez <[email protected]>");
+MODULE_LICENSE("GPL v2");

Hi,

Also, I think that 'devfreq' word is not proper for device driver name. imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.

And, I have a question.
This driver adds the devfreq device with either passive governor or userspace governor.

As I understood, the devfreq device with passive governor will be operated with imx8m-ddrc.c driver.
But, when is operating with userspace governor?

I think that you better to add the explanation to description for two scenarios how to operate with interconnect provider on either passive governor or userspace governor usage case.

Quoted 81 lines · Chanwoo Choi

Quoted 80 lines

Hi,

Quoted 77 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Signed-off-by Leonard Crestez <[email protected]>

---
  drivers/devfreq/Kconfig       |   9 ++
  drivers/devfreq/Makefile      |   1 +
  drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
  3 files changed, 160 insertions(+)

create mode 100644 drivers/devfreq/imx-devfreq.c

drivers/devfreq/Kconfig +9 -0

index 923a6132e741..fef5ce831e90 100644
@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
  	select DEVFREQ_GOV_USERSPACE
  	help
  	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
  	  adjusting DRAM frequency.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
  config ARM_TEGRA_DEVFREQ
  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
  	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
  		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
  		ARCH_TEGRA_210_SOC || \

drivers/devfreq/Makefile +1 -0

index 3eb4d5e6635c..61d0edee16f7 100644
@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
  obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
  # DEVFREQ Drivers
  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
  obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
  obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
  # DEVFREQ Event Drivers

drivers/devfreq/imx-devfreq.c new file +17 -0

new file mode 100644
index 000000000000..620b344e87aa
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 23

Quoted 6 lines

drivers/devfreq/imx-devfreq.c new file +6 -0

+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)

Don't use space for the indentation. Please use only tab.

Quoted 46 lines

Quoted 46 lines

Quoted 19 lines

drivers/devfreq/imx-devfreq.c new file +19 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)

drivers/devfreq/imx-devfreq.c line 52

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)

ditto. Please use tab for the indentation.

Quoted 14 lines

drivers/devfreq/imx-devfreq.c new file +14 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static void imx_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 71

Quoted 5 lines

drivers/devfreq/imx-devfreq.c new file +5 -0

+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;

How about changing the variable name 'priv' to 'imx' or 'imx_data'? because it is not easy to catch the role of 'priv' from variable name.

Quoted 6 lines

Quoted 6 lines

drivers/devfreq/imx-devfreq.c line 73

Quoted 3 lines

drivers/devfreq/imx-devfreq.c new file +2 -0

+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;

How about changing the variable name 'govdata' to 'gov_data'?
- govdata -> gov_data

Quoted 12 lines

Quoted 12 lines

drivers/devfreq/imx-devfreq.c line 80

Quoted 7 lines

drivers/devfreq/imx-devfreq.c new file +7 -0

+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);

nitpick: because the clock-name is not mandatory. Don't need to specify the clock name to inform the role of clock of other developer/user?

Quoted 1 line

Quoted 1 line

For example, "ddr", "bus" and so on.

Quoted 2 lines

And, this driver doesn't include the 'clk_prepare_enable'. how to enable the clock?

Quoted 38 lines

Quoted 38 lines

Quoted 25 lines

drivers/devfreq/imx-devfreq.c new file +25 -0

+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));

drivers/devfreq/imx-devfreq.c line 115

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}

You better to change the exception handling as following: It is more simple.

Quoted 8 lines

Quoted 8 lines

	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
		goto err;
	} else {
		ret = PTR_ERR(priv->passive_data.parent);
		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
		goto err;
	}

Quoted 27 lines

Quoted 27 lines

Quoted 27 lines

drivers/devfreq/imx-devfreq.c new file +27 -0

+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+
+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 148

Quoted 5 lines

drivers/devfreq/imx-devfreq.c new file +5 -0

+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");

If this driver is for bus frequency, you better to use 'bus' for the clock-name for the readability.

Quoted 4 lines · Chanwoo Choi

Hi,

Also, I think that 'devfreq' word is not proper for device driver name. imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.

I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate because I also want to use it for PL301 NICs.

Quoted 8 lines

And, I have a question.
This driver adds the devfreq device with either passive governor or userspace governor.

As I understood, the devfreq device with passive governor will be operated with imx8m-ddrc.c driver.
But, when is operating with userspace governor?

There are multiple scalable buses inside the SOC, for example there's a NIC for display controllers and one for (pci+usb). They can use userspace governor for explicit frequency control.

Quoted 3 lines

I think that you better to add the explanation to description for two scenarios how to operate with interconnect provider on either passive governor or userspace governor usage case.

I'll elaborate the example in bindings.

Quoted 83 lines · Chanwoo Choi

Quoted 81 lines · Chanwoo Choi

Quoted 80 lines

Hi,

Quoted 77 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Signed-off-by Leonard Crestez <[email protected]>

---
  drivers/devfreq/Kconfig       |   9 ++
  drivers/devfreq/Makefile      |   1 +
  drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
  3 files changed, 160 insertions(+)

create mode 100644 drivers/devfreq/imx-devfreq.c

drivers/devfreq/Kconfig +9 -0

index 923a6132e741..fef5ce831e90 100644
@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
  	select DEVFREQ_GOV_USERSPACE
  	help
  	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
  	  adjusting DRAM frequency.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
  config ARM_TEGRA_DEVFREQ
  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
  	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
  		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
  		ARCH_TEGRA_210_SOC || \

drivers/devfreq/Makefile +1 -0

index 3eb4d5e6635c..61d0edee16f7 100644
@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
  obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
  # DEVFREQ Drivers
  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
  obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
  obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
  # DEVFREQ Event Drivers

drivers/devfreq/imx-devfreq.c new file +17 -0

new file mode 100644
index 000000000000..620b344e87aa
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 23

Quoted 6 lines

drivers/devfreq/imx-devfreq.c new file +6 -0

+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)

Don't use space for the indentation. Please use only tab.

OK

Quoted 46 lines

Quoted 46 lines

Quoted 46 lines

Quoted 19 lines

drivers/devfreq/imx-devfreq.c new file +19 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)

drivers/devfreq/imx-devfreq.c line 52

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)

ditto. Please use tab for the indentation.

Quoted 14 lines

drivers/devfreq/imx-devfreq.c new file +14 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static void imx_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 71

Quoted 5 lines

drivers/devfreq/imx-devfreq.c new file +5 -0

+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;

How about changing the variable name 'priv' to 'imx' or 'imx_data'? because it is not easy to catch the role of 'priv' from variable name.

The name "priv" refers to private data of current device: it is short and not ambiguous in this context. I don't think that mentioning "imx" adds any additional useful information.

It doesn't seem like there's much of a convention for "local variable containing private data", for example exynos-bus.c uses "struct exynos_bus* bus" internally.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/devfreq/imx-devfreq.c line 73

Quoted 4 lines

drivers/devfreq/imx-devfreq.c new file +2 -0

+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;

How about changing the variable name 'govdata' to 'gov_data'?
- govdata -> gov_data

OK

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

drivers/devfreq/imx-devfreq.c line 80

Quoted 7 lines

drivers/devfreq/imx-devfreq.c new file +7 -0

+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);

nitpick: because the clock-name is not mandatory. Don't need to specify the clock name to inform the role of clock of other developer/user?

Quoted 1 line

Quoted 1 line

Quoted 1 line

For example, "ddr", "bus" and so on.

I'll call this bus, but I'm not sure it's useful when a single clock is involved.

Quoted 2 lines

Quoted 2 lines

And, this driver doesn't include the 'clk_prepare_enable'. how to enable the clock?

Clocks are either always on or perhaps controlled by some other peripheral. This driver only provides scaling.

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Quoted 26 lines

drivers/devfreq/imx-devfreq.c new file +25 -0

+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));

drivers/devfreq/imx-devfreq.c line 115

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}

You better to change the exception handling as following: It is more simple.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
		goto err;
	} else {
		ret = PTR_ERR(priv->passive_data.parent);
		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
		goto err;
	}

But -ENODEV is not an error, it means no passive parent was found.

Quoted 28 lines

Quoted 28 lines

Quoted 28 lines

Quoted 28 lines

drivers/devfreq/imx-devfreq.c new file +27 -0

+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+
+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 148

Quoted 5 lines

drivers/devfreq/imx-devfreq.c new file +5 -0

+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");

If this driver is for bus frequency, you better to use 'bus' for the clock-name for the readability.

OK

Quoted 8 lines · Leonard Crestez

Quoted 4 lines · Chanwoo Choi

Hi,

Also, I think that 'devfreq' word is not proper for device driver name. imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.

I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate because I also want to use it for PL301 NICs.

OK.

Quoted 13 lines

Quoted 8 lines

And, I have a question.
This driver adds the devfreq device with either passive governor or userspace governor.

As I understood, the devfreq device with passive governor will be operated with imx8m-ddrc.c driver.
But, when is operating with userspace governor?

There are multiple scalable buses inside the SOC, for example there's a NIC for display controllers and one for (pci+usb). They can use userspace governor for explicit frequency control.

Quoted 5 lines

Quoted 3 lines

I think that you better to add the explanation to description for two scenarios how to operate with interconnect provider on either passive governor or userspace governor usage case.

I'll elaborate the example in bindings.

OK.

Quoted 93 lines

Quoted 83 lines · Chanwoo Choi

Quoted 81 lines · Chanwoo Choi

Quoted 80 lines

Hi,

Quoted 77 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Signed-off-by Leonard Crestez <[email protected]>

---
   drivers/devfreq/Kconfig       |   9 ++
   drivers/devfreq/Makefile      |   1 +
   drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
   3 files changed, 160 insertions(+)

create mode 100644 drivers/devfreq/imx-devfreq.c

drivers/devfreq/Kconfig +9 -0

index 923a6132e741..fef5ce831e90 100644
@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
   	select DEVFREQ_GOV_USERSPACE
   	help
   	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
   	  adjusting DRAM frequency.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
   config ARM_TEGRA_DEVFREQ
   	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
   	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
   		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
   		ARCH_TEGRA_210_SOC || \

drivers/devfreq/Makefile +1 -0

index 3eb4d5e6635c..61d0edee16f7 100644
@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
   obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
   # DEVFREQ Drivers
   obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
   obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
   obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
   obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
   obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
   # DEVFREQ Event Drivers

drivers/devfreq/imx-devfreq.c new file +17 -0

new file mode 100644
index 000000000000..620b344e87aa
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 23

Quoted 6 lines

drivers/devfreq/imx-devfreq.c new file +6 -0

+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)

Don't use space for the indentation. Please use only tab.

OK

Quoted 55 lines

Quoted 46 lines

Quoted 46 lines

Quoted 46 lines

Quoted 19 lines

drivers/devfreq/imx-devfreq.c new file +19 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)

drivers/devfreq/imx-devfreq.c line 52

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)

ditto. Please use tab for the indentation.

Quoted 14 lines

drivers/devfreq/imx-devfreq.c new file +14 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static void imx_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 71

Quoted 5 lines

drivers/devfreq/imx-devfreq.c new file +5 -0

+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;

How about changing the variable name 'priv' to 'imx' or 'imx_data'? because it is not easy to catch the role of 'priv' from variable name.

Quoted 7 lines

The name "priv" refers to private data of current device: it is short and not ambiguous in this context. I don't think that mentioning "imx" adds any additional useful information.

It doesn't seem like there's much of a convention for "local variable containing private data", for example exynos-bus.c uses "struct exynos_bus* bus" internally.

OK. it is nitpick. Keep your style.

Quoted 24 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/devfreq/imx-devfreq.c line 73

Quoted 4 lines

drivers/devfreq/imx-devfreq.c new file +2 -0

+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;

How about changing the variable name 'govdata' to 'gov_data'?
- govdata -> gov_data

OK

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

drivers/devfreq/imx-devfreq.c line 80

Quoted 7 lines

drivers/devfreq/imx-devfreq.c new file +7 -0

+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);

nitpick: because the clock-name is not mandatory. Don't need to specify the clock name to inform the role of clock of other developer/user?

Quoted 1 line

Quoted 1 line

Quoted 1 line

For example, "ddr", "bus" and so on.

Quoted 8 lines

I'll call this bus, but I'm not sure it's useful when a single clock is involved.

Quoted 2 lines

Quoted 2 lines

And, this driver doesn't include the 'clk_prepare_enable'. how to enable the clock?

Clocks are either always on or perhaps controlled by some other peripheral. This driver only provides scaling.

It is not proper use-case of clock. If device driver want to control the clock, it have to be enabled on device driver.

Even it clock is always, the user don't know the state of clock. Also, user can't know what kind of device driver control the clock.

It have to be controlled on this device driver
before changing the clock frequency.

Quoted 47 lines

Quoted 46 lines

Quoted 46 lines

Quoted 46 lines

Quoted 25 lines

drivers/devfreq/imx-devfreq.c new file +25 -0

+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));

drivers/devfreq/imx-devfreq.c line 115

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}

You better to change the exception handling as following: It is more simple.

	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
		goto err;
	} else {
		ret = PTR_ERR(priv->passive_data.parent);
		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
		goto err;
	}

Quoted 1 line

But -ENODEV is not an error, it means no passive parent was found.

OK. just I want to make 'if statement' more simple. This style is complicated.

Quoted 35 lines

Quoted 35 lines

Quoted 35 lines

Quoted 35 lines

Quoted 22 lines

drivers/devfreq/imx-devfreq.c new file +22 -0

+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+

drivers/devfreq/imx-devfreq.c line 148

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),
+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");

If this driver is for bus frequency, you better to use 'bus' for the clock-name for the readability.

Quoted 125 lines · Chanwoo Choi

Quoted 8 lines · Leonard Crestez

Quoted 4 lines · Chanwoo Choi

Hi,

Also, I think that 'devfreq' word is not proper for device driver name. imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.

I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate because I also want to use it for PL301 NICs.

OK.

Quoted 13 lines

Quoted 8 lines

And, I have a question.
This driver adds the devfreq device with either passive governor or userspace governor.

As I understood, the devfreq device with passive governor will be operated with imx8m-ddrc.c driver.
But, when is operating with userspace governor?

There are multiple scalable buses inside the SOC, for example there's a NIC for display controllers and one for (pci+usb). They can use userspace governor for explicit frequency control.

Quoted 5 lines

Quoted 3 lines

I think that you better to add the explanation to description for two scenarios how to operate with interconnect provider on either passive governor or userspace governor usage case.

I'll elaborate the example in bindings.

OK.

Quoted 92 lines

Quoted 91 lines · Chanwoo Choi

Quoted 89 lines · Chanwoo Choi

Quoted 88 lines

Hi,

Quoted 73 lines · Leonard Crestez

Add initial support for dynamic frequency switching on pieces of the imx interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does not map register areas.

Signed-off-by Leonard Crestez <[email protected]>

---
   drivers/devfreq/Kconfig       |   9 ++
   drivers/devfreq/Makefile      |   1 +
   drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
   3 files changed, 160 insertions(+)

create mode 100644 drivers/devfreq/imx-devfreq.c

drivers/devfreq/Kconfig +9 -0

index 923a6132e741..fef5ce831e90 100644
@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
   	select DEVFREQ_GOV_USERSPACE
   	help
   	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
   	  adjusting DRAM frequency.
 
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
   config ARM_TEGRA_DEVFREQ
   	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
   	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
   		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
   		ARCH_TEGRA_210_SOC || \

drivers/devfreq/Makefile +1 -0

index 3eb4d5e6635c..61d0edee16f7 100644
@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
   obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
   # DEVFREQ Drivers
   obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
   obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
   obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
   obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
   obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
 
   # DEVFREQ Event Drivers

drivers/devfreq/imx-devfreq.c new file +13 -0

new file mode 100644
index 000000000000..620b344e87aa
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>

drivers/devfreq/imx-devfreq.c line 23

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)

Don't use space for the indentation. Please use only tab.

Quoted 1 line

Quoted 1 line

OK

The spaces are required in order to align arguments to open paranthesis. Should I drop that?

It seems that check_patch.pl and process/coding-style.rst doesn't have a strong opinion on this; my personal preference is for long argument lists to just use double indentation.

Quoted 99 lines

Quoted 56 lines

Quoted 47 lines

Quoted 47 lines

Quoted 47 lines

Quoted 20 lines

drivers/devfreq/imx-devfreq.c new file +19 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)

drivers/devfreq/imx-devfreq.c line 52

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)

ditto. Please use tab for the indentation.

Quoted 14 lines

drivers/devfreq/imx-devfreq.c new file +14 -0

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static void imx_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/devfreq/imx-devfreq.c line 71

Quoted 5 lines

drivers/devfreq/imx-devfreq.c new file +5 -0

+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;

How about changing the variable name 'priv' to 'imx' or 'imx_data'? because it is not easy to catch the role of 'priv' from variable name.

Quoted 7 lines

The name "priv" refers to private data of current device: it is short and not ambiguous in this context. I don't think that mentioning "imx" adds any additional useful information.

It doesn't seem like there's much of a convention for "local variable containing private data", for example exynos-bus.c uses "struct exynos_bus* bus" internally.

OK. it is nitpick. Keep your style.

Quoted 24 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/devfreq/imx-devfreq.c line 73

Quoted 4 lines

drivers/devfreq/imx-devfreq.c new file +2 -0

+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;

How about changing the variable name 'govdata' to 'gov_data'?
- govdata -> gov_data

OK

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

drivers/devfreq/imx-devfreq.c line 80

Quoted 7 lines

drivers/devfreq/imx-devfreq.c new file +7 -0

+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);

nitpick: because the clock-name is not mandatory. Don't need to specify the clock name to inform the role of clock of other developer/user?

Quoted 1 line

Quoted 1 line

Quoted 1 line

For example, "ddr", "bus" and so on.

Quoted 8 lines

I'll call this bus, but I'm not sure it's useful when a single clock is involved.

Quoted 2 lines

Quoted 2 lines

And, this driver doesn't include the 'clk_prepare_enable'. how to enable the clock?

Clocks are either always on or perhaps controlled by some other peripheral. This driver only provides scaling.

Quoted 8 lines

It is not proper use-case of clock. If device driver want to control the clock, it have to be enabled on device driver.

Even it clock is always, the user don't know the state of clock. Also, user can't know what kind of device driver control the clock.

It have to be controlled on this device driver
before changing the clock frequency.

From clock framework perspective prepare/enable and rate bits can be
controlled separately.

Many peripherals are grouped with their own bus (for example a PL301 NIC) which is normally off and only gets enabled when explicitly requested by drivers. If this devfreq driver always enabled bus clocks then it would waste power for no reason.

For example a display controller will first enable clocks to allow access to device registers, then configure a resolution and make a bandwith request which gets translated a min_freq request. Then when the display is blanked the entire display bus should be powered off, even if this makes control registers inaccessible.

This series only enables scaling for the main NOC which can't be turned off anyway.

Quoted 47 lines

Quoted 47 lines

Quoted 46 lines

Quoted 46 lines

Quoted 46 lines

Quoted 25 lines

drivers/devfreq/imx-devfreq.c new file +25 -0

+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));

drivers/devfreq/imx-devfreq.c line 115

Quoted 10 lines

drivers/devfreq/imx-devfreq.c new file +10 -0

+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}

You better to change the exception handling as following: It is more simple.

	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
		goto err;
	} else {
		ret = PTR_ERR(priv->passive_data.parent);
		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
		goto err;
	}