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

["Hogander, Jouni" <jouni.hogander@intel.com>]

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?

> 
> Also it kinda looks to me like for VBT port mapping we also
> want PHY_F essentially since the modern platforms make the
> VBT port mapping PHY based and xelpd_port_mapping() uses
> PORT_TC1<->DVO_PORT_*F. Not that we actually use enum phy
> in the VBT code atm, but I'm thinking we probably should
> since it might allow us to get rid of all those different
> mapping tables. Though the whole intel_port_to_phy()
> disaster needs to get cleaned up first IMO.
> 

BR,

Jouni Högander