cancel
Showing results for 
Show  only  | Search instead for 
Did you mean: 
simoneeic
Observer
Observer
727 Views
Registered: ‎02-11-2020

Delaying AXI Read Until Data is Valid Causes UIO Read Error in Linux

Jump to solution

Hi,

I have a time-to-digital converter (TDC) design in FPGA that detects how much time a value from an external pin is 1 (lets consider T the time that the signal is 1). This time is then gathered and used to calculate another value. This calculation is done as a pipeline and it should take X cycles to complete (I have defined X as 15 cycles for now).

I want to send this data to the PS side so i use an AXI lite interface with 4 registers. One of the registers contains the calculated value that will only be valid after T + X clock cycles. So, in order to always read valid data from the PS side, i have changed the default code from the AXI lite interface so that "axi_rvalid <= valid_data;". This is supposed to delay the AXI read transaction until the calculated value is valid.

I have tested this implementation in a standalone application and it seems to be working as expected. So, i created a petalinux image with this design and used UIO drivers to read the AXI lite registers but i always get an error when i try to read the AXI register that contains the calculated value. I have also tried to read the AXI registers with devmem command and i also get an error. In both cases the Linux freezes until a reboot is made.

I have also tested a design where the default AXI lite code is used and so the value of the AXI Read Valid is maintained: "axi_rvalid <= 1'b1;". This means that the data read from the AXI registers might not be valid but the UIO reads are always successful. 

The only difference between the two designs is the "axi_rvalid" value.

Questions:

- What may be the cause of the error?

- Is there a way to keep the first design logic and solve the UIO read error? 

- Is there a better way to do this? (I know i could do an AXI read only after an interrupt triggered by the valid_data signal, but this approach would introduce an extra latency that i want to avoid)


Code snippets:

	// Implement axi_arvalid generation
	// axi_rvalid is asserted for one S_AXI_ACLK clock cycle when both 
	// S_AXI_ARVALID and axi_arready are asserted. The slave registers 
	// data are available on the axi_rdata bus at this instance. The 
	// assertion of axi_rvalid marks the validity of read data on the 
	// bus and axi_rresp indicates the status of read transaction.axi_rvalid 
	// is deasserted on reset (active low). axi_rresp and axi_rdata are 
	// cleared to zero on reset (active low).  
	always @( posedge S_AXI_ACLK )
	begin
	  if ( S_AXI_ARESETN == 1'b0 )
	    begin
	      axi_rvalid <= 0;
	      axi_rresp  <= 0;
	    end 
	  else
	    begin    
	      /* DEFAULT AXI CODE CHANGE -> added "valid_data" 
                (meaning that AXI can only read when data is valid; it should wait until valid)*/
	      if (axi_arready && S_AXI_ARVALID && ~axi_rvalid)
	        begin
	          // Valid read data is available at the read data bus
	          axi_rvalid <= valid_data; /********* CHANGED ************/
                  //axi_rvalid <= 1'b1;
	          //axi_rvalid <= w_gray_tdc_valid;
	          axi_rresp  <= 2'b0; // 'OKAY' response
	        end   
	      else if (axi_rvalid && S_AXI_RREADY)
	        begin
	          // Read data is accepted by the master
	          axi_rvalid <= 1'b0;
	        end                
	    end
	end    

 

// Implement memory mapped register select and read logic generation
	// Slave register read enable is asserted when valid address is available
	// and the slave is ready to accept the read address.
	assign slv_reg_rden = axi_arready & S_AXI_ARVALID & ~axi_rvalid;
	always @(*)
	begin
	      // Address decoding for reading registers
	      case ( axi_araddr[ADDR_LSB+OPT_MEM_ADDR_BITS:ADDR_LSB] )
	        2'h0   : reg_data_out <= slv_reg0;
	        2'h1   : reg_data_out <= slv_reg1;
	        2'h2   : reg_data_out <= slv_reg2;
	        2'h3   : reg_data_out <= data; /******* TDC Calculated Value ******/
	        default : reg_data_out <= 0;
	      endcase
	end

 
Best regards,
Simão Araújo

0 Kudos
1 Solution

Accepted Solutions
dgisselq
Scholar
Scholar
591 Views
Registered: ‎05-21-2015

@simoneeic ,

Sadly, Xilinx's default AXI and AXI-lite designs have been broken for years, and even years after bug reports were made.

If you want to start with a working AXI-lite slave design, then I might recommend this one.  I've liked the logic within it so much that I tend to copy it myself any time I need an AXI-lite slave.

If you want to add a feature that will delay reads until a time stamp, then I would recommend using the skid buffers and adjusting the axil_read_ready signal within the design.  You should be able to add a condition to it, such as:

assign axil_read_ready = (current_conditions) && your_new_condition;

I'd still highly recommend you formally verify your result--lest you fall into the same trap Xilinx got stuck in: believing their logic works when it does not.  When you do, however, you might also need to make an exception for how long the core should have to wait for a read result.

Dan

View solution in original post

0 Kudos
8 Replies
dgisselq
Scholar
Scholar
646 Views
Registered: ‎05-21-2015

@simoneeic ,

You actually have a couple of AXI bugs in the design logic you've shared.

  • First, you should never set RVALID based upon ARVALID && ARREADY && anything else.  What would then happen if ARVALID && ARREADY but the anything else weren't true?  The request would get dropped and your system would hang.  The proper setting is to either use a skid buffer, or to set ARREADY to be equal to RVALID.  You can then check for ARVALID && ARREADY and know you won't drop requests.
  • You can then drop RVALID anytime RREADY is true, independent of RVALID, but this isn't a bug, so I'm a bit off topic here.
  • Beware of Xilinx's example AXI components.  Xilinx's example AXI slaves are not protocol compliant, and a reoccurring complaint here on the forums follows from someone copying Xilinx's AXI designs and then struggling when their design naturally hangs as a result.
  • Your RDATA value *MUST* be registered.  The AXI specification specifically disallows combinatoial paths between inputs and outputs.  Further, RDATA is not allowed to change any time RVALID is true but RREADY is false.  Worse, ARADDR is not guaranteed past the ARVALID, so the logic you've posted might well read from the wrong address--even though it will probably work in a test bench.  The correct answer for RDATA is to set it on the positive edge of S_AXI_ACLK, but only when/if !RVALID || RREADY.  (You could also set it any time ARVALID && ARREADY--but that only works if you don't use the skid buffer, and the general solution remains !RVALID || RREADY).

