Add serprog programmer

[funkeleinhorn]

This pull request adds a new programmer which allows programming over the serprog protocol used by flashrom and flashprog. The spec change to serprog enabling features like this can be found here: https://review.coreboot.org/c/flashrom/+/81428. A programmer implementation already supporting this latest spec additions which is in the process of being up-streamed can be found here: https://github.com/funkeleinhorn/pico-serprog/tree/next

The current implementation works but has quite bad performance because for each programmer command a round trip is needed which slows the flashing down. Caching the commands and sending multiple commands at once where possible would improve the performance significantly. If there is a way in avrdude to cache the commands and sending them in batches/chunks or if you have any other optimizations please let me know.

[mcuee]

Thanks for the PR. I can not test this PR but at least github action checks are all good now.

I am not able to answer your question about optimization, hopefully @stefanrueger or @dl8dtl can help.

[dl8dtl]

I don't think there's a way to cache serial communication right now, and in most cases, it wouldn't make too much sense as one had to await the response for the previous action anyway.

There might be other programmers that could benefit from it (I could think of the FTDI bit-banger), but nobody ever thought about what it needs to implement something like that.

[stefanrueger]

@funkeleinhorn Thanks for your contribution to AVRDUDE; she(!) will be all the better for it.

The code looks competently written and functional (though I have not tested it). Happy to merge the PR, in principle, after the code has addressed the following comments:

• The serprog_open() routine gets the cs parameter from the -P portname. There are programmers that use this technique but it's not recommended for new code. Please use an extended parameter -x cs=0 for ease of documentation, clarity and to allow serial adapter names for -P (where you can use, eg, -P ch340 instead of a numbered -P /dev/ttyUSBn)

• The serprog entry in avrdude.conf.in could be a bit more informative. For a start the given link should be accessible and work; it is OK to have a longer link in one line, so that it's easy to click through to the website. See also Issue #1437 for an example of what makes a good entry.

• Better use mmt_malloc(n) instead of cfg_malloc(__func__, n) and mmt_strdup(s) instead of cfg_strdup(__func__, s). This is a recent thing

• Instead of free(pgm->cookie); better use

     mmt_free(pgm->cookie);
     pgm->cookie = NULL;
  

  This hardens against accidental double pgm->teardown() (a recent thing, too)

