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@xxxxxxxxx>
>> Tested-by: Xilin Wu <sophon@xxxxxxxxx>
>> ---
> [...]
>
>> +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