OpenSBI Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] OpenSBI LLVM related fixes
@ 2024-12-10  5:23 Anup Patel
  2024-12-10  5:23 ` [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM Anup Patel
  2024-12-10  5:23 ` [PATCH v2 2/2] Makefile: Don't enable V-extension using -march option Anup Patel
  0 siblings, 2 replies; 9+ messages in thread
From: Anup Patel @ 2024-12-10  5:23 UTC (permalink / raw)
  To: opensbi

This series does few LLVM related fixes encountered using
"Ubuntu clang version 18.1.3 (1ubuntu1)".

These patches can also be found in the riscv_llvm_compile_fixes_v2
branch at: https://github.com/avpatel/opensbi.git

Changes since v1:
 - Removed -fno-tree-vectorize from PATCH2

Anup Patel (2):
  lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM
  Makefile: Don't enable V-extension using -march option

 Makefile                       | 8 ++++----
 lib/sbi/sbi_trap_v_ldst.c      | 5 +++--
 lib/utils/cppc/fdt_cppc_rpmi.c | 7 +++++--
 3 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.43.0



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM
  2024-12-10  5:23 [PATCH v2 0/2] OpenSBI LLVM related fixes Anup Patel
@ 2024-12-10  5:23 ` Anup Patel
  2024-12-11  0:05   ` Samuel Holland
                     ` (2 more replies)
  2024-12-10  5:23 ` [PATCH v2 2/2] Makefile: Don't enable V-extension using -march option Anup Patel
  1 sibling, 3 replies; 9+ messages in thread
From: Anup Patel @ 2024-12-10  5:23 UTC (permalink / raw)
  To: opensbi

The following error is observed when compiling fdt_cppc_rpmi
driver using LLVM:

lib/utils/cppc/fdt_cppc_rpmi.c:87:3: error: label followed by a declaration is a C23 extension [-Werror,-Wc23-extensions]
   87 |                 u64 db_val_u64 = 0;

To fix the above issue, move the variable declaration at the
start of function.

Fixes: 591a98bdd549 ("lib: utils/cppc: Add RPMI CPPC driver")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 lib/utils/cppc/fdt_cppc_rpmi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/utils/cppc/fdt_cppc_rpmi.c b/lib/utils/cppc/fdt_cppc_rpmi.c
index 26e2d4f6..b6789901 100644
--- a/lib/utils/cppc/fdt_cppc_rpmi.c
+++ b/lib/utils/cppc/fdt_cppc_rpmi.c
@@ -59,6 +59,11 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
 	u8 db_val_u8 = 0;
 	u16 db_val_u16 = 0;
 	u32 db_val_u32 = 0;
