RE: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP OCM

[Potthuri, Sai Krishna]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Tuesday, August 16, 2022 1:36 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@xxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Michal Simek
> <michal.simek@xxxxxxxxxx>; Borislav Petkov <bp@xxxxxxxxx>; Mauro
> Carvalho Chehab <mchehab@xxxxxxxxxx>; Tony Luck <tony.luck@xxxxxxxxx>;
> James Morse <james.morse@xxxxxxx>; Robert Richter <rric@xxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx;
> saikrishna12468@xxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>; Datta,
> Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>
> Subject: Re: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP
> OCM
> 
> On 16/08/2022 10:32, Sai Krishna Potthuri wrote:
> > Add EDAC support for Xilinx ZynqMP OCM Controller, this driver reports
> > CE and UE errors based on the interrupts, and also creates ue/ce sysfs
> > entries for error injection.
> >
> > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@xxxxxxx>
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx>
> 
> A bit confusing SoB order, although sometimes rational. Are you sure about
> authorship here?
Yes, I am the author of this file and Shubhrajyoti contributed to this.
> 
> > ---
> >  MAINTAINERS                    |   7 +
> >  drivers/edac/Kconfig           |   9 +
> >  drivers/edac/Makefile          |   1 +
> >  drivers/edac/zynqmp_ocm_edac.c | 643
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 660 insertions(+)
> >  create mode 100644 drivers/edac/zynqmp_ocm_edac.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > edc96cdb85e8..cd4c6c90bca3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21692,6 +21692,13 @@ F:
> 	Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-
> dpdma.yaml
> >  F:	drivers/dma/xilinx/xilinx_dpdma.c
> >  F:	include/dt-bindings/dma/xlnx-zynqmp-dpdma.h
> >
> > +XILINX ZYNQMP OCM EDAC DRIVER
> > +M:	Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx>
> > +M:	Sai Krishna Potthuri <sai.krishna.potthuri@xxxxxxx>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
> > +F:	drivers/edac/zynqmp_ocm_edac.c
> > +
> >  XILINX ZYNQMP PSGTR PHY DRIVER
> >  M:	Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
> >  M:	Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index
> > 58ab63642e72..fece60f586af 100644
> > --- a/drivers/edac/Kconfig
> > +++ b/drivers/edac/Kconfig
> > @@ -539,4 +539,13 @@ config EDAC_DMC520
> >  	  Support for error detection and correction on the
> >  	  SoCs with ARM DMC-520 DRAM controller.
> >
> > +config EDAC_ZYNQMP_OCM
> > +	tristate "Xilinx ZynqMP OCM Controller"
> > +	depends on ARCH_ZYNQMP
> 
> || COMPILE_TEST
I will fix in v2.
> 
> 
> > +	help
> > +	  Support for error de
> 
> 
> > +/**
> > + * zynqmp_ocm_edac_get_eccstate - Return the controller ecc status
> > + * @base:	Pointer to the ddr memory controller base address
> > + *
> > + * Get the ECC enable/disable status for the controller
> > + *
> > + * Return: ecc status 0/1.
> > + */
> > +static bool zynqmp_ocm_edac_get_eccstate(void __iomem *base) {
> > +	return readl(base + ECC_CTRL_OFST) & OCM_ECC_ENABLE_MASK; }
> > +
> > +static const struct of_device_id zynqmp_ocm_edac_match[] = {
> > +	{ .compatible = "xlnx,zynqmp-ocmc-1.0"},
> > +	{ /* end of table */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, zynqmp_ocm_edac_match);
> 
> This goes to the end. Do not embed static variables in the middle of the code.
I will fix in v2.
> 
> 
> > +
> > +/**
> > + * zynqmp_set_ocm_edac_sysfs_attributes - create sysfs attributes
> > + * @edac_dev:	Pointer to the edac device struct
> > + *
> > + * Creates sysfs entries for error injection  */ static void
> > +zynqmp_set_ocm_edac_sysfs_attributes(struct edac_device_ctl_info
> > +						*edac_dev)
> > +{
> > +	edac_dev->sysfs_attributes = zynqmp_ocm_edac_sysfs_attributes; }
> > +
> > +/**
> > + * zynqmp_ocm_edac_probe - Check controller and bind driver
> > + * @pdev:	Pointer to the platform_device struct
> > + *
> > + * Probes a specific controller instance for binding with the driver.
> > + *
> > + * Return: 0 if the controller instance was successfully bound to the
> > + * driver; otherwise error code.
> > + */
> 
> Drop the kerneldoc for probe(). It's obvious and exactly the same
> everywhere. You could keep it if you write something different than theh
> same message for 1000 other probes.
I will fix in v2.
> 
> > +static int zynqmp_ocm_edac_probe(struct platform_device *pdev) {
> > +	struct zynqmp_ocm_edac_priv *priv;
> > +	struct edac_device_ctl_info *dci;
> > +	void __iomem *baseaddr;
> > +	struct resource *res;
> > +	int irq, ret;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	baseaddr = devm_ioremap_resource(&pdev->dev, res);
> 
> There is a wrapper for these.
I will fix in v2.
> 
> > +	if (IS_ERR(baseaddr))
> > +		return PTR_ERR(baseaddr);
> > +
> > +	if (!zynqmp_ocm_edac_get_eccstate(baseaddr)) {
> > +		edac_printk(KERN_INFO, EDAC_DEVICE,
> > +			    "ECC not enabled - Disabling EDAC driver\n");
> 
> How do you disable the driver? What if there are two devices - how does this
> disables the driver for second device?
Here I am checking the ECC capability of the controller, if ecc is enabled then
i will proceed with the probe otherwise return from here.
If there are two devices, then probe will be called twice, and each device has its
own capabilities.
"Disabling EDAC driver" statement might creating the confusion, i will remove
that statement.
> 
> > +		return -ENXIO;
> > +	}
> > +
> > +	dci = edac_device_alloc_ctl_info(sizeof(*priv),
> ZYNQMP_OCM_EDAC_STRING,
> > +					 1, ZYNQMP_OCM_EDAC_STRING, 1,
> 0, NULL, 0,
> > +					 edac_device_alloc_index());
> > +	if (!dci) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE,
> > +			    "Unable to allocate EDAC device\n");
> 
> No ENOMEM prints.
I will fix in v2.
> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	priv = dci->pvt_info;
> > +	platform_set_drvdata(pdev, dci);
> > +	dci->dev = &pdev->dev;
> > +	priv->baseaddr = baseaddr;
> > +	dci->mod_name = pdev->dev.driver->name;
> > +	dci->ctl_name = ZYNQMP_OCM_EDAC_STRING;
> > +	dci->dev_name = dev_name(&pdev->dev);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE,
> > +			    "No irq %d in DT\n", irq);
> 
> The same, no need for printks. Core does it.
I will fix in v2.
> 
> > +		ret = irq;
> > +		goto free_dev_ctl;
> > +	}
> > +
> > +	ret = devm_request_irq(&pdev->dev, irq,
> > +			       zynqmp_ocm_edac_intr_handler,
> > +			       0, dev_name(&pdev->dev), dci);
> > +	if (ret) {
> > +		edac_printk(KERN_ERR, EDAC_DEVICE, "Failed to request
> Irq\n");
> > +		goto free_dev_ctl;
> > +	}
> > +
> > +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IEN_OFST);
> > +
> > +	zynqmp_set_ocm_edac_sysfs_attributes(dci);
> > +	ret = edac_device_add_device(dci);
> > +	if (ret)
> > +		goto free_dev_ctl;
> > +
> > +	return 0;
> > +
> > +free_dev_ctl:
> > +	edac_device_free_ctl_info(dci);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_ocm_edac_remove - Unbind driver from controller
> > + * @pdev:	Pointer to the platform_device struct
> > + *
> > + * Return: Unconditionally 0
> > + */
> 
> Same comment for kerneldoc.
I will fix in v2.

Regards
Sai Krishna
> 
> > +static int zynqmp_ocm_edac_remove(struct platform_device *pdev) {
> > +	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
> > +	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
> > +
> > +	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IDS_OFST);
> > +	edac_device_del_device(&pdev->dev);
> > +	edac_device_free_ctl_info(dci);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver zynqmp_ocm_edac_driver = {
> > +	.driver = {
> > +		   .name = "zynqmp-ocm-edac",
> > +		   .of_match_table = zynqmp_ocm_edac_match,
> > +		   },
> > +	.probe = zynqmp_ocm_edac_probe,
> > +	.remove = zynqmp_ocm_edac_remove,
> > +};
> > +
> > +module_platform_driver(zynqmp_ocm_edac_driver);
> > +
> > +MODULE_AUTHOR("Advanced Micro Devices, Inc");
> > +MODULE_DESCRIPTION("ZynqMP OCM ECC driver");
> MODULE_LICENSE("GPL");
> 
> 
> Best regards,
> Krzysztof