AN2720SW - Some feedback

[colinh]

Hi All

The "Codewarrior Stationery" version of AN2720SW (or at least the one I found on the forum) has a few errors and/or bits of misleading coding. From flash.c:

    FSTAT |= (FSTAT_PVIOL|FSTAT_ACCERR);   /* Clear any errors  */

There are a couple of variations on the above in the code, and I think these should all be replaced with
FSTAT = (FSTAT_ACCERR_MASK | FSTAT_PVIOL_MASK); 
which is how it is done in some other places in the code.  There are two reasons for this, (1) The |= assignment causes a read/modify/write sequence which can inadvertently clear other flags, (2) The FSTAT_PVIOL and FSTAT_ACCERR are the bit members of the struct - doing a bit-wise OR of them doesn't really make sense (and I don't consider myself a complete C guru but I think the behaviour of such a bit-wise OR, as opposed to a logical OR, might be undefined)


    FCLKDIV |= fclk_val | FCLKDIV_PRDIV8_MASK;

The |= assignment look suspect - why or in the old value?  Will work OK (since out of reset FCLKDIV is zero and it is a write once register) but it looks like bad coding.

So, does anyone know of a post 09/09/2004 version of this code? 


Thanks
Colin

[Lundin]

To sum up this topic...

It would be great if Freescale made their registers compatible with the C language. C programmers clearing flag registers with read-modify-write is a very common bug I see in code written for Freescale processors. From a C programming point-of-view, the programmer has done nothing wrong. His code can be fully compatible with ISO C, as well as a bunch of programming standards for safety-critical systems, yet it contains this bug.

The same applies to oddities such as the SPI and SCI status flags that are cleared by reading the register. The C standard does not cover that. A read instruction can never be a write instruction, or so you thought before writing code for Freescale micros. Plus, all the debugger manufacturers are unaware of these registers and like to read them as often as possible, ruining your debug-session. Codewarrior is one example of this.

You can always argue and say that the programmer should know the hardware. But it would be a whole lot easier for people to learn the hardware if a flag was cleared in a sane way, ie by writing 0 to it. Not by writing 1 to it, or reading it. All three combinations exist in the S12.

[kef]

Lundin wrote:
It would be great if Freescale made their registers compatible with the C language. C programmers clearing flag registers with read-modify-write

Freescale registers *are* compatible with C.

is a very common bug I see in code written for Freescale processors. From a C programming point-of-view, the programmer has done nothing wrong. His code can be fully compatible with ISO C, as well as a bunch

Nothing wrong, just a code with a bug.

of programming standards for safety-critical systems, yet it contains this bug.

The same applies to oddities such as the SPI and SCI status flags that are cleared by reading the register. The C standard does not cover that.

Freescale isn't alone and not the first to use such technique. For example old already Philips SCC2692 UART chip. IIRC it had status/control register. When you read it you read status, when you write it you write access write-only control register.

A read instruction can never be a write instruction, or so you thought before writing code for Freescale micros. Plus, all the debugger

Something new to learn, thought after code is done :smileyhappy:.

manufacturers are unaware of these registers and like to read them as often as possible, ruining your debug-session. Codewarrior is one example of this.

It's bad, but it's also good. When FSL releases old family but new chip, with more SCI's or different addresses, debugger doesn't have to be redone for this tiny change. Codewarrior is part of FSL and has all necessary informations in time, but what about third parties?

You can always argue and say that the programmer should know the hardware. But it would be a whole lot easier for people to learn the

That's right, programmer should know hardware.

hardware if a flag was cleared in a sane way, ie by writing 0 to it. Not by writing 1 to it, or reading it. All three combinations exist in the S12.

No.

    In FSL, in order to keep flags untouched, we write all zeros to flags register. In Microchip PIC - we should write all ones! Why PIC works with such "careless programming" and FSL doesn't? I think it's because PIC peripherals are synchronized to instruction clock, which is osc/4; and PIC completes its R-M-W instruction (everything: read, modify and write) at osc clock. Hardware can't set more flags while CPU is modifying others using bit set/ bit clear instructions. In contrast FSL CPU12 and peripherals are operating at the same clock. Instruction takes more than one clock tick. Peripheral can flip some bits after CPU read flags but before it wrote back to flags. Writing an one or writing a zero ... it's practically the same. I prefer writing 0x10 to clear bit4, rather than writing 0xEF. Two solutions exist:

1) PIC variant: clocking peripherals at bset/bclr instruction time. 2) S08 variant: not packing more than one flag to the same register. (Note again that recent S08 additions: IIC, flash, and maybe more peripherals have more than one flag per register and thus are not bitfields compatible).

