Re: [PATCH 3/7] soc: qcom: add QCOM PBS driver

From: Krzysztof Kozlowski
Date: Sat Jun 24 2023 - 05:55:22 EST

Next message: Krzysztof Kozlowski: "Re: [PATCH 3/7] soc: qcom: add QCOM PBS driver"
Previous message: Sui Jingfeng: "Re: [20/39] drm: renesas: shmobile: Replace .dev_private with container_of()"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH 3/7] soc: qcom: add QCOM PBS driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 21/06/2023 20:59, Anjelique Melendez wrote:
> Add the Qualcomm PBS (Programmable Boot Sequencer) driver. The QCOM PBS
> driver supports configuring software PBS trigger events through PBS RAM
> on Qualcomm Technologies, Inc (QTI) PMICs.
>
> Signed-off-by: Anjelique Melendez <quic_amelende@xxxxxxxxxxx>
> ---
> drivers/soc/qcom/Kconfig | 9 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom-pbs.c | 343 ++++++++++++++++++++++++++++++
> include/linux/soc/qcom/qcom-pbs.h | 36 ++++
> 4 files changed, 389 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom-pbs.c
> create mode 100644 include/linux/soc/qcom/qcom-pbs.h
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a491718f8064..226b668f4690 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -260,6 +260,15 @@ config QCOM_APR
> used by audio driver to configure QDSP6
> ASM, ADM and AFE modules.
>
> +config QCOM_PBS
> + tristate "PBS trigger support for Qualcomm PMIC"
> + depends on SPMI
> + help
> + This driver supports configuring software programmable boot sequencer (PBS)
> + trigger event through PBS RAM on Qualcomm Technologies, Inc. PMICs.
> + This module provides the APIs to the client drivers that wants to send the
> + PBS trigger event to the PBS RAM.
> +
> config QCOM_ICC_BWMON
> tristate "QCOM Interconnect Bandwidth Monitor driver"
> depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 0f43a88b4894..4e154af3877a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -31,5 +31,6 @@ obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> +obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += ice.o
> diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
> new file mode 100644
> index 000000000000..4a2bb7ff8031
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom-pbs.c
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt) "PBS: %s: " fmt, __func__
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/spmi.h>
> +#include <linux/soc/qcom/qcom-pbs.h>
> +
> +#define PBS_CLIENT_TRIG_CTL 0x42
> +#define PBS_CLIENT_SW_TRIG_BIT BIT(7)
> +#define PBS_CLIENT_SCRATCH1 0x50
> +#define PBS_CLIENT_SCRATCH2 0x51
> +
> +static LIST_HEAD(pbs_dev_list);
> +static DEFINE_MUTEX(pbs_list_lock);

No file-scope variables. Drop both. You don't even need it.

