Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
On 7/24/10, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Jul 23, 2010 at 6:20 PM, Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On 7/22/10, Grant Likely grant.lik...@secretlab.ca wrote: On Thu, Jul 22, 2010 at 10:48 AM, Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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 something)? 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. 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. -- With best wishes Dmitry ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
Hello, On 7/22/10, Grant Likely grant.lik...@secretlab.ca wrote: On Thu, Jul 22, 2010 at 10:48 AM, Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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 something)? 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. -- With best wishes Dmitry ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
On Fri, Jul 23, 2010 at 6:20 PM, Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On 7/22/10, Grant Likely grant.lik...@secretlab.ca wrote: On Thu, Jul 22, 2010 at 10:48 AM, Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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 something)? 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
Hello, On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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 something)? BTW: While I'm at it, should I change all mpc8540-pci/-pcix device names to include respective SoC name? -- With best wishes Dmitry ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
On Thu, 22 Jul 2010 20:48:15 +0400 Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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. 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
On Jul 22, 2010, at 1:25 PM, Scott Wood wrote: On Thu, 22 Jul 2010 20:48:15 +0400 Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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. 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? I'd be good with that solution. - k ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
Hello, On Thu, Jul 22, 2010 at 10:40 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 22, 2010, at 1:25 PM, Scott Wood wrote: On Thu, 22 Jul 2010 20:48:15 +0400 Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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. 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? 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? -- With best wishes Dmitry ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
On Thu, Jul 22, 2010 at 10:48 AM, Dmitry Eremin-Solenikov
dbarysh...@gmail.com wrote:
Hello,
On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote:
On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote:
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.
Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com
---
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 something)?
Try the attached patch (lightly tested). If it works for you then
I'll post it for wider review.
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 grant.lik...@secretlab.ca
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(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index f3f1ec8..2ead562 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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;
--
1.7.0.4
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
On Thu, Jul 22, 2010 at 1:03 PM, Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 10:40 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 22, 2010, at 1:25 PM, Scott Wood wrote: On Thu, 22 Jul 2010 20:48:15 +0400 Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 7:38 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 21, 2010, at 7:03 PM, Dmitry Eremin-Solenikov wrote: 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. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com --- 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. 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? 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. Two initial suggestions were: 1) creating special platform device 2) creating special of_platform device from dts g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mpc85xx_edac: change to use new definitions for PCI EDAC regspace
On Thu, 22 Jul 2010 23:03:03 +0400 Dmitry Eremin-Solenikov dbarysh...@gmail.com wrote: Hello, On Thu, Jul 22, 2010 at 10:40 PM, Kumar Gala ga...@kernel.crashing.org wrote: On Jul 22, 2010, at 1:25 PM, Scott Wood wrote: 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 Right, that's why I suggested creating a platform device rather than just a function call. 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
