Improve phy-mapphone pm
Subsystems (2)
cover Improve phy-mapphone pm
Hi,
Here are two non-urgent patches to get phy-mapphone runtime PM working better. Currently it only works when toggled from the userspace via the sysfs entry for for "auto" and "on".
Regards,
Tony
Tony Lindgren (2):
phy: core: Add phy_pm_runtime_enabled
phy: mapphone-mdm6600: Improve phy related runtime PM calls
drivers/phy/motorola/phy-mapphone-mdm6600.c | 71 +++++++++++++++------ drivers/phy/phy-core.c | 9 +++ include/linux/phy/phy.h | 6 ++ 3 files changed, 66 insertions(+), 20 deletions(-)
Signature · 1 line
2.19.1
PATCH 1/2 phy: core: Add phy_pm_runtime_enabled
The phy driver may need to check phy_pm_runtime_enabled() in suspend as PM runtime for phy may be already disabled when phy power_off() is called.
--- drivers/phy/phy-core.c | 9 +++++++++ include/linux/phy/phy.h | 6 ++++++ 2 files changed, 15 insertions(+)
drivers/phy/phy-core.c +9 -0
@@ -209,6 +209,15 @@ int phy_pm_runtime_put_sync(struct phy *phy) }EXPORT_SYMBOL_GPL (phy_pm_runtime_put_sync); +bool phy_pm_runtime_enabled (struct phy *phy) +{ +if (!phy) +return false; + +return pm_runtime_enabled (&phy->dev); +} +EXPORT_SYMBOL_GPL (phy_pm_runtime_enabled); +void phy_pm_runtime_allow (struct phy *phy) {if (!phy)
include/linux/phy/phy.h +6 -0
@@ -158,6 +158,7 @@ int phy_pm_runtime_get(struct phy *phy);int phy_pm_runtime_get_sync (struct phy *phy);int phy_pm_runtime_put (struct phy *phy);int phy_pm_runtime_put_sync (struct phy *phy); +bool phy_pm_runtime_enabled (struct phy *phy);void phy_pm_runtime_allow (struct phy *phy);void phy_pm_runtime_forbid (struct phy *phy);int phy_init (struct phy *phy);
@@ -240,6 +241,11 @@ static inline int phy_pm_runtime_put_sync(struct phy *phy)return -ENOSYS ; } +static inline bool phy_pm_runtime_enabled (struct phy *phy) +{ + return false +} +static inline void phy_pm_runtime_allow (struct phy *phy) {return ;
Signature · 1 line
2.19.1
Re: [PATCH 1/2] phy: core: Add phy_pm_runtime_enabled
Quoted 27 lines · Tony Lindgren
The phy driver may need to check
phy_pm_runtime_enabled()in suspend as PM runtime for phy may be already disabled when phypower_off()is called.Cc Pavel Machek <[email protected]>Cc Sebastian Reichel <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- drivers/phy/phy-core.c | 9 +++++++++ include/linux/phy/phy.h | 6 ++++++ 2 files changed, 15 insertions(+)drivers/phy/phy-core.c +0
-0include/linux/phy/phy.h +1
-0@@ -158,6 +158,7 @@ int phy_pm_runtime_get(struct phy *phy);int phy_pm_runtime_get_sync (struct phy *phy);int phy_pm_runtime_put (struct phy *phy);int phy_pm_runtime_put_sync (struct phy *phy); +bool phy_pm_runtime_enabled (struct phy *phy);void phy_pm_runtime_allow (struct phy *phy);void phy_pm_runtime_forbid (struct phy *phy);int phy_init (struct phy *phy);
include/linux/phy/phy.h static inline int phy_pm_runtime_put_sync(struct phy *phy) · line 246
Quoted 6 lines
include/linux/phy/phy.h +3
-0@@ -240,6 +241,11 @@ static inline int phy_pm_runtime_put_sync(struct phy *phy)return -ENOSYS ; } +static inline bool phy_pm_runtime_enabled (struct phy *phy) +{ +return false
Missing semicolon.
include/linux/phy/phy.h static inline int phy_pm_runtime_put_sync(struct phy *phy) · line 251
Quoted 5 lines
include/linux/phy/phy.h +2
-0+} +static inline void phy_pm_runtime_allow (struct phy *phy) {return ;
Johan
Re: [PATCH 1/2] phy: core: Add phy_pm_runtime_enabled
Quoted 31 lines · Johan Hovold
Quoted 30 lines · Tony Lindgren
The phy driver may need to check
phy_pm_runtime_enabled()in suspend as PM runtime for phy may be already disabled when phypower_off()is called.Cc Pavel Machek <[email protected]>Cc Sebastian Reichel <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- drivers/phy/phy-core.c | 9 +++++++++ include/linux/phy/phy.h | 6 ++++++ 2 files changed, 15 insertions(+)drivers/phy/phy-core.c +0
-0include/linux/phy/phy.h +1
-0@@ -158,6 +158,7 @@ int phy_pm_runtime_get(struct phy *phy);int phy_pm_runtime_get_sync (struct phy *phy);int phy_pm_runtime_put (struct phy *phy);int phy_pm_runtime_put_sync (struct phy *phy); +bool phy_pm_runtime_enabled (struct phy *phy);void phy_pm_runtime_allow (struct phy *phy);void phy_pm_runtime_forbid (struct phy *phy);int phy_init (struct phy *phy);@@ -240,6 +241,11 @@ static inline int phy_pm_runtime_put_sync(struct phy *phy)return -ENOSYS ; }
Quoted 5 lines
include/linux/phy/phy.h static inline int phy_pm_runtime_put_sync(struct phy *phy) · line 246
Quoted 3 lines
include/linux/phy/phy.h +3
-0+static inline bool phy_pm_runtime_enabled (struct phy *phy) +{ +return falseMissing semicolon.
Oops thanks for catching that. I guess I did not try building without CONFIG_GENERIC_PHY. Will fix and repost.
Regards,
Tony
Re: [PATCH 1/2] phy: core: Add phy_pm_runtime_enabled
Hi!
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
The phy driver may need to check
phy_pm_runtime_enabled()in suspend as PM runtime for phy may be already disabled when phypower_off()is called.Cc Pavel Machek <[email protected]>Cc Sebastian Reichel <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- drivers/phy/phy-core.c | 9 +++++++++ include/linux/phy/phy.h | 6 ++++++ 2 files changed, 15 insertions(+)drivers/phy/phy-core.c +0
-0include/linux/phy/phy.h +1
-0@@ -158,6 +158,7 @@ int phy_pm_runtime_get(struct phy *phy);int phy_pm_runtime_get_sync (struct phy *phy);int phy_pm_runtime_put (struct phy *phy);int phy_pm_runtime_put_sync (struct phy *phy); +bool phy_pm_runtime_enabled (struct phy *phy);void phy_pm_runtime_allow (struct phy *phy);void phy_pm_runtime_forbid (struct phy *phy);int phy_init (struct phy *phy);@@ -240,6 +241,11 @@ static inline int phy_pm_runtime_put_sync(struct phy *phy)return -ENOSYS ; }
Quoted 8 lines
Quoted 5 lines
include/linux/phy/phy.h static inline int phy_pm_runtime_put_sync(struct phy *phy) · line 246
Quoted 3 lines
include/linux/phy/phy.h +3
-0+static inline bool phy_pm_runtime_enabled (struct phy *phy) +{ +return falseMissing semicolon.
Oops thanks for catching that. I guess I did not try building without
CONFIG_GENERIC_PHY. Will fix and repost.
Did this series get lost/forgotten somewhere? Is it still needed? Any way I can help?
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 1/2] phy: core: Add phy_pm_runtime_enabled
Hi!
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
include/linux/phy/phy.h static inline int phy_pm_runtime_put_sync(struct phy *phy) · line 246
Quoted 7 lines
include/linux/phy/phy.h +3
-0@@ -240,6 +241,11 @@ static inline int phy_pm_runtime_put_sync(struct phy *phy)return -ENOSYS ; } +static inline bool phy_pm_runtime_enabled (struct phy *phy) +{ +return falseMissing semicolon.
Quoted 5 lines
Quoted 2 lines
Oops thanks for catching that. I guess I did not try building without
CONFIG_GENERIC_PHY. Will fix and repost.Did this series get lost/forgotten somewhere? Is it still needed? Any way I can help?
Aha, it seems v2 of the series was applied, which touches different files and that's why I did not detect it. Sorry for the noise.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
kbuild test robot
build botRe: [PATCH 1/2] phy: core: Add phy_pm_runtime_enabled
In reply to Tony LindgrenI love your patch! Yet something to improve:
Hi Tony,
I love your patch! Yet something to improve:
[auto build test ERROR on phy/next]
[also build test ERROR on v4.20-rc3 next-20181119]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/Improve-phy-mapphone-pm/20181119-222603 base: https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next config: riscv-tinyconfig (attached as .config) compiler: riscv64-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=riscv
All errors (new ones prefixed by >>):
In file included from include/linux/usb/otg.h:13,
from include/linux/usb/of.h:12,
from drivers/usb/phy/of.c:9:
include/linux/phy/phy.h: In function 'phy_pm_runtime_enabled':
>> include/linux/phy/phy.h:246:14: error: expected ';' before '}' token
return false
^
;
}
~ vim +246 include/linux/phy/phy.h
243
244 static inline bool phy_pm_runtime_enabled(struct phy *phy)
245 {
> 246 return false
247 }
248
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
PATCH 2/2 phy: mapphone-mdm6600: Improve phy related runtime PM calls
In reply to Tony LindgrenI noticed that phy_pm_runtime_get_sync() and phy_pm_runtime_put() are not currently doing anything for phy-mapphone-mdm6600, only the sysfs interface for works for "auto" and "on".
This is because of the shared GPIO pins between mdm6600 USB port and n_gsm port. We have not enabled runtime PM for the phy driver until after we've booted up mdm6600 properly to the USB mode. Otherwise phy_create() would have called pm_runtime_enable() and pm_runtime_no_callbacks() automatically on init.
Let's fix this by registering the phy a bit later after we've powered up the mdm6600 USB port.
And as the PM runtime support is only needed for the n_gsm mode and not for USB, we can allow the device to idle between phy_mdm6600_power_on() and phy_mdm6600_power_off(). Note that for suspend, runtime_pm is already disabled for the phy so we need to check for phy_pm_runtime_enabled().
--- drivers/phy/motorola/phy-mapphone-mdm6600.c | 71 +++++++++++++++------ 1 file changed, 51 insertions(+), 20 deletions(-)
drivers/phy/motorola/phy-mapphone-mdm6600.c +51 -20
@@ -16,6 +16,7 @@#include <linux/gpio/consumer.h> #include <linux/of_platform.h> #include <linux/phy/phy.h> +#include <linux/pinctrl/consumer.h> #define PHY_MDM6600_PHY_DELAY_MS 4000/* PHY enable 2.2s to 3.5s */ #define PHY_MDM6600_ENABLED_DELAY_MS 8000/* 8s more total for MDM6600 */
@@ -124,12 +125,22 @@ static int phy_mdm6600_power_on(struct phy *x) {struct phy_mdm6600 *ddata =phy_get_drvdata (x);struct gpio_desc *enable_gpio = ddata->ctrl_gpios[PHY_MDM6600_ENABLE ]; +int error;if (!ddata->enabled)return -ENODEV ; + error =pinctrl_pm_select_default_state (ddata->dev); +if (error) +dev_warn (ddata->dev,"%s: error with default_state: %i\n" , + __func__, error); +gpiod_set_value_cansleep (enable_gpio,1 ); +/* Allow aggressive PM for USB, it's only needed for n_gsm port */ +if (phy_pm_runtime_enabled (ddata->generic_phy)) +phy_pm_runtime_put (ddata->generic_phy); +return 0 ; }
@@ -137,12 +148,26 @@ static int phy_mdm6600_power_off(struct phy *x) {struct phy_mdm6600 *ddata =phy_get_drvdata (x);struct gpio_desc *enable_gpio = ddata->ctrl_gpios[PHY_MDM6600_ENABLE ]; +int error;if (!ddata->enabled)return -ENODEV ; +/* Paired with phy_pm_runtime_put() in phy_mdm6600_power_on() */ +if (phy_pm_runtime_enabled (ddata->generic_phy)) { + error =phy_pm_runtime_get (ddata->generic_phy); +if (error <0 && error != -EINPROGRESS ) +dev_warn (ddata->dev,"%s: phy_pm_runtime_get: %i\n" , + __func__, error); + } +gpiod_set_value_cansleep (enable_gpio,0 ); + error =pinctrl_pm_select_sleep_state (ddata->dev); +if (error) +dev_warn (ddata->dev,"%s: error with sleep_state: %i\n" , + __func__, error); +return 0 ; }
@@ -555,28 +580,17 @@ 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;phy_mdm6600_init_irq (ddata); - - ddata->generic_phy =devm_phy_create (ddata->dev,NULL , &gpio_usb_ops); -if (IS_ERR (ddata->generic_phy)) { - error =PTR_ERR (ddata->generic_phy); - goto cleanup; - } - -phy_set_drvdata (ddata->generic_phy, ddata); - - ddata->phy_provider = -devm_of_phy_provider_register (ddata->dev, - of_phy_simple_xlate); -if (IS_ERR (ddata->phy_provider)) { - error =PTR_ERR (ddata->phy_provider); - goto cleanup; - } -schedule_delayed_work (&ddata->bootup_work,0 );/*
@@ -600,14 +614,31 @@ static int phy_mdm6600_probe(struct platform_device *pdev)if (error <0 ) {dev_warn (ddata->dev,"failed to wake modem: %i\n" , error);pm_runtime_put_noidle (ddata->dev); + goto cleanup; } + + ddata->generic_phy =devm_phy_create (ddata->dev,NULL , &gpio_usb_ops); +if (IS_ERR (ddata->generic_phy)) { + error =PTR_ERR (ddata->generic_phy); + goto idle; + } + +phy_set_drvdata (ddata->generic_phy, ddata); + + ddata->phy_provider = +devm_of_phy_provider_register (ddata->dev, + of_phy_simple_xlate); +if (IS_ERR (ddata->phy_provider)) + error =PTR_ERR (ddata->phy_provider); + +idle:pm_runtime_mark_last_busy (ddata->dev);pm_runtime_put_autosuspend (ddata->dev); -return 0 ; - cleanup: -phy_mdm6600_device_power_off (ddata); +if (error <0 ) +phy_mdm6600_device_power_off (ddata); +return error; }
Signature · 1 line
2.19.1
Re: [PATCH 2/2] phy: mapphone-mdm6600: Improve phy related runtime PM calls
Hi Tony,
Quoted 45 lines · Tony Lindgren
I noticed that
phy_pm_runtime_get_sync()andphy_pm_runtime_put()are not currently doing anything for phy-mapphone-mdm6600, only the sysfs interface for works for "auto" and "on".This is because of the shared GPIO pins between mdm6600 USB port and
n_gsmport. We have not enabled runtime PM for the phy driver until after we've booted up mdm6600 properly to the USB mode. Otherwisephy_create()would have calledpm_runtime_enable()andpm_runtime_no_callbacks()automatically on init.Let's fix this by registering the phy a bit later after we've powered up the mdm6600 USB port.
And as the PM runtime support is only needed for the
n_gsmmode and not for USB, we can allow the device to idle betweenphy_mdm6600_power_on()andphy_mdm6600_power_off(). Note that for suspend,runtime_pmis already disabled for the phy so we need to check forphy_pm_runtime_enabled().Cc Pavel Machek <[email protected]>Cc Sebastian Reichel <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- drivers/phy/motorola/phy-mapphone-mdm6600.c | 71 +++++++++++++++------ 1 file changed, 51 insertions(+), 20 deletions(-)drivers/phy/motorola/phy-mapphone-mdm6600.c +2
-0@@ -16,6 +16,7 @@#include <linux/gpio/consumer.h> #include <linux/of_platform.h> #include <linux/phy/phy.h> +#include <linux/pinctrl/consumer.h> #define PHY_MDM6600_PHY_DELAY_MS 4000/* PHY enable 2.2s to 3.5s */ #define PHY_MDM6600_ENABLED_DELAY_MS 8000/* 8s more total for MDM6600 */ @@ -124,12 +125,22 @@ static int phy_mdm6600_power_on(struct phy *x) {struct phy_mdm6600 *ddata =phy_get_drvdata (x);struct gpio_desc *enable_gpio = ddata->ctrl_gpios[PHY_MDM6600_ENABLE ]; +int error;if (!ddata->enabled)return -ENODEV ;
drivers/phy/motorola/phy-mapphone-mdm6600.c static int phy_mdm6600_power_on(struct phy *x) · line 142
Quoted 10 lines
drivers/phy/motorola/phy-mapphone-mdm6600.c +8
-0+ error =pinctrl_pm_select_default_state (ddata->dev); +if (error) +dev_warn (ddata->dev,"%s: error with default_state: %i\n" , + __func__, error); +gpiod_set_value_cansleep (enable_gpio,1 ); +/* Allow aggressive PM for USB, it's only needed for n_gsm port */ +if (phy_pm_runtime_enabled (ddata->generic_phy)) +phy_pm_runtime_put (ddata->generic_phy);
phy_*() API's are generally added to be used by the consumer driver. I guess in this case we can directly use pm_runtime_enabled(x).
Thanks
Kishon
Re: [PATCH 2/2] phy: mapphone-mdm6600: Improve phy related runtime PM calls
Quoted 7 lines · Kishon Vijay Abraham I
drivers/phy/motorola/phy-mapphone-mdm6600.c static int phy_mdm6600_power_on(struct phy *x) · line 142
Quoted 3 lines · Tony Lindgren
drivers/phy/motorola/phy-mapphone-mdm6600.c +3
-0+/* Allow aggressive PM for USB, it's only needed for n_gsm port */ +if (phy_pm_runtime_enabled (ddata->generic_phy)) +phy_pm_runtime_put (ddata->generic_phy);phy_*() API's are generally added to be used by the consumer driver. I guess in this case we can directly use
pm_runtime_enabled(x).
OK sure that works for me too.
Regards,
Tony