Re: [avr-libc-dev] [Patch] Generalize wdt.h by removing hardcoded device names

[Georg-Johann Lay]

Senthil Kumar Selvaraj schrieb:
> The attached patch to wdt.h gets rid of the huge device specific 
> conditional branches for wdt_enable and wdt_disable. The branching is
> now based primarily on architecture (xmega, tiny and everything else)
> and then on the legality of using the OUT instruction to write to the
> watch dog control register.
>
> For the latter, I had to make wdt_{enable/disable} an inline function
> and use the _SFR_IO_REG_P macro to emit different inline assembly
> fragments for OUT versus STS (as suggested by Johann in the binutils
> mailing list). The compiler optimizes away the
> range comparison done in _SFR_IO_REG_P, so the generated code should be
> identical. I verified that it is indeed identical (at -O1 and -Os),
> except for a couple of devices (atmega{8,32,64,128}a) for which the
> previous conditional branch was taking the STS path when OUT is more 
> optimal.
>
> I do see bigger code at -O0 though. Is that an acceptable tradeoff?

What's essential is that the instruction sequence is emit as expected 
which is the case as it is one single asm.  -O0 is overhead almost 
anywhere in the code, hence the user may expect that's also some 
overhead with wdt.h at -O0.

In some cases GCC gives better code at -O0 if the argument is const 
qualified, e.g. __builtin_constant_p together with

    ...  wdt_enable (const uint8_t value)

instead of

    ...  wdt_enable (uint8_t value)


"always_inline" is not C-Standard, better use __always_inline__.

Johann


