Hi Jonathan,
Sorry I didn't see this earlier. I don't follow linux-kernel, so I
didn't see it until it appeared on linux-pci. I don't have time for a
full review right now since the merge window is about to open, but
I'll give you some first impressions to get started.
On Fri, Mar 11, 2016 at 07:26:24AM +0000, Yong, Jonathan wrote:
> Simplified Precision Time Measurement driver, activates PTM feature
> if a PCIe PTM requester (as per PCI Express 3.1 Base Specification
> section 7.32)is found, but not before checking if the rest of the
> PCI hierarchy can support it.
>
> The driver does not take part in facilitating PTM conversations,
> neither does it provide any useful services, it is only responsible
> for setting up the required configuration space bits.
>
> As of writing, there aren't any PTM capable devices on the market
> yet, but it is supported by the Intel Apollo Lake platform.
>
> Signed-off-by: Yong, Jonathan <jonathan.yong@xxxxxxxxx>
> ---
> drivers/pci/pci-sysfs.c | 7 +
> drivers/pci/pci.h | 21 +++
> drivers/pci/pcie/Kconfig | 8 +
> drivers/pci/pcie/Makefile | 2 +-
> drivers/pci/pcie/pcie_ptm.c | 353 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/pci/probe.c | 3 +
> 6 files changed, 393 insertions(+), 1 deletion(-)
> create mode 100644 drivers/pci/pcie/pcie_ptm.c
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 95d9e7b..c634fd11 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1335,6 +1335,9 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> /* Active State Power Management */
> pcie_aspm_create_sysfs_dev_files(dev);
>
> + /* PTM */
Don't add comments like this that merely repeat the code.
> + pci_create_ptm_sysfs(dev);
> +
> if (!pci_probe_reset_function(dev)) {
> retval = device_create_file(&dev->dev, &reset_attr);
> if (retval)
> @@ -1433,6 +1436,10 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
> }
>
> pcie_aspm_remove_sysfs_dev_files(dev);
> +
> + /* PTM */
> + pci_release_ptm_sysfs(dev);
> +
> if (dev->reset_fn) {
> device_remove_file(&dev->dev, &reset_attr);
> dev->reset_fn = 0;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9a1660f..fb90420 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -320,6 +320,27 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
>
> void pci_enable_acs(struct pci_dev *dev);
>
> +#ifdef CONFIG_PCIEPORTBUS
> +int pci_enable_ptm(struct pci_dev *dev);
> +void pci_create_ptm_sysfs(struct pci_dev *dev);
> +void pci_release_ptm_sysfs(struct pci_dev *dev);
> +void pci_disable_ptm(struct pci_dev *dev);
> +#else
> +static inline int pci_enable_ptm(struct pci_dev *dev)
> +{
> + return -ENXIO;
> +}
> +static inline void pci_create_ptm_sysfs(struct pci_dev *dev)
> +{
> +}
> +static inline void pci_release_ptm_sysfs(struct pci_dev *dev)
> +{
> +}
> +static inline void pci_disable_ptm(struct pci_dev *dev)
> +{
> +}
> +#endif
> +
> struct pci_dev_reset_methods {
> u16 vendor;
> u16 device;
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index e294713..f65ff4d 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -80,3 +80,11 @@ endchoice
> config PCIE_PME
> def_bool y
> depends on PCIEPORTBUS && PM
> +
> +config PCIE_PTM
> + bool "Turn on Precision Time Management by default"
> + depends on PCIEPORTBUS
> + help
> + Say Y here to enable PTM feature on PCI Express devices that
> + support them as they are found during device enumeration. Otherwise
> + the feature can be enabled manually through sysfs entries.
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index 00c62df..d18b4c7 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -5,7 +5,7 @@
> # Build PCI Express ASPM if needed
> obj-$(CONFIG_PCIEASPM) += aspm.o
>
> -pcieportdrv-y := portdrv_core.o portdrv_pci.o portdrv_bus.o
> +pcieportdrv-y := portdrv_core.o portdrv_pci.o portdrv_bus.o pcie_ptm.o
> pcieportdrv-$(CONFIG_ACPI) += portdrv_acpi.o
>
> obj-$(CONFIG_PCIEPORTBUS) += pcieportdrv.o
> diff --git a/drivers/pci/pcie/pcie_ptm.c b/drivers/pci/pcie/pcie_ptm.c
> new file mode 100644
> index 0000000..a128c79
> --- /dev/null
> +++ b/drivers/pci/pcie/pcie_ptm.c
> @@ -0,0 +1,353 @@
> +/*
> + * PCI Express Precision Time Measurement
> + * Copyright (c) 2016, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include "../pci.h"
> +
> +#define PCI_PTM_REQ 0x0001 /* Requester capable */
> +#define PCI_PTM_RSP 0x0002 /* Responder capable */
> +#define PCI_PTM_ROOT 0x0004 /* Root capable */
> +#define PCI_PTM_GRANULITY 0xFF00 /* Local clock granulity */
s/granulity/granularity/
> +#define PCI_PTM_ENABLE 0x0001 /* PTM enable */
> +#define PCI_PTM_ROOT_SEL 0x0002 /* Root select */
> +
> +#define PCI_PTM_HEADER_REG_OFFSET 0x00
> +#define PCI_PTM_CAPABILITY_REG_OFFSET 0x04
> +#define PCI_PTM_CONTROL_REG_OFFSET 0x08
> +
> +#define PCI_EXT_CAP_ID_PTM 0x001f
These should go in include/uapi/linux/pci_regs.h along with the other
PCI capability definitions. Obviously they should match the style of
the existing code there.
> +#ifdef CONFIG_PCIE_PTM
> +static bool disable_ptm;
> +#else
> +static bool disable_ptm = 1;
> +#endif
> +
> +module_param_named(disable_ptm, disable_ptm, bool, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(disable_ptm,
> + "Don't automatically enable even if supported.");
> +
> +static inline u8 get_granularity(u32 in)
> +{
> + return (in & PCI_PTM_GRANULITY) >> 8;
> +}
> +
> +static ssize_t ptm_status_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + u16 word;
> + int pos;
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PTM);
> + if (!pos)
> + return -ENXIO;
> +
> + pci_read_config_word(pdev, pos + PCI_PTM_CONTROL_REG_OFFSET, &word);
> + return sprintf(buf, "%u\n", word & PCI_PTM_ENABLE);
> +}
> +
> +static ssize_t ptm_status_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> + unsigned long val;
> + ssize_t ret;
> + int pos;
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PTM);
> + if (!pos)
> + return -ENXIO;
> +
> + ret = kstrtoul(buf, 0, &val);
> + if (ret)
> + return ret;
> + if (val)
> + return pci_enable_ptm(pdev);
> + pci_disable_ptm(pdev);
> + return 0;
> +}
> +
> +static DEVICE_ATTR_RW(ptm_status);
> +
> +void pci_release_ptm_sysfs(struct pci_dev *dev)
> +{
> + if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM))
> + return;
> + device_remove_file(&dev->dev, &dev_attr_ptm_status);
> +}
> +
> +void pci_create_ptm_sysfs(struct pci_dev *dev)
> +{
> + if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM))
> + return;
> + device_create_file(&dev->dev, &dev_attr_ptm_status);
> +}
> +
> +/**
> + * pci_enable_ptm - Try to activate PTM functionality on device.
> + * @dev: PCI Express device with PTM requester role to enable.
> + *
> + * The function will crawl through the PCI Hierarchy to determine if it is
> + * possible to enable the Precision Time Measurement requester role on @dev,
> + * and if so, activate it by setting the granularity field.
> + *
> + * NOTE: Each requester must be associated with a PTM root (not to be confused
> + * with a root port or root complex). There can be multiple PTM roots in a
> + * a system forming multiple domains. All intervening bridges/switches in a
> + * domain must support PTM responder roles to relay PTM dialogues.
> + */
> +int pci_enable_ptm(struct pci_dev *dev)
> +{
This function is way too long to review as it is. Can you split it up
somehow?
> + struct pci_dev *curr, **steps;
> + size_t i = 0, root = 0;
> + int pos, pos2;
> + u8 granularity = 0;
> + u16 word;
> + int ret = 0;
> +
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
> + if (!pos) {
> + dev_dbg(&dev->dev, "Not PTM capable, skipping.\n");
> + return -ENXIO;
> + }
> +
> + if (disable_ptm)
> + return 0;
> +
> + /* Skip if not requester role. */
> + pci_read_config_word(dev, pos + PCI_PTM_CAPABILITY_REG_OFFSET, &word);
> + if (!(word & PCI_PTM_REQ)) {
> + dev_dbg(&dev->dev, "Not a PTM requester, skipping for now.\n");
> + return 0;
> + }
> +
> + /* Just copy and enable PTM granularity for integrated endpoints. */
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) {
> + dev_dbg(&dev->dev,
> + "Root integrated endpoint, attempting to copy root granularity.\n");
> + curr = pci_upstream_bridge(dev);
> + if (!curr)
> + return 0;
> +
> + pos2 = pci_find_ext_capability(curr, PCI_EXT_CAP_ID_PTM);
> + if (!pos2)
> + return 0;
> +
> + /* Get granularity field. */
> + pci_read_config_word(curr,
> + pos2 + PCI_PTM_CAPABILITY_REG_OFFSET,
> + &word);
> + word &= PCI_PTM_GRANULITY;
> +
> + /* Copy it over. */
> + word |= PCI_PTM_ENABLE;
> + pci_write_config_word(dev, pos + PCI_PTM_CONTROL_REG_OFFSET,
> + word);
> +
> + /* Enable PTM on root complex if not already so. */
> + pci_read_config_word(curr, pos2 + PCI_PTM_CONTROL_REG_OFFSET,
> + &word);
> + if (!(word & PCI_PTM_ENABLE)) {
> + pci_read_config_word(curr,
> + pos2 + PCI_PTM_CONTROL_REG_OFFSET, &word);
> + word |= PCI_PTM_ENABLE | PCI_PTM_ROOT_SEL;
> + pci_write_config_word(curr,
> + pos2 + PCI_PTM_CONTROL_REG_OFFSET, word);
> + }
> + } else {
> + /* For holding all the bridge/switches in between. */
> + steps = kzalloc(sizeof(*steps) * dev->bus->number + 1,
> + GFP_KERNEL);
> + if (!steps)
> + return -ENOMEM;
> +
> + /* Gather all the upstream devices. */
> + curr = pci_upstream_bridge(dev);
> + if (curr) {
> + dev_dbg(&dev->dev,
> + "Upstream is %d:%d:%x.%d\n",
> + pci_domain_nr(curr->bus),
> + curr->bus->number,
> + PCI_SLOT(curr->devfn),
> + PCI_FUNC(curr->devfn)
> + );
> + } else {
> + dev_dbg(&dev->dev, "No upstream??\n");
> + ret = -ENXIO;
> + goto abort;
> + }
> +
> + do {
> + /* sanity check */
> + if (i > dev->bus->number)
> + break;
> + steps[i++] = curr;
> + } while ((curr = pci_upstream_bridge(curr)));
> +
> + /* Check upstream chains capability. */
> + dev_dbg(&dev->dev,
> + "Checking hierarchy capabilities\n");
> + for (i = 0; i < dev->bus->number + 1; i++) {
> + curr = steps[i];
> + if (!curr)
> + break;
> +
> + pos2 = pci_find_ext_capability(curr,
> + PCI_EXT_CAP_ID_PTM);
> +
> + if (!pos2) {
> + dev_dbg(&curr->dev,
> + "PTM Hierarchy %zx: not PTM aware\n",
> + i);
> + break;
> + }
> +
> + /* End if upstream cannot respond. */
> + pci_read_config_word(curr,
> + pos2 + PCI_PTM_CAPABILITY_REG_OFFSET, &word);
> + if (!(word & PCI_PTM_RSP)) {
> + dev_dbg(&curr->dev,
> + "PTM Hierarchy: skipping non-responder\n");
> + break;
> + }
> +
> + /* Is root capable? */
> + if (word & PCI_PTM_ROOT) {
> + root = i;
> + granularity = get_granularity(word);
> + }
> + }
> +
> + if (!steps[root]) {
> + dev_dbg(&dev->dev, "Cannot find root, aborting\n");
> + ret = -ENXIO;
> + goto abort;
> + }
> +
> + dev_dbg(&dev->dev,
> + "Found PTM root at %d:%d:%x.%d granularity %u\n",
> + pci_domain_nr(steps[root]->bus),
> + steps[root]->bus->number,
> + PCI_SLOT(steps[root]->devfn),
> + PCI_FUNC(steps[root]->devfn),
> + granularity);
> +
> + /* Program granularity field. */
> + for (i = root;;) {
> + curr = steps[i];
> + pos2 = pci_find_ext_capability(curr,
> + PCI_EXT_CAP_ID_PTM);
> + pci_read_config_word(curr,
> + pos2 + PCI_PTM_CONTROL_REG_OFFSET, &word);
> +
> + /* If not yet PTM enabled. */
> + if (!(word & PCI_PTM_ENABLE)) {
> + pci_read_config_word(curr,
> + pos2 + PCI_PTM_CAPABILITY_REG_OFFSET,
> + &word);
> + /* If requester capable, program granularity. */
> + if (word & PCI_PTM_REQ) {
> + dev_dbg(&curr->dev,
> + "Programming granularity %u\n",
> + granularity);
> + pci_write_config_word(curr,
> + pos2 +
> + PCI_PTM_CONTROL_REG_OFFSET,
> + ((u16)granularity) << 8);
> + }
> + if ((word & PCI_PTM_ROOT) &&
> + granularity != 0 &&
> + ((granularity < get_granularity(word))
> + || get_granularity(word) == 0)) {
> + dev_dbg(&curr->dev,
> + "Updating granularity %u to %u\n",
> + granularity,
> + get_granularity(word));
> + granularity = get_granularity(word);
> + }
> + }
> + if (!i)
> + break;
> + i--;
> + }
> +
> + /* Program current device granularity and enable it. */
> + pci_read_config_word(dev, pos + PCI_PTM_CONTROL_REG_OFFSET,
> + &word);
> + word = (word & ~PCI_PTM_GRANULITY) | ((u16)granularity) << 8
> + | PCI_PTM_ENABLE;
> + pci_write_config_word(dev, pos + PCI_PTM_CONTROL_REG_OFFSET,
> + word);
> + dev_dbg(&dev->dev,
> + "Using granularity %u, %x\n", granularity,
> + word);
> +
> + /* Enable PTM root. */
> + pos2 = pci_find_ext_capability(steps[root],
> + PCI_EXT_CAP_ID_PTM);
> + pci_read_config_word(steps[root],
> + pos2 + PCI_PTM_CONTROL_REG_OFFSET, &word);
> + word |= PCI_PTM_ROOT_SEL | PCI_PTM_ENABLE;
> + pci_write_config_word(steps[root],
> + pos2 + PCI_PTM_CONTROL_REG_OFFSET, word);
> +
> + /* PTM enable from the bottom up. */
> + for (i = 0; i <= root; i++) {
> + pos2 = pci_find_ext_capability(steps[i],
> + PCI_EXT_CAP_ID_PTM);
> + pci_read_config_word(steps[i],
> + pos2 + PCI_PTM_CONTROL_REG_OFFSET, &word);
> + word |= PCI_PTM_ENABLE;
> + pci_write_config_word(steps[i],
> + pos2 + PCI_PTM_CONTROL_REG_OFFSET,
> + word);
> + }
I haven't read the PTM spec yet so I don't know how it works. But in
general, I don't like having to tweak a setting all the way up the
hierarchy based on a leaf device. That makes it hard to handle
hotplug correctly, because obviously there may be many leaf devices
that share part of all of the upstream path.
> +abort:
> + kfree(steps);
> + }
> + return ret;
> +}
> +
> +static int do_disable_ptm(struct pci_dev *dev, void *v)
> +{
> + int pos;
> + u16 word;
> +
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
> + if (!pos)
> + return 0;
> +
> + pci_read_config_word(dev, pos + PCI_PTM_CONTROL_REG_OFFSET, &word);
> + word &= ~PCI_PTM_ENABLE;
> + pci_write_config_word(dev, pos + PCI_PTM_CONTROL_REG_OFFSET, word);
> + return 0;
> +}
> +
> +/**
> + * pci_disable_ptm - Turn off PTM functionality on device.
> + * @dev: PCI Express device with PTM function to disable.
> + *
> + * Disables PTM functionality by clearing the PTM enable bit, if device is a
> + * switch/bridge it will also disable PTM function on other devices behind it.
> + */
> +void pci_disable_ptm(struct pci_dev *dev)
> +{
> + if (pci_is_bridge(dev))
> + pci_walk_bus(dev->bus, &do_disable_ptm, NULL);
> + else
> + do_disable_ptm(dev, NULL);
> +}
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6d7ab9b..8dc8bbc 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1623,6 +1623,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
> pci_enable_acs(dev);
>
> pci_cleanup_aer_error_status_regs(dev);
> +
> + /* Enable PTM Capabilities */
Another superfluous comment. No need to repeat what the code already
says.
> + pci_enable_ptm(dev);
> }
>
> /*
> --
> 2.4.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
