| Message ID | 20090629234214.12670.9082.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Grant Likely |
| Headers | show |
On Mon, Jun 29, 2009 at 7:42 PM, Grant Likely<grant.likely@secretlab.ca> wrote: > From: Grant Likely <grant.likely@secretlab.ca> > > AC97 bus register read/write hooks need to provide locking, but the > mpc5200-psc-ac97 driver does not. This patch adds a mutex around > the register access routines. Does your touchscreen driver use this mutex? Or was this mutex needed just for the AC97 driver?
Hi Grant, On Mon, Jun 29, 2009 at 05:42:21PM -0600, Grant Likely wrote: > From: Grant Likely <grant.likely@secretlab.ca> > > AC97 bus register read/write hooks need to provide locking, but the > mpc5200-psc-ac97 driver does not. This patch adds a mutex around > the register access routines. > > Signed-off-by: Grant Likely <grant.likely@secretlab.ca> > --- > > sound/soc/fsl/mpc5200_dma.c | 1 + > sound/soc/fsl/mpc5200_dma.h | 1 + > sound/soc/fsl/mpc5200_psc_ac97.c | 17 ++++++++++++++++- > 3 files changed, 18 insertions(+), 1 deletions(-) > > > diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c > index efec33a..f0a2d40 100644 > --- a/sound/soc/fsl/mpc5200_dma.c > +++ b/sound/soc/fsl/mpc5200_dma.c > @@ -456,6 +456,7 @@ int mpc5200_audio_dma_create(struct of_device *op) > return -ENODEV; > > spin_lock_init(&psc_dma->lock); > + mutex_init(&psc_dma->mutex); > psc_dma->id = be32_to_cpu(*prop); > psc_dma->irq = irq; > psc_dma->psc_regs = regs; > diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h > index 2000803..8d396bb 100644 > --- a/sound/soc/fsl/mpc5200_dma.h > +++ b/sound/soc/fsl/mpc5200_dma.h > @@ -55,6 +55,7 @@ struct psc_dma { > unsigned int irq; > struct device *dev; > spinlock_t lock; > + struct mutex mutex; > u32 sicr; > uint sysclk; > int imr; > diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c > index 794a247..7eb5499 100644 > --- a/sound/soc/fsl/mpc5200_psc_ac97.c > +++ b/sound/soc/fsl/mpc5200_psc_ac97.c > @@ -34,13 +34,20 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) > int status; > unsigned int val; > > + mutex_lock(&psc_dma->mutex); > + > /* Wait for command send status zero = ready */ > status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & > MPC52xx_PSC_SR_CMDSEND), 100, 0); > if (status == 0) { > pr_err("timeout on ac97 bus (rdy)\n"); > + mutex_unlock(&psc_dma->mutex); > return -ENODEV; > } > + > + /* Force clear the data valid bit */ > + in_be32(&psc_dma->psc_regs->ac97_data); > + No mutex involved here. I think this is either a seperate patch or it needs at least to be mentioned in the patch description. > /* Send the read */ > out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24)); > > @@ -50,16 +57,19 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) > if (status == 0) { > pr_err("timeout on ac97 read (val) %x\n", > in_be16(&psc_dma->psc_regs->sr_csr.status)); > + mutex_unlock(&psc_dma->mutex); > return -ENODEV; > } > /* Get the data */ > val = in_be32(&psc_dma->psc_regs->ac97_data); > if (((val >> 24) & 0x7f) != reg) { > pr_err("reg echo error on ac97 read\n"); > + mutex_unlock(&psc_dma->mutex); > return -ENODEV; > } > val = (val >> 8) & 0xffff; > > + mutex_unlock(&psc_dma->mutex); > return (unsigned short) val; > } > > @@ -68,16 +78,21 @@ static void psc_ac97_write(struct snd_ac97 *ac97, > { > int status; > > + mutex_lock(&psc_dma->mutex); > + > /* Wait for command status zero = ready */ > status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & > MPC52xx_PSC_SR_CMDSEND), 100, 0); > if (status == 0) { > pr_err("timeout on ac97 bus (write)\n"); > - return; > + goto out; > } > /* Write data */ > out_be32(&psc_dma->psc_regs->ac97_cmd, > ((reg & 0x7f) << 24) | (val << 8)); > + > + out: > + mutex_unlock(&psc_dma->mutex); > } > > static void psc_ac97_warm_reset(struct snd_ac97 *ac97) > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
On Mon, Jun 29, 2009 at 08:26:12PM -0400, Jon Smirl wrote: > Does your touchscreen driver use this mutex? Or was this mutex needed > just for the AC97 driver? It's in the register accesses so everything accessing the chip registers will use it.
On Tue, Jun 30, 2009 at 2:18 AM, Wolfram Sang<w.sang@pengutronix.de> wrote: Wolfram, do you have any way to test the on the Phytec hardware? The WM9712 on the baseboard supports a touchscreen, is it brought out to a header? > Hi Grant, > > On Mon, Jun 29, 2009 at 05:42:21PM -0600, Grant Likely wrote: >> From: Grant Likely <grant.likely@secretlab.ca> >> >> AC97 bus register read/write hooks need to provide locking, but the >> mpc5200-psc-ac97 driver does not. This patch adds a mutex around >> the register access routines. >> >> Signed-off-by: Grant Likely <grant.likely@secretlab.ca> >> --- >> >> sound/soc/fsl/mpc5200_dma.c | 1 + >> sound/soc/fsl/mpc5200_dma.h | 1 + >> sound/soc/fsl/mpc5200_psc_ac97.c | 17 ++++++++++++++++- >> 3 files changed, 18 insertions(+), 1 deletions(-) >> >> >> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c >> index efec33a..f0a2d40 100644 >> --- a/sound/soc/fsl/mpc5200_dma.c >> +++ b/sound/soc/fsl/mpc5200_dma.c >> @@ -456,6 +456,7 @@ int mpc5200_audio_dma_create(struct of_device *op) >> return -ENODEV; >> >> spin_lock_init(&psc_dma->lock); >> + mutex_init(&psc_dma->mutex); >> psc_dma->id = be32_to_cpu(*prop); >> psc_dma->irq = irq; >> psc_dma->psc_regs = regs; >> diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h >> index 2000803..8d396bb 100644 >> --- a/sound/soc/fsl/mpc5200_dma.h >> +++ b/sound/soc/fsl/mpc5200_dma.h >> @@ -55,6 +55,7 @@ struct psc_dma { >> unsigned int irq; >> struct device *dev; >> spinlock_t lock; >> + struct mutex mutex; >> u32 sicr; >> uint sysclk; >> int imr; >> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c >> index 794a247..7eb5499 100644 >> --- a/sound/soc/fsl/mpc5200_psc_ac97.c >> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c >> @@ -34,13 +34,20 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) >> int status; >> unsigned int val; >> >> + mutex_lock(&psc_dma->mutex); >> + >> /* Wait for command send status zero = ready */ >> status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & >> MPC52xx_PSC_SR_CMDSEND), 100, 0); >> if (status == 0) { >> pr_err("timeout on ac97 bus (rdy)\n"); >> + mutex_unlock(&psc_dma->mutex); >> return -ENODEV; >> } >> + >> + /* Force clear the data valid bit */ >> + in_be32(&psc_dma->psc_regs->ac97_data); >> + > > No mutex involved here. I think this is either a seperate patch or it needs at > least to be mentioned in the patch description. > >> /* Send the read */ >> out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24)); >> >> @@ -50,16 +57,19 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) >> if (status == 0) { >> pr_err("timeout on ac97 read (val) %x\n", >> in_be16(&psc_dma->psc_regs->sr_csr.status)); >> + mutex_unlock(&psc_dma->mutex); >> return -ENODEV; >> } >> /* Get the data */ >> val = in_be32(&psc_dma->psc_regs->ac97_data); >> if (((val >> 24) & 0x7f) != reg) { >> pr_err("reg echo error on ac97 read\n"); >> + mutex_unlock(&psc_dma->mutex); >> return -ENODEV; >> } >> val = (val >> 8) & 0xffff; >> >> + mutex_unlock(&psc_dma->mutex); >> return (unsigned short) val; >> } >> >> @@ -68,16 +78,21 @@ static void psc_ac97_write(struct snd_ac97 *ac97, >> { >> int status; >> >> + mutex_lock(&psc_dma->mutex); >> + >> /* Wait for command status zero = ready */ >> status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & >> MPC52xx_PSC_SR_CMDSEND), 100, 0); >> if (status == 0) { >> pr_err("timeout on ac97 bus (write)\n"); >> - return; >> + goto out; >> } >> /* Write data */ >> out_be32(&psc_dma->psc_regs->ac97_cmd, >> ((reg & 0x7f) << 24) | (val << 8)); >> + >> + out: >> + mutex_unlock(&psc_dma->mutex); >> } >> >> static void psc_ac97_warm_reset(struct snd_ac97 *ac97) >> >> _______________________________________________ >> Linuxppc-dev mailing list >> Linuxppc-dev@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/linuxppc-dev > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkpJrkcACgkQD27XaX1/VRsY/QCgyx8IMANjokbNnrOQlXINRLeW > lT4AnAy3ES9r3wriGkRN7ivnLA3zyRHb > =RUPr > -----END PGP SIGNATURE----- > >
On Tue, Jun 30, 2009 at 09:42:06AM -0400, Jon Smirl wrote: > Wolfram, do you have any way to test the on the Phytec hardware? The > WM9712 on the baseboard supports a touchscreen, is it brought out to a > header? You can probably test adequately by writing a dummy AC97 driver that sits in a thread and does reads (which should be all that's required to test the interaction).
On Tue, Jun 30, 2009 at 12:18 AM, Wolfram Sang<w.sang@pengutronix.de> wrote: >> + >> + /* Force clear the data valid bit */ >> + in_be32(&psc_dma->psc_regs->ac97_data); >> + > > No mutex involved here. I think this is either a seperate patch or it needs at > least to be mentioned in the patch description. Oops, that was sloppy. Yes, I'll put this into a separate patch. Thanks. g.
On Tue, Jun 30, 2009 at 2:59 AM, Mark Brown<broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jun 29, 2009 at 08:26:12PM -0400, Jon Smirl wrote: > >> Does your touchscreen driver use this mutex? Or was this mutex needed >> just for the AC97 driver? > > It's in the register accesses so everything accessing the chip registers > will use it. The mutexes are needed. The ac97 bus doesn't have any knowledge about what codec(s) will be wired up to it so it must protect against multiple access. In my case both the wm9712 audio codec driver and the wm9712 touchscreen driver perform register accesses. g.
On Tue, Jun 30, 2009 at 12:50 PM, Grant Likely<grant.likely@secretlab.ca> wrote: > On Tue, Jun 30, 2009 at 12:18 AM, Wolfram Sang<w.sang@pengutronix.de> wrote: >>> + >>> + /* Force clear the data valid bit */ >>> + in_be32(&psc_dma->psc_regs->ac97_data); >>> + >> >> No mutex involved here. I think this is either a separate patch or it needs at >> least to be mentioned in the patch description. > > Oops, that was sloppy. Yes, I'll put this into a separate patch. Thanks. Now that you have added the mutexes, do you ever need to force clear the valid bit? Maybe log an error if this happens so that we can track down why.
On Tue, Jun 30, 2009 at 12:33 PM, Jon Smirl<jonsmirl@gmail.com> wrote: > On Tue, Jun 30, 2009 at 12:50 PM, Grant Likely<grant.likely@secretlab.ca> wrote: >> On Tue, Jun 30, 2009 at 12:18 AM, Wolfram Sang<w.sang@pengutronix.de> wrote: >>>> + >>>> + /* Force clear the data valid bit */ >>>> + in_be32(&psc_dma->psc_regs->ac97_data); >>>> + >>> >>> No mutex involved here. I think this is either a separate patch or it needs at >>> least to be mentioned in the patch description. >> >> Oops, that was sloppy. Yes, I'll put this into a separate patch. Thanks. > > Now that you have added the mutexes, do you ever need to force clear > the valid bit? > Maybe log an error if this happens so that we can track down why. I know exactly why it happened. spin_event_timeout() had a bug that would cause it to report a false positive timeout. The bug is fixed and queued for merging. However, I still think this explicit clear should be applied. It is just a cheap register read and it prevents the driver from getting wedged after a timeout does event, for whatever reason. g.
Hi Jon, > Wolfram, do you have any way to test the on the Phytec hardware? The > WM9712 on the baseboard supports a touchscreen, is it brought out to a > header? Sorry, I didn't get it: Shall I test something specific? The touchscreen connector is X19 BTW (assuming a PCM-973 baseboard). Regards, Wolfram
On Wed, Jul 1, 2009 at 4:56 AM, Wolfram Sang<w.sang@pengutronix.de> wrote: > Hi Jon, > >> Wolfram, do you have any way to test the on the Phytec hardware? The >> WM9712 on the baseboard supports a touchscreen, is it brought out to a >> header? > > Sorry, I didn't get it: Shall I test something specific? I don't own a touchscreen. AFAIK no one has ever plugged a touchscreen into the PCM-973 to see if works since there hasn't been a driver previously. Just do a general test so that you can tell customers that it works. > > The touchscreen connector is X19 BTW (assuming a PCM-973 baseboard). > > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.9 (GNU/Linux) > > iEYEARECAAYFAkpLJMEACgkQD27XaX1/VRs/cwCgn7p7ffxr8WGoW7MwALkBMKtD > VtYAoIa5/viinyvFF5cQP3BvH9WP3a5I > =Zrq3 > -----END PGP SIGNATURE----- > >
> > Sorry, I didn't get it: Shall I test something specific? > > I don't own a touchscreen. > > AFAIK no one has ever plugged a touchscreen into the PCM-973 to see if > works since there hasn't been a driver previously. Just do a general > test so that you can tell customers that it works. I am not sure, we have a suitable touchscreen. Also, I am afraid this is a bit too much to do besides my regular day work right now. Sorry. Regards, Wolfram
On Wed, Jul 1, 2009 at 7:55 AM, Wolfram Sang<w.sang@pengutronix.de> wrote: > >> > Sorry, I didn't get it: Shall I test something specific? >> >> I don't own a touchscreen. >> >> AFAIK no one has ever plugged a touchscreen into the PCM-973 to see if >> works since there hasn't been a driver previously. >> Just do a general >> test so that you can tell customers that it works. > > I am not sure, we have a suitable touchscreen. Also, I am afraid this is a bit > too much to do besides my regular day work right now. Sorry. I've now tested this on a client's board which uses the pcm030 with a custom base board (based on the development board) which uses the same codec. It works there. I don't have a discrete touchscreen to wire up to the PCM-973 though. g.
-----Original Message----- From: linuxppc-dev-bounces+emillbrandt=dekaresearch.com@lists.ozlabs.org [mailto:linuxppc-dev-bounces+emillbrandt=dekaresearch.com@lists.ozlabs.o rg] On Behalf Of Grant Likely Sent: Wednesday, July 01, 2009 10:45 To: Wolfram Sang Cc: linuxppc-dev@ozlabs.org; alsa-devel@alsa-project.org; broonie@sirena.org.uk Subject: Re: [PATCH] alsa/soc: add locking to mpc5200-psc-ac97 driver On Wed, Jul 1, 2009 at 7:55 AM, Wolfram Sang<w.sang@pengutronix.de> wrote: > >> > Sorry, I didn't get it: Shall I test something specific? >> >> I don't own a touchscreen. >> >> AFAIK no one has ever plugged a touchscreen into the PCM-973 to see if >> works since there hasn't been a driver previously. >> Just do a general >> test so that you can tell customers that it works. > > I am not sure, we have a suitable touchscreen. Also, I am afraid this is a bit > too much to do besides my regular day work right now. Sorry. I've now tested this on a client's board which uses the pcm030 with a custom base board (based on the development board) which uses the same codec. It works there. I don't have a discrete touchscreen to wire up to the PCM-973 though. g.
diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c index efec33a..f0a2d40 100644 --- a/sound/soc/fsl/mpc5200_dma.c +++ b/sound/soc/fsl/mpc5200_dma.c @@ -456,6 +456,7 @@ int mpc5200_audio_dma_create(struct of_device *op) return -ENODEV; spin_lock_init(&psc_dma->lock); + mutex_init(&psc_dma->mutex); psc_dma->id = be32_to_cpu(*prop); psc_dma->irq = irq; psc_dma->psc_regs = regs; diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h index 2000803..8d396bb 100644 --- a/sound/soc/fsl/mpc5200_dma.h +++ b/sound/soc/fsl/mpc5200_dma.h @@ -55,6 +55,7 @@ struct psc_dma { unsigned int irq; struct device *dev; spinlock_t lock; + struct mutex mutex; u32 sicr; uint sysclk; int imr; diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c index 794a247..7eb5499 100644 --- a/sound/soc/fsl/mpc5200_psc_ac97.c +++ b/sound/soc/fsl/mpc5200_psc_ac97.c @@ -34,13 +34,20 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) int status; unsigned int val; + mutex_lock(&psc_dma->mutex); + /* Wait for command send status zero = ready */ status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & MPC52xx_PSC_SR_CMDSEND), 100, 0); if (status == 0) { pr_err("timeout on ac97 bus (rdy)\n"); + mutex_unlock(&psc_dma->mutex); return -ENODEV; } + + /* Force clear the data valid bit */ + in_be32(&psc_dma->psc_regs->ac97_data); + /* Send the read */ out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24)); @@ -50,16 +57,19 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) if (status == 0) { pr_err("timeout on ac97 read (val) %x\n", in_be16(&psc_dma->psc_regs->sr_csr.status)); + mutex_unlock(&psc_dma->mutex); return -ENODEV; } /* Get the data */ val = in_be32(&psc_dma->psc_regs->ac97_data); if (((val >> 24) & 0x7f) != reg) { pr_err("reg echo error on ac97 read\n"); + mutex_unlock(&psc_dma->mutex); return -ENODEV; } val = (val >> 8) & 0xffff; + mutex_unlock(&psc_dma->mutex); return (unsigned short) val; } @@ -68,16 +78,21 @@ static void psc_ac97_write(struct snd_ac97 *ac97, { int status; + mutex_lock(&psc_dma->mutex); + /* Wait for command status zero = ready */ status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & MPC52xx_PSC_SR_CMDSEND), 100, 0); if (status == 0) { pr_err("timeout on ac97 bus (write)\n"); - return; + goto out; } /* Write data */ out_be32(&psc_dma->psc_regs->ac97_cmd, ((reg & 0x7f) << 24) | (val << 8)); + + out: + mutex_unlock(&psc_dma->mutex); } static void psc_ac97_warm_reset(struct snd_ac97 *ac97)