Closed CyrilKoe closed 9 months ago
@CyrilKoe, thanks for the PR, I will have a look at it ASAP (note: you have draft
in the title, but the PR is opened in 'ready for review' state: could you convert it as draft PR in case it is not ready please?)
Second: an open PR #237 is already prepared to add documentation (@anga93 will handle it soon). Could you please move your documentation there, since it is orthogonal to the technical changes of this PR, which will be merged first?
@alex96295 Docs were removed, I will remove the draft after CI finishes
Ready to be reviewed @alex96295
@CyrilKoe thanks for the great work
I went through it and only have minor comments that you can find in this PR. Before we merge, I also have an observation. I think the current directory structure inside the target/
folder could be improved:
vanilla
and bd
are not currently reflected in the directories names; I suggested as an alternative to rename xilinx/
in xilinx_vanilla/
and then add both xilinx_vanilla/
and xilinx_bd/
under a xilinx/
directory within target/
(see suggested tree below)xilinx/
directory could be removed, or moved to the top-level xilinx/
folder if you apply the suggestions in the step above, pointing to the documentationips/
folder where the ooc designs are kept, for both favorsHere follows a tree with what I had in mind for the directory structure (plus comments marked with #
)
target
├── synth
│ └── carfield_synth_wrap.sv
└── xilinx # wrap xilinx_bd/ and xilinx_vanilla in xilinx top directory
├── xilinx_bd
│ ├── constraints # keep ooc carfield constraints at top since they are typically reused
│ │ ├── carfield_ip.xdc
│ │ ├── ooc_carfield_ip.xdc
│ │ ├── vcu128.xdc
│ │ └── zcu102.xdc
│ ├── ips # add carfield in a ips/ folder as ooc IP
│ │ └── xlnx_carfield
│ │ ├── scripts
│ │ │ └── run.tcl # script is renamed run.tcl since the tree structure makes you understand where you are already (i.e., implementing carfield ooc)
│ │ └── src
│ │ ├── carfield_xilinx_ip.v
│ │ └── carfield_xilinx.sv
│ ├── scripts
│ │ ├── carfield_bd_vcu128.tcl
│ │ └── run.tcl
│ └── xilinx_bd.mk
└── xilinx_vanilla
├── constraints
│ ├── carfield.xdc
│ ├── vcu128.xdc
│ └── zcu102.xdc
├── ips # renamed 'xilinx/' to 'ips/' as discussed above
│ ├── common.mk
│ ├── xlnx_clk_wiz
│ │ ├── Makefile
│ │ └── tcl
│ │ └── run.tcl
│ ├── xlnx_mig_ddr4
│ │ ├── Makefile
│ │ └── tcl
│ │ └── run.tcl
│ └── xlnx_vio
│ ├── Makefile
│ └── tcl
│ └── run.tcl
├── scripts
│ ├── flash_spi.tcl
│ ├── overrides.sh
│ ├── program.tcl
│ ├── prologue.tcl
│ ├── run.tcl
│ └── write_cfgmem.tcl
├── src
│ ├── carfield_top_xilinx.sv
│ ├── cdc_dst_axi_err.sv
│ ├── dram_wrapper.sv
│ ├── fan_ctrl.sv
│ ├── overrides
│ │ └── tc_clk_xilinx.sv
│ └── phy_definitions.svh
└── xilinx_vanilla.mk
Could you please have a look at it and provide your opinion? :)
@alex96295 thanks for the review and the feedback, about the directory structure here are the reasons motivating it:
car-xil-all
), I needed to have a common xilinx.mk
that includes both flavoured makefiles before describing the common rules at the end (to allow using flavor-dependant variables as target dependencies for these rules). It could be that xilinx_vanilla.mk
includes the other flavors but it would make it long and hard to read :xilinx_ips
and this is why I consider them as a "common" more than a "vanilla" feature. The IP config file would then contain if FLAVOR==vanilla_2
if reconfiguration is needed. But appart from run.tcl
everything is common and should not be associated with vanilla only to avoid duplication.This is why I think the top level Xilinx directory, is necessary and should contain common scripts (flash
; program
; overrides
; write_cgmem
); common srcs (like the axi_cdc_err_slave
it should maybe even be added helow /hw
at the root of the repository) and the common xilinx.mk
. But:
flavor_vanilla
and flavor_bd
could be renamed in xilinx_flavor_vanilla
and xilinx_flavor_bd
. For more clarity if one do not care about the fpga flow.user_ips
folder can be created below the flavor_bd
to split constraints and scripts between block design related and carfield ip related. I would recommend keeping it as user_ips
as the goal is quite different from xilinx_ip
. (Note: why having carfield_ip.xdc one directory above in your tree?)users_ip
could even be actually at the root as it would be a common between different flavor of block designs, but this may be exagerated in the current state...carfield.xdc
could be a common by taking a variable to override each path with a given prefix.carfield_soc.sv
)Precision, in my view new flavors should be added like this:
├── xilinx
│ ├── scripts
│ ├── constraints
│ ├── src
│ ├── xilinx_flavor_vanilla
│ ├── xilinx_flavor_vanilla_generated
│ ├── xilinx_flavor_bd_ethernet
│ ├── xilinx_flavor_bd_pcie_device
│ ├── xilinx.mk
Of course these may not be relevant for Carfield but may be for future SoCs.
@alex96295 thanks for the review and the feedback, about the directory structure here are the reasons motivating it:
* **Makefiles:** In order to have a normalized flow and reduce phonies (to only `car-xil-all`), I needed to have a common `xilinx.mk` that includes both flavoured makefiles before describing the common rules **at the end** (to allow using flavor-dependant variables as target dependencies for these rules). It could be that `xilinx_vanilla.mk` includes the other flavors but it would make it long and hard to read : "Common variables, Vanilla variables, Vanilla targets, Incude other flavors, Flavor dependant Variables, Common targets" * **Future flavors:** If a contributor decides to add a new flavor (let's imagine a template-generated RTL top level - as Occamy - that would be very different from the vanilla flavor while still not relying on a block design). It would also require `xilinx_ips` and this is why I consider them as a "common" more than a "vanilla" feature. The IP config file would then contain `if FLAVOR==vanilla_2` if reconfiguration is needed. But appart from `run.tcl` everything is common and should not be associated with vanilla only to avoid duplication. * **Extension to other projects:** one can copy this emulation flow in a new project without the vanilla flavor if they only wish to implement a block design (which is easier in most of the cases, and also the Vivado legit way to do). Vanilla directory should not be required.
This is why I think the top level Xilinx directory, is necessary and should contain common scripts (
flash
;program
;overrides
;write_cgmem
); common srcs (like theaxi_cdc_err_slave
it should maybe even be added helow/hw
at the root of the repository) and the commonxilinx.mk
. But:* `flavor_vanilla` and `flavor_bd` could be renamed in `xilinx_flavor_vanilla` and `xilinx_flavor_bd`. For more clarity if one do not care about the fpga flow. * A `user_ips` folder can be created below the `flavor_bd` to split constraints and scripts between block design related and carfield ip related. I would recommend keeping it as `user_ips` as the goal is quite different from `xilinx_ip`. (_Note:_ why having carfield_ip.xdc one directory above in your tree?) _Improvement:_ `users_ip` could even be actually at the root as it would be a common between different flavor of block designs, but this may be exagerated in the current state... * _Improvement:_ `carfield.xdc` could be a common by taking a variable to override each path with a given prefix. * The readme could be removed as there is the doc. As any flavor will share anyways the same `carfield_soc.sv`)
Precision, in my view new flavors should be added like this:
├── xilinx │ ├── scripts │ ├── constraints │ ├── src │ ├── xilinx_flavor_vanilla │ ├── xilinx_flavor_vanilla_generated │ ├── xilinx_flavor_bd_ethernet │ ├── xilinx_flavor_bd_pcie_device │ ├── xilinx.mk
Of course these may not be relevant for Carfield but may be for future SoCs.
Yes, so first of all my apologies because I based my tree
on ck/xilinx_bd
instead of ck/xilinx_bd_wip
where you addressed some of the points I mentioned already. I agree we should try to maximize common code, I see in your branch you already did it.
I agree with calling ooc IPs from user user_ips
, and if it is possible for you to implement it in this PR, that would be great. I left carfield_ip.xdc
at the top in my tree because I thought that the constraints need to be re-included in the top level BD anyway from what I could recall, maybe it's not the case for this flow and we can keep it at user_ip
level
I don't think that renaming the flavor_*
directory is required as you are already inside the xilinx
folder, so no need to prefix it again
For the carfield.xdc and override: up to you if you have time to do it now. I was noticing some overlapping between constraints and IMHO we should try to minimize those as much as possible
TL;DR: action items
user_ips
and xilinx_ips
foldersIf you could address those above, then we are ready to merge for me :)
After reconsidering; carfield_ip
could be included in xilinx_ips
, but it would be easier then to change the xilinx_ips
into a .mk
solution rather than an independant Makefile
system (what @paulsc96 recommended on Cheshire).
Like this I could access the common bender targets and other useful variables to buile the carfield ip.
I don't have time right now; but i'll do soonish it if you think it works.
After reconsidering;
carfield_ip
could be included inxilinx_ips
, but it would be easier then to change thexilinx_ips
into a.mk
solution rather than an independantMakefile
system (what @paulsc96 recommended on Cheshire). Like this I could access the common bender targets and other useful variables to buile the carfield ip.I don't have time right now; but i'll do soonish it if you think it works.
of course, that works: using a makefrag is recommended. For what concerns adding it to xilinx_ips
, we said that's a kind of common place for the various flows, while carfield ooc is just for bd design. So maybe a standalone folder with its own makefrag would help. I liked the user_ips
idea because they are two logically separated things
This won't prevent changing the xilinx_ips in a makefrag solution as well
@alex96295 switched to makefrag for xilinx_ips, as the resulting makefile is the most complex, I prefered to put also carfield_ip under it to not reproduce the complexity. Pipeline passed with fpga implem
I see, thanks :) Just one thing, what do you mean by:
to not reproduce the complexity
Just that xilinx_ips.mk is pretty complex, so having another complex user_ips.mk would be a lot. And it makes sense that carfield_ip is compiled with the same makefile as the outputs are exactly the same (ip.xci)
Just that xilinx_ips.mk is pretty complex, so having another complex user_ips.mk would be a lot. And it makes sense that carfield_ip is compiled with the same makefile as the outputs are exactly the same (ip.xci)
Yes, I agree
If fpga mapping passes on CI, I guess that a rebase on main will be the last step, and then we can merge :)
The ethernet IP is quite complex and cannot be implemented by hand on FPGA, i.e. by instantiating Xilinx IP within the source files in System Verilog.
To solve this issue, we add two bitstream flavors:
Summary of changes