lkml.org 
[lkml]   [2018]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v7 5/7] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC
Date
Hi Boris,

Thanks for the review.


> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Friday, September 21, 2018 6:26 PM
> On Mon, Sep 17, 2018 at 07:55:03PM +0530, Manish Narani wrote:
> > Add EDAC ECC support for ZynqMP DDRC IP. The IP supports interrupts
> > for corrected and uncorrected errors. Add interrupt handlers for the same.
> >
> > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > ---
> > drivers/edac/Kconfig | 2 +-
> > drivers/edac/synopsys_edac.c | 304
> > ++++++++++++++++++++++++++++++++++++++++---
> > 2 files changed, 287 insertions(+), 19 deletions(-)
>
> ...
> > + if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
>
> This whole chunk together with the if, looks like it could be a separate function
> called maybe request_irq() or so.
May be I can keep it like edac_setup_irq() ?

>
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + edac_printk(KERN_ERR, EDAC_MC,
> > + "No irq %d in DT\n", irq);
>
> Align arguments on opening brace.
Okay

>
> Then, here it is "irq"...
>
> > + rc = irq;
> > + goto free_edac_mc;
> > + }
> > +
> > + rc = devm_request_irq(&pdev->dev, irq,
> > + synps_edac_intr_handler,
> > + 0, dev_name(&pdev->dev), mci);
>
> Also align on opening brace.
>
> > + if (rc < 0) {
> > + edac_printk(KERN_ERR, EDAC_MC, "Failed to request
> Irq\n");
>
> ... here it is "Irq".
>
> I'd say it should be "IRQ" everywhere.
Okay

>
> > + goto free_edac_mc;
> > + }
> > +
> > + /* Enable UE/CE Interrupts */
> > + writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
> > + priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
> > + }
> > +
> > rc = edac_mc_add_mc(mci);
> > if (rc) {
> > edac_printk(KERN_ERR, EDAC_MC,
> > @@ -684,7 +945,8 @@ static int synps_edac_mc_probe(struct
> platform_device *pdev)
> > * Start capturing the correctable and uncorrectable errors. A write of
> > * 0 starts the counters.
> > */
> > - writel(0x0, baseaddr + ECC_CTRL_OFST);
> > + if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT))
> > + writel(0x0, baseaddr + ECC_CTRL_OFST);
> > return rc;
> >
> > free_edac_mc:
> > @@ -702,7 +964,13 @@ static int synps_edac_mc_probe(struct
> > platform_device *pdev) static int synps_edac_mc_remove(struct
> > platform_device *pdev) {
> > struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> > + struct synps_edac_priv *priv;
> >
> > + priv = mci->pvt_info;
> > + if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)
> > + /* Disable UE/CE Interrupts */
> > + writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
> > + priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
>
> This could be a disable_irq() function. Makes the code easier to follow.
Okay. Will update this in v8.

Thanks,
Manish
\
 
 \ /
  Last update: 2018-09-24 12:21    [W:0.247 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site