[En-Nut-Discussion] Do we need something like sbv/sbi to complement sbi/cbi?

[Ulrich Prinz]

Hi!

Am 01.01.2012 17:26, schrieb Uwe Bonnes:
> Hello,
> 
> a happy new year to all!
> 
>>>>>> "Ulrich" == Ulrich Prinz <ulrich.prinz at googlemail.com> writes:
>
> But I see one major obstacle: 
> The ST provided headers mostly only define bit values, not the bit position.
> 
> As I don't know of a macro to invert the BV() macro, we have to add bit
> position defines to the header files.
> Should these definitions go into stm32_<device>.h?
> 
It is the best Place to add them there.

> ...
>     Ulrich> Please look at the stm32_gpio.c and it's relevant .h file.  All
>     Ulrich> gpio functions are single cycle single access regardless of
>     Ulrich> their names.
> 
>     Ulrich> Using the bitband functions as a reference any Cortex Register
>     Ulrich> can be access atomic for switching single bits in it.
> 
> Isn't the STM GPIO a bad example. With the BSRR registers, atomic access is
> also possible without bit banding...
> So i think  e.g.
> #define GpioPinSetHigh(bank, bit) \
>     GPIOMEM(GPIOBITBAND((bank+GPIOBBREG_BSRR), (int)bit)) = 1

Wrong:
If you need to set a ping if a variable has a value that is non zero and
reset the pin as long as the variable is zero, you produce a lot of code
if you use BSRR / BRR as you have to do an if, free a register, write a
bitmask into it and write that bitmask into the gpio register.
if (myvar)
  BSRR = MY_BIT;
else
  BRR = MY_BIT;

If you use bitband any value written to the bits register will result in
a written 1 if it is not equal zero and a written 0 if it is.

bitband(MY_BIT)=myvar;

That's atomic :)

> could be happily replaced with
> #define GpioPinSetHigh(bank, bit) GPIOMEM(GPIOBBREG_BSRR, BV(bit))
> and
> #define GpioPinSetLow(bank, bit)  GPIOMEM(GPIOBBREG_BSRR, BV(bit+16))

Ups...
The second isn't atomic as it requires the shift left as long as the
value is not constant at first pass of the compiler.

Sou you should use GPIOMEM(.._BRR...
As the ARM the Cortex has the BitResetRegister too, it is only shadowed
to the upper 16 bits of the BSRR.

> 
> But for other devices the remark about bitbanding still holds.
> 
>     >> The s/cbi macro expands in devnut_m3n expands like #define cbi(_reg,
>     >> _bit) outr(_reg, inr(_reg) & ~_BV(_bit)) and was used like cbi(
>     >> &TIM->SR, TIM_SR_UIF ); and so things got wrong.
> 
>     Ulrich> You can check the bitband access from gpio and transfer it to
>     Ulrich> any register of the Cortex. It is importand that the bitband
>     Ulrich> macros expect the bit-number not the bit mask.
> 
> Again, the ST Headers mostly lake the Bit number and only define the
> bitvalue
> 
To be honest only a few bits need this special attraction.
For instance the most things could be done like I did in the PLL setup:

	myreg = PLL->CtrlReg;

	if () myreg |= STM_BITMASK;
	else myreg &= STM_OTHER_BITMASK

	...
	PLL->CtrlReg = myreg;

Only for special bit that need imediate special attention to speed up
things and to optionally guarantee atomic access, you could add the
bitband thingy.

In 99,99% of all usecases this is access to gpio for chip selects, ready
signals an such.

>     >>  The coding style rules say that direct register access like
>     TIM2-> SR |= TIM_SR_UIF
>     >> is deprecated. So either explicit writing outr(_reg, inr(_reg) &
>     >> _mask) is needed, or some macros to do so. In my tree I added
>     >> 
>     Ulrich> For Cortex it doesn't make any difference and it is the
>     Ulrich> preferred method to use register pointers as ARM architecture in
>     Ulrich> general does not need any inr or outr things.
> 
>     >> #define sbv(_reg, _mask) outr(_reg, inr(_reg) | _mask) #define
>     >> cbv(_reg, _mask) outr(_reg, inr(_reg) & ~_mask) #define
>     >> bv_is_set(_reg, _mask) ((inr(_reg) & _mask)) != 0)
>     >> 
>     Ulrich> For compatibility or coding style ( even I hate this typing
>     Ulrich> overhead making code unreadable) you can substitude inr( reg) by
>     Ulrich> (reg) and outr( reg, v) by (reg) = (v)
> 
> But http://www.ethernut.de/en/documents/programming-style-guide.html tells:
> 
> "Don't use the IOREG = value, value = IOREG notation
>  Use outb(), outw(), outr(), inb(), inw() or inr(). 
>  This allows trapping and emulation."

Yes and I am feeling guilty... Ahh, No I don't.

If you need to trap that register just set a watchpoint in GDB and it
will be trapped for you without any additional overhead in production of
code just to overcome a debugger problem that existed in old ARM
architectures.

In the examples and in general drivers that reside in /dev it is ok for
me, but in arch/dev it is a waste of time and typing.

If you do it like

outr( reg) (reg)
inr(reg) (reg)
you still can do outr( DEV->REG) = value

or

outr( reg, v) (reg) = (v)
outr( DEV->REG, value)

If one feels better then...