linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] EDAC, mpc85xx: Make mpc85xx-pci-edac a platform device
@ 2015-12-10  0:02 Scott Wood
  2015-12-10  7:50 ` Johannes Thumshirn
  0 siblings, 1 reply; 2+ messages in thread
From: Scott Wood @ 2015-12-10  0:02 UTC (permalink / raw)
  To: linuxppc-dev, linux-edac
  Cc: Scott Wood, Jia Hongtao, Borislav Petkov, Johannes Thumshirn,
	Michael Ellerman

Originally the mpc85xx-pci-edac driver bound directly to the PCI
controller node.

Commit 905e75c46dba5f30 ("powerpc/fsl-pci: Unify pci/pcie
initialization code") turned the PCI controller code into a platform
device.  Since we can't have two drivers binding to the same device,
the edac code was changed to be called into as a library-style
submodule.  However, this doesn't work if the edac driver is built as a
module.

Commit 8d8fcba6d1eab ("EDAC: Rip out the edac_subsys reference
counting") exposed another problem with this approach --
mpc85xx_pci_err_probe() was being called in the same early boot phase
that the PCI controller is initialized, rather than in the
device_initcall phase that the EDAC layer expects.  This caused a crash
on boot.

To fix this, the PCI controller code now creates a child platform
device specifically for EDAC, which the mpc85xx-pci-edac driver binds
to.

Signed-off-by: Scott Wood <scottwood@freescale.com>
Cc: Jia Hongtao <B38951@freescale.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Michael Ellerman <mpe@ellerman.id.au>
---
v2: Make mpc85xx_pci_err_probe() static again.

 arch/powerpc/sysdev/fsl_pci.c | 28 +++++++++++++++++++++++++++-
 arch/powerpc/sysdev/fsl_pci.h |  9 ---------
 drivers/edac/mpc85xx_edac.c   | 36 +++++++++++++++++++++++++++++++-----
 include/linux/fsl/edac.h      |  8 ++++++++
 4 files changed, 66 insertions(+), 15 deletions(-)
 create mode 100644 include/linux/fsl/edac.h

diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 610f472..a1ac80b 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -21,10 +21,12 @@
 #include <linux/pci.h>
 #include <linux/delay.h>
 #include <linux/string.h>
+#include <linux/fsl/edac.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/memblock.h>
 #include <linux/log2.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
 #include <linux/syscore_ops.h>
@@ -1255,6 +1257,25 @@ void fsl_pcibios_fixup_phb(struct pci_controller *phb)
 #endif
 }
 
+static int add_err_dev(struct platform_device *pdev)
+{
+	struct platform_device *errdev;
+	struct mpc85xx_edac_pci_plat_data pd = {
+		.of_node = pdev->dev.of_node
+	};
+
+	errdev = platform_device_register_resndata(&pdev->dev,
+						   "mpc85xx-pci-edac",
+						   PLATFORM_DEVID_AUTO,
+						   pdev->resource,
+						   pdev->num_resources,
+						   &pd, sizeof(pd));
+	if (IS_ERR(errdev))
+		return PTR_ERR(errdev);
+
+	return 0;
+}
+
 static int fsl_pci_probe(struct platform_device *pdev)
 {
 	struct device_node *node;
@@ -1262,8 +1283,13 @@ static int fsl_pci_probe(struct platform_device *pdev)
 
 	node = pdev->dev.of_node;
 	ret = fsl_add_bridge(pdev, fsl_pci_primary == node);
+	if (ret)
+		return ret;
 
-	mpc85xx_pci_err_probe(pdev);
+	ret = add_err_dev(pdev);
+	if (ret)
+		dev_err(&pdev->dev, "couldn't register error device: %d\n",
+			ret);
 
 	return 0;
 }
diff --git a/arch/powerpc/sysdev/fsl_pci.h b/arch/powerpc/sysdev/fsl_pci.h
index c1cec77..1515885 100644
--- a/arch/powerpc/sysdev/fsl_pci.h
+++ b/arch/powerpc/sysdev/fsl_pci.h
@@ -130,15 +130,6 @@ void fsl_pci_assign_primary(void);
 static inline void fsl_pci_assign_primary(void) {}
 #endif
 
-#ifdef CONFIG_EDAC_MPC85XX
-int mpc85xx_pci_err_probe(struct platform_device *op);
-#else
-static inline int mpc85xx_pci_err_probe(struct platform_device *op)
-{
-	return -ENOTSUPP;
-}
-#endif
-
 #ifdef CONFIG_FSL_PCI
 extern int fsl_pci_mcheck_exception(struct pt_regs *);
 #else
diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index 3eab063..406d75a 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c
@@ -20,6 +20,7 @@
 #include <linux/edac.h>
 #include <linux/smp.h>
 #include <linux/gfp.h>
+#include <linux/fsl/edac.h>
 
 #include <linux/of_platform.h>
 #include <linux/of_device.h>
@@ -238,10 +239,12 @@ static irqreturn_t mpc85xx_pci_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-int mpc85xx_pci_err_probe(struct platform_device *op)
+static int mpc85xx_pci_err_probe(struct platform_device *op)
 {
 	struct edac_pci_ctl_info *pci;
 	struct mpc85xx_pci_pdata *pdata;
+	struct mpc85xx_edac_pci_plat_data *plat_data;
+	struct device_node *of_node;
 	struct resource r;
 	int res = 0;
 
@@ -266,7 +269,15 @@ int mpc85xx_pci_err_probe(struct platform_device *op)
 	pdata->name = "mpc85xx_pci_err";
 	pdata->irq = NO_IRQ;
 
-	if (mpc85xx_pcie_find_capability(op->dev.of_node) > 0)
+	plat_data = op->dev.platform_data;
+	if (!plat_data) {
+		dev_err(&op->dev, "no platform data");
+		res = -ENXIO;
+		goto err;
+	}
+	of_node = plat_data->of_node;
+
+	if (mpc85xx_pcie_find_capability(of_node) > 0)
 		pdata->is_pcie = true;
 
 	dev_set_drvdata(&op->dev, pci);
@@ -284,7 +295,7 @@ int mpc85xx_pci_err_probe(struct platform_device *op)
 
 	pdata->edac_idx = edac_pci_idx++;
 
-	res = of_address_to_resource(op->dev.of_node, 0, &r);
+	res = of_address_to_resource(of_node, 0, &r);
 	if (res) {
 		printk(KERN_ERR "%s: Unable to get resource for "
 		       "PCI err regs\n", __func__);
@@ -339,7 +350,7 @@ int mpc85xx_pci_err_probe(struct platform_device *op)
 	}
 
 	if (edac_op_state == EDAC_OPSTATE_INT) {
-		pdata->irq = irq_of_parse_and_map(op->dev.of_node, 0);
+		pdata->irq = irq_of_parse_and_map(of_node, 0);
 		res = devm_request_irq(&op->dev, pdata->irq,
 				       mpc85xx_pci_isr,
 				       IRQF_SHARED,
@@ -386,8 +397,22 @@ err:
 	devres_release_group(&op->dev, mpc85xx_pci_err_probe);
 	return res;
 }
-EXPORT_SYMBOL(mpc85xx_pci_err_probe);
 
+static const struct platform_device_id mpc85xx_pci_err_match[] = {
+	{
+		.name = "mpc85xx-pci-edac"
+	},
+	{}
+};
+
+static struct platform_driver mpc85xx_pci_err_driver = {
+	.probe = mpc85xx_pci_err_probe,
+	.id_table = mpc85xx_pci_err_match,
+	.driver = {
+		.name = "mpc85xx_pci_err",
+		.suppress_bind_attrs = true,
+	},
+};
 #endif				/* CONFIG_PCI */
 
 /**************************** L2 Err device ***************************/
@@ -1211,6 +1236,7 @@ static void __init mpc85xx_mc_clear_rfxe(void *data)
 static struct platform_driver * const drivers[] = {
 	&mpc85xx_mc_err_driver,
 	&mpc85xx_l2_err_driver,
+	&mpc85xx_pci_err_driver,
 };
 
 static int __init mpc85xx_mc_init(void)
diff --git a/include/linux/fsl/edac.h b/include/linux/fsl/edac.h
new file mode 100644
index 0000000..90d64d4
--- /dev/null
+++ b/include/linux/fsl/edac.h
@@ -0,0 +1,8 @@
+#ifndef FSL_EDAC_H
+#define FSL_EDAC_H
+
+struct mpc85xx_edac_pci_plat_data {
+	struct device_node *of_node;
+};
+
+#endif
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH v2] EDAC, mpc85xx: Make mpc85xx-pci-edac a platform device
  2015-12-10  0:02 [PATCH v2] EDAC, mpc85xx: Make mpc85xx-pci-edac a platform device Scott Wood
@ 2015-12-10  7:50 ` Johannes Thumshirn
  0 siblings, 0 replies; 2+ messages in thread
