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

[Darren Hart <dvhart@infradead.org>]

On Thu, Apr 21, 2016 at 03:47:29PM +0300, Giedrius Statkevičius wrote:
> On Thu, Apr 21, 2016 at 04:33:25PM +0530, Rajneesh Bhardwaj wrote:
> [...]
> 
> Just some minor things I've spotted and one error is probably unchecked.
> 
> [...]
> > diff --git a/arch/x86/include/asm/pmc_core.h b/arch/x86/include/asm/pmc_core.h
> > new file mode 100644
> > index 0000000..3ea61bf
> > --- /dev/null
> > +++ b/arch/x86/include/asm/pmc_core.h
> > @@ -0,0 +1,53 @@
> > +/*
> > + * Intel Core SOC Power Management Controller Header File
> > + *
> > + * Copyright (c) 2016, Intel Corporation.
> > + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@intel.com)
> > + *
> > + * 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
> > +
> > +/* Skylake Power Management Controller PCI Device ID */
> > +#define PCI_DEVICE_ID_SKL_PMC   0x9d21
> > +#define PMC_BASE_ADDR_OFFSET    0x48
> > +#define PMC_SLP_S0_RESIDENCY_COUNTER 0x13c
> > +#define PMC_MMIO_REG_LEN        0x100
> > +#define PMC_REG_BIT_WIDTH       0x20
> > +#define SLP_S0_RESIDENCY_COUNTER_GRANULARITY 0x64
> 
> Inconsistent spaces. Maybe just use one space.
> 
> [...]
> 
> > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> > new file mode 100644
> > index 0000000..42cee87
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * Intel Core SOC Power Management Controller Driver
> > + *
> > + * Copyright (c) 2016, Intel Corporation.
> > + * Author: Rajneesh Bhardwaj (rajneesh.bhardwaj@intel.com)
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/pci.h>
> > +#include <linux/device.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <asm/cpu_device_id.h>
> > +#include <asm/pmc_core.h>
> > +
> > +static struct pmc_dev pmc;
> > +
> > +static const struct pci_device_id pmc_pci_ids[] = {
> > +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_SKL_PMC), (kernel_ulong_t)NULL },
> 
> Minor thing here but maybe just use a simple 0 here instead of
> (kernel_ulong_t)NULL? In the other places as well.
> 
> [...]
> 

Please provide the rational for a request for change. What do you think is wrong
with the above? There is precedent in the kernel today for this usage. Is it
wrong? Unnecessary? Subjectively ugly?

> > +static int pmc_core_dbgfs_register(struct pmc_dev *pmc)
> > +{
> > +	return 0; /* nothing to register */
> > +}
> > +
> > +#endif /* CONFIG_DEBUG_FS */
> > +
> > +static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > +	{ X86_VENDOR_INTEL, 6, 0x4e, X86_FEATURE_MWAIT,
> > +		(kernel_ulong_t)NULL}, /* SKL CPU*/
> > +	{ X86_VENDOR_INTEL, 6, 0x5e, X86_FEATURE_MWAIT,
> > +		(kernel_ulong_t)NULL}, /* SKL CPU*/
> > +	{}
> 
> A space after CPU? SKL? I assume this means skylake so perhaps add the whole
> name.
> 

The three letter CPU code is very common throughout the kernel of Intel CPUs.
NHM,WSM,SNB,IVB,BYT,HSW,BDW,SKL, etc.

Those are becoming more and more difficult to grep for though, so a full
expansion now and then would certainly be nice :-)

> > +};
> > +MODULE_DEVICE_TABLE(x86cpu, intel_pmc_core_ids);
> > +
> > +static int pmc_core_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > +{
> > +	int err;
> > +	const struct x86_cpu_id *cpu_id;
> > +
> > +	cpu_id = x86_match_cpu(intel_pmc_core_ids);
> > +	if (!cpu_id)
> > +		return -EINVAL;
> > +
> > +	err = pci_enable_device(dev);
> > +	if (err) {
> > +		dev_err(&dev->dev, "PMC Core: failed to enable Power Management Controller.\n");
> > +		goto exit;
> > +	}
> > +
> > +	pci_read_config_dword(dev, PMC_BASE_ADDR_OFFSET, &pmc.base_addr);
> 
> Shouldn't the return value of this function be checked if an error occured? Also

Yes please.

> while we are at it maybe the label names could be improved. For example:
> err_disable_dev; err_return
> 

This is more typical, although "out" or "err_out" if only used in the error
case. However, if the label does nothing but return with an error code, be sure
to be consistent in it's usage. Don't do "return -EINVAL" and "ret = -EINVAL;
goto err_out;" in the same function.


Thanks,

> > +	err = pmc_core_dbgfs_register(&pmc);
> > +	if (err) {
> > +		dev_err(&dev->dev, "PMC Core: debugfs register failed\n");
> > +		iounmap(pmc.regmap);
> > +		goto disable;
> > +	}
> > +
> > +	pmc.feature_available = 1;
> > +	return 0;
> > +
> > +disable:
> > +	pci_disable_device(dev);
> > +exit:
> > +	return err;
> > +}
> > +
> 

-- 
Darren Hart
Intel Open Source Technology Center