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

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

[Yash Shah]

DT documentation for L2 cache controller added.

Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 .../devicetree/bindings/edac/sifive-edac-l2.txt    | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/edac/sifive-edac-l2.txt

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.
+
+- compatible: Should be "sifive,<chip>-ccache" and "sifive,ccache<version>".
+  Supported compatible strings are:
+  "sifive,fu540-c000-ccache" for the SiFive cache controller v0 as integrated
+  onto the SiFive FU540 chip, and "sifive,ccache0" for the SiFive
+  cache controller v0 IP block with no chip integration tweaks.
+  Please refer to sifive-blocks-ip-versioning.txt for details
+
+- interrupts: Must contain 3 entries for FU540 (DirError, DataError, and
+  DataFail signals) or 4 entries for other chips (DirError, DirFail, DataError,
+  and DataFail signals)
+
+- interrupt-parent: Must be core interrupt controller
+
+- reg: Physical base address and size of L2 cache controller registers map
+  A second range can indicate L2 Loosely Integrated Memory
+
+- reg-names: Names for the cells of reg, must contain "control" and "sideband"
+
+Example:
+
+cache-controller@2010000 {
+	compatible = "sifive,fu540-c000-ccache", "sifive,ccache0";
+	interrupt-parent = <&plic>;
+	interrupts = <1 2 3>;
+	reg = <0x0 0x2010000 0x0 0x1000 0x0 0x8000000 0x0 0x2000000>;
+	reg-names = "control", "sideband";
+};

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

[Rob Herring]

On Tue, Mar 12, 2019 at 02:51:00PM +0530, Yash Shah wrote:
> DT documentation for L2 cache controller added.
> 
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  .../devicetree/bindings/edac/sifive-edac-l2.txt    | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/edac/sifive-edac-l2.txt
> 
> 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.

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

> +
> +- compatible: Should be "sifive,<chip>-ccache" and "sifive,ccache<version>".
> +  Supported compatible strings are:
> +  "sifive,fu540-c000-ccache" for the SiFive cache controller v0 as integrated
> +  onto the SiFive FU540 chip, and "sifive,ccache0" for the SiFive
> +  cache controller v0 IP block with no chip integration tweaks.
> +  Please refer to sifive-blocks-ip-versioning.txt for details
> +
> +- interrupts: Must contain 3 entries for FU540 (DirError, DataError, and
> +  DataFail signals) or 4 entries for other chips (DirError, DirFail, DataError,
> +  and DataFail signals)

3 or 4, but you only have 1 chip compatible defined?

> +
> +- interrupt-parent: Must be core interrupt controller

This is implied and could be in a parent node.

> +
> +- reg: Physical base address and size of L2 cache controller registers map
> +  A second range can indicate L2 Loosely Integrated Memory
> +
> +- reg-names: Names for the cells of reg, must contain "control" and "sideband"
> +
> +Example:
> +
> +cache-controller@2010000 {
> +	compatible = "sifive,fu540-c000-ccache", "sifive,ccache0";
> +	interrupt-parent = <&plic>;
> +	interrupts = <1 2 3>;
> +	reg = <0x0 0x2010000 0x0 0x1000 0x0 0x8000000 0x0 0x2000000>;
> +	reg-names = "control", "sideband";
> +};
> -- 
> 1.9.1
>

