stm32f4xx_hal_spi.c, HAL_SPI_TransmitReceive - conversion of HW register pointer

[Marcin Wolcendorf]

Posted on June 04, 2018 at 16:25

Hi Everyone,

In HAL_SPI_TransmitReceive, around line 937, there is a gem like that:

 *((__IO uint8_t*)&hspi->Instance->DR) = (*pTxData);

Isn't it a bit dangerous to convert the HW register, which always have a specific size and  usually also access restrictions (in case of DR - reading it will delete the received data, for example), instead of converting the data read from the buffer? Wouldn't it be better to have something like that instead:

hspi->Instance->DR = (uint32_t)(*pTxData);

BR,

M.W.

#hw-register-access #spi #hal_spi_transmitreceive #data-integrity

[Tesla DeLorean]

Posted on June 04, 2018 at 17:21

You don't understand the purpose of the code.

The code defines how wide the write occurs at a bus level. You can do this in assembler more simply. It does not read the register.

The register behaves differently depending if you write 8, 16 or 32-bit wide values.

Not going to help writing 4 bytes when you only have 3

[Marcin Wolcendorf]

Posted on June 04, 2018 at 18:26

You don't understand the purpose of the code.

Apparently I do not. 

And I am quite certain I am not the only one. Yet - there is no comment in the code explaining the strange construct. 

The register behaves differently depending if you write 8, 16 or 32-bit wide values.

Really? Could you point me to the right passage in the documentation?

Is there a difference in the execution time between 8-bit, 16-bit and 32-bit data transmission to the APB bus? Considering the DR is 16-bit wide (and the structure for register mapping - SPI_TypeDef - makes it 32-bit),  the CPU is 32-bit, the busses are 32-bit, and these 2 passages from the RM0090 rev 16,

p. 923:

For an 8-bit data frame, the buffers are 8-bit and only the LSB of the register

(SPI_DR[7:0]) is used for transmission/reception. When in reception mode, the MSB ofthe register (SPI_DR[15:8]) is forced to 0.For a 16-bit data frame, the buffers are 16-bit and the entire register, SPI_DR[15:0] isused for transmission/reception.

(so - I can write 16 bits and only 8 will be used in 8-bit mode, when I read 16 bits in 8-bit mode, the upper 8 bits will be 0)

and p. 64:

Note:

When a 16- or an 8-bit access is performed on an APB register, the access is transformed

into a 32-bit access: the bridge duplicates the 16- or 8-bit data to feed the 32-bit vector.

(The SPI is on the APB bus, see the 

Table 1. STM32F4xx register boundary addresses

) - so all the accesses are converted to 32-bit anyway, with an added danger, that the values written are repeated.

Again - why would there be strange, unreadable, undocumented conversion, if any access to SPI is converted to 32-bit anyway, and there is no execution time difference?

[AvaTar]

Posted on June 05, 2018 at 07:51

Really? Could you point me to the right passage in the documentation?

The SPI peripheral section, and the DR register in particular.

The same applies to other peripherals, like the UART, I2C and ADC. Access as 8 bit and 16 bit is mentioned explicitly.

The ST coder could have added a comment, to explain what he wanted to achieve with this cumbersome-looking code.

It seems documentation doesn't pay for free software ...

[Marcin Wolcendorf]

Posted on June 05, 2018 at 09:25

Really? Could you point me to the right passage in the documentation?

The SPI peripheral section, and the DR register in particular.

The same applies to other peripherals, like the UART, I2C and ADC. Access as 8 bit and 16 bit is mentioned explicitly.

I've read that. I have even mentioned it in the post you're replying to. 

There is no difference in the register operation, regardless of the width of the access by CPU (and the WTF code in question does exactly that - changes the width of the data transfer to/from the CPU), as the bus bridge will make all the accesses to the APB peripherals 32-bit by duplicating the data (again - see my previous post). There is a difference in DR operation depending on the setup of the SPI with some consequence if (and only if) you put 1 byte to DR in 16-bit mode (in which case you will have it sent twice because of the APB bus bridge duplication). To avoid this it is enough to convert all the *input data* (not the register pointer!) to 16- or 32-bit. Which brings me back to the original question - why converting the HW register pointer and not the data put into the register? So far I cannot see any reason for register pointer conversion.

AvaTar wrote:

The ST coder could have added a comment, to explain what he wanted to achieve with this cumbersome-looking code.

