Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uart_stm32: Fix conflit between poll_out and irq API #40173

Merged
merged 1 commit into from
Nov 23, 2021
+17 −4
Merged

uart_stm32: Fix conflit between poll_out and irq API #40173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions drivers/serial/uart_stm32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,13 +473,19 @@ static void uart_stm32_poll_out(const struct device *dev,
unsigned char c)
{
USART_TypeDef *UartInstance = UART_STRUCT(dev);
struct uart_stm32_data *data = DEV_DATA(dev);

/* Wait for TXE flag to be raised */
while (!LL_USART_IsActiveFlag_TXE(UartInstance)) {
while (1) {
if (atomic_cas(&data->tx_lock, 0, 1)) {
if (LL_USART_IsActiveFlag_TXE(UartInstance)) {
break;
}
atomic_set(&data->tx_lock, 0);
}
}

#ifdef CONFIG_PM
struct uart_stm32_data *data = DEV_DATA(dev);

if (!data->tx_poll_stream_on) {
data->tx_poll_stream_on = true;
Expand All @@ -497,6 +503,7 @@ static void uart_stm32_poll_out(const struct device *dev,
#endif /* CONFIG_PM */

LL_USART_TransmitData8(UartInstance, (uint8_t)c);
atomic_set(&data->tx_lock, 0);
}

static int uart_stm32_err_check(const struct device *dev)
Expand Down Expand Up @@ -593,10 +600,12 @@ static int uart_stm32_fifo_read(const struct device *dev, uint8_t *rx_data,
static void uart_stm32_irq_tx_enable(const struct device *dev)
{
USART_TypeDef *UartInstance = UART_STRUCT(dev);

#ifdef CONFIG_PM
struct uart_stm32_data *data = DEV_DATA(dev);

while (atomic_cas(&data->tx_lock, 0, 1) == false) {
}

#ifdef CONFIG_PM
data->tx_poll_stream_on = false;
uart_stm32_pm_constraint_set(dev);
#endif
Expand All @@ -606,9 +615,12 @@ static void uart_stm32_irq_tx_enable(const struct device *dev)
static void uart_stm32_irq_tx_disable(const struct device *dev)
{
USART_TypeDef *UartInstance = UART_STRUCT(dev);
struct uart_stm32_data *data = DEV_DATA(dev);

LL_USART_DisableIT_TC(UartInstance);

atomic_set(&data->tx_lock, 0);

#ifdef CONFIG_PM
uart_stm32_pm_constraint_release(dev);
#endif
Expand Down
1 change: 1 addition & 0 deletions drivers/serial/uart_stm32.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ struct uart_stm32_data {
uint32_t baud_rate;
/* clock device */
const struct device *clock;
atomic_t tx_lock;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use Zephyr spinlock instead of re-implementing it in this driver?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is spinlock available before kernel init ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to lock interrupt so spinlock isn't adapted. I used a simple lock because poll_out could be call in interrupt function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is spinlock available before kernel init ?

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to lock interrupt so spinlock isn't adapted. I used a simple lock because poll_out could be call in interrupt function.

Well, you are checking the nucleo_l432kc which is based on STM32L432xxxx chip which is based on ARM Cortex-M4 which doesn't have atomic instructions (please correct me if I wrong). So, in this case we end up with atomics-in-C implementation which simply uses spinlock internally. And for single core Zephyr spinlock is just interrupt lock :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to lock interrupt so spinlock isn't adapted. I used a simple lock because poll_out could be call in interrupt function.

Well, you are checking the nucleo_l432kc which is based on STM32L432xxxx chip which is based on ARM Cortex-M4 which doesn't have atomic instructions (please correct me if I wrong). So, in this case we end up with atomics-in-C implementation which simply uses spinlock internally. And for single core Zephyr spinlock is just interrupt lock :)

Yes, you're right but I don't understand what you want to do when you say "use Zephyr spinlock instead of re-implementing it in this drive". The function z_impl_atomic_cas protect correctly my lock variable against the uart interrupt thanks spinlock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evgeniy-paltsev Do you have more comments ?

#ifdef CONFIG_UART_INTERRUPT_DRIVEN
uart_irq_callback_user_data_t user_cb;
void *user_data;
Expand Down