drivers: dma: cc23x0: Add power management

[jpanisbl]

This patch adds PM support to DMA driver for the TI cc23x0 SoC.

Datasheet: https://www.ti.com/lit/ds/symlink/cc2340r5.pdf

Another PR, introducing system PM for this SoC, must be merged before: #91401

Other PRs are submitted to add PM support in DMA client drivers (serial, crypto, ADC, SPI):

• drivers: serial: cc23x0: Add power management #92220
• drivers: crypto: cc23x0: Add power management #92221
• drivers: adc: cc23x0: Add power management #92222
• drivers: spi: cc23x0: Add power management #92223

[jpanisbl]

Hi @kartben There's only 1 reviewer for this PR. Would it be possible to add at least one please ?

[teburd]

This is quite confusing to look at, its unclear why on configure we do a pm get, and then never do a pm put.

Either power management is kept outside of the driver, or its kept inside the driver. Half in/half out is going to lead to confusion, particularly if all our DMA drivers don't work the same way.

I know @bjarki-andreasen and @JordanYates have been looking more closely at PM related stuff and how it should work but this seems a bit awkward to me to have a pm get in configure.

[bjarki-andreasen]

The additions in this PR are not aligned with the way device PM is used in tree and soon to be documented (See PR #90275) It will be fastest if you read that PR instead of me repeating everything written in there :)

Sorry about the confusion, documentation on the area is lacking (for now)

[teburd]

Marked DNM until the PM discussion is sorted out and we actually know how PM should work across zephyr device drivers, otherwise we're going to be left with large inconsistencies.

[jpanisbl]

This is quite confusing to look at, its unclear why on configure we do a pm get, and then never do a pm put.

Either power management is kept outside of the driver, or its kept inside the driver. Half in/half out is going to lead to confusion, particularly if all our DMA drivers don't work the same way.

I know @bjarki-andreasen and @JordanYates have been looking more closely at PM related stuff and how it should work but this seems a bit awkward to me to have a pm get in configure.

Hi @teburd I think there's a confusion here. The function used is pm_device_state_get() which is just used to obtain the PM state of the device (i.e. to get an information).
So, the function used is not pm_policy_state_lock_get() which indeed should be followed somewhere by pm_policy_state_lock_put()...if it was used, which is not the case in that particular driver.

The comment in dma_cc23x0_pm_action() explains why DMA clients are responsible for taking care of pm_policy_state_lock_get/put(). The way DMA interrupts are handled on this hardware is unusual, as explained in the comment I'm referring to.

[jpanisbl]

The additions in this PR are not aligned with the way device PM is used in tree and soon to be documented (See PR #90275) It will be fastest if you read that PR instead of me repeating everything written in there :)

Sorry about the confusion, documentation on the area is lacking (for now)

Thank you for the link @bjarki-andreasen . I'll read it carefully and will take it into account.

[jpanisbl]

Hi @bjarki-andreasen

In the link you shared, not sure I understand exactly what may or may not apply to this PR. A clarification would be welcome. What's missing ? Is it just pm_device_driver_init(), and eventually pm_device_driver_deinit() ? What should not be there ?

Meanwhile, I prepared a new version which is more aligned with another PR you previously approved, even if there's none pm_device_driver_init() in this one (#92222). Hopefully it will be aligned with your expectations. In this new version, PM lock/unlock is handled in the driver for MEMORY_TO_MEMORY transfers (it is not handled for PERIPHERAL_TO_MEMORY nor MEMORY_TO_PERIPHERAL transfers for reasons that are explained in the code).

[bjarki-andreasen]

ok, looking at the code and the dma.h header, the intended behavior of device PM is as follows:

        const struct device *dma_dev = DEVICE_DT_GET(MY_DMA_NODE);

        /* Before setting up and starting any transfers, we need to PM resume the DMA device itself */
        pm_device_runtime_get(dma_dev);

        /* The first call to pm_device_runtime (its reference counted) will result in the DMA device being resumed.
         * This means your driver will get a call to dma_cc23x0_pm_action with the action TURN_ON followed by
         * RESUME. Your driver here can ignore the TURN_ON call for simplicity. Once you your driver gets
         * the RESUME action, that is when your driver locks the sleep states with dma_cc23x0_pm_policy_state_lock_get()
         * Your driver does not need to read its using pm_device_state_get(), and does not need to count anything internally,
         * only one call to RESUME will occur, and will be followed by one call to SUSPEND later, at which point you should call
         * dma_cc23x0_pm_policy_state_lock_put()
         */

         /* now that the DMA is powered, the DMA driver does not need to track anything related to power, just handle the dma
          * API requests.
          */
        dma_config(dma_dev, ...)
        dma_start(dma_dev, ...)

        ...

        dma_stop(dma_dev, ...)

        /* once the user/driver is done with the DMA, it will be stopped, no reason to save any contexts at this point
         * (though you can if when SUSPEND is called if you really want to). User now releases the device, and the 
         * driver gets that one call to SUSPEND
         */
        pm_device_runtime_put(dma_dev);

The driver really just needs to respond to the RESUME and SUSPEND actions power PM, reference counting and managing of devices is handled outside the driver :)

[jpanisbl]

ok, looking at the code and the dma.h header, the intended behavior of device PM is as follows:

        const struct device *dma_dev = DEVICE_DT_GET(MY_DMA_NODE);

        pm_device_runtime_get(dma_dev);

        [...]

        pm_device_runtime_put(dma_dev);

The driver really just needs to respond to the RESUME and SUSPEND actions power PM, reference counting and managing of devices is handled outside the driver :)

Thank you for this explanation @bjarki-andreasen. IIUC the logic, you assume here that "Device Runtime PM" (i.e CONFIG_PM_DEVICE_RUNTIME) is necessarily used by the user app for doing a MEM_TO_MEM transfer, for instance. That's OK for me. :)

With your implementation, do you confirm that it's still possible to use "System Managed Device PM" (i.e. CONFIG_PM_DEVICE_SYSTEM_MANAGED) in DMA client device drivers as long as PM lock/unlock is handled properly in these drivers ? (I guess it's still possible, but I just want to ensure that my understanding is good).

I'm referring to this documentation: https://docs.zephyrproject.org/latest/services/pm/device.html

[bjarki-andreasen]

ok, looking at the code and the dma.h header, the intended behavior of device PM is as follows:

        const struct device *dma_dev = DEVICE_DT_GET(MY_DMA_NODE);

        pm_device_runtime_get(dma_dev);

        [...]

        pm_device_runtime_put(dma_dev);

The driver really just needs to respond to the RESUME and SUSPEND actions power PM, reference counting and managing of devices is handled outside the driver :)

Thank you for this explanation @bjarki-andreasen. IIUC the logic, you assume here that "Device Runtime PM" (i.e CONFIG_PM_DEVICE_RUNTIME) is necessarily used by the user app for doing a MEM_TO_MEM transfer, for instance. That's OK for me. :)

With your implementation, do you confirm that it's still possible to use "System Managed Device PM" (i.e. CONFIG_PM_DEVICE_SYSTEM_MANAGED) in DMA client device drivers as long as PM lock/unlock is handled properly in these drivers ? (I guess it's still possible, but I just want to ensure that my understanding is good).

I'm referring to this documentation: https://docs.zephyrproject.org/latest/services/pm/device.html

Possibly, as even mentioned in the docs, system managed is quite unstable, you will need to manually, from the application, force the system to not enter one of the states which would suspend the device, state locks from devices only works with PM_DEVICE_RUNTIME :)

With PM_DEVICE_RUNTIME, user resumes device, and device locks system state. With SYSTEM_MANAGED, system state resumes device, so its completely backwards comparatively, actually, in that case, your device would prevent the system from ever sleeping again. Try instead using zephyr,pm-disabling-states; to move this logic out of the driver :)

[jpanisbl]

Hello @bjarki-andreasen

I thought again about your pm_device_runtime_get/put(dma_dev) suggestion, and I have several comments. Do not hesitate to tell me if I misunderstand something.

[1] Not sure your implementation is less confusing than the current one. I mean, with your suggestion, pm_device_runtime_get/put(dma_dev) must be added somehow in all the DMA client drivers. It's not that obvious, especially for someone that will develop a user app, that runtime "PM get/put" must be explicitly called. A proof that it's not obvious: you approved this PR adding PM support to ADC driver, but this PR will no longer be good if runtime PM is introduced for DMA driver. :)

[2] With your implementation, what happens if several DMA clients are running simultaneously ? Let's have a look at the following scenario with crypto and ADC from 2 different threads, for instance:

• crypto calls "PM get",
• ADC calls "PM get",
• crypto calls "PM put",
• Here ADC (which did not call "PM put" yet since it still needs DMA) will get into trouble, since DMA is now suspended.

