Hi,
Il giorno mer 28 ott 2026 alle ore 23:53 Jonas Karlman ha scritto:
>
> 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.
In case, I would add TRM document version (1.3 ?) too.
Regards,
Massimo
> >
> > 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
>
