drivers: usb: udc: Add SAM USBC driver#103703
drivers: usb: udc: Add SAM USBC driver#103703nashif merged 4 commits intozephyrproject-rtos:mainfrom
Conversation
2e0bdef to
e724fb0
Compare
e724fb0 to
1b9a117
Compare
josuah
left a comment
There was a problem hiding this comment.
Trying to move this driver review forward with some feedback.
f0129c2 to
fedfed1
Compare
| uint8_t setup[8] __aligned(4); | ||
| }; | ||
|
|
||
| BUILD_ASSERT(offsetof(struct udc_sam_usbc_data, setup) + | ||
| sizeof(((struct udc_sam_usbc_data *)0)->setup) == | ||
| sizeof(struct udc_sam_usbc_data), | ||
| "setup[] must be last field (DMA overrun protection)"); |
There was a problem hiding this comment.
Perhaps it would work without padding or __aligned. Anyway it looks to complicated. Even if it is the last field, it may "overrun" something else. Is it not better to define an aligned setup array, with appropriate size that DMA may not overrun? It does not have to be in the struct udc_sam_usbc_data, can be placed in UDC_SAM_USBC_DEVICE_DEFINE(n) macro, and have a pointer to the array in struct udc_sam_usbc_config.
There was a problem hiding this comment.
After doing research on this topic and analyzing the options my conclusion is that this is the recommended solution. There is no documentation about the internal USB-DMA overrun size so, it is not possible to know how big can the corruption. Any other attempt, even including padding after, may result in problems in the future when compiler change. Changing the place of the variable will not change the USB-DMA implementation which can create the same problem.
After moving any value before the setup makes the code work correctly and alignment is necessary to make sure that DMA work correctly.
What bothers me is seeing that some tests are failing precisely because of BUILD_ASSERT(). I think the correct question would be: How some build passes and how some build fails? I mean, how that check can "eventually" fail? Maybe we have a bigger problem then the DMA doing overrun.
INFO - 416/973 sam4l_ek/sam4lc4c sample.usbd.loopback NOT RUN (build <zephyr>)
INFO - 417/973 sam4l_ek/sam4lc4c sample.usbd.shell NOT RUN (build <zephyr>)
INFO - 418/973 sam4l_ek/sam4lc4c sample.usbd.console NOT RUN (build <zephyr>)
INFO - 419/973 sam4l_ek/sam4lc4c sample.usb.legacy.webusb NOT RUN (build <zephyr>)
INFO - 420/973 sam4l_ek/sam4lc4c sample.usbd.hid-keyboard.out-report NOT RUN (build <zephyr>)
INFO - 421/973 sam4l_ek/sam4lc4c sample.usb.legacy.console NOT RUN (build <zephyr>)
INFO - 422/973 sam4l_ek/sam4lc4c sample.usbd.hid-keyboard.large-report NOT RUN (build <zephyr>)
INFO - 423/973 sam4l_ek/sam4lc4c sample.usb.legacy.hid-mouse NOT RUN (build <zephyr>)
INFO - 424/973 sam4l_ek/sam4lc4c sample.usb_device_next.cdc-acm-workqueue NOT RUN (build <zephyr>)
INFO - 425/973 sam4l_ek/sam4lc4c sample.usbd.hid-keyboard.large-out-report NOT RUN (build <zephyr>)
INFO - 426/973 sam4l_ek/sam4lc4c sample.usb_device_next.midi NOT RUN (build <zephyr>)
INFO - 427/973 sam4l_ek/sam4lc4c sample.usbd.hid-keyboard NOT RUN (build <zephyr>)
INFO - 428/973 sam4l_ek/sam4lc4c sample.usb_device_next.cdc-acm NOT RUN (build <zephyr>)
INFO - 429/973 sam4l_ek/sam4lc4c sample.subsys.usb.uvc.encoder.jpeg FILTERED (runtime filter)
...
INFO - 464/973 sam4l_ek/sam4lc4c sample.tracing.transport.usb.ctf ERROR Build failure (build <zephyr>)
INFO - /__w/zephyr/zephyr/twister-out/sam4l_ek_sam4lc4c/zephyr/samples/subsys/tracing/sample.tracing.transport.usb.ctf/build.log
...
[109/172] Building C object zephyr/subsys/usb/device_next/CMakeFiles/subsys__usb__device_next.dir/usbd_msg.c.obj
[110/172] Linking C static library zephyr/subsys/usb/device_next/libsubsys__usb__device_next.a
[111/172] Building C object zephyr/drivers/usb/udc/CMakeFiles/drivers__usb__udc.dir/udc_common.c.obj
[112/172] Building C object zephyr/drivers/usb/udc/CMakeFiles/drivers__usb__udc.dir/udc_sam_usbc.c.obj
FAILED: zephyr/drivers/usb/udc/CMakeFiles/drivers__usb__udc.dir/udc_sam_usbc.c.obj
ccache /opt/toolchains/zephyr-sdk-0.17.4/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc -DKERNEL -DK_HEAP_MEM_POOL_SIZE=0 -DPICOLIBC_LONG_LONG_PRINTF_SCANF -D_POSIX_THREAD_SAFE_FUNCTIONS=200809L -D__LINUX_ERRNO_EXTENSIONS__ -D__PROGRAM_START -D__ZEPHYR_SUPERVISOR__ -D__ZEPHYR__=1 -I/__w/zephyr/zephyr/drivers/usb/common -I/__w/zephyr/zephyr/twister-out/sam4l_ek_sam4lc4c/zephyr/samples/subsys/tracing/sample.tracing.transport.usb.ctf/zephyr/include/generated/zephyr -I/__w/zephyr/zephyr/include -I/__w/zephyr/zephyr/twister-out/sam4l_ek_sam4lc4c/zephyr/samples/subsys/tracing/sample.tracing.transport.usb.ctf/zephyr/include/generated -I/__w/zephyr/zephyr/soc/atmel/sam -I/__w/zephyr/zephyr/lib/libc/picolibc/include -I/__w/zephyr/zephyr/lib/posix/c_lib_ext/getopt -I/__w/zephyr/zephyr/soc/atmel/sam/common/. -I/__w/zephyr/zephyr/soc/atmel/sam/sam4l/. -I/__w/zephyr/zephyr/kernel/include -I/__w/zephyr/zephyr/arch/arm/include -I/__w/zephyr/zephyr/subsys/tracing/include -I/__w/zephyr/zephyr/subsys/tracing/ctf/. -I/__w/zephyr/modules/hal/cmsis_6/CMSIS/Core/Include -I/__w/zephyr/zephyr/modules/cmsis_6/. -I/__w/zephyr/modules/hal/atmel/include -I/__w/zephyr/modules/hal/atmel/asf/sam/include/sam4l -isystem /__w/zephyr/zephyr/lib/libc/common/include -Wshadow -fno-strict-aliasing -Werror -Os -imacros /__w/zephyr/zephyr/twister-out/sam4l_ek_sam4lc4c/zephyr/samples/subsys/tracing/sample.tracing.transport.usb.ctf/zephyr/include/generated/zephyr/autoconf.h -fno-printf-return-value -fno-common -g -gdwarf-4 -fdiagnostics-color=always -mcpu=cortex-m4 -mthumb -mabi=aapcs -mfp16-format=ieee -mtp=soft --sysroot=/opt/toolchains/zephyr-sdk-0.17.4/arm-zephyr-eabi/arm-zephyr-eabi -imacros /__w/zephyr/zephyr/include/zephyr/toolchain/zephyr_stdint.h -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wdouble-promotion -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-pic -fno-pie -fno-asynchronous-unwind-tables -ftls-model=local-exec -fno-reorder-functions --param=min-pagesize=0 -fno-defer-pop -fmacro-prefix-map=/__w/zephyr/zephyr/samples/subsys/tracing=CMAKE_SOURCE_DIR -fmacro-prefix-map=/__w/zephyr/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/__w/zephyr=WEST_TOPDIR -ffunction-sections -fdata-sections -specs=picolibc.specs -std=c17 -MD -MT zephyr/drivers/usb/udc/CMakeFiles/drivers__usb__udc.dir/udc_sam_usbc.c.obj -MF zephyr/drivers/usb/udc/CMakeFiles/drivers__usb__udc.dir/udc_sam_usbc.c.obj.d -o zephyr/drivers/usb/udc/CMakeFiles/drivers__usb__udc.dir/udc_sam_usbc.c.obj -c /__w/zephyr/zephyr/drivers/usb/udc/udc_sam_usbc.c
In file included from /__w/zephyr/zephyr/include/zephyr/toolchain.h:52,
from /__w/zephyr/zephyr/include/zephyr/kernel_includes.h:23,
from /__w/zephyr/zephyr/include/zephyr/kernel.h:17,
from /__w/zephyr/zephyr/include/zephyr/drivers/usb/udc.h:15,
from /__w/zephyr/zephyr/drivers/usb/udc/udc_common.h:15,
from /__w/zephyr/zephyr/drivers/usb/udc/udc_sam_usbc.c:7:
/__w/zephyr/zephyr/include/zephyr/toolchain/gcc.h:87:36: error: static assertion failed: "setup[] must be last field (DMA overrun protection)"
87 | #define BUILD_ASSERT(EXPR, MSG...) _Static_assert((EXPR), "" MSG)
| ^~~~~~~~~~~~~~
/__w/zephyr/zephyr/drivers/usb/udc/udc_sam_usbc.c:100:1: note: in expansion of macro 'BUILD_ASSERT'
100 | BUILD_ASSERT(offsetof(struct udc_sam_usbc_data, setup) +
| ^~~~~~~~~~~~
ninja: build stopped: subcommand failed.
CC: @stephanosio ^
There was a problem hiding this comment.
What bothers me is seeing that some tests are failing precisely because of BUILD_ASSERT(). I think the correct question would be: How some build passes and how some build fails?
What I said.
Perhaps it would work without padding or __aligned.
Just look at struct k_thread or struct k_event, the size of these depends on build configuration. Event if they would be aligned, struct udc_sam_usbc_data has holes.
There was a problem hiding this comment.
align the struct and removed the BUILD_ASSERT
There was a problem hiding this comment.
Since BUILD_ASSERT is removed, I think you can keep the __aligned(4);.
Add Atmel SAM USBC driver for SAM4L family. The driver was tested using CDC-ACM and testusb samples. Fixes: zephyrproject-rtos#74665 Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Add usbd entry in the sam4l_ek.yaml file. Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Add usbd entry in the sam4l_wm400_cape.yaml file. Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Add sam4l_ek board in the integration_platform list to build the atmel,sam_udc_usbc compatible driver. Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
|



Summary
Add USB Device (USBC) driver for SAM4L enabling USB device_next stack support for these MCUs.
drivers/usb/udc/udc_sam_usbc.cTest Plan
Fixes: #74665
Depends on: #103769