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

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

[Junhao Xie]

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;
+};
+
 /**
  * struct qcom_scm_qseecom_resp - QSEECOM SCM call response.
  * @result:    Result or status of the SCM call. See &enum qcom_scm_qseecom_result.
@@ -111,6 +126,15 @@ enum qcom_scm_qseecom_tz_cmd_info {
 	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
 };
 
+enum qcom_scm_storage_result {
+	STORAGE_RESULT_SUCCESS			= 0,
+	STORAGE_RESULT_NO_MEMORY		= 1,
+	STORAGE_RESULT_INVALID_PARAMETER	= 2,
+	STORAGE_RESULT_STORAGE_ERROR		= 3,
+	STORAGE_RESULT_ACCESS_DENIED		= 4,
+	STORAGE_RESULT_NOT_SUPPORTED		= 5,
+};
+
 #define QSEECOM_MAX_APP_NAME_SIZE		64
 #define SHMBRIDGE_RESULT_NOTSUPP		4
 
@@ -2214,6 +2238,159 @@ static void qcom_scm_qtee_init(struct qcom_scm *scm)
 	devm_add_action_or_reset(scm->dev, qcom_scm_qtee_free, qtee_dev);
 }
 
+#if IS_ENABLED(CONFIG_MTD_QCOM_SCM_STORAGE)
+
+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)
+{
+	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;
+
+	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;
+	if (payload)
+		details->data_ptr = qcom_tzmem_to_phys(payload_buf);
+	details->length = size;
+
+	desc.svc = QCOM_SCM_SVC_STORAGE;
+	desc.cmd = QCOM_SCM_STORAGE_CMD;
+	desc.arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RO, QCOM_SCM_VAL,
+				     QCOM_SCM_RW, QCOM_SCM_VAL);
+	desc.args[0] = qcom_tzmem_to_phys(cmd);
+	desc.args[1] = sizeof(*cmd);
+	desc.args[2] = qcom_tzmem_to_phys(details);
+	desc.args[3] = sizeof(*details);
+	desc.owner = ARM_SMCCC_OWNER_SIP;
+
+	ret = qcom_scm_call(__scm->dev, &desc, &scm_res);
+	if (ret)
+		return ret;
+
+	if (payload)
+		memcpy(payload, payload_buf, size);
+
+	switch (scm_res.result[0]) {
+	case STORAGE_RESULT_SUCCESS:
+		return 0;
+	case STORAGE_RESULT_NO_MEMORY:
+		return -ENOMEM;
+	case STORAGE_RESULT_INVALID_PARAMETER:
+		return -EINVAL;
+	case STORAGE_RESULT_STORAGE_ERROR:
+		return -EIO;
+	case STORAGE_RESULT_ACCESS_DENIED:
+		return -EACCES;
+	case STORAGE_RESULT_NOT_SUPPORTED:
+		return -EOPNOTSUPP;
+	default:
+		return -EIO;
+	}
+}
+EXPORT_SYMBOL_GPL(qcom_scm_storage_send_cmd);
+
+/*
+ * Allowlist of platforms known to support and have tested SCM storage interface.
+ * This is a safety mechanism to prevent exposing potentially dangerous firmware
+ * access on untested platforms. New platforms should be added here after validation.
+ */
+static const struct of_device_id qcom_scm_storage_allowlist[] = {
+	{ .compatible = "radxa,dragon-q6a" },
+	{ }
+};
+
+static bool qcom_scm_storage_machine_is_allowed(void)
+{
+	struct device_node *np;
+	bool match;
+
+	np = of_find_node_by_path("/");
+	if (!np)
+		return false;
+
+	match = of_match_node(qcom_scm_storage_allowlist, np);
+	of_node_put(np);
+
+	return match;
+}
+
+static void qcom_scm_storage_free(void *data)
+{
+	struct platform_device *storage_dev = data;
+
+	platform_device_unregister(storage_dev);
+}
+
+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;
+	}
+
+	if (!qcom_scm_storage_machine_is_allowed()) {
+		dev_info(scm->dev, "scm storage untested, skipping\n");
+		return 0;
+	}
+
+	dev_info(scm->dev, "scm storage size %llu bytes\n",
+		 info.total_blocks * info.block_size);
+
+	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);
+}
+
+#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.
+	 */
+	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,
+};
+
+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];
+} __packed;
+
 #define QCOM_SCM_PERM_READ       0x4
 #define QCOM_SCM_PERM_WRITE      0x2
 #define QCOM_SCM_PERM_EXEC       0x1
