search

pulp-platform / pulpissimo

This is the top-level project for the PULPissimo Platform. It instantiates a PULPissimo open-source system with a PULP SoC domain, but no cluster.
Other
376 stars 163 forks source link

Quartus support #78

Closed MarekPikula closed 1 year ago

MarekPikula commented 5 years ago

I'm working on making the project's codebase compatible with Intel/Altera Quartus for synthesis. Although Intel claims support for SystemVerilog-2009 it's not entirely true (I'm currently documenting all the stuff I've stumbled upon [here](https://marekpikula.github.io/quartus-sv-gotchas/Intel Quartus SystemVerilog gotchas.html)).

I'm preparing patches, which make the code synthesizing correctly. So far without any automated scripts for project creation and bitstream generation.

I'd like to ask if the project is interested in such patches.

EDIT:

FYI for now I evaluated somewhat pruned PULPissimo with Ibex and without RI5CY, FPU, uDMA and HWPE, so patches I'll provide for now will make PULPissimo work only in this configuration until I find some time to check other cores.

Here's the list of cores with modifications and appropriate PRs and indication if patches were merged:

bluewww commented 5 years ago

I just looked through your document and the changes are unfortunately incompatible with the styleguide we use. Using sv-2009 instead of sv-2012 or sv-2017 is fine, but the stuff you point out in your document shows that quartus' sv support is horribly broken.

I think using if else instead of unique case is a no-go since they are not equivalent. Something with case plus //parallel_case would be needed. More information can be found here.

While I appreciate your effort its unlikely that we can accept your patches into master.

What might be possible is that you can put your work into a branch (your patched work with the helpful guiding documents) which we can refer to in the README.md in master.

MarekPikula commented 5 years ago

Sure, to be honest that's the response I expected :wink: I'll prepare all the necessary patches and you'll see which changes can go to master and which not.

Which unique case issue are you referring to? (because there are multiple)

bluewww commented 5 years ago

I was referring to the unique case example in your document

MarekPikula commented 5 years ago

You mean "12.5.2 Constant expression in case statement"? There are also "case defaults sometimes not working" and "unique case inside not supported".

MarekPikula commented 5 years ago

According to this thread on SO it is equivalent as constant expression cases are to be evaluated in order of statements.

bluewww commented 5 years ago

Indeed I'm referring to the example in 12.5.2 Constant expression in case statement. Sorry for not making this clear.

bluewww commented 5 years ago

There is a unique infront of the case, meaning all the statements can/are evaluated in parallel. This is not equivalent to evaluation in order of statements for all cases.

MarekPikula commented 5 years ago

In provided example it's clear that one of the statements must "win", since they write to the same wire. In this case this statement must have some sort of priority context. I will see how these two versions differ in terms of RTL in Vivado shortly.

MarekPikula commented 5 years ago

To my surprise it's not evaluated in the same way, although resulting logic is pretty much the same (in this case). I agree though that in other cases it might not be true.

Here is with case: constant-case-1

Here is with if ... else: constant-case-equivalent

FYI Quartus evaluates if ... else in the same way.

MarekPikula commented 5 years ago

Which branch should I base on for apb_gpio? pulp_soc/ips_list.yml point to detached commit.

bluewww commented 5 years ago

Which branch should I base on for apb_gpio? pulp_soc/ips_list.yml point to detached commit.

Ugh, that's not good. Ideally I think we first point to pulpissimo to the apb_gpio (if still compatible)

MarekPikula commented 5 years ago

What do you mean? In this specific context both results are equivalent taking into account logic that uses exception_pc.

I'm not sure I understand the second part of your comment. What compatibility are you referring to?

bluewww commented 5 years ago

To my surprise it's not evaluated in the same way, although resulting logic is pretty much the same (in this case). I agree though that in other cases it might not be true.

Here is with case: [...]

Here is with if ... else: [...]

FYI Quartus evaluates if ... else in the same way.

This looks fine to me I was not complaining about the results. First and foremost both formulations are obviously logically equivalent, so simulation results are not impacted.

