[v3,2/3] phy: qcom-snps-femto-v2: add system sleep PM ops

The downstream driver [1] implements set_suspend(), which deals with
both runtime and system sleep/resume. The upstream driver already has
runtime PM ops, so add the system sleep PM ops as well, reusing the same
code as the runtime PM ops.

[1] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c

Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Hi Bjorn,

On Thu, Jun 22, 2023 at 02:43:07PM -0700, Bjorn Andersson wrote:
> On Thu, Jun 22, 2023 at 03:40:19PM -0400, Adrien Thierry wrote:
> > The downstream driver [1] implements set_suspend(), which deals with
> > both runtime and system sleep/resume. The upstream driver already has
> > runtime PM ops, so add the system sleep PM ops as well, reusing the same
> > code as the runtime PM ops.
> > 
> > [1] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c
> > 
> > Signed-off-by: Adrien Thierry <athierry@redhat.com>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > index ce1d2f8b568a..378a5029f61e 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > @@ -179,7 +179,7 @@ static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
> >  	readl_relaxed(base + offset);
> >  }
> >  
> > -static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
> > +static int qcom_snps_hsphy_do_suspend(struct qcom_snps_hsphy *hsphy)
> >  {
> >  	dev_dbg(&hsphy->phy->dev, "Suspend QCOM SNPS PHY\n");
> >  
> > @@ -199,7 +199,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
> >  	return 0;
> >  }
> >  
> > -static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
> > +static int qcom_snps_hsphy_do_resume(struct qcom_snps_hsphy *hsphy)
> >  {
> >  	int ret;
> >  
> > @@ -214,25 +214,25 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
> >  	return 0;
> >  }
> >  
> > -static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device *dev)
> > +static int __maybe_unused qcom_snps_hsphy_suspend(struct device *dev)
> >  {
> >  	struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev);
> >  
> >  	if (!hsphy->phy_initialized)
> >  		return 0;
> >  
> > -	qcom_snps_hsphy_suspend(hsphy);
> > +	qcom_snps_hsphy_do_suspend(hsphy);
> >  	return 0;
> >  }
> >  
> > -static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev)
> > +static int __maybe_unused qcom_snps_hsphy_resume(struct device *dev)
> >  {
> >  	struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev);
> >  
> >  	if (!hsphy->phy_initialized)
> >  		return 0;
> >  
> > -	qcom_snps_hsphy_resume(hsphy);
> > +	qcom_snps_hsphy_do_resume(hsphy);
> >  	return 0;
> >  }
> >  
> > @@ -518,8 +518,10 @@ static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
> >  MODULE_DEVICE_TABLE(of, qcom_snps_hsphy_of_match_table);
> >  
> >  static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
> > -	SET_RUNTIME_PM_OPS(qcom_snps_hsphy_runtime_suspend,
> > -			   qcom_snps_hsphy_runtime_resume, NULL)
> > +	SET_RUNTIME_PM_OPS(qcom_snps_hsphy_suspend,
> > +			   qcom_snps_hsphy_resume, NULL)
> > +	SET_SYSTEM_SLEEP_PM_OPS(qcom_snps_hsphy_suspend,
> > +				qcom_snps_hsphy_resume)
> 
> Won't this cause issues if you system suspend the device while it's
> already runtime suspended?
>

I'm not sure if it would cause issues, but after reflexion and discussion
with Andrew, I think it's unnecessary to add system PM ops in the femto
PHY driver. 

In the dwc3 core, both system and runtime suspend end up calling
dwc3_suspend_common(). From there, what happens depends on the USB mode
and whether the controller is entering system or runtime suspend.

HOST mode:
  (1) system suspend on a non-wakeup controller

  The [1] if branch is taken. dwc3_core_exit() is called, which ends up
  calling phy_power_off() and phy_exit(). Those two functions decrease the
  PM runtime count at some point, so they will trigger the PHY runtime sleep
  (assuming the count is right).

  (2) runtime suspend / system suspend on a wakeup controller  

  The [1] branch is not taken. dwc3_suspend_common() calls
  phy_pm_runtime_put_sync(dwc->usb2_generic_phy). Assuming the ref count is
  right, the PHY runtime suspend op is called.

DEVICE mode:

  dwc3_core_exit() is called on both runtime and system sleep
  unless the controller is already runtime suspended.

OTG mode:
  (1) system suspend : dwc3_core_exit() is called

  (2) runtime suspend : do nothing

With that in mind:

1) Does the PHY need to implement system sleep callbacks? 

dwc3 core system sleep callback will already runtime suspend the PHY, and
also call phy_power_off() and phy_exit() for non-wakeup controllers. So,
adding system PM ops to the femto PHY driver would be redundant.

2) Should the ref_clk be disabled for runtime sleep / wakeup controller
system sleep, or only for non-wakeup controller system sleep?

