On 01/11/2023 16:30, Laurent Pinchart wrote:
Hi Tomi,
Thank you for the patch.
On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
The probe function calls dispc_softreset() before runtime PM is enabled
and without enabling any of the DSS clocks. This happens to work by
luck, and we need to make sure the DSS HW is active and the fclk is
enabled.
To fix the above, add a new function, dispc_init_hw(), which does:
- pm_runtime_set_active()
- clk_prepare_enable(fclk)
- dispc_softreset().
This ensures that the reset can be successfully accomplished.
Note that we use pm_runtime_set_active(), not the normal
pm_runtime_get(). The reason for this is that at this point we haven't
enabled the runtime PM yet and also we don't want the normal resume
callback to be called: the dispc resume callback does some initial HW
setup, and it expects that the HW was off (no video ports are
streaming). If the bootloader has enabled the DSS and has set up a
boot time splash-screen, the DSS would be enabled and streaming which
might lead to issues with the normal resume callback.
I think the right way to do this would be, in probe(), to
- power on the device
- enable runtime PM, masking the device as active
- at end of probe, calling pm_runtime_put_autosuspend()
Can you explain what that would accomplish, or why the code in this
patch is wrong?
If I understand it right, you're suggesting a more "normal" power up at
the probe time, and then leaving the DSS enabled, but with autosuspend.
That would require powering up, doing a reset, and calling
dispc_runtime_resume. Which can be done, but I'm not sure why it's
better, as we're not interested in "normal" power up at probe time.
But I can see that my approach looks perhaps a bit odd just by looking
at these patches. This work was related to keeping the bootloader's
splash screen on the screen for a longer time, i.e. delaying reset.
For that, I wanted an early function (dispc_init_hw) which would,
instead of always resetting the DSS as it does in this version, peek at
the DSS hardware, and see if the DSS is already streaming. If no, do a
reset and proceed normally. If yes, skip the reset, leave the clocks
enabled, and keep DSS PM active.
Later, when we'd be doing the first modeset, the driver would do the
initial reset.
So, that's why I wanted an independent function for the HW probing/init,
which is called before runtime PM is enabled, and I did not want normal
runtime resume to be called as dispc_runtime_resume() would break the
display.
I think a better solution would be to set up the fb of tidss's fbdev to
use the reserved memory, used for the boot splash screen. But I didn't
figure out a way to do this. But even there we'd like to delay the reset
until the first modeset (when the fbdev display is getting enabled).
Tomi