From: Johannes Thumshirn @ 2015-12-10  7:50 UTC (permalink / raw)
  To: Scott Wood, linuxppc-dev, linux-edac
  Cc: Jia Hongtao, Borislav Petkov, Michael Ellerman

On Wed, 2015-12-09 at 18:02 -0600, Scott Wood wrote:
> Originally the mpc85xx-pci-edac driver bound directly to the PCI
> controller node.
> 
> Commit 905e75c46dba5f30 ("powerpc/fsl-pci: Unify pci/pcie
> initialization code") turned the PCI controller code into a platform
> device.  Since we can't have two drivers binding to the same device,
> the edac code was changed to be called into as a library-style
> submodule.  However, this doesn't work if the edac driver is built as a
> module.
> 
> Commit 8d8fcba6d1eab ("EDAC: Rip out the edac_subsys reference
> counting") exposed another problem with this approach --
> mpc85xx_pci_err_probe() was being called in the same early boot phase
> that the PCI controller is initialized, rather than in the
> device_initcall phase that the EDAC layer expects.  This caused a crash
> on boot.
> 
> To fix this, the PCI controller code now creates a child platform
> device specifically for EDAC, which the mpc85xx-pci-edac driver binds
> to.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> Cc: Jia Hongtao <B38951@freescale.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> ---
> v2: Make mpc85xx_pci_err_probe() static again.
> 
>  arch/powerpc/sysdev/fsl_pci.c | 28 +++++++++++++++++++++++++++-
>  arch/powerpc/sysdev/fsl_pci.h |  9 ---------
>  drivers/edac/mpc85xx_edac.c   | 36 +++++++++++++++++++++++++++++++-----
>  include/linux/fsl/edac.h      |  8 ++++++++
>  4 files changed, 66 insertions(+), 15 deletions(-)
>  create mode 100644 include/linux/fsl/edac.h
> 
> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
> index 610f472..a1ac80b 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -21,10 +21,12 @@
>  #include <linux/pci.h>
>  #include <linux/delay.h>
>  #include <linux/string.h>
> +#include <linux/fsl/edac.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/memblock.h>
>  #include <linux/log2.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
> @@ -1255,6 +1257,25 @@ void fsl_pcibios_fixup_phb(struct pci_controller *phb)
>  #endif
>  }
>  
> +static int add_err_dev(struct platform_device *pdev)
> +{
> +	struct platform_device *errdev;
> +	struct mpc85xx_edac_pci_plat_data pd = {
> +		.of_node = pdev->dev.of_node
> +	};
> +
> +	errdev = platform_device_register_resndata(&pdev->dev,
> +						   "mpc85xx-pci-edac",
> +						   PLATFORM_DEVID_AUTO,
> +						   pdev->resource,
> +						   pdev->num_resources,
> +						   &pd, sizeof(pd));
> +	if (IS_ERR(errdev))
> +		return PTR_ERR(errdev);
> +
> +	return 0;
> +}
> +
>  static int fsl_pci_probe(struct platform_device *pdev)
>  {
>  	struct device_node *node;
> @@ -1262,8 +1283,13 @@ static int fsl_pci_probe(struct platform_device *pdev)
>  
>  	node = pdev->dev.of_node;
>  	ret = fsl_add_bridge(pdev, fsl_pci_primary == node);
> +	if (ret)
> +		return ret;
>  
> -	mpc85xx_pci_err_probe(pdev);
> +	ret = add_err_dev(pdev);
> +	if (ret)
> +		dev_err(&pdev->dev, "couldn't register error device: %d\n",
> +			ret);
>  
>  	return 0;
>  }
> diff --git a/arch/powerpc/sysdev/fsl_pci.h b/arch/powerpc/sysdev/fsl_pci.h
> index c1cec77..1515885 100644
> --- a/arch/powerpc/sysdev/fsl_pci.h
> +++ b/arch/powerpc/sysdev/fsl_pci.h
> @@ -130,15 +130,6 @@ void fsl_pci_assign_primary(void);
>  static inline void fsl_pci_assign_primary(void) {}
>  #endif
>  
> -#ifdef CONFIG_EDAC_MPC85XX
> -int mpc85xx_pci_err_probe(struct platform_device *op);
> -#else
> -static inline int mpc85xx_pci_err_probe(struct platform_device *op)
> -{
> -	return -ENOTSUPP;
> -}
> -#endif
> -
>  #ifdef CONFIG_FSL_PCI
>  extern int fsl_pci_mcheck_exception(struct pt_regs *);
>  #else
> diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> index 3eab063..406d75a 100644
> --- a/drivers/edac/mpc85xx_edac.c
> +++ b/drivers/edac/mpc85xx_edac.c
> @@ -20,6 +20,7 @@
>  #include <linux/edac.h>
>  #include <linux/smp.h>
>  #include <linux/gfp.h>
> +#include <linux/fsl/edac.h>
>  
>  #include <linux/of_platform.h>
>  #include <linux/of_device.h>
> @@ -238,10 +239,12 @@ static irqreturn_t mpc85xx_pci_isr(int irq, void
> *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -int mpc85xx_pci_err_probe(struct platform_device *op)
> +static int mpc85xx_pci_err_probe(struct platform_device *op)
>  {
>  	struct edac_pci_ctl_info *pci;
>  	struct mpc85xx_pci_pdata *pdata;
> +	struct mpc85xx_edac_pci_plat_data *plat_data;
> +	struct device_node *of_node;
>  	struct resource r;
>  	int res = 0;
>  
> @@ -266,7 +269,15 @@ int mpc85xx_pci_err_probe(struct platform_device *op)
>  	pdata->name = "mpc85xx_pci_err";
>  	pdata->irq = NO_IRQ;
>  
> -	if (mpc85xx_pcie_find_capability(op->dev.of_node) > 0)
> +	plat_data = op->dev.platform_data;
> +	if (!plat_data) {
> +		dev_err(&op->dev, "no platform data");
> +		res = -ENXIO;
> +		goto err;
> +	}
> +	of_node = plat_data->of_node;
> +
> +	if (mpc85xx_pcie_find_capability(of_node) > 0)
>  		pdata->is_pcie = true;
>  
>  	dev_set_drvdata(&op->dev, pci);
> @@ -284,7 +295,7 @@ int mpc85xx_pci_err_probe(struct platform_device *op)
>  
>  	pdata->edac_idx = edac_pci_idx++;
>  
> -	res = of_address_to_resource(op->dev.of_node, 0, &r);
> +	res = of_address_to_resource(of_node, 0, &r);
>  	if (res) {
>  		printk(KERN_ERR "%s: Unable to get resource for "
>  		       "PCI err regs\n", __func__);
> @@ -339,7 +350,7 @@ int mpc85xx_pci_err_probe(struct platform_device *op)
>  	}
>  
>  	if (edac_op_state == EDAC_OPSTATE_INT) {
> -		pdata->irq = irq_of_parse_and_map(op->dev.of_node, 0);
> +		pdata->irq = irq_of_parse_and_map(of_node, 0);
>  		res = devm_request_irq(&op->dev, pdata->irq,
>  				       mpc85xx_pci_isr,
>  				       IRQF_SHARED,
> @@ -386,8 +397,22 @@ err:
>  	devres_release_group(&op->dev, mpc85xx_pci_err_probe);
>  	return res;
>  }
> -EXPORT_SYMBOL(mpc85xx_pci_err_probe);
>  
> +static const struct platform_device_id mpc85xx_pci_err_match[] = {
> +	{
> +		.name = "mpc85xx-pci-edac"
> +	},
> +	{}
> +};
> +
> +static struct platform_driver mpc85xx_pci_err_driver = {
> +	.probe = mpc85xx_pci_err_probe,
> +	.id_table = mpc85xx_pci_err_match,
> +	.driver = {
> +		.name = "mpc85xx_pci_err",
> +		.suppress_bind_attrs = true,
> +	},
> +};
>  #endif				/* CONFIG_PCI */
>  
>  /**************************** L2 Err device ***************************/
> @@ -1211,6 +1236,7 @@ static void __init mpc85xx_mc_clear_rfxe(void *data)
>  static struct platform_driver * const drivers[] = {
>  	&mpc85xx_mc_err_driver,
>  	&mpc85xx_l2_err_driver,
> +	&mpc85xx_pci_err_driver,
>  };
>  

One thing, if we don't have CONFIG_PCI it won't build AFICS, or I'm I totally
wrong now (my version had this bug as well, btw)?

With the above clarified/fixed feel free to add my
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-12-10  7:50 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-10  0:02 [PATCH v2] EDAC, mpc85xx: Make mpc85xx-pci-edac a platform device Scott Wood
2015-12-10  7:50 ` Johannes Thumshirn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).