> +
> +struct pbs_dev {
> + struct device *dev;
> + struct device_node *dev_node;
> + struct regmap *regmap;
> + struct mutex lock;
> + struct list_head link;
> +
> + u32 base;
> +};
> +
> +static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
> +{
> + int ret;
> +
> + address += pbs->base;
> + ret = regmap_bulk_read(pbs->regmap, address, val, 1);
> + if (ret)
> + pr_err("Failed to read address=%#x sid=%#x ret=%d\n",

dev_err

> + address, to_spmi_device(pbs->dev->parent)->usid, ret);
> +
> + return ret;
> +}
> +
> +static int qcom_pbs_write(struct pbs_dev *pbs, u16 address, u8 val)
> +{
> + int ret;
> +
> + address += pbs->base;
> + ret = regmap_bulk_write(pbs->regmap, address, &val, 1);
> + if (ret < 0)
> + pr_err("Failed to write address=%#x sid=%#x ret=%d\n",
> + address, to_spmi_device(pbs->dev->parent)->usid, ret);
> + else
> + pr_debug("Wrote %#x to addr %#x\n", val, address);

No, there is regmap debug for this. Drop such debug statements from the
driver.

Actually the error print is also wrong, IMO.

> +
> + return ret;
> +}
> +
> +static int qcom_pbs_masked_write(struct pbs_dev *pbs, u16 address, u8 mask, u8 val)
> +{
> + int ret;
> +
> + address += pbs->base;
> + ret = regmap_update_bits(pbs->regmap, address, mask, val);
> + if (ret < 0)
> + pr_err("Failed to write address=%#x ret=%d\n", address, ret);
> + else
> + pr_debug("Wrote %#x to addr %#x\n", val, address);

Drop

> +
> + return ret;
> +}
> +
> +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> +{
> + u16 retries = 2000, delay = 1000;
> + int ret;
> + u8 val;
> +
> + while (retries--) {
> + ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> + if (ret < 0)
> + return ret;
> +
> + if (val == 0xFF) {
> + /* PBS error - clear SCRATCH2 register */
> + ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> + if (ret < 0)
> + return ret;
> +
> + pr_err("NACK from PBS for bit %u\n", bit_pos);
> + return -EINVAL;
> + }
> +
> + if (val & BIT(bit_pos)) {
> + pr_debug("PBS sequence for bit %u executed!\n", bit_pos);

dev_dbg

> + break;
> + }
> +
> + usleep_range(delay, delay + 100);
> + }
> +
> + if (!retries) {
> + pr_err("Timeout for PBS ACK/NACK for bit %u\n", bit_pos);

dev_err
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * qcom_pbs_trigger_single_event() - Trigger PBS sequence without using bitmap.
> + * @pbs: Pointer to PBS device
> + *
> + * This function is used to trigger the PBS that is hooked on the
> + * SW_TRIGGER directly in PBS client.
> + *
> + * Return: 0 on success, < 0 on failure
> + */
> +int qcom_pbs_trigger_single_event(struct pbs_dev *pbs)
> +{
> + int ret = 0;
> +
> + if (IS_ERR_OR_NULL(pbs))
> + return -EINVAL;
> +
> + mutex_lock(&pbs->lock);
> + ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_TRIG_CTL, PBS_CLIENT_SW_TRIG_BIT,
> + PBS_CLIENT_SW_TRIG_BIT);
> + if (ret < 0)
> + pr_err("Failed to write register %x ret=%d\n", PBS_CLIENT_TRIG_CTL, ret);

dev_* everywhere

> + mutex_unlock(&pbs->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(qcom_pbs_trigger_single_event);
> +

...

> +/**
> + * get_pbs_client_device() - Get the PBS device used by client
> + * @dev: Client device
> + *
> + * This function is used to get the PBS device that is being
> + * used by the client.
> + *
> + * Returns: pbs_dev on success, ERR_PTR on failure
> + */
> +struct pbs_dev *get_pbs_client_device(struct device *dev)
> +{
> + struct device_node *pbs_dev_node;
> + struct pbs_dev *pbs;
> +
> + pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs-client", 0);
> + if (!pbs_dev_node) {
> + pr_err("Missing qcom,pbs-client property\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + mutex_lock(&pbs_list_lock);
> + list_for_each_entry(pbs, &pbs_dev_list, link) {

It does not make sense. You have the reference to the device, so you
have the pbs (via container_of). Don't add some
global-list-lookup-functions.

Look for example at Abel Vesa's ICE patchset.

> + if (pbs_dev_node == pbs->dev_node) {
> + of_node_put(pbs_dev_node);
> + mutex_unlock(&pbs_list_lock);
> + return pbs;
> + }
> + }
> + mutex_unlock(&pbs_list_lock);

Where is device_link handling?

> +
> + pr_debug("Unable to find PBS dev_node\n");
> + of_node_put(pbs_dev_node);
> + return ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL(get_pbs_client_device);
> +
> +static int qcom_pbs_probe(struct platform_device *pdev)
> +{
> + struct pbs_dev *pbs;
> + u32 val;
> + int ret;
> +
> + pbs = devm_kzalloc(&pdev->dev, sizeof(*pbs), GFP_KERNEL);
> + if (!pbs)
> + return -ENOMEM;
> +
> + pbs->dev = &pdev->dev;
> + pbs->dev_node = pdev->dev.of_node;

Why do you need to store it?

> + pbs->regmap = dev_get_regmap(pbs->dev->parent, NULL);
> + if (!pbs->regmap) {
> + dev_err(pbs->dev, "Couldn't get parent's regmap\n");
> + return -EINVAL;
> + }
> +
> + ret = device_property_read_u32(pbs->dev, "reg", &val);
> + if (ret < 0) {
> + dev_err(pbs->dev, "Couldn't find reg, ret = %d\n", ret);
> + return ret;
> + }
> +
> + pbs->base = val;
> + mutex_init(&pbs->lock);
> +
> + platform_set_drvdata(pdev, pbs);
> +
> + mutex_lock(&pbs_list_lock);
> + list_add(&pbs->link, &pbs_dev_list);
> + mutex_unlock(&pbs_list_lock);
> +
> + return 0;
> +}
> +
> +static int qcom_pbs_remove(struct platform_device *pdev)
> +{
> + struct pbs_dev *pbs = platform_get_drvdata(pdev);
> +
> + mutex_lock(&pbs_list_lock);
> + list_del(&pbs->link);
> + mutex_unlock(&pbs_list_lock);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_pbs_match_table[] = {
> + { .compatible = "qcom,pbs" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_pbs_match_table);
> +
> +static struct platform_driver qcom_pbs_driver = {
> + .driver = {
> + .name = "qcom-pbs",
> + .of_match_table = qcom_pbs_match_table,
> + },
> + .probe = qcom_pbs_probe,
> + .remove = qcom_pbs_remove,
> +};
> +module_platform_driver(qcom_pbs_driver)
> +
> +MODULE_DESCRIPTION("QCOM PBS DRIVER");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom-pbs");

Drop alias. Not needed. If you need it, you have missing ID table.

> diff --git a/include/linux/soc/qcom/qcom-pbs.h b/include/linux/soc/qcom/qcom-pbs.h
> new file mode 100644
> index 000000000000..4b065951686a
> --- /dev/null

Best regards,
Krzysztof

Next message: Krzysztof Kozlowski: "Re: [PATCH 3/7] soc: qcom: add QCOM PBS driver"
Previous message: Sui Jingfeng: "Re: [20/39] drm: renesas: shmobile: Replace .dev_private with container_of()"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH 3/7] soc: qcom: add QCOM PBS driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]