2020-11-04 23:49:51

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v1 21/30] usb: host: ehci-tegra: Support OPP and SoC core voltage scaling

Add initial OPP and SoC core voltage scaling support to the Tegra EHCI
driver. This is required for enabling system-wide DVFS on older Tegra
SoCs.

Tested-by: Peter Geis <[email protected]>
Tested-by: Nicolas Chauvet <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/usb/host/Kconfig | 1 +
drivers/usb/host/ehci-tegra.c | 79 +++++++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index ab12c4bf0ef1..35c42bc05c5a 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -278,6 +278,7 @@ config USB_EHCI_TEGRA
depends on ARCH_TEGRA
select USB_EHCI_ROOT_HUB_TT
select USB_TEGRA_PHY
+ select PM_OPP
help
This driver enables support for the internal USB Host Controllers
found in NVIDIA Tegra SoCs. The controllers are EHCI compliant.
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 869d9c4de5fc..0976577f54b4 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -17,6 +17,7 @@
#include <linux/of_device.h>
#include <linux/of_gpio.h>
#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
#include <linux/pm_runtime.h>
#include <linux/reset.h>
#include <linux/slab.h>
@@ -364,6 +365,79 @@ static void tegra_ehci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
free_dma_aligned_buffer(urb);
}

+static void tegra_ehci_deinit_opp_table(void *data)
+{
+ struct device *dev = data;
+ struct opp_table *opp_table;
+
+ opp_table = dev_pm_opp_get_opp_table(dev);
+ dev_pm_opp_of_remove_table(dev);
+ dev_pm_opp_put_regulators(opp_table);
+ dev_pm_opp_put_opp_table(opp_table);
+}
+
+static int devm_tegra_ehci_init_opp_table(struct device *dev)
+{
+ unsigned long rate = ULONG_MAX;
+ struct opp_table *opp_table;
+ const char *rname = "core";
+ struct dev_pm_opp *opp;
+ int err;
+
+ /* legacy device-trees don't have OPP table */
+ if (!device_property_present(dev, "operating-points-v2"))
+ return 0;
+
+ /* voltage scaling is optional */
+ if (device_property_present(dev, "core-supply"))
+ opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
+ else
+ opp_table = dev_pm_opp_get_opp_table(dev);
+
+ if (IS_ERR(opp_table))
+ return dev_err_probe(dev, PTR_ERR(opp_table),
+ "failed to prepare OPP table\n");
+
+ err = dev_pm_opp_of_add_table(dev);
+ if (err) {
+ dev_err(dev, "failed to add OPP table: %d\n", err);
+ goto put_table;
+ }
+
+ /* find suitable OPP for the maximum clock rate */
+ opp = dev_pm_opp_find_freq_floor(dev, &rate);
+ err = PTR_ERR_OR_ZERO(opp);
+ if (err) {
+ dev_err(dev, "failed to get OPP: %d\n", err);
+ goto remove_table;
+ }
+
+ dev_pm_opp_put(opp);
+
+ /*
+ * First dummy rate-set initializes voltage vote by setting voltage
+ * in accordance to the clock rate.
+ */
+ err = dev_pm_opp_set_rate(dev, rate);
+ if (err) {
+ dev_err(dev, "failed to initialize OPP clock: %d\n", err);
+ goto remove_table;
+ }
+
+ err = devm_add_action(dev, tegra_ehci_deinit_opp_table, dev);
+ if (err)
+ goto remove_table;
+
+ return 0;
+
+remove_table:
+ dev_pm_opp_of_remove_table(dev);
+put_table:
+ dev_pm_opp_put_regulators(opp_table);
+
+ return err;
+}
+
static const struct tegra_ehci_soc_config tegra30_soc_config = {
.has_hostpc = true,
};
@@ -431,6 +505,11 @@ static int tegra_ehci_probe(struct platform_device *pdev)
goto cleanup_hcd_create;
}

+ err = devm_tegra_ehci_init_opp_table(&pdev->dev);
+ if (err)
+ return dev_err_probe(&pdev->dev, err,
+ "Failed to initialize OPP\n");
+
err = clk_prepare_enable(tegra->clk);
if (err)
goto cleanup_hcd_create;
--
2.27.0


2020-11-05 16:10:03

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH v1 21/30] usb: host: ehci-tegra: Support OPP and SoC core voltage scaling

On Thu, Nov 05, 2020 at 02:44:18AM +0300, Dmitry Osipenko wrote:
> Add initial OPP and SoC core voltage scaling support to the Tegra EHCI
> driver. This is required for enabling system-wide DVFS on older Tegra
> SoCs.
>
> Tested-by: Peter Geis <[email protected]>
> Tested-by: Nicolas Chauvet <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---

