[PATCH 06/12] drm/xe/vrsr: Initialize VRSR feature

[Gupta, Anshuman]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas at kernel.org>
> Sent: Wednesday, April 2, 2025 1:27 AM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>
> Cc: intel-xe at lists.freedesktop.org; linux-acpi at vger.kernel.org; linux-
> pci at vger.kernel.org; rafael at kernel.org; lenb at kernel.org;
> bhelgaas at google.com; ilpo.jarvinen at linux.intel.com; De Marchi, Lucas
> <lucas.demarchi at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>; Nilawar,
> Badal <badal.nilawar at intel.com>; Gupta, Varun <varun.gupta at intel.com>;
> ville.syrjala at linux.intel.com; Shankar, Uma <uma.shankar at intel.com>
> Subject: Re: [PATCH 06/12] drm/xe/vrsr: Initialize VRSR feature
> 
> On Tue, Apr 01, 2025 at 09:02:19PM +0530, Anshuman Gupta wrote:
> > From: Badal Nilawar <badal.nilawar at intel.com>
> >
> > Initialize VRSR feature by requesting Auxilary Power and PERST# assertion
> > delay. Include an API to enable VRSR feature.
> 
> s/Auxilary/Auxiliary/
> 
> I would include the name of the API directly.
> 
> > +#define     PCODE_D3_VRSR_PERST_SHIFT	16
> 
> PCODE_D3_VRSR_PERST_SHIFT is not used by this series; maybe omit it?
> 
> > +#define	    POWER_D3_VRSR_PSERST_MASK	REG_GENMASK(31,
> 16)
> 
> s/PSERST/PERST/ (looks like a typo?)
> 
> > +static void xe_pm_vrsr_init(struct xe_device *xe)
> > +{
> > +	int ret;
> > +
> > +	/* Check if platform support d3cold vrsr */
> 
> Nicer to consistently capitalize as "VRSR" in comments, commit
> logs, and messages.
> 
> Similar with "D3cold" ("D3cold" is used ~100 times in the tree,
> "D3Cold" ~20, mostly in xe).
> 
> > +	if (!xe->info.has_vrsr)
> > +		return;
> > +
> > +	if (!xe_pm_vrsr_capable(xe))
> > +		return;
> > +
> > +	/*
> > +	 * If the VRSR initialization fails, the device will proceed with the regular
> > +	 * D3 Cold flow
> > +	 */
> > +	ret = pci_acpi_aux_power_setup(xe);
> > +	if (ret) {
> > +		drm_info(&xe->drm, "VRSR capable %s\n", "No");
> 
> Kinda weird to use %s when the text is a known constant.
> 
> > +		return;
> > +	}
> > +
> > +	xe->d3cold.vrsr_capable = true;
> > +	drm_info(&xe->drm, "VRSR capable %s\n", "Yes");
> 
> Same.
Thanks for review comment, will address these in v2.
Thanks,
Anshuman.
> 
> > +}