Hi Maddy/Anju,
Comments below :)
Anju T Sudhakar <a...@linux.vnet.ibm.com> writes:
> Code to create platform device for the IMC counters.
> Paltform devices are created based on the IMC compatibility
> string.
>
> New Config flag "CONFIG_HV_PERF_IMC_CTRS" add to contain the
> IMC counter changes.
I don't think we need a separate config, it can just use
CONFIG_PPC_POWERNV.
I don't think we'll ever want to turn it off for powernv, unless we're
trying to build a small kernel, in which case we'll turn of PERF
entirely.
> diff --git a/arch/powerpc/platforms/powernv/opal-imc.c
> b/arch/powerpc/platforms/powernv/opal-imc.c
> new file mode 100644
> index 000000000000..5b1045c81af4
> --- /dev/null
> +++ b/arch/powerpc/platforms/powernv/opal-imc.c
> @@ -0,0 +1,73 @@
> +/*
> + * OPAL IMC interface detection driver
> + * Supported on POWERNV platform
> + *
> + * Copyright (C) 2017 Madhavan Srinivasan, IBM Corporation.
> + * (C) 2017 Anju T Sudhakar, IBM Corporation.
> + * (C) 2017 Hemant K Shaw, IBM Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
We usually don't include that part in every file.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
> +#include <linux/fs.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/poll.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/crash_dump.h>
> +#include <asm/opal.h>
> +#include <asm/io.h>
> +#include <asm/uaccess.h>
> +#include <asm/cputable.h>
> +#include <asm/imc-pmu.h>
> +
> +static int opal_imc_counters_probe(struct platform_device *pdev)
> +{
> + struct device_node *imc_dev = NULL;
> +
> + if (!pdev || !pdev->dev.of_node)
> + return -ENODEV;
We don't need that level of paranoia :)
> + /*
> + * Check whether this is kdump kernel. If yes, just return.
> + */
> + if (is_kdump_kernel())
> + return -ENODEV;
Hmm, that's a bit unusual. Is there any particular reason to do that for
this driver?
> + imc_dev = pdev->dev.of_node;
> + if (!imc_dev)
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +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,
> +};
> +
This can't be built as a module, so it should not be using MODULE macros.
> +MODULE_DEVICE_TABLE(of, opal_imc_match);
Drop that.
> +module_platform_driver(opal_imc_driver);
Use builtin_platform_driver().
> +MODULE_DESCRIPTION("PowerNV OPAL IMC driver");
> +MODULE_LICENSE("GPL");
Drop those.
> diff --git a/arch/powerpc/platforms/powernv/opal.c
> b/arch/powerpc/platforms/powernv/opal.c
> index 59684b4af4d1..fbdca259ea76 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -705,6 +707,17 @@ static void opal_pdev_init(const char *compatible)
> of_platform_device_create(np, NULL, NULL);
> }
>
> +#ifdef CONFIG_HV_PERF_IMC_CTRS
> +static void __init 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);
> +}
> +#endif
That doesn't need the #ifdef.
> static int kopald(void *unused)
> {
> unsigned long timeout = msecs_to_jiffies(opal_heartbeat) + 1;
> @@ -778,6 +791,11 @@ static int __init opal_init(void)
> /* Setup a heatbeat thread if requested by OPAL */
> opal_init_heartbeat();
>
> +#ifdef CONFIG_HV_PERF_IMC_CTRS
> + /* Detect IMC pmu counters support and create PMUs */
> + opal_imc_init_dev();
> +#endif
> +
Neither here.
> /* Create leds platform devices */
> leds = of_find_node_by_path("/ibm,opal/leds");
> if (leds) {
> --
> 2.11.0
cheers
