[PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared

[PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

When doing register reads, it is possible for there to be a stale
data ready bit set which will cause subsequent reads to return
prematurely with incorrect data.  This patch fixes the issues by
ensuring stale data is cleared before starting another transaction.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 sound/soc/fsl/mpc5200_psc_ac97.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)


diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c
index 794a247..9b8503f 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -41,6 +41,10 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
 		pr_err("timeout on ac97 bus (rdy)\n");
 		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));
 

[PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver

[Grant Likely]

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 |   13 ++++++++++++-
 3 files changed, 14 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 9b8503f..7eb5499 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -34,11 +34,14 @@ 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;
 	}
 
@@ -54,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;
 }
 
@@ -72,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)

Re: [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared

[Jon Smirl]

On Thu, Jul 2, 2009 at 1:57 PM, Grant Likely<grant.likely@secretlab.ca> wro=
te:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> When doing register reads, it is possible for there to be a stale
> data ready bit set which will cause subsequent reads to return
> prematurely with incorrect data. =A0This patch fixes the issues by
> ensuring stale data is cleared before starting another transaction.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Jon Smirl <jonsmirl@gmail.com>

> ---
>
> =A0sound/soc/fsl/mpc5200_psc_ac97.c | =A0 =A04 ++++
> =A01 files changed, 4 insertions(+), 0 deletions(-)
>
>
> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc=
_ac97.c
> index 794a247..9b8503f 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -41,6 +41,10 @@ static unsigned short psc_ac97_read(struct snd_ac97 *a=
c97, unsigned short reg)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 bus (rdy)\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
> =A0 =A0 =A0 =A0}
> +
> + =A0 =A0 =A0 /* Force clear the data valid bit */
> + =A0 =A0 =A0 in_be32(&psc_dma->psc_regs->ac97_data);
> +
> =A0 =A0 =A0 =A0/* Send the read */
> =A0 =A0 =A0 =A0out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0=
x7f) << 24));
>
>
>



--=20
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver

[Jon Smirl]

On Thu, Jul 2, 2009 at 1:57 PM, Grant Likely<grant.likely@secretlab.ca> wro=
te:
> 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. =A0This patch adds a mutex around
> the register access routines.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Jon Smirl <jonsmirl@gmail.com>

