avrdudes / avrdude

AVRDUDE is a utility to program AVR microcontrollers
GNU General Public License v2.0
728 stars 137 forks source link

Provide sib memory for AVR8X parts #1479

Closed MCUdude closed 1 year ago

MCUdude commented 1 year ago

All AVRs that have UPDI have a memory called SIB (Systen Infor, which lets you read various information. Even though the official documentation states that this memory is only 16 bytes, it's actually 32 bytes and contains an ASCII-formatted string.

Right now, there's no simple way of reading the SIB, as it can't be read using -Usib:r:-:r or read sib using terminal mode. It would be really neat if the SIB could be read using -U or -t !

The SIB string below was read from an ATmega4808, and is decoded by pymcuprog as such:

megaAVR P:0D:1-3M2 (01.59B20.0)

def decode_sib(sib):
    """
    Turns the SIB into something readable

    :param sib: SIB data to decode
    """
    sib_info = {}
    logger = getLogger(__name__)

    # Do some simple checks:
    try:
        # SIB should contain only ASCII characters
        sib_string = sib.decode('ascii')
    except UnicodeDecodeError:
        logger.error("SIB read returned invalid characters")
        return None

    # Vital information is stored in the first 19 characters
    if len(sib_string) < 19:
        logger.error("SIB read returned incomplete string")
        return None

    logger.info("SIB: '%s'", sib_string)

    # Parse fixed width fields according to spec
    family = sib[0:7].strip().decode()
    logger.info("Device family ID: '%s'", family)
    sib_info['family'] = family

    nvm = sib[8:11].strip().decode()
    logger.info("NVM interface: '%s'", nvm)
    sib_info['NVM'] = nvm.split(':')[1]

    ocd = sib[11:14].strip().decode()
    logger.info("Debug interface: '%s'", ocd)
    sib_info['OCD'] = ocd.split(':')[1]

    osc = sib[15:19].strip().decode()
    logger.info("PDI oscillator: '%s'", osc)
    sib_info['OSC'] = osc

    extra = sib[19:].strip().decode()
    logger.info("Extra info: '%s'", extra)
    sib_info['extra'] = extra

    return sib_info

megaAVR P:0D:1-3M2 (XX.YYYYY.0)

@SpenceKonde wrote XX is the silicon die rev represented as ascii characters that represent a single hexadecimal byte. YYYYY appears to be yet another bloody die identifier: There are two large scale clades here: The AVR Dx-series parts with word write and all sorts of yummy features like that have YYYYY = "KV00x" where x seems to have increased monotonically in order of product release, but more quickly than one would expect - some numbers between numbers or letters that are used, but are apparently not used. The shitty tiny-derived parts with the crapola 16/20 MHz fuse selectable oscillator, generally paired with much worse calibration facilities than the actual tinies have (EA certainly does take the worst of both worlds) have YYYYY = "59Bxy where y is any hexadecimal digit, and x is a 0 or 1. MegaAVR 0's have 59B2y, and EA's are 59Fyy

stefanrueger commented 1 year ago

official documentation states that this memory is only 16 bytes, it's actually 32 bytes

AVRDUDE has a good reputation of following data-sheets and .ATDF files (unless they are broken). I am at sixes and sevens over undocumented features without clear benefits and/or the user understanding or requesting that AVRDUDE uses them. What if a new part comes along when all hell breaks loose when reading 32 bytes instead of the documented 8 or 16?

MCUdude commented 1 year ago

I don't know why the datasheet only specifies 16 bytes when the SIB provides 32 bytes of data. And I'm not the correct person to decide whether it is considered "safe" to read 32 bytes or not. Can we read an arbitrary number of bytes from the SIB. or do we have to either read 8, 16, or 32 bytes? I suspect the latter. @xedbg may know more about this.

without clear benefits and/or the user understanding or requesting that AVRDUDE uses them.

I'm also not the right person to decide whether this is a useful feature or not. What I would consider useful information is NVM version, debug info, PDI osc. info and chip revision. I just wanted to point out that this memory contains information about that part that a user may want to read, but Avrdude has no easy way of accessing it.

xedbg commented 1 year ago

You can't read an arbitrary number of bytes since it uses the KEY size, which is selectable as a "size_c" field in UPDI, with 8-byte and 16-byte being datasheet options. SIB read is a low-level UPDI command that is always available and simply dumps a string from the die into the UART. This string contains both public (at the start) and not-so-public (at the end) information. So all the information in () is useful to Microchip (yes, like die identifiers), but should not really be used by AVRDUDE for example. But I am pretty sure that reading 32 bytes is safe (because this is what an Atmel-ICE does). Just parse out the useful fields and make switches based on them, and discard the 'internals'.

MCUdude commented 1 year ago

The only thing inside the parenthesis that is useful to me is the chip revision. However, this can be read in a different way, shown in PR #1474. However, When only reading 16 bytes, you're not getting full oscillator information.

xedbg commented 1 year ago

Correct - its 3M2.

stefanrueger commented 1 year ago

It would be really neat if the SIB could be read using -U or -t

This would be a special memory and would need special treatment in the updi programmers. Could probably be done. The byte read routine would need to read 8, 16 or 32 bytes (depending on the address and/or declared memory size) and return the relevant byte. SIB contents could be cached in allocated programmer memory. The byte write routine would need to read first, compare what the user wants writing with what is there and then either return all OK or return with error when the user wants to write something different (because SIB is read-only). @dbuchwald?

MCUdude commented 1 year ago

Caching the entire 32-byte memory in pdata is a good idea, and can easily be done when initializing the part. I think I can do this and submit a PR, but "emulating" it as a memory that can be read using -t/T and -U is the difficult part.

BTW I'm not sure the jtag2updi programmer can read the SIB. When looking at its source code, it seems like it reads the SIB to figure out the NVM version for internal use, but I'm not sure it can read the SIB on request.

dbuchwald commented 1 year ago

I’m not sure if I understand the question correctly, but if the idea is to read SIB as a “memory”, it would have to be emulated by caching.

Unlike literally every other memory type, this data is not being read by “read byte from offset” command, but instead “read SIB” command.

And yes, it’s always being read prior to link initialisation in serialupdi.c, because we need the SIB output (specifically - NVM interface version) to property communicate with the device.

As far as I remember, it is already being cached in ppm->cookie->sib_info (see updi_state.c), so if you want to read it as memory, just read it from from there.

See serialupdi.c lines 689-709 for handling different write operation models per memory type. I guess something similar needs to be done in lines 683-687: if reading from “sib” memory, read from the cache, otherwise, read via UPDI. I assume no paged load of SIB memory, right?

Let me know if there is a branch where you track these changes, I will implement the changes in serialupdi.c.

Dawid

stefanrueger commented 1 year ago

it’s always being read prior to link initialisation in serialupdi.c

@dbuchwald Ahh, so everything is already there for making SIB a memory, including the undocumented 32 byte readout. Brilliant. I have created a draft PR for you and others to check out. If you want to you could create your own PR based on my draft.