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@xxxxxxxxxxx>
>> Reviewed-by: James Morse <james.morse@xxxxxxx>
>>
>> ---
>>      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