diff mbox series

[v3,16/29] drm/i915/xe2lpd: Add display power well

Message ID 20230912044837.1672060-17-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable Lunar Lake display | expand

Commit Message

Lucas De Marchi Sept. 12, 2023, 4:48 a.m. UTC 
From: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>

Add Display Power Well for LNL platform. It's mostly the same as MTL
platform so reuse the code. PGPICA1 contains type-C capable port slices
which requires the well to power powered up, so add new power well
definition for it.

BSpec: 68886
Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 .../i915/display/intel_display_power_map.c    | 36 ++++++++++++++-
 .../i915/display/intel_display_power_well.c   | 44 +++++++++++++++++++
 .../i915/display/intel_display_power_well.h   |  1 +
 .../gpu/drm/i915/display/intel_dp_aux_regs.h  |  5 +++
 4 files changed, 85 insertions(+), 1 deletion(-)

Comments

Matt Roper Sept. 12, 2023, 4:04 p.m. UTC | #1 
On Mon, Sep 11, 2023 at 09:48:24PM -0700, Lucas De Marchi wrote:
> From: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
> 
> Add Display Power Well for LNL platform. It's mostly the same as MTL

It might be better to say "Xe2_LPD" and "Xe_LPD+" instead of LNL/MTL?

> platform so reuse the code. PGPICA1 contains type-C capable port slices
> which requires the well to power powered up, so add new power well
> definition for it.

Maybe add a sentence noting that the dc_off fake powerwell will be added
in a follow-up patch so that people don't think it was overlooked here?

> 
> BSpec: 68886
> Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  .../i915/display/intel_display_power_map.c    | 36 ++++++++++++++-
>  .../i915/display/intel_display_power_well.c   | 44 +++++++++++++++++++
>  .../i915/display/intel_display_power_well.h   |  1 +
>  .../gpu/drm/i915/display/intel_dp_aux_regs.h  |  5 +++
>  4 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> index 0f1b93d139ca..31c11586ede5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_map.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> @@ -1536,6 +1536,38 @@ static const struct i915_power_well_desc_list xelpdp_power_wells[] = {
>  	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
>  };
>  
> +I915_DECL_PW_DOMAINS(xe2lpd_pwdoms_pica_tc,
> +		     POWER_DOMAIN_PORT_DDI_LANES_TC1,
> +		     POWER_DOMAIN_PORT_DDI_LANES_TC2,
> +		     POWER_DOMAIN_PORT_DDI_LANES_TC3,
> +		     POWER_DOMAIN_PORT_DDI_LANES_TC4,
> +		     POWER_DOMAIN_AUX_USBC1,
> +		     POWER_DOMAIN_AUX_USBC2,
> +		     POWER_DOMAIN_AUX_USBC3,
> +		     POWER_DOMAIN_AUX_USBC4,
> +		     POWER_DOMAIN_AUX_TBT1,
> +		     POWER_DOMAIN_AUX_TBT2,
> +		     POWER_DOMAIN_AUX_TBT3,
> +		     POWER_DOMAIN_AUX_TBT4,
> +		     POWER_DOMAIN_INIT);
> +
> +static const struct i915_power_well_desc xe2lpd_power_wells_pica[] = {
> +	{
> +		.instances = &I915_PW_INSTANCES(I915_PW("PICA_TC",
> +							&xe2lpd_pwdoms_pica_tc,
> +							.id = DISP_PW_ID_NONE),
> +					       ),
> +		.ops = &xe2lpd_pica_power_well_ops,
> +	},
> +};
> +
> +static const struct i915_power_well_desc_list xe2lpd_power_wells[] = {
> +	I915_PW_DESCRIPTORS(i9xx_power_wells_always_on),
> +	I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
> +	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
> +	I915_PW_DESCRIPTORS(xe2lpd_power_wells_pica),
> +};
> +
>  static void init_power_well_domains(const struct i915_power_well_instance *inst,
>  				    struct i915_power_well *power_well)
>  {
> @@ -1643,7 +1675,9 @@ int intel_display_power_map_init(struct i915_power_domains *power_domains)
>  		return 0;
>  	}
>  
> -	if (DISPLAY_VER(i915) >= 14)
> +	if (DISPLAY_VER(i915) >= 20)
> +		return set_power_wells(power_domains, xe2lpd_power_wells);
> +	else if (DISPLAY_VER(i915) >= 14)
>  		return set_power_wells(power_domains, xelpdp_power_wells);
>  	else if (IS_DG2(i915))
>  		return set_power_wells(power_domains, xehpd_power_wells);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> index ca0714eba17a..adfe9901bde4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -1833,6 +1833,43 @@ static bool xelpdp_aux_power_well_enabled(struct drm_i915_private *dev_priv,
>  		XELPDP_DP_AUX_CH_CTL_POWER_STATUS;
>  }
>  
> +static void xe2lpd_pica_power_well_enable(struct drm_i915_private *dev_priv,
> +					  struct i915_power_well *power_well)
> +{
> +	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
> +		     XE2LPD_PICA_CTL_POWER_REQUEST,
> +		     XE2LPD_PICA_CTL_POWER_REQUEST);

