Re: [PATCH RFC v6 1/9] dt-bindings: interconnect: Add bindings for imx8m noc

[Leonard Crestez <leonard.crestez@nxp.com>]

On 17.12.2019 02:08, Chanwoo Choi wrote:
> On 12/17/19 12:09 AM, Leonard Crestez wrote:
>> On 16.12.2019 05:18, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 12/16/19 10:12 AM, Chanwoo Choi wrote:
>>>> On 11/15/19 5:09 AM, Leonard Crestez wrote:
>>>>> Add initial dt bindings for the interconnects inside i.MX chips.
>>>>> Multiple external IPs are involved but SOC integration means the
>>>>> software controllable interfaces are very similar.
>>>>>
>>>>> 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 <leonard.crestez@nxp.com>
>>>>> ---
>>>>>    .../bindings/interconnect/fsl,imx8m-noc.yaml  | 104 ++++++++++++++++++
>>>>>    1 file changed, 104 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml b/Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..5cd94185fec3
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml
>>>>> @@ -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&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&amp;sdata=HYMJJHWyiKRhf7GDjKoOwjDpcZuYqlFlmRrDZnIRx5w%3D&amp;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&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C769d8e354f3b4d00b84508d782854a17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637121381290437871&amp;sdata=fIbrUUOUtZ5nt%2FH1tm3dzaI1J%2FGn5Gc54ms93HnBnQg%3D&amp;reserved=0
>>>>> +
>>>>> +title: Generic i.MX bus frequency device
>>>>> +
>>>>> +maintainers:
>>>>> +  - Leonard Crestez <leonard.crestez@nxp.com>
>>>>> +
>>>>> +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
>>>>> +  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>;
>>
>> 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
> 
>>
>> 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.
> 
>> But at the bus level DDRC only has AXI and APB slave ports.
> 
> 'DDRC' indicates the 'drivers/devfreq/imx8m-ddrc.c?
> 
>>
>> 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.

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?

>>> [1] [PATCH RFC v5 04/10] PM / devfreq: Add devfreq_get_devfreq_by_node
>>>
>>>>
>>>>> +      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
>>>>> +      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).
>>
>> OK, I'll elaborate explanation on noc binding.
> 
> Thanks. If possible, you better to add almost example cases.
> 
>>
>>>>
>>>>> +    #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>;
>>>>> +                    };
>>>>> +            };
>>>>> +    };
>>
>>
> 
>