Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Chris Packham]

On 23/06/17 02:37, Jan Lübbe wrote:
> Hi Chris,
> 
> On Fr, 2017-06-09 at 15:14 +0200, Jan Lübbe wrote:
>>> +static void mvebu_init_csrows(struct mem_ctl_info *mci,
>>> + struct mvebu_mc_pdata *pdata)
>> [...]
>>> + devtype = (ctl >> 20) & 0x3;
>>> + switch (devtype) {
>>> + case 0x0:
>>> + dimm->dtype = DEV_X32;
>>> + break;
>>> + case 0x2:   /* could be X8 too, but no way to tell
>> */
>>> + dimm->dtype = DEV_X16;
>>> + break;
>>> + case 0x3:
>>> + dimm->dtype = DEV_X4;
>>> + break;
>>> + default:
>>> + dimm->dtype = DEV_UNKNOWN;
>>> + break;
>>> + }
>> This register is documented as reserved? I pulled the X8/X16
>> information from the Address Control Register (CSnStruct bits).
> 
> Do you have more information on how to decode the Bus width?
> 
> It's not clear from the MV78230/78x60 docs:
> - The SDRAM Configuration Register, offset 15 bit is:
>0 = Half (32 bit data bus)
>1 = Full (64 bit data bus)

For the integrated version it still is just half and full but half == 16 
and full == 32.

> - The SDRAM Address Control Register, offsets 0-1, 4-5, 8-9 and 12-13
> (for CS 0, 1, 3 and 4):
>0 = X8
>1 = X16
>2 and 3 are not documented

This definition is the same for the integrated version.

> Is this clearer in your documentation, so that we can have the same code
> handle both variants? Otherwise, we'd probably need separate DT
> compatibles.

I think we do need to use the compatible string to decide how to 
interpret full/half.

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Chris Packham]

On 23/06/17 02:37, Jan Lübbe wrote:
> Hi Chris,
> 
> On Fr, 2017-06-09 at 15:14 +0200, Jan Lübbe wrote:
>>> +static void mvebu_init_csrows(struct mem_ctl_info *mci,
>>> + struct mvebu_mc_pdata *pdata)
>> [...]
>>> + devtype = (ctl >> 20) & 0x3;
>>> + switch (devtype) {
>>> + case 0x0:
>>> + dimm->dtype = DEV_X32;
>>> + break;
>>> + case 0x2:   /* could be X8 too, but no way to tell
>> */
>>> + dimm->dtype = DEV_X16;
>>> + break;
>>> + case 0x3:
>>> + dimm->dtype = DEV_X4;
>>> + break;
>>> + default:
>>> + dimm->dtype = DEV_UNKNOWN;
>>> + break;
>>> + }
>> This register is documented as reserved? I pulled the X8/X16
>> information from the Address Control Register (CSnStruct bits).
> 
> Do you have more information on how to decode the Bus width?
> 
> It's not clear from the MV78230/78x60 docs:
> - The SDRAM Configuration Register, offset 15 bit is:
>0 = Half (32 bit data bus)
>1 = Full (64 bit data bus)

For the integrated version it still is just half and full but half == 16 
and full == 32.

> - The SDRAM Address Control Register, offsets 0-1, 4-5, 8-9 and 12-13
> (for CS 0, 1, 3 and 4):
>0 = X8
>1 = X16
>2 and 3 are not documented

This definition is the same for the integrated version.

> Is this clearer in your documentation, so that we can have the same code
> handle both variants? Otherwise, we'd probably need separate DT
> compatibles.

I think we do need to use the compatible string to decide how to 
interpret full/half.

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Jan Lübbe]

Hi Chris,

