Re: [Buildroot] [PATCH 1/1] package/riscv-coremark: Create package with CoreMark benchmark for RISC-V

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Eryk,

On Tue, 6 Aug 2024 18:39:58 +0200
Eryk Szpotanski <eszpotanski@antmicro.com> wrote:

> From 3c09f5c971248c8c9c05a8bab79a5ddb45b40eb7 Mon Sep 17 00:00:00 2001
> From: Eryk Szpotanski <eszpotanski@antmicro.com>
> Date: Thu, 9 May 2024 12:34:08 +0200
> Subject: [PATCH 1/1] package/riscv-coremark: Create package with CoreMark
>  benchmark for RISC-V
> 
> Signed-off-by: Eryk Szpotanski <eszpotanski@antmicro.com>

Thanks a lot for your patch! However, it looks like you haven't used
"git send-email", so your patch is badly line-wrapped and cannot be
applied. Could you use "git send-email" to send a new iteration?

See a few quick review comments below.

First, why do we need a riscv-coremark as opposed to an
architecture-agnostic coremark?

> ---
>  package/Config.in                             |  1 +
>  ...ype-to-match-it-with-definition-from.patch | 27 +++++++++++++++++++
>  package/riscv-coremark/Config.in              |  8 ++++++
>  package/riscv-coremark/riscv-coremark.hash    |  3 +++
>  package/riscv-coremark/riscv-coremark.mk      | 22 +++++++++++++++
>  5 files changed, 61 insertions(+)

Please add yourself to the DEVELOPERS as the maintainer for this
package.


> b/package/riscv-coremark/0001-Update-clock_t-type-to-match-it-with-definition-from.patch
> @@ -0,0 +1,27 @@
> +From a6c8e2e00e5534b340885181d99ecf19286a9ff8 Mon Sep 17 00:00:00 2001
> +From: Eryk Szpotanski <eszpotanski@antmicro.com>
> +Date: Tue, 6 Aug 2024 13:44:21 +0200
> +Subject: [PATCH] Update clock_t type, to match it with external definition
> +Upstream: N/A This patch adjust the clock_t type to match it with the
> definition from RISC-V toolchain

Why is that not applicable upstream? Especially if upstream is anyway
RISC-V specific?


> diff --git a/package/riscv-coremark/Config.in
> b/package/riscv-coremark/Config.in
> new file mode 100644
> index 0000000000..c9ed47befa
> --- /dev/null
> +++ b/package/riscv-coremark/Config.in
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_RISCV_COREMARK
> + bool "riscv-coremark"
> + depends on BR2_RISCV_64

As asked above: why is this RISC-V specific?


> diff --git a/package/riscv-coremark/riscv-coremark.mk
> b/package/riscv-coremark/riscv-coremark.mk
> new file mode 100644
> index 0000000000..819fac2313
> --- /dev/null
> +++ b/package/riscv-coremark/riscv-coremark.mk
> @@ -0,0 +1,22 @@
> +################################################################################
> +#
> +# RISC-V Coremark
> +#
> +################################################################################
> +
> +RISCV_COREMARK_SITE_METHOD = git
> +RISCV_COREMARK_GIT_SUBMODULES = YES
> +RISCV_COREMARK_VERSION = 6e1d72b864e45f67031ffaedb0b01b5d030d6d3c
> +RISCV_COREMARK_SITE = https://github.com/riscv-boom/riscv-coremark.git
> +RISCV_COREMARK_LICENSE = BSD-3

Should be BSD-3-Clause

> +RISCV_COREMARK_LICENSE_FILE = LICENSE
> +
> +define RISCV_COREMARK_BUILD_CMDS
> + $(TARGET_MAKE_ENV) $(MAKE) CC="$(TARGET_CC)" -C $(@D)/coremark
> PORT_DIR=$(@D)/riscv64 compile

Could you try to use $(TARGET_CONFIGURE_OPTS) instead of just CC="$(TARGET_CC)" ?

> +endef
> +
> +define RISCV_COREMARK_INSTALL_TARGET_CMDS
> + $(INSTALL) -D $(@D)/coremark/coremark.riscv
> $(TARGET_DIR)/usr/bin/coremark.riscv

Please add -m 0755 to this $(INSTALL) invocation.

Overall, it looks reasonable, really the main concern is why do we need
a RISC-V specific version of this? What if we want coremark on other
architectures?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot