On Tuesday 04 April 2017 06:28 AM, Daniel Axtens wrote:
Hi all,
I'm trying to get my head around these patches - at this point I'm just
doing a first pass, so I may have more substantive structural comments
later on. In the mean time - here are some minor C nits:
+ * Copyright (C) 2016 Madhavan Srinivasan, IBM Corporation.
+ * (C) 2016 Hemant K Shaw, IBM Corporation.
Should these be bumped to 2017?
Facepalm. my bad. Will fix it.
+
+ do {
+ pages = PAGE_SIZE * i;
+ pcni->vbase[i++] = (u64)phys_to_virt(pcni->pbase +
+ pages);
+ } while (i < (pcni->size / PAGE_SIZE));
+ }
I had to scroll back up to the top of this function to make sure I
understood what this loop does. Would it be better to write it as:
for (i = 0; i < (pcni->size / PAGE_SIZE); i++) {
pages = PAGE_SIZE * i;
pcni->vbase[i] = (u64)....
}
Idea is to map all of the nest counter area since event
offset could be anywhere within this region.
Will document that here.
And, just checking - this is expected to work on both 4 and 64kB pages?
Yes. thats the intended. That said, i need to fix the
IMC_NEST_MAX_PAGES value for 4K page size.
Reason being, there was a recent change in the
size of memory allocated for nest counters in the
HOMER region for the microcode.
+
+ return 0;
+err:
+ return -ENODEV;
You're not releasing any resources here - would it be better to just
replace the gotos with this return? I haven't checked to see if you
change the function later on to allocate memory - if so please ignore :)
We check in multiple places in the function and return on fail.
Thats why we made it as a generic return with goto.
+}
+
+static const struct of_device_id opal_imc_match[] = {
+ { .compatible = IMC_DTB_COMPAT },
+ {},
+};
+
+static struct platform_driver opal_imc_driver = {
+ .driver = {
+ .name = "opal-imc-counters",
+ .of_match_table = opal_imc_match,
+ },
+ .probe = opal_imc_counters_probe,
+};
+
+MODULE_DEVICE_TABLE(of, opal_imc_match);
+module_platform_driver(opal_imc_driver);
+MODULE_DESCRIPTION("PowerNV OPAL IMC driver");
+MODULE_LICENSE("GPL");
diff --git a/arch/powerpc/platforms/powernv/opal.c
b/arch/powerpc/platforms/powernv/opal.c
index e0f856bfbfe8..85ea1296f030 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -14,6 +14,7 @@
#include <linux/printk.h>
#include <linux/types.h>
#include <linux/of.h>
+#include <linux/of_address.h>
#include <linux/of_fdt.h>
#include <linux/of_platform.h>
#include <linux/interrupt.h>
@@ -30,6 +31,7 @@
#include <asm/opal.h>
#include <asm/firmware.h>
#include <asm/mce.h>
+#include <asm/imc-pmu.h>
#include "powernv.h"
@@ -631,6 +633,15 @@ static void opal_pdev_init(const char *compatible)
of_platform_device_create(np, NULL, NULL);
}
+static void opal_imc_init_dev(void)
+{
+ struct device_node *np;
+
+ np = of_find_compatible_node(NULL, NULL, IMC_DTB_COMPAT);
+ if (np)
+ of_platform_device_create(np, NULL, NULL);
+}
Should this function be tagged __init?
Yes. Thats right. Will make the changes.
Thanks for review
Maddy
+
static int kopald(void *unused)
{
unsigned long timeout = msecs_to_jiffies(opal_heartbeat) + 1;
@@ -704,6 +715,9 @@ static int __init opal_init(void)
/* Setup a heatbeat thread if requested by OPAL */
opal_init_heartbeat();
+ /* Detect IMC pmu counters support and create PMUs */
+ opal_imc_init_dev();
+
/* Create leds platform devices */
leds = of_find_node_by_path("/ibm,opal/leds");
if (leds) {
--
2.7.4