On Fr, 2017-06-09 at 15:14 +0200, Jan Lübbe wrote:
> > +static void mvebu_init_csrows(struct mem_ctl_info *mci,
> > + struct mvebu_mc_pdata *pdata)
> [...]
> > + devtype = (ctl >> 20) & 0x3;
> > + switch (devtype) {
> > + case 0x0:
> > + dimm->dtype = DEV_X32;
> > + break;
> > + case 0x2:   /* could be X8 too, but no way to tell
> */
> > + dimm->dtype = DEV_X16;
> > + break;
> > + case 0x3:
> > + dimm->dtype = DEV_X4;
> > + break;
> > + default:
> > + dimm->dtype = DEV_UNKNOWN;
> > + break;
> > + }
> This register is documented as reserved? I pulled the X8/X16
> information from the Address Control Register (CSnStruct bits).

Do you have more information on how to decode the Bus width?

It's not clear from the MV78230/78x60 docs:
- The SDRAM Configuration Register, offset 15 bit is:
  0 = Half (32 bit data bus)
  1 = Full (64 bit data bus)
- The SDRAM Address Control Register, offsets 0-1, 4-5, 8-9 and 12-13
(for CS 0, 1, 3 and 4):
  0 = X8
  1 = X16
  2 and 3 are not documented

Is this clearer in your documentation, so that we can have the same code
handle both variants? Otherwise, we'd probably need separate DT
compatibles.

Regards,
Jan
-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Jan Lübbe]

Hi Chris,

On Fr, 2017-06-09 at 15:14 +0200, Jan Lübbe wrote:
> > +static void mvebu_init_csrows(struct mem_ctl_info *mci,
> > + struct mvebu_mc_pdata *pdata)
> [...]
> > + devtype = (ctl >> 20) & 0x3;
> > + switch (devtype) {
> > + case 0x0:
> > + dimm->dtype = DEV_X32;
> > + break;
> > + case 0x2:   /* could be X8 too, but no way to tell
> */
> > + dimm->dtype = DEV_X16;
> > + break;
> > + case 0x3:
> > + dimm->dtype = DEV_X4;
> > + break;
> > + default:
> > + dimm->dtype = DEV_UNKNOWN;
> > + break;
> > + }
> This register is documented as reserved? I pulled the X8/X16
> information from the Address Control Register (CSnStruct bits).

Do you have more information on how to decode the Bus width?

It's not clear from the MV78230/78x60 docs:
- The SDRAM Configuration Register, offset 15 bit is:
  0 = Half (32 bit data bus)
  1 = Full (64 bit data bus)
- The SDRAM Address Control Register, offsets 0-1, 4-5, 8-9 and 12-13
(for CS 0, 1, 3 and 4):
  0 = X8
  1 = X16
  2 and 3 are not documented

Is this clearer in your documentation, so that we can have the same code
handle both variants? Otherwise, we'd probably need separate DT
compatibles.

Regards,
Jan
-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Chris Packham]

On 10/06/17 01:19, Jan Lübbe wrote:
>> +
>> +if (edac_op_state == EDAC_OPSTATE_INT) {
>> +/* acquire interrupt that reports errors */
>> +pdata->irq = platform_get_irq(pdev, 0);
>> +res = devm_request_irq(>dev,
>> +   pdata->irq,
>> +   mvebu_mc_isr,
>> +   0,
>> +   "[EDAC] MC err",
>> +   mci);
> Which IRQ do you use? The current DT doesn't configure interrupts. Also
> it seems that the events are passed through additional layers of
> mask/status registers which are not yet represented in the Armada-XP IRQ
> hierarchy. So my driver currently uses polling.

Yes I'd been forcing polling too. To get this working properly I think 
we'd need to add another irqchip driver for the SoC Err interrupts. 
Which is kind of where I'd got stuck, the datasheet is a little 
confusing in that area. I think I'd figured out that the root interrupt 
comes through INT 4 which could be cascaded to the as yet unwritten SoC 
Err irqchip.

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Chris Packham]

