FreeRTOS on Cortex M4 of VF61 GPIO library bug

There is a severe bug in the FreeRTOS GPIO library for the VF61. I was following the “FreeRTOS on the Cortex-M4 of a Colibri VF61” guide, and was trying out some of the demo apps for the VF61. I decided to create my own test app to exorcise some GPIO pins. I based my app on the gpio_sample demo app, but added code to explicitly set a pin high or low using the GPIO_WritePinOutput() function. I found that the GPIO_WritePinOutput() function would not properly control the output state of a pin! I dove into the code, and could not believe what I found.

First of all, each GPIO port has 4 registers for controlling the output pins. There is the port data output register (PDOR), the port set output register (PSOR), the port clear output register (PCOR) and the port toggle output register (PTOR). The reason for the set, clear and toggle registers is to make it possible to set, clear or toggle output pins in a thread-and-interrupt safe manner by making the operations atomic. Writing a “1” bit (or bits) to the PSOR, PCOR or PTOR will set, clear or toggle the corresponding bit(s) in the PDOR. This avoids having to do a read-modify-write operation on the PDOR, which is not thread or interrupt safe.

I found that the WritePinOutput() function was doing a read-modify-write operation (caution- not thread safe), but was doing it on the PSOR register instead of the PDOR register. That is so wrong. First, it is not thread safe. Second, it can set a pin high but can never set a pin low.

The incorrect code was as follows:

void GPIO_WritePinOutput(GPIO_Type* base, uint32_t pin, gpio_pin_action_t pinVal)
{
	if (pinVal == gpioPinSet)
		GPIO_PSOR_REG(base) |= GPIO_OFFSET(pin);  /* Set pin output to high level.*/
	else
		GPIO_PSOR_REG(base) &= ~(GPIO_OFFSET(pin));  /* Set pin output to low level.*/
}

The correct code should be as follows:

void GPIO_WritePinOutput(GPIO_Type* base, uint32_t pin, gpio_pin_action_t pinVal)
{
	if (pinVal == gpioPinSet)
		GPIO_PSOR_REG(base) = GPIO_OFFSET(pin);  /* Set pin output to high level.*/
	else
		GPIO_PCOR_REG(base) = GPIO_OFFSET(pin);  /* Set pin output to low level.*/
}

So then I looked at the code for the GPIO_TogglePinOutput() function. I found that even though it seemed to work, if was also implemented wrong. It had the following line of code:

GPIO_PTOR_REG(base) |= GPIO_OFFSET(pin);

This is wrong. There is no need to do a read-modify-write operation on the PTOR (toggle) register. The correct code should be as follows:

GPIO_PTOR_REG(base) = GPIO_OFFSET(pin);

With these changes everything is thread-and-interrupt safe, and more important than that, it actually works.

I think that these changes need to be incorporated into the FreeRTOS source code on your GIT server.

hi @irbsd

Thanks for explanation of your issue.

First of all, each GPIO port has 4 registers for controlling the output pins. There is the port data output register (PDOR), the port set output register (PSOR), the port clear output register (PCOR) and the port toggle output register (PTOR). The reason for the set, clear and toggle registers is to make it possible to set, clear or toggle output pins in a thread-and-interrupt safe manner by making the operations atomic. Writing a “1” bit (or bits) to the PSOR, PCOR or PTOR will set, clear or toggle the corresponding bit(s) in the PDOR. This avoids having to do a read-modify-write operation on the PDOR, which is not thread or interrupt safe.

Well, the type of registers depends on the architecture of the SOC. M4 on VF61 has 4 types of registers for modifying the output of Pin, but usually one register should work too. Regarding thread safe, the user must take care that he is using the library function correctly to be thread safe. As you wrote, ** for VF61 luckily this is done in the Hardware**. Actually this GPIO driver was imported from M4 for iMX7 where there are no PCOR, PDOR and PTOR registers, which also explains the incorrect implementation.

I found that the WritePinOutput() function was doing a read-modify-write operation (caution- not thread safe), but was doing it on the PSOR register instead of the PDOR register. That is so wrong. First, it is not thread safe. Second, it can set a pin high but can never set a pin low.

This is true, the PDOR register should be used. Your solution with PSOR and PCOR is much better.

So then I looked at the code for the GPIO_TogglePinOutput() function. I found that even though it seemed to work, if was also implemented wrong. It had the following line of code

This is also true. We will change it on our Server.

With these changes everything is thread-and-interrupt safe, and more important than that, it actually works.
I think that these changes need to be incorporated into the FreeRTOS source code on your GIT server.

You are right. We will update the sources on our GitServer. Thanks a lot for the findings.

Best regards, Jaski