[PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3

[PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3

[charu]

omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access
McBSP registers as 16-bit registers.

In OMAP3, McBSP registers (DRR_REG and DXR_REG) are limited to
32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is
not allowed and can corrupt register content.

This patch adds omap3_mcbsp_pollwrite and
omap3_mcbsp_pollread functions for OMAP3 cpu, inorder to
perform 32 bit access of above mentioned McBSP registers.

Signed-off-by: Charulatha V <charu@ti.com>
Signed-off-by: Syed Rafiuddin <rafiuddin.syed@ti.com>
---
 arch/arm/plat-omap/include/mach/mcbsp.h |    4 +
 arch/arm/plat-omap/mcbsp.c              |   92 ++++++++++++++++++++++++++++++-
 2 files changed, 95 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h b/arch/arm/plat-omap/include/mach/mcbsp.h
index 0c5f937..b0f9260 100644
--- a/arch/arm/plat-omap/include/mach/mcbsp.h
+++ b/arch/arm/plat-omap/include/mach/mcbsp.h
@@ -406,6 +406,10 @@ void omap_mcbsp_set_spi_mode(unsigned int id, const struct omap_mcbsp_spi_cfg *
 /* Polled read/write functions */
 int omap_mcbsp_pollread(unsigned int id, u16 * buf);
 int omap_mcbsp_pollwrite(unsigned int id, u16 buf);
+
+int omap3_mcbsp_pollread(unsigned int id, u32 *buf);
+int omap3_mcbsp_pollwrite(unsigned int id, u32 buf);
+
 int omap_mcbsp_set_io_type(unsigned int id, omap_mcbsp_io_type_t io_type);
 
 #endif
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index b6b520d..1212d11 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -425,7 +425,7 @@ void omap_mcbsp_stop(unsigned int id)
 }
 EXPORT_SYMBOL(omap_mcbsp_stop);
 
-/* polled mcbsp i/o operations */
+/* polled mcbsp i/o operations for omap1 and omap2420 */
 int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
 {
 	struct omap_mcbsp *mcbsp;
@@ -515,6 +515,96 @@ int omap_mcbsp_pollread(unsigned int id, u16 *buf)
 }
 EXPORT_SYMBOL(omap_mcbsp_pollread);
 
+/* polled mcbsp i/o operations for omap3 */
+int omap3_mcbsp_pollwrite(unsigned int id, u32 buf)
+{
+	struct omap_mcbsp *mcbsp;
+	void __iomem *base;
+
+	if (!omap_mcbsp_check_valid_id(id)) {
+		printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
+		return -ENODEV;
+	}
+
+	mcbsp = id_to_mcbsp_ptr(id);
+	base = mcbsp->io_base;
+
+	OMAP_MCBSP_WRITE(base, DXR, buf);
+	/* if frame sync error - clear the error */
+	if (OMAP_MCBSP_READ(base, SPCR2) & XSYNC_ERR) {
+		/* clear error */
+		OMAP_MCBSP_WRITE(base, SPCR2, OMAP_MCBSP_READ(base, SPCR2)
+					& (~XSYNC_ERR));
+		/* resend */
+		return -EPERM;
+	} else {
+		/* wait for transmit confirmation */
+		int attemps = 0;
+		while (!(OMAP_MCBSP_READ(base, SPCR2) & XRDY)) {
+			if (attemps++ > 1000) {
+				OMAP_MCBSP_WRITE(base, SPCR2,
+					OMAP_MCBSP_READ(base, SPCR2) &
+					(~XRST));
+				udelay(10);
+				OMAP_MCBSP_WRITE(base, SPCR2,
+					OMAP_MCBSP_READ(base, SPCR2) |
+					(XRST));
+				udelay(10);
+				dev_err(mcbsp->dev, "Could not write to"
+					" McBSP%d Register\n", mcbsp->id);
+				return -ENOENT;
+			}
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(omap3_mcbsp_pollwrite);
+
+int omap3_mcbsp_pollread(unsigned int id, u32 *buf)
+{
+	struct omap_mcbsp *mcbsp;
+	void __iomem *base;
+
+	if (!omap_mcbsp_check_valid_id(id)) {
+		printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
+		return -ENODEV;
+	}
+	mcbsp = id_to_mcbsp_ptr(id);
+
+	base = mcbsp->io_base;
+	/* if frame sync error - clear the error */
+	if (OMAP_MCBSP_READ(base, SPCR1) & RSYNC_ERR) {
+		/* clear error */
+		OMAP_MCBSP_WRITE(base, SPCR1, OMAP_MCBSP_READ(base, SPCR1)
+					& (~RSYNC_ERR));
+		/* resend */
+		return -EPERM;
+	} else {
+		/* wait for recieve confirmation */
+		int attemps = 0;
+		while (!(OMAP_MCBSP_READ(base, SPCR1) & RRDY)) {
+			if (attemps++ > 1000) {
+				OMAP_MCBSP_WRITE(base, SPCR1,
+					OMAP_MCBSP_READ(base, SPCR1) &
+					(~RRST));
+				udelay(10);
+				OMAP_MCBSP_WRITE(base, SPCR1,
+					OMAP_MCBSP_READ(base, SPCR1) |
+					(RRST));
+				udelay(10);
+				dev_err(mcbsp->dev, "Could not read from"
+					" McBSP%d Register\n", mcbsp->id);
+				return -ENOENT;
+			}
+		}
+	}
+	*buf = OMAP_MCBSP_READ(base, DRR);
+
+	return 0;
+}
+EXPORT_SYMBOL(omap3_mcbsp_pollread);
+
 /*
  * IRQ based word transmission.
  */
-- 
1.6.0.4

Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3

[Tony Lindgren]

* charu@ti.com <charu@ti.com> [091019 23:41]:
> omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access
> McBSP registers as 16-bit registers.
> 
> In OMAP3, McBSP registers (DRR_REG and DXR_REG) are limited to
> 32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is
> not allowed and can corrupt register content.
> 
> This patch adds omap3_mcbsp_pollwrite and
> omap3_mcbsp_pollread functions for OMAP3 cpu, inorder to
> perform 32 bit access of above mentioned McBSP registers.

How about making the functions generic, I don't think we want
to have separate omap1_mcbsp_pollread, omap2_mcbsp_pollread,
omap3_mcbsp_pollread..

If it's not obvious how to implement it for some omap, just
return an error based on the CPU type.

Regards,

Tony
 
> Signed-off-by: Charulatha V <charu@ti.com>
> Signed-off-by: Syed Rafiuddin <rafiuddin.syed@ti.com>
> ---
>  arch/arm/plat-omap/include/mach/mcbsp.h |    4 +
>  arch/arm/plat-omap/mcbsp.c              |   92 ++++++++++++++++++++++++++++++-
>  2 files changed, 95 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h b/arch/arm/plat-omap/include/mach/mcbsp.h
> index 0c5f937..b0f9260 100644
> --- a/arch/arm/plat-omap/include/mach/mcbsp.h
> +++ b/arch/arm/plat-omap/include/mach/mcbsp.h
> @@ -406,6 +406,10 @@ void omap_mcbsp_set_spi_mode(unsigned int id, const struct omap_mcbsp_spi_cfg *
>  /* Polled read/write functions */
>  int omap_mcbsp_pollread(unsigned int id, u16 * buf);
>  int omap_mcbsp_pollwrite(unsigned int id, u16 buf);
> +
> +int omap3_mcbsp_pollread(unsigned int id, u32 *buf);
> +int omap3_mcbsp_pollwrite(unsigned int id, u32 buf);
> +
>  int omap_mcbsp_set_io_type(unsigned int id, omap_mcbsp_io_type_t io_type);
>  
>  #endif
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index b6b520d..1212d11 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -425,7 +425,7 @@ void omap_mcbsp_stop(unsigned int id)
>  }
>  EXPORT_SYMBOL(omap_mcbsp_stop);
>  
> -/* polled mcbsp i/o operations */
> +/* polled mcbsp i/o operations for omap1 and omap2420 */
>  int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
>  {
>  	struct omap_mcbsp *mcbsp;
> @@ -515,6 +515,96 @@ int omap_mcbsp_pollread(unsigned int id, u16 *buf)
>  }
>  EXPORT_SYMBOL(omap_mcbsp_pollread);
>  
> +/* polled mcbsp i/o operations for omap3 */
> +int omap3_mcbsp_pollwrite(unsigned int id, u32 buf)
> +{
> +	struct omap_mcbsp *mcbsp;
> +	void __iomem *base;
> +
> +	if (!omap_mcbsp_check_valid_id(id)) {
> +		printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> +		return -ENODEV;
> +	}
> +
> +	mcbsp = id_to_mcbsp_ptr(id);
> +	base = mcbsp->io_base;
> +
> +	OMAP_MCBSP_WRITE(base, DXR, buf);
> +	/* if frame sync error - clear the error */
> +	if (OMAP_MCBSP_READ(base, SPCR2) & XSYNC_ERR) {
> +		/* clear error */
> +		OMAP_MCBSP_WRITE(base, SPCR2, OMAP_MCBSP_READ(base, SPCR2)
> +					& (~XSYNC_ERR));
> +		/* resend */
> +		return -EPERM;
> +	} else {
> +		/* wait for transmit confirmation */
> +		int attemps = 0;
> +		while (!(OMAP_MCBSP_READ(base, SPCR2) & XRDY)) {
> +			if (attemps++ > 1000) {
> +				OMAP_MCBSP_WRITE(base, SPCR2,
> +					OMAP_MCBSP_READ(base, SPCR2) &
> +					(~XRST));
> +				udelay(10);
> +				OMAP_MCBSP_WRITE(base, SPCR2,
> +					OMAP_MCBSP_READ(base, SPCR2) |
> +					(XRST));
> +				udelay(10);
> +				dev_err(mcbsp->dev, "Could not write to"
> +					" McBSP%d Register\n", mcbsp->id);
> +				return -ENOENT;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(omap3_mcbsp_pollwrite);
> +
> +int omap3_mcbsp_pollread(unsigned int id, u32 *buf)
> +{
> +	struct omap_mcbsp *mcbsp;
> +	void __iomem *base;
> +
> +	if (!omap_mcbsp_check_valid_id(id)) {
> +		printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> +		return -ENODEV;
> +	}
> +	mcbsp = id_to_mcbsp_ptr(id);
> +
> +	base = mcbsp->io_base;
> +	/* if frame sync error - clear the error */
> +	if (OMAP_MCBSP_READ(base, SPCR1) & RSYNC_ERR) {
> +		/* clear error */
> +		OMAP_MCBSP_WRITE(base, SPCR1, OMAP_MCBSP_READ(base, SPCR1)
> +					& (~RSYNC_ERR));
> +		/* resend */
> +		return -EPERM;
> +	} else {
> +		/* wait for recieve confirmation */
> +		int attemps = 0;
> +		while (!(OMAP_MCBSP_READ(base, SPCR1) & RRDY)) {
> +			if (attemps++ > 1000) {
> +				OMAP_MCBSP_WRITE(base, SPCR1,
> +					OMAP_MCBSP_READ(base, SPCR1) &
> +					(~RRST));
> +				udelay(10);
> +				OMAP_MCBSP_WRITE(base, SPCR1,
> +					OMAP_MCBSP_READ(base, SPCR1) |
> +					(RRST));
> +				udelay(10);
> +				dev_err(mcbsp->dev, "Could not read from"
> +					" McBSP%d Register\n", mcbsp->id);
> +				return -ENOENT;
> +			}
> +		}
> +	}
> +	*buf = OMAP_MCBSP_READ(base, DRR);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(omap3_mcbsp_pollread);
> +
>  /*
>   * IRQ based word transmission.
>   */
> -- 
> 1.6.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3

[Varadarajan, Charu Latha]

 

>-----Original Message-----
>From: Tony Lindgren [mailto:tony@atomide.com] 
>Sent: Friday, November 13, 2009 2:41 AM
>To: Varadarajan, Charu Latha
>Cc: linux-omap@vger.kernel.org; Syed, Rafiuddin
>Subject: Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3
>
>* charu@ti.com <charu@ti.com> [091019 23:41]:
>> omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access
>> McBSP registers as 16-bit registers.
>> 
>> In OMAP3, McBSP registers (DRR_REG and DXR_REG) are limited to
>> 32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is
>> not allowed and can corrupt register content.
>> 
>> This patch adds omap3_mcbsp_pollwrite and
>> omap3_mcbsp_pollread functions for OMAP3 cpu, inorder to
>> perform 32 bit access of above mentioned McBSP registers.
>
>How about making the functions generic, I don't think we want
>to have separate omap1_mcbsp_pollread, omap2_mcbsp_pollread,
>omap3_mcbsp_pollread..
>
>If it's not obvious how to implement it for some omap, just
>return an error based on the CPU type.


My v1 patch handles this in a single generic API. 
[http://www.spinics.net/lists/linux-omap/msg19074.html and
http://patchwork.kernel.org/patch/53631/]

As per the review comments received for v1 patch, v2 patch is 
modified to have new API "omap3_mcbsp_pollread/write" in 
addition to "omap_mcbsp_pollread/write" APIs. 

Review comments for V1 mentioned that API signartures should not be 
changed from 16bit to 32 bit input parameters. 
If the API needs to be made generic, API signature has to be 
modified as given below (as in patch v1):
-int omap_mcbsp_pollread(unsigned int id, u16 *buf)
+int omap_mcbsp_pollread(unsigned int id, u32 *buf)

If agreed for this generic API change, I shall post v3 patch
which would handle cputypes dynamically inside the API


>
>Regards,
>
>Tony
> 
>> Signed-off-by: Charulatha V <charu@ti.com>
>> Signed-off-by: Syed Rafiuddin <rafiuddin.syed@ti.com>
>> ---
>>  arch/arm/plat-omap/include/mach/mcbsp.h |    4 +
>>  arch/arm/plat-omap/mcbsp.c              |   92 
>++++++++++++++++++++++++++++++-
>>  2 files changed, 95 insertions(+), 1 deletions(-)
>> 
>> diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h 
>b/arch/arm/plat-omap/include/mach/mcbsp.h
>> index 0c5f937..b0f9260 100644
>> --- a/arch/arm/plat-omap/include/mach/mcbsp.h
>> +++ b/arch/arm/plat-omap/include/mach/mcbsp.h
>> @@ -406,6 +406,10 @@ void omap_mcbsp_set_spi_mode(unsigned 
>int id, const struct omap_mcbsp_spi_cfg *
>>  /* Polled read/write functions */
>>  int omap_mcbsp_pollread(unsigned int id, u16 * buf);
>>  int omap_mcbsp_pollwrite(unsigned int id, u16 buf);
>> +
>> +int omap3_mcbsp_pollread(unsigned int id, u32 *buf);
>> +int omap3_mcbsp_pollwrite(unsigned int id, u32 buf);
>> +
>>  int omap_mcbsp_set_io_type(unsigned int id, 
>omap_mcbsp_io_type_t io_type);
>>  
>>  #endif
<snip>

Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3

[Tony Lindgren]

* Varadarajan, Charu Latha <charu@ti.com> [091118 00:58]:
>  
> 
> >-----Original Message-----
> >From: Tony Lindgren [mailto:tony@atomide.com] 
> >Sent: Friday, November 13, 2009 2:41 AM
> >To: Varadarajan, Charu Latha
> >Cc: linux-omap@vger.kernel.org; Syed, Rafiuddin
> >Subject: Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3
> >
> >* charu@ti.com <charu@ti.com> [091019 23:41]:
> >> omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access
> >> McBSP registers as 16-bit registers.
> >> 
> >> In OMAP3, McBSP registers (DRR_REG and DXR_REG) are limited to
> >> 32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is
> >> not allowed and can corrupt register content.
> >> 
> >> This patch adds omap3_mcbsp_pollwrite and
> >> omap3_mcbsp_pollread functions for OMAP3 cpu, inorder to
> >> perform 32 bit access of above mentioned McBSP registers.
> >
> >How about making the functions generic, I don't think we want
> >to have separate omap1_mcbsp_pollread, omap2_mcbsp_pollread,
> >omap3_mcbsp_pollread..
> >
> >If it's not obvious how to implement it for some omap, just
> >return an error based on the CPU type.
> 
> 
> My v1 patch handles this in a single generic API. 
> [http://www.spinics.net/lists/linux-omap/msg19074.html and
> http://patchwork.kernel.org/patch/53631/]

Well first of all we don't have any references to this function
in our tree. Second, we have:

#define OMAP_MCBSP_REG_DRR2	0x00
#define OMAP_MCBSP_REG_DRR1	0x02
#define OMAP_MCBSP_REG_DXR2	0x04
#define OMAP_MCBSP_REG_DXR1	0x06

Meaning that both DRR and DXR are 32-bits in length. And based
on that it seems that at least omap_mcbsp_pollwrite is broken
if it supports only one 16-bit write instead of 16 + 16 bits
that the hardware seems to support.

> As per the review comments received for v1 patch, v2 patch is 
> modified to have new API "omap3_mcbsp_pollread/write" in 
> addition to "omap_mcbsp_pollread/write" APIs. 

That does not make much sense based on the fact that the
hardware has 32-bits for DRR and DXR. 
 
> Review comments for V1 mentioned that API signartures should not be 
> changed from 16bit to 32 bit input parameters. 
> If the API needs to be made generic, API signature has to be 
> modified as given below (as in patch v1):
> -int omap_mcbsp_pollread(unsigned int id, u16 *buf)
> +int omap_mcbsp_pollread(unsigned int id, u32 *buf)
> 
> If agreed for this generic API change, I shall post v3 patch
> which would handle cputypes dynamically inside the API

To me it smells like the all drivers using the the
omap_mcbsp_pollread/write are broken legacy drivers.
So maybe we should just remove these two functions?

If we really want to keep these around, we should review
the drivers using these functions. And if we end up keeping
these functions around, I like the first patch better, except
it does not seem to support DRR2 and DXR2 on older hardware,
so that's broken too.

IMHO, we should rather set up this driver as a proper
bus driver as suggested by Paul in a related thread.

Regards,

Tony