On 10/06/17 01:19, Jan Lübbe wrote:
>> +
>> +if (edac_op_state == EDAC_OPSTATE_INT) {
>> +/* acquire interrupt that reports errors */
>> +pdata->irq = platform_get_irq(pdev, 0);
>> +res = devm_request_irq(>dev,
>> +   pdata->irq,
>> +   mvebu_mc_isr,
>> +   0,
>> +   "[EDAC] MC err",
>> +   mci);
> Which IRQ do you use? The current DT doesn't configure interrupts. Also
> it seems that the events are passed through additional layers of
> mask/status registers which are not yet represented in the Armada-XP IRQ
> hierarchy. So my driver currently uses polling.

Yes I'd been forcing polling too. To get this working properly I think 
we'd need to add another irqchip driver for the SoC Err interrupts. 
Which is kind of where I'd got stuck, the datasheet is a little 
confusing in that area. I think I'd figured out that the root interrupt 
comes through INT 4 which could be cascaded to the as yet unwritten SoC 
Err irqchip.

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Jan Lübbe]

Hi Chris!

On Do, 2017-06-08 at 16:11 +1200, Chris Packham wrote:
> This adds an EDAC driver for the memory controller and L2 cache used on
> a number of Marvell Armada SoCs.
Why have two separate drivers in the same file and enabled with the same
Kconfig option?

[...]
> +static void mvebu_mc_check(struct mem_ctl_info *mci)
> +{
> + struct mvebu_mc_pdata *pdata = mci->pvt_info;
> + u32 reg;
> + u32 err_addr;
> + u32 sdram_ecc;
> + u32 comp_ecc;
> + u32 syndrome;
> +
> + reg = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ADDR);
> + if (!reg)
> + return;
> +
> + err_addr = reg & ~0x3;
The ERR_ADDR register is not a physical address but contains multiple
fields (bank, col, chip select and error type).

> + sdram_ecc = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_RCVD);
> + comp_ecc = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CALC);
> + syndrome = sdram_ecc ^ comp_ecc;
> +
> + /* first bit clear in ECC Err Reg, 1 bit error, correctable by HW */
> + if (!(reg & 0x1))
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1,
> +  err_addr >> PAGE_SHIFT,
> +  err_addr & PAGE_MASK, syndrome,
> +  0, 0, -1,
> +  mci->ctl_name, "");
> + else/* 2 bit error, UE */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
> +  err_addr >> PAGE_SHIFT,
> +  err_addr & PAGE_MASK, 0,
> +  0, 0, -1,
> +  mci->ctl_name, "");
> +
> + /* clear the error */
> + writel(0, pdata->mc_vbase + MVEBU_SDRAM_ERR_ADDR);
This field is documented to be read-only. I had to write the Error
Interrupt Cause Register (0x14d0) or Message Interrupt Cause Register
(0x14d8) to allow the capture of new error details.

[...]
> +static void get_total_mem(struct mvebu_mc_pdata *pdata)
> +{
> + struct device_node *np = NULL;
> + struct resource res;
> + int ret;
> + unsigned long total_mem = 0;
> +
> + for_each_node_by_type(np, "memory") {
> + ret = of_address_to_resource(np, 0, );
> + if (ret)
> + continue;
> +
> + total_mem += resource_size();
> + }
> +
> + pdata->total_mem = total_mem;
> +}
I think we can calculate the sizes by reading back the individual CS
configuration registers.

> +static void mvebu_init_csrows(struct mem_ctl_info *mci,
> + struct mvebu_mc_pdata *pdata)
[...]
> + devtype = (ctl >> 20) & 0x3;
> + switch (devtype) {
> + case 0x0:
> + dimm->dtype = DEV_X32;
> + break;
> + case 0x2:   /* could be X8 too, but no way to tell */
> + dimm->dtype = DEV_X16;
> + break;
> + case 0x3:
> + dimm->dtype = DEV_X4;
> + break;
> + default:
> + dimm->dtype = DEV_UNKNOWN;
> + break;
> + }
This register is documented as reserved? I pulled the X8/X16 information
from the Address Control Register (CSnStruct bits).

> + dimm->edac_mode = EDAC_SECDED;
> +}

