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: 7374 bytes --]

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.

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