[v2] drm/i915/display: Implement new combo phy initialization step
diff mbox series

Message ID 20200624193249.20725-1-jose.souza@intel.com
State New
Headers show
Series
  • [v2] drm/i915/display: Implement new combo phy initialization step
Related show

Commit Message

José Roberto de Souza June 24, 2020, 7:32 p.m. UTC
This is new step that was recently added to the combo phy
initialization.

v2:
- using intel_de_rmw()

BSpec: 49291
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 .../gpu/drm/i915/display/intel_combo_phy.c    | 23 +++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h               |  7 ++++++
 2 files changed, 30 insertions(+)

Comments

Matt Roper July 6, 2020, 11:08 p.m. UTC | #1
On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote:
> This is new step that was recently added to the combo phy
> initialization.
> 
> v2:
> - using intel_de_rmw()

Actually, I'm not sure whether this is valid; I believe we always have
to read from a lane register and can only write to the group registers:

        "Reads using a port group address usually cannot return correct
        data. For read/modify/write to a group, the read should be to
        one of the lane addresses, then the write to the group address."

> 
> BSpec: 49291
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_combo_phy.c    | 23 +++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h               |  7 ++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> index 77b04bb3ec62..115069833348 100644
> --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
>  	if (!icl_combo_phy_enabled(dev_priv, phy))
>  		return false;
>  
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy),
> +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);

If I'm reading the bspec correctly, it looks like both 00 and 01
represent div2 for this register.  So maybe rather than looking for 01
exactly here we should just check that bit 30 is off?


> +
> +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy),
> +				     DCC_MODE_SELECT_MASK,
> +				     DCC_MODE_SELECT_CONTINUOSLY);
> +	}
> +
>  	ret = cnl_verify_procmon_ref_values(dev_priv, phy);
>  
>  	if (phy_is_master(dev_priv, phy)) {
> @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
>  		intel_de_write(dev_priv, ICL_PHY_MISC(phy), val);
>  
>  skip_phy_misc:
> +		if (INTEL_GEN(dev_priv) >= 12) {

We may want to keep an eye on this part of the bspec; there seems to be
something a bit off with the bspec tagging of this block and I'm not
100% sure it was actually intended to apply to RKL too or not.

It seems like the bspec has these steps as the very first thing, even
before the "check whether the PHY is already initialized" step.
But since we're checking that the bits are set the way we want in the
verify function too, I don't think the ordering should matter.

> +			intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy),
> +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);

As noted above, maybe we should just clear bit 30 and leave bit 29 set
however it already was?


Matt

> +
> +			intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy),
> +				     DCC_MODE_SELECT_MASK,
> +				     DCC_MODE_SELECT_CONTINUOSLY);
> +		}
> +
>  		cnl_set_procmon_ref_values(dev_priv, phy);
>  
>  		if (phy_is_master(dev_priv, phy)) {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f09120cac89a..5469c9029f6d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define ICL_PORT_PCS_DW1_AUX(phy)	_MMIO(_ICL_PORT_PCS_DW_AUX(1, phy))
>  #define ICL_PORT_PCS_DW1_GRP(phy)	_MMIO(_ICL_PORT_PCS_DW_GRP(1, phy))
>  #define ICL_PORT_PCS_DW1_LN0(phy)	_MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy))
> +#define   DCC_MODE_SELECT_MASK		(0x3 << 20)
> +#define   DCC_MODE_SELECT_CONTINUOSLY	(0x3 << 20)
>  #define   COMMON_KEEPER_EN		(1 << 26)
>  #define   LATENCY_OPTIM_MASK		(0x3 << 2)
>  #define   LATENCY_OPTIM_VAL(x)		((x) << 2)
> @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define   N_SCALAR(x)			((x) << 24)
>  #define   N_SCALAR_MASK			(0x7F << 24)
>  
> +#define ICL_PORT_TX_DW8_GRP(phy)		_MMIO(_ICL_PORT_TX_DW_GRP(8, phy))
> +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL		REG_BIT(31)
> +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK	REG_GENMASK(30, 29)
> +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2	REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1)
> +
>  #define _ICL_DPHY_CHKN_REG			0x194
>  #define ICL_DPHY_CHKN(port)			_MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG)
>  #define   ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP	REG_BIT(7)
> -- 
> 2.27.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
José Roberto de Souza July 6, 2020, 11:10 p.m. UTC | #2
On Mon, 2020-07-06 at 16:08 -0700, Matt Roper wrote:
> On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote:
> > This is new step that was recently added to the combo phy
> > initialization.
> > 
> > v2:
> > - using intel_de_rmw()
> 
> Actually, I'm not sure whether this is valid; I believe we always have
> to read from a lane register and can only write to the group registers:
> 
>         "Reads using a port group address usually cannot return correct
>         data. For read/modify/write to a group, the read should be to
>         one of the lane addresses, then the write to the group address."