Do we need rmw here?  Bit 31 is the only writable bit in the register
(bit 30 is RO and can't be clobbered), so a simple write should suffice?
Ditto on the disable below.

> +
> +	if (intel_de_wait_for_set(dev_priv, XE2LPD_PICA_PW_CTL,
> +				  XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
> +		drm_dbg_kms(&dev_priv->drm, "pica power well enable timeout\n");
> +
> +		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when enabled");
> +	}
> +}
> +
> +static void xe2lpd_pica_power_well_disable(struct drm_i915_private *dev_priv,
> +					   struct i915_power_well *power_well)
> +{
> +	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
> +		     XE2LPD_PICA_CTL_POWER_REQUEST,
> +		     0);
> +
> +	if (intel_de_wait_for_clear(dev_priv, XE2LPD_PICA_PW_CTL,
> +				    XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
> +		drm_dbg_kms(&dev_priv->drm, "pica power well disable timeout\n");
> +
> +		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when disabled");
> +	}
> +}
> +
> +static bool xe2lpd_pica_power_well_enabled(struct drm_i915_private *dev_priv,
> +					   struct i915_power_well *power_well)
> +{
> +	return intel_de_read(dev_priv, XE2LPD_PICA_PW_CTL) &
> +		XE2LPD_PICA_CTL_POWER_STATUS;
> +}
> +
>  const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
>  	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = i9xx_always_on_power_well_noop,
> @@ -1952,3 +1989,10 @@ const struct i915_power_well_ops xelpdp_aux_power_well_ops = {
>  	.disable = xelpdp_aux_power_well_disable,
>  	.is_enabled = xelpdp_aux_power_well_enabled,
>  };
> +
> +const struct i915_power_well_ops xe2lpd_pica_power_well_ops = {
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
> +	.enable = xe2lpd_pica_power_well_enable,
> +	.disable = xe2lpd_pica_power_well_disable,
> +	.is_enabled = xe2lpd_pica_power_well_enabled,
> +};
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.h b/drivers/gpu/drm/i915/display/intel_display_power_well.h
> index a8736588314d..9357a9a73c06 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.h
> @@ -176,5 +176,6 @@ extern const struct i915_power_well_ops icl_aux_power_well_ops;
>  extern const struct i915_power_well_ops icl_ddi_power_well_ops;
>  extern const struct i915_power_well_ops tgl_tc_cold_off_ops;
>  extern const struct i915_power_well_ops xelpdp_aux_power_well_ops;
> +extern const struct i915_power_well_ops xe2lpd_pica_power_well_ops;
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> index b4e96baae25a..c869f5661530 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> @@ -86,4 +86,9 @@
>  		 _XELPDP_DP_AUX_CH_DATA(__xe2lpd_aux_ch_idx(aux_ch), i) :	\
>  		 _XELPDP_DP_AUX_CH_DATA(aux_ch, i))
>  
> +/* PICA Power Well Control */
> +#define XE2LPD_PICA_PW_CTL			_MMIO(0x16fe04)

Is this the right header for this?  It doesn't seem directly related to
DP AUX.


Matt

> +#define   XE2LPD_PICA_CTL_POWER_REQUEST		REG_BIT(31)
> +#define   XE2LPD_PICA_CTL_POWER_STATUS		REG_BIT(30)
> +
>  #endif /* __INTEL_DP_AUX_REGS_H__ */
> -- 
> 2.40.1
>
Lucas De Marchi Sept. 12, 2023, 4:30 p.m. UTC | #2 
On Tue, Sep 12, 2023 at 09:04:17AM -0700, Matt Roper wrote:
>On Mon, Sep 11, 2023 at 09:48:24PM -0700, Lucas De Marchi wrote:
>> From: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
>>
>> Add Display Power Well for LNL platform. It's mostly the same as MTL
>
>It might be better to say "Xe2_LPD" and "Xe_LPD+" instead of LNL/MTL?

