Hi Quentin,
On 10/28/2026 7:23 PM, Quentin Schulz wrote:
> Hi Jonas,
>
> On 10/28/26 5:57 PM, Jonas Karlman wrote:
>> Hi Quentin,
>>
>> On 10/28/2026 1:41 PM, Quentin Schulz wrote:
>>> Hi Jonas,
>>>
>>> On 10/16/26 12:42 AM, Jonas Karlman wrote:
>>>> The CLK_PCIE30PHY_REF_M and CLK_PCIE30PHY_REF_N clocks are by default
>>>> gated coming out from system reset. The probe() ops of the PMUCRU driver
>>>
>>> Are they? I don't have an RK3568 board so cannot test this, but the TRM
>>> says the bitfields are 0 at reset with "When high, disable clock", so I
>>> am not sure what exactly this is for (except after a reboot where the
>>> Linux kernel has disabled the PCIe clocks maybe?).
>>
>> I guess the TRM is incorrect, because as I mention in my commit message
>> these clocks comes gated out from system reset, or I would never have
>> phrased the commit message as such.
>>
>
> Well, it could also be inferred from the fact Rockchip enabled it in the
> CRU driver unconditionally. So I wanted to make sure it was indeed
> checked rather than simply inferred. Thank you for the confirmation.
I would most likely have phrased the message very differently if the out
of system reset state was not tested, or at least mention that it would
have been a refactor based on current code if that was the case.
My expectation (and practice) are that when something is definitively
worded in commit message it has been verified to be true, else there is
a note and/or the wording is much more relaxed.
>
>> See below runtime log on a Radxa ROCK 3B (from power off state):
>>
>> DDR 5c7dbc4d49 kwin.chen 26/05/21-10:58:25,fwver: v1.26
>> [...]
>> out
>> PRE: pmu_clkgate_con[2]=6000
>> POST: pmu_clkgate_con[2]=6000
>>
>> U-Boot SPL 2027.01-rc1-00097-gec7e6bacaadc-dirty (Oct 28 2026 - 16:35:51
>> +0000)
>> Trying to boot from RAM
>> [...]
>> U-Boot 2027.01-rc1-00097-gec7e6bacaadc-dirty (Oct 28 2026 - 16:35:51
>> +0000)
>>
>> Model: Radxa ROCK 3B
>> SoC: RK3568
>> DRAM: 8 GiB (total 7.7 GiB)
>> PRE: pmu_clkgate_con[2]=6000
>> POST: pmu_clkgate_con[2]=0
>>
>> Using following code in probe() ops:
>>
>> printf("PRE: pmu_clkgate_con[2]=%x\n",
>> readl(&priv->pmucru->pmu_clkgate_con[2]));
>>
>> /* Ungate PCIe30phy refclk_m and refclk_n */
>> if (CONFIG_IS_ENABLED(PCI))
>> rk_clrreg(&priv->pmucru->pmu_clkgate_con[2], 0x3 << 13);
>>
>> printf("POST: pmu_clkgate_con[2]=%x\n",
>> readl(&priv->pmucru->pmu_clkgate_con[2]));
>>
>> I guess theoretically it could be the DDR init blob that gates the
>> refclks, however running bare minimal TPL on same board show same:
>>
>> U-Boot TPL 2026.10-01297-g83bfcb8d6e91-dirty (Oct 28 2026 - 16:45:25)
>> x0: 00000000 fdcc0098
>> x1: 00000000 00000008
>> x2: 00000000 fdcc1000
>> x3: 00000000 00000006
>> x4: 00000000 00000050
>> x5: 00000000 00000000
>> x6: 00000000 00020002
>> sp: 00000000 fdcc0f30
>> BOOTSOURCE_ID fdcc0010: 0000000a
>> PMU_CRU fdd00000: 00004064
>> PMU_CRU fdd00004: 000014c1
>> PMU_CRU fdd0000c: 00000007
>> PMU_CRU fdd00010: 00007f00
>> PMU_CRU fdd00040: 00002063
>> PMU_CRU fdd00044: 00001481
>> PMU_CRU fdd0004c: 00000007
>> PMU_CRU fdd00050: 00007f00
>> PMU_CRU fdd00100: 00000080
>> PMU_CRU fdd00108: 00000001
>> PMU_CRU fdd0010c: 00000001
>> PMU_CRU fdd00110: 00000800
>> PMU_CRU fdd00118: 00000001
>> PMU_CRU fdd0011c: 00000009
>> PMU_CRU fdd00120: 0000050f
>> PMU_CRU fdd00124: 00008bbb
>> PMU_CRU fdd00188: 00006000 (pmu_clkgate_con[2])
>> PMU_CRU fdd00240: 00040020
>> DRAM init failed
>>
>> Above should hopefully make it clear that the reset value in TRM is
>> likely incorrect ;-)
>>
>
> Wouldn't be the first nor the last time this happens :)
>
>>>
>>>> unconditionally ungate these clocks to help ensure PCIe works in U-Boot
>>>> proper.
>>>>
>>>> Change to only ungate these clocks when the PCI Kconfig option is
>>>> enabled for current boot phase to delay the ungate as late as possible.
>>>>
>>>> Signed-off-by: Jonas Karlman <[email protected]>
>>>> ---
>>>> Changes in v3:
>>>> - New optional patch to help delay the ungate of PCIe ref clocks until
>>>> U-Boot proper
>>>>
>>>> This patch is strictly not needed, however it seems very unnecessary to
>>>> ungate PCIe ref clocks in SPL when PCIe typically only is used in proper.
>>>>
>>>
>>> Considering the kernel seems (I had a cursory look) to be able to
>>> control those clocks and have them handled via the pcie node, it seems
>>> fine to me to possibly not have the clock enabled by U-Boot (e.g. with
>>> U-Boot proper or xPL not enabling PCI).
>>>
>>> So, I'm fine with the content of the patch, not the commit log. Can you
>>> please confirm that they are indeed not at 0 when reset (and/or that we
>>> do this to be safe when doing reboots) and rework the commit log?
>>
>> As mentioned above, the commit message has been correct from start, so
>> please be explicit about what you think is unclear and needs to change.
>>
>
> You could have just said that it doesn't match the TRM, that would have
> told me that it was checked IRL.
I did not even notice that the reset state in TRM was different, all my
changes are typically confirmed using runtime testing ;-)
>
> I can reword the first sentence to
>
> The CLK_PCIE30PHY_REF_M and CLK_PCIE30PHY_REF_N clocks are by default
> gated coming out from system reset unlike stated in the TRM.
>
> when applying, does that work for you?
Sure, please feel free to make any change you see fit to any of my
patches :-)
Regards,
Jonas
>
> Reviewed-by: Quentin Schulz <[email protected]>
>
> Thanks!
> Quentin
