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:

quoted

+ * 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.

quoted

+
+		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.

quoted

+
+	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.

quoted

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

quoted

+
  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