ok

>
>> platform so reuse the code. PGPICA1 contains type-C capable port slices
>> which requires the well to power powered up, so add new power well
>> definition for it.
>
>Maybe add a sentence noting that the dc_off fake powerwell will be added
>in a follow-up patch so that people don't think it was overlooked here?

ok

>
>>
>> BSpec: 68886
>> Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  .../i915/display/intel_display_power_map.c    | 36 ++++++++++++++-
>>  .../i915/display/intel_display_power_well.c   | 44 +++++++++++++++++++
>>  .../i915/display/intel_display_power_well.h   |  1 +
>>  .../gpu/drm/i915/display/intel_dp_aux_regs.h  |  5 +++
>>  4 files changed, 85 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c b/drivers/gpu/drm/i915/display/intel_display_power_map.c
>> index 0f1b93d139ca..31c11586ede5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power_map.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c
>> @@ -1536,6 +1536,38 @@ static const struct i915_power_well_desc_list xelpdp_power_wells[] = {
>>  	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
>>  };
>>
>> +I915_DECL_PW_DOMAINS(xe2lpd_pwdoms_pica_tc,
>> +		     POWER_DOMAIN_PORT_DDI_LANES_TC1,
>> +		     POWER_DOMAIN_PORT_DDI_LANES_TC2,
>> +		     POWER_DOMAIN_PORT_DDI_LANES_TC3,
>> +		     POWER_DOMAIN_PORT_DDI_LANES_TC4,
>> +		     POWER_DOMAIN_AUX_USBC1,
>> +		     POWER_DOMAIN_AUX_USBC2,
>> +		     POWER_DOMAIN_AUX_USBC3,
>> +		     POWER_DOMAIN_AUX_USBC4,
>> +		     POWER_DOMAIN_AUX_TBT1,
>> +		     POWER_DOMAIN_AUX_TBT2,
>> +		     POWER_DOMAIN_AUX_TBT3,
>> +		     POWER_DOMAIN_AUX_TBT4,
>> +		     POWER_DOMAIN_INIT);
>> +
>> +static const struct i915_power_well_desc xe2lpd_power_wells_pica[] = {
>> +	{
>> +		.instances = &I915_PW_INSTANCES(I915_PW("PICA_TC",
>> +							&xe2lpd_pwdoms_pica_tc,
>> +							.id = DISP_PW_ID_NONE),
>> +					       ),
>> +		.ops = &xe2lpd_pica_power_well_ops,
>> +	},
>> +};
>> +
>> +static const struct i915_power_well_desc_list xe2lpd_power_wells[] = {
>> +	I915_PW_DESCRIPTORS(i9xx_power_wells_always_on),
>> +	I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
>> +	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
>> +	I915_PW_DESCRIPTORS(xe2lpd_power_wells_pica),
>> +};
>> +
>>  static void init_power_well_domains(const struct i915_power_well_instance *inst,
>>  				    struct i915_power_well *power_well)
>>  {
>> @@ -1643,7 +1675,9 @@ int intel_display_power_map_init(struct i915_power_domains *power_domains)
>>  		return 0;
>>  	}
>>
>> -	if (DISPLAY_VER(i915) >= 14)
>> +	if (DISPLAY_VER(i915) >= 20)
>> +		return set_power_wells(power_domains, xe2lpd_power_wells);
>> +	else if (DISPLAY_VER(i915) >= 14)
>>  		return set_power_wells(power_domains, xelpdp_power_wells);
>>  	else if (IS_DG2(i915))
>>  		return set_power_wells(power_domains, xehpd_power_wells);
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> index ca0714eba17a..adfe9901bde4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> @@ -1833,6 +1833,43 @@ static bool xelpdp_aux_power_well_enabled(struct drm_i915_private *dev_priv,
>>  		XELPDP_DP_AUX_CH_CTL_POWER_STATUS;
>>  }
>>
>> +static void xe2lpd_pica_power_well_enable(struct drm_i915_private *dev_priv,
>> +					  struct i915_power_well *power_well)
>> +{
>> +	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
>> +		     XE2LPD_PICA_CTL_POWER_REQUEST,
>> +		     XE2LPD_PICA_CTL_POWER_REQUEST);
>
>Do we need rmw here?  Bit 31 is the only writable bit in the register
>(bit 30 is RO and can't be clobbered), so a simple write should suffice?
>Ditto on the disable below.
>
>> +
>> +	if (intel_de_wait_for_set(dev_priv, XE2LPD_PICA_PW_CTL,
>> +				  XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
>> +		drm_dbg_kms(&dev_priv->drm, "pica power well enable timeout\n");
>> +
>> +		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when enabled");
>> +	}
>> +}
>> +
>> +static void xe2lpd_pica_power_well_disable(struct drm_i915_private *dev_priv,
>> +					   struct i915_power_well *power_well)
>> +{
>> +	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
>> +		     XE2LPD_PICA_CTL_POWER_REQUEST,
>> +		     0);
>> +
>> +	if (intel_de_wait_for_clear(dev_priv, XE2LPD_PICA_PW_CTL,
>> +				    XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
>> +		drm_dbg_kms(&dev_priv->drm, "pica power well disable timeout\n");
>> +
>> +		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when disabled");
>> +	}
>> +}
>> +
>> +static bool xe2lpd_pica_power_well_enabled(struct drm_i915_private *dev_priv,
>> +					   struct i915_power_well *power_well)
>> +{
>> +	return intel_de_read(dev_priv, XE2LPD_PICA_PW_CTL) &
>> +		XE2LPD_PICA_CTL_POWER_STATUS;
>> +}
>> +
>>  const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
>>  	.sync_hw = i9xx_power_well_sync_hw_noop,
>>  	.enable = i9xx_always_on_power_well_noop,
>> @@ -1952,3 +1989,10 @@ const struct i915_power_well_ops xelpdp_aux_power_well_ops = {
>>  	.disable = xelpdp_aux_power_well_disable,
>>  	.is_enabled = xelpdp_aux_power_well_enabled,
>>  };
>> +
>> +const struct i915_power_well_ops xe2lpd_pica_power_well_ops = {
>> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>> +	.enable = xe2lpd_pica_power_well_enable,
>> +	.disable = xe2lpd_pica_power_well_disable,
>> +	.is_enabled = xe2lpd_pica_power_well_enabled,
>> +};
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.h b/drivers/gpu/drm/i915/display/intel_display_power_well.h
>> index a8736588314d..9357a9a73c06 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.h
>> @@ -176,5 +176,6 @@ extern const struct i915_power_well_ops icl_aux_power_well_ops;
>>  extern const struct i915_power_well_ops icl_ddi_power_well_ops;
>>  extern const struct i915_power_well_ops tgl_tc_cold_off_ops;
>>  extern const struct i915_power_well_ops xelpdp_aux_power_well_ops;
>> +extern const struct i915_power_well_ops xe2lpd_pica_power_well_ops;
>>
>>  #endif
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
>> index b4e96baae25a..c869f5661530 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
>> @@ -86,4 +86,9 @@
>>  		 _XELPDP_DP_AUX_CH_DATA(__xe2lpd_aux_ch_idx(aux_ch), i) :	\
>>  		 _XELPDP_DP_AUX_CH_DATA(aux_ch, i))
>>
>> +/* PICA Power Well Control */
>> +#define XE2LPD_PICA_PW_CTL			_MMIO(0x16fe04)
>
>Is this the right header for this?  It doesn't seem directly related to
>DP AUX.

In Xe2_LPD, those AUX registers are also

I had the same question myself while rebasing this patch as this is
not related to DP. However the register itself seems to have the same
functionality as the other registers above with power request/status.
And if you look at bspec 69009, all of them are under pica for Xe2_LPD,
too.

I'm not sure what was the criteria for the split of this DP header.
It's clearer for things like gt, engine, snps phy, etc, but this one
seems to have been more or less arbitrary.

Any suggestion for a better place? A new display/intel_pica_regs.h
maybe? That may not be as future proof in the cases register move from
one place to the other like happened in Xe2_LPD: see bspec 69009.  All
the PORT_BUF_CTL*, PORT_HOTPLUG_CTL, etc should then be in this header,
which doesn't match previous platforms.

If instead of matching HW we try to match where it's used in SW, then
maybe a intel_display_power_well_regs.h, but that too doesn't match
previous platforms.

+Jani

Lucas De Marchi

>
>
>Matt
>
>> +#define   XE2LPD_PICA_CTL_POWER_REQUEST		REG_BIT(31)
>> +#define   XE2LPD_PICA_CTL_POWER_STATUS		REG_BIT(30)
>> +
>>  #endif /* __INTEL_DP_AUX_REGS_H__ */
>> --
>> 2.40.1
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
Matt Roper Sept. 14, 2023, 4:36 p.m. UTC | #3 
On Tue, Sep 12, 2023 at 11:30:50AM -0500, Lucas De Marchi wrote:
> On Tue, Sep 12, 2023 at 09:04:17AM -0700, Matt Roper wrote:
> > On Mon, Sep 11, 2023 at 09:48:24PM -0700, Lucas De Marchi wrote:
> > > From: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
> > > 
> > > Add Display Power Well for LNL platform. It's mostly the same as MTL
> > 
> > It might be better to say "Xe2_LPD" and "Xe_LPD+" instead of LNL/MTL?
> 
> ok
> 
> > 
> > > platform so reuse the code. PGPICA1 contains type-C capable port slices
> > > which requires the well to power powered up, so add new power well
> > > definition for it.
> > 
> > Maybe add a sentence noting that the dc_off fake powerwell will be added
> > in a follow-up patch so that people don't think it was overlooked here?
> 
> ok
> 
> > 
> > > 
> > > BSpec: 68886
> > > Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  .../i915/display/intel_display_power_map.c    | 36 ++++++++++++++-
> > >  .../i915/display/intel_display_power_well.c   | 44 +++++++++++++++++++
> > >  .../i915/display/intel_display_power_well.h   |  1 +
> > >  .../gpu/drm/i915/display/intel_dp_aux_regs.h  |  5 +++
> > >  4 files changed, 85 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> > > index 0f1b93d139ca..31c11586ede5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_map.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> > > @@ -1536,6 +1536,38 @@ static const struct i915_power_well_desc_list xelpdp_power_wells[] = {
> > >  	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
> > >  };
> > > 
> > > +I915_DECL_PW_DOMAINS(xe2lpd_pwdoms_pica_tc,
> > > +		     POWER_DOMAIN_PORT_DDI_LANES_TC1,
> > > +		     POWER_DOMAIN_PORT_DDI_LANES_TC2,
> > > +		     POWER_DOMAIN_PORT_DDI_LANES_TC3,
> > > +		     POWER_DOMAIN_PORT_DDI_LANES_TC4,
> > > +		     POWER_DOMAIN_AUX_USBC1,
> > > +		     POWER_DOMAIN_AUX_USBC2,
> > > +		     POWER_DOMAIN_AUX_USBC3,
> > > +		     POWER_DOMAIN_AUX_USBC4,
> > > +		     POWER_DOMAIN_AUX_TBT1,
> > > +		     POWER_DOMAIN_AUX_TBT2,
> > > +		     POWER_DOMAIN_AUX_TBT3,
> > > +		     POWER_DOMAIN_AUX_TBT4,
> > > +		     POWER_DOMAIN_INIT);
> > > +
> > > +static const struct i915_power_well_desc xe2lpd_power_wells_pica[] = {
> > > +	{
> > > +		.instances = &I915_PW_INSTANCES(I915_PW("PICA_TC",
> > > +							&xe2lpd_pwdoms_pica_tc,
> > > +							.id = DISP_PW_ID_NONE),
> > > +					       ),
> > > +		.ops = &xe2lpd_pica_power_well_ops,
> > > +	},
> > > +};
> > > +
> > > +static const struct i915_power_well_desc_list xe2lpd_power_wells[] = {
> > > +	I915_PW_DESCRIPTORS(i9xx_power_wells_always_on),
> > > +	I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
> > > +	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
> > > +	I915_PW_DESCRIPTORS(xe2lpd_power_wells_pica),
> > > +};
> > > +
> > >  static void init_power_well_domains(const struct i915_power_well_instance *inst,
> > >  				    struct i915_power_well *power_well)
> > >  {
> > > @@ -1643,7 +1675,9 @@ int intel_display_power_map_init(struct i915_power_domains *power_domains)
> > >  		return 0;
> > >  	}
> > > 
> > > -	if (DISPLAY_VER(i915) >= 14)
> > > +	if (DISPLAY_VER(i915) >= 20)
> > > +		return set_power_wells(power_domains, xe2lpd_power_wells);
> > > +	else if (DISPLAY_VER(i915) >= 14)
> > >  		return set_power_wells(power_domains, xelpdp_power_wells);
> > >  	else if (IS_DG2(i915))
> > >  		return set_power_wells(power_domains, xehpd_power_wells);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > index ca0714eba17a..adfe9901bde4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > @@ -1833,6 +1833,43 @@ static bool xelpdp_aux_power_well_enabled(struct drm_i915_private *dev_priv,
> > >  		XELPDP_DP_AUX_CH_CTL_POWER_STATUS;
> > >  }
> > > 
> > > +static void xe2lpd_pica_power_well_enable(struct drm_i915_private *dev_priv,
> > > +					  struct i915_power_well *power_well)
> > > +{
> > > +	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
> > > +		     XE2LPD_PICA_CTL_POWER_REQUEST,
> > > +		     XE2LPD_PICA_CTL_POWER_REQUEST);
> > 
> > Do we need rmw here?  Bit 31 is the only writable bit in the register
> > (bit 30 is RO and can't be clobbered), so a simple write should suffice?
> > Ditto on the disable below.
> > 
> > > +
> > > +	if (intel_de_wait_for_set(dev_priv, XE2LPD_PICA_PW_CTL,
> > > +				  XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
> > > +		drm_dbg_kms(&dev_priv->drm, "pica power well enable timeout\n");
> > > +
> > > +		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when enabled");
> > > +	}
> > > +}
> > > +
> > > +static void xe2lpd_pica_power_well_disable(struct drm_i915_private *dev_priv,
> > > +					   struct i915_power_well *power_well)
> > > +{
> > > +	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
> > > +		     XE2LPD_PICA_CTL_POWER_REQUEST,
> > > +		     0);
> > > +
> > > +	if (intel_de_wait_for_clear(dev_priv, XE2LPD_PICA_PW_CTL,
> > > +				    XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
> > > +		drm_dbg_kms(&dev_priv->drm, "pica power well disable timeout\n");
> > > +
> > > +		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when disabled");
> > > +	}
> > > +}
> > > +
> > > +static bool xe2lpd_pica_power_well_enabled(struct drm_i915_private *dev_priv,
> > > +					   struct i915_power_well *power_well)
> > > +{
> > > +	return intel_de_read(dev_priv, XE2LPD_PICA_PW_CTL) &
> > > +		XE2LPD_PICA_CTL_POWER_STATUS;
> > > +}
> > > +
> > >  const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
> > >  	.sync_hw = i9xx_power_well_sync_hw_noop,
> > >  	.enable = i9xx_always_on_power_well_noop,
> > > @@ -1952,3 +1989,10 @@ const struct i915_power_well_ops xelpdp_aux_power_well_ops = {
> > >  	.disable = xelpdp_aux_power_well_disable,
> > >  	.is_enabled = xelpdp_aux_power_well_enabled,
> > >  };
> > > +
> > > +const struct i915_power_well_ops xe2lpd_pica_power_well_ops = {
> > > +	.sync_hw = i9xx_power_well_sync_hw_noop,
> > > +	.enable = xe2lpd_pica_power_well_enable,
> > > +	.disable = xe2lpd_pica_power_well_disable,
> > > +	.is_enabled = xe2lpd_pica_power_well_enabled,
> > > +};
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.h b/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > index a8736588314d..9357a9a73c06 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > @@ -176,5 +176,6 @@ extern const struct i915_power_well_ops icl_aux_power_well_ops;
> > >  extern const struct i915_power_well_ops icl_ddi_power_well_ops;
> > >  extern const struct i915_power_well_ops tgl_tc_cold_off_ops;
> > >  extern const struct i915_power_well_ops xelpdp_aux_power_well_ops;
> > > +extern const struct i915_power_well_ops xe2lpd_pica_power_well_ops;
> > > 
> > >  #endif
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > > index b4e96baae25a..c869f5661530 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
> > > @@ -86,4 +86,9 @@
> > >  		 _XELPDP_DP_AUX_CH_DATA(__xe2lpd_aux_ch_idx(aux_ch), i) :	\
> > >  		 _XELPDP_DP_AUX_CH_DATA(aux_ch, i))
> > > 
> > > +/* PICA Power Well Control */
> > > +#define XE2LPD_PICA_PW_CTL			_MMIO(0x16fe04)
> > 
> > Is this the right header for this?  It doesn't seem directly related to
> > DP AUX.
> 
> In Xe2_LPD, those AUX registers are also
> 
> I had the same question myself while rebasing this patch as this is
> not related to DP. However the register itself seems to have the same
> functionality as the other registers above with power request/status.
> And if you look at bspec 69009, all of them are under pica for Xe2_LPD,
> too.
> 
> I'm not sure what was the criteria for the split of this DP header.
> It's clearer for things like gt, engine, snps phy, etc, but this one
> seems to have been more or less arbitrary.
> 
> Any suggestion for a better place? A new display/intel_pica_regs.h
> maybe? That may not be as future proof in the cases register move from
> one place to the other like happened in Xe2_LPD: see bspec 69009.  All
> the PORT_BUF_CTL*, PORT_HOTPLUG_CTL, etc should then be in this header,
> which doesn't match previous platforms.

I kind of like display/intel_pica_regs.h.  As you noted, a bunch of
stuff that's in intel_cx0_phy_regs.h today should really probably be in
a PICA header instead because those aren't actually PHY registers (the
real CX0 PHY registers are the things that can only be accessed over the
message bus like PHY_C10_VDR_CMN and such).

I'm not too concerned about where this lands in the short term though;
even i915_reg.h would be fine as a short-term dumping ground if
necessary.  Don't let the comment here block the LNL display work; we
can always do a separate follow-up series to clarify the placement of
some of our registers for MTL+LNL.


Matt

> 
> If instead of matching HW we try to match where it's used in SW, then
> maybe a intel_display_power_well_regs.h, but that too doesn't match
> previous platforms.
> 
> +Jani
> 
> Lucas De Marchi
> 
> > 
> > 
> > Matt
> > 
> > > +#define   XE2LPD_PICA_CTL_POWER_REQUEST		REG_BIT(31)
> > > +#define   XE2LPD_PICA_CTL_POWER_STATUS		REG_BIT(30)
> > > +
> > >  #endif /* __INTEL_DP_AUX_REGS_H__ */
> > > --
> > > 2.40.1
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c b/drivers/gpu/drm/i915/display/intel_display_power_map.c
index 0f1b93d139ca..31c11586ede5 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_map.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c
@@ -1536,6 +1536,38 @@  static const struct i915_power_well_desc_list xelpdp_power_wells[] = {
 	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
 };
 
+I915_DECL_PW_DOMAINS(xe2lpd_pwdoms_pica_tc,
+		     POWER_DOMAIN_PORT_DDI_LANES_TC1,
+		     POWER_DOMAIN_PORT_DDI_LANES_TC2,
+		     POWER_DOMAIN_PORT_DDI_LANES_TC3,
+		     POWER_DOMAIN_PORT_DDI_LANES_TC4,
+		     POWER_DOMAIN_AUX_USBC1,
+		     POWER_DOMAIN_AUX_USBC2,
+		     POWER_DOMAIN_AUX_USBC3,
+		     POWER_DOMAIN_AUX_USBC4,
+		     POWER_DOMAIN_AUX_TBT1,
+		     POWER_DOMAIN_AUX_TBT2,
+		     POWER_DOMAIN_AUX_TBT3,
+		     POWER_DOMAIN_AUX_TBT4,
+		     POWER_DOMAIN_INIT);
+
+static const struct i915_power_well_desc xe2lpd_power_wells_pica[] = {
+	{
+		.instances = &I915_PW_INSTANCES(I915_PW("PICA_TC",
+							&xe2lpd_pwdoms_pica_tc,
+							.id = DISP_PW_ID_NONE),
+					       ),
+		.ops = &xe2lpd_pica_power_well_ops,
+	},
+};
+
+static const struct i915_power_well_desc_list xe2lpd_power_wells[] = {
+	I915_PW_DESCRIPTORS(i9xx_power_wells_always_on),
+	I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
+	I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
+	I915_PW_DESCRIPTORS(xe2lpd_power_wells_pica),
+};
+
 static void init_power_well_domains(const struct i915_power_well_instance *inst,
 				    struct i915_power_well *power_well)
 {
@@ -1643,7 +1675,9 @@  int intel_display_power_map_init(struct i915_power_domains *power_domains)
 		return 0;
 	}
 
-	if (DISPLAY_VER(i915) >= 14)
+	if (DISPLAY_VER(i915) >= 20)
+		return set_power_wells(power_domains, xe2lpd_power_wells);
+	else if (DISPLAY_VER(i915) >= 14)
 		return set_power_wells(power_domains, xelpdp_power_wells);
 	else if (IS_DG2(i915))
 		return set_power_wells(power_domains, xehpd_power_wells);
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
index ca0714eba17a..adfe9901bde4 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
@@ -1833,6 +1833,43 @@  static bool xelpdp_aux_power_well_enabled(struct drm_i915_private *dev_priv,
 		XELPDP_DP_AUX_CH_CTL_POWER_STATUS;
 }
 
+static void xe2lpd_pica_power_well_enable(struct drm_i915_private *dev_priv,
+					  struct i915_power_well *power_well)
+{
+	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
+		     XE2LPD_PICA_CTL_POWER_REQUEST,
+		     XE2LPD_PICA_CTL_POWER_REQUEST);
+
+	if (intel_de_wait_for_set(dev_priv, XE2LPD_PICA_PW_CTL,
+				  XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
+		drm_dbg_kms(&dev_priv->drm, "pica power well enable timeout\n");
+
+		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when enabled");
+	}
+}
+
+static void xe2lpd_pica_power_well_disable(struct drm_i915_private *dev_priv,
+					   struct i915_power_well *power_well)
+{
+	intel_de_rmw(dev_priv, XE2LPD_PICA_PW_CTL,
+		     XE2LPD_PICA_CTL_POWER_REQUEST,
+		     0);
+
+	if (intel_de_wait_for_clear(dev_priv, XE2LPD_PICA_PW_CTL,
+				    XE2LPD_PICA_CTL_POWER_STATUS, 1)) {
+		drm_dbg_kms(&dev_priv->drm, "pica power well disable timeout\n");
+
+		drm_WARN(&dev_priv->drm, 1, "Power well PICA timeout when disabled");
+	}
+}
+
+static bool xe2lpd_pica_power_well_enabled(struct drm_i915_private *dev_priv,
+					   struct i915_power_well *power_well)
+{
+	return intel_de_read(dev_priv, XE2LPD_PICA_PW_CTL) &
+		XE2LPD_PICA_CTL_POWER_STATUS;
+}
+
 const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
 	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = i9xx_always_on_power_well_noop,
