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

[Alan Stern]

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