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

[Jiucheng Xu <jiucheng.xu@amlogic.com>]

On 2022/8/5 16:38, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
>
> 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.
Okay, I just only run checkpatch. I will use the other tools to check my 
patches.
>
>> +		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.
Okay, I will remove them.
>
>> +
>> +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.
>> + */
> (...)
Do you mean a new blank line need be added?
>
>> +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?
Yes, I will modify it.
>
>> +	.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.

Do you mean use of_device_id.data? Could your please give me an

example code in kernel source?

>
>> +	} 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?

Yes, I run "./scripts/checkpatch --strict *.patch", and no errors/warnings.

Any other options should be used to check strictly?

I think it could be removed.

>
>> +	},
>> +	{
>> +		.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.
Do you mean use a common compatible and different driver data?
>
>> +	},
>> +	{}
>> +};
>> +
>> +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.
Sorry, I couldn't know why the driver is non-hotpluggable. Could you 
tell the detail?
>> +};
>> +
>> +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.
Do you mean the header should be in driver dir, or the structures should 
be within .c file?
>
> Best regards,
> Krzysztof
>
-- 
Thanks,
Jiucheng