clk: qcom: lemans: add QUP SE clock set-rate/DFS/enable support - #26
clk: qcom: lemans: add QUP SE clock set-rate/DFS/enable support#26nnunna94 wants to merge 4 commits into
Conversation
|
Please explain how this was tested and why these changes are needed. |
remove all comments except where you needed - or want to offer- the guidance (LLMs destroy the flow by flooding source code with documentation that is not required). Assume that the user will have access to the same LLM that you do. |
|
currently the validation for this PR APIs is done via unit tests which were excluded in this PR Jorge A. Ramirez-Ortiz (@ldts). End to End validation is planned with buses SPI driver. working with buses POC offline for the same. |
ah, so if nobody is consuming these clocks, can it wait for your next PR? where the clocks are being consumed and the code is used? you could just add the consumer PRs on top of this. I think it would make the patch series better. |
|
Note: |
| GCC_CLOCK_BRANCH_ENA_VOTE_1, 16), | ||
|
|
||
| /* | ||
| * QUPv3 wrapper 1: S0-S1 at 120 MHz, S2-S6 at 100 MHz. S0-S5 VOTE_1, |
There was a problem hiding this comment.
no runtime protection for the domain explicitly flagged as "must never be touched"
/*
* QUPv3 wrapper 1: S0-S1 at 120 MHz, S2-S6 at 100 MHz. S0-S5 VOTE_1,
* S6 VOTE_3. S3 is the console UART's QUP SE and must never be
* enabled/disabled/set-rate by this driver or its self-test.
*/
...
QUP_SE_DOMAIN("gcc_qupv3_wrap1_s3_clk", 0x144f0, qup_se_100mhz,
GCC_CLOCK_BRANCH_ENA_VOTE_1, 25),gcc_qupv3_wrap1_s3_clk is registered as an ordinary domain in the table
like every other SE, with no special-case handling anywhere in
clk-qcom.c's qcom_qup_clk_enable()/disable()/set_rate(),
qcom_clk_get_by_name(), or qcom_clk_enable_dfs(). The only thing
preventing a future consumer (the commit message names "a future TEE-side
SPI/I2C driver") from calling qcom_clk_get_by_name("gcc_qupv3_wrap1_s3_clk")
and then disabling/reconfiguring it — which per the comment would silently
kill the OP-TEE console — is the comment itself.
Suggest: either omit this domain from the table entirely (if nothing
should ever drive it through this API) or add an explicit guard in
qcom_clk_get_by_name()/the clk_ops callbacks that rejects operations on
it, so the "must never" constraint is enforced rather than only documented.
There was a problem hiding this comment.
I think this comment section popped up when i added a self unit test and it went ahead and modified the se rate... the client driver is aware of the se that is used for uart logging and is programmed already in xbl ideally. here in optee, bus driver just enables DFS for the same se, nothing more i believe as per buses driver requirement. i should remove those comments to avoid confusion. buses driver is the only consumer for all these domains across images and they are aware as to which domain scale in which mode Selvam Sathappan (@zelvam95).
There was a problem hiding this comment.
Yes, please clean up all comments that don't necessary add value/any qTEE references/etc.. and we can retain only comments that don't explain the obvious/adds value to upstream community/maintainers/etc.
| srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clock-qcom-pas.c | ||
| srcs-y += clk-qcom.c | ||
| srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clk-qcom-pas.c | ||
| srcs-$(CFG_QCOM_CLK_BSP) += platform/$(PLATFORM_FLAVOR)/clk-qcom-lemans.c |
There was a problem hiding this comment.
CFG_QCOM_CLK_BSP hardcodes clk-qcom-lemans.c instead of clk-qcom-$(PLATFORM_FLAVOR).c
srcs-y += clk-qcom.c
srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clk-qcom-pas.c
srcs-$(CFG_QCOM_CLK_BSP) += platform/$(PLATFORM_FLAVOR)/clk-qcom-lemans.cThe CFG_QCOM_PAS_PTA line correctly parameterizes on $(PLATFORM_FLAVOR)
for both the source path and the filename. The new CFG_QCOM_CLK_BSP line
one row below parameterizes the directory on $(PLATFORM_FLAVOR) but
hardcodes the filename to clk-qcom-lemans.c. Today this is harmless
because only lemans/target.mk sets CFG_QCOM_CLK_BSP ?= y (confirmed:
kodiak never sets it, and there's no global default anywhere in
mk/config.mk or core/drivers/clk/qcom/sub.mk that would flip it on for
another flavor) — but the moment a second platform enables
CFG_QCOM_CLK_BSP=y, the build will look for
platform/<that-flavor>/clk-qcom-lemans.c, which won't exist, and fail.
Suggest: rename clk-qcom-lemans.c to clk-qcom-$(PLATFORM_FLAVOR).c's
expansion for lemans (i.e. keep today's lemans filename as-is only if that
happens to already match the intended convention) or change the sub.mk
line to platform/$(PLATFORM_FLAVOR)/clk-qcom-$(PLATFORM_FLAVOR).c,
matching the line above it.
There was a problem hiding this comment.
There was a discussion on this file naming on last PR, we discussed on keeping it as clk_bsp.c and you suggest qup.c i beleive. since then i did not rename it... will rename it.
There was a problem hiding this comment.
Yes I remember that discussion. Feel free to rename that if that makes sense.
FYI -> The above comment is slightly different & is not particularly pointing at that. This is related to how we have added the below src include in sub.mk. I want to avoid the explict mention of lemans in the RHS for the file name since that would require you to modify the sub.mk for each platform/cause platform when you try to enable this for some platform other than lemans.
srcs-$(CFG_QCOM_CLK_BSP) += platform/$(PLATFORM_FLAVOR)/clk-qcom-**lemans**.c
For ex: If you enable this for lets say Kodiak, it will search for the file platform/kodiak/clk-qcom-lemans.c instead of clk-qcom-kodiak.c. Hope this particular comment is clear now.
|
Line length: a handful of lines in the frequency-plan tables exceed 80 columns Ensure there are no checkpatch issues in all the commits of this PR & that it can compile for all targets. |
There was a problem hiding this comment.
Pull request overview
This PR adds Qualcomm QUPv3 Serial Engine (QUP SE) clock support for the Lemans (Hoya) platform in OP-TEE, enabling TEE-side SPI/I2C drivers to request, set rates for, and enable DFS on SE clocks without a secure DT.
Changes:
- Introduces a per-target QUP SE clock “BSP” contract (domains + frequency plans + PLL source votes) and registers QUP SE clocks into the common clk framework.
- Adds CMD_DB support for reading auxiliary resource blobs (used to map CX/MX voltage corners to RPMh ordinals for safe set-rate voting).
- Refactors/renames the QCOM clock driver source layout (clock-qcom.c → clk-qcom.c; PAS files similarly) and wires in Lemans BSP build/config.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/include/drivers/qcom/cmd_db/cmd_db.h | Declares cmd_db_get_aux() for retrieving CMD_DB auxiliary blobs. |
| core/drivers/qcom/cmd_db/cmd_db.c | Implements cmd_db_get_aux() and fixes zero-length entry handling. |
| core/include/drivers/clk_qcom.h | Adds QUP SE clock provider APIs (get-by-name, enable DFS, get domain) under CFG_QCOM_CLK_BSP. |
| core/include/drivers/clk_qcom_bsp.h | New BSP data contract types for QUP SE domains, rate plans, and source voting. |
| core/drivers/clk/qcom/clk-qcom.c | New consolidated QCOM clock driver including QUP SE provider, DFS programming, and CX/MX RPMh voting. |
| core/drivers/clk/qcom/clock-qcom.c | Removed legacy QCOM clock driver file (superseded by clk-qcom.c). |
| core/drivers/clk/qcom/sub.mk | Updates build sources to new filenames and adds Lemans BSP compilation under CFG_QCOM_CLK_BSP. |
| core/drivers/clk/qcom/platform/lemans/clock_group_qcom.h | Adds common RCG/DFS register offsets and field masks needed by the walker. |
| core/drivers/clk/qcom/platform/lemans/clk-qcom-lemans.c | Provides Lemans-specific QUP SE domain tables, rate plans, and PLL source vote table. |
| core/drivers/clk/qcom/platform/lemans/clk-qcom-pas.c | Lemans PAS clock enable/reset support under renamed filename. |
| core/drivers/clk/qcom/platform/kodiak/clk-qcom-pas.c | Kodiak PAS support under renamed filename. |
| core/arch/arm/plat-qcom/hoya/lemans/target.mk | Enables CFG_QCOM_CLK_BSP by default and attempts to force CMD_DB/RPMh dependencies. |
Suppressed comments (1)
core/include/drivers/clk_qcom_bsp.h:61
- The qcom_clk_domain documentation describes
@configsas "freq_hz == 0 terminated" and@n_configsas excluding the terminator, but the current clk-qcom implementation uses@n_configsas the iteration bound and does not stop on freq_hz == 0. The doc should match the length-based contract used by the code.
* @configs Frequency-configuration array, freq_hz == 0 terminated.
* @n_configs Number of usable rows in @configs (excluding terminator).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # QUPv3 serial-engine (bus) clock set-rate/DFS walker, consumed on-demand by a | ||
| # future TEE-side SPI/I2C driver. Set-rate votes CX/MX via RPMh, so pull | ||
| # cmd_db/RPMh client in whenever the walker is built. | ||
| ifeq ($(CFG_QCOM_CLK_BSP),y) | ||
| $(call force,CFG_QCOM_CMD_DB,y) | ||
| $(call force,CFG_QCOM_RPMH_CLIENT,y) | ||
| endif | ||
| CFG_QCOM_CLK_BSP ?= y |
| QUP_SE_DOMAIN("gcc_qupv3_wrap1_s3_clk", 0x144f0, qup_se_100mhz, | ||
| GCC_CLOCK_BRANCH_ENA_VOTE_1, 25), |
| /* | ||
| * One frequency configuration row for an RCG. | ||
| * | ||
| * @freq_hz Output frequency in Hz. A terminating row has freq_hz == 0. | ||
| * @mux_sel RCG source-select index (CFG_RCGR SRC_SEL), pre-resolved. | ||
| * @div2x Twice the half-integer source divider; the register encodes | ||
| * SRC_DIV as (div2x - 1), 0 means no divide. |
| /* GCC-relative BSP offsets need a GCC mapping; register one if PAS PTA didn't. */ | ||
| #ifndef CFG_QCOM_PAS_PTA | ||
| register_phys_mem(MEM_AREA_IO_NSEC, GCC_BASE, GCC_SIZE); | ||
| #endif |
| srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clock-qcom-pas.c | ||
| srcs-y += clk-qcom.c | ||
| srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clk-qcom-pas.c | ||
| srcs-$(CFG_QCOM_CLK_BSP) += platform/$(PLATFORM_FLAVOR)/clk-qcom-lemans.c |
Align the Qualcomm clock driver filenames with the clk-<vendor> naming used by the rest of the clk subsystem, and with the clk_qcom.h header they implement. No functional change. Update sub.mk in the same commit so the tree still builds; renaming the sources alone leaves srcs-y pointing at the old paths. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude:claude-opus-5
Add an API to copy a resource's auxiliary data blob by resource ID. For ARC resources this is the list of corner levels the rail supports, which RPMh commands index into rather than accepting a raw voltage. This is a prerequisite for the QUP SE clock driver's CX/MX voltage vote, which resolves a rail's supported-corner ordinal list via this API before voting a corner over RPMh. copy_aux_data() previously clamped the copy to the caller's buffer and returned success, so a caller with an undersized buffer received a silently truncated blob it had no way to detect. Return TEE_ERROR_SHORT_BUFFER with the required size instead, and copy nothing. Both existing callers query metadata only (NULL data buffer, zero length) and never reach this path. Also fix cmd_db_get_entry_by_res_id() to clear result->len on the zero-length-entry path, which cmd_db_get_aux() exercises. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude:claude-opus-5
Register each QUPv3 serial-engine RCG on lemans as a standard struct clk with no parent (clk_ops: enable/disable/set_rate/get_rate), consumed on demand by a future TEE-side SPI/I2C driver via qcom_clk_get_by_name() (this platform has no secure DT, so DT-based acquire isn't available). set_rate walks a per-domain frequency-config table (mux/divider/MND/ DFS-index), and votes a CX/MX voltage corner via RPMh around the rate change -- raise before programming, lower after -- using an aggregate reference-counted vote model. The voltage vote requires cmd_db_get_addr()/cmd_db_get_aux() to resolve the rail's RPMh resource address and its supported-corner ordinal list, so CFG_QCOM_CMD_DB/ CFG_QCOM_RPMH_CLIENT are now force-enabled whenever CFG_QCOM_CLK_BSP=y (default y). CX and MX are voted as a pair, so a failure between the two would leave the rails disagreeing with the cached corner while the no-change shortcut suppressed the corrective re-vote. Track that mismatch and re-vote both rails on the next call. The rail-vote backend lives in clk-qcom-vreg.c behind the qcom_clk_vreg_vote() contract, so a non-RPMh target can supply its own without touching the RCG walker. The per-target domain table and frequency plans live in platform/<flavor>/clk-qcom-bsp.c, keeping the filename flavor-generic so enabling CFG_QCOM_CLK_BSP on another target needs no sub.mk change. Known gap, documented but not blocking: the voltage vote is bypassed on a hardware-driven DFS switch (it only covers explicit set_rate). Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude:claude-opus-5
f04d2b3 to
ded401b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
core/drivers/clk/qcom/clk-qcom-vreg.c:82
- All errors from the MX lookup are currently treated as “MX is absent.” In particular, a transient
TEE_ERROR_BAD_STATEor corrupted command DB would permanently setreadyand allow rate changes with CX-only votes. OnlyTEE_ERROR_ITEM_NOT_FOUNDshould make MX optional; propagate other failures so initialization can be retried safely.
/* MX tracks CX on this target; vote it too when the rail exists. */
if (!cmd_db_get_addr("mx.lvl", &v->mx_addr))
v->have_mx = true;
core/drivers/clk/qcom/clk-qcom.c:466
- When the CLK_OFF poll times out, the function returns an error but leaves this SE's shared branch-vote bit asserted. The clock core therefore keeps its enable count at zero while hardware remains requested on, leaking power and making the failed enable non-transactional. Clear the vote bit before returning the timeout.
REG_POLL_TIMEOUT(cbcr, 10 * 1000, 10, &ret, cbcr_branch_on);
return ret < 0 ? TEE_ERROR_TIMEOUT : TEE_SUCCESS;
core/drivers/clk/qcom/clk-qcom.c:482
- Disabling an SE clears only its branch vote; it never removes
qup->cornerfrom the aggregate CX/MX reference counts. Since the first successfulclk_set_rate()adds that reference even while the clock is disabled, every SE ever configured permanently pins the rails at its historical corner. Release the corner on the final disable and reacquire it before the next enable, while retaining the configured corner separately for rate reporting/re-enable.
cbcr = gcc_base + dom->cbcr_offset;
io_clrbits32(gcc_base + dom->vote_reg_offset, BIT(dom->vote_bit));
core/drivers/clk/qcom/clk-qcom.c:420
rateis anunsigned long, but passing it to theuint32_thelper silently truncates requests aboveUINT32_MAX. On AArch64, for example, 4 GHz + 294,967,296 Hz becomes zero and resolves to the plan's minimum rate instead of being rejected. Validate the public clock API value before narrowing it.
struct qcom_qup_clk *qup = clk->priv;
uint32_t res_hz = 0;
TEE_Result res = TEE_SUCCESS;
res = qcom_domain_set_rate(qup->domain, rate, &qup->corner, &res_hz);
| /* Raise the rail before speeding up; abort on failure. */ | ||
| if (next > prev) { | ||
| res = qcom_clk_vreg_vote(prev, next); | ||
| if (res) | ||
| return res; | ||
| *corner = next; | ||
| } |
| clk = clk_alloc(bsp->domains[i].name, &qcom_qup_clk_ops, | ||
| NULL, 0); | ||
| if (!clk) | ||
| return TEE_ERROR_OUT_OF_MEMORY; |
The CBCR-to-CMD_RCGR distance used to derive one register's location from another is not architectural: it is 8 bytes on lemans but not constant across chipsets (nord's QUP SEs sit 0x10 apart, and nord's QUPv3 wrapper 3 keeps no fixed relation at all). Deriving offsets that way silently breaks on any target where the assumption doesn't hold. Give struct qcom_clk_domain and struct qcom_clk_src_vote a full physical address per register (cmd_rcgr_addr/cbcr_addr/vote_reg_addr) instead of one GCC-relative offset each, matching how the reference clock driver's own HWIO_<reg>_ADDR macros are built. Add struct qcom_clk_window so a domain names the register window its addresses fall inside rather than assuming a single global GCC base -- needed because some targets split their QUP SEs across more than one clock controller. The walker resolves each address against its domain's window and bounds-checks it before use. Convert lemans' header macros from GCC-relative offsets to (GCC_BASE + offset) form to match, and update its BSP table to the renamed struct fields. No functional change on lemans: GCC_BASE is still the only window, and every resolved address is identical to the previous offset-based one. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude:claude-opus-5
nnunna94, If this is also waiting for consumers/dependent PRs to be ready, Can you please move this as well to draft Naresh? |
#33 i think spi changes are also in review here..once done we can merge together . |
It needs to be raised a single committable PR with the consumer Naresh; So ideally the SPI changes PR can pull these changes with your signed off/author/etc. and have it as part of their PRs. Jorge A. Ramirez-Ortiz (@ldts), Hope that makes sense? |
that should be ok but this becomes a big PR... FYI.. the pr will have clock, tlmm and spi driver all three ..since this is a dependency for spi team, this landed a PR first i believe... |
Just checked the number of lines it'd be <= 4600 in its current state if we combine the three PRs and if we clean up the commits/remove comments etc. it might reduce further as well so should be Ok I think; The fuse PR also was almost around those many lines. |
This series adds QUPv3 serial-engine (QUP SE) clock support to the Qualcomm
lemans target, for on-demand use by a TEE-side SPI/I2C driver.