Skip to contentSkip to conversation

MPC85xx: add definitions for PCI error detection soc part

PATCH NAK Dmitry Eremin-Solenikov 6 participants 16 messages Jul 22, 2010 – Jul 24, 2010 1 message not in the archive

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

PATCH 1/2 MPC85xx: add definitions for PCI error detection soc part

Add definitions for PCI error detection device to be handled by (already existing) mpc85xx_edac.c driver.

---
 arch/powerpc/boot/dts/mpc8536ds.dts        |    7 +++++++
 arch/powerpc/boot/dts/mpc8536ds_36b.dts    |    7 +++++++
 arch/powerpc/boot/dts/mpc8540ads.dts       |    8 ++++++++
 arch/powerpc/boot/dts/mpc8541cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8544ds.dts        |    7 +++++++
 arch/powerpc/boot/dts/mpc8548cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8555cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8560ads.dts       |    7 +++++++
 arch/powerpc/boot/dts/mpc8568mds.dts       |    7 +++++++
 arch/powerpc/boot/dts/sbc8548.dts          |    7 +++++++
 arch/powerpc/boot/dts/sbc8560.dts          |    7 +++++++
 arch/powerpc/boot/dts/socrates.dts         |    7 +++++++
 arch/powerpc/boot/dts/stx_gp3_8560.dts     |    7 +++++++
 arch/powerpc/boot/dts/tqm8540.dts          |   10 ++++++++++
 arch/powerpc/boot/dts/tqm8541.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8548-bigflash.dts |    7 +++++++
 arch/powerpc/boot/dts/tqm8548.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8555.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8560.dts          |    7 +++++++
 arch/powerpc/boot/dts/xpedite5200.dts      |    7 +++++++
 arch/powerpc/boot/dts/xpedite5200_xmon.dts |    7 +++++++
 21 files changed, 172 insertions(+), 0 deletions(-)
