New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minimal spi master #207
base: main
Are you sure you want to change the base?
Minimal spi master #207
Conversation
To minimize the risk during rebase and squash I created a new branch and a new PR. |
I have verified both the builds with and without the SPI master. So far it looks good. |
54c1329
to
ae1433a
Compare
5c829a2
to
8c325ef
Compare
This PR should be merged after #208, since it is a risk here that cache fetches the wrong bitstream. |
@@ -58,6 +58,10 @@ module tb_tk1(); | |||
localparam ADDR_CPU_MON_FIRST = 8'h61; | |||
localparam ADDR_CPU_MON_LAST = 8'h62; | |||
|
|||
localparam ADDR_SPI_EN = 8'h80; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Här skulle vi nog också ifdef:a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the answer if we should use ifdef in the tb as well. One argument is that it is easier to have on build and make sure it all works. But on the other hand, if it is substantial difference and it is a risk that the test fails, yeah then it is better to have two.
But I think you will have to answer for what is the best approach here Joachim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having excess symbolic names should pose not problem. A linter may give warnings (This is not Go after all).
But ports in the DUT and associated wires should cause at least warnings during build of the som environment, and cause confusion. Testbench functionality such a state monitor, state display may also become problematic.
And most importantly. The test cases in a testbench should test the functionality of the DUT. This means that the DUT and the TB are in sync in terms of functionality. A self-checking test case that tests a functionality that don't exist in the DUT should fail.
To summarize. If we have functionality in the DUT (the core) that is compile time optional, we should also have the test functionality for that functionality compile time optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great. Can you add, test and verify it?
@@ -89,13 +93,24 @@ module tb_tk1(); | |||
wire tb_gpio3; | |||
wire tb_gpio4; | |||
|
|||
wire tb_spi_ss; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mer ifdef.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my response in the other comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
8c325ef
to
899a531
Compare
00e177a
to
fdea1bd
Compare
50fe870
to
fbd68eb
Compare
SPI control addresses were missing from tk1_mem.h. I added them. |
- NOTE: This is an optional feature, not built by default. Not included in the tk1 for sale at Tillitis shop. - This makes it possible to interface the SPI flash onboard TKey. - To include the SPI master in the build, use `make application_fpga.bin YOSYS_FLAG=-DINCLUDE_SPI_MASTER`. Signed-off-by: Joachim Strömbergson <joachim@assured.se>
fbd68eb
to
afa28ab
Compare
This supersedes #203
Rebased on main, and squashed.
Closes #213