diff mbox series

[RFCv4,3/7] dt-bindings: devfreq: imx: Describe interconnect properties

Message ID 3f27038292c09c8bf07a086eac759132c100aedb.1566570260.git.leonard.crestez@nxp.com (mailing list archive)
State Superseded
Headers show
Series interconnect: Add imx support via devfreq | expand

Commit Message

Leonard Crestez Aug. 23, 2019, 2:36 p.m. UTC
The interconnect-node-id property is parsed by the imx interconnect
driver to find nodes on which frequencies can be adjusted.

Add #interconnect-cells so that device drivers can request paths from
bus nodes instead of requiring a separate "virtual" node to represent
the interconnect itself.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml | 5 +++++
 Documentation/devicetree/bindings/devfreq/imx.yaml      | 5 +++++
 2 files changed, 10 insertions(+)

Comments

Rob Herring (Arm) Sept. 17, 2019, 8:19 p.m. UTC | #1
On Fri, Aug 23, 2019 at 05:36:56PM +0300, Leonard Crestez wrote:
> The interconnect-node-id property is parsed by the imx interconnect
> driver to find nodes on which frequencies can be adjusted.
> 
> Add #interconnect-cells so that device drivers can request paths from
> bus nodes instead of requiring a separate "virtual" node to represent
> the interconnect itself.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml | 5 +++++
>  Documentation/devicetree/bindings/devfreq/imx.yaml      | 5 +++++
>  2 files changed, 10 insertions(+)

Please combine this with the other series for devfreq support.

Rob
Leonard Crestez Sept. 23, 2019, 5:42 p.m. UTC | #2
On 17.09.2019 23:20, Rob Herring wrote:
> On Fri, Aug 23, 2019 at 05:36:56PM +0300, Leonard Crestez wrote:
>> The interconnect-node-id property is parsed by the imx interconnect
>> driver to find nodes on which frequencies can be adjusted.
>>
>> Add #interconnect-cells so that device drivers can request paths from
>> bus nodes instead of requiring a separate "virtual" node to represent
>> the interconnect itself.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml | 5 +++++
>>   Documentation/devicetree/bindings/devfreq/imx.yaml      | 5 +++++
>>   2 files changed, 10 insertions(+)
> 
> Please combine this with the other series for devfreq support.

I understand that having two series which add to the same bindings file 
is odd but the devfreq and interconnect parts are independent to a very 
large degree and devfreq can be useful on it's own.

The only reason devfreq bindings are updated is to avoid adding a 
"virtual" node for interconnect. Since DT is a big source of confusion 
here can you read https://patchwork.kernel.org/cover/11111865/#22883457 
and maybe offer some advice?

--
Regards,
Leonard
Rob Herring (Arm) Sept. 23, 2019, 9:03 p.m. UTC | #3
On Mon, Sep 23, 2019 at 12:42 PM Leonard Crestez
<leonard.crestez@nxp.com> wrote:
>
> On 17.09.2019 23:20, Rob Herring wrote:
> > On Fri, Aug 23, 2019 at 05:36:56PM +0300, Leonard Crestez wrote:
> >> The interconnect-node-id property is parsed by the imx interconnect
> >> driver to find nodes on which frequencies can be adjusted.
> >>
> >> Add #interconnect-cells so that device drivers can request paths from
> >> bus nodes instead of requiring a separate "virtual" node to represent
> >> the interconnect itself.
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> ---
> >>   Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml | 5 +++++
> >>   Documentation/devicetree/bindings/devfreq/imx.yaml      | 5 +++++
> >>   2 files changed, 10 insertions(+)
> >
> > Please combine this with the other series for devfreq support.
>
> I understand that having two series which add to the same bindings file
> is odd but the devfreq and interconnect parts are independent to a very
> large degree and devfreq can be useful on it's own.

To start with, I'm suspicious of any 'devfreq' binding because that's
a Linux thing. I somewhat expect that the interconnect binding should
replace the devfreq binding, but I haven't been able to investigate.

> The only reason devfreq bindings are updated is to avoid adding a
> "virtual" node for interconnect. Since DT is a big source of confusion
> here can you read https://patchwork.kernel.org/cover/11111865/#22883457
> and maybe offer some advice?

