lowRISC / lowrisc-nexys4

FPGA demo for Digilent NEXYS 4 board
Other
22 stars 15 forks source link

Rely on $XILINX_VIVADO env var to find vivado binary #5

Closed olofk closed 6 years ago

olofk commented 6 years ago

If there are several versions of Vivado installed, a user might set $XILINX_VIVADO to one installation, while having another in $PATH. This ensures only the one defined by $XILINX_VIVADO is used

asb commented 6 years ago

Looks good to me, thanks!

jrrk commented 6 years ago

@olofk Have you thought how you will handle data2mem which is part of the SDK and in a different path to vivado ? It also gets added to the path when you initialise the Xilinx tools and is needed when you make incremental updates to bare_metal software contents.

olofk commented 6 years ago

I just noticed the call to data2mem 5 minutes ago :)

You're right, this needs to be fixed as well. It's a bit of misunderstanding on my part really. I didn't realize that XILINX_VIVADO was set by the sourced Xilinx scripts. With ISE I always sourced the settings file, but since Vivado I have just started to call the vivado binary directly. Therefore I assumed that the XILINX_VIVADO env var was something LowRISC-specific and I set it manually.

If you instead source the settings file directly, my patch is not needed at all

I could supply another patch instead which tells the user explicitly that they need to source the settings file if it doesn't detect the XILINX_VIVADO env var. How about that?

asb commented 6 years ago

That sounds like a good alternative. I know I've had confusion before about setting XILINX_VIVADO vs sourcing their scripts.

olofk commented 6 years ago

Feel free to revert this patch if you like. Don't think it will cause any harm though. I'll cook a new one with better instructions in the meantime

asb commented 6 years ago

I've gone ahead and reverted. As you say there's no harm here, but there's no real reason to propagate this change to other branches and reverting it means we won't keep seeing the diff.

olofk commented 6 years ago

6 supersedes this PR