[RESEND PATCH 1/1] usb: dwc2: add multiple clock handling

[Frank Wang]

Hi Heiko, John and Greg,

On 2017/2/7 8:06, Heiko Stuebner wrote:
> Hi Frank,
>
> Am Sonntag, 5. Februar 2017, 10:51:01 CET schrieb Frank Wang:
>> Originally, dwc2 just handle one clock named otg, however, it may have
>> two or more clock need to manage for some new SoCs, so this adds
>> change clk to clk's array of dwc2_hsotg to handle more clocks operation.
>>
>> Signed-off-by: Frank Wang <frank.wang at rock-chips.com>
>> ---
>>   drivers/usb/dwc2/core.h     |  5 ++++-
>>   drivers/usb/dwc2/platform.c | 39 ++++++++++++++++++++++++++-------------
>>   2 files changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 1a7e830..d10a466 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -121,6 +121,9 @@ static inline void dwc2_writel(u32 value, void __iomem
>> *addr) /* Maximum number of Endpoints/HostChannels */
>>   #define MAX_EPS_CHANNELS	16
>>
>> +/* Maximum number of dwc2 clocks */
>> +#define DWC2_MAX_CLKS 3
> why 3 clocks?

Showing from the chapter 2.4 of dwc otg databook v3.10, it seems there 
should be five clocks, am I right?
hclk/pmu_hclk/utmi_clk/ulpi_clk/utmifs_clk48

> I.e. the binding currently only specifies the "otg" clock, so you should
> definitly amend it to also specifiy this "pmu" clock - in a separate patch
> before this one of course :-) .
> "pmu" also looks like a good name for that clock-binding and these new clocks
> of course should be optional in the binding.

No problem, I will amend the binding when the implementation scheme is 
clear.

> And ideally also just specify this mysterious third clock as well while you're
> at it.
>
>> +
>>   /* dwc2-hsotg declarations */
>>   static const char * const dwc2_hsotg_supply_names[] = {
>>   	"vusb_d",               /* digital USB supply, 1.2V */
>> @@ -913,7 +916,7 @@ struct dwc2_hsotg {
>>   	spinlock_t lock;
>>   	void *priv;
>>   	int     irq;
>> -	struct clk *clk;
>> +	struct clk *clks[DWC2_MAX_CLKS];
>>   	struct reset_control *reset;
>>
>>   	unsigned int queuing_high_bandwidth:1;
> [...]
>
>> @@ -123,17 +123,20 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
>>   static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
>>   {
>>   	struct platform_device *pdev = to_platform_device(hsotg->dev);
>> -	int ret;
>> +	int clk, ret;
>>
>>   	ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
>>   				    hsotg->supplies);
>>   	if (ret)
>>   		return ret;
>>
>> -	if (hsotg->clk) {
>> -		ret = clk_prepare_enable(hsotg->clk);
>> -		if (ret)
>> +	for (clk = 0; clk < DWC2_MAX_CLKS && hsotg->clks[clk]; clk++) {
>> +		ret = clk_prepare_enable(hsotg->clks[clk]);
>> +		if (ret) {
>> +			while (--clk >= 0)
>> +				clk_disable_unprepare(hsotg->clks[clk]);
>>   			return ret;
>> +		}
>>   	}
>>
>>   	if (hsotg->uphy) {
>> @@ -168,7 +171,7 @@ int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
>>   static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>>   {
>>   	struct platform_device *pdev = to_platform_device(hsotg->dev);
>> -	int ret = 0;
>> +	int clk, ret = 0;
>>
>>   	if (hsotg->uphy) {
>>   		usb_phy_shutdown(hsotg->uphy);
>> @@ -182,8 +185,9 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg
>> *hsotg) if (ret)
>>   		return ret;
>>
>> -	if (hsotg->clk)
>> -		clk_disable_unprepare(hsotg->clk);
>> +	for (clk = DWC2_MAX_CLKS - 1; clk >= 0; clk--)
>> +		if (hsotg->clks[clk])
>> +			clk_disable_unprepare(hsotg->clks[clk]);
>>   	ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
>>   				     hsotg->supplies);
>> @@ -209,7 +213,7 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>>
>>   static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
>>   {
>> -	int i, ret;
>> +	int i, clk, ret;
>>
>>   	hsotg->reset = devm_reset_control_get_optional(hsotg->dev, "dwc2");
>>   	if (IS_ERR(hsotg->reset)) {
>> @@ -282,11 +286,20 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
>> *hsotg) hsotg->phyif = GUSBCFG_PHYIF8;
>>   	}
>>
>> -	/* Clock */
>> -	hsotg->clk = devm_clk_get(hsotg->dev, "otg");
>> -	if (IS_ERR(hsotg->clk)) {
>> -		hsotg->clk = NULL;
>> -		dev_dbg(hsotg->dev, "cannot get otg clock\n");
>> +	/* Clocks */
>> +	for (clk = 0; clk < DWC2_MAX_CLKS; clk++) {
>> +		hsotg->clks[clk] = of_clk_get(hsotg->dev->of_node, clk);
>> +		if (IS_ERR(hsotg->clks[clk])) {
>> +			ret = PTR_ERR(hsotg->clks[clk]);
>> +			if (ret == -EPROBE_DEFER) {
>> +				while (--clk >= 0)
>> +					clk_put(hsotg->clks[clk]);
>> +				return ret;
>> +			}
>> +
>> +			hsotg->clks[clk] = NULL;
>> +			break;
>> +		}
> I guess this depends on the feelings of the usb-people, but for me it might
> make more sense to not carry the clocks in an anonymous array but instead as
> named members in struct dwc2_hsotg - aka clk_otg, clk_pmu, clk_??? .
>
> Because the otg clocks is actually marked as required in the binding while our
> two new clocks are optional and some future code might actually want to
> control these separate clocks to save some power somewhere.
>
>
> Sidenote: I don't really understand why the driver allows the otg clock to be
> missing, as it is a required property in the binding.

Yes, if there are only two clocks need to control, separate member in 
struct dwc2_hsotg is really better,
but for more clocks, the operations of each clock 
(get/prepare/unprepare) may become tedious

How about keeping the original 'otg' clk and adding a new clk array 
member for optional clocks in struct dwc2_hsotg?
because excepting 'otg' clk, the others are all optional clocks.

Of course, if we only consider hclk and pmu_hclk, I guess the separate 
member scheme is still better.

John and Greg, would you like to give some comments please?


BR.
Frank

> Heiko
>
>
>