drivers: video: stm32u5x dcmi: enable dcmi support for stm32u5x family#94536
drivers: video: stm32u5x dcmi: enable dcmi support for stm32u5x family#94536cornway wants to merge 1 commit intozephyrproject-rtos:mainfrom
Conversation
|
Hello @cornway, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
requested review from
FRASTM,
GeorgeCGV,
avolmat-st,
djiatsaf-st,
erwango,
etienne-lms,
gautierg-st,
josuah,
loicpoulain,
marwaiehm-st,
mathieuchopstm and
ngphibang
|
Hi @cornway and thank you for this patch. |
drivers/video/video_stm32_dcmi.c
Outdated
| err = HAL_DCMI_Start_DMA(&data->hdcmi, DCMI_MODE_CONTINUOUS, | ||
| uint32_t dcmi_mode = DCMI_MODE_CONTINUOUS; | ||
| #if defined(CONFIG_SOC_SERIES_STM32U5X) | ||
| dcmi_mode = DCMI_MODE_SNAPSHOT; |
There was a problem hiding this comment.
This is a video driver, why would you use Snapshot mode?
There was a problem hiding this comment.
To the best of My knowledge GPDMA needs LL structure to run in circular mode
So as a shortcut I did DCMI_MODE_SNAPSHOT, and then restart dma each time frame is completed
I need more time to add proper LL
Thank you for pointing that out
Please let me know if that can be done in a separate commit/PR or you'd like to have it in one place
There was a problem hiding this comment.
It should be in this PR in my opinion.
dts/arm/st/u5/stm32u5.dtsi
Outdated
| }; | ||
|
|
||
| #define GPDMA1_REQUEST_DCMI_PSSI 86U | ||
| #define DCMI_PSSI_IRQn 119 |
There was a problem hiding this comment.
we don't use #define in devicetree files.
There was a problem hiding this comment.
Will remove, thank you
I need more time to configure GPDMA to run in circular mode, otherwise shapshot is just a short cut |
Thanks for the reply. In that case, it is better to have it run in continuous mode now to avoid introducing such U5 specific behavior code in the driver. |
cornway
force-pushed
the
cornway/add-stm32u5x-dcmi-support
branch
2 times, most recently
from
264da15 to
8478660
Compare
I need more time to configure GPDMA to run in circular mode, otherwise shapshot is just a short cut
Updated PR with removed shapshot mode (Added gpdma LL for sicrcular mode) |
cornway
force-pushed
the
cornway/add-stm32u5x-dcmi-support
branch
from
8478660 to
2312148
Compare
Adding missing logic to handle stm32u5x dcmi interface Adding JPEG mode support: affects all soc's for which dcmi has already been implemented Signed-off-by: Roman Pustobaiev <romanpustobayev@gmail.com>
cornway
force-pushed
the
cornway/add-stm32u5x-dcmi-support
branch
from
2312148 to
18c0a8b
Compare
|
|
@JarmouniA @avolmat-st |
|
Several Zephyr developers were present at the OSS-EU conference or otherwise on a short leave. Hopefully more time for review from everyone coming through. |
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Thanks for adding support for more hardware! Some feedback to try to take this PR further up.
| ret |= HAL_DMAEx_List_BuildNode(&pNodeConfig, &Node); | ||
| ret |= HAL_DMAEx_List_InsertNode_Tail(&Queue, &Node); | ||
| ret |= HAL_DMAEx_List_SetCircularMode(&Queue); | ||
| ret |= HAL_DMAEx_List_LinkQ(hdma, &Queue); | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
using ret |= is very compact, but if there are different error messages coming, then return ret; would become inaccurate, and then code that does do { ret = try_again(); } while (ret == -EAGAIN); can behave differently.
There was a problem hiding this comment.
By the way, ret here is a HAL return code value, not an errno value.
Suggestion:
if (HAL_DMAEx_List_BuildNode(&pNodeConfig, &Node) != HAL_OK ||
HAL_DMAEx_List_InsertNode_Tail(&Queue, &Node) != HAL_OK ||
HAL_DMAEx_List_SetCircularMode(&Queue) != HAL_OK ||
HAL_DMAEx_List_LinkQ(hdma, &Queue) != HAL_OK) {
return -EIO;
}
return 0;| /* | ||
| * Stop DMA as in JPEG mode | ||
| * it may be still waiting for more data to come, so we can't restart it directly | ||
| */ |
There was a problem hiding this comment.
Is that really specific to STM32U5 or can it be used with other SoCs?
| HAL_StatusTypeDef ret = HAL_OK; | ||
| DMA_NodeConfTypeDef pNodeConfig; | ||
|
|
||
| /* Set node configuration ################################################*/ |
There was a problem hiding this comment.
Every vendor/author might have their own decoration style, how about a plain comment /* Set node configuration */?
| /* Size of a framebuffer is unknown at the moment, assign maximum */ | ||
| if (data->fmt.pixelformat == VIDEO_PIX_FMT_JPEG) { | ||
| buffer_size = vbuf->size; | ||
| } else if (buffer_size > vbuf->size) { |
There was a problem hiding this comment.
I would be tempted to split the JPEG support in a different commit.
- The commit short subject does not talk about JPEG so harder to look at when it was introduced.
- Sometimes commits are reverted upstream to fix CI or locally troubleshoot. Then either JPEG or STM32U5 would be reverted but not both.
| if (HAL_DMAEx_List_Init(&hdma) != HAL_OK) { | ||
| LOG_ERR("HAL_DMAEx_List_Init Failed"); | ||
| return -EINVAL; | ||
| } | ||
| if (stm32_dma_list_init(&hdma) != HAL_OK) { | ||
| LOG_ERR("stm32_dma_list_init Failed"); | ||
| return -EINVAL; | ||
| } | ||
| if (HAL_DMA_ConfigChannelAttributes(&hdma, DMA_CHANNEL_NPRIV) != HAL_OK) { | ||
| LOG_ERR("HAL_DMA_ConfigChannelAttributes Failed"); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Just noticing the different style through the source, in case you were interested in smoothing it up:
- above uses
err = HAL_...(); if (err != HAL_OK) {} - here it uses
if (HAL_...() != HAL_OK) {}
drivers/video/video_stm32_dcmi.c
Outdated
| #elif defined(CONFIG_SOC_SERIES_STM32L4X) | ||
| hdma.Instance = __LL_DMA_GET_CHANNEL_INSTANCE(config->dma.reg, config->dma.channel); | ||
| #elif defined(CONFIG_SOC_SERIES_STM32U5X) | ||
| hdma.Instance = LL_DMA_GET_CHANNEL_INSTANCE(config->dma.reg, config->dma.channel); |
There was a problem hiding this comment.
This shouldn't be required after rebase on top of #94869
| return -EIO; | ||
| } | ||
|
|
||
| #if defined(CONFIG_SOC_SERIES_STM32U5X) |
There was a problem hiding this comment.
I suggest checking if this DMAU5 compat shouldn't be check instead of U5 series.
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
| return; | ||
| } | ||
|
|
||
| HAL_DCMI_Suspend(hdcmi); |
There was a problem hiding this comment.
Should check the return value. If suspending fails, should return straight at least with an error level trace message?
| static DMA_QListTypeDef Queue; | ||
|
|
||
| HAL_StatusTypeDef ret = HAL_OK; | ||
| DMA_NodeConfTypeDef pNodeConfig; |
There was a problem hiding this comment.
Prefer have all fields at least with known values.
Could you rename the variable using snake_case format (ditto for Node and Queue above: node, queue)?
| DMA_NodeConfTypeDef pNodeConfig; | |
| DMA_NodeConfTypeDef node_config = {0}; | |
| ret |= HAL_DMAEx_List_BuildNode(&pNodeConfig, &Node); | ||
| ret |= HAL_DMAEx_List_InsertNode_Tail(&Queue, &Node); | ||
| ret |= HAL_DMAEx_List_SetCircularMode(&Queue); | ||
| ret |= HAL_DMAEx_List_LinkQ(hdma, &Queue); | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
By the way, ret here is a HAL return code value, not an errno value.
Suggestion:
if (HAL_DMAEx_List_BuildNode(&pNodeConfig, &Node) != HAL_OK ||
HAL_DMAEx_List_InsertNode_Tail(&Queue, &Node) != HAL_OK ||
HAL_DMAEx_List_SetCircularMode(&Queue) != HAL_OK ||
HAL_DMAEx_List_LinkQ(hdma, &Queue) != HAL_OK) {
return -EIO;
}
return 0;| int err = | ||
| HAL_DCMI_Start_DMA(&dev_data->hdcmi, DCMI_MODE_CONTINUOUS, | ||
| (uint32_t)dev_data->vbuf->buffer, dev_data->vbuf->bytesused / 4); | ||
| if (err != HAL_OK) { |
There was a problem hiding this comment.
ret should be of type HAL_StatusTypeDef.
| int err = | |
| HAL_DCMI_Start_DMA(&dev_data->hdcmi, DCMI_MODE_CONTINUOUS, | |
| (uint32_t)dev_data->vbuf->buffer, dev_data->vbuf->bytesused / 4); | |
| if (err != HAL_OK) { | |
| if (HAL_DCMI_Start_DMA(&dev_data->hdcmi, DCMI_MODE_CONTINUOUS, | |
| (uint32_t)dev_data->vbuf->buffer, | |
| dev_data->vbuf->bytesused / 4) != HAL_OK) { |
| reg = <0x4202C000 0x400>; | ||
| interrupts = <119 0>; | ||
| interrupt-names = "dcmi"; | ||
| clocks = <&rcc STM32_CLOCK(AHB2, 12U)>; |
There was a problem hiding this comment.
Nitpicking:
| clocks = <&rcc STM32_CLOCK(AHB2, 12U)>; | |
| clocks = <&rcc STM32_CLOCK(AHB2, 12)>; |
| }; | ||
| }; | ||
|
|
||
| dcmi: dcmi@4202C000 { |
There was a problem hiding this comment.
Use lower case on node name unit value:
| dcmi: dcmi@4202C000 { | |
| dcmi: dcmi@4202c000 { |
|
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
7 participants
Adding missing logic to handle stm32u5x dcmi interface