Device runtime support uart ns16550

[smalae]

Device runtime support for uart_ns16550 driver by enabling CONFIG_UART_NS16550_ENABLE_DEVICE_PM

This is a second attempt after #92507 got reverted in #93568

[github-actions]

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_silabs	zephyrproject-rtos/hal_silabs@13343bc (main)	zephyrproject-rtos/hal_silabs#130	zephyrproject-rtos/hal_silabs#130/files

⛔ DNM label due to: 1 project with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[fabiobaltieri]

The main issue with this is that if it used on a system with PM=y and PM_DEVICE_SYSTEM_MANAGED=y (which is the default) then this is going to call uart_ns16550_configure every time the cpu springs back to life, aka potentially multiple times per second, that is unless the device is either flagged as wakeup source or set up for pm device runtime. That said, there's nothing you can do about it, at least UART_NS16550_ENABLE_DEVICE_PM is disabled by default so one would have to explicitly turn it on to shot themselves in the foot.

[fabiobaltieri]

ok so what's the story exactly? suspend does nothing, resume just reconfigures? both platforms I tried using this driver did not seem to like this at all, is it a silabs specific thing? does this have a hope of being useful on other platforms using this driver?

[smalae]

Yes, this behavior is specific to devices where the peripheral is fully powered down during sleep states. To address this, I’ve updated the code to explicitly disable the peripheral clock and restore pin states when entering sleep mode

[fabiobaltieri]

Ok but then now we end up with a kconfig option that a user is expected to set and the only thing we are attaching to it is a very vague description:

This enables support device PM implementation for ns16550 driver.

that does not really help explaining, it sounds like "of course I want it wherever possible" but it's really "I need it for this class of devices"

How about having this behavior controlled by a devicetree property instead, with a proper description of what it does and when should it be enabled instead, and then you can have it set on the dts files for all silabs devices and the user would not have to worry about it.

[smalae]

The main issue with this is that if it used on a system with PM=y and PM_DEVICE_SYSTEM_MANAGED=y (which is the default) then this is going to call uart_ns16550_configure every time the cpu springs back to life, aka potentially multiple times per second, that is unless the device is either flagged as wakeup source or set up for pm device runtime. That said, there's nothing you can do about it, at least UART_NS16550_ENABLE_DEVICE_PM is disabled by default so one would have to explicitly turn it on to shot themselves in the foot.

I agree—calling uart_ns16550_configure repeatedly due to frequent system resume cycles could be undesirable in some scenarios. As you pointed out, the mitigation is that UART_NS16550_ENABLE_DEVICE_PM is disabled by default, so users need to explicitly opt in to this behavior. This approach helps ensure that only users who are aware of the power management implications will enable it.
Let me know if you have further suggestions or see other areas that could be improved!

[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

[fabiobaltieri]

As you pointed out, the mitigation is that UART_NS16550_ENABLE_DEVICE_PM is disabled by default, so users need to explicitly opt in to this behavior. This approach helps ensure that only users who are aware of the power management implications will enable it.
Let me know if you have further suggestions or see other areas that could be improved!

Yup seems like we are exposing the user to something that should be automatic, see #94375 (comment), I think this should be a property and set for the affected platforms.

[bjarki-andreasen]

Please check out https://docs.zephyrproject.org/latest/services/pm/device.html#device-model-with-device-power-management-support

The implementation in this PR is quite a bit away from the expected implementation.

[bjarki-andreasen]

not needed, the file can be safely included regardless

[bjarki-andreasen]

This should not be needed. The pm action needs to be directly referenced by pm_device_driver_init(), it will always be used.

Please check out the example code here https://docs.zephyrproject.org/latest/services/pm/device.html#device-model-with-device-power-management-support which describes how the driver should implement power management

[asmellby]

I believe the reasoning for making it conditional is to avoid something like #93568 happening again by making it opt-in, since the ns16550 driver is used by several slightly different HW IPs that have the same register interface, instantiated in several different SoCs with differing integration.

That said, I'm wondering if it would be better for this to be implemented as TURN_ON instead of RESUME, and to add a power domain driver for SiWx91x (or is there already a generic one for SoC PDs that does nothing but dispatch on/off events to its children when suspended/resumed? I don't recall). This would model what actually happens in hardware better (register unretention upon sleep), and would allow SoC-level opt-in to the PM behavior: SoCs that need register repainting on wakeup need to associate the UART to a power domain, while those that don't leave it out. The PM subsystem will call TURN_ON once at init if no power domain exists, and behavior will be identical to the status quo.

[bjarki-andreasen]

I mean, per device power management opt-in is definitely not the intended design, sure its possible to add a bunch of ifdefs, but even if you do so, the implementation when CONFIG_UART_NS16550_ENABLE_DEVICE_PM=y should still follow the documentation.