• I think most bits of serprog_close() belong to serprog_disable(). Historically, ->program_enable() was used to enter programming mode and ->disable() to leave programming mode (I realise their names don't make this very clear). Please review whether the initial part of serprog_close() should actually be put in serprog_disable(). This does not matter for AVRDUDE, but consider a libavrdude application that might do a few pgm->program_enable() and pgm->disable() sequences within a single pgm->open() and pgm->close() stretch. I also realise that this might be wrong in other programmers that you may have chosen as a model, but the team has not yet come round to make all aspects of existing programmer code ready for libavrdude use (which is our fault not yours).

• Programmers should never exit(), they should return error codes and let caller decide what to do. (Imagine a gui built with libavrdude that runs all day long; if the programmer exits, the gui exits too, and the user would think the gui had crashed; see Issue #774)

• Please review the use of the cmd_bitmap array and whether the local copies of it are really needed. Maybe they are and I didn't see it, maybe not. Note that the struct pdata variables are all akin to static variables for all intents and purposes; you might #define my (*(struct pdata *)(pgm->cookie)) and use my.cmd_bitmap throughout.

• Serprog should be documented in avrdude.texi and avrdude.1, at least for the -x extended parameter

• I find (cmd_bitmap[cmd / 8] >> (cmd % 8)) & 1 clearer than (((cmd_bitmap[cmd / 8]) >> cmd % 8) & 1) == 1 (most programmers know a[x] has pretty high precendence, but precedence of a >> b % c is not necessarily clear to all)

• AVRDUDE messages normally start with lowercase and rarely use the exclamation mark, so please change to this style pmsg_error("can not get list of supported serprog commands\n")

• Although AVRDUDE exhibits many different coding styles owing to a range of contributors over the decades, I encourage this coding style; this is not mandatory if you feel strongly about your "handwriting", but I still encourage the following:

  <  #define S_CMD_NOP               0x00    /* No operation                                 */
  ---
  > #define S_CMD_NOP          0x00 // No operation
  

  and similar (comments to right of code best start at column 32, counting from 0). Also I find the following clearer

  136,141c134
  <     if (resp_status_code == S_ACK)
  <         return 0;
  <     else if (resp_status_code == S_NAK)
  <         return 1;
  <     else
  <         return -1;
  ---
  >     return status == S_ACK? 0: status == S_NAK? 1: -1;
  

Thanks again for your cool contribution, @funkeleinhorn. I look forward to merge it.

[stefanrueger]

sending multiple commands at once where possible would improve the performance significantly

In this case the programmer would need to be able to cache the commands, too. It would need to be able to read from serial input at the same time as processing replies and doing the programming (and the serial line would need to be full duplex). Could be done in the firmware of the hardware programmer, in theory (eg, interrupt-driven serial I/O with large buffers), but AVRDUDE tends not to expect a lot from the programmer FW implementation. So, I prefer slower programming that is robust and works with a large range of hardware.

[stefanrueger]

Please use an extended parameter -x cs=0

Assuming, cs is a pin name that needs a number assigned, the other possibility is to introduce another parameter cs for programmers and use cs = ...; in serprog's programmer definition. One could introduce cs in the same way as vfyled: try

$ grep -r vfyled src

Whether -x cs=0 or a new programmer pin name is better depends on likely use and whether existing hardware exists that already requires a particular mapping (eg, because it is sold with particular cables that make a particular connection). If the latter, I'd prefer a new programmer pin name. If every user rigs up the hardware in an arbitrary way, then -x cs=... is probably the better solution.

[stefanrueger]

I have taken the liberty to suggest message adaptation to AVRDUDE style and a few other small bits where review would be more cumbersome than just suggesting commits. The cs parameter is the main outstanding bit along with moving most of the _close() code into the _disable() function.

[MCUdude]

@funkeleinhorn does there exist a pre-compiled binary I can drag and drop onto my Pi Pico? If yes, we can point to this in the Avrdude documentation, to make it easier for those who want a working, RP2040-based programmer without having to compile the firmware themselves.

[funkeleinhorn]

@funkeleinhorn does there exist a pre-compiled binary I can drag and drop onto my Pi Pico? If yes, we can point to this in the Avrdude documentation, to make it easier for those who want a working, RP2040-based programmer without having to compile the firmware themselves.

Sadly pico-serprog does not have binary releases at the moment. I think this should be changed in the future. I will suggest this to stacksmashing but he is a bit slow to respond. An easy option to get a binary at the moment is to do: nix build github:funkeleinhorn/pico-serprog/next but this requires Nix with Flakes enabled and still building for yourself.

According to the fork of pico-serprog from Riku_V there should be binary releases from libreboot but I was not able to find them and they lack support for the latest spec additions needed for this programmer.

[funkeleinhorn]

I have taken the liberty to suggest message adaptation to AVRDUDE style and a few other small bits where review would be more cumbersome than just suggesting commits. The cs parameter is the main outstanding bit along with moving most of the _close() code into the _disable() function.

Thanks for this contribution! I will fix the remaining parts and then perform a test with actual hardware to see if everything is still working.

[funkeleinhorn]

along with moving most of the _close() code into the _disable() function.

I guess this means moving the matching parts from open() to enable() too so an disable() can be undone with an enable() call right? I will fix this accordingly. If I am wrong and the enable() function can stay empty let me know and I revert the changes to the open/enable part.

[funkeleinhorn]

Okay I think I addressed all of the comments now. I was wondering if there is a way to do error handling in enable()/disable() as they both have an return type of void? @stefanrueger @dl8dtl could you please have a look again if everything is fine now? When I am back at our local hackerspace later I will test the latest version against actual hardware and let you know if everything is still working.

[stefanrueger]

disable() can be undone with an enable() call right?

You would think so from the naming, but no. I have come late to the party and am not aware of the full AVRDUDE history, but reading programmer code suggest to me that pgm->disable() undoes a pgm->program_enable(). Note it is program_enable() rather than enable(). The programmers for classic parts required a step that was called enter programming mode (put into pgm->program_enable()) and another one leave programming mode (put into pgm->disable()).

And yes, pgm->close() should bracket with pgm->open().

pgm->enable() is normally used to change the PROGRAMMER structure, if the part that's been chosen requires that. Looks like a hack, and I suspect that's so because it is. I don't think deploying enable() is useful in serprog.c

The order of calls in main.c (and that's the gold standard) is

• pgm->initpgm(pgm)
• pgm->setup(pgm)
• pgm->parseextparams(pgm, extended_params)
• pgm->open(pgm, port)
• pgm->enable(pgm, p)
• pgm->initialize(pgm, p) which normally calls pgm->program_enable(pgm, p) as last thing
• pgm->end_programming(pgm, p) (which is rarely defined for programmers)
• pgm->powerdown(pgm) (before pgm->disable(), go figure!)
• pgm->disable(pgm)
• pgm->close(pgm)
• pgm->teardown(pgm)

I haven't done enough AVRDUDE archaeology to figure out how this all comes about, but conceptually it must have made sense in the beginning. I suspect (but don't know) that at some point some -c programmer deviated from the intended pattern, introduced their own and others copied/pasted the wrong programmer function functionality and thus cemented it for eternity.

It wouldn't matter for AVRDUDE as long as programming works, BUT we now are in a phase where the team focuses more on libavrdude functionality. For that we ought to make sure that the function functionality is similar/comparable across programmers.

So, sadly, not knowing what's supposed to be undone by what and in which order, I cannot give more specific advice rather than asking you, @funkeleinhorn, as the expert for serprog.c to carefully read above and make the right decision what to move where. It could be that your original choice was correct and I was mistaken in my belief that most things in close() ought to be moved to disable().

error handling in enable()/disable() as they both have an return type of void?

A very good point! And I don't have an answer to that. I thought about changing the function signature a couple of years ago but decided against that as it would have been a lot of work. So right now we are unable to inform the caller of errors in these functions...

[dl8dtl]

I haven't done enough AVRDUDE archaeology to figure out how this all comes about, but conceptually it must have made sense in the beginning.

Most of that is owed to how the actual ISP protocol works, that was the beginning of AVRDUDE (usually over a printer port by that time). Later on, stuff has been added to cope with "more intelligent" protocols like STK500, and so on.

[funkeleinhorn]

Okay I looked into the lifecycle of programmers as described above and decided that in my eyes it makes most sense to move the parts from enable() to initalize(). This allows to open a serprog programmer when using libavrdude without interfering with the SPI bus as the programmer only starts to output signals to the SPI bus in initalize() directly before the actual programming starts and stops directly afterwards in disable(). This way the AVR can communicate with SPI flashes etc. on the same bus between programming iterations while the serprog programmer can stay open. I tested my latest commit e72d67285959ff5f7bc9b9f0071ccb90fa43b422 with my fork of pico-serprog against an ATmega128RFA1:

❯ sudo ./build_linux/src/avrdude -c serprog -P /dev/ttyACM0 -p m128rfa1 -x cs=1 -U lock:r:lock.bin
avrdude: serprog protocol version: 1
avrdude: programmer name: pico-serprog
avrdude: AVR device initialized and ready to accept instructions
avrdude: device signature = 0x1ea701 (probably m128rfa1)

avrdude: processing -U lock:r:lock.bin:r
avrdude: reading lock memory ...
avrdude: writing output file lock.bin

avrdude done.  Thank you.

❯ sudo ./build_linux/src/avrdude -c serprog -P /dev/ttyACM0 -p m128rfa1 -x cs=1 -U flash:r:flash.bin
avrdude: serprog protocol version: 1
avrdude: programmer name: pico-serprog
avrdude: AVR device initialized and ready to accept instructions
avrdude: device signature = 0x1ea701 (probably m128rfa1)

avrdude: processing -U flash:r:flash.bin:r
avrdude: reading flash memory ...
Reading | ################################################## | 100% 81.97 s 
avrdude: writing output file flash.bin

avrdude done.  Thank you.

I think this programmer implementation is now ready for review/testing. @stefanrueger @dl8dtl please have a look if everything is fine now and if you think this looks like the right place for this code.

[funkeleinhorn]

I have not looked into detail yet but I was wondering if pgm->paged_write() and pgm->paged_read() could be used to speed up reading/writing the flash as it seems like they need way less round trip times as they can write bigger chunks of data then successive pgm->cmd() calls?

[stefanrueger]

@funkeleinhorn Thanks for reviewing the use of pgm-> functions and updating the programmer!

wondering if pgm->paged_write() and pgm->paged_read() could be used to speed up reading/writing

Sure! That is the usual way to go, provided the programmer FW itself can r/w a block of data from eeprom/flash. Paged r/w expects to be given the task of r/w-ing integer multiples of page_size bytes at page boundaries. Devices that have more flash than 128 kB, eg, the ATmega2560, normally need sending a load extended address ISP command load_ext_addr before r/w-ing a page. This is in addition to setting the 16-bit word-address (and you can tell that a device has more than 128 kB flash if it sports that load_ext_addr command). The majority of programmers implements paged r/w. Have a look, eg, at avrftdi.c. I am not saying it's particularly similar/related to your programmer (it's probably not) but just so you see one way someone has implemented that. You should do whatever works best with your programmer FW.

Also, only ATmega256RFR2 ATmega128RFR2 ATmega64RFR2 ATmega2564RFR2 ATmega1284RFR2 ATmega644RFR2 have 3 pages worth of usersig memory and page size 256. That is a third page-oriented memory of classic parts that almost no ISP programmer has considered so far. Your's might be an exception? Assuming the programmer FW can handle usersig that is.

Are you particularly invested in the formatting of serprog.c or it is just that you happened to use an old template that uses outdated indentation? If the latter, I'd be happy to run the code through an indentation program after you are finished and before squashing and merging your PR.

[funkeleinhorn]

Sure! That is the usual way to go, provided the programmer FW itself can r/w a block of data from eeprom/flash.

Okayy I will then look into that the next days and see if I can implement that for my programmer to improve the performance.

Are you particularly invested in the formatting of serprog.c or it is just that you happened to use an old template that uses outdated indentation?

No I do not really have a strong opinion on the formatting. Feel free to fix it to match the style of the project before you merge it.

[stefanrueger]

usersig

don't worry about usersig: it cannot be programmed via ISP

https://github.com/avrdudes/avrdude/blob/37ac6ebaa0330470d14cff37256dfd94293fda83/src/avrdude.conf.in#L14159-L14168

So, it's flash and eeprom for paged r/w via ISP. The serprog prtotocol seems to have read n bytes and write n bytes commands, which might be used for paged r/w. The protocol doesn't mention how to select the memory, so I assume it's flash? In which case you'd have to fall-back on a looped bytewise r/w for eeprom paged r/w.

[stefanrueger]

@funkeleinhorn Pro tip: once you come round to it, testing is best done via

$ ./tools/test-avrdude -p "-c serprog -P /dev/ttyACM0 -p m128rfa1 -x cs=1" -d 0

See here for other examples.

[funkeleinhorn]

I addressed your comments and reworked the relevant parts apart of paged read and write. I will try to implement paged reads/writes but it could take some time as I am quite busy at the moment. If you want this could be done in a separate PR as I think it is an improvement but not a blocking feature.

[funkeleinhorn]

The serprog prtotocol seems to have read n bytes and write n bytes commands, which might be used for paged r/w.

I would use the 0x13 Perform SPI operation for the paged write and read as it is also used for the cmd() handler and allows for full-duplex communication. Also it is more widely supported by programmer as far as I know.

In which case you'd have to fall-back on a looped byte-wise r/w for EEPROM paged r/w.

If I have read the ISP protocol spec correctly EEPROM only supports byte-wise access via ISP anyways. From section 3.4:

A device with Page Programming mode of the Flash will, however, use byte programming for the EEPROM memory.

[stefanrueger]

@funkeleinhorn Thanks for the changes!

I would use the 0x13 Perform SPI operation for the paged write and read as it is also used for the cmd() handler

OK, I think I have now a better appreciation of the serprog programmer: If I understand it correctly, it sets up a hw comms channel via SPI to the AVR part; then the full SPI commands are sent via that channel. The firmware of the serprog programmer is agnostic to the attached thingy (could be anything from the BIOS of a nuclear power plant control panel to an ATtiny13 MCU on a breadboard); therefore the paged r/w would need to send exactly the same number of bytes (SPI commands and data payload) to the external serprog programmer as if it was doing bytewise r/w: I predict that the time savings from the paged r/w command in terms of overhead would be modest at best.

I have run an indentation program over the source and made small stylistic changes. Check out git diff -w HEAD~2

The only real change is replacing bitclock with 1/bitclock as the source code has pgm->bitclock as clock period in seconds: https://github.com/avrdudes/avrdude/blob/a3e4c8083e38b257915881999c16e2e4438ec6d1/src/main.c#L1396-L1399

Would you like a squash merge or do you prefer the current history? @funkeleinhorn

I have one last (I promise) question, see below.

The only other thing Ieft to do would be to test the programmer, ideally by reporting the outcome of this bash command

$ ./tools/test-avrdude -p "-c serprog -P /dev/ttyACM0 -p m128rfa1 -x cs=1 -B 4" -d 0

The -B 4 also tests whether a user SPI frequency of 250000 Hz can be set. Can you do that @funkeleinhorn? Or you @MCUdude, @mcuee? Would be brill!

[funkeleinhorn]

Output of sudo ./tools/test-avrdude -p "-c serprog -P /dev/ttyACM0 -p m128rfa1 -x cs=1 -B 4" -d 0 -e ./build_linux/src/avrdude for commit acf59cb61c86b49dd13118e571784141f7cdd35e

Testing ./build_linux/src/avrdude version 7.3-20240521 (acf59cb6)
Prepare "-c serprog -P /dev/ttyACM0 -p m128rfa1 -x cs=1 -B 4" and press 'enter' or 'space' to continue. Press any other key to skip
✅   0.065 s: fuse access: clear, set and read eesave fuse bit
✅   0.058 s: fuse access: set eesave fusebit to delete EEPROM on chip erase
✅   0.088 s: chip erase
✅  25.743 s: flash -U write/verify holes_rjmp_loops_131072B.hex
✅  25.380 s: flash -T write/verify holes_rjmp_loops_131072B.hex
✅   0.088 s: eeprom check whether programmer can flip 0s to 1s
✅  14.681 s: eeprom -U write/verify holes_pack_my_box_4096B.hex
✅  27.899 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_4096B.hex
✅  12.781 s: chip erase and spot check flash is actually erased
✅   0.663 s: spot check eeprom is erased, too

Would you like a squash merge or do you prefer the current history? @funkeleinhorn

It think I would prefer the current history.

I have run an indentation program over the source and made small stylistic changes.

Is this indentation program and your config somewhere available? If so I could format my future commits accordingly.

[stefanrueger]

@funkeleinhorn That test output looks amazing. Shows your implementation works correct and that all functional features are there for the user. I carefully read your responses (thank you), and I think that all is good. Will merge at the next occasion. Thank you for a smashing contribution.

[funkeleinhorn]

Thanks for all the help and comments to get this ready to be merged! I really appreciated all your feedback on this and hope AVRDUDE users will find this new programmer useful. I was wondering if I should get added to the AUTHORS file as it (if I understand it correctly) lists all copyright holders of AVRDUDE and with this being merged I now also hold a copyright. If so you could use the same information as in serprog.c and serprog.h: Sydney Louisa Wilke <[email protected]>

[stefanrueger]

@funkeleinhorn Great shout: I agree you should be part of the the AUTHORS list. You have contributed a programmer, and that defo qualifies in my view!

I've had a quick look (some grep, sed, comm ju-jitsu), and the file looks very much unmaintained. I found the following ones with copyright notices in one file or another but, like @funkeleinhorn, no mention in AUTHORS:

Old crew?
      3 Erik Walthinsen
      1 Christian Starkjohann
      3 David Moore
      1 Klaus Leidinger <[email protected]>
usbtiny 2007
      1 Dick Streefland
      1 Limor Fried
ch341a 2016
      2 Alexey Sadkov
avrftdi_jtag 2023
      1 Jeff Kent
linuxspi 2013
      3 Kevin Cuzner <[email protected]>
linuxspi 2018
      2 Ralf Ramsauer <[email protected]>
dfu/flip1/flip2 2012
      4 Kirill Levchenko
arduino 2009
      2 Lars Immisch

linuxgpio 2013
      2 Radoslav Kolev <[email protected]>
linuxgpio 2023
      1 Sebastian Kuzminsky <[email protected]>
serprog 2024
      2 Sydney Louisa Wilke <[email protected]>

ft245r 2011-12 (some code?)
      1 Roger E. Wolff <[email protected]>

I suggest to add all these to AUTHORS in historic order (ie, most recent contribution further down) unless I hear otherwise from @dl8dtl @mcuee @mariusgreuel @MCUdude who might have other ideas on order or who might be able to judge whether or not the partial contribution to programmers (eg, Wolff) warrants mentioning in AUTHORS.

And then there are the following who are listed, but have no obvious mention in the code as authors somewhere:

	Jan-Hinnerk Reichert <[email protected]>
	Alex Shepherd <[email protected]>
	Colin O'Flynn <[email protected]>
	David Hoerl <[email protected]>
	Rene Liebscher <[email protected]>
	Jim Paris <[email protected]>
	Jan Egil Ruud <[email protected]>
	David Mosberger <[email protected]>
	Xiaofan Chen

This might be justified (as, for example, in Xiaofan's case who has contributed a lot in terms of testing, pulling issues together and maintaining a brilliant overview of platforms and systems). For others I don't know, but assume similar level of contributions in the past. Again, unless I hear from @dl8dtl etc I suggest keeping the names in.

[dl8dtl]

Usually, the people mentioned there who are not in any copyright statement contributed otherwise during development. For example, I remember David Hoerl being one of the early adopters for MacOS. I'm surprised Jan-Egil Ruud didn't appear in any copyright; he contributed the UPDI code to jtag3.c.

You'll probably find some of those folks mentioned in the old ChangeLog files which we now abandoned. Some of their names (like Jan-Egil) also went into the Git logs upon transitioning the project from Savannah.

I would probably have chosen to order the folks alphabetically (regardless of by first or last name), but it's also fine by me to try maintaining some kind of "historical order".

[stefanrueger]

wondering if I should get added to the AUTHORS file

@funkeleinhorn done