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

[Chanwoo Choi <cw00.choi@samsung.com>]

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://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&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C2d1f1868afa140702a6b08d781d6ab68%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637120631307418544&amp;sdata=H2q5nQlKYyLIivkBYUTaRD1Nu3WcnphPJny3k%2BK%2BGFE%3D&amp;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&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C2d1f1868afa140702a6b08d781d6ab68%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637120631307418544&amp;sdata=T6PgQ1DWI4OLOx3gifRRt%2FNImdVrgDUoswZ%2FNKw3oR8%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://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=f3678b4e6b75dccfe8bb87d005da2d68c70fdeab
[2] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing

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

> 
>>
>> [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>;
>>>> +                    };
>>>> +            };
>>>> +    };
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics