Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: "David E. Box" <david.e.box@linux.intel.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: dvhart@infradead.org, andy@infradead.org, bhelgaas@google.com,
	alexander.h.duyck@linux.intel.com, linux-kernel@vger.kernel.org,
	platform-driver-x86@vger.kernel.org, linux-pci@vger.kernel.org,
	Andy Shevchenko <andy.shevchenko@gmail.com>
Subject: Re: [PATCH V4 2/3] mfd: Intel Platform Monitoring Technology support
Date: Tue, 28 Jul 2020 13:35:33 -0700
Message-ID: <5757b1989f2acff2f3e9b9a9e595e5cc54da1958.camel@linux.intel.com> (raw)
In-Reply-To: <20200728075859.GH1850026@dell>

Hi Lee,

Thanks for this thorough review. Ack on all the comments with
particular thanks for spoting the missing continue.

David

On Tue, 2020-07-28 at 08:58 +0100, Lee Jones wrote:
> On Fri, 17 Jul 2020, David E. Box wrote:
> 
> > Intel Platform Monitoring Technology (PMT) is an architecture for
> > enumerating and accessing hardware monitoring facilities. PMT
> > supports
> > multiple types of monitoring capabilities. This driver creates
> > platform
> > devices for each type so that they may be managed by capability
> > specific
> > drivers (to be introduced). Capabilities are discovered using PCIe
> > DVSEC
> > ids. Support is included for the 3 current capability types,
> > Telemetry,
> > Watcher, and Crashlog. The features are available on new Intel
> > platforms
> > starting from Tiger Lake for which support is added.
> > 
> > Also add a quirk mechanism for several early hardware differences
> > and bugs.
> > For Tiger Lake, do not support Watcher and Crashlog capabilities
> > since they
> > will not be compatible with future product. Also, fix use a quirk
> > to fix
> > the discovery table offset.
> > 
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Co-developed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com
> > >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> 
> This should be in chronological order.
> 
> > ---
> >  MAINTAINERS             |   5 +
> >  drivers/mfd/Kconfig     |  10 ++
> >  drivers/mfd/Makefile    |   1 +
> >  drivers/mfd/intel_pmt.c | 215
> > ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 231 insertions(+)
> >  create mode 100644 drivers/mfd/intel_pmt.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b4a43a9e7fbc..2e42bf0c41ab 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8845,6 +8845,11 @@ F:	drivers/mfd/intel_soc_pmic*
> >  F:	include/linux/mfd/intel_msic.h
> >  F:	include/linux/mfd/intel_soc_pmic*
> >  
> > +INTEL PMT DRIVER
> > +M:	"David E. Box" <david.e.box@linux.intel.com>
> > +S:	Maintained
> > +F:	drivers/mfd/intel_pmt.c
> > +
> >  INTEL PRO/WIRELESS 2100, 2200BG, 2915ABG NETWORK CONNECTION
> > SUPPORT
> >  M:	Stanislav Yakovlev <stas.yakovlev@gmail.com>
> >  L:	linux-wireless@vger.kernel.org
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index a37d7d171382..1a62ce2c68d9 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -670,6 +670,16 @@ config MFD_INTEL_PMC_BXT
> >  	  Register and P-unit access. In addition this creates devices
> >  	  for iTCO watchdog and telemetry that are part of the PMC.
> >  
> > +config MFD_INTEL_PMT
> > +	tristate "Intel Platform Monitoring Technology support"
> 
> Nit: "Intel Platform Monitoring Technology (PMT) support"
> 
> > +	depends on PCI
> > +	select MFD_CORE
> > +	help
> > +	  The Intel Platform Monitoring Technology (PMT) is an
> > interface that
> > +	  provides access to hardware monitor registers. This driver
> > supports
> > +	  Telemetry, Watcher, and Crashlog PMT capabilities/devices for
> > +	  platforms starting from Tiger Lake.
> > +
> >  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 9367a92f795a..1961b4737985 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -216,6 +216,7 @@ 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_INTEL_PMT)	+= intel_pmt.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_pmt.c b/drivers/mfd/intel_pmt.c
> > new file mode 100644
> > index 000000000000..6857eaf4ff86
> > --- /dev/null
> > +++ b/drivers/mfd/intel_pmt.c
> > @@ -0,0 +1,215 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel Platform Monitoring Technology MFD driver
> 
> s/MFD/(PMT)/
> 
> > + * Copyright (c) 2020, Intel Corporation.
> > + * All Rights Reserved.
> > + *
> > + * Authors: David E. Box <david.e.box@linux.intel.com>
> 
> Looks odd to use a plural for a single author.
> 
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/types.h>
> 
> Alphabetical please.
> 
> > +/* Intel DVSEC capability vendor space offsets */
> > +#define INTEL_DVSEC_ENTRIES		0xA
> > +#define INTEL_DVSEC_SIZE		0xB
> > +#define INTEL_DVSEC_TABLE		0xC
> > +#define INTEL_DVSEC_TABLE_BAR(x)	((x) & GENMASK(2, 0))
> > +#define INTEL_DVSEC_TABLE_OFFSET(x)	((x) & GENMASK(31, 3))
> > +#define INTEL_DVSEC_ENTRY_SIZE		4
> > +
> > +/* PMT capabilities */
> > +#define DVSEC_INTEL_ID_TELEMETRY	2
> > +#define DVSEC_INTEL_ID_WATCHER		3
> > +#define DVSEC_INTEL_ID_CRASHLOG		4
> > +
> > +#define TELEMETRY_DEV_NAME		"pmt_telemetry"
> > +#define WATCHER_DEV_NAME		"pmt_watcher"
> > +#define CRASHLOG_DEV_NAME		"pmt_crashlog"
> 
> Please don't define names of things.  It makes grepping a pain, at
> the
> very least.  Just use the 'raw' string in-place.
> 
> > +struct intel_dvsec_header {
> > +	u16	length;
> > +	u16	id;
> > +	u8	num_entries;
> > +	u8	entry_size;
> > +	u8	tbir;
> > +	u32	offset;
> > +};
> > +
> > +enum pmt_quirks {
> > +	/* Watcher capability not supported */
> > +	PMT_QUIRK_NO_WATCHER	= BIT(0),
> > +
> > +	/* Crashlog capability not supported */
> > +	PMT_QUIRK_NO_CRASHLOG	= BIT(1),
> > +
> > +	/* Use shift instead of mask to read discovery table offset */
> > +	PMT_QUIRK_TABLE_SHIFT	= BIT(2),
> > +};
> > +
> > +struct pmt_platform_info {
> > +	unsigned long quirks;
> > +};
> > +
> > +static const struct pmt_platform_info tgl_info = {
> > +	.quirks = PMT_QUIRK_NO_WATCHER | PMT_QUIRK_NO_CRASHLOG |
> > +		  PMT_QUIRK_TABLE_SHIFT,
> > +};
> > +
> > +static int
> > +pmt_add_dev(struct pci_dev *pdev, struct intel_dvsec_header
> > *header,
> > +	    struct pmt_platform_info *info)
> 
> My personal preference is to a) only break when you have to and b) to
> align with the '('.  Perhaps point b) is satisfied and it's just the
> patch format that's shifting the tab though?
> 
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *res, *tmp;
> > +	struct mfd_cell *cell;
> > +	const char *name;
> > +	int count = header->num_entries;
> > +	int size = header->entry_size;
> > +	int i;
> > +
> > +	switch (header->id) {
> > +	case DVSEC_INTEL_ID_TELEMETRY:
> > +		name = TELEMETRY_DEV_NAME;
> > +		break;
> > +	case DVSEC_INTEL_ID_WATCHER:
> > +		if (info->quirks & PMT_QUIRK_NO_WATCHER) {
> > +			dev_info(dev, "Watcher not supported\n");
> > +			return 0;
> > +		}
> > +		name = WATCHER_DEV_NAME;
> > +		break;
> > +	case DVSEC_INTEL_ID_CRASHLOG:
> > +		if (info->quirks & PMT_QUIRK_NO_CRASHLOG) {
> > +			dev_info(dev, "Crashlog not supported\n");
> > +			return 0;
> > +		}
> > +		name = CRASHLOG_DEV_NAME;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> 
> Doesn't deserve an error message?
> 
> > +	}
> > +
> > +	if (!header->num_entries || !header->entry_size) {
> > +		dev_warn(dev, "Invalid count or size for %s header\n",
> > name);
> > +		return -EINVAL;
> 
> If you're returning an error, this should be dev_err().
> 
> Even if you only handle it as a warning at the call site.
> 
> > +	}
> > +
> > +	cell = devm_kzalloc(dev, sizeof(*cell), GFP_KERNEL);
> > +	if (!cell)
> > +		return -ENOMEM;
> > +
> > +	res = devm_kcalloc(dev, count, sizeof(*res), GFP_KERNEL);
> > +	if (!res)
> > +		return -ENOMEM;
> > +
> > +	if (info->quirks & PMT_QUIRK_TABLE_SHIFT)
> > +		header->offset >>= 3;
> > +
> > +	for (i = 0, tmp = res; i < count; i++, tmp++) {
> > +		tmp->start = pdev->resource[header->tbir].start +
> > +			     header->offset + i * (size << 2);
> 
> Deserves a comment I think.
> 
> > +		tmp->end = tmp->start + (size << 2) - 1;
> > +		tmp->flags = IORESOURCE_MEM;
> > +	}
> > +
> > +	cell->resources = res;
> > +	cell->num_resources = count;
> > +	cell->name = name;
> > +
> > +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cell, 1,
> > NULL, 0,
> > +				    NULL);
> > +}
> > +
> > +static int
> > +pmt_pci_probe(struct pci_dev *pdev, const struct pci_device_id
> > *id)
> > +{
> > +	struct intel_dvsec_header header;
> > +	struct pmt_platform_info *info;
> > +	bool found_devices = false;
> > +	int ret, pos = 0;
> > +	u32 table;
> > +	u16 vid;
> > +
> > +	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;
> > +
> > +	pos = pci_find_next_ext_capability(pdev, pos,
> > PCI_EXT_CAP_ID_DVSEC);
> > +	while (pos) {
> 
> If you do:
> 
> 	do {
> 		int pos;
> 
> 		pos = pci_find_next_ext_capability(pdev, pos,
> PCI_EXT_CAP_ID_DVSEC);
> 		if (!pos)
> 			break;
> 
> Then you can invoke pci_find_next_ext_capability() once, no?
> 
> > +		pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1,
> > &vid);
> > +		if (vid != PCI_VENDOR_ID_INTEL)
> > +			continue;
> > +
> > +		pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER2,
> > +				     &header.id);
> > +		pci_read_config_byte(pdev, pos + INTEL_DVSEC_ENTRIES,
> > +				     &header.num_entries);
> > +		pci_read_config_byte(pdev, pos + INTEL_DVSEC_SIZE,
> > +				     &header.entry_size);
> > +		pci_read_config_dword(pdev, pos + INTEL_DVSEC_TABLE,
> > +				      &table);
> > +
> > +		header.tbir = INTEL_DVSEC_TABLE_BAR(table);
> > +		header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
> > +
> > +		ret = pmt_add_dev(pdev, &header, info);
> > +		if (ret)
> > +			dev_warn(&pdev->dev,
> > +				 "Failed to add devices for DVSEC id
> > %d\n",
> 
> "device", so not all devices, right?
> 
> > +				 header.id);
> 
> Don't you want to continue here?
> 
> Else you're going to set found_devices for a failed device.
> 
> > +		found_devices = true;
> > +
> > +		pos = pci_find_next_ext_capability(pdev, pos,
> > +						   PCI_EXT_CAP_ID_DVSEC
> > );
> > +	}
> > +
> > +	if (!found_devices) {
> > +		dev_err(&pdev->dev, "No supported PMT capabilities
> > found.\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	pm_runtime_put(&pdev->dev);
> > +	pm_runtime_allow(&pdev->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static void pmt_pci_remove(struct pci_dev *pdev)
> > +{
> > +	pm_runtime_forbid(&pdev->dev);
> > +	pm_runtime_get_sync(&pdev->dev);
> > +}
> > +
> > +#define PCI_DEVICE_ID_INTEL_PMT_TGL	0x9a0d
> 
> What's this for?
> 
> If this is PCI_DEVICE_DATA magic, it would be worth tying it to the
> struct i.e. remove the empty line between it and the table below.
> 
> > +static const struct pci_device_id pmt_pci_ids[] = {
> > +	{ PCI_DEVICE_DATA(INTEL, PMT_TGL, &tgl_info) },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(pci, pmt_pci_ids);
> > +
> > +static struct pci_driver pmt_pci_driver = {
> > +	.name = "intel-pmt",
> > +	.id_table = pmt_pci_ids,
> > +	.probe = pmt_pci_probe,
> > +	.remove = pmt_pci_remove,
> > +};
> > +module_pci_driver(pmt_pci_driver);
> > +
> > +MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
> > +MODULE_DESCRIPTION("Intel Platform Monitoring Technology MFD
> > driver");
> 
> s/MFD/(PMT)/
> 
> > +MODULE_LICENSE("GPL v2");


  reply index

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200505013206.11223-1-david.e.box@linux.intel.com>
2020-05-05  1:32 ` [PATCH 1/3] pci: Add Designated Vendor Specific Capability David E. Box
2020-05-05  8:49   ` Andy Shevchenko
2020-05-05 15:00     ` David E. Box
2020-05-05 16:34   ` Bjorn Helgaas
2020-05-05  2:31 ` [PATCH 2/3] mfd: Intel Platform Monitoring Technology support David E. Box
2020-05-05  2:31   ` [PATCH 3/3] platform/x86: Intel PMT Telemetry capability driver David E. Box
2020-05-05 13:49     ` Andy Shevchenko
2020-05-05 21:09       ` David E. Box
2020-05-08  2:33       ` David E. Box
2020-05-05  2:53   ` [PATCH 2/3] mfd: Intel Platform Monitoring Technology support Randy Dunlap
2020-05-05 14:55     ` David E. Box
2020-05-05  9:02   ` Andy Shevchenko
2020-05-05 15:15     ` David E. Box
2020-05-08  2:18 ` [PATCH v2 0/3] Intel Platform Monitoring Technology David E. Box
2020-05-08  9:59   ` Andy Shevchenko
2020-07-14  6:23   ` [PATCH V3 " David E. Box
2020-07-17 19:06     ` [PATCH V4 " David E. Box
2020-07-27 10:23       ` Andy Shevchenko
2020-07-27 16:29         ` David E. Box
2020-07-29 21:37       ` [PATCH V5 " David E. Box
2020-08-10 14:15         ` David E. Box
2020-08-10 14:42           ` Umesh A
2020-08-11  8:04           ` Lee Jones
2020-08-11 14:50             ` David E. Box
2020-07-29 21:37       ` [PATCH V5 1/3] PCI: Add defines for Designated Vendor-Specific Extended Capability David E. Box
2020-07-29 21:37       ` [PATCH V5 2/3] mfd: Intel Platform Monitoring Technology support David E. Box
2020-07-29 21:37       ` [PATCH V5 3/3] platform/x86: Intel PMT Telemetry capability driver David E. Box
2020-07-17 19:06     ` [PATCH V4 1/3] PCI: Add defines for Designated Vendor-Specific Extended Capability David E. Box
2020-07-17 20:11       ` Andy Shevchenko
2020-07-17 19:06     ` [PATCH V4 2/3] mfd: Intel Platform Monitoring Technology support David E. Box
2020-07-28  7:58       ` Lee Jones
2020-07-28 20:35         ` David E. Box [this message]
2020-07-29 22:59         ` Mark D Rustad
2020-07-30 17:53           ` David E. Box
2020-07-31  6:19           ` Lee Jones
2020-07-17 19:06     ` [PATCH V4 3/3] platform/x86: Intel PMT Telemetry capability driver David E. Box
2020-07-14  6:23   ` [PATCH V3 1/3] PCI: Add defines for Designated Vendor-Specific Capability David E. Box
2020-07-14  8:40     ` Andy Shevchenko
2020-07-16  2:55     ` Randy Dunlap
2020-07-16 15:07       ` Bjorn Helgaas
2020-07-16 15:07         ` Randy Dunlap
2020-07-16 17:18       ` Alexander Duyck
2020-07-16 18:31         ` David E. Box
2020-07-14  6:23   ` [PATCH V3 2/3] mfd: Intel Platform Monitoring Technology support David E. Box
2020-07-14  6:23   ` [PATCH V3 3/3] platform/x86: Intel PMT Telemetry capability driver David E. Box
2020-07-14  8:51     ` Andy Shevchenko
2020-07-15  7:39     ` Alexey Budankov
2020-07-15 23:59       ` David E. Box
2020-07-16  5:57         ` Alexey Budankov
2020-07-16  2:57     ` Randy Dunlap
2020-05-08  2:18 ` [PATCH v2 1/3] PCI: Add defines for Designated Vendor-Specific Capability David E. Box
2020-05-08  2:18 ` [PATCH v2 2/3] mfd: Intel Platform Monitoring Technology support David E. Box
2020-05-08  9:15   ` Andy Shevchenko
2020-05-08  2:18 ` [PATCH v2 3/3] platform/x86: Intel PMT Telemetry capability driver David E. Box
2020-05-08  9:57   ` Andy Shevchenko
2020-05-09 16:27     ` David E. Box

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5757b1989f2acff2f3e9b9a9e595e5cc54da1958.camel@linux.intel.com \
    --to=david.e.box@linux.intel.com \
    --cc=alexander.h.duyck@linux.intel.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=andy@infradead.org \
    --cc=bhelgaas@google.com \
    --cc=dvhart@infradead.org \
    --cc=lee.jones@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git