Yep, v3 is doing the right thing: https://patchwork.freedesktop.org/series/78796/

> 
> > BSpec: 49291
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_combo_phy.c    | 23 +++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h               |  7 ++++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > index 77b04bb3ec62..115069833348 100644
> > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
> >  	if (!icl_combo_phy_enabled(dev_priv, phy))
> >  		return false;
> >  
> > +	if (INTEL_GEN(dev_priv) >= 12) {
> > +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy),
> > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
> 
> If I'm reading the bspec correctly, it looks like both 00 and 01
> represent div2 for this register.  So maybe rather than looking for 01
> exactly here we should just check that bit 30 is off?
> 
> 
> > +
> > +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy),
> > +				     DCC_MODE_SELECT_MASK,
> > +				     DCC_MODE_SELECT_CONTINUOSLY);
> > +	}
> > +
> >  	ret = cnl_verify_procmon_ref_values(dev_priv, phy);
> >  
> >  	if (phy_is_master(dev_priv, phy)) {
> > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> >  		intel_de_write(dev_priv, ICL_PHY_MISC(phy), val);
> >  
> >  skip_phy_misc:
> > +		if (INTEL_GEN(dev_priv) >= 12) {
> 
> We may want to keep an eye on this part of the bspec; there seems to be
> something a bit off with the bspec tagging of this block and I'm not
> 100% sure it was actually intended to apply to RKL too or not.
> 
> It seems like the bspec has these steps as the very first thing, even
> before the "check whether the PHY is already initialized" step.
> But since we're checking that the bits are set the way we want in the
> verify function too, I don't think the ordering should matter.
> 
> > +			intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy),
> > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
> 
> As noted above, maybe we should just clear bit 30 and leave bit 29 set
> however it already was?
> 
> 
> Matt
> 
> > +
> > +			intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy),
> > +				     DCC_MODE_SELECT_MASK,
> > +				     DCC_MODE_SELECT_CONTINUOSLY);
> > +		}
> > +
> >  		cnl_set_procmon_ref_values(dev_priv, phy);
> >  
> >  		if (phy_is_master(dev_priv, phy)) {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f09120cac89a..5469c9029f6d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >  #define ICL_PORT_PCS_DW1_AUX(phy)	_MMIO(_ICL_PORT_PCS_DW_AUX(1, phy))
> >  #define ICL_PORT_PCS_DW1_GRP(phy)	_MMIO(_ICL_PORT_PCS_DW_GRP(1, phy))
> >  #define ICL_PORT_PCS_DW1_LN0(phy)	_MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy))
> > +#define   DCC_MODE_SELECT_MASK		(0x3 << 20)
> > +#define   DCC_MODE_SELECT_CONTINUOSLY	(0x3 << 20)
> >  #define   COMMON_KEEPER_EN		(1 << 26)
> >  #define   LATENCY_OPTIM_MASK		(0x3 << 2)
> >  #define   LATENCY_OPTIM_VAL(x)		((x) << 2)
> > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >  #define   N_SCALAR(x)			((x) << 24)
> >  #define   N_SCALAR_MASK			(0x7F << 24)
> >  
> > +#define ICL_PORT_TX_DW8_GRP(phy)		_MMIO(_ICL_PORT_TX_DW_GRP(8, phy))
> > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL		REG_BIT(31)
> > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK	REG_GENMASK(30, 29)
> > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2	REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1)
> > +
> >  #define _ICL_DPHY_CHKN_REG			0x194
> >  #define ICL_DPHY_CHKN(port)			_MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG)
> >  #define   ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP	REG_BIT(7)
> > -- 
> > 2.27.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Matt Roper July 6, 2020, 11:28 p.m. UTC | #3
On Mon, Jul 06, 2020 at 04:10:15PM -0700, Souza, Jose wrote:
> On Mon, 2020-07-06 at 16:08 -0700, Matt Roper wrote:
> > On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote:
> > > This is new step that was recently added to the combo phy
> > > initialization.
> > > 
> > > v2:
> > > - using intel_de_rmw()
> > 
> > Actually, I'm not sure whether this is valid; I believe we always have
> > to read from a lane register and can only write to the group registers:
> > 
> >         "Reads using a port group address usually cannot return correct
> >         data. For read/modify/write to a group, the read should be to
> >         one of the lane addresses, then the write to the group address."
> 
> Yep, v3 is doing the right thing: https://patchwork.freedesktop.org/series/78796/

