AXI4 #14

Paebbels · 2020-07-05T00:30:46Z

This PR is blocked by #13.

This PR adds ARM's AMBA AXI v4 interfaces:

AXI4 (full)
AXI4-Lite
AXI4-Stream

A common package is added, too, to hold common interface descriptions (types) and subtypes.

/cc @JimLewis, @NJDFan

… declared via package instances.

Paebbels · 2020-07-05T00:50:09Z

These files have been successfully checked by Riviera-PRO 2020.04.

AMBA/AXI/v4/AXI4-Common.vhdl

JimLewis · 2020-07-05T17:11:13Z

AMBA/AXI/v4/AXI4-Lite.generic.vhdl

+use work.AXI4_Lite.all;
+
+
+package AXI4_Lite_generic is


Again on naming Axi4Lite_generic or Axi4Lite_GenericPkg
Also, maybe we should do AXI4 Full and then have AXI4 Lite be a subtype of the full with the unused arrays as NULL.
Even with FULL, there are going to be variations - IE awuser width.

AMBA/AXI/v4/AXI4-Lite.presized.vhdl

JimLewis · 2020-07-05T17:16:19Z

AMBA/AXI/v4/AXI4-Common.vhdl

+	subtype AXI_Protect_Type  is std_ulogic_vector(2 downto 0);
+	subtype AXI_Response_Type is std_ulogic_vector(1 downto 0);
+
+	type AXI_System_Interface is record


Looks like there are tabs in the file. This causes rendering differences in different tools.
Can we do spaces instead?

tabs are intended. Space for indentation will be fixed with next upload.

JimLewis · 2020-07-10T13:41:56Z

AMBA/AXI/v4/AXI4.unconstrained.vhdl

+
+		-- Payload signals
+		ID        : 
+		Address   : Address_Type;


These names should be the same as the AxiBus names.
If you are going to shorten them, by removing the AW, ok
but keep the reset of the name the same, hence,
it should be Addr and not Address and
Prot and not Protect.

The AXI interface in itself is a bad example for naming. So we can either improve it to something better, that allows good code review in new code or to keep that legacy for ever and create unreadable files wherever you use AXI.

I'm also in favor of using more readable/descriptive names unless the abbreviations are very well and generally established. The downside is of course the mismatch with the standard. Same type of value/cost trade-off we need to do with master/slave. b is a good example of a completely unreadable name while I think id is a well established abbreviation.

Similar to ID, I think the rest of the AXI abbreviated names (Addr, Strb, ...) are well established, and hence, we should not be changing them.

JimLewis · 2020-07-10T13:46:18Z

AMBA/AXI/v4/AXI4Common.vhdl

+use     IEEE.std_logic_1164.all;
+use     IEEE.numeric_std.all;
+
+


Is there a reason to have a subtype for these?

To many levels of indirection leads to obfuscation.

On of the problems in OSVVM is that there is not enough abstraction. E.g. barriers use std_logic instead of a Barrier_Type. Now OSVVM is fixed to std_logic or it needs to offer many overloads.

On the other hand, OSVVM has good abstraction with ModellIDs to be AlertLogIDType.

When it comes to abstractions we need to consider if the type is at risk of changing and what the user implications would be. I agree that a public interface to a barrier can have its own type as that would allow the concept to evolve without breaking backward compatibility (at least to some degree). Here I'm thinking Data_Type is an example of a type not likely to change and even if it did it wouldn't affect a verification component user only using the interface type.

JimLewis · 2020-07-10T13:48:18Z

AMBA/AXI/v4/AXI4.unconstrained.vhdl

+		ReadAddress    : Axi4_Address_Interface;
+		ReadData       : Axi4_ReadData_Interface;
+	end record;
+


The current version of the OSVVM Interface uses WriteAddress, WriteData, ... also, however, I am wondering if we should not be using the AXI names, AW, W, B, AR, R? Simply from the stand point, when people see it in the code, they understand it.

IF we are going to change it in OSVVM, now is the time.

I also think that if you are going to use WriteAddress instead of AW, then you need to use AW in the record field names. And hence, it is AW.Addr. WriteAddress.Addr makes people think about the association too much. WriteAddress.AWAddr is also ok.

We need tighter correlation of the interface record naming and AXI naming for reviews.

I am just struggling with the fact that when it is AxiBus.WriteAddress.AWAddr (or AxiBus.WriteAddress.Addr) that I want an alias - and I would like to avoid wanting to use aliases.

Abbreviations is not good coding style. People don't need to think, they just need to read, because we have no abbreviations anymore. More over, if people don't know what AWAddr is, that it demonstrates two things:

the have not understood AXI and it's signal names

that AXI uses bad naming

An other rule for coding style is DRY (don't repeat yourself).

As we have this discussion now, you see why I want aliases for records to rearrange and rename for such situations.

LarsAsplund

I've not really reviewed against the standard. Just general comments.

LarsAsplund · 2020-07-14T09:44:02Z

AMBA/AXI/v4/AXI4.unconstrained.vhdl

@@ -0,0 +1,160 @@
+-- EMACS settings: -*-	tab-width: 2; indent-tabs-mode: t -*-


I think we should use spaces only. Tabs will cause more problems for people than it helps since most are using spaces (on Github). It would also be impossible for us to create a header that would adapt to all editors/viewers that people are using.

LarsAsplund · 2020-07-14T09:45:20Z

AMBA/AXI/v4/AXI4.unconstrained.vhdl

+-- vim: tabstop=2:shiftwidth=2:noexpandtab
+-- kate: tab-width 2; replace-tabs off; indent-width 2;
+-- =============================================================================
+-- Authors:         Rob Gaddi


Will all respect for your contributions I think this is for git to keep track of. Maintaining this list is just extra work.

LarsAsplund · 2020-07-14T09:49:03Z

AMBA/AXI/v4/AXI4.unconstrained.vhdl

+
+		-- Payload signals
+		ID        : 
+		Address   : Address_Type;


I'm also in favor of using more readable/descriptive names unless the abbreviations are very well and generally established. The downside is of course the mismatch with the standard. Same type of value/cost trade-off we need to do with master/slave. b is a good example of a completely unreadable name while I think id is a well established abbreviation.

LarsAsplund · 2020-07-14T09:50:13Z

AMBA/AXI/v4/AXI4.unconstrained.vhdl

+		Valid     : std_ulogic;
+		Ready     : std_ulogic;
+
+		-- Payload signals


I think we should consider defining transaction types that holds the payload. They can be used at a higher level of abstraction

LarsAsplund · 2020-07-14T10:23:54Z

AMBA/AXI/v4/AXI4Common.vhdl

+use     IEEE.std_logic_1164.all;
+use     IEEE.numeric_std.all;
+
+


When it comes to abstractions we need to consider if the type is at risk of changing and what the user implications would be. I agree that a public interface to a barrier can have its own type as that would allow the concept to evolve without breaking backward compatibility (at least to some degree). Here I'm thinking Data_Type is an example of a type not likely to change and even if it did it wouldn't affect a verification component user only using the interface type.

LarsAsplund · 2020-07-14T14:49:10Z

AMBA/AXI/v4/AXI4.unconstrained.vhdl

+		User      : User_Type;
+	end record;
+
+	type Axi4_Interface is record


Shouldn't the system interface be included? I guess that would mean we can't define one view as the converse of the other.

LarsAsplund · 2020-07-14T15:01:47Z

AMBA/AXI/v4/AXI4Common.vhdl

+  constant AXI4_PROTECT_NONE          : Protect_Type := "000"; 	
+
+
+	type AXI4_System_Interface is record


Indentation

LarsAsplund · 2020-07-14T15:06:48Z

AMBA/AXI/v4/AXI4.unconstrained.vhdl

+package Axi4 is
+	type Axi4_Address_Interface is record
+		-- Handshake signals
+		Valid     : std_ulogic;


valid and ready are used many places. Create a handshake type maybe?

At the place where you use them you will need to break the abstraction to initialize the signal you are driving.

LarsAsplund · 2020-07-14T15:28:04Z

AMBA/AXI/v4/AXI4Lite.generic.vhdl

+	);
+
+	subtype Axi4Lite_WriteData_SizedInterface is Axi4Lite_WriteData_Interface(
+		Data(DATA_BITS - 1 downto 0),


Another style issue. Alignment causes extra manual work as long as we have no open formatter. As a result we will have more whitespace diffs to deal with during code review. There's probably not a widespread consensus on what alignment to use either

LarsAsplund · 2020-07-14T15:31:06Z

AMBA/AXI/v4/AXI4Stream.unconstrained.vhdl

+
+
+package Axi4Stream is
+	type Axi4Stream_Interface is record


This interface is not complete.

Paebbels added 4 commits July 5, 2020 02:22

Added common types for AXI4.

23908ed

Added unconstraint interface description for AXI4-Lite.

ed32569

Added generic package, so commonly used presized AXI interface can be…

7c4a6f5

… declared via package instances.

Added commonly used presized AXI4-Lite interfaces.

2c9e68c

Paebbels added the Internal Mainly internal interface between components. label Jul 5, 2020

Paebbels self-assigned this Jul 5, 2020

Added views.

315bda5

Paebbels added this to the Prepare first release milestone Jul 5, 2020

JimLewis marked this pull request as ready for review July 5, 2020 17:04

JimLewis reviewed Jul 5, 2020

View reviewed changes

AMBA/AXI/v4/AXI4-Common.vhdl Outdated Show resolved Hide resolved

JimLewis reviewed Jul 5, 2020

View reviewed changes

AMBA/AXI/v4/AXI4-Common.vhdl Outdated Show resolved Hide resolved

JimLewis reviewed Jul 5, 2020

View reviewed changes

AMBA/AXI/v4/AXI4-Lite.presized.vhdl Outdated Show resolved Hide resolved

JimLewis reviewed Jul 5, 2020

View reviewed changes

Paebbels added SoC Bus Architecture Streaming Memory-Mapped labels Jul 5, 2020

Paebbels added 5 commits July 6, 2020 02:20

Updated after style reviews.

f8abd03

Added predefined constants for AXI4.

4bf6fb5

Added missing Cache field in AXI4-Lite.

c3d53ca

Added implementation for AXI4-Stream.

24b8cb5

Added AXI4 definitions (incomplete).

995b4b3

JimLewis reviewed Jul 10, 2020

View reviewed changes

LarsAsplund reviewed Jul 14, 2020

View reviewed changes

umarcor mentioned this pull request Jul 11, 2021

USB-UART bridge/peripheral for boards without a uC or an external bridge stnolting/neorv32#102

Closed

Paebbels closed this May 18, 2023

Paebbels deleted the AXI branch May 18, 2023 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AXI4 #14

AXI4 #14

Paebbels commented Jul 5, 2020 •

edited

Loading

Paebbels commented Jul 5, 2020

JimLewis Jul 5, 2020 •

edited

Loading

JimLewis Jul 5, 2020

Paebbels Jul 11, 2020

JimLewis Jul 10, 2020

Paebbels Jul 11, 2020

LarsAsplund Jul 14, 2020

JimLewis Jul 19, 2020

JimLewis Jul 10, 2020

Paebbels Jul 11, 2020

LarsAsplund Jul 14, 2020

JimLewis Jul 10, 2020 •

edited

Loading

Paebbels Jul 11, 2020

LarsAsplund left a comment

LarsAsplund Jul 14, 2020

LarsAsplund Jul 14, 2020

LarsAsplund Jul 14, 2020

LarsAsplund Jul 14, 2020

LarsAsplund Jul 14, 2020

LarsAsplund Jul 14, 2020

LarsAsplund Jul 14, 2020

LarsAsplund Jul 14, 2020

JimLewis Jul 14, 2020

LarsAsplund Jul 14, 2020

LarsAsplund Jul 14, 2020

		@@ -0,0 +1,160 @@
		-- EMACS settings: -- tab-width: 2; indent-tabs-mode: t --

		constant AXI4_PROTECT_NONE : Protect_Type := "000";


		type AXI4_System_Interface is record

AXI4 #14

AXI4 #14

Conversation

Paebbels commented Jul 5, 2020 • edited Loading

Paebbels commented Jul 5, 2020

JimLewis Jul 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimLewis Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LarsAsplund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paebbels commented Jul 5, 2020 •

edited

Loading

JimLewis Jul 5, 2020 •

edited

Loading

JimLewis Jul 10, 2020 •

edited

Loading