Re: [PATCH 1/2] firmware: qcom: scm: Add SCM storage interface support

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 12/18/25 7:02 PM, Junhao Xie wrote:
> Add infrastructure to support accessing TrustZone-protected storage
> devices through SCM (Secure Channel Manager) calls. Some Qualcomm
> platforms protect their firmware storage (typically SPI NOR flash)
> via TrustZone, making it inaccessible from the non-secure world.
> 
> Currently allowlisted for Radxa Dragon Q6A (QCS6490) where it has been
> validated. Additional platforms can be added as they are tested.
> 
> Signed-off-by: Junhao Xie <bigfoot@radxa.com>
> Tested-by: Xilin Wu <sophon@radxa.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c       | 183 +++++++++++++++++++++++++
>  drivers/firmware/qcom/qcom_scm.h       |   3 +
>  include/linux/firmware/qcom/qcom_scm.h |  47 +++++++
>  3 files changed, 233 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 731074ca1ebbe..b117e1b58e363 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -66,6 +66,21 @@ struct qcom_scm_mem_map_info {
>  	__le64 mem_size;
>  };
>  
> +struct qcom_scm_storage_cmd {
> +	__le64 storage_type;
> +	__le64 slot_num;
> +	__le64 lun;
> +	__le64 guid_ptr;
> +	__le64 storage_cmd;
> +};
> +
> +struct qcom_scm_storage_cmd_details {
> +	__le64 lba;
> +	__le64 length;
> +	__le64 data_ptr;
> +	__le64 data_size;
> +};

