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
Martinhoff-maker,
asmellby,
jerome-pouiller,
jhedberg 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
| power-domains = <&siwx91x_soc_pd>; | ||
| zephyr,pm-device-runtime-auto; |
There was a problem hiding this comment.
don't you think we can also apply it to ulpdma ? in ps4 sleep state, we will have have the same behavior than the HP uDMA (no register retention)
|
|
||
| /* Connect the DMA interrupt */ | ||
| cfg->irq_configure(); | ||
| cfg->irq_configure(); |
There was a problem hiding this comment.
we don't need to reconfigure the irq after each wake-up, are we ? this can be done only in the init function IMO
There was a problem hiding this comment.
Just to make sure that we got the same understanding (and it's between all the PM PR):
Since you got no runtime get and put inside of this uDMA driver, you consider that the device will always be suspended or turned off if the power domain is suspended ? Then since we are on the same power-domain than the peripheral that use the uDMA, when you will do a "get" inside of this peripheral, it will turn on the power domain and then put the uDMA from OFF to suspended (but that will be good since everything's is alive in our "suspended" mode) ?
6 participants
This commit enables the pm device driver support for the dma_silabs_siwx91x driver.