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

[Philipp Zabel <p.zabel@pengutronix.de>]

On Do, 2026-06-25 at 18:10 +0400, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@outlook.com>
> 
> 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 <george.moussalem@outlook.com>
> ---
>  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.

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