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
