| 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: <CAHp75Ve-7eNQFUz3i8Ye2DFn30M0gTNRcsFrL=q7nQaY_+iZcg@mail.gmail.com>
Date: Wed, 29 Apr 2015 16:58:30 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Lee Jones <lee.jones@...aro.org>
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, 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.
>
>> 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.
>
>> 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'.
>
>> +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?
>
>> +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.
>
>> +{
>> + 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?
>
>> +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?
>
>> + 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
>
>> + 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.
>
>> + 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);
>> +}
>> +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.
>
>> +#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 */
--
With Best Regards,
Andy Shevchenko
--
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