[PATCH] OMAP: McBSP: Drop unnecessary status/error bit clearing on reg_cache retrieved register values

[PATCH] OMAP: McBSP: Drop unnecessary status/error bit clearing on reg_cache retrieved register values

[Janusz Krzysztofik]

The MsBSP register cache will never have any error/status flags set, since 
these flags are never written to the reg_cache. So it is kind of not 
necessary to clear these flags, which are actually always 0.

In other words, clearing the status/error flags are not necessary, since the 
reg_cache will never got these bits set. We can just write back the 
register content from the cache as it is when clearing an error condition.

Created against l-o for-next commit 62a7c2cc4c8e57e80ccf379536f362fe6e863ac3
dated 2010-02-22.
Tested on Amstrad Delta.

Reported-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>

---
Friday 15 January 2010 10:11:28 Peter Ujfalusi wrote:
> ...
>
> > @@ -653,7 +657,7 @@ int omap_mcbsp_pollwrite(unsigned int id
> >       if (MCBSP_READ(mcbsp, SPCR2) & XSYNC_ERR) {
> >               /* clear error */
> >               MCBSP_WRITE(mcbsp, SPCR2,
> > -                             MCBSP_READ(mcbsp, SPCR2) & (~XSYNC_ERR));
> > +                             MCBSP_READ_CACHE(mcbsp, SPCR2) & (~XSYNC_ERR));
>
> Well, I think here also, the reg_cache does not have the XSYNC_ERR set, it
> is only set in the McBSP register.

Peter,
Yes, that's right.

> >               /* resend */
> >               return -1;
> >       } else {
> > @@ -662,10 +666,12 @@ int omap_mcbsp_pollwrite(unsigned int id
> >               while (!(MCBSP_READ(mcbsp, SPCR2) & XRDY)) {
> >                       if (attemps++ > 1000) {
> >                               MCBSP_WRITE(mcbsp, SPCR2,
> > -                                         MCBSP_READ(mcbsp, SPCR2) & (~XRST));
> > +                                             MCBSP_READ_CACHE(mcbsp, SPCR2) &
> > +                                             (~XRST)); 
>
> Also here, the XRST will surely not set in the cached SPCR2...

Probably not. XRST and other xRST flags seem to be controlled by software 
only, and can be actually set by omap_mcbsp_request() to get required 
functional blocks out of reset state. Here it is toggled back and forth, so 
the code has to reset it explicitly and then set it back again.

> This applies fro all other cases regarding to status/error bits in McBSP.

You probably managed to point out all applicable cases, since I was not able 
to find more, neither while examining the code itself nor reviewing the 
SPRU592E reference one more time. 

> >                               udelay(10);
> >                               MCBSP_WRITE(mcbsp, SPCR2,
> > -                                         MCBSP_READ(mcbsp, SPCR2) | (XRST));
> > +                                             MCBSP_READ_CACHE(mcbsp, SPCR2) |
> > +                                             (XRST)); 
> >                               udelay(10);
> >                               dev_err(mcbsp->dev, "Could not write to"
> >                                       " McBSP%d Register\n", mcbsp->id);

Thanks,
Janusz

 arch/arm/plat-omap/mcbsp.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

--- git/arch/arm/plat-omap/mcbsp.c.orig	2010-02-23 11:36:43.000000000 +0100
+++ git/arch/arm/plat-omap/mcbsp.c	2010-02-23 16:39:45.000000000 +0100
@@ -133,8 +133,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han
 		dev_err(mcbsp_tx->dev, "TX Frame Sync Error! : 0x%x\n",
 			irqst_spcr2);
 		/* Writing zero to XSYNC_ERR clears the IRQ */
-		MCBSP_WRITE(mcbsp_tx, SPCR2,
-			    MCBSP_READ_CACHE(mcbsp_tx, SPCR2) & ~(XSYNC_ERR));
+		MCBSP_WRITE(mcbsp_tx, SPCR2, MCBSP_READ_CACHE(mcbsp_tx, SPCR2));
 	} else {
 		complete(&mcbsp_tx->tx_irq_completion);
 	}
@@ -154,8 +153,7 @@ static irqreturn_t omap_mcbsp_rx_irq_han
 		dev_err(mcbsp_rx->dev, "RX Frame Sync Error! : 0x%x\n",
 			irqst_spcr1);
 		/* Writing zero to RSYNC_ERR clears the IRQ */
-		MCBSP_WRITE(mcbsp_rx, SPCR1,
-			    MCBSP_READ_CACHE(mcbsp_rx, SPCR1) & ~(RSYNC_ERR));
+		MCBSP_WRITE(mcbsp_rx, SPCR1, MCBSP_READ_CACHE(mcbsp_rx, SPCR1));
 	} else {
 		complete(&mcbsp_rx->tx_irq_completion);
 	}
