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.