Regards

[JimDon]

Kef,
 "Peripheral can flip some bits after CPU read flags but before it wrote back to flags"

Is that the reason you are not supposed to OR in these registers?

What you say mans a lot of sense for a status registers. If course on an  "write one to clear" registers, it could also reset other bits (if they were set to a 1) that perhaps you did not mean to clear.

It could also work to your advantage:

REGISTER != 0;   // clear all currently set flags

In the case of this flash code it is harmless, but on an interrupt status register, it could be a real bug if you cleared the status of one interrupt in anothers handler by mistake.

[kef]

Jim,

[Peripheral can set some flags between instruction read and write cycle] - that's the reason for the need to do something regarding unwanted clearing the flag, which happens in the cases when hardware sets flag between CPU flags read and flags write cycles.

My first MCU was HC11 and it had 5 output compares + 3 input captures timer. Single register for all 8 IC/OC flags. Probably it was planned first as Lundin suggested, clear bit by clearing or zeroing or writing zero to it. But in this case we have to write ones to other flags. That would mean that we need to 

flags = ~(1<<flag_bit_position);

or in HC11 asm

LDAA   #0xEF  ; for  bit 4 

STAA  flags

And what about bset/bclr? These can't be used at all since

BSET  flags,x, #0xEF  ; won't do anything, bit 4 is set

BCLR  flags,x,#0x10   ; could clear flags that happen between R and W cycles

Dead end. But making it "write one to clear" makes it possible to use BCLR to clear flags.

BCLR  flags,x,#0xEF   ; will clear just bit4 and nothing else! bingo

But that's only a guess. But if I'm right, then HC05/08 developers could be pushed (maybe) to use single flag/register because HC08 BSET/BCLR can't clear/set more than one bit, and that's incompatible with more than one flag per register, no combination would work

bset flags,#4   ; (hc08 bset) bad, no matter, do we need to write 0 or 1 to clear flag

bclr flags,#4    ; (hc08 bclr) bad, no matter, do we need to write 0 or 1 to clear flag

Regards

[colinh]

We're getting slightly off forum (8 bit micros) but I'm cool with that...

With respect to hc05/hc08 you still need to be careful using bset/bclr on a register if it has any flags that can be cleared by writing 1 (I'm pretty sure I've stumbed across some over the years)

This is because, even though they can only modify a single bit, bset and bclr still work at the byte level - they just do it automatically for you. The CPU reads the whole byte, sets or clears the appropriate bit, and writes back the result - have a look at the cycle by cycle execution for these instructions in the family user guide if you are interested (you can also see this happening on an MMDS emulator cycle trace)

If you want to clear/set multiple bits you need to do this yourself doing the old LDA, AND/OR, STA sequence with appropriate bit mask. You would generally protect this with a SEI/CLI around it to make it interrupt safe - unless of course it's in an ISR.

Colin

[kef]

You must know it better, I've no HC05 and almost no S08 experience. But I saw in S08 datasheets that many peripherals do have single interrupt flag per register combined with some R/W configuration bits in the same register. These flags (RTI, TPM timer, ADC etc peripherals) seem to be BSET/BCLR and bitfields safe, you wan't loose interrupts unless you change config bits.

What's interesting is that some S08 peripherals, those with single flag per register, have some flags clearable by writing 0, and others  - by writing 1.

BTW, another example of AN with flag clearing bugs: AN3291 How to Use IIC Module on M68HC08, HCS08, and HCS12 MCUs.

1) Example 6 at page 8:

IIC1S_IICIF=1; // clear the interrupt event flag;

^^ write accessing a bitfield. Can inadvertedly clear ARBL flag.

2) example 7 at page 8

IIC0_IBSR_IBIF=1; // clear the interrupt event flag;

^^ write accessing a bitfield. Can clear IBAL flag

1) and 2) are used below many times in many other AN3291 examples.

Message Edited by kef on 2008-01-14 01:31 PM

[bigmac]

Hello,

We have an anomaly where C aims to be one step removed from the "hardware", yet here we are dealing with the peculiarities of the hardware registers that requires an appreciation of the type of assembly code generated by the C code.

The existing device header files, in addition to defining the location of each hardware register, also provide macros so that the programmer does not require to directly deal with the bit fields for these registers.  It would seem appropriate to me that each header file should, where possible, also contain additional macros specifically for the "safe" clearing of the various flags.

Just a thought ...

Regards,

Mac

