| Message ID | 1322669957-8259-1-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Kumar Gala |
| Headers | show |
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.
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(-) This seems like paper taping over the real issue. We should be able to call of_platform_bus_probe() multiple times. - k
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.
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.
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"; }; };
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.
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()?
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: >> >>> 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. Does of_platform_populate() handle this? - k
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.
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. >
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.
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
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);
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(-)