Ah, I overlooked that, thanks.  I think my other comments below may
still apply to v3 though.


Matt

> 
> > 
> > > BSpec: 49291
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  .../gpu/drm/i915/display/intel_combo_phy.c    | 23 +++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h               |  7 ++++++
> > >  2 files changed, 30 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > index 77b04bb3ec62..115069833348 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
> > >  	if (!icl_combo_phy_enabled(dev_priv, phy))
> > >  		return false;
> > >  
> > > +	if (INTEL_GEN(dev_priv) >= 12) {
> > > +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy),
> > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
> > 
> > If I'm reading the bspec correctly, it looks like both 00 and 01
> > represent div2 for this register.  So maybe rather than looking for 01
> > exactly here we should just check that bit 30 is off?
> > 
> > 
> > > +
> > > +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy),
> > > +				     DCC_MODE_SELECT_MASK,
> > > +				     DCC_MODE_SELECT_CONTINUOSLY);
> > > +	}
> > > +
> > >  	ret = cnl_verify_procmon_ref_values(dev_priv, phy);
> > >  
> > >  	if (phy_is_master(dev_priv, phy)) {
> > > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> > >  		intel_de_write(dev_priv, ICL_PHY_MISC(phy), val);
> > >  
> > >  skip_phy_misc:
> > > +		if (INTEL_GEN(dev_priv) >= 12) {
> > 
> > We may want to keep an eye on this part of the bspec; there seems to be
> > something a bit off with the bspec tagging of this block and I'm not
> > 100% sure it was actually intended to apply to RKL too or not.
> > 
> > It seems like the bspec has these steps as the very first thing, even
> > before the "check whether the PHY is already initialized" step.
> > But since we're checking that the bits are set the way we want in the
> > verify function too, I don't think the ordering should matter.
> > 
> > > +			intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy),
> > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
> > 
> > As noted above, maybe we should just clear bit 30 and leave bit 29 set
> > however it already was?
> > 
> > 
> > Matt
> > 
> > > +
> > > +			intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy),
> > > +				     DCC_MODE_SELECT_MASK,
> > > +				     DCC_MODE_SELECT_CONTINUOSLY);
> > > +		}
> > > +
> > >  		cnl_set_procmon_ref_values(dev_priv, phy);
> > >  
> > >  		if (phy_is_master(dev_priv, phy)) {
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index f09120cac89a..5469c9029f6d 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> > >  #define ICL_PORT_PCS_DW1_AUX(phy)	_MMIO(_ICL_PORT_PCS_DW_AUX(1, phy))
> > >  #define ICL_PORT_PCS_DW1_GRP(phy)	_MMIO(_ICL_PORT_PCS_DW_GRP(1, phy))
> > >  #define ICL_PORT_PCS_DW1_LN0(phy)	_MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy))
> > > +#define   DCC_MODE_SELECT_MASK		(0x3 << 20)
> > > +#define   DCC_MODE_SELECT_CONTINUOSLY	(0x3 << 20)
> > >  #define   COMMON_KEEPER_EN		(1 << 26)
> > >  #define   LATENCY_OPTIM_MASK		(0x3 << 2)
> > >  #define   LATENCY_OPTIM_VAL(x)		((x) << 2)
> > > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> > >  #define   N_SCALAR(x)			((x) << 24)
> > >  #define   N_SCALAR_MASK			(0x7F << 24)
> > >  
> > > +#define ICL_PORT_TX_DW8_GRP(phy)		_MMIO(_ICL_PORT_TX_DW_GRP(8, phy))
> > > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL		REG_BIT(31)
> > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK	REG_GENMASK(30, 29)
> > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2	REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1)
> > > +
> > >  #define _ICL_DPHY_CHKN_REG			0x194
> > >  #define ICL_DPHY_CHKN(port)			_MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG)
> > >  #define   ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP	REG_BIT(7)
> > > -- 
> > > 2.27.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
José Roberto de Souza July 7, 2020, 12:14 a.m. UTC | #4
On Mon, 2020-07-06 at 16:28 -0700, Matt Roper wrote:
> On Mon, Jul 06, 2020 at 04:10:15PM -0700, Souza, Jose wrote:
> > On Mon, 2020-07-06 at 16:08 -0700, Matt Roper wrote:
> > > On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote:
> > > > This is new step that was recently added to the combo phy
> > > > initialization.
> > > > 
> > > > v2:
> > > > - using intel_de_rmw()
> > > 
> > > Actually, I'm not sure whether this is valid; I believe we always have
> > > to read from a lane register and can only write to the group registers:
> > > 
> > >         "Reads using a port group address usually cannot return correct
> > >         data. For read/modify/write to a group, the read should be to
> > >         one of the lane addresses, then the write to the group address."
> > 
> > Yep, v3 is doing the right thing: https://patchwork.freedesktop.org/series/78796/
> 
> Ah, I overlooked that, thanks.  I think my other comments below may
> still apply to v3 though.

Ops missed those, answers bellow.

> 
> 
> Matt
> 
> > > > BSpec: 49291
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  .../gpu/drm/i915/display/intel_combo_phy.c    | 23 +++++++++++++++++++
> > > >  drivers/gpu/drm/i915/i915_reg.h               |  7 ++++++
> > > >  2 files changed, 30 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > > index 77b04bb3ec62..115069833348 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
> > > >  	if (!icl_combo_phy_enabled(dev_priv, phy))
> > > >  		return false;
> > > >  
> > > > +	if (INTEL_GEN(dev_priv) >= 12) {
> > > > +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy),
> > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
> > > 
> > > If I'm reading the bspec correctly, it looks like both 00 and 01
> > > represent div2 for this register.  So maybe rather than looking for 01
> > > exactly here we should just check that bit 30 is off?

That is a micro optimization that would cause loss in code readability, someone casually reading might get confused of why only bit 30 is checked,
also we would need to add a different macro? Don't look worthy.
The default value of bit 31 is off so it will trigger the phy reprogram anyways.

> > > 
> > > 
> > > > +
> > > > +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy),
> > > > +				     DCC_MODE_SELECT_MASK,
> > > > +				     DCC_MODE_SELECT_CONTINUOSLY);
> > > > +	}
> > > > +
> > > >  	ret = cnl_verify_procmon_ref_values(dev_priv, phy);
> > > >  
> > > >  	if (phy_is_master(dev_priv, phy)) {
> > > > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> > > >  		intel_de_write(dev_priv, ICL_PHY_MISC(phy), val);
> > > >  
> > > >  skip_phy_misc:
> > > > +		if (INTEL_GEN(dev_priv) >= 12) {
> > > 
> > > We may want to keep an eye on this part of the bspec; there seems to be
> > > something a bit off with the bspec tagging of this block and I'm not
> > > 100% sure it was actually intended to apply to RKL too or not.

Ok, I'm receiving notifications of this page, will update if needed but it would be odd only RKL not needing this sequence while TGL and DG1 need it.

> > > 
> > > It seems like the bspec has these steps as the very first thing, even
> > > before the "check whether the PHY is already initialized" step.
> > > But since we're checking that the bits are set the way we want in the
> > > verify function too, I don't think the ordering should matter.

Yep, re program it each time is a waste of time.

> > > 
> > > > +			intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy),
> > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
> > > 
> > > As noted above, maybe we should just clear bit 30 and leave bit 29 set
> > > however it already was?
> > > 
> > > 
> > > Matt
> > > 
> > > > +
> > > > +			intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy),
> > > > +				     DCC_MODE_SELECT_MASK,
> > > > +				     DCC_MODE_SELECT_CONTINUOSLY);
> > > > +		}
> > > > +
> > > >  		cnl_set_procmon_ref_values(dev_priv, phy);
> > > >  
> > > >  		if (phy_is_master(dev_priv, phy)) {
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index f09120cac89a..5469c9029f6d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> > > >  #define ICL_PORT_PCS_DW1_AUX(phy)	_MMIO(_ICL_PORT_PCS_DW_AUX(1, phy))
> > > >  #define ICL_PORT_PCS_DW1_GRP(phy)	_MMIO(_ICL_PORT_PCS_DW_GRP(1, phy))
> > > >  #define ICL_PORT_PCS_DW1_LN0(phy)	_MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy))
> > > > +#define   DCC_MODE_SELECT_MASK		(0x3 << 20)
> > > > +#define   DCC_MODE_SELECT_CONTINUOSLY	(0x3 << 20)
> > > >  #define   COMMON_KEEPER_EN		(1 << 26)
> > > >  #define   LATENCY_OPTIM_MASK		(0x3 << 2)
> > > >  #define   LATENCY_OPTIM_VAL(x)		((x) << 2)
> > > > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> > > >  #define   N_SCALAR(x)			((x) << 24)
> > > >  #define   N_SCALAR_MASK			(0x7F << 24)
> > > >  
> > > > +#define ICL_PORT_TX_DW8_GRP(phy)		_MMIO(_ICL_PORT_TX_DW_GRP(8, phy))
> > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL		REG_BIT(31)
> > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK	REG_GENMASK(30, 29)
> > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2	REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1)
> > > > +
> > > >  #define _ICL_DPHY_CHKN_REG			0x194
> > > >  #define ICL_DPHY_CHKN(port)			_MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG)
> > > >  #define   ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP	REG_BIT(7)
> > > > -- 
> > > > 2.27.0
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Matt Roper July 7, 2020, 12:27 a.m. UTC | #5
On Mon, Jul 06, 2020 at 05:14:04PM -0700, Souza, Jose wrote:
> On Mon, 2020-07-06 at 16:28 -0700, Matt Roper wrote:
> > On Mon, Jul 06, 2020 at 04:10:15PM -0700, Souza, Jose wrote:
> > > On Mon, 2020-07-06 at 16:08 -0700, Matt Roper wrote:
> > > > On Wed, Jun 24, 2020 at 12:32:49PM -0700, José Roberto de Souza wrote:
> > > > > This is new step that was recently added to the combo phy
> > > > > initialization.
> > > > > 
> > > > > v2:
> > > > > - using intel_de_rmw()
> > > > 
> > > > Actually, I'm not sure whether this is valid; I believe we always have
> > > > to read from a lane register and can only write to the group registers:
> > > > 
> > > >         "Reads using a port group address usually cannot return correct
> > > >         data. For read/modify/write to a group, the read should be to
> > > >         one of the lane addresses, then the write to the group address."
> > > 
> > > Yep, v3 is doing the right thing: https://patchwork.freedesktop.org/series/78796/
> > 
> > Ah, I overlooked that, thanks.  I think my other comments below may
> > still apply to v3 though.
> 
> Ops missed those, answers bellow.
> 
> > 
> > 
> > Matt
> > 
> > > > > BSpec: 49291
> > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  .../gpu/drm/i915/display/intel_combo_phy.c    | 23 +++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/i915_reg.h               |  7 ++++++
> > > > >  2 files changed, 30 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > > > index 77b04bb3ec62..115069833348 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
> > > > > @@ -264,6 +264,18 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
> > > > >  	if (!icl_combo_phy_enabled(dev_priv, phy))
> > > > >  		return false;
> > > > >  
> > > > > +	if (INTEL_GEN(dev_priv) >= 12) {
> > > > > +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy),
> > > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> > > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
> > > > 
> > > > If I'm reading the bspec correctly, it looks like both 00 and 01
> > > > represent div2 for this register.  So maybe rather than looking for 01
> > > > exactly here we should just check that bit 30 is off?
> 
> That is a micro optimization that would cause loss in code readability, someone casually reading might get confused of why only bit 30 is checked,
> also we would need to add a different macro? Don't look worthy.
> The default value of bit 31 is off so it will trigger the phy reprogram anyways.

