Adding the possibility of indicate external sources throw a enviromen…

[zipotron]

…t variable, useful when NeoRV32 is called as a core of bigger project.
Example of use:
https://github.com/zipotron/neorv32-complex-setups/blob/master/synthetize.sh

[umarcor]

This should not be necessary, because you can already override those variables when calling make.

[zipotron]

Thats true, but look how looks like:

make -C neorv32/setups/osflow BOARD=ULX3S Default NEORV32_MEM_SRC="../../rtl/core/mem/neorv32_imem.default.vhd ../../rtl/core/mem/neorv32_dmem.default.vhd ../../rtl/hardware_dependent/ulx3s/ulx3s_sdram.vhd" NEORV32_VERILOG_SRC="../../rtl/hardware_dependent/ulx3s/ecp5pll.sv"

And you have to check what is already added in the makefile because in not incremental, I mean this part:

"../../rtl/core/mem/neorv32_imem.default.vhd ../../rtl/core/mem/neorv32_dmem.default.vhd"

and if one of those files change, every repo that is using neorv32 as a core will be broken.
I just suggest a way to easy add extra source for certain platform, externally. Is just an idea, maybe there is others better solutions.

[zipotron]

And.... Is not being overrided anyway! Look:

make -C neorv32/setups/osflow BOARD=ULX3S Default NEORV32_MEM_SRC="../../rtl/core/mem/neorv32_imem.default.vhd ../../rtl/core/mem/neorv32_dmem.default.vhd ../../rtl/hardware_dependent/ulx3s/xxxxxx.vhd" NEORV32_VERILOG_SRC="../../rtl/hardware_dependent/ulx3s/xxxxx.sv"

This should fails and is not failing, this means in not getting overrided by the given value.

and this:

make -C neorv32/setups/osflow BOARD=ULX3S Default ULX3S_VHDL=../../../hardware_dependent/ulx3s/xxxxxxx.vhd ULX3S_VERILOG=../../../hardware_dependent/ulx3s/xxxxxxx.sv

Yes, this fails as is expected.

[umarcor]

Thats true, but look how looks like:

make -C neorv32/setups/osflow BOARD=ULX3S Default NEORV32_MEM_SRC="../../rtl/core/mem/neorv32_imem.default.vhd ../../rtl/core/mem/neorv32_dmem.default.vhd ../../rtl/hardware_dependent/ulx3s/ulx3s_sdram.vhd" NEORV32_VERILOG_SRC="../../rtl/hardware_dependent/ulx3s/ecp5pll.sv"

Might be rewritten to:

MEM_SRC='../../rtl/core/mem/neorv32_imem.default.vhd ../../rtl/core/mem/neorv32_dmem.default.vhd'
make -C neorv32/setups/osflow \
  BOARD=ULX3S \
  Default \
  NEORV32_MEM_SRC="$MEM_SRC ../../rtl/hardware_dependent/ulx3s/ulx3s_sdram.vhd" \
  NEORV32_VERILOG_SRC="../../rtl/hardware_dependent/ulx3s/ecp5pll.sv"

Not much difference compared to:

make -C neorv32/setups/osflow \
  BOARD=ULX3S \
  Default \
  ULX3S_VHDL=../../../hardware_dependent/ulx3s/ulx3s_sdram.vhd \
  ULX3S_VERILOG=../../../hardware_dependent/ulx3s/ecp5pll.sv`

And you have to check what is already added in the makefile because in not incremental, I mean this part:

"../../rtl/core/mem/neorv32_imem.default.vhd ../../rtl/core/mem/neorv32_dmem.default.vhd"

That is a legit concern. We have three use cases:

• Keep the default memory sources.
• Override the default memory sources.
• Extend memory sources.

The last one is not supported at the moment, and we might enhance the makefiles in order to do it. However, you added ULX3S_VHDL, which implies you are not addressing the problem from NEORV32's perspective, but from the point of view of an specific external project.

There are several places where extra sources might be added. See:

neorv32/setups/osflow/filesets.mk

Line 51 in d072a12

NEORV32_SRC := ${NEORV32_PKG} ${NEORV32_APP_SRC} ${NEORV32_MEM_ENTITIES} ${NEORV32_MEM_SRC} ${NEORV32_CORE_SRC}

Your suggestion is to add content between ${NEORV32_MEM_SRC} and ${NEORV32_CORE_SRC}. A sensible name might be ${NEORV32_MEM_SRC_EXTRA}. At the same time ${NEORV32_CORE_SRC_EXTRA} might be a sensible name to be put at the end.

By the same token, NEORV32_VERILOG_SRC_EXTRA might be added. Currently, NEORV32_VERILOG_SRC is used twice in https://github.com/stnolting/neorv32/blob/master/setups/osflow/synthesis.mk. So, maybe NEORV32_VERILOG_ALL := ${NEORV32_VERILOG_SRC} ${NEORV32_VERILOG_SRC_EXTRA} needs to be added in filesets.mk.

[umarcor]

And.... Is not being overrided anyway!

See #214.

[zipotron]

See #214.

Those changes seams solve the issue, I didn't test yet, but looks nice. Thanks!

[umarcor]

@zipotron, note that those changes solve the "overriding" issue. But I did not implement the *_EXTRA variables we discussed above for "extending" instead of "overriding". So, feel free to reopen this by renaming the envvars in the proposal.

[zipotron]

@umarcor , Then, I suggest to leave this new environment variables for *EXTRA purposes, plus your changes. Do you need anything else for make this PR mergible?

[zipotron]

@umarcor , I merged with master, I leaved this line:

82 NEORV32_VERILOG_SRC="$(ULX3S_VERILOG)" \

As it is, I know that is redundant, but, I think that keeps the style

[umarcor]

@zipotron, see #213 (comment).

[zipotron]

@umarcor Apologize, I readed that comment few days ago, plus too much work today, and completely forgot. I guess now is more in the topic line

Mmm, I can see that Jenkins report a issue regarding Mixed language... lets me check

[zipotron]

@umarcor I guess is ready for merge

[zipotron]

Hello @stnolting @umarcor , I also added to the PR in the list of "external" setups my external setup repo. Please, whenever you can review and lets me know if is missing anything to do for do this PR ready to merge. I would like to have this changes in "master" branch, because this will make my setup compile easier for new users (now is needed to go to the neorv32 linked project and checkout a specific branch).
Thank you very much.

[umarcor]

Apart from a minor change (see below), LGTM!

[zipotron]

@umarcor Not good modification, rollback!

[zipotron]

I think is ready for merge

[umarcor]

LGTM!