Re: [PATCH v1 1/1] x86/platform/intel-mid: Add Power Management Unit driver

[Ingo Molnar]

* Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> Add Power Management Unit driver to handle power states of South Complex
> devices on Intel Tangier. In the future it might be expanded to cover North
> Complex devices as well.
> 
> With this driver the power state of the host controllers such as SPI, I2C,
> UART, eMMC, and DMA would be managed.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/include/asm/intel-mid.h     |   8 +
>  arch/x86/pci/intel_mid_pci.c         |  35 +++-
>  arch/x86/platform/intel-mid/Makefile |   2 +-
>  arch/x86/platform/intel-mid/pmu.c    | 392 +++++++++++++++++++++++++++++++++++
>  drivers/pci/Makefile                 |   3 +
>  drivers/pci/pci-mid.c                |  77 +++++++
>  6 files changed, 515 insertions(+), 2 deletions(-)
>  create mode 100644 arch/x86/platform/intel-mid/pmu.c
>  create mode 100644 drivers/pci/pci-mid.c

So this collides with perf's 'PMU' naming massively. Can we pick another name 
before hillarious kernel-wide confusion spreads?

how about intel/mid/pm.c plus renaming all the pmu* internal names to pm*?

We could call it 'power management interface', and in a single line mention that 
this is also a 'Power Management Unit' in Intel-speak?

>  extern int intel_mid_pci_init(void);
> +int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state);
> +
> +#define INTEL_MID_PMU_LSS_OFFSET	4
> +#define INTEL_MID_PMU_LSS_TYPE		(1 << 7)
> +
> +int intel_mid_pmu_get_lss_id(struct pci_dev *pdev);

Yeah, so please be consistent about 'extern'.

Also, I had a look at the resulting arch/x86/include/asm/intel-mid.h and the 
vertical alignments are all over the map.

Here is how it looks like:

#define FSB_FREQ_83SKU  83200
#define FSB_FREQ_100SKU 99840
#define FSB_FREQ_133SKU 133000

#define FSB_FREQ_167SKU 167000
#define FSB_FREQ_200SKU 200000
#define FSB_FREQ_267SKU 267000
#define FSB_FREQ_333SKU 333000
#define FSB_FREQ_400SKU 400000

/* Bus Select SoC Fuse value */
#define BSEL_SOC_FUSE_MASK      0x7
#define BSEL_SOC_FUSE_001       0x1 /* FSB 133MHz */
#define BSEL_SOC_FUSE_101       0x5 /* FSB 100MHz */
#define BSEL_SOC_FUSE_111       0x7 /* FSB 83MHz */

#define SFI_MTMR_MAX_NUM 8
#define SFI_MRTC_MAX    8

Can we please improve that?

> +
> +static void mrst_power_off_unused_dev(struct pci_dev *dev)
> +{
> +	mid_power_off_dev(dev);
> +}
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0801, mrst_power_off_unused_dev);

So we add mrst_power_off_unused_dev() just to make it to mid_power_off_dev()?

Also, newlines ran out when the above bit was written.

> +++ b/arch/x86/platform/intel-mid/pmu.c
> @@ -0,0 +1,392 @@
> +/*
> + * Intel MID Power Management Unit device driver

Could we please write a bit longer description about what this driver does, what 
interfaces/capabilities it enables, etc.? People like the warm fuzzy feeling 
associated with knowing what's going on.

> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/types.h>

Was that types.h include really needed?

> +/* Registers */
> +#define PM_STS		0x00
> +#define PM_CMD		0x04
> +#define PM_ICS		0x08
> +#define PM_WKC(x)	(0x10 + (x) * 4)
> +#define PM_WKS(x)	(0x18 + (x) * 4)
> +#define PM_SSC(x)	(0x20 + (x) * 4)
> +#define PM_SSS(x)	(0x30 + (x) * 4)
> +
> +/* Bits in PM_STS */
> +#define PM_STS_BUSY		(1 << 8)
> +
> +/* Bits in PM_CMD */
> +#define PM_CMD_CMD(x)		((x) << 0)
> +#define PM_CMD_IOC		(1 << 8)
> +#define PM_CMD_D3cold		(1 << 21)
> +
> +/* List of commands */
> +#define CMD_SET_CFG		0x01
> +
> +/* Bits in PM_ICS */
> +#define PM_ICS_INT_STATUS(x)	((x) & 0xff)
> +#define PM_ICS_IE		(1 << 8)
> +#define PM_ICS_IP		(1 << 9)
> +#define PM_ICS_SW_INT_STS	(1 << 10)
> +
> +/* List of interrupts */
> +#define INT_INVALID		0
> +#define INT_CMD_COMPLETE	1
> +#define INT_CMD_ERR		2
> +#define INT_WAKE_EVENT		3
> +#define INT_LSS_POWER_ERR	4
> +#define INT_S0iX_MSG_ERR	5
> +#define INT_NO_C6		6
> +#define INT_TRIGGER_ERR		7
> +#define INT_INACTIVITY		8
> +
> +/* South Complex devices */
> +#define LSS_MAX_SHARED_DEVS		4
> +#define LSS_MAX_DEVS			64
> +
> +#define LSS_WS_BITS			1	/* wake state width */
> +#define LSS_PWS_BITS			2	/* power state width */
> +
> +/* Supported device IDs */
> +#define PCI_DEVICE_ID_TANGIER		0x11a1

Again essentially randomized vertical alignment. Is anyone supposed to read this 
code?

> +	/* Find device in cache or first free cell */
> +	for (j = 0; j < LSS_MAX_SHARED_DEVS; j++)
> +		if (lss[j].pdev == pdev || !lss[j].pdev)
> +			break;

... missing curly braces. That's a problem in other places as well, please fix all 
of them.

> +static int tng_set_initial_state(struct mid_pmu *pmu)
> +{
> +	unsigned int i, j;
> +	int ret;
> +
> +	/* Enable wake events */
> +	mid_pmu_set_wake(pmu, 0, 0xffffffff);
> +	mid_pmu_set_wake(pmu, 1, 0xffffffff);
> +
> +	/* Power off unused devices */
> +	mid_pmu_set_state(pmu, 0, 0xffffffff);
> +	mid_pmu_set_state(pmu, 1, 0xffffffff);
> +	mid_pmu_set_state(pmu, 2, 0xffffffff);
> +	mid_pmu_set_state(pmu, 3, 0xffffffff);

What are these magic numbers of 0/1/2/3?

> +static struct pci_driver mid_pmu_pci_driver = {
> +	.name		= "intel_mid_pmu",
> +	.probe		= mid_pmu_probe,
> +	.id_table	= mid_pmu_pci_ids,
> +};

This structure initialization has a nice vertical layout.

> +static struct pci_platform_pm_ops mid_pci_platform_pm = {
> +	.is_manageable = mid_pci_power_manageable,
> +	.set_state = mid_pci_set_power_state,
> +	.choose_state = mid_pci_choose_state,
> +	.sleep_wake = mid_pci_sleep_wake,
> +	.run_wake = mid_pci_run_wake,
> +	.need_resume = mid_pci_need_resume,
> +};

This one, not so much.

Thanks,

	Ingo