[colinh]

Code:

   61:      FPROT = 0xFF;          /* Disable all protection (only in special modes)*/  002a c6ff         LDAB  #255  002c 7b0000       STAB  _FPROT   62:         63:      FSTAT |= (FSTAT_PVIOL|FSTAT_ACCERR);   /* Clear any errors  */  002f 1e00002002   BRSET _FSTAT,#32,*+7 ;abs = 0036  0034 87           CLRA    0035 8f           SKIP2  0036 8601         LDAA  #1  0038 1e00001002   BRSET _FSTAT,#16,*+7 ;abs = 003f  003d c7           CLRB    003e 21           SKIP1  003f 50           NEGB    0040 37           PSHB    0041 aab4         ORAA  5,SP+  0043 ba0000       ORAA  _FSTAT  0046 7a0000       STAA  _FSTAT   64:  }

Just in case anyone had any doubts about how the

FSTAT |= (FSTAT_PVIOL|FSTAT_ACCERR)

code works out I have attached the Codewarrior listing. 
 You will see that you end up evaluating the current status of the individual bits into ACCA (result = 0 or 1) and then ORing this into FSTAT. 
 Now FSTAT bit 0 is not writable so all that effort is wasted.  The code still works due to the read modify write sequence on the register. 
 You may as well just go FSTAT = FSTAT if thats want you really wanted (assuming the optimiser didn't get too smart)
The side effect of this implementation is that the CBEIF flag also gets cleared but this is not immediately obvious looking at the code.
Better still - just code it as FSTAT = (FSTAT_ACCERR_MASK | FSTAT_PVIOL_MASK);:

Cheers
Colin

[bigmac]

Hello Colin,

I agree with you that the published code does appear somewhat sloppy, if not buggy.  The assembly examples I have seen of similar code, for both 16-bit and 8-bit MCUs, appear to use the following -

   ldaa  #FSTAT_ACCERR_MASK|FSTAT_PVIOL_MASK

   staa  FSTAT

I would think that your modified C code should generate something similar.

Regards,

Mac

[colinh]

Mac - Thanks for the comments.

Not sure of the protocols for uploading new cuts of the code - I'd be happy to do so for peer review . In the ideal world we'd end up with a Codewarrior stationery compatible version of the code available directly off the Freescale web-site.

Cheers
Colin

[kef]

Colin,

there are more AN's with coding slips. I agree. Oring FSTAT (or any other register with more than one flag, clearable by writing one to it) with anything is a bug. Oring FCLKDIV with anything is a bad coding.

Using write accesses to bitfields representing flags, which are clearable by writing ones to them, is a bug. Of course this doesn't apply to flags registers with single flag bit, but S12 has almost no such registers, almost every flags registers have more than one flag. S08 is different, ?all? S08flags registers (except FSTAT) are bitfileds safe and oring safe.

I wan't to note that read modify write instructions and read modify write C sequences aren't that evil that makes our life difficult. *We* should be aware of what we are doing accessing all those registers and bits. It is bad that often we see just a bit and don't wan't to see the bigger picture (flags register)... Example of perfectly valid flags clearing read modify write sequences:

FSTAT &= (FSTAT_ACCERR_MASK | FSTAT_PVIOL_MASK); 
FSTAT &= CBEIF_MASK;

Please note no ~ character before mask as would be expected for regular RAM variables ( v &= ~bitmask).

[colinh]

kef,

I wasn't meaning to imply read/modify/write operations are evil   - they  have a very useful role and I use them quite a lot - just with care around the S12 (or HC11 for old buggers) !

With respect to your example - yes it is perfectly valid - it performs a write 1 if it's already 1 function. In the context of the FSTAT operations performed in the AN2720 though, it would be inefficient to do it this way.  It also clouds the logic that is going on - keep it simple and the next guy has a better chance of working out what you were doing!

Anyway, thanks for the feedback - it gave me something to think about!

Cheers
Colin

[JimDon]

Well, he is correct that the FDIV register can just be set, and I don't see that you need to 'or' the bits into the FSTAT registers or test the bits to set them, but the code works fine.

Perhaps there is some odd errata that this accounts for, as it looks deliberate.

[colinh]

Hi Jim

Re FDIV initialisation:
Certainly not wanting to flame the guy who wrote the code (I've got nothing to gain from that) but there is a difference between good code and code that works.  If there is a trick that needed to be done then it should be commented - personally I think it was just a coding slip that propagated.  Then again I'd be happy to hear from a Freescaler who can shed some light on it.

Regards
Colin