Re: [PATCH v5] platform:x86: Add PMC Driver for Intel Core SoC

[Andy Shevchenko]

On Tue, May 24, 2016 at 5:25 PM, Rajneesh Bhardwaj
<rajneesh.bhardwaj@xxxxxxxxx> wrote:

Thanks for an update!

By the way, please keep me in the Cc list with my
andriy.shevchenko@xxxxxxxxxxxxxxx address.

Additionally to what I said in the previous mail for v4.

> This patch adds the Power Management Controller driver as a pci driver

pci -> PCI

Check entire file.

> for Intel Core SoC architecture.

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6096,6 +6096,15 @@ S:       Maintained
>  F:     arch/x86/include/asm/intel_telemetry.h
>  F:     drivers/platform/x86/intel_telemetry*
>
> +INTEL PMC CORE DRIVER
> +M:     Rajneesh Bhardwaj <rajneesh.bhardwaj@xxxxxxxxx>
> +M:     Vishwanath Somayaji <vishwanath.somayaji@xxxxxxxxx>
> +L:     platform-driver-x86@xxxxxxxxxxxxxxx
> +S:     Maintained
> +F:     arch/x86/include/asm/pmc_core.h

> +F:     drivers/platform/x86/intel_pmc_core.h
> +F:     drivers/platform/x86/intel_pmc_core.c

F:     drivers/platform/x86/intel_pmc_core*

?

> +++ b/arch/x86/include/asm/pmc_core.h

> +#ifndef _ASM_PMC_CORE_H
> +#define _ASM_PMC_CORE_H
> +
> +/* API to read slp_s0 residency */

I think in the comment you may use SLP_S0 like you did in the commit message.

> +int intel_pmc_slp_s0_counter_read(u32 *data);
> +

> +#endif
> +
> +/* _ASM_PMC_CORE_H */

Would be one line.

> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -846,6 +846,18 @@ config INTEL_IMR
>
>           If you are running on a Galileo/Quark say Y here.
>
> +config INTEL_PMC_CORE
> +       bool "Intel PMC Core driver"
> +       depends on X86 && PCI
> +       ---help---
> +         The Intel Platform Controller Hub for Intel Core SoCs provides access
> +         to Power Management Controller registers via a pci interface. This

PCI

> +         driver can utilize debugging capabilities and supported features as
> +         exposed by the Power Management Controller.
> +
> +         Supported features:
> +               - slp_s0_residency counter.

SLP_S0

> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -69,3 +69,4 @@ obj-$(CONFIG_INTEL_PUNIT_IPC)  += intel_punit_ipc.o
>  obj-$(CONFIG_INTEL_TELEMETRY)  += intel_telemetry_core.o \
>                                    intel_telemetry_pltdrv.o \
>                                    intel_telemetry_debugfs.o
> +obj-$(CONFIG_INTEL_PMC_CORE)    += intel_pmc_core.o

Alphabetical order?

> +++ b/drivers/platform/x86/intel_pmc_core.c

> +/**
> + * intel_pmc_slp_s0_counter_read() - Read slp_s0 residency.

SLP_S0

> + * @data: Out param that contains current slp_s0 count.

Ditto.

> + *

+ * Description:

> + * This API currently supports Intel Skylake SoC and Sunrise
> + * Point Platform Controller Hub. Future platform support
> + * should be added for platforms that support low power modes
> + * beyond Package C10 state.
> + *
> + * SLP_S0_RESIDENCY counter counts in 100 us granularity per
> + * step hence function populates the multiplied value in out
> + * parameter @data.
> + *
> + * Return: an error code or 0 on success.
> + */
> +int intel_pmc_slp_s0_counter_read(u32 *data)
> +{
> +       struct pmc_dev *pmcdev = &pmc;
> +       u32 value;
> +
> +       if (!pmcdev->has_slp_s0_res)

I would name this just initialized, so if you ever add something else
there will be no need to rename.

if (!pmcdev->initialized)

> +               return -EACCES;
> +
> +       value = pmc_core_reg_read(pmcdev, SPT_PMC_SLP_S0_RES_COUNTER_OFFSET);
> +       *data = pmc_core_adjust_slp_s0_step(value);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_pmc_slp_s0_counter_read);
> +

> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static int pmc_core_dev_state_show(struct seq_file *s, void *unused)
> +{
> +       struct pmc_dev *pmcdev = s->private;
> +       u32 counter_val;
> +
> +       counter_val = pmc_core_reg_read(pmcdev,
> +                                       SPT_PMC_SLP_S0_RES_COUNTER_OFFSET);
> +       seq_printf(s, "%u\n", pmc_core_adjust_slp_s0_step(counter_val));
> +
> +       return 0;
> +}
> +
> +static int pmc_core_dev_state_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, pmc_core_dev_state_show, inode->i_private);
> +}
> +
> +static const struct file_operations pmc_core_dev_state_ops = {
> +       .open           = pmc_core_dev_state_open,
> +       .read           = seq_read,
> +       .llseek         = seq_lseek,
> +       .release        = single_release,
> +};

I suppose DEFINE_SIMPLE_ATTRIBUTE might reduce amount of LOC.

> +static int pmc_core_dbgfs_register(struct pmc_dev *pmc)

Use pmcdev for pointer for the sake of consistency.