I'd argue that *fixing* this code would be better. 

[T J]

Posted on June 05, 2018 at 14:12

All of you are way more advanced.

I am never sure which is little indian or the other one...

So of course we use:

(__IO uint8_t*)&hspi->Instance->DR // to force an 8 bit transaction.

it takes the same time

Turvey.Clive.002

‌ is suggesting that from ARM to ARM from Brand to Brand you can trust its going to work:

(__IO uint8_t*)&hspi->Instance->DR // it will always work on any ARM system.

and of course

it takes the same time

[AvaTar]

Posted on June 05, 2018 at 14:41

Turvey.Clive.002

is suggesting that from ARM to ARM from Brand to Brand you can trust its going to work:

(__IO uint8_t*)&hspi->Instance->DR // it will always work on any ARM system.

On the bus itself, yes.

What happens inside the peripheral (SPI) is totally up to the vendor - ST in this case.

Some other vendors considered it a good idea to transplant their 8-bit peripherals directly on Cortex M devices. Not even sure if all the registers (8-bit, of course) were 32-bit aligned. They are bought up in the meantime.

[Tesla DeLorean]

Posted on June 05, 2018 at 14:47

>>I'd argue that *fixing* this code would be better.

But the code to break the transaction to 8-bit wide is doing it's job, it looks incongruous because you haven't needed to use this form before, in assembler you would have used STRB, STRH or STR (like MOVE.B, MOVE.W or MOVE.L in other architectures) and think nothing of it. In C you could use a union, but the clean approach of using unnamed unions is not supported by all compilers in all modes.

You need to stop thinking of peripheral registers as 'memory' cells, they are logic, reads and writes to the same register location may have different effects, and act on different registers within the peripheral. The wiring of the bus presents a width to the peripheral, so it can act on the portion of the data on the bus that is active/valid. In architectures without such width data you might have multiple DR mapping to the same logic acting at different widths, ie DR8, DR16 and DR32.

The CRC->DR might be a better example as it explicitly recognizes 1, 2 or 4-byte transactions and advances the computation suitably.

These things are done to reduce the complexity of the logic on the silicon, protections and interlocks are eschewed and responsibility for doing things safely/correctly are pushed into the software domain.

SPI on different STM32 can have 8 or 16-bit widths, or have a FIFO/queuing method which decomposes the write at an APB Bus level to the width configure on the SPI Bus level.

[Marcin Wolcendorf]

Posted on June 06, 2018 at 09:12

But the code to break the transaction to 8-bit wide is doing it's job, it looks incongruous because you haven't needed to use this form before, in assembler you would have used STRB, STRH or STR (like MOVE.B, MOVE.W or MOVE.L in other architectures) and think nothing of it. In C you could use a union, but the clean approach of using unnamed unions is not supported by all compilers in all modes.

Nothing I read here so far has convinced me you need it at all. We are not talking about *some* peripheral in *some* device, no. We are talking about SPI in stm32f4. A peripheral on the APB (which makes all the register accesses 32-bit).

But since you mentioned it - CRC engine is on AHB, that does not have a duplicating bridge.

The CRC->DR might be a better example as it explicitly recognizes 1, 2 or 4-byte transactions and advances the computation suitably.

This is strange. Where did you get this information?

The 

RM0090 Rev 16

 on page 114 says:

4.4 CRC registers

The CRC calculation unit contains two data registers and a control register.

The peripheral

The CRC registers have to be accessed by words (32 bits).

It explicitly says the 8-bit or 16-bit accesses to the CRC engine registers (including DR) are not allowed, only the 32-bit are. 

And since I looked at CRC engine, I also rechecked the SPI register description in RM0090. I must have missed it before, but his is what it says on page 919:

28.5 SPI and I

2

S registers

The peripheral registers have to be accessed by half-words (16 bits) or words (32 bits).

So - not only it is convoluted, but by ST own documentation this code is also plainly wrong! There should never be an 8-bit access to the SPI->DR. It works by pure accident - that the APB bridge is duplicating the data.

OK, I got my answer - it is a bug, not a feature after all. It is convoluted, unexplained, unnecessary and, above all, wrong. 

ST (if there is anyone from you here) - could you fix it? Please?

[Marcin Wolcendorf]

Posted on June 06, 2018 at 09:35

As I replied to @Clive One - this code is violating what is written in the documentation (only 16- or 32-bit accesses to SPI registers), so it is on all accounts wrong.