edac: Add APM X-Gene SoC EDAC driver

drivers/edac/xgene_edac.c new file +11 -0

+
+       rc = platform_driver_register(&xgene_edac_mc_driver);
+       if (rc) {
+               edac_printk(KERN_ERR, EDAC_MOD_STR, "MCU fails to register\n");
+               goto reg_mc_failed;
+       }
+       rc = platform_driver_register(&xgene_edac_pmd_driver);
+       if (rc) {
+               edac_printk(KERN_ERR, EDAC_MOD_STR, "PMD fails to register\n");
+               goto reg_pmd_failed;
+       }

Quoted 12 lines · On Tuesday 28 April 2015 16:10:44 Loc Ho wrote:

drivers/edac/xgene_edac.c new file +11 -0

+
+       rc = platform_driver_register(&xgene_edac_mc_driver);
+       if (rc) {
+               edac_printk(KERN_ERR, EDAC_MOD_STR, "MCU fails to register\n");
+               goto reg_mc_failed;
+       }
+       rc = platform_driver_register(&xgene_edac_pmd_driver);
+       if (rc) {
+               edac_printk(KERN_ERR, EDAC_MOD_STR, "PMD fails to register\n");
+               goto reg_pmd_failed;
+       }

drivers/edac/xgene_edac.c line 2143

Quoted 10 lines

drivers/edac/xgene_edac.c new file +10 -0

+       rc = platform_driver_register(&xgene_edac_l3_driver);
+       if (rc) {
+               edac_printk(KERN_ERR, EDAC_MOD_STR, "L3 fails to register\n");
+               goto reg_l3_failed;
+       }
+       rc = platform_driver_register(&xgene_edac_soc_driver);
+       if (rc) {
+               edac_printk(KERN_ERR, EDAC_MOD_STR, "SoC fails to register\n");
+               goto reg_soc_failed;
+       }

I had not looked at the driver before, but I have one comment now:

I think this can be simplified to registering a single platform_driver that just matches all four compatible strings, with an appropriate data:

Quoted 7 lines

Quoted 1 line

I had not looked at the driver before, but I have one comment now:

I think this can be simplified to registering a single platform_driver that just matches all four compatible strings, with an appropriate data:

Review

Quoted 7 lines

(quoted diff) +7 -0

+static struct of_device_id xgene_edac_of_match[] = {
+       { .compatible = "apm,xgene-edac-mcu", .data = &xgene_edac_mcu_data },
+       { .compatible = "apm,xgene-edac-pmd", .data = &xgene_edac_pmd_data },
+       { .compatible = "apm,xgene-edac-l3",  .data = &xgene_edac_l3_data },
+       { .compatible = "apm,xgene-edac-soc", .data = &xgene_edac_soc_data },
+       {},
+};

What exactly is shared here to combine all this into one driver? I would argue these are all independent functions and should be separate drivers. Add some library functions if there are common parts.

This patch adds support for the APM X-Gene SoC EDAC driver.

Signed-off-by Feng Kan <[email protected]>

Signed-off-by Loc Ho <[email protected]>

---
 drivers/edac/Kconfig      |    9 +-
 drivers/edac/Makefile     |    1 +
 drivers/edac/xgene_edac.c | 2175 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 2184 insertions(+), 1 deletions(-)
 create mode 100644 drivers/edac/xgene_edac.c

drivers/edac/Kconfig +0 -0

index cb59619..6289dd9 100644

This patch adds documentation for the APM X-Gene SoC EDAC DTS binding.

Signed-off-by Feng Kan <[email protected]>

Signed-off-by Loc Ho <[email protected]>

---
 .../devicetree/bindings/edac/apm-xgene-edac.txt    |  107 ++++++++++++++++++++
 1 files changed, 107 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt new file +4 -0

new file mode 100644
index 0000000..548299a
@@ -0,0 +1,107 @@
+* APM X-Gene SoC EDAC nodes
+
+EDAC nodes are defined to describe on-chip error detection and correction.
+There are four types of EDAC:

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt new file +85 -1

+
+- compatible           : Shall be "apm,xgene-edac-pmd".
+- regmap-pcp           : Regmap of the PCP resource.
+- regmap-efuse         : Regmap of the PMD efuse resource.
+- reg                  : First resource shall be the PMD resource.
+- interrupts            : Interrupt-specifier for PMD error IRQ(s).
+
+The following section describes the L3 DT node binding.
+
+- compatible           : Shall be "apm,xgene-edac-l3".
+- regmap-pcp           : Regmap of the PCP resource.
+- reg                  : First resource shall be the L3 resource.
+- interrupts            : Interrupt-specifier for L3 error IRQ(s).
+
+The following section describes the SoC DT node binding.
+
+- compatible           : Shall be "apm,xgene-edac-soc"".
+- regmap-pcp           : Regmap of the PCP resource.
+- reg                  : First resource shall be the SoC resource.
+                         Second resource shall be the register bus resource.
+- interrupts           : Interrupt-specifier for SoC error IRQ(s).
+
+Example:
+       efuse: efuse at 1054a000 {
+               compatible = "syscon";
+               reg = <0x0 0x1054a000 0x0 0x20>;
+       };
+
+       pcperror: pcperror at 78800000 {
+               compatible = "syscon";
+               reg = <0x0 0x78800000 0x0 0x100>;
+       };
+
+       csw: csw at 7e200000 {
+               compatible = "syscon";
+               reg = <0x0 0x7e200000 0x0 0x1000>;
+       };
+
+       mcba: mcba at 7e700000 {
+               compatible = "syscon";
+               reg = <0x0 0x7e700000 0x0 0x1000>;
+       };
+
+       mcbb: mcbb at 7e720000 {
+               compatible = "syscon";
+               reg = <0x0 0x7e720000 0x0 0x1000>;
+       };
+
+       edacmc0: edacmc0 at 7e800000 {
+               compatible = "apm,xgene-edac-mc";
+               regmap-pcp = <&pcperror>;
+               regmap-csw = <&csw>;
+               regmap-mcba = <&mcba>;
+               regmap-mcbb = <&mcbb>;
+               reg = <0x0 0x7e800000 0x0 0x1000>;
+               interrupts = <0x0 0x20 0x4>,
+                            <0x0 0x21 0x4>;
+       };
+
+       edacpmd0: edacpmd0 at 7c000000 {
+               compatible = "apm,xgene-edac-pmd";
+               regmap-pcp = <&pcperror>;
+               regmap-efuse = <&efuse>;
+               reg = <0x0 0x7c000000 0x0 0x200000>;
+               interrupts = <0x0 0x20 0x4>,
+                            <0x0 0x21 0x4>;
+       };
+
+       edacl3: edacl3 at 7e600000 {
+               compatible = "apm,xgene-edac-l3";
+               regmap-pcp = <&pcperror>;
+               reg = <0x0 0x7e600000 0x0 0x1000>;
+               interrupts = <0x0 0x20 0x4>,
+                            <0x0 0x21 0x4>;
+       };
+
+       edacsoc: edacsoc at 7e930000 {
+               compatible = "apm,xgene-edac-soc";
+               regmap-pcp = <&pcperror>;
+               reg = <0x0 0x7e930000 0x0 0x1000>,
+                     <0x0 0x7e000000 0x0 0x1000>;
+               interrupts = <0x0 0x20 0x4>,
+                            <0x0 0x21 0x4>,
+                            <0x0 0x27 0x4>;
+       };
--

1.7.1

Quoted 19 lines

 .../devicetree/bindings/edac/apm-xgene-edac.txt    |  107 ++++++++++++++++++++
 1 files changed, 107 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/apm-xgene-edac.txt

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt new file +9 -0

new file mode 100644
index 0000000..548299a
@@ -0,0 +1,107 @@
+* APM X-Gene SoC EDAC nodes
+
+EDAC nodes are defined to describe on-chip error detection and correction.
+There are four types of EDAC:
+
+  memory controller    - Memory controller
+  PMD (L1/L2)          - Processor module unit (PMD) L1/L2 cache
+  L3                   - CPU L3 cache
+  SoC                  - SoC IP such as SATA, Ethernet, and etc

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt line 14

Quoted 5 lines

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt new file +5 -0

+
+The following section describes the memory controller DT node binding.
+
+Required properties:
+- compatible           : Shall be "apm,xgene-edac-mc".

You are still describing what you want for EDAC driver not what the h/w looks like AFAICT.

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt line 18

Quoted 4 lines

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt new file +4 -0

+- regmap-pcp           : Regmap of the PCP resource.
+- regmap-csw           : Regmap of the CSW resource.
+- regmap-mcba          : Regmap of the MCB-A resource.
+- regmap-mcbb          : Regmap of the MCB-B resource.

What are these blocks? Regardless, I doubt the linkage here belongs in DT.

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt line 20

Quoted 3 lines

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt new file +2 -0

+- reg                  : First resource shall be the MCU resource.
+- interrupts            : Interrupt-specifier for MCU error IRQ(s).

What is MCU?

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt line 22

Quoted 3 lines

Documentation/devicetree/bindings/edac/apm-xgene-edac.txt new file +2 -0

+
+The following section describes the L1/L2 DT node binding.

Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest.

Quoted 2 lines

Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest.

Okay.. As comment in following emails, I will break up the driver into multiple drivers and focus only on the memory controller driver first.

Quoted 5 lines

Quoted 2 lines

Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest.

Okay.. As comment in following emails, I will break up the driver into multiple drivers and focus only on the memory controller driver first.

Please no multiple EDAC drivers. Or do you mean something else here?

Hi,

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest.

Okay.. As comment in following emails, I will break up the driver into multiple drivers and focus only on the memory controller driver first.

Please no multiple EDAC drivers. Or do you mean something else here?

We will have the following:

xgene-edac-mc.c
xgene-edac-pmd.c
xgene-edac-l3.c
xgene-edac-soc.c

Or what would you suggest.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest.

Okay.. As comment in following emails, I will break up the driver into multiple drivers and focus only on the memory controller driver first.

Please no multiple EDAC drivers. Or do you mean something else here?

Quoted 8 lines

We will have the following:

