Re: [PATCH 1/2] edac: sifive: Add DT documentation for SiFive L2 cache Controller

[James Morse]

Hi Rob, Yash,

On 28/03/2019 13:16, Rob Herring wrote:
> On Tue, Mar 12, 2019 at 02:51:00PM +0530, Yash Shah wrote:
>> DT documentation for L2 cache controller added.

>> diff --git a/Documentation/devicetree/bindings/edac/sifive-edac-l2.txt b/Documentation/devicetree/bindings/edac/sifive-edac-l2.txt
>> new file mode 100644
>> index 0000000..abce09f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/edac/sifive-edac-l2.txt
>> @@ -0,0 +1,31 @@
>> +SiFive L2 Cache EDAC driver device tree bindings
>> +-------------------------------------------------
>> +This driver uses the EDAC framework to report L2 cache controller ECC errors.
> 
> Bindings are for h/w blocks, not drivers. (And Boris may want a single 
> driver, but bindings should reflect the h/w, not what Linux (currently) 
> wants.

For h/w block compatibles and edac, I think all we need now is to ensure the DT contains
the three compatible strings: platform (if there is one), soc and ip-name (if its a
re-usable thing).
This is so that linux can pick the biggest of the three (usually platform) to probe the
driver from, as this lets us capture platform properties we only find out about later.

The single-driver idea is because ras/edac gets done late, (its not necessary to boot
linux on the board), and the edac core has difficulty with multiple components feeding
into it.

I don't think we need platform-specific-drivers until someone has a platform that needs
one for this multiple-component issue. To let us do that later (and possibly your
customer's customer to do it), we'd like to avoid probing based on the smallest
compatible, and use the biggest instead.
This lets us remove the platform-compatible from the L2-cache-controller driver's list,
and let some platform driver use it as a library instead. This is to avoid 'but not this
one' checks in the driver.

Yes it results in more one-line patches to enable support in the kernel, but these are
also statements that this platform/soc supports ras/edac. Its possible to build a soc out
of components that all support ras, but not have it working end-to-end.
(oh its optional, was it turned on? Does firmware need to enable something? Is there a
side-band signal, was it wired up everywhere).


> Are the only controls for ECC? Are all the cache attributes discoverable 
> (size, ways, line size, level, etc.)? 

>> +Example:
>> +
>> +cache-controller@2010000 {
>> +	compatible = "sifive,fu540-c000-ccache", "sifive,ccache0";

/me googles
fu540-c000 is the soc, but it doesn't have dram, so it must get used in other platforms.

If there is anything the platform/firmware can influence with this thing please ensure
they have properties in here. (firmware-privilege enables, or pins that have to be tied
high/lwo)

As an example of the problem we're tring to avoid: someone could re-use the design of
"sifive,ccache0" in another soc, where the DRAM controller also supports edac. From just
"sifive,ccache0", they can't tell their system (which needs a multi-component driver) from
yours, which just needs this one.

There may be a clever way of getting DT to do this...


>> +	interrupt-parent = <&plic>;
>> +	interrupts = <1 2 3>;
>> +	reg = <0x0 0x2010000 0x0 0x1000 0x0 0x8000000 0x0 0x2000000>;
>> +	reg-names = "control", "sideband";
>> +};
>> -- 
>> 1.9.1
>>


Thanks,

James