@@ -181,4 +211,21 @@ int qcom_scm_qtee_invoke_smc(phys_addr_t inbuf, size_t inbuf_size,
 int qcom_scm_qtee_callback_response(phys_addr_t buf, size_t buf_size,
 				    u64 *result, u64 *response_type);
 
+#if IS_ENABLED(CONFIG_MTD_QCOM_SCM_STORAGE)
+
+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);
+
+#else /* CONFIG_MTD_QCOM_SCM_STORAGE */
+
+static inline 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)
+{
+	return -EOPNOTSUPP;
+}
+
+#endif /* CONFIG_MTD_QCOM_SCM_STORAGE */
+
 #endif
-- 
2.51.2

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

[Konrad Dybcio]

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

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

[Junhao Xie]

On 2025/12/19 19:45, Konrad Dybcio wrote:
> 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)

I will add the missing __packed to qcom_scm_storage_cmd and
qcom_scm_storage_cmd_details.

> [...]
>
>> +#if IS_ENABLED(CONFIG_MTD_QCOM_SCM_STORAGE)
> I would vouch for this to be always compiled-in

I mimicked CONFIG_QCOM_QSEECOM here, but it seems unneeded. I will remove this macro check.

>> +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)
>

I have already aligned the parameter, but mix of tabs and spaces
causes leading plus in diff to make them appear misaligned?
(checkpatch --strict doesn't report any errors or warnings.)

>> +{
>> +	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!)

Thanks, I will fix it.

>> +
>> +	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

Thanks for the suggestion, I will rework this for better readability in v2.

> [...]
>
>> +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

I will switch to __qcom_scm_is_call_available instead of qcom_scm_storage_machine_is_allowed,
Thank you for your suggestion!

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

I will change to use 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?

For consistency with the existing code in this file,
I will stick to platform_device_add.

>> +}
>> +
>> +#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 */

Thanks, I will fix it.

>> +	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

Thank you for your addition, I will add these codes to qcom_scm_storage_result.

>> +};
>> +
>> +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

I will change to using __le64 and __le32.

> Konrad
>

Thank you for the review, I will incorporate these changes in v2.

Best regards,
Junhao Xie

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

[Bjorn Andersson]

On Fri, Dec 19, 2025 at 02:02:04AM +0800, 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.
> 

By adding the relevant compatible, I'm able to read something from the
SPI-NOR on the SC8280XP CRD as well.

This brings us to the next question, what data do you actually have in
your SPI-NOR? In what way do you use the mtd device that is presented?

On the laptop targets, the SPI-NOR is partitioned with a GPT partition
table, and there's one of more partitions that would be of interest to
parse/access from the kernel...

