Re: [PATCH 2/6] usb: dwc3: Get clocks individually

[Robert Hancock]

On Fri, 2022-01-14 at 18:39 -0500, Sean Anderson wrote:
> Instead of grabbing all clocks in bulk, grab them individually. This will
> allow us to get the frequency or otherwise deal with discrete clocks. This
> may break some platforms if they use a clock which doesn't use one of the
> documented names.

Another approach would be to keep the bulk get and prepare_enable and just
search through the clocks in the bulk data to find the "ref" clock, i.e.
something like:

		for (i = 0; i < dwc->num_clks; ++i)
			if (!strcmp("ref", dwc->clks[i].id)) {
				ref_clk_rate = clk_get_rate(dwc->clks[i].clk);
				break;
			}

That's probably simpler than all the extra complexity to get each of the clocks
individually and release them in the right order.

> 
> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
> ---
> 
>  drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++--------
>  drivers/usb/dwc3/core.h |  5 ++--
>  2 files changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f4c09951b517..699ab9abdc47 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -745,6 +745,38 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  	return 0;
>  }
>  
> +static int dwc3_clk_enable(struct dwc3 *dwc)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(dwc->bus_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(dwc->ref_clk);
> +	if (ret)
> +		goto disable_bus_clk;
> +
> +	ret = clk_prepare_enable(dwc->susp_clk);
> +	if (ret)
> +		goto disable_ref_clk;
> +
> +	return 0;
> +
> +disable_ref_clk:
> +	clk_disable_unprepare(dwc->ref_clk);
> +disable_bus_clk:
> +	clk_disable_unprepare(dwc->bus_clk);
> +	return ret;
> +}
> +
> +static void dwc3_clk_disable(struct dwc3 *dwc)
> +{
> +	clk_disable_unprepare(dwc->susp_clk);
> +	clk_disable_unprepare(dwc->ref_clk);
> +	clk_disable_unprepare(dwc->bus_clk);
> +}
> +
>  static void dwc3_core_exit(struct dwc3 *dwc)
>  {
>  	dwc3_event_buffers_cleanup(dwc);
> @@ -758,7 +790,7 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
>  	phy_power_off(dwc->usb2_generic_phy);
>  	phy_power_off(dwc->usb3_generic_phy);
> -	clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> +	dwc3_clk_disable(dwc);
>  	reset_control_assert(dwc->reset);
>  }
>  
> @@ -1605,25 +1637,31 @@ static int dwc3_probe(struct platform_device *pdev)
>  		return PTR_ERR(dwc->reset);
>  
>  	if (dev->of_node) {
> -		ret = devm_clk_bulk_get_all(dev, &dwc->clks);
> -		if (ret == -EPROBE_DEFER)
> -			return ret;
>  		/*
>  		 * Clocks are optional, but new DT platforms should support all
>  		 * clocks as required by the DT-binding.
>  		 */
> -		if (ret < 0)
> -			dwc->num_clks = 0;
> -		else
> -			dwc->num_clks = ret;
> +		dwc->bus_clk = devm_clk_get_optional(dev, "bus_early");
> +		if (IS_ERR(dwc->bus_clk))
> +			return dev_err_probe(dev, PTR_ERR(dwc->bus_clk),
> +					     "could not get bus clock\n");
>  
> +		dwc->ref_clk = devm_clk_get_optional(dev, "ref");
> +		if (IS_ERR(dwc->ref_clk))
> +			return dev_err_probe(dev, PTR_ERR(dwc->ref_clk),
> +					     "could not get ref clock\n");
> +
> +		dwc->susp_clk = devm_clk_get_optional(dev, "suspend");
> +		if (IS_ERR(dwc->susp_clk))
> +			return dev_err_probe(dev, PTR_ERR(dwc->susp_clk),
> +					     "could not get suspend clock\n");
>  	}
>  
>  	ret = reset_control_deassert(dwc->reset);
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
> +	ret = dwc3_clk_enable(dwc);
>  	if (ret)
>  		goto assert_reset;
>  
> @@ -1711,7 +1749,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  
>  disable_clks:
> -	clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> +	dwc3_clk_disable(dwc);
>  assert_reset:
>  	reset_control_assert(dwc->reset);
>  
> @@ -1755,7 +1793,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
> +	ret = dwc3_clk_enable(dwc);
>  	if (ret)
>  		goto assert_reset;
>  
> @@ -1766,7 +1804,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>  	return 0;
>  
>  disable_clks:
> -	clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> +	dwc3_clk_disable(dwc);
>  assert_reset:
>  	reset_control_assert(dwc->reset);
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index e1cc3f7398fb..32dfcf3a83d5 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1134,8 +1134,9 @@ struct dwc3 {
>  	struct usb_gadget	*gadget;
>  	struct usb_gadget_driver *gadget_driver;
>  
> -	struct clk_bulk_data	*clks;
> -	int			num_clks;
> +	struct clk		*bus_clk;
> +	struct clk		*ref_clk;
> +	struct clk		*susp_clk;
>  
>  	struct reset_control	*reset;
>  
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com