| Message ID | 20230911035501.36706-3-tony@atomide.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] phy: mapphone-mdm6600: Fix runtime disable on probe | expand |
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
* 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
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;
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(-)