xgene-edac-mc.c
xgene-edac-pmd.c
xgene-edac-l3.c
xgene-edac-soc.c

Or what would you suggest.

xgene-edac.c

This granularity is insane. On x86 we have single EDAC drivers for multiple CPU families and platforms.

You can start carving out stuff when this is absolutely needed, i.e. when an IP block is in multiple hw incarnations. And even then I'd be sceptical and would really want to know whether it is even worth it to have an EDAC module for it.

So let's be conservative please.

Quoted 17 lines · Loc Ho

Hi,

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest.

Okay.. As comment in following emails, I will break up the driver into multiple drivers and focus only on the memory controller driver first.

Please no multiple EDAC drivers. Or do you mean something else here?

We will have the following:

xgene-edac-mc.c
xgene-edac-pmd.c
xgene-edac-l3.c
xgene-edac-soc.c

Quoted 1 line

Or what would you suggest.

xgene-edac.c

This granularity is insane. On x86 we have single EDAC drivers for multiple CPU families and platforms.

You can start carving out stuff when this is absolutely needed, i.e. when an IP block is in multiple hw incarnations. And even then I'd be sceptical and would really want to know whether it is even worth it to have an EDAC module for it.

I think that it is silly to group otherwise independent things
together and generally not what we do anywhere else in the kernel. They all likely have different capabilities and control mechanisms.

Quoted 3 lines · Rob Herring

I think that it is silly to group otherwise independent things
together and generally not what we do anywhere else in the kernel. They all likely have different capabilities and control mechanisms.

So let's look at the other extremity here: the moment someone releases yet another "independent" IP block with some RAS functionality, same someone will create yet another <vendor>_edac_<ip_block> driver. How many independent IP blocks are there with RAS functionality? 10, 20, 100...? That number is most likely growing, I'd bet.

Oh, and then there'll probably be functionality which is needed by two IP blocks so it needs to be shared. So we either copy'paste stuff or create a lib only for that functionality...

Even worse, what if two EDAC drivers for two IP blocks would need to talk to each other. That'll be fun.

Or there'll be a v2 of the IP block which has almost the same functionality but no 100% - just a *little* different. So then we go create <vendor>_edac_<ip_block-v2> driver. Yuck!

What I would prefer is to concentrate all vendor-specific RAS functionality in one single driver. Shared functionality is then taken care of automagically with all the synergies (I hate that word!) involved and no unnecessary too finer-grained splitting.

In that case, we would only have to enable loading more than one EDAC drivers on a system which has different RAS IP blocks. Now *that* is a much cleaner solution IMO which will keep the sanity in EDAC-land above 0.

The problem with your approach is that a lot of these blocks are not vendor specific. You will probably see a server chip that contains a memory controller from DesignWare, a cache controller from ARM, and some other device from the chip vendor, and each of them comes with EDAC support. Then you get three other vendors that have various combinations of the same memory controller, cache controller and other EDAC devices. Not all of these chips would have ARM CPU cores, some may be e.g. MIPS or PowerPC.

Quoted 8 lines · Arnd Bergmann

The problem with your approach is that a lot of these blocks are not vendor specific. You will probably see a server chip that contains a memory controller from DesignWare, a cache controller from ARM, and some other device from the chip vendor, and each of them comes with EDAC support. Then you get three other vendors that have various combinations of the same memory controller, cache controller and other EDAC devices. Not all of these chips would have ARM CPU cores, some may be e.g. MIPS or PowerPC.

I don't mean system-vendor specific but IP-block vendor specific. So doing a designware-edac, arm-edac, apm-edac and so on...

Also, I really want for people thinking of writing EDAC drivers to think hard before doing so. Whether it is even worth to expose *every* RAS functionality in some driver. And to consider that exposing it might cause more trouble than benefit.

We've had that experience on x86 with reporting innocuous DRAM ECC errors which were corrected by the hardware. People weren't even reading the error message and running around wanting to RMA their processors because something in dmesg screamed "Hardware Error".

And now APEI does the firmware-first thing where it gets to look at the error first and then decide to report it up to the OS or not.

Which is a good thing because in most cases it unnecessarily upsets the user.

So to get back on track: I think having the IP-block vendor stuff in one EDAC module would make this whole heterogeneity much more manageable.

On that system above, we will load - if I can count correctly - 6 EDAC drivers anyway. But the EDAC pile would remain sane.

Ok, but I'd also do multiple drivers for one vendor if they have several blocks that are clearly unrelated. My understanding was that this is the case here, but I have not looked too closely.

Note that a lot of vendors do not like to say who they licensed IP blocks from, or they are contractually required not to say it.

Yes, that is helpful feedback, but seems unrelated to the discussion about how to split this driver or not.

Ok, let me try to state what I think we agree on:

- For EDAC devices that are clearly unrelated (in particular, made by different vendors, but possibly part of the same chip), there should be one module per device type that registers one platform_driver.

- For EDAC drivers that are variants of one another (e.g. different generations of the same memory controller), we want one module with one platform_driver that can handle all variants.

