Skip to content

xvc: use the server's negotiated buffer size instead of a fixed 512B chunk - #736

Open
bergyla wants to merge 5 commits into
trabucayre:masterfrom
bergyla:xvc-use-negotiated-buffer-size
Open

bergyla wants to merge 5 commits into
trabucayre:masterfrom
bergyla:xvc-use-negotiated-buffer-size

Conversation

@bergyla

@bergyla bergyla commented Sep 23, 2026 •

Copy link
Copy Markdown

Bug

Efinix::programJTAG() (src/efinix.cpp) chunks the SRAM configuration
payload into fixed 512-byte blocks before calling Jtag::shiftDR():

int xfer_len = 512;
...
uint8_t tx[512];

Each block becomes one wire-level JTAG shift. When the active cable is the
XVC client (src/xvc_client.cpp), each block also becomes one full
shift: command round-trip to the XVC server — regardless of the buffer
size the server actually advertised in its getinfo: reply
(xvcServer_v1.0:<bytes>). XVC_client already parses that size
correctly into _buffer_size, but nothing upstream of programJTAG()
ever asked for it: XVC_client::get_buffer_size() returned a hardcoded
2048 (src/xvc_client.hpp:76) that no caller invoked, and
programJTAG()'s own chunking used its own separate hardcoded 512
regardless of adapter.

Against a bridge that negotiates a 16 KiB buffer (getinfo →
xvcServer_v1.0:32768, i.e. _buffer_size = 32768/2 = 16384), this turns
a 175 KB bitstream load into ~350 separate shift: round-trips of ~475 B
payload each, instead of the ~9-11 round-trips the negotiated buffer size
would allow.

Fix

Adds a new virtual accessor JtagInterface::preferred_xfer_bits(default_bits)
with an identity default (return default_bits;), so every adapter except
XVC is unaffected. XVC_client overrides it to return
max(_buffer_size*8, default_bits). Jtag forwards it to the active
interface. Efinix::programJTAG() now asks the adapter for its preferred
transfer size instead of hardcoding 512:

int xfer_len = _jtag->preferred_xfer_bits(512*8) / 8;

tx changes from a fixed uint8_t[512] to a std::vector<uint8_t> sized
to xfer_len accordingly.

No other adapter overrides preferred_xfer_bits(), so the default path
(return default_bits = the same 512*8 as before) is unchanged for
every cable other than xvc-client. Efinix::programSPI() and
SPIFlash (src/spiFlash.cpp) were checked and do not share this
constant or this chunking pattern — they program via SPI page writes
(write_page()), not via shiftDR(), so preferred_xfer_bits() does not
apply there.

Measurement

Protocol-level (local XVC mock server, no board, same chunking loop as
programJTAG(), 60000 B payload, mock negotiates 16384 B buffer):

shift commands max payload/shift
before (xfer_len = 512 fixed) 123 512 B
after (preferred_xfer_bits()) 9 16384 B

13.7x fewer shift round-trips for the same payload.

Silicon (Ti60 Eval & T8 Xyloni via TheVice ESP32-P4 XVC bridge, TCK 15 MHz,
thevice_top_xyloni.bit, 175 KB, 3 runs each, IDCODE-verified before/after
each load):

Client Load time Bridge transfer count
stock v1.1.1 (xfer_len = 512 fixed) 0.90 s 382 OUT / 375 IN (~475 B vectors)
patched (preferred_xfer_bits()) 0.67 / 0.65 / 0.65 s 156 OUT / 174080 B

~28% faster end-to-end bitstream load, no behavioural change for any
non-XVC adapter (identity default on preferred_xfer_bits()).

Scope

Changed: src/xvc_client.hpp, src/jtagInterface.hpp, src/jtag.hpp,
src/efinix.cpp. No other adapter's get_buffer_size()/chunking logic
touched; programSPI() untouched (checked, not applicable — see above).

…chunk

openFPGALoader as an XVC client was transferring TDI data in tiny bursts
regardless of the buffer size the XVC server actually advertised via
getinfo (e.g. 16384 bytes), causing far more round-trips than necessary.

Root cause: XVC_client::get_buffer_size() (src/xvc_client.hpp) returned a
hardcoded, unused placeholder (2048) instead of the negotiated
_buffer_size, and nothing called it anyway -- the real bottleneck was
Efinix::programJTAG() (src/efinix.cpp), which chunks the SRAM
configuration data with a hardcoded `xfer_len = 512` regardless of
adapter, so every writeTDI() call to the XVC client carried at most 512
bytes and turned into one small "shift:" packet.

