diff mbox series

[1/3] phy: usb: Leave some clocks running during suspend

Message ID 20211201180653.35097-2-alcooperx@gmail.com
State Changes Requested
Headers show
Series phy: phy-brcm-usb: Fixes for phy-brcm-usb driver | expand

Commit Message

Alan Cooper Dec. 1, 2021, 6:06 p.m. UTC 
The PHY client driver does a phy_exit() call on suspend or rmmod and
the PHY driver needs to know the difference because some clocks need
to be kept running for suspend but can be shutdown on unbind/rmmod
(or if there are no PHY clients at all).

The fix is to use a PM notifier so the driver can tell if a PHY
client is calling exit() because of a system suspend or a driver
unbind/rmmod.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/phy/broadcom/phy-brcm-usb.c | 38 +++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Florian Fainelli Dec. 1, 2021, 8:39 p.m. UTC | #1 
On 12/1/21 10:06 AM, Al Cooper wrote:
> The PHY client driver does a phy_exit() call on suspend or rmmod and
> the PHY driver needs to know the difference because some clocks need
> to be kept running for suspend but can be shutdown on unbind/rmmod
> (or if there are no PHY clients at all).
> 
> The fix is to use a PM notifier so the driver can tell if a PHY
> client is calling exit() because of a system suspend or a driver
> unbind/rmmod.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Vinod Koul Dec. 2, 2021, 4:35 a.m. UTC | #2 
On 01-12-21, 13:06, Al Cooper wrote:
> The PHY client driver does a phy_exit() call on suspend or rmmod and
> the PHY driver needs to know the difference because some clocks need
> to be kept running for suspend but can be shutdown on unbind/rmmod
> (or if there are no PHY clients at all).
> 
> The fix is to use a PM notifier so the driver can tell if a PHY
> client is calling exit() because of a system suspend or a driver
> unbind/rmmod.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---
>  drivers/phy/broadcom/phy-brcm-usb.c | 38 +++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/phy/broadcom/phy-brcm-usb.c b/drivers/phy/broadcom/phy-brcm-usb.c
> index 116fb23aebd9..0f1deb6e0eab 100644
> --- a/drivers/phy/broadcom/phy-brcm-usb.c
> +++ b/drivers/phy/broadcom/phy-brcm-usb.c
> @@ -18,6 +18,7 @@
>  #include <linux/soc/brcmstb/brcmstb.h>
>  #include <dt-bindings/phy/phy.h>
>  #include <linux/mfd/syscon.h>
> +#include <linux/suspend.h>
>  
>  #include "phy-brcm-usb-init.h"
>  
> @@ -70,12 +71,35 @@ struct brcm_usb_phy_data {
>  	int			init_count;
>  	int			wake_irq;
>  	struct brcm_usb_phy	phys[BRCM_USB_PHY_ID_MAX];
> +	struct notifier_block	pm_notifier;
> +	bool			pm_active;
>  };
>  
>  static s8 *node_reg_names[BRCM_REGS_MAX] = {
>  	"crtl", "xhci_ec", "xhci_gbl", "usb_phy", "usb_mdio", "bdc_ec"
>  };
>  
> +static int brcm_pm_notifier(struct notifier_block *notifier,
> +			    unsigned long pm_event,
> +			    void *unused)
> +{
> +	struct brcm_usb_phy_data *priv =
> +		container_of(notifier, struct brcm_usb_phy_data, pm_notifier);
> +
> +	switch (pm_event) {
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_SUSPEND_PREPARE:
> +		priv->pm_active = true;
> +		break;
> +	case PM_POST_RESTORE:
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_SUSPEND:
> +		priv->pm_active = false;
> +		break;
> +	}
> +	return NOTIFY_DONE;
> +}
> +
>  static irqreturn_t brcm_usb_phy_wake_isr(int irq, void *dev_id)
>  {
>  	struct phy *gphy = dev_id;
> @@ -91,6 +115,9 @@ static int brcm_usb_phy_init(struct phy *gphy)
>  	struct brcm_usb_phy_data *priv =
>  		container_of(phy, struct brcm_usb_phy_data, phys[phy->id]);
>  
> +	if (priv->pm_active)
> +		return 0;
> +
>  	/*
>  	 * Use a lock to make sure a second caller waits until
>  	 * the base phy is inited before using it.
> @@ -120,6 +147,9 @@ static int brcm_usb_phy_exit(struct phy *gphy)
>  	struct brcm_usb_phy_data *priv =
>  		container_of(phy, struct brcm_usb_phy_data, phys[phy->id]);
>  
> +	if (priv->pm_active)
> +		return 0;
> +
>  	dev_dbg(&gphy->dev, "EXIT\n");
>  	if (phy->id == BRCM_USB_PHY_2_0)
>  		brcm_usb_uninit_eohci(&priv->ini);
> @@ -488,6 +518,9 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	priv->pm_notifier.notifier_call = brcm_pm_notifier;
> +	register_pm_notifier(&priv->pm_notifier);
> +
>  	mutex_init(&priv->mutex);
>  
>  	/* make sure invert settings are correct */
> @@ -528,7 +561,10 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
>  
>  static int brcm_usb_phy_remove(struct platform_device *pdev)
>  {
> +	struct brcm_usb_phy_data *priv = dev_get_drvdata(&pdev->dev);
> +
>  	sysfs_remove_group(&pdev->dev.kobj, &brcm_usb_phy_group);
> +	unregister_pm_notifier(&priv->pm_notifier);
>  
>  	return 0;
>  }
> @@ -539,6 +575,7 @@ static int brcm_usb_phy_suspend(struct device *dev)
>  	struct brcm_usb_phy_data *priv = dev_get_drvdata(dev);
>  
>  	if (priv->init_count) {
> +		dev_dbg(dev, "SUSPEND\n");

debug artifact?

>  		priv->ini.wake_enabled = device_may_wakeup(dev);
>  		if (priv->phys[BRCM_USB_PHY_3_0].inited)
>  			brcm_usb_uninit_xhci(&priv->ini);
> @@ -578,6 +615,7 @@ static int brcm_usb_phy_resume(struct device *dev)
>  	 * Uninitialize anything that wasn't previously initialized.
>  	 */
>  	if (priv->init_count) {
> +		dev_dbg(dev, "RESUME\n");

here too

>  		if (priv->wake_irq >= 0)
>  			disable_irq_wake(priv->wake_irq);
>  		brcm_usb_init_common(&priv->ini);
> -- 
> 2.17.1
Alan Cooper Jan. 6, 2022, 9:01 p.m. UTC | #3 
On Wed, Dec 1, 2021 at 11:35 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 01-12-21, 13:06, Al Cooper wrote:
> > The PHY client driver does a phy_exit() call on suspend or rmmod and
> > the PHY driver needs to know the difference because some clocks need
> > to be kept running for suspend but can be shutdown on unbind/rmmod
> > (or if there are no PHY clients at all).
> >
> > The fix is to use a PM notifier so the driver can tell if a PHY
> > client is calling exit() because of a system suspend or a driver
> > unbind/rmmod.
> >
> > Signed-off-by: Al Cooper <alcooperx@gmail.com>
> > ---
> >  drivers/phy/broadcom/phy-brcm-usb.c | 38 +++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/phy/broadcom/phy-brcm-usb.c b/drivers/phy/broadcom/phy-brcm-usb.c
> > index 116fb23aebd9..0f1deb6e0eab 100644
> > --- a/drivers/phy/broadcom/phy-brcm-usb.c
> > +++ b/drivers/phy/broadcom/phy-brcm-usb.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/soc/brcmstb/brcmstb.h>
> >  #include <dt-bindings/phy/phy.h>
> >  #include <linux/mfd/syscon.h>
> > +#include <linux/suspend.h>
> >
> >  #include "phy-brcm-usb-init.h"
> >
> > @@ -70,12 +71,35 @@ struct brcm_usb_phy_data {
> >       int                     init_count;
> >       int                     wake_irq;
> >       struct brcm_usb_phy     phys[BRCM_USB_PHY_ID_MAX];
> > +     struct notifier_block   pm_notifier;
> > +     bool                    pm_active;
> >  };
> >
> >  static s8 *node_reg_names[BRCM_REGS_MAX] = {
> >       "crtl", "xhci_ec", "xhci_gbl", "usb_phy", "usb_mdio", "bdc_ec"
> >  };
> >
> > +static int brcm_pm_notifier(struct notifier_block *notifier,
> > +                         unsigned long pm_event,
> > +                         void *unused)
> > +{
> > +     struct brcm_usb_phy_data *priv =
> > +             container_of(notifier, struct brcm_usb_phy_data, pm_notifier);
> > +
> > +     switch (pm_event) {
> > +     case PM_HIBERNATION_PREPARE:
> > +     case PM_SUSPEND_PREPARE:
> > +             priv->pm_active = true;
> > +             break;
> > +     case PM_POST_RESTORE:
> > +     case PM_POST_HIBERNATION:
> > +     case PM_POST_SUSPEND:
> > +             priv->pm_active = false;
> > +             break;
> > +     }
> > +     return NOTIFY_DONE;
> > +}
> > +
> >  static irqreturn_t brcm_usb_phy_wake_isr(int irq, void *dev_id)
> >  {
> >       struct phy *gphy = dev_id;
> > @@ -91,6 +115,9 @@ static int brcm_usb_phy_init(struct phy *gphy)
> >       struct brcm_usb_phy_data *priv =
> >               container_of(phy, struct brcm_usb_phy_data, phys[phy->id]);
> >
> > +     if (priv->pm_active)
> > +             return 0;
> > +
> >       /*
> >        * Use a lock to make sure a second caller waits until
> >        * the base phy is inited before using it.
> > @@ -120,6 +147,9 @@ static int brcm_usb_phy_exit(struct phy *gphy)
> >       struct brcm_usb_phy_data *priv =
> >               container_of(phy, struct brcm_usb_phy_data, phys[phy->id]);
> >
> > +     if (priv->pm_active)
> > +             return 0;
> > +
> >       dev_dbg(&gphy->dev, "EXIT\n");
> >       if (phy->id == BRCM_USB_PHY_2_0)
> >               brcm_usb_uninit_eohci(&priv->ini);
> > @@ -488,6 +518,9 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
> >       if (err)
> >               return err;
> >
> > +     priv->pm_notifier.notifier_call = brcm_pm_notifier;
> > +     register_pm_notifier(&priv->pm_notifier);
> > +
> >       mutex_init(&priv->mutex);
> >
> >       /* make sure invert settings are correct */
> > @@ -528,7 +561,10 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
> >
> >  static int brcm_usb_phy_remove(struct platform_device *pdev)
> >  {
> > +     struct brcm_usb_phy_data *priv = dev_get_drvdata(&pdev->dev);
> > +
> >       sysfs_remove_group(&pdev->dev.kobj, &brcm_usb_phy_group);
> > +     unregister_pm_notifier(&priv->pm_notifier);
> >
> >       return 0;
> >  }
> > @@ -539,6 +575,7 @@ static int brcm_usb_phy_suspend(struct device *dev)
> >       struct brcm_usb_phy_data *priv = dev_get_drvdata(dev);
> >
> >       if (priv->init_count) {
> > +             dev_dbg(dev, "SUSPEND\n");
>
> debug artifact?
>
> >               priv->ini.wake_enabled = device_may_wakeup(dev);
> >               if (priv->phys[BRCM_USB_PHY_3_0].inited)
> >                       brcm_usb_uninit_xhci(&priv->ini);
> > @@ -578,6 +615,7 @@ static int brcm_usb_phy_resume(struct device *dev)
> >        * Uninitialize anything that wasn't previously initialized.
> >        */
> >       if (priv->init_count) {
> > +             dev_dbg(dev, "RESUME\n");
>
> here too

I left these in intentionally because they are very useful for debug.

>
> >               if (priv->wake_irq >= 0)
> >                       disable_irq_wake(priv->wake_irq);
> >               brcm_usb_init_common(&priv->ini);
> > --
> > 2.17.1
>
> --
> ~Vinod
diff mbox series

Patch

diff --git a/drivers/phy/broadcom/phy-brcm-usb.c b/drivers/phy/broadcom/phy-brcm-usb.c
index 116fb23aebd9..0f1deb6e0eab 100644
--- a/drivers/phy/broadcom/phy-brcm-usb.c
+++ b/drivers/phy/broadcom/phy-brcm-usb.c
@@ -18,6 +18,7 @@ 
 #include <linux/soc/brcmstb/brcmstb.h>
 #include <dt-bindings/phy/phy.h>
 #include <linux/mfd/syscon.h>
+#include <linux/suspend.h>
 
 #include "phy-brcm-usb-init.h"
 
@@ -70,12 +71,35 @@  struct brcm_usb_phy_data {
 	int			init_count;
 	int			wake_irq;
 	struct brcm_usb_phy	phys[BRCM_USB_PHY_ID_MAX];
+	struct notifier_block	pm_notifier;
+	bool			pm_active;
 };
 
 static s8 *node_reg_names[BRCM_REGS_MAX] = {
 	"crtl", "xhci_ec", "xhci_gbl", "usb_phy", "usb_mdio", "bdc_ec"
 };
 
+static int brcm_pm_notifier(struct notifier_block *notifier,
+			    unsigned long pm_event,
+			    void *unused)
+{
+	struct brcm_usb_phy_data *priv =
+		container_of(notifier, struct brcm_usb_phy_data, pm_notifier);
+
+	switch (pm_event) {
+	case PM_HIBERNATION_PREPARE:
+	case PM_SUSPEND_PREPARE:
+		priv->pm_active = true;
+		break;
+	case PM_POST_RESTORE:
+	case PM_POST_HIBERNATION:
+	case PM_POST_SUSPEND:
+		priv->pm_active = false;
+		break;
+	}
+	return NOTIFY_DONE;
+}
+
 static irqreturn_t brcm_usb_phy_wake_isr(int irq, void *dev_id)
 {
 	struct phy *gphy = dev_id;
@@ -91,6 +115,9 @@  static int brcm_usb_phy_init(struct phy *gphy)
 	struct brcm_usb_phy_data *priv =
 		container_of(phy, struct brcm_usb_phy_data, phys[phy->id]);
 
+	if (priv->pm_active)
+		return 0;
+
 	/*
 	 * Use a lock to make sure a second caller waits until
 	 * the base phy is inited before using it.
@@ -120,6 +147,9 @@  static int brcm_usb_phy_exit(struct phy *gphy)
 	struct brcm_usb_phy_data *priv =
 		container_of(phy, struct brcm_usb_phy_data, phys[phy->id]);
 
+	if (priv->pm_active)
+		return 0;
+
 	dev_dbg(&gphy->dev, "EXIT\n");
 	if (phy->id == BRCM_USB_PHY_2_0)
 		brcm_usb_uninit_eohci(&priv->ini);
@@ -488,6 +518,9 @@  static int brcm_usb_phy_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	priv->pm_notifier.notifier_call = brcm_pm_notifier;
+	register_pm_notifier(&priv->pm_notifier);
+
 	mutex_init(&priv->mutex);
 
 	/* make sure invert settings are correct */
@@ -528,7 +561,10 @@  static int brcm_usb_phy_probe(struct platform_device *pdev)
 
 static int brcm_usb_phy_remove(struct platform_device *pdev)
 {
+	struct brcm_usb_phy_data *priv = dev_get_drvdata(&pdev->dev);
+
 	sysfs_remove_group(&pdev->dev.kobj, &brcm_usb_phy_group);
+	unregister_pm_notifier(&priv->pm_notifier);
 
 	return 0;
 }
@@ -539,6 +575,7 @@  static int brcm_usb_phy_suspend(struct device *dev)
 	struct brcm_usb_phy_data *priv = dev_get_drvdata(dev);
 
 	if (priv->init_count) {
+		dev_dbg(dev, "SUSPEND\n");
 		priv->ini.wake_enabled = device_may_wakeup(dev);
 		if (priv->phys[BRCM_USB_PHY_3_0].inited)
 			brcm_usb_uninit_xhci(&priv->ini);
@@ -578,6 +615,7 @@  static int brcm_usb_phy_resume(struct device *dev)
 	 * Uninitialize anything that wasn't previously initialized.
 	 */
 	if (priv->init_count) {
+		dev_dbg(dev, "RESUME\n");
 		if (priv->wake_irq >= 0)
 			disable_irq_wake(priv->wake_irq);
 		brcm_usb_init_common(&priv->ini);