> ---
>
> =A0sound/soc/fsl/mpc5200_dma.c =A0 =A0 =A0| =A0 =A01 +
> =A0sound/soc/fsl/mpc5200_dma.h =A0 =A0 =A0| =A0 =A01 +
> =A0sound/soc/fsl/mpc5200_psc_ac97.c | =A0 13 ++++++++++++-
> =A03 files changed, 14 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)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
>
> =A0 =A0 =A0 =A0spin_lock_init(&psc_dma->lock);
> + =A0 =A0 =A0 mutex_init(&psc_dma->mutex);
> =A0 =A0 =A0 =A0psc_dma->id =3D be32_to_cpu(*prop);
> =A0 =A0 =A0 =A0psc_dma->irq =3D irq;
> =A0 =A0 =A0 =A0psc_dma->psc_regs =3D 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 {
> =A0 =A0 =A0 =A0unsigned int irq;
> =A0 =A0 =A0 =A0struct device *dev;
> =A0 =A0 =A0 =A0spinlock_t lock;
> + =A0 =A0 =A0 struct mutex mutex;
> =A0 =A0 =A0 =A0u32 sicr;
> =A0 =A0 =A0 =A0uint sysclk;
> =A0 =A0 =A0 =A0int imr;
> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc=
_ac97.c
> index 9b8503f..7eb5499 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -34,11 +34,14 @@ static unsigned short psc_ac97_read(struct snd_ac97 *=
ac97, unsigned short reg)
> =A0 =A0 =A0 =A0int status;
> =A0 =A0 =A0 =A0unsigned int val;
>
> + =A0 =A0 =A0 mutex_lock(&psc_dma->mutex);
> +
> =A0 =A0 =A0 =A0/* Wait for command send status zero =3D ready */
> =A0 =A0 =A0 =A0status =3D spin_event_timeout(!(in_be16(&psc_dma->psc_regs=
->sr_csr.status) &
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MPC52xx_PS=
C_SR_CMDSEND), 100, 0);
> =A0 =A0 =A0 =A0if (status =3D=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 bus (rdy)\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
> =A0 =A0 =A0 =A0}
>
> @@ -54,16 +57,19 @@ static unsigned short psc_ac97_read(struct snd_ac97 *=
ac97, unsigned short reg)
> =A0 =A0 =A0 =A0if (status =3D=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 read (val) %x\n",
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0in_be16(&p=
sc_dma->psc_regs->sr_csr.status));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0/* Get the data */
> =A0 =A0 =A0 =A0val =3D in_be32(&psc_dma->psc_regs->ac97_data);
> =A0 =A0 =A0 =A0if (((val >> 24) & 0x7f) !=3D reg) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("reg echo error on ac97 read\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0val =3D (val >> 8) & 0xffff;
>
> + =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0 =A0 =A0 =A0return (unsigned short) val;
> =A0}
>
> @@ -72,16 +78,21 @@ static void psc_ac97_write(struct snd_ac97 *ac97,
> =A0{
> =A0 =A0 =A0 =A0int status;
>
> + =A0 =A0 =A0 mutex_lock(&psc_dma->mutex);
> +
> =A0 =A0 =A0 =A0/* Wait for command status zero =3D ready */
> =A0 =A0 =A0 =A0status =3D spin_event_timeout(!(in_be16(&psc_dma->psc_regs=
->sr_csr.status) &
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MPC52xx_PS=
C_SR_CMDSEND), 100, 0);
> =A0 =A0 =A0 =A0if (status =3D=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 bus (write)\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0/* Write data */
> =A0 =A0 =A0 =A0out_be32(&psc_dma->psc_regs->ac97_cmd,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0((reg & 0x7f) << 24) | (va=
l << 8));
> +
> + out:
> + =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0}
>
> =A0static void psc_ac97_warm_reset(struct snd_ac97 *ac97)
>
>



--=20
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver

[michael]

Hi,

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 |   13 ++++++++++++-
>  3 files changed, 14 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 9b8503f..7eb5499 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -34,11 +34,14 @@ 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;
>   
maybe define an err variable and and a goto out.

Michael

Re: [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver

[Grant Likely]

On Thu, Jul 2, 2009 at 1:08 PM, michael<michael@evidence.eu.com> wrote:
> Hi,
>
> 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. =A0This patch adds a mutex around
>> the register access routines.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>>
>> =A0sound/soc/fsl/mpc5200_dma.c =A0 =A0 =A0| =A0 =A01 +
>> =A0sound/soc/fsl/mpc5200_dma.h =A0 =A0 =A0| =A0 =A01 +
>> =A0sound/soc/fsl/mpc5200_psc_ac97.c | =A0 13 ++++++++++++-
>> =A03 files changed, 14 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)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
>> =A0 =A0 =A0 =A0 =A0spin_lock_init(&psc_dma->lock);
>> + =A0 =A0 =A0 mutex_init(&psc_dma->mutex);
>> =A0 =A0 =A0 =A0psc_dma->id =3D be32_to_cpu(*prop);
>> =A0 =A0 =A0 =A0psc_dma->irq =3D irq;
>> =A0 =A0 =A0 =A0psc_dma->psc_regs =3D 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 {
>> =A0 =A0 =A0 =A0unsigned int irq;
>> =A0 =A0 =A0 =A0struct device *dev;
>> =A0 =A0 =A0 =A0spinlock_t lock;
>> + =A0 =A0 =A0 struct mutex mutex;
>> =A0 =A0 =A0 =A0u32 sicr;
>> =A0 =A0 =A0 =A0uint sysclk;
>> =A0 =A0 =A0 =A0int imr;
>> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c
>> b/sound/soc/fsl/mpc5200_psc_ac97.c
>> index 9b8503f..7eb5499 100644
>> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
>> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
>> @@ -34,11 +34,14 @@ static unsigned short psc_ac97_read(struct snd_ac97
>> *ac97, unsigned short reg)
>> =A0 =A0 =A0 =A0int status;
>> =A0 =A0 =A0 =A0unsigned int val;
>> =A0+ =A0 =A0 =A0 mutex_lock(&psc_dma->mutex);
>> +
>> =A0 =A0 =A0 =A0/* Wait for command send status zero =3D ready */
>> =A0 =A0 =A0 =A0status =3D
>> spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MPC52xx_P=
SC_SR_CMDSEND), 100, 0);
>> =A0 =A0 =A0 =A0if (status =3D=3D 0) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 bus (rdy)\n");
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
>>
>
> maybe define an err variable and and a goto out.

My first iteration did that, but after looking at it I decided I like
this better and it adds (slightly) fewer lines of code.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared

[Mark Brown]

On Thu, Jul 02, 2009 at 11:57:19AM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> When doing register reads, it is possible for there to be a stale
> data ready bit set which will cause subsequent reads to return
> prematurely with incorrect data.  This patch fixes the issues by
> ensuring stale data is cleared before starting another transaction.

Applied both, thanks.