Dan

0 Kudos
simoneeic
Observer
Observer
601 Views
Registered: ‎02-11-2020

Hi @dgisselq, i appreciate your response.

The code that i posted here is the default code from Vivado AXI Lite, I only made two changes and they are "axi_rvalid <= valid_data;" and "2'h3 : reg_data_out <= data;". As I said this logic in standalone works so that is why I use it. 
Despite that, do you recommend any alternative axi lite code from a repository? 

Best regards,
Simão Araújo

0 Kudos
dgisselq
Scholar
Scholar
592 Views
Registered: ‎05-21-2015

@simoneeic ,

Sadly, Xilinx's default AXI and AXI-lite designs have been broken for years, and even years after bug reports were made.

If you want to start with a working AXI-lite slave design, then I might recommend this one.  I've liked the logic within it so much that I tend to copy it myself any time I need an AXI-lite slave.

If you want to add a feature that will delay reads until a time stamp, then I would recommend using the skid buffers and adjusting the axil_read_ready signal within the design.  You should be able to add a condition to it, such as:

assign axil_read_ready = (current_conditions) && your_new_condition;

I'd still highly recommend you formally verify your result--lest you fall into the same trap Xilinx got stuck in: believing their logic works when it does not.  When you do, however, you might also need to make an exception for how long the core should have to wait for a read result.

Dan

View solution in original post

0 Kudos
vanmierlo
Mentor
Mentor
413 Views
Registered: ‎06-10-2008

Instead of waiting (indefinitely?) for valid_data you can also set the result code axi_rresp to 0b10 SLVERR to indicate the transaction cannot take place. Linux will throw an exception which you can probably catch.

0 Kudos
simoneeic
Observer
Observer
402 Views
Registered: ‎02-11-2020

Hi @dgisselq,

I am trying the AXI-lite slave you recommended but i am getting the error in the figure below. It doesn´t seem to make sense as the given range is enough, perhaps there is another error that vivado is not identifying. The S_AXI_AWADDR is declared as follows:

// Width of S_AXI data bus
parameter integer C_S_AXI_DATA_WIDTH	= 32,
// Width of S_AXI address bus
parameter integer C_S_AXI_ADDR_WIDTH	= 4,
		
// Users to add parameters here
parameter [0:0]	OPT_SKIDBUFFER = 1'b1,
parameter [0:0]	OPT_LOWPOWER = 0,
localparam	ADDRLSB = $clog2(C_S_AXI_DATA_WIDTH)-3
// User parameters ends		
                
...

// Write address (issued by master, acceped by Slave)
input wire [C_S_AXI_ADDR_WIDTH-1 : 0] S_AXI_AWADDR,

simoneeic_0-1613475267187.png

Best regards,
Simão Araújo

0 Kudos
simoneeic
Observer
Observer
399 Views
Registered: ‎02-11-2020

Hi @vanmierlo,

Thanks for your suggestion. If i can't use valid_data to delay the AXI read that may be a good alternative.

Best regards,
Simão Araújo

0 Kudos
dgisselq
Scholar
Scholar
378 Views
Registered: ‎05-21-2015

@simoneeic ,

The problem is that you are defining S_AXI_AWADDR as having C_S_AXI_ADDR_WIDTH (i.e. some small number) bits, but then attempting to reference C_S_AXI_DATA_WIDTH bits (i.e. 32) of it.  There just aren't that many bits in S_AXI_AWADDR, and that's why Vivado is giving you an error.  It looks like a copy mistake, since the original (as far as I can tell) referenced S_AXI_AWADDR[C_S_AXI_ADDR_WIDTH-1:ADDRLSB] not S_AXI_AWADDR[C_S_AXI_DATA_WIDTH-1:ADDRLSB].

Dan

simoneeic
Observer
Observer
329 Views
Registered: ‎02-11-2020

Hi @dgisselq,

Just to let you know that your suggestion worked for me (marked as the accepted solution). I had a copy mistake as you mentioned. I am now able to delay and AXI read until my valid signal becomes true and it works in Linux too.
I would also like to thank you and congratulate you on the really great blog Zip CPU that you created. FPGA users and enthusiasts need more content like that.

Best regards,
Simão Araújo


0 Kudos