I'm no expert on OPP stuff, but some of what you have done here looks
peculiar.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 869d9c4de5fc..0976577f54b4 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -17,6 +17,7 @@
> #include <linux/of_device.h>
> #include <linux/of_gpio.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> #include <linux/pm_runtime.h>
> #include <linux/reset.h>
> #include <linux/slab.h>
> @@ -364,6 +365,79 @@ static void tegra_ehci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> free_dma_aligned_buffer(urb);
> }
>
> +static void tegra_ehci_deinit_opp_table(void *data)
> +{
> + struct device *dev = data;
> + struct opp_table *opp_table;
> +
> + opp_table = dev_pm_opp_get_opp_table(dev);
> + dev_pm_opp_of_remove_table(dev);
> + dev_pm_opp_put_regulators(opp_table);
> + dev_pm_opp_put_opp_table(opp_table);
> +}
> +
> +static int devm_tegra_ehci_init_opp_table(struct device *dev)
> +{
> + unsigned long rate = ULONG_MAX;
> + struct opp_table *opp_table;
> + const char *rname = "core";
> + struct dev_pm_opp *opp;
> + int err;
> +
> + /* legacy device-trees don't have OPP table */
> + if (!device_property_present(dev, "operating-points-v2"))
> + return 0;
> +
> + /* voltage scaling is optional */
> + if (device_property_present(dev, "core-supply"))
> + opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
> + else
> + opp_table = dev_pm_opp_get_opp_table(dev);
> +
> + if (IS_ERR(opp_table))
> + return dev_err_probe(dev, PTR_ERR(opp_table),
> + "failed to prepare OPP table\n");
> +
> + err = dev_pm_opp_of_add_table(dev);
> + if (err) {
> + dev_err(dev, "failed to add OPP table: %d\n", err);
> + goto put_table;
> + }
> +
> + /* find suitable OPP for the maximum clock rate */
> + opp = dev_pm_opp_find_freq_floor(dev, &rate);
> + err = PTR_ERR_OR_ZERO(opp);
> + if (err) {
> + dev_err(dev, "failed to get OPP: %d\n", err);
> + goto remove_table;
> + }
> +
> + dev_pm_opp_put(opp);
> +
> + /*
> + * First dummy rate-set initializes voltage vote by setting voltage
> + * in accordance to the clock rate.
> + */
> + err = dev_pm_opp_set_rate(dev, rate);
> + if (err) {
> + dev_err(dev, "failed to initialize OPP clock: %d\n", err);
> + goto remove_table;
> + }
> +
> + err = devm_add_action(dev, tegra_ehci_deinit_opp_table, dev);
> + if (err)
> + goto remove_table;
> +
> + return 0;
> +
> +remove_table:
> + dev_pm_opp_of_remove_table(dev);
> +put_table:
> + dev_pm_opp_put_regulators(opp_table);

Do you really want to use the same error unwinding for opp_table values
obtained from dev_pm_opp_set_regulators() as from
dev_pm_opp_get_opp_table()?

> +
> + return err;
> +}
> +
> static const struct tegra_ehci_soc_config tegra30_soc_config = {
> .has_hostpc = true,
> };
> @@ -431,6 +505,11 @@ static int tegra_ehci_probe(struct platform_device *pdev)
> goto cleanup_hcd_create;
> }
>
> + err = devm_tegra_ehci_init_opp_table(&pdev->dev);
> + if (err)
> + return dev_err_probe(&pdev->dev, err,
> + "Failed to initialize OPP\n");

Why log a second error message? Just return err.

Alan Stern

2020-11-05 17:56:36

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v1 21/30] usb: host: ehci-tegra: Support OPP and SoC core voltage scaling

05.11.2020 19:07, Alan Stern пишет:
> Do you really want to use the same error unwinding for opp_table values
> obtained from dev_pm_opp_set_regulators() as from
> dev_pm_opp_get_opp_table()?

They both are pointing at the same opp_table, which is refcounted.

The dev_pm_opp_set_regulators() is dev_pm_opp_get_opp_table() + it sets
regulator for the table.

https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/opp/core.c#L1756

2020-11-05 18:06:51

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v1 21/30] usb: host: ehci-tegra: Support OPP and SoC core voltage scaling

05.11.2020 19:07, Alan Stern пишет:
>> + err = devm_tegra_ehci_init_opp_table(&pdev->dev);
>> + if (err)
>> + return dev_err_probe(&pdev->dev, err,
>> + "Failed to initialize OPP\n");
> Why log a second error message? Just return err.

Indeed, thanks.