RE: [PATCH v7 5/7] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC

[Manish Narani]

Hi Boris,

Thanks for the review.


> From: Borislav Petkov [mailto:bp@xxxxxxxxx]
> 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@xxxxxxxxxx>
> > ---
> >  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