Message-ID: <5757b1989f2acff2f3e9b9a9e595e5cc54da1958.camel@linux.intel.com>
Date:   Tue, 28 Jul 2020 13:35:33 -0700
From:   "David E. Box" <david.e.box@...ux.intel.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     dvhart@...radead.org, andy@...radead.org, bhelgaas@...gle.com,
        alexander.h.duyck@...ux.intel.com, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, linux-pci@...r.kernel.org,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH V4 2/3] mfd: Intel Platform Monitoring Technology support

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@...il.com>
> > Co-developed-by: Alexander Duyck <alexander.h.duyck@...ux.intel.com
> > >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@...ux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@...ux.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@...ux.intel.com>
> > +S:	Maintained
> > +F:	drivers/mfd/intel_pmt.c
> > +
> >  INTEL PRO/WIRELESS 2100, 2200BG, 2915ABG NETWORK CONNECTION
> > SUPPORT
> >  M:	Stanislav Yakovlev <stas.yakovlev@...il.com>
> >  L:	linux-wireless@...r.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@...ux.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@...ux.intel.com>");
> > +MODULE_DESCRIPTION("Intel Platform Monitoring Technology MFD
> > driver");
> 
> s/MFD/(PMT)/
> 
> > +MODULE_LICENSE("GPL v2");

Powered by blists - more mailing lists