+#if __riscv_xlen != 32
+	u64 db_val_u64 = 0;
+#else
+	u32 db_val_u32_hi = 0;
+#endif
 
 	switch (cppc->fc_db_width) {
 	case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_8:
@@ -84,14 +89,12 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
 		break;
 	case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_64:
 #if __riscv_xlen != 32
-		u64 db_val_u64 = 0;
 		db_val_u64 = readq((void *)cppc->fc_db_addr);
 		db_val_u64 = cppc->fc_db_setmask |
 				(db_val_u64 & cppc->fc_db_preservemask);
 
 		writeq(db_val_u64, (void *)cppc->fc_db_addr);
 #else
-		u32 db_val_u32_hi = 0;
 		db_val_u32 = readl((void *)cppc->fc_db_addr);
 		db_val_u32_hi = readl((void *)(cppc->fc_db_addr + 4));
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] Makefile: Don't enable V-extension using -march option
  2024-12-10  5:23 [PATCH v2 0/2] OpenSBI LLVM related fixes Anup Patel
  2024-12-10  5:23 ` [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM Anup Patel
@ 2024-12-10  5:23 ` Anup Patel
  2024-12-11  0:11   ` Samuel Holland
  1 sibling, 1 reply; 9+ messages in thread
From: Anup Patel @ 2024-12-10  5:23 UTC (permalink / raw)
  To: opensbi

Enabling V-extension using -march option causes OpenSBI boot-time
hang with LLVM compiler.

As a work-around, don't enable V-extension using -march option and
instead use a custom OpenSBI specific define inform availability of
V-extension to lib/sbi/sbi_trap_v_ldst.c.

Fixes: c2acc5e5b0d8 ("lib: sbi_misaligned_ldst: Add handling of vector load/store")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 Makefile                  | 8 ++++----
 lib/sbi/sbi_trap_v_ldst.c | 5 +++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 5ac95a0f..1b949a9f 100644
--- a/Makefile
+++ b/Makefile
@@ -190,7 +190,7 @@ CC_SUPPORT_STRICT_ALIGN := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib
 CC_SUPPORT_ZICSR_ZIFENCEI := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib -march=rv$(OPENSBI_CC_XLEN)imafd_zicsr_zifencei -x c /dev/null -o /dev/null 2>&1 | grep -e "zicsr" -e "zifencei" > /dev/null && echo n || echo y)
 
 # Check whether the assembler and the compiler support the Vector extension
-CC_SUPPORT_VECT := $(shell echo | $(CC) -dM -E -march=rv$(OPENSBI_CC_XLEN)gv - | grep -q riscv.*vector && echo y || echo n)
+CC_SUPPORT_VECT := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib -march=rv$(OPENSBI_CC_XLEN)gv -dM -E -x c /dev/null 2>&1 | grep -q riscv.*vector && echo y || echo n)
 
 ifneq ($(OPENSBI_LD_PIE),y)
 $(error Your linker does not support creating PIEs, opensbi requires this.)
@@ -298,9 +298,6 @@ endif
 ifndef PLATFORM_RISCV_ISA
   ifneq ($(PLATFORM_RISCV_TOOLCHAIN_DEFAULT), 1)
     PLATFORM_RISCV_ISA := rv$(PLATFORM_RISCV_XLEN)imafdc
-    ifeq ($(CC_SUPPORT_VECT), y)
-      PLATFORM_RISCV_ISA := $(PLATFORM_RISCV_ISA)v
-    endif
     ifeq ($(CC_SUPPORT_ZICSR_ZIFENCEI), y)
       PLATFORM_RISCV_ISA := $(PLATFORM_RISCV_ISA)_zicsr_zifencei
     endif
@@ -363,6 +360,9 @@ GENFLAGS	+=	$(firmware-genflags-y)
 CFLAGS		=	-g -Wall -Werror -ffreestanding -nostdlib -fno-stack-protector -fno-strict-aliasing -ffunction-sections -fdata-sections
 CFLAGS		+=	-fno-omit-frame-pointer -fno-optimize-sibling-calls
 # Optionally supported flags
+ifeq ($(CC_SUPPORT_VECT),y)
+CFLAGS		+=	-DOPENSBI_CC_SUPPORT_VECT
+endif
 ifeq ($(CC_SUPPORT_SAVE_RESTORE),y)
 CFLAGS		+=	-mno-save-restore
 endif
diff --git a/lib/sbi/sbi_trap_v_ldst.c b/lib/sbi/sbi_trap_v_ldst.c
index 9929215c..75b79baa 100644
--- a/lib/sbi/sbi_trap_v_ldst.c
+++ b/lib/sbi/sbi_trap_v_ldst.c
@@ -17,7 +17,8 @@
 #include <sbi/sbi_unpriv.h>
 #include <sbi/sbi_trap.h>
 
-#ifdef __riscv_vector
+#ifdef OPENSBI_CC_SUPPORT_VECT
+
 #define VLEN_MAX 65536
 
 static inline void set_vreg(ulong vlenb, ulong which,
@@ -340,4 +341,4 @@ int sbi_misaligned_v_st_emulator(int wlen, union sbi_ldst_data in_val,
 {
 	return 0;
 }
-#endif /* __riscv_vector  */
+#endif /* OPENSBI_CC_SUPPORT_VECT  */
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM
  2024-12-10  5:23 ` [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM Anup Patel
@ 2024-12-11  0:05   ` Samuel Holland
  2024-12-11  9:59   ` Xiang W
  2024-12-15  6:03   ` Anup Patel
  2 siblings, 0 replies; 9+ messages in thread
From: Samuel Holland @ 2024-12-11  0:05 UTC (permalink / raw)
  To: opensbi

On 2024-12-09 11:23 PM, Anup Patel wrote:
> The following error is observed when compiling fdt_cppc_rpmi
> driver using LLVM:
> 
> lib/utils/cppc/fdt_cppc_rpmi.c:87:3: error: label followed by a declaration is a C23 extension [-Werror,-Wc23-extensions]
>    87 |                 u64 db_val_u64 = 0;
> 
> To fix the above issue, move the variable declaration at the
> start of function.
> 
> Fixes: 591a98bdd549 ("lib: utils/cppc: Add RPMI CPPC driver")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  lib/utils/cppc/fdt_cppc_rpmi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Samuel Holland <samuel.holland@sifive.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] Makefile: Don't enable V-extension using -march option
  2024-12-10  5:23 ` [PATCH v2 2/2] Makefile: Don't enable V-extension using -march option Anup Patel
@ 2024-12-11  0:11   ` Samuel Holland
  2024-12-15  6:04     ` Anup Patel
  0 siblings, 1 reply; 9+ messages in thread
From: Samuel Holland @ 2024-12-11  0:11 UTC (permalink / raw)
  To: opensbi

On 2024-12-09 11:23 PM, Anup Patel wrote:
> Enabling V-extension using -march option causes OpenSBI boot-time
> hang with LLVM compiler.
> 
> As a work-around, don't enable V-extension using -march option and
> instead use a custom OpenSBI specific define inform availability of
> V-extension to lib/sbi/sbi_trap_v_ldst.c.
> 
> Fixes: c2acc5e5b0d8 ("lib: sbi_misaligned_ldst: Add handling of vector load/store")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  Makefile                  | 8 ++++----
>  lib/sbi/sbi_trap_v_ldst.c | 5 +++--
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 5ac95a0f..1b949a9f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -190,7 +190,7 @@ CC_SUPPORT_STRICT_ALIGN := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib
>  CC_SUPPORT_ZICSR_ZIFENCEI := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib -march=rv$(OPENSBI_CC_XLEN)imafd_zicsr_zifencei -x c /dev/null -o /dev/null 2>&1 | grep -e "zicsr" -e "zifencei" > /dev/null && echo n || echo y)
>  
>  # Check whether the assembler and the compiler support the Vector extension
> -CC_SUPPORT_VECT := $(shell echo | $(CC) -dM -E -march=rv$(OPENSBI_CC_XLEN)gv - | grep -q riscv.*vector && echo y || echo n)
> +CC_SUPPORT_VECT := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib -march=rv$(OPENSBI_CC_XLEN)gv -dM -E -x c /dev/null 2>&1 | grep -q riscv.*vector && echo y || echo n)
>  
>  ifneq ($(OPENSBI_LD_PIE),y)
>  $(error Your linker does not support creating PIEs, opensbi requires this.)
> @@ -298,9 +298,6 @@ endif
>  ifndef PLATFORM_RISCV_ISA
>    ifneq ($(PLATFORM_RISCV_TOOLCHAIN_DEFAULT), 1)
>      PLATFORM_RISCV_ISA := rv$(PLATFORM_RISCV_XLEN)imafdc
> -    ifeq ($(CC_SUPPORT_VECT), y)
> -      PLATFORM_RISCV_ISA := $(PLATFORM_RISCV_ISA)v
> -    endif
>      ifeq ($(CC_SUPPORT_ZICSR_ZIFENCEI), y)
>        PLATFORM_RISCV_ISA := $(PLATFORM_RISCV_ISA)_zicsr_zifencei
>      endif
> @@ -363,6 +360,9 @@ GENFLAGS	+=	$(firmware-genflags-y)
>  CFLAGS		=	-g -Wall -Werror -ffreestanding -nostdlib -fno-stack-protector -fno-strict-aliasing -ffunction-sections -fdata-sections
>  CFLAGS		+=	-fno-omit-frame-pointer -fno-optimize-sibling-calls
>  # Optionally supported flags
> +ifeq ($(CC_SUPPORT_VECT),y)
> +CFLAGS		+=	-DOPENSBI_CC_SUPPORT_VECT