[...]
> + if (!devres_open_group(>dev, mvebu_mc_err_probe, GFP_KERNEL))
> + return -ENOMEM;
devres automatically opens a group per device, so this shouln't be
needed (although other EADC drivers do the same).

> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = 1;
> + layers[1].is_virt_csrow = false;
I'm not sure if this is needed. In my driver, only one CHIP_SELECT layer
with size 4 (max number of chip selects) is configured and seems to work
fine.

[...]
> + mci->scrub_mode = SCRUB_SW_SRC;
I'm not sure if this works as expected ARM as it is currently
implemented, but that's a topic for a different mail.

[...]
> + /* setup MC registers */
> + writel(0, pdata->mc_vbase + MVEBU_SDRAM_ERR_ADDR);
> + ctl = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CNTL);
> + ctl = (ctl & 0xff00) | 0x1;
> + writel(ctl, pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CNTL);
This configures the single bit error threshold to 1. My driver does the
same.

> +
> + if (edac_op_state == EDAC_OPSTATE_INT) {
> + /* acquire interrupt that reports errors */
> + pdata->irq = platform_get_irq(pdev, 0);
> + res = devm_request_irq(>dev,
> +pdata->irq,
> +mvebu_mc_isr,
> +0,
> +"[EDAC] MC err",
> +mci);
Which IRQ do you use? The current DT doesn't configure interrupts. Also
it seems that the events are passed through additional layers 

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Jan Lübbe]

Hi Chris!

On Do, 2017-06-08 at 16:11 +1200, Chris Packham wrote:
> This adds an EDAC driver for the memory controller and L2 cache used on
> a number of Marvell Armada SoCs.
Why have two separate drivers in the same file and enabled with the same
Kconfig option?

[...]
> +static void mvebu_mc_check(struct mem_ctl_info *mci)
> +{
> + struct mvebu_mc_pdata *pdata = mci->pvt_info;
> + u32 reg;
> + u32 err_addr;
> + u32 sdram_ecc;
> + u32 comp_ecc;
> + u32 syndrome;
> +
> + reg = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ADDR);
> + if (!reg)
> + return;
> +
> + err_addr = reg & ~0x3;
The ERR_ADDR register is not a physical address but contains multiple
fields (bank, col, chip select and error type).

> + sdram_ecc = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_RCVD);
> + comp_ecc = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CALC);
> + syndrome = sdram_ecc ^ comp_ecc;
> +
> + /* first bit clear in ECC Err Reg, 1 bit error, correctable by HW */
> + if (!(reg & 0x1))
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1,
> +  err_addr >> PAGE_SHIFT,
> +  err_addr & PAGE_MASK, syndrome,
> +  0, 0, -1,
> +  mci->ctl_name, "");
> + else/* 2 bit error, UE */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
> +  err_addr >> PAGE_SHIFT,
> +  err_addr & PAGE_MASK, 0,
> +  0, 0, -1,
> +  mci->ctl_name, "");
> +
> + /* clear the error */
> + writel(0, pdata->mc_vbase + MVEBU_SDRAM_ERR_ADDR);
This field is documented to be read-only. I had to write the Error
Interrupt Cause Register (0x14d0) or Message Interrupt Cause Register
(0x14d8) to allow the capture of new error details.

[...]
> +static void get_total_mem(struct mvebu_mc_pdata *pdata)
> +{
> + struct device_node *np = NULL;
> + struct resource res;
> + int ret;
> + unsigned long total_mem = 0;
> +
> + for_each_node_by_type(np, "memory") {
> + ret = of_address_to_resource(np, 0, );
> + if (ret)
> + continue;
> +
> + total_mem += resource_size();
> + }
> +
> + pdata->total_mem = total_mem;
> +}
I think we can calculate the sizes by reading back the individual CS
configuration registers.

