clk: bcm: rpi: Restore the requested rate when preparing a clock

[mairacanal]

Lowering the rate on unprepare writes to the firmware directly, so the rate the clock framework has cached keeps describing a state the hardware is no longer in.

Nothing puts that rate back. A consumer that requests the same rate it had requested before the clock was unprepared is short-circuited by clk_core_set_rate_nolock(), which skips clocks that are already at the requested rate, so .set_rate is never reached. The clock is re-enabled at the rate unprepare left behind, and neither the consumer nor the framework has any indication that the request had no effect.

Restore the rate the framework last resolved when preparing a clock, so that lowering the rate on unprepare stays invisible to consumers.

[popcornmix]

I do locally have an exactly equivalent change claude came up with.

But I wasn't able to provoke the failure mode reported (I believe that pixel clock being marked critical avoided that code path).

But I do agree this patch is wanted.

[mairacanal]

But I wasn't able to provoke the failure mode reported (I believe that pixel clock being marked critical avoided that code path).

I wasn't able to reproduce it as well, but from the user's report, I believe that this change makes sense.

I'm planning on testing if we can remove some of these CLK_IS_CRITICAL flags after this patch, but I'll need to test this possible change in all Pis with different displays to reduce the possibility of a regression.

[popcornmix]

You may hit some of the known firmware issues when removing CLK_IS_CRITICAL.
I do have a test firmware I'm running that I believe works with all CLK_IS_CRITICAL removed - let me know if you want to test it. I suspect there will be some regressions, especially with code that relies on its current incorrect behaviour, like using a block without requesting a clock, most likely with bare metal/u-boot/old kernels.

[nicoske]

Tested the patch on the board from the issue, on top of rpi-6.18.y at 59abfb5. It doesn't help: first modeset still comes up at the firmware minimum, and after a DPMS off/on cycle vcgencmd measure_clock pixel reads 86400000.

I think the problem is these clocks are registered with CLK_GET_RATE_NOCACHE: clk_hw_get_rate() in prepare does a live GET_CLOCK_RATE query instead of returning what the consumer asked for. On my firmware that query returns a bogus 2400000000 for pixel (checked with vcmailbox, it reports that whatever the real rate is), so prepare restores garbage. Even with a sane reading it would restore the minimum unprepare just wrote. Maybe the driver should remember the last rate requested through .set_rate and restore that in prepare?