On 08-10-24, 11:30, Peter Griffin wrote: > > > + PHY_MODE_DP, > > > + PHY_MODE_UFS_HIBERN8_ENTER, > > > + PHY_MODE_UFS_HIBERN8_EXIT, > > > > I am not sure I like this. why should this be the model? Phy drivers > > should listen to pm events and handle this in pm_suspend/resume calls, > > why do we need this special mode here... > > There are a couple of reasons I added it here: > > 1) Whilst link hibern8 mode can be used as part of runtime PM and > system PM, it is also used outside of those contexts by ufshcd.c. The > host controller can enable UFSHCD_CAP_HIBERN8_WITH_CLK_GATING (which > will be the case for gs101 / Pixel 6) and the UFS clocks are gated and > link put into hibern8 mode for periods of inactivity. When that > happens the rest of the system isn't entering any sort of sleep state. > > 2) From looking at the existing code upstream ufs-qcom.c and > phy-qcom-qmp-ufs.c look to have similar requirements in that it needs > to program a set of specific register values depending on the UFS > gear. To achieve that they added PHY_MODE_UFS_HS_B and > PHY_MODE_UFS_HS_A modes here and then use phy_set_mode_ext() API in > ufs_qcom_power_up_sequence() to signal to the phy driver the UFS gear, > which is then used to choose which set of values to program to the > phy. > > The two new UFS phy modes added here for hibern8 are for a very > similar purpose (to choose a bunch of register values to program), so > I considered it consistent with what was already being done upstream > to signal between UFS host drivers and UFS phy drivers. Arguably I > guess we could have one "mode" PHY_MODE_UFS_HIBERN8 and use the > submode parameter to indicate whether we are entering (1) or exiting > (0) from it. I wasn't really sure what the rules/guidelines for the > submode parameter were though. Yes but not exactly. The HIBERN8_ENTER|EXIT sound like PM events rather than a PHY mode. If this is resultant from inactivity, then we should hook this up to runtime pm ? -- ~Vinod