@@ -1952,3 +1989,10 @@  const struct i915_power_well_ops xelpdp_aux_power_well_ops = {
 	.disable = xelpdp_aux_power_well_disable,
 	.is_enabled = xelpdp_aux_power_well_enabled,
 };
+
+const struct i915_power_well_ops xe2lpd_pica_power_well_ops = {
+	.sync_hw = i9xx_power_well_sync_hw_noop,
+	.enable = xe2lpd_pica_power_well_enable,
+	.disable = xe2lpd_pica_power_well_disable,
+	.is_enabled = xe2lpd_pica_power_well_enabled,
+};
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.h b/drivers/gpu/drm/i915/display/intel_display_power_well.h
index a8736588314d..9357a9a73c06 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.h
@@ -176,5 +176,6 @@  extern const struct i915_power_well_ops icl_aux_power_well_ops;
 extern const struct i915_power_well_ops icl_ddi_power_well_ops;
 extern const struct i915_power_well_ops tgl_tc_cold_off_ops;
 extern const struct i915_power_well_ops xelpdp_aux_power_well_ops;
+extern const struct i915_power_well_ops xe2lpd_pica_power_well_ops;
 
 #endif
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
index b4e96baae25a..c869f5661530 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_regs.h
@@ -86,4 +86,9 @@ 
 		 _XELPDP_DP_AUX_CH_DATA(__xe2lpd_aux_ch_idx(aux_ch), i) :	\
 		 _XELPDP_DP_AUX_CH_DATA(aux_ch, i))
 
+/* PICA Power Well Control */
+#define XE2LPD_PICA_PW_CTL			_MMIO(0x16fe04)
+#define   XE2LPD_PICA_CTL_POWER_REQUEST		REG_BIT(31)
+#define   XE2LPD_PICA_CTL_POWER_STATUS		REG_BIT(30)
+
 #endif /* __INTEL_DP_AUX_REGS_H__ */