@@ -934,8 +932,7 @@ int omap_mcbsp_pollwrite(unsigned int id
 	/* if frame sync error - clear the error */
 	if (MCBSP_READ(mcbsp, SPCR2) & XSYNC_ERR) {
 		/* clear error */
-		MCBSP_WRITE(mcbsp, SPCR2,
-				MCBSP_READ_CACHE(mcbsp, SPCR2) & (~XSYNC_ERR));
+		MCBSP_WRITE(mcbsp, SPCR2, MCBSP_READ_CACHE(mcbsp, SPCR2));
 		/* resend */
 		return -1;
 	} else {
@@ -975,8 +972,7 @@ int omap_mcbsp_pollread(unsigned int id,
 	/* if frame sync error - clear the error */
 	if (MCBSP_READ(mcbsp, SPCR1) & RSYNC_ERR) {
 		/* clear error */
-		MCBSP_WRITE(mcbsp, SPCR1,
-				MCBSP_READ_CACHE(mcbsp, SPCR1) & (~RSYNC_ERR));
+		MCBSP_WRITE(mcbsp, SPCR1, MCBSP_READ_CACHE(mcbsp, SPCR1));
 		/* resend */
 		return -1;
 	} else {
--
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] OMAP: McBSP: Drop unnecessary status/error bit clearing on reg_cache retrieved register values

[Jarkko Nikula]

On Tue, 23 Feb 2010 16:50:38 +0100
Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:

> The MsBSP register cache will never have any error/status flags set, since 
> these flags are never written to the reg_cache. So it is kind of not 
> necessary to clear these flags, which are actually always 0.
> 
> In other words, clearing the status/error flags are not necessary, since the 
> reg_cache will never got these bits set. We can just write back the 
> register content from the cache as it is when clearing an error condition.
> 
> Created against l-o for-next commit 62a7c2cc4c8e57e80ccf379536f362fe6e863ac3
> dated 2010-02-22.
> Tested on Amstrad Delta.
> 
> Reported-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> 
Acked-by: Jarkko Nikula <jhnikula@gmail.com>

Re: [PATCH] OMAP: McBSP: Drop unnecessary status/error bit clearing on reg_cache retrieved register values

[Janusz Krzysztofik]

Thursday 04 March 2010 07:49:57 Jarkko Nikula napisał(a):
> On Tue, 23 Feb 2010 16:50:38 +0100
>
> Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:
> > The MsBSP register cache will never have any error/status flags set,
> > since these flags are never written to the reg_cache. So it is kind of
> > not necessary to clear these flags, which are actually always 0.
> >
> > In other words, clearing the status/error flags are not necessary, since
> > the reg_cache will never got these bits set. We can just write back the
> > register content from the cache as it is when clearing an error
> > condition.
> >
> > Created against l-o for-next commit
> > 62a7c2cc4c8e57e80ccf379536f362fe6e863ac3 dated 2010-02-22.
> > Tested on Amstrad Delta.
> >
> > Reported-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
> > Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
>
> Acked-by: Jarkko Nikula <jhnikula@gmail.com>

Jarkko, thanks.

Tony,
It looks like my initial submission extra comments (quoted discussion with 
Peter) was so confusing, that you had erratically marked this path 
with "changes requested" in your patchowrk, so it get archived. Here is the 
URL, just in case you have troubles finding it now for applying :)
http://patchwork.kernel.org/patch/81476/

Thanks,
Janusz
--
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

[APPLIED] [PATCH] OMAP: McBSP: Drop unnecessary status/error bit clearing on

[Tony Lindgren]

This patch has been applied to the linux-omap
by youw fwiendly patch wobot.

Branch in linux-omap: omap-fixes

Initial commit ID (Likely to change): 28c60b4bee6a948791d68d305701ec2d63ed22fc

PatchWorks
http://patchwork.kernel.org/patch/81476/

Git (Likely to change, and takes a while to get mirrored)
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=28c60b4bee6a948791d68d305701ec2d63ed22fc