[avr-libc-dev] Re: more bootloader info. (fwd)

[Theodore A. Roth]

I'm forwarding this to the dev list to get wider input before I consider
altering the boot API.

Comments?

Ted Roth

---------- Forwarded message ----------
Date: Tue, 24 Feb 2004 12:07:29 -0700
From: E. Weddington <address@hidden>
To: Theodore A. Roth <address@hidden>
Subject: Re: more bootloader info.

On 23 Feb 2004 at 22:05, Theodore A. Roth wrote:

> Hi Eric,
>
> I'm going to try getting the bootloader docs updated this week.
>
> Here's my final page erase and write function. There's two versions. One is
> based on the example in the docs and the other is hopefully optimized down to
> the bare minimum code to get the job done. I needed to resort to some asm to
> avoid the eeprom check in boot_rww_enable(). I'm wondering if there should be
> another macro added to <avr/boot.h> since boot_rww_enable() is really 
> overkill.

I would agree with another macro, or rather, paring down of boot_rww_enable().
See more below.

> My final bootloader ended up being 2041 bytes. It does the crc-xmodem, and a
> whole mess of validations. If you are interested in having a look at it, let 
> me
> know and I'll shoot you a tarball. I don't think it's really suited for
> inclusion in in avr-libc since it does a lot of things that the casual user
> isn't going to care about.

I would suggest asking the list (or at least Joerg) about that. If you're
willing to open source license the xmodem bootloader, I think it would be a
great addition!

Or, at the very least, a better bootloader API, and perhaps some xmodem API and
an example of how to put it together. ..... Of course that's asking for a lot,
but I think it would look great!

>
> #if 0
> static void
> boot_page_erase_and_write (uint32_t page, uint8_t *buf)
> {
>     int i;
>
>     boot_page_erase (page);
>     while (boot_rww_busy ())
>     {
>         boot_rww_enable ();
>     }
>
>     for (i=0; i<SPM_PAGESIZE; i+=2)
>     {
>         /* Set up little-endian word. */
>         uint16_t w = *buf++;
>         w += (*buf++) << 8;
>
>         boot_page_fill (page + i, w);
>     }
>
>     boot_page_write (page);
>     while (boot_rww_busy ())
>     {
>         boot_rww_enable ();
>     }
> }
> #else
> /* This method came from Markus Pfaff via this post to avr-libc-dev mailing
>    list:
>
>      http://mail.gnu.org/archive/html/avr-libc-dev/2003-09/msg00021.html
>
>    I've modified it to avoid extra checks for eeprom is ready since you only
>    need to do that once up front. This saves quite a bit of code space over 
> the
>    method above.
>
>    This version assumes that you are running with interrupts globally
>    disabled. But then again, so does the version above. */
>
> static void
> boot_page_erase_and_write (uint32_t page, uint8_t *buf)
> {
>     int j;
>
>     while (!eeprom_is_ready ())
>         ;
>
>     boot_page_erase (page);
>     boot_spm_busy_wait ();      /* Wait until the memory is erased. */
>
>     for (j=0; j<SPM_PAGESIZE; j+=2)
>     {
>         /* Set up little-endian word. */
>         uint16_t w = *buf++;
>         w += (*buf++) << 8;
>
>         boot_page_fill (page + j, w);
>     }
>
>     boot_page_write (page);     /* Store buffer in flash page. */
>     boot_spm_busy_wait();       /* Wait until the memory is written. */
>
>     /* Reenable RWW-section again. We need this if we want to jump back to the
>        application after bootloading. */
>     /* FIXME: I'd really like to avoid the inline asm here, but the boot API
>        in <avr/boot.h> doesn't provide this yet and the boot_rww_enable()
>        macro provides more than we need here. */
>     asm volatile (
>         "sts %0, %1\n\t"
>         "spm\n\t"
>         : "=m" (__SPM_REG)
>         : "r" ((uint8_t)__BOOT_RWW_ENABLE)
>     );
> }
> #endif

Of course, seeing the second algorithm, and how much simpler it is I think I
would prefer going with that.

This may break previous code, but I think that you now have a better
understanding than I did of what it takes to implement a bootloader, and I
would suggest refactoring boot.h to show this. 'Cause if what I wrote is
bloated (which you've shown it is), then let's remove the bloat.

Currently the boot_rww_enable() macro is defined as:


#define __boot_rww_enable()                      \
({                                               \
    boot_spm_busy_wait();                        \
    while(!eeprom_is_ready());                   \
    __asm__ __volatile__                         \
    (                                            \
        "sts %0, %1\n\t"                         \
        "spm\n\t"                                \
        : "=m" (__SPM_REG)                       \
        : "r" ((uint8_t)__BOOT_RWW_ENABLE)       \
    );                                           \
})

I would suggest removing the first 2 lines:

boot_spm_busy_wait();
while(!eeprom_is_ready());

and this is equivalent to what you need.

I would also *strongly* suggest adding an eeprom macro, something like:

#define eeprom_busy_wait()    do{}while(!eeprom_is_ready())

That way you can write:

eeprom_busy_wait();
boot_page_erase (page);
boot_spm_busy_wait ();      /* Wait until the memory is erased. */
...

I don't care what you name the macro. But I think this helps to avoid confusion
about an empty while loop; new users don't necessarily think to look for a
semicolon immediately following a while statement.

------

Again, sorry for the hassle and thanks for cleaning it up.

HTH
Eric