Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt
@ 2019-11-18  2:10 Shiping Ji
  2019-11-21 20:43 ` Rob Herring
  0 siblings, 1 reply; 3+ messages in thread
From: Shiping Ji @ 2019-11-18  2:10 UTC (permalink / raw)
  To: bp, james.morse, robh+dt, mark.rutland
  Cc: devicetree, linux-kernel, mchehab, linux-edac, sashal, hangl,
	lewan, ruizhao, scott.branden, yuqing.shen, ray.jui, shji,
	wangglei

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt
  2019-11-18  2:10 [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt Shiping Ji
@ 2019-11-21 20:43 ` Rob Herring
  2019-12-03 22:04   ` Shiping Ji
  0 siblings, 1 reply; 3+ messages in thread
From: Rob Herring @ 2019-11-21 20:43 UTC (permalink / raw)
  To: Shiping Ji
  Cc: bp, james.morse, mark.rutland, devicetree, linux-kernel, mchehab,
	linux-edac, sashal, hangl, lewan, ruizhao, scott.branden,
	yuqing.shen, ray.jui, shji, wangglei

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt
  2019-11-21 20:43 ` Rob Herring
@ 2019-12-03 22:04   ` Shiping Ji
  0 siblings, 0 replies; 3+ messages in thread
From: Shiping Ji @ 2019-12-03 22:04 UTC (permalink / raw)
  To: Rob Herring
  Cc: bp, james.morse, mark.rutland, devicetree, linux-kernel, mchehab,
	linux-edac, sashal, hangl, lewan, ruizhao, scott.branden,
	yuqing.shen, ray.jui, shji, wangglei

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-18  2:10 [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt Shiping Ji
2019-11-21 20:43 ` Rob Herring
2019-12-03 22:04   ` Shiping Ji

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git