Skip to content

Snippets for AXI-Stream signals and generics#41

Closed
valerionew wants to merge 2 commits intoVHDL-LS:masterfrom
valerionew:patch-1
Closed

Snippets for AXI-Stream signals and generics#41
valerionew wants to merge 2 commits intoVHDL-LS:masterfrom
valerionew:patch-1

Conversation

@valerionew
Copy link
Copy Markdown
Contributor

No description provided.

@GlenNicholls
Copy link
Copy Markdown
Collaborator

This makes assumptions about the signal port names that there would only ever be 1 master and 1 slave in a port map which isn’t always the case. Personally, I don’t like the idea of providing this because 1. The full AXI-S signal list is provided, 2. It forces user to have all caps for each port signal, 3. The names are not changeable by the user, 4. The generic width name is not changeable by the user.

IMO this doesn’t belong in the tool itself and should instead be a personal snippet.

@valerionew
Copy link
Copy Markdown
Contributor Author

While I see your points and i admit that this particular version of the snippet may be a bit too personal, i think there is room for improvement here towards a more general version of these snippets. As long as having many snippets does not interfere with the correct functioning of the software, why not adding more, as long as they are significant

@GlenNicholls
Copy link
Copy Markdown
Collaborator

I agree with your point, AXI is common enough that I am okay with having helper snippets for it. However, I would like them to be general enough to support most use cases. I.e., the following things should be considered:

  • Full AXI-S signal set including sideband
  • Intuitive way to set width, e.g. calculations inline where applicable
  • User-defined names
  • Option to instance ports and signals

I'm not sure if I'm missing anything obvious.

@GlenNicholls
Copy link
Copy Markdown
Collaborator

GlenNicholls commented Jun 24, 2021

Also, per your commit about making tab spacing consistent, I would prefer not to align anything for the declaration. Some users use tabs, some use spaces and VSC doesn't convert \t in snippets to spaces when it detects tabs using spaces. Not only this, but not everyone aligns ports on colons, direction, etc. I would prefer to leave it up to the user to align their ports/signals whether that be manually or another command/extension that does this.

@Bochlin
Copy link
Copy Markdown
Member

Bochlin commented Jul 18, 2021

I think that having AXI snippets (as well as snippets for other industry standard busses) would be a very good addition. Some comments though:

I think that aligning could be done as long as it is done using spaces, e.g:

s_axi_awaddr  : in  std_ulogic_vector(11 downto 0);
s_axi_awready : out std_ulogic;

With regards to coding style, there is also the matter of the usage of upper/lower case. While I personally avoid using UPPER_CASE for names except for generics/constants I know that there are shops where e.g. ports are always written in upper case (and to be fair, the AXI signals are defined in UPPER_CASE in the specification). It should also be noted that while the std and ieee libraries do name types using UPPER_CASE many still refer to them using lower case.

Another problem, albeit probably smaller, is the type used for the signals. 'std_logic(_vector)' is in my experience by far the most commonly used, however at my current work, we use 'std_ulogic(_vector)' to allow for (theoretically) faster simulation and the ability for the simulator to detect multiple drivers at compile time.

Using generics to set the widths should also be optional. While it provides a single point of definition for the address and data widths, it does at the same time indicate to the user of the entity that they may be set freely, i.e. the design supports different widths which it may not do. Another use case for not using generics could be that the widths are defined using custom constants in a package

I also think that the name/prefix of the interface, e.g. the M_AXI_ should also be a variable, although it should have a default.

For the full AXI interface, it should be enough with a snippet with all the AXI signals and the user will then have to remove the unused ones.

With AXI5 being released, it might also make sense to add a "4" somewhere in the snippet name.

The above requires the snippets to support a couple of customizations.

  1. UPPER_CASE vs. lower_case identifiers
  2. UPPER_CASE vs. lower_case subtype_indication
  3. Using generics for widths or not
  4. Customizable prefix
  5. Sideband signals for AXI Streaming interfaces.

Some proposals for further discussion (not sure how VS Code treats identical names and upper/lower case in the prefix). One way to have upper/lower case variation in the identifiers would be to make two variants of each template, one with the prefix in lower case and one with the prefix in uppercase.

Snippets

Name Prefix Description
AXI4-Stream Master Interface axi4sm / AXI4SM Basic AXI4 Stream Master interface with tdata, tvalid, tready
AXI4-Stream Slave Interface axi4ss / AXI4SS Basic AXI4 Stream Slave interface with tdata, tvalid, tready
AXI4-Stream Master Interface axi4sm / AXI4SM Basic AXI4 Stream Master interface with tdata, tvalid, tready
AXI4-Stream Slave Interface axi4ss / AXI4SS Basic AXI4 Stream Slave interface with tdata, tvalid, tready
AXI4-Stream Master Interface /w Sideband ? Full AXI4 Stream Master interface
AXI4-Stream Slave Interface /w Sideband ? Full AXI4 Stream Slave interface
AXI4-Lite Master Interface axi4lm AXI4 Lite Master interface
AXI4-Lite Slave Interface axi4ls AXI4 Lite Slave interface
AXI4 Master Interface axi4m AXI4 Lite Master interface
AXI4 Slave Interface axi4s AXI4 Lite Slave interface

@valerionew
Copy link
Copy Markdown
Contributor Author

Thanks @Bochlin for your precious comment.

How about we close this PR and open an issue to further discuss the most sensible implementation of the snippets?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants