-
Notifications
You must be signed in to change notification settings - Fork 2
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
AXI4 #14
AXI4 #14
Changes from all commits
23908ed
ed32569
7c4a6f5
2c9e68c
315bda5
f8abd03
4bf6fb5
c3d53ca
24b8cb5
995b4b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,160 @@ | ||
-- EMACS settings: -*- tab-width: 2; indent-tabs-mode: t -*- | ||
-- vim: tabstop=2:shiftwidth=2:noexpandtab | ||
-- kate: tab-width 2; replace-tabs off; indent-width 2; | ||
-- ============================================================================= | ||
-- Authors: Rob Gaddi | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will all respect for your contributions I think this is for git to keep track of. Maintaining this list is just extra work. |
||
-- Patrick Lehmann | ||
-- | ||
-- Package: VHDL-2019 AXI4 interface descriptions | ||
-- | ||
-- Description: | ||
-- ------------------------------------- | ||
-- This package | ||
-- | ||
-- License: | ||
-- ============================================================================= | ||
-- Copyright 2016-2020 Open Source VHDL Group | ||
-- | ||
-- Licensed under the Apache License, Version 2.0 (the "License"); | ||
-- you may not use this file except in compliance with the License. | ||
-- You may obtain a copy of the License at | ||
-- | ||
-- http://www.apache.org/licenses/LICENSE-2.0 | ||
-- | ||
-- Unless required by applicable law or agreed to in writing, software | ||
-- distributed under the License is distributed on an "AS IS" BASIS, | ||
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
-- See the License for the specific language governing permissions and | ||
-- limitations under the License. | ||
-- ============================================================================= | ||
|
||
library IEEE; | ||
use IEEE.std_logic_1164.all; | ||
use IEEE.numeric_std.all; | ||
|
||
use work.Axi4Common.all; | ||
|
||
|
||
package Axi4 is | ||
type Axi4_Address_Interface is record | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer prefixing this record with interface type or maybe just type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also like having type in the name |
||
-- Handshake signals | ||
Valid : std_ulogic; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the place where you use them you will need to break the abstraction to initialize the signal you are driving. |
||
Ready : std_ulogic; | ||
|
||
-- Payload signals | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should consider defining transaction types that holds the payload. They can be used at a higher level of abstraction |
||
ID : | ||
Address : Address_Type; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These names should be the same as the AxiBus names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
Len : Length_Type; | ||
Size : Size_Type; | ||
Burst : Burst_Type; | ||
Lock : Lock_Type; | ||
QoS : QualityOfService_Type; | ||
Region : Region_Type; | ||
Cache : Cache_Type; | ||
Protect : Protect_Type; | ||
|
||
User : User_Type; | ||
end record; | ||
|
||
type Axi4_WriteData_Interface is record | ||
-- Handshake signals | ||
Valid : std_ulogic; | ||
Ready : std_ulogic; | ||
|
||
-- Payload signals | ||
Data : Data_Type; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an example of what could be bundled into a transaction type |
||
Strobe : Strobe_Type; | ||
Last : std_ulogic; | ||
|
||
User : User_Type; | ||
end record; | ||
|
||
type Axi4_WriteResponse_Interface is record | ||
-- Handshake signals | ||
Valid : std_ulogic; | ||
Ready : std_ulogic; | ||
|
||
-- Payload signals | ||
ID : | ||
Response : Response_Type; | ||
|
||
User : User_Type; | ||
end record; | ||
|
||
type Axi4_ReadData_Interface is record | ||
-- Handshake signals | ||
Valid : std_ulogic; | ||
Ready : std_ulogic; | ||
|
||
-- Payload signals | ||
ID : | ||
Response : Response_Type; | ||
|
||
Data : Data_Type; | ||
Last : std_ulogic; | ||
|
||
User : User_Type; | ||
end record; | ||
|
||
type Axi4_Interface is record | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the system interface be included? I guess that would mean we can't define one view as the converse of the other. |
||
WriteAddress : Axi4_Address_Interface; | ||
WriteData : Axi4_WriteData_Interface; | ||
WriteResponse : Axi4_WriteResponse_Interface; | ||
ReadAddress : Axi4_Address_Interface; | ||
ReadData : Axi4_ReadData_Interface; | ||
end record; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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. |
||
-- All lower-level views are defined from the driver's point of view. | ||
view Axi4_Address_MasterView of Axi4_Address_Interface is | ||
-- Handshake signals | ||
Valid : out; | ||
Ready : in; | ||
|
||
-- Payload signals | ||
Address : out; | ||
Cache : out; | ||
Protect : out; | ||
end view; | ||
alias Axi4_Address_SlaveView is Axi4_Address_MasterView'converse; | ||
|
||
view Axi4_WriteData_MasterView of Axi4_WriteData_Interface is | ||
-- Handshake signals | ||
Valid : out; | ||
Ready : in; | ||
|
||
-- Payload signals | ||
Data : out; | ||
Strobe : out; | ||
end view; | ||
alias Axi4_WriteData_SlaveView is Axi4_WriteData_MasterView'converse; | ||
|
||
view Axi4_WriteResponse_MasterView of Axi4_WriteResponse_Interface is | ||
-- Handshake signals | ||
Valid : in; | ||
Ready : out; | ||
|
||
-- Payload signals | ||
Response : in; | ||
end view; | ||
alias Axi4_WriteResponse_SlaveView is Axi4_WriteResponse_MasterView'converse; | ||
|
||
view Axi4_ReadData_MasterView of Axi4_ReadData_Interface is | ||
-- Handshake signals | ||
Valid : in; | ||
Ready : out; | ||
|
||
-- Payload signals | ||
Data : in; | ||
Response : in; | ||
end view; | ||
alias Axi4_ReadData_SlaveView is Axi4_ReadData_MasterView'converse; | ||
|
||
view Axi4_MasterView of Axi4_Interface is | ||
WriteAddress : view Axi4_Address_MasterView; | ||
WriteData : view Axi4_WriteData_MasterView; | ||
WriteResponse : view Axi4_WriteResponse_MasterView; | ||
ReadAddress : view Axi4_Address_MasterView; | ||
ReadData : view Axi4_ReadData_MasterView; | ||
end view; | ||
alias Axi4_SlaveView is Axi4_MasterView'converse; | ||
end package; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
-- EMACS settings: -*- tab-width: 2; indent-tabs-mode: t -*- | ||
-- vim: tabstop=2:shiftwidth=2:noexpandtab | ||
-- kate: tab-width 2; replace-tabs off; indent-width 2; | ||
-- ============================================================================= | ||
-- Authors: Patrick Lehmann | ||
-- | ||
-- Package: VHDL-2019 Common types in AXI4 interface descriptions | ||
-- | ||
-- Description: | ||
-- ------------------------------------- | ||
-- This package | ||
-- | ||
-- License: | ||
-- ============================================================================= | ||
-- Copyright 2016-2020 Open Source VHDL Group | ||
-- | ||
-- Licensed under the Apache License, Version 2.0 (the "License"); | ||
-- you may not use this file except in compliance with the License. | ||
-- You may obtain a copy of the License at | ||
-- | ||
-- http://www.apache.org/licenses/LICENSE-2.0 | ||
-- | ||
-- Unless required by applicable law or agreed to in writing, software | ||
-- distributed under the License is distributed on an "AS IS" BASIS, | ||
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
-- See the License for the specific language governing permissions and | ||
-- limitations under the License. | ||
-- ============================================================================= | ||
|
||
library IEEE; | ||
use IEEE.std_logic_1164.all; | ||
use IEEE.numeric_std.all; | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason to have a subtype for these? To many levels of indirection leads to obfuscation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On of the problems in OSVVM is that there is not enough abstraction. E.g. barriers use On the other hand, OSVVM has good abstraction with ModellIDs to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
package Axi4Common is | ||
-- Common to all AXI buses | ||
subtype Data_Type is std_ulogic_vector; | ||
subtype User_Type is std_ulogic_vector; | ||
|
||
-- Unique to AXI-Stream | ||
subtype Keep_Type is std_ulogic_vector; | ||
|
||
-- Unique to AXI-Lite | ||
subtype Address_Type is unresolved_unsigned; | ||
subtype Strobe_Type is std_ulogic_vector; | ||
|
||
|
||
-- Unique to AXI | ||
subtype ID_Type is unresolved_unsigned; | ||
subtype Region_Type is std_ulogic_vector(3 downto 0); | ||
subtype Cache_Type is std_ulogic_vector(3 downto 0); | ||
subtype QualityOfService_Type is std_ulogic_vector(3 downto 0); | ||
|
||
|
||
subtype Size_Type is unresolved_unsigned(2 downto 0); | ||
constant AXI4_SIZE_1 : Size_Type := "000"; | ||
constant AXI4_SIZE_2 : Size_Type := "001"; | ||
constant AXI4_SIZE_4 : Size_Type := "010"; | ||
constant AXI4_SIZE_8 : Size_Type := "011"; | ||
constant AXI4_SIZE_16 : Size_Type := "100"; | ||
constant AXI4_SIZE_32 : Size_Type := "101"; | ||
constant AXI4_SIZE_64 : Size_Type := "110"; | ||
constant AXI4_SIZE_128 : Size_Type := "111"; | ||
|
||
|
||
subtype Burst_Type is std_ulogic_vector(1 downto 0); | ||
constant AXI4_BURST_FIXED : Burst_Type := "00"; | ||
constant AXI4_BURST_INCR : Burst_Type := "01"; | ||
constant AXI4_BURST_WRAP : Burst_Type := "10"; | ||
|
||
|
||
subtype Response_Type is std_ulogic_vector(1 downto 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too much indentation |
||
constant AXI4_RESPONSE_OKAY : Response_Type := "00"; | ||
constant AXI4_RESPONSE_EX_OKAY : Response_Type := "01"; | ||
constant AXI4_RESPONSE_SLAVE_ERROR : Response_Type := "10"; | ||
constant AXI4_RESPONSE_DECODE_ERROR : Response_Type := "11"; | ||
|
||
|
||
subtype Protect_Type is std_ulogic_vector(2 downto 0); | ||
-- Bit 0: 0 Unprivileged access 1 Privileged access | ||
-- Bit 1: 0 Secure access 1 Non-secure access | ||
-- Bit 2: 0 Data access 1 Instruction access | ||
constant AXI4_PROTECT_INIT : Protect_Type := "UUU"; | ||
constant AXI4_PROTECT_NONE : Protect_Type := "000"; | ||
|
||
|
||
type AXI4_System_Interface is record | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation |
||
Clock : std_ulogic; | ||
Reset_n : std_ulogic; | ||
end record; | ||
|
||
end package; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
-- EMACS settings: -*- tab-width: 2; indent-tabs-mode: t -*- | ||
-- vim: tabstop=2:shiftwidth=2:noexpandtab | ||
-- kate: tab-width 2; replace-tabs off; indent-width 2; | ||
-- ============================================================================= | ||
-- Authors: Patrick Lehmann | ||
-- | ||
-- Generic Package: Generic AXI4-Lite interface descriptions for pre-constraining | ||
-- | ||
-- Description: | ||
-- ------------------------------------- | ||
-- This package | ||
-- | ||
-- License: | ||
-- ============================================================================= | ||
-- Copyright 2016-2020 Open Source VHDL Group | ||
-- | ||
-- Licensed under the Apache License, Version 2.0 (the "License"); | ||
-- you may not use this file except in compliance with the License. | ||
-- You may obtain a copy of the License at | ||
-- | ||
-- http://www.apache.org/licenses/LICENSE-2.0 | ||
-- | ||
-- Unless required by applicable law or agreed to in writing, software | ||
-- distributed under the License is distributed on an "AS IS" BASIS, | ||
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
-- See the License for the specific language governing permissions and | ||
-- limitations under the License. | ||
-- ============================================================================= | ||
|
||
use work.Axi4Lite.all; | ||
|
||
|
||
package Axi4Lite_Generic is | ||
generic ( | ||
constant ADDRESS_BITS : positive; | ||
constant DATA_BITS : positive; | ||
constant STROBE_BITS : positive := DATA_BITS / 8 | ||
); | ||
|
||
subtype Axi4Lite_Address_SizedInterface is Axi4Lite_Address_Interface( | ||
Address(ADDRESS_BITS - 1 downto 0) | ||
); | ||
|
||
subtype Axi4Lite_WriteData_SizedInterface is Axi4Lite_WriteData_Interface( | ||
Data(DATA_BITS - 1 downto 0), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
Strobe(STROBE_BITS - 1 downto 0) | ||
); | ||
|
||
subtype Axi4Lite_ReadData_SizedInterface is Axi4Lite_ReadData_Interface( | ||
Data(DATA_BITS - 1 downto 0) | ||
); | ||
|
||
subtype Axi4Lite_SizedInterface is Axi4Lite_Interface( | ||
WriteAddress( | ||
Address(ADDRESS_BITS - 1 downto 0) | ||
), | ||
WriteData( | ||
Data(DATA_BITS - 1 downto 0), | ||
Strobe(STROBE_BITS - 1 downto 0) | ||
), | ||
ReadAddress( | ||
Address(ADDRESS_BITS - 1 downto 0) | ||
), | ||
ReadData( | ||
Data(DATA_BITS - 1 downto 0) | ||
) | ||
); | ||
end package; |
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 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.