Peter Ujfalusi wrote:
Hello,
Hi Peter,
I think there are some inconsistency in a way how for example the SPCR1 and
SPCR2 registers are used.
On Thursday 14 January 2010 18:13:36 ext Janusz Krzysztofik wrote:
Change the way McBSP registers are updated: use cached values instead of
relying upon those read back from the device.
With this patch, I have finally managed to get rid of all random
playback/recording hangups on my OMAP1510 based Amstrad Delta hardware.
Before that, values read back from McBSP registers to be used for updating
them happened to be errornous.
From the hardware side, the issue appeared to be caused by a relatively
high power requirements of an external USB adapter connected to the
board's printer dedicated USB port.
I think there is one important point that makes this patch worth of
applying, apart from my hardware quality. With the current code, if it
ever happens to any machine, no matter if OMAP1510 or newer, to read
incorrect value from a McBSP register, this wrong value will get written
back without any checking. That can lead to hardware damage if, for
example, an input pin is turned into output as a result.
Applies on top of patch 3 from this series:
[PATCH v9 3/4] OMAP: McBSP: Introduce caching in register write operations
Tested on OMAP1510 based Amstrad Delta using linux-omap for-next, commit
fb7380d70e041e4b3892f6b19dff7efb609d15a4 (2.6.33-rc3+ dated 2010-01-11).
Compile-tested with omap_3430sdp_defconfig.
Signed-off-by: Janusz Krzysztofik <jkrzyszt@xxxxxxxxxxxx>
---
No functional changes since v3.
arch/arm/plat-omap/mcbsp.c | 78
+++++++++++++++++++++++++++------------------ 1 file changed, 47
insertions(+), 31 deletions(-)
--- git/arch/arm/plat-omap/mcbsp.c.orig 2010-01-14 00:36:14.000000000 +0100
+++ git/arch/arm/plat-omap/mcbsp.c 2010-01-14 02:05:23.000000000 +0100
@@ -114,7 +114,8 @@ 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, irqst_spcr2 & ~(XSYNC_ERR));
+ MCBSP_WRITE(mcbsp_tx, SPCR2,
+ MCBSP_READ_CACHE(mcbsp_tx, SPCR2) & ~(XSYNC_ERR));
The reg_cache will never have the XSYNC_ERR, or any other flags set, since these
flags has never written to the reg_cache.
So it is kind of not necessary to clear the flag, which is actually always 0.
Agree.
Another thing is that as far as I understand the reason behind of this series is
that you have a problem, that you can not trust on the values that you read from
the McBSP registers, if this is true, than the problem can occur when the above
path has been taken:
...
irqst_spcr2 = MCBSP_READ(mcbsp_tx, SPCR2);
...
if (irqst_spcr2 & XSYNC_ERR) {
But since you read from McBSP registers much rarely, than probably the
corruption is not that visible?
Sure no software solution can correct my hardware issue in case of
register bits manintained by the hardware itself. Well, maybe software
that limits heat dissipation by lowering overal power consumption could
do to some extent ;).
What I'm going to address here is only a case when writing back possibly
corrupted bits can be avoided if correct values are well known and
can be determined without reading them back from the register itself.
Anyway, clearing the status/error flags are not necessary, since the reg_cache
will never got these bits set, you could just write back the SPCR2/SPCR1
register content from the cache as it is...
} else {
complete(&mcbsp_tx->tx_irq_completion);
}
@@ -134,7 +135,8 @@ 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, irqst_spcr1 & ~(RSYNC_ERR));
+ MCBSP_WRITE(mcbsp_rx, SPCR1,
+ MCBSP_READ_CACHE(mcbsp_rx, SPCR1) & ~(RSYNC_ERR));
Same here.
...
@@ -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.
/* 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...
This applies fro all other cases regarding to status/error bits in McBSP.
OK, I can try to identify all those cases.
What about introducing this simplification in a separate followup patch,
quoting your rationale in its changelog? I can try to prepare one if you
agree.
Thanks,
Janusz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
