Re: [PATCH 02/13] dt-bindings: memory: snps: Add Baikal-T1 DDRC support

[Serge Semin]

On Thu, Sep 08, 2022 at 05:27:30PM +0200, Krzysztof Kozlowski wrote:
> On 08/09/2022 17:08, Serge Semin wrote:
> > On Thu, Sep 08, 2022 at 11:58:50AM +0200, Krzysztof Kozlowski wrote:
> >> On 08/09/2022 11:46, Serge Semin wrote:
> >>> On Mon, Sep 05, 2022 at 12:14:21PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 26/08/2022 11:54, Serge Semin wrote:
> >>>>> On Tue, Aug 23, 2022 at 11:12:28AM +0300, Krzysztof Kozlowski wrote:
> >>>>>> On 22/08/2022 22:19, Serge Semin wrote:
> >>>>>>> Baikal-T1 DDR controller is based on the DW uMCTL2 DDRC IP-core v2.51a
> >>>>>>> with up to DDR3 protocol capability and 32-bit data bus + 8-bit ECC. There
> >>>>>>> are individual IRQs for each ECC and DFI events.The dedicated scrubber
> >>>>>>
> >>>>>
> >>>>>> Missing space before "The".
> >>>>>
> >>>>> Ok. Thanks.
> >>>>>
> >>>>>>
> >>>>>>> clock source is absent since it's fully synchronous to the core clock.
> >>>>>>
> >>>>>
> >>>>>> You need allOf:if-then restricting this per variant.
> >>>>>
> >>>>> I really don't like the allOf-if-if-etc pattern because it gets to be
> >>>>> very bulky if all the vendor-specific and generic platform
> >>>>> peculiarities are placed in there. I am more keen of having a
> >>>>> generic DT-schema which would be then allOf-ed by the vendor-specific
> >>>>> device bindings. What do you think I'd provide such design in this
> >>>>> case too?
> >>>>
> >>>> Sure, it would work.
> >>>>
> >>>>>
> >>>>> But I'll need to move the compatible property definition to the
> >>>>> "select" property. Like this:
> >>>>>
> >>>>> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml:
> >>>>> +[...]
> >>>>> +# Please create a separate DT-schema for your DW uMCTL2 DDR controller
> >>>>> +# and make sure it's assigned with the vendor-specific compatible string.
> >>>>> +select:
> >>>>> +  properties:
> >>>>> +    compatible:
> >>>>> +      oneOf:
> >>>>> +        - deprecated: true
> >>>>> +          description: Synopsys DW uMCTL2 DDR controller v3.80a
> >>>>> +          const: snps,ddrc-3.80a
> >>>>> +        - description: Synopsys DW uMCTL2 DDR controller
> >>>>> +          const: snps,dw-umctl2-ddrc
> >>>>> +        - description: Xilinx ZynqMP DDR controller v2.40a
> >>>>> +          const: xlnx,zynqmp-ddrc-2.40a
> >>>>> +  required:
> >>>>> +    - compatible
> >>>>
> >>>
> >>>> Not entirely. If you need select, then add it with compatibles, but all
> >>>> descriptions and deprecated are staying in properties.
> >>>
> >>> Ok. But note in such case the compatible string constraints will get
> >>> to be opened for any non-common string. Like this:
> >>>
> >>> + properties:
> >>> +   compatible:
> >>> +     oneOf:
> >>> +       - const: snps,ddrc-3.80a
> >>> +       - {}
> >>
> >> Not really. If you define here specific device compatibles in select,
> >> they must be here as well.
> >>
> >>>
> >>> It's required for the DT-schemas referencing the common one, otherwise
> >>> they will fail DT-nodes evaluation due to the "compatible" property
> >>> missing the vendor-specific string.
> >>
> > 
> >> o you probably mix here purposes. Either you define common schema or
> >> device specific one. If you define common, usually it does not enforce
> >> any compatibles. You do not need select, no need for compatibles either,
> >> although you can add above syntax if it is valid. If you write here
> >> specific device bindings, then compatibles should be listed. Judging
> >> from what you wrote it's neither this nor that...
> > 
> > My idea was to provide both the common DT-schema and the
> > vendor-specific ones. But the later one would refer to the common
> > schema in the framework of the "allOf:" composition. Like this:
> > 
> > snps,dw-umctl2-ddrc.yaml:
> > + [...]
> > + select:
> > +   properties:
> > +     compatible:
> > +       enum:
> > +         - snps,ddrc-3.80a
> > + [...]
> > + properties:
> > +   compatible:
> > +     oneOf:
> > +       - const: snps,ddrc-3.80a
> > +       - {}
> 

> This is not the approach snps,dwc3.yaml is doing. It is closer to
> snps,dw-pcie.yaml, but that one ends with additionalProperties:true so
> it is expected to be constrained by something else.

Right. I should have used the "additionalProperties: true" instead in
the common DT-schema below.

> 
> You can choose either way, but what you wrote before looked different -
> basically loosing the compatibles documentation.

Ok. I'll make sure the compatibles documentation won't be lost.

> 
> > +   interrupts:
> > +   [...]
> > +   interrupt-names:
> > +   [...]

> > + additionalProperties: false

* here should have been "true" of course.

> > + [...]
> > 
> > baikal,bt1-ddrc.yaml:
> > + [...]
> > + allOf:
> > +   - "schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml:#"
> > + [...]
> > + unevaluateProperties: false
> > + [...]
> > 
> > Thus the common schema as before would provide the widest set of the
> > properties and their constraints, while the vendor-specific one would
> > be !obligated! to follow the common schema conventions, but provide a
> > more specific set of the properties and constraints. A similar
> > approach is implemented for instance in the DW USB3 DT-schemas, but
> > with the generic compatible string fallback. In this case we don't
> > need the fallback string, but in order for the common schema being
> > applicable for both the common and vendor-specific DT-nodes the
> > compatible property constraints need to be designed as is provided in
> > the example above.
> 

> Anyway, it's getting complicated so I am not sure about which option now
> we discuss. You cannot have deprecated entries in select and compatibles
> described there, without describing them in properties.

Ok. That's what I intended in the later approach. I'll resend the
series with the updated bindings. We can continue the discussion after
that in the new emails thread.

-Sergey

> 
> > 
> > Alternatively we can split the snps,dw-umctl2-ddrc.yaml schema up into
> > the two ones:
> > snps,dw-umctl2-ddrc-common.yaml
> > and
> > snps,dw-umctl2-ddrc.yaml
> > So the first schema would contain all the common properties definition
> > and would be only used for being referenced in the particular device
> > DT-bindings (select: false). The snps,dw-umctl2-ddrc.yaml and
> > vendor-specific DT-schemas would just have it "allOf:"ed.
> > 
> > Personally I'd prefer the design pattern with the always-true
> > compatible property constraint as in the example above since it seems
> > easier to maintain than having the common and generic device
> > DT-schemas.
> > 
> > Note having a common DT-schema and a vendor-specific one referencing
> > the common schema is very much useful practice for the devices based
> > on the same IP-core. Vendor-device driver authors tend to create their
> > own bindings for the same clocks/resets/phys/etc thus making the
> > drivers much harder to maintain (for a bright example see what has
> > been done for the DW PCIe RP/EP IP-core). The worst part of it is that
> > ones the DT-bindings interface is implemented it can't be changed
> > since the kernel needs to be backward compatible with the DT-sources
> > compiled before. So the main goal of the common DT-schema is to fix
> > the common DT interface and make sure the new vendor-specific device
> > drivers would at least consider either to follow the IP-core DT
> > convention or to widen it up if required.
> 
> 
> Best regards,
> Krzysztof