Fix:
- XVC_client::get_buffer_size() now returns the real negotiated
  _buffer_size.
- A new JtagInterface::preferred_xfer_bits(default_bits) hook lets a
  caller ask an adapter for a preferred bulk-transfer size before
  chunking a large shiftDR()/writeTDI(). The default implementation is
  the identity function (returns default_bits unchanged), so no adapter
  other than XVC_client is affected; only XVC_client overrides it, based
  on its real buffer size.
- Efinix::programJTAG() now asks _jtag->preferred_xfer_bits(512*8) for
  its chunk size instead of hardcoding 512, and its stack buffer became
  a std::vector<uint8_t> sized accordingly.

Measured (local test harness driving Jtag::shiftDR() with the exact
Efinix::programJTAG() chunking loop against a protocol-verified mock XVC
server, buffer size 16384B, 60000B payload):
  before: 123 shift commands, 512B max, ~488B average
  after:    9 shift commands, 16384B max, ~6668B average
@trabucayre

Copy link
Copy Markdown
Owner

The idea looks good but I see some improvements:

  • the preferred_xfer_bitsmay be called once in CTOR
  • the std::vector<uint8_t> may also be in class member and reused for all transfer (in programJtag but also with SPI methods)

Another thing I have here (not fully related to this patch) is the fact a bit reversal is performed during programSPI instead of when the bitstream is parsed. With a bitstream with the bit reversal applied the temporary buffer will be no more required, the only thing to know is the burst size.

@bergyla

bergyla commented Sep 23, 2026

Copy link
Copy Markdown
Author

Thanks for the review. Agreed on both points: I'll move the preferred_xfer_bits query into the constructor and keep a single reusable transfer buffer as a class member, shared by the JTAG and SPI paths. I'll push an updated commit shortly.

On the bit reversal at parse time: agreed that it would make the temporary buffer unnecessary. I'd rather keep it out of this patch and send it as a separate PR, since it touches the bitstream parsing path for all cables.

@trabucayre

Copy link
Copy Markdown
Owner

I have just pushed (not tested) the bit reversal for all case when the bitstream is used with SRAM load. Could you confirm these commits are fine to you and simplify code?
Thanks

@bergyla

bergyla commented Sep 23, 2026

Copy link
Copy Markdown
Author

I have just pushed (not tested) the bit reversal for all case when the bitstream is used with SRAM load. Could you confirm these commits are fine to you and simplify code? Thanks

Thanks a lot for jumping in and pushing these, much appreciated.

A few words on our setup, so the test matrix below makes sense: we drive Efinix
boards (a Titanium Ti60 dev board and a Trion T8 Xyloni) in two ways. One is the
classic path, FT2232H directly on the host. The other is an XVC server running
on a small ESP32-P4 board that we're building as a network JTAG bridge, with
the FTDI adapter attached to the P4's USB host port. That second path is where
the fixed 512-byte chunking hurt, since every shift is a network round trip,
and it's what motivated this PR.

Your changes look right to me: reversing at parse time and shifting straight
from the parsed data is exactly the simplification I was hoping for. I'll take
the three commits as they are, verify SRAM loading on both boards over both
paths (FTDI direct and via the XVC bridge), and report the results here. Once
that's confirmed I'll push the constructor / member-buffer cleanup on top.

Thanks again for the quick and helpful review.

Two review comments from trabucayre on PR trabucayre#736:

- XVC_client::preferred_xfer_bits() recomputed _buffer_size*8 on every
  call. _buffer_size is only known once, right after the getinfo:
  handshake in the constructor, so compute the result there and cache
  it in a new _preferred_xfer_bits member; the override now just
  compares against the cached value.

- Efinix::programJTAG() allocated a local std::vector<uint8_t> tx(...)
  per call. Moved to a new Efinix::_xfer_buf member, resized in place
  instead of reallocated. Also applied to both Efinix::spi_put()
  overloads, which allocated a per-call VLA for the same purpose.

No behavioural change: a standalone harness that replicates
programJTAG()'s chunk loop against a protocol-compliant XVC mock
server produces byte-identical shift: command counts/sizes before and
after this commit (8 commands, 60000 B payload, diff-empty).

Redmine trabucayre#494.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants