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

drivers: uart: samd0: add missing .configure API functionality #23234

Merged
merged 1 commit into from
Apr 17, 2020
Merged

drivers: uart: samd0: add missing .configure API functionality #23234

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
122 changes: 122 additions & 0 deletions drivers/serial/uart_sam0.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <soc.h>
#include <drivers/uart.h>
#include <drivers/dma.h>
#include <string.h>

#ifndef SERCOM_USART_CTRLA_MODE_USART_INT_CLK
#define SERCOM_USART_CTRLA_MODE_USART_INT_CLK SERCOM_USART_CTRLA_MODE(0x1)
Expand Down Expand Up @@ -42,6 +43,7 @@ struct uart_sam0_dev_cfg {

/* Device run time data */
struct uart_sam0_dev_data {
struct uart_config config_cache;
#ifdef CONFIG_UART_INTERRUPT_DRIVEN
uart_irq_callback_user_data_t cb;
void *cb_data;
Expand Down Expand Up @@ -111,6 +113,7 @@ static int uart_sam0_set_baudrate(SercomUsart *const usart, u32_t baudrate,
return 0;
}


#if CONFIG_UART_ASYNC_API

static void uart_sam0_dma_tx_done(void *arg, u32_t id, int error_code)
Expand Down Expand Up @@ -377,10 +380,121 @@ static void uart_sam0_rx_timeout(struct k_work *work)

#endif

static int uart_sam0_configure(struct device *dev,
const struct uart_config *new_cfg)
{
int retval;

const struct uart_sam0_dev_cfg *const cfg = DEV_CFG(dev);
struct uart_sam0_dev_data *const dev_data = DEV_DATA(dev);
SercomUsart * const usart = cfg->regs;
stephanosio marked this conversation as resolved.
Show resolved Hide resolved

wait_synchronization(usart);

usart->CTRLA.bit.ENABLE = 0;
wait_synchronization(usart);

if (new_cfg->flow_ctrl != UART_CFG_FLOW_CTRL_NONE) {
/* Flow control not yet supported though in principle possible
* on this soc family.
*/
return -ENOTSUP;
}

dev_data->config_cache.flow_ctrl = new_cfg->flow_ctrl;

SERCOM_USART_CTRLA_Type CTRLA_temp = usart->CTRLA;
SERCOM_USART_CTRLB_Type CTRLB_temp = usart->CTRLB;

switch (new_cfg->parity) {
case UART_CFG_PARITY_NONE:
CTRLA_temp.bit.FORM = 0x0;
break;
case UART_CFG_PARITY_ODD:
CTRLA_temp.bit.FORM = 0x1;
CTRLB_temp.bit.PMODE = 1;
break;
case UART_CFG_PARITY_EVEN:
CTRLA_temp.bit.FORM = 0x1;
CTRLB_temp.bit.PMODE = 0;
break;
default:
return -ENOTSUP;
}

dev_data->config_cache.parity = new_cfg->parity;

switch (new_cfg->stop_bits) {
case UART_CFG_STOP_BITS_1:
CTRLB_temp.bit.SBMODE = 0;
break;
case UART_CFG_STOP_BITS_2:
CTRLB_temp.bit.SBMODE = 1;
break;
default:
return -ENOTSUP;
}

dev_data->config_cache.stop_bits = new_cfg->stop_bits;

switch (new_cfg->data_bits) {
case UART_CFG_DATA_BITS_5:
CTRLB_temp.bit.CHSIZE = 0x5;
break;
case UART_CFG_DATA_BITS_6:
CTRLB_temp.bit.CHSIZE = 0x6;
break;
case UART_CFG_DATA_BITS_7:
CTRLB_temp.bit.CHSIZE = 0x7;
break;
case UART_CFG_DATA_BITS_8:
CTRLB_temp.bit.CHSIZE = 0x0;
break;
case UART_CFG_DATA_BITS_9:
CTRLB_temp.bit.CHSIZE = 0x1;
break;
default:
return -ENOTSUP;
}

dev_data->config_cache.data_bits = new_cfg->data_bits;

usart->CTRLA = CTRLA_temp;
wait_synchronization(usart);

usart->CTRLB = CTRLB_temp;
wait_synchronization(usart);

retval = uart_sam0_set_baudrate(usart, new_cfg->baudrate,
SOC_ATMEL_SAM0_GCLK0_FREQ_HZ);
if (retval != 0) {
return retval;
}

dev_data->config_cache.baudrate = new_cfg->baudrate;

usart->CTRLA.bit.ENABLE = 1;
wait_synchronization(usart);

return 0;
}

static int uart_sam0_config_get(struct device *dev,
Copy link
Collaborator

Choose a reason for hiding this comment

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

jw - Is it worth considering a way to accomplish this task without using memcpy()? I ask because I am curious about it from a secure coding perspective atm. Can this use of memcpy introduce a vulnerability? Please see uart_nrfx_uart.c and uart_ns16550.c for example implementations.

Copy link
Contributor Author

@KubaFYI KubaFYI Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In choosing the memcpy() I was aiming for making absolutely sure that the implementation passes relevant tests which currently use memcmp() to establish whether the set and retrieved struct uart_config's match. Doing things the way uart_nrfx_uart.c or uart_ns16550.c do it can potentially introduce a corner case where the test won't pass because of potential content of padding in the struct which won't be carried across without memcpy().

I guess the proper thing to do would be to edit the tests to use comparison of individual struct member variables instead of memcmp(), but I wanted to avoid doing changes not related to the work I'm doing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah that makes sense, thanks for clarifying! I am learning more with your help.
I wonder if you might want to add the cast to void in anticipation of the MISRA-C coding guidelines currently in discussion, namely regarding memcpy --> (void) memcpy() (see #18344). Adding @ceolin in case he wants to share thoughts here

Copy link
Member

Choose a reason for hiding this comment

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

As long as the copy size is static (i.e. sizeof(dev_data->config_cache)), that is highly unlikely.

As for the out_cfg pointer checking, that is a whole another issue.

Copy link
Member

Choose a reason for hiding this comment

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

Enforcement of MISRA-C, especially arguably ridiculous ones like (void)func() is a no-go for now IIRC.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @jenmwms, regarding MISRA, it is not sepcific to memcpy. The rule says that we should check all return values from non-void functions. It happens, that memcpy() return is quite useless, so we just explicitly ignore it casting to void.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stephanosio

As for the out_cfg pointer checking, that is a whole another issue.

Do you mean the checking in function or in the test. In short, is there any other changes I should make to this PR?

Copy link
Member

Choose a reason for hiding this comment

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

@KubaFYI As far as my comments are concerned, no changes necessary in this PR.

I was simply alluding that we can go very far with advanced security checks and functional safety rules; but, given that the current Zephyr codebase is really not ready for compliance, I see no reason to enforce them in this particular PR.

struct uart_config *out_cfg)
{
struct uart_sam0_dev_data *const dev_data = DEV_DATA(dev);
memcpy(out_cfg, &(dev_data->config_cache),
sizeof(dev_data->config_cache));

return 0;
}

static int uart_sam0_init(struct device *dev)
{
int retval;
const struct uart_sam0_dev_cfg *const cfg = DEV_CFG(dev);
struct uart_sam0_dev_data *const dev_data = DEV_DATA(dev);

SercomUsart *const usart = cfg->regs;

#ifdef MCLK
Expand Down Expand Up @@ -416,6 +530,11 @@ static int uart_sam0_init(struct device *dev)
SERCOM_USART_CTRLA_CPOL | SERCOM_USART_CTRLA_DORD;
wait_synchronization(usart);

dev_data->config_cache.flow_ctrl = UART_CFG_FLOW_CTRL_NONE;
dev_data->config_cache.parity = UART_CFG_PARITY_NONE;
dev_data->config_cache.stop_bits = UART_CFG_STOP_BITS_1;
dev_data->config_cache.data_bits = UART_CFG_DATA_BITS_8;

/* Enable receiver and transmitter */
usart->CTRLB.reg = SERCOM_USART_CTRLB_CHSIZE(0) |
SERCOM_USART_CTRLB_RXEN | SERCOM_USART_CTRLB_TXEN;
Expand All @@ -426,6 +545,7 @@ static int uart_sam0_init(struct device *dev)
if (retval != 0) {
return retval;
}
dev_data->config_cache.data_bits = cfg->baudrate;

#if CONFIG_UART_INTERRUPT_DRIVEN || CONFIG_UART_ASYNC_API
cfg->irq_config_func(dev);
Expand Down Expand Up @@ -900,6 +1020,8 @@ static int uart_sam0_rx_disable(struct device *dev)
static const struct uart_driver_api uart_sam0_driver_api = {
.poll_in = uart_sam0_poll_in,
.poll_out = uart_sam0_poll_out,
.configure = uart_sam0_configure,
.config_get = uart_sam0_config_get,
#if CONFIG_UART_INTERRUPT_DRIVEN
.fifo_fill = uart_sam0_fifo_fill,
.fifo_read = uart_sam0_fifo_read,
Expand Down