Re: [PATCH v6 09/52] dt-bindings: memory: tegra30: mc: Document new interconnect property

[Krzysztof Kozlowski]

On Tue, Oct 27, 2020 at 10:18:35PM +0300, Dmitry Osipenko wrote:
> 27.10.2020 12:05, Krzysztof Kozlowski пишет:
> > On Mon, Oct 26, 2020 at 01:16:52AM +0300, Dmitry Osipenko wrote:
> >> Memory controller is interconnected with memory clients and with the
> >> External Memory Controller. Document new interconnect property which
> >> turns memory controller into interconnect provider.
> >>
> >> Acked-by: Rob Herring <robh@xxxxxxxxxx>
> >> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> >> ---
> >>  .../bindings/memory-controllers/nvidia,tegra30-mc.yaml       | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-mc.yaml
> >> index 84fd57bcf0dc..5436e6d420bc 100644
> >> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-mc.yaml
> >> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-mc.yaml
> >> @@ -57,6 +57,9 @@ properties:
> >>    "#iommu-cells":
> >>      const: 1
> >>  
> >> +  "#interconnect-cells":
> >> +    const: 1
> >> +
> >>  patternProperties:
> >>    "^emc-timings-[0-9]+$":
> >>      type: object
> >> @@ -120,6 +123,7 @@ required:
> >>    - clock-names
> >>    - "#reset-cells"
> >>    - "#iommu-cells"
> >> +  - "#interconnect-cells"
> > 
> > Rob,
> > 
> > You were fine with adding a new required property which breaks all
> > existing DTBs?
> 
> This is a required property for the new bindings and optional for the
> older. Does it really need to be made optional in the binding?

Mhmm... that's an interesting point. I assumed that the bindings should
reflect current status of the ABI, but I could imagine that you update
the bindings while keeping the driver working with older DTBs.

How do you actually track then the ABI? If incompatible change can be
added to the bindings, later anyone anytime can also update the driver
to enforce the bindings. To require such property.

Best regards,
Krzysztof


> 
> > Were these bindings marked as unstable? The patchset does not even
> > say/scream that it breaks the ABI, so this might be quite a surprise for
> > someone...
> 
> Please see tegra_mc_interconnect_setup() in "memory: tegra-mc: Add
> interconnect framework" patch, which check presence of the new ICC DT
> property.