nit: Since you're changing this, could you please spell out "VECTOR"? The
abbreviation isn't great for greppability.

Reviewed-by: Samuel Holland <samuel.holland@sifive.com>

> +endif
>  ifeq ($(CC_SUPPORT_SAVE_RESTORE),y)
>  CFLAGS		+=	-mno-save-restore
>  endif
> diff --git a/lib/sbi/sbi_trap_v_ldst.c b/lib/sbi/sbi_trap_v_ldst.c
> index 9929215c..75b79baa 100644
> --- a/lib/sbi/sbi_trap_v_ldst.c
> +++ b/lib/sbi/sbi_trap_v_ldst.c
> @@ -17,7 +17,8 @@
>  #include <sbi/sbi_unpriv.h>
>  #include <sbi/sbi_trap.h>
>  
> -#ifdef __riscv_vector
> +#ifdef OPENSBI_CC_SUPPORT_VECT
> +
>  #define VLEN_MAX 65536
>  
>  static inline void set_vreg(ulong vlenb, ulong which,
> @@ -340,4 +341,4 @@ int sbi_misaligned_v_st_emulator(int wlen, union sbi_ldst_data in_val,
>  {
>  	return 0;
>  }
> -#endif /* __riscv_vector  */
> +#endif /* OPENSBI_CC_SUPPORT_VECT  */



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM
  2024-12-10  5:23 ` [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM Anup Patel
  2024-12-11  0:05   ` Samuel Holland
@ 2024-12-11  9:59   ` Xiang W
  2024-12-15  6:02     ` Anup Patel
  2024-12-15  6:03   ` Anup Patel
  2 siblings, 1 reply; 9+ messages in thread
From: Xiang W @ 2024-12-11  9:59 UTC (permalink / raw)
  To: opensbi

? 2024-12-10?? 10:53 +0530?Anup Patel???
> The following error is observed when compiling fdt_cppc_rpmi
> driver using LLVM:
> 
> lib/utils/cppc/fdt_cppc_rpmi.c:87:3: error: label followed by a declaration is a C23 extension [-Werror,-Wc23-extensions]
> ?? 87 |???????????????? u64 db_val_u64 = 0;
> 
> To fix the above issue, move the variable declaration at the
> start of function.
> 
> Fixes: 591a98bdd549 ("lib: utils/cppc: Add RPMI CPPC driver")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
> ?lib/utils/cppc/fdt_cppc_rpmi.c | 7 +++++--
> ?1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/utils/cppc/fdt_cppc_rpmi.c b/lib/utils/cppc/fdt_cppc_rpmi.c
> index 26e2d4f6..b6789901 100644
> --- a/lib/utils/cppc/fdt_cppc_rpmi.c
> +++ b/lib/utils/cppc/fdt_cppc_rpmi.c
> @@ -59,6 +59,11 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
> ?	u8 db_val_u8 = 0;
> ?	u16 db_val_u16 = 0;
> ?	u32 db_val_u32 = 0;
> +#if __riscv_xlen != 32
> +	u64 db_val_u64 = 0;
> +#else
> +	u32 db_val_u32_hi = 0;
> +#endif
> ?
> ?	switch (cppc->fc_db_width) {
> ?	case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_8:
> @@ -84,14 +89,12 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
> ?		break;
> ?	case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_64:
> ?#if __riscv_xlen != 32
> -		u64 db_val_u64 = 0;
> ?		db_val_u64 = readq((void *)cppc->fc_db_addr);
> ?		db_val_u64 = cppc->fc_db_setmask |
> ?				(db_val_u64 & cppc->fc_db_preservemask);
> ?
> ?		writeq(db_val_u64, (void *)cppc->fc_db_addr);
> ?#else
> -		u32 db_val_u32_hi = 0;
> ?		db_val_u32 = readl((void *)cppc->fc_db_addr);
> ?		db_val_u32_hi = readl((void *)(cppc->fc_db_addr + 4));
> ?
> -- 
> 2.43.0
> 
> 

We may be able to remove db_val_u32_hi. as follows

Regards,
Xiang W

diff --git a/lib/utils/cppc/fdt_cppc_rpmi.c b/lib/utils/cppc/fdt_cppc_rpmi.c
index 26e2d4f6..f1f757ff 100644
--- a/lib/utils/cppc/fdt_cppc_rpmi.c
+++ b/lib/utils/cppc/fdt_cppc_rpmi.c
@@ -59,6 +59,7 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
 	u8 db_val_u8 = 0;
 	u16 db_val_u16 = 0;
 	u32 db_val_u32 = 0;
+	u64 db_val_u64 = 0;
 
 	switch (cppc->fc_db_width) {
 	case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_8:
@@ -84,24 +85,20 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
 		break;
 	case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_64:
 #if __riscv_xlen != 32
-		u64 db_val_u64 = 0;
 		db_val_u64 = readq((void *)cppc->fc_db_addr);
 		db_val_u64 = cppc->fc_db_setmask |
 				(db_val_u64 & cppc->fc_db_preservemask);
 
 		writeq(db_val_u64, (void *)cppc->fc_db_addr);
 #else
-		u32 db_val_u32_hi = 0;
-		db_val_u32 = readl((void *)cppc->fc_db_addr);
-		db_val_u32_hi = readl((void *)(cppc->fc_db_addr + 4));
-
-		db_val_u32 = (u32)cppc->fc_db_setmask |
-				(db_val_u32 & (u32)cppc->fc_db_preservemask);
-		db_val_u32_hi = (u32)(cppc->fc_db_setmask >> 32) |
-				(db_val_u32 & (u32)(cppc->fc_db_preservemask >> 32));
+		db_val_u64 = readl((void *)(cppc->fc_db_addr + 4));
+		db_val_u64 <<= 32;
+		db_val_u64 |= readl((void *)cppc->fc_db_addr);
+		db_val_u64 = cppc->fc_db_setmask |
+				(db_val_u64 & cppc->fc_db_preservemask);
 
-		writel(db_val_u32, (void *)cppc->fc_db_addr);
-		writel(db_val_u32_hi, (void *)(cppc->fc_db_addr + 4));
+		writel(db_val_u64, (void *)cppc->fc_db_addr);
+		writel(db_val_u64 >> 32, (void *)(cppc->fc_db_addr + 4));
 #endif
 		break;
 	default:




^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM
  2024-12-11  9:59   ` Xiang W
@ 2024-12-15  6:02     ` Anup Patel
  0 siblings, 0 replies; 9+ messages in thread
From: Anup Patel @ 2024-12-15  6:02 UTC (permalink / raw)
  To: opensbi

On Wed, Dec 11, 2024 at 3:31?PM Xiang W <wxjstz@126.com> wrote:
>
> ? 2024-12-10?? 10:53 +0530?Anup Patel???
> > The following error is observed when compiling fdt_cppc_rpmi
> > driver using LLVM:
> >
> > lib/utils/cppc/fdt_cppc_rpmi.c:87:3: error: label followed by a declaration is a C23 extension [-Werror,-Wc23-extensions]
> >    87 |                 u64 db_val_u64 = 0;
> >
> > To fix the above issue, move the variable declaration at the
> > start of function.
> >
> > Fixes: 591a98bdd549 ("lib: utils/cppc: Add RPMI CPPC driver")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  lib/utils/cppc/fdt_cppc_rpmi.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/utils/cppc/fdt_cppc_rpmi.c b/lib/utils/cppc/fdt_cppc_rpmi.c
> > index 26e2d4f6..b6789901 100644
> > --- a/lib/utils/cppc/fdt_cppc_rpmi.c
> > +++ b/lib/utils/cppc/fdt_cppc_rpmi.c
> > @@ -59,6 +59,11 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
> >       u8 db_val_u8 = 0;
> >       u16 db_val_u16 = 0;
> >       u32 db_val_u32 = 0;
> > +#if __riscv_xlen != 32
> > +     u64 db_val_u64 = 0;
> > +#else
> > +     u32 db_val_u32_hi = 0;
> > +#endif
> >
> >       switch (cppc->fc_db_width) {
> >       case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_8:
> > @@ -84,14 +89,12 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
> >               break;
> >       case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_64:
> >  #if __riscv_xlen != 32
> > -             u64 db_val_u64 = 0;
> >               db_val_u64 = readq((void *)cppc->fc_db_addr);
> >               db_val_u64 = cppc->fc_db_setmask |
> >                               (db_val_u64 & cppc->fc_db_preservemask);
> >
> >               writeq(db_val_u64, (void *)cppc->fc_db_addr);
> >  #else
> > -             u32 db_val_u32_hi = 0;
> >               db_val_u32 = readl((void *)cppc->fc_db_addr);
> >               db_val_u32_hi = readl((void *)(cppc->fc_db_addr + 4));
> >
> > --
> > 2.43.0
> >
> >
>
> We may be able to remove db_val_u32_hi. as follows
>
> Regards,
> Xiang W
>
> diff --git a/lib/utils/cppc/fdt_cppc_rpmi.c b/lib/utils/cppc/fdt_cppc_rpmi.c
> index 26e2d4f6..f1f757ff 100644
> --- a/lib/utils/cppc/fdt_cppc_rpmi.c
> +++ b/lib/utils/cppc/fdt_cppc_rpmi.c
> @@ -59,6 +59,7 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
>         u8 db_val_u8 = 0;
>         u16 db_val_u16 = 0;
>         u32 db_val_u32 = 0;
> +       u64 db_val_u64 = 0;
>
>         switch (cppc->fc_db_width) {
>         case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_8:
> @@ -84,24 +85,20 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
>                 break;
>         case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_64:
>  #if __riscv_xlen != 32
> -               u64 db_val_u64 = 0;
>                 db_val_u64 = readq((void *)cppc->fc_db_addr);
>                 db_val_u64 = cppc->fc_db_setmask |
>                                 (db_val_u64 & cppc->fc_db_preservemask);
>
>                 writeq(db_val_u64, (void *)cppc->fc_db_addr);
>  #else
> -               u32 db_val_u32_hi = 0;
> -               db_val_u32 = readl((void *)cppc->fc_db_addr);
> -               db_val_u32_hi = readl((void *)(cppc->fc_db_addr + 4));
> -
> -               db_val_u32 = (u32)cppc->fc_db_setmask |
> -                               (db_val_u32 & (u32)cppc->fc_db_preservemask);
> -               db_val_u32_hi = (u32)(cppc->fc_db_setmask >> 32) |
> -                               (db_val_u32 & (u32)(cppc->fc_db_preservemask >> 32));
> +               db_val_u64 = readl((void *)(cppc->fc_db_addr + 4));
> +               db_val_u64 <<= 32;
> +               db_val_u64 |= readl((void *)cppc->fc_db_addr);
> +               db_val_u64 = cppc->fc_db_setmask |
> +                               (db_val_u64 & cppc->fc_db_preservemask);
>
> -               writel(db_val_u32, (void *)cppc->fc_db_addr);
> -               writel(db_val_u32_hi, (void *)(cppc->fc_db_addr + 4));
> +               writel(db_val_u64, (void *)cppc->fc_db_addr);
> +               writel(db_val_u64 >> 32, (void *)(cppc->fc_db_addr + 4));
>  #endif

Your suggestion looks good to me but it is more of an improvement.
Feel free to send this as a separate improvement patch.

Regards,
Anup


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM
  2024-12-10  5:23 ` [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM Anup Patel
  2024-12-11  0:05   ` Samuel Holland
  2024-12-11  9:59   ` Xiang W
@ 2024-12-15  6:03   ` Anup Patel
  2 siblings, 0 replies; 9+ messages in thread
From: Anup Patel @ 2024-12-15  6:03 UTC (permalink / raw)
  To: opensbi

On Tue, Dec 10, 2024 at 10:53?AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The following error is observed when compiling fdt_cppc_rpmi
> driver using LLVM:
>
> lib/utils/cppc/fdt_cppc_rpmi.c:87:3: error: label followed by a declaration is a C23 extension [-Werror,-Wc23-extensions]
>    87 |                 u64 db_val_u64 = 0;
>
> To fix the above issue, move the variable declaration at the
> start of function.
>
> Fixes: 591a98bdd549 ("lib: utils/cppc: Add RPMI CPPC driver")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

Applied this patch to the riscv/opensbi repo.

Regards,
Anup

> ---
>  lib/utils/cppc/fdt_cppc_rpmi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/lib/utils/cppc/fdt_cppc_rpmi.c b/lib/utils/cppc/fdt_cppc_rpmi.c
> index 26e2d4f6..b6789901 100644
> --- a/lib/utils/cppc/fdt_cppc_rpmi.c
> +++ b/lib/utils/cppc/fdt_cppc_rpmi.c
> @@ -59,6 +59,11 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
>         u8 db_val_u8 = 0;
>         u16 db_val_u16 = 0;
>         u32 db_val_u32 = 0;
> +#if __riscv_xlen != 32
> +       u64 db_val_u64 = 0;
> +#else
> +       u32 db_val_u32_hi = 0;
> +#endif
>
>         switch (cppc->fc_db_width) {
>         case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_8:
> @@ -84,14 +89,12 @@ static void rpmi_cppc_fc_db_trigger(struct rpmi_cppc *cppc)
>                 break;
>         case RPMI_CPPC_FAST_CHANNEL_DB_WIDTH_64:
>  #if __riscv_xlen != 32
> -               u64 db_val_u64 = 0;
>                 db_val_u64 = readq((void *)cppc->fc_db_addr);
>                 db_val_u64 = cppc->fc_db_setmask |
>                                 (db_val_u64 & cppc->fc_db_preservemask);
>
>                 writeq(db_val_u64, (void *)cppc->fc_db_addr);
>  #else
> -               u32 db_val_u32_hi = 0;
>                 db_val_u32 = readl((void *)cppc->fc_db_addr);
>                 db_val_u32_hi = readl((void *)(cppc->fc_db_addr + 4));
>
> --
> 2.43.0
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] Makefile: Don't enable V-extension using -march option
  2024-12-11  0:11   ` Samuel Holland
@ 2024-12-15  6:04     ` Anup Patel
  0 siblings, 0 replies; 9+ messages in thread
From: Anup Patel @ 2024-12-15  6:04 UTC (permalink / raw)
  To: opensbi

On Wed, Dec 11, 2024 at 5:41?AM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> On 2024-12-09 11:23 PM, Anup Patel wrote:
> > Enabling V-extension using -march option causes OpenSBI boot-time
> > hang with LLVM compiler.
> >
> > As a work-around, don't enable V-extension using -march option and
> > instead use a custom OpenSBI specific define inform availability of
> > V-extension to lib/sbi/sbi_trap_v_ldst.c.
> >
> > Fixes: c2acc5e5b0d8 ("lib: sbi_misaligned_ldst: Add handling of vector load/store")
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  Makefile                  | 8 ++++----
> >  lib/sbi/sbi_trap_v_ldst.c | 5 +++--
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 5ac95a0f..1b949a9f 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -190,7 +190,7 @@ CC_SUPPORT_STRICT_ALIGN := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib
> >  CC_SUPPORT_ZICSR_ZIFENCEI := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib -march=rv$(OPENSBI_CC_XLEN)imafd_zicsr_zifencei -x c /dev/null -o /dev/null 2>&1 | grep -e "zicsr" -e "zifencei" > /dev/null && echo n || echo y)
> >
> >  # Check whether the assembler and the compiler support the Vector extension
> > -CC_SUPPORT_VECT := $(shell echo | $(CC) -dM -E -march=rv$(OPENSBI_CC_XLEN)gv - | grep -q riscv.*vector && echo y || echo n)
> > +CC_SUPPORT_VECT := $(shell $(CC) $(CLANG_TARGET) $(RELAX_FLAG) -nostdlib -march=rv$(OPENSBI_CC_XLEN)gv -dM -E -x c /dev/null 2>&1 | grep -q riscv.*vector && echo y || echo n)
> >
> >  ifneq ($(OPENSBI_LD_PIE),y)
> >  $(error Your linker does not support creating PIEs, opensbi requires this.)
> > @@ -298,9 +298,6 @@ endif
> >  ifndef PLATFORM_RISCV_ISA
> >    ifneq ($(PLATFORM_RISCV_TOOLCHAIN_DEFAULT), 1)
> >      PLATFORM_RISCV_ISA := rv$(PLATFORM_RISCV_XLEN)imafdc
> > -    ifeq ($(CC_SUPPORT_VECT), y)
> > -      PLATFORM_RISCV_ISA := $(PLATFORM_RISCV_ISA)v
> > -    endif
> >      ifeq ($(CC_SUPPORT_ZICSR_ZIFENCEI), y)
> >        PLATFORM_RISCV_ISA := $(PLATFORM_RISCV_ISA)_zicsr_zifencei
> >      endif
> > @@ -363,6 +360,9 @@ GENFLAGS  +=      $(firmware-genflags-y)
> >  CFLAGS               =       -g -Wall -Werror -ffreestanding -nostdlib -fno-stack-protector -fno-strict-aliasing -ffunction-sections -fdata-sections
> >  CFLAGS               +=      -fno-omit-frame-pointer -fno-optimize-sibling-calls
> >  # Optionally supported flags
> > +ifeq ($(CC_SUPPORT_VECT),y)
> > +CFLAGS               +=      -DOPENSBI_CC_SUPPORT_VECT
>
> nit: Since you're changing this, could you please spell out "VECTOR"? The
> abbreviation isn't great for greppability.

I have taken care of this at the time of merging this patch.

>
> Reviewed-by: Samuel Holland <samuel.holland@sifive.com>

Applied this patch to the riscv/opensbi repo.

Regards,
Anup

>
> > +endif
> >  ifeq ($(CC_SUPPORT_SAVE_RESTORE),y)
> >  CFLAGS               +=      -mno-save-restore
> >  endif
> > diff --git a/lib/sbi/sbi_trap_v_ldst.c b/lib/sbi/sbi_trap_v_ldst.c
> > index 9929215c..75b79baa 100644
> > --- a/lib/sbi/sbi_trap_v_ldst.c
> > +++ b/lib/sbi/sbi_trap_v_ldst.c
> > @@ -17,7 +17,8 @@
> >  #include <sbi/sbi_unpriv.h>
> >  #include <sbi/sbi_trap.h>
> >
> > -#ifdef __riscv_vector
> > +#ifdef OPENSBI_CC_SUPPORT_VECT
> > +
> >  #define VLEN_MAX 65536
> >
> >  static inline void set_vreg(ulong vlenb, ulong which,
> > @@ -340,4 +341,4 @@ int sbi_misaligned_v_st_emulator(int wlen, union sbi_ldst_data in_val,
> >  {
> >       return 0;
> >  }
> > -#endif /* __riscv_vector  */
> > +#endif /* OPENSBI_CC_SUPPORT_VECT  */
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-12-15  6:04 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-10  5:23 [PATCH v2 0/2] OpenSBI LLVM related fixes Anup Patel
2024-12-10  5:23 ` [PATCH v2 1/2] lib: utils/fdt_cppc_rpmi: Fix compile error with LLVM Anup Patel
2024-12-11  0:05   ` Samuel Holland
2024-12-11  9:59   ` Xiang W
2024-12-15  6:02     ` Anup Patel
2024-12-15  6:03   ` Anup Patel
2024-12-10  5:23 ` [PATCH v2 2/2] Makefile: Don't enable V-extension using -march option Anup Patel
2024-12-11  0:11   ` Samuel Holland
2024-12-15  6:04     ` Anup Patel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox