[PATCH 1/3] phy: mapphone-mdm6600: Fix runtime disable on probe

[PATCH 1/3] phy: mapphone-mdm6600: Fix runtime disable on probe

[Tony Lindgren]

Commit d644e0d79829 ("phy: mapphone-mdm6600: Fix PM error handling in
phy_mdm6600_probe") caused a regression where we now unconditionally
disable runtime PM at the end of the probe while it is only needed on
errors.

Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Miaoqian Lin <linmq006@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sebastian Reichel <sre@kernel.org>
Fixes: d644e0d79829 ("phy: mapphone-mdm6600: Fix PM error handling in phy_mdm6600_probe")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/phy/motorola/phy-mapphone-mdm6600.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/motorola/phy-mapphone-mdm6600.c b/drivers/phy/motorola/phy-mapphone-mdm6600.c
--- a/drivers/phy/motorola/phy-mapphone-mdm6600.c
+++ b/drivers/phy/motorola/phy-mapphone-mdm6600.c
@@ -627,10 +627,12 @@ static int phy_mdm6600_probe(struct platform_device *pdev)
 	pm_runtime_put_autosuspend(ddata->dev);
 
 cleanup:
-	if (error < 0)
+	if (error < 0) {
 		phy_mdm6600_device_power_off(ddata);
-	pm_runtime_disable(ddata->dev);
-	pm_runtime_dont_use_autosuspend(ddata->dev);
+		pm_runtime_disable(ddata->dev);
+		pm_runtime_dont_use_autosuspend(ddata->dev);
+	}
+
 	return error;
 }
 
-- 
2.42.0

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 2/3] phy: mapphone-mdm6600: Fix runtime PM for remove

[Tony Lindgren]

Otherwise we will get an underflow on remove.

Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sebastian Reichel <sre@kernel.org>
Fixes: f7f50b2a7b05 ("phy: mapphone-mdm6600: Add runtime PM support for n_gsm on USB suspend")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/phy/motorola/phy-mapphone-mdm6600.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/phy/motorola/phy-mapphone-mdm6600.c b/drivers/phy/motorola/phy-mapphone-mdm6600.c
--- a/drivers/phy/motorola/phy-mapphone-mdm6600.c
+++ b/drivers/phy/motorola/phy-mapphone-mdm6600.c
@@ -640,6 +640,11 @@ static void phy_mdm6600_remove(struct platform_device *pdev)
 {
 	struct phy_mdm6600 *ddata = platform_get_drvdata(pdev);
 	struct gpio_desc *reset_gpio = ddata->ctrl_gpios[PHY_MDM6600_RESET];
+	int error;
+
+	error = pm_runtime_resume_and_get(ddata->dev);
+	if (error)
+		return;
 
 	pm_runtime_dont_use_autosuspend(ddata->dev);
 	pm_runtime_put_sync(ddata->dev);
-- 
2.42.0

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 2/3] phy: mapphone-mdm6600: Fix runtime PM for remove

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 1308 bytes --]

Hi,

On Mon, Sep 11, 2023 at 06:54:56AM +0300, Tony Lindgren wrote:
> Otherwise we will get an underflow on remove.
> 
> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Fixes: f7f50b2a7b05 ("phy: mapphone-mdm6600: Add runtime PM support for n_gsm on USB suspend")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/phy/motorola/phy-mapphone-mdm6600.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/phy/motorola/phy-mapphone-mdm6600.c b/drivers/phy/motorola/phy-mapphone-mdm6600.c
> --- a/drivers/phy/motorola/phy-mapphone-mdm6600.c
> +++ b/drivers/phy/motorola/phy-mapphone-mdm6600.c
> @@ -640,6 +640,11 @@ static void phy_mdm6600_remove(struct platform_device *pdev)
>  {
>  	struct phy_mdm6600 *ddata = platform_get_drvdata(pdev);
>  	struct gpio_desc *reset_gpio = ddata->ctrl_gpios[PHY_MDM6600_RESET];
> +	int error;
> +
> +	error = pm_runtime_resume_and_get(ddata->dev);
> +	if (error)
> +		return;

Isn't this just about the use counters, i.e.
pm_runtime_get_noresume(); should be enough?

-- Sebastian

>  	pm_runtime_dont_use_autosuspend(ddata->dev);
>  	pm_runtime_put_sync(ddata->dev);
> -- 
> 2.42.0

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 2/3] phy: mapphone-mdm6600: Fix runtime PM for remove