> +static void mvebu_init_csrows(struct mem_ctl_info *mci,
> + struct mvebu_mc_pdata *pdata)
[...]
> + devtype = (ctl >> 20) & 0x3;
> + switch (devtype) {
> + case 0x0:
> + dimm->dtype = DEV_X32;
> + break;
> + case 0x2:   /* could be X8 too, but no way to tell */
> + dimm->dtype = DEV_X16;
> + break;
> + case 0x3:
> + dimm->dtype = DEV_X4;
> + break;
> + default:
> + dimm->dtype = DEV_UNKNOWN;
> + break;
> + }
This register is documented as reserved? I pulled the X8/X16 information
from the Address Control Register (CSnStruct bits).

> + dimm->edac_mode = EDAC_SECDED;
> +}

[...]
> + if (!devres_open_group(>dev, mvebu_mc_err_probe, GFP_KERNEL))
> + return -ENOMEM;
devres automatically opens a group per device, so this shouln't be
needed (although other EADC drivers do the same).

> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = 1;
> + layers[1].is_virt_csrow = false;
I'm not sure if this is needed. In my driver, only one CHIP_SELECT layer
with size 4 (max number of chip selects) is configured and seems to work
fine.

[...]
> + mci->scrub_mode = SCRUB_SW_SRC;
I'm not sure if this works as expected ARM as it is currently
implemented, but that's a topic for a different mail.

[...]
> + /* setup MC registers */
> + writel(0, pdata->mc_vbase + MVEBU_SDRAM_ERR_ADDR);
> + ctl = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CNTL);
> + ctl = (ctl & 0xff00) | 0x1;
> + writel(ctl, pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CNTL);
This configures the single bit error threshold to 1. My driver does the
same.

> +
> + if (edac_op_state == EDAC_OPSTATE_INT) {
> + /* acquire interrupt that reports errors */
> + pdata->irq = platform_get_irq(pdev, 0);
> + res = devm_request_irq(>dev,
> +pdata->irq,
> +mvebu_mc_isr,
> +0,
> +"[EDAC] MC err",
> +mci);
Which IRQ do you use? The current DT doesn't configure interrupts. Also
it seems that the events are passed through additional layers 

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Borislav Petkov]

On Thu, Jun 08, 2017 at 04:11:21PM +1200, Chris Packham wrote:
> This adds an EDAC driver for the memory controller and L2 cache used on
> a number of Marvell Armada SoCs.
> 
> Signed-off-by: Chris Packham 
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  drivers/edac/Kconfig  |   7 +
>  drivers/edac/Makefile |   1 +
>  drivers/edac/mvebu_edac.c | 506 
> ++
>  drivers/edac/mvebu_edac.h |  66 ++
>  4 files changed, 580 insertions(+)
>  create mode 100644 drivers/edac/mvebu_edac.c
>  create mode 100644 drivers/edac/mvebu_edac.h

...

