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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 16, 2022 at 05:01:35PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 16, 2022 at 02:11:54PM +0000, Hogander, Jouni wrote:
> > On Wed, 2022-02-16 at 12:07 +0200, Ville Syrjälä wrote:
> > > 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@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
> > > > 
> > > > According to spec port E is "No connection". Better place to fix
> > > > this
> > > > could be intel_phy_is_snps() itself?
> > > 
> > > I think the crucial question is where are all the places that
> > > the results of intel_port_to_phy() get used.
> > > 
> > > I do see that for all the actual snps phy registers we
> > > do want PHY_E, but maybe it would be better to have a local
> > > SNPS_PHY enum just for intel_snps_phy.c, and leave the other
> > > phy thing for everything else?
> > > 
> > > Not sure if there is some other register we index with the
> > > phy that specifically wants PHY_E?
> > 
> > I went through registers accesses in intel_snps_phy.c. It is actually
> > only this one register which offset is wrong with PHY_E. Everything
> > else seems to be assuming PHY_E including those SNPS_* registers (as
> > you mentioned). I'm starting to think it would be overkill to open up
> > this phy enum for this purpose. I would propose to stick with current
> > patch. Maybe just update commit message. What do you think?
> 
> I would put it the other way. It is *only* the SNPS PHY IP registers
> that use the wonky offsets (unless you found some others?). Everythting
> on the Intel IP side wants it to be PHY_F.
> 
> So still would make more sense to me to add a new enum for the
> SNPS PHY instance and remap across the boundary. Otherwise we're
> just propagating this madness everwhere rather than containing in
> the SNPS PHY implementation.

Seems people want this is asap. I suppose it'll do as a temporary
measure given the phy stuff is already such mess.
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

As for the proper way to do stuff, I'm thinking roughly:
enum intel_spns_phy {
	SNPS_PHY_A,
	...
	SNPS_PHY_TC1, // == current PHY_E in value
};
and I think that can stay entirely inside intel_snps_phy.c.

As for our currnet enum phy I think we could start with something like
this:
enum phy {
	PHY_A,
	...
	PHY_F,

	PHY_TC1 = PHY_F,
	...
};

I think that should make it line up with PHY_MISC stuff and the 
VBT as well. So in the VBT code we could nuke all those crazy mapping
tables and just do:
 old platform: port -> VBT port
 new platform: phy -> VBT port

And we could probably have encoder->phy which gets populated
in the encoder init per-platform, similar to hpd_pin. That
would get rid of the intel_port_to_phy() disaster.

-- 
Ville Syrjälä
Intel