search

alexforencich / cocotbext-pcie

PCI express simulation framework for Cocotb
MIT License
139 stars 47 forks source link

Xilinx US+ Configuration Flow Control Interface incorrect #19

Open bengiac opened 8 months ago

bengiac commented 8 months ago

I believe _run_cfg_fc_logic for usp_model.py should be combinational instead of clocked to match xilinx behavior

also it appears the _run_cfg_fc_logic is missing the sel == 0000 case

alexforencich commented 8 months ago

I wrote that based on the documentation, which TBH isn't really all that clear on the precise behavior. Do you have a simulation or ILA trace that indicates what the behavior should be? You're right that the 000 case is missing, but TBH I'm also not sure if all of the other cases are completely correct. Well, except for TX credit available, which I'm pretty sure is correct (and is the only one that I actually use in designs). And the UltraScale model presumably has the same issue.

Also, I have not bothered to implement cfg_fc_npd and cfg_fc_nph....these signals do not appear to work correctly on the actual hardware (they get stuck at 0), so there isn't really any point in trying to reverse-engineer exactly how they're supposed to work since AFAICT they're completely useless.

bengiac commented 8 months ago

thanks for the clarification of the state of these interfaces I too agree the precise behavior is not clear from the documentation

This is all I have so far, which suggests the selection is combinational, assuming they aren't double registering it image

alexforencich commented 8 months ago

TBH, double registering would make more sense than combinatorial. But I guess either is possible, I think some more ILA captures are necessary, particularly under load so the values can be examined as stuff happens.

bengiac commented 8 months ago

appears you are correct, that it is double registered image

alexforencich commented 8 months ago

Hmmmm. It really looks like it's actually only single registered. Right? fc_sel of 0 selects "receive credits available to the link partner", which I suspect reports infinite completion credits (cplh/cpld 0). And that's exactly what you see here: cplh/cpld is zero one cycle after fc_sel is set to 0.

bengiac commented 8 months ago

do you mean 2? fc_sel is set 0 then 2 cycles later cplh/cpld is 0, possibly my terminology that I use is different to yours, as I see it, first capture is fc_sel is held for 2 cycles, and fc_xx changes with it, second capture is fc_sel held for 4 cycles, and fc_xx is delayed by 2 cycles

alexforencich commented 8 months ago

Oh, you're holding it for 2 clock cycles? I guess there is a two-cycle delay, then. It's not super obvious how long things are held for on the ILA trace.