Design something that matches the structure of the h/w not how Linux
drivers happen to be structured. I can't tell what that is without any
context around adding a couple of properties. Nor do I have the time
to dig into each SoC vendor's bus structure if it's even documented
publicly.

I also don't follow why you need 'interconnect-node-id' and if you do,
it should be a common property.

Rob
Leonard Crestez Sept. 24, 2019, 2:39 p.m. UTC | #4
On 24.09.2019 00:04, Rob Herring wrote:
> On Mon, Sep 23, 2019 at 12:42 PM Leonard Crestez
> <leonard.crestez@nxp.com> wrote:
>>
>> On 17.09.2019 23:20, Rob Herring wrote:
>>> On Fri, Aug 23, 2019 at 05:36:56PM +0300, Leonard Crestez wrote:
>>>> The interconnect-node-id property is parsed by the imx interconnect
>>>> driver to find nodes on which frequencies can be adjusted.
>>>>
>>>> Add #interconnect-cells so that device drivers can request paths from
>>>> bus nodes instead of requiring a separate "virtual" node to represent
>>>> the interconnect itself.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml | 5 +++++
>>>>    Documentation/devicetree/bindings/devfreq/imx.yaml      | 5 +++++
>>>>    2 files changed, 10 insertions(+)
>>>
>>> Please combine this with the other series for devfreq support.
>>
>> I understand that having two series which add to the same bindings file
>> is odd but the devfreq and interconnect parts are independent to a very
>> large degree and devfreq can be useful on it's own.
> 
> To start with, I'm suspicious of any 'devfreq' binding because that's
> a Linux thing. I somewhat expect that the interconnect binding should
> replace the devfreq binding, but I haven't been able to investigate.
> 
> Design something that matches the structure of the h/w not how Linux
> drivers happen to be structured. I can't tell what that is without any
> context around adding a couple of properties. Nor do I have the time
> to dig into each SoC vendor's bus structure if it's even documented
> publicly.

Device tree binding files are organized based on linux subsystems but 
otherwise there's little particularly specific to "devfreq" in this new 
binding.

An imx NIC or NOC is a physical component of a SOC which can run at a 
variable clock speed. The device binding uses standard clk and opp 
tables in meaningful ways so that first part is reasonable.

I also want to implement an interconnect provider but an "interconnect" 
is not a single device but rather a graph of discrete buses. Some options:
* Add a custom virtual device, easy but not upstreamable.
* Have a single icc provider device use the individual buses as 
"proxies" for OF parting. Implemented in this series but not very pretty.
* Pick the "main noc" as the single interconnect provider?

Alternatively the approach of defining the graph in the driver could be 
dropped and everything could be described in DT (quite verbose). This 
seems to be what Samsung is doing:

https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=176291

It might even possible to use of_graph; this is complicated but the NICs 
and NOCs do in fact have discrete ports with assignable properties.

 > I also don't follow why you need 'interconnect-node-id' and if you do,
 > it should be a common property.

The "interconnect-node-id" property in devicetree identifies nodes from 
the interconnect graph; for example the DDRC node identifies itself as 
IMX8MM_ICS_DRAM.

--
Regards,
Leonard
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml b/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
index 31db204e6845..014449a9dd01 100644
--- a/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
+++ b/Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml
@@ -31,10 +31,15 @@  properties:
       - const: dram_alt
       - const: dram_apb
 
   operating-points-v2: true
 
+  interconnect-node-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+  '#interconnect-cells':
+    const: 1
+
   devfreq-events:
     description: Phandle of PMU node
     $ref: "/schemas/types.yaml#/definitions/phandle"
 
 required:
diff --git a/Documentation/devicetree/bindings/devfreq/imx.yaml b/Documentation/devicetree/bindings/devfreq/imx.yaml
index 634870496d5e..f2f9b76c752f 100644
--- a/Documentation/devicetree/bindings/devfreq/imx.yaml
+++ b/Documentation/devicetree/bindings/devfreq/imx.yaml
@@ -43,10 +43,15 @@  properties:
   clocks:
     maxItems: 1
 
   operating-points-v2: true
 
+  interconnect-node-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+  '#interconnect-cells':
+    const: 1
+
   devfreq:
     description: |
       Phandle to another devfreq device to match OPPs with by using the
       passive governor.
     $ref: "/schemas/types.yaml#/definitions/phandle"