Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: "David E. Box" <david.e.box@linux.intel.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Andy Shevchenko <andy@infradead.org>,
	alexander.h.duyck@intel.com,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-pci@vger.kernel.org,
	Alexander Duyck <alexander.h.duyck@linux.intel.com>
Subject: Re: [PATCH 3/3] platform/x86: Intel PMT Telemetry capability driver
Date: Tue, 05 May 2020 14:09:15 -0700
Message-ID: <dabb105d548746f26608c5a19b6b8529cf5529b8.camel@linux.intel.com> (raw)
In-Reply-To: <CAHp75VdnVg7q-Nr-3cO-NyKzk0ckfauOso3yDM4qUF3ofSK_VQ@mail.gmail.com>

On Tue, 2020-05-05 at 16:49 +0300, Andy Shevchenko wrote:
> On Tue, May 5, 2020 at 5:32 AM David E. Box <
> david.e.box@linux.intel.com> wrote:
> 
> ...
> 
> > Register mappings are not provided by the driver. Instead, a GUID
> > is read
> > from a header for each endpoint. The GUID identifies the device and
> > is to
> > be used with an XML, provided by the vendor, to discover the
> > available set
> > of metrics and their register mapping.  This allows firmware
> > updates to
> > modify the register space without needing to update the driver
> > every time
> > with new mappings. Firmware writes a new GUID in this case to
> > specify the
> > new mapping.  Software tools with access to the associated XML file
> > can
> > then interpret the changes.
> 
> Is old hardware going to support this in the future?
> (I have in mind Apollo Lake / Broxton)

I don't know of any plans for this.

> 
> > This module manages access to all PMT Telemetry endpoints on a
> > system,
> > regardless of the device exporting them. It creates an
> > intel_pmt_telem
> 
> Name is not the best we can come up with. Would anyone else use PMT?
> Would it be vendor-agnostic ABI?
> (For example, I know that MIPI standardizes tracing protocols, like
> STM, do we have any plans to standardize this one?)

Not at this time. The technology may be used as a feature on non-Intel
devices, but it is Intel owned. Hence the use of DVSEC which allows
hardware to enumerate and get driver support for IP from other vendors.

> 
> telem -> telemetry.
> 
> > class to manage the list. For each endpoint, sysfs files provide
> > GUID and
> > size information as well as a pointer to the parent device the
> > telemetry
> > comes from. Software may discover the association between endpoints
> > and
> > devices by iterating through the list in sysfs, or by looking for
> > the
> > existence of the class folder under the device of interest.  A
> > device node
> > of the same name allows software to then map the telemetry space
> > for direct
> > access.
> 
> ...
> 
> > +       tristate "Intel PMT telemetry driver"
> 
> I think user should understand what is it from the title (hint: spell
> PMT fully).
> 
> ...
> 
> >  obj-$(CONFIG_PMC_ATOM)                 += pmc_atom.o
> > +obj-$(CONFIG_INTEL_PMT_TELEM)          += intel_pmt_telem.o
> 
> Keep this and Kconfig section in order with the other stuff.
> 
> ...
> 
> bits.h?
> 
> > +#include <linux/cdev.h>
> > +#include <linux/intel-dvsec.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/xarray.h>
> 
> ...
> 
> > +/* platform device name to bind to driver */
> > +#define TELEM_DRV_NAME         "pmt_telemetry"
> 
> Shouldn't be part of MFD header?

Can place in the dvsec header shared by MFD and drivers.

> 
> ...
> 
> > +#define TELEM_TBIR_MASK                0x7
> 
> GENMASK() ?
> 
> > +struct pmt_telem_priv {
> > +       struct device                   *dev;
> > +       struct intel_dvsec_header       *dvsec;
> > +       struct telem_header             header;
> > +       unsigned long                   base_addr;
> > +       void __iomem                    *disc_table;
> > +       struct cdev                     cdev;
> > +       dev_t                           devt;
> > +       int                             devid;
> > +};
> 
> ...
> 
> > +       unsigned long phys = priv->base_addr;
> > +       unsigned long pfn = PFN_DOWN(phys);
> > +       unsigned long psize;
> > +
> > +       psize = (PFN_UP(priv->base_addr + priv->header.size) - pfn)
> > * PAGE_SIZE;
> > +       if (vsize > psize) {
> > +               dev_err(priv->dev, "Requested mmap size is too
> > large\n");
> > +               return -EINVAL;
> > +       }
> 
> ...
> 
> 
> > +static ssize_t guid_show(struct device *dev, struct
> > device_attribute *attr,
> > +                        char *buf)
> > +{
> > +       struct pmt_telem_priv *priv = dev_get_drvdata(dev);
> > +
> > +       return sprintf(buf, "0x%x\n", priv->header.guid);
> > +}
> 
> So, it's not a GUID but rather some custom number? Can we actually do
> a real GUID / UUID here?

I wish but this is the name it was called. We should have pushed back
more on this. My concern now in calling the attribute something
different is that it will not align with public documentation.

...

> 
> > +       /* Local access and BARID only for now */
> > +       switch (priv->header.access_type) {
> > +       case TELEM_ACCESS_LOCAL:
> > +               if (priv->header.tbir) {
> > +                       dev_err(&pdev->dev,
> > +                               "Unsupported BAR index %d for
> > access type %d\n",
> > +                               priv->header.tbir, priv-
> > >header.access_type);
> > +                       return -EINVAL;
> > +               }
> > +               fallthrough;
> 
> What's the point?

The next case has the break. That case is only there to validate that
it's not the default which would be an error. Will switch this to break
though to make it explicit.

Ack on everything else. Thanks.


  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 [this message]
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
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=dabb105d548746f26608c5a19b6b8529cf5529b8.camel@linux.intel.com \
    --to=david.e.box@linux.intel.com \
    --cc=alexander.h.duyck@intel.com \
    --cc=alexander.h.duyck@linux.intel.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=andy@infradead.org \
    --cc=bhelgaas@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@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