I'm a little hesitant here. In my submission I'm disabling it in both, but
looking at the downstream code you provided, it seems it's only disabled
for system sleep. ref_clk is disabled by phy->set_suspend() [2] which IIUC
is only called in the system sleep path through dwc3_core_exit() [3].
Moreover, in host mode the upstream code seems to make a distinction
between 1) runtime sleep / system sleep for wakeup controller, and 2)
system sleep for non-wakeup controller where phy_power_off() and
phy_exit() are only called in the latter. This suggests the PHY is not
supposed to be in a fully powered-off state for runtime sleep and system
sleep for wakeup controller. So, it's possible the ref_clk should be kept
enabled in this case. What is your take on this? I could only disable
ref_clk in the femto phy->exit() callback to keep ref_clk enabled during
runtime sleep and make sure it's only disabled on system sleep for
non-wakeup controller.

Hoping I'm not missing anything here.

Best,

Adrien

[1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/core.c#L1988
[2] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c#L524
[3] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/dwc3/core.c#L1915

> Regards,
> Bjorn
> 
> >  };
> >  
> >  static void qcom_snps_hsphy_override_param_update_val(
> > -- 
> > 2.40.1
> >

Writing another email to not muddy the waters in the previous email.

I discovered that the femto PHY PM count doesn't seem to be right. Even
when the dwc3 core runtime suspends and calls
phy_pm_runtime_put_sync(dwc->usb2_generic_phy) [1], the count equals 1
after that and the PHY is not runtime suspended.

This is because on boot, the count is incremented twice because
phy_power_on() is called twice:

First:
phy_power_on+0x120/0x184
dwc3_core_init+0x68c/0xda4
dwc3_probe+0xc84/0x1304

Second:
phy_power_on+0x120/0x184
usb_phy_roothub_power_on+0x48/0xa0
usb_add_hcd+0x94/0x604
xhci_plat_probe+0x4bc/0x6e4
xhci_generic_plat_probe+0xa0/0x104

That makes the femto PHY runtime PM impossible to test at the moment. I'm
not sure if this should be fixed on the dwc3 side or the xhci side, but
this should probably be a topic for another patch series.

Best,

Adrien

[1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/core.c#L2005

On Tue, Jun 27, 2023 at 02:12:45PM -0400, Adrien Thierry wrote:
> Writing another email to not muddy the waters in the previous email.
> 
> I discovered that the femto PHY PM count doesn't seem to be right. Even
> when the dwc3 core runtime suspends and calls
> phy_pm_runtime_put_sync(dwc->usb2_generic_phy) [1], the count equals 1
> after that and the PHY is not runtime suspended.
> 
> This is because on boot, the count is incremented twice because
> phy_power_on() is called twice:
> 
> First:
> phy_power_on+0x120/0x184
> dwc3_core_init+0x68c/0xda4
> dwc3_probe+0xc84/0x1304
> 
> Second:
> phy_power_on+0x120/0x184
> usb_phy_roothub_power_on+0x48/0xa0
> usb_add_hcd+0x94/0x604
> xhci_plat_probe+0x4bc/0x6e4
> xhci_generic_plat_probe+0xa0/0x104
> 
> That makes the femto PHY runtime PM impossible to test at the moment. I'm
> not sure if this should be fixed on the dwc3 side or the xhci side, but
> this should probably be a topic for another patch series.
> 

Thanks for digging into this, I had forgotten about this discussion.
Unfortunately I'm confused about the (lack of?) outcome:

https://lore.kernel.org/linux-arm-msm/1648103831-12347-4-git-send-email-quic_c_sanm@quicinc.com/

Regards,
Bjorn

> Best,
> 
> Adrien
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/core.c#L2005
>

On Tue, Jun 27, 2023 at 02:08:41PM -0400, Adrien Thierry wrote:
> Hi Bjorn,
> 
> On Thu, Jun 22, 2023 at 02:43:07PM -0700, Bjorn Andersson wrote:
> > On Thu, Jun 22, 2023 at 03:40:19PM -0400, Adrien Thierry wrote:
> > > The downstream driver [1] implements set_suspend(), which deals with
> > > both runtime and system sleep/resume. The upstream driver already has
> > > runtime PM ops, so add the system sleep PM ops as well, reusing the same
> > > code as the runtime PM ops.
> > > 
> > > [1] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c
> > > 
> > > Signed-off-by: Adrien Thierry <athierry@redhat.com>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 18 ++++++++++--------
> > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > > index ce1d2f8b568a..378a5029f61e 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > > @@ -179,7 +179,7 @@ static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
> > >  	readl_relaxed(base + offset);
> > >  }
> > >  
> > > -static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
> > > +static int qcom_snps_hsphy_do_suspend(struct qcom_snps_hsphy *hsphy)
> > >  {
> > >  	dev_dbg(&hsphy->phy->dev, "Suspend QCOM SNPS PHY\n");
> > >  
> > > @@ -199,7 +199,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
> > >  	return 0;
> > >  }
> > >  
> > > -static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
> > > +static int qcom_snps_hsphy_do_resume(struct qcom_snps_hsphy *hsphy)
> > >  {
> > >  	int ret;
> > >  
> > > @@ -214,25 +214,25 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
> > >  	return 0;
> > >  }
> > >  
> > > -static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device *dev)
> > > +static int __maybe_unused qcom_snps_hsphy_suspend(struct device *dev)
> > >  {
> > >  	struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev);
> > >  
> > >  	if (!hsphy->phy_initialized)
> > >  		return 0;
> > >  
> > > -	qcom_snps_hsphy_suspend(hsphy);
> > > +	qcom_snps_hsphy_do_suspend(hsphy);
> > >  	return 0;
> > >  }
> > >  
> > > -static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev)
> > > +static int __maybe_unused qcom_snps_hsphy_resume(struct device *dev)
> > >  {
> > >  	struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev);
> > >  
> > >  	if (!hsphy->phy_initialized)
> > >  		return 0;
> > >  
> > > -	qcom_snps_hsphy_resume(hsphy);
> > > +	qcom_snps_hsphy_do_resume(hsphy);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -518,8 +518,10 @@ static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
> > >  MODULE_DEVICE_TABLE(of, qcom_snps_hsphy_of_match_table);
> > >  
> > >  static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
> > > -	SET_RUNTIME_PM_OPS(qcom_snps_hsphy_runtime_suspend,
> > > -			   qcom_snps_hsphy_runtime_resume, NULL)
> > > +	SET_RUNTIME_PM_OPS(qcom_snps_hsphy_suspend,
> > > +			   qcom_snps_hsphy_resume, NULL)
> > > +	SET_SYSTEM_SLEEP_PM_OPS(qcom_snps_hsphy_suspend,
> > > +				qcom_snps_hsphy_resume)
> > 
> > Won't this cause issues if you system suspend the device while it's
> > already runtime suspended?
> >
> 
> I'm not sure if it would cause issues, but after reflexion and discussion
> with Andrew, I think it's unnecessary to add system PM ops in the femto
> PHY driver. 
> 

