[PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Marek Szyprowski]

On 25.09.2025 18:48, Stefan Wahren wrote:
> Am 25.09.25 um 09:57 schrieb Marek Szyprowski:
>> On 31.07.2025 23:06, Maíra Canal wrote:
>>> Currently, when we prepare or unprepare RPi's clocks, we don't actually
>>> enable/disable the firmware clock. This means that
>>> `clk_disable_unprepare()` doesn't actually change the clock state at
>>> all, nor does it lowers the clock rate.
>>>
>>> >From the Mailbox Property Interface documentation [1], we can see that
>>> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
>>> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
>>> prepare and an unprepare hook for RPi's firmware clock.
>>>
>>> As now the clocks are actually turned off, some of them are now marked
>>> CLK_IS_CRITICAL, as those are required to be on during the whole system
>>> operation.
>>>
>>> Link:https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface 
>>> [1]
>>> Signed-off-by: Maíra Canal<mcanal at igalia.com>
>>>
>>> ---
>>>
>>> About the pixel clock: currently, if we actually disable the pixel
>>> clock during a hotplug, the system will crash. This happens in the
>>> RPi 4.
>>>
>>> The crash happens after we disabled the CRTC (thus, the pixel clock),
>>> but before the end of atomic commit tail. As vc4's pixel valve doesn't
>>> directly hold a reference to its clock – we use the HDMI encoder to
>>> manage the pixel clock – I believe we might be disabling the clock
>>> before we should.
>>>
>>> After this investigation, I decided to keep things as they current are:
>>> the pixel clock is never disabled, as fixing it would go out of
>>> the scope of this series.
>>> ---
>>>    drivers/clk/bcm/clk-raspberrypi.c | 56 
>>> ++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 55 insertions(+), 1 deletion(-)
>> This patch landed recently in linux-next as commit 919d6924ae9b ("clk:
>> bcm: rpi: Turn firmware clock on/off when preparing/unpreparing"). In my
>> tests I found that it breaks booting of RaspberryPi3B+ board in ARM
>> 32bit mode. Surprisingly the same board in ARM 64bit mode correctly
>> boots a kernel compiled from the same source. The RPi3B+ board freezes
>> after loading the DRM modules (kernel compiled from 
>> arm/multi_v7_defconfig):
> thanks for spotting and bisecting this. Sorry, I only reviewed the 
> changes and didn't had the time to test any affected board.
>
> I was able to reproduce this issue and the following workaround avoid 
> the hang in my case:
>
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c 
> b/drivers/clk/bcm/clk-raspberrypi.c
> index 1a9162f0ae31..94fd4f6e2837 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -137,6 +137,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>         [RPI_FIRMWARE_V3D_CLK_ID] = {
>                 .export = true,
>                 .maximize = true,
> +               .flags = CLK_IS_CRITICAL,
>         },
>         [RPI_FIRMWARE_PIXEL_CLK_ID] = {
>                 .export = true,
>
Right, this fixes (frankly speaking 'hides') the issue. Feel free to add:

Reported-by: Marek Szyprowski <m.szyprowski at samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski at samsung.com>


> The proper fix should be in the clock consumer drivers. I found that 
> vc4_v3d doesn't ensure that the clock is enabled before accessing the 
> registers. Unfortunately the following change doesn't fix the issue 
> for me :-(
>
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c 
> b/drivers/gpu/drm/vc4/vc4_v3d.c
> index bb09df5000bd..5e43523732b4 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -441,7 +441,7 @@ static int vc4_v3d_bind(struct device *dev, struct 
> device *master, void *data)
>         vc4->v3d = v3d;
>         v3d->vc4 = vc4;
>
> -       v3d->clk = devm_clk_get_optional(dev, NULL);
> +       v3d->clk = devm_clk_get_optional_enabled(dev, NULL);
>         if (IS_ERR(v3d->clk))
>                 return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed 
> to get V3D clock\n");

Well, this can be sorted out in the drivers as a next step.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland