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

[Krzysztof Kozlowski]

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.

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

> +   interrupts:
> +   [...]
> +   interrupt-names:
> +   [...]
> + additionalProperties: false
> + [...]
> 
> 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.

> 
> 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