2016-12-22 14:09:56

by Alexey Brodkin

[permalink] [raw]
Subject: [PATCH 0/2] Minor fixes for CCMs

It turned out current implementation of CCM support doesn't work at all.
There're 2 isseus:
* Data/code which is supposed to be in DCCM or ICCM accordingly gets
merged in common .data and .text sections so CCMs won't be used
* Kerenl will panic on early boot because comparison of CCM sizes
is implemented incorrectly

This short series fixes both issues above so CCMs are usable again.
And both patches indeed should be back-ported to stable kernels.

Alexey Brodkin (2):
arc: rename xCCM sections so they are not merged in global .data/.text
arc: Fix xCCM size check

arch/arc/include/asm/linkage.h | 6 +++---
arch/arc/kernel/setup.c | 4 ++--
arch/arc/kernel/vmlinux.lds.S | 8 ++++----
3 files changed, 9 insertions(+), 9 deletions(-)

--
2.7.4


2016-12-22 14:09:57

by Alexey Brodkin

[permalink] [raw]
Subject: [PATCH 1/2] arc: rename xCCM sections so they are not merged in global .data/.text

If Linux kernel is compiled with "-ffunction-sections" each function is placed in
its own section named ".text.function_name". This is required for
discarding of not-used functions during final linkage. But in the end
all ".text.XXX" sections are merged in the global ".text" section of
vmlinux Elf.

The same happens with data sections when "-fdata-sections" are used.

That means our ".data.arcfp" and ".text.arcfp" sections get silently
merged in global ".data" and ".text" sections even though we want to
put them in separate memory regions in case ICCM and/or DCCM exist in
the ARC core.

Solution is as simple as addition of one extra period in section name.

Signed-off-by: Alexey Brodkin <[email protected]>
Cc: Igor Guryanov <[email protected]>
Cc: [email protected]
---
arch/arc/include/asm/linkage.h | 6 +++---
arch/arc/kernel/vmlinux.lds.S | 8 ++++----
2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arc/include/asm/linkage.h b/arch/arc/include/asm/linkage.h
index b29f1a9fd6f7..3a5f13d65ee1 100644
--- a/arch/arc/include/asm/linkage.h
+++ b/arch/arc/include/asm/linkage.h
@@ -28,7 +28,7 @@
/* annotation for data we want in DCCM - if enabled in .config */
.macro ARCFP_CODE
#ifdef CONFIG_ARC_HAS_ICCM
- .section .text.arcfp, "ax",@progbits
+ .section .text..arcfp, "ax",@progbits
#else
.section .text, "ax",@progbits
#endif
@@ -47,13 +47,13 @@
#else /* !__ASSEMBLY__ */

#ifdef CONFIG_ARC_HAS_ICCM
-#define __arcfp_code __attribute__((__section__(".text.arcfp")))
+#define __arcfp_code __attribute__((__section__(".text..arcfp")))
#else
#define __arcfp_code __attribute__((__section__(".text")))
#endif

#ifdef CONFIG_ARC_HAS_DCCM
-#define __arcfp_data __attribute__((__section__(".data.arcfp")))
+#define __arcfp_data __attribute__((__section__(".data..arcfp")))
#else
#define __arcfp_data __attribute__((__section__(".data")))
#endif
diff --git a/arch/arc/kernel/vmlinux.lds.S b/arch/arc/kernel/vmlinux.lds.S
index f35ed578e007..f69ae479ee73 100644
--- a/arch/arc/kernel/vmlinux.lds.S
+++ b/arch/arc/kernel/vmlinux.lds.S
@@ -37,8 +37,8 @@ SECTIONS
}

