| Message ID | CAG9w5kMVs04gOeqXtsjYUPJCgcpGNFkpi6C4J7fnsJgVdCvRqw@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] package/riscv-coremark: Create package with CoreMark benchmark for RISC-V | expand |
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
diff --git a/package/Config.in b/package/Config.in index 2ac351cce5..09dab28ef3 100644 --- a/package/Config.in +++ b/package/Config.in @@ -145,6 +145,7 @@ menu "Debugging, profiling and benchmark" source "package/racehound/Config.in" source "package/ramsmp/Config.in" source "package/ramspeed/Config.in" + source "package/riscv-coremark/Config.in" source "package/rt-tests/Config.in" source "package/rwmem/Config.in" source "package/sentry-native/Config.in" diff --git a/package/riscv-coremark/0001-Update-clock_t-type-to-match-it-with-definition-from.patch b/package/riscv-coremark/0001-Update-clock_t-type-to-match-it-with-definition-from.patch new file mode 100644 index 0000000000..fda488198d --- /dev/null +++ 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 + +Signed-off-by: Eryk Szpotanski <eszpotanski@antmicro.com> +--- + riscv64/core_portme.h | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/riscv64/core_portme.h b/riscv64/core_portme.h +index 4e28afd..1e7708e 100755 +--- a/riscv64/core_portme.h ++++ b/riscv64/core_portme.h +@@ -71,7 +71,7 @@ typedef clock_t CORE_TICKS; + Note these need to match the size of the clock output and the xLen the processor supports + */ + typedef unsigned long int size_t; +-typedef unsigned long int clock_t; ++typedef long int clock_t; + typedef clock_t CORE_TICKS; + #endif + +-- +2.45.2 + 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 + help + CoreMark benchmark with utility files provided to + support RISC-V architecture + + https://github.com/riscv-boom/riscv-coremark/ diff --git a/package/riscv-coremark/riscv-coremark.hash b/package/riscv-coremark/riscv-coremark.hash new file mode 100644 index 0000000000..5be3db6bf8 --- /dev/null +++ b/package/riscv-coremark/riscv-coremark.hash @@ -0,0 +1,3 @@ +# Locally computed +sha256 0dde3285b9a1e6bb22bab82e485b5cf58a3e6a026d9cdc22102619a5584a6c7f riscv-coremark-6e1d72b864e45f67031ffaedb0b01b5d030d6d3c-br1.tar.gz +sha256 fc8a88c0c1b5a6bc7358f18c43cdd562c1bfd83b08d81a43f726917582c0e260 LICENSE 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 +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 +endef + +define RISCV_COREMARK_INSTALL_TARGET_CMDS + $(INSTALL) -D $(@D)/coremark/coremark.riscv $(TARGET_DIR)/usr/bin/coremark.riscv +endef