[avr-libc-dev] boot.h: Errors in implementation of boot_lock_bits_set()

[pfaff - Markus Pfaff]

Hi,

I doubt that the code for boot_lock_bits_set will never be able to
change any boot lock bits. Here's the code from boot.h (updated from CVS
5 minutes ago):

==excerpt from boot.h==================================================

#define __BOOT_LOCK_BITS_MASK     (_BV(BLB01) | _BV(BLB02) \
                                 | _BV(BLB11) | _BV(BLB12))

...

#define __boot_lock_bits_set(lock_bits)                    \
({                                                         \
    uint8_t value = (uint8_t)(lock_bits | __BOOT_LOCK_BITS_MASK); \
    boot_spm_busy_wait();                                  \
    while(!eeprom_is_ready());                             \
    __asm__ __volatile__                                   \
    (                                                      \
        "ldi r30, 1\n\t"                                   \
        "ldi r31, 0\n\t"                                   \
        "mov r0, %2\n\t"                                   \
        "sts %0, %1\n\t"                                   \
        "spm\n\t"                                          \
        : "=m" (__SPM_REG)                                 \
        : "r" ((uint8_t)__BOOT_LOCK_BITS_SET),             \
          "r" (value)                                      \
        : "r0", "r30", "r31"                               \
    );                                                     \
})

==excerpt from boot.h==================================================


The BOOT_LOCK_BIT_MASK filters the lock bits that can be written from
software running in the avr. The idea seems to reflect the datasheet:

Datasheet of Mega32:
"The only accessible Lock bits are the Boot Lock bits that may prevent
the Application and Boot Loader section from any software update by the
MCU."

These bits can be programmed (= set to '0') by software, but they can be
cleared (= set to '1') by chip erase only:

Datasheet of Mega32:
"The Lock bits can only be erased to "1" with the Chip Erase command."

Writing a '1' to one of the bits has no effect therefore. Only writing a
'0' will have the effect of "programming" the bit:

Datasheet of Mega32:
"If bits 5..2 in R0 are cleared (zero), the corresponding Boot Lock bit
will be programmed if an SPM instruction is executed within four cycles
after BLBSET and SPMEN are set in SPMCR. The Z-pointer is don't care
during this operation, but for future compatibility it is recommended to
load the Z-pointer with $0001 (same as used for reading the Lock bits).
For future compatibility It is also recommended to set bits 7, 6, 1, and
0 in R0 to "1" when writing the Lock bits. When programming the Lock
bits the entire Flash can be read during the operation."

Without having tested this I'd propose to change the code to something
like:

#define __BOOT_LOCK_BITS_MASK (0xFF ^ 
                               (_BV(BLB01) |
                                _BV(BLB02) |
                                _BV(BLB11) |
                                _BV(BLB12)
                               )

...

#define __boot_lock_bits_set(lock_bits)
({                                     
    uint8_t value = (uint8_t)(lock_bits & __BOOT_LOCK_BITS_MASK);
    // ... as above


By the way: where are the constants BLB01 to BLB12 defined? I didn't
find them.

Markus