-
Couldn't load subscription status.
- Fork 0
put ignition rev2+ builds into play #426
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
Conversation
c129531 to
73fe012
Compare
73fe012 to
9876054
Compare
could we just get rid of this then? |
|
|
||
| cosmo_ignition_top_inst: entity work.cosmo_ignition_top | ||
| generic map( | ||
| IS_REV1 => true |
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.
Ah, I see we do need to keep that rev1 build around. I was thinking we had version resistors here.
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.
Yes given the changes, rev1 becomes a special case, and all future cosmos need a different image.
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.
Rather than comment throughout I'll just leave a top level suggestion. We should make the naming here explicit about which version of boards will be supported (we can debate rev numbers vs HCV letters). But I don't think having an _r1 build and a build without any suffixes makes it immediately clear what is supported. On other boards we've done things like add letters when the same image supports multiple versions, e.g., _abc.bit.
.github/workflows/build.yml
Outdated
| path: "./buck-out/v2/gen/root/**/*" | ||
| - name: Cleanup | ||
| run: bash .github/cleanup.sh | ||
| cosmo_ignition_r1_only: |
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.
lets make this cosmo_ignition_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'm ok with this and will fix tomorrow
| path: "./buck-out/v2/gen/root/**/*" | ||
| - name: Cleanup | ||
| run: bash .github/cleanup.sh | ||
| cosmo_ignition: |
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.
lets make this cosmo_ignition_b ?
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.
So I don't love the sustainability of this longer term. In a nicer world I would have noticed this incompatible hw change during review and we could have had one image to rule them all, but alas.
What I specifically dislike about this is that historical runs and filenames change when new hw lands and, while it is now explicit for people looking at that point and forward, going back in history and figuring out what is related to what becomes more painful. It also forces downstream sw flag days: facade has to pick up new file names etc and a simple rollback becomes more complicated etc.
I put more detailed comments in a thread above, but while I'm sympathetic to the goals here, I don't like this solution for the reasons enumerated there, but am open to chatting about alternatives that help meet these goals without ensuring future make-work for everyone. |
20af799 to
84c6f97
Compare
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.
We chatted and are going to have the fpga-releaser tool handle bitstream file renaming per which HCVs are applicable. This gives us the option to decouple that from the internal file naming structure of the design in quartz.
84c6f97 to
54b7c4c
Compare
This puts the changes in place for rev2+ cosmo ignition images. This also creates a new fpga build for rev1 which is essentially orphaned due to different pin config.
In building this, we had a timing failure in oximux 16 on the cosmo-hp build so I adjusted the 1hot detection logic to be more logic-efficient.