[alsa-devel] ASoC driver parts probing order (MPC5200/MPC5121)
Hi all,
I am writing a AC97 ASoC driver for the MPC5121e SoC from Freescale. This SoC has almost the same PSC (Programmable Serial Controllers) as the MPC5200B, for which there already is an AC97 driver: sound/soc/fsl/mpc5200-ac97.c, so I'd like to extend that one to also support the MPC5121e. This driver is supposed to work together with a second part, implementing the actual PCM driver: sound/soc/fsl/mpc5200_dma.c. Both drivers register themselves via platform_driver_register() in a regular module init function. The problem is, they do share the same driver data struct! So psc_ac97_of_probe() starts off calling
psc_dma = dev_get_drvdata(&op->dev);
right ahead and accessing it without checking. The DMA part of the driver does indeed create this struct psc_dma and calls dev_set_drvdata() on it at the end of the probe function. So obviously, it is supposed that the DMA driver somehow gets probed before the PSC driver, but I can't see where this is enforced. AFAIK, the order is fairly random, so it could be the other way around.... and indeed, in my case it is. Unfortunately I don't have MPC5200 hardware with AC97 to test this out, but only a MPC5121e based board. If I mimic this on the MPC5121e, it won't work, because the PSC driver gets probed first and obviously crashes with a NULL-pointer dereference in the next line of code!
If this is supposed to be broken, I'd like to fix it of course, but:
1.- I can't test it on a MPC5200B, so therefor I need help. 2.- Simply turning the dev_get/set_drvdata and chip initialization order around does work in my case, but doesn't seem a robust solution to me. Any idea how this should be handled?
Best regards,
Hi David,
I am writing a AC97 ASoC driver for the MPC5121e SoC from Freescale. This SoC has almost the same PSC (Programmable Serial Controllers) as the MPC5200B, for which there already is an AC97 driver: sound/soc/fsl/mpc5200-ac97.c, so I'd like to extend that one to also support the MPC5121e.
Yes, this seems feasible. It has been done like this for the uart-driver, sadly not for the spi-driver :(
So obviously, it is supposed that the DMA driver somehow gets probed before the PSC driver, but I can't see where this is enforced. AFAIK, the order is fairly random, so it could be the other way
Check arch/powerpc/sysdev/bestcomm/bestcomm.c at the end:
/* If we're not a module, we must make sure everything is setup before */ /* anyone tries to use us ... that's why we use subsys_initcall instead */ /* of module_init. */ subsys_initcall(mpc52xx_bcom_init);
while the mpc5121-driver has simply module_init() here. subsys_initcall() is also often used for I2C host drivers to ensure client drivers can access them early.
1.- I can't test it on a MPC5200B, so therefor I need help.
I can do tests.
Regards,
Wolfram
Hi Wolfram,
Thanks for replying.
On Thu, 20 Oct 2011 12:59:40 +0200 Wolfram Sang w.sang@pengutronix.de wrote:
Hi David,
I am writing a AC97 ASoC driver for the MPC5121e SoC from Freescale. This SoC has almost the same PSC (Programmable Serial Controllers) as the MPC5200B, for which there already is an AC97 driver: sound/soc/fsl/mpc5200-ac97.c, so I'd like to extend that one to also support the MPC5121e.
Yes, this seems feasible. It has been done like this for the uart-driver, sadly not for the spi-driver :(
So obviously, it is supposed that the DMA driver somehow gets probed before the PSC driver, but I can't see where this is enforced. AFAIK, the order is fairly random, so it could be the other way
Check arch/powerpc/sysdev/bestcomm/bestcomm.c at the end:
/* If we're not a module, we must make sure everything is setup before */ /* anyone tries to use us ... that's why we use subsys_initcall instead */ /* of module_init. */ subsys_initcall(mpc52xx_bcom_init);
while the mpc5121-driver has simply module_init() here. subsys_initcall() is also often used for I2C host drivers to ensure client drivers can access them early.
I think I was not clear enough. I meant sound/soc/fsl/mpc5200_dma.c, not the actual Bestcom DMA driver. Btw, I am writing a DMA-less driver for the MPC5121e for now. It uses the PSC FIFO and FIFO alarm interrupts. DMA support could be added later, when de MPC5121 DMA driver becomes capable of doing device IO. But, if I extend sound/soc/fsl/mpc5200-ac97.c, I will still need to write a second part doing the actual PCM audio stuff... basically the same thing sound/soc/fsl/mpc5200_dma.c does, but without DMA. It's the shared drv_data struct that goes sour.
1.- I can't test it on a MPC5200B, so therefor I need help.
I can do tests.
If you could try out latest mainline, to see if the driver still works, that would be great for a start. I am almost certain it either doesn't work anymore, or you have a 50% chance it crashes while booting.
Best regards,
I think I was not clear enough. I meant sound/soc/fsl/mpc5200_dma.c, not the actual Bestcom DMA driver.
Ah, okay, sorry misunderstood that.
Btw, I am writing a DMA-less driver for the MPC5121e for now. It uses the PSC FIFO and FIFO alarm interrupts. DMA support could be added later, when de MPC5121 DMA driver becomes capable of doing device IO.
OK, seems sensible for a start. MPC5121 does not get much love, so small steps are definately a good idea.
1.- I can't test it on a MPC5200B, so therefor I need help.
I can do tests.
If you could try out latest mainline, to see if the driver still works, that would be great for a start. I am almost certain it either doesn't work anymore, or you have a 50% chance it crashes while booting.
OK, I will leave for Prague in the next hours, so until I return, I can do only remote testing. But I should see OOPSes this way nonetheless. Might take a few days, though...
Regards,
Wolfram
Hi Wolfram!
On Thu, 20 Oct 2011 14:13:03 +0200 Wolfram Sang w.sang@pengutronix.de wrote:
I think I was not clear enough. I meant sound/soc/fsl/mpc5200_dma.c, not the actual Bestcom DMA driver.
Ah, okay, sorry misunderstood that.
Never mind, I was too hasty to explain correctly what I meant ;-)
Btw, I am writing a DMA-less driver for the MPC5121e for now. It uses the PSC FIFO and FIFO alarm interrupts. DMA support could be added later, when de MPC5121 DMA driver becomes capable of doing device IO.
OK, seems sensible for a start. MPC5121 does not get much love, so small steps are definately a good idea.
Yes :-( This platform could get a big boost just by having open-sourced GPU drivers for example, but that is never going to happen, and the binary drivers are completely broken and useless.... so no GPU here.
1.- I can't test it on a MPC5200B, so therefor I need help.
I can do tests.
If you could try out latest mainline, to see if the driver still works, that would be great for a start. I am almost certain it either doesn't work anymore, or you have a 50% chance it crashes while booting.
OK, I will leave for Prague in the next hours, so until I return, I can do only remote testing. But I should see OOPSes this way nonetheless. Might take a few days, though...
Ah, you have it hooked up the the Pengutronix remote workbench? Cool! Have a nice trip :-)
Best regards,
On Thu, Oct 20, 2011 at 12:23:17PM +0200, David Jander wrote:
of the probe function. So obviously, it is supposed that the DMA driver somehow gets probed before the PSC driver, but I can't see where this is enforced. AFAIK, the order is fairly random, so it could be the other way around.... and indeed, in my case it is.
The order the device model devices get probed in is entirely random and any driver which is relying on this is buggy.
2.- Simply turning the dev_get/set_drvdata and chip initialization order around does work in my case, but doesn't seem a robust solution to me. Any idea how this should be handled?
I'd expect that delaying whatever relies on both devices being there until the ASoC level probes happen is the way forward - you're guaranteed that both deivces will be present there and the probe ordering is guaranteed stable.
On Thu, 20 Oct 2011 13:35:13 +0100 Mark Brown broonie@opensource.wolfsonmicro.com wrote:
of the probe function. So obviously, it is supposed that the DMA driver somehow gets probed before the PSC driver, but I can't see where this is enforced. AFAIK, the order is fairly random, so it could be the other way around.... and indeed, in my case it is.
The order the device model devices get probed in is entirely random and any driver which is relying on this is buggy.
That's what I understood, but I was being reluctant to fix something I couldn't prove to be broken, nor being able to verify the fix. Fortunately Wolfram Sang offered to help out with testing on the MPC5200B.
2.- Simply turning the dev_get/set_drvdata and chip initialization order around does work in my case, but doesn't seem a robust solution to me. Any idea how this should be handled?
I'd expect that delaying whatever relies on both devices being there until the ASoC level probes happen is the way forward - you're guaranteed that both deivces will be present there and the probe ordering is guaranteed stable.
Thanks. I will investigate that path. If you have a quick look at the named files(mpc5200-ac97.c and mpc5200_dma.c) in mainline, can you confirm that it is indeed the wrong way of doing this?
Best regards,
participants (3)
-
David Jander -
Mark Brown -
Wolfram Sang