Hi Vinod,
Thanks for your feedback!
On Thu, 17 Oct 2024 at 13:32, Vinod Koul <vkoul@xxxxxxxxxx> 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@xxxxxxxxxxx>
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.
