Re: [PATCH v1] drm/xe/pm: Enable WAKE# support

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Oct 28, 2025 at 06:12:09AM +0100, Raag Jadav wrote:
> On Mon, Oct 27, 2025 at 04:58:18PM -0400, Rodrigo Vivi wrote:
> > On Fri, Oct 24, 2025 at 03:51:48PM +0530, Raag Jadav wrote:
> > > CRI supports signalling the host through WAKE# pin on smbus alerts while
> > > in D3cold. Enable/disable this feature based on device/host configuration.
> > > 
> > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_device_types.h |  4 ++
> > >  drivers/gpu/drm/xe/xe_pci.c          |  4 ++
> > >  drivers/gpu/drm/xe/xe_pcode_api.h    |  5 ++
> > >  drivers/gpu/drm/xe/xe_pm.c           | 72 ++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/xe/xe_pm.h           |  1 +
> > >  5 files changed, 86 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > > index 8f62ee7a73ac..dae2bd06bcea 100644
> > > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > > @@ -517,6 +517,10 @@ struct xe_device {
> > >  		 * Default threshold value is 300mb.
> > >  		 */
> > >  		u32 vram_threshold;
> > > +
> > > +		/** @d3cold.wake: Indicates WAKE# capability of device */
> > > +		u32 wake;
> > > +
> > >  		/** @d3cold.lock: protect vram_threshold */
> > >  		struct mutex lock;
> > >  	} d3cold;
> > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > index 6e59642e7820..edf63f55f345 100644
> > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > @@ -1105,6 +1105,8 @@ static int xe_pci_suspend(struct device *dev)
> > >  	if (err)
> > >  		return err;
> > >  
> > > +	xe_pm_set_wake(xe, true);
> > 
> > why does it need to be here and not something static inside xe_pm itself?
> 
> The expectation is this should be done only if xe_pm_suspend() is
> successful or you can guarantee it.

so, please make sure it is the last call before the return 0 inside
that function so you don't need to make this public function in the
header, but keep that static in xe_pm...

> 
> > > +
> > >  	/*
> > >  	 * Enabling D3Cold is needed for S2Idle/S0ix.
> > >  	 * It is save to allow here since xe_pm_suspend has evicted
> > > @@ -1156,6 +1158,8 @@ static int xe_pci_runtime_suspend(struct device *dev)
> > >  	if (err)
> > >  		return err;
> > >  
> > > +	xe_pm_set_wake(xe, xe->d3cold.allowed);
> > > +
> > >  	pci_save_state(pdev);
> > >  
> > >  	if (xe->d3cold.allowed) {
> > > diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
> > > index 92bfcba51e19..3f8d43c1b4fe 100644
> > > --- a/drivers/gpu/drm/xe/xe_pcode_api.h
> > > +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
> > > @@ -50,6 +50,11 @@
> > >  #define	READ_PL_FROM_FW				0x1
> > >  #define	READ_PL_FROM_PCODE			0x0
> > >  
> > > +#define   PCODE_D3COLD_WAKE			0x5A
> > > +#define     READ_MODE				0x0
> > > +#define     WRITE_MODE				0x1
> > > +#define       I2C_WAKE_ENABLE			REG_BIT(1)
> > 
> > please share the spec with me
> > 
> > > +
> > >  #define   PCODE_LATE_BINDING			0x5C
> > >  #define     GET_CAPABILITY_STATUS		0x0
> > >  #define       V1_FAN_SUPPORTED			REG_BIT(0)
> > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > > index 53507e09f7bc..30e4a787b9cb 100644
> > > --- a/drivers/gpu/drm/xe/xe_pm.c
> > > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > > @@ -7,6 +7,8 @@
> > >  
> > >  #include <linux/fault-inject.h>
> > >  #include <linux/pm_runtime.h>
> > > +#include <linux/pm_wakeup.h>
> > > +#include <linux/string_choices.h>
> > >  #include <linux/suspend.h>
> > >  
> > >  #include <drm/drm_managed.h>
> > > @@ -22,6 +24,7 @@
> > >  #include "xe_i2c.h"
> > >  #include "xe_irq.h"
> > >  #include "xe_late_bind_fw.h"
> > > +#include "xe_pcode_api.h"
> > >  #include "xe_pcode.h"
> > >  #include "xe_pxp.h"
> > >  #include "xe_sriov_vf_ccs.h"
> > > @@ -422,6 +425,74 @@ static int xe_pm_notifier_callback(struct notifier_block *nb,
> > >  	return NOTIFY_DONE;
> > >  }
> > >  
> > > +void xe_pm_set_wake(struct xe_device *xe, bool d3cold)
> > > +{
> > > +	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
> > > +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> > > +	struct pci_dev *root_pdev;
> > > +	bool wakeup;
> > > +	u32 val = 0;
> > > +	int ret;
> > > +
> > > +	/* WAKE# is not needed, let PME do the job. */
> > > +	if (!d3cold)
> > > +		return;
> > 
> > Please NO! check this outside and only call the function if needed instead
> > of passing the need as argument.
> 
> Sure.
> 
> > > +
> > > +	/* Currently WAKE# is supported for I2C only */
> > > +	if (!REG_FIELD_GET(I2C_WAKE_ENABLE, xe->d3cold.wake))
> > > +		return;
> > 
> > let's perhaps make it a
> > 
> > static void i2c_set_wake(struct xe_device *xe)
> > {
> > ...
> > }
> 
> No, there are other usecases supported that we're not adding here (yet)
> and we don't want to come back revamping/duplicating code when we need them.
> Simply add a new bit and call it a day.

fair enough...

> 
> Raag
> 
> > > +
> > > +	ret = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_D3COLD_WAKE, READ_MODE, 0), &val, NULL);
> > > +	if (ret)
> > > +		return;
> > > +
> > > +	root_pdev = pcie_find_root_port(pdev);
> > > +	wakeup = root_pdev && device_may_wakeup(&root_pdev->dev);
> > > +
> > > +	if (wakeup)
> > > +		val |= I2C_WAKE_ENABLE;
> > > +	else
> > > +		val &= ~I2C_WAKE_ENABLE;
> > > +
> > > +	ret = xe_pcode_write(root_tile, PCODE_MBOX(PCODE_D3COLD_WAKE, WRITE_MODE, 0), val);
> > > +	if (ret)
> > > +		return;
> > > +
> > > +	drm_dbg(&xe->drm, "WAKE# %s\n", str_enabled_disabled(wakeup));
> > > +}
> > > +
> > > +static void xe_pm_wake_init(struct xe_device *xe)
> > > +{
> > > +	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
> > > +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> > > +	struct pci_dev *root_pdev;
> > > +	int err;
> > > +
> > > +	if (xe->info.platform != XE_CRESCENTISLAND)
> > > +		return;
> > > +
> > > +	err = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_D3COLD_WAKE, READ_MODE, 0),
> > > +			    &xe->d3cold.wake, NULL);
> > > +	if (err || !xe->d3cold.wake) {
> > > +		drm_dbg(&xe->drm, "WAKE# not supported by device\n");
> > > +		return;
> > > +	}
> > > +
> > > +	root_pdev = pcie_find_root_port(pdev);
> > > +	if (!root_pdev || !device_can_wakeup(&root_pdev->dev)) {
> > > +		drm_dbg(&xe->drm, "WAKE# not supported by host\n");
> > > +		xe->d3cold.wake = 0;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (!xe_i2c_present(xe))
> > > +		xe->d3cold.wake &= ~I2C_WAKE_ENABLE;
> > > +
> > > +	drm_dbg(&xe->drm, "WAKE# configuration 0x%08x\n", xe->d3cold.wake);
> > > +out:
> > > +	xe_pcode_write(root_tile, PCODE_MBOX(PCODE_D3COLD_WAKE, WRITE_MODE, 0), xe->d3cold.wake);
> > > +}
> > > +
> > >  /**
> > >   * xe_pm_init - Initialize Xe Power Management
> > >   * @xe: xe device instance
> > > @@ -459,6 +530,7 @@ int xe_pm_init(struct xe_device *xe)
> > >  			goto err_unregister;
> > >  	}
> > >  
> > > +	xe_pm_wake_init(xe);
> > >  	xe_pm_runtime_init(xe);
> > >  	return 0;
> > >  
> > > diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> > > index f7f89a18b6fc..9cedba8a7f80 100644
> > > --- a/drivers/gpu/drm/xe/xe_pm.h
> > > +++ b/drivers/gpu/drm/xe/xe_pm.h
> > > @@ -30,6 +30,7 @@ void xe_pm_runtime_get_noresume(struct xe_device *xe);
> > >  bool xe_pm_runtime_resume_and_get(struct xe_device *xe);
> > >  void xe_pm_assert_unbounded_bridge(struct xe_device *xe);
> > >  int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold);
> > > +void xe_pm_set_wake(struct xe_device *xe, bool d3cold);
> > >  void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
> > >  bool xe_rpm_reclaim_safe(const struct xe_device *xe);
> > >  struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
> > > -- 
> > > 2.34.1
> > >