What I was getting at is that with unique case you force tools to acknowledge that you want parallel logic or otherwise it has to warn you (e.g. when statements logically overlap during simulation). An if else or case statement implies a priority encoding, which sometimes tools detect to be able to turn into parallel logic (for synthesis). If you are sure that the your statements can be parallel you can force some tool with //parallel_case to regards them as such.

MarekPikula commented 5 years ago

Ah, I see your point. Later I'll experiment with different approaches and we'll see if I'll come with some universal solution.

MarekPikula commented 5 years ago

I updated the description with list of cores requiring changes in my configuration. Are you @bluewww one of maintainers in all of them? I'll send patches in the next few weeks and then we'll decide which changes can go upstream and which should land in separate branch.

MarekPikula commented 5 years ago

Could you @bluewww indicate against which branches should I prepare my patches?

MarekPikula commented 5 years ago

That's all for today. Other changes are either significant or I don't know which branch should I base on.

MarekPikula commented 4 years ago

Would you @bluewww consider bumping tech_cells_generic to master? I can see that master is much more polished than current version and for example doesn't introduce duplicate pulp_clock_mux2, what is a problem for Quartus.

MarekPikula commented 4 years ago

And please point out what branch I should PR against for the rest of cores.

bluewww commented 4 years ago

@MarekPikula Currently this PR is in progress for pulp_soc (and PULPissimo) that updates all IPs: https://github.com/pulp-platform/pulp_soc/pull/12. Currently still being tested.

You can see the cores/IPs that will be used soon in https://github.com/pulp-platform/pulp_soc/pull/12 so ideally PRs would target that version

MarekPikula commented 4 years ago

What about pulp-platform/apb_gpio? Should I target 64bit branch?

bluewww commented 4 years ago

PULPissimo is still using a quite old version of apb_gpio. The update will either target master or 64bit but I have to figure that out yet.

MarekPikula commented 4 years ago

Actually PULPissimo uses pulp-platform/apb_gpio@730a9204dbb0d7057f45ef833d0a6e868c46107b, which is from 64bit branch.

image

bluewww commented 4 years ago

Oh I see. I'm currently in touch with the guy who was doing development on it.

bluewww commented 4 years ago

We will be targeting the 64bit branch on apb_gpio but it will be merged to master anyway. I pinged the maintainers to cleanup the branch and merge it.

MarekPikula commented 4 years ago

I figured that axi_node is not instantiated in PULPissimo, so I won't bother preparing patch, since I have no means and time resources to test it.

MarekPikula commented 4 years ago

I have a question about common_cells. It's one of those breaking changes, so it will land in separate branch, since Quartus doesn't support type parameters used by various modules (including CDC used by riscv-dbg).

Since common_cells are used in various other projects, PULPissimo isn't using all modules present in repo. Should I make changes to all modules, or only to those used by PULPissimo?

bluewww commented 4 years ago

I would start off with just the ones referred to by PULPissimo. I don't know how far you want to expand your quartus project.

MarekPikula commented 4 years ago

My assignment was to evaluate just the PULPissimo on Cyclone V for comparison to previous core and I probably won't evaluate other projects from PULP Platform any time soon.

MarekPikula commented 4 years ago

I guess with my commits and documentation, if anyone will want to port other projects to work with Quartus, will know what to do.

MarekPikula commented 4 years ago

Could you create a quartus branch in common_cells based on v1.13.1 flag? I'd like to push a PR.

bluewww commented 4 years ago

Done https://github.com/pulp-platform/common_cells/tree/quartus

MarekPikula commented 4 years ago

I forgot to mention that constant expression case was a false assumption as pointed in lowRISC/ibex#336 and is no issue for Quartus.

MarekPikula commented 1 year ago

Hi @bluewww, sorry to dig up an old issue. I was cleaning up my GitHub PR list and noticed all those unmerged Quartus compatibility changes. Please let me know if they are even considered to be merged or if I should simply close all those PRs. I'm no longer interested in these changes, as I don't do any active development on Intel FPGAs.

bluewww commented 1 year ago

Hi @MarekPikula . Unfortunately, we don't have the developer bandwidth (and actually also the hardware and tooling) to get your stuff merged. There have been some less intrusive changes which we merged but if there are no real users then we won't be able tell if we are breaking things for quartus anyway.

I suggested you close your PRs.