Re: [PATCH v4 1/4] perf/amlogic: Add support for Amlogic meson G12 SoC DDR PMU driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 05/08/2022 09:14, Jiucheng Xu wrote:
> This patch adds support Amlogic meson G12 series SoC
> DDR bandwidth PMU driver framework and interfaces.
> 
> The PMU not only can monitor the total DDR bandwidth,
> but also the bandwidth which is from individual IP module.
> 
> Example usage:
> 
>  $ perf stat -a -e aml_ddr_bw/total_rw_bytes/ -I 1000 sleep 10
> 
> - or -

Thank you for your patch. There is something to discuss/improve.

> +
> +static void fmt_attr_fill(struct attribute **fmt_attr)
> +{
> +	int i;
> +
> +	for (i = 0; fmt_attr[i]; i++)
> +		ddr_perf_format_attrs[i] = fmt_attr[i];
> +
> +	ddr_perf_format_attrs[i] = NULL;
> +}
> +
> +static int ddr_pmu_parse_dt(struct platform_device *pdev, struct dmc_hw_info *info)
> +{
> +	/*struct pinctrl *p;*/

No dead code in kernel.

> +	void __iomem *base;
> +	int i, ret = -EINVAL;
> +
> +	for (i = 0; i < info->dmc_nr; i++) {

Your binding does not allow to have more than 1. Remove dead code.

> +		/* resource 0 for ddr register base */
> +		base = devm_platform_ioremap_resource(pdev, i);
> +		if (IS_ERR(base)) {
> +			dev_err(&pdev->dev, "couldn't ioremap ddr reg %d\n", i);

Error message is not needed.

> +			return PTR_ERR(base);
> +		}
> +		info->ddr_reg[i] = base;
> +	}
> +
> +	/* resource i for pll register base */
> +	base = devm_platform_ioremap_resource(pdev, i);
> +	if (IS_ERR(base)) {
> +		dev_err(&pdev->dev, "couldn't ioremap for pll reg\n");

Error message is not needed.

> +		return PTR_ERR(base);
> +	}
> +	info->pll_reg = base;
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "couldn't get irq\n");

Error message is not needed.

> +		return ret;
> +	}
> +	info->irq_num = ret;
> +
> +	ret = devm_request_irq(&pdev->dev, info->irq_num, dmc_irq_handler,
> +			       IRQF_NOBALANCING, dev_name(&pdev->dev),
> +			       (void *)info);
> +	if (ret < 0)
> +		dev_err(&pdev->dev, "ddr request irq failed\n");
> +
> +	return ret;
> +}
> +
> +int meson_ddr_pmu_create(struct platform_device *pdev, struct ddr_pmu *pmu)
> +{
> +	int ret;
> +	char *name;
> +	struct pmu tmp_pmu = {
> +		.module		= THIS_MODULE,
> +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
> +		.task_ctx_nr	= perf_invalid_context,
> +		.attr_groups	= attr_groups,
> +		.event_init	= meson_ddr_perf_event_init,
> +		.add		= meson_ddr_perf_event_add,
> +		.del		= meson_ddr_perf_event_del,
> +		.start		= meson_ddr_perf_event_start,
> +		.stop		= meson_ddr_perf_event_stop,
> +		.read		= meson_ddr_perf_event_update,
> +	};
> +
> +	pmu->pmu = tmp_pmu;
> +
> +	ret = ddr_pmu_parse_dt(pdev, &pmu->info);
> +	if (ret < 0)
> +		return ret;
> +
> +	fmt_attr_fill(pmu->info.fmt_attr);
> +
> +	pmu->cpu = raw_smp_processor_id();

smp_processor_id()
(or why do you need to use raw?)


> +
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, DDR_PERF_DEV_NAME);
> +	if (!name) {
> +		dev_err(&pdev->dev, "couldn't allocat name\n");

No error messages for allocation. Run basic tools on your code -
coccinelle, checkaptch, smatch and sparse.

> +		return -ENOMEM;
> +	}
> +
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, name, NULL,
> +				      ddr_perf_offline_cpu);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> +		return ret;
> +	}
> +	pmu->cpuhp_state = ret;
> +
> +	/* Register the pmu instance for cpu hotplug */
> +	ret = cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
> +		goto cpuhp_instance_err;
> +	}
> +
> +	fill_event_attr(pmu);
> +
> +	ret = perf_pmu_register(&pmu->pmu, name, -1);
> +	if (ret) {
> +		dev_err(&pdev->dev, "perf pmu register failed\n");
> +		goto pmu_register_err;
> +	}
> +
> +	pmu->name = name;
> +	pmu->dev = &pdev->dev;
> +	pmu->pmu_enabled = false;
> +
> +	platform_set_drvdata(pdev, pmu);
> +
> +	return 0;
> +
> +pmu_register_err:
> +	cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +cpuhp_instance_err:
> +	cpuhp_remove_state(pmu->cpuhp_state);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(meson_ddr_pmu_create);

Why is this exported?

Also misses kerneldoc.

> +
> +int meson_ddr_pmu_remove(struct platform_device *pdev)
> +{
> +	struct ddr_pmu *pmu = platform_get_drvdata(pdev);
> +
> +	perf_pmu_unregister(&pmu->pmu);
> +	cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +	cpuhp_remove_state(pmu->cpuhp_state);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(meson_ddr_pmu_remove);

Why is this exported?

Also misses kerneldoc.

> diff --git a/drivers/perf/amlogic/meson_g12_ddr_pmu.c b/drivers/perf/amlogic/meson_g12_ddr_pmu.c
> new file mode 100644
> index 000000000000..ba1908733bee
> --- /dev/null
> +++ b/drivers/perf/amlogic/meson_g12_ddr_pmu.c
> @@ -0,0 +1,388 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Amlogic, Inc. All rights reserved.
> + */

(...)

> +static int dmc_g12_irq_handler(struct dmc_hw_info *info,
> +			       struct dmc_counter *counter)
> +{
> +	unsigned int val;
> +	int ret = -EINVAL;
> +
> +	val = readl(info->ddr_reg[0] + DMC_MON_G12_CTRL0);
> +	if (val & DMC_QOS_IRQ) {
> +		dmc_g12_get_counters(info, counter);
> +		/* clear irq flags */
> +		writel(val, info->ddr_reg[0] + DMC_MON_G12_CTRL0);
> +		ret = 0;
> +	}
> +	return ret;
> +}
> +
> +static struct dmc_pmu_hw_ops g12_ops = {

static const?

> +	.enable		= dmc_g12_counter_enable,
> +	.disable	= dmc_g12_counter_disable,
> +	.irq_handler	= dmc_g12_irq_handler,
> +	.get_counters	= dmc_g12_get_counters,
> +	.config_axi_id	= dmc_g12_config_axi_id,
> +};
> +
> +static int __init g12_ddr_pmu_probe(struct platform_device *pdev)
> +{
> +	struct ddr_pmu *pmu;
> +
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "amlogic,g12a-ddr-pmu")) {
> +		format_attr_nna.attr.mode = 0;
> +		format_attr_gdc.attr.mode = 0;
> +		format_attr_arm1.attr.mode = 0;
> +		format_attr_mipi_isp.attr.mode = 0;

No. That's not correct patter. You must use variant specific driver data.

> +	} else if (of_device_is_compatible(pdev->dev.of_node,
> +					   "amlogic,sm1-ddr-pmu")) {
> +		format_attr_gdc.attr.mode = 0;
> +		format_attr_arm1.attr.mode = 0;
> +		format_attr_mipi_isp.attr.mode = 0;

No. That's not correct patter. You must use variant specific driver data.

> +	}
> +
> +	pmu = devm_kzalloc(&pdev->dev, sizeof(struct ddr_pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	/*
> +	 * G12 series Soc have single dmc controller and
> +	 * 4x ddr bandwidth monitor channels
> +	 */
> +	pmu->info.dmc_nr = 1;
> +	pmu->info.chann_nr = 4;
> +	pmu->info.ops = &g12_ops;
> +	pmu->info.fmt_attr = g12_pmu_format_attrs;
> +
> +	return meson_ddr_pmu_create(pdev, pmu);
> +}
> +
> +static int __exit g12_ddr_pmu_remove(struct platform_device *pdev)
> +{
> +	meson_ddr_pmu_remove(pdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id meson_ddr_pmu_dt_match[] = {
> +	{
> +		.compatible = "amlogic,g12-ddr-pmu",

Undocumented compatible. Did you run checkpatch and fix all the errors?

> +	},
> +	{
> +		.compatible = "amlogic,g12a-ddr-pmu",
> +	},
> +	{
> +		.compatible = "amlogic,g12b-ddr-pmu",
> +	},
> +	{
> +		.compatible = "amlogic,sm1-ddr-pmu",

Why four different entries for the same devices without driver data?
This is confusing.

> +	},
> +	{}
> +};
> +
> +static struct platform_driver g12_ddr_pmu_driver = {
> +	.driver = {
> +		.name = "amlogic,ddr-pmu",
> +		.of_match_table = meson_ddr_pmu_dt_match,
> +	},
> +	.remove = __exit_p(g12_ddr_pmu_remove),

You made the driver non-hotpluggable - why?
In the same time it is still unbindable, whis is a bit confusing. If you
can unbind it, you should be able to hot-unplug it.

> +};
> +
> +module_platform_driver_probe(g12_ddr_pmu_driver, g12_ddr_pmu_probe);
> +MODULE_AUTHOR("Jiucheng Xu");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Amlogic G12 series SoC DDR PMU");
> diff --git a/include/soc/amlogic/meson_ddr_pmu.h b/include/soc/amlogic/meson_ddr_pmu.h
> new file mode 100644
> index 000000000000..882efe3c2f58
> --- /dev/null
> +++ b/include/soc/amlogic/meson_ddr_pmu.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2022 Amlogic, Inc. All rights reserved.
> + */
> +
> +#ifndef __MESON_DDR_PMU_H__
> +#define __MESON_DDR_PMU_H__
> +
> +#define MAX_CHANNEL_NUM		8
> +
> +enum {
> +	ALL_CHAN_COUNTER_ID,
> +	CHAN1_COUNTER_ID,
> +	CHAN2_COUNTER_ID,
> +	CHAN3_COUNTER_ID,
> +	CHAN4_COUNTER_ID,
> +	CHAN5_COUNTER_ID,
> +	CHAN6_COUNTER_ID,
> +	CHAN7_COUNTER_ID,
> +	CHAN8_COUNTER_ID,
> +	COUNTER_MAX_ID,
> +};
> +
> +struct dmc_hw_info;
> +
> +struct dmc_counter {
> +	u64 all_cnt;	/* The count of all requests come in/out ddr controller */
> +	union {
> +		u64 all_req;
> +		struct {
> +			u64 all_idle_cnt;
> +			u64 all_16bit_cnt;
> +		};
> +	};
> +	u64 channel_cnt[MAX_CHANNEL_NUM]; /* To save a DMC bandwidth-monitor channel counter */
> +};
> +
> +struct dmc_pmu_hw_ops {
> +	void (*enable)(struct dmc_hw_info *info);
> +	void (*disable)(struct dmc_hw_info *info);
> +	/* Bind an axi line to a bandwidth-monitor channel */
> +	void (*config_axi_id)(struct dmc_hw_info *info, int axi_id, int chann);
> +	int (*irq_handler)(struct dmc_hw_info *info,
> +			   struct dmc_counter *counter);
> +	void (*get_counters)(struct dmc_hw_info *info,
> +			     struct dmc_counter *counter);
> +};
> +
> +struct dmc_hw_info {
> +	struct dmc_pmu_hw_ops *ops;
> +	void __iomem *ddr_reg[4];
> +	unsigned long timer_value;	/* Timer value in TIMER register */
> +	void __iomem *pll_reg;
> +	int irq_num;			/* irq vector number */
> +	int dmc_nr;			/* The number of dmc controller */
> +	int chann_nr;			/* The number of dmc bandwidth monitor channels */
> +	int id;				/* The number of supported channels */
> +	struct attribute **fmt_attr;
> +};
> +
> +struct ddr_pmu {
> +	struct pmu pmu;
> +	struct dmc_hw_info info;
> +	struct dmc_counter counters;	/* save counters from hw */
> +	bool pmu_enabled;
> +	struct device *dev;
> +	char *name;
> +	struct hlist_node node;
> +	enum cpuhp_state cpuhp_state;
> +	int cpu;			/* for cpu hotplug */
> +};

Linux-wide headers should not include your private data structures.
Entier header looks unused - should be made private.

Best regards,
Krzysztof