| Message ID | 20241002201555.3332138-2-peter.griffin@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add phy support for hibern8 enter & exit on gs101 | expand |
Hi Peter, On 02-10-24, 21:15, Peter Griffin wrote: > Some UFS phys need to write hibernation specific values > when entering and exiting hibernate state. > > Add two new UFS phy modes to the phy framework so that this > is possible. One such platform that requires this is Pixel 6 > which uses the gs101 SoC. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > include/linux/phy/phy.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index 03cd5bae92d3..1874e55e2bb9 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -42,7 +42,9 @@ enum phy_mode { > PHY_MODE_MIPI_DPHY, > PHY_MODE_SATA, > PHY_MODE_LVDS, > - PHY_MODE_DP > + 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... Also, this is not a "mode" for phy
Hi Vinod, Thanks for your review. On Mon, 7 Oct 2024 at 07:28, Vinod Koul <vkoul@kernel.org> wrote: > > Hi Peter, > > On 02-10-24, 21:15, Peter Griffin wrote: > > Some UFS phys need to write hibernation specific values > > when entering and exiting hibernate state. > > > > Add two new UFS phy modes to the phy framework so that this > > is possible. One such platform that requires this is Pixel 6 > > which uses the gs101 SoC. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > include/linux/phy/phy.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > > index 03cd5bae92d3..1874e55e2bb9 100644 > > --- a/include/linux/phy/phy.h > > +++ b/include/linux/phy/phy.h > > @@ -42,7 +42,9 @@ enum phy_mode { > > PHY_MODE_MIPI_DPHY, > > PHY_MODE_SATA, > > PHY_MODE_LVDS, > > - PHY_MODE_DP > > + 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. Thanks, Peter
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 ?
Hi Vinod, Thanks for your feedback! On Thu, 17 Oct 2024 at 13:32, Vinod Koul <vkoul@kernel.org> wrote: > > 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 ? As I mentioned above HIBERN8 is implemented and runs independently of runtime pm in the upstream UFS stack. To be clear that isn't a ufs-exynos thing, but rather the core ufs stack in drivers/ufs/core/ufshcd.c. I've added Bart, Alim, Avri to this thread, as they are listed as reviewers of the core ufshcd file and also Sahitya who originally introduced this UFSHCD_CAP_HIBERN8_WITH_CLK_GATING functionality. Hopefully they can provide more details as to why hibern8 has been implemented like this outside of runtime pm. The original commit where this was introduced is commit 1ab27c9cf8b63dd8dec9e17b5c17721c7f3b6cc7 Author: Sahitya Tummala <quic_stummala@quicinc.com> Date: Thu Sep 25 15:32:32 2014 +0300 ufs: Add support for clock gating which mentions in the description that the timeout is typically less than the runtime suspend timeout. I think the main difference here versus other SoC UFS controllers and UFS phys is that the others don't appear to need any additional phy register writes when entering/leaving hibern8, which the ufs-exynos phy does. It looks like all the hibern8 callbacks upstream in the core UFS stack were added by Samsung for ufs-exynos. In downstream they don't split the ufs controller and ufs phy parts into separate drivers, it is all done from the controller glue driver so it isn't an issue. With regards to whether it is a "mode" or not, HIBERN8 is a M-Phy power state, but then PHY_MODE_UFS_HS_A and PHY_MODE_UFS_HS_B are also m-phy power states (HS_BURST with rate A or HS_BURST with rate b) so it seemed in keeping with what you are already doing here. regards, Peter.
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index 03cd5bae92d3..1874e55e2bb9 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -42,7 +42,9 @@ enum phy_mode { PHY_MODE_MIPI_DPHY, PHY_MODE_SATA, PHY_MODE_LVDS, - PHY_MODE_DP + PHY_MODE_DP, + PHY_MODE_UFS_HIBERN8_ENTER, + PHY_MODE_UFS_HIBERN8_EXIT, }; enum phy_media {
Some UFS phys need to write hibernation specific values when entering and exiting hibernate state. Add two new UFS phy modes to the phy framework so that this is possible. One such platform that requires this is Pixel 6 which uses the gs101 SoC. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- include/linux/phy/phy.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)