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 <pgwipeout@xxxxxxxxx>
> Tested-by: Nicolas Chauvet <kwizart@xxxxxxxxx>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
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
