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

JTAG state machine #379

Closed
wants to merge 1 commit into from
Closed

Conversation

aystarik
Copy link
Contributor

No description provided.

@trabucayre
Copy link
Owner

Hi,
Do you see any objection I apply some commits manually. There is straigforward ones

  • ch347 and GD32 are unrelated to the rest and may be applied directly
  • it's also true for d03ad17 and a45a9e5 seems a good improvement.

It will be more easy to have a closer looker to others a bit more tricky.

Thanks!

@aystarik
Copy link
Contributor Author

aystarik commented Sep 19, 2023

Hi, not at all. Please feel free to cherry-pick however you like.

@aystarik aystarik force-pushed the to-upstream branch 4 times, most recently from 704f05b to 748094f Compare September 22, 2023 15:38
@aystarik aystarik changed the title GOWIN, CH347, JTAG state machine GOWIN, JTAG state machine Sep 22, 2023
src/gowin.cpp Outdated
Comment on lines 748 to 751
#define spi_gowin_write(_wr, _rd, _len) do { \
_jtag->shiftDR(_wr, _rd, _len); \
_jtag->toggleClk(6); } while (0)

Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned previously I prefer a small, inline, function to avoid too many copy of the same code sequence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, at some point I had a patch with this function added back, must have fell through...

src/gowin.cpp Outdated Show resolved Hide resolved
src/gowin.cpp Outdated Show resolved Hide resolved
src/gowin.cpp Outdated
*/
char mess[256];
try {
string hdr = _fs->getHeaderVal("checkSum");
if (hdr.empty()) {
Copy link
Owner

Choose a reason for hiding this comment

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

must be !hdr.empty()

  • if empty -> can't compare so fail
  • if !empty -> to compare to know ko/ok

src/gowin.cpp Outdated
Comment on lines 376 to 400
if (ucode != user_code) {
snprintf(mess, 256, "Read 0x%08x (checksum: 0x%08x, user_code: 0x%08x)\n",
ucode, checksum, user_code);
printError("CRC check : FAIL");
printError(mess);
}
printSuccess("CRC check: Success");
}
Copy link
Owner

Choose a reason for hiding this comment

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

maybe:

if (ucode == user_code) {
    printSuccess("CRC check: Success");
    return;
}

So if no match: message in lines 385-387 is displayed and this avoid to have twice error handling.

Also, in your code if fail, since there is not return the success message is always displayed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, rewrote it

@aystarik aystarik force-pushed the to-upstream branch 2 times, most recently from 453f5c9 to 8f27630 Compare September 24, 2023 06:58
src/gowin.cpp Outdated
Comment on lines 530 to 654
printInfo("Write SRAM ", false);
if (_verbose)
displayReadReg("before write sram", readStatusReg());
ProgressBar progress("Writing to SRAM", length, 50, _verbose);
send_command(CONFIG_ENABLE); // config enable
send_command(INIT_ADDR); // address initialize
send_command(XFER_WRITE); // transfer configuration data

_jtag->shiftDR(data, NULL, length);
progress.done();
send_command(0x0a);
uint32_t checksum = static_cast<FsParser *>(_fs)->checksum();
checksum = htole32(checksum);
_jtag->shiftDR((uint8_t *)&checksum, NULL, 32);
send_command(0x08);

send_command(CONFIG_DISABLE); // config disable
send_command(NOOP); // noop

if (_verbose)
displayReadReg("after write sram", readStatusReg());
if (readStatusReg() & STATUS_DONE_FINAL) {
printSuccess("DONE");
Copy link
Owner

Choose a reason for hiding this comment

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

please keep progressBar (I know it's a slow xfer but it's more visual for user)

@aystarik aystarik force-pushed the to-upstream branch 3 times, most recently from 0da3510 to ff320ed Compare September 25, 2023 09:10
src/gowin.cpp Outdated
Comment on lines 657 to 782
if (readStatusReg() & STATUS_DONE_FINAL) {
printError("FAIL");
return false;
} else {
printSuccess("DONE");
return true;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Why fail when flag is set?
Maybe also keeping test for STATUS_MEMORY_ERASE to check if this step is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at UG290 p.38, figure 6-15. DoneFinal==0 is success for Erase SRAM and Erase FLASH
My understanding is that DoneFinal is FPGA has loaded config in SRAM. There is no flag for success of FLASH erase as such, so they issue load after erase and if FPGA stays clean, FLASH have been erased.

@aystarik
Copy link
Contributor Author

Do you have plans to import these remaining patches or should just close the pull request?

@trabucayre
Copy link
Owner

Sorry for the big long delay.
I'm okay, I wish just to take time to do some test with real hardware. I have experimented, by the past, some issue with gowin devices (mainly littlebee) when something goes wrong.

@trabucayre
Copy link
Owner

I have tried tomorrow with a gw1n1: SRAM is working but FLASH no. I have to compare original code with modified one to see why.

@aystarik
Copy link
Contributor Author

Did erase flash happen? Major difference in erase flash -- I've made "shift-dr 32 bit" only for GW1N-4 as written in documentation, it helped with erase of GW1N-9, but may be GW1N-1 still prefers to have it. In programming main difference with original core is that I do not add blocks before the bitstream, but modify 4 bytes with auto-load pattern in bitstream itself. I've checked with LA, and gowin programmer does exactly that. Actually, may be, you'll get some insight to what gowin programmer intentions are by using programmer_cli to generate svf file -- it can do that for flash programming, and it somewhat easier to read than the LA log.

@trabucayre
Copy link
Owner

Hi,
I have created a branch with a mix between your PR and the master branch. There is a few modifications (mainly to use disableCfg/enableCfg and writeFLASH is original one but I have to compare and merge). This version works with my gw1n1 device (and I have to test with all FPGAs I have).

Using SVF is a good way but has some drawback: there is no loop so a wait for a ''timeout'' duration is applied before simply reading status with a delay more or less dependent to the device and not always true for all. When using ILA or sniffing USB traffic you have sometime access to a more rich information. So a mix between these two approach is sometime good.

@aystarik
Copy link
Contributor Author

aystarik commented Oct 23, 2023

Hi, I still can't find how you came up with

/* we have to send
* bootcode a X=0, Y=0 (4Bytes)
* 5 x 32 dummy bits
* full bitstream
*/

TN653 (TN653-1.07E, 11/18/2019) does not mention it, SVF and LA log from programmer do not show it...

@trabucayre
Copy link
Owner

In fact I have observed the 5x32 dummy bits by reading USB trafic, maybe this part is no more true and was a gowin programmer's bug.
I have to test by simply adding pattern at the beginning of the flash

@aystarik
Copy link
Contributor Author

in my devel branch i have made a small exec which only reads the fs file -- it may be of use to compare the data from LA and from the file

@aystarik
Copy link
Contributor Author

BTW, I can't find TN653 listed in gowin docs site, it might have been superseded by UG290

@trabucayre
Copy link
Owner

You can find this AN here

I have tried by removing dummy byte and it works.
When I have (2019) started to integrate gowin FPGAs I have compared fs file with USB packets content and seen this sequence (pattern + dummy) before configuration data. Maybe it's a bug somewhere, or a solution to obfuscate trafic...
The question here is to know if this pattern must be BEFORE fs or if the firt 4B must be overriden with this sequence...

@aystarik
Copy link
Contributor Author

I've compared whole file data with output from LA (whole session) and patterns replace first 4 bytes in the data stream. Everything else is the same between file and LA stream

@aystarik
Copy link
Contributor Author

No, I have no problem downloading the 2019 version of this file by link -- programmer manual still has one after the ug290 link. I've pointed out that it is not possible to find it in the list of documentation.

@trabucayre
Copy link
Owner

Ah ok. sorry for the mistake. I keep this TN because some informations was lost on UG290 (in fact I play with both app notes to find required informations).
Maybe I have to add a comment with the direct link to this file.

@aystarik
Copy link
Contributor Author

Yes, documentation is worst chinese style -- text does not explain anything, figures contradict text, and there are several documents which contradict each other and the output of their own program... BTW, have you noticed that it is possible to write auto-boot pattern over read-back pattern without flash erase -- it only needs to clear some more bits.
'''
read_pattern = 0xf0f00f0f|ab_pattern
'''

@trabucayre
Copy link
Owner

Hi.
I have tested code with more or less all Gowin I have and updated my branch.
If you're agree with modifications I apply this patch.

For readback vs autoboot: it's may interesting to add this feature to verify code after write (or to force autoboot pattern after write).

Thanks.

@aystarik
Copy link
Contributor Author

Hi, looks good, thanks!

@trabucayre
Copy link
Owner

I have applied gowin commit. Now it remains jtag rework part.

Thanks

@aystarik
Copy link
Contributor Author

Do you need any help with jtag parts?

@trabucayre
Copy link
Owner

Not now. I have to review and to find the best compromise between speed and pedagogy.

@aystarik aystarik changed the title GOWIN, JTAG state machine JTAG state machine Nov 1, 2023
@aystarik
Copy link
Contributor Author

... to find the best compromise between speed and pedagogy.
Hi, could you please describe "compromise between speed and pedagogy" in more detail? I struggle to even put these words in the same sentence...

@trabucayre
Copy link
Owner

Exactly.
In one hand there is orignal code with detail on state transitions. But this code is slow since same transition are computed all the time.
In another hand there is your approach where all transition are precomputed (at compile time ?) and automatically faster. But it's less visual/pedagogic.

@aystarik
Copy link
Contributor Author

But it's less visual/pedagogic.

My thinking was that printing of transitions from tms bits is enough? It is the commented out code in checking the correctness of tables in the beginning of the file? May be to use it if verbose was asked for?

@aystarik
Copy link
Contributor Author

aystarik commented Nov 28, 2023

dive() -- this is recursive depth first search of the shortest path on the directed graph of the JTAG state machine, recursive is safe here as the number of nodes is fixed and quite small. create_paths() calls it for every combination of states thus creating square array of paths. create_paths is called from Jtag constructor once. print_path() can then pretty-print computed TMS sequence.

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