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
