I have 5 HW counters and 5 relevant events. Do you mean I need reject the group?+ list_for_each_entry((sibling), &(event)->sibling_list,
+ sibling_list) {
+ if (sibling->pmu != event->pmu &&
+ !is_software_event(sibling))
+ return -EINVAL;
It looks like you don't have multiple sets of hardware counters; if so you'd also need to reject the group if it contains more than one event for this PMU.
+static int ddr_perf_offline_cpu(unsigned int cpu, struct hlist_node *node)
+{
+ struct ddr_pmu *pmu = hlist_entry_safe(node, struct ddr_pmu, node);
+ int target;
+
+ if (cpu != pmu->cpu)
+ return 0;
+
+ target = cpumask_any_but(cpu_online_mask, cpu);
+ if (target >= nr_cpu_ids)
+ return 0;
+
+ perf_pmu_migrate_context(&pmu->pmu, cpu, target);
+ pmu->cpu = target;
+
+ WARN_ON(irq_set_affinity_hint(pmu->info.irq_num, cpumask_of(pmu->cpu)));
This is wrong, it needs to be irq_set_affinity().
Okay, Could you please tell what is the major difference between the two API?
Okay, actually there will be multiple interrupts , but not in current G12 series.+ goto err2;
+ }
+
+ irq_name = of_get_property(node, "interrupt-names", NULL);
+ if (!irq_name)
+ irq_name = "ddr_pmu";
That's not how the "interrupt-names" property works. If you only have a single interrupt then there's not much need for it to be named in the DT at all. If you do want to use named interrupts then use platform_get_irq_byname(), and the name should probably have a bnit more functional meaning. Either way, please don't abuse the DT like this.
+
+ ret = request_irq(info->irq_num, dmc_irq_handler,
+ IRQF_SHARED, irq_name, (void *)info);
Who else is sharing the IRQ? If it's other instances of this PMU then that's still manageable under the normal paradigm, the driver just needs to coordinate affinity chanmges between all instances. If it's random other devices, then maybe it's time to reason about how system PMUs could use proper IRQ-safe locking and abandon the affinity stuff, since this seems to be coming up more and more.
The IRQ is private. I will change it.
I have not got your point yet. I have no idea how to use .is_visible to hide the irrelevant attribute. I need a little time to think it.+static void append_attr_list(struct attribute *attr)
+{
+ int i;
+
+ for (i = 0; g12_pmu_format_attrs[i] && i < 255; i++)
+ ;
Eww, what? :(
+
+ g12_pmu_format_attrs[i] = attr;
+
+ g12_pmu_format_attrs[i + 1] = NULL;
(that's pointless either way)
OK, I think I see what's going on here now. Dynamically patching the attribute arrays is pretty grim - it's far cleaner and more sustainable to statically define the whole array with all the possible attributes, then use .is_visible to hide the ones which aren't relevant to the current system.
+ struct device_node *node = pdev->dev.of_node;
+ const char *model;
+
+ if (of_property_read_string(node, "model", &model))
+ return -EINVAL;
No, use of_device_is_compatible(), and define the binding properly.
Why the "model" property couldn't be used? Do you mean use existing property rather than creating new one?
+ dev_info(&pdev->dev, "%s", model);
+
+ if (strcmp(model, "g12a") == 0) {
+ dev_info(&pdev->dev, "ddr pmu for g12a\n");
+ } else if (strcmp(model, "g12b") == 0) {
+ dev_info(&pdev->dev, "ddr pmu for g12b\n");
+
+ append_attr_list(&format_attr_nna.attr);
+ append_attr_list(&format_attr_gdc.attr);
+ append_attr_list(&format_attr_arm1.attr);
+ append_attr_list(&format_attr_mipi_isp.attr);
+ append_attr_list(&format_attr_sd_emmc_a.attr);
+ } else if (strcmp(model, "sm1") == 0) {
+ dev_info(&pdev->dev, "ddr pmu for sm1\n");
+
+ append_attr_list(&format_attr_nna.attr);
+ }
+#endif
+ return aml_ddr_pmu_create(pdev, &g12_ops, g12_pmu_format_attrs);
+}
+
+static int __exit g12_ddr_pmu_remove(struct platform_device *pdev)
+{
+ aml_ddr_pmu_remove(pdev);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id aml_ddr_pmu_dt_match[] = {
+ {
+ .compatible = "amlogic,g12-ddr-pmu",
+ },
+ {}
+};
+#endif
+
+static struct platform_driver g12_ddr_pmu_driver = {
+ .driver = {
+ .name = "amlogic,ddr-pmu",
+ .owner = THIS_MODULE,
The driver core sets this automatically.
+ #ifdef CONFIG_OF
+ .of_match_table = aml_ddr_pmu_dt_match,
+ #endif
+ },
+ .remove = g12_ddr_pmu_remove,
+};
+
+static int __init aml_g12_ddr_pmu_init(void)
+{
+ return platform_driver_probe(&g12_ddr_pmu_driver, g12_ddr_pmu_probe);
+}
+
+static void __exit aml_g12_ddr_pmu_exit(void)
+{
+ platform_driver_unregister(&g12_ddr_pmu_driver);
+}
+
+module_init(aml_g12_ddr_pmu_init);
+module_exit(aml_g12_ddr_pmu_exit);
Use module_platform_driver_probe() (if of course you really think the __init shenanigans are beneficial, otherwise just use regular module_platform_driver() for even less surprise).
Thanks,
Robin.
Thanks for your time, Robin! TBH I'm a little nervous since this is my first submitting to upstream. Your comments are great and helpful. I will update the driver.