Re: [PATCH 3/3] EDAC: mv64x60: replace in_le32/out_le32 with ioread32/iowrite32

From: Chris Packham
Date: Wed May 17 2017 - 17:17:08 EST

Next message: Gustavo A. R. Silva: "[PATCH] media: platform: coda: remove variable self assignment"
Previous message: Boris Ostrovsky: "Re: [patch V2 16/17] init: Introduce SYSTEM_SCHEDULING state"
In reply to: Borislav Petkov: "Re: [PATCH 3/3] EDAC: mv64x60: replace in_le32/out_le32 with ioread32/iowrite32"
Next in thread: Arnd Bergmann: "Re: [PATCH 3/3] EDAC: mv64x60: replace in_le32/out_le32 with ioread32/iowrite32"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 18/05/17 06:18, Borislav Petkov wrote:
> Top-posting so that the PPC list can see the whole patch below.
>
> Since I don't know PPC, let me add PPC ML to CC for a confirmation this
> change is correct.
>
> Which brings me to the tangential: this driver is from 2006-ish and
> is for some "Marvell MV64x60 Memory Controller kernel module for PPC
> platforms".

Many of the peripherals from the PPC MV64x60 were brought through to the
ARM Orion, Kirkwood, and Armada SoCs (e.g. mv643xx_eth.c,
i2c-mv64xxx.c). I believe the memory controller ECC is also in the same
category.

I haven't looked into the cache/pci controller aspects of the
mv64x60_edac.c driver yet.

> If you're going to touch it, then you should test on the PPC
> hardware too, so that you don't break the driver there.

I don't disagree. I however don't have access to any such hardware (I've
got 2 armada variant boards that have ECC). Maybe someone on the PPC
list can help me out?

One thing I would like confirmation on is is in_le32 -> ioread32 the
correct change? I tossed up between ioread32 and readl. Looking at
mv643xx_eth.c which supports both the MV643XX and Orion it's using readl
so perhaps I should be using that.

> Unless that hardware is obsolete now and we don't care and, and ..?

MV64x60 is pretty old. I considered gutting mv64x60_edac.c to make a
separate driver but that would just be more code to maintain.

