Lattice ECP5 JTAG Support #2213

This memcpy() wants to be for 8 bytes otherwise the upper 4 will be undefined value.

This is an over-long line - please split this into two lines. Ideally please use the multi-line comment format

/*
 * This is a 
 * multi-line
 * comment
 */

and limit the line length to not more than 120 characters

Please use lower-case in the parameter names - reg, mask, shift - upper case is reserved exclusively for macro names.

Does this have to be a uint16_t, or can it be a uint8_t, given the largest value stored appears to be 142?

This should really check that calloc() succeeds so we can not-crash if it fails - see

for an example of the general pattern for how we expect to go about this (same for the target_flash_s allocation below)

It's good that the thing ctx points to can't change, but why not the pointer itself being const? ie const ecp5_ctx_s *const ctx - it doesn't look like the ctx pointer itself can change, so might as well tell the compiler that.

Why is it reset rather than exit_flash here? Some comments on the intent involved for the steps taken would be useful for the next person please.

This has to do with SRAM programming support and also that the ECP5, as far as I can tell, has no way to properly exit SPI passthrough mode.

I'll add a note about that, or just move the reset call into ecp5_exit_flash.

As an aisd ecp5_exit_flash, just ends up calling ecp5_detach which is a stub at the moment, the ecp5_reset was in ecp5_detach but as I was working on getting SRAM programming working, when we detached and reset the FPGA it would shred the bitstream that is programming in the SRAM and have it load from flash again, which is not what we want I don't think.

I also need figure out a way to differentiate when we enter flash mode for the attached SPI flash, or are writing to SRAM, as the flow of things needs to be somewhat different there. that's still a big WIP, so it's kinda messy at the moment.

This routine appears to be a stub? what's its purpose - target->detach left NULL won't crash per the target API layer.

This is a stub from my attempting to get pure SRAM programming working, and also likely needing to issue an FPGA reset when we detach the probe.

I can remove it for now, but it might come back if I end up needing it, I just kinda left a handful of stubs/dead code around like that, but if you really want to merge this in before SRAM is working and happy and things are fully ironed out then I can just cut all that out, i'm not partial one way or another.

Please mark constants like this acting on unsigned values as U so there's no signed-unsigned conversion going on by accident. Please drop the {}'s here as they're superfluous as it's a single statement block.

We'd suggest a small rename - ecp5_free_ctx or so - the consistency of ecp5_ being a prefix is important for general consistency. Please put this function below the handler so that handler is the first function in the file.

Please prefer to calloc(sizeof(uint8_t), ctx->buffer_len) - it'll lead to better performance of calloc because of how the parameters work.

Braces can be dropped here and the if below as they're single-statement

There's no need to do the (aki) thing here.. just simply "NB: " or "Note: " on the front of the first line of the note is absolutely fine.

offset, please, rather than off - we have characters to spare, you can use them.

Same comment about single-line statement braces being droppable.

Please use lower case in hex constants. We need to get the lint pass going again on Codeberg CI, but this is a lint it would error on.

Because this needs to work when built as firmware, we can't use %zu ( 😔 ) - so please use %" PRIu32 and (uint32_t) on length.

Please replace the spaces train with \t - aproximately same output, for less characters.

Similar comment about spaces vs \t

There's no need to prefix the variable with a _ - just calling it target_lattice_defs is absolutely fine.