> 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;
> +};
> +
>  /**
>   * struct qcom_scm_qseecom_resp - QSEECOM SCM call response.
>   * @result:    Result or status of the SCM call. See &enum qcom_scm_qseecom_result.
> @@ -111,6 +126,15 @@ enum qcom_scm_qseecom_tz_cmd_info {
>  	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
>  };
>  
> +enum qcom_scm_storage_result {

These aren't really "enumerations", they are specifically defined
constants, please use #define.

> +	STORAGE_RESULT_SUCCESS			= 0,
> +	STORAGE_RESULT_NO_MEMORY		= 1,
> +	STORAGE_RESULT_INVALID_PARAMETER	= 2,
> +	STORAGE_RESULT_STORAGE_ERROR		= 3,
> +	STORAGE_RESULT_ACCESS_DENIED		= 4,
> +	STORAGE_RESULT_NOT_SUPPORTED		= 5,
> +};
> +
>  #define QSEECOM_MAX_APP_NAME_SIZE		64
>  #define SHMBRIDGE_RESULT_NOTSUPP		4
>  
> @@ -2214,6 +2238,159 @@ static void qcom_scm_qtee_init(struct qcom_scm *scm)
>  	devm_add_action_or_reset(scm->dev, qcom_scm_qtee_free, qtee_dev);
>  }
>  
> +#if IS_ENABLED(CONFIG_MTD_QCOM_SCM_STORAGE)
> +
> +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)
> +{
> +	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;
> +
> +	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;

storage_type is CPU endian, cmd->storage_type is __le64, so all of these
needs cpu_to_le64().

> +	cmd->storage_cmd = cmd_id;
> +
> +	details = data + sizeof(*cmd);
> +	details->lba = lba;
> +	if (payload)
> +		details->data_ptr = qcom_tzmem_to_phys(payload_buf);
> +	details->length = size;
> +
> +	desc.svc = QCOM_SCM_SVC_STORAGE;
> +	desc.cmd = QCOM_SCM_STORAGE_CMD;
> +	desc.arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RO, QCOM_SCM_VAL,
> +				     QCOM_SCM_RW, QCOM_SCM_VAL);
> +	desc.args[0] = qcom_tzmem_to_phys(cmd);
> +	desc.args[1] = sizeof(*cmd);
> +	desc.args[2] = qcom_tzmem_to_phys(details);
> +	desc.args[3] = sizeof(*details);
> +	desc.owner = ARM_SMCCC_OWNER_SIP;
> +
> +	ret = qcom_scm_call(__scm->dev, &desc, &scm_res);
> +	if (ret)
> +		return ret;
> +
> +	if (payload)
> +		memcpy(payload, payload_buf, size);
> +
> +	switch (scm_res.result[0]) {
> +	case STORAGE_RESULT_SUCCESS:
> +		return 0;
> +	case STORAGE_RESULT_NO_MEMORY:
> +		return -ENOMEM;
> +	case STORAGE_RESULT_INVALID_PARAMETER:
> +		return -EINVAL;
> +	case STORAGE_RESULT_STORAGE_ERROR:
> +		return -EIO;
> +	case STORAGE_RESULT_ACCESS_DENIED:
> +		return -EACCES;
> +	case STORAGE_RESULT_NOT_SUPPORTED:
> +		return -EOPNOTSUPP;
> +	default:
> +		return -EIO;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_storage_send_cmd);
> +
> +/*
> + * Allowlist of platforms known to support and have tested SCM storage interface.
> + * This is a safety mechanism to prevent exposing potentially dangerous firmware
> + * access on untested platforms. New platforms should be added here after validation.
> + */
> +static const struct of_device_id qcom_scm_storage_allowlist[] = {
> +	{ .compatible = "radxa,dragon-q6a" },
> +	{ }
> +};
> +
> +static bool qcom_scm_storage_machine_is_allowed(void)
> +{
> +	struct device_node *np;
> +	bool match;
> +
> +	np = of_find_node_by_path("/");
> +	if (!np)
> +		return false;
> +
> +	match = of_match_node(qcom_scm_storage_allowlist, np);
> +	of_node_put(np);
> +
> +	return match;

This function can be rewritten as:

	return !!of_machine_device_match(qcom_scm_storage_allowlist);

> +}
> +
> +static void qcom_scm_storage_free(void *data)
> +{
> +	struct platform_device *storage_dev = data;
> +
> +	platform_device_unregister(storage_dev);
> +}
> +
> +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;
> +	}
> +
> +	if (!qcom_scm_storage_machine_is_allowed()) {
> +		dev_info(scm->dev, "scm storage untested, skipping\n");
> +		return 0;
> +	}
> +
> +	dev_info(scm->dev, "scm storage size %llu bytes\n",
> +		 info.total_blocks * info.block_size);
> +
> +	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);
> +}
> +
> +#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.
> +	 */
> +	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 {

As with qcom_scm_storage_result above, this isn't really an enumeration,
but as it provides you with the type for the function parameters below,
I think it's okay. So, feel free to leave this and qcom_scm_storage_type
as is.

Regards,
Bjorn

> +	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,
> +};
> +
> +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];
> +} __packed;
> +
>  #define QCOM_SCM_PERM_READ       0x4
>  #define QCOM_SCM_PERM_WRITE      0x2
>  #define QCOM_SCM_PERM_EXEC       0x1
> @@ -181,4 +211,21 @@ int qcom_scm_qtee_invoke_smc(phys_addr_t inbuf, size_t inbuf_size,
>  int qcom_scm_qtee_callback_response(phys_addr_t buf, size_t buf_size,
>  				    u64 *result, u64 *response_type);
>  
> +#if IS_ENABLED(CONFIG_MTD_QCOM_SCM_STORAGE)
> +
> +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);
> +
> +#else /* CONFIG_MTD_QCOM_SCM_STORAGE */
> +
> +static inline 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)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +#endif /* CONFIG_MTD_QCOM_SCM_STORAGE */
> +
>  #endif
> -- 
> 2.51.2
> 

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

[Junhao Xie]

On 2026/1/4 1:22, Bjorn Andersson wrote:
> On Fri, Dec 19, 2025 at 02:02:04AM +0800, 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.
>>
> By adding the relevant compatible, I'm able to read something from the
> SPI-NOR on the SC8280XP CRD as well.
>
> This brings us to the next question, what data do you actually have in
> your SPI-NOR? In what way do you use the mtd device that is presented?
>
> On the laptop targets, the SPI-NOR is partitioned with a GPT partition
> table, and there's one of more partitions that would be of interest to
> parse/access from the kernel...

On the Radxa Dragon Q6A, the SPI-NOR stores boot firmware (XBL, TZ, etc.)
and is accessed via SCM only when the system is booted from SPI-NOR.

Earlier Q6A revisions used LE firmware, where the QSPI controller was
directly accessible from Linux. This platform is now transitioning to WP,
where QSPI is exclusively used by TZ and SPI-NOR can only be accessed via
SCM, similar to laptop platforms.

