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
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
