clk: qcom: lemans: add QUP SE clock set-rate/DFS/enable support - #26
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: |
| 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 |
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; |
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. |
|
please rework all the abstractions and propose again:. you have a added a new driver (a voltage regulator?) and hidden it under clock. doesnt seem like something we want to do. if it is just a clock helper but unrelated to the actual clock, what about just qcom_corner_vote.c, or qcom_rail_vote.c? |
|
rename -bsp.c to -cfg.c |
Thats just a file to aggregate on voltage requests from clock driver side and send it to RPMH driver. this voting mechanism changes based on architecture so had to separate it out from clk-qcom.c to support both shikra and lemans with same clk-qcom.c the file name may be adjusted accordingly. |
so what is the abstraction then? is this common to architecture or platform? do you need to move clk/qcom/platform to clk/qcom/$arch/$platform and then have clk/qcom/hoya/qcom_corner_vote.c and clk/qcom/qcom_corner_vote.h? |
The idea was to use this file for all hoya and wildcat architectures, for any rpm based architectures introduce a qcom_vreg_rpm.c file or so and based on chipset we decide which file to include in mk files keeping the API interfaces same for clk-qcom.c to remain the same. As of now we did not introduce any architecture folders as such. |
ok so it is not per architecture then. so lets have qcom_vreg_rpmh.c and qcom_vreg.h? the problem with vreg is that one expects a regulator driver...not sure if we can come with a better name equally short.. |
|
Address above two comments, rest looks good. |
addressed how? I saw no change in the code and no comments to the request of the investigation. |
91028d4 to
c860fe6
Compare
|
Please update the copyrights. |
c860fe6 to
4f179b8
Compare
updated copyrights |
|
please run scripts/checkpatch.sh on each commit and fix the warnings |
c2523a3 to
e283296
Compare
|
Ran Checkpatch on the commits and updated the latest commit series. the only warnings left are something recommending to use u32 instead of uint32. resolved most of them. |
ca169e2 to
1a117cb
Compare
e283296 to
acbb44d
Compare
|
You can add my reviewed-by:
|
|
Jorge A. Ramirez-Ortiz (@ldts) feel free to merge this since you have mostly reviewed this PR. I will let nnunna94 post this series upstream where I can help further review it. |
1a117cb to
dab3efd
Compare
c800e25 to
51e236e
Compare
is this something i need to add in commit messages ? |
Yes right, please refer to the commit msgs I shared as reference. You can have my tag as well -
|
b7471bc to
acbb44d
Compare
This file already lives under drivers/clk/qcom/, so repeating qcom in its own name is redundant, and inconsistent with how other vendor subdirectories (sam/, stm32) name their own files. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude:opus-5 Reviewed-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> Reviewed-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com> Reviewed-by: Vinod Kumar Amanaganti <vinoda@qti.qualcomm.com>
The QUP SE clock driver's CX/MX voltage vote needs a rail's supported corner ordinals, which for ARC resources live in the auxiliary data blob RPMh commands index into rather than a raw voltage. Add cmd_db_get_aux() to fetch it by resource ID. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude:opus-5 Reviewed-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> Reviewed-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com> Reviewed-by: Vinod Kumar Amanaganti <vinoda@qti.qualcomm.com>
Some clock rates need a higher CX/MX voltage corner than others, and a corner may be shared by multiple RCGs; rail_vote() refcounts per corner over RPMh so each rail is only raised for as long as some caller actually needs it, and CX/MX are resolved independently since they don't necessarily share the same hlvl encoding. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude:opus-5 Assisted-by: Claude:sonnet-5 Reviewed-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> Reviewed-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com> Reviewed-by: Vinod Kumar Amanaganti <vinoda@qti.qualcomm.com>
Lemans has no secure DT, so register each QUPv3 SE clock as a plain struct clk, modeling the PLL/RCG/branch as separate objects so the generic framework's own refcounting governs each PLL vote. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude:opus-5 Assisted-by: Claude:sonnet-5 Reviewed-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com> Reviewed-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com> Reviewed-by: Vinod Kumar Amanaganti <vinoda@qti.qualcomm.com>
acbb44d to
d481d77
Compare
added Reviewed-by Tags to all 4 commits. |
nnunna94, Have you raised upstream PR for this? Can you please share it here? I think for merging in qcom-next, raising an upstream PR is like a gating check now (expected to be enforced). We could request Jorge to merge it once you have raised the upstream PR. |
OP-TEE/optee_os#7996 Upstream PR for the same |
6909f6d
into
qualcomm-linux:qcom-next
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.