| 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
| ||
|
Message-ID: <20150527102241.GC11677@x1>
Date: Wed, 27 May 2015 11:22:41 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
linux-acpi@...r.kernel.org, linux-pm@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Vinod Koul <vinod.koul@...el.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
linux-kernel@...r.kernel.org, dmaengine@...r.kernel.org,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Jarkko Nikula <jarkko.nikula@...ux.intel.com>
Subject: Re: [PATCH v2 8/8] mfd: Add support for Intel Sunrisepoint LPSS
devices
On Mon, 25 May 2015, Andy Shevchenko wrote:
> The new coming Intel platforms such as Skylake will contain Sunrisepoint PCH.
> The main difference to the previous platforms is that the LPSS devices are
> compound devices where usually main (SPI, HSUART, or I2C) and DMA IPs are
> present.
>
> This patch brings the driver for such devices found on Sunrisepoint PCH.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
> drivers/mfd/Kconfig | 24 ++
> drivers/mfd/Makefile | 3 +
> drivers/mfd/intel-lpss-acpi.c | 84 +++++++
> drivers/mfd/intel-lpss-pci.c | 113 +++++++++
> drivers/mfd/intel-lpss.c | 534 ++++++++++++++++++++++++++++++++++++++++++
> drivers/mfd/intel-lpss.h | 62 +++++
> 6 files changed, 820 insertions(+)
> create mode 100644 drivers/mfd/intel-lpss-acpi.c
> create mode 100644 drivers/mfd/intel-lpss-pci.c
> create mode 100644 drivers/mfd/intel-lpss.c
> create mode 100644 drivers/mfd/intel-lpss.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index d5ad04d..8cdaa12 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -325,6 +325,30 @@ config INTEL_SOC_PMIC
> thermal, charger and related power management functions
> on these systems.
>
> +config MFD_INTEL_LPSS
> + tristate
> + depends on X86
> + select COMMON_CLK
> + select MFD_CORE
> +
> +config MFD_INTEL_LPSS_ACPI
> + tristate "Intel Low Power Subsystem support in ACPI mode"
> + select MFD_INTEL_LPSS
> + depends on ACPI
> + help
> + This driver supports Intel Low Power Subsystem (LPSS) devices such as
> + I2C, SPI and HS-UART starting from Intel Sunrisepoint (Intel Skylake
> + PCH) in ACPI mode.
> +
> +config MFD_INTEL_LPSS_PCI
> + tristate "Intel Low Power Subsystem support in PCI mode"
> + select MFD_INTEL_LPSS
> + depends on PCI
> + help
> + This driver supports Intel Low Power Subsystem (LPSS) devices such as
> + I2C, SPI and HS-UART starting from Intel Sunrisepoint (Intel Skylake
> + PCH) in PCI mode.
> +
> config MFD_INTEL_MSIC
> bool "Intel MSIC"
> depends on INTEL_SCU_IPC
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 0e5cfeb..cdf29b9 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -161,6 +161,9 @@ obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
> obj-$(CONFIG_MFD_TPS65090) += tps65090.o
> obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
> obj-$(CONFIG_MFD_ATMEL_HLCDC) += atmel-hlcdc.o
> +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_PALMAS) += palmas.o
> obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o
> diff --git a/drivers/mfd/intel-lpss-acpi.c b/drivers/mfd/intel-lpss-acpi.c
> new file mode 100644
> index 0000000..0d92d73
> --- /dev/null
> +++ b/drivers/mfd/intel-lpss-acpi.c
> @@ -0,0 +1,84 @@
> +/*
> + * Intel LPSS ACPI support.
> + *
> + * Copyright (C) 2015, Intel Corporation
> + *
> + * Authors: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> + * Mika Westerberg <mika.westerberg@...ux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/platform_device.h>
[...]
> +#include "intel-lpss.h"
> +int intel_lpss_probe(struct device *dev,
> + const struct intel_lpss_platform_info *info)
> +{
> + struct intel_lpss *lpss;
> + int ret;
> +
> + if (!info || !info->mem || info->irq <= 0)
> + return -EINVAL;
> +
> + lpss = devm_kzalloc(dev, sizeof(*lpss), GFP_KERNEL);
> + if (!lpss)
> + return -ENOMEM;
> +
> + lpss->priv = devm_ioremap(dev, info->mem->start + LPSS_PRIV_OFFSET,
> + LPSS_PRIV_SIZE);
> + if (!lpss->priv)
> + return -ENOMEM;
> +
> + lpss->info = info;
> + lpss->dev = dev;
> + lpss->caps = readl(lpss->priv + LPSS_PRIV_CAPS);
> +
> + dev_set_drvdata(dev, lpss);
> +
> + ret = intel_lpss_assign_devs(lpss);
> + if (ret)
> + return ret;
> +
> + intel_lpss_init_dev(lpss);
[...]
> + lpss->devid = ida_simple_get(&intel_lpss_devid_ida, 0, 0, GFP_KERNEL);
> + if (lpss->devid < 0)
> + return lpss->devid;
> +
> + ret = intel_lpss_register_clock(lpss);
> + if (ret < 0)
> + goto err_clk_register;
Still not convinced by this. I'd like Mike (who you *still* have not
CC'ed), to review.
> + intel_lpss_ltr_expose(lpss);
> +
> + ret = intel_lpss_debugfs_add(lpss);
> + if (ret)
> + dev_warn(lpss->dev, "Failed to create debugfs entries\n");
> +
> + if (intel_lpss_has_idma(lpss)) {
> + /*
> + * Ensure the DMA driver is loaded before the host
> + * controller device appears, so that the host controller
> + * driver can request its DMA channels as early as
> + * possible.
> + *
> + * If the DMA module is not there that's OK as well.
> + */
> + intel_lpss_request_dma_module(LPSS_IDMA_DRIVER_NAME);
> +
> + ret = mfd_add_devices(dev, lpss->devid, lpss->devs, 2,
> + info->mem, info->irq, NULL);
> + } else {
> + ret = mfd_add_devices(dev, lpss->devid, lpss->devs + 1, 1,
> + info->mem, info->irq, NULL);
> + }
I'm still not happy with the mfd_cells being manipulated in this way,
or with the duplication you have within them. Why don't you place the
IDMA device it its own mfd_cell, then:
> + if (intel_lpss_has_idma(lpss)) {
> + intel_lpss_request_dma_module(LPSS_IDMA_DRIVER_NAME);
> +
> + ret = mfd_add_devices(dev, TBC, idma_dev, ARRAY_SIZE(idma_dev),
> + info->mem, info->irq, NULL);
> + /* Error check */
> + }
> +
> + ret = mfd_add_devices(dev, TBC, proto_dev, ARRAY_SIZE(proto_dev),
> + info->mem, info->irq, NULL);
> + if (ret < 0)
if (!ret)
[...]
> +static int resume_lpss_device(struct device *dev, void *data)
> +{
> + pm_runtime_resume(dev);
> + return 0;
> +}
> +
> +int intel_lpss_prepare(struct device *dev)
> +{
> + /*
> + * Resume both child devices before entering system sleep. This
> + * ensures that they are in proper state before they get suspended.
> + */
> + device_for_each_child_reverse(dev, NULL, resume_lpss_device);
Why can't you do this in intel_lpss_suspend()?
Then you can get rid of all the hand-rolled nonsense you have in the
header file and use SET_SYSTEM_SLEEP_PM_OPS instead.
Does something happen after .prepare() and before .suspend() that
prevents this from working?
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_lpss_prepare);
> +
> +int intel_lpss_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_lpss_suspend);
> +
> +int intel_lpss_resume(struct device *dev)
> +{
> + struct intel_lpss *lpss = dev_get_drvdata(dev);
> +
> + intel_lpss_init_dev(lpss);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_lpss_resume);
> +
> +static int __init intel_lpss_init(void)
> +{
> + intel_lpss_debugfs = debugfs_create_dir("intel_lpss", NULL);
Any reason this can't be done in .probe()?
> + return 0;
> +}
> +module_init(intel_lpss_init);
> +
> +static void __exit intel_lpss_exit(void)
> +{
> + debugfs_remove(intel_lpss_debugfs);
.remove()?
> +}
> +module_exit(intel_lpss_exit);
> +
> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@...ux.intel.com>");
> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@...ux.intel.com>");
> +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@...ux.intel.com>");
> +MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@...ux.intel.com>");
> +MODULE_DESCRIPTION("Intel LPSS core driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> new file mode 100644
> index 0000000..f28cb28a
> --- /dev/null
> +++ b/drivers/mfd/intel-lpss.h
> @@ -0,0 +1,62 @@
> +/*
> + * Intel LPSS core support.
> + *
> + * Copyright (C) 2015, Intel Corporation
> + *
> + * Authors: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> + * Mika Westerberg <mika.westerberg@...ux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __MFD_INTEL_LPSS_H
> +#define __MFD_INTEL_LPSS_H
> +
> +struct device;
> +struct resource;
> +
> +struct intel_lpss_platform_info {
> + struct resource *mem;
> + int irq;
> + unsigned long clk_rate;
> + const char *clk_con_id;
> +};
> +
> +int intel_lpss_probe(struct device *dev,
> + const struct intel_lpss_platform_info *info);
> +void intel_lpss_remove(struct device *dev);
> +
> +#ifdef CONFIG_PM
> +int intel_lpss_prepare(struct device *dev);
> +int intel_lpss_suspend(struct device *dev);
> +int intel_lpss_resume(struct device *dev);
> +
> +#ifdef CONFIG_PM_SLEEP
> +#define INTEL_LPSS_SLEEP_PM_OPS \
> + .prepare = intel_lpss_prepare, \
> + .suspend = intel_lpss_suspend, \
> + .resume = intel_lpss_resume, \
> + .freeze = intel_lpss_suspend, \
> + .thaw = intel_lpss_resume, \
> + .poweroff = intel_lpss_suspend, \
> + .restore = intel_lpss_resume,
> +#endif
> +
> +#define INTEL_LPSS_RUNTIME_PM_OPS \
> + .runtime_suspend = intel_lpss_suspend, \
> + .runtime_resume = intel_lpss_resume,
> +
> +#else /* !CONFIG_PM */
> +#define INTEL_LPSS_SLEEP_PM_OPS
> +#define INTEL_LPSS_RUNTIME_PM_OPS
> +#endif /* CONFIG_PM */
> +
> +#define INTEL_LPSS_PM_OPS(name) \
> +const struct dev_pm_ops name = { \
> + INTEL_LPSS_SLEEP_PM_OPS \
> + INTEL_LPSS_RUNTIME_PM_OPS \
> +}
> +
> +#endif /* __MFD_INTEL_LPSS_H */
If you _really_ need .prepare, then it's likely that some other
platform might too. It will be the same amount of code to just make
this generic, so do that instead please.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists