[PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Timur Tabi]

Commit 46d026ac ("powerpc/85xx: consolidate of_platform_bus_probe calls")
replaced platform-specific of_device_id tables with a single function
that probes the most of the busses in 85xx device trees.  If a specific
platform needed additional busses probed, then it could call
of_platform_bus_probe() again.  Typically, the additional platform-specific
busses are children of existing busses that have already been probed.
of_platform_bus_probe() does not handle those child busses automatically.

Unfortunately, this doesn't actually work.  The second (platform-specific)
call to of_platform_bus_probe() never finds any of the busses it's asked
to find.

To remedy this, the platform-specific of_device_id tables are eliminated,
and their entries are merged into mpc85xx_common_ids[], so that all busses
are probed at once.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/platforms/85xx/common.c      |    6 ++++++
 arch/powerpc/platforms/85xx/mpc85xx_mds.c |   11 +----------
 arch/powerpc/platforms/85xx/p1022_ds.c    |   13 +------------
 3 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/common.c b/arch/powerpc/platforms/85xx/common.c
index 9fef530..67dac22 100644
--- a/arch/powerpc/platforms/85xx/common.c
+++ b/arch/powerpc/platforms/85xx/common.c
@@ -21,6 +21,12 @@ static struct of_device_id __initdata mpc85xx_common_ids[] = {
 	{ .compatible = "fsl,qe", },
 	{ .compatible = "fsl,cpm2", },
 	{ .compatible = "fsl,srio", },
+	/* So that the DMA channel nodes can be probed individually: */
+	{ .compatible = "fsl,eloplus-dma", },
+	/* For the PMC driver */
+	{ .compatible = "fsl,mpc8548-guts", },
+	/* Probably unnecessary? */
+	{ .compatible = "gpio-leds", },
 	{},
 };
 
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
index 495cfd9..4a555ee 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
@@ -410,12 +410,6 @@ static int __init board_fixups(void)
 machine_arch_initcall(mpc8568_mds, board_fixups);
 machine_arch_initcall(mpc8569_mds, board_fixups);
 
-static struct of_device_id mpc85xx_ids[] = {
-	{ .compatible = "fsl,mpc8548-guts", },
-	{ .compatible = "gpio-leds", },
-	{},
-};
-
 static int __init mpc85xx_publish_devices(void)
 {
 	if (machine_is(mpc8568_mds))
@@ -423,10 +417,7 @@ static int __init mpc85xx_publish_devices(void)
 	if (machine_is(mpc8569_mds))
 		simple_gpiochip_init("fsl,mpc8569mds-bcsr-gpio");
 
-	mpc85xx_common_publish_devices();
-	of_platform_bus_probe(NULL, mpc85xx_ids, NULL);
-
-	return 0;
+	return mpc85xx_common_publish_devices();
 }
 
 machine_device_initcall(mpc8568_mds, mpc85xx_publish_devices);
diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
index 2bf4342..ea6190b 100644
--- a/arch/powerpc/platforms/85xx/p1022_ds.c
+++ b/arch/powerpc/platforms/85xx/p1022_ds.c
@@ -326,18 +326,7 @@ static void __init p1022_ds_setup_arch(void)
 	pr_info("Freescale P1022 DS reference board\n");
 }
 
-static struct of_device_id __initdata p1022_ds_ids[] = {
-	/* So that the DMA channel nodes can be probed individually: */
-	{ .compatible = "fsl,eloplus-dma", },
-	{},
-};
-
-static int __init p1022_ds_publish_devices(void)
-{
-	mpc85xx_common_publish_devices();
-	return of_platform_bus_probe(NULL, p1022_ds_ids, NULL);
-}
-machine_device_initcall(p1022_ds, p1022_ds_publish_devices);
+machine_device_initcall(p1022_ds, mpc85xx_common_publish_devices);
 
 machine_arch_initcall(p1022_ds, swiotlb_setup_bus_notifier);
 
-- 
1.7.3.4

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Timur Tabi]

Timur Tabi wrote:
> Commit 46d026ac ("powerpc/85xx: consolidate of_platform_bus_probe calls")
> replaced platform-specific of_device_id tables with a single function
> that probes the most of the busses in 85xx device trees.  If a specific
> platform needed additional busses probed, then it could call
> of_platform_bus_probe() again.  Typically, the additional platform-specific
> busses are children of existing busses that have already been probed.
> of_platform_bus_probe() does not handle those child busses automatically.
> 
> Unfortunately, this doesn't actually work.  The second (platform-specific)
> call to of_platform_bus_probe() never finds any of the busses it's asked
> to find.
> 
> To remedy this, the platform-specific of_device_id tables are eliminated,
> and their entries are merged into mpc85xx_common_ids[], so that all busses
> are probed at once.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---

Kumar, without this patch, audio is broken on the P1022DS in your 'merge'
branch.  I posted it back in November, six days after you applied the
patch that broke the P1022DS.

Is there any chance of getting this into 3.3?  This is a real bug fix, and
I would hate for audio to be broken in 3.3.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Kumar Gala]

On Nov 30, 2011, at 10:19 AM, Timur Tabi wrote:

> Commit 46d026ac ("powerpc/85xx: consolidate of_platform_bus_probe =
calls")
> replaced platform-specific of_device_id tables with a single function
> that probes the most of the busses in 85xx device trees.  If a =
specific
> platform needed additional busses probed, then it could call
> of_platform_bus_probe() again.  Typically, the additional =
platform-specific
> busses are children of existing busses that have already been probed.
> of_platform_bus_probe() does not handle those child busses =
automatically.
>=20
> Unfortunately, this doesn't actually work.  The second =
(platform-specific)
> call to of_platform_bus_probe() never finds any of the busses it's =
asked
> to find.
>=20
> To remedy this, the platform-specific of_device_id tables are =
eliminated,
> and their entries are merged into mpc85xx_common_ids[], so that all =
busses
> are probed at once.
>=20
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> arch/powerpc/platforms/85xx/common.c      |    6 ++++++
> arch/powerpc/platforms/85xx/mpc85xx_mds.c |   11 +----------
> arch/powerpc/platforms/85xx/p1022_ds.c    |   13 +------------
> 3 files changed, 8 insertions(+), 22 deletions(-)

This seems like paper taping over the real issue.  We should be able to =
call of_platform_bus_probe() multiple times.

- k=

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Timur Tabi]

Kumar Gala wrote:

> This seems like paper taping over the real issue.  We should be able to call of_platform_bus_probe() multiple times.

I tried debugging it, but I couldn't figure it out.  My guess is that the
nodes probed by of_platform_bus_probe() are somehow "reserved", so that
the second time it's called, they're skipped.  I figured that this was
just a side-effect of the way the OF layer works.

If someone more familiar with the OF layer wants to look at it, that's
great.  Until then, my patch is the only one that allows the audio and
async DMA drivers to work, and any other drivers that probe on nodes that
are grandchildren of the root node.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Grant Likely]

On Fri, 16 Mar 2012 15:50:44 -0500, Timur Tabi <timur@freescale.com> wrote:
> Kumar Gala wrote:
> 
> > This seems like paper taping over the real issue.  We should be able to call of_platform_bus_probe() multiple times.
> 
> I tried debugging it, but I couldn't figure it out.  My guess is that the
> nodes probed by of_platform_bus_probe() are somehow "reserved", so that
> the second time it's called, they're skipped.  I figured that this was
> just a side-effect of the way the OF layer works.

The problem is that you want to create devices for grandchildren
nodes when the bus ids passed in don't match any of the child nodes so
the of_platform_bus_probe() doesn't iterate down to that level.  This
is correct and expected behaviour.

g.

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Timur Tabi]

Grant Likely wrote:
> The problem is that you want to create devices for grandchildren
> nodes when the bus ids passed in don't match any of the child nodes so
> the of_platform_bus_probe() doesn't iterate down to that level.  This
> is correct and expected behaviour.

Well, I'm not still not 100% sure on what I'm supposed to do, so I tried this:

static struct of_device_id __initdata p1022_ds_ids[] = {
	/* The audio driver probes the SSI DMA channels individually */
	{ .compatible = "fsl,ssi-dma-channel", },
	{},
};

static int __init p1022_ds_publish_devices(void)
{
	struct device_node *np;
	int ret;

	mpc85xx_common_publish_devices();

	for_each_compatible_node(np, NULL, "fsl,eloplus-dma") {
		ret = of_platform_bus_probe(np, p1022_ds_ids, NULL);
		if (ret)
			return ret;
	}

	return 0;
}

