Re: [PATCH 2/7] dt-bindings: devfreq: Add bindings for generic imx buses

[Rob Herring <robh+dt@kernel.org>]

On Mon, Aug 12, 2019 at 7:32 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 8/12/2019 10:47 PM, Rob Herring wrote:
> > On Mon, Aug 12, 2019 at 12:49 PM Leonard Crestez <leonard.crestez@nxp.com> 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.
> >>
> >> +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 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.
> >
> > No need to use the interconnect binding?
>
> Separate RFC: https://patchwork.kernel.org/patch/11078673/
>
> The interconnect is represented by a separate "virtual" node which might
> not be OK. There was also a recent RFC from samsung which turns devfreq
> nodes into interconnect providers:
>      https://patchwork.kernel.org/cover/11054417/
>
> Is that preferable?

Virtual nodes are not OK.

>
> >> +required:
> >> +  - compatible
> >> +  - clocks
> >
> > reg?
>
> This is deliberately optional: for some NICs the GPV register area is
> not exposed in the memory map. This is unusual but an accurate
> description of the hardware.

Different h/w blocks should have different compatibles. GPV is an Arm
thing and I'd expect FlexNOC to be different.

> The current driver doesn't even attempt to map registers, it only
> adjusts the clock.

Irrelevant to the binding...

>
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/clock/imx8mm-clock.h>
> >> +    noc: noc@32700000 {
> >> +            compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
> >
> > Doesn't match the schema. (Well, it does with 'contains', but
> > fsl,imx8mm-noc is not documented.)
>
> I'm confused about how per-SOC compatible strings works with validation.
> There is a rule that every SOC dtsi needs to add soc prefix to all
> device nodes but of_device_id in driver code doesn't need to be updated.
>
> Without using "contains" on the "compatible" property then all
> SOC-specific compatible strings would need to be mentioned in every yaml
> files. Unless I'm missing something this means updating update every
> binding file for each new SOC?

Yes. The main exception is if various SoCs are just packaging,
binning, or fuse differences.

>
> I guess it can be useful because it also validates the compatible
> sequence itself.

Right. Order matters.

>
> For this current example something like this seems to work:
>
>    compatible:
>      oneOf:
>        - items:
>          - enum:
>            - fsl,imx8mm-nic
>            - fsl,imx8mq-nic
>          - const: fsl,imx8m-nic
>        - items:
>          - enum:
>            - fsl,imx8mm-noc
>            - fsl,imx8mq-noc
>          - const: fsl,imx8m-noc

Looks correct.

Rob