SPIv1 peripheral

[CezaryGapinski]

As you noticed I'm working on the STM32L0 line implementation. I've noticed that SPI implemented in the STM32L0 devices is pretty similar to the actual SPIv1 peripheral. There are two differences in registers: SPI_CR2 register: Bit 4 - reserved for STM32F1, FRF (Frame format 0 - Motorola, 1 TI mode) for STM32L0 SPI_SR register: Bit8 - reserved for STM32F1, FRE (Frame error) for STM32L0

But there is also problem with accessing bits in this registers. SPIv1 use bit-banding and if it is nice for STM32F1 MCUs is a problematic for STM32L0 without this feature. What I noticed SPIv2 is not using bit-banding even for devices with bit-banding functionality.

I would like to implement SPI feature in STM32L0 line thats why I've question if it is good option to just replace actual SPIv1 peripheral without bit-banding access or I should create next type of peripheral for example SPIv3.

If it is a good option to extend present driver it is also possible to add FRF and FRE bits with some sort of #define option configured in Kconfig-stm32ChipFamilyChoices file or it should be implemented with other approach?

[FreddieChopin]

Hi!

Sorry for the delay in replying - I've devoted most of my free time this week to implementing devicetree (*.dts) parser in Python (;

What I noticed SPIv2 is not using bit-banding even for devices with bit-banding functionality.

I assume you are talking about STM32F7. Then you'll probably be as surprised as I was to learn that ARM Cortex-M7 does NOT support bit-banding. It turns out that bit-banding is a feature of ONLY ARM-Cortex-M3 and -M4.

I would like to implement SPI feature in STM32L0 line thats why I've question if it is good option to just replace actual SPIv1 peripheral without bit-banding access or I should create next type of peripheral for example SPIv3.

We should stay with SPIv1 and just modify some parts to use bit opeations instead of bit-banding. I've noticed that you started that work already, but with a strange method (; I suggest to duplicate the test whether we are using core which supports bit-banding ( https://github.com/DISTORTEC/distortos/blob/master/source/architecture/ARM/ARMv6-M-ARMv7-M/include/distortos/architecture/ARMv7-M-bit-banding.h#L17 ) by turning it into a single macro. Then - depending on the value of this macro - some parts can use bit-banding or bit operations (+ optional locking when accessing shared stuff like RCC). There's absolutely no need to use templates for the Parameters as at any time there will be exactly one version used.

I'd also prefer to leave all the uses of the STM32_BITBAND_ADDRESS and similar macros, possibly just duplicating the constructors/initializers for the two variants.

If it is a good option to extend present driver it is also possible to add FRF and FRE bits with some sort of #define option configured in Kconfig-stm32ChipFamilyChoices file or it should be implemented with other approach?

Generally I would leave that for now, as these bit wouldn't be used anyway now. However for bits that would be used, you could follow the scheme I used in USARTs, for example here https://github.com/DISTORTEC/distortos/blob/master/source/chip/STM32/peripherals/USARTv1/Kconfig-peripheralsOptions#L82 Unless FRE can be set by hardware even when FRF is not, I would leave both of these bits for now - there's no need to complicate the code by something that has no current use.

[CezaryGapinski]

Hi!

Sorry for the delay in replying - I've devoted most of my free time this week to implementing devicetree (*.dts) parser in Python (;

Hi! No problem and nice to know about that. I guess that would be cutting-edge technique for small RTOSes :).

What I noticed SPIv2 is not using bit-banding even for devices with bit-banding functionality.

I assume you are talking about STM32F7. Then you'll probably be as surprised as I was to learn that ARM Cortex-M7 does NOT support bit-banding. It turns out that bit-banding is a feature of ONLY ARM-Cortex-M3 and -M4.

Yes, it is surprise, I wonder what was the reason of that.

I would like to implement SPI feature in STM32L0 line thats why I've question if it is good option to just replace actual SPIv1 peripheral without bit-banding access or I should create next type of peripheral for example SPIv3.

