Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: Shiping Ji <shiping.linux@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: bp@alien8.de, james.morse@arm.com, mark.rutland@arm.com,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	mchehab@kernel.org, linux-edac@vger.kernel.org,
	sashal@kernel.org, hangl@microsoft.com, lewan@microsoft.com,
	ruizhao@microsoft.com, scott.branden@broadcom.com,
	yuqing.shen@broadcom.com, ray.jui@broadcom.com,
	shji@microsoft.com, wangglei@gmail.com
Subject: Re: [PATCH v7 1/2] dt-bindings: edac: arm-dmc520.txt
Date: Tue, 3 Dec 2019 14:04:53 -0800
Message-ID: <87af6922-41d6-c017-b31b-75eaa2c5551b@gmail.com> (raw)
In-Reply-To: <20191121204350.GA26174@bogus>

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

      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-18  2:10 Shiping Ji
2019-11-21 20:43 ` Rob Herring
2019-12-03 22:04   ` Shiping Ji [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87af6922-41d6-c017-b31b-75eaa2c5551b@gmail.com \
    --to=shiping.linux@gmail.com \
    --cc=bp@alien8.de \
    --cc=devicetree@vger.kernel.org \
    --cc=hangl@microsoft.com \
    --cc=james.morse@arm.com \
    --cc=lewan@microsoft.com \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mchehab@kernel.org \
    --cc=ray.jui@broadcom.com \
    --cc=robh@kernel.org \
    --cc=ruizhao@microsoft.com \
    --cc=sashal@kernel.org \
    --cc=scott.branden@broadcom.com \
    --cc=shji@microsoft.com \
    --cc=wangglei@gmail.com \
    --cc=yuqing.shen@broadcom.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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