Message-ID: <20220826084726.7ra7342pa33bci5h@mobilestation>
Date:   Fri, 26 Aug 2022 11:47:27 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     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>,
        Rob Herring <robh+dt@...nel.org>,
        Manish Narani <manish.narani@...inx.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 Tue, Aug 23, 2022 at 11:11:08AM +0300, Krzysztof Kozlowski wrote:
> 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.

Right. Sorry for the confusing change. Indeed this update belongs to a
different patch. I'll move it to the patchset #0 to the patch with the
Zynq DT-bindings detachment.

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

That's what I noted in the property description. Anyway please see the
interrupt-names property for the possible interrupts setup. To sum up
some of the IRQs might be absent or some of them merged into a single
signal.

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

Some of the clock sources might be absent or tied up to another one
(for instance pclk, aclk and sbr can be clocked from a single core
clock source). It depends on the IP-core synthesize parameters.

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

The same as for the clock.

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

What do you mean? Which defines do you think would be proper? If you
meant the IRQ DT-bindings macros, then what difference does it make
for a generic device in the DT-binding example? Note since the device
is defined as generic it can be placed on different platforms with
different interrupt controller requirements. So what do you mean by
"proper" in this case?

-Serge

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