avr-libc-dev
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [avr-libc-dev] [bug #43828] wdt.h: Wrong inline assembler arguments


From: Sivanupandi, Pitchumani
Subject: Re: [avr-libc-dev] [bug #43828] wdt.h: Wrong inline assembler arguments
Date: Thu, 21 May 2015 10:56:13 +0000

> -----Original Message-----
> From: Georg-Johann Lay [mailto:address@hidden
> Sent: Thursday, April 30, 2015 7:04 PM
> To: Sivanupandi, Pitchumani
> Cc: address@hidden
> Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments
> 
> Am 04/30/2015 um 12:09 PM schrieb Sivanupandi, Pitchumani:
> >> -----Original Message-----
> >> From: Georg-Johann Lay [mailto:address@hidden
> >> Sent: Wednesday, April 29, 2015 3:11 PM
> >> To: Sivanupandi, Pitchumani
> >> Cc: address@hidden
> >> Subject: Re: [bug #43828] wdt.h: Wrong inline assembler arguments
> >>
> >> Am 04/28/2015 um 12:53 PM schrieb Sivanupandi, Pitchumani:
> >>> Hi Johann,
> >>>
> >>> ...
> >>>> Below is the patch to fix this issue.
> >>>>
> >>>> diff --git a/avr-libc/include/avr/wdt.h
> >>>> b/avr-libc/include/avr/wdt.h index [...]
> >>>
> >>> Is this change Ok?
> >>
> >> That patch is malformed, presumably because of missing / spurious
> >> spaces or new lines.
> >
> > Attached the patch.
> 
> Several notes:
> 
> Some asm use ORI with operand of constraint r, e.g. temp or temp_reg in
> wdt_disable().  Correct constraint is d (or a subset thereof).
> 
> Some flag values are using constraint "I" which is 0 <= * <= 63.  Dunno
> whether there are devices that have flags located in bit 6 or 7.  Proposed
> constraint is "n", "I" doesn't have any benefit here.
> 
> Some memory (non-I/O) addresses use "M" as constraint which is 0 <= * <=
> 0xff.
>   Dunno whether there are devices that have SFRs with memory addresses
> outside that range.  LDS and STS are fine with "i" (known at link time) or
> even "n"
> (known at compile time).  "M" has no advantage here.
> 
> All the asms are changing memory but don't mention that.  Usually it is
> not wanted that (non-volatile) memory accesses are dragged across the
> inline asm.
> Easy fix is to clobber memory, more exact is to explicitly describe the
> effect on memory.  For example, one sequence reads:
> 
>    "sts %0, %2"
>    : /* no outputs */
>    : "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
> 
> Explicit memory modelling would read something like:
> 
>    "sts %1, %3"
>    : "+m" (_WD_CONTROL_REG)
>    : "n" (_SFR_MEM_ADDR(_WD_CONTROL_REG)),
 
I am ok with your changes.

In addition, I would like to add below change (wdt_enable for __AVR_TINY__) 
to avoid overwriting unintended bits.

@@ -202,8 +201,8 @@
       [SIGNATURE] "r" ((uint8_t)0xD8), \
       [WDTREG] "I" (_SFR_IO_ADDR(_WD_CONTROL_REG)), \
       [WDVALUE] "r" ((uint8_t)((value & 0x08 ? _WD_PS3_MASK : 0x00) \
-      | _BV(WDE) | value)) \
-    : "r16" \
+      | _BV(WDE) | (value & 0x07) )) \
+    : "r16", "memory" \
 )

 #define wdt_disable() \

> There are magic values like 0xd8, 0x8, 0x7.  Can't these be represented by
> macros from io.h?

Signature (0xD8) is not defined in device headers. 0x8 and 0x7 are bit value 
and mask.

I'll commit your patch if no other comments from anyone.

Thanks.

Regards,
Pitchumani

reply via email to

[Prev in Thread] Current Thread [Next in Thread]