We should stay with SPIv1 and just modify some parts to use bit opeations instead of bit-banding. I've noticed that you started that work already, but with a strange method (; I suggest to duplicate the test whether we are using core which supports bit-banding ( https://github.com/DISTORTEC/distortos/blob/master/source/architecture/ARM/ARMv6-M-ARMv7-M/include/distortos/architecture/ARMv7-M-bit-banding.h#L17 ) by turning it into a single macro. Then - depending on the value of this macro - some parts can use bit-banding or bit operations (+ optional locking when accessing shared stuff like RCC). There's absolutely no need to use templates for the Parameters as at any time there will be exactly one version used.

Ok, as you could see that was just my idea to modify current functionality for non-banding operations. Perhaps I don't understand something, but my idea was result of some problems when I tried to surround the block of codes with macro define as you proposed. What to do with variables with addresses for the bit-banding area which are not used in non bit-banding operations:

/// address of bitband alias of SPE bit in SPI_CR1 register
uintptr_t speBbAddress_;

/// address of bitband alias of ERRIE bit in SPI_CR2 register
uintptr_t errieBbAddress_;

/// address of bitband alias of RXNEIE bit in SPI_CR2 register
uintptr_t rxneieBbAddress_;

/// address of bitband alias of TXEIE bit in SPI_CR2 register
uintptr_t txeieBbAddress_;

/// address of bitband alias of appropriate SPIxEN bit in RCC register
uintptr_t rccEnBbAddress_;

/// address of bitband alias of appropriate SPIxRST bit in RCC register
uintptr_t rccRstBbAddress_;

I'd also prefer to leave all the uses of the STM32_BITBAND_ADDRESS and similar macros, possibly just duplicating the constructors/initializers for the two variants.

Two different constructors are needed, but when I looked into SPIv2, it has other variables for RCC register operations:

/// bitmask of appropriate SPIxEN bit in RCC register at \a rccEnOffset_ offset
uint32_t rccEnBitmask_;

/// offset of RCC register with appropriate SPIxEN bit, bytes
size_t rccEnOffset_;

/// bitmask of appropriate SPIxRST bit in RCC register at \a rccRstOffset_ offset
uint32_t rccRstBitmask_;

/// offset of RCC register with appropriate SPIxRST bit, bytes
size_t rccRstOffset_;

which are not present already in SPIv1. These variables and constructors should be surrounded by macro (#if ... #else ... #endif) definitions? I just worried about a lot of preprocessor blocks within whole file.

[FreddieChopin]

Currently the code of the SPIv1 driver is more-or less something like this:

class Parameters
{
constructor
functions
variables
}

constructors of Parameters objects

uses of Parameters in functions

I would propose to do it like this:

#if BITBANDING

class Parameters
{
constructor // may use different arguments
functions // same API, different implementation
variables // may be different
}

constructors of Parameters objects // using bitbanding macros

#else

class Parameters
{
constructor // may use different arguments
functions // same API, different implementation
variables // may be different
}

constructors of Parameters objects // using registers, offsets, masks, ...

#endif

uses of Parameters in functions

Such organization will no doubt cause some duplication, but I think this can be kept to a minimum by mixing things up a little bit. For example instead of creating just two blocks we could divide that in a way to keep the doxygen comments common (and maybe some functions which would be identical anyway).

class Parameters
{
#if BITBANDING
// constructor doxygen comments
constructor
#else
// constructor doxygen comments
constructor
#endif

// function 1 doxygen comment

R function1(T) const {
#if BITBANDING
function 1 implementation for bitbanding
#else
function 1 implementation without bitbanding
#endif
}

...

#if BITBANDING
variables for bitbanding
#else
variables without bitbanding
#endif
}

#if BITBANDING
constructors of Parameters objects with bitbanding
#else
constructors of Parameters objects without bitbanding
#endif

uses of Parameters in functions

Something like this should generally work fine and reduce duplication to an acceptable level.

If there is some bitbanding used inside "regular" functions of the driver, just surround that with #if ... #else ... #endif or maybe move to Parameters class.

Does that sound OK?

I guess that would be cutting-edge technique for small RTOSes :).

