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@xxxxxxxxx> > > > > > > > > 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
