driver: dma: dma_silabs_siwx91x: Add pm device support for dma driver #94374
Conversation
fimohame
force-pushed
the
device_pm_silabs_siwx91x_dma
branch
5 times, most recently
from
c73b89c to
620f008
Compare
zephyrbot
requested review from
asmellby,
jerome-pouiller,
jhedberg,
Martinhoff-maker and
teburd
fimohame
force-pushed
the
device_pm_silabs_siwx91x_dma
branch
from
620f008 to
e603e6d
Compare
fimohame
force-pushed
the
device_pm_silabs_siwx91x_dma
branch
3 times, most recently
from
0a0e6e1 to
0ac614f
Compare
|
|
As discussed, since we agree to use CONFIG_PM_DEVICE=y and having in mind that going to sleep will completely turn off the device without any register retention on siwx917 board. |
drivers/dma/dma_silabs_siwx91x.c
Outdated
| return -EBUSY; | ||
| } | ||
| } else if (IS_ENABLED(CONFIG_PM_DEVICE) && (action == PM_DEVICE_ACTION_SUSPEND)) { | ||
| return 0; |
There was a problem hiding this comment.
As explained here: #94695 (comment)
You also need to turn off clocks as I understand for the future with pm device runtime mode. (Because you can suspend the device without sleeping)
drivers/dma/dma_silabs_siwx91x.c
Outdated
| } | ||
| if (action == PM_DEVICE_ACTION_RESUME) { | ||
| ret = clock_control_on(cfg->clock_dev, cfg->clock_subsys); | ||
| if (ret) { |
There was a problem hiding this comment.
clock_control_on() may return -EALREADY which is not really an error (I know... this API is annoying).
| return -EINVAL; | ||
| } | ||
| udma_handle = UDMAx_Initialize(&udma_resources, udma_resources.desc, NULL, | ||
| (uint32_t *)&data->udma_handle); |
There was a problem hiding this comment.
Do you think UDMAx_Initialize() should be called on PM_DEVICE_ACTION_RESUME or on PM_DEVICE_ACTION_TURN_ON?
I believe Device runtime PM will only play with PM_DEVICE_ACTION_SUSPEND. PM_DEVICE_ACTION_SUSPEND may stop the clocks and the regulator but I believe it will retains the registers.
I assume System Managed PM (or power domains) won't retains the registers but it will call PM_DEVICE_ACTION_TURN_OFF and PM_DEVICE_ACTION_TURN_ON. So, UDMAx_Initialize() should be only done in PM_DEVICE_ACTION_TURN_ON.
@Martinhoff-maker, @asmellby, can you confirm my understanding?
fimohame
force-pushed
the
device_pm_silabs_siwx91x_dma
branch
from
0ac614f to
cc83786
Compare
fimohame
force-pushed
the
device_pm_silabs_siwx91x_dma
branch
2 times, most recently
from
2701c32 to
569979a
Compare
This commit enables the pm device driver support for the dma_silabs_siwx91x driver. Signed-off-by: S Mohamed Fiaz <[email protected]>
fimohame
force-pushed
the
device_pm_silabs_siwx91x_dma
branch
from
569979a to
845009c
Compare
|
| case PM_DEVICE_ACTION_SUSPEND: | ||
| break; | ||
| case PM_DEVICE_ACTION_TURN_ON: | ||
| ret = clock_control_on(cfg->clock_dev, cfg->clock_subsys); |
There was a problem hiding this comment.
Do we need to turn this clock in PM_DEVICE_ACTION_TURN_ON, or this can be done later in PM_DEVICE_ACTION_RESUME?
There was a problem hiding this comment.
I think it was discussed to place it here
This commit enables the pm device driver support for the dma_silabs_siwx91x driver.