Let me know if you disagree with the above. If that's fine, I think it leaves the case where we have one chip that has multiple EDAC blocks and we can't easily tell if those blocks are all inter-related or not. For this case, I would like to rely on your judgment as subsystem maintainer on whether the parts look related enough to have a single driver (with matching all device variants) or one driver for each class of device, as well as spotting cases where a part already has a driver from a different chip vendor that was licensing the same IP block.

Quoted 3 lines · Arnd Bergmann

Ok, but I'd also do multiple drivers for one vendor if they have several blocks that are clearly unrelated. My understanding was that this is the case here, but I have not looked too closely.

AFAIU, this is a driver for a bunch of xgene IP blocks.

Now, if someone integrates xgene memory controller in some other hw platform, we could still serve it with the same xgene-edac driver - it'll simply use only the memory controller part of the functionality.

Quoted 2 lines

Note that a lot of vendors do not like to say who they licensed IP blocks from, or they are contractually required not to say it.

Then there's that :-\

But the moment they supply an EDAC driver, one can recognize the IP block from the registers and functionality, no? Or do they change stuff a little bit so that it looks sufficiently different?

Quoted 10 lines

Ok, let me try to state what I think we agree on:

- For EDAC devices that are clearly unrelated (in particular, made by different vendors, but possibly part of the same chip), there should be one module per device type that registers one platform_driver.

- For EDAC drivers that are variants of one another (e.g. different generations of the same memory controller), we want one module with one platform_driver that can handle all variants.

Quoted 9 lines

Let me know if you disagree with the above. If that's fine, I think it leaves the case where we have one chip that has multiple EDAC blocks and we can't easily tell if those blocks are all inter-related or not. For this case, I would like to rely on your judgment as subsystem maintainer on whether the parts look related enough to have a single driver (with matching all device variants) or one driver for each class of device, as well as spotting cases where a part already has a driver from a different chip vendor that was licensing the same IP block.

Well, so the way I see it is, if we could map the EDAC drivers to the IP blocks. I.e., I'd like to have EDAC drivers mapped to the *IP* *blocks* vendor *instead* of the platform vendor.

We will have to be able to load multiple EDAC drivers anyway but it'll be much easier this way.

So let's have an example:

Let's say there's one xgene_edac driver which contains all the RAS functionality of xgene IP blocks. Memory controller, L3 cache, whatever...

Now, three *platform* vendors license, say, the xgene memory controller.

With my scheme, xgene_edac will be loaded on all those three vendors' Linux. Done.

But if those three vendors went and created an EDAC module each for their system, it would be a lot of repeated copy'pasting and bloat.

Now, the other dimension doesn't look better either:

xgene_edac_mc
xgene_edac_mc-v2
xgene_edac_l3
xgene_edac-l2
...

Also, in both cases, if any of those drivers need to talk to each other or know of one another in any way, the situation becomes really funny as they need to create something ad-hoc each time.

And this all can be avoided by having a single IP-blocks RAS driver.

Does that make more sense? Can it even be done with devicetree and all?

They don't normally change stuff intentionally, but you can have drivers for the same hardware that are written so differently that it becomes hard to spot that they are doing the same thing.

We have a bunch of XGMAC drivers for the same basic licensed IP block, each one configured a bit differently and extended with some soc-specific bits, and drivers written from scratch.

I would also not assume that people are trying hard to hide something from you, it's more that some guy is given the task to write a driver for an IP block, but is not told who made that, so the binding will just come with the name of the company he's working for.

Right, but they would then also have to load the code for the other blocks they did not license.

Quoted 5 lines

xgene_edac_mc
xgene_edac_mc-v2
xgene_edac_l3
xgene_edac-l2
...

Not following you here, what is the problem here?

Agreed. If the drivers need to talk to one another, that is a clear sign that the devices are not actually independent and that they really belong into a single driver. That's not the case I'm thinking of here.

The devicetree just describes the available hardware blocks. Typically you have one node for one block (e.g. a memory controller), and then have one driver for that block.

Things get a little funny if you want to have multiple drivers for the same device, e.g. you want a driver in drivers/memory/ to support autorefresh in sleep states and another driver in drivers/edac to support error handling. If you get something like this, you need some coordination between the two drivers, but that is unrelated to devicetree or not.

A platform driver should match against a set of similar devices that are identified by a "compatible" string, and we try to list the name of the ip block here, which could be something like "arm,pl12345rev1.2" if it is licensed from another company, or if that is not known, describe it by using the product number of the first chip it appeared in, e.g. "amcc,405ez-memory-controller". APM does not use numbers for their chips, so "apm,xgene-memory-controller" would refer to the memory controller used in the "xgene" chip. If they come out with a second product that uses the same one, they would use the same string, or use "apm,ygene-memory-controller" for the one that is in "ygene" if that is slightly different. The driver can then match against both and handle the differences based on the compatible string.

The point where it gets silly is when you try to handle multiple unrelated devices in the same driver and you get a probe function that just does:

apm_edac_probe(struct platform_device *dev)

