[PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt

[PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt

[Shiping Ji]

This is the device tree bindings for new EDAC driver dmc520_edac.c.

Signed-off-by: Lei Wang <leiwang_git@outlook.com>
Reviewed-by: James Morse <james.morse@arm.com>

---
     Changes in v7:
         - Added arm prefix to the interrupt-config property

---
 .../devicetree/bindings/edac/arm-dmc520.txt   | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/edac/arm-dmc520.txt

diff --git a/Documentation/devicetree/bindings/edac/arm-dmc520.txt b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
new file mode 100644
index 000000000000..476cf8b76f2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
@@ -0,0 +1,26 @@
+* ARM DMC-520 EDAC node
+
+Required properties:
+- compatible  : "brcm,dmc-520", "arm,dmc-520".
+- reg   : Address range of the DMC-520 registers.
+- interrupts  : DMC-520 interrupt numbers. The example below specifies
+     two interrupt lines for dram_ecc_errc_int and
+     dram_ecc_errd_int.
+- arm,interrupt-config : This is an array of interrupt masks. For each of the
+     above interrupt line, add one interrupt mask element to
+     it. That is, there is a 1:1 mapping from each interrupt
+     line to an interrupt mask. An interrupt mask can represent
+     multiple interrupts being enabled. Refer to interrupt_control
+     register in DMC-520 TRM for interrupt mapping. In the example
+     below, the interrupt configuration enables dram_ecc_errc_int
+     and dram_ecc_errd_int. And each interrupt is connected to
+     a separate interrupt line.
+
+Example:
+
+dmc0: dmc@200000 {
+ compatible = "brcm,dmc-520", "arm,dmc-520";
+ reg = <0x200000 0x80000>;
+ interrupts = <0x0 0x349 0x4>, <0x0 0x34B 0x4>;
+ arm,interrupt-config = <0x4>, <0x8>;
+};
-- 
2.17.1

Re: [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt

[Rob Herring]

On Sun, Nov 17, 2019 at 06:10:43PM -0800, Shiping Ji wrote:
> This is the device tree bindings for new EDAC driver dmc520_edac.c.
> 
> Signed-off-by: Lei Wang <leiwang_git@outlook.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> 
> ---
>      Changes in v7:
>          - Added arm prefix to the interrupt-config property
> 
> ---
>  .../devicetree/bindings/edac/arm-dmc520.txt   | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/edac/arm-dmc520.txt
> 
> diff --git a/Documentation/devicetree/bindings/edac/arm-dmc520.txt b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> new file mode 100644
> index 000000000000..476cf8b76f2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> @@ -0,0 +1,26 @@
> +* ARM DMC-520 EDAC node
> +
> +Required properties:
> +- compatible  : "brcm,dmc-520", "arm,dmc-520".
> +- reg   : Address range of the DMC-520 registers.
> +- interrupts  : DMC-520 interrupt numbers. The example below specifies
> +     two interrupt lines for dram_ecc_errc_int and
> +     dram_ecc_errd_int.
> +- arm,interrupt-config : This is an array of interrupt masks. For each of the
> +     above interrupt line, add one interrupt mask element to
> +     it. That is, there is a 1:1 mapping from each interrupt
> +     line to an interrupt mask. An interrupt mask can represent
> +     multiple interrupts being enabled. Refer to interrupt_control
> +     register in DMC-520 TRM for interrupt mapping. In the example
> +     below, the interrupt configuration enables dram_ecc_errc_int
> +     and dram_ecc_errd_int. And each interrupt is connected to
> +     a separate interrupt line.

Looking at this again, I think I now understand what you are trying to 
do. Your mask is just what interrupt line each one is. We have a 
standard way of doing this either by using indices of 'interrupts' or 
with interrupt-names. The latter probably works best in this case.

You need to define *all* the interrupt-names:
combined
ram_ecc_errc
ram_ecc_errd
dram_ecc_errc
dram_ecc_errd
failed_access
failed_prog
link_err
temperature_event
arch_fsm
phy_request

I'm not sure if all the '*_oflow' interrupts should be listed too. It 
doesn't seem all that useful to get a 2nd interrupt.

Your node should list all that are hooked up in the h/w, not just the 
ones you need for EDAC.

Rob

Re: [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt

[Shiping Ji]

On 11/21/2019 12:43 PM, Rob Herring wrote:
> On Sun, Nov 17, 2019 at 06:10:43PM -0800, Shiping Ji wrote:
>> This is the device tree bindings for new EDAC driver dmc520_edac.c.
>>
>> Signed-off-by: Lei Wang <leiwang_git@outlook.com>
>> Reviewed-by: James Morse <james.morse@arm.com>
>>
>> ---
>>      Changes in v7:
>>          - Added arm prefix to the interrupt-config property
>>
>> ---
>>  .../devicetree/bindings/edac/arm-dmc520.txt   | 26 +++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/edac/arm-dmc520.txt
>>
>> diff --git a/Documentation/devicetree/bindings/edac/arm-dmc520.txt b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
>> new file mode 100644
>> index 000000000000..476cf8b76f2a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
>> @@ -0,0 +1,26 @@
>> +* ARM DMC-520 EDAC node
>> +
>> +Required properties:
>> +- compatible  : "brcm,dmc-520", "arm,dmc-520".
>> +- reg   : Address range of the DMC-520 registers.
>> +- interrupts  : DMC-520 interrupt numbers. The example below specifies
>> +     two interrupt lines for dram_ecc_errc_int and
>> +     dram_ecc_errd_int.
>> +- arm,interrupt-config : This is an array of interrupt masks. For each of the
>> +     above interrupt line, add one interrupt mask element to
>> +     it. That is, there is a 1:1 mapping from each interrupt
>> +     line to an interrupt mask. An interrupt mask can represent
>> +     multiple interrupts being enabled. Refer to interrupt_control
>> +     register in DMC-520 TRM for interrupt mapping. In the example
>> +     below, the interrupt configuration enables dram_ecc_errc_int
>> +     and dram_ecc_errd_int. And each interrupt is connected to
>> +     a separate interrupt line.
> 
> Looking at this again, I think I now understand what you are trying to 
> do. Your mask is just what interrupt line each one is. We have a 
> standard way of doing this either by using indices of 'interrupts' or 
> with interrupt-names. The latter probably works best in this case.
> 
> You need to define *all* the interrupt-names:
> combined
> ram_ecc_errc
> ram_ecc_errd
> dram_ecc_errc
> dram_ecc_errd
> failed_access
> failed_prog
> link_err
> temperature_event
> arch_fsm
> phy_request

Thanks for interrupt-names suggestion!

We did experiments and it looks cleaner now. In the device tree we define only the interrupts and interrupt-names that are of interest:

dmc0: dmc@200000 {
 compatible = "brcm,dmc-520", "arm,dmc-520";
 reg = <0x200000 0x80000>;
 interrupts = <0x0 0x349 0x4>, <0x0 0x34B 0x4>;
 interrupt-names = "dram_ecc_errc", "dram_ecc_errd";
};

In the driver code, we maintain an interrupt table with all known interrupts (name, irq, mask, etc.) Upon probing, we go through every known interrupt name and call platform_get_irq_byname(). If any interrupt has been defined then we update the interrupt number in the table.

In isr function we lookup the mask and perform specific logic.
 
> I'm not sure if all the '*_oflow' interrupts should be listed too. It 
> doesn't seem all that useful to get a 2nd interrupt.

No, we do not list them.

> Your node should list all that are hooked up in the h/w, not just the > ones you need for EDAC.

Do you suggest to list all of them in the device tree node? Currently we list only the enabled ones and update the mask accordingly.

Please let us know whether our approach makes sense, we will submit new patch after your confirmation.
 
> Rob
> 

-- 
Best regards,
Shiping Ji

Re: [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt

[Yuqing Shen]

Hi, Shiping
This commit looks good to me.
Yuqing

On Sun, Nov 17, 2019 at 7:10 PM Shiping Ji <shiping.linux@gmail.com> wrote:
>
> This is the device tree bindings for new EDAC driver dmc520_edac.c.
>
> Signed-off-by: Lei Wang <leiwang_git@outlook.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> Reviewed-by: Yuqing Shen <yuqing.shen@broadcom.com>
>  Tested-by: Yuqing Shen <yuqing.shen@broadcom.com>
> ---
>      Changes in v7:
>          - Added arm prefix to the interrupt-config property
>
> ---
>  .../devicetree/bindings/edac/arm-dmc520.txt   | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/edac/arm-dmc520.txt
>
> diff --git a/Documentation/devicetree/bindings/edac/arm-dmc520.txt b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> new file mode 100644
> index 000000000000..476cf8b76f2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/arm-dmc520.txt
> @@ -0,0 +1,26 @@
> +* ARM DMC-520 EDAC node
> +
> +Required properties:
> +- compatible  : "brcm,dmc-520", "arm,dmc-520".
> +- reg   : Address range of the DMC-520 registers.
> +- interrupts  : DMC-520 interrupt numbers. The example below specifies
> +     two interrupt lines for dram_ecc_errc_int and
> +     dram_ecc_errd_int.
> +- arm,interrupt-config : This is an array of interrupt masks. For each of the
> +     above interrupt line, add one interrupt mask element to
> +     it. That is, there is a 1:1 mapping from each interrupt
> +     line to an interrupt mask. An interrupt mask can represent
> +     multiple interrupts being enabled. Refer to interrupt_control
> +     register in DMC-520 TRM for interrupt mapping. In the example
> +     below, the interrupt configuration enables dram_ecc_errc_int
> +     and dram_ecc_errd_int. And each interrupt is connected to
> +     a separate interrupt line.
> +
> +Example:
> +
> +dmc0: dmc@200000 {
> + compatible = "brcm,dmc-520", "arm,dmc-520";
> + reg = <0x200000 0x80000>;
> + interrupts = <0x0 0x349 0x4>, <0x0 0x34B 0x4>;
> + arm,interrupt-config = <0x4>, <0x8>;
> +};
> --
> 2.17.1
>