It works, but it looks clunky.

For the record, this is what the DMA controller node looks like.  It's a
child of the SOC node.  The SSI driver probes on the "fsl,ssi-dma-channel"
nodes directly.

dma@c300 {
        cell-index = <0x1>;
        ranges = <0x0 0xc100 0x200>;
        reg = <0xc300 0x4>;
        compatible = "fsl,eloplus-dma";
        #size-cells = <0x1>;
        #address-cells = <0x1>;

        dma-channel@180 {
                interrupts = <0x4f 0x2 0x0 0x0>;
                cell-index = <0x3>;
                reg = <0x180 0x80>;
                compatible = "fsl,eloplus-dma-channel";
        };

        dma-channel@100 {
                interrupts = <0x4e 0x2 0x0 0x0>;
                cell-index = <0x2>;
                reg = <0x100 0x80>;
                compatible = "fsl,eloplus-dma-channel";
        };

        dma-channel@80 {
                phandle = <0x4>;
                linux,phandle = <0x4>;
                interrupts = <0x4d 0x2 0x0 0x0>;
                cell-index = <0x1>;
                reg = <0x80 0x80>;
                compatible = "fsl,ssi-dma-channel";
        };

        dma-channel@0 {
                phandle = <0x3>;
                linux,phandle = <0x3>;
                interrupts = <0x4c 0x2 0x0 0x0>;
                cell-index = <0x0>;
                reg = <0x0 0x80>;
                compatible = "fsl,ssi-dma-channel";
        };
};

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Grant Likely]

On Mon, 19 Mar 2012 13:43:29 -0500, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
> > The problem is that you want to create devices for grandchildren
> > nodes when the bus ids passed in don't match any of the child nodes so
> > the of_platform_bus_probe() doesn't iterate down to that level.  This
> > is correct and expected behaviour.
> 
> Well, I'm not still not 100% sure on what I'm supposed to do, so I tried this:
> 
> static struct of_device_id __initdata p1022_ds_ids[] = {
> 	/* The audio driver probes the SSI DMA channels individually */
> 	{ .compatible = "fsl,ssi-dma-channel", },
> 	{},
> };
> 
> static int __init p1022_ds_publish_devices(void)
> {
> 	struct device_node *np;
> 	int ret;
> 
> 	mpc85xx_common_publish_devices();
> 
> 	for_each_compatible_node(np, NULL, "fsl,eloplus-dma") {
> 		ret = of_platform_bus_probe(np, p1022_ds_ids, NULL);
> 		if (ret)
> 			return ret;

Does a driver bind against "fsl,eloplus-dma"?  If so, then I would
call of_platform_populate() from within the fsl,eloplus-dma driver's
probe method.

g.

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Timur Tabi]

Grant Likely wrote:

> Does a driver bind against "fsl,eloplus-dma"?  If so, then I would
> call of_platform_populate() from within the fsl,eloplus-dma driver's
> probe method.

Well, there are two "DMA" drivers.

The one in drivers/dma binds on fsl,eloplus-dma, and then manually scans
for children with the "fsl,eloplus-dma-channel" compatible.  This driver
works today because the fsl,eloplus-dma nodes are children of the SOC
node, which is already probed.

The other is in sound/soc/fsl and binds on "fsl,ssi-dma-channel".  These
are also children of the fsl,eloplus-dma node.  This driver does NOT work
today, because the fsl,eloplus-dma is not probed.

I do not want the two drivers to depend on each other.  They are
completely separate.  So I don't want to call of_platform_populate() from
the drivers/dma driver.  I could call it from the sound/soc/fsl driver,
however.  I will try to see if that works.

They only problem I see with this is that I am thinking about modifying
the drivers/dma driver to probe on "fsl,eloplus-dma-channel" channels
directly.  If I do that, then who should call of_platform_populate()?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Kumar Gala]

On Mar 19, 2012, at 11:04 AM, Grant Likely wrote:

> On Fri, 16 Mar 2012 15:50:44 -0500, Timur Tabi <timur@freescale.com> =
wrote:
>> Kumar Gala wrote:
>>=20
>>> This seems like paper taping over the real issue.  We should be able =
to call of_platform_bus_probe() multiple times.
>>=20
>> I tried debugging it, but I couldn't figure it out.  My guess is that =
the
>> nodes probed by of_platform_bus_probe() are somehow "reserved", so =
that
>> the second time it's called, they're skipped.  I figured that this =
was
>> just a side-effect of the way the OF layer works.
>=20
> The problem is that you want to create devices for grandchildren
> nodes when the bus ids passed in don't match any of the child nodes so
> the of_platform_bus_probe() doesn't iterate down to that level.  This
> is correct and expected behaviour.
>=20
> g.

