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

[Melissa Wen <mwen@igalia.com>]

On 26/09/2025 07:36, Stefan Wahren wrote:
> Hi Marek,
>
> Am 26.09.25 um 09:27 schrieb 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@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@samsung.com>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
> AFAIK the offending clock change isn't in the downstream kernel, so I 
> like to see the opinion of María and the Raspberry Pi people first.

Hi,

I see in the downstream kernel the CLOCK_V3D was removed in favor of 
firmware clock:
https://github.com/raspberrypi/linux/blob/rpi-6.12.y/drivers/clk/bcm/clk-bcm2835.c#L2076

Also, v3d in RPi4 is set to use the firmware clock:
https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi#L97

I think v3d clock is missed on boot, but I also think the issue should 
be solved by setting the v3d firmware clock for Pi3.
WDYT? Can you check it on your side? Something like:

diff --git a/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi 
b/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
index 8b3c21d9f333..3289cb5dfa8e 100644
--- a/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
@@ -14,6 +14,7 @@ &hdmi {
  };

  &v3d {
+       clocks = <&firmware_clocks 5>;
         power-domains = <&power RPI_POWER_DOMAIN_V3D>;
  };

Best regards,

Melissa

>
> Currently I know that in the error case the following clocks are 
> disabled during boot of Raspberry Pi 3B+:
> fw-clk-vec
> fw-clk-isp
> fw-clk-v3d
>
> So it's very likely that the vc4 drivers tries to access the register 
> after the these clocks has been disabled and then the system freeze. 
> The workaround above was just a wild guess, so currently I don't know 
> why this change avoid the freeze.
>
> Best regards