ASoC driver parts probing order (MPC5200/MPC5121)

ASoC driver parts probing order (MPC5200/MPC5121)

[David Jander]

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,

-- 
David Jander
Protonic Holland.

Re: ASoC driver parts probing order (MPC5200/MPC5121)

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1340 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: ASoC driver parts probing order (MPC5200/MPC5121)

[David Jander]

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,

-- 
David Jander
Protonic Holland.

Re: ASoC driver parts probing order (MPC5200/MPC5121)

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1162 bytes --]


> 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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: ASoC driver parts probing order (MPC5200/MPC5121)

[David Jander]

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,

-- 
David Jander
Protonic Holland.

Re: ASoC driver parts probing order (MPC5200/MPC5121)

[Mark Brown]

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.

Re: ASoC driver parts probing order (MPC5200/MPC5121)

[David Jander]

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,

-- 
David Jander
Protonic Holland.