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

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

On 19.12.2019 17:56, Chanwoo Choi wrote:
> 2019년 12월 19일 (목) 오후 11:33, 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%7C40406bfd0f134eeddb3208d7849c0968%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637123678022512623&amp;sdata=IYSYlNtA0fYKLkTr4OjyHkKa6FAi2m8UFr4nXZ9pr7s%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%7C40406bfd0f134eeddb3208d7849c0968%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637123678022512623&amp;sdata=nrbPGpgBci5DDm6qRROeFa78hARR9UCXeP59TnPtCuI%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%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
>>>
>>>>
>>>> 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.
> 
> 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.
> 
>>
>> 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
> 
>>
>> 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?

NOC would use the passive governor with devfreq_passive_data.parent sent 
to DDRC so that it matches rates when DDRC uses ondemand governor. This 
is a minor feature meant to make ondemand scaling behave more usefully, 
otherwise NOC might be a bottleneck for when CPU uses lots of ram 
bandwidth. It would make sense to split it into a separate patch.

The same imx-bus driver can also be used for peripheral buses (for 
example PL301_MIPI using imx8m-nic compatible) and in that case it will 
just use the userspace governor by default.

Useful scaling for the mipi bus would be performed via interconenct: the 
display controller (LCDIF) could request bandwidth along an icc_path 
that goes: LCDIF->PL301_MIPI->NOC->DDRC and then the interconnect 
provider would aggregate requests and translate them into separate 
minimum frequencies for PL301_MIPI, NOC and DDRC.

Camera devices would share this path while GPU and VPUs would only share 
NOC and DDRC. This detail is specific to 8mm, interconnect graph is 
different on 8mq and 8mn.

The "interconnect" driver is very loosely coupled to "devfreq" drivers: 
it just makes QOS_MIN_FREQUENCY requests. There is already a distinct 
driver for imx8m-ddrc and in theory fsl,imx8m-noc and fsl,imx8m-nic 
could have distinct drivers as well. It would be pointless because all 
they currently do is set a single clock rate so they're all handled by a 
single imx-bus driver, similar to exynos-bus.

The interconnect driver shares the imx8m-noc binding with imx-bus 
because "virtual nodes" are strongly discouraged as they don't "describe 
hardware". Earlier versions of the series had a virtual node.

>> 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
> 
>>
>>>>> [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.

Here's a more elaborate example:

#include <dt-bindings/clock/imx8mm-clock.h>
#include <dt-bindings/interconnect/imx8mm.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>

noc: interconnect@32700000 {
         compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
         reg = <0x32700000 0x100000>;
         clocks = <&clk IMX8MM_CLK_NOC>;
         #interconnect-cells = <1>;

         /* Used to make requests for DEV_PM_QOS_MIN_FREQUENCY: */
         fsl,scalable-node-ids = <IMX8MM_ICN_NOC>,
                                 <IMX8MM_ICN_MIPI>,
                                 <IMX8MM_ICS_DRAM>;
         fsl,scalable-nodes = <&noc>,
                              <&pl301_mipi>,
                              <&ddrc>;

         /* For passive governor: */
         devfreq = <&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>;
                 };
         };
};

pl301_mipi: interconnect@32500000 {
         compatible = "fsl,imx8m-nic";
         reg = <0x32500000 0x100000>;
         clocks = <&clk IMX8MM_CLK_DISP_AXI>;
         operating-points-v2 = <&pl301_mipi_opp_table>;

         pl301_mipi_opp_table: opp-table {
                 compatible = "operating-points-v2";

                 opp-200M {
                         opp-hz = /bits/ 64 <200000000>;
                 };
                 opp-500M {
                         opp-hz = /bits/ 64 <500000000>;
                 };
         };
};

ddrc: memory-controller@3d400000 {
         compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
         reg = <0x3d400000 0x400000>;
         glock-names = "core", "pll", "alt", "apb";
         clocks = <&clk IMX8MM_CLK_DRAM_CORE>,
                  <&clk IMX8MM_DRAM_PLL>,
                  <&clk IMX8MM_CLK_DRAM_ALT>,
                  <&clk IMX8MM_CLK_DRAM_APB>;
         /* For ondemand governor: */
         devfreq-events = <&ddr_pmu>;
};

ddr_pmu: ddr-pmu@3d800000 {
         compatible = "fsl,imx8mm-ddr-pmu", "fsl,imx8m-ddr-pmu";
         reg = <0x3d800000 0x400000>;
         interrupt-parent = <&gic>;
         interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
};