Maybe worth having other point of views. I do not want to bother the whole world, but maybe @fabiobaltieri (who is also reviewer for #92222) and @ceolin (who is also reviewer for #92221) might have interesting suggestions.

Just a reminder/summary for them, in case they would jump into the discussion. DMA related interrupts are a bit unusual on this SoC, as explained in the code:

 * For MEMORY_TO_MEMORY transfers, the transfer completion is signaled on
 * the DMA interrupt i.e. dma_cc23x0_isr(). So, this driver can take care
 * of PM lock/unlock for SW channels.
 *
 * For MEMORY_TO_PERIPHERAL and PERIPHERAL_TO_MEMORY transfers, we assume
 * that DMA clients (peripheral drivers) should take care of PM lock/unlock.
 * This assumption is made for that SoC because if a peripheral channel
 * is used, then the transfer completion is signaled on the peripheral's
 * interrupt, which is handled in the DMA client driver. This operating
 * mode is specific to this SoC.

That's why, I guess, @teburd spoke about a confusing "half in / half out" implementation. And that's what we are discussing now, we are looking for the best implementation.

[JordanYates]

[2] With your implementation, what happens if several DMA clients are running simultaneously ? Let's have a look at the following scenario with crypto and ADC from 2 different threads, for instance:

The point of device runtime PM is precisely to handle these sorts of use-cases. It performs reference counting automatically, such that the DMA will be left enabled as long as a single user has not yet called pm_device_runtime_put.

[jpanisbl]

[2] With your implementation, what happens if several DMA clients are running simultaneously ? Let's have a look at the following scenario with crypto and ADC from 2 different threads, for instance:

The point of device runtime PM is precisely to handle these sorts of use-cases. It performs reference counting automatically, such that the DMA will be left enabled as long as a single user has not yet called pm_device_runtime_put.

Oh, interesting. That's good then. :) Thank you for the explanation @JordanYates .

Do you confirm that my comment [1] remains true, though ? i.e. If runtime PM is implemented in this DMA driver, my PRs adding PM to the DMA client drivers (serial, crypto, ADC, SPI) will need to be modified so pm_device_runtime_get/put(dma_dev) is called somehow in each of them...is it correct ?

[bjarki-andreasen]

Hello @bjarki-andreasen

I thought again about your pm_device_runtime_get/put(dma_dev) suggestion, and I have several comments. Do not hesitate to tell me if I misunderstand something.

[1] Not sure your implementation is less confusing than the current one. I mean, with your suggestion, pm_device_runtime_get/put(dma_dev) must be added somehow in all the DMA client drivers. It's not that obvious, especially for someone that will develop a user app, that runtime "PM get/put" must be explicitly called. A proof that it's not obvious: you approved this PR adding PM support to ADC driver, but this PR will no longer be good if runtime PM is introduced for DMA driver. :)

Yep, any user of some hardware must request that hardware before using it, and all hardware must implement power management to work together for power management. I find this entirely logical :)

[2] With your implementation, what happens if several DMA clients are running simultaneously ? Let's have a look at the following scenario with crypto and ADC from 2 different threads, for instance:

• crypto calls "PM get",
• ADC calls "PM get",
• crypto calls "PM put",
• Here ADC (which did not call "PM put" yet since it still needs DMA) will get into trouble, since DMA is now suspended.

pm_device_runtime_get() has get in it because it gets a reference to the device (usage count ++), so:

• crypto calls get (usage count goes from 0 to 1, RESUME action called on DMA),
• ADC calls get (usage count goes from 1 to 2, DMA already resumed so no action)
• crypto calls put (usage count goes from 2 to 1, DMA still needed by something, so no action)
• ADC is totally groovy

Maybe worth having other point of views. I do not want to bother the whole world, but maybe @fabiobaltieri (who is also reviewer for #92222) and @ceolin (who is also reviewer for #92221) might have interesting suggestions.

Just a reminder/summary for them, in case they would jump into the discussion. DMA related interrupts are a bit unusual on this SoC, as explained in the code:

 * For MEMORY_TO_MEMORY transfers, the transfer completion is signaled on
 * the DMA interrupt i.e. dma_cc23x0_isr(). So, this driver can take care
 * of PM lock/unlock for SW channels.
 *
 * For MEMORY_TO_PERIPHERAL and PERIPHERAL_TO_MEMORY transfers, we assume
 * that DMA clients (peripheral drivers) should take care of PM lock/unlock.
 * This assumption is made for that SoC because if a peripheral channel
 * is used, then the transfer completion is signaled on the peripheral's
 * interrupt, which is handled in the DMA client driver. This operating
 * mode is specific to this SoC.

That's why, I guess, @teburd spoke about a confusing "half in / half out" implementation. And that's what we are discussing now, we are looking for the best implementation.

[jpanisbl]

OK, great. So let's go for it, I'll update this PR and the related DMA client drivers. Thank you all for your time.

[jpanisbl]

OK, great. So let's go for it, I'll update this PR and the related DMA client drivers. Thank you all for your time.

Done for this PR. I re-requested a review from @teburd but that would be great if somebody else could review the PR too, which is now XS size following @bjarki-andreasen's suggestion. :)

[ceolin]

Shouldn't you be handling TURN_ON and OFF in case this device is part of a power domain ?

Also, I am not sure if those early return in the switch statement aren't violating our code guidelines. I rather have one variable holding the return value and one return statement in the end of this function :p

[jpanisbl]

Thank you for the review @ceolin. Your feedbacks above are now implemented.

[ceolin]

Sorry for jump late into the discussion. This final version is looking good to me

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jpanisbl]

Marked DNM until the PM discussion is sorted out and we actually know how PM should work across zephyr device drivers, otherwise we're going to be left with large inconsistencies.

Hi @teburd I think the PM discussion is sorted out now :)

[bjarki-andreasen]

Looks good! Nice work :)