Re: [PATCH 2/6] remoteproc: qcom: Add M0 BTSS secure PIL driver

Thanks, that was quick!

On 6/25/26 18:18, Philipp Zabel wrote:
> On Do, 2026-06-25 at 18:10 +0400, George Moussalem via B4 Relay wrote:
>> From: George Moussalem <[email protected]>
>>
>> Add support to bring up the M0 core of the bluetooth subsystem found in
>> the IPQ5018 SoC.
>>
>> The signed firmware loaded is authenticated by TrustZone. If successful,
>> the M0 core boots the firmware and the peripheral is taken out of reset
>> using a Secure Channel Manager call to TrustZone.
>>
>> Signed-off-by: George Moussalem <[email protected]>
>> ---
>>  drivers/remoteproc/Kconfig            |  12 ++
>>  drivers/remoteproc/Makefile           |   1 +
>>  drivers/remoteproc/qcom_m0_btss_pil.c | 261 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 274 insertions(+)
>>
> [...]
>> diff --git a/drivers/remoteproc/qcom_m0_btss_pil.c b/drivers/remoteproc/qcom_m0_btss_pil.c
>> new file mode 100644
>> index 000000000000..7168e270e4d4
>> --- /dev/null
>> +++ b/drivers/remoteproc/qcom_m0_btss_pil.c
>> @@ -0,0 +1,261 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2026 The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/elf.h>
>> +#include <linux/firmware/qcom/qcom_scm.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of_reserved_mem.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/soc/qcom/mdt_loader.h>
>> +
>> +#include "qcom_common.h"
>> +
>> +#define BTSS_PAS_ID	0xc
>> +
>> +struct m0_btss {
>> +	struct device *dev;
>> +	phys_addr_t mem_phys;
>> +	phys_addr_t mem_reloc;
>> +	void __iomem *mem_region;
>> +	size_t mem_size;
>> +	struct reset_control *btss_reset;
> 
> Why is this stored here? It doesn't seem to be used.

will remove it and use devm_reset_control_get_exclusive_deasserted as
suggested below.

> 
> [...]
>> +static int m0_btss_pil_probe(struct platform_device *pdev)
>> +{
>> +	// struct reset_control *btss_reset;
> 
> It looks like this shouldn't be commented out.
> 
>> +	struct device *dev = &pdev->dev;
>> +	const char *fw_name = NULL;
>> +	struct m0_btss *desc;
>> +	struct clk *lpo_clk;
>> +	struct rproc *rproc;
>> +	int ret;
>> +
>> +	ret = of_property_read_string(dev->of_node, "firmware-name",
>> +				      &fw_name);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	rproc = devm_rproc_alloc(dev, "m0btss", &m0_btss_ops,
>> +				 fw_name, sizeof(*desc));
>> +	if (!rproc) {
>> +		dev_err(dev, "failed to allocate rproc\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	desc = rproc->priv;
>> +	desc->dev = dev;
>> +
>> +	ret = m0_btss_alloc_memory_region(desc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	lpo_clk = devm_clk_get_enabled(dev, "btss_lpo_clk");
>> +	if (IS_ERR(lpo_clk))
>> +		return dev_err_probe(dev, PTR_ERR(lpo_clk),
>> +				     "Failed to get lpo clock\n");
>> +
>> +	desc->btss_reset = devm_reset_control_get(dev, "btss_reset");
> 
> Please use devm_reset_control_get_exclusive() directly.
> 
>> +	if (IS_ERR_OR_NULL(desc->btss_reset))
>> +		return dev_err_probe(dev, PTR_ERR(desc->btss_reset),
>> +				     "unable to acquire btss_reset\n");
>> +
>> +	ret = reset_control_deassert(desc->btss_reset);
>> +	if (ret)
>> +		return dev_err_probe(rproc->dev.parent, ret,
>> +				     "Failed to deassert reset\n");
> 
> Shouldn't this be asserted on remove? Otherwise after an unbind/bind
> cycle probe will enable the clock with reset already deasserted.
> That may or may not be problematic.
> 
> Consider using devm_reset_control_get_exclusive_deasserted().
> 
> 
> regards
> Philipp

Regards,
George