arch/powerpc/boot/dts/mpc8536ds.dts +7 -0
index 815cebb..6c02e5a 100644
@@ -278,6 +278,13 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8536-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		crypto@30000 {
 			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
 				     "fsl,sec2.1", "fsl,sec2.0";
arch/powerpc/boot/dts/mpc8536ds_36b.dts +7 -0
index d95b260..89ef5c1 100644
@@ -278,6 +278,13 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8536-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		crypto@30000 {
 			compatible = "fsl,sec3.0", "fsl,sec2.4", "fsl,sec2.2",
 				     "fsl,sec2.1", "fsl,sec2.0";
arch/powerpc/boot/dts/mpc8540ads.dts +8 -0
index 9dc2929..d89f470 100644
@@ -259,6 +259,14 @@
 			interrupts = <42 2>;
 			interrupt-parent = <&mpic>;
 		};
+
+		pci-error@8e00 {
+			compatible = "fsl,mpc8540-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		mpic: pic@40000 {
 			interrupt-controller;
 			#address-cells = <0>;
arch/powerpc/boot/dts/mpc8541cds.dts +14 -0
index 9a3ad31..c2c5732 100644
@@ -226,6 +226,20 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8541-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
+		pci-error@9e00 {
+			compatible = "fsl,mpc8541-pci-error";
+			reg = <0x9e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <25 2>;
+		};
+
 		crypto@30000 {
 			compatible = "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
arch/powerpc/boot/dts/mpc8544ds.dts +7 -0
index 98e94b4..387da1f 100644
@@ -242,6 +242,13 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8544-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		global-utilities@e0000 {	//global utilities block
 			compatible = "fsl,mpc8548-guts";
 			reg = <0xe0000 0x1000>;
arch/powerpc/boot/dts/mpc8548cds.dts +14 -0
index 0f52624..973966e 100644
@@ -328,6 +328,20 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8548-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
+		pci-error@9e00 {
+			compatible = "fsl,mpc8548-pci-error";
+			reg = <0x9e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <25 2>;
+		};
+
 		global-utilities@e0000 {	//global utilities reg
 			compatible = "fsl,mpc8548-guts";
 			reg = <0xe0000 0x1000>;
arch/powerpc/boot/dts/mpc8555cds.dts +14 -0
index 065b2f0..bcdc344 100644
@@ -226,6 +226,20 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8555-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
+		pci-error@9e00 {
+			compatible = "fsl,mpc8555-pci-error";
+			reg = <0x9e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <25 2>;
+		};
+
 		crypto@30000 {
 			compatible = "fsl,sec2.0";
 			reg = <0x30000 0x10000>;
arch/powerpc/boot/dts/mpc8560ads.dts +7 -0
index a5bb1ec..04bc095 100644
@@ -216,6 +216,13 @@
 			device_type = "open-pic";
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8560-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		cpm@919c0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
arch/powerpc/boot/dts/mpc8568mds.dts +7 -0
index 92fb178..66af3ab 100644
@@ -329,6 +329,13 @@
 			};
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8568-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		global-utilities@e0000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
arch/powerpc/boot/dts/sbc8548.dts +7 -0
index 94a3322..002428c 100644
@@ -333,6 +333,13 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8548-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		global-utilities@e0000 {	//global utilities reg
 			compatible = "fsl,mpc8548-guts";
 			reg = <0xe0000 0x1000>;
arch/powerpc/boot/dts/sbc8560.dts +7 -0
index 9e13ed8..1dc389a 100644
@@ -239,6 +239,13 @@
 			device_type = "open-pic";
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8560-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		cpm@919c0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
arch/powerpc/boot/dts/socrates.dts +7 -0
index feb4ef6..06b08f5 100644
@@ -216,6 +216,13 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8544-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		global-utilities@e0000 {	//global utilities block
 			compatible = "fsl,mpc8548-guts";
 			reg = <0xe0000 0x1000>;
arch/powerpc/boot/dts/stx_gp3_8560.dts +7 -0
index b670d03..78e9e22 100644
@@ -213,6 +213,13 @@
 			device_type = "open-pic";
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8560-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		cpm@919c0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
arch/powerpc/boot/dts/tqm8540.dts +10 -0
index b5c0940..3e73e35 100644
@@ -160,6 +160,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupts = <29 2 30 2 34 2>;
 			interrupt-parent = <&mpic>;
+			tbi-handle = <&tbi0>;
 			phy-handle = <&phy2>;
 
 			mdio@520 {
@@ -205,6 +206,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupts = <35 2 36 2 40 2>;
 			interrupt-parent = <&mpic>;
+			tbi-handle = <&tbi1>;
 			phy-handle = <&phy1>;
 
 			mdio@520 {
@@ -232,6 +234,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupts = <41 2>;
 			interrupt-parent = <&mpic>;
+			tbi-handle = <&tbi2>;
 			phy-handle = <&phy3>;
 
 			mdio@520 {
@@ -276,6 +279,13 @@
 			device_type = "open-pic";
 			compatible = "chrp,open-pic";
 		};
+
+		pci-error@8e00 {
+			compatible = "fsl,mpc8540-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
 	};
 
 	pci0: pci@e0008000 {
arch/powerpc/boot/dts/tqm8541.dts +7 -0
index f49d091..92106fc 100644
@@ -261,6 +261,13 @@
 			compatible = "chrp,open-pic";
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8541-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		cpm@919c0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
arch/powerpc/boot/dts/tqm8548-bigflash.dts +7 -0
index 5dbb36e..b1944a1 100644
@@ -338,6 +338,13 @@
 			compatible = "chrp,open-pic";
 			device_type = "open-pic";
 		};
+
+		pci-error@8e00 {
+			compatible = "fsl,mpc8548-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
 	};
 
 	localbus@a0005000 {
arch/powerpc/boot/dts/tqm8548.dts +7 -0
index a050ae4..be2b1de 100644
@@ -338,6 +338,13 @@
 			compatible = "chrp,open-pic";
 			device_type = "open-pic";
 		};
+
+		pci-error@8e00 {
+			compatible = "fsl,mpc8548-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
 	};
 
 	localbus@e0005000 {
arch/powerpc/boot/dts/tqm8555.dts +7 -0
index 81bad8c..7b64c71 100644
@@ -261,6 +261,13 @@
 			compatible = "chrp,open-pic";
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8555-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		cpm@919c0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
arch/powerpc/boot/dts/tqm8560.dts +7 -0
index 22ec39b..82408d5 100644
@@ -232,6 +232,13 @@
 			compatible = "chrp,open-pic";
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8560-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		cpm@919c0 {
 			#address-cells = <1>;
 			#size-cells = <1>;
arch/powerpc/boot/dts/xpedite5200.dts +7 -0
index a0cf53f..211dbbf 100644
@@ -352,6 +352,13 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8548-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		global-utilities@e0000 {	// global utilities reg
 			compatible = "fsl,mpc8548-guts";
 			reg = <0xe0000 0x1000>;
arch/powerpc/boot/dts/xpedite5200_xmon.dts +7 -0
index c5b2975..ebe9365 100644
@@ -356,6 +356,13 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		pci-error@8e00 {
+			compatible = "fsl,mpc8548-pci-error";
+			reg = <0x8e00 0x100>;
+			interrupt-parent = <&mpic>;
+			interrupts = <24 2>;
+		};
+
 		global-utilities@e0000 {	// global utilities reg
 			compatible = "fsl,mpc8548-guts";
 			reg = <0xe0000 0x1000>;
Signature · 7 lines
1.7.1


------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
#1 lore raw

PATCH 2/2 mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
 drivers/edac/mpc85xx_edac.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/edac/mpc85xx_edac.c +9 -9
index 52ca09b..d5a61b8 100644
@@ -236,9 +236,6 @@ static int __devinit mpc85xx_pci_err_probe(struct of_device *op,
 		goto err;
 	}
 
-	/* we only need the error registers */
-	r.start += 0xe00;
-
 	if (!devm_request_mem_region(&op->dev, r.start, resource_size(&r),
 					pdata->name)) {
 		printk(KERN_ERR "%s: Error while requesting mem region\n",
@@ -328,12 +325,15 @@ static int mpc85xx_pci_err_remove(struct of_device *op)
 }
 
 static struct of_device_id mpc85xx_pci_err_of_match[] = {
-	{
-	 .compatible = "fsl,mpc8540-pcix",
-	 },
-	{
-	 .compatible = "fsl,mpc8540-pci",
-	},
+	{ .compatible = "fsl,mpc8536-pci-error", },
+	{ .compatible = "fsl,mpc8540-pci-error", },
+	{ .compatible = "fsl,mpc8541-pci-error", },
+	{ .compatible = "fsl,mpc8544-pci-error", },
+	{ .compatible = "fsl,mpc8548-pci-error", },
+	{ .compatible = "fsl,mpc8555-pci-error", },
+	{ .compatible = "fsl,mpc8560-pci-error", },
+	{ .compatible = "fsl,mpc8568-pci-error", },
+	{ .compatible = "fsl,mpc8572-pci-error", },
 	{},
 };
 
Signature · 7 lines
1.7.1


------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
#2 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Quoted 9 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
drivers/edac/mpc85xx_edac.c | 18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)

Nak.

We already have a node in the dts for the PCI controller. Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

- k

#3 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Hello,

Quoted 8 lines · Kumar Gala
Quoted 5 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

Quoted 8 lines
Quoted 4 lines

---
drivers/edac/mpc85xx_edac.c |   18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)

Nak.

We already have a node in the dts for the PCI controller.  Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus. Would it be suitable to just call of_platform_device_create for it (Or do i miss someth<ing)?

BTW: While I'm at it, should I change all mpc8540-pci/-pcix device names to include respective SoC name?

Signature · 7 lines
With best wishes
Dmitry

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
#4 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Quoted 16 lines · On Thu, 22 Jul 2010 20:48:15 +0400 Dmitry Eremin-Solenikov…

Hello,

Quoted 13 lines · Kumar Gala
Quoted 9 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
drivers/edac/mpc85xx_edac.c |   18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)
Quoted 8 lines
Quoted 3 lines

Nak.

We already have a node in the dts for the PCI controller.  Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus.

Right, and it's also not great for a driver for one aspect of PCI to claim to be the driver for the whole thing.

But changing the device tree because of this Linux-internal concern is also not good.

How about keeping the error stuff as a separate device from Linux's perspective, but have the main Freescale PCI code create it as a platform device instead of putting it in the device tree?

-Scott

#5 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Quoted 31 lines · Scott Wood
Quoted 16 lines · On Thu, 22 Jul 2010 20:48:15 +0400 Dmitry Eremin-Solenikov…

Hello,

Quoted 13 lines · Kumar Gala
Quoted 9 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
drivers/edac/mpc85xx_edac.c | 18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)
Quoted 8 lines
Quoted 3 lines

Nak.

We already have a node in the dts for the PCI controller. Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus.

Right, and it's also not great for a driver for one aspect of PCI to claim to be the driver for the whole thing.

Quoted 6 lines

But changing the device tree because of this Linux-internal concern is also not good.

How about keeping the error stuff as a separate device from Linux's perspective, but have the main Freescale PCI code create it as a platform device instead of putting it in the device tree?

I'd be good with that solution.

- k
------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

#6 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Hello,

Quoted 34 lines · Kumar Gala
Quoted 31 lines · Scott Wood
Quoted 16 lines · On Thu, 22 Jul 2010 20:48:15 +0400 Dmitry Eremin-Solenikov…

Hello,

Quoted 13 lines · Kumar Gala
Quoted 9 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
drivers/edac/mpc85xx_edac.c |   18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)
Quoted 8 lines
Quoted 3 lines

Nak.

We already have a node in the dts for the PCI controller.  Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus.

Right, and it's also not great for a driver for one aspect of PCI to claim to be the driver for the whole thing.

Quoted 8 lines
Quoted 6 lines

But changing the device tree because of this Linux-internal concern is also not good.

How about keeping the error stuff as a separate device from Linux's perspective, but have the main Freescale PCI code create it as a platform device instead of putting it in the device tree?

I'd be good with that solution.

Then we come back to the question that was raised before (during initial review of edac driver): as PCI code is probbed long before other parts of the kernel and mpc85xx_edac code can be compiled as module, it's not possible to directly call mpc85xx_edac code from fsl_pci.c

Two initial suggestions were:
1) creating special platform device
2) creating special of_platform device from dts

Which approach should I choose? Did i miss any other opportunities?

Signature · 7 lines
With best wishes
Dmitry

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
#7 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Quoted 44 lines · Dmitry Eremin-Solenikov

Hello,

Quoted 41 lines · Kumar Gala
Quoted 37 lines · Scott Wood
Quoted 16 lines · On Thu, 22 Jul 2010 20:48:15 +0400 Dmitry Eremin-Solenikov…

Hello,

Quoted 13 lines · Kumar Gala
Quoted 9 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
drivers/edac/mpc85xx_edac.c |   18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)
Quoted 8 lines
Quoted 3 lines

Nak.

We already have a node in the dts for the PCI controller.  Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus.

Right, and it's also not great for a driver for one aspect of PCI to claim to be the driver for the whole thing.

But changing the device tree because of this Linux-internal concern is also not good.

How about keeping the error stuff as a separate device from Linux's perspective, but have the main Freescale PCI code create it as a platform device instead of putting it in the device tree?

Quoted 6 lines
Quoted 1 line

I'd be good with that solution.

Then we come back to the question that was raised before (during initial review of edac driver): as PCI code is probbed long before other parts of the kernel and mpc85xx_edac code can be compiled as module, it's not possible to directly call mpc85xx_edac code from fsl_pci.c

Not sure what you mean here. If the driver is compiled as a module, then it must be handled the same way all other drivers are handled. The platform code registers an of_platform_device at boot time, and the driver gets bound to it whenever it happens to show up. None of the children of that bus can be available until after the driver is probed.

Quoted 3 lines

Two initial suggestions were:
1) creating special platform device
2) creating special of_platform device from dts

g.

Signature · 2 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
#8 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

In reply to Dmitry Eremin-Solenikov
Quoted 9 lines · On Thu, 22 Jul 2010 23:03:03 +0400 Dmitry Eremin-Solenikov…

Hello,

Quoted 6 lines · Kumar Gala
Quoted 3 lines · Scott Wood

How about keeping the error stuff as a separate device from Linux's perspective, but have the main Freescale PCI code create it as a platform device instead of putting it in the device tree?

Quoted 6 lines
Quoted 1 line

I'd be good with that solution.

Then we come back to the question that was raised before (during initial review of edac driver): as PCI code is probbed long before other parts of the kernel and mpc85xx_edac code can be compiled as module, it's not possible to directly call mpc85xx_edac code from fsl_pci.c

Right, that's why I suggested creating a platform device rather than just a function call.

Quoted 5 lines

Two initial suggestions were:
1) creating special platform device
2) creating special of_platform device from dts

Which approach should I choose? Did i miss any other opportunities?

#1, as it keeps the split out of the device tree.

Besides the theoretical/aesthetic issues of putting Linux implementation concerns into the device tree, #2 would mean that the edac driver wouldn't work when Linux is booted with an old device tree.

-Scott

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

#9 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

In reply to Dmitry Eremin-Solenikov
Quoted 18 lines · Dmitry Eremin-Solenikov

Hello,

Quoted 15 lines · Kumar Gala
Quoted 9 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
drivers/edac/mpc85xx_edac.c |   18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)

Nak.

Quoted 8 lines
Quoted 1 line

We already have a node in the dts for the PCI controller.  Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus. Would it be suitable to just call of_platform_device_create for it (Or do i miss someth<ing)?

Try the attached patch (lightly tested). If it works for you then I'll post it for wider review.

Quoted 2 lines

BTW: While I'm at it, should I change all mpc8540-pci/-pcix device names to include respective SoC name?

It is good practice to include both the specific name, and the name of the device it is backwards compatible to.

g.

From d84af195dbcd99ce172bf639538231141176d402 Mon Sep 17 00:00:00 2001
From: Grant Likely <[email protected]>
Date: Thu, 22 Jul 2010 13:01:11 -0600
Subject: [PATCH] of/device: Register children with a compatible value in of_platform_bus_probe()

Currently, of_platform_bus_probe() completely skips nodes which do not explicitly match the 'matches' table passed in. Or, if the root node matches, then it registers all the children unconditionally. However, there are situations, such as registering devices from the root node, when it is desirable to register child nodes, but only if they actually represent devices. For example, the root node may contain both a local bus and a PCI device, but it also contains the chosen, aliases and cpus nodes which don't represent real devices.

This patch changes of_platform_bus_probe() to register all nodes at the top level if they either match the matches table (the current behaviour), or if they have a 'compatible' value (indicating it represents a device).

---
 drivers/of/platform.c |   31 ++++++++++++++++++++++++++-----
 1 files changed, 26 insertions(+), 5 deletions(-)
drivers/of/platform.c +26 -5
index f3f1ec8..2ead562 100644
@@ -709,16 +709,37 @@ int of_platform_bus_probe(struct device_node *root,
 		rc = of_platform_bus_create(root, matches, &dev->dev);
 		goto bail;
 	}
+
+	/* Register each child node if either:
+	 *  a) it has a 'compatible' value indicating they are a device, or
+	 *  b) it is specified by the 'matches' table (by name or device_type)
+	 * If a node is specified in the matches table, then all its children
+	 * also get registered.
+	 */
 	for_each_child_of_node(root, child) {
-		if (!of_match_node(matches, child))
+		void *compat = of_get_property(child, "compatible", NULL);
+		struct of_device_id *match = of_match_node(matches, child);
+
+		/* Skip if node neither matches nor has a compatible property */
+		if (!compat && !match)
 			continue;
 
-		pr_debug("  match: %s\n", child->full_name);
+		pr_debug("  register device: %s\n", child->full_name);
+
+		/* Passed the first test, register node as a platform device */
 		dev = of_platform_device_create(child, NULL, parent);
-		if (dev == NULL)
+		if (!dev) {
 			rc = -ENOMEM;
-		else
-			rc = of_platform_bus_create(child, matches, &dev->dev);
+			of_node_put(child);
+			break;
+		}
+
+		/* Only register child nodes if specified by matches table */
+		if (!match)
+			continue;
+
+		pr_debug("  register children of: %s\n", child->full_name);
+		rc = of_platform_bus_create(child, matches, &dev->dev);
 		if (rc) {
 			of_node_put(child);
 			break;
Signature · 1 line
1.7.0.4

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

#10 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Hello,

Quoted 32 lines · Grant Likely
Quoted 23 lines · Dmitry Eremin-Solenikov

Hello,

Quoted 13 lines · Kumar Gala
Quoted 9 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
drivers/edac/mpc85xx_edac.c | 18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)
Quoted 5 lines

Nak.

We already have a node in the dts for the PCI controller. Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

Quoted 6 lines

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus. Would it be suitable to just call of_platform_device_create for it (Or do i miss someth<ing)?

Quoted 2 lines

Try the attached patch (lightly tested). If it works for you then I'll post it for wider review.

Yes, this patch worked for me. However it looks a bit like a hack for me.

Signature · 7 lines
With best wishes
Dmitry

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

A message that is not in the archive (sent off-list or not yet received): AANLkTik-3cujkh774H6WQB8sHD+gkKLT2FQp=8HYaWBt-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#12 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Quoted 35 lines · Dmitry Eremin-Solenikov

Hello,

Quoted 32 lines · Grant Likely
Quoted 29 lines · Dmitry Eremin-Solenikov

Hello,

Quoted 13 lines · Kumar Gala
Quoted 9 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes, present on soc bus.

---
drivers/edac/mpc85xx_edac.c |   18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)
Quoted 5 lines

Nak.

We already have a node in the dts for the PCI controller.  Lets update the platform code to add the pci controller to the of_platform_bus_probe list.

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus. Would it be suitable to just call of_platform_device_create for it (Or do i miss someth<ing)?

Quoted 4 lines
Quoted 2 lines

Try the attached patch (lightly tested).  If it works for you then I'll post it for wider review.

Yes, this patch worked for me. However it looks a bit like a hack for me.

I'll probably refine it a bit before merging, but I don't think it is a hack. It reflects the behaviour that makes sense when registering devices hanging off the root node. If a device node is a child of the root, then we know it isn't hanging off an i2c or pci bus, or anything else. It is essentially a system device.

The troublesome bit is that the root node also has memory, cpus, chosen and aliases nodes which are not devices. In the vast majority of cases, we want all the device nodes that are children of the root to be registered, but we don't want to register the special nodes. Checking for the presence of a compatible property is a pretty good test for determining whether or not a node actually represents a device, especially because all users of of_platform_bus_probe() seem to be FDT users where we've been very strict about enforcing that drivers must use the compatible property for matching to device nodes.

Cheers,
g.

#13 lore raw

Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace

Quoted 51 lines · Grant Likely
Quoted 38 lines · Dmitry Eremin-Solenikov

Hello,

Quoted 35 lines · Grant Likely
Quoted 32 lines · Dmitry Eremin-Solenikov

Hello,

Quoted 15 lines · Kumar Gala
Quoted 11 lines · Dmitry Eremin-Solenikov

Currently (as mpc8540-pci) devices are not created on of_platform bus, mpc85xx_edac can't probe to them. Follow the change to dts trees to bind
not to the main mpc8540-pci node but to special mpc85xx-pci-error nodes,
present on soc bus.

---
drivers/edac/mpc85xx_edac.c | 18 +++++++++---------

1 files changed, 9 insertions(+), 9 deletions(-)
Quoted 6 lines

Nak.

We already have a node in the dts for the PCI controller. Lets update the platform code to add the pci controller to the of_platform_bus_probe
list.

I've had that idea. However it's really look strange to me to call of_platform_bus_probe() on the bus node, for which we (IMO) explicitly won't like for
child devices (PCI devices) to be added to of_platform bus. Would it be suitable to just call of_platform_device_create for it (Or do i miss someth<ing)?

Quoted 4 lines
Quoted 2 lines

Try the attached patch (lightly tested). If it works for you then I'll post it for wider review.

Yes, this patch worked for me. However it looks a bit like a hack for me.

I'll probably refine it a bit before merging, but I don't think it is a hack. It reflects the behaviour that makes sense when registering devices hanging off the root node. If a device node is a child of the root, then we know it isn't hanging off an i2c or pci bus, or anything else. It is essentially a system device.

Quoted 9 lines

The troublesome bit is that the root node also has memory, cpus, chosen and aliases nodes which are not devices. In the vast majority of cases, we want all the device nodes that are children of the root to be registered, but we don't want to register the special nodes. Checking for the presence of a compatible property is a pretty good test for determining whether or not a node actually represents a device, especially because all users of of_platform_bus_probe() seem to be FDT users where we've been very strict about enforcing that drivers must use the compatible property for matching to device nodes.

Now it's clear to me, thanks for the explanation.

BTW: On 2.6.35-rc6 I had to make 'compat' and 'match' variables const.

Signature · 7 lines
With best wishes
Dmitry

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
#14 lore raw
In reply to Dmitry Eremin-Solenikov

Hi Dmitry,

Quoted 19 lines · Dmitry Eremin-Solenikov

Add definitions for PCI error detection device to be handled by (already existing) mpc85xx_edac.c driver.

---
 arch/powerpc/boot/dts/mpc8536ds.dts        |    7 +++++++
 arch/powerpc/boot/dts/mpc8536ds_36b.dts    |    7 +++++++
 arch/powerpc/boot/dts/mpc8540ads.dts       |    8 ++++++++
 arch/powerpc/boot/dts/mpc8541cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8544ds.dts        |    7 +++++++
 arch/powerpc/boot/dts/mpc8548cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8555cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8560ads.dts       |    7 +++++++
 arch/powerpc/boot/dts/mpc8568mds.dts       |    7 +++++++
 arch/powerpc/boot/dts/sbc8548.dts          |    7 +++++++
 arch/powerpc/boot/dts/sbc8560.dts          |    7 +++++++
 arch/powerpc/boot/dts/socrates.dts         |    7 +++++++
 arch/powerpc/boot/dts/stx_gp3_8560.dts     |    7 +++++++
 arch/powerpc/boot/dts/tqm8540.dts          |   10 ++++++++++
Quoted 8 lines
 arch/powerpc/boot/dts/tqm8541.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8548-bigflash.dts |    7 +++++++
 arch/powerpc/boot/dts/tqm8548.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8555.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8560.dts          |    7 +++++++
 arch/powerpc/boot/dts/xpedite5200.dts      |    7 +++++++
 arch/powerpc/boot/dts/xpedite5200_xmon.dts |    7 +++++++
 21 files changed, 172 insertions(+), 0 deletions(-)

It looks like the dts files for the MPC8572-based boards weren't included in this change despite patch 2/2 adding support for them. I'd guess some other Freescale CPUs (eg P1020, P2020, etc) could be supported by the same driver if you are inclined to add them to this patch series.

Best,
Peter

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

#15 lore raw

Hello,

Quoted 31 lines · Peter Tyser

Hi Dmitry,

Quoted 21 lines · Dmitry Eremin-Solenikov

Add definitions for PCI error detection device to be handled by (already existing) mpc85xx_edac.c driver.

---
 arch/powerpc/boot/dts/mpc8536ds.dts        |    7 +++++++ arch/powerpc/boot/dts/mpc8536ds_36b.dts    |    7 +++++++ arch/powerpc/boot/dts/mpc8540ads.dts       |    8 ++++++++ arch/powerpc/boot/dts/mpc8541cds.dts       |   14 ++++++++++++++ arch/powerpc/boot/dts/mpc8544ds.dts        |    7 +++++++

 arch/powerpc/boot/dts/mpc8548cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8555cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8560ads.dts       |    7 +++++++
 arch/powerpc/boot/dts/mpc8568mds.dts       |    7 +++++++

 arch/powerpc/boot/dts/sbc8548.dts          |    7 +++++++ arch/powerpc/boot/dts/sbc8560.dts          |    7 +++++++ arch/powerpc/boot/dts/socrates.dts         |    7 +++++++ arch/powerpc/boot/dts/stx_gp3_8560.dts     |    7 +++++++ arch/powerpc/boot/dts/tqm8540.dts          |   10 ++++++++++ arch/powerpc/boot/dts/tqm8541.dts          |    7 +++++++ arch/powerpc/boot/dts/tqm8548-bigflash.dts |    7 +++++++

Quoted 6 lines
 arch/powerpc/boot/dts/tqm8548.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8555.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8560.dts          |    7 +++++++
 arch/powerpc/boot/dts/xpedite5200.dts      |    7 +++++++
 arch/powerpc/boot/dts/xpedite5200_xmon.dts |    7 +++++++
 21 files changed, 172 insertions(+), 0 deletions(-)
Quoted 5 lines

It looks like the dts files for the MPC8572-based boards weren't included in this change despite patch 2/2 adding support for them.  I'd guess some other Freescale CPUs (eg P1020, P2020, etc) could be supported by the same driver if you are inclined to add them to this patch series.

I just did a quick search for all dts including mpc8540-pci node and added respective mpc85xx-pci-error node. Current MPC85xx EDAC driver doesn't support error handling on PCI-E busses (which MPC8572 and other CPUs you mentioned have). I'll maybe look into PCI-E EDAC later. Also I don't have access to P10xx/P20xx manuals, so support for them may require some more time.

Signature · 7 lines
With best wishes
Dmitry

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
#16 lore raw
Quoted 36 lines
Quoted 30 lines · Dmitry Eremin-Solenikov
Quoted 19 lines

Add definitions for PCI error detection device to be handled by (already existing) mpc85xx_edac.c driver.

---
 arch/powerpc/boot/dts/mpc8536ds.dts        |    7 +++++++
 arch/powerpc/boot/dts/mpc8536ds_36b.dts    |    7 +++++++
 arch/powerpc/boot/dts/mpc8540ads.dts       |    8 ++++++++
 arch/powerpc/boot/dts/mpc8541cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8544ds.dts        |    7 +++++++
 arch/powerpc/boot/dts/mpc8548cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8555cds.dts       |   14 ++++++++++++++
 arch/powerpc/boot/dts/mpc8560ads.dts       |    7 +++++++
 arch/powerpc/boot/dts/mpc8568mds.dts       |    7 +++++++
 arch/powerpc/boot/dts/sbc8548.dts          |    7 +++++++
 arch/powerpc/boot/dts/sbc8560.dts          |    7 +++++++
 arch/powerpc/boot/dts/socrates.dts         |    7 +++++++
 arch/powerpc/boot/dts/stx_gp3_8560.dts     |    7 +++++++
 arch/powerpc/boot/dts/tqm8540.dts          |   10 ++++++++++
Quoted 8 lines
 arch/powerpc/boot/dts/tqm8541.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8548-bigflash.dts |    7 +++++++
 arch/powerpc/boot/dts/tqm8548.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8555.dts          |    7 +++++++
 arch/powerpc/boot/dts/tqm8560.dts          |    7 +++++++
 arch/powerpc/boot/dts/xpedite5200.dts      |    7 +++++++
 arch/powerpc/boot/dts/xpedite5200_xmon.dts |    7 +++++++
 21 files changed, 172 insertions(+), 0 deletions(-)
Quoted 5 lines

It looks like the dts files for the MPC8572-based boards weren't included in this change despite patch 2/2 adding support for them. I'd guess some other Freescale CPUs (eg P1020, P2020, etc) could be supported by the same driver if you are inclined to add them to this patch series.

Quoted 6 lines

I just did a quick search for all dts including mpc8540-pci node and added respective mpc85xx-pci-error node. Current MPC85xx EDAC driver doesn't support error handling on PCI-E busses (which MPC8572 and other CPUs you mentioned have). I'll maybe look into PCI-E EDAC later. Also I don't have access to P10xx/P20xx manuals, so support for them may require some more time.

Thanks for the explanation. I noticed this because patch 2/2 adds support for the 8572 to the mpc85xx_pci_err_of_match[] table. Sounds like that was the bug instead of my comment above.

Best,
Peter

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first