[Tony Lindgren]

* Sebastian Reichel <sebastian.reichel@collabora.com> [230912 15:00]:
> Isn't this just about the use counters, i.e.
> pm_runtime_get_noresume(); should be enough?

Good point I'll check. We are toggling the out-of-bandwidth modem wake
gpio line with runtime PM, it may not matter on remove.

Regards,

Tony

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 3/3] phy: mapphone-mdm6600: Fix pinctrl_pm handling for sleep pins

[Tony Lindgren]

Looks like the driver sleep pins configuration is unusable. Adding the
sleep pins causes the usb phy to not respond. We need to use the default
pins in probe, and only set sleep pins at phy_mdm6600_device_power_off().

The sleep pins are needed as otherwise the modem hardware can wake up even
with the phy driver unloaded as the reset gpio pin can glitch during the
deeper SoC idle states.

Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sebastian Reichel <sre@kernel.org>
Fixes: 2ad2af081622 ("phy: mapphone-mdm6600: Improve phy related runtime PM calls")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/phy/motorola/phy-mapphone-mdm6600.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/phy/motorola/phy-mapphone-mdm6600.c b/drivers/phy/motorola/phy-mapphone-mdm6600.c
--- a/drivers/phy/motorola/phy-mapphone-mdm6600.c
+++ b/drivers/phy/motorola/phy-mapphone-mdm6600.c
@@ -456,6 +456,7 @@ static void phy_mdm6600_device_power_off(struct phy_mdm6600 *ddata)
 {
 	struct gpio_desc *reset_gpio =
 		ddata->ctrl_gpios[PHY_MDM6600_RESET];
+	int error;
 
 	ddata->enabled = false;
 	phy_mdm6600_cmd(ddata, PHY_MDM6600_CMD_BP_SHUTDOWN_REQ);
@@ -471,6 +472,11 @@ static void phy_mdm6600_device_power_off(struct phy_mdm6600 *ddata)
 	} else {
 		dev_err(ddata->dev, "Timed out powering down\n");
 	}
+
+	error = pinctrl_pm_select_sleep_state(ddata->dev);
+	if (error)
+		dev_warn(ddata->dev, "%s: error with sleep_state: %i\n",
+			 __func__, error);
 }
 
 static void phy_mdm6600_deferred_power_on(struct work_struct *work)
@@ -571,12 +577,6 @@ static int phy_mdm6600_probe(struct platform_device *pdev)
 	ddata->dev = &pdev->dev;
 	platform_set_drvdata(pdev, ddata);
 
-	/* Active state selected in phy_mdm6600_power_on() */
-	error = pinctrl_pm_select_sleep_state(ddata->dev);
-	if (error)
-		dev_warn(ddata->dev, "%s: error with sleep_state: %i\n",
-			 __func__, error);
-
 	error = phy_mdm6600_init_lines(ddata);
 	if (error)
 		return error;
-- 
2.42.0

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 3/3] phy: mapphone-mdm6600: Fix pinctrl_pm handling for sleep pins

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 2804 bytes --]

Hi,

On Mon, Sep 11, 2023 at 06:54:57AM +0300, Tony Lindgren wrote:
> Looks like the driver sleep pins configuration is unusable. Adding the
> sleep pins causes the usb phy to not respond. We need to use the default
> pins in probe, and only set sleep pins at phy_mdm6600_device_power_off().
> 
> The sleep pins are needed as otherwise the modem hardware can wake up even
> with the phy driver unloaded as the reset gpio pin can glitch during the
> deeper SoC idle states.
> 
> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Fixes: 2ad2af081622 ("phy: mapphone-mdm6600: Improve phy related runtime PM calls")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---

Apparently phy_power_off is not called on device removal, so I
understand the need to setup sleep pins in phy_mdm6600_device_power_off()
in addition to the exsting setup in phy_mdm6600_power_off().

But I'm a bit confused about the change required in probe(), since
phy_mdm6600_power_on() selects the default state. I wouldn't expect
any access before the phy is powered on? Anyways,

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/phy/motorola/phy-mapphone-mdm6600.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/motorola/phy-mapphone-mdm6600.c b/drivers/phy/motorola/phy-mapphone-mdm6600.c
> --- a/drivers/phy/motorola/phy-mapphone-mdm6600.c
> +++ b/drivers/phy/motorola/phy-mapphone-mdm6600.c
> @@ -456,6 +456,7 @@ static void phy_mdm6600_device_power_off(struct phy_mdm6600 *ddata)
>  {
>  	struct gpio_desc *reset_gpio =
>  		ddata->ctrl_gpios[PHY_MDM6600_RESET];
> +	int error;
>  
>  	ddata->enabled = false;
>  	phy_mdm6600_cmd(ddata, PHY_MDM6600_CMD_BP_SHUTDOWN_REQ);
> @@ -471,6 +472,11 @@ static void phy_mdm6600_device_power_off(struct phy_mdm6600 *ddata)
>  	} else {
>  		dev_err(ddata->dev, "Timed out powering down\n");
>  	}
> +
> +	error = pinctrl_pm_select_sleep_state(ddata->dev);
> +	if (error)
> +		dev_warn(ddata->dev, "%s: error with sleep_state: %i\n",
> +			 __func__, error);
>  }
>  
>  static void phy_mdm6600_deferred_power_on(struct work_struct *work)
> @@ -571,12 +577,6 @@ static int phy_mdm6600_probe(struct platform_device *pdev)
>  	ddata->dev = &pdev->dev;
>  	platform_set_drvdata(pdev, ddata);
>  
> -	/* Active state selected in phy_mdm6600_power_on() */
> -	error = pinctrl_pm_select_sleep_state(ddata->dev);
> -	if (error)
> -		dev_warn(ddata->dev, "%s: error with sleep_state: %i\n",
> -			 __func__, error);
> -
>  	error = phy_mdm6600_init_lines(ddata);
>  	if (error)
>  		return error;
> -- 
> 2.42.0

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 3/3] phy: mapphone-mdm6600: Fix pinctrl_pm handling for sleep pins

[Tony Lindgren]

* Sebastian Reichel <sebastian.reichel@collabora.com> [230912 15:14]:
> Apparently phy_power_off is not called on device removal, so I
> understand the need to setup sleep pins in phy_mdm6600_device_power_off()
> in addition to the exsting setup in phy_mdm6600_power_off().
>
> But I'm a bit confused about the change required in probe(), since
> phy_mdm6600_power_on() selects the default state. I wouldn't expect
> any access before the phy is powered on? Anyways,

Maybe we should just set the sleep state in remove and leave it out of the
phy functions. If a separate state is needed for the phy_mdm6600_power_off(),
it could be the pinctrl idle state.

We need the reset pin in probe to get the modem started. The modem may
also be started in uart mode for firmware flashing depending on how the
gpio pins are set.

Regards,

Tony

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: mapphone-mdm6600: Fix runtime disable on probe

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 1550 bytes --]

Hi,

On Mon, Sep 11, 2023 at 06:54:55AM +0300, Tony Lindgren wrote:
> Commit d644e0d79829 ("phy: mapphone-mdm6600: Fix PM error handling in
> phy_mdm6600_probe") caused a regression where we now unconditionally
> disable runtime PM at the end of the probe while it is only needed on
> errors.
> 
> Cc: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Miaoqian Lin <linmq006@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Fixes: d644e0d79829 ("phy: mapphone-mdm6600: Fix PM error handling in phy_mdm6600_probe")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/phy/motorola/phy-mapphone-mdm6600.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/motorola/phy-mapphone-mdm6600.c b/drivers/phy/motorola/phy-mapphone-mdm6600.c
> --- a/drivers/phy/motorola/phy-mapphone-mdm6600.c
> +++ b/drivers/phy/motorola/phy-mapphone-mdm6600.c
> @@ -627,10 +627,12 @@ static int phy_mdm6600_probe(struct platform_device *pdev)
>  	pm_runtime_put_autosuspend(ddata->dev);
>  
>  cleanup:
> -	if (error < 0)
> +	if (error < 0) {
>  		phy_mdm6600_device_power_off(ddata);
> -	pm_runtime_disable(ddata->dev);
> -	pm_runtime_dont_use_autosuspend(ddata->dev);
> +		pm_runtime_disable(ddata->dev);
> +		pm_runtime_dont_use_autosuspend(ddata->dev);
> +	}
> +
>  	return error;
>  }
>  
> -- 
> 2.42.0

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy