[PATCH v11] drm/xe/vsec: Support BMG devices

[Ruhl, Michael J]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> Sent: Wednesday, August 14, 2024 9:56 AM
> To: Ruhl, Michael J <michael.j.ruhl at intel.com>
> Cc: intel-xe at lists.freedesktop.org; platform-driver-x86 at vger.kernel.org;
> david.e.box at linux.intel.com; ilpo.jarvinen at linux.intel.com; Brost, Matthew
> <matthew.brost at intel.com>; hdegoede at redhat.com; Vivi, Rodrigo
> <rodrigo.vivi at intel.com>
> Subject: Re: [PATCH v11] drm/xe/vsec: Support BMG devices
> 
> On Tue, Aug 13, 2024 at 02:29:27PM +0000, Ruhl, Michael J wrote:
> > > From: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> > > Sent: Tuesday, August 13, 2024 10:11 AM On Mon, Aug 12, 2024 at
> > > 04:04:22PM -0400, Michael J. Ruhl wrote:
> 
> ...
> 
> > > > +#define SOC_BASE		0x280000
> > > > +
> > > > +#define BMG_PMT_BASE		0xDB000
> > > > +#define BMG_DISCOVERY_OFFSET	(SOC_BASE +
> BMG_PMT_BASE)
> > >
> > > > +#define BMG_TELEMETRY_BASE	0xE0000
> > > > +#define BMG_TELEMETRY_OFFSET	(SOC_BASE +
> BMG_TELEMETRY_BASE)
> > >
> > > This looks like double indirection.
> > > Wouldn't suffix _BASE_OFFSET be better for PMT and TELEMETRY cases?
> >
> > I am not sure I understand.
> >
> > Are  you saying rename BMG_PMT_BASE to BMG_PMT_BASE_OFFSET?
> 
> Yes. Same for BMG_TELEMETRY_.

Got it. I will update.
 
> ...
> 
> > > > +#define BMG_DEVICE_ID 0xE2F8
> > >
> > > Is this defined in any specification? I mean is the format the same
> > > as PCI device ID?
> >
> > I think that this is defined in BMG PMT yaml definition.  It is
> > provide in the PMT discovery data, so it is defined by the specific device.
> 
> Is there any documentation / specification about this?
> Can it be UUID or 64-bit number or other format?
> _Where_ is this being specified?

The GUID is defined by the YAML file associated with the PMT device.  In this
case 16 bits are a device ID.

>From the cover letter of the PMT patch set (Intel Platform Monitoring Technology):

-
The GUID uniquely identifies the register space of any monitor data exposed
by the capability. The GUID is associated with an XML file from the vendor
that describes the mapping of the register space along with properties of the
monitor data.
--

I was told that this was the value to use for this specific device/feature.

It is specified internally.  Not sure if there is any "documentation" available beyond
that.

> ...
> 
> > > > +	switch (record_id) {
> > > > +	case PUNIT:
> > > > +		*index = 0;
> > > > +		if (cap_type == TELEMETRY)
> > > > +			*offset = PUNIT_TELEMETRY_OFFSET;
> > > > +		else
> > > > +			*offset = PUNIT_WATCHER_OFFSET;
> > > > +		break;
> > > > +
> > > > +	case OOBMSM_0:
> > > > +		*index = 1;
> > > > +		if (cap_type == WATCHER)
> > > > +			*offset = OOBMSM_0_WATCHER_OFFSET;
> > > > +		break;
> > > > +
> > > > +	case OOBMSM_1:
> > > > +		*index = 1;
> > > > +		if (cap_type == TELEMETRY)
> > > > +			*offset = OOBMSM_1_TELEMETRY_OFFSET;
> > > > +		break;
> > >
> > > default case?
> >
> > I validate the record_id and cap_type values at the beginning of the
> > function, so default would be redundant.
> >
> > The goal was to validate, then set data.
> >
> > So adding the default will remove the record_id check from the if.  Do
> > you prefer that path?
> 
> Yes.

Ok, I will update.

Thanks!

Mike

 
> > > > +	}
> 
> --
> With Best Regards,
> Andy Shevchenko
>