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

[KubaFYI]

The UART driver for samd0 was is missing the .configure
functionality expected from api for serial driver. This commit
fixes this by providing basic configuration.

Note, using the function will briefly switch the peripheral off
taking the data line low.

Signed-off-by: Kuba Sanak [email protected]

[zephyrbot]

All checks are passing now.

checkpatch (informational only, not a failure)

-:40: WARNING:LINE_SPACING: Missing a blank line after declarations
#40: FILE: drivers/serial/uart_sam0.c:392:
+	struct uart_sam0_dev_data *const dev_data = DEV_DATA(dev);
+	SercomUsart * const usart = cfg->regs;

-:136: WARNING:LINE_SPACING: Missing a blank line after declarations
#136: FILE: drivers/serial/uart_sam0.c:488:
+	struct uart_sam0_dev_data *const dev_data = DEV_DATA(dev);
+	memcpy(out_cfg, &(dev_data->config_cache),

- total: 0 errors, 2 warnings, 168 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[stephanosio]

This PR adds the .configure interface implementation without .config_get. Both should be implemented to pass the UART driver test.

p.s. As far as I can see, all the other UART drivers that support .configure implement .config_get as well.

[nandojve]

We've been converting to new DT macros all Atmel SAM variations. The #24110 is about SAM Serial Drivers. I can rebase on top of it and add changes. I waiting some feedback on #24110 anyway but is important know if this can be merged soon.

[stephanosio]

The conventional way of implementing config_get would be to cache configurations in struct uart_config or data variables in the driver context and simply return those values.

The assumption is, and should be, that an application will not make arbitrary direct configuration changes to the UART hardware.

(e.g. see uart_ns16550.c, uart_nrfx_uart.c and others)

p.s. I am neither +1 nor -1 on the current implementation; the API specification does not state exactly how it should be implemented. With that being the case, the current implementation would cause a problem if an application relies on the behaviour in which the configured and retrieved baudrates exactly match, or the retrieved baudrates are expected to match one of the preset values.

EDIT:

the API specification does not state exactly how it should be implemented

though the test (tests/drivers/uart/uart_basic_api) does.

[stephanosio]

As far as the UART driver test (tests/drivers/uart/uart_basic_api) goes, the configured and retrieved data MUST EXACTLY MATCH:

zephyr/tests/drivers/uart/uart_basic_api/src/test_uart_config.c

Line 79 in 608dba4

if (memcmp(&uart_cfg, &uart_cfg_check, sizeof(uart_cfg)) != 0) {

So the current implementation will not pass the test.

[KubaFYI]

@stephanosio Ah, this branch was actually created before the test_uart_config.c file even existed so I didn't know the uart_configs are expected to match exactly. Will fix that now.

[stephanosio]

Looks ok to me.

[jenmwms]

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.

[KubaFYI]

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.

[jenmwms]

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

[stephanosio]

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.

[stephanosio]

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

[ceolin]

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.

[KubaFYI]

@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?

[stephanosio]

@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.