Message-ID: <20230416142055.GA2798@thinkpad>
Date:   Sun, 16 Apr 2023 19:50:55 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] soc: qcom: Introduce RPM master stats driver

On Fri, Apr 14, 2023 at 01:37:18PM +0200, Konrad Dybcio wrote:
> Introduce a driver to query and expose detailed, per-subsystem (as opposed
> to the existing qcom_stats driver which exposes SoC-wide data) about low
> power mode states of a given RPM master. That includes the APSS (ARM),
> MPSS (modem) and other remote cores, depending on the platform
> configuration.
> 
> This is a vastly cleaned up and restructured version of a similar
> driver found in msm-5.4.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
> ---
>  drivers/soc/qcom/Kconfig            |  11 +++
>  drivers/soc/qcom/Makefile           |   1 +
>  drivers/soc/qcom/rpm_master_stats.c | 160 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 172 insertions(+)
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a491718f8064..e597799e8121 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -135,6 +135,17 @@ config QCOM_RMTFS_MEM
>  
>  	  Say y here if you intend to boot the modem remoteproc.
>  
> +config QCOM_RPM_MASTER_STATS
> +	tristate "Qualcomm RPM Master stats"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  The RPM Master sleep stats driver provides detailed per-subsystem
> +	  sleep/wake data, read from the RPM message RAM. It can be used to
> +	  assess whether all the low-power modes available are entered as
> +	  expected or to check which part of the SoC prevents it from sleeping.
> +
> +	  Say y here if you intend to debug or monitor platform sleep.
> +
>  config QCOM_RPMH
>  	tristate "Qualcomm RPM-Hardened (RPMH) Communication"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 0f43a88b4894..7349371fdea1 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
>  qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
>  obj-$(CONFIG_QCOM_RAMP_CTRL)	+= ramp_controller.o
>  obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
> +obj-$(CONFIG_QCOM_RPM_MASTER_STATS)	+= rpm_master_stats.o
>  obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
>  qcom_rpmh-y			+= rpmh-rsc.o
>  qcom_rpmh-y			+= rpmh.o
> diff --git a/drivers/soc/qcom/rpm_master_stats.c b/drivers/soc/qcom/rpm_master_stats.c
> new file mode 100644
> index 000000000000..73080c92bf89
> --- /dev/null
> +++ b/drivers/soc/qcom/rpm_master_stats.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023, Linaro Limited
> + *
> + * This driver supports what is known as "Master Stats v2", which seems to be
> + * the only version which has ever shipped, all the way from 2013 to 2023.

It'd better to mention "Qualcomm downstream" in the somewhere above comment to
make it clear for users.

> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +struct master_stats_data {
> +	void __iomem *base;
> +	const char *label;
> +};
> +
> +struct rpm_master_stats {
> +	uint32_t active_cores;
> +	uint32_t num_shutdowns;
> +	uint64_t shutdown_req;
> +	uint64_t wakeup_idx;
> +	uint64_t bringup_req;
> +	uint64_t bringup_ack;
> +	uint32_t wakeup_reason; /* 0 = "rude wakeup", 1 = scheduled wakeup */
> +	uint32_t last_sleep_trans_dur;
> +	uint32_t last_wake_trans_dur;
> +
> +	/* Per-subsystem (*not necessarily* SoC-wide) XO shutdown stats */
> +	uint32_t xo_count;
> +	uint64_t xo_last_enter;
> +	uint64_t last_exit;
> +	uint64_t xo_total_dur;

Why can't you use u64, u32?

Also, sort these members as below:

u64
u32

> +};
> +
> +static int master_stats_show(struct seq_file *s, void *unused)
> +{
> +	struct master_stats_data *d = s->private;
> +	struct rpm_master_stats stat;
> +
> +	memcpy_fromio(&stat, d->base, sizeof(stat));
> +
> +	seq_printf(s, "%s:\n", d->label);
> +
> +	seq_printf(s, "\tLast shutdown @ %llu\n", stat.shutdown_req);
> +	seq_printf(s, "\tLast bringup req @ %llu\n", stat.bringup_req);
> +	seq_printf(s, "\tLast bringup ack @ %llu\n", stat.bringup_ack);
> +	seq_printf(s, "\tLast wakeup idx: %llu\n", stat.wakeup_idx);
> +	seq_printf(s, "\tLast XO shutdown enter @ %llu\n", stat.xo_last_enter);
> +	seq_printf(s, "\tLast XO shutdown exit @ %llu\n", stat.last_exit);
> +	seq_printf(s, "\tXO total duration: %llu\n", stat.xo_total_dur);
> +	seq_printf(s, "\tLast sleep transition duration: %u\n", stat.last_sleep_trans_dur);
> +	seq_printf(s, "\tLast wake transition duration: %u\n", stat.last_wake_trans_dur);
> +	seq_printf(s, "\tXO shutdown count: %u\n", stat.xo_count);
> +	seq_printf(s, "\tWakeup reason: 0x%x\n", stat.wakeup_reason);
> +	seq_printf(s, "\tShutdown count: %u\n", stat.num_shutdowns);
> +	seq_printf(s, "\tActive cores bitmask: 0x%x\n", stat.active_cores);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(master_stats);
> +
> +static int master_stats_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *msgram_np;
> +	struct master_stats_data *d;

"d" is not a good choice for a local variable (might be OK above but definitely
not here)

> +	struct dentry *dent, *root;
> +	struct resource res;
> +	int count, i, ret;
> +
> +	count = of_property_count_strings(dev->of_node, "qcom,master-names");
> +	if (count < 0)
> +		return count;
> +
> +	d = devm_kzalloc(dev, count * sizeof(*d), GFP_KERNEL);
> +	if (!d)
> +		return -ENOMEM;
> +
> +	root = debugfs_create_dir("rpm_master_stats", NULL);

It's better to use "qcom_rpm_master_stats".

> +	platform_set_drvdata(pdev, root);
> +
> +	for (i = 0; i < count; i++) {
> +		msgram_np = of_parse_phandle(dev->of_node, "qcom,rpm-msg-ram", i);
> +		if (!msgram_np) {
> +			debugfs_remove_recursive(root);
> +			return dev_err_probe(dev, -EINVAL,

-ENODEV

> +					     "Couldn't parse MSG RAM phandle idx %d", i);
> +		}
> +
> +		/*
> +		 * Purposefully skip devm_platform helpers as we're using a
> +		 * shared resource.
> +		 */
> +		ret = of_address_to_resource(msgram_np, 0, &res);

Missing of_node_put() here due to of_parse_phandle().

> +		if (ret < 0) {
> +			debugfs_remove_recursive(root);
> +			return ret;
> +		}
> +
> +		d[i].base = devm_ioremap(dev, res.start, resource_size(&res));
> +		if (IS_ERR(d[i].base)) {
> +			debugfs_remove_recursive(root);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Could not map the MSG RAM slice idx %d!\n", i);
> +		}
> +
> +		ret = of_property_read_string_index(dev->of_node, "qcom,master-names", i,
> +						    &d[i].label);
> +		if (ret < 0) {
> +			debugfs_remove_recursive(root);
> +			return dev_err_probe(dev, ret,
> +					     "Could not read name idx %d!\n", i);
> +		}
> +
> +		/*
> +		 * Generally it's not advised to fail on debugfs errors, but this
> +		 * driver's only job is exposing data therein.
> +		 */
> +		dent = debugfs_create_file(d[i].label, 0444, root,
> +					   &d[i], &master_stats_fops);
> +		if (!dent) {

Don't check for NULL, instead use IS_ERR() if you really care about error
checking here.

> +			debugfs_remove_recursive(root);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	device_set_pm_not_required(dev);
> +
> +	return 0;
> +}
> +
> +static void master_stats_remove(struct platform_device *pdev)
> +{
> +	struct dentry *root = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(root);
> +}
> +
> +static const struct of_device_id rpm_master_table[] = {
> +	{ .compatible = "qcom,rpm-master-stats" },
> +	{ },
> +};
> +
> +static struct platform_driver master_stats_driver = {
> +	.probe = master_stats_probe,
> +	.remove_new = master_stats_remove,
> +	.driver = {
> +		.name = "rpm_master_stats",
> +		.of_match_table = rpm_master_table,
> +	},
> +};
> +module_platform_driver(master_stats_driver);
> +
> +MODULE_DESCRIPTION("RPM Master Statistics driver");

Qualcomm RPM Master Statistics driver

- Mani

> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.40.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists