lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 16 Jan 2020 13:21:08 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Darren Hart <dvhart@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Zha Qipeng <qipeng.zha@...el.com>,
        "David E . Box" <david.e.box@...ux.intel.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 35/36] platform/x86: intel_pmc_ipc: Convert to MFD

On Mon, 13 Jan 2020, Mika Westerberg wrote:

> This driver only creates a bunch of platform devices sharing resources
> belonging to the PMC device. This is pretty much what MFD subsystem is
> for so move the driver there, renaming it to intel_pmc_bxt.c which
> should be more clear what it is.
> 
> MFD subsystem provides nice helper APIs for subdevice creation so
> convert the driver to use those. Unfortunately the ACPI device includes
> separate resources for most of the subdevices so we cannot simply call
> mfd_add_devices() to create all of them but instead we need to call it
> separately for each device.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/mfd/Kconfig                           |  16 +-
>  drivers/mfd/Makefile                          |   1 +
>  drivers/mfd/intel_pmc_bxt.c                   | 543 +++++++++++++++
>  drivers/platform/x86/Kconfig                  |  16 +-
>  drivers/platform/x86/Makefile                 |   1 -
>  drivers/platform/x86/intel_pmc_ipc.c          | 650 ------------------
>  .../platform/x86/intel_telemetry_debugfs.c    |   2 +-
>  drivers/usb/typec/tcpm/Kconfig                |   2 +-
>  .../linux/mfd/intel_pmc_bxt.h                 |  11 +-
>  9 files changed, 573 insertions(+), 669 deletions(-)
>  create mode 100644 drivers/mfd/intel_pmc_bxt.c
>  delete mode 100644 drivers/platform/x86/intel_pmc_ipc.c
>  rename arch/x86/include/asm/intel_pmc_ipc.h => include/linux/mfd/intel_pmc_bxt.h (83%)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 59515142438e..b3b83932aaec 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -551,7 +551,7 @@ config INTEL_SOC_PMIC
>  
>  config INTEL_SOC_PMIC_BXTWC
>  	tristate "Support for Intel Broxton Whiskey Cove PMIC"
> -	depends on INTEL_PMC_IPC
> +	depends on MFD_INTEL_PMC_BXT
>  	select MFD_CORE
>  	select REGMAP_IRQ
>  	help
> @@ -632,6 +632,20 @@ config MFD_INTEL_MSIC
>  	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
>  	  devices used in Intel Medfield platforms.
>  
> +config MFD_INTEL_PMC_BXT
> +	tristate "Intel PMC Driver for Broxton"
> +	depends on X86
> +	depends on X86_PLATFORM_DEVICES
> +	depends on ACPI
> +	select INTEL_SCU_IPC
> +	select MFD_CORE
> +	help
> +	  This driver provides support for PMC (Power Management

"the PMC"

> +	  Controller) on Intel Broxton and Apollo Lake. PMC is a

"the PMC"

> +	  multi-function device that exposes IPC, General Control
> +	  Register and P-unit access. In addition this creates devices
> +	  for iTCO watchdog and telemetry that are part of the PMC.
>
>  config MFD_IPAQ_MICRO
>  	bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
>  	depends on SA1100_H3100 || SA1100_H3600
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index aed99f08739f..34563a6a047b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -211,6 +211,7 @@ obj-$(CONFIG_MFD_INTEL_LPSS)	+= intel-lpss.o
>  obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o
>  obj-$(CONFIG_MFD_INTEL_LPSS_ACPI)	+= intel-lpss-acpi.o
>  obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
> +obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
>  obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
>  obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
>  obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
> diff --git a/drivers/mfd/intel_pmc_bxt.c b/drivers/mfd/intel_pmc_bxt.c
> new file mode 100644
> index 000000000000..6cc87df4f573
> --- /dev/null
> +++ b/drivers/mfd/intel_pmc_bxt.c
> @@ -0,0 +1,543 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the Intel Broxton PMC
> + *
> + * (C) Copyright 2014-2015, 2020 Intel Corporation
> + *
> + * This driver is based on Intel SCU IPC driver(intel_scu_ipc.c) by
> + *     Sreedhara DS <sreedhara.ds@...el.com>
> + *
> + * PMC running in ARC processor communicates with other entity running in IA
> + * core through IPC mechanism which in turn messaging between IA core ad PMC.
> + */

"The PMC" .. "on the ARC" ... "another entity" ... "the IA"
"an IPC" ... "sends messages" ... "the IA and the PMC."

> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel_pmc_bxt.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/intel_scu_ipc.h>
> +
> +#include <linux/platform_data/itco_wdt.h>

Why are these 2 header files separated form the rest?

> +/* Residency with clock rate at 19.2MHz to usecs */
> +#define S0IX_RESIDENCY_IN_USECS(d, s)		\
> +({						\
> +	u64 result = 10ull * ((d) + (s));	\
> +	do_div(result, 192);			\
> +	result;					\

OOI, what does this line do?

> +})
> +
> +/* exported resources from IFWI */

Proper grammar please.

"Exported"

> +#define PLAT_RESOURCE_IPC_INDEX		0
> +#define PLAT_RESOURCE_IPC_SIZE		0x1000
> +#define PLAT_RESOURCE_GCR_OFFSET	0x1000
> +#define PLAT_RESOURCE_GCR_SIZE		0x1000
> +#define PLAT_RESOURCE_BIOS_DATA_INDEX	1
> +#define PLAT_RESOURCE_BIOS_IFACE_INDEX	2
> +#define PLAT_RESOURCE_TELEM_SSRAM_INDEX	3
> +#define PLAT_RESOURCE_ISP_DATA_INDEX	4
> +#define PLAT_RESOURCE_ISP_IFACE_INDEX	5
> +#define PLAT_RESOURCE_GTD_DATA_INDEX	6
> +#define PLAT_RESOURCE_GTD_IFACE_INDEX	7
> +#define PLAT_RESOURCE_ACPI_IO_INDEX	0
> +
> +/*
> + * BIOS does not create an ACPI device for each PMC function,
> + * but exports multiple resources from one ACPI device(IPC) for
> + * multiple functions. This driver is responsible to create a

"for creating"

> + * platform device and to export resources for those functions.

Prefer "child device" or "sub-device".

> + */
> +#define TCO_DEVICE_NAME			"iTCO_wdt"
> +#define SMI_EN_OFFSET			0x40
> +#define SMI_EN_SIZE			4
> +#define TCO_BASE_OFFSET			0x60
> +#define TCO_REGS_SIZE			16
> +#define PUNIT_DEVICE_NAME		"intel_punit_ipc"
> +#define TELEMETRY_DEVICE_NAME		"intel_telemetry"
> +#define TELEM_SSRAM_SIZE		240
> +#define TELEM_PMC_SSRAM_OFFSET		0x1B00
> +#define TELEM_PUNIT_SSRAM_OFFSET	0x1A00
> +
> +/* PMC register bit definitions */
> +
> +/* PMC_CFG_REG bit masks */
> +#define PMC_CFG_NO_REBOOT_MASK		BIT_MASK(4)
> +#define PMC_CFG_NO_REBOOT_EN		(1 << 4)
> +#define PMC_CFG_NO_REBOOT_DIS		(0 << 4)
> +
> +static struct intel_pmc_dev {
> +	struct device *dev;
> +
> +	/* iTCO */

Not sure these are required, the variables are clear enough.

> +	struct resource tco_res[2];
> +
> +	/* gcr */
> +	void __iomem *gcr_mem_base;
> +	spinlock_t gcr_lock;
> +
> +	/* punit */
> +	struct resource punit_res[6];
> +	unsigned int punit_res_count;
> +
> +	/* Telemetry */
> +	struct resource *telem_base;
> +} pmcdev;

Why not create this dynamically?

> +static inline u64 gcr_data_readq(u32 offset)
> +{
> +	return readq(pmcdev.gcr_mem_base + offset);
> +}

Abstraction for the sake of abstraction is generally frowned upon.

> +static inline int is_gcr_valid(u32 offset)
> +{
> +	if (!pmcdev.gcr_mem_base)
> +		return -EACCES;

Is this actually possible?

> +	if (offset > PLAT_RESOURCE_GCR_SIZE)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_pmc_gcr_read64() - Read a 64-bit PMC GCR register
> + * @offset:	offset of GCR register from GCR address base
> + * @data:	data pointer for storing the register output
> + *
> + * Reads the 64-bit PMC GCR register at given offset.
> + *
> + * Return:	negative value on error or 0 on success.
> + */
> +int intel_pmc_gcr_read64(u32 offset, u64 *data)
> +{
> +	int ret;
> +
> +	spin_lock(&pmcdev.gcr_lock);
> +
> +	ret = is_gcr_valid(offset);
> +	if (ret < 0) {
> +		spin_unlock(&pmcdev.gcr_lock);
> +		return ret;
> +	}
> +
> +	*data = readq(pmcdev.gcr_mem_base + offset);
> +
> +	spin_unlock(&pmcdev.gcr_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_read64);

What calls this?

It's probably nicer to provide the caller with a memory address.

Maybe using Regmap?

> +/**
> + * intel_pmc_gcr_update() - Update PMC GCR register bits
> + * @offset:	offset of GCR register from GCR address base
> + * @mask:	bit mask for update operation
> + * @val:	update value
> + *
> + * Updates the bits of given GCR register as specified by
> + * @mask and @val.
> + *
> + * Return:	negative value on error or 0 on success.
> + */
> +static int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
> +{
> +	u32 new_val;
> +	int ret = 0;
> +
> +	spin_lock(&pmcdev.gcr_lock);
> +
> +	ret = is_gcr_valid(offset);
> +	if (ret < 0)
> +		goto gcr_unlock;
> +
> +	new_val = readl(pmcdev.gcr_mem_base + offset);
> +
> +	new_val &= ~mask;
> +	new_val |= val & mask;
> +
> +	writel(new_val, pmcdev.gcr_mem_base + offset);
> +
> +	new_val = readl(pmcdev.gcr_mem_base + offset);
> +
> +	/* check whether the bit update is successful */
> +	if ((new_val & mask) != (val & mask)) {
> +		ret = -EIO;
> +		goto gcr_unlock;
> +	}
> +
> +gcr_unlock:
> +	spin_unlock(&pmcdev.gcr_lock);
> +	return ret;
> +}

Looks like Regmap could save you the trouble here.

> +static int update_no_reboot_bit(void *priv, bool set)
> +{
> +	u32 value = set ? PMC_CFG_NO_REBOOT_EN : PMC_CFG_NO_REBOOT_DIS;
> +
> +	return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +				    PMC_CFG_NO_REBOOT_MASK, value);
> +}

Only used by the Watchdog?  Maybe move in there?

> +static ssize_t intel_pmc_simple_cmd_store(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf, size_t count)
> +{
> +	struct intel_scu_ipc_dev *scu = dev_get_drvdata(dev);
> +	int subcmd;
> +	int cmd;
> +	int ret;
> +
> +	ret = sscanf(buf, "%d %d", &cmd, &subcmd);
> +	if (ret != 2) {
> +		dev_err(dev, "Error args\n");

Maybe give the user a clue as to the format?

> +		return -EINVAL;
> +	}
> +
> +	ret = intel_scu_ipc_dev_simple_command(scu, cmd, subcmd);
> +	if (ret) {
> +		dev_err(dev, "command %d error with %d\n", cmd, ret);

s/error/failed/

Use English grammar please.

> +		return ret;
> +	}
> +	return (ssize_t)count;

No need to cast:

$ git grep -W "static ssize_t" | grep return.*count | grep ssize_t | wc -l
11
$ git grep -W "static ssize_t" | grep return.*count | grep -v ssize_t | wc -l
3256

> +}
> +static DEVICE_ATTR(simplecmd, 0200, NULL, intel_pmc_simple_cmd_store);

I assume you've drafted some documentation for this?

If not, you need to.

> +static ssize_t intel_pmc_northpeak_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct intel_scu_ipc_dev *scu = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int subcmd;
> +	int ret;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	if (val)
> +		subcmd = 1;
> +	else
> +		subcmd = 0;

'\n'

> +	ret = intel_scu_ipc_dev_simple_command(scu, PMC_IPC_NORTHPEAK_CTRL, subcmd);
> +	if (ret) {
> +		dev_err(dev, "command north %d error with %d\n", subcmd, ret);

Use English grammar please.

> +		return ret;
> +	}
> +	return (ssize_t)count;
> +}
> +static DEVICE_ATTR(northpeak, 0200, NULL, intel_pmc_northpeak_store);

So a user can write any command to the PMC from userspace?

Is that a good idea?  No security implications for doing so?

...

I assume you've drafted some documentation for this?

If not, you need to.

> +static struct attribute *intel_pmc_attrs[] = {
> +	&dev_attr_northpeak.attr,
> +	&dev_attr_simplecmd.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group intel_pmc_group = {
> +	.attrs = intel_pmc_attrs,
> +};
> +
> +static const struct attribute_group *intel_pmc_groups[] = {
> +	&intel_pmc_group,
> +	NULL
> +};
> +
> +static int pmc_create_punit_device(void)
> +{
> +	struct mfd_cell punit = {
> +		.name = PUNIT_DEVICE_NAME,
> +		.resources = pmcdev.punit_res,
> +		.num_resources = pmcdev.punit_res_count,
> +	};

It's more common/preferred to create static structs.

Lots of examples in drivers/mfd.

> +	if (!pmcdev.punit_res_count)
> +		return 0;
> +
> +	return devm_mfd_add_devices(pmcdev.dev, PLATFORM_DEVID_AUTO, &punit, 1,
> +				    NULL, 0, NULL);
> +}
> +
> +static int pmc_create_tco_device(void)
> +{
> +	struct itco_wdt_platform_data tco_info = {
> +		.name = "Apollo Lake SoC",
> +		.version = 5,
> +		.no_reboot_priv = &pmcdev,
> +		.update_no_reboot_bit = update_no_reboot_bit,
> +	};
> +	struct mfd_cell tco = {
> +		.name = TCO_DEVICE_NAME,
> +		.ignore_resource_conflicts = true,
> +		.platform_data = &tco_info,
> +		.pdata_size = sizeof(tco_info),
> +		.resources = pmcdev.tco_res,
> +		.num_resources = ARRAY_SIZE(pmcdev.tco_res),
> +	};
> +
> +	if (!pmcdev.tco_res[0].start)
> +		return 0;

How would this be possible?

> +	return devm_mfd_add_devices(pmcdev.dev, PLATFORM_DEVID_AUTO, &tco, 1,
> +				    NULL, 0, NULL);
> +}
> +
> +static int pmc_create_telemetry_device(void)
> +{
> +	struct resource telem_res[] = {
> +		DEFINE_RES_MEM(TELEM_PUNIT_SSRAM_OFFSET, TELEM_SSRAM_SIZE),
> +		DEFINE_RES_MEM(TELEM_PMC_SSRAM_OFFSET, TELEM_SSRAM_SIZE),
> +	};
> +	struct mfd_cell telem  = {
> +		.name = TELEMETRY_DEVICE_NAME,
> +		.resources = telem_res,
> +		.num_resources = ARRAY_SIZE(telem_res),
> +	};
> +
> +	if (!pmcdev.telem_base)
> +		return 0;
> +
> +	return devm_mfd_add_devices(pmcdev.dev, PLATFORM_DEVID_AUTO, &telem, 1,
> +				    pmcdev.telem_base, 0, NULL);
> +}
> +
> +static int pmc_create_devices(void)
> +{
> +	int ret;
> +
> +	/* If we have ACPI based watchdog use that instead */
> +	if (!acpi_has_watchdog()) {
> +		ret = pmc_create_tco_device();
> +		if (ret) {
> +			dev_err(pmcdev.dev, "Failed to add tco platform device\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = pmc_create_punit_device();
> +	if (ret) {
> +		dev_err(pmcdev.dev, "Failed to add punit platform device\n");
> +		return ret;
> +	}
> +
> +	ret = pmc_create_telemetry_device();
> +	if (ret)
> +		dev_warn(pmcdev.dev, "Failed to add telemetry platform device\n");
> +
> +	return ret;
> +}

Once you have split out the 'struct mfd_cells' from the functions
above, you can move the devm_mfd_add_devices() calls into probe() and
do away with all of these functions which will greatly simplify the
driver as a whole.

> +static int pmc_plat_get_res(struct platform_device *pdev,
> +			    struct intel_scu_ipc_pdata *pdata)
> +{
> +	struct resource *res, *punit_res = pmcdev.punit_res;
> +	struct resource *tco_res = pmcdev.tco_res;
> +	void __iomem *addr;
> +	int size;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IO,
> +				    PLAT_RESOURCE_ACPI_IO_INDEX);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get IO resource\n");

What is the IO resource here used for?

> +		return -ENXIO;

Is "No such device or address" the correct response for this?

> +

Superfluous '\n'.

> +	}

'\n'

> +	tco_res[0].flags = IORESOURCE_IO;
> +	tco_res[0].start = res->start + TCO_BASE_OFFSET;
> +	tco_res[0].end = tco_res[0].start + TCO_REGS_SIZE - 1;
> +	tco_res[1].flags = IORESOURCE_IO;
> +	tco_res[1].start = res->start + SMI_EN_OFFSET;
> +	tco_res[1].end = tco_res[1].start + SMI_EN_SIZE - 1;
> +
> +	dev_dbg(&pdev->dev, "IO: %pR\n", res);

Do all of these dev_dgb() prints really still serve a purpose?

> +	pmcdev.punit_res_count = 0;
> +
> +	/* This is index 0 to cover BIOS data register */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM,
> +				    PLAT_RESOURCE_BIOS_DATA_INDEX);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get res of P-unit BIOS data\n");
> +		return -ENXIO;
> +	}
> +	punit_res[pmcdev.punit_res_count++] = *res;
> +	dev_dbg(&pdev->dev, "P-unit BIOS data: %pR\n", res);
> +
> +	/* This is index 1 to cover BIOS interface register */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM,
> +				    PLAT_RESOURCE_BIOS_IFACE_INDEX);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get res of P-unit BIOS iface\n");
> +		return -ENXIO;
> +	}
> +	punit_res[pmcdev.punit_res_count++] = *res;
> +	dev_dbg(&pdev->dev, "P-unit BIOS interface: %pR\n", res);
> +
> +	/* This is index 2 to cover ISP data register, optional */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM,
> +				    PLAT_RESOURCE_ISP_DATA_INDEX);
> +	if (res) {
> +		punit_res[pmcdev.punit_res_count++] = *res;
> +		dev_dbg(&pdev->dev, "P-unit ISP data: %pR\n", res);
> +	}
> +
> +	/* This is index 3 to cover ISP interface register, optional */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM,
> +				    PLAT_RESOURCE_ISP_IFACE_INDEX);
> +	if (res) {
> +		punit_res[pmcdev.punit_res_count++] = *res;
> +		dev_dbg(&pdev->dev, "P-unit ISP interface: %pR\n", res);
> +	}
> +
> +	/* This is index 4 to cover GTD data register, optional */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM,
> +				    PLAT_RESOURCE_GTD_DATA_INDEX);
> +	if (res) {
> +		punit_res[pmcdev.punit_res_count++] = *res;
> +		dev_dbg(&pdev->dev, "P-unit GTD data: %pR\n", res);
> +	}
> +
> +	/* This is index 5 to cover GTD interface register, optional */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM,
> +				    PLAT_RESOURCE_GTD_IFACE_INDEX);
> +	if (res) {
> +		punit_res[pmcdev.punit_res_count++] = *res;
> +		dev_dbg(&pdev->dev, "P-unit GTD interface: %pR\n", res);
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM,
> +				    PLAT_RESOURCE_IPC_INDEX);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get IPC resource\n");
> +		return -ENXIO;
> +	}