Glad you looked further into this, I had a brief chat with Andrew and
was building a similar understanding.

> In the dwc3 core, both system and runtime suspend end up calling
> dwc3_suspend_common(). From there, what happens depends on the USB mode
> and whether the controller is entering system or runtime suspend.
> 
> HOST mode:
>   (1) system suspend on a non-wakeup controller
> 
>   The [1] if branch is taken. dwc3_core_exit() is called, which ends up
>   calling phy_power_off() and phy_exit(). Those two functions decrease the
>   PM runtime count at some point, so they will trigger the PHY runtime sleep
>   (assuming the count is right).
> 
>   (2) runtime suspend / system suspend on a wakeup controller  
> 
>   The [1] branch is not taken. dwc3_suspend_common() calls
>   phy_pm_runtime_put_sync(dwc->usb2_generic_phy). Assuming the ref count is
>   right, the PHY runtime suspend op is called.
> 
> DEVICE mode:
> 
>   dwc3_core_exit() is called on both runtime and system sleep
>   unless the controller is already runtime suspended.
> 
> OTG mode:
>   (1) system suspend : dwc3_core_exit() is called
> 
>   (2) runtime suspend : do nothing
> 
> With that in mind:
> 
> 1) Does the PHY need to implement system sleep callbacks? 
> 
> dwc3 core system sleep callback will already runtime suspend the PHY, and
> also call phy_power_off() and phy_exit() for non-wakeup controllers. So,
> adding system PM ops to the femto PHY driver would be redundant.
> 

It seems these decisions must come from the controller, in order to
handle the differences between a wakeup capable port and not. So I'm
thinking that it's correct that it should not implement this.

> 2) Should the ref_clk be disabled for runtime sleep / wakeup controller
> system sleep, or only for non-wakeup controller system sleep?
> 
> I'm a little hesitant here. In my submission I'm disabling it in both, but
> looking at the downstream code you provided, it seems it's only disabled
> for system sleep. ref_clk is disabled by phy->set_suspend() [2] which IIUC
> is only called in the system sleep path through dwc3_core_exit() [3].
> Moreover, in host mode the upstream code seems to make a distinction
> between 1) runtime sleep / system sleep for wakeup controller, and 2)
> system sleep for non-wakeup controller where phy_power_off() and
> phy_exit() are only called in the latter. This suggests the PHY is not
> supposed to be in a fully powered-off state for runtime sleep and system
> sleep for wakeup controller. So, it's possible the ref_clk should be kept
> enabled in this case. What is your take on this? I could only disable
> ref_clk in the femto phy->exit() callback to keep ref_clk enabled during
> runtime sleep and make sure it's only disabled on system sleep for
> non-wakeup controller.
> 

I suggested the same to Andrew, in our chat. Keeping sufficient
resources on, to allow the system to be woken up again by a USB device
is needed if requested. As such the handling of ref must differ between
the two cases.

It still looks a bit strange to me, having the runtime PM callbacks
prepare for wakeup from system suspend...

> Hoping I'm not missing anything here.
> 

I think you managed to capture it all. Sorry for leading you down this
incorrect path.

Regards,
Bjorn

> Best,
> 
> Adrien
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/core.c#L1988
> [2] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c#L524
> [3] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/dwc3/core.c#L1915
> 
> > Regards,
> > Bjorn
> > 
> > >  };
> > >  
> > >  static void qcom_snps_hsphy_override_param_update_val(
> > > -- 
> > > 2.40.1
> > > 
>