diff mbox series

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

Message ID 20220822191957.28546-2-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State New, archived
Headers show
Series EDAC/synopsys: Add generic resources and Baikal-T1 support | expand

Commit Message

Serge Semin Aug. 22, 2022, 7:19 p.m. UTC 
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@baikalelectronics.ru>
---
 .../snps,dw-umctl2-ddrc.yaml                  | 65 +++++++++++++++++--
 1 file changed, 60 insertions(+), 5 deletions(-)

Comments

Krzysztof Kozlowski Aug. 23, 2022, 8:11 a.m. UTC | #1 
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@baikalelectronics.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@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
Serge Semin Aug. 26, 2022, 8:47 a.m. UTC | #2 
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@baikalelectronics.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@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.

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
Krzysztof Kozlowski Aug. 30, 2022, 3:01 p.m. UTC | #3 
On 26/08/2022 11:47, Serge Semin wrote:

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

Yet still your device has clock lines - clock inputs, right? Therefore
still 4 clocks will be going there or you want to say that the pin is
not connected (or pulled down)?

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

The macros/defines representing these numbers.


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

Proper means text instead of hard-coded number. This piece of code has
meaning in a specific context, because you used interrupts matching some
specific interrupt controllers. In that controller context, the "4" has
a meaning. Just like "0". You cannot say that this piece of code is
interrupt-controller-independent, because it is not. 4 has meaning.

If you want it to be independent, drop all the flags... If you use flags
from a specific implementation, then use proper defines matching them,
not hard-coded numbers.

Best regards,
Krzysztof
Serge Semin Sept. 9, 2022, 7:49 a.m. UTC | #4 
On Tue, Aug 30, 2022 at 06:01:56PM +0300, Krzysztof Kozlowski wrote:
> On 26/08/2022 11:47, Serge Semin wrote:
> 
> >>
> >>> +
> >>> +  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.
> 
> Yet still your device has clock lines - clock inputs, right? Therefore
> still 4 clocks will be going there or you want to say that the pin is
> not connected (or pulled down)?

As we agreed my device will have dedicated DT-schema referencing this
one. The DT-bindings in the subject is the generic IP-core bindings.
So some of the lines indeed may be absent depending on the synthesize
parameters or the particular platform specifics. My device DT-schema
will contain more specific clocks/resets/irqs properties constraints.

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

> The macros/defines representing these numbers.
> 
> 
> > 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?
> 
> Proper means text instead of hard-coded number. This piece of code has
> meaning in a specific context, because you used interrupts matching some
> specific interrupt controllers. In that controller context, the "4" has
> a meaning. Just like "0". You cannot say that this piece of code is
> interrupt-controller-independent, because it is not. 4 has meaning.
> 
> If you want it to be independent, drop all the flags... If you use flags
> from a specific implementation, then use proper defines matching them,
> not hard-coded numbers.

I'll replace the number 4 with the IRQ-level triggered macro and drop
the flag 0 then.

-Sergey

> 
> Best regards,
> Krzysztof
diff mbox series

Patch

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
-  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
   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.
 
 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
+
+  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
+
+  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 ]
+
 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>;
+      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";
+    };
 ...