The exposed MTD device provides generic access to this storage. One of its
practical uses is firmware update and recovery. Since Q6A is a development
board, being able to update boot firmware directly from Linux (for example
using edl-ng together with this patch series) is preferred over EFI capsule
based flows.

>> 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;
>> +};
>> +
>>   /**
>>    * struct qcom_scm_qseecom_resp - QSEECOM SCM call response.
>>    * @result:    Result or status of the SCM call. See &enum qcom_scm_qseecom_result.
>> @@ -111,6 +126,15 @@ enum qcom_scm_qseecom_tz_cmd_info {
>>   	QSEECOM_TZ_CMD_INFO_VERSION		= 3,
>>   };
>>   
>> +enum qcom_scm_storage_result {
> These aren't really "enumerations", they are specifically defined
> constants, please use #define.

I will convert them into macros.

>> +	STORAGE_RESULT_SUCCESS			= 0,
>> +	STORAGE_RESULT_NO_MEMORY		= 1,
>> +	STORAGE_RESULT_INVALID_PARAMETER	= 2,
>> +	STORAGE_RESULT_STORAGE_ERROR		= 3,
>> +	STORAGE_RESULT_ACCESS_DENIED		= 4,
>> +	STORAGE_RESULT_NOT_SUPPORTED		= 5,
>> +};
>> +
>>   #define QSEECOM_MAX_APP_NAME_SIZE		64
>>   #define SHMBRIDGE_RESULT_NOTSUPP		4
>>   
>> @@ -2214,6 +2238,159 @@ static void qcom_scm_qtee_init(struct qcom_scm *scm)
>>   	devm_add_action_or_reset(scm->dev, qcom_scm_qtee_free, qtee_dev);
>>   }
>>   
>> +#if IS_ENABLED(CONFIG_MTD_QCOM_SCM_STORAGE)
>> +
>> +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)
>> +{
>> +	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;
>> +
>> +	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;
> storage_type is CPU endian, cmd->storage_type is __le64, so all of these
> needs cpu_to_le64().

I will add the missing endian conversion for all le values.

>> +	cmd->storage_cmd = cmd_id;
>> +
>> +	details = data + sizeof(*cmd);
>> +	details->lba = lba;
[...]
>> +static bool qcom_scm_storage_machine_is_allowed(void)
>> +{
>> +	struct device_node *np;
>> +	bool match;
>> +
>> +	np = of_find_node_by_path("/");
>> +	if (!np)
>> +		return false;
>> +
>> +	match = of_match_node(qcom_scm_storage_allowlist, np);
>> +	of_node_put(np);
>> +
>> +	return match;
> This function can be rewritten as:
>
> 	return !!of_machine_device_match(qcom_scm_storage_allowlist);

I will replace this with a check using __qcom_scm_is_call_available(),
which avoids the need for a machine allowlist and provides a more
generic capability-based test, Thank you for your suggestion!

>> +}
>> +
[...]
>> --- 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 {
> As with qcom_scm_storage_result above, this isn't really an enumeration,
> but as it provides you with the type for the function parameters below,
> I think it's okay. So, feel free to leave this and qcom_scm_storage_type
> as is.
>
> Regards,
> Bjorn
>

Thank you for the review, I will incorporate these changes in v2.

Best regards,
Junhao Xie

[PATCH 2/2] mtd: devices: Add Qualcomm SCM storage driver

[Junhao Xie]

Add MTD driver for accessing storage devices managed by Qualcomm's
TrustZone firmware. On some platforms, BIOS/firmware storage (typically
SPI NOR flash) is not directly accessible from the non-secure world and
all operations must go through SCM (Secure Channel Manager) calls.

Signed-off-by: Junhao Xie <bigfoot@radxa.com>
Tested-by: Xilin Wu <sophon@radxa.com>
---
 drivers/mtd/devices/Kconfig            |  17 ++
 drivers/mtd/devices/Makefile           |   1 +
 drivers/mtd/devices/qcom_scm_storage.c | 256 +++++++++++++++++++++++++
 3 files changed, 274 insertions(+)
 create mode 100644 drivers/mtd/devices/qcom_scm_storage.c

diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index e518dfeee6542..4f73e89a11947 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -194,6 +194,23 @@ config MTD_INTEL_DG
 	  To compile this driver as a module, choose M here: the module
 	  will be called mtd-intel-dg.
 
+config MTD_QCOM_SCM_STORAGE
+	tristate "Qualcomm TrustZone protected storage MTD driver"
+	depends on MTD
+	depends on QCOM_SCM || COMPILE_TEST
+	help
+	  This provides an MTD device to access storage (typically SPI NOR
+	  flash) that is managed by Qualcomm's TrustZone firmware. On some
+	  platforms, the firmware storage is not directly accessible from
+	  the non-secure world and all operations must go through secure
+	  monitor calls.
+
+	  This driver is only functional on devices where the bootloader
+	  has configured TrustZone to expose the storage interface.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called qcom_scm_storage.
+
 comment "Disk-On-Chip Device Drivers"
 
 config MTD_DOCG3
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index 9fe4ce9cffde9..d71d07f811fa2 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
 obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
 obj-$(CONFIG_MTD_POWERNV_FLASH)	+= powernv_flash.o
 obj-$(CONFIG_MTD_INTEL_DG)	+= mtd_intel_dg.o
+obj-$(CONFIG_MTD_QCOM_SCM_STORAGE)	+= qcom_scm_storage.o
 
 
 CFLAGS_docg3.o			+= -I$(src)
diff --git a/drivers/mtd/devices/qcom_scm_storage.c b/drivers/mtd/devices/qcom_scm_storage.c
new file mode 100644
index 0000000000000..bf5a9f423ed7c
--- /dev/null
+++ b/drivers/mtd/devices/qcom_scm_storage.c
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Qualcomm TrustZone SCM Storage Flash driver
+ *
+ * Copyright (c) 2025 Junhao Xie <bigfoot@radxa.com>
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/firmware/qcom/qcom_scm.h>
+
+/*
+ * This driver provides MTD access to storage devices managed by Qualcomm's
+ * TrustZone firmware. The storage (typically SPI NOR flash) is not directly
+ * accessible from the non-secure world and all operations must go through
+ * SCM (Secure Channel Manager) calls.
+ *
+ * A bounce buffer is required because the interface requires
+ * block-aligned addresses and sizes
+ */
+struct qcom_scm_storage {
+	struct device *dev;
+	struct mutex lock;	/* Protects SCM storage operations */
+	struct mtd_info mtd;
+	struct qcom_scm_storage_info info;
+	size_t buffer_size;
+	u8 *buffer;
+};
+
+static int qcom_scm_storage_erase(struct mtd_info *mtd,
+				  struct erase_info *instr)
+{
+	struct qcom_scm_storage *host =
+		container_of(mtd, struct qcom_scm_storage, mtd);
+
+	if (instr->addr % host->info.block_size ||
+	    instr->len % host->info.block_size)
+		return -EINVAL;
+
+	guard(mutex)(&host->lock);
+
+	return qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
+					 QCOM_SCM_STORAGE_ERASE,
+					 instr->addr / host->info.block_size,
+					 0, instr->len);
+}
+
+static int qcom_scm_storage_read(struct mtd_info *mtd,
+				 loff_t from, size_t len,
+				 size_t *retlen, u_char *buf)
+{
+	struct qcom_scm_storage *host =
+		container_of(mtd, struct qcom_scm_storage, mtd);
+	size_t block_size = host->info.block_size;
+	loff_t block_start, block_off, lba;
+	size_t chunk, to_read;
+	int ret = 0;
+
+	if (retlen)
+		*retlen = 0;
+
+	if (from + len > mtd->size)
+		return -EINVAL;
+
+	if (len == 0)
+		return 0;
+
+	guard(mutex)(&host->lock);
+
+	while (len > 0) {
+		block_start = round_down(from, block_size);
+		block_off = from - block_start;
+		lba = block_start / block_size;
+
+		if (block_off || len < block_size) {
+			chunk = min_t(size_t, block_size - block_off, len);
+			to_read = block_size;
+		} else {
+			chunk = round_down(len, block_size);
+			chunk = min_t(size_t, chunk, host->buffer_size);
+			to_read = chunk;
+		}
+
+		ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
+						QCOM_SCM_STORAGE_READ,
+						lba, host->buffer,
+						to_read);
+		if (ret)
+			return ret;
+
+		memcpy(buf, host->buffer + block_off, chunk);
+
+		buf += chunk;
+		from += chunk;
+		len -= chunk;
+		if (retlen)
+			*retlen += chunk;
+	}
+
+	return 0;
+}
+
+static int qcom_scm_storage_write(struct mtd_info *mtd,
+				  loff_t to, size_t len,
+				  size_t *retlen, const u_char *buf)
+{
+	struct qcom_scm_storage *host =
+		container_of(mtd, struct qcom_scm_storage, mtd);
+	size_t block_size = host->info.block_size;
+	loff_t block_start, block_off, lba;
+	size_t chunk, to_write;
+	int ret = 0;
+
+	if (retlen)
+		*retlen = 0;
+
+	if (to + len > mtd->size)
+		return -EINVAL;
+
+	if (len == 0)
+		return 0;
+
+	guard(mutex)(&host->lock);
+
+	while (len > 0) {
+		block_start = round_down(to, block_size);
+		block_off = to - block_start;
+		lba = block_start / block_size;
+
+		if (block_off || len < block_size) {
+			chunk = min_t(size_t, block_size - block_off, len);
+			to_write = block_size;
+
+			ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
+							QCOM_SCM_STORAGE_READ,
+							lba, host->buffer,
+							block_size);
+			if (ret)
+				return ret;
+		} else {
+			chunk = round_down(len, block_size);
+			chunk = min_t(size_t, chunk, host->buffer_size);
+			to_write = chunk;
+		}
+
+		memcpy(host->buffer + block_off, buf, chunk);
+
+		ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
+						QCOM_SCM_STORAGE_WRITE,
+						lba, host->buffer,
+						to_write);
+		if (ret)
+			return ret;
+
+		buf += chunk;
+		to += chunk;
+		len -= chunk;
+		if (retlen)
+			*retlen += chunk;
+	}
+
+	return 0;
+}
+
+static int qcom_scm_storage_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_scm_storage *host;
+	int ret;
+
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, host);
+	host->dev = dev;
+
+	ret = devm_mutex_init(dev, &host->lock);
+	if (ret)
+		return ret;
+
+	host->buffer_size = SZ_256K;
+	host->buffer = devm_kzalloc(dev, host->buffer_size, GFP_KERNEL);
+	if (!host->buffer)
+		return -ENOMEM;
+
+	ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
+					QCOM_SCM_STORAGE_GET_INFO,
+					0, &host->info,
+					sizeof(host->info));
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "failed to get storage info\n");
+
+	if (!host->info.block_size || !host->info.total_blocks)
+		return dev_err_probe(dev, -EINVAL,
+				     "invalid storage geometry\n");
+
+	if (host->info.block_size > host->buffer_size)
+		return dev_err_probe(dev, -EINVAL,
+				     "block size %u exceeds buffer size\n",
+				     host->info.block_size);
+
+	host->mtd.name = dev_name(dev);
+	host->mtd.owner = THIS_MODULE;
+	host->mtd.dev.parent = dev;
+	host->mtd.size = host->info.total_blocks * host->info.block_size;
+	host->mtd.erasesize = host->info.block_size;
+	host->mtd.writesize = host->info.block_size;
+	host->mtd.writebufsize = host->info.block_size;
+	host->mtd.type = MTD_NORFLASH;
+	host->mtd.flags = MTD_WRITEABLE;
+	host->mtd._erase = qcom_scm_storage_erase;
+	host->mtd._read = qcom_scm_storage_read;
+	host->mtd._write = qcom_scm_storage_write;
+
+	ret = mtd_device_register(&host->mtd, NULL, 0);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "scm storage 0x%llx registered with size %llu bytes\n",
+		 host->info.serial_num, host->mtd.size);
+
+	return 0;
+}
+
+static void qcom_scm_storage_remove(struct platform_device *pdev)
+{
+	struct qcom_scm_storage *host = platform_get_drvdata(pdev);
+
+	WARN_ON(mtd_device_unregister(&host->mtd));
+}
+
+static const struct platform_device_id qcom_scm_storage_ids[] = {
+	{ "qcom_scm_storage", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, qcom_scm_storage_ids);
+
+static struct platform_driver qcom_scm_storage_driver = {
+	.probe	= qcom_scm_storage_probe,
+	.remove	= qcom_scm_storage_remove,
+	.driver = {
+		.name	= "qcom_scm_storage",
+	},
+	.id_table = qcom_scm_storage_ids,
+};
+module_platform_driver(qcom_scm_storage_driver);
+
+MODULE_AUTHOR("Junhao Xie <bigfoot@radxa.com>");
+MODULE_DESCRIPTION("Qualcomm TrustZone SCM Storage Flash driver");
+MODULE_LICENSE("GPL");
-- 
2.51.2

Re: [PATCH 2/2] mtd: devices: Add Qualcomm SCM storage driver

[Konrad Dybcio]

On 12/18/25 7:02 PM, Junhao Xie wrote:
> Add MTD driver for accessing storage devices managed by Qualcomm's
> TrustZone firmware. On some platforms, BIOS/firmware storage (typically
> SPI NOR flash) is not directly accessible from the non-secure world and
> all operations must go through SCM (Secure Channel Manager) calls.
> 
> Signed-off-by: Junhao Xie <bigfoot@radxa.com>
> Tested-by: Xilin Wu <sophon@radxa.com>
> ---

[...]

> +struct qcom_scm_storage {
> +	struct device *dev;
> +	struct mutex lock;	/* Protects SCM storage operations */
> +	struct mtd_info mtd;
> +	struct qcom_scm_storage_info info;
> +	size_t buffer_size;
> +	u8 *buffer;
> +};
> +
> +static int qcom_scm_storage_erase(struct mtd_info *mtd,
> +				  struct erase_info *instr)
> +{
> +	struct qcom_scm_storage *host =
> +		container_of(mtd, struct qcom_scm_storage, mtd);
> +
> +	if (instr->addr % host->info.block_size ||
> +	    instr->len % host->info.block_size)

While it's the same value, it seems like mtd->erasesize would be
"idiomatic" here

> +		return -EINVAL;
> +
> +	guard(mutex)(&host->lock);
> +
> +	return qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
> +					 QCOM_SCM_STORAGE_ERASE,
> +					 instr->addr / host->info.block_size,
> +					 0, instr->len);
> +}
> +
> +static int qcom_scm_storage_read(struct mtd_info *mtd,
> +				 loff_t from, size_t len,
> +				 size_t *retlen, u_char *buf)
> +{
> +	struct qcom_scm_storage *host =
> +		container_of(mtd, struct qcom_scm_storage, mtd);

Feel free to unwrap this line

> +	size_t block_size = host->info.block_size;
> +	loff_t block_start, block_off, lba;
> +	size_t chunk, to_read;
> +	int ret = 0;

This initialization seems unnecessary

[...]

> +static int qcom_scm_storage_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_scm_storage *host;
> +	int ret;
> +
> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, host);
> +	host->dev = dev;
> +
> +	ret = devm_mutex_init(dev, &host->lock);
> +	if (ret)
> +		return ret;
> +
> +	host->buffer_size = SZ_256K;