> +{
> +       struct dentry *dir, *file;

> +       int ret = 0;

Redundant.

> +
> +       dir = debugfs_create_dir("pmc_core", NULL);
> +       if (!dir)
> +               return -ENOMEM;
> +
> +       pmc->dbgfs_dir = dir;

I'm not sure you need an additional variable here.

> +       file = debugfs_create_file("slp_s0_residency_usec", S_IFREG | S_IRUGO,
> +                                  dir, pmc, &pmc_core_dev_state_ops);
> +
> +       if (!file) {
> +               pmc_core_dbgfs_unregister(pmc);
> +               ret = -ENODEV;
> +       }
> +       return ret;
> +}
> +#else
> +static inline int pmc_core_dbgfs_register(struct pmc_dev *pmc)
> +{
> +       return 0; /* nothing to register */

Useless comment.

> +}
> +
> +static inline void pmc_core_dbgfs_unregister(struct pmc_dev *pmc)
> +{
> +       /* nothing to deregister */

Ditto.

> +}
> +#endif /* CONFIG_DEBUG_FS */

> +static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id)
> +{
> +       struct device *ptr_dev = &dev->dev;
> +       struct pmc_dev *pmcdev = &pmc;
> +       const struct x86_cpu_id *cpu_id;
> +       int err;
> +
> +       cpu_id = x86_match_cpu(intel_pmc_core_ids);
> +       if (!cpu_id) {
> +               dev_dbg(&dev->dev, "PMC Core: cpuid mismatch.\n");
> +               return -EINVAL;
> +       }
> +
> +       err = pcim_enable_device(dev);
> +       if (err < 0) {
> +               dev_dbg(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n");
> +               return err;
> +       }
> +
> +       err = pci_read_config_dword(dev,
> +                                   SPT_PMC_BASE_ADDR_OFFSET,
> +                                   &pmcdev->base_addr);
> +       if (err < 0) {
> +               dev_dbg(&dev->dev, "PMC Core: failed to read pci config space.\n");

PCI

> +               return err;
> +       }
> +       dev_dbg(&dev->dev, "PMC Core: PWRMBASE is %#x\n", pmcdev->base_addr);
> +
> +       pmcdev->regmap = devm_ioremap_nocache(ptr_dev,
> +                                             pmcdev->base_addr,
> +                                             SPT_PMC_MMIO_REG_LEN);

I suggest to rename regmap to avoid confusion with regmap framework
widely used in the drivers. Choose something like base, regs, regbase,
etc.

> +       if (!pmcdev->regmap) {
> +               dev_dbg(&dev->dev, "PMC Core: ioremap failed.\n");
> +               return -ENOMEM;
> +       }
> +
> +       err = pmc_core_dbgfs_register(pmcdev);
> +       if (err < 0) {
> +               dev_err(&dev->dev, "PMC Core: debugfs register failed.\n");
> +               return err;
> +       }
> +
> +       pmc.has_slp_s0_res = true;
> +       return 0;
> +}
> +


> +static void intel_pmc_core_remove(struct pci_dev *pdev)
> +{
> +       pmc_core_dbgfs_unregister(&pmc);
> +}
> +
> +static struct pci_driver intel_pmc_core_driver = {
> +       .name = "intel_pmc_core",
> +       .id_table = pmc_pci_ids,
> +       .probe = pmc_core_probe,
> +       .remove = intel_pmc_core_remove,
> +};
> +module_pci_driver(intel_pmc_core_driver);
> +
> +MODULE_AUTHOR("Rajneesh Bhardwaj <rajneesh.bhardwaj@xxxxxxxxx>");
> +MODULE_AUTHOR("Vishwanath Somayaji <vishwanath.somayaji@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Intel CORE SoC Power Management Controller Interface");
> +MODULE_LICENSE("GPL v2");

So, since you have symbol defined as boolean, most of the above lines
are redundant including ->remove() and module.h inclusion.

Do we need it as a module? In that case you have to set to false
pmcdev->initialized variable.

> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -0,0 +1,53 @@
> +/*
> + * Intel Core SOC Power Management Controller Header File
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + * All Rights Reserved.

> + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@xxxxxxxxx)
> + * Author: Vishwanath Somayaji (vishwanath.somayaji@xxxxxxxxx)

Authors: Author 1
          Author 2

Check the other files.

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + */
> +
> +#ifndef PMC_CORE_H
> +#define PMC_CORE_H
> +
> +/* Sunrise Point Power Management Controller PCI Device ID */
> +#define SPT_PMC_PCI_DEVICE_ID                  0x9d21
> +#define SPT_PMC_BASE_ADDR_OFFSET               0x48
> +#define SPT_PMC_SLP_S0_RES_COUNTER_OFFSET      0x13c
> +#define SPT_PMC_MMIO_REG_LEN                   0x100
> +#define SPT_PMC_REG_BIT_WIDTH                  0x20
> +#define SPT_PMC_SLP_S0_RES_COUNTER_STEP                0x64
> +
> +/**
> + * struct pmc_dev - pmc device structure
> + * @base_addr:         comtains pmc base address
> + * @regmap:            pointer to io-remapped memory location
> + * @dbgfs_dir:         path to debug fs interface
> + * @feature_available: flag to indicate whether
> + *                     the feature is available
> + *                     on a particular platform or not.
> + *
> + * pmc_dev contains info about power management controller device.
> + */
> +struct pmc_dev {
> +       u32 base_addr;
> +       void __iomem *regmap;
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +       struct dentry *dbgfs_dir;
> +#endif /* CONFIG_DEBUG_FS */
> +       bool has_slp_s0_res;
> +};

I doubt this header makes any sense to be exist.

-- 
With Best Regards,
Andy Shevchenko