Re: [PATCH 2/3] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>]

On 10/02/2014 05:58 AM, Mark Rutland wrote:
>>>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
>>>> @@ -0,0 +1,15 @@
>>>> +Altera SoCFPGA L2 cache Error Detection and Correction [EDAC]
>>>> +
>>>> +Required Properties:
>>>> +- compatible : Should be "altr,l2-edac"
>>> That string looks too generic.
>>>
>>> Given the EDAC seems to be a portion of the L2, is there not already an
>>> L2 binding?
>>>
>>> Just because Linux expects two drivers doesn't mean we should partition
>>> the HW description this way.
>> Thank you for the quick feedback.
>> What should the string look like? I was trying to keep it short with the
>> altr, prefix but I don't mind changing it to something better.
>> We're using the ARM PL310 L2 cache controller. The ECC is separate from
>> the PL310 IP and is part of the System Manager. This is true of ECC for
>> both the L2 and OCRAM.
> Ah, I see. Apologies, I assumed that this was part of the L2C.
>
> [...]
>
>>>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
>>>> new file mode 100644
>>>> index 0000000..31ab205
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
>>>> @@ -0,0 +1,16 @@
>>>> +Altera SoCFPGA On-Chip RAM Error Detection and Correction [EDAC]
>>>> +
>>>> +OCRAM ECC Required Properties:
>>>> +- compatible : Should be "altr,ocram-edac"
>>>> +- reg : Address and size for ECC error interrupt clear registers.
>>>> +- iram : phandle to On-Chip RAM definition.
>>> Why not just describe this in the OCRAM node? Surely the register is
>>> within the OCRAM controller?
>> The ECC registers not in the OCRAM controller but they are in the System
>> Manager.  Maybe both the L2 cache and OCRAM ECC bindings should live
>> there and the device tree node for System Manager would have OCRAM and
>> L2 cache sub-nodes.
> It certainly sounds like the ECC registers should be described as a
> portion of the system manager somehow.
>
> Thanks,
> Mark.
Hi Mark.

After looking at this, it still seems like adding individual nodes as 
shown in this patch makes the most sense.

We don't currently have another driver to use as a parent so I'd need to 
create a driver (System Manager) that only probed these drivers. 
Although that would make the device tree look nicer, it isn't a clean 
solution overall.

Thanks for looking this over and I appreciate your feedback - it made me 
mull over other alternatives.

Thor
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html