Let's make them __packed if only to denote that they're shared structures
(no change to the compiler output because it's n*u64)

[...]

> +#if IS_ENABLED(CONFIG_MTD_QCOM_SCM_STORAGE)

I would vouch for this to be always compiled-in

> +int qcom_scm_storage_send_cmd(enum qcom_scm_storage_type storage_type,
> +			      enum qcom_scm_storage_cmd_id cmd_id,
> +			      u64 lba, void *payload, size_t size)

Please align the parameter whitespace (checkpatch, maybe w/ --strict
should point that out)

> +{
> +	struct qcom_scm_res scm_res = {};
> +	struct qcom_scm_desc desc = {};
> +	struct qcom_scm_storage_cmd *cmd;
> +	struct qcom_scm_storage_cmd_details *details;
> +	size_t buf_size;
> +	void *payload_buf;
> +	int ret;

Reverse-Christmas-tree would be neat (it's in a week!)

> +
> +	buf_size = sizeof(*cmd) + sizeof(*details);
> +	if (payload)
> +		buf_size += size;
> +	void *data __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
> +							 buf_size,
> +							 GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	memset(data, 0, buf_size);
> +	if (payload) {
> +		payload_buf = data + sizeof(*cmd) + sizeof(*details);
> +		memcpy(payload_buf, payload, size);
> +	}
> +
> +	cmd = data;
> +	cmd->storage_type = storage_type;
> +	cmd->storage_cmd = cmd_id;
> +
> +	details = data + sizeof(*cmd);
> +	details->lba = lba;

I'm debating whether adding something like:

struct qcom_scm_storage_payload {
	struct qcom_scm_storage_cmd *cmd;
	struct qcom_scm_storage_cmd_details *details;
	void *data[];
};

would improve readability, but perhaps for just 3 items it's simply not
worth the boilerplate

[...]


> +static int qcom_scm_storage_init(struct qcom_scm *scm)
> +{
> +	struct qcom_scm_storage_info info;
> +	struct platform_device *storage_dev;
> +	int ret;
> +
> +	ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
> +					QCOM_SCM_STORAGE_GET_INFO,
> +					0, &info, sizeof(info));
> +	if (ret < 0) {
> +		dev_info(scm->dev, "scm storage not available: %d\n", ret);
> +		return 0;
> +	}

You can first call __qcom_scm_is_call_available for even more robustness

> +
> +	if (!qcom_scm_storage_machine_is_allowed()) {
> +		dev_info(scm->dev, "scm storage untested, skipping\n");
> +		return 0;
> +	}

FWIW UEFI uses these APIs, so if the implementation is correct, I see no
reason to worry

> +
> +	dev_info(scm->dev, "scm storage size %llu bytes\n",
> +		 info.total_blocks * info.block_size);

dev_dbg?

> +
> +	storage_dev = platform_device_alloc("qcom_scm_storage", -1);
> +	if (!storage_dev)
> +		return -ENOMEM;
> +
> +	storage_dev->dev.parent = scm->dev;
> +
> +	ret = platform_device_add(storage_dev);
> +	if (ret) {
> +		platform_device_put(storage_dev);
> +		return ret;
> +	}
> +
> +	return devm_add_action_or_reset(scm->dev, qcom_scm_storage_free,
> +					storage_dev);

fauxbus?

> +}
> +
> +#else /* CONFIG_MTD_QCOM_SCM_STORAGE */
> +
> +static int qcom_scm_storage_init(struct qcom_scm *scm)
> +{
> +	return 0;
> +}
> +
> +#endif /* CONFIG_MTD_QCOM_SCM_STORAGE */
> +
>  /**
>   * qcom_scm_is_available() - Checks if SCM is available
>   */
> @@ -2449,6 +2626,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	/* Initialize the QTEE object interface. */
>  	qcom_scm_qtee_init(scm);
>  
> +	/*
> +	 * Initialize the SCM storage interface.
> +	 */

/* This fits in a single-line comment */


> +	ret = qcom_scm_storage_init(scm);
> +	WARN(ret < 0, "failed to initialize scm storage: %d\n", ret);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
> index a56c8212cc0c4..3b68b33c5ccc3 100644
> --- a/drivers/firmware/qcom/qcom_scm.h
> +++ b/drivers/firmware/qcom/qcom_scm.h
> @@ -149,6 +149,9 @@ int qcom_scm_shm_bridge_enable(struct device *scm_dev);
>  #define QCOM_SCM_SMMU_CONFIG_ERRATA1		0x03
>  #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL	0x02
>  
> +#define QCOM_SCM_SVC_STORAGE			0x1a
> +#define QCOM_SCM_STORAGE_CMD			0x01
> +
>  #define QCOM_SCM_SVC_WAITQ			0x24
>  #define QCOM_SCM_WAITQ_RESUME			0x02
>  #define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index a55ca771286bf..41f799d8de54f 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -53,6 +53,36 @@ enum qcom_scm_ice_cipher {
>  	QCOM_SCM_ICE_CIPHER_AES_256_CBC = 4,
>  };
>  
> +enum qcom_scm_storage_cmd_id {
> +	QCOM_SCM_STORAGE_INIT      = 0,
> +	QCOM_SCM_STORAGE_READ      = 1,
> +	QCOM_SCM_STORAGE_WRITE     = 2,
> +	QCOM_SCM_STORAGE_ERASE     = 3,
> +	QCOM_SCM_STORAGE_GET_INFO  = 4,
> +	QCOM_SCM_STORAGE_DEINIT    = 5,

6 -> _MAC_MISMATCH -> EBADMSG? (invalid data hash)
7 -> _ALREADY_RUNNING -> -EALREADY
8 -> _PARTITION_NOT_FOUND -> -ENOENT?
9 -> _READONLY -> -EROFS

> +};
> +
> +enum qcom_scm_storage_type {
> +	QCOM_SCM_STORAGE_NULL    = 0,
> +	QCOM_SCM_STORAGE_SPINOR  = 1,
> +};
> +
> +#define QCOM_SCM_STORAGE_FW_VER_LEN	32
> +#define QCOM_SCM_STORAGE_MEM_TYPE_LEN	5
> +#define QCOM_SCM_STORAGE_PROD_NAME_LEN	32
> +
> +struct qcom_scm_storage_info {
> +	u64 total_blocks;
> +	u32 block_size;
> +	u32 page_size;
> +	u32 num_physical;
> +	u64 manufacturer_id;
> +	u64 serial_num;
> +	char fw_version[QCOM_SCM_STORAGE_FW_VER_LEN];
> +	char memory_type[QCOM_SCM_STORAGE_MEM_TYPE_LEN];
> +	char product_name[QCOM_SCM_STORAGE_PROD_NAME_LEN];

I would strongly assume all variables here are little-endian as well

Konrad