risc-v/litex: Remove CONFIG_BOARDCTL_RESET override for arty_a7:nsh config.

[jerenkrantz]

Fixes build failure as up_flush_dcache_all is not defined in arty_a7:nsh builds.

Summary

arty_a7:nsh builds fail with:

riscv64-unknown-elf-ld: .../nuttxspace/nuttx/staging/libboards.a(boardctl.o): in function `boardctl':
.../nuttxspace/nuttx/boards/boardctl.c:415: undefined reference to `up_flush_dcache_all'

It doesn't appear that CONFIG_BOARDCTL_RESET is required for nsh, so removing the option seems prudent as the default is N. If there is a better fix to bring in the up_flush_dcache_all definition, I'm happy to pursue that instead.

Impact

The configuration doesn't compile without this change.

Testing

I'm able to successfully build arty_a7:nsh images with this change.

[acassis]

@jerenkrantz I think you found a BUG in boards/boardctl.c since up_flush_dcache_all() is not implemented for all architectures.

@XuNeo could you please take a look? Since you added it here: #13908

I don't know what is the proper solution, maybe up_flush_dcache_all() could be defined as weak and only called if it exist.

@xiaoxiang781216 @anchao @raiden00pl please take a look

[xiaoxiang781216]

@jerenkrantz I think you found a BUG in boards/boardctl.c since up_flush_dcache_all() is not implemented for all architectures.

@XuNeo could you please take a look? Since you added it here: #13908

I don't know what is the proper solution, maybe up_flush_dcache_all() could be defined as weak and only called if it exist.

@xiaoxiang781216 @anchao @raiden00pl please take a look

but it's strange why the linker error happen if the board doesn't enable CONFIG_ARCH_DCACHE since the dummy macro is provided in this case:
https://github.com/apache/nuttx/blob/master/include/nuttx/cache.h#L447-L451

/****************************************************************************
 * Name: up_flush_dcache_all
 *
 * Description:
 *   Flush the entire data cache by cleaning and invalidating the D cache.
 *
 * Input Parameters:
 *   None
 *
 * Returned Value:
 *   None
 *
 ****************************************************************************/

#ifdef CONFIG_ARCH_DCACHE
void up_flush_dcache_all(void);
#else
#  define up_flush_dcache_all()
#endif

need refine

[XuNeo]

Looks like ARCH_DCACHE is selected since this commit.
But dcache related API is not fully implemented.

nuttx/include/nuttx/cache.h

Lines 343 to 369 in d557b87

	****************************************************************************/
	#ifdef CONFIG_ARCH_DCACHE
	void up_flush_dcache(uintptr_t start, uintptr_t end);
	#else
	# define up_flush_dcache(start, end)
	#endif
	/****************************************************************************
	* Name: up_flush_dcache_all
	*
	* Description:
	* Flush the entire data cache by cleaning and invalidating the D cache.
	*
	* Input Parameters:
	* None
	*
	* Returned Value:
	* None
	*
	****************************************************************************/
	#ifdef CONFIG_ARCH_DCACHE
	void up_flush_dcache_all(void);
	#else
	# define up_flush_dcache_all()
	#endif

[xiaoxiang781216]

so, the best solution is implemented up_flush_dcache_all for riscv arch like others. @jerenkrantz @acassis .

[jerenkrantz]

Ack - thanks for the pointer! As I initially thought, this could actually be a code gap rather than a config gap. =)

I'll take a peek at trying to figure out a default risc-v implementation for up_flush_dcache_all. That said, I'll be traveling and without access to my arty_a7 rig, so it may take a week or so to make any substantial progress.

If I get to a point where I have a PR to discuss, I'll update this PR with a pointer to a new PR. Obviously, if anyone else wants to beat me to it, that's great too! Ha!

[jerpelea]

it would be better implement the feature

[jerenkrantz]

In digging into this a bit more, it appears that there is already an up_invalidate_dcache_all in litex_cache.S.

If I further look upstream at https://github.com/enjoy-digital/litex/blob/master/litex/soc/cores/cpu/vexriscv_smp/system.h, flush_cpu_dcache is defined 0x500F as flushing rather than invalidating. nuttx on litex is currently doing 0x500F as invalidation. (I believe this system.h is intended for inclusion in Linux distributions.)

Adding the below diff to litex_cache.S does allow it to build with BOARDCTL_RESET=y override in place.

Before submitting a separate PR, I'd like to ensure that this is the right track. IOW, I suspect that the invalidating might actually be flushing based upon upstream definitions. I'm not sure whether breaking that assumption for invalidation would have any effects. (I'm not seeing any a declared mechanism for invalidation in litex repos, but perhaps it is there in another location?)

diff --git a/arch/risc-v/src/litex/litex_cache.S b/arch/risc-v/src/litex/litex_cache.S
index 37bb1d65b8..2820a38431 100644
--- a/arch/risc-v/src/litex/litex_cache.S
+++ b/arch/risc-v/src/litex/litex_cache.S
@@ -56,6 +56,28 @@ up_invalidate_dcache_all:
   .word 0x500F
 #endif

+/****************************************************************************
+ * Name: up_flush_dcache_all
+ *
+ * Description:
+ *   Flush the entire contents of D cache.
+ *
+ * Input Parameters:
+ *   None
+ *
+ * Returned Value:
+ *   None
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_DCACHE
+  .globl  up_flush_dcache_all
+  .type   up_flush_dcache_all, function
+
+up_flush_dcache_all:
+  .word 0x500F
+#endif
+
 /****************************************************************************
  * Name: up_invalidate_icache_all
  *

[jerenkrantz]

I have opened #17696 with this code diff discussed above.