Re: [PATCH v5] dt-bindings: memory-controllers: Convert Altera SDRAM EDAC .txt to YAML

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8113 bytes --]

On Fri, Jun 20, 2025 at 03:02:45PM +0100, Conor Dooley wrote:
> On Fri, Jun 20, 2025 at 02:55:16PM +0530, Shankari Anand wrote:
> > Hi all,
> > 
> > I wanted to follow up on an issue I’ve noticed with the patch I
> > recently submitted.
> > 
> > While the indentation appears correct in my local `.patch` file and
> > IDE, it seems that the formatting doesn’t render correctly in the
> > email view. I suspect this might be due to the way tabs or whitespace
> > are handled when the patch is transmitted via email.
> > 
> > I'm investigating my email configuration and environment to ensure
> > this doesn’t happen again. I’ll make sure future submissions are
> > double-checked and rendered correctly both locally and on-list.
> 
> Meh, it doesn't really matter if things don't render exactly as they
> would in an editor on the list, I would not worry about it. The binding
> seems to be correctly indented when the patch is applied and that's
> what matters.
> 
> > On Fri, Jun 20, 2025 at 2:44 PM Shankari Anand
> > <shankari.ak0208@gmail.com> wrote:
> > >
> > > Convert the Altera SOCFPGA SDRAM EDAC devicetree binding from the
> > > .txt format to a YAML schema.
> > >
> > > Added a 'reg' property as dt_binding_check flagged its absence.
> > > The controller is memory-mapped; address is confirmed from Intel's manual.
> > >
> > > Also added two strings: altr,sdram-edac-a10 and altr,sdram-edac-s10,
> > > compatible with altr,sdram-edac but use two interrupts;
> > > Schema enforces interrupt count per variant.
> > >
> > > Signed-off-by: Shankari Anand <shankari.ak0208@gmail.com>
> > > ---
> > > v4 -> v5: Updated commit message, fixed placement of reg, added allOf condition per variant, fixed example section, dropped description as specified
> > >
> > > Reference for reg address - https://www.intel.com/content/www/us/en/programmable/hps/arria-10/hps.html#topic/sfo1429889205804.html ,
> > > https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#bal1505408006582.html
> > > ---
> > >
> > >  .../arm/altera/socfpga-sdram-edac.txt         |  15 ---
> > >  .../memory-controllers/altr,sdram-edac.yaml   | 104 ++++++++++++++++++
> > >  2 files changed, 104 insertions(+), 15 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > >  create mode 100644 Documentation/devicetree/bindings/memory-controllers/altr,sdram-edac.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > > deleted file mode 100644
> > > index f5ad0ff69fae..000000000000
> > > --- a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > > +++ /dev/null
> > > @@ -1,15 +0,0 @@
> > > -Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
> > > -The EDAC accesses a range of registers in the SDRAM controller.
> > > -
> > > -Required properties:
> > > -- compatible : should contain "altr,sdram-edac" or "altr,sdram-edac-a10"
> > > -- altr,sdr-syscon : phandle of the sdr module
> > > -- interrupts : Should contain the SDRAM ECC IRQ in the
> > > -       appropriate format for the IRQ controller.
> > > -
> > > -Example:
> > > -       sdramedac {
> > > -               compatible = "altr,sdram-edac";
> > > -               altr,sdr-syscon = <&sdr>;
> > > -               interrupts = <0 39 4>;
> > > -       };
> > > diff --git a/Documentation/devicetree/bindings/memory-controllers/altr,sdram-edac.yaml b/Documentation/devicetree/bindings/memory-controllers/altr,sdram-edac.yaml
> > > new file mode 100644
> > > index 000000000000..652289c2c2f4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/memory-controllers/altr,sdram-edac.yaml
> > > @@ -0,0 +1,104 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/memory-controllers/altr,sdram-edac.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Altera SoCFPGA SDRAM EDAC Controller
> > > +
> > > +maintainers:
> > > +  - Matthew Gerlach <matthew.gerlach@altera.com>
> > > +
> > > +description: |
> > > +  EDAC-compatible controller for SDRAM error detection and correction on
> > > +  Altera (Intel) SoCFPGA platforms.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - altr,sdram-edac
> > > +      - altr,sdram-edac-a10
> > > +      - altr,sdram-edac-s10
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  altr,sdr-syscon:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description: Phandle to the SDRAM system controller (SDR) syscon node.
> > > +
> > > +  interrupts:
> > > +    minItems: 1
> > > +    maxItems: 2
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - altr,sdr-syscon
> > > +  - interrupts
> > > +
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - altr,sdram-edac-a10
> > > +              - altr,sdram-edac-s10
> > > +    then:
> > > +      properties:
> > > +        interrupts:
> > > +          minItems: 2
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    / {
> > > +        model = "Example SoC with SDRAM EDAC";
> > > +        compatible = "arm-soc";
> > > +
> > > +        #address-cells = <1>;
> > > +        #size-cells = <1>;
> > > +
> 
> > > +        intc: interrupt-controller@0 {
> > > +            compatible = "arm,cortex-a9-gic";
> > > +            interrupt-controller;
> > > +            #interrupt-cells = <3>;
> > > +            reg = <0x0 0x1000>, <0x1000 0x1000>; // Dummy
> > > +        };
> > > +
> > > +        sdr: syscon@f8000000 {
> > > +            compatible = "altr,sdr-ctl", "syscon";
> > > +            reg = <0xf8000000 0x1000>; // Dummy
> > > +        };
> 
> These dummy devices are not needed, nor are the fake model and
> compatible above. The test scripts are able to tolerate there being
> references missing, and it makes the examples clearer when they just
> cover the node in question. I think you could deplete two of the
> memory-controller nodes too, since all 3 are effectively the same.

In fact, using "arm-soc" actually causes problems, as that's not a real
compatible. Additionally, making the reg property mandatory causes
warnings in the altr,socfpga-ecc-manager binding because there's no reg
property in the example there. Can you take care of that please?

> 
> Otherwise, this looks fine to me.
> 
> Cheers,
> Conor.
> 
> > > +
> > > +        memory-controller@f8004000 {
> > > +            compatible = "altr,sdram-edac-a10";
> > > +            reg = <0xf8004000 0x1000>;
> > > +            altr,sdr-syscon = <&sdr>;
> > > +            interrupt-parent = <&intc>;
> > > +            interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>,
> > > +                         <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> > > +        };
> > > +
> > > +        memory-controller@f8011000 {
> > > +            compatible = "altr,sdram-edac-s10";
> > > +            reg = <0xf8011000 0x1000>;
> > > +            altr,sdr-syscon = <&sdr>;
> > > +            interrupt-parent = <&intc>;
> > > +            interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
> > > +                         <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> > > +        };
> > > +
> > > +        memory-controller@ffc25000 {
> > > +            compatible = "altr,sdram-edac";
> > > +            reg = <0xffc25000 0x1000>;
> > > +            altr,sdr-syscon = <&sdr>;
> > > +            interrupt-parent = <&intc>;
> > > +            interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > > +        };
> > > +    };
> > > --
> > > 2.34.1
> > >



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]