> Maybe someone has some insights...
>
> On Fri, May 12, 2017 at 04:20:02PM +1200, Chris Packham wrote:
>> To allow this driver to be used on non-powerpc platforms it needs to use
>> io accessors suitable for all platforms.
>>
>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/edac/mv64x60_edac.c | 84 ++++++++++++++++++++++-----------------------
>> 1 file changed, 42 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c
>> index ddc5082f7577..102ec29f864b 100644
>> --- a/drivers/edac/mv64x60_edac.c
>> +++ b/drivers/edac/mv64x60_edac.c
>> @@ -32,21 +32,21 @@ static void mv64x60_pci_check(struct edac_pci_ctl_info *pci)
>> struct mv64x60_pci_pdata *pdata = pci->pvt_info;
>> u32 cause;
>>
>> - cause = in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>> + cause = ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>> if (!cause)
>> return;
>>
>> printk(KERN_ERR "Error in PCI %d Interface\n", pdata->pci_hose);
>> printk(KERN_ERR "Cause register: 0x%08x\n", cause);
>> printk(KERN_ERR "Address Low: 0x%08x\n",
>> - in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_ADDR_LO));
>> + ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_ADDR_LO));
>> printk(KERN_ERR "Address High: 0x%08x\n",
>> - in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_ADDR_HI));
>> + ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_ADDR_HI));
>> printk(KERN_ERR "Attribute: 0x%08x\n",
>> - in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_ATTR));
>> + ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_ATTR));
>> printk(KERN_ERR "Command: 0x%08x\n",
>> - in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CMD));
>> - out_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE, ~cause);
>> + ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_CMD));
>> + iowrite32(~cause, pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>>
>> if (cause & MV64X60_PCI_PE_MASK)
>> edac_pci_handle_pe(pci, pci->ctl_name);
>> @@ -61,7 +61,7 @@ static irqreturn_t mv64x60_pci_isr(int irq, void *dev_id)
>> struct mv64x60_pci_pdata *pdata = pci->pvt_info;
>> u32 val;
>>
>> - val = in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>> + val = ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>> if (!val)
>> return IRQ_NONE;
>>
>> @@ -93,7 +93,7 @@ static int __init mv64x60_pci_fixup(struct platform_device *pdev)
>> if (!pci_serr)
>> return -ENOMEM;
>>
>> - out_le32(pci_serr, in_le32(pci_serr) & ~0x1);
>> + iowrite32(ioread32(pci_serr) & ~0x1, pci_serr);
>> iounmap(pci_serr);
>>
>> return 0;
>> @@ -161,10 +161,10 @@ static int mv64x60_pci_err_probe(struct platform_device *pdev)
>> goto err;
>> }
>>
>> - out_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE, 0);
>> - out_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_MASK, 0);
>> - out_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_MASK,
>> - MV64X60_PCIx_ERR_MASK_VAL);
>> + iowrite32(0, pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>> + iowrite32(0, pdata->pci_vbase + MV64X60_PCI_ERROR_MASK);
>> + iowrite32(MV64X60_PCIx_ERR_MASK_VAL,
>> + pdata->pci_vbase + MV64X60_PCI_ERROR_MASK);
>>
>> if (edac_pci_add_device(pci, pdata->edac_idx) > 0) {
>> edac_dbg(3, "failed edac_pci_add_device()\n");
>> @@ -233,23 +233,23 @@ static void mv64x60_sram_check(struct edac_device_ctl_info *edac_dev)
>> struct mv64x60_sram_pdata *pdata = edac_dev->pvt_info;
>> u32 cause;
>>
>> - cause = in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>> + cause = ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>> if (!cause)
>> return;
>>
>> printk(KERN_ERR "Error in internal SRAM\n");
>> printk(KERN_ERR "Cause register: 0x%08x\n", cause);
>> printk(KERN_ERR "Address Low: 0x%08x\n",
>> - in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_ADDR_LO));
>> + ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_ADDR_LO));
>> printk(KERN_ERR "Address High: 0x%08x\n",
>> - in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_ADDR_HI));
>> + ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_ADDR_HI));
>> printk(KERN_ERR "Data Low: 0x%08x\n",
>> - in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_DATA_LO));
>> + ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_DATA_LO));
>> printk(KERN_ERR "Data High: 0x%08x\n",
>> - in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_DATA_HI));
>> + ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_DATA_HI));
>> printk(KERN_ERR "Parity: 0x%08x\n",
>> - in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_PARITY));
>> - out_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE, 0);
>> + ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_PARITY));
>> + iowrite32(0, pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>>
>> edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name);
>> }
>> @@ -260,7 +260,7 @@ static irqreturn_t mv64x60_sram_isr(int irq, void *dev_id)
>> struct mv64x60_sram_pdata *pdata = edac_dev->pvt_info;
>> u32 cause;
>>
>> - cause = in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>> + cause = ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>> if (!cause)
>> return IRQ_NONE;
>>
>> @@ -322,7 +322,7 @@ static int mv64x60_sram_err_probe(struct platform_device *pdev)
>> }
>>
>> /* setup SRAM err registers */
>> - out_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE, 0);
>> + iowrite32(0, pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>>
>> edac_dev->mod_name = EDAC_MOD_STR;
>> edac_dev->ctl_name = pdata->name;
>> @@ -398,7 +398,7 @@ static void mv64x60_cpu_check(struct edac_device_ctl_info *edac_dev)
>> struct mv64x60_cpu_pdata *pdata = edac_dev->pvt_info;
>> u32 cause;
>>
>> - cause = in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE) &
>> + cause = ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE) &
>> MV64x60_CPU_CAUSE_MASK;
>> if (!cause)
>> return;
>> @@ -406,16 +406,16 @@ static void mv64x60_cpu_check(struct edac_device_ctl_info *edac_dev)
>> printk(KERN_ERR "Error on CPU interface\n");
>> printk(KERN_ERR "Cause register: 0x%08x\n", cause);
>> printk(KERN_ERR "Address Low: 0x%08x\n",
>> - in_le32(pdata->cpu_vbase[0] + MV64x60_CPU_ERR_ADDR_LO));
>> + ioread32(pdata->cpu_vbase[0] + MV64x60_CPU_ERR_ADDR_LO));
>> printk(KERN_ERR "Address High: 0x%08x\n",
>> - in_le32(pdata->cpu_vbase[0] + MV64x60_CPU_ERR_ADDR_HI));
>> + ioread32(pdata->cpu_vbase[0] + MV64x60_CPU_ERR_ADDR_HI));
>> printk(KERN_ERR "Data Low: 0x%08x\n",
>> - in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_DATA_LO));
>> + ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_DATA_LO));
>> printk(KERN_ERR "Data High: 0x%08x\n",
>> - in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_DATA_HI));
>> + ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_DATA_HI));
>> printk(KERN_ERR "Parity: 0x%08x\n",
>> - in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_PARITY));
>> - out_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE, 0);
>> + ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_PARITY));
>> + iowrite32(0, pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE);
>>
>> edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name);
>> }
>> @@ -426,7 +426,7 @@ static irqreturn_t mv64x60_cpu_isr(int irq, void *dev_id)
>> struct mv64x60_cpu_pdata *pdata = edac_dev->pvt_info;
>> u32 cause;
>>
>> - cause = in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE) &
>> + cause = ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE) &
>> MV64x60_CPU_CAUSE_MASK;
>> if (!cause)
>> return IRQ_NONE;
>> @@ -515,9 +515,9 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
>> }
>>
>> /* setup CPU err registers */
>> - out_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE, 0);
>> - out_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_MASK, 0);
>> - out_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_MASK, 0x000000ff);
>> + iowrite32(0, pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE);
>> + iowrite32(0, pdata->cpu_vbase[1] + MV64x60_CPU_ERR_MASK);
>> + iowrite32(0x000000ff, pdata->cpu_vbase[1] + MV64x60_CPU_ERR_MASK);
>>
>> edac_dev->mod_name = EDAC_MOD_STR;
>> edac_dev->ctl_name = pdata->name;
>> @@ -596,13 +596,13 @@ static void mv64x60_mc_check(struct mem_ctl_info *mci)
>> u32 comp_ecc;
>> u32 syndrome;
>>
>> - reg = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> + reg = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> if (!reg)
>> return;
>>
>> err_addr = reg & ~0x3;
>> - sdram_ecc = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_RCVD);
>> - comp_ecc = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CALC);
>> + sdram_ecc = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_RCVD);
>> + comp_ecc = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CALC);
>> syndrome = sdram_ecc ^ comp_ecc;
>>
>> /* first bit clear in ECC Err Reg, 1 bit error, correctable by HW */
>> @@ -620,7 +620,7 @@ static void mv64x60_mc_check(struct mem_ctl_info *mci)
>> mci->ctl_name, "");
>>
>> /* clear the error */
>> - out_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR, 0);
>> + iowrite32(0, pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> }
>>
>> static irqreturn_t mv64x60_mc_isr(int irq, void *dev_id)
>> @@ -629,7 +629,7 @@ static irqreturn_t mv64x60_mc_isr(int irq, void *dev_id)
>> struct mv64x60_mc_pdata *pdata = mci->pvt_info;
>> u32 reg;
>>
>> - reg = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> + reg = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> if (!reg)
>> return IRQ_NONE;
>>
>> @@ -664,7 +664,7 @@ static void mv64x60_init_csrows(struct mem_ctl_info *mci,
>>
>> get_total_mem(pdata);
>>
>> - ctl = in_le32(pdata->mc_vbase + MV64X60_SDRAM_CONFIG);
>> + ctl = ioread32(pdata->mc_vbase + MV64X60_SDRAM_CONFIG);
>>
>> csrow = mci->csrows[0];
>> dimm = csrow->channels[0]->dimm;
>> @@ -753,7 +753,7 @@ static int mv64x60_mc_err_probe(struct platform_device *pdev)
>> goto err;
>> }
>>
>> - ctl = in_le32(pdata->mc_vbase + MV64X60_SDRAM_CONFIG);
>> + ctl = ioread32(pdata->mc_vbase + MV64X60_SDRAM_CONFIG);
>> if (!(ctl & MV64X60_SDRAM_ECC)) {
>> /* Non-ECC RAM? */
>> printk(KERN_WARNING "%s: No ECC DIMMs discovered\n", __func__);
>> @@ -779,10 +779,10 @@ static int mv64x60_mc_err_probe(struct platform_device *pdev)
>> mv64x60_init_csrows(mci, pdata);
>>
>> /* setup MC registers */
>> - out_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR, 0);
>> - ctl = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CNTL);
>> + iowrite32(0, pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> + ctl = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CNTL);
>> ctl = (ctl & 0xff00ffff) | 0x10000;
>> - out_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CNTL, ctl);
>> + iowrite32(ctl, pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CNTL);
>>
>> res = edac_mc_add_mc(mci);
>> if (res) {
>> --
>> 2.11.0.24.ge6920cf
>>
>

Next message: Gustavo A. R. Silva: "[PATCH] media: platform: coda: remove variable self assignment"
Previous message: Boris Ostrovsky: "Re: [patch V2 16/17] init: Introduce SYSTEM_SCHEDULING state"
In reply to: Borislav Petkov: "Re: [PATCH 3/3] EDAC: mv64x60: replace in_le32/out_le32 with ioread32/iowrite32"
Next in thread: Arnd Bergmann: "Re: [PATCH 3/3] EDAC: mv64x60: replace in_le32/out_le32 with ioread32/iowrite32"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]