Date:   Tue, 23 Aug 2022 11:11:08 +0300
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Michal Simek <michal.simek@...inx.com>,
        Borislav Petkov <bp@...en8.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Manish Narani <manish.narani@...inx.com>
Cc:     Serge Semin <fancer.lancer@...il.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Michail Ivanov <Michail.Ivanov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Punnaiah Choudary Kalluri 
        <punnaiah.choudary.kalluri@...inx.com>,
        Dinh Nguyen <dinguyen@...nel.org>,
        James Morse <james.morse@....com>,
        Robert Richter <rric@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/13] dt-bindings: memory: snps: Extend schema with
 IRQs/resets/clocks props

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@...kalelectronics.ru>
> ---
>  .../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@...70000 {
> +      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

Powered by blists - more mailing lists