> diff --git a/drivers/edac/mvebu_edac.c b/drivers/edac/mvebu_edac.c
> new file mode 100644
> index ..624cf10f821b
> --- /dev/null
> +++ b/drivers/edac/mvebu_edac.c
> @@ -0,0 +1,506 @@
> +/*
> + * EDAC driver for Marvell ARM SoCs
> + *
> + * Copyright (C) 2017 Allied Telesis Labs
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

We have all those fancy edac_printk() macros, no need to use pr_* ones.

...

> +static int mvebu_mc_err_probe(struct platform_device *pdev)
> +{
> + struct mem_ctl_info *mci;
> + struct edac_mc_layer layers[2];
> + struct mvebu_mc_pdata *pdata;
> + struct resource *r;
> + u32 ctl;
> + int res = 0;
> +
> + if (!devres_open_group(>dev, mvebu_mc_err_probe, GFP_KERNEL))
> + return -ENOMEM;
> +
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = 1;
> + layers[1].is_virt_csrow = false;
> + mci = edac_mc_alloc(edac_mc_idx, ARRAY_SIZE(layers), layers,
> + sizeof(struct mvebu_mc_pdata));
> + if (!mci) {
> + pr_err("%s: No memory for CPU err\n", __func__);
> + devres_release_group(>dev, mvebu_mc_err_probe);
> + return -ENOMEM;
> + }

Make that call after all platform_get_resource(),
devm_ioremap_resource() so that you can save yourself the unwinding
"goto err" and return directly then.

> +
> + pdata = mci->pvt_info;
> + mci->pdev = >dev;
> + platform_set_drvdata(pdev, mci);
> + pdata->name = "mvebu_mc_err";
> + mci->dev_name = dev_name(>dev);
> + pdata->edac_idx = edac_mc_idx++;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r) {
> + pr_err("%s: Unable to get resource for MC err regs\n",
> +__func__);
> + res = -ENOENT;
> + goto err;
> + }
> +
> + pdata->mc_vbase = devm_ioremap_resource(>dev, r);
> + if (IS_ERR(pdata->mc_vbase)) {
> + pr_err("%s: Unable to setup MC err regs\n", __func__);
> + res = PTR_ERR(pdata->mc_vbase);
> + goto err;
> + }
> +
> + ctl = readl(pdata->mc_vbase + MVEBU_SDRAM_CONFIG);
> + if (!(ctl & MVEBU_SDRAM_ECC)) {
> + /* Non-ECC RAM? */
> + pr_warn("%s: No ECC DIMMs discovered\n", __func__);
> + res = -ENODEV;
> + goto err;
> + }
> +
> + edac_dbg(3, "init mci\n");
> + mci->mtype_cap = MEM_FLAG_RDDR | MEM_FLAG_DDR;
> + mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
> + mci->edac_cap = EDAC_FLAG_SECDED;
> + mci->mod_name = EDAC_MOD_STR;
> + mci->mod_ver = MVEBU_REVISION;
> + mci->ctl_name = mvebu_ctl_name;
> +
> + if (edac_op_state == EDAC_OPSTATE_POLL)
> + mci->edac_check = mvebu_mc_check;
> +
> + mci->ctl_page_to_phys = NULL;
> +
> + mci->scrub_mode = SCRUB_SW_SRC;
> +
> + mvebu_init_csrows(mci, pdata);
> +
> + /* setup MC registers */
> + writel(0, pdata->mc_vbase + MVEBU_SDRAM_ERR_ADDR);
> + ctl = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CNTL);
> + ctl = (ctl & 0xff00) | 0x1;
> + writel(ctl, pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CNTL);
> +
> + if (edac_op_state == EDAC_OPSTATE_INT) {
> + /* acquire interrupt that reports errors */
> + pdata->irq = platform_get_irq(pdev, 0);
> + res = devm_request_irq(>dev,
> +pdata->irq,
> +mvebu_mc_isr,
> +0,
> +"[EDAC] MC err",
> +mci);
> + if (res < 0) {
> + pr_err("%s: Unable to request irq %d\n", 

Re: [RFC PATCH 1/4] EDAC: mvebu: Add driver for Marvell Armada SoCs

[Borislav Petkov]

On Thu, Jun 08, 2017 at 04:11:21PM +1200, Chris Packham wrote:
> This adds an EDAC driver for the memory controller and L2 cache used on
> a number of Marvell Armada SoCs.
> 
> Signed-off-by: Chris Packham 
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  drivers/edac/Kconfig  |   7 +
>  drivers/edac/Makefile |   1 +
>  drivers/edac/mvebu_edac.c | 506 
> ++
>  drivers/edac/mvebu_edac.h |  66 ++
>  4 files changed, 580 insertions(+)
>  create mode 100644 drivers/edac/mvebu_edac.c
>  create mode 100644 drivers/edac/mvebu_edac.h

...

> diff --git a/drivers/edac/mvebu_edac.c b/drivers/edac/mvebu_edac.c
> new file mode 100644
> index ..624cf10f821b
> --- /dev/null
> +++ b/drivers/edac/mvebu_edac.c
> @@ -0,0 +1,506 @@
> +/*
> + * EDAC driver for Marvell ARM SoCs
> + *
> + * Copyright (C) 2017 Allied Telesis Labs
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

We have all those fancy edac_printk() macros, no need to use pr_* ones.

...

> +static int mvebu_mc_err_probe(struct platform_device *pdev)
> +{
> + struct mem_ctl_info *mci;
> + struct edac_mc_layer layers[2];
> + struct mvebu_mc_pdata *pdata;
> + struct resource *r;
> + u32 ctl;
> + int res = 0;
> +
> + if (!devres_open_group(>dev, mvebu_mc_err_probe, GFP_KERNEL))
> + return -ENOMEM;
> +
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = 1;
> + layers[1].is_virt_csrow = false;
> + mci = edac_mc_alloc(edac_mc_idx, ARRAY_SIZE(layers), layers,
> + sizeof(struct mvebu_mc_pdata));
> + if (!mci) {
> + pr_err("%s: No memory for CPU err\n", __func__);
> + devres_release_group(>dev, mvebu_mc_err_probe);
> + return -ENOMEM;
> + }

Make that call after all platform_get_resource(),
devm_ioremap_resource() so that you can save yourself the unwinding
"goto err" and return directly then.

> +
> + pdata = mci->pvt_info;
> + mci->pdev = >dev;
> + platform_set_drvdata(pdev, mci);
> + pdata->name = "mvebu_mc_err";
> + mci->dev_name = dev_name(>dev);
> + pdata->edac_idx = edac_mc_idx++;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r) {
> + pr_err("%s: Unable to get resource for MC err regs\n",
> +__func__);
> + res = -ENOENT;
> + goto err;
> + }
> +
> + pdata->mc_vbase = devm_ioremap_resource(>dev, r);
> + if (IS_ERR(pdata->mc_vbase)) {
> + pr_err("%s: Unable to setup MC err regs\n", __func__);
> + res = PTR_ERR(pdata->mc_vbase);
> + goto err;
> + }
> +
> + ctl = readl(pdata->mc_vbase + MVEBU_SDRAM_CONFIG);
> + if (!(ctl & MVEBU_SDRAM_ECC)) {
> + /* Non-ECC RAM? */
> + pr_warn("%s: No ECC DIMMs discovered\n", __func__);
> + res = -ENODEV;
> + goto err;
> + }
> +
> + edac_dbg(3, "init mci\n");
> + mci->mtype_cap = MEM_FLAG_RDDR | MEM_FLAG_DDR;
> + mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
> + mci->edac_cap = EDAC_FLAG_SECDED;
> + mci->mod_name = EDAC_MOD_STR;
> + mci->mod_ver = MVEBU_REVISION;
> + mci->ctl_name = mvebu_ctl_name;
> +
> + if (edac_op_state == EDAC_OPSTATE_POLL)
> + mci->edac_check = mvebu_mc_check;
> +
> + mci->ctl_page_to_phys = NULL;
> +
> + mci->scrub_mode = SCRUB_SW_SRC;
> +
> + mvebu_init_csrows(mci, pdata);
> +
> + /* setup MC registers */
> + writel(0, pdata->mc_vbase + MVEBU_SDRAM_ERR_ADDR);
> + ctl = readl(pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CNTL);
> + ctl = (ctl & 0xff00) | 0x1;
> + writel(ctl, pdata->mc_vbase + MVEBU_SDRAM_ERR_ECC_CNTL);
> +
> + if (edac_op_state == EDAC_OPSTATE_INT) {
> + /* acquire interrupt that reports errors */
> + pdata->irq = platform_get_irq(pdev, 0);
> + res = devm_request_irq(>dev,
> +pdata->irq,
> +mvebu_mc_isr,
> +0,
> +"[EDAC] MC err",
> +mci);
> + if (res < 0) {
> + pr_err("%s: Unable to request irq %d\n", __func__,
> +