#ifdef CONFIG_ARC_HAS_ICCM
- .text.arcfp : {
- *(.text.arcfp)
+ .text..arcfp : {
+ *(.text..arcfp)
. = ALIGN(CONFIG_ARC_ICCM_SZ * 1024);
}
#endif
@@ -151,8 +151,8 @@ SECTIONS
#ifdef CONFIG_ARC_HAS_DCCM
. = CONFIG_ARC_DCCM_BASE;
__arc_dccm_base = .;
- .data.arcfp : {
- *(.data.arcfp)
+ .data..arcfp : {
+ *(.data..arcfp)
}
. = ALIGN(CONFIG_ARC_DCCM_SZ * 1024);
#endif
--
2.7.4

2016-12-22 14:10:24

by Alexey Brodkin

[permalink] [raw]
Subject: [PATCH 2/2] arc: Fix xCCM size check

CONFIG_ARC_ICCM_SZ in menuconfig is specified in kB while
"cpu->Xccm.sz" contains value in bytes thus direct comparison fails
leading to boot-time panic like that:
----------------------->8---------------------
IDENTITY : ARCVER [0x52] ARCNUM [0x1] CHIPID [ 0x0]
processor [1] : ARC HS38 R2.1 (ARCv2 ISA)
Timers : Timer0 Timer1 Local-64-bit-Ctr (not used)
ISA Extn : atomic ll64 unalign (not used)
: mpy[opt 9] div_rem norm barrel-shift swap minmax swape
BPU : full match, cache:2048, Predict Table:16384
MMU [v0] : 0k PAGE, JTLB 0 (0x0), uDTLB 0, uITLB 0
I-Cache : N/A
D-Cache : N/A
Peripherals : 0xf0000000, IO-Coherency (disabled)
Vector Table : 0x80000000
FPU : SP DP
DEBUG : ActionPoint smaRT RTT
Extn [CCM] : DCCM @ e0000000, 256 KB / ICCM: @ 60000000, 256 KB
OS ABI [v4] : 64-bit data any register aligned
Extn [SMP] : ARConnect (v2): 2 cores with IPI IDU DEBUG GFRC
Kernel panic - not syncing: Linux built with incorrect DCCM Size

---[ end Kernel panic - not syncing: Linux built with incorrect DCCM Size
----------------------->8---------------------

Signed-off-by: Alexey Brodkin <[email protected]>
Cc: Igor Guryanov <[email protected]>
Cc: [email protected]
---
arch/arc/kernel/setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index ee574f37f365..601dab6fe5e0 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -333,12 +333,12 @@ static void arc_chk_core_config(void)
if ((unsigned int)__arc_dccm_base != cpu->dccm.base_addr)
panic("Linux built with incorrect DCCM Base address\n");

- if (CONFIG_ARC_DCCM_SZ != cpu->dccm.sz)
+ if (CONFIG_ARC_DCCM_SZ != TO_KB(cpu->dccm.sz))
panic("Linux built with incorrect DCCM Size\n");
#endif

#ifdef CONFIG_ARC_HAS_ICCM
- if (CONFIG_ARC_ICCM_SZ != cpu->iccm.sz)
+ if (CONFIG_ARC_ICCM_SZ != TO_KB(cpu->iccm.sz))
panic("Linux built with incorrect ICCM Size\n");
#endif

--
2.7.4

2016-12-23 00:25:23

by Vineet Gupta

[permalink] [raw]
Subject: Re: [PATCH 1/2] arc: rename xCCM sections so they are not merged in global .data/.text

On 12/22/2016 06:09 AM, Alexey Brodkin wrote:
> If Linux kernel is compiled with "-ffunction-sections" each function is placed in
> its own section named ".text.function_name". This is required for
> discarding of not-used functions during final linkage. But in the end
> all ".text.XXX" sections are merged in the global ".text" section of
> vmlinux Elf.
>
> The same happens with data sections when "-fdata-sections" are used.
>
> That means our ".data.arcfp" and ".text.arcfp" sections get silently
> merged in global ".data" and ".text" sections even though we want to
> put them in separate memory regions in case ICCM and/or DCCM exist in
> the ARC core.
>
> Solution is as simple as addition of one extra period in section name.
>
> Signed-off-by: Alexey Brodkin <[email protected]>
> Cc: Igor Guryanov <[email protected]>
> Cc: [email protected]

For stable backport -I think there is no point in CC'ing them initially.
Better to make a note in the patch and conclude at the time of patch review and I
can in the end add the CC tag so this gets pulled automatically w/o CC'ing the
stable mailing list at all !

> ---
> arch/arc/include/asm/linkage.h | 6 +++---
> arch/arc/kernel/vmlinux.lds.S | 8 ++++----
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arc/include/asm/linkage.h b/arch/arc/include/asm/linkage.h
> index b29f1a9fd6f7..3a5f13d65ee1 100644
> --- a/arch/arc/include/asm/linkage.h
> +++ b/arch/arc/include/asm/linkage.h
> @@ -28,7 +28,7 @@
> /* annotation for data we want in DCCM - if enabled in .config */
> .macro ARCFP_CODE
> #ifdef CONFIG_ARC_HAS_ICCM
> - .section .text.arcfp, "ax",@progbits
> + .section .text..arcfp, "ax",@progbits

Why not turn this around and call it arcfp.text and arcfp.data etc - just like
done for other special sections such as sched, cpuidle, lock... Just so we are
consistent with normal kernel convention !

> #else
> .section .text, "ax",@progbits
> #endif
> @@ -47,13 +47,13 @@
> #else /* !__ASSEMBLY__ */
>
> #ifdef CONFIG_ARC_HAS_ICCM
> -#define __arcfp_code __attribute__((__section__(".text.arcfp")))
> +#define __arcfp_code __attribute__((__section__(".text..arcfp")))
> #else
> #define __arcfp_code __attribute__((__section__(".text")))
> #endif
>
> #ifdef CONFIG_ARC_HAS_DCCM
> -#define __arcfp_data __attribute__((__section__(".data.arcfp")))
> +#define __arcfp_data __attribute__((__section__(".data..arcfp")))
> #else
> #define __arcfp_data __attribute__((__section__(".data")))
> #endif
> diff --git a/arch/arc/kernel/vmlinux.lds.S b/arch/arc/kernel/vmlinux.lds.S
> index f35ed578e007..f69ae479ee73 100644
> --- a/arch/arc/kernel/vmlinux.lds.S
> +++ b/arch/arc/kernel/vmlinux.lds.S
> @@ -37,8 +37,8 @@ SECTIONS
> }
>
> #ifdef CONFIG_ARC_HAS_ICCM
> - .text.arcfp : {
> - *(.text.arcfp)
> + .text..arcfp : {
> + *(.text..arcfp)
> . = ALIGN(CONFIG_ARC_ICCM_SZ * 1024);
> }
> #endif
> @@ -151,8 +151,8 @@ SECTIONS
> #ifdef CONFIG_ARC_HAS_DCCM
> . = CONFIG_ARC_DCCM_BASE;
> __arc_dccm_base = .;
> - .data.arcfp : {
> - *(.data.arcfp)
> + .data..arcfp : {
> + *(.data..arcfp)
> }
> . = ALIGN(CONFIG_ARC_DCCM_SZ * 1024);
> #endif

2016-12-23 00:27:57

by Vineet Gupta

[permalink] [raw]
Subject: Re: [PATCH 0/2] Minor fixes for CCMs

On 12/22/2016 06:09 AM, Alexey Brodkin wrote:
> It turned out current implementation of CCM support doesn't work at all.
> There're 2 isseus:
> * Data/code which is supposed to be in DCCM or ICCM accordingly gets
> merged in common .data and .text sections so CCMs won't be used
> * Kerenl will panic on early boot because comparison of CCM sizes
> is implemented incorrectly

FWIW, for current hsdk bringup I won't recommend to switch them on just yet !

>
> This short series fixes both issues above so CCMs are usable again.
> And both patches indeed should be back-ported to stable kernels.
>
> Alexey Brodkin (2):
> arc: rename xCCM sections so they are not merged in global .data/.text
> arc: Fix xCCM size check
>
> arch/arc/include/asm/linkage.h | 6 +++---
> arch/arc/kernel/setup.c | 4 ++--
> arch/arc/kernel/vmlinux.lds.S | 8 ++++----
> 3 files changed, 9 insertions(+), 9 deletions(-)
>

2016-12-23 00:34:12

by Vineet Gupta

[permalink] [raw]
Subject: Re: [PATCH 2/2] arc: Fix xCCM size check

On 12/22/2016 06:09 AM, Alexey Brodkin wrote:
> CONFIG_ARC_ICCM_SZ in menuconfig is specified in kB while
> "cpu->Xccm.sz" contains value in bytes thus direct comparison fails
> leading to boot-time panic like that:
> ----------------------->8---------------------
> IDENTITY : ARCVER [0x52] ARCNUM [0x1] CHIPID [ 0x0]
> processor [1] : ARC HS38 R2.1 (ARCv2 ISA)
> Timers : Timer0 Timer1 Local-64-bit-Ctr (not used)
> ISA Extn : atomic ll64 unalign (not used)
> : mpy[opt 9] div_rem norm barrel-shift swap minmax swape
> BPU : full match, cache:2048, Predict Table:16384
> MMU [v0] : 0k PAGE, JTLB 0 (0x0), uDTLB 0, uITLB 0
> I-Cache : N/A
> D-Cache : N/A
> Peripherals : 0xf0000000, IO-Coherency (disabled)
> Vector Table : 0x80000000
> FPU : SP DP
> DEBUG : ActionPoint smaRT RTT
> Extn [CCM] : DCCM @ e0000000, 256 KB / ICCM: @ 60000000, 256 KB
> OS ABI [v4] : 64-bit data any register aligned
> Extn [SMP] : ARConnect (v2): 2 cores with IPI IDU DEBUG GFRC
> Kernel panic - not syncing: Linux built with incorrect DCCM Size
>
> ---[ end Kernel panic - not syncing: Linux built with incorrect DCCM Size
> ----------------------->8---------------------
>
> Signed-off-by: Alexey Brodkin <[email protected]>
> Cc: Igor Guryanov <[email protected]>
> Cc: [email protected]
> ---
> arch/arc/kernel/setup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index ee574f37f365..601dab6fe5e0 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -333,12 +333,12 @@ static void arc_chk_core_config(void)
> if ((unsigned int)__arc_dccm_base != cpu->dccm.base_addr)
> panic("Linux built with incorrect DCCM Base address\n");
>
> - if (CONFIG_ARC_DCCM_SZ != cpu->dccm.sz)
> + if (CONFIG_ARC_DCCM_SZ != TO_KB(cpu->dccm.sz))

Could we just avoid this existing TO_KB non sense in multiple places by keeping
the *ccm.sz unit consistent with CONFIG_ARC_*CCM_SZ ?
so
- cpu->iccm.sz = 4096 << iccm.sz; /* 8K to 512K */
+ cpu->iccm.sz = 4 << iccm.sz; /* 8K to 512K */

Since we only want to keep ccm size to kb granularity

And while at it, rename @sz placeholder in bcr_(i|d)ccm_arc(v2,compact) to sz_k

-Vineet

> panic("Linux built with incorrect DCCM Size\n");
> #endif
>
> #ifdef CONFIG_ARC_HAS_ICCM
> - if (CONFIG_ARC_ICCM_SZ != cpu->iccm.sz)
> + if (CONFIG_ARC_ICCM_SZ != TO_KB(cpu->iccm.sz))
> panic("Linux built with incorrect ICCM Size\n");
> #endif
>