> If ok, could someone commit please? I don't have commit access.
>
> Regards
> Senthil
>
> ChangeLog
>
> 2014-10-06  Senthil Kumar Selvaraj <address@hidden>
>
>         * include/avr/wdt.h: Generalize implementation of wdt_enable and
>         wdt_disable based on arch and addressability of _WD_CONTROL_REG.
>
>
> diff --git avr-libc/include/avr/wdt.h avr-libc/include/avr/wdt.h
> index 57a20c3..0c2b021 100644
> --- avr-libc/include/avr/wdt.h
> +++ avr-libc/include/avr/wdt.h
> @@ -132,45 +132,7 @@
>  */
>  
>  
> -#if defined(__AVR_ATxmega16A4__) \
> -|| defined(__AVR_ATxmega16A4U__) \
> -|| defined(__AVR_ATxmega16C4__) \
> -|| defined(__AVR_ATxmega16D4__) \
> -|| defined(__AVR_ATxmega32A4__) \
> -|| defined(__AVR_ATxmega32A4U__) \
> -|| defined(__AVR_ATxmega32C4__) \
> -|| defined(__AVR_ATxmega32D4__) \
> -|| defined(__AVR_ATxmega64A1U__) \
> -|| defined(__AVR_ATxmega64A3__) \
> -|| defined(__AVR_ATxmega64A3U__) \
> -|| defined(__AVR_ATxmega64A4U__) \
> -|| defined(__AVR_ATxmega64B1__) \
> -|| defined(__AVR_ATxmega64B3__) \
> -|| defined(__AVR_ATxmega64C3__) \
> -|| defined(__AVR_ATxmega64D3__) \
> -|| defined(__AVR_ATxmega64D4__) \
> -|| defined(__AVR_ATxmega128A1__) \
> -|| defined(__AVR_ATxmega128A1U__) \
> -|| defined(__AVR_ATxmega128A3__) \
> -|| defined(__AVR_ATxmega128A3U__) \
> -|| defined(__AVR_ATxmega128A4U__) \
> -|| defined(__AVR_ATxmega128B1__) \
> -|| defined(__AVR_ATxmega128B3__) \
> -|| defined(__AVR_ATxmega128C3__) \
> -|| defined(__AVR_ATxmega128D3__) \
> -|| defined(__AVR_ATxmega128D4__) \
> -|| defined(__AVR_ATxmega192A3__) \
> -|| defined(__AVR_ATxmega192A3U__) \
> -|| defined(__AVR_ATxmega192C3__) \
> -|| defined(__AVR_ATxmega192D3__) \
> -|| defined(__AVR_ATxmega256A3__) \
> -|| defined(__AVR_ATxmega256A3U__) \
> -|| defined(__AVR_ATxmega256C3__) \
> -|| defined(__AVR_ATxmega256D3__) \
> -|| defined(__AVR_ATxmega256A3B__) \
> -|| defined(__AVR_ATxmega256A3BU__) \
> -|| defined(__AVR_ATxmega384C3__) \
> -|| defined(__AVR_ATxmega384D3__)
> +#if defined(__AVR_XMEGA__)
>  
>  /*
>     wdt_enable(timeout) for xmega devices
> @@ -225,175 +187,7 @@ __asm__ __volatile__ (  \
>      : "r0" \
>  );
>  
> -#elif defined(__AVR_AT90CAN32__) \
> -|| defined(__AVR_AT90CAN64__) \
> -|| defined(__AVR_AT90CAN128__) \
> -|| defined(__AVR_AT90PWM1__) \
> -|| defined(__AVR_AT90PWM2__) \
> -|| defined(__AVR_AT90PWM216__) \
> -|| defined(__AVR_AT90PWM2B__) \
> -|| defined(__AVR_AT90PWM3__) \
> -|| defined(__AVR_AT90PWM316__) \
> -|| defined(__AVR_AT90PWM3B__) \
> -|| defined(__AVR_AT90PWM161__) \
> -|| defined(__AVR_AT90PWM81__) \
> -|| defined(__AVR_AT90USB1286__) \
> -|| defined(__AVR_AT90USB1287__) \
> -|| defined(__AVR_AT90USB162__) \
> -|| defined(__AVR_AT90USB646__) \
> -|| defined(__AVR_AT90USB647__) \
> -|| defined(__AVR_AT90USB82__) \
> -|| defined(__AVR_ATmega128A__) \
> -|| defined(__AVR_ATmega1280__) \
> -|| defined(__AVR_ATmega1281__) \
> -|| defined(__AVR_ATmega1284__) \
> -|| defined(__AVR_ATmega1284P__) \
> -|| defined(__AVR_ATmega128RFA1__) \
> -|| defined(__AVR_ATmega1284RFR2__) \
> -|| defined(__AVR_ATmega128RFR2__) \
> -|| defined(__AVR_ATmega164__) \
> -|| defined(__AVR_ATmega164A__) \
> -|| defined(__AVR_ATmega164P__) \
> -|| defined(__AVR_ATmega164PA__) \
> -|| defined(__AVR_ATmega165__) \
> -|| defined(__AVR_ATmega165A__) \
> -|| defined(__AVR_ATmega165P__) \
> -|| defined(__AVR_ATmega165PA__) \
> -|| defined(__AVR_ATmega168__) \
> -|| defined(__AVR_ATmega168A__) \
> -|| defined(__AVR_ATmega168P__) \
> -|| defined(__AVR_ATmega168PA__) \
> -|| defined(__AVR_ATmega169__) \
> -|| defined(__AVR_ATmega169A__) \
> -|| defined(__AVR_ATmega169P__) \
> -|| defined(__AVR_ATmega169PA__) \
> -|| defined(__AVR_ATmega16HVA__) \
> -|| defined(__AVR_ATmega16HVA2__) \
> -|| defined(__AVR_ATmega16HVB__) \
> -|| defined(__AVR_ATmega16HVBREVB__) \
> -|| defined(__AVR_ATmega16M1__) \
> -|| defined(__AVR_ATmega16U2__) \
> -|| defined(__AVR_ATmega16U4__) \
> -|| defined(__AVR_ATmega2560__) \
> -|| defined(__AVR_ATmega2561__) \
> -|| defined(__AVR_ATmega2564RFR2__) \
> -|| defined(__AVR_ATmega256RFR2__) \
> -|| defined(__AVR_ATmega32A__) \
> -|| defined(__AVR_ATmega324__) \
> -|| defined(__AVR_ATmega324A__) \
> -|| defined(__AVR_ATmega324P__) \
> -|| defined(__AVR_ATmega324PA__) \
> -|| defined(__AVR_ATmega325__) \
> -|| defined(__AVR_ATmega325A__) \
> -|| defined(__AVR_ATmega325P__) \
> -|| defined(__AVR_ATmega325PA__) \
> -|| defined(__AVR_ATmega3250__) \
> -|| defined(__AVR_ATmega3250A__) \
> -|| defined(__AVR_ATmega3250P__) \
> -|| defined(__AVR_ATmega3250PA__) \
> -|| defined(__AVR_ATmega328__) \
> -|| defined(__AVR_ATmega328P__) \
> -|| defined(__AVR_ATmega329__) \
> -|| defined(__AVR_ATmega329A__) \
> -|| defined(__AVR_ATmega329P__) \
> -|| defined(__AVR_ATmega329PA__) \
> -|| defined(__AVR_ATmega3290__) \
> -|| defined(__AVR_ATmega3290A__) \
> -|| defined(__AVR_ATmega3290P__) \
> -|| defined(__AVR_ATmega3290PA__) \
> -|| defined(__AVR_ATmega32C1__) \
> -|| defined(__AVR_ATmega32HVB__) \
> -|| defined(__AVR_ATmega32HVBREVB__) \
> -|| defined(__AVR_ATmega32M1__) \
> -|| defined(__AVR_ATmega32U2__) \
> -|| defined(__AVR_ATmega32U4__) \
> -|| defined(__AVR_ATmega32U6__) \
> -|| defined(__AVR_ATmega406__) \
> -|| defined(__AVR_ATmega48__) \
> -|| defined(__AVR_ATmega48A__) \
> -|| defined(__AVR_ATmega48P__) \
> -|| defined(__AVR_ATmega48PA__) \
> -|| defined(__AVR_ATmega644RFR2__) \
> -|| defined(__AVR_ATmega64RFR2__) \
> -|| defined(__AVR_ATmega64A__) \
> -|| defined(__AVR_ATmega640__) \
> -|| defined(__AVR_ATmega644__) \
> -|| defined(__AVR_ATmega644A__) \
> -|| defined(__AVR_ATmega644P__) \
> -|| defined(__AVR_ATmega644PA__) \
> -|| defined(__AVR_ATmega645__) \
> -|| defined(__AVR_ATmega645A__) \
> -|| defined(__AVR_ATmega645P__) \
> -|| defined(__AVR_ATmega6450__) \
> -|| defined(__AVR_ATmega6450A__) \
> -|| defined(__AVR_ATmega6450P__) \
> -|| defined(__AVR_ATmega649__) \
> -|| defined(__AVR_ATmega649A__) \
> -|| defined(__AVR_ATmega6490__) \
> -|| defined(__AVR_ATmega6490A__) \
> -|| defined(__AVR_ATmega6490P__) \
> -|| defined(__AVR_ATmega649P__) \
> -|| defined(__AVR_ATmega64C1__) \
> -|| defined(__AVR_ATmega64HVE__) \
> -|| defined(__AVR_ATmega64M1__) \
> -|| defined(__AVR_ATmega8A__) \
> -|| defined(__AVR_ATmega88__) \
> -|| defined(__AVR_ATmega88A__) \
> -|| defined(__AVR_ATmega88P__) \
> -|| defined(__AVR_ATmega88PA__) \
> -|| defined(__AVR_ATmega8HVA__) \
> -|| defined(__AVR_ATmega8U2__) \
> -|| defined(__AVR_ATtiny48__) \
> -|| defined(__AVR_ATtiny88__) \
> -|| defined(__AVR_ATtiny87__) \
> -|| defined(__AVR_ATtiny167__) \
> -|| defined(__AVR_AT90SCR100__) \
> -|| defined(__AVR_ATA6285__) \
> -|| defined(__AVR_ATA6286__) \
> -|| defined(__AVR_ATA6289__) \
> -|| defined(__AVR_ATA5272__) \
> -|| defined(__AVR_ATA5505__) \
> -|| defined(__AVR_ATA5790__) \
> -|| defined(__AVR_ATA5795__)
> -
> -/* Use STS instruction. */
> - 
> -#define wdt_enable(value)   \
> -__asm__ __volatile__ (  \
> -    "in __tmp_reg__,__SREG__" "\n\t"    \
> -    "cli" "\n\t"    \
> -    "wdr" "\n\t"    \
> -    "sts %0,%1" "\n\t"  \
> -    "out __SREG__,__tmp_reg__" "\n\t"   \
> -    "sts %0,%2" "\n\t" \
> -    : /* no outputs */  \
> -    : "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)), \
> -    "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE))), \
> -    "r" ((uint8_t) ((value & 0x08 ? _WD_PS3_MASK : 0x00) | \
> -        _BV(WDE) | (value & 0x07)) ) \
> -    : "r0"  \
> -)
> -
> -#define wdt_disable() \
> -__asm__ __volatile__ (  \
> -    "in __tmp_reg__, __SREG__" "\n\t" \
> -    "cli" "\n\t" \
> -    "sts %0, %1" "\n\t" \
> -    "sts %0, __zero_reg__" "\n\t" \
> -    "out __SREG__,__tmp_reg__" "\n\t" \
> -    : /* no outputs */ \
> -    : "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)), \
> -    "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE))) \
> -    : "r0" \
> -)
> -
> -
> -#elif defined(__AVR_ATtiny4__) \
> -|| defined(__AVR_ATtiny5__) \
> -|| defined(__AVR_ATtiny9__) \
> -|| defined(__AVR_ATtiny10__) \
> -|| defined(__AVR_ATtiny20__) \
> -|| defined(__AVR_ATtiny40__) 
> +#elif defined(__AVR_TINY__)
>  
>  #define wdt_enable(value) \
>  __asm__ __volatile__ ( \
> @@ -433,48 +227,84 @@ __asm__ __volatile__ ( \
>      : "r16" \
>  ); \
>  }while(0)
> -    
> -#else  
> -
> -/* Use OUT instruction. */
> -
> -#define wdt_enable(value)   \
> -    __asm__ __volatile__ (  \
> -        "in __tmp_reg__,__SREG__" "\n\t"    \
> -        "cli" "\n\t"    \
> -        "wdr" "\n\t"    \
> -        "out %0,%1" "\n\t"  \
> -        "out __SREG__,__tmp_reg__" "\n\t"   \
> -        "out %0,%2" \
> -        : /* no outputs */  \
> -        : "I" (_SFR_IO_ADDR(_WD_CONTROL_REG)), \
> -        "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE))),   \
> -        "r" ((uint8_t) ((value & 0x08 ? _WD_PS3_MASK : 0x00) | \
> -            _BV(WDE) | (value & 0x07)) ) \
> -        : "r0"  \
> -    )
>  
> -/**
> -   \ingroup avr_watchdog
> -   Disable the watchdog timer, if possible.  This attempts to turn off the 
> -   Enable bit in the watchdog control register. See the datasheet for 
> -   details.
> -*/
> -#define wdt_disable() \
> -__asm__ __volatile__ (  \
> -    "in __tmp_reg__, __SREG__" "\n\t" \
> -     "cli" "\n\t" \
> -    "out %0, %1" "\n\t" \
> -    "out %0, __zero_reg__" "\n\t" \
> -    "out __SREG__,__tmp_reg__" "\n\t" \
> -    : /* no outputs */ \
> -    : "I" (_SFR_IO_ADDR(_WD_CONTROL_REG)), \
> -    "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE))) \
> -    : "r0" \
> -)
> +#else
>  
> -#endif
> +static inline
> +__attribute__ ((always_inline))
> +void wdt_enable (uint8_t value)
> +{
> +       if (_SFR_IO_REG_P (_WD_CONTROL_REG))
> +       {
> +               __asm__ __volatile__ (
> +                               "in __tmp_reg__,__SREG__" "\n\t"
> +                               "cli" "\n\t"
> +                               "wdr" "\n\t"
> +                               "out %0, %1" "\n\t"
> +                               "out __SREG__,__tmp_reg__" "\n\t"
> +                               "out %0, %2" "\n \t"
> +                               : /* no outputs */
> +                               : "I" (_SFR_IO_ADDR(_WD_CONTROL_REG)),
> +                               "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE))),
> +                               "r" ((uint8_t) ((value & 0x08 ? _WD_PS3_MASK : 
> 0x00) |
> +                                               _BV(WDE) | (value & 0x07)) )
> +                               : "r0"
> +               );
> +       }
> +       else
> +       {
> +               __asm__ __volatile__ (
> +                               "in __tmp_reg__,__SREG__" "\n\t"
> +                               "cli" "\n\t"
> +                               "wdr" "\n\t"
> +                               "sts %0, %1" "\n\t"
> +                               "out __SREG__,__tmp_reg__" "\n\t"
> +                               "sts %0, %2" "\n \t"
> +                               : /* no outputs */
> +                               : "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
> +                               "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE))),
> +                               "r" ((uint8_t) ((value & 0x08 ? _WD_PS3_MASK : 
> 0x00) |
> +                                               _BV(WDE) | (value & 0x07)) )
> +                               : "r0"
> +               );
> +       }
> +}
> +
> +static inline
> +__attribute__ ((always_inline))
> +void wdt_disable (void)
> +{
> +       if (_SFR_IO_REG_P (_WD_CONTROL_REG))
> +       {
> +               __asm__ __volatile__ (
> +                               "in __tmp_reg__, __SREG__" "\n\t"
> +                               "cli" "\n\t"
> +                               "out %0, %1" "\n\t"
> +                               "out %0, __zero_reg__" "\n\t"
> +                               "out __SREG__,__tmp_reg__" "\n\t"
> +                               : /* no outputs */
> +                               : "I" (_SFR_IO_ADDR(_WD_CONTROL_REG)),
> +                               "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE)))
> +                               : "r0"
> +               );
> +       }
> +       else
> +       {
> +               __asm__ __volatile__ (
> +                               "in __tmp_reg__, __SREG__" "\n\t"
> +                               "cli" "\n\t"
> +                               "sts %0, %1" "\n\t"
> +                               "sts %0, __zero_reg__" "\n\t"
> +                               "out __SREG__,__tmp_reg__" "\n\t"
> +                               : /* no outputs */
> +                               : "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
> +                               "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE)))
> +                               : "r0"
> +               );
> +       }
> +}
>  
> +#endif
>  
>  
>  /**