{
	if (dev_is_xgene_mc(dev))
		return apm_probe_xgene_mc(dev);
	if (dev_is_xgene_l2(dev))
		return apm_probe_xgene_l2(dev);
	if (dev_is_ygene_mc(dev))
		...

Quoted 4 lines

The point where it gets silly is when you try to handle multiple unrelated devices in the same driver and you get a probe function that just does:

Ok, what if they're related through RAS?

I.e., xgene_edac contains all RAS-specific functionality of the xgene IP blocks. This doesn't sound silly to me, frankly, and it concentrates it all in one place. Communication between blocks is trivial and there's no code duplication.

Other vendors using those IP blocks would simply add new DT descriptions.

Quoted 8 lines

apm_edac_probe(struct platform_device *dev)

{
      if (dev_is_xgene_mc(dev))
              return apm_probe_xgene_mc(dev);
      if (dev_is_xgene_l2(dev))
              return apm_probe_xgene_l2(dev);
      if (dev_is_ygene_mc(dev))
              ...

So we do that on x86 - amd64_edac does AMD per-family setup and sb_edac too, for the last 4 Intel uarches. And it works just fine. So if "silly" is the only problem, I can certainly live with it.

But if those three vendors went and created an EDAC module each for their system, it would be a lot of repeated copy'pasting and bloat.

Now, the other dimension doesn't look better either:

xgene_edac_mc
xgene_edac_mc-v2
xgene_edac_l3
xgene_edac-l2
...

Also, in both cases, if any of those drivers need to talk to each other or know of one another in any way, the situation becomes really funny as they need to create something ad-hoc each time.

I've looked at the driver in a little more detail now, and found that it is a weird conglomerate at the moment. Basically you have four drivers in one file here, appended to one another, but not sharing any common functions except the module_init() that registers the four drivers. This is what Rob was referring to when he suggested splitting it up into four files, and these would in total be smaller than the common file (by being able to use module_platform_driver()).

However, there is another dimension to this, which supports your point about making it one driver for the platform: The split into four drivers is completely artificial, because all four use the same IRQs and implement four separate interrupt handlers for it (using IRQF_SHARED), each handling only the events they are interested in. Similarly, they all access the same register set ("pcperror"), aside from having their own separate registers, and they use the "syscon" framework to get to the registers. This seems to be an inferior design, as the pcperror

Quoted 8 lines

I've looked at the driver in a little more detail now, and found that it is a weird conglomerate at the moment. Basically you have four drivers in one file here, appended to one another, but not sharing any common functions except the module_init() that registers the four drivers. This is what Rob was referring to when he suggested splitting it up into four files, and these would in total be smaller than the common file (by being able to use module_platform_driver()).

Quoted 8 lines

However, there is another dimension to this, which supports your point about making it one driver for the platform: The split into four drivers is completely artificial, because all four use the same IRQs and implement four separate interrupt handlers for it (using IRQF_SHARED), each handling only the events they are interested in. Similarly, they all access the same register set ("pcperror"), aside from having their own separate registers, and they use the "syscon" framework to get to the registers. This seems to be an inferior design, as the pcperror

Doh, now that you mention it...

So why isn't this thing registering a single IRQ handler which multiplexes between the _check routines depending on the bits set in PCPHPERRINTSTS/MEMERRINTSTS? (L3 alternatively, ctx->dev_csr + L3C_ESR).

This would really make it a single driver which acts according to the bits set in those error registers. It can't get any simpler than that.

Loc?

I had read all the emails interaction. Yes I can write a single EDAC driver. But I actually have multiple instances and single instance of the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, one L3 and one SoC. If you can suggest to me how this would work with devicetree, I might be able to make it works. But from HW point of view (take the memory controller as an example), there is really 4 hardware blocks and some shared IP's block for status. It is much simple representation with multiple instance of the driver.

I think we should first agreed on the DT binding and let's not worry about APEI. Then, whether we have one file or multiple file. Again, the HW consist of:

1. One top level interrupt and status registers
2. CSW, MCB A and MCB B for detection of any active memory controllers
3. 4 individual memory controller instance
4. 4 CPU domain instance with its own individual L2 registers
5. Each CPU has instance own L1
6. One L3 instance
7. One SoC instance

As for as what is useful, we find that the memory controller and the L1 and L2 are useful as it provides info for errors - for HW/system debugging as well as margin of DDR.

Quoted 8 lines

I had read all the emails interaction. Yes I can write a single EDAC driver. But I actually have multiple instances and single instance of the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, one L3 and one SoC. If you can suggest to me how this would work with devicetree, I might be able to make it works. But from HW point of view (take the memory controller as an example), there is really 4 hardware blocks and some shared IP's block for status. It is much simple representation with multiple instance of the driver.

Your whole error reporting is done roughly through the same registers, as Arnd pointed out. Why can't you build your single driver around that and based on the error type being reported, multiplex inside a *single* interrupt handler to the proper decoding routines.

Quoted 5 lines

I think we should first agreed on the DT binding and let's not worry about APEI. Then, whether we have one file or multiple file. Again, the HW consist of:

Quoted 7 lines

1. One top level interrupt and status registers
2. CSW, MCB A and MCB B for detection of any active memory controllers
3. 4 individual memory controller instance
4. 4 CPU domain instance with its own individual L2 registers
5. Each CPU has instance own L1
6. One L3 instance
7. One SoC instance

And all of those are separate IP blocks and they can be integrated with other, non APM hardware, or do those things all belong together?

IOW,
can we expect such IP blocks to appear in other hw platforms or are they all custom APM design which need to go together due to proprietary coherency protocol or whatever, for example?

Quoted 4 lines

As for as what is useful, we find that the memory controller and the L1 and L2 are useful as it provides info for errors - for HW/system debugging as well as margin of DDR.

Sounds like all is useful.

The top level interrupt may be different and APM specific unless other vendors adapt the same bit definitions. I highly doubt other vendor will use the same bit definitions. The CSW is APM only. The MCB A, MCB B, and memory controller are APM only. The L3, and SoC are APM specify only. For L1 and L2, I will need to check with the CPU designer - but likely APM specific.

Quoted 6 lines

The top level interrupt may be different and APM specific unless other vendors adapt the same bit definitions. I highly doubt other vendor will use the same bit definitions. The CSW is APM only. The MCB A, MCB B, and memory controller are APM only. The L3, and SoC are APM specify only. For L1 and L2, I will need to check with the CPU designer - but likely APM specific.

So it sounds to me like this whole driver will control APM-specific hw so a single driver should be fine.

Hi,

Quoted 9 lines

Quoted 8 lines

I had read all the emails interaction. Yes I can write a single EDAC driver. But I actually have multiple instances and single instance of the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, one L3 and one SoC. If you can suggest to me how this would work with devicetree, I might be able to make it works. But from HW point of view (take the memory controller as an example), there is really 4 hardware blocks and some shared IP's block for status. It is much simple representation with multiple instance of the driver.

Quoted 4 lines

Your whole error reporting is done roughly through the same registers, as Arnd pointed out. Why can't you build your single driver around that and based on the error type being reported, multiplex inside a *single* interrupt handler to the proper decoding routines.

Yes... I can do this but I don't see how it will scale when some SoC in the future may have more or less instance of the memory controllers for example. I guess for now, I will just iteration 4 time with an single base address or four resources in a loop. Let's agreed on the DT layout and I will generate an new patch then. See below...

Quoted 5 lines

Quoted 5 lines

I think we should first agreed on the DT binding and let's not worry about APEI. Then, whether we have one file or multiple file. Again, the HW consist of:

1. One top level interrupt and status registers

Quoted 9 lines

Quoted 6 lines

2. CSW, MCB A and MCB B for detection of any active memory controllers
3. 4 individual memory controller instance
4. 4 CPU domain instance with its own individual L2 registers
5. Each CPU has instance own L1
6. One L3 instance
7. One SoC instance

And all of those are separate IP blocks and they can be integrated with other, non APM hardware, or do those things all belong together?

The top level interrupt may be different and APM specific unless other vendors adapt the same bit definitions. I highly doubt other vendor will use the same bit definitions. The CSW is APM only. The MCB A, MCB B, and memory controller are APM only. The L3, and SoC are APM specify only. For L1 and L2, I will need to check with the CPU designer - but likely APM specific.

Quoted 4 lines

IOW,
can we expect such IP blocks to appear in other hw platforms or are they all custom APM design which need to go together due to proprietary coherency protocol or whatever, for example?

See above.

Quoted 7 lines

Quoted 4 lines

As for as what is useful, we find that the memory controller and the L1 and L2 are useful as it provides info for errors - for HW/system debugging as well as margin of DDR.

Sounds like all is useful.

Now let us discuss the DT layout if it is an single driver:

xgeneedac: xgeneedac at 7e800000 {
        compatible = "apm,xgene-edac";
        regmap-efuse = <&efuse>; /* efuse */
        reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt
status resource */
                <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource
                <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */
                <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */
                <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */
                <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */
                <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */

                <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */
                <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */
                <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */
                <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */
                <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */
                <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */
                <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */
                <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */

      interrupts = <0x0 0x20 0x4>,
                       <0x0 0x21 0x4>,
                       <0x0 0x27 0x4>;

Quoted 13 lines

Now let us discuss the DT layout if it is an single driver:

xgeneedac: xgeneedac at 7e800000 {
        compatible = "apm,xgene-edac";
        regmap-efuse = <&efuse>; /* efuse */
        reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt
status resource */
                <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource
                <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */
                <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */
                <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */
                <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */
                <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */

Quoted 8 lines

                <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */
                <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */
                <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */
                <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */
                <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */
                <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */
                <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */
                <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */

Uggg, no. You were on the right track earlier in the thread. One node per instance of each block.

Quoted 22 lines

Quoted 21 lines

Now let us discuss the DT layout if it is an single driver:

xgeneedac: xgeneedac at 7e800000 {
        compatible = "apm,xgene-edac";
        regmap-efuse = <&efuse>; /* efuse */
        reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt
status resource */
                <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource
                <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */
                <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */
                <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */
                <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */
                <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */
                <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */
                <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */
                <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */
                <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */
                <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */
                <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */
                <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */
                <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */

Quoted 3 lines

Uggg, no. You were on the right track earlier in the thread. One node per instance of each block.

I agree these should be separate nodes, but I also think that we want a separate node for the pcp/pcperror/xgeneedac device. The compatible string for that should match whatever the datasheet calls that block, no idea why we now have the third name for that.

The specific parts could either be subnodes of the pcperror device, or they could be separate device nodes that reference the pcperror device through a phandle, so the driver can make the connection between them. This probably depends on what exactly all those nodes are:
if the registers in there are all exclusively related to EDAC handling of the pcperror, subnodes would be best, but if the registers also contain functionality that is not related to EDAC handling, we probably want to have separate top-level nodes that a driver could bind to, e.g. for doing power management on the memory controller.

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

I had read all the emails interaction. Yes I can write a single EDAC driver. But I actually have multiple instances and single instance of the same IP's. For example, I have 4 DDR controllers, 4 CPU domains, one L3 and one SoC. If you can suggest to me how this would work with devicetree, I might be able to make it works. But from HW point of view (take the memory controller as an example), there is really 4 hardware blocks and some shared IP's block for status. It is much simple representation with multiple instance of the driver.

Your whole error reporting is done roughly through the same registers, as Arnd pointed out. Why can't you build your single driver around that and based on the error type being reported, multiplex inside a *single* interrupt handler to the proper decoding routines.

Quoted 5 lines

Yes... I can do this but I don't see how it will scale when some SoC in the future may have more or less instance of the memory controllers for example. I guess for now, I will just iteration 4 time with an single base address or four resources in a loop. Let's agreed on the DT layout and I will generate an new patch then. See below...

You are right, it does not scale.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

I think we should first agreed on the DT binding and let's not worry about APEI. Then, whether we have one file or multiple file. Again, the HW consist of:

1. One top level interrupt and status registers

For these registers, are there ECC specific functions here or just normal interrupt control/status bits (mask/unmask/status)?

Assuming
the later, then you should make this block an interrupt-controller. Then this is the interrupt-parent for the rest of the blocks.

Quoted 46 lines

Quoted 10 lines

Quoted 7 lines

2. CSW, MCB A and MCB B for detection of any active memory controllers
3. 4 individual memory controller instance
4. 4 CPU domain instance with its own individual L2 registers
5. Each CPU has instance own L1
6. One L3 instance
7. One SoC instance

And all of those are separate IP blocks and they can be integrated with other, non APM hardware, or do those things all belong together?

The top level interrupt may be different and APM specific unless other vendors adapt the same bit definitions. I highly doubt other vendor will use the same bit definitions. The CSW is APM only. The MCB A, MCB B, and memory controller are APM only. The L3, and SoC are APM specify only. For L1 and L2, I will need to check with the CPU designer - but likely APM specific.

Quoted 4 lines

IOW,
can we expect such IP blocks to appear in other hw platforms or are they all custom APM design which need to go together due to proprietary coherency protocol or whatever, for example?

See above.

Quoted 7 lines

Quoted 4 lines

As for as what is useful, we find that the memory controller and the L1 and L2 are useful as it provides info for errors - for HW/system debugging as well as margin of DDR.

Sounds like all is useful.

Now let us discuss the DT layout if it is an single driver:

xgeneedac: xgeneedac at 7e800000 {
        compatible = "apm,xgene-edac";
        regmap-efuse = <&efuse>; /* efuse */
        reg = <0x0 0x78800000 0x0 0x100>, /* Top level interrupt
status resource */
                <0x0 0x7e200000 0x0 0x1000>, /* CSW for MCB active resource
                <0x0 0x7e700000 0x0 0x1000>, /* MCB A resource */
                <0x0 0x7e720000 0x0 0x1000>, /* MCB B resource */
                <0x0 0x7e800000 0x0 0x1000>, /* MCU 0 resource */
                <0x0 0x7e840000 0x0 0x1000>, /* MCU 1 resource */
                <0x0 0x7e880000 0x0 0x1000>, /* MCU 2 resource */

Quoted 8 lines

                <0x0 0x7e8c0000 0x0 0x1000>, /* MCU 3 resource */
                <0x0 0x7c000000 0x0 0x200000>, /* CPU 0 domain for L1 and L2 */
                <0x0 0x7c200000 0x0 0x200000>, /* CPU 1 domain for L1 and L2 */
                <0x0 0x7c400000 0x0 0x200000>, /* CPU 2 domain for L1 and L2 */
                <0x0 0x7c600000 0x0 0x200000>, /* CPU 3 domain for L1 and L2 */
                <0x0 0x7e600000 0x0 0x1000>, /* L3 resource */
                <0x0 0x7e930000 0x0 0x1000>, /* SoC bus resource */
                <0x0 0x7e000000 0x0 0x1000>, /* SoC device resource */

Uggg, no. You were on the right track earlier in the thread. One node per instance of each block.

Quoted 4 lines

      interrupts = <0x0 0x20 0x4>,
                       <0x0 0x21 0x4>,
                       <0x0 0x27 0x4>;

Can you describe what each of these interrupts do?

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I think we should first agreed on the DT binding and let's not worry about APEI. Then, whether we have one file or multiple file. Again, the HW consist of:

1. One top level interrupt and status registers

For these registers, are there ECC specific functions here or just normal interrupt control/status bits (mask/unmask/status)? Assuming the later, then you should make this block an interrupt-controller. Then this is the interrupt-parent for the rest of the blocks.

Hi Rob,

Quoted 11 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I think we should first agreed on the DT binding and let's not worry about APEI. Then, whether we have one file or multiple file. Again, the HW consist of:

1. One top level interrupt and status registers

For these registers, are there ECC specific functions here or just normal interrupt control/status bits (mask/unmask/status)? Assuming the later, then you should make this block an interrupt-controller. Then this is the interrupt-parent for the rest of the blocks.

This is the only item remain before I generate an patch with just the memory controller. Most of the code that I see are actually an interrupt controller HW. As it is just an interrupt mask and status registers, is there an example in Linux that I can model after? Also, I am not quite convince as to why we can't just share the interrupt and request it by each memory controller?

Quoted 14 lines · Loc Ho

Hi Rob,

Quoted 11 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I think we should first agreed on the DT binding and let's not worry about APEI. Then, whether we have one file or multiple file. Again, the HW consist of:

1. One top level interrupt and status registers

For these registers, are there ECC specific functions here or just normal interrupt control/status bits (mask/unmask/status)? Assuming the later, then you should make this block an interrupt-controller. Then this is the interrupt-parent for the rest of the blocks.

Quoted 6 lines

This is the only item remain before I generate an patch with just the memory controller. Most of the code that I see are actually an interrupt controller HW. As it is just an interrupt mask and status registers, is there an example in Linux that I can model after? Also, I am not quite convince as to why we can't just share the interrupt and request it by each memory controller?

Pretty much anything that calls irq_set_chained_handler. If you don't need to touch the shared registers and can set them up once at boot time, then you could just do shared irq handlers. But if you have to clear the interrupt within the PCP, you need a demuxer.

Hi,

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest.

Okay.. As comment in following emails, I will break up the driver into multiple drivers and focus only on the memory controller driver first.

Please no multiple EDAC drivers. Or do you mean something else here?

We will have the following:

xgene-edac-mc.c
xgene-edac-pmd.c
xgene-edac-l3.c
xgene-edac-soc.c

Or what would you suggest. There are the following HW:

1. 4 DDR controller with one shared top level interrupt, two shared memory bridges

2. 4 CPU's domain with one shared top level interrupt, shared L2 for two CPU's, and individual L1
3. 1 L3 with one shared top level interrupt

4. One SoC memory parity block with one shared top level interupt

Quoted 16 lines

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Similar comments for the rest. I would define memory controller bindings and EDAC driver, then worry about the rest.

Okay.. As comment in following emails, I will break up the driver into multiple drivers and focus only on the memory controller driver first.

Please no multiple EDAC drivers. Or do you mean something else here?

We will have the following:

xgene-edac-mc.c
xgene-edac-pmd.c
xgene-edac-l3.c
xgene-edac-soc.c

Quoted 4 lines

Or what would you suggest. There are the following HW:

1. 4 DDR controller with one shared top level interrupt, two shared memory bridges

And ECC registers are in the DDR controllers?

I would expect the DT to have 4 DDR controller nodes and probably 2 bridges. The memory bridges have control registers?

Quoted 4 lines

2. 4 CPU's domain with one shared top level interrupt, shared L2 for two CPU's, and individual L1
3. 1 L3 with one shared top level interrupt

Presumably the registers for ECC are wired to some other block and not CPU registers?

Quoted 2 lines

4. One SoC memory parity block with one shared top level interupt

Shared interrupts are easily supported, but implicit in the DT. Otherwise, it seems this block is independent from the rest, correct?

arch/arm64/include/asm/edac.h new file +7 -0

@@ -0,0 +1,31 @@
+/*
+ * ARM64 EDAC Header File 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *

Quoted 6 lines · Loc Ho

+/*
+ * ECC atomic, DMA, SMP and interrupt safe scrub function.
+ * Implements the per arch atomic_scrub() that EDAC use for software
+ * ECC scrubbing. It reads memory and then writes back the original
+ * value, allowing the hardware to detect and correct memory errors.
+ */

The comment here is misleading since the function doesn't do anything.

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/326986.html

Quoted 6 lines

+/*
+ * ECC atomic, DMA, SMP and interrupt safe scrub function.
+ * Implements the per arch atomic_scrub() that EDAC use for software
+ * ECC scrubbing. It reads memory and then writes back the original
+ * value, allowing the hardware to detect and correct memory errors.
+ */

The comment here is misleading since the function doesn't do anything.

arch/arm64/include/asm/edac.h line 28

Quoted 4 lines

arch/arm64/include/asm/edac.h new file +4 -0

+static inline void atomic_scrub(void *va, u32 size)
+{
+     /* Stub function for now until an ARM64 HW has a way to test it. */
+}

Can we have a WARN_ONCE() here?

We had some discussions last year, restarted this year and the conclusion was to leave this function as a no-op since it is not safe in the presence of KVM or non-coherent DMA:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/326986.html