[RESEND] ARM: dts: OMAP3-N900: Add microphone bias voltages

Commit Message

Jarkko Nikula March 1, 2015, 7:07 p.m. UTC

From: Pavel Machek <[e-mail]>

N900 audio recording needs that codec provides bias voltage for integrated
digital microphone and headset microphone depending which one is used.
Digital microphone uses 2 V bias and it comes from the codec A part. Codec
B part drives the headset microphone bias and that is set to 2.5 V.

Signed-off-by: Pavel Machek <[e-mail]>
[Jarkko: Headset mic bias changed to 2 (2.5 V) as it was before commit
e2e8bfdf6157 ("ASoC: tlv320aic3x: Convert mic bias to a supply widget")]
Signed-off-by: Jarkko Nikula <[e-mail]>
---
 arch/arm/boot/dts/omap3-n900.dts | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Jarkko Nikula March 30, 2015, 4:30 p.m. UTC | #1

On 03/01/2015 09:07 PM, Jarkko Nikula wrote:
> From: Pavel Machek <[e-mail]>
> 
> N900 audio recording needs that codec provides bias voltage for integrated
> digital microphone and headset microphone depending which one is used.
> Digital microphone uses 2 V bias and it comes from the codec A part. Codec
> B part drives the headset microphone bias and that is set to 2.5 V.
> 
> Signed-off-by: Pavel Machek <[e-mail]>
> [Jarkko: Headset mic bias changed to 2 (2.5 V) as it was before commit
> e2e8bfdf6157 ("ASoC: tlv320aic3x: Convert mic bias to a supply widget")]
> Signed-off-by: Jarkko Nikula <[e-mail]>
> ---
>  arch/arm/boot/dts/omap3-n900.dts | 4 ++++
>  1 file changed, 4 insertions(+)
> 
Ping?

Tony Lindgren March 30, 2015, 4:42 p.m. UTC | #2

* Jarkko Nikula <[e-mail]> [150330 09:31]:
> On 03/01/2015 09:07 PM, Jarkko Nikula wrote:
> > From: Pavel Machek <[e-mail]>
> > 
> > N900 audio recording needs that codec provides bias voltage for integrated
> > digital microphone and headset microphone depending which one is used.
> > Digital microphone uses 2 V bias and it comes from the codec A part. Codec
> > B part drives the headset microphone bias and that is set to 2.5 V.
> > 
> > Signed-off-by: Pavel Machek <[e-mail]>
> > [Jarkko: Headset mic bias changed to 2 (2.5 V) as it was before commit
> > e2e8bfdf6157 ("ASoC: tlv320aic3x: Convert mic bias to a supply widget")]
> > Signed-off-by: Jarkko Nikula <[e-mail]>
> > ---
> >  arch/arm/boot/dts/omap3-n900.dts | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> Ping?

Oops sorry I have missed this one. This looks like a regression fix
for the v4.0-rc series? Should it be cc stable v3.16+ or something?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jarkko Nikula March 30, 2015, 5:45 p.m. UTC | #3

On 03/30/2015 07:42 PM, Tony Lindgren wrote:
> * Jarkko Nikula <[e-mail]> [150330 09:31]:
>> On 03/01/2015 09:07 PM, Jarkko Nikula wrote:
>>> From: Pavel Machek <[e-mail]>
>>>
>>> N900 audio recording needs that codec provides bias voltage for integrated
>>> digital microphone and headset microphone depending which one is used.
>>> Digital microphone uses 2 V bias and it comes from the codec A part. Codec
>>> B part drives the headset microphone bias and that is set to 2.5 V.
>>>
>>> Signed-off-by: Pavel Machek <[e-mail]>
>>> [Jarkko: Headset mic bias changed to 2 (2.5 V) as it was before commit
>>> e2e8bfdf6157 ("ASoC: tlv320aic3x: Convert mic bias to a supply widget")]
>>> Signed-off-by: Jarkko Nikula <[e-mail]>
>>> ---
>>>  arch/arm/boot/dts/omap3-n900.dts | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>> Ping?
> 
> Oops sorry I have missed this one. This looks like a regression fix
> for the v4.0-rc series? Should it be cc stable v3.16+ or something?
> 
Well, there has been regression but finding exactly how far should the
fix go didn't look instantly straightforward due all DT, codec driver
mic bias etc changes and I ended up not cc'ing stable.