I was under the impression that the whole idea of distortos is "cutting-edge", given the state of competitors <: devicetree will be a revolution in the project. But I really hope that it will be a positive one, bringing a lot of desired features (like ease of GPIO configuration, DMA assignments, simplified code generation, ...).

[CezaryGapinski]

Hi! Thanks for explanations. Second version of path is here Let me know if it is ok with #define BITBANDING in ARMv7-M-bit-banding.h file. Perhaps it should looks like here? #define BITBANDING (defined(CONFIG_ARCHITECTURE_ARM_CORTEX_M3) || defined(CONFIG_ARCHITECTURE_ARM_CORTEX_M4))

and next block of code in SPIv1 should be surrounded by #if BITBANDING == 1 ... #else ... #endif

Perhaps the name should be more specific to arm cortex, for example ARM_CORTEX_BITBANDING?

[FreddieChopin]

OK, great (; We're getting really close.

First of all, I think we should name the macro DISTORTOS_BITBANDING_SUPPORTED or maybe DISTORTOS_ARM_CORTEX_BITBANDING_SUPPORTED, but the later is pretty long.

Perhaps it should looks like here?

define BITBANDING (defined(CONFIG_ARCHITECTURE_ARM_CORTEX_M3) || defined(CONFIG_ARCHITECTURE_ARM_CORTEX_M4))

Actually code like this results in a warning from GCC 7, because - according to the standard - use of "defined()" operator inside another macro leads to undefined behaviour. I've fixed a couple of similar warnings quite recently. As these two configuration macros which we need here (CONFIG_ARCHITECTURE_ARM_CORTEX_M3 and CONFIG_ARCHITECTURE_ARM_CORTEX_M4) are either undefined or are defined without any value, there's nothing more we can do about that, and a construct similar to what you proposed is not possible in that case. Just leave that as in the current patch, but with more specific name.

Also note that you have spi1Parameters twice (;

As for the block with construction of objects, it would be probably less noisy if you would do that like this:

#if DISTORTOS_BITBANDING_SUPPORTED == 1

// spi1Parameters constructor
// spi2Parameters constructor
// spi3Parameters constructor
// spi4Parameters constructor
// ...

#else

// spi1Parameters constructor
// spi2Parameters constructor
// spi3Parameters constructor
// spi4Parameters constructor
// ...

#endif

In this option above it's easier to add more whitespace to increase the "distance" between preprocessor directives and real code. But I'm fine with both options, whichever you prefer.

[CezaryGapinski]

First of all, I think we should name the macro DISTORTOS_BITBANDING_SUPPORTED or maybe DISTORTOS_ARM_CORTEX_BITBANDING_SUPPORTED, but the later is pretty long.

As always you have right ;). Ok, I named the macro DISTORTOS_BITBANDING_SUPPORTED.

Also note that you have spi1Parameters twice (;

Thanks ;), now should be without typos.

As for the block with construction of objects, it would be probably less noisy if you would do that like this:

if DISTORTOS_BITBANDING_SUPPORTED == 1

// spi1Parameters constructor // spi2Parameters constructor // spi3Parameters constructor // spi4Parameters constructor // ...

else

// spi1Parameters constructor // spi2Parameters constructor // spi3Parameters constructor // spi4Parameters constructor // ...

endif

In this option above it's easier to add more whitespace to increase the "distance" between preprocessor directives and real code. But I'm fine with both options, whichever you prefer.

Yes, now it looks like much more nicer. BTW, I would like just to remind about the unification of #ifdef and #if defined(...) ;)

[FreddieChopin]

Current version of patch looks OK (;