Subject: Re: [E3-hacking] [RFC][PATCH] ARM: OMAP: McBSP: Use register cache

From: Janusz Krzysztofik <[email-masked]>
Date: Fri, 4 Sep 2009 12:34:05 +0200

Friday 04 September 2009 07:39:36 Eero Nurkkala wrote:
>
> Register values should not get corrupted. Could you please point out the
> registers that may get corrupted? Do they get corrupted during audio
> play? I haven't seen corrupted register values.
>

Hi Eero,

Please have a look at some debug log lines below. Those were generated on my
Amstrad Delta videophone, using an application (asterisk softpbx with alsa
channel) that can restart a receiver of a prevoiusly requested McBSP by
calling omap_mcbsp_start() on user request. As you can see, corrupted values
can happen to get read from any register that is expected to return
predictable results.

Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: **** McBSP1 regs ****
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR2:  0x1000
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR1:  0x009d
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR2:  0x3000
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR1:  0x7da1
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR2: 0x02f0
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR1: 0x7031
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR2:  0x0045
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR1:  0x0040
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR2:  0x1045
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR1:  0x0040
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR2: 0x310f
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR1: 0x0000
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: PCR0:  0x0003
Sep  4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: ***********************
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: **** McBSP1 regs ****
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR2:  0x0000
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR1:  0x1086
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR2:  0x0000
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR1:  0x7da1
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR2: 0x02f0
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR1: 0x0031
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR2:  0x1045
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR1:  0x1040
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR2:  0x3045
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR1:  0x1040
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR2: 0x000f
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR1: 0x0000
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: PCR0:  0x1003
Sep  4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: ***********************
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: **** McBSP1 regs ****
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR2:  0x1000
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR1:  0x109e
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR2:  0x1000
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR1:  0x7da1
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR2: 0x02f0
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR1: 0x1031
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR2:  0x1045
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR1:  0x1040
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR2:  0x2045
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR1:  0x1040
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR2: 0x100f
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR1: 0x1000
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: PCR0:  0x2003
Sep  4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: ***********************

Those corruptions, even if should never happen, were actually caused by a
wireless adapter connected to the machine USB port. Extra piece of hardware,
very usefull in some cases, but obviously not taken into account while the
machine was being designed. While playback seems unaffected, WiFi noise in
capture stream prevents from using the videophone in speakerphone mode, but
in normal (handset) mode, the noise level seems still acceptable. The only
missing piece of a puzzle is my patch, that prevents the kernel driver from
putting corrupted values into the McBSP control registers.

That learned, please reconsider:
1. Can my patch break anything, related or not?
2. How does it affect performance of otherwise unaffected machines?
3. Is correcting a poorly designed machine behaviour worth of the change?

Thanks,
Janusz

---

From: Eero Nurkkala <[email-masked]>
Date: Fri, 04 Sep 2009 14:07:11 +0300

> 1. Can my patch break anything, related or not?

That needs to be investigated in more detail.

> 2. How does it affect performance of otherwise unaffected machines?

Like you already know, shouldn't.

> 3. Is correcting a poorly designed machine behaviour worth of the change?

I'd say this is in the right track. All McBSP registers
(not status etc) will need to be stored in memory. (Now, they're not).
And all those register contents will need to be written back
in certain situations. So this is the case when there is an external
audio chip, that takes audio in in bursts. So once the
burst is complete, all McBSP clocks will be disabled for a duration.

Then (at least >= 3430) device will hit OFF mode, meaning
all register contents are lost, also McBSP's. They will
need to be written back at some point.

My quick verdict is, that your patch is, going into right
direction. But the thing is that my words don't count much ;)

(Possibly worth taking the patch in, if guaranteed to not break
 any others).

- Eero

---

From: Anuj Aggarwal <[email-masked]>
Date: Sat, 5 Sep 2009 23:01:05 +0530

> need to be written back at some point.
Isn't the better way to achieve this thing, at least from the OFF mode
perspective, is to provide similar implementation for McBSP in
omap3_core_save_context () / omap3_core_restore_context(), in
file arch/arm/mach-omap2/pm34xx.c? Code for other core peripherals
is already there and we have to add support for rest of the core-domain
peripherals.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email-masked]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Best Regards,
Anuj Aggarwal

---

From: <[email-masked]>
Date: Sat, 5 Sep 2009 22:46:53 +0200

> is already there and we have to add support for rest of the core-domain

Yes and no. I'm not sure McBSP context would be needed to be restored
after every wake up from PER off mode.. isn't that how it's handled now
for other peripherals? But maybe restore context for McBSP whenever
there's need for it. Rewriting millions of registers just for the case doesn't
sound good. IMO, with an irq from an external device, a rewrite of  the
McBSP registers occurs. You loose the point of the OFF mode if you always
start rewriting all registers? (you're not saving power.. the opposite).
Or did I not understand something? (most likely the case) =)

- Eero