|
Message
From: Damjan Lampret<lampret@o...>
Date: Thu Feb 12 16:22:38 CET 2004
Subject: [pci] PCI Bridge Core bugs found
Hi !I don't know what your particular problem is, but I can comment on the
ASIC/FPGA thing. Flextronics Semiconductor made both FPGA prototype (VGA
card in a form of PCI plug in card using PCI and VGA controller) and ASIC
prototype. VGA card was plugged into a PC and Linux driver was written for X
Windows - ie Linux ran over this VGA PCI card displaying X Windows on VGA
monitor. In addition an ASIC was done where PCI is used to connect the rest
of the ASIC to PCI bus.
I also heard some other companies are using it in their products.
And I also heard some had problems synthesizing it because you need to know
how to written constraints properly for your synthesis and implementation
(with implementation I mean back end aka P&R...). So maybe your problem is
related to constraints (to be honest I haven't read the rest of your email)
regards,
Damjan
----- Original Message -----
From: "Andriy Knysh" <AndriyK@C...>
To: "'Discussion list about free, open source PCI IP core'"
<pci@o...>
Sent: Wednesday, February 11, 2004 10:03 PM
Subject: [pci] PCI Bridge Core bugs found
> Hello everybody,
>
> I've got a question to all of you: have you ever synthesized and tested
the
> PCI Bridge Core in a real
> application? Have you ever used the Core with Wishbone bus connected to a
> real device?
> I mean real application (FPGA or ASIC), not in a simulator, because in my
> experience
> almost everything works just fine in a simulator, but does not work in the
> real world.
>
> I'm asking you this, because I have found a few major flaws in the Core,
and
> in my opinion,
> the Core (this is mostly related to Wishbone bus) would never work in an
> FPGA or ASIC.
>
> I have sent that e-mail to the authors (Tadej and Miha), but never got an
> answer back.
>
> So, I'm asking you all if you have tested the core. If not, you should
read
> the rest of the e-mail and check,
> if I'm wrong or right.
>
> Thanks for your attention.
>
> Regards,
> Andriy Knysh, Ph.D.
>
>
____________________________________________________________________________
> _______
> When I started to make my PCI board I was thinking about using
> the Altera PCI Core, but obviously it was too expensive.
> So I downloaded the PCI Bridge core from opencores.org and added Altera
> Stratix
> memory instances for the PCI and WB FIFOs (M4K memory blocks).
> Now it is working very well with my PCI controller based on Altera Stratix
> FPGA.
>
> Unfortunately, during my design I stuck at some moment and after spending
a
> few days I found a few bugs in the PCI core.
>
> First, in the pci_wb_master.v file. I noticed that when I write a value to
> any of my registers (all my registers are 32 bit) in the
> FPGA (not a configuration space register - this was working fine), I get
the
> address of the PCI transaction written to the
> register instead of the data. Initially I thought it was a bug in my PC
> driver or my application, but after some time I eventually
> got to the file pci_wb_master.v of the PCI Bridge.
>
> Here I have found a few bugs related to the problem mentioned above.
> First, in the following state machine (line 733 in the original
> pci_wb_master.v file)
>
> // state machine logic
> always@(c_state or
> wb_ack_i or
> wb_rty_i or
> wb_err_i or
> w_attempt or
> r_attempt or
> retried or
> burst_chopped or
> burst_chopped_delayed or
> rty_i_delayed or
> pci_tar_read_request or
> rty_counter_almost_max_value or
> set_retry or
> last_data_to_pcir_fifo or
> first_wb_data_access or
> pciw_fifo_control_in or
> pciw_fifo_empty_in or
> burst_transfer or
> last_data_from_pciw_fifo_reg
> )
>
> I have found the following (line 844 in the original file
pci_wb_master.v):
>
> if (~pciw_fifo_empty_in)
> pciw_fifo_renable = 1'b0 ; // prepare next value (address when new
> trans., data when burst tran.)
> else
> pciw_fifo_renable = 1'b0 ;
>
> I guess this is a bug, since in order to read the next data from the FIFO
it
> should say:
>
> if (~pciw_fifo_empty_in)
> pciw_fifo_renable = 1'b1 ; // prepare next value (address when new
> trans., data when burst tran.)
> else
> pciw_fifo_renable = 1'b0 ;
>
>
> Second, in the following state machine (line 622 in the original file)
>
> // state machine register control and registered outputs (without wb_adr_o
> counter)
> always@(posedge wb_clock_in or posedge reset_in)
> begin
> ........................................................................
>
> I have found this logic (line 659 in the original file):
>
> if ((pciw_fifo_renable_out && ~addr_into_cnt) || addr_into_cnt_reg)
> begin
> wb_sel_o <= #`FF_DELAY ~pciw_fifo_cbe_in;
> wb_dat_o <= #`FF_DELAY pciw_fifo_addr_data_in;
> end
>
> Here the authors are trying to write to wb_dat_o data output register when
> one of the following conditions is met:
> 1) pciw_fifo_renable_out is 1 and addr_into_cnt is 0
> 2) addr_into_cnt_reg is 1
>
> Let's consider these two conditions. The first one says that if
> pciw_fifo_renable_out is 1 (e.g. reading from the FIFO
> is enabled since pciw_fifo_renable_out = pciw_fifo_renable ||
> addr_into_cnt_reg) and addr_into_cnt is 0.
> This condition has no chance to be true for the following reasons:
>
> On line 780 of the original file the authors wrote:
>
> 3'b100 : // Write request for PCIW_FIFO
> to WB bus transaction
> begin // If there is new transaction
> if (burst_chopped_delayed)
> begin
> addr_into_cnt = 1'b0 ; //
> address must not be latched into address counter
> pciw_fifo_renable = 1'b1 ; //
> first location is address (in FIFO), next will be data
> end
> else
> begin
> if
> (pciw_fifo_control_in[`ADDR_CTRL_BIT])
> addr_into_cnt = 1'b1 ; //
> address must be latched into address counter
> else
> addr_into_cnt = 1'b0 ;
> pciw_fifo_renable = 1'b1 ; //
> first location is address (in FIFO), next will be data
> end
> read_count_load = 1'b0 ; // no need
> for cache line when there is write
> n_state = S_WRITE ;
>
> Let's consider single write to a WB slave from PCI.
> I have highlighted this in bold. Only here
> pciw_fifo_renable is set to 1 and addr_into_cnt is
> set to 0. It seems like it is a good chance for that
> condition to be true, but at the same time you
> change the state to S_WRITE. And, since the authors have made that state
> machine asynchronous (not a good idea by itself),
> the system goes to S_WRITE state immediately (line 821 in the original
> file).
> And, since neither wb_ack_i nor wb_err_i nor
> wb_rty_I is set to 1 at that time, the system goes directly to default
state
> (line 908 in the original file). From here, if the
> retry counter is disabled or it has not reached the max value yet), the
line
> 927
> (pciw_fifo_renable = 1'b0 ;) will be executed
> immediately. At the next rising edge of the system clock (it is
> unpredictable
> when it comes since the state machine is
> asynchronous), the state machine (line 622) will be in the S_WRITE state,
> and line 659
> will be executed, but the first condition will be
> not met (since pciw_fifo_renable is 0 at that time).
> The second condition ( .... ||
> addr_into_cnt_reg) is met when, for example, it was an address phase on
line
> 789
>
> if (pciw_fifo_control_in[`ADDR_CTRL_BIT])
>
> addr_into_cnt = 1'b1 ; // address must be latched into address counter
>
> and then at the next rising edge of the system clock
> (also asynchronous !!!) line 498 will be executed and addr_into_cnt_reg
will
> be set to
> 1 as well as on line 641 c_state will be set to
> S_WRITE. Then on line 826 addr_into_cnt is immediately set to 0 (since it
is
> asynchronous),
> but addr_into_cnt_reg remains 1 until the next
> rising edge of the system clock when on line 659 the second condition is
met
> and the last
> address from the FIFO is latched to wb_dat_o (
> wb_dat_o <= #`FF_DELAY pciw_fifo_addr_data_in;).
>
> This problem has its roots in the asynchronous
> nature of the state machine when a signal changes its state before or at
the
> rising edge
> of the system clock. This is a classical problem
> with metastability - I have seen it a lot. What is interesting is that it
> almost always works
> in a simulator, but almost never in a real system
> (simulators do not take into account that a signal changes its state not
> instantly -
> it is almost eternity in the world of electrons and
> atoms).
>
> To fix the problem I just rewrote the following
> lines (659-663):
>
> if (!addr_into_cnt)
> begin
> wb_sel_o <= #`FF_DELAY ~pciw_fifo_cbe_in;
> wb_dat_o <= #`FF_DELAY
> pciw_fifo_addr_data_in;
> end
>
> I think it is absolutely not necessary to check for
> addr_into_cnt_reg or to check if we read from the FIFO.
> It is only necessary to check if it is a data phase
> (not address) so we can latch the new values into wb_dat_o and wb_sel_o.
> After the changes have been done, the core is
> working on Altera Stratix just fine without any problems.
>
> Another problem that I found (again, it is related
> to the asynchronous nature of the state machine) is in the
> file pci_wb_slave.v, starting on line 651:
>
> // state machine logic
> always@(
> c_state or
> wattempt or .............................
>
>
> Since the state machine has been made asynchronous,
> all the assignments from line 680 to line 710 never get any chance to be
> synthesized and executed (except, maybe in a
> simulator !!!!!!!).
> The only logic that will be synthesized is enclosed
> in the case statement on line 712:
> case (c_state)
> S_IDLE: begin
> ..............................................
>
> What is bothering me is that since the authors have
> put some logic before the case statement, they had some algorithm in mind.
> And, since that logic never get synthesized and
> executed, that algorithm does not work, and I suspect there are some
> major flaws in that implementation.
>
> Again, thanks for your attention.
>
> Andriy Knysh, Ph.D.
>
>
>
> _______________________________________________
> http://www.opencores.org/mailman/listinfo/pci
|
 |
Browse
