* [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
2016-06-30 8:51 [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency Gong Qianyu
@ 2016-06-30 9:39 ` Masahiro Yamada
2016-06-30 17:02 ` york sun
2016-06-30 17:45 ` Mark Rutland
2 siblings, 0 replies; 9+ messages in thread
From: Masahiro Yamada @ 2016-06-30 9:39 UTC (permalink / raw)
To: u-boot
Hi.
2016-06-30 17:51 GMT+09:00 Gong Qianyu <Qianyu.Gong@nxp.com>:
> From: Mingkai Hu <mingkai.hu@nxp.com>
>
> Data coherency is enabled only when the CPUECTLR.SMPEN bit is
> set. The SMPEN bit should be set before enabling the data cache.
> If not enabled, the cache is not coherent with other cores and
> data corruption could occur.
>
> Signed-off-by: Mingkai Hu <mingkai.hu@nxp.com>
> Signed-off-by: Gong Qianyu <Qianyu.Gong@nxp.com>
>
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index 670e323..735dd67 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -81,6 +81,11 @@ reset:
> msr cpacr_el1, x0 /* Enable FP/SIMD */
> 0:
>
> + /* Enalbe SMPEN bit */
> + mrs x0, S3_1_c15_c2_1 /* cpuactlr_el1 */
> + orr x0, x0, #0x40
> + msr S3_1_c15_c2_1, x0
> +
> /* Apply ARM core specific erratas */
> bl apply_core_errata
>
I guess this code is necessary in U-Boot
if we want to boot the system without ARM Trusted Firmware.
I can see equivalent code only in
arch/arm/mach-uniphier/arm64/smp.S (my SoC)
So, I guess all of the other SoCs use ATF
and setup this register there.
One more thing, I could find the description about this register
only in each Cortex-A* TRM, but not in v8 ARM ARM.
However, I assume this register exists in all of ARMv8 variants.
(but I am not 100% sure because I am not an expert in this area.)
Otherwise, this patch looks good to me
(and if it is accepted, I can remove equivalent code from my local file)
Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
2016-06-30 8:51 [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency Gong Qianyu
2016-06-30 9:39 ` Masahiro Yamada
@ 2016-06-30 17:02 ` york sun
2016-06-30 23:54 ` Prabhakar Kushwaha
2016-06-30 17:45 ` Mark Rutland
2 siblings, 1 reply; 9+ messages in thread
From: york sun @ 2016-06-30 17:02 UTC (permalink / raw)
To: u-boot
On 06/30/2016 02:03 AM, Gong Qianyu wrote:
> From: Mingkai Hu <mingkai.hu@nxp.com>
>
> Data coherency is enabled only when the CPUECTLR.SMPEN bit is
> set. The SMPEN bit should be set before enabling the data cache.
> If not enabled, the cache is not coherent with other cores and
> data corruption could occur.
>
> Signed-off-by: Mingkai Hu <mingkai.hu@nxp.com>
> Signed-off-by: Gong Qianyu <Qianyu.Gong@nxp.com>
>
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index 670e323..735dd67 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -81,6 +81,11 @@ reset:
> msr cpacr_el1, x0 /* Enable FP/SIMD */
> 0:
>
> + /* Enalbe SMPEN bit */
> + mrs x0, S3_1_c15_c2_1 /* cpuactlr_el1 */
> + orr x0, x0, #0x40
> + msr S3_1_c15_c2_1, x0
> +
> /* Apply ARM core specific erratas */
> bl apply_core_errata
>
>
Qianyu,
I wonder what impact this patch has. Did you find it effective on A53
core? According to ARM documents, A57 and A72 seem don't care this bit.
Quote
"
Note
Any processor instruction cache and TLB maintenance operations can
execute the request, regardless of the value of the SMPEN bit.
This bit has no impact on data cache maintenance operations.
In the Cortex-A57 processor, the L1 data cache and L2 cache are
always coherent, for shared or non-shared data, regardless of the value
of the SMPEN bit.
"
York
^ permalink raw reply [flat|nested] 9+ messages in thread* [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
2016-06-30 17:02 ` york sun
@ 2016-06-30 23:54 ` Prabhakar Kushwaha
2016-07-01 8:33 ` Mingkai Hu
2016-07-01 21:44 ` Edward L Swarthout
0 siblings, 2 replies; 9+ messages in thread
From: Prabhakar Kushwaha @ 2016-06-30 23:54 UTC (permalink / raw)
To: u-boot
Hi York,
> -----Original Message-----
> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of york sun
> Sent: Thursday, June 30, 2016 10:32 PM
> To: Qianyu Gong <qianyu.gong@nxp.com>; albert.u.boot at aribaud.net; u-
> boot at lists.denx.de; s.temerkhanov at gmail.com;
> yamada.masahiro at socionext.com
> Cc: Mingkai Hu <mingkai.hu@nxp.com>
> Subject: Re: [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data
> coherency
>
> On 06/30/2016 02:03 AM, Gong Qianyu wrote:
> > From: Mingkai Hu <mingkai.hu@nxp.com>
> >
> > Data coherency is enabled only when the CPUECTLR.SMPEN bit is set. The
> > SMPEN bit should be set before enabling the data cache.
> > If not enabled, the cache is not coherent with other cores and data
> > corruption could occur.
> >
> > Signed-off-by: Mingkai Hu <mingkai.hu@nxp.com>
> > Signed-off-by: Gong Qianyu <Qianyu.Gong@nxp.com>
> >
> > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> > index 670e323..735dd67 100644
> > --- a/arch/arm/cpu/armv8/start.S
> > +++ b/arch/arm/cpu/armv8/start.S
> > @@ -81,6 +81,11 @@ reset:
> > msr cpacr_el1, x0 /* Enable FP/SIMD */
> > 0:
> >
> > + /* Enalbe SMPEN bit */
> > + mrs x0, S3_1_c15_c2_1 /* cpuactlr_el1 */
> > + orr x0, x0, #0x40
> > + msr S3_1_c15_c2_1, x0
> > +
> > /* Apply ARM core specific erratas */
> > bl apply_core_errata
> >
> >
>
> Qianyu,
>
> I wonder what impact this patch has. Did you find it effective on A53 core?
> According to ARM documents, A57 and A72 seem don't care this bit.
> Quote
>
I have seen big difference on LS1012A with A53 cores after enabling this bit.
If I don't enable this bit many IPs like SATA, SDHC show coherency issue.
--prabhakar
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
2016-06-30 23:54 ` Prabhakar Kushwaha
@ 2016-07-01 8:33 ` Mingkai Hu
2016-07-01 21:44 ` Edward L Swarthout
1 sibling, 0 replies; 9+ messages in thread
From: Mingkai Hu @ 2016-07-01 8:33 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Prabhakar Kushwaha
> Sent: Friday, July 01, 2016 7:55 AM
> To: york sun; Qianyu Gong; albert.u.boot at aribaud.net; u-boot at lists.denx.de;
> s.temerkhanov at gmail.com; yamada.masahiro at socionext.com
> Cc: Mingkai Hu
> Subject: RE: [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
>
> Hi York,
>
>
> > -----Original Message-----
> > From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of york
> > sun
> > Sent: Thursday, June 30, 2016 10:32 PM
> > To: Qianyu Gong <qianyu.gong@nxp.com>; albert.u.boot at aribaud.net; u-
> > boot at lists.denx.de; s.temerkhanov at gmail.com;
> > yamada.masahiro at socionext.com
> > Cc: Mingkai Hu <mingkai.hu@nxp.com>
> > Subject: Re: [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data
> > coherency
> >
> > On 06/30/2016 02:03 AM, Gong Qianyu wrote:
> > > From: Mingkai Hu <mingkai.hu@nxp.com>
> > >
> > > Data coherency is enabled only when the CPUECTLR.SMPEN bit is set.
> > > The SMPEN bit should be set before enabling the data cache.
> > > If not enabled, the cache is not coherent with other cores and data
> > > corruption could occur.
> > >
> > > Signed-off-by: Mingkai Hu <mingkai.hu@nxp.com>
> > > Signed-off-by: Gong Qianyu <Qianyu.Gong@nxp.com>
> > >
> > > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> > > index 670e323..735dd67 100644
> > > --- a/arch/arm/cpu/armv8/start.S
> > > +++ b/arch/arm/cpu/armv8/start.S
> > > @@ -81,6 +81,11 @@ reset:
> > > msr cpacr_el1, x0 /* Enable FP/SIMD */
> > > 0:
> > >
> > > + /* Enalbe SMPEN bit */
> > > + mrs x0, S3_1_c15_c2_1 /* cpuactlr_el1 */
> > > + orr x0, x0, #0x40
> > > + msr S3_1_c15_c2_1, x0
> > > +
> > > /* Apply ARM core specific erratas */
> > > bl apply_core_errata
> > >
> > >
> >
> > Qianyu,
> >
> > I wonder what impact this patch has. Did you find it effective on A53 core?
> > According to ARM documents, A57 and A72 seem don't care this bit.
> > Quote
> >
>
> I have seen big difference on LS1012A with A53 cores after enabling this bit.
> If I don't enable this bit many IPs like SATA, SDHC show coherency issue.
>
Hi York,
This bit is used to enable hardware management of data coherency with other cores in the cluster
for A53.
"Data coherency is enabled only when the CPUECTLR.SMPEN bit is set. You must set the
SMPEN bit before enabling the data cache. If you do not, then the cache is not coherent with
other cores and data corruption could occur.
"
For A57/A72, this bit enables the processor to receive instruction cache and TLB maintenance
operations broadcast from other processors in the cluster.
We will change the commit message and integrate Mark's comments to guard this setting for
relevant CPUs.
Thanks,
Mingkai
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
2016-06-30 23:54 ` Prabhakar Kushwaha
2016-07-01 8:33 ` Mingkai Hu
@ 2016-07-01 21:44 ` Edward L Swarthout
2016-07-04 2:08 ` Mingkai Hu
2016-07-05 5:06 ` Prabhakar Kushwaha
1 sibling, 2 replies; 9+ messages in thread
From: Edward L Swarthout @ 2016-07-01 21:44 UTC (permalink / raw)
To: u-boot
From: Prabhakar Kushwaha
>> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of york
>> On 06/30/2016 02:03 AM, Gong Qianyu wrote:
>> > From: Mingkai Hu <mingkai.hu@nxp.com>
>> >
>> > Data coherency is enabled only when the CPUECTLR.SMPEN bit is set.
>> > The SMPEN bit should be set before enabling the data cache.
>> > If not enabled, the cache is not coherent with other cores and data
>> > corruption could occur.
>> >
>> > + /* Enalbe SMPEN bit */
>> > + mrs x0, S3_1_c15_c2_1 /* cpuactlr_el1 */
>> > + orr x0, x0, #0x40
>> > + msr S3_1_c15_c2_1, x0
>> > +
>>
>> I wonder what impact this patch has. Did you find it effective on A53 core?
>> According to ARM documents, A57 and A72 seem don't care this bit.
>
>I have seen big difference on LS1012A with A53 cores after enabling this bit.
>If I don't enable this bit many IPs like SATA, SDHC show coherency issue.
But LS1012A only has a single A53 core.
The multicore part, LS1043A, sets this bit in the bootrom:
34: d539f221 mrs x1, s3_1_c15_c2_1
38: b27a0021 orr x1, x1, #0x40
3c: d519f221 msr s3_1_c15_c2_1, x1
Ed
^ permalink raw reply [flat|nested] 9+ messages in thread* [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
2016-07-01 21:44 ` Edward L Swarthout
@ 2016-07-04 2:08 ` Mingkai Hu
2016-07-05 5:06 ` Prabhakar Kushwaha
1 sibling, 0 replies; 9+ messages in thread
From: Mingkai Hu @ 2016-07-04 2:08 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Edward L Swarthout
> Sent: Saturday, July 02, 2016 5:44 AM
> To: Prabhakar Kushwaha; york sun; Qianyu Gong; albert.u.boot at aribaud.net;
> u-boot at lists.denx.de; s.temerkhanov at gmail.com;
> yamada.masahiro at socionext.com
> Cc: Mingkai Hu
> Subject: RE: [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data
> coherency
>
> From: Prabhakar Kushwaha
>
> >> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of york
> >> On 06/30/2016 02:03 AM, Gong Qianyu wrote:
> >> > From: Mingkai Hu <mingkai.hu@nxp.com>
> >> >
> >> > Data coherency is enabled only when the CPUECTLR.SMPEN bit is set.
> >> > The SMPEN bit should be set before enabling the data cache.
> >> > If not enabled, the cache is not coherent with other cores and data
> >> > corruption could occur.
> >> >
> >> > + /* Enalbe SMPEN bit */
> >> > + mrs x0, S3_1_c15_c2_1 /* cpuactlr_el1 */
> >> > + orr x0, x0, #0x40
> >> > + msr S3_1_c15_c2_1, x0
> >> > +
> >>
> >> I wonder what impact this patch has. Did you find it effective on A53
> core?
> >> According to ARM documents, A57 and A72 seem don't care this bit.
> >
> >I have seen big difference on LS1012A with A53 cores after enabling this
> bit.
> >If I don't enable this bit many IPs like SATA, SDHC show coherency issue.
>
> But LS1012A only has a single A53 core.
> The multicore part, LS1043A, sets this bit in the bootrom:
>
> 34: d539f221 mrs x1, s3_1_c15_c2_1
> 38: b27a0021 orr x1, x1, #0x40
> 3c: d519f221 msr s3_1_c15_c2_1, x1
>
This bit needs to be set even only one core.
And bootrom did not set this bit, so the coherency issue was caused.
Thanks,
Mingkai
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
2016-07-01 21:44 ` Edward L Swarthout
2016-07-04 2:08 ` Mingkai Hu
@ 2016-07-05 5:06 ` Prabhakar Kushwaha
1 sibling, 0 replies; 9+ messages in thread
From: Prabhakar Kushwaha @ 2016-07-05 5:06 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Edward L Swarthout
> Sent: Saturday, July 02, 2016 3:14 AM
> To: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>; york sun
> <york.sun@nxp.com>; Qianyu Gong <qianyu.gong@nxp.com>;
> albert.u.boot at aribaud.net; u-boot at lists.denx.de;
> s.temerkhanov at gmail.com; yamada.masahiro at socionext.com
> Cc: Mingkai Hu <mingkai.hu@nxp.com>
> Subject: RE: [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data
> coherency
>
> From: Prabhakar Kushwaha
>
> >> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of york
> >> On 06/30/2016 02:03 AM, Gong Qianyu wrote:
> >> > From: Mingkai Hu <mingkai.hu@nxp.com>
> >> >
> >> > Data coherency is enabled only when the CPUECTLR.SMPEN bit is set.
> >> > The SMPEN bit should be set before enabling the data cache.
> >> > If not enabled, the cache is not coherent with other cores and data
> >> > corruption could occur.
> >> >
> >> > + /* Enalbe SMPEN bit */
> >> > + mrs x0, S3_1_c15_c2_1 /* cpuactlr_el1 */
> >> > + orr x0, x0, #0x40
> >> > + msr S3_1_c15_c2_1, x0
> >> > +
> >>
> >> I wonder what impact this patch has. Did you find it effective on A53 core?
> >> According to ARM documents, A57 and A72 seem don't care this bit.
> >
> >I have seen big difference on LS1012A with A53 cores after enabling this bit.
> >If I don't enable this bit many IPs like SATA, SDHC show coherency issue.
>
> But LS1012A only has a single A53 core.
> The multicore part, LS1043A, sets this bit in the bootrom:
>
> 34: d539f221 mrs x1, s3_1_c15_c2_1
> 38: b27a0021 orr x1, x1, #0x40
> 3c: d519f221 msr s3_1_c15_c2_1, x1
>
Bootrom team was under same impression like this bit not required for LS1012A. Hence they missed it.
But as per Table 4-125 present in A53 TRM r0p4. This bit is required for hardware coherency.
" Set the SMPEN bit before enabling the caches, even if there is only one core in the system."
--prabhakar
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency
2016-06-30 8:51 [U-Boot] [PATCH] armv8: Enable CPUECTLR.SMPEN for data coherency Gong Qianyu
2016-06-30 9:39 ` Masahiro Yamada
2016-06-30 17:02 ` york sun
@ 2016-06-30 17:45 ` Mark Rutland
2 siblings, 0 replies; 9+ messages in thread
From: Mark Rutland @ 2016-06-30 17:45 UTC (permalink / raw)
To: u-boot
On Thu, Jun 30, 2016 at 04:51:48PM +0800, Gong Qianyu wrote:
> From: Mingkai Hu <mingkai.hu@nxp.com>
>
> Data coherency is enabled only when the CPUECTLR.SMPEN bit is
> set. The SMPEN bit should be set before enabling the data cache.
> If not enabled, the cache is not coherent with other cores and
> data corruption could occur.
>
> Signed-off-by: Mingkai Hu <mingkai.hu@nxp.com>
> Signed-off-by: Gong Qianyu <Qianyu.Gong@nxp.com>
>
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index 670e323..735dd67 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -81,6 +81,11 @@ reset:
> msr cpacr_el1, x0 /* Enable FP/SIMD */
> 0:
>
> + /* Enalbe SMPEN bit */
> + mrs x0, S3_1_c15_c2_1 /* cpuactlr_el1 */
> + orr x0, x0, #0x40
> + msr S3_1_c15_c2_1, x0
Please note that this register is IMPLEMENTATION DEFINED, and not
architectural, even though it happens to be common among ARM Ltd
implementations.
This is also not something that one can usually set on the Non-secure
side, and I'd expect Secure FW such as the ARM Trusted Firmware to
handle this.
If this is necessary within U-Boot, it should be guarded such that it
only runs on the relevant CPUs.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 9+ messages in thread