Re: [PATCH 01/13] dt-bindings: memory: snps: Extend schema with IRQs/resets/clocks props

[Krzysztof Kozlowski]

On 22/08/2022 22:19, Serge Semin wrote:
> First of all the DW uMCTL2 DDRC IP-core supports the individual IRQ lines
> for each standard event: ECC Corrected Error, ECC Uncorrected Error, ECC
> Address Protection, Scrubber-Done signal, DFI Parity/CRC Error. It's
> possible that the platform engineers merge them up in the IRQ controller
> level. So let's add both configuration support to the DT-schema.
> 
> Secondly each IP-core interface is supplied with a clock source like APB
> reference clock, AXI-ports clock, main DDRC core reference clock and
> Scrubber low-power clock. In addition to that each clock domain can have a
> dedicated reset signal. Let's add the properties for at least the denoted
> clock sources and the corresponding reset controls.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> ---
>  .../snps,dw-umctl2-ddrc.yaml                  | 65 +++++++++++++++++--
>  1 file changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> index 787d91d64eee..8db92210cfe1 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
> @@ -13,13 +13,13 @@ maintainers:
>  
>  description: |
>    Synopsys DesignWare Enhanced uMCTL2 DDR Memory Controller is cappable of

Typo in original text: capable

> -  working with DDR devices up to (LP)DDR4 protocol. It can be equipped
> +  working with DDR devices upporting to (LP)DDR4 protocol. It can be equipped

Typo - supporting?

>    with SEC/DEC ECC feature if DRAM data bus width is either 16-bits or
>    32-bits or 64-bits wide.
>  
> -  The ZynqMP DDR controller is based on the DW uMCTL2 v2.40a controller.
> -  It has an optional SEC/DEC ECC support in 64-bit and 32-bit bus width
> -  configurations.
> +  For instance the ZynqMP DDR controller is based on the DW uMCTL2 v2.40a
> +  controller. It has an optional SEC/DEC ECC support in 64-bit and 32-bit
> +  bus width configurations.

These changes do not look related to your patch, so split them.

>  
>  properties:
>    compatible:
> @@ -28,11 +28,55 @@ properties:
>        - xlnx,zynqmp-ddrc-2.40a
>  
>    interrupts:
> -    maxItems: 1
> +    description:
> +      DW uMCTL2 DDRC IP-core provides individual IRQ signal for each event":"
> +      ECC Corrected Error, ECC Uncorrected Error, ECC Address Protection,
> +      Scrubber-Done signal, DFI Parity/CRC Error. Some platforms may have the
> +      signals merged before they reach the IRQ controller or have some of them
> +      absent in case if the corresponding feature is unavailable/disabled.
> +    minItems: 1
> +    maxItems: 5

List has to be strictly ordered, so instead list and describe the
items... unless you are sure that any of these interrupt lines can be
merged into any other one?

> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 5
> +    oneOf:
> +      - description: Common ECC CE/UE/Scrubber/DFI Errors IRQ
> +        items:
> +          - const: ecc
> +      - description: Individual ECC CE/UE/Scrubber/DFI Errors IRQs
> +        items:
> +          enum: [ ecc_ce, ecc_ue, ecc_ap, ecc_sbr, dfi_e ]
>  
>    reg:
>      maxItems: 1
>  
> +  clocks:
> +    description:
> +      A standard set of the clock sources contains CSRs bus clock, AXI-ports
> +      reference clock, DDRC core clock, Scrubber standalone clock
> +      (synchronous to the DDRC clock).
> +    minItems: 1
> +    maxItems: 4

I expect list to be strictly defined, not flexible.

> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 4
> +    items:
> +      enum: [ pclk, aclk, core, sbr ]
> +
> +  resets:
> +    description:
> +      Each clock domain can have separate reset signal.
> +    minItems: 1
> +    maxItems: 4
> +
> +  reset-names:
> +    minItems: 1
> +    maxItems: 4
> +    items:
> +      enum: [ prst, arst, core, sbr ]

The same.

> +
>  required:
>    - compatible
>    - reg
> @@ -48,4 +92,15 @@ examples:
>        interrupt-parent = <&gic>;
>        interrupts = <0 112 4>;
>      };
> +  - |
> +    memory-controller@fd070000 {
> +      compatible = "snps,ddrc-3.80a";
> +      reg = <0x3d400000 0x400000>;
> +
> +      interrupts = <0 147 4>, <0 148 4>, <0 149 4>, <0 150 4>;

Use proper defines.

> +      interrupt-names = "ecc_ce", "ecc_ue", "ecc_sbr", "dfi_e";
> +
> +      clocks = <&rcu 0>, <&rcu 5>, <&rcu 6>, <&rcu 7>;
> +      clock-names = "pclk", "aclk", "core", "sbr";
> +    };
>  ...


Best regards,
Krzysztof