Re: [PATCH 1/3] phy: Add UFS phy hibernate modes

[Peter Griffin]

Hi Vinod,

Thanks for your review.

On Mon, 7 Oct 2024 at 07:28, Vinod Koul <vkoul@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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