| 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: <20150429150619.GW9169@x1>
Date: Wed, 29 Apr 2015 16:06:19 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Jarkko Nikula <jarkko.nikula@...ux.intel.com>
Subject: Re: [PATCH v1 3/3] mfd: Add support for Intel Sunrisepoint LPSS
devices
On Wed, 29 Apr 2015, Andy Shevchenko wrote:
> On Wed, Apr 29, 2015 at 11:23 AM, Lee Jones <lee.jones@...aro.org> wrote:
> > On Tue, 31 Mar 2015, Andy Shevchenko wrote:
>
> Thanks for review. My comments / answers below.
>
> >> The new coming Intel platforms such as Skylake will contain Sunrisepoint PCH.
> >
> > Can you use the same naming conventions as the Skylake's predecessors?
> >
> > Currently lpc_ich and lpc_sch exist.
>
> As far as I understand it is not an LPC bus, thus seems above is
> irrelevant. Moreover, PCH in Skylake is something new if looking into
> hardware.
Perhaps the aforementioned files just need 'intel_' appended then.
> >> 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 | 106 +++++++++
> >> drivers/mfd/intel-lpss.c | 523 ++++++++++++++++++++++++++++++++++++++++++
> >> drivers/mfd/intel-lpss.h | 62 +++++
> >
> > Can you move this to include/?
>
> What the point? I don't see the users for that now or in the nearest future.
The point is to keep drivers/mfd/ clean rather than to share the header
with external entities.
> >> 6 files changed, 802 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..b1a6778 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 "Intel Low Power Subsystem support"
> >> + depends on X86
> >> + select COMMON_CLK
> >> + select MFD_CORE
> >> + help
> >> + This driver provides necessary plumbing for Intel Low Power
> >> + Subsystem (LPSS) devices such as I2C, SPI and HS-UART starting
> >> + from Intel Sunrisepoint (Intel Skylake PCH) and newer chipsets.
> >> +
> >> +config MFD_INTEL_LPSS_ACPI
> >> + tristate "Intel Low Power Subsystem support in ACPI mode"
> >> + depends on MFD_INTEL_LPSS && ACPI
> >> + help
> >> + This driver support Intel Low Power Subsystem devices in ACPI
> >> + mode.
> >> +
> >> +config MFD_INTEL_LPSS_PCI
> >> + tristate "Intel Low Power Subsystem support in PCI mode"
> >> + depends on MFD_INTEL_LPSS && PCI
> >> + help
> >> + This driver support Intel Low Power Subsystem devices 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"
> >> +
> >> +static const struct intel_lpss_platform_info spt_info = {
> >> + .clk_rate = 120000000,
> >> +};
> >> +
> >> +static const struct acpi_device_id intel_lpss_acpi_ids[] = {
> >> + /* SPT */
> >> + { "INT3446", (kernel_ulong_t)&spt_info },
> >> + { "INT3447", (kernel_ulong_t)&spt_info },
> >> + { }
> >> +};
> >> +MODULE_DEVICE_TABLE(acpi, intel_lpss_acpi_ids);
> >> +
> >> +static int intel_lpss_acpi_probe(struct platform_device *pdev)
> >> +{
> >> + struct intel_lpss_platform_info *info;
> >> + const struct acpi_device_id *id;
> >> +
> >> + id = acpi_match_device(intel_lpss_acpi_ids, &pdev->dev);
> >> + if (!id)
> >> + return -ENODEV;
> >> +
> >> + info = devm_kmemdup(&pdev->dev, (void *)id->driver_data, sizeof(*info),
> >> + GFP_KERNEL);
> >> + if (!info)
> >> + return -ENOMEM;
> >> +
> >> + info->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> + info->irq = platform_get_irq(pdev, 0);
> >> +
> >> + pm_runtime_set_active(&pdev->dev);
> >> + pm_runtime_enable(&pdev->dev);
> >> +
> >> + return intel_lpss_probe(&pdev->dev, info);
> >> +}
> >> +
> >> +static int intel_lpss_acpi_remove(struct platform_device *pdev)
> >> +{
> >> + intel_lpss_remove(&pdev->dev);
> >> + pm_runtime_disable(&pdev->dev);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static INTEL_LPSS_PM_OPS(intel_lpss_acpi_pm_ops);
> >> +
> >> +static struct platform_driver intel_lpss_acpi_driver = {
> >> + .probe = intel_lpss_acpi_probe,
> >> + .remove = intel_lpss_acpi_remove,
> >> + .driver = {
> >> + .name = "intel-lpss",
> >> + .acpi_match_table = intel_lpss_acpi_ids,
> >> + .pm = &intel_lpss_acpi_pm_ops,
> >> + },
> >> +};
> >> +
> >> +module_platform_driver(intel_lpss_acpi_driver);
> >> +
> >> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@...ux.intel.com>");
> >> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@...ux.intel.com>");
> >> +MODULE_DESCRIPTION("Intel LPSS ACPI driver");
> >> +MODULE_LICENSE("GPL v2");
> >> diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
> >> new file mode 100644
> >> index 0000000..fd8d25d
> >> --- /dev/null
> >> +++ b/drivers/mfd/intel-lpss-pci.c
> >> @@ -0,0 +1,106 @@
> >> +/*
> >> + * Intel LPSS PCI 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/ioport.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/pm.h>
> >> +#include <linux/pm_runtime.h>
> >> +
> >> +#include "intel-lpss.h"
> >> +
> >> +static int intel_lpss_pci_probe(struct pci_dev *pdev,
> >> + const struct pci_device_id *id)
> >> +{
> >> + struct intel_lpss_platform_info *info;
> >> + int ret;
> >> +
> >> + ret = pcim_enable_device(pdev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + info = devm_kmemdup(&pdev->dev, (void *)id->driver_data, sizeof(*info),
> >> + GFP_KERNEL);
> >> + if (!info)
> >> + return -ENOMEM;
> >> +
> >> + info->mem = &pdev->resource[0];
> >> + info->irq = pdev->irq;
> >> +
> >> + ret = intel_lpss_probe(&pdev->dev, info);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + pm_runtime_put(&pdev->dev);
> >> + return 0;
> >> +}
> >> +
> >> +static void intel_lpss_pci_remove(struct pci_dev *pdev)
> >> +{
> >> + pm_runtime_get_sync(&pdev->dev);
> >> + intel_lpss_remove(&pdev->dev);
> >> +}
> >> +
> >> +static INTEL_LPSS_PM_OPS(intel_lpss_pci_pm_ops);
> >> +
> >> +static const struct intel_lpss_platform_info spt_info = {
> >> + .clk_rate = 120000000,
> >> +};
> >> +
> >> +static const struct intel_lpss_platform_info spt_uart_info = {
> >> + .clk_rate = 120000000,
> >> + .clk_con_id = "baudclk",
> >> +};
> >> +
> >> +static const struct pci_device_id intel_lpss_pci_ids[] = {
> >> + /* SPT-LP */
> >> + { PCI_VDEVICE(INTEL, 0x9d27), (kernel_ulong_t)&spt_uart_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d28), (kernel_ulong_t)&spt_uart_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d29), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d2a), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d60), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d61), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d62), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d63), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d64), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d65), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0x9d66), (kernel_ulong_t)&spt_uart_info },
> >> + /* SPT-H */
> >> + { PCI_VDEVICE(INTEL, 0xa127), (kernel_ulong_t)&spt_uart_info },
> >> + { PCI_VDEVICE(INTEL, 0xa128), (kernel_ulong_t)&spt_uart_info },
> >> + { PCI_VDEVICE(INTEL, 0xa129), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0xa12a), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0xa160), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0xa161), (kernel_ulong_t)&spt_info },
> >> + { PCI_VDEVICE(INTEL, 0xa166), (kernel_ulong_t)&spt_uart_info },
> >> + { }
> >> +};
> >> +MODULE_DEVICE_TABLE(pci, intel_lpss_pci_ids);
> >> +
> >> +static struct pci_driver intel_lpss_pci_driver = {
> >> + .name = "intel-lpss",
> >> + .id_table = intel_lpss_pci_ids,
> >> + .probe = intel_lpss_pci_probe,
> >> + .remove = intel_lpss_pci_remove,
> >> + .driver = {
> >> + .pm = &intel_lpss_pci_pm_ops,
> >> + },
> >> +};
> >> +
> >> +module_pci_driver(intel_lpss_pci_driver);
> >> +
> >> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@...ux.intel.com>");
> >> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@...ux.intel.com>");
> >> +MODULE_DESCRIPTION("Intel LPSS PCI driver");
> >> +MODULE_LICENSE("GPL v2");
> >> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> >> new file mode 100644
> >> index 0000000..78d1921
> >> --- /dev/null
> >> +++ b/drivers/mfd/intel-lpss.c
> >> @@ -0,0 +1,523 @@
> >> +/*
> >> + * Intel Sunrisepoint LPSS core support.
> >> + *
> >> + * Copyright (C) 2015, Intel Corporation
> >> + *
> >> + * Authors: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> >> + * Mika Westerberg <mika.westerberg@...ux.intel.com>
> >> + * Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> >> + * Jarkko Nikula <jarkko.nikula@...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/clk.h>
> >> +#include <linux/clkdev.h>
> >> +#include <linux/clk-provider.h>
> >> +#include <linux/debugfs.h>
> >> +#include <linux/idr.h>
> >> +#include <linux/ioport.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mfd/core.h>
> >> +#include <linux/platform_data/dma-dw.h>
> >> +#include <linux/pm_qos.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/seq_file.h>
> >> +
> >> +#include "intel-lpss.h"
> >> +
> >> +#define LPSS_DEV_OFFSET 0x000
> >> +#define LPSS_DEV_SIZE 0x200
> >> +#define LPSS_PRIV_OFFSET 0x200
> >> +#define LPSS_PRIV_SIZE 0x100
> >> +#define LPSS_IDMA_OFFSET 0x800
> >> +#define LPSS_IDMA_SIZE 0x800
> >> +
> >> +/* Offsets from lpss->priv */
> >> +#define LPSS_PRIV_RESETS 0x04
> >> +#define LPSS_PRIV_RESETS_FUNC BIT(2)
> >> +#define LPSS_PRIV_RESETS_IDMA 0x3
> >> +
> >> +#define LPSS_PRIV_ACTIVELTR 0x10
> >> +#define LPSS_PRIV_IDLELTR 0x14
> >> +
> >> +#define LPSS_PRIV_LTR_REQ BIT(15)
> >> +#define LPSS_PRIV_LTR_SCALE_MASK 0xc00
> >> +#define LPSS_PRIV_LTR_SCALE_1US 0x800
> >> +#define LPSS_PRIV_LTR_SCALE_32US 0xc00
> >> +#define LPSS_PRIV_LTR_VALUE_MASK 0x3ff
> >> +
> >> +#define LPSS_PRIV_CLK_GATE 0x38
> >> +#define LPSS_PRIV_CLK_GATE_IP_SHIFT 0
> >> +#define LPSS_PRIV_CLK_GATE_IDMA_SHIFT 2
> >> +#define LPSS_PRIV_CLK_GATE_EN_MASK 0x2 /* Dynamic gating disabled */
> >> +
> >> +#define LPSS_PRIV_CAPS 0xfc
> >> +#define LPSS_PRIV_CAPS_NO_IDMA BIT(8)
> >> +#define LPSS_PRIV_CAPS_TYPE_SHIFT 4
> >> +#define LPSS_PRIV_CAPS_TYPE_MASK (0xf << LPSS_PRIV_CAPS_TYPE_SHIFT)
> >> +
> >> +/* This matches the type field in CAPS register */
> >> +enum intel_lpss_dev_type {
> >> + LPSS_DEV_I2C = 0,
> >> + LPSS_DEV_UART,
> >> + LPSS_DEV_SPI,
> >> +};
> >> +
> >> +struct intel_lpss {
> >> + const struct intel_lpss_platform_info *info;
> >> + enum intel_lpss_dev_type type;
> >> + struct clk *clk;
> >> + struct clk_lookup **clocks;
> >> + unsigned int ndevs;
> >> + const struct mfd_cell *devs;
> >> + struct device *dev;
> >> + void __iomem *priv;
> >> + int devid;
> >> + u32 caps;
> >> + u32 active_ltr;
> >> + u32 idle_ltr;
> >> + struct dentry *debugfs;
> >> +};
> >> +
> >> +static const struct dw_dma_platform_data intel_lpss_idma_pdata = {
> >> + .type = DW_DMAC_TYPE_INTEL,
> >> + .nr_channels = 2,
> >> + .is_private = true,
> >> + .block_size = 0x1ffff,
> >> + .nr_masters = 1,
> >> + .data_width = {2},
> >
> > .data_width = { 2 },
>
> Will fix.
>
> >
> >> +};
> >> +
> >> +static const struct resource intel_lpss_dev_resources[] = {
> >> + DEFINE_RES_MEM_NAMED(LPSS_DEV_OFFSET, LPSS_DEV_SIZE, "lpss_dev"),
> >> + DEFINE_RES_MEM_NAMED(LPSS_PRIV_OFFSET, LPSS_PRIV_SIZE, "lpss_priv"),
> >> + DEFINE_RES_IRQ(0),
> >> +};
> >
> > Why has this been called 'dev' and 'priv'? Neither of which are
> > particularlly helpful address names and I can't find them in the
> > datasheet. Why not 'base' and 'extended', or something?
>
> dev stands for main device controller
> priv -- for private space. The abbrevation is inherited from LPSS
> drivers for BayTrail and Co.
>
> In documentation priv space called 'additional registers'.
In my mind 'additional registers' != 'private registers'.
> >> +static const struct resource intel_lpss_idma_resources[] = {
> >> + DEFINE_RES_MEM(LPSS_IDMA_OFFSET, LPSS_IDMA_SIZE),
> >> + DEFINE_RES_IRQ(0),
> >> +};
> >> +
> >> +/*
> >> + * Cells needs to be ordered so that the iDMA is created first. This is
> >> + * because we need to be sure the DMA is available when the host controller
> >> + * driver is probed.
> >> + */
> >> +static const struct mfd_cell intel_lpss_i2c_devs[] = {
> >> + {
> >> + .name = "dw_dmac",
> >> + .num_resources = ARRAY_SIZE(intel_lpss_idma_resources),
> >> + .resources = intel_lpss_idma_resources,
> >> + .platform_data = (void *)&intel_lpss_idma_pdata,
> >> + .pdata_size = sizeof(intel_lpss_idma_pdata),
> >> + },
> >> + {
> >> + .name = "i2c_designware",
> >> + .num_resources = ARRAY_SIZE(intel_lpss_dev_resources),
> >> + .resources = intel_lpss_dev_resources,
> >> + },
> >> +};
> >> +
> >> +static const struct mfd_cell intel_lpss_uart_devs[] = {
> >> + {
> >> + .name = "dw_dmac",
> >> + .num_resources = ARRAY_SIZE(intel_lpss_idma_resources),
> >> + .resources = intel_lpss_idma_resources,
> >> + .platform_data = (void *)&intel_lpss_idma_pdata,
> >> + .pdata_size = sizeof(intel_lpss_idma_pdata),
> >> + },
> >> + {
> >> + .name = "dw-apb-uart",
> >> + .num_resources = ARRAY_SIZE(intel_lpss_dev_resources),
> >> + .resources = intel_lpss_dev_resources,
> >> + },
> >> +};
> >> +
> >> +static const struct mfd_cell intel_lpss_spi_devs[] = {
> >> + {
> >> + .name = "dw_dmac",
> >> + .num_resources = ARRAY_SIZE(intel_lpss_idma_resources),
> >> + .resources = intel_lpss_idma_resources,
> >> + .platform_data = (void *)&intel_lpss_idma_pdata,
> >> + .pdata_size = sizeof(intel_lpss_idma_pdata),
> >> + },
> >> + {
> >> + .name = "pxa2xx-spi",
> >> + .num_resources = ARRAY_SIZE(intel_lpss_dev_resources),
> >> + .resources = intel_lpss_dev_resources,
> >> + },
> >> +};
> >> +
> >> +static DEFINE_IDA(intel_lpss_devid_ida);
> >
> > What's wrong with using PLATFORM_DEVID_AUTO?
>
> We have to provide right clocks.
> Is it possible to use AUTO and be sure that right clocks will go to
> corresponding consumers?
I guess Mike will have to help us with that, as I've never seen it
done this way before.
I guess you need to create a clock driver and register it properly as
a platform device.
> >> +static struct dentry *intel_lpss_debugfs;
> >> +
> >> +static void intel_lpss_cache_ltr(struct intel_lpss *lpss)
> >> +{
> >> + lpss->active_ltr = readl(lpss->priv + LPSS_PRIV_ACTIVELTR);
> >> + lpss->idle_ltr = readl(lpss->priv + LPSS_PRIV_IDLELTR);
> >> +}
> >> +
> >> +static int intel_lpss_debugfs_add(struct intel_lpss *lpss)
> >> +{
> >> + struct dentry *dir;
> >> +
> >> + dir = debugfs_create_dir(dev_name(lpss->dev), intel_lpss_debugfs);
> >> + if (IS_ERR(dir))
> >> + return PTR_ERR(dir);
> >> +
> >> + /* Cache the values into lpss structure */
> >> + intel_lpss_cache_ltr(lpss);
> >> +
> >> + debugfs_create_x32("capabilities", S_IRUGO, dir, &lpss->caps);
> >> + debugfs_create_x32("active_ltr", S_IRUGO, dir, &lpss->active_ltr);
> >> + debugfs_create_x32("idle_ltr", S_IRUGO, dir, &lpss->idle_ltr);
> >> +
> >> + lpss->debugfs = dir;
> >> + return 0;
> >> +}
> >> +
> >> +static void intel_lpss_debugfs_remove(struct intel_lpss *lpss)
> >> +{
> >> + debugfs_remove_recursive(lpss->debugfs);
> >> +}
> >
> > A comment here explaining what you're doing below wouldn't go amiss.
>
> Ok.
>
> >
> >> +static void intel_lpss_ltr_set(struct device *dev, s32 val)
> >> +{
> >> + struct intel_lpss *lpss = dev_get_drvdata(dev);
> >> + u32 ltr;
> >> +
> >> + ltr = readl(lpss->priv + LPSS_PRIV_ACTIVELTR);
> >> +
> >> + if (val == PM_QOS_LATENCY_ANY || val < 0) {
> >> + ltr &= ~LPSS_PRIV_LTR_REQ;
> >> + } else {
> >> + ltr |= LPSS_PRIV_LTR_REQ;
> >> + ltr &= ~LPSS_PRIV_LTR_SCALE_MASK;
> >> + ltr &= ~LPSS_PRIV_LTR_VALUE_MASK;
> >> +
> >> + if (val > LPSS_PRIV_LTR_VALUE_MASK)
> >> + ltr |= LPSS_PRIV_LTR_SCALE_32US | val >> 5;
> >> + else
> >> + ltr |= LPSS_PRIV_LTR_SCALE_1US | val;
> >> + }
> >> +
> >> + if (ltr == lpss->active_ltr)
> >> + return;
> >> +
> >> + writel(ltr, lpss->priv + LPSS_PRIV_ACTIVELTR);
> >> + writel(ltr, lpss->priv + LPSS_PRIV_IDLELTR);
> >> +
> >> + /* Cache the values into lpss structure */
> >> + intel_lpss_cache_ltr(lpss);
> >> +}
> >> +
> >> +static void intel_lpss_ltr_add(struct intel_lpss *lpss)
> >> +{
> >> + lpss->dev->power.set_latency_tolerance = intel_lpss_ltr_set;
> >> + dev_pm_qos_expose_latency_tolerance(lpss->dev);
> >> +}
> >> +
> >> +static void intel_lpss_ltr_remove(struct intel_lpss *lpss)
> >> +{
> >> + dev_pm_qos_hide_latency_tolerance(lpss->dev);
> >> + lpss->dev->power.set_latency_tolerance = NULL;
> >> +}
> >> +
> >> +static int intel_lpss_assign_devs(struct intel_lpss *lpss)
> >> +{
> >> + unsigned int type;
> >> +
> >> + type = lpss->caps & LPSS_PRIV_CAPS_TYPE_MASK;
> >> + type >>= LPSS_PRIV_CAPS_TYPE_SHIFT;
> >> +
> >> + switch (type) {
> >> + case LPSS_DEV_I2C:
> >> + lpss->devs = intel_lpss_i2c_devs;
> >> + break;
> >> + case LPSS_DEV_UART:
> >> + lpss->devs = intel_lpss_uart_devs;
> >> + break;
> >> + case LPSS_DEV_SPI:
> >> + lpss->devs = intel_lpss_spi_devs;
> >> + break;
> >> + default:
> >> + return -ENODEV;
> >> + }
> >> +
> >> + lpss->type = type;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static bool intel_lpss_has_idma(const struct intel_lpss *lpss)
> >> +{
> >> + return (lpss->caps & LPSS_PRIV_CAPS_NO_IDMA) == 0;
> >> +}
> >> +
> >> +static void intel_lpss_reset(const struct intel_lpss *lpss)
> >
> > Not very descriptive function name. 'de|assert'?
>
> It actually handles the reset signal. I don't see how de/assert will be better.
What do you mean by 'handle'? By the comment below it appers to
"bring the device out of reset" i.e. deassert the reset line, no?
intel_lpss_deassert_reset()?
> >> +{
> >> + u32 value = LPSS_PRIV_RESETS_FUNC | LPSS_PRIV_RESETS_IDMA;
> >> +
> >> + /* Bring out the device from reset */
> >> + writel(value, lpss->priv + LPSS_PRIV_RESETS);
> >> +}
> >
> > All of this clock stuff below need s Clock Ack.
>
> So, is anyone in your mind to whom we can Cc the new version?
COMMON CLK FRAMEWORK
M: Mike Turquette <mturquette@...aro.org>
M: Stephen Boyd <sboyd@...eaurora.org>
L: linux-kernel@...r.kernel.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
S: Maintained
F: drivers/clk/
X: drivers/clk/clkdev.c
F: include/linux/clk-pr*
F: include/linux/clk/
> >> +static void intel_lpss_unregister_clock_tree(struct clk *clk)
> >> +{
> >> + struct clk *parent;
> >> +
> >> + while (clk) {
> >> + parent = clk_get_parent(clk);
> >> + clk_unregister(clk);
> >> + clk = parent;
> >> + }
> >> +}
> >> +
> >> +static int intel_lpss_register_clock_divider(struct intel_lpss *lpss,
> >> + const char *devname,
> >> + struct clk **clk)
> >> +{
> >> + char name[32];
> >> + struct clk *tmp = *clk;
> >> +
> >> + snprintf(name, sizeof(name), "%s-enable", devname);
> >> + tmp = clk_register_gate(NULL, name, __clk_get_name(tmp), 0,
> >> + lpss->priv, 0, 0, NULL);
> >> + if (IS_ERR(tmp))
> >> + return PTR_ERR(tmp);
> >> +
> >> + snprintf(name, sizeof(name), "%s-div", devname);
> >> + tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
> >> + 0, lpss->priv, 1, 15, 16, 15, 0,
> >> + NULL);
> >> + if (IS_ERR(tmp))
> >> + return PTR_ERR(tmp);
> >> + *clk = tmp;
> >> +
> >> + snprintf(name, sizeof(name), "%s-update", devname);
> >> + tmp = clk_register_gate(NULL, name, __clk_get_name(tmp),
> >> + CLK_SET_RATE_PARENT, lpss->priv, 31, 0, NULL);
> >> + if (IS_ERR(tmp))
> >> + return PTR_ERR(tmp);
> >> + *clk = tmp;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int intel_lpss_register_clock(struct intel_lpss *lpss)
> >> +{
> >> + const struct mfd_cell *devs = lpss->devs;
> >> + struct clk_lookup **clocks;
> >> + struct clk *clk, *root;
> >> + char devname[24];
> >> + int ret = -ENOMEM;
> >> +
> >> + if (!lpss->info->clk_rate)
> >> + return 0;
> >> +
> >> + clocks = devm_kcalloc(lpss->dev, lpss->ndevs, sizeof(*clocks),
> >> + GFP_KERNEL);
> >> + if (!clocks)
> >> + return -ENOMEM;
> >> +
> >> + /* Root clock */
> >> + root = clk_register_fixed_rate(NULL, dev_name(lpss->dev), NULL,
> >> + CLK_IS_ROOT, lpss->info->clk_rate);
> >> + if (IS_ERR(root))
> >> + return PTR_ERR(root);
> >> +
> >> + clk = root;
> >> +
> >> + snprintf(devname, sizeof(devname), "%s.%d", devs[1].name, lpss->devid);
> >> +
> >> + /*
> >> + * Support for clock divider only if it has some preset value.
> >> + * Otherwise we assume that the divider is not used.
> >> + */
> >> + if (lpss->type != LPSS_DEV_I2C) {
> >> + ret = intel_lpss_register_clock_divider(lpss, devname, &clk);
> >> + if (ret)
> >> + goto err_clk_register;
> >> + }
> >> +
> >> + /* Clock for the host controller */
> >> + clocks[0] = clk_register_clkdev(clk, lpss->info->clk_con_id, "%s",
> >> + devname);
> >> + if (IS_ERR(clocks[0]))
> >> + goto err_clk_register;
> >> +
> >> + /*
> >> + * If the slice has integrated DMA block create clock for that as
> >> + * well now.
> >> + */
> >> + if (lpss->ndevs > 1) {
> >> + clocks[1] = clk_register_clkdev(root, "hclk", "%s.%d",
> >> + devs[0].name, lpss->devid);
> >> + if (IS_ERR(clocks[1]))
> >> + goto err_hclk_register;
> >> + }
> >> +
> >> + lpss->clocks = clocks;
> >> + lpss->clk = clk;
> >> +
> >> + return 0;
> >> +
> >> +err_hclk_register:
> >> + clkdev_drop(clocks[0]);
> >> +
> >> +err_clk_register:
> >> + intel_lpss_unregister_clock_tree(clk);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void intel_lpss_unregister_clock(struct intel_lpss *lpss)
> >> +{
> >> + unsigned int i;
> >> +
> >> + if (IS_ERR_OR_NULL(lpss->clk))
> >> + return;
> >> +
> >> + for (i = 0; i < lpss->ndevs; i++)
> >> + clkdev_drop(lpss->clocks[i]);
> >> +
> >> + intel_lpss_unregister_clock_tree(lpss->clk);
> >> +}
> >> +
> >> +int intel_lpss_probe(struct device *dev,
> >> + const struct intel_lpss_platform_info *info)
> >> +{
> >> + const struct mfd_cell *devs;
> >> + struct intel_lpss *lpss;
> >> + struct resource priv;
> >> + int ret;
> >> +
> >> + if (!info || !info->mem || info->irq <= 0)
> >> + return -EINVAL;
> >> +
> >> + lpss = devm_kzalloc(dev, sizeof(*lpss), GFP_KERNEL);
> >> + if (!lpss)
> >> + return -ENOMEM;
> >> +
> >> + memset(&priv, 0, sizeof(priv));
> >> + priv.start = info->mem->start + LPSS_PRIV_OFFSET;
> >> + priv.end = priv.start + LPSS_PRIV_SIZE - 1;
> >> + priv.flags = IORESOURCE_MEM;
> >> +
> >> + lpss->priv = devm_ioremap_resource(dev, &priv);
> >> + if (!lpss->priv)
> >> + return -ENOMEM;
> >> +
> >> + intel_lpss_reset(lpss);
> >> +
> >> + lpss->info = info;
> >> + lpss->dev = dev;
> >> + lpss->caps = readl(lpss->priv + LPSS_PRIV_CAPS);
> >> + lpss->ndevs = intel_lpss_has_idma(lpss) ? 2 : 1;
> >
> > This is fragile.
> >
> > I'd prefer you dynamically allocate resources here, instead of
> > statically up the top then 'jumping' over the ones you don't need.
>
> (1)
>
> >
> >> + dev_set_drvdata(dev, lpss);
> >> +
> >> + ret = intel_lpss_assign_devs(lpss);
> >> + if (ret < 0)
> >> + return ret;
> >
> > Is ret allowed to be >0? If not just 'if (ret)'.
>
> It might be a leftover when we previously returned a type of the
> device. Will amend.
>
> >
> >> + lpss->devid = ida_simple_get(&intel_lpss_devid_ida, 0, 0, GFP_KERNEL);
> >> + if (lpss->devid < 0)
> >> + return lpss->devid;
> >
> > PLATFORM_DEVID_AUTO does this for you.
>
> What about clock matching?
This is a pretty weird case. Hopefully Mike or Stephen will have a
better idea. Normally the clock handing will be handled by the Clock
Framework and we don't have to worry about these things.
Don't ACPI/PCI have ways to link to clocks in the same way DT does?
> >> + ret = intel_lpss_register_clock(lpss);
> >> + if (ret < 0)
> >> + goto err_clk_register;
> >> +
> >> + intel_lpss_ltr_add(lpss);
> >
> > Add what, to what? What is 'ltr'?
>
> LTR stands for Latency Tolerance Reporting, the PCI property.
>
> https://www.pcisig.com/specifications/pciexpress/specifications/ECN_LatencyTolnReporting_14Aug08.pdf
So what's the _add part? What are you adding and to what?
> >> + ret = intel_lpss_debugfs_add(lpss);
> >> + if (ret)
> >> + dev_warn(lpss->dev, "Failed to create debugfs entries\n");
> >> +
> >> + devs = intel_lpss_has_idma(lpss) ? lpss->devs : lpss->devs + 1;
> >
> > *cringe*
>
> Here the comment also for (1).
>
> In case of DMA IP present on compound device we have to register it
> first, otherwise main device driver may miss that (SPI checks DMA on
> ->probe() stage). Unfortunately there is no possibility to tell MFD
> framework the order of the device initialization except the direct
> order in the array. That's why this solution was implemented. If you
> have any better idea, please, share.
Relying on the order a given set of devices will probe() is fragile.
That's why -EPROBE_DEFER exists.
> >> + ret = mfd_add_devices(dev, lpss->devid, devs, lpss->ndevs,
> >> + info->mem, info->irq, NULL);
> >> + if (ret < 0)
> >> + goto err_remove_ltr;
> >> +
> >> + return 0;
> >> +
> >> +err_remove_ltr:
> >> + intel_lpss_debugfs_remove(lpss);
> >> + intel_lpss_ltr_remove(lpss);
> >> +
> >> +err_clk_register:
> >> + ida_simple_remove(&intel_lpss_devid_ida, lpss->devid);
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(intel_lpss_probe);
> >> +
> >> +void intel_lpss_remove(struct device *dev)
> >> +{
> >> + struct intel_lpss *lpss = dev_get_drvdata(dev);
> >> +
> >> + mfd_remove_devices(dev);
> >> + intel_lpss_debugfs_remove(lpss);
> >> + intel_lpss_ltr_remove(lpss);
> >> + intel_lpss_unregister_clock(lpss);
> >> + ida_simple_rnemove(&intel_lpss_devid_ida, lpss->devid);
Does this even compile?
> >> +}
> >> +EXPORT_SYMBOL_GPL(intel_lpss_remove);
> >> +
> >> +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(dev, NULL, resume_lpss_device);
> >> + 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_reset(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);
> >> + return 0;
> >> +}
> >> +module_init(intel_lpss_init);
> >> +
> >> +static void __exit intel_lpss_exit(void)
> >> +{
> >> + debugfs_remove(intel_lpss_debugfs);
> >> +}
> >> +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..f28cb28
> >> --- /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);
> >
> > Why are you exporting these?
>
> LPSS devices are located in its own power domain. That's why we do all this.
Not sure I get that.
Looking closer it appears that the reason you do it is because you
are registering the power ops in the bus specific (-acpi.c, -pci.c)
probes. What does that have to do with power domains?
> >> +#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
> >
> > SET_SYSTEM_SLEEP_PM_OPS?
> >
> >> +#define INTEL_LPSS_RUNTIME_PM_OPS \
> >> + .runtime_suspend = intel_lpss_suspend, \
> >> + .runtime_resume = intel_lpss_resume,
> >
> > SET_RUNTIME_PM_OPS
> >
> >> +#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 \
> >> +}
> >
> > Why is this in the header?
> >
> >> +#endif /* __MFD_INTEL_LPSS_H */
>
>
>
--
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