My main thinking was that the BIOS might have already programmed this
register according to the new bspec directions, but chosen 00 instead of
01 as its value.  So testing a "CLK_DIV_SEL_HIGHDIV" rather than
CLK_SEL_MASK would allow us to avoid an unnecessary reprogram.  But as
you said, it probably doesn't hurt anything to reprogram more often than
we need to, so we can probably leave it as-is.

Might be worth adding some extra explanation about that to the commit
message.  But otherwise,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>



> 
> > > > 
> > > > 
> > > > > +
> > > > > +		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy),
> > > > > +				     DCC_MODE_SELECT_MASK,
> > > > > +				     DCC_MODE_SELECT_CONTINUOSLY);
> > > > > +	}
> > > > > +
> > > > >  	ret = cnl_verify_procmon_ref_values(dev_priv, phy);
> > > > >  
> > > > >  	if (phy_is_master(dev_priv, phy)) {
> > > > > @@ -375,6 +387,17 @@ static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> > > > >  		intel_de_write(dev_priv, ICL_PHY_MISC(phy), val);
> > > > >  
> > > > >  skip_phy_misc:
> > > > > +		if (INTEL_GEN(dev_priv) >= 12) {
> > > > 
> > > > We may want to keep an eye on this part of the bspec; there seems to be
> > > > something a bit off with the bspec tagging of this block and I'm not
> > > > 100% sure it was actually intended to apply to RKL too or not.
> 
> Ok, I'm receiving notifications of this page, will update if needed but it would be odd only RKL not needing this sequence while TGL and DG1 need it.
> 
> > > > 
> > > > It seems like the bspec has these steps as the very first thing, even
> > > > before the "check whether the PHY is already initialized" step.
> > > > But since we're checking that the bits are set the way we want in the
> > > > verify function too, I don't think the ordering should matter.
> 
> Yep, re program it each time is a waste of time.
> 
> > > > 
> > > > > +			intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy),
> > > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
> > > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
> > > > > +				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
> > > > 
> > > > As noted above, maybe we should just clear bit 30 and leave bit 29 set
> > > > however it already was?
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > > +
> > > > > +			intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy),
> > > > > +				     DCC_MODE_SELECT_MASK,
> > > > > +				     DCC_MODE_SELECT_CONTINUOSLY);
> > > > > +		}
> > > > > +
> > > > >  		cnl_set_procmon_ref_values(dev_priv, phy);
> > > > >  
> > > > >  		if (phy_is_master(dev_priv, phy)) {
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index f09120cac89a..5469c9029f6d 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -1974,6 +1974,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> > > > >  #define ICL_PORT_PCS_DW1_AUX(phy)	_MMIO(_ICL_PORT_PCS_DW_AUX(1, phy))
> > > > >  #define ICL_PORT_PCS_DW1_GRP(phy)	_MMIO(_ICL_PORT_PCS_DW_GRP(1, phy))
> > > > >  #define ICL_PORT_PCS_DW1_LN0(phy)	_MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy))
> > > > > +#define   DCC_MODE_SELECT_MASK		(0x3 << 20)
> > > > > +#define   DCC_MODE_SELECT_CONTINUOSLY	(0x3 << 20)
> > > > >  #define   COMMON_KEEPER_EN		(1 << 26)
> > > > >  #define   LATENCY_OPTIM_MASK		(0x3 << 2)
> > > > >  #define   LATENCY_OPTIM_VAL(x)		((x) << 2)
> > > > > @@ -2072,6 +2074,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> > > > >  #define   N_SCALAR(x)			((x) << 24)
> > > > >  #define   N_SCALAR_MASK			(0x7F << 24)
> > > > >  
> > > > > +#define ICL_PORT_TX_DW8_GRP(phy)		_MMIO(_ICL_PORT_TX_DW_GRP(8, phy))
> > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_SEL		REG_BIT(31)
> > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK	REG_GENMASK(30, 29)
> > > > > +#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2	REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1)
> > > > > +
> > > > >  #define _ICL_DPHY_CHKN_REG			0x194
> > > > >  #define ICL_DPHY_CHKN(port)			_MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG)
> > > > >  #define   ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP	REG_BIT(7)
> > > > > -- 
> > > > > 2.27.0
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_combo_phy.c b/drivers/gpu/drm/i915/display/intel_combo_phy.c
index 77b04bb3ec62..115069833348 100644
--- a/drivers/gpu/drm/i915/display/intel_combo_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_combo_phy.c
@@ -264,6 +264,18 @@  static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
 	if (!icl_combo_phy_enabled(dev_priv, phy))
 		return false;
 
+	if (INTEL_GEN(dev_priv) >= 12) {
+		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_TX_DW8_GRP(phy),
+				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
+				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
+				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
+				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
+
+		ret &= check_phy_reg(dev_priv, phy, ICL_PORT_PCS_DW1_GRP(phy),
+				     DCC_MODE_SELECT_MASK,
+				     DCC_MODE_SELECT_CONTINUOSLY);
+	}
+
 	ret = cnl_verify_procmon_ref_values(dev_priv, phy);
 
 	if (phy_is_master(dev_priv, phy)) {
@@ -375,6 +387,17 @@  static void icl_combo_phys_init(struct drm_i915_private *dev_priv)
 		intel_de_write(dev_priv, ICL_PHY_MISC(phy), val);
 
 skip_phy_misc:
+		if (INTEL_GEN(dev_priv) >= 12) {
+			intel_de_rmw(dev_priv, ICL_PORT_TX_DW8_GRP(phy),
+				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK,
+				     ICL_PORT_TX_DW8_ODCC_CLK_SEL |
+				     ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2);
+
+			intel_de_rmw(dev_priv, ICL_PORT_PCS_DW1_GRP(phy),
+				     DCC_MODE_SELECT_MASK,
+				     DCC_MODE_SELECT_CONTINUOSLY);
+		}
+
 		cnl_set_procmon_ref_values(dev_priv, phy);
 
 		if (phy_is_master(dev_priv, phy)) {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f09120cac89a..5469c9029f6d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1974,6 +1974,8 @@  static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define ICL_PORT_PCS_DW1_AUX(phy)	_MMIO(_ICL_PORT_PCS_DW_AUX(1, phy))
 #define ICL_PORT_PCS_DW1_GRP(phy)	_MMIO(_ICL_PORT_PCS_DW_GRP(1, phy))
 #define ICL_PORT_PCS_DW1_LN0(phy)	_MMIO(_ICL_PORT_PCS_DW_LN(1, 0, phy))
+#define   DCC_MODE_SELECT_MASK		(0x3 << 20)
+#define   DCC_MODE_SELECT_CONTINUOSLY	(0x3 << 20)
 #define   COMMON_KEEPER_EN		(1 << 26)
 #define   LATENCY_OPTIM_MASK		(0x3 << 2)
 #define   LATENCY_OPTIM_VAL(x)		((x) << 2)
@@ -2072,6 +2074,11 @@  static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define   N_SCALAR(x)			((x) << 24)
 #define   N_SCALAR_MASK			(0x7F << 24)
 
+#define ICL_PORT_TX_DW8_GRP(phy)		_MMIO(_ICL_PORT_TX_DW_GRP(8, phy))
+#define ICL_PORT_TX_DW8_ODCC_CLK_SEL		REG_BIT(31)
+#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK	REG_GENMASK(30, 29)
+#define ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_DIV2	REG_FIELD_PREP(ICL_PORT_TX_DW8_ODCC_CLK_DIV_SEL_MASK, 0x1)
+
 #define _ICL_DPHY_CHKN_REG			0x194
 #define ICL_DPHY_CHKN(port)			_MMIO(_ICL_COMBOPHY(port) + _ICL_DPHY_CHKN_REG)
 #define   ICL_DPHY_CHKN_AFE_OVER_PPI_STRAP	REG_BIT(7)