drivers: qcom: Add CMD_DB, RPMH, and QFPROM drivers for Kodiak#5
drivers: qcom: Add CMD_DB, RPMH, and QFPROM drivers for Kodiak#5zelvam95 wants to merge 11 commits intoqualcomm-linux:qcom-nextfrom
Conversation
|
@zelvam95 thank you for the extensive and detailed documentation — that’s really nice. Before going into a more formal the review, I’d suggest we tighten alignment with the coding standards to avoid future maintenance pain:
If a function is named rpmh_add_resource_command, there’s no need for a variable like old_num_resource_command. Keep variable names concise and non-redundant — duplication of meaning adds friction.
Names such as old_num_resource_commands are particularly problematic. State transitions should be expressed structurally in the code, not baked into identifiers.
Applying just these few rules will already make the codebase significantly easier to maintain and reason about. |
a0c6dc6 to
498d1ff
Compare
|
@zelvam95 I can see you are adding additional functionality and that is great. However we will just be delaying the upstreaming since the very basic is not in good shape yet (coding standards, decent naming of variables and functions so the code is easy to read and maintain). I fear that we are going to end up with thousands of lines to review which wont get merged (I will have to NAK from my end, not sure what Sumit will do). My advice would be that you give priority to the review before adding functionality (which is the easy bit at this point). making the code easy to read and maintain is on par with functionality when upstreaming (ie, functional code will not be merged unless it is easy to follow up and maintain). |
Hi Jorge, Sorry, but I haven't added any "additional" or "new" functionality. I am prioritizing the review comments addressing. I have addressed your comment + Kishore's comment as part of these recent pushes. |
|
um, didnt you just add TCSR_MUTEX_BASE? ah ok, yeah I see. anyway, still this is just a reminder :) and thanks for following up. |
I actually got above comment from Kishore & I found it valid and I tried to address this comment which required the addition of TCSR HW Based mutex. As discussed, I am prioritizing all the review comments and will respond here to the comments once they are resolved & start a fresh review. Just to update -> I have taken your feedback and what you said about variable names/function names is valid. I agree that is something that needs to be fixed & I am in-process of doing it. |
29b04f6 to
426468c
Compare
Add CMD_DB (Command Database) driver for Qualcomm platforms. CMD_DB provides resource address lookup functionality from a shared memory database maintained by the AOP (Always-On Processor). The driver supports: - Resource address queries by resource ID - Priority and version information retrieval - Auxiliary data access Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Add platform-specific configuration for CMD_DB driver on Kodiak. The configuration defines AOP message RAM parameters required to locate the Command Database in shared memory. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Enable CMD_DB driver for Kodiak platform and register the AOP message RAM memory region required for CMD_DB operation. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Add Resource Power Manager Hardened (RPMH) client driver for power management on Qualcomm platforms. The driver provides: - TCS (Task Control Structure) command execution - Client handle management for multiple DRVs - Synchronous command completion - AOP (Always On Processor) synchronization - Sleep/wake power management support Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Add platform-specific configuration for RPMH driver on Kodiak. The configuration defines TCS (Trigger Command Set) allocation and interrupt settings for the RPMH hardware. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Enable RPMH client driver for Kodiak platform and register the RPMH RSC (Resource State Coordinator) memory region required for RPMH operation. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
89bd033 to
814f33c
Compare
Add support for Qualcomm QFPROM (Fuse Programmable Read-Only Memory), a one-time programmable (OTP) memory used for storing security credentials and device configuration. The driver provides: - Read/write operations for QFPROM rows with hardware sequencing - Support for RAW and CORR (error-corrected) address spaces - FEC (Forward Error Correction) support with error detection - Region-based permission checking via runtime register reads - Hardware mutex support for multi-core synchronization - Batch write operations for efficient programming The driver provides a secure interface for fuse operations, handling all necessary hardware sequencing including voltage ramping, clock configuration, and write completion polling. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Add support for secure fuse provisioning using sec.elf V2 format files. This enables provisioning of various fuse regions including security keys, OEM configuration, and access permissions during boot. The implementation provides: - sec.elf V2 format parsing - Support for ENCKEY and EFUSE segments with proper sequencing - Sequential region provisioning to maintain security dependencies - Boot-time provisioning with automatic discovery from DDR - Integration with existing QFPROM driver infrastructure Fuse provisioning follows a specific sequence to ensure system security: encryption keys are provisioned first, followed by efuse regions in dependency order (GENERAL, SHK, OEM_SPARE, OEM_CONFIG, SECBOOT, FEC_EN, RW_PERM). The provisioning engine includes comprehensive validation of file format, segment offsets, and cryptographic integrity to ensure secure operation. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Add platform-specific configuration for QFPROM driver on Kodiak. The configuration includes fuse region definitions, hardware register offsets, and platform-specific voltage/clock control for secure fuse operations. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Enable QFPROM driver for Kodiak platform and register the Security Control and Clock Control memory regions required for secure OTP fuse operations. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
Enable QFPROM fuse provisioning driver for Kodiak platform. This allows secure bulk fuse programming operations through a dedicated secure ELF memory region. Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@qti.qualcomm.com>
814f33c to
03ed234
Compare
|
@ldts / @kishorebatta-ossqcom / @b49020 Can you please help to review the draft PR recent revision? I have addressed all the review comments to the best of my understanding. |
|
|
||
| #define CMD_DB_AOP_DATA_ADDR (AOP_MSG_RAM_END - 0x10000 + 0xC) | ||
|
|
||
| #define CMD_DB_SIZE 0x20000 |
There was a problem hiding this comment.
The size is also present in the AOP_DATA I think:
cmd_db_size = (uint64_t)(*((uint32_t *)CMD_DB_AOP_DATA_SZ));
Should we read the size from the buffer, or leave hard coded?
| slv_id_valid = (cmd_db_validate_slv_id(query_info->slv_id) == | ||
| CMD_DB_SUCCESS); | ||
|
|
||
| for (i = 0; i < CMD_DB_MAX_SLV_ID; i++) { |
There was a problem hiding this comment.
Small optimization to not enter the for() loop if the slv id is not valid.
There was a problem hiding this comment.
Oh, nevermind, the slv_id_valid is a hint, got it
| { | ||
| struct cmd_db_query_info query = {0}; | ||
| struct cmd_db_query_result_type result = {0}; | ||
|
|
There was a problem hiding this comment.
If res_id is NULL as detected in the call to cmd_db_validate_res_id() then we lose the return code and return 0. Perhaps we should return the _ERR_INVALID_PARAM in this case instead of 0? Or return what we get back from cmd_db_validate_res_id()
| uint32_t size; | ||
| uint8_t info[16]; | ||
| uint32_t seg_num; | ||
| } __packed; |
There was a problem hiding this comment.
I am wondering if we need to include the reserved bytes in these structures, since otherwise we may see failures when using sizeof() and offsets.
So
uint32_t reserved[3];
as the last entry
| RPMH_NUM_SETS = 3, | ||
| }; | ||
|
|
||
| enum RSCSW_DRV_MAPPING { |
There was a problem hiding this comment.
Might be worth changing this to lower case to match the coding style, otherwise it looks a bit like a macro.
| RSC_DRV_VIRTUAL_MAX = 0x3FFFFFFF, | ||
| }; | ||
|
|
||
| struct rpmh_command_t { |
There was a problem hiding this comment.
Left over _t from when this was a typedef?
| bool completion; | ||
| }; | ||
|
|
||
| struct rpmh_command_set_t { |
| * | ||
| * Return: A bitmask of which sets are dirty | ||
| */ | ||
| uint32_t sets_dirty(struct rpmh_resource_command *rc, |
There was a problem hiding this comment.
Curious as to why not:
rpmh_resource_sets_dirty
| * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. | ||
| */ | ||
|
|
||
| #ifndef __RPMH_RESOURCE_COMMANDS_H__ |
There was a problem hiding this comment.
The exlusion guards are following slightly different conventions for the new files, some are prefixed with _DRIVERS, should we align?
| struct rpmh_command_set_internal; | ||
|
|
||
| /* Complete struct definitions needed by TCS layer */ | ||
| struct rpmh_client_t { |
|
|
||
| /* Forward declarations */ | ||
| struct rpmh_client_t; | ||
| struct rpmh_command_set_internal; |
| */ | ||
| struct platform_ops { | ||
| /* Enable QFPROM voltage rail for fuse blowing */ | ||
| TEE_Result (*enable_voltage)(void); |
There was a problem hiding this comment.
I think we have dicussed before, but I cannot remember on which PR. I recall that funciton pointers were generally not liked in the OP-TEE code, if so, perhaps this needs a revisit.
| @@ -0,0 +1,158 @@ | |||
| // SPDX-License-Identifier: BSD-2-Clause | |||
There was a problem hiding this comment.
Is this file really specific to Kodiak? Or could it be a common file for Hoya (since I know this will change for other architectures)?
| if (!value || !ctx || !ctx->config) | ||
| return; | ||
|
|
||
| base = ctx->config->qfprom_raw_base; |
There was a problem hiding this comment.
Minor comment that I see this repeated throughout the code:
base = ctx->config->qfprom_raw_base;
reg = (vaddr_t)phys_to_virt(base + QFPROM_FEC_ESR_OFFSET,
MEM_AREA_IO_SEC, 4);
if (!reg)
return;
With QFPROM_FEC_ESR_OFFSET replaced with a different register as needed. I wonder if it's worth creating a single function to perform this operation?
| * | ||
| * Resource address lookup from shared memory database maintained by AOP. | ||
| */ | ||
|
|
| static uint64_t cmd_db_conv_str_to_uint64(const char *res_id) | ||
| { | ||
| uint64_t value = 0; | ||
| uint8_t i; |
There was a problem hiding this comment.
Do we need to initialize all local variables always to 0 or default value? Is it really required? I previous have got comments that its not required unless its used directly without initialization somewhere else?
There was a problem hiding this comment.
For ex in this func, I am initializing "i" in for loop and not using it before that and so I wanted to know if initialization is really required? If so in all functions for all local variables, we need to initialize? Just wanted to understand & plan to update accordingly in all places as applicable.
There was a problem hiding this comment.
I think the guidelines say we only need to initialise stuff before use. There is no need to initialise everything, especially if it is set later in the function. So for i, in the above case, I would say no as per:
https://kernelnewbies.org/FAQ/CodingStyle
FWIW this seems outdated in terms of saving CPU cycles, and decent compiler would optimize this anyway.
| static uint64_t cmd_db_conv_str_to_uint64(const char *res_id) | ||
| { | ||
| uint64_t value = 0; | ||
| uint8_t i; |
| return 0; | ||
| } | ||
|
|
||
| for (i = 0; i < CMD_DB_RES_ID_MAX_CHARS; i++) { |
There was a problem hiding this comment.
loop using the len value from earlier check
| return TEE_ERROR_GENERIC; | ||
| } | ||
|
|
||
| IMSG("CMD_DB: Initialized (addr=0x%08" PRIxPA ", v=0x%08x)", |
There was a problem hiding this comment.
I dont think we need to be verbose about it. Could we get rid of most of the DMSGs so conditions dont need the curly braces and the code can be read quickly?
| } | ||
|
|
||
| static enum cmd_db_err_type | ||
| cmd_db_search_entry(struct cmd_db_query_info *query_info, uint64_t res_id, |
There was a problem hiding this comment.
can you add helper functions so the code can be understood easily. The debug/EMSG info makes it easier to read when in fact it should be the code itself.
most of the EMSGs here can be dropped IMO. We the caller can EMSG.
| struct cmd_db_target_config config; | ||
| paddr_t addr; | ||
|
|
||
| config = cmd_db_get_target_config(); |
There was a problem hiding this comment.
The cmd_db_get_target_config() usage with value semantics follows standard kernel patterns for small configuration structures as per my understanding? & its a static inline func defined in target specific file... I believe this is acceptable as it is? Let me know if you think otherwise.
There was a problem hiding this comment.
no, that is ok. keep it. I misread
| struct cmd_db_query_info query = {0}; | ||
| struct cmd_db_query_result_type result = {0}; | ||
|
|
||
| if (cmd_db_validate_res_id(res_id) != CMD_DB_SUCCESS) |
There was a problem hiding this comment.
We can, but can we return -ve error code for functions in OPTEE? Is that acceptable? Note that the function is for querying the len and so if we just return +ve error code it could be confusing whether its the length/error?
There was a problem hiding this comment.
right so interface functions should return TEE_Result.
it seems to me this interface should return TEE_Result and the value be retrieved as a parameter instead.
if it was a static, maybe return -1 for errors and positives for success but being an externally accessible driver function I'd use TEE_Result
| return 0; | ||
|
|
||
| if (drv_id > MAX_DRV_ID) { | ||
| DMSG("CMD_DB: Invalid driver ID %u (max %u)", |
There was a problem hiding this comment.
should we be reporting errors instead of returning a 0.
| struct cmd_db_query_result_type result = {0}; | ||
|
|
||
| if (cmd_db_init() != TEE_SUCCESS) { | ||
| DMSG("CMD_DB: Initialization failed"); |
There was a problem hiding this comment.
too much debug information. we should report errors.
| #include <stdint.h> | ||
| #include <stdbool.h> | ||
|
|
||
| void hal_qfprom_set_blow_timer(uint32_t value); |
There was a problem hiding this comment.
Minor comment on documentation. OP-TEE does not mandate documenting of exported APIs, but it is encouraged. The main point I am noticing is an inconsistency between headers with some having quite detailed documentation, and others, like this one, not so much.
For these patch sets at least, is it perhaps worth aligning on one or the other approach?
|
|
||
| struct segment_hdr { | ||
| uint32_t offset; | ||
| uint32_t type; |
There was a problem hiding this comment.
type and attribute needs to be uint16_t
| * | ||
| * Exported for use by TCS management layer. | ||
| */ | ||
| const struct drv_config_data *const DRV_CONFIG_DATA = &optee_config_data; |
There was a problem hiding this comment.
I think this should be in lower case as otherwise DRV_CONFIG_DATA looks like a macro, #define or enum
| */ | ||
|
|
||
| /* TCS Configuration */ | ||
| #define RPMH_TCS_ACTIVE 2 /* TCS 0-1: AMCs */ |
There was a problem hiding this comment.
It feels like this made more sense as an enum, but I'll leave that up to you to decide
| uint32_t region_start, region_end; | ||
| uint32_t offset, row_idx; | ||
|
|
||
| if (type == QFPROM_ADDR_SPACE_RAW) { |
There was a problem hiding this comment.
WARNING: braces {} are not necessary for any arm of this statement
#134: FILE: core/drivers/qcom/qfprom/qfprom_core.c:88:
+ if (type == QFPROM_ADDR_SPACE_RAW) {
[...]
+ } else {
[...]
| const struct qfprom_region_info *region = | ||
| &drv->config->region_data[i]; | ||
|
|
||
| if (addr_type == QFPROM_ADDR_SPACE_RAW) { |
There was a problem hiding this comment.
WARNING: braces {} are not necessary for any arm of this statement
#936: FILE: core/drivers/qcom/qfprom/qfprom_core.c:890:
+ if (addr_type == QFPROM_ADDR_SPACE_RAW) {
[...]
+ } else if (addr_type == QFPROM_ADDR_SPACE_CORR) {
[...]
+ } else {
[...]
| return CMD_DB_SLV_ID_INVALID; | ||
| } | ||
|
|
||
| driver_init(cmd_db_init); |
There was a problem hiding this comment.
should OP-TEE boot if cmd_db is not available? it seems to me it could just be early_init and then panic() if it can start.
I dont understand why are we calling init from a number of places? we should keep it simple and start it first during OP-TEE boot
| return 0; | ||
| } | ||
|
|
||
| static uint32_t cmd_db_extract_priority(struct cmd_db_query_result_type *result, |
There was a problem hiding this comment.
lets not prefix static functions with cmd_db_
This patch series adds support for Qualcomm-specific drivers on the Kodiak platform, enabling power management and secure fuse operations in OP-TEE.
The series introduces three drivers:
Architecture Overview
The drivers form a dependency chain:
QFPROM Driver → uses → RPMH Driver → uses → CMD_DB Driver
Hardware Mapping:
Driver Descriptions
CMD_DB Driver (Patches 1-3)
The Command Database driver provides resource address lookup from a shared memory database maintained by the AOP (Always-On Processor). It enables other drivers to query hardware resource addresses by name.
Key Features:
API:
cmd_db_query_addr()- Get resource address by namecmd_db_query()- Full resource information lookupRPMH Client Driver (Patches 4-6)
The Resource Power Manager Hardened client driver provides power management capabilities through hardware TCS (Trigger Command Sets). It manages voltage regulators, clocks, and other power resources.
Key Features:
API:
rpmh_create_handle()- Create client handlerpmh_issue_command()- Send power management commandrpmh_enter_sleep()/rpmh_exit_sleep()- Power state transitionsDependencies:
QFPROM Driver (Patches 7-11)
The Qualcomm Fuse driver provides secure OTP (One-Time Programmable) fuse read/write operations for secure boot, key provisioning, and device configuration.
Key Features:
API:
qfprom_read_row()- Read fuse valueqfprom_write_row()- Write fuse value (with voltage control)qfprom_write_multiple_rows()- Bulk fuse programmingDependencies:
Interaction Flow: QFPROM Fuse Write
The QFPROM fuse write operation follows this sequence:
qfprom_write_row()rpmh_issue_command("mx.lvl", 15)cmd_db_query_addr("mx.lvl")rpmh_issue_command("mx.lvl", 9)Platform Configuration
All drivers include platform-specific configuration for Kodiak:
Memory Regions:
Hardware Configuration:
Testing
The series has been tested on Kodiak hardware with:
Note: The QFPROM fuse write flow implicitly exercises all three drivers:
Patch Organization
Patches 1-3: CMD_DB driver and Kodiak enablement
Patches 4-6: RPMH driver and Kodiak enablement
Patches 7-9: QFPROM driver and Kodiak enablement
Patches 10-11: QFPROM fuse provisioning and enablement
Each driver follows the pattern: