[dv,sw] Makefile refactor #230

sriyerg · 2019-09-20T07:02:08Z

added centrallized Makefile infra for maximizing reuse across SW
test, lib and boot_rom targets
existing Make flows still remain (for the timebeing; will remove it
once all approve of this)
all make commands are to be run from sw/ area
TODOs
add some documentation
fix support for external apps, eg coremark
find better way for adding .c -> .o and .c -> .ppo recipes
move boot_rom image gen and sw lib gen to SV build step so that it is not executed per test

sw/opts.mk

hw/dv/tools/rules.mk

moidx · 2019-09-20T08:40:24Z

Hi @sriyerg, for reference #201 is the Meson build system implementation we have been discussing.

sw/Makefile

sw/opts.mk

tjaychen · 2019-09-20T17:53:35Z

sw/opts.mk

+SW_BUILD_DIR  ?= $(SW_ROOT_DIR)/$(SW_DIR)
+
+LIB_NAME      ?= ot
+LIB_TARGET    ?= $(LIB_BUILD_DIR)/lib${LIB_NAME}.a


could we allow the individual lib*/srcs.mk to add to this variable? I'm envisioning in the future if we have multiple libs we need to create, it would be nice to only have to modify the sw/Makefile only for the new includes, and have everything else just work out.

Yes, any / all srcs.mk files can append more targets to LIB_TARGET to add more custom targets if needed. Might have to make some fixes to the rules though, but it could definitely work.

sgtm, probably not a high priority for now :) Maybe if the readme just has a comment on how this could be done that would be more than enough.

tjaychen · 2019-09-20T17:56:34Z

hw/dv/tools/Makefile

+
+# SW_OPTS are options for SW apps that come with their own Makefile (example coremark)
+CL_SW_OPTS        +=
+SW_OPTS           += ${CL_SW_OPTS}


is this one consumed by the sw_build step?

SW_OPTS are Makeflags for external apps such as coremark which have its own Makefile. SW_FLAGS are compiler flags we can pass to our own local sw. Both are consumed by sw_build step.

ah that makes sense. but i don't see SW_OPTS used anywhere though.
Speaking of which, the coremark case doesn't seem to quite work right now (at least in my trial). Looks like the LIB_DIR variable isn't passing on correctly, it could just be my environment. I haven't looked too closely.

I won't comment too much more on this for now, i'll try to digest the meson usage, since both you and Miguel say that's better.

I fixed the hw/dv/tools/rules.mk to set Makeflags to SW_OPTS, so it should now work.

tjaychen · 2019-09-20T17:59:22Z

sw/rules.mk

+# compile sw sources
+# TOOD: figure out a way to 'templatise' .o/.ppo ruleset for each dir containing srcs
+
+$(SW_BUILD_DIR)/%.o: $(SW_DIR)/$$*.c


for this one, i think @eunchan does something like it in his private repo makefile where he assumes there are multiple 'tops'.

Might be good to reference that.

sorry not multiple tops, multiple IPs.

tjaychen · 2019-09-20T18:03:22Z

ah sounds good.

…

On Fri, Sep 20, 2019 at 10:57 AM sriyerg ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sw/Makefile <#230 (comment)>: > @@ -0,0 +1,21 @@ +# Copyright lowRISC contributors. +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +# Generate a baremetal application for the microcontroller +SW_ROOT_DIR := $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST)))) +SW_DIR ?= + +# sources +LOCAL_SW ?= $(shell test -e ${SW_DIR}/srcs.mk && echo -n 1) + +ifeq ($(LOCAL_SW), 1) Yeah, if LOCAL_SW is 0, then its an external all (such as coremark which ships with its own Make flow. ). Hi @sriyerg <https://github.com/sriyerg>, for reference #201 <#201> is the Meson build system implementation we have been discussing. I think the meson flow is much better. I am working on trying it out. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#230?email_source=notifications&email_token=AAH2RSUXBJEVSZFISQCM4CTQKUFJ3A5CNFSM4IYTP652YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFORLZI#discussion_r326740215>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAH2RSXTFVO4TN2IC4RSRH3QKUFJ3ANCNFSM4IYTP65Q> .