[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

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

[Rob Herring]

On Thu, Mar 28, 2019 at 1:47 PM James Morse <james.morse@arm.com> wrote:
>
> 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.

DT is not the only what to instantiate drivers. If the OS really wants
to have a single driver for multiple h/w blocks, then it needs to
instantiate a driver itself (based on the top-level compatible
probably) and then that driver can find the DT nodes it needs itself.

> 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.

I honestly don't understand the issue with EDAC is here. Highbank is
separate drivers for L2 ECC (PL310) and DDR. Both are used on
highbank. Only the DDR driver is used midway. (I think we never got
around to how to report A15 L2 ECC errors within Linux.)

In any case, it's all irrelevant to the DT binding. We don't design
bindings around what some particular OS wants.

Rob

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

[Borislav Petkov]

On Fri, Mar 29, 2019 at 09:11:24AM -0500, Rob Herring wrote:
> I honestly don't understand the issue with EDAC is here.

The EDAC core supports only one driver and if you need to load more, you
need to dance around that.

Also, if those drivers need to talk amongst each other, then they need
to build something ad-hoc so that they can.

And the other architectures can very well do one driver per platform -
only ARM wants to do this special thing because DT said so. Or whatever.

> Highbank is separate drivers for L2 ECC (PL310) and DDR. Both are used
> on highbank.

That's because your L2 driver does allocate an edac_device
(edac_device_alloc_ctl_info()) and the DDR one an edac_mc
(edac_mc_add_mc_with_groups).

For example, altera_edac does edac_device_alloc_ctl_info() for each IP
block just fine. So a single driver *can* work.

> Only the DDR driver is used midway. (I think we never got around to
> how to report A15 L2 ECC errors within Linux.)
>
> In any case, it's all irrelevant to the DT binding. We don't design
> bindings around what some particular OS wants.

And just because DT dictates one driver per IP block, I'm not going to
redesign EDAC to fit that scheme. You or someone else who feels strongly
about it, is more than welcome to do so, of course. And then maintain it
too.

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

[Rob Herring]

On Fri, Mar 29, 2019 at 9:27 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Mar 29, 2019 at 09:11:24AM -0500, Rob Herring wrote:
> > I honestly don't understand the issue with EDAC is here.
>
> The EDAC core supports only one driver and if you need to load more, you
> need to dance around that.
>
> Also, if those drivers need to talk amongst each other, then they need
> to build something ad-hoc so that they can.
>
> And the other architectures can very well do one driver per platform -
> only ARM wants to do this special thing because DT said so. Or whatever.
>
> > Highbank is separate drivers for L2 ECC (PL310) and DDR. Both are used
> > on highbank.
>
> That's because your L2 driver does allocate an edac_device
> (edac_device_alloc_ctl_info()) and the DDR one an edac_mc
> (edac_mc_add_mc_with_groups).
>
> For example, altera_edac does edac_device_alloc_ctl_info() for each IP
> block just fine. So a single driver *can* work.
>
> > Only the DDR driver is used midway. (I think we never got around to
> > how to report A15 L2 ECC errors within Linux.)
> >
> > In any case, it's all irrelevant to the DT binding. We don't design
> > bindings around what some particular OS wants.
>
> And just because DT dictates one driver per IP block, I'm not going to
> redesign EDAC to fit that scheme. You or someone else who feels strongly
> about it, is more than welcome to do so, of course. And then maintain it
> too.

DT dictates aligning with what the h/w looks like which has little to
do with OS driver design. I never said you should change EDAC and I
outlined how things should be handled if it is one driver. DT and OS
subsystems are independent things. I can't tell you how to design the
subsystem and you can't dictate DT design (based on EDAC design).

Rob

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

[Borislav Petkov]

On Fri, Mar 29, 2019 at 02:41:05PM -0500, Rob Herring wrote:
> DT dictates aligning with what the h/w looks like which has little to
> do with OS driver design.

Ok, then, where does this goal for doing a driver or compilation unit
per IP block come from?

Because everytime an ARM EDAC driver pops up, we are having the same
discussion.

> I never said you should change EDAC and I outlined how things should
> be handled if it is one driver.

Ok, we will add that to the EDAC driver design document we're currently
working on.

> DT and OS subsystems are independent things. I can't tell you how to
> design the subsystem and you can't dictate DT design (based on EDAC
> design).

I don't think I've ever intentionally or unintentionally dictated DT
design - all I've opposed to is having multiple EDAC drivers on ARM.

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

[James Morse]

Hi Rob,

On 29/03/2019 14:11, Rob Herring wrote:
> On Thu, Mar 28, 2019 at 1:47 PM James Morse <james.morse@arm.com> wrote:
>> 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.
> 
> DT is not the only what to instantiate drivers. If the OS really wants
> to have a single driver for multiple h/w blocks, then it needs to
> instantiate a driver itself (based on the top-level compatible
> probably) and then that driver can find the DT nodes it needs itself.

I think this is where we are heading. (but I need to get my head round this top-level thing).

Can the OS do both, depending on the platform?
e.g. on a system with one component the driver runs 'standalone', whereas on a bigger
system with multiple components the same driver is used as a library by something else.

I don't see how this would work if the common component's DT entry looks the same on both
platforms. Wouldn't this depend on the order stuff is done in, or 'but not this one'
checks in the driver?


> In any case, it's all irrelevant to the DT binding. We don't design
> bindings around what some particular OS wants.

I agree.

What we want to do is spot the problems on the horizon so we either have the right
information in the DT today, or at least know what it looks like so we don't cause a
regression when a new platform makes previous behaviour generic/a-library.


Thanks,

James

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

[Rob Herring]

On Fri, Mar 29, 2019 at 3:24 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Mar 29, 2019 at 02:41:05PM -0500, Rob Herring wrote:
> > DT dictates aligning with what the h/w looks like which has little to
> > do with OS driver design.
>
> Ok, then, where does this goal for doing a driver or compilation unit
> per IP block come from?
>
> Because everytime an ARM EDAC driver pops up, we are having the same
> discussion.
>
> > I never said you should change EDAC and I outlined how things should
> > be handled if it is one driver.
>
> Ok, we will add that to the EDAC driver design document we're currently
> working on.
>
> > DT and OS subsystems are independent things. I can't tell you how to
> > design the subsystem and you can't dictate DT design (based on EDAC
> > design).
>
> I don't think I've ever intentionally or unintentionally dictated DT
> design - all I've opposed to is having multiple EDAC drivers on ARM.

No, but folks just extend 1 driver to mean 1 DT node because that's
easy and certainly the more common case.

Rob

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

[Rob Herring]

On Mon, Apr 1, 2019 at 11:36 AM James Morse <james.morse@arm.com> wrote:
>
> Hi Rob,
>
> On 29/03/2019 14:11, Rob Herring wrote:
> > On Thu, Mar 28, 2019 at 1:47 PM James Morse <james.morse@arm.com> wrote:
> >> 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.
> >
> > DT is not the only what to instantiate drivers. If the OS really wants
> > to have a single driver for multiple h/w blocks, then it needs to
> > instantiate a driver itself (based on the top-level compatible
> > probably) and then that driver can find the DT nodes it needs itself.
>
> I think this is where we are heading. (but I need to get my head round this top-level thing).
>
> Can the OS do both, depending on the platform?
> e.g. on a system with one component the driver runs 'standalone', whereas on a bigger
> system with multiple components the same driver is used as a library by something else.
>
> I don't see how this would work if the common component's DT entry looks the same on both
> platforms. Wouldn't this depend on the order stuff is done in, or 'but not this one'
> checks in the driver?

Yeah, it could get a bit messy. I think we'd have to always do things
as described above for anything using that set of components. If you
truly have some set of multiple blocks and any combination of them can
appear, then we shouldn't be trying to have a single driver and EDAC
needs to change to support that IMO. However, I'd guess things are not
that stable to have many different combinations of components. SoCs
have new DDR controllers practically every generation for example.

> > In any case, it's all irrelevant to the DT binding. We don't design
> > bindings around what some particular OS wants.
>
> I agree.
>
> What we want to do is spot the problems on the horizon so we either have the right
> information in the DT today, or at least know what it looks like so we don't cause a
> regression when a new platform makes previous behaviour generic/a-library.

If we follow what the current OS wants, then what is right will change.

Rob