Re: [PATCH v4 1/4] perf/amlogic: Add support for Amlogic meson G12 SoC DDR PMU driver
From: Jiucheng Xu
Date: Fri Aug 05 2022 - 05:55:08 EST
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