[ EXTERNAL EMAIL ]Okay, I just only run checkpatch. I will use the other tools to check my patches.
On 05/08/2022 09:14, Jiucheng Xu wrote:
This patch adds support Amlogic meson G12 series SoCThank you for your patch. There is something to discuss/improve.
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 -
+No dead code in kernel.
+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;*/
+ void __iomem *base;Your binding does not allow to have more than 1. Remove dead code.
+ int i, ret = -EINVAL;
+
+ for (i = 0; i < info->dmc_nr; i++) {
+ /* resource 0 for ddr register base */Error message is not needed.
+ base = devm_platform_ioremap_resource(pdev, i);
+ if (IS_ERR(base)) {
+ dev_err(&pdev->dev, "couldn't ioremap ddr reg %d\n", i);
+ return PTR_ERR(base);Error message is not needed.
+ }
+ 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");
+ return PTR_ERR(base);Error message is not needed.
+ }
+ info->pll_reg = base;
+
+ ret = platform_get_irq(pdev, 0);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "couldn't get irq\n");
+ return ret;smp_processor_id()
+ }
+ 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();
(or why do you need to use raw?)
+No error messages for allocation. Run basic tools on your code -
+ name = devm_kasprintf(&pdev->dev, GFP_KERNEL, DDR_PERF_DEV_NAME);
+ if (!name) {
+ dev_err(&pdev->dev, "couldn't allocat name\n");
coccinelle, checkaptch, smatch and sparse.
Okay, I will remove them.
+ return -ENOMEM;Why is this exported?
+ }
+
+ 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);
Also misses kerneldoc.
Do you mean a new blank line need be added?
+Why is this exported?
+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);
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.
+ */
Yes, I will modify it.
+static int dmc_g12_irq_handler(struct dmc_hw_info *info,static const?
+ 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 = {
+ .enable = dmc_g12_counter_enable,No. That's not correct patter. You must use variant specific driver data.
+ .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;
+ } else if (of_device_is_compatible(pdev->dev.of_node,No. That's not correct patter. You must use variant specific driver data.
+ "amlogic,sm1-ddr-pmu")) {
+ format_attr_gdc.attr.mode = 0;
+ format_attr_arm1.attr.mode = 0;
+ format_attr_mipi_isp.attr.mode = 0;
+ }Undocumented compatible. Did you run checkpatch and fix all the errors?
+
+ 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",
Do you mean use a common compatible and different driver data?
+ },Why four different entries for the same devices without driver data?
+ {
+ .compatible = "amlogic,g12a-ddr-pmu",
+ },
+ {
+ .compatible = "amlogic,g12b-ddr-pmu",
+ },
+ {
+ .compatible = "amlogic,sm1-ddr-pmu",
This is confusing.
Sorry, I couldn't know why the driver is non-hotpluggable. Could you tell the detail?
+ },You made the driver non-hotpluggable - why?
+ {}
+};
+
+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),
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.
Do you mean the header should be in driver dir, or the structures should be within .c file?+};Linux-wide headers should not include your private data structures.
+
+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 */
+};
Entier header looks unused - should be made private.
--
Best regards,
Krzysztof