tjaychen · 2019-09-23T16:42:05Z

this LGTM overall, but I guess you're still working on adding more things?

Let me know if it's ready for final approval and you want to finish the rest off in a different PR.

tjaychen

LGTM

@moidx
Miguel could you scan through quickly? I know we may change this to meson later, but just wanted to make sure there isn't anything that really stands out to you.

@sriyerg
I just realized we'll probably need to follow on with another PR quickly to change all the documented instructions, since right now they all go to the actual directory (for example boot_rom) to build.

Could you actually file an issue on that and maybe have it as a P0 or P1 so we don't forget?

sriyerg · 2019-09-23T20:43:41Z

LGTM

@moidx
Miguel could you scan through quickly? I know we may change this to meson later, but just wanted to make sure there isn't anything that really stands out to you.

@sriyerg
I just realized we'll probably need to follow on with another PR quickly to change all the documented instructions, since right now they all go to the actual directory (for example boot_rom) to build.

Could you actually file an issue on that and maybe have it as a P0 or P1 so we don't forget?

Filed issue #244 for updating the SW commands in documentation. I will work on it once this PR is merged.

moidx

Added a couple fo nit comments. Also, can you squash some of the changes or modify the commit descriptions so that they have enough details once submitted? Squash and merge is not supported in this repo.

Also, It'd be nice if you remove the old Makefiles after updating the documentation.

Thanks!

moidx · 2019-09-24T00:56:14Z

sw/tests/hmac/sha256_test.c

@@ -7,6 +7,7 @@

 #include "common.h"
 #include "flash_ctrl.h"
+#include "flash_ctrl_regs.h"


This is fixed in #228. Can you rebase and revert this change?

Done, thanks.

moidx · 2019-09-24T00:59:10Z

sw/rules.mk

+# SPDX-License-Identifier: Apache-2.0
+#
+##########################################################################
+## The following variables must be set prior to invoking this Makefile  ##


Are these comments still accurate? If not, can you remove it and add a comment describing what should go here?

Updated, thanks.

tjaychen · 2019-09-24T07:09:21Z

i actually thought Sri removed all the old makefiles already. It was one of the things we discussed earlier today.

moidx · 2019-09-24T07:32:43Z

i actually thought Sri removed all the old makefiles already. It was one of the things we discussed earlier today.

The deletion was done in a separate commit within this PR. My suggestion to delete later is based on the fact that after we merge these changes the documentation will be broken. It is always preferred to keep the repo in green state at the commit level, so that it is easier to maintain the history.

It is not a big deal right now, but we should start thinking about staging things this way to avoid running into maintenance issues later.

tjaychen · 2019-09-24T07:53:47Z

ah yeah, Sri and I talked about that also. He actually opened an issue already to follow on with another PR to fix the documentation. I mostly favored removing the old makefiles because it would probably cause quite a bit confusion having them both there.

[sw] Makefile refactor - added centrallized Makefile infra for maximizing reuse across SW test, lib and boot_rom targets - existing Make flows still remain (for the timebeing; will remove it once all approve of this) - all make commands are to be run from sw/ area - updated with chip_info generation - TODOs - add some documentation - need to fix support for external apps, eg coremark [dv,sw] coremark port now works - updates to get external app (coremark) compiling with the refactored flow - added ability to compile targets with external Makefiles with STANDALONE_SW var - added sw/benchmarks/srcs.mk to supply STANDALONE_CMD to build custom app [dv,sw] removed existing Makefiles (deprecated)

sriyerg

LGTM

@moidx
Miguel could you scan through quickly? I know we may change this to meson later, but just wanted to make sure there isn't anything that really stands out to you.

@sriyerg
I just realized we'll probably need to follow on with another PR quickly to change all the documented instructions, since right now they all go to the actual directory (for example boot_rom) to build.