Does of_platform_populate() handle this?

- k=

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Timur Tabi]

Timur Tabi wrote:
> They only problem I see with this is that I am thinking about modifying
> the drivers/dma driver to probe on "fsl,eloplus-dma-channel" channels
> directly.  If I do that, then who should call of_platform_populate()?

Grant, could you tell me if there's anything actually work with my patch?
 All I'm doing is adding a couple more commonly used entries to
mpc85xx_common_ids[].  After all, they're common IDs, so don't they belong
into an array called common_ids?

I've been waiting for months for this problem to be fixed, and 3.3 is
broken without it.  We've already established that you cannot actually
call of_platform_bus_probe() twice on the same level, so it's not like my
patch description is wrong or anything.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Timur Tabi]

Grant, do you have a moment to consider my question?  Like I said, I'm
anxious to get a fix into 3.3.

Timur Tabi wrote:
> Timur Tabi wrote:
>> They only problem I see with this is that I am thinking about modifying
>> the drivers/dma driver to probe on "fsl,eloplus-dma-channel" channels
>> directly.  If I do that, then who should call of_platform_populate()?
> 
> Grant, could you tell me if there's anything actually work with my patch?
>  All I'm doing is adding a couple more commonly used entries to
> mpc85xx_common_ids[].  After all, they're common IDs, so don't they belong
> into an array called common_ids?
> 
> I've been waiting for months for this problem to be fixed, and 3.3 is
> broken without it.  We've already established that you cannot actually
> call of_platform_bus_probe() twice on the same level, so it's not like my
> patch description is wrong or anything.
> 


-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Grant Likely]

On Tue, 27 Mar 2012 13:18:10 -0500, Timur Tabi <timur@freescale.com> wrote:
> Grant, do you have a moment to consider my question?  Like I said, I'm
> anxious to get a fix into 3.3.

I've been out of town for the past week, so email processing volume
has been low.  Answer below.

> Timur Tabi wrote:
> > Timur Tabi wrote:
> >> They only problem I see with this is that I am thinking about modifying
> >> the drivers/dma driver to probe on "fsl,eloplus-dma-channel" channels
> >> directly.  If I do that, then who should call of_platform_populate()?
> > 
> > Grant, could you tell me if there's anything actually work with my patch?
> >  All I'm doing is adding a couple more commonly used entries to
> > mpc85xx_common_ids[].  After all, they're common IDs, so don't they belong
> > into an array called common_ids?

That's between you and Kumar.  I don't have any problem with it since
it's all contained within the mpc85xx code.

If those nodes really do make sense to represent as independent
platform devices, then adding them to the match list for bus types
isn't a problem.

g.

Re: [PATCH] powerpc/85xx: don't call of_platform_bus_probe() twice

[Kumar Gala]

On Nov 30, 2011, at 10:19 AM, Timur Tabi wrote:

> Commit 46d026ac ("powerpc/85xx: consolidate of_platform_bus_probe calls")
> replaced platform-specific of_device_id tables with a single function
> that probes the most of the busses in 85xx device trees.  If a specific
> platform needed additional busses probed, then it could call
> of_platform_bus_probe() again.  Typically, the additional platform-specific
> busses are children of existing busses that have already been probed.
> of_platform_bus_probe() does not handle those child busses automatically.
> 
> Unfortunately, this doesn't actually work.  The second (platform-specific)
> call to of_platform_bus_probe() never finds any of the busses it's asked
> to find.
> 
> To remedy this, the platform-specific of_device_id tables are eliminated,
> and their entries are merged into mpc85xx_common_ids[], so that all busses
> are probed at once.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> arch/powerpc/platforms/85xx/common.c      |    6 ++++++
> arch/powerpc/platforms/85xx/mpc85xx_mds.c |   11 +----------
> arch/powerpc/platforms/85xx/p1022_ds.c    |   13 +------------
> 3 files changed, 8 insertions(+), 22 deletions(-)

applied to merge

- k