Re: [PATCH 3/3] platform/x86: Intel PMT Telemetry capability driver
On Tue, 2020-05-05 at 16:49 +0300, Andy Shevchenko wrote: > ... > > > + /* TODO: replace with device properties??? */ > > So, please, fulfill. swnode I guess is what you are looking for. I kept the platform data in v2 because swnode properties doesn't look like a good fit. We are only passing information that was read from the pci device. It is not hard coded, platform specific data. David
Re: [PATCH 3/3] platform/x86: Intel PMT Telemetry capability driver
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@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
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> ...
>
> > +/* 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_MASK0x7
>
> 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(>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.
Re: [PATCH 3/3] platform/x86: Intel PMT Telemetry capability driver
On Tue, May 5, 2020 at 5:32 AM David E. Box 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)
> 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?)
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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
...
> +/* platform device name to bind to driver */
> +#define TELEM_DRV_NAME "pmt_telemetry"
Shouldn't be part of MFD header?
...
> +#define TELEM_TBIR_MASK0x7
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?
Because of TODO below I suppose it's not carved in stone (yet) and
basically a protocol defined by firmware (which can be amended).
...
> + /* TODO: replace with device properties??? */
So, please, fulfill. swnode I guess is what you are looking for.
> + priv->dvsec = dev_get_platdata(>dev);
> + if (!priv->dvsec) {
> + dev_err(>dev, "Platform data not found\n");
> + return -ENODEV;
> + }
...
> + /* Local access and BARID only for now */
> + switch (priv->header.access_type) {
> + case TELEM_ACCESS_LOCAL:
> + if (priv->header.tbir) {
> + dev_err(>dev,
> + "Unsupported BAR index %d for access type
> %d\n",
> + priv->header.tbir, priv->header.access_type);
> + return -EINVAL;
> + }
> + fallthrough;
What's the point?
> +
> + case TELEM_ACCESS_BARID:
> + break;
> + default:
> + dev_err(>dev, "Unsupported access type %d\n",
> + priv->header.access_type);
> + return -EINVAL;
> + }
> + err = alloc_chrdev_region(>devt, 0, 1, TELEM_DRV_NAME);
err or ret? Be consistent in the module.
> + if (err < 0) {
' < 0' Do we need it?
> + dev_err(>dev,
> + "PMT telemetry chrdev_region err: %d\n", err);
> + return err;
> + }
...
> + err = pmt_telem_create_dev(priv);
> + if (err < 0)
' < 0' Do we need it?
[PATCH 3/3] platform/x86: Intel PMT Telemetry capability driver
PMT Telemetry is a capability of the Intel Platform Monitoring Technology. The Telemetry capability provides access to device telemetry metrics that provide hardware performance data to users from continuous, memory mapped, read-only register spaces. 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. This module manages access to all PMT Telemetry endpoints on a system, regardless of the device exporting them. It creates an intel_pmt_telem 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. Signed-off-by: David E. Box Signed-off-by: Alexander Duyck --- .../ABI/testing/sysfs-class-intel_pmt_telem | 46 +++ MAINTAINERS | 1 + drivers/platform/x86/Kconfig | 10 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_pmt_telem.c| 356 ++ drivers/platform/x86/intel_pmt_telem.h| 20 + 6 files changed, 434 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-intel_pmt_telem create mode 100644 drivers/platform/x86/intel_pmt_telem.c create mode 100644 drivers/platform/x86/intel_pmt_telem.h diff --git a/Documentation/ABI/testing/sysfs-class-intel_pmt_telem b/Documentation/ABI/testing/sysfs-class-intel_pmt_telem new file mode 100644 index ..cdd9a16b31f3 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-intel_pmt_telem @@ -0,0 +1,46 @@ +What: /sys/class/intel_pmt_telem/ +Date: April 2020 +KernelVersion: 5.8 +Contact: David Box +Description: + The intel_pmt_telem/ class directory contains information for + devices that expose hardware telemetry using Intel Platform + Monitoring Technology (PMT) + +What: /sys/class/intel_pmt_telem/telemX +Date: April 2020 +KernelVersion: 5.8 +Contact: David Box +Description: + The telemX directory contains files describing an instance of a + PMT telemetry device that exposes hardware telemetry. Each + telemX device has an associated /dev/telemX node. This node can + be opened and mapped to access the telemetry space of the + device. The register layout of the telemetry space is + determined from an XML file of specific guid for the corresponding + parent device. + +What: /sys/class/intel_pmt_telem/telemX/guid +Date: April 2020 +KernelVersion: 5.8 +Contact: David Box +Description: + (RO) The guid for this telemetry device. The guid identifies + the version of the XML file for the parent device that should + be used to determine the register layout. + +What: /sys/class/intel_pmt_telem/telemX/size +Date: April 2020 +KernelVersion: 5.8 +Contact: David Box +Description: + (RO) The size of telemetry region in bytes that corresponds to + the mapping size for the /dev/telemX device node. + +What: /sys/class/intel_pmt_telem/telemX/offset +Date: April 2020 +KernelVersion: 5.8 +Contact: David Box +Description: + (RO) The offset of telemetry region in bytes that corresponds to + the mapping for the /dev/telemX device node. diff --git a/MAINTAINERS b/MAINTAINERS index bacf7ecd4d21..c49a9d3a28d2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8787,6 +8787,7 @@ INTEL PMT DRIVER M: "David E. Box" S: Maintained F: drivers/mfd/intel_pmt.c +F: drivers/platform/x86/intel_pmt_* INTEL UNCORE FREQUENCY CONTROL M: Srinivas Pandruvada diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 0ad7ad8cf8e1..dd734eb66e74 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1368,6 +1368,16 @@ config INTEL_TELEMETRY directly via debugfs files. Various tools may use this interface for SoC state monitoring. +config INTEL_PMT_TELEM + tristate "Intel PMT telemetry driver" + help +The Intel Platform Monitory
