On Sat, Nov 20, 2021 at 03:17:04PM -0800, David E. Box wrote:
> Intel Platform Monitoring Technology (PMT) support is indicated by
> presence of an Intel defined PCIe Designated Vendor Specific Extended
> Capabilities (DVSEC) structure with a PMT specific ID. The current MFD
> implementation creates child devices for each PMT feature, currently
> telemetry and crashlog.
Apparently "watcher," too?
> However DVSEC structures may also be used by Intel to indicate
> support for other features. The Out Of Band Management Services
> Module (OOBMSM)
OOBMSM refers to something outside this series? Oh, I see ... looks
like that's a specific device (PCI_DEVICE_ID_INTEL_VSEC_OOBMSM).
> uses DVSEC to enumerate several features,
> including PMT. In order to support them it is necessary to modify the
> intel_pmt driver to handle the creation of the child devices more
> generically. Additionally, since these are not platform devices (which
> is what MFD is really intended for) move the implementation to the more
> appropriate Auxiliary bus and host in platform/x86/intel.
It'd be useful to mention *why* the auxiliary bus is more appropriate.
It's not obvious to me.
> Also, rename
> the driver from intel_pmt to intel_vsec to better reflect the purpose.
>
> Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> Reviewed-by: Mark Gross <markgross@xxxxxxxxxx>
> -static int pmt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> -{
> - ...
> -
> - pm_runtime_put(&pdev->dev);
> - pm_runtime_allow(&pdev->dev);
What happened to this runtime PM functionality? Is it no longer
needed when using the auxiliary bus? I don't see corresponding
functionality there.
> - return 0;
> -}
> -
> -static void pmt_pci_remove(struct pci_dev *pdev)
> -{
> - pm_runtime_forbid(&pdev->dev);
> - pm_runtime_get_sync(&pdev->dev);
> -}
> +static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *header,
> + unsigned long quirks)
> +{
> + ...
> + res = kcalloc(header->num_entries, sizeof(*res), GFP_KERNEL);
> + if (!res) {
> + kfree(intel_vsec_dev);
> + return -ENOMEM;
> + }
> +
> + if (quirks & VSEC_QUIRK_TABLE_SHIFT)
> + header->offset >>= TABLE_OFFSET_SHIFT;
> +
> + /*
> + * The DVSEC/VSEC contains the starting offset and count for a block of
> + * discovery tables. Create a resource list of these tables to the
> + * auxiliary device driver.
"res" looks like an array of resources, not a list, i.e., I don't see
any ->next pointers here.
> + */
> + for (i = 0, tmp = res; i < header->num_entries; i++, tmp++) {
> + tmp->start = pdev->resource[header->tbir].start +
> + header->offset + i * (header->entry_size * sizeof(u32));
> + tmp->end = tmp->start + (header->entry_size * sizeof(u32)) - 1;
> + tmp->flags = IORESOURCE_MEM;
> + }
> +
> + intel_vsec_dev->pcidev = pdev;
> + intel_vsec_dev->resource = res;
> + intel_vsec_dev->num_resources = header->num_entries;
> + intel_vsec_dev->quirks = quirks;
> + intel_vsec_dev->ida = &intel_vsec_ida;
> +
> + return intel_vsec_add_aux(pdev, intel_vsec_dev, intel_vsec_name(header->id));
> +}
> +/* TGL info */
> +static const struct intel_vsec_platform_info tgl_info = {
> + .quirks = VSEC_QUIRK_NO_WATCHER | VSEC_QUIRK_NO_CRASHLOG | VSEC_QUIRK_TABLE_SHIFT,
> +};
I guess these are essentially device defects, i.e., TGL advertises
watcher and crashlog via VSEC, but doesn't actually support them?
> +#define PCI_DEVICE_ID_INTEL_VSEC_ADL 0x467d
> +#define PCI_DEVICE_ID_INTEL_VSEC_DG1 0x490e
> +#define PCI_DEVICE_ID_INTEL_VSEC_OOBMSM 0x09a7
> +#define PCI_DEVICE_ID_INTEL_VSEC_TGL 0x9a0d
> +static const struct pci_device_id intel_vsec_pci_ids[] = {
> + { PCI_DEVICE_DATA(INTEL, VSEC_ADL, &tgl_info) },
> + { PCI_DEVICE_DATA(INTEL, VSEC_DG1, &dg1_info) },
> + { PCI_DEVICE_DATA(INTEL, VSEC_OOBMSM, NULL) },
> + { PCI_DEVICE_DATA(INTEL, VSEC_TGL, &tgl_info) },
IIUC, you're implicitly saying that only these listed Device IDs can
have these VSEC capabilities, or at least, that these VSEC-described
features are only supported on these Device IDs.
That's not the way PCI capabilities work in general, so this doesn't
feel like a perfect fit to me, but I guess it's probably the only way
to identify the devices you care about.
Bjorn