Should this just be = host->info->page_size?

[...]

> +	dev_info(dev, "scm storage 0x%llx registered with size %llu bytes\n",
> +		 host->info.serial_num, host->mtd.size);

dev_dbg()?

Konrad

Re: [PATCH 2/2] mtd: devices: Add Qualcomm SCM storage driver

[Junhao Xie]

On 2025/12/19 20:05, Konrad Dybcio wrote:
> On 12/18/25 7:02 PM, Junhao Xie wrote:
>> Add MTD driver for accessing storage devices managed by Qualcomm's
>> TrustZone firmware. On some platforms, BIOS/firmware storage (typically
>> SPI NOR flash) is not directly accessible from the non-secure world and
>> all operations must go through SCM (Secure Channel Manager) calls.
>>
>> Signed-off-by: Junhao Xie <bigfoot@radxa.com>
>> Tested-by: Xilin Wu <sophon@radxa.com>
>> ---
> [...]
>
>> +struct qcom_scm_storage {
>> +	struct device *dev;
>> +	struct mutex lock;	/* Protects SCM storage operations */
>> +	struct mtd_info mtd;
>> +	struct qcom_scm_storage_info info;
>> +	size_t buffer_size;
>> +	u8 *buffer;
>> +};
>> +
>> +static int qcom_scm_storage_erase(struct mtd_info *mtd,
>> +				  struct erase_info *instr)
>> +{
>> +	struct qcom_scm_storage *host =
>> +		container_of(mtd, struct qcom_scm_storage, mtd);
>> +
>> +	if (instr->addr % host->info.block_size ||
>> +	    instr->len % host->info.block_size)
> While it's the same value, it seems like mtd->erasesize would be
> "idiomatic" here

I will change to use host->mtd.erasesize here.

>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&host->lock);
>> +
>> +	return qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
>> +					 QCOM_SCM_STORAGE_ERASE,
>> +					 instr->addr / host->info.block_size,
>> +					 0, instr->len);
>> +}
>> +
>> +static int qcom_scm_storage_read(struct mtd_info *mtd,
>> +				 loff_t from, size_t len,
>> +				 size_t *retlen, u_char *buf)
>> +{
>> +	struct qcom_scm_storage *host =
>> +		container_of(mtd, struct qcom_scm_storage, mtd);
> Feel free to unwrap this line

I will unwrap it to a single line.

>> +	size_t block_size = host->info.block_size;
>> +	loff_t block_start, block_off, lba;
>> +	size_t chunk, to_read;
>> +	int ret = 0;
> This initialization seems unnecessary

Yes, I will remove this unnecessary initialization.

> [...]
>> +static int qcom_scm_storage_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct qcom_scm_storage *host;
>> +	int ret;
>> +
>> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
>> +	if (!host)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, host);
>> +	host->dev = dev;
>> +
>> +	ret = devm_mutex_init(dev, &host->lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	host->buffer_size = SZ_256K;
> Should this just be = host->info->page_size?

The value of page_size is smaller than what we want for
buffering SCM transfers. The buffer is intended for batching
larger operations, so a larger fixed size is used here.

struct qcom_scm_storage_info:
  total_blocks = 8192
  block_size = 4096
  page_size = 256
  num_physical = 0
  manufacturer_id = 0
  serial_num = 1663215
  fw_version = 
  memory_type = NOR

> [...]
>
>> +	dev_info(dev, "scm storage 0x%llx registered with size %llu bytes\n",
>> +		 host->info.serial_num, host->mtd.size);
> dev_dbg()?

I will change to use dev_dbg.

> Konrad
>

Thank you for the review, I will incorporate these changes in v2.

Best regards,
Junhao Xie

Re: [PATCH 2/2] mtd: devices: Add Qualcomm SCM storage driver

[Konrad Dybcio]

On 12/19/25 6:12 PM, Junhao Xie wrote:
> On 2025/12/19 20:05, Konrad Dybcio wrote:
>> On 12/18/25 7:02 PM, Junhao Xie wrote:
>>> Add MTD driver for accessing storage devices managed by Qualcomm's
>>> TrustZone firmware. On some platforms, BIOS/firmware storage (typically
>>> SPI NOR flash) is not directly accessible from the non-secure world and
>>> all operations must go through SCM (Secure Channel Manager) calls.
>>>
>>> Signed-off-by: Junhao Xie <bigfoot@radxa.com>
>>> Tested-by: Xilin Wu <sophon@radxa.com>
>>> ---

[...]

>>> +	host->buffer_size = SZ_256K;
>> Should this just be = host->info->page_size?
> 
> The value of page_size is smaller than what we want for
> buffering SCM transfers. The buffer is intended for batching
> larger operations, so a larger fixed size is used here.
> 
> struct qcom_scm_storage_info:
>   total_blocks = 8192
>   block_size = 4096
>   page_size = 256
>   num_physical = 0
>   manufacturer_id = 0
>   serial_num = 1663215
>   fw_version = 
>   memory_type = NOR

I don't see how the big buffer is any useful TBF - you read into this
buffer before outputting to *buf and copy data from *buf into the
host_buffer instead of writing straight from the former. If anything,
that's an unnecessary copy.

Konrad

Re: [PATCH 2/2] mtd: devices: Add Qualcomm SCM storage driver

[Junhao Xie]

On 2025/12/22 17:43, Konrad Dybcio wrote:
> On 12/19/25 6:12 PM, Junhao Xie wrote:
>> On 2025/12/19 20:05, Konrad Dybcio wrote:
>>> On 12/18/25 7:02 PM, Junhao Xie wrote:
>>>> Add MTD driver for accessing storage devices managed by Qualcomm's
>>>> TrustZone firmware. On some platforms, BIOS/firmware storage (typically
>>>> SPI NOR flash) is not directly accessible from the non-secure world and
>>>> all operations must go through SCM (Secure Channel Manager) calls.
>>>>
>>>> Signed-off-by: Junhao Xie <bigfoot@radxa.com>
>>>> Tested-by: Xilin Wu <sophon@radxa.com>
>>>> ---
> [...]
>
>>>> +	host->buffer_size = SZ_256K;
>>> Should this just be = host->info->page_size?
>> The value of page_size is smaller than what we want for
>> buffering SCM transfers. The buffer is intended for batching
>> larger operations, so a larger fixed size is used here.
>>
>> struct qcom_scm_storage_info:
>>   total_blocks = 8192
>>   block_size = 4096
>>   page_size = 256
>>   num_physical = 0
>>   manufacturer_id = 0
>>   serial_num = 1663215
>>   fw_version = 
>>   memory_type = NOR
> I don't see how the big buffer is any useful TBF - you read into this
> buffer before outputting to *buf and copy data from *buf into the
> host_buffer instead of writing straight from the former. If anything,
> that's an unnecessary copy.

The purpose of using a larger buffer here is to reduce the number of
SCM calls rather than to avoid memcpy.

Each QCOM_SCM_STORAGE_READ/WRITE involves an SMC transition with a
relatively high fixed overhead. Issuing calls at a 4 KiB granularity
would result in many SCM calls and poor throughput. The larger buffer
allows batching multiple contiguous blocks into a single SCM
transaction, significantly reducing the number of SMCs. In comparison,
the extra memcpy cost is negligible.

I agree that the current code always bouncing through the buffer is not
ideal. In v2, I will pass aligned accesses directly to
qcom_scm_storage_send_cmd() to avoid the extra copy, and only use the
bounce buffer for unaligned or partial accesses. I will also add a
comment explaining the rationale for the buffer size.

> Konrad
>

Thanks again for the review.

Best regards,
Junhao Xie