Could you actually file an issue on that and maybe have it as a P0 or P1 so we don't forget?

I work on doc as a part of another PR - I am dependent on this one for msging flow updates.

sriyerg · 2019-09-24T22:45:02Z

sw/rules.mk

+# SPDX-License-Identifier: Apache-2.0
+#
+##########################################################################
+## The following variables must be set prior to invoking this Makefile  ##


Updated, thanks.

moidx

LGTM. Thanks for working on this @sriyerg

sriyerg requested review from gkelly, tjaychen and moidx September 20, 2019 07:02

sriyerg changed the title ~~[dv,sw] sw Makefile refactor~~ [dv,sw] Makefile refactor Sep 20, 2019

sriyerg commented Sep 20, 2019

View reviewed changes

sw/opts.mk Outdated Show resolved Hide resolved

sriyerg commented Sep 20, 2019

View reviewed changes

hw/dv/tools/rules.mk Outdated Show resolved Hide resolved

tjaychen reviewed Sep 20, 2019

View reviewed changes

sw/Makefile Outdated Show resolved Hide resolved

tjaychen reviewed Sep 20, 2019

View reviewed changes

sw/opts.mk Outdated Show resolved Hide resolved

tjaychen reviewed Sep 20, 2019

View reviewed changes

sriyerg mentioned this pull request Sep 23, 2019

[CI] Add SW build using make targets #229

Merged

tjaychen approved these changes Sep 23, 2019

View reviewed changes

sriyerg mentioned this pull request Sep 23, 2019

[sw, doc] Update documentation to reflect the updated sw flow (PR #230) #244

Closed

moidx requested changes Sep 24, 2019

View reviewed changes

sriyerg mentioned this pull request Sep 24, 2019

[dv,sw] C messaging flow: part 1 (C side) #259

Closed

sriyerg commented Sep 25, 2019

View reviewed changes

sriyerg requested a review from moidx September 25, 2019 01:37

moidx approved these changes Sep 25, 2019

View reviewed changes

sriyerg merged commit 789c27c into lowRISC:master Sep 25, 2019

sriyerg deleted the top_dv branch September 25, 2019 03:42

imphil mentioned this pull request Oct 30, 2019

Pass discovered modules to fusesoc generators #79

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dv,sw] Makefile refactor #230

[dv,sw] Makefile refactor #230

sriyerg commented Sep 20, 2019 •

edited

Loading

moidx commented Sep 20, 2019

tjaychen Sep 20, 2019

sriyerg Sep 20, 2019

tjaychen Sep 23, 2019

tjaychen Sep 20, 2019

sriyerg Sep 20, 2019

tjaychen Sep 20, 2019

sriyerg Sep 21, 2019

tjaychen Sep 23, 2019

tjaychen Sep 20, 2019

tjaychen Sep 20, 2019

tjaychen commented Sep 20, 2019 via email

tjaychen commented Sep 23, 2019

tjaychen left a comment

sriyerg commented Sep 23, 2019

moidx left a comment

moidx Sep 24, 2019

sriyerg Sep 25, 2019

moidx Sep 24, 2019

sriyerg Sep 24, 2019

tjaychen commented Sep 24, 2019

moidx commented Sep 24, 2019

tjaychen commented Sep 24, 2019

sriyerg left a comment

sriyerg Sep 24, 2019

moidx left a comment

[dv,sw] Makefile refactor #230

[dv,sw] Makefile refactor #230

Conversation

sriyerg commented Sep 20, 2019 • edited Loading

moidx commented Sep 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjaychen commented Sep 20, 2019 via email

tjaychen commented Sep 23, 2019

tjaychen left a comment

Choose a reason for hiding this comment

sriyerg commented Sep 23, 2019

moidx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjaychen commented Sep 24, 2019

moidx commented Sep 24, 2019

tjaychen commented Sep 24, 2019

sriyerg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moidx left a comment

Choose a reason for hiding this comment

sriyerg commented Sep 20, 2019 •

edited

Loading