But well, I guess first kernel where this commit makes sense is 3.16+
due commit f7d0f2a08567 ("ARM: dts: omap3-n900: Add sound support").
Although it applies on top of commit 14e3e295b2b9 ("ARM: dts:
omap3-n900: Add TLV320AIC3X support") too (3.12+) but not before that.

Tony Lindgren March 30, 2015, 5:50 p.m. UTC | #4

* Jarkko Nikula <[e-mail]> [150330 10:46]:
> On 03/30/2015 07:42 PM, Tony Lindgren wrote:
> > * Jarkko Nikula <[e-mail]> [150330 09:31]:
> >> On 03/01/2015 09:07 PM, Jarkko Nikula wrote:
> >>> From: Pavel Machek <[e-mail]>
> >>>
> >>> N900 audio recording needs that codec provides bias voltage for integrated
> >>> digital microphone and headset microphone depending which one is used.
> >>> Digital microphone uses 2 V bias and it comes from the codec A part. Codec
> >>> B part drives the headset microphone bias and that is set to 2.5 V.
> >>>
> >>> Signed-off-by: Pavel Machek <[e-mail]>
> >>> [Jarkko: Headset mic bias changed to 2 (2.5 V) as it was before commit
> >>> e2e8bfdf6157 ("ASoC: tlv320aic3x: Convert mic bias to a supply widget")]
> >>> Signed-off-by: Jarkko Nikula <[e-mail]>
> >>> ---
> >>>  arch/arm/boot/dts/omap3-n900.dts | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >> Ping?
> > 
> > Oops sorry I have missed this one. This looks like a regression fix
> > for the v4.0-rc series? Should it be cc stable v3.16+ or something?
> > 
> Well, there has been regression but finding exactly how far should the
> fix go didn't look instantly straightforward due all DT, codec driver
> mic bias etc changes and I ended up not cc'ing stable.
> 
> But well, I guess first kernel where this commit makes sense is 3.16+
> due commit f7d0f2a08567 ("ARM: dts: omap3-n900: Add sound support").
> Although it applies on top of commit 14e3e295b2b9 ("ARM: dts:
> omap3-n900: Add TLV320AIC3X support") too (3.12+) but not before that.

OK I think debian is using v3.16 kernel and that's pretty much the
first kernel that is usable with dts on many omap3 devices so might
make sense for that. I can add it if you think it makes sense.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sebastian Reichel March 31, 2015, 12:32 p.m. UTC | #5

Hi,

On Mon, Mar 30, 2015 at 10:50:52AM -0700, Tony Lindgren wrote:
> * Jarkko Nikula <[e-mail]> [150330 10:46]:
> > Well, there has been regression but finding exactly how far should the
> > fix go didn't look instantly straightforward due all DT, codec driver
> > mic bias etc changes and I ended up not cc'ing stable.
> > 
> > But well, I guess first kernel where this commit makes sense is 3.16+
> > due commit f7d0f2a08567 ("ARM: dts: omap3-n900: Add sound support").
> > Although it applies on top of commit 14e3e295b2b9 ("ARM: dts:
> > omap3-n900: Add TLV320AIC3X support") too (3.12+) but not before that.
> 
> OK I think debian is using v3.16 kernel

Yes. It will be used for Debian jessie (not yet released) and the
N900 related drivers are enabled in the armmp flavour. Unfortunately
it does not work together with thumb using userland because the
errata 430973 workaround is not enabled.

See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=768890

I guess it should be tried to change the workaround, so that it does
only change the behaviour of affected platforms. Otherwise its a
hard decision for distributions to enable the workaround.

> and that's pretty much the first kernel that is usable with dts on
> many omap3 devices so might make sense for that.

DT support for N900's soundcard has been added in 3.16, so before
that the audio stuff didn't work at all.

> I can add it if you think it makes sense.

I guess backporting this makes sense because of fewer "broken" DTB
files in the wild.

-- Sebastian

Tony Lindgren April 1, 2015, 7:47 p.m. UTC | #6

* Sebastian Reichel <[e-mail]> [150331 05:33]:
> Hi,
> 
> On Mon, Mar 30, 2015 at 10:50:52AM -0700, Tony Lindgren wrote:
> > * Jarkko Nikula <[e-mail]> [150330 10:46]:
> > > Well, there has been regression but finding exactly how far should the
> > > fix go didn't look instantly straightforward due all DT, codec driver
> > > mic bias etc changes and I ended up not cc'ing stable.
> > > 
> > > But well, I guess first kernel where this commit makes sense is 3.16+
> > > due commit f7d0f2a08567 ("ARM: dts: omap3-n900: Add sound support").
> > > Although it applies on top of commit 14e3e295b2b9 ("ARM: dts:
> > > omap3-n900: Add TLV320AIC3X support") too (3.12+) but not before that.
> > 
> > OK I think debian is using v3.16 kernel
> 
> Yes. It will be used for Debian jessie (not yet released) and the
> N900 related drivers are enabled in the armmp flavour. Unfortunately
> it does not work together with thumb using userland because the
> errata 430973 workaround is not enabled.
> 
> See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=768890

Hmm I believe many ARMv8 boards will be randomly oopsing
with armhf without that. I sort of recall random oopses just
with running apt-get update on ARMv8 omaps on armhf without that:

http://www.spinics.net/lists/linux-omap/msg108511.html

See also 5c86c5339c56 ("ARM: omap2plus_defconfig: Enable ARM erratum
430973 for omap3").

I wonder if the ARMv8 revision range might be wrong 430973 in
kernel or errata? Also I recall that 430973 change to the
arch/arm/mm/proc-v7-2level.S fixed the issue, this should be
verified though.
 
> I guess it should be tried to change the workaround, so that it does
> only change the behaviour of affected platforms. Otherwise its a
> hard decision for distributions to enable the workaround.

Well we should figure out first why flush BTAC/BTB is needed in
cpu_v7_switch_mm.. And if what I'm describing above is still
reproducable.
 
> > and that's pretty much the first kernel that is usable with dts on
> > many omap3 devices so might make sense for that.
> 
> DT support for N900's soundcard has been added in 3.16, so before
> that the audio stuff didn't work at all.
> 
> > I can add it if you think it makes sense.
> 
> I guess backporting this makes sense because of fewer "broken" DTB
> files in the wild.

Anyways yeah adding $subject patch into omap-for-v4.0/fixes with
cc stable v3.16+.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sebastian Reichel April 3, 2015, 4:35 p.m. UTC | #7

Hi,

On Wed, Apr 01, 2015 at 12:47:36PM -0700, Tony Lindgren wrote:
> > > OK I think debian is using v3.16 kernel
> > 
> > Yes. It will be used for Debian jessie (not yet released) and the
> > N900 related drivers are enabled in the armmp flavour. Unfortunately
> > it does not work together with thumb using userland because the
> > errata 430973 workaround is not enabled.
> > 
> > See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=768890
> 
> Hmm I believe many ARMv8 boards will be randomly oopsing
> with armhf without that. I sort of recall random oopses just
> with running apt-get update on ARMv8 omaps on armhf without that:

Since I don't know of any ARMv8 omaps, I will read ARMv8 as ARMv7.

And yes, for armhf userland one gets random oopses at least on the
Nokia N900. AFAIK this is not true for all ARMv7 processors
(especially non omaps), though.

> http://www.spinics.net/lists/linux-omap/msg108511.html
> 
> See also 5c86c5339c56 ("ARM: omap2plus_defconfig: Enable ARM erratum
> 430973 for omap3").

For me the random oopses occur without this config flag and are
fixed by it. The workaround is not very suitable for multi platform
kernels, though, since its enabled also for unaffected platforms.

As far as I can see the CONFIG_ARM_ERRATA_430973 flag is checked
in proc-v7.S and in proc-v7-2level.S. I think the first file is
irrelevant, since it can be fixed later (see workaround in
nokia_n900_legacy_init in pdata-quirks.c).

So basically the problem comes down to proc-v7-2level.S

> I wonder if the ARMv8 revision range might be wrong 430973 in
> kernel or errata?

what revision range? I think the errata is enabled unconditionally
or disabled completly.

> Also I recall that 430973 change to the
> arch/arm/mm/proc-v7-2level.S fixed the issue, this should be
> verified though.

If I understand the errata correctly the BTAC/BTB flushing is
important. It would be nice if it could be limited to affected
devices, though.

> > I guess it should be tried to change the workaround, so that it does
> > only change the behaviour of affected platforms. Otherwise its a
> > hard decision for distributions to enable the workaround.
> 
> Well we should figure out first why flush BTAC/BTB is needed in
> cpu_v7_switch_mm.. And if what I'm describing above is still
> reproducable.

Reading the help text in the kernel the flushing is the actual
workaround. The other changes only make it possible to do the
flushing.

Maybe an option would be to provide two cpu_v7_switch_mm
implementations (one with the errata and one without). Then
the system can start with the simple implementation. Once
the boot as progressed far enough to know, that the hardware
is affected by the errata, it could switch to the implementation
with the flushing.

-- Sebastian

Tony Lindgren April 3, 2015, 6:39 p.m. UTC | #8

* Sebastian Reichel <[e-mail]> [150403 09:37]:
> Hi,
> 
> On Wed, Apr 01, 2015 at 12:47:36PM -0700, Tony Lindgren wrote:
> > > > OK I think debian is using v3.16 kernel
> > > 
> > > Yes. It will be used for Debian jessie (not yet released) and the
> > > N900 related drivers are enabled in the armmp flavour. Unfortunately
> > > it does not work together with thumb using userland because the
> > > errata 430973 workaround is not enabled.
> > > 
> > > See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=768890
> > 
> > Hmm I believe many ARMv8 boards will be randomly oopsing
> > with armhf without that. I sort of recall random oopses just
> > with running apt-get update on ARMv8 omaps on armhf without that:
> 
> Since I don't know of any ARMv8 omaps, I will read ARMv8 as ARMv7.

Sorry right, s/ARMv8/Cortex-A8/ :)
 
> And yes, for armhf userland one gets random oopses at least on the
> Nokia N900. AFAIK this is not true for all ARMv7 processors
> (especially non omaps), though.
> 
> > http://www.spinics.net/lists/linux-omap/msg108511.html
> > 
> > See also 5c86c5339c56 ("ARM: omap2plus_defconfig: Enable ARM erratum
> > 430973 for omap3").
> 
> For me the random oopses occur without this config flag and are
> fixed by it. The workaround is not very suitable for multi platform
> kernels, though, since its enabled also for unaffected platforms.
> 
> As far as I can see the CONFIG_ARM_ERRATA_430973 flag is checked
> in proc-v7.S and in proc-v7-2level.S. I think the first file is
> irrelevant, since it can be fixed later (see workaround in
> nokia_n900_legacy_init in pdata-quirks.c).

Yes so it seems, and the bootloaders should really set it. It's
also disabled for multiplatform builds.
 
> So basically the problem comes down to proc-v7-2level.S
> 
> > I wonder if the ARMv8 revision range might be wrong 430973 in
> > kernel or errata?
> 
> what revision range? I think the errata is enabled unconditionally
> or disabled completly.

The Cortex-A8 range claimed to be affected in Kconfig is listed
as r1p0..r1p2. But I recall seeing this also on omap3 (37xx)
which is r3p2. Also searching for r3p2 430973 produces:

http://e2e.ti.com/support/arm/sitara_arm/f/791/p/254742/891611

And that points to Siarhei's test program that should expose it
run together with other processes:

https://cloud.github.com/downloads/ssvb/ssvb.github.com/ssvb-cpuburn-a8.S

> > Also I recall that 430973 change to the
> > arch/arm/mm/proc-v7-2level.S fixed the issue, this should be
> > verified though.
> 
> If I understand the errata correctly the BTAC/BTB flushing is
> important. It would be nice if it could be limited to affected
> devices, though.

Agreed.

> > > I guess it should be tried to change the workaround, so that it does
> > > only change the behaviour of affected platforms. Otherwise its a
> > > hard decision for distributions to enable the workaround.
> > 
> > Well we should figure out first why flush BTAC/BTB is needed in
> > cpu_v7_switch_mm.. And if what I'm describing above is still
> > reproducable.
> 
> Reading the help text in the kernel the flushing is the actual
> workaround. The other changes only make it possible to do the
> flushing.
> 
> Maybe an option would be to provide two cpu_v7_switch_mm
> implementations (one with the errata and one without). Then
> the system can start with the simple implementation. Once
> the boot as progressed far enough to know, that the hardware
> is affected by the errata, it could switch to the implementation
> with the flushing.

That seems like a good way to deal with it but we should first
verify it Siarhei's test program.

Also, I think that the armel distro did not have these issue
while armhf did, so there may something more to this bug.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Robert Nelson April 3, 2015, 7:21 p.m. UTC | #9

>> And yes, for armhf userland one gets random oopses at least on the
>> Nokia N900. AFAIK this is not true for all ARMv7 processors
>> (especially non omaps), though.
>>
>> > http://www.spinics.net/lists/linux-omap/msg108511.html
>> >
>> > See also 5c86c5339c56 ("ARM: omap2plus_defconfig: Enable ARM erratum
>> > 430973 for omap3").
>>
>> For me the random oopses occur without this config flag and are
>> fixed by it. The workaround is not very suitable for multi platform
>> kernels, though, since its enabled also for unaffected platforms.
>>
>> As far as I can see the CONFIG_ARM_ERRATA_430973 flag is checked
>> in proc-v7.S and in proc-v7-2level.S. I think the first file is
>> irrelevant, since it can be fixed later (see workaround in
>> nokia_n900_legacy_init in pdata-quirks.c).
>
> Yes so it seems, and the bootloaders should really set it. It's
> also disabled for multiplatform builds.

These are now done in u-boot as of: v2015.04-rc4

http://git.denx.de/?p=u-boot.git;a=commit;h=c6f90e1418a84fe5fa463b38403bd1845cb6a59c

Regards,

Pavel Machek April 3, 2015, 8:42 p.m. UTC | #10

Hi!

> Maybe an option would be to provide two cpu_v7_switch_mm
> implementations (one with the errata and one without). Then
> the system can start with the simple implementation. Once
> the boot as progressed far enough to know, that the hardware
> is affected by the errata, it could switch to the implementation
> with the flushing.

Actually... we should start with the slow&safe option, and then switch
to faster implementation if workaround is not needed.
									Pavel

Ivaylo Dimitrov April 3, 2015, 10:08 p.m. UTC | #11

Hi

On  3.04.2015 19:35, Sebastian Reichel wrote:
>
> Maybe an option would be to provide two cpu_v7_switch_mm
> implementations (one with the errata and one without). Then
> the system can start with the simple implementation. Once
> the boot as progressed far enough to know, that the hardware
> is affected by the errata, it could switch to the implementation
> with the flushing.

Or rather the opposite, as if the kernel itself is thumb-compiled, it 
most probably will have no chance to progress enough to switch to the 
correct implementation before hanging.
>
> -- Sebastian
>

Ivo
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren April 3, 2015, 10:15 p.m. UTC | #12

* Ivaylo Dimitrov <[e-mail]> [150403 15:08]:
> Hi
> 
> On  3.04.2015 19:35, Sebastian Reichel wrote:
> >
> >Maybe an option would be to provide two cpu_v7_switch_mm
> >implementations (one with the errata and one without). Then
> >the system can start with the simple implementation. Once
> >the boot as progressed far enough to know, that the hardware
> >is affected by the errata, it could switch to the implementation
> >with the flushing.
> 
> Or rather the opposite, as if the kernel itself is thumb-compiled, it most
> probably will have no chance to progress enough to switch to the correct
> implementation before hanging.

We should first verify the same bug happens with armel also.
I just verified the CPU load in the background makes armhf
apps segfault without $subject workaround enabled.

If the segfaulting does not happen with armel, then chances
it's some kind of neon related issue and the fix can be more
targeted.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivaylo Dimitrov April 3, 2015, 10:47 p.m. UTC | #13

Hi,

> We should first verify the same bug happens with armel also.
> I just verified the CPU load in the background makes armhf
> apps segfault without $subject workaround enabled.
>
> If the segfaulting does not happen with armel, then chances
> it's some kind of neon related issue and the fix can be more
> targeted.
>
> Regards,
>
> Tony
>

I can assure you that at least SoC in N900 is affected by ARM errata 
430973, no matter armel or armhf. Though the crash is usually with 
SIGILL(because of the wrong T bit in CPSR the code is executed with), I 
don't remember seeing SIGSEGV, but that could be my ageing memory. I am 
the maintainer of the so-called cssu-thumb Fremantle upgrade [1], which 
is armel, thumb2 compiled, so I have some experience with the matter  :) 
. Without that errata workaround enabled in kernel, it is impossible to 
boot Fremantle with cssu-thumb on top. BTW it was the same back in the 
days when there was Ubuntu 12.04 for N900 - it was hardfp, thumb2 
compiled. Again, without that errata workaround enabled, there was no 
way to boot it.

Regards,
Ivo

[1] http://talk.maemo.org/showthread.php?t=84829
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren April 3, 2015, 10:52 p.m. UTC | #14

* Ivaylo Dimitrov <[e-mail]> [150403 15:47]:
> 
> Hi,
> 
> >We should first verify the same bug happens with armel also.
> >I just verified the CPU load in the background makes armhf
> >apps segfault without $subject workaround enabled.
> >
> >If the segfaulting does not happen with armel, then chances
> >it's some kind of neon related issue and the fix can be more
> >targeted.
> >
> >Regards,
> >
> >Tony
> >
> 
> I can assure you that at least SoC in N900 is affected by ARM errata 430973,
> no matter armel or armhf. Though the crash is usually with SIGILL(because of
> the wrong T bit in CPSR the code is executed with), I don't remember seeing
> SIGSEGV, but that could be my ageing memory. I am the maintainer of the
> so-called cssu-thumb Fremantle upgrade [1], which is armel, thumb2 compiled,
> so I have some experience with the matter  :) . Without that errata
> workaround enabled in kernel, it is impossible to boot Fremantle with
> cssu-thumb on top. BTW it was the same back in the days when there was
> Ubuntu 12.04 for N900 - it was hardfp, thumb2 compiled. Again, without that
> errata workaround enabled, there was no way to boot it.

Right, it affects n900 for sure. My point is that it also seems to
affect 37xx versions not listed to suffer from this issue.

Regards,

Tony

 
> [1] http://talk.maemo.org/showthread.php?t=84829
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matthijs van Duin April 5, 2015, 4:13 a.m. UTC | #15

On 4 April 2015 at 00:52, Tony Lindgren <[e-mail]> wrote:
> Right, it affects n900 for sure. My point is that it also seems to
> affect 37xx versions not listed to suffer from this issue.

They shouldn't... erratum 430973 only affected Cortex-A8 r1, and the
dm37xx should have an r3p2 right?

A word of caution though: at least on the DM814x and AM335x, secure
ROM sets bit 6 (IBE) in the Auxiliary Control Register, thereby
enabling BTB invalidate instructions (which normally execute as nops).
This is presumably a leftover of the erratum 430973 workaround, but it
means there is a risk of running afoul of erratum 687067 if BTB
invalidate by MVA instructions are actually used.

I would actually suggest clearing IBE if it set on Cortex-A8 r2 or
later processors and a secure monitor call is available to do so
(there is on the DM814x and AM335x, dunno about the 37xx), also for
performance reasons: BTB invalidates are quite expensive instructions
(when enabled).

Matthijs
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivaylo Dimitrov April 5, 2015, 7:23 a.m. UTC | #16

On  5.04.2015 07:13, Matthijs van Duin wrote:
> I would actually suggest clearing IBE if it set on Cortex-A8 r2 or
> later processors and a secure monitor call is available to do so
> (there is on the DM814x and AM335x, dunno about the 37xx), also for
> performance reasons: BTB invalidates are quite expensive instructions
> (when enabled).

There is (or should be) SM call, it is explained in 36xx TRM(SWPU177AA) 
as well, 26.4.1, "Booting Overview". Though I wonder why SMC is needed 
to write ACR on non-HS devices. A simple MRC should suffice, unless I 
miss something.

Clearing the IBE bit on devices that don't need erratum 430973 
workaround, along with enabling that workaround in kernel is the best 
approach IMO. That way we will gain both stability on cores that need 
the workaround (like on N900 and the other devices with p1) and won't 
lose performance on cores that are not affected.

>
> Matthijs
>

Regards,
Ivo
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sebastian Reichel April 5, 2015, 1 p.m. UTC | #17

On Fri, Apr 03, 2015 at 02:21:48PM -0500, Robert Nelson wrote:
> >> And yes, for armhf userland one gets random oopses at least on the
> >> Nokia N900. AFAIK this is not true for all ARMv7 processors
> >> (especially non omaps), though.
> >>
> >> > http://www.spinics.net/lists/linux-omap/msg108511.html
> >> >
> >> > See also 5c86c5339c56 ("ARM: omap2plus_defconfig: Enable ARM erratum
> >> > 430973 for omap3").
> >>
> >> For me the random oopses occur without this config flag and are
> >> fixed by it. The workaround is not very suitable for multi platform
> >> kernels, though, since its enabled also for unaffected platforms.
> >>
> >> As far as I can see the CONFIG_ARM_ERRATA_430973 flag is checked
> >> in proc-v7.S and in proc-v7-2level.S. I think the first file is
> >> irrelevant, since it can be fixed later (see workaround in
> >> nokia_n900_legacy_init in pdata-quirks.c).
> >
> > Yes so it seems, and the bootloaders should really set it. It's
> > also disabled for multiplatform builds.
> 
> These are now done in u-boot as of: v2015.04-rc4
> 
> http://git.denx.de/?p=u-boot.git;a=commit;h=c6f90e1418a84fe5fa463b38403bd1845cb6a59c

Seems like "include/configs/nokia_rx51.h" has been forgotten. Note,
that the kernel must still be able to enable the bit itself (in case
NOLO is used without u-boot).

-- Sebastian

Pali Rohár April 5, 2015, 1:26 p.m. UTC | #18

On Sunday 05 April 2015 15:00:45 Sebastian Reichel wrote:
> On Fri, Apr 03, 2015 at 02:21:48PM -0500, Robert Nelson wrote:
> > >> And yes, for armhf userland one gets random oopses at
> > >> least on the Nokia N900. AFAIK this is not true for all
> > >> ARMv7 processors (especially non omaps), though.
> > >> 
> > >> > http://www.spinics.net/lists/linux-omap/msg108511.html
> > >> > 
> > >> > See also 5c86c5339c56 ("ARM: omap2plus_defconfig:
> > >> > Enable ARM erratum 430973 for omap3").
> > >> 
> > >> For me the random oopses occur without this config flag
> > >> and are fixed by it. The workaround is not very suitable
> > >> for multi platform kernels, though, since its enabled
> > >> also for unaffected platforms.
> > >> 
> > >> As far as I can see the CONFIG_ARM_ERRATA_430973 flag is
> > >> checked in proc-v7.S and in proc-v7-2level.S. I think
> > >> the first file is irrelevant, since it can be fixed
> > >> later (see workaround in nokia_n900_legacy_init in
> > >> pdata-quirks.c).
> > > 
> > > Yes so it seems, and the bootloaders should really set it.
> > > It's also disabled for multiplatform builds.
> > 
> > These are now done in u-boot as of: v2015.04-rc4
> > 
> > http://git.denx.de/?p=u-boot.git;a=commit;h=c6f90e1418a84fe5
> > fa463b38403bd1845cb6a59c
> 
> Seems like "include/configs/nokia_rx51.h" has been forgotten.
> Note, that the kernel must still be able to enable the bit
> itself (in case NOLO is used without u-boot).
> 
> -- Sebastian

You must not expect that bootloader set something. Nokia N900 has 
closed and signed bootloader which cannot be easily replaced and 
or fixed.

For N900 there is U-Boot but it does not support lot of things 
(load kernel via usb or serial) so for developers it is better to 
use original Nokia bootloader.

Sebastian Reichel April 5, 2015, 1:39 p.m. UTC | #19

Hi,

On Sun, Apr 05, 2015 at 06:13:47AM +0200, Matthijs van Duin wrote:
> On 4 April 2015 at 00:52, Tony Lindgren <[e-mail]> wrote:
> > Right, it affects n900 for sure. My point is that it also seems to
> > affect 37xx versions not listed to suffer from this issue.
> 
> They shouldn't... erratum 430973 only affected Cortex-A8 r1, and the
> dm37xx should have an r3p2 right?
> 
> A word of caution though: at least on the DM814x and AM335x, secure
> ROM sets bit 6 (IBE) in the Auxiliary Control Register, thereby
> enabling BTB invalidate instructions (which normally execute as nops).
> This is presumably a leftover of the erratum 430973 workaround, but it
> means there is a risk of running afoul of erratum 687067 if BTB
> invalidate by MVA instructions are actually used.
> 
> I would actually suggest clearing IBE if it set on Cortex-A8 r2 or
> later processors and a secure monitor call is available to do so
> (there is on the DM814x and AM335x, dunno about the 37xx), also for
> performance reasons: BTB invalidates are quite expensive instructions
> (when enabled).

So if I understand the issue correct, it would be ok to enable the
430973 workaround in cpu_v7_switch_mm for arm multiplatform kernels
(mcr p15, 0, r2, c7, c5, 6 @ flush BTAC/BTB). On unaffected
Cortex-A8 platforms the IBE bit should be unset (resulting in a nop
instead of the BTB flush).

So assuming, that the same is true for non Cortex-A8 platforms: Can
the workaround simply be enabled by default if CPU_V7 is selected?

-- Sebastian

Sebastian Reichel April 5, 2015, 1:45 p.m. UTC | #20

Hi,

On Sun, Apr 05, 2015 at 03:26:14PM +0200, Pali Rohár wrote:
> > > > Yes so it seems, and the bootloaders should really set it.
> > > > It's also disabled for multiplatform builds.
> > > 
> > > These are now done in u-boot as of: v2015.04-rc4
> > > 
> > > http://git.denx.de/?p=u-boot.git;a=commit;h=c6f90e1418a84fe5
> > > fa463b38403bd1845cb6a59c
> > 
> > Seems like "include/configs/nokia_rx51.h" has been forgotten.
> > Note, that the kernel must still be able to enable the bit
> > itself (in case NOLO is used without u-boot).
> 
> You must not expect that bootloader set something. Nokia N900 has 
> closed and signed bootloader which cannot be easily replaced and 
> or fixed.
> 
> For N900 there is U-Boot but it does not support lot of things 
> (load kernel via usb or serial) so for developers it is better to 
> use original Nokia bootloader.

Yes, I'm aware of that. I think having the IBE bit set in u-boot has
advantages nevertheless. I think the kernel can't enable the bit
sufficiently early if its compiled in thumb-mode itself
(CONFIG_THUMB2_KERNEL).

So I suggest to enable the IBE bit in the kernel (that part is
already done) *and* in u-boot.

-- Sebastian

Pali Rohár April 5, 2015, 1:52 p.m. UTC | #21

On Sunday 05 April 2015 15:45:28 Sebastian Reichel wrote:
> Hi,
> 
> On Sun, Apr 05, 2015 at 03:26:14PM +0200, Pali Rohár wrote:
> > > > > Yes so it seems, and the bootloaders should really set
> > > > > it. It's also disabled for multiplatform builds.
> > > > 
> > > > These are now done in u-boot as of: v2015.04-rc4
> > > > 
> > > > http://git.denx.de/?p=u-boot.git;a=commit;h=c6f90e1418a8
> > > > 4fe5 fa463b38403bd1845cb6a59c
> > > 
> > > Seems like "include/configs/nokia_rx51.h" has been
> > > forgotten. Note, that the kernel must still be able to
> > > enable the bit itself (in case NOLO is used without
> > > u-boot).
> > 
> > You must not expect that bootloader set something. Nokia
> > N900 has closed and signed bootloader which cannot be
> > easily replaced and or fixed.
> > 
> > For N900 there is U-Boot but it does not support lot of
> > things (load kernel via usb or serial) so for developers it
> > is better to use original Nokia bootloader.
> 
> Yes, I'm aware of that. I think having the IBE bit set in
> u-boot has advantages nevertheless. I think the kernel can't
> enable the bit sufficiently early if its compiled in
> thumb-mode itself (CONFIG_THUMB2_KERNEL).
> 
> So I suggest to enable the IBE bit in the kernel (that part is
> already done) *and* in u-boot.
> 
> -- Sebastian

Yes. Code for both (U-Boot and linux kernel) exists and are 
already included.

In linux kernel code is since 3.14-rc6 and in U-Boot code is 
there since beginning of Nokia N900 support (2013.04) directly in 
board code.

I believe that you guys do not remove or break that code :-)

Nokia N900 needs slightly different code for setting IBE bit as 
other omap3 boards. For info is in commit message of commit 
4748a7240284b0f68bd47a66365c2cd561939830.

Matthijs van Duin April 5, 2015, 4:50 p.m. UTC | #22

On 5 April 2015 at 09:23, Ivaylo Dimitrov <[e-mail]> wrote:
> Though I wonder why SMC is needed to write ACR on non-HS devices. A simple
> MRC should suffice, unless I miss something.

Public-world access to ACR varies per bit:
bit 1 (L2EN) is documented as banked, but at least on r3p2 turns out
to be common r/w.
bits 30-31 are secure read-only and public RAZ.
remaining bits are secure read/write and public read-only.

The net effect is that doing an MRC from public world will only modify
the L2EN bit.

There's no bit in the non-secure access control register to affect all
of this, so GP vs HS doesn't matter here (from a CPU point of view; it
may matter for the availability of SM calls obviously).

Matthijs
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matthijs van Duin April 5, 2015, 4:52 p.m. UTC | #23

On 5 April 2015 at 18:50, Matthijs van Duin <[e-mail]> wrote:
> MRC

I mean MCR of course
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivaylo Dimitrov April 5, 2015, 9:08 p.m. UTC | #24

On  5.04.2015 19:50, Matthijs van Duin wrote:
> On 5 April 2015 at 09:23, Ivaylo Dimitrov <[e-mail]> wrote:
>> Though I wonder why SMC is needed to write ACR on non-HS devices. A simple
>> MRC should suffice, unless I miss something.
>
> Public-world access to ACR varies per bit:
> bit 1 (L2EN) is documented as banked, but at least on r3p2 turns out
> to be common r/w.
> bits 30-31 are secure read-only and public RAZ.
> remaining bits are secure read/write and public read-only.
>
> The net effect is that doing an MRC from public world will only modify
> the L2EN bit.
>
> There's no bit in the non-secure access control register to affect all
> of this, so GP vs HS doesn't matter here (from a CPU point of view; it
> may matter for the availability of SM calls obviously).
>
> Matthijs
>

But then the first part(setting the IBE bit in ACR to 1) of the errata 
workaround is wrong, as it uses a plain MCR to set the IBE bit, see 
http://lxr.free-electrons.com/source/arch/arm/mm/proc-v7.S#L340. Which 
is weird, given that the workaround was posted by ARM iirc.

Ivo
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matthijs van Duin April 5, 2015, 11:52 p.m. UTC | #25

Cortex-A8 errata doc states in its workaround for erratum 430973:

> By default, the BTB Invalidate instruction is treated as a NOP on Cortex-A8.
> However, it is possible to enable the BTB Invalidate instruction such that it
> actually does a full invalidate of the BTB by setting the IBE bit (bit 6) in
> the CP15 Auxiliary Control Register. As a consequence of erratum 687067, the
> L1 System Array Debug Register should be cleared to 0 before the IBE bit is
> set using the following code sequence:
> MOV r1, #0
> MCR p15, 0, r1, c15, c1, 0            ; write instruction data 0 register
> MRC p15, 0, R1, c1, c0, 1             ; read Aux Ctl Register
> ORR R1, R1 #(1 << 6)                  ; set IBE to 1
> MCR p15, 0, R1, c1, c0, 1             ; write Aux Ctl Register
> The above code needs to be executed in Secure state. ARM Limited recommends
> that this code is added to the boot monitor.

The 430973 workaround code in proc-v7.S will do absolutely nothing if
executed in non-secure state. Ditto for the 458693 workaround, and the
460075 workaround should trigger an undefined instruction exception.
Maybe linux is started in secure mode on some targets and this code
was written for one of those?

I scanned DM814x secure ROM for any (ARM or Thumb) write to
Instruction L1 System Array Debug Register 0, but I found none, hence
my warning to watch out for erratum 687067.

Adding the full set of BTB invalidates while making sure IBE is
disabled on sufficiently recent Cortex-A8 revisions would be optimal
for the Cortex-A8. But, apparently (based on the description of the
ARMv7 CPUID registers) there are also processors which only require
BTB invalidates when code is modified, but not when context-switching,
so there may be performance considerations there...

Matthijs
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren April 6, 2015, 3:19 p.m. UTC | #26

* Matthijs van Duin <[e-mail]> [150405 16:53]:
> Cortex-A8 errata doc states in its workaround for erratum 430973:
> 
> > By default, the BTB Invalidate instruction is treated as a NOP on Cortex-A8.
> > However, it is possible to enable the BTB Invalidate instruction such that it
> > actually does a full invalidate of the BTB by setting the IBE bit (bit 6) in
> > the CP15 Auxiliary Control Register. As a consequence of erratum 687067, the
> > L1 System Array Debug Register should be cleared to 0 before the IBE bit is
> > set using the following code sequence:
> > MOV r1, #0
> > MCR p15, 0, r1, c15, c1, 0            ; write instruction data 0 register
> > MRC p15, 0, R1, c1, c0, 1             ; read Aux Ctl Register
> > ORR R1, R1 #(1 << 6)                  ; set IBE to 1
> > MCR p15, 0, R1, c1, c0, 1             ; write Aux Ctl Register
> > The above code needs to be executed in Secure state. ARM Limited recommends
> > that this code is added to the boot monitor.
> 
> The 430973 workaround code in proc-v7.S will do absolutely nothing if
> executed in non-secure state. Ditto for the 458693 workaround, and the
> 460075 workaround should trigger an undefined instruction exception.
> Maybe linux is started in secure mode on some targets and this code
> was written for one of those?

That's only for HS omaps, for those we currently only do it in the
nokia_n900_legacy_init that calls rx51_secure_update_aux_cr.

> I scanned DM814x secure ROM for any (ARM or Thumb) write to
> Instruction L1 System Array Debug Register 0, but I found none, hence
> my warning to watch out for erratum 687067.

OK
 
> Adding the full set of BTB invalidates while making sure IBE is
> disabled on sufficiently recent Cortex-A8 revisions would be optimal
> for the Cortex-A8. But, apparently (based on the description of the
> ARMv7 CPUID registers) there are also processors which only require
> BTB invalidates when code is modified, but not when context-switching,
> so there may be performance considerations there...

Attempting to summarize all that's been discussed.. It sounds like we
need the following implemented:

1. For cortex-a8 revisions affected by 458693, we can do a custom
   cpu_v7_switch_mm function that always does flush BTAC/BTB.

2. For HS cortex-a8 processors other than n900 affected by 458693,
   we need to implement functions similar to rx51_secure_update_aux_cr,
   the bootrom on n900 is different from TI HS omaps so the SMC call
   numbering may be different.

3. For later cortex-a8 processors not affected by 458693, we need
   to clear IBE bit to avoid erratum 687067.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren April 6, 2015, 3:24 p.m. UTC | #27

* Sebastian Reichel <[e-mail]> [150405 06:40]:
> Hi,
> 
> On Sun, Apr 05, 2015 at 06:13:47AM +0200, Matthijs van Duin wrote:
> > On 4 April 2015 at 00:52, Tony Lindgren <[e-mail]> wrote:
> > > Right, it affects n900 for sure. My point is that it also seems to
> > > affect 37xx versions not listed to suffer from this issue.
> > 
> > They shouldn't... erratum 430973 only affected Cortex-A8 r1, and the
> > dm37xx should have an r3p2 right?
> > 
> > A word of caution though: at least on the DM814x and AM335x, secure
> > ROM sets bit 6 (IBE) in the Auxiliary Control Register, thereby
> > enabling BTB invalidate instructions (which normally execute as nops).
> > This is presumably a leftover of the erratum 430973 workaround, but it
> > means there is a risk of running afoul of erratum 687067 if BTB
> > invalidate by MVA instructions are actually used.
> > 
> > I would actually suggest clearing IBE if it set on Cortex-A8 r2 or
> > later processors and a secure monitor call is available to do so
> > (there is on the DM814x and AM335x, dunno about the 37xx), also for
> > performance reasons: BTB invalidates are quite expensive instructions
> > (when enabled).
> 
> So if I understand the issue correct, it would be ok to enable the
> 430973 workaround in cpu_v7_switch_mm for arm multiplatform kernels
> (mcr p15, 0, r2, c7, c5, 6 @ flush BTAC/BTB). On unaffected
> Cortex-A8 platforms the IBE bit should be unset (resulting in a nop
> instead of the BTB flush).

Well we only want to enable for cortex-a8 revisions affected by
430973, so a custom cpu_v7_switch_mm seems like the way to go there.

And then we need checks for clearing IBE for unaffected cortex-a8
revisions. I'll check to today if I have IBE bit set on the systems
where I'm seeing issues that should not be affected by 430973.
 
> So assuming, that the same is true for non Cortex-A8 platforms: Can
> the workaround simply be enabled by default if CPU_V7 is selected?

It should be enabled conditionally.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren April 6, 2015, 3:40 p.m. UTC | #28

* Tony Lindgren <[e-mail]> [150406 08:24]:
> * Matthijs van Duin <[e-mail]> [150405 16:53]:
> > Cortex-A8 errata doc states in its workaround for erratum 430973:
> > 
> > > By default, the BTB Invalidate instruction is treated as a NOP on Cortex-A8.
> > > However, it is possible to enable the BTB Invalidate instruction such that it
> > > actually does a full invalidate of the BTB by setting the IBE bit (bit 6) in
> > > the CP15 Auxiliary Control Register. As a consequence of erratum 687067, the
> > > L1 System Array Debug Register should be cleared to 0 before the IBE bit is
> > > set using the following code sequence:
> > > MOV r1, #0
> > > MCR p15, 0, r1, c15, c1, 0            ; write instruction data 0 register
> > > MRC p15, 0, R1, c1, c0, 1             ; read Aux Ctl Register
> > > ORR R1, R1 #(1 << 6)                  ; set IBE to 1
> > > MCR p15, 0, R1, c1, c0, 1             ; write Aux Ctl Register
> > > The above code needs to be executed in Secure state. ARM Limited recommends
> > > that this code is added to the boot monitor.
> > 
> > The 430973 workaround code in proc-v7.S will do absolutely nothing if
> > executed in non-secure state. Ditto for the 458693 workaround, and the
> > 460075 workaround should trigger an undefined instruction exception.
> > Maybe linux is started in secure mode on some targets and this code
> > was written for one of those?
> 
> That's only for HS omaps, for those we currently only do it in the
> nokia_n900_legacy_init that calls rx51_secure_update_aux_cr.
> 
> > I scanned DM814x secure ROM for any (ARM or Thumb) write to
> > Instruction L1 System Array Debug Register 0, but I found none, hence
> > my warning to watch out for erratum 687067.
> 
> OK
>  
> > Adding the full set of BTB invalidates while making sure IBE is
> > disabled on sufficiently recent Cortex-A8 revisions would be optimal
> > for the Cortex-A8. But, apparently (based on the description of the
> > ARMv7 CPUID registers) there are also processors which only require
> > BTB invalidates when code is modified, but not when context-switching,
> > so there may be performance considerations there...
> 
> Attempting to summarize all that's been discussed.. It sounds like we
> need the following implemented:
> 
> 1. For cortex-a8 revisions affected by 458693, we can do a custom
>    cpu_v7_switch_mm function that always does flush BTAC/BTB.
> 
> 2. For HS cortex-a8 processors other than n900 affected by 458693,
>    we need to implement functions similar to rx51_secure_update_aux_cr,
>    the bootrom on n900 is different from TI HS omaps so the SMC call
>    numbering may be different.
> 
> 3. For later cortex-a8 processors not affected by 458693, we need
>    to clear IBE bit to avoid erratum 687067.

Oops sorry, wrong numbers for errata above.. s/458693/430973/, here's
a better version:

1. For cortex-a8 revisions affected by 430973, we can do a custom
   cpu_v7_switch_mm function that always does flush BTAC/BTB.
 
2. For HS cortex-a8 processors other than n900 affected by 430973,
   we need to implement functions similar to rx51_secure_update_aux_cr,
   the bootrom on n900 is different from TI HS omaps so the SMC call
   numbering may be different.
 
3. For later cortex-a8 processors not affected by 430973, we need
   to clear IBE bit to avoid erratum 687067.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivaylo Dimitrov April 6, 2015, 5:14 p.m. UTC | #29

On  6.04.2015 18:40, Tony Lindgren wrote:
> * Tony Lindgren <[e-mail]> [150406 08:24]:
>> * Matthijs van Duin <[e-mail]> [150405 16:53]:
>>> Cortex-A8 errata doc states in its workaround for erratum 430973:
>>>
>>>> By default, the BTB Invalidate instruction is treated as a NOP on Cortex-A8.
>>>> However, it is possible to enable the BTB Invalidate instruction such that it
>>>> actually does a full invalidate of the BTB by setting the IBE bit (bit 6) in
>>>> the CP15 Auxiliary Control Register. As a consequence of erratum 687067, the
>>>> L1 System Array Debug Register should be cleared to 0 before the IBE bit is
>>>> set using the following code sequence:
>>>> MOV r1, #0
>>>> MCR p15, 0, r1, c15, c1, 0            ; write instruction data 0 register
>>>> MRC p15, 0, R1, c1, c0, 1             ; read Aux Ctl Register
>>>> ORR R1, R1 #(1 << 6)                  ; set IBE to 1
>>>> MCR p15, 0, R1, c1, c0, 1             ; write Aux Ctl Register
>>>> The above code needs to be executed in Secure state. ARM Limited recommends
>>>> that this code is added to the boot monitor.
>>>
>>> The 430973 workaround code in proc-v7.S will do absolutely nothing if
>>> executed in non-secure state. Ditto for the 458693 workaround, and the
>>> 460075 workaround should trigger an undefined instruction exception.
>>> Maybe linux is started in secure mode on some targets and this code
>>> was written for one of those?
>>
>> That's only for HS omaps, for those we currently only do it in the
>> nokia_n900_legacy_init that calls rx51_secure_update_aux_cr.
>>
>>> I scanned DM814x secure ROM for any (ARM or Thumb) write to
>>> Instruction L1 System Array Debug Register 0, but I found none, hence
>>> my warning to watch out for erratum 687067.
>>
>> OK
>>
>>> Adding the full set of BTB invalidates while making sure IBE is
>>> disabled on sufficiently recent Cortex-A8 revisions would be optimal
>>> for the Cortex-A8. But, apparently (based on the description of the
>>> ARMv7 CPUID registers) there are also processors which only require
>>> BTB invalidates when code is modified, but not when context-switching,
>>> so there may be performance considerations there...
>>
>> Attempting to summarize all that's been discussed.. It sounds like we
>> need the following implemented:
>>
>> 1. For cortex-a8 revisions affected by 458693, we can do a custom
>>     cpu_v7_switch_mm function that always does flush BTAC/BTB.
>>
>> 2. For HS cortex-a8 processors other than n900 affected by 458693,
>>     we need to implement functions similar to rx51_secure_update_aux_cr,
>>     the bootrom on n900 is different from TI HS omaps so the SMC call
>>     numbering may be different.
>>
>> 3. For later cortex-a8 processors not affected by 458693, we need
>>     to clear IBE bit to avoid erratum 687067.
>
> Oops sorry, wrong numbers for errata above.. s/458693/430973/, here's
> a better version:
>
> 1. For cortex-a8 revisions affected by 430973, we can do a custom
>     cpu_v7_switch_mm function that always does flush BTAC/BTB.
>

Why custom function, if IBE bit is zero, BTB invalidate instruction is a 
NOP. Do you think that "mcr p15, 0, r2, c7, c5, 6" executed as a NOP 
will put so much overhead, that it deserves a custom function?

> 2. For HS cortex-a8 processors other than n900 affected by 430973,
>     we need to implement functions similar to rx51_secure_update_aux_cr,
>     the bootrom on n900 is different from TI HS omaps so the SMC call
>     numbering may be different.
>
> 3. For later cortex-a8 processors not affected by 430973, we need
>     to clear IBE bit to avoid erratum 687067.
>

Maybe it should be implemented something like:

1. if Cortex-A8, always execute invalidate BTB instruction in
    cpu_v7_switch_mm

2. For Cortex-A8 revisions affected by 430973, set IBE bit to 1, set it
    to 0 for all others. That should happen as soon as possible,
    otherwise kernel may crash on affected revisions if thumb-
    compiled.

> Regards,
>
> Tony
>

Regards,

Ivo
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sebastian Reichel April 6, 2015, 5:38 p.m. UTC | #30

Hi,

On Sun, Apr 05, 2015 at 03:52:10PM +0200, Pali Rohár wrote:
> > So I suggest to enable the IBE bit in the kernel (that part is
> > already done) *and* in u-boot.
> > 
> > -- Sebastian
> 
> Yes. Code for both (U-Boot and linux kernel) exists and are 
> already included.
> 
> In linux kernel code is since 3.14-rc6 and in U-Boot code is 
> there since beginning of Nokia N900 support (2013.04) directly in 
> board code.

ah. I wasn't aware, that u-boot already sets the IBE bit on the N900.
Thanks for the information.

-- Sebastian

Tony Lindgren April 6, 2015, 5:42 p.m. UTC | #31

* Ivaylo Dimitrov <[e-mail]> [150406 10:15]:
> On  6.04.2015 18:40, Tony Lindgren wrote:
> >
> >Oops sorry, wrong numbers for errata above.. s/458693/430973/, here's
> >a better version:
> >
> >1. For cortex-a8 revisions affected by 430973, we can do a custom
> >    cpu_v7_switch_mm function that always does flush BTAC/BTB.
> >
> 
> Why custom function, if IBE bit is zero, BTB invalidate instruction is a
> NOP. Do you think that "mcr p15, 0, r2, c7, c5, 6" executed as a NOP will
> put so much overhead, that it deserves a custom function?

Hmm but it still seems to do something also on cortex-a8 r3p2 that
is supposedly not affected by 430973.. Based on my tests so far, at least
armhf running cpuburn-a8 in the background and doing apt-get update
segfaults constantly without flush BTAC/BTB. This seems to be the case
no matter how the aux ctrl reg bits are set.. This should be reproducable
on any pandboard xm BTW.
 
> >2. For HS cortex-a8 processors other than n900 affected by 430973,
> >    we need to implement functions similar to rx51_secure_update_aux_cr,
> >    the bootrom on n900 is different from TI HS omaps so the SMC call
> >    numbering may be different.
> >
> >3. For later cortex-a8 processors not affected by 430973, we need
> >    to clear IBE bit to avoid erratum 687067.
> >
> 
> Maybe it should be implemented something like:
> 
> 1. if Cortex-A8, always execute invalidate BTB instruction in
>    cpu_v7_switch_mm

This part still seems to need more investigating for why it's still
needed also r3p2 as I describe above. Otherwise we may be hiding some
other bug.
 
> 2. For Cortex-A8 revisions affected by 430973, set IBE bit to 1, set it
>    to 0 for all others. That should happen as soon as possible,
>    otherwise kernel may crash on affected revisions if thumb-
>    compiled.

Yes this makes sense.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matthijs van Duin April 6, 2015, 6:14 p.m. UTC | #32

* Ivaylo Dimitrov <[e-mail]> [150406 10:15]:
> Why custom function, if IBE bit is zero, BTB invalidate instruction is a
> NOP. Do you think that "mcr p15, 0, r2, c7, c5, 6" executed as a NOP will
> put so much overhead, that it deserves a custom function?

Executing them as nop is Cortex-A8 specific. On other ARMv7 CPUs, BTB
invalidation on context switch may be unnecessary yet expensive.

In general I think you'll want a version with and one without BTB
flushing, and select depending on CPUID (ID_MMFR1, bits 28-31), with
overrides for known processor issues (i.e. the cortex-a8 has a wrong
value there in all cases: it reports value 2, while it should be
treated as 1 or 4 depending on cpu revision).


On 6 April 2015 at 19:42, Tony Lindgren <[e-mail]> wrote:
> Hmm but it still seems to do something also on cortex-a8 r3p2 that
> is supposedly not affected by 430973.. Based on my tests so far, at least
> armhf running cpuburn-a8 in the background and doing apt-get update
> segfaults constantly without flush BTAC/BTB. This seems to be the case
> no matter how the aux ctrl reg bits are set..

That sounds.... really odd.  The TRM is fairly explicit about BTB
flush executing as nop when IBE is not set. Of course the TRM is not
exactly flawless, but still...


>> 2. For Cortex-A8 revisions affected by 430973, set IBE bit to 1, set it
>>    to 0 for all others.

Note btw that erratum 687067 affects *all* Cortex-A8 revisions, so
there's a bit of a catch-22 there. The proper workaround for it
(zeroing some particular debug register) can only be done in secure
privileged mode, and there's no straightforward way to check whether
this has been done.

However, it only affects BTB invalidate by MVA, afaik BTB invalidate
all is safe.

>>  That should happen as soon as possible,
>>  otherwise kernel may crash on affected revisions if thumb-compiled.

The right place to do this to be honest would be in the bootloader,
but I guess that's not always convenient to arrange...

Matthijs
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren April 7, 2015, 2:23 a.m. UTC | #33

* Matthijs van Duin <[e-mail]> [150406 11:15]:
> * Ivaylo Dimitrov <[e-mail]> [150406 10:15]:
> > Why custom function, if IBE bit is zero, BTB invalidate instruction is a
> > NOP. Do you think that "mcr p15, 0, r2, c7, c5, 6" executed as a NOP will
> > put so much overhead, that it deserves a custom function?
> 
> Executing them as nop is Cortex-A8 specific. On other ARMv7 CPUs, BTB
> invalidation on context switch may be unnecessary yet expensive.
> 
> In general I think you'll want a version with and one without BTB
> flushing, and select depending on CPUID (ID_MMFR1, bits 28-31), with
> overrides for known processor issues (i.e. the cortex-a8 has a wrong
> value there in all cases: it reports value 2, while it should be
> treated as 1 or 4 depending on cpu revision).
> 
> 
> On 6 April 2015 at 19:42, Tony Lindgren <[e-mail]> wrote:
> > Hmm but it still seems to do something also on cortex-a8 r3p2 that
> > is supposedly not affected by 430973.. Based on my tests so far, at least
> > armhf running cpuburn-a8 in the background and doing apt-get update
> > segfaults constantly without flush BTAC/BTB. This seems to be the case
> > no matter how the aux ctrl reg bits are set..
> 
> That sounds.... really odd.  The TRM is fairly explicit about BTB
> flush executing as nop when IBE is not set. Of course the TRM is not
> exactly flawless, but still...

Oops, sorry user error.. I was trying to clear IBE as a banked register
like L2 enable bit and of course it did not get cleared.. Clearing it
with a smc call really clears it. And in that case my test case seems to
work reliably on r3p2 without erratum 430973 enabled. 

Anyways, if the bootloader enables IBE bit, then the kernel must also
always enable the erratum 430973 flush BTAC/BTB parts on all cortex-a8.

Or else the kernel must clear the IBE bit based on the version, which
may be trickier with all the SoC and vendor specific smc calls.
 
> >> 2. For Cortex-A8 revisions affected by 430973, set IBE bit to 1, set it
> >>    to 0 for all others.
> 
> Note btw that erratum 687067 affects *all* Cortex-A8 revisions, so
> there's a bit of a catch-22 there. The proper workaround for it
> (zeroing some particular debug register) can only be done in secure
> privileged mode, and there's no straightforward way to check whether
> this has been done.
> 
> However, it only affects BTB invalidate by MVA, afaik BTB invalidate
> all is safe.

I'm now thinking the kernel should just always set the 430973 specific
cpu_v7_switch_mm for all cortex-a8 if IBE bit is set. That avoids
the whole mess of early SoC detection and smc calls. And if IBE bit
is not set, then we jsut use the rgular cpu_v7_switch_mm.

This will work as long as we can read the aux ctrl register IBE
bit using mrc, which I believe is the case for all cortex-a8 based
omap variants.
 
> >>  That should happen as soon as possible,
> >>  otherwise kernel may crash on affected revisions if thumb-compiled.
> 
> The right place to do this to be honest would be in the bootloader,
> but I guess that's not always convenient to arrange...

Yeah I agree.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sebastian Reichel April 7, 2015, 3:12 a.m. UTC | #34

Hi,

On Mon, Apr 06, 2015 at 07:23:13PM -0700, Tony Lindgren wrote:
> I'm now thinking the kernel should just always set the 430973 specific
> cpu_v7_switch_mm for all cortex-a8 if IBE bit is set. That avoids
> the whole mess of early SoC detection and smc calls. And if IBE bit
> is not set, then we just use the regular cpu_v7_switch_mm.
>
> This will work as long as we can read the aux ctrl register IBE
> bit using mrc, which I believe is the case for all cortex-a8 based
> omap variants.

If I understood it correctly we can simply call the BTB flush on
cortex-a8 if IBE bit is not set, since it would be translated as
nop.

So it should be safe to include the call on all cortex-a8 based
cpus. We may need a non-BTB-flushing function for non-cortex-a8
based cpus, though.

-- Sebastian

Matthijs van Duin April 7, 2015, 3:49 a.m. UTC | #35

On 7 April 2015 at 04:23, Tony Lindgren <[e-mail]> wrote:
> Oops, sorry user error.. I was trying to clear IBE as a banked register
> like L2 enable bit and of course it did not get cleared.. Clearing it
> with a smc call really clears it. And in that case my test case seems to
> work reliably on r3p2 without erratum 430973 enabled.

So if I understand correctly, you actually had crashes which only
occurred with IBE enabled and the 430973 workaround disabled?  That's
quite interesting, since it seems to me that can only be the result of
erratum 687067, which means
1. secrom indeed fails to implement the 687067 workaround.
2. "BTB invalidate by MVA" is used somewhere in the kernel.
The 430973 workaround would likely conceal this problem due to
regularly flushing the whole BTB, but I'm not sure how wise it is to
rely on that...


> I'm now thinking the kernel should just always set the 430973 specific
> cpu_v7_switch_mm for all cortex-a8 if IBE bit is set.

And simply take the performance hit if secrom bogusly sets it and the
bootloader doesn't fix it?

Sounds reasonable enough to me, given how platform-specific the
appropriate auxcr config is, as well as the means by which it can be
changed.

There's more secrom misconfiguration that the bootloader should fix
anyhow to make optimal use of the processor...


> This will work as long as we can read the aux ctrl register IBE
> bit using mrc, which I believe is the case for all cortex-a8 based
> omap variants.

Aux control is always readable, only write is an issue.

On 7 April 2015 at 05:12, Sebastian Reichel <[e-mail]> wrote:
> If I understood it correctly we can simply call the BTB flush on
> cortex-a8 if IBE bit is not set, since it would be translated as
> nop.

Indeed you can safely use the BTB-flushing context switch on any cortex-a8.

There's still value in checking if IBE is set on r2p1 or later, and if
so emit a warning about suboptimal performance.

> So it should be safe to include the call on all cortex-a8 based
> cpus. We may need a non-BTB-flushing function for non-cortex-a8
> based cpus, though.

I just looked it up, apparently BTB-flushing on context switch is not
needed architecturally in ARMv7 (though it was in ARMv6), so that
version should probably indeed only be used for the cortex-a8.

Matthijs
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Russell King - ARM Linux April 7, 2015, 1:58 p.m. UTC | #36

On Mon, Apr 06, 2015 at 08:14:30PM +0200, Matthijs van Duin wrote:
> * Ivaylo Dimitrov <[e-mail]> [150406 10:15]:
> > Why custom function, if IBE bit is zero, BTB invalidate instruction is a
> > NOP. Do you think that "mcr p15, 0, r2, c7, c5, 6" executed as a NOP will
> > put so much overhead, that it deserves a custom function?
> 
> Executing them as nop is Cortex-A8 specific. On other ARMv7 CPUs, BTB
> invalidation on context switch may be unnecessary yet expensive.

That can be solved (see the patch I just posted) so I wouldn't worry
too much about that issue.

Tony Lindgren April 7, 2015, 2:48 p.m. UTC | #37

* Matthijs van Duin <[e-mail]> [150406 20:50]:
> On 7 April 2015 at 04:23, Tony Lindgren <[e-mail]> wrote:
> > Oops, sorry user error.. I was trying to clear IBE as a banked register
> > like L2 enable bit and of course it did not get cleared.. Clearing it
> > with a smc call really clears it. And in that case my test case seems to
> > work reliably on r3p2 without erratum 430973 enabled.
> 
> So if I understand correctly, you actually had crashes which only
> occurred with IBE enabled and the 430973 workaround disabled?

That's right. It seems to happen at least with r3p2 that has 430973
fixed. 

> That's quite interesting, since it seems to me that can only be the
> result of erratum 687067, which means
> 1. secrom indeed fails to implement the 687067 workaround.
> 2. "BTB invalidate by MVA" is used somewhere in the kernel.
> The 430973 workaround would likely conceal this problem due to
> regularly flushing the whole BTB, but I'm not sure how wise it is to
> rely on that...

Yes it seems to be hidden behind 430973. Anyways, we can print some
warnings in the kernel for incorrect revision and IBE handling.
 
> > I'm now thinking the kernel should just always set the 430973 specific
> > cpu_v7_switch_mm for all cortex-a8 if IBE bit is set.
> 
> And simply take the performance hit if secrom bogusly sets it and the
> bootloader doesn't fix it?

Yes it seems Russell's patch should do that for cortex-a8.
 
> Sounds reasonable enough to me, given how platform-specific the
> appropriate auxcr config is, as well as the means by which it can be
> changed.

Right, we have quite a few combinations already for omap3.. 34xx/36xx,
HS/GP, TI vs Nokia bootrom.. Just proves how useless all these
"security" "features" are in the long run :) They will just keep
biting people over and over in the long run even if not used.
 
> There's more secrom misconfiguration that the bootloader should fix
> anyhow to make optimal use of the processor...

Yeah. 
 
> > This will work as long as we can read the aux ctrl register IBE
> > bit using mrc, which I believe is the case for all cortex-a8 based
> > omap variants.
> 
> Aux control is always readable, only write is an issue.

OK, hopefully that's the case for 36xx HS version too.. I recall some
registers reading as zero on N9 but hopefully not for the aux control
register.
 
> On 7 April 2015 at 05:12, Sebastian Reichel <[e-mail]> wrote:
> > If I understood it correctly we can simply call the BTB flush on
> > cortex-a8 if IBE bit is not set, since it would be translated as
> > nop.
> 
> Indeed you can safely use the BTB-flushing context switch on any cortex-a8.
> 
> There's still value in checking if IBE is set on r2p1 or later, and if
> so emit a warning about suboptimal performance.
> 
> > So it should be safe to include the call on all cortex-a8 based
> > cpus. We may need a non-BTB-flushing function for non-cortex-a8
> > based cpus, though.
> 
> I just looked it up, apparently BTB-flushing on context switch is not
> needed architecturally in ARMv7 (though it was in ARMv6), so that
> version should probably indeed only be used for the cortex-a8.

OK

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grazvydas Ignotas April 9, 2015, 10:37 p.m. UTC | #38

On Tue, Apr 7, 2015 at 5:23 AM, Tony Lindgren <[e-mail]> wrote:
> * Matthijs van Duin <[e-mail]> [150406 11:15]:
>>
>> On 6 April 2015 at 19:42, Tony Lindgren <[e-mail]> wrote:
>> > Hmm but it still seems to do something also on cortex-a8 r3p2 that
>> > is supposedly not affected by 430973.. Based on my tests so far, at least
>> > armhf running cpuburn-a8 in the background and doing apt-get update
>> > segfaults constantly without flush BTAC/BTB. This seems to be the case
>> > no matter how the aux ctrl reg bits are set..
>>
>> That sounds.... really odd.  The TRM is fairly explicit about BTB
>> flush executing as nop when IBE is not set. Of course the TRM is not
>> exactly flawless, but still...
>
> Oops, sorry user error.. I was trying to clear IBE as a banked register
> like L2 enable bit and of course it did not get cleared.. Clearing it
> with a smc call really clears it. And in that case my test case seems to
> work reliably on r3p2 without erratum 430973 enabled.

May I ask how do you perform the smc call? I wanted to clear IBE too
to experiment, but it just hangs my board, even if I just write back
the same value. Here is what I do:

        asm ("mrc p15, 0, %0, c1, c0, 1" : "=r"(val));

        asm (".arch_extension sec\n\t"
             "mov r0,  %0\n"
             "mov r12, #3\n"
             "smc #0\n"
             :: "r"(val) : "r0", "r12");

I just run this from a sysfs write handler, does it need to be run on
SRAM or something?

Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matthijs van Duin April 16, 2015, 4:53 p.m. UTC | #39

On 10 April 2015 at 00:37, Grazvydas Ignotas <[e-mail]> wrote:
> May I ask how do you perform the smc call?

A point worth mentioning is that TI advises that r1-r4 may be
clobbered in general, and at least on GP dm814x and am335x devices r4
is in fact clobbered, even though it is normally a callee-save
register.

Also, on aforementioned devices, the secure-world MMU is enabled, with
translation table 0 being 1KB @ 0x402f2000, used for secure VA
0x00000000 - 0x0fffffff (the rest goes via translation table 1 in
secure ROM). It is generally not consulted in practice since secrom
locks two dTLB and two iTLB entries. The GP secure monitor doesn't
actually perform any data access whatsoever, so the two dTLB can and
should be unlocked considering that TLB entries are a rather scarce
resource (32 per side) and a dTLB miss incurs a 24-cycle minimum
penalty (compare with 8-cycle minimum penalty for L1 cache miss).

Unlocking the second iTLB entry is also safe, but if both iTLB entries
are unlocked, you need to preserve or repopulate the relevant
translation entry (VA 0x00300000 -> PA 0x00000000) to be able to
perform an SMC. Everything that happens in secure monitor mode is
heavily double-checked by the SSM, so while you're free to choose a
cache policy, any creativity beyond that will piss off the SSM and
cause it to hit the "MPU security violation" global reset.

Once appropriate fixes to the auxiliary control register have been
applied, generally no further SMCs are needed hence all TLB entries
can be unlocked. Since it would be reasonable for a bootloader to do
this, I recommend checking whether a fix is needed and not
unconditionally performing an SMC.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren May 4, 2015, 4:07 p.m. UTC | #40

* Sebastian Reichel <[e-mail]> [150331 05:33]:
> Hi,
> 
> On Mon, Mar 30, 2015 at 10:50:52AM -0700, Tony Lindgren wrote:
> > * Jarkko Nikula <[e-mail]> [150330 10:46]:
> > > Well, there has been regression but finding exactly how far should the
> > > fix go didn't look instantly straightforward due all DT, codec driver
> > > mic bias etc changes and I ended up not cc'ing stable.
> > > 
> > > But well, I guess first kernel where this commit makes sense is 3.16+
> > > due commit f7d0f2a08567 ("ARM: dts: omap3-n900: Add sound support").
> > > Although it applies on top of commit 14e3e295b2b9 ("ARM: dts:
> > > omap3-n900: Add TLV320AIC3X support") too (3.12+) but not before that.
> > 
> > OK I think debian is using v3.16 kernel
> 
> Yes. It will be used for Debian jessie (not yet released) and the
> N900 related drivers are enabled in the armmp flavour. Unfortunately
> it does not work together with thumb using userland because the
> errata 430973 workaround is not enabled.

Oops I just noticed the original $subject fix is still pending
as we got distracted with all this 430973 stuff. Applying Jarkko's
fix into omap-for-v4.1/fixes with Cc stable v3.16+.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index db80f9d376fa..9c8bdf2c93a1 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -484,6 +484,8 @@ 
 		DRVDD-supply = <&vmmc2>;
 		IOVDD-supply = <&vio>;
 		DVDD-supply = <&vio>;
+
+		ai3x-micbias-vg = <1>;
 	};
 
 	tlv320aic3x_aux: tlv320aic3x@19 {
@@ -495,6 +497,8 @@ 
 		DRVDD-supply = <&vmmc2>;
 		IOVDD-supply = <&vio>;
 		DVDD-supply = <&vio>;
+
+		ai3x-micbias-vg = <2>;
 	};
 
 	tsl2563: tsl2563@29 {