Skip to content
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

Closed
wants to merge 10 commits into from
160 changes: 160 additions & 0 deletions AMBA/AXI/v4/AXI4.unconstrained.vhdl
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
-- EMACS settings: -*- tab-width: 2; indent-tabs-mode: t -*-

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.

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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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. b is a good example of a completely unreadable name while I think id is a well established abbreviation.

Choose a reason for hiding this comment

The 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;

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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;

Copy link

@JimLewis JimLewis Jul 10, 2020

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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:

  1. the have not understood AXI and it's signal names
  2. 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.

-- 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;
92 changes: 92 additions & 0 deletions AMBA/AXI/v4/AXI4Common.vhdl
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;


Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 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.

Choose a reason for hiding this comment

The 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 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.

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);

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

Clock : std_ulogic;
Reset_n : std_ulogic;
end record;

end package;
68 changes: 68 additions & 0 deletions AMBA/AXI/v4/AXI4Lite.generic.vhdl
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),

Choose a reason for hiding this comment

The 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;
Loading