'\n'

> +	size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;

'size' seems like a superfluous variable.

> +	res->end = res->start + size - 1;
> +
> +	addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(addr))
> +		return PTR_ERR(addr);
> +
> +	pdata->ipc_regs = addr;

Wouldn't this fit better in intel_scu_ipc_probe()?

> +	pmcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
> +	dev_dbg(&pdev->dev, "IPC: %pR\n", res);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM,
> +				    PLAT_RESOURCE_TELEM_SSRAM_INDEX);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get telemetry SSRAM resource\n");

Is this actually an error?  If so, it should return an error code.

> +	} else {
> +		dev_dbg(&pdev->dev, "Telemetry SSRAM: %pR\n", res);
> +		pmcdev.telem_base = res;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_pmc_s0ix_counter_read() - Read S0ix residency.

What is residency?

> + * @data: Out param that contains current S0ix residency count.
> + *
> + * Return: an error code or 0 on success.
> + */
> +int intel_pmc_s0ix_counter_read(u64 *data)
> +{
> +	u64 deep, shlw;
> +
> +	if (!pmcdev.gcr_mem_base)
> +		return -EACCES;

Again, is this possible?

> +	deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);

Can this fail?

> +	shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);

Cant this fail?

> +	*data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_pmc_s0ix_counter_read);

What calls this?

> +static const struct acpi_device_id intel_pmc_acpi_ids[] = {
> +	{ "INT34D2", 0},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, intel_pmc_acpi_ids);
> +
> +static int intel_pmc_probe(struct platform_device *pdev)
> +{
> +	struct intel_scu_ipc_pdata pdata;

Can we request this dynamically please?

> +	struct intel_scu_ipc_dev *scu;
> +	int ret;
> +
> +	memset(&pdata, 0, sizeof(pdata));
> +	pdata.irq = platform_get_irq(pdev, 0);
> +	if (pdata.irq < 0)
> +		return -EINVAL;
> +
> +	pmcdev.dev = &pdev->dev;

Same for pmcdev.

> +	spin_lock_init(&pmcdev.gcr_lock);
> +
> +	ret = pmc_plat_get_res(pdev, &pdata);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request resource\n");

"resources"

> +		return ret;
> +	}
> +
> +	scu = intel_scu_ipc_probe(&pdev->dev, &pdata);

This is a parent or child device?

> +	if (IS_ERR(scu))
> +		return PTR_ERR(scu);
> +
> +	platform_set_drvdata(pdev, scu);
> +
> +	ret = pmc_create_devices();
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to create pmc devices\n");
> +		goto err_ipc;
> +	}
> +
> +	return 0;
> +
> +err_ipc:
> +	intel_scu_ipc_remove(scu);
> +
> +	return ret;
> +}
> +
> +static int intel_pmc_remove(struct platform_device *pdev)
> +{
> +	intel_scu_ipc_remove(platform_get_drvdata(pdev));
> +	pmcdev.dev = NULL;
> +	return 0;
> +}
> +
> +static struct platform_driver intel_pmc_driver = {
> +	.remove = intel_pmc_remove,
> +	.probe = intel_pmc_probe,
> +	.driver = {
> +		.name = "intel_pmc_bxt",
> +		.acpi_match_table = ACPI_PTR(intel_pmc_acpi_ids),
> +		.dev_groups = intel_pmc_groups,
> +	},
> +};
> +
> +static int __init intel_pmc_init(void)
> +{
> +	return platform_driver_register(&intel_pmc_driver);
> +}
> +
> +static void __exit intel_pmc_exit(void)
> +{
> +	platform_driver_unregister(&intel_pmc_driver);
> +}
> +
> +MODULE_AUTHOR("Zha Qipeng <qipeng.zha@...el.com>");
> +MODULE_DESCRIPTION("Intel Broxton PMC driver");
> +MODULE_LICENSE("GPL v2");
> +
> +/* Some modules are dependent on this, so init earlier */
> +fs_initcall(intel_pmc_init);

Prefer if you didn't have to rely on this.

Can you use -EPROBE_DEFER instead?

> +module_exit(intel_pmc_exit);

[...]

> diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/include/linux/mfd/intel_pmc_bxt.h
> similarity index 83%
> rename from arch/x86/include/asm/intel_pmc_ipc.h
> rename to include/linux/mfd/intel_pmc_bxt.h
> index 22848df5faaf..f03a80df0728 100644
> --- a/arch/x86/include/asm/intel_pmc_ipc.h
> +++ b/include/linux/mfd/intel_pmc_bxt.h

Need to review this too.

> @@ -1,6 +1,9 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _ASM_X86_INTEL_PMC_IPC_H_
> -#define  _ASM_X86_INTEL_PMC_IPC_H_
> +#ifndef MFD_INTEL_PMC_BXT_H
> +#define MFD_INTEL_PMC_BXT_H
> +
> +#include <linux/errno.h>
> +#include <linux/types.h>
>  
>  /* Commands */
>  #define PMC_IPC_USB_PWR_CTRL		0xF0
> @@ -25,7 +28,7 @@
>  #define PMC_GCR_TELEM_DEEP_S0IX_REG	0x78
>  #define PMC_GCR_TELEM_SHLW_S0IX_REG	0x80
>  
> -#if IS_ENABLED(CONFIG_INTEL_PMC_IPC)
> +#if IS_ENABLED(CONFIG_MFD_INTEL_PMC_BXT)
>  
>  int intel_pmc_s0ix_counter_read(u64 *data);
>  int intel_pmc_gcr_read64(u32 offset, u64 *data);
> @@ -42,6 +45,6 @@ static inline int intel_pmc_gcr_read64(u32 offset, u64 *data)
>  	return -EINVAL;
>  }
>  
> -#endif /*CONFIG_INTEL_PMC_IPC*/
> +#endif /* CONFIG_MFD_INTEL_PMC_BXT */
>  
>  #endif

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists