When adding runtime PM support to the Tegra XHCI driver, it is desirable
to move the function calls to enable the clocks, regulators and PHY from
the tegra_xusb_probe into the runtime PM handlers. Currently, the
clocks, regulators and PHY are all enabled before we call
usb_create_hcd() in tegra_xusb_probe(), however, we cannot call
pm_runtime_get_sync() at this point because the platform device data is
not yet initialised. Fortunately, the function usb_create_hcd() can be
called before we enable the clocks, regulators and PHY and so prepare
for adding runtime PM support, by moving the call to usb_create_hcd()
before we enable the hardware.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/usb/host/xhci-tegra.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 2c076ea80522..02b0b24faa58 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -1054,10 +1054,23 @@ static int tegra_xusb_probe(struct platform_device *pdev)
}
}
+ tegra->hcd = usb_create_hcd(&tegra_xhci_hc_driver, &pdev->dev,
+ dev_name(&pdev->dev));
+ if (!tegra->hcd) {
+ err = -ENOMEM;
+ goto put_padctl;
+ }
+
+ /*
+ * This must happen after usb_create_hcd(), because usb_create_hcd()
+ * will overwrite the drvdata of the device with the hcd it creates.
+ */
+ platform_set_drvdata(pdev, tegra);
+
err = tegra_xusb_clk_enable(tegra);
if (err) {
dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
- goto put_padctl;
+ goto put_usb2;
}
err = regulator_bulk_enable(tegra->soc->num_supplies, tegra->supplies);
@@ -1080,19 +1093,6 @@ static int tegra_xusb_probe(struct platform_device *pdev)
goto disable_phy;
}
- tegra->hcd = usb_create_hcd(&tegra_xhci_hc_driver, &pdev->dev,
- dev_name(&pdev->dev));
- if (!tegra->hcd) {
- err = -ENOMEM;
- goto disable_phy;
- }
-
- /*
- * This must happen after usb_create_hcd(), because usb_create_hcd()
- * will overwrite the drvdata of the device with the hcd it creates.
- */
- platform_set_drvdata(pdev, tegra);
-
tegra->hcd->regs = tegra->regs;
tegra->hcd->rsrc_start = regs->start;
tegra->hcd->rsrc_len = resource_size(regs);
@@ -1100,7 +1100,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
err = usb_add_hcd(tegra->hcd, tegra->xhci_irq, IRQF_SHARED);
if (err < 0) {
dev_err(&pdev->dev, "failed to add USB HCD: %d\n", err);
- goto put_usb2;
+ goto disable_phy;
}
device_wakeup_enable(tegra->hcd->self.controller);
@@ -1155,14 +1155,14 @@ static int tegra_xusb_probe(struct platform_device *pdev)
usb_put_hcd(xhci->shared_hcd);
remove_usb2:
usb_remove_hcd(tegra->hcd);
-put_usb2:
- usb_put_hcd(tegra->hcd);
disable_phy:
tegra_xusb_phy_disable(tegra);
disable_regulator:
regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies);
disable_clk:
tegra_xusb_clk_disable(tegra);
+put_usb2:
+ usb_put_hcd(tegra->hcd);
put_padctl:
tegra_xusb_padctl_put(tegra->padctl);
return err;
--
2.7.4
Add runtime PM support to the Tegra XHCI driver and move the function
calls to enable/disable the clocks, regulators and PHY into the runtime
PM callbacks.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/usb/host/xhci-tegra.c | 80 ++++++++++++++++++++++++++++++-------------
1 file changed, 56 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 02b0b24faa58..42aa67858b53 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -18,6 +18,7 @@
#include <linux/phy/tegra/xusb.h>
#include <linux/platform_device.h>
#include <linux/pm.h>
+#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
#include <linux/slab.h>
@@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct platform_device *pdev)
*/
platform_set_drvdata(pdev, tegra);
- err = tegra_xusb_clk_enable(tegra);
- if (err) {
- dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
- goto put_usb2;
- }
-
- err = regulator_bulk_enable(tegra->soc->num_supplies, tegra->supplies);
- if (err) {
- dev_err(&pdev->dev, "failed to enable regulators: %d\n", err);
- goto disable_clk;
- }
+ pm_runtime_enable(&pdev->dev);
- err = tegra_xusb_phy_enable(tegra);
+ err = pm_runtime_get_sync(&pdev->dev);
if (err < 0) {
- dev_err(&pdev->dev, "failed to enable PHYs: %d\n", err);
- goto disable_regulator;
+ dev_err(&pdev->dev, "failed to enable device: %d\n", err);
+ goto disable_rpm;
}
tegra_xusb_ipfs_config(tegra, regs);
@@ -1090,7 +1081,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
err = tegra_xusb_load_firmware(tegra);
if (err < 0) {
dev_err(&pdev->dev, "failed to load firmware: %d\n", err);
- goto disable_phy;
+ goto put_rpm;
}
tegra->hcd->regs = tegra->regs;
@@ -1100,7 +1091,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
err = usb_add_hcd(tegra->hcd, tegra->xhci_irq, IRQF_SHARED);
if (err < 0) {
dev_err(&pdev->dev, "failed to add USB HCD: %d\n", err);
- goto disable_phy;
+ goto put_rpm;
}
device_wakeup_enable(tegra->hcd->self.controller);
@@ -1155,13 +1146,10 @@ static int tegra_xusb_probe(struct platform_device *pdev)
usb_put_hcd(xhci->shared_hcd);
remove_usb2:
usb_remove_hcd(tegra->hcd);
-disable_phy:
- tegra_xusb_phy_disable(tegra);
-disable_regulator:
- regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies);
-disable_clk:
- tegra_xusb_clk_disable(tegra);
-put_usb2:
+put_rpm:
+ pm_runtime_put_sync(&pdev->dev);
+disable_rpm:
+ pm_runtime_disable(&pdev->dev);
usb_put_hcd(tegra->hcd);
put_padctl:
tegra_xusb_padctl_put(tegra->padctl);
@@ -1181,13 +1169,55 @@ static int tegra_xusb_remove(struct platform_device *pdev)
dma_free_coherent(&pdev->dev, tegra->fw.size, tegra->fw.virt,
tegra->fw.phys);
+ pm_runtime_put_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+
+ tegra_xusb_padctl_put(tegra->padctl);
+
+ return 0;
+}
+
+static int tegra_xusb_runtime_suspend(struct device *dev)
+{
+ struct tegra_xusb *tegra = dev_get_drvdata(dev);
+
tegra_xusb_phy_disable(tegra);
regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies);
tegra_xusb_clk_disable(tegra);
- tegra_xusb_padctl_put(tegra->padctl);
+ return 0;
+}
+
+static int tegra_xusb_runtime_resume(struct device *dev)
+{
+ struct tegra_xusb *tegra = dev_get_drvdata(dev);
+ int err;
+
+ err = tegra_xusb_clk_enable(tegra);
+ if (err) {
+ dev_err(dev, "failed to enable clocks: %d\n", err);
+ return err;
+ }
+
+ err = regulator_bulk_enable(tegra->soc->num_supplies, tegra->supplies);
+ if (err) {
+ dev_err(dev, "failed to enable regulators: %d\n", err);
+ goto disable_clk;
+ }
+
+ err = tegra_xusb_phy_enable(tegra);
+ if (err < 0) {
+ dev_err(dev, "failed to enable PHYs: %d\n", err);
+ goto disable_regulator;
+ }
return 0;
+
+disable_regulator:
+ regulator_bulk_disable(tegra->soc->num_supplies, tegra->supplies);
+disable_clk:
+ tegra_xusb_clk_disable(tegra);
+ return err;
}
#ifdef CONFIG_PM_SLEEP
@@ -1211,6 +1241,8 @@ static int tegra_xusb_resume(struct device *dev)
#endif
static const struct dev_pm_ops tegra_xusb_pm_ops = {
+ SET_RUNTIME_PM_OPS(tegra_xusb_runtime_suspend,
+ tegra_xusb_runtime_resume, NULL)
SET_SYSTEM_SLEEP_PM_OPS(tegra_xusb_suspend, tegra_xusb_resume)
};
--
2.7.4
The Tegra XHCI controller requires that the XUSBA (for superspeed) and
XUSBC (for host) power-domains are enabled. Commit 8df127456f29
("soc/tegra: pmc: Enable XUSB partitions on boot") was added to force
on these power-domains if the XHCI driver is enabled while proper
power-domain support is added, to ensure the device did not hang on
boot. However, rather than forcing on these power-domains in the PMC
driver we can use the legacy Tegra powergate APIs to turn on these
power-domains during the probe of the Tegra XHCI driver.
In the near future we plan to move the Tegra XHCI driver to use the
generic PM domain framework for power-domains and so to prepare for
this only use the legacy Tegra powergate API if there is not PM
domain associated with device (ie. dev.pm_domain is NULL). Please
note that in the future the superspeed and host resets will be handled
by the generic PM domain provider and so these are only these are only
needed in the case where there is no generic PM domain.
Signed-off-by: Jon Hunter <[email protected]>
---
drivers/usb/host/xhci-tegra.c | 68 +++++++++++++++++++++++++++++++------------
1 file changed, 49 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 42aa67858b53..f69e5edfd604 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -22,6 +22,7 @@
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
#include <linux/slab.h>
+#include <soc/tegra/pmc.h>
#include "xhci.h"
@@ -931,20 +932,6 @@ static int tegra_xusb_probe(struct platform_device *pdev)
if (IS_ERR(tegra->padctl))
return PTR_ERR(tegra->padctl);
- tegra->host_rst = devm_reset_control_get(&pdev->dev, "xusb_host");
- if (IS_ERR(tegra->host_rst)) {
- err = PTR_ERR(tegra->host_rst);
- dev_err(&pdev->dev, "failed to get xusb_host reset: %d\n", err);
- goto put_padctl;
- }
-
- tegra->ss_rst = devm_reset_control_get(&pdev->dev, "xusb_ss");
- if (IS_ERR(tegra->ss_rst)) {
- err = PTR_ERR(tegra->ss_rst);
- dev_err(&pdev->dev, "failed to get xusb_ss reset: %d\n", err);
- goto put_padctl;
- }
-
tegra->host_clk = devm_clk_get(&pdev->dev, "xusb_host");
if (IS_ERR(tegra->host_clk)) {
err = PTR_ERR(tegra->host_clk);
@@ -1008,11 +995,48 @@ static int tegra_xusb_probe(struct platform_device *pdev)
goto put_padctl;
}
+ if (!pdev->dev.pm_domain) {
+ tegra->host_rst = devm_reset_control_get(&pdev->dev,
+ "xusb_host");
+ if (IS_ERR(tegra->host_rst)) {
+ err = PTR_ERR(tegra->host_rst);
+ dev_err(&pdev->dev,
+ "failed to get xusb_host reset: %d\n", err);
+ goto put_padctl;
+ }
+
+ tegra->ss_rst = devm_reset_control_get(&pdev->dev, "xusb_ss");
+ if (IS_ERR(tegra->ss_rst)) {
+ err = PTR_ERR(tegra->ss_rst);
+ dev_err(&pdev->dev, "failed to get xusb_ss reset: %d\n",
+ err);
+ goto put_padctl;
+ }
+
+ err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_XUSBA,
+ tegra->ss_clk,
+ tegra->ss_rst);
+ if (err) {
+ dev_err(&pdev->dev,
+ "failed to enable XUSBA domain: %d\n", err);
+ goto put_padctl;
+ }
+
+ err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_XUSBC,
+ tegra->host_clk,
+ tegra->host_rst);
+ if (err) {
+ dev_err(&pdev->dev,
+ "failed to enable XUSBC domain: %d\n", err);
+ goto disable_xusba;
+ }
+ }
+
tegra->supplies = devm_kcalloc(&pdev->dev, tegra->soc->num_supplies,
sizeof(*tegra->supplies), GFP_KERNEL);
if (!tegra->supplies) {
err = -ENOMEM;
- goto put_padctl;
+ goto disable_xusbc;
}
for (i = 0; i < tegra->soc->num_supplies; i++)
@@ -1022,7 +1046,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
tegra->supplies);
if (err) {
dev_err(&pdev->dev, "failed to get regulators: %d\n", err);
- goto put_padctl;
+ goto disable_xusbc;
}
for (i = 0; i < tegra->soc->num_types; i++)
@@ -1032,7 +1056,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
sizeof(*tegra->phys), GFP_KERNEL);
if (!tegra->phys) {
err = -ENOMEM;
- goto put_padctl;
+ goto disable_xusbc;
}
for (i = 0, k = 0; i < tegra->soc->num_types; i++) {
@@ -1048,7 +1072,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
"failed to get PHY %s: %ld\n", prop,
PTR_ERR(phy));
err = PTR_ERR(phy);
- goto put_padctl;
+ goto disable_xusbc;
}
tegra->phys[k++] = phy;
@@ -1059,7 +1083,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
dev_name(&pdev->dev));
if (!tegra->hcd) {
err = -ENOMEM;
- goto put_padctl;
+ goto disable_xusbc;
}
/*
@@ -1151,6 +1175,12 @@ static int tegra_xusb_probe(struct platform_device *pdev)
disable_rpm:
pm_runtime_disable(&pdev->dev);
usb_put_hcd(tegra->hcd);
+disable_xusbc:
+ if (!&pdev->dev.pm_domain)
+ tegra_powergate_power_off(TEGRA_POWERGATE_XUSBC);
+disable_xusba:
+ if (!&pdev->dev.pm_domain)
+ tegra_powergate_power_off(TEGRA_POWERGATE_XUSBA);
put_padctl:
tegra_xusb_padctl_put(tegra->padctl);
return err;
--
2.7.4
On 14.02.2018 18:34, Jon Hunter wrote:
> Add runtime PM support to the Tegra XHCI driver and move the function
> calls to enable/disable the clocks, regulators and PHY into the runtime
> PM callbacks.
>
> Signed-off-by: Jon Hunter <[email protected]>
> ---
> drivers/usb/host/xhci-tegra.c | 80 ++++++++++++++++++++++++++++++-------------
> 1 file changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> index 02b0b24faa58..42aa67858b53 100644
> --- a/drivers/usb/host/xhci-tegra.c
> +++ b/drivers/usb/host/xhci-tegra.c
> @@ -18,6 +18,7 @@
> #include <linux/phy/tegra/xusb.h>
> #include <linux/platform_device.h>
> #include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/reset.h>
> #include <linux/slab.h>
> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct platform_device *pdev)
> */
> platform_set_drvdata(pdev, tegra);
>
> - err = tegra_xusb_clk_enable(tegra);
> - if (err) {
> - dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
> - goto put_usb2;
> - }
> -
> - err = regulator_bulk_enable(tegra->soc->num_supplies, tegra->supplies);
> - if (err) {
> - dev_err(&pdev->dev, "failed to enable regulators: %d\n", err);
> - goto disable_clk;
> - }
> + pm_runtime_enable(&pdev->dev);
>
> - err = tegra_xusb_phy_enable(tegra);
> + err = pm_runtime_get_sync(&pdev->dev);
> if (err < 0) {
Does this mean that if runtime PM is disabled then clocks and regulator will never be enabled
for Tegra xhci?
How about keeping the clock and regualtor enabling in probe, and instead add something like:
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
pm_runtime_get_noresume(&pdev->dev);
-Mathias
On 01/03/18 14:18, Mathias Nyman wrote:
> On 14.02.2018 18:34, Jon Hunter wrote:
>> Add runtime PM support to the Tegra XHCI driver and move the function
>> calls to enable/disable the clocks, regulators and PHY into the runtime
>> PM callbacks.
>>
>> Signed-off-by: Jon Hunter <[email protected]>
>> ---
>> drivers/usb/host/xhci-tegra.c | 80
>> ++++++++++++++++++++++++++++++-------------
>> 1 file changed, 56 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-tegra.c
>> b/drivers/usb/host/xhci-tegra.c
>> index 02b0b24faa58..42aa67858b53 100644
>> --- a/drivers/usb/host/xhci-tegra.c
>> +++ b/drivers/usb/host/xhci-tegra.c
>> @@ -18,6 +18,7 @@
>> #include <linux/phy/tegra/xusb.h>
>> #include <linux/platform_device.h>
>> #include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/reset.h>
>> #include <linux/slab.h>
>> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
>> platform_device *pdev)
>> */
>> platform_set_drvdata(pdev, tegra);
>> - err = tegra_xusb_clk_enable(tegra);
>> - if (err) {
>> - dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
>> - goto put_usb2;
>> - }
>> -
>> - err = regulator_bulk_enable(tegra->soc->num_supplies,
>> tegra->supplies);
>> - if (err) {
>> - dev_err(&pdev->dev, "failed to enable regulators: %d\n", err);
>> - goto disable_clk;
>> - }
>> + pm_runtime_enable(&pdev->dev);
>> - err = tegra_xusb_phy_enable(tegra);
>> + err = pm_runtime_get_sync(&pdev->dev);
>> if (err < 0) {
>
> Does this mean that if runtime PM is disabled then clocks and regulator
> will never be enabled
> for Tegra xhci?
>
> How about keeping the clock and regualtor enabling in probe, and instead
> add something like:
>
> pm_runtime_set_active(&pdev->dev);
> pm_runtime_enable(&pdev->dev);
> pm_runtime_get_noresume(&pdev->dev);
For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
AFAIK there is not and so yes we should handle the case when PM_RUNTIME
is disabled.
Typically we do something like ...
pm_runtime_enable(&pdev->dev);
if (!pm_runtime_enabled(&pdev->dev))
ret = tegra_xusb_runtime_resume(&pdev->dev);
else
ret = pm_runtime_get_sync(&pdev->dev);
That way we can keep the regulator and clock stuff in the handler. I
will update this series.
Cheers
Jon
--
nvpublic
On Thu, Mar 08, 2018 at 09:31:07PM +0000, Jon Hunter wrote:
>
> On 01/03/18 14:18, Mathias Nyman wrote:
> > On 14.02.2018 18:34, Jon Hunter wrote:
> >> Add runtime PM support to the Tegra XHCI driver and move the function
> >> calls to enable/disable the clocks, regulators and PHY into the runtime
> >> PM callbacks.
> >>
> >> Signed-off-by: Jon Hunter <[email protected]>
> >> ---
> >> drivers/usb/host/xhci-tegra.c | 80
> >> ++++++++++++++++++++++++++++++-------------
> >> 1 file changed, 56 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/xhci-tegra.c
> >> b/drivers/usb/host/xhci-tegra.c
> >> index 02b0b24faa58..42aa67858b53 100644
> >> --- a/drivers/usb/host/xhci-tegra.c
> >> +++ b/drivers/usb/host/xhci-tegra.c
> >> @@ -18,6 +18,7 @@
> >> #include <linux/phy/tegra/xusb.h>
> >> #include <linux/platform_device.h>
> >> #include <linux/pm.h>
> >> +#include <linux/pm_runtime.h>
> >> #include <linux/regulator/consumer.h>
> >> #include <linux/reset.h>
> >> #include <linux/slab.h>
> >> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
> >> platform_device *pdev)
> >> */
> >> platform_set_drvdata(pdev, tegra);
> >> - err = tegra_xusb_clk_enable(tegra);
> >> - if (err) {
> >> - dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
> >> - goto put_usb2;
> >> - }
> >> -
> >> - err = regulator_bulk_enable(tegra->soc->num_supplies,
> >> tegra->supplies);
> >> - if (err) {
> >> - dev_err(&pdev->dev, "failed to enable regulators: %d\n", err);
> >> - goto disable_clk;
> >> - }
> >> + pm_runtime_enable(&pdev->dev);
> >> - err = tegra_xusb_phy_enable(tegra);
> >> + err = pm_runtime_get_sync(&pdev->dev);
> >> if (err < 0) {
> >
> > Does this mean that if runtime PM is disabled then clocks and regulator
> > will never be enabled
> > for Tegra xhci?
> >
> > How about keeping the clock and regualtor enabling in probe, and instead
> > add something like:
> >
> > pm_runtime_set_active(&pdev->dev);
> > pm_runtime_enable(&pdev->dev);
> > pm_runtime_get_noresume(&pdev->dev);
>
> For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
> AFAIK there is not and so yes we should handle the case when PM_RUNTIME
> is disabled.
>
> Typically we do something like ...
>
> pm_runtime_enable(&pdev->dev);
> if (!pm_runtime_enabled(&pdev->dev))
> ret = tegra_xusb_runtime_resume(&pdev->dev);
> else
> ret = pm_runtime_get_sync(&pdev->dev);
>
> That way we can keep the regulator and clock stuff in the handler. I
> will update this series.
Is there any good reason why we don't depend on PM for 32-bit as well?
I'm not aware of any differences in drivers that are 32-bit specific for
Tegra, and I'm not even sure the !PM case gets any testing at all. And
even if, do we really still want to support that?
I don't see any advantage these days for having it disabled.
Thierry
On 09.03.2018 10:36, Thierry Reding wrote:
> On Thu, Mar 08, 2018 at 09:31:07PM +0000, Jon Hunter wrote:
>>
>> On 01/03/18 14:18, Mathias Nyman wrote:
>>> On 14.02.2018 18:34, Jon Hunter wrote:
>>>> Add runtime PM support to the Tegra XHCI driver and move the function
>>>> calls to enable/disable the clocks, regulators and PHY into the runtime
>>>> PM callbacks.
>>>>
>>>> Signed-off-by: Jon Hunter <[email protected]>
>>>> ---
>>>> drivers/usb/host/xhci-tegra.c | 80
>>>> ++++++++++++++++++++++++++++++-------------
>>>> 1 file changed, 56 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-tegra.c
>>>> b/drivers/usb/host/xhci-tegra.c
>>>> index 02b0b24faa58..42aa67858b53 100644
>>>> --- a/drivers/usb/host/xhci-tegra.c
>>>> +++ b/drivers/usb/host/xhci-tegra.c
>>>> @@ -18,6 +18,7 @@
>>>> #include <linux/phy/tegra/xusb.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/pm.h>
>>>> +#include <linux/pm_runtime.h>
>>>> #include <linux/regulator/consumer.h>
>>>> #include <linux/reset.h>
>>>> #include <linux/slab.h>
>>>> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
>>>> platform_device *pdev)
>>>> */
>>>> platform_set_drvdata(pdev, tegra);
>>>> - err = tegra_xusb_clk_enable(tegra);
>>>> - if (err) {
>>>> - dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
>>>> - goto put_usb2;
>>>> - }
>>>> -
>>>> - err = regulator_bulk_enable(tegra->soc->num_supplies,
>>>> tegra->supplies);
>>>> - if (err) {
>>>> - dev_err(&pdev->dev, "failed to enable regulators: %d\n", err);
>>>> - goto disable_clk;
>>>> - }
>>>> + pm_runtime_enable(&pdev->dev);
>>>> - err = tegra_xusb_phy_enable(tegra);
>>>> + err = pm_runtime_get_sync(&pdev->dev);
>>>> if (err < 0) {
>>>
>>> Does this mean that if runtime PM is disabled then clocks and regulator
>>> will never be enabled
>>> for Tegra xhci?
>>>
>>> How about keeping the clock and regualtor enabling in probe, and instead
>>> add something like:
>>>
>>> pm_runtime_set_active(&pdev->dev);
>>> pm_runtime_enable(&pdev->dev);
>>> pm_runtime_get_noresume(&pdev->dev);
>>
>> For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
>> AFAIK there is not and so yes we should handle the case when PM_RUNTIME
>> is disabled.
>>
>> Typically we do something like ...
>>
>> pm_runtime_enable(&pdev->dev);
>> if (!pm_runtime_enabled(&pdev->dev))
>> ret = tegra_xusb_runtime_resume(&pdev->dev);
>> else
>> ret = pm_runtime_get_sync(&pdev->dev);
>>
>> That way we can keep the regulator and clock stuff in the handler. I
>> will update this series.
>
> Is there any good reason why we don't depend on PM for 32-bit as well?
> I'm not aware of any differences in drivers that are 32-bit specific for
> Tegra, and I'm not even sure the !PM case gets any testing at all. And
> even if, do we really still want to support that?
>
> I don't see any advantage these days for having it disabled.
I don't know much about Tegra, but I'd still like to turn this question around:
Is there any reason why clks and regulators can't initially be turned on in probe,
and then let runtime PM handle them later if PM is supported?
Shouldn't this work in all cases, and it avoids creating new dependencies?
Thanks
Mathias
On 09/03/18 08:36, Thierry Reding wrote:
> On Thu, Mar 08, 2018 at 09:31:07PM +0000, Jon Hunter wrote:
>>
>> On 01/03/18 14:18, Mathias Nyman wrote:
>>> On 14.02.2018 18:34, Jon Hunter wrote:
>>>> Add runtime PM support to the Tegra XHCI driver and move the function
>>>> calls to enable/disable the clocks, regulators and PHY into the runtime
>>>> PM callbacks.
>>>>
>>>> Signed-off-by: Jon Hunter <[email protected]>
>>>> ---
>>>> drivers/usb/host/xhci-tegra.c | 80
>>>> ++++++++++++++++++++++++++++++-------------
>>>> 1 file changed, 56 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-tegra.c
>>>> b/drivers/usb/host/xhci-tegra.c
>>>> index 02b0b24faa58..42aa67858b53 100644
>>>> --- a/drivers/usb/host/xhci-tegra.c
>>>> +++ b/drivers/usb/host/xhci-tegra.c
>>>> @@ -18,6 +18,7 @@
>>>> #include <linux/phy/tegra/xusb.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/pm.h>
>>>> +#include <linux/pm_runtime.h>
>>>> #include <linux/regulator/consumer.h>
>>>> #include <linux/reset.h>
>>>> #include <linux/slab.h>
>>>> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
>>>> platform_device *pdev)
>>>> */
>>>> platform_set_drvdata(pdev, tegra);
>>>> - err = tegra_xusb_clk_enable(tegra);
>>>> - if (err) {
>>>> - dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
>>>> - goto put_usb2;
>>>> - }
>>>> -
>>>> - err = regulator_bulk_enable(tegra->soc->num_supplies,
>>>> tegra->supplies);
>>>> - if (err) {
>>>> - dev_err(&pdev->dev, "failed to enable regulators: %d\n", err);
>>>> - goto disable_clk;
>>>> - }
>>>> + pm_runtime_enable(&pdev->dev);
>>>> - err = tegra_xusb_phy_enable(tegra);
>>>> + err = pm_runtime_get_sync(&pdev->dev);
>>>> if (err < 0) {
>>>
>>> Does this mean that if runtime PM is disabled then clocks and regulator
>>> will never be enabled
>>> for Tegra xhci?
>>>
>>> How about keeping the clock and regualtor enabling in probe, and instead
>>> add something like:
>>>
>>> pm_runtime_set_active(&pdev->dev);
>>> pm_runtime_enable(&pdev->dev);
>>> pm_runtime_get_noresume(&pdev->dev);
>>
>> For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
>> AFAIK there is not and so yes we should handle the case when PM_RUNTIME
>> is disabled.
>>
>> Typically we do something like ...
>>
>> pm_runtime_enable(&pdev->dev);
>> if (!pm_runtime_enabled(&pdev->dev))
>> ret = tegra_xusb_runtime_resume(&pdev->dev);
>> else
>> ret = pm_runtime_get_sync(&pdev->dev);
>>
>> That way we can keep the regulator and clock stuff in the handler. I
>> will update this series.
>
> Is there any good reason why we don't depend on PM for 32-bit as well?
Not that I am aware of.
> I'm not aware of any differences in drivers that are 32-bit specific for
> Tegra, and I'm not even sure the !PM case gets any testing at all. And
> even if, do we really still want to support that?
>
> I don't see any advantage these days for having it disabled.
It would be fine IMO.
Cheers
Jon
--
nvpublic
On 09/03/18 09:13, Mathias Nyman wrote:
> On 09.03.2018 10:36, Thierry Reding wrote:
>> On Thu, Mar 08, 2018 at 09:31:07PM +0000, Jon Hunter wrote:
>>>
>>> On 01/03/18 14:18, Mathias Nyman wrote:
>>>> On 14.02.2018 18:34, Jon Hunter wrote:
>>>>> Add runtime PM support to the Tegra XHCI driver and move the function
>>>>> calls to enable/disable the clocks, regulators and PHY into the
>>>>> runtime
>>>>> PM callbacks.
>>>>>
>>>>> Signed-off-by: Jon Hunter <[email protected]>
>>>>> ---
>>>>> drivers/usb/host/xhci-tegra.c | 80
>>>>> ++++++++++++++++++++++++++++++-------------
>>>>> 1 file changed, 56 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/host/xhci-tegra.c
>>>>> b/drivers/usb/host/xhci-tegra.c
>>>>> index 02b0b24faa58..42aa67858b53 100644
>>>>> --- a/drivers/usb/host/xhci-tegra.c
>>>>> +++ b/drivers/usb/host/xhci-tegra.c
>>>>> @@ -18,6 +18,7 @@
>>>>> #include <linux/phy/tegra/xusb.h>
>>>>> #include <linux/platform_device.h>
>>>>> #include <linux/pm.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>> #include <linux/regulator/consumer.h>
>>>>> #include <linux/reset.h>
>>>>> #include <linux/slab.h>
>>>>> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
>>>>> platform_device *pdev)
>>>>> */
>>>>> platform_set_drvdata(pdev, tegra);
>>>>> - err = tegra_xusb_clk_enable(tegra);
>>>>> - if (err) {
>>>>> - dev_err(&pdev->dev, "failed to enable clocks: %d\n", err);
>>>>> - goto put_usb2;
>>>>> - }
>>>>> -
>>>>> - err = regulator_bulk_enable(tegra->soc->num_supplies,
>>>>> tegra->supplies);
>>>>> - if (err) {
>>>>> - dev_err(&pdev->dev, "failed to enable regulators: %d\n",
>>>>> err);
>>>>> - goto disable_clk;
>>>>> - }
>>>>> + pm_runtime_enable(&pdev->dev);
>>>>> - err = tegra_xusb_phy_enable(tegra);
>>>>> + err = pm_runtime_get_sync(&pdev->dev);
>>>>> if (err < 0) {
>>>>
>>>> Does this mean that if runtime PM is disabled then clocks and regulator
>>>> will never be enabled
>>>> for Tegra xhci?
>>>>
>>>> How about keeping the clock and regualtor enabling in probe, and
>>>> instead
>>>> add something like:
>>>>
>>>> pm_runtime_set_active(&pdev->dev);
>>>> pm_runtime_enable(&pdev->dev);
>>>> pm_runtime_get_noresume(&pdev->dev);
>>>
>>> For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
>>> AFAIK there is not and so yes we should handle the case when PM_RUNTIME
>>> is disabled.
>>>
>>> Typically we do something like ...
>>>
>>> pm_runtime_enable(&pdev->dev);
>>> if (!pm_runtime_enabled(&pdev->dev))
>>> ret = tegra_xusb_runtime_resume(&pdev->dev);
>>> else
>>> ret = pm_runtime_get_sync(&pdev->dev);
>>>
>>> That way we can keep the regulator and clock stuff in the handler. I
>>> will update this series.
>>
>> Is there any good reason why we don't depend on PM for 32-bit as well?
>> I'm not aware of any differences in drivers that are 32-bit specific for
>> Tegra, and I'm not even sure the !PM case gets any testing at all. And
>> even if, do we really still want to support that?
>>
>> I don't see any advantage these days for having it disabled.
>
> I don't know much about Tegra, but I'd still like to turn this question
> around:
>
> Is there any reason why clks and regulators can't initially be turned on
> in probe,
> and then let runtime PM handle them later if PM is supported?
I personally prefer having the regulator, clock, etc handling for
enabling and disabling for a device in their own handler. Duplicating
the calls to the regulator and clock frameworks in probe and the rpm
handlers seems more prone to mistakes. Hence, that's why I would prefer
the option I suggested above, which IMO is the best of both worlds.
> Shouldn't this work in all cases, and it avoids creating new dependencies?
Yes but when you want to use frameworks such as genpd it becomes more
complex to support !PM and that is why for 64-bit Tegra we have a
dependency today.
Cheers
Jon
--
nvpublic