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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Aug 13, 2024 at 02:29:27PM +0000, Ruhl, Michael J wrote:
> > From: Andy Shevchenko <andriy.shevchenko@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_.

...

> > > +#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?

...

> > > +	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.

> > > +	}

-- 
With Best Regards,
Andy Shevchenko