[Intel-gfx] [PATCH 3/3] drm/i915: Fix for PHY_MISC_TC1 offset

[Lucas De Marchi]

On Wed, Feb 16, 2022 at 09:36:02AM +0000, Hogander, Jouni wrote:
>On Wed, 2022-02-16 at 10:50 +0200, Ville Syrjälä wrote:
>> On Tue, Feb 15, 2022 at 11:21:54AM +0530, Ramalingam C wrote:
>> > From: Jouni Högander <jouni.hogander at intel.com>
>> >
>> > Currently ICL_PHY_MISC macro is returning offset 0x64C10 for PHY_E
>> > port. Correct offset is 0x64C14.
>>
>> Why is it PHY_E and not PHY_F?
>
>This is a valid question. It seems we have followed intel_phy_is_snps()
>here:
>
>// snip
>else if (IS_DG2(dev_priv))
>		/*
>		 * All four "combo" ports and the TC1 port (PHY E) use
>		 * Synopsis PHYs.
>		 */
>		return phy <= PHY_E;
>// snip

And this is actually the bug that we had. We wouldn't need to bring the
incomplete support for the 5th port if this single had changed:  it's
often preferred to prepare the driver first and enable the port/phy as
the last step:

-		return phy <= PHY_E;
+		return phy <= PHY_D;

With possibly a change in the commit above. Because in
drivers/gpu/drm/i915/display/intel_snps_phy.c we do:

intel_snps_phy_wait_for_calibration()
{
...
         for_each_phy_masked(phy, ~0) {
                 if (!intel_phy_is_snps(i915, phy))
                         continue;
...
}

Relying on intel_phy_is_snps() to mask out the unavailable phys.

However, since now we almost have the extra port wired up, I'm not going
to push back on it. Let's just add a comment on the commit message.
And since going with this approach is also acked by Ville who preferred
to contain the additional mapping inside intel_phy_snps.c:

Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

Lucas De Marchi


>
>According to spec port E is "No connection". Better place to fix this
>could be intel_phy_is_snps() itself?
>
>>
>> > Fix this by handling PHY_E port seprately.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> > Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
>> > Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_snps_phy.c | 2 +-
>> >  drivers/gpu/drm/i915/i915_reg.h               | 6 ++++--
>> >  2 files changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c
>> > b/drivers/gpu/drm/i915/display/intel_snps_phy.c
>> > index c60575cb5368..f08061c748b3 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_snps_phy.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c
>> > @@ -32,7 +32,7 @@ void intel_snps_phy_wait_for_calibration(struct
>> > drm_i915_private *i915)
>> >  		if (!intel_phy_is_snps(i915, phy))
>> >  			continue;
>> >
>> > -		if (intel_de_wait_for_clear(i915, ICL_PHY_MISC(phy),
>> > +		if (intel_de_wait_for_clear(i915, DG2_PHY_MISC(phy),
>> >  					    DG2_PHY_DP_TX_ACK_MASK,
>> > 25))
>> >  			drm_err(&i915->drm, "SNPS PHY %c failed to
>> > calibrate after 25ms.\n",
>> >  				phy);
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > b/drivers/gpu/drm/i915/i915_reg.h
>> > index 4d12abb2d7ff..354c25f483cb 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -9559,8 +9559,10 @@ enum skl_power_gate {
>> >
>> >  #define _ICL_PHY_MISC_A		0x64C00
>> >  #define _ICL_PHY_MISC_B		0x64C04
>> > -#define ICL_PHY_MISC(port)	_MMIO_PORT(port, _ICL_PHY_MISC_A, \
>> > -						 _ICL_PHY_MISC_B)
>> > +#define _DG2_PHY_MISC_TC1	0x64C14 /* TC1="PHY E" but offset as if
>> > "PHY F" */
>> > +#define ICL_PHY_MISC(port)	_MMIO_PORT(port, _ICL_PHY_MISC_A,
>> > _ICL_PHY_MISC_B)
>> > +#define DG2_PHY_MISC(port)	((port) == PHY_E ?
>> > _MMIO(_DG2_PHY_MISC_TC1) : \
>> > +				 ICL_PHY_MISC(port))
>> >  #define  ICL_PHY_MISC_MUX_DDID			(1 << 28)
>> >  #define  ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN	(1 << 23)
>> >  #define  DG2_PHY_DP_TX_ACK_MASK			REG_GENMASK(23,
>> > 20)
>> > --
>> > 2.20.1
>
>BR,
>
>Jouni Högander