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.