[PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Shuah Khan]

Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
clock is specified. Call clk_disable_unprepare() from remove and probe
error path only when susp_clk has been set from remove and probe error
paths.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index e27899b..f97a3d7 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	if (IS_ERR(exynos->susp_clk)) {
 		dev_info(dev, "no suspend clk specified\n");
 		exynos->susp_clk = NULL;
-	}
-	clk_prepare_enable(exynos->susp_clk);
+	} else
+		clk_prepare_enable(exynos->susp_clk);
 
 	if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
 		exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
@@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	regulator_disable(exynos->vdd33);
 err2:
 	clk_disable_unprepare(exynos->axius_clk);
-	clk_disable_unprepare(exynos->susp_clk);
+	if (exynos->susp_clk)
+		clk_disable_unprepare(exynos->susp_clk);
 	clk_disable_unprepare(exynos->clk);
 	return ret;
 }
@@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
 	platform_device_unregister(exynos->usb3_phy);
 
 	clk_disable_unprepare(exynos->axius_clk);
-	clk_disable_unprepare(exynos->susp_clk);
+	if (exynos->susp_clk)
+		clk_disable_unprepare(exynos->susp_clk);
 	clk_disable_unprepare(exynos->clk);
 
 	regulator_disable(exynos->vdd33);
-- 
2.7.4

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Sergei Shtylyov]

Hello!

On 01/10/2017 05:21 AM, Shuah Khan wrote:

> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
> clock is specified. Call clk_disable_unprepare() from remove and probe
> error path only when susp_clk has been set from remove and probe error
> paths.
>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index e27899b..f97a3d7 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  	if (IS_ERR(exynos->susp_clk)) {
>  		dev_info(dev, "no suspend clk specified\n");
>  		exynos->susp_clk = NULL;
> -	}
> -	clk_prepare_enable(exynos->susp_clk);
> +	} else
> +		clk_prepare_enable(exynos->susp_clk);

    CodingStyle: need {} here as well since another branch has them.

[...]

MBR, Sergei

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
> clock is specified. Call clk_disable_unprepare() from remove and probe
> error path only when susp_clk has been set from remove and probe error
> paths.

It is legal to call clk_prepare_enable() and clk_disable_unprepare()
for NULL clock.  Also your patch changes susp_clk handling while
leaves axius_clk handling (which also can be NULL) untouched.

Do you actually see some runtime problem with the current code?

If not then the patch should probably be dropped.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index e27899b..f97a3d7 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  	if (IS_ERR(exynos->susp_clk)) {
>  		dev_info(dev, "no suspend clk specified\n");
>  		exynos->susp_clk = NULL;
> -	}
> -	clk_prepare_enable(exynos->susp_clk);
> +	} else
> +		clk_prepare_enable(exynos->susp_clk);
>  
>  	if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
>  		exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>  	regulator_disable(exynos->vdd33);
>  err2:
>  	clk_disable_unprepare(exynos->axius_clk);
> -	clk_disable_unprepare(exynos->susp_clk);
> +	if (exynos->susp_clk)
> +		clk_disable_unprepare(exynos->susp_clk);
>  	clk_disable_unprepare(exynos->clk);
>  	return ret;
>  }
> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>  	platform_device_unregister(exynos->usb3_phy);
>  
>  	clk_disable_unprepare(exynos->axius_clk);
> -	clk_disable_unprepare(exynos->susp_clk);
> +	if (exynos->susp_clk)
> +		clk_disable_unprepare(exynos->susp_clk);
>  	clk_disable_unprepare(exynos->clk);
>  
>  	regulator_disable(exynos->vdd33);

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Shuah Khan]

On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>> clock is specified. Call clk_disable_unprepare() from remove and probe
>> error path only when susp_clk has been set from remove and probe error
>> paths.
> 
> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
> for NULL clock.  Also your patch changes susp_clk handling while
> leaves axius_clk handling (which also can be NULL) untouched.
> 
> Do you actually see some runtime problem with the current code?
> 
> If not then the patch should probably be dropped.
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics

Hi Bartlomiej,

I am seeing the "no suspend clk specified" message in dmesg.
After that it sets the exynos->susp_clk = NULL and starts
calling clk_prepare_enable(exynos->susp_clk);

That can't be good. If you see the logic right above this
one for exynos->clk, it returns error and fails the probe.
This this case it doesn't, but tries to use null susp_clk.

I believe this patch is necessary.

thanks,
-- Shuah

> 
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>> index e27899b..f97a3d7 100644
>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>  	if (IS_ERR(exynos->susp_clk)) {
>>  		dev_info(dev, "no suspend clk specified\n");
>>  		exynos->susp_clk = NULL;
>> -	}
>> -	clk_prepare_enable(exynos->susp_clk);
>> +	} else
>> +		clk_prepare_enable(exynos->susp_clk);
>>  
>>  	if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
>>  		exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>  	regulator_disable(exynos->vdd33);
>>  err2:
>>  	clk_disable_unprepare(exynos->axius_clk);
>> -	clk_disable_unprepare(exynos->susp_clk);
>> +	if (exynos->susp_clk)
>> +		clk_disable_unprepare(exynos->susp_clk);
>>  	clk_disable_unprepare(exynos->clk);
>>  	return ret;
>>  }
>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>>  	platform_device_unregister(exynos->usb3_phy);
>>  
>>  	clk_disable_unprepare(exynos->axius_clk);
>> -	clk_disable_unprepare(exynos->susp_clk);
>> +	if (exynos->susp_clk)
>> +		clk_disable_unprepare(exynos->susp_clk);
>>  	clk_disable_unprepare(exynos->clk);
>>  
>>  	regulator_disable(exynos->vdd33);
> 

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Shuah Khan]

On 01/10/2017 07:16 AM, Shuah Khan wrote:
> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>>> error path only when susp_clk has been set from remove and probe error
>>> paths.
>>
>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>> for NULL clock.  Also your patch changes susp_clk handling while
>> leaves axius_clk handling (which also can be NULL) untouched.
>>
>> Do you actually see some runtime problem with the current code?
>>
>> If not then the patch should probably be dropped.
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
> 
> Hi Bartlomiej,
> 
> I am seeing the "no suspend clk specified" message in dmesg.
> After that it sets the exynos->susp_clk = NULL and starts
> calling clk_prepare_enable(exynos->susp_clk);
> 
> That can't be good. If you see the logic right above this
> one for exynos->clk, it returns error and fails the probe.
> This this case it doesn't, but tries to use null susp_clk.
> 
> I believe this patch is necessary.

Let me clarify this a bit further. Since we already know
susp_clk is null, with this patch we can avoid extra calls
to clk_prepare_enable() and clk_disable_unprepare().

One can say, it also adds extra checks, hence I will let you
decide one way or the other. :)

thanks,
-- Shuah

> 
> thanks,
> -- Shuah
> 
>>
>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>> ---
>>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>>> index e27899b..f97a3d7 100644
>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>  	if (IS_ERR(exynos->susp_clk)) {
>>>  		dev_info(dev, "no suspend clk specified\n");
>>>  		exynos->susp_clk = NULL;
>>> -	}
>>> -	clk_prepare_enable(exynos->susp_clk);
>>> +	} else
>>> +		clk_prepare_enable(exynos->susp_clk);
>>>  
>>>  	if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
>>>  		exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>  	regulator_disable(exynos->vdd33);
>>>  err2:
>>>  	clk_disable_unprepare(exynos->axius_clk);
>>> -	clk_disable_unprepare(exynos->susp_clk);
>>> +	if (exynos->susp_clk)
>>> +		clk_disable_unprepare(exynos->susp_clk);
>>>  	clk_disable_unprepare(exynos->clk);
>>>  	return ret;
>>>  }
>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>>>  	platform_device_unregister(exynos->usb3_phy);
>>>  
>>>  	clk_disable_unprepare(exynos->axius_clk);
>>> -	clk_disable_unprepare(exynos->susp_clk);
>>> +	if (exynos->susp_clk)
>>> +		clk_disable_unprepare(exynos->susp_clk);
>>>  	clk_disable_unprepare(exynos->clk);
>>>  
>>>  	regulator_disable(exynos->vdd33);
>>
> 

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Shuah Khan]

On 01/10/2017 04:20 AM, Sergei Shtylyov wrote:
> Hello!
> 
> On 01/10/2017 05:21 AM, Shuah Khan wrote:
> 
>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>> clock is specified. Call clk_disable_unprepare() from remove and probe
>> error path only when susp_clk has been set from remove and probe error
>> paths.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>> index e27899b..f97a3d7 100644
>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>      if (IS_ERR(exynos->susp_clk)) {
>>          dev_info(dev, "no suspend clk specified\n");
>>          exynos->susp_clk = NULL;
>> -    }
>> -    clk_prepare_enable(exynos->susp_clk);
>> +    } else
>> +        clk_prepare_enable(exynos->susp_clk);
> 
>    CodingStyle: need {} here as well since another branch has them.
> 
> [...]
> 
> MBR, Sergei
> 

Thanks. There is some concerns whether or not this patch is needed.
If we decide to include the patch, I will send v2 with your comment.

thanks,
-- Shuah

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
> On 01/10/2017 07:16 AM, Shuah Khan wrote:
> > On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
> >>
> >> Hi,
> >>
> >> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
> >>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
> >>> clock is specified. Call clk_disable_unprepare() from remove and probe
> >>> error path only when susp_clk has been set from remove and probe error
> >>> paths.
> >>
> >> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
> >> for NULL clock.  Also your patch changes susp_clk handling while
> >> leaves axius_clk handling (which also can be NULL) untouched.
> >>
> >> Do you actually see some runtime problem with the current code?
> >>
> >> If not then the patch should probably be dropped.
> >>
> >> Best regards,
> >> --
> >> Bartlomiej Zolnierkiewicz
> >> Samsung R&D Institute Poland
> >> Samsung Electronics
> > 
> > Hi Bartlomiej,
> > 
> > I am seeing the "no suspend clk specified" message in dmesg.
> > After that it sets the exynos->susp_clk = NULL and starts
> > calling clk_prepare_enable(exynos->susp_clk);
> > 
> > That can't be good. If you see the logic right above this
> > one for exynos->clk, it returns error and fails the probe.
> > This this case it doesn't, but tries to use null susp_clk.

exynos->susp_clk is optional, exynos->clk is not.

> > I believe this patch is necessary.
> 
> Let me clarify this a bit further. Since we already know
> susp_clk is null, with this patch we can avoid extra calls
> to clk_prepare_enable() and clk_disable_unprepare().
> 
> One can say, it also adds extra checks, hence I will let you
> decide one way or the other. :)

I would prefer to leave the things as they are currently.

The code in question is not performance sensitive so extra
calls are not a problem.  No extra checks means less code.

Also the current code seems to be more in line with the rest
of the kernel.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> thanks,
> -- Shuah
> 
> > 
> > thanks,
> > -- Shuah
> > 
> >>
> >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >>> ---
> >>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
> >>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> >>> index e27899b..f97a3d7 100644
> >>> --- a/drivers/usb/dwc3/dwc3-exynos.c
> >>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> >>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>  	if (IS_ERR(exynos->susp_clk)) {
> >>>  		dev_info(dev, "no suspend clk specified\n");
> >>>  		exynos->susp_clk = NULL;
> >>> -	}
> >>> -	clk_prepare_enable(exynos->susp_clk);
> >>> +	} else
> >>> +		clk_prepare_enable(exynos->susp_clk);
> >>>  
> >>>  	if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
> >>>  		exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
> >>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>  	regulator_disable(exynos->vdd33);
> >>>  err2:
> >>>  	clk_disable_unprepare(exynos->axius_clk);
> >>> -	clk_disable_unprepare(exynos->susp_clk);
> >>> +	if (exynos->susp_clk)
> >>> +		clk_disable_unprepare(exynos->susp_clk);
> >>>  	clk_disable_unprepare(exynos->clk);
> >>>  	return ret;
> >>>  }
> >>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
> >>>  	platform_device_unregister(exynos->usb3_phy);
> >>>  
> >>>  	clk_disable_unprepare(exynos->axius_clk);
> >>> -	clk_disable_unprepare(exynos->susp_clk);
> >>> +	if (exynos->susp_clk)
> >>> +		clk_disable_unprepare(exynos->susp_clk);
> >>>  	clk_disable_unprepare(exynos->clk);
> >>>  
> >>>  	regulator_disable(exynos->vdd33);

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Shuah Khan]

On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>>>>> error path only when susp_clk has been set from remove and probe error
>>>>> paths.
>>>>
>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>>>> for NULL clock.  Also your patch changes susp_clk handling while
>>>> leaves axius_clk handling (which also can be NULL) untouched.
>>>>
>>>> Do you actually see some runtime problem with the current code?
>>>>
>>>> If not then the patch should probably be dropped.
>>>>
>>>> Best regards,
>>>> --
>>>> Bartlomiej Zolnierkiewicz
>>>> Samsung R&D Institute Poland
>>>> Samsung Electronics
>>>
>>> Hi Bartlomiej,
>>>
>>> I am seeing the "no suspend clk specified" message in dmesg.
>>> After that it sets the exynos->susp_clk = NULL and starts
>>> calling clk_prepare_enable(exynos->susp_clk);
>>>
>>> That can't be good. If you see the logic right above this
>>> one for exynos->clk, it returns error and fails the probe.
>>> This this case it doesn't, but tries to use null susp_clk.
> 
> exynos->susp_clk is optional, exynos->clk is not.

Right. That is clear since we don't fail the probe.

> 
>>> I believe this patch is necessary.
>>
>> Let me clarify this a bit further. Since we already know
>> susp_clk is null, with this patch we can avoid extra calls
>> to clk_prepare_enable() and clk_disable_unprepare().
>>
>> One can say, it also adds extra checks, hence I will let you
>> decide one way or the other. :)
> 
> I would prefer to leave the things as they are currently.
> 
> The code in question is not performance sensitive so extra
> calls are not a problem.  No extra checks means less code.
> 
> Also the current code seems to be more in line with the rest
> of the kernel.

What functionality is missing without the suspend clock? Would
it make sense to change the info. message to include what it
means. At the moment it doesn't anything more than "no suspend
clock" which is a very cryptic user visible message. It would be
helpful for it to also include what functionality is impacted.

thanks,
-- Shuah

> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
>> thanks,
>> -- Shuah
>>
>>>
>>> thanks,
>>> -- Shuah
>>>
>>>>
>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>>> ---
>>>>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>>>>> index e27899b..f97a3d7 100644
>>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>>>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>  	if (IS_ERR(exynos->susp_clk)) {
>>>>>  		dev_info(dev, "no suspend clk specified\n");
>>>>>  		exynos->susp_clk = NULL;
>>>>> -	}
>>>>> -	clk_prepare_enable(exynos->susp_clk);
>>>>> +	} else
>>>>> +		clk_prepare_enable(exynos->susp_clk);
>>>>>  
>>>>>  	if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
>>>>>  		exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
>>>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>  	regulator_disable(exynos->vdd33);
>>>>>  err2:
>>>>>  	clk_disable_unprepare(exynos->axius_clk);
>>>>> -	clk_disable_unprepare(exynos->susp_clk);
>>>>> +	if (exynos->susp_clk)
>>>>> +		clk_disable_unprepare(exynos->susp_clk);
>>>>>  	clk_disable_unprepare(exynos->clk);
>>>>>  	return ret;
>>>>>  }
>>>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>>>>>  	platform_device_unregister(exynos->usb3_phy);
>>>>>  
>>>>>  	clk_disable_unprepare(exynos->axius_clk);
>>>>> -	clk_disable_unprepare(exynos->susp_clk);
>>>>> +	if (exynos->susp_clk)
>>>>> +		clk_disable_unprepare(exynos->susp_clk);
>>>>>  	clk_disable_unprepare(exynos->clk);
>>>>>  
>>>>>  	regulator_disable(exynos->vdd33);
> 

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, January 10, 2017 09:28:52 AM Shuah Khan wrote:
> On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
> >> On 01/10/2017 07:16 AM, Shuah Khan wrote:
> >>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
> >>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
> >>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
> >>>>> error path only when susp_clk has been set from remove and probe error
> >>>>> paths.
> >>>>
> >>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
> >>>> for NULL clock.  Also your patch changes susp_clk handling while
> >>>> leaves axius_clk handling (which also can be NULL) untouched.
> >>>>
> >>>> Do you actually see some runtime problem with the current code?
> >>>>
> >>>> If not then the patch should probably be dropped.
> >>>>
> >>>> Best regards,
> >>>> --
> >>>> Bartlomiej Zolnierkiewicz
> >>>> Samsung R&D Institute Poland
> >>>> Samsung Electronics
> >>>
> >>> Hi Bartlomiej,
> >>>
> >>> I am seeing the "no suspend clk specified" message in dmesg.
> >>> After that it sets the exynos->susp_clk = NULL and starts
> >>> calling clk_prepare_enable(exynos->susp_clk);
> >>>
> >>> That can't be good. If you see the logic right above this
> >>> one for exynos->clk, it returns error and fails the probe.
> >>> This this case it doesn't, but tries to use null susp_clk.
> > 
> > exynos->susp_clk is optional, exynos->clk is not.
> 
> Right. That is clear since we don't fail the probe.
> 
> > 
> >>> I believe this patch is necessary.
> >>
> >> Let me clarify this a bit further. Since we already know
> >> susp_clk is null, with this patch we can avoid extra calls
> >> to clk_prepare_enable() and clk_disable_unprepare().
> >>
> >> One can say, it also adds extra checks, hence I will let you
> >> decide one way or the other. :)
> > 
> > I would prefer to leave the things as they are currently.
> > 
> > The code in question is not performance sensitive so extra
> > calls are not a problem.  No extra checks means less code.
> > 
> > Also the current code seems to be more in line with the rest
> > of the kernel.
> 
> What functionality is missing without the suspend clock? Would
> it make sense to change the info. message to include what it
> means. At the moment it doesn't anything more than "no suspend
> clock" which is a very cryptic user visible message. It would be
> helpful for it to also include what functionality is impacted.

Well, all I know is what the original commit descriptions says and
that the commit itself comes from patchset adding Exynos7 USB 3.0
support [1]:

commit 72d996fc7a01c2e4d581a15db7d001e2799ffb29
Author: Vivek Gautam <gautam.vivek@samsung.com>
Date:   Fri Nov 21 19:05:46 2014 +0530

    usb: dwc3: exynos: Add provision for suspend clock
    
    DWC3 controller on Exynos SoC series have separate control for
    suspend clock which replaces pipe3_rx_pclk as clock source to
    a small part of DWC3 core that operates when SS PHY is in its
    lowest power state (P3) in states SS.disabled and U3.
    
    Suggested-by: Anton Tikhomirov <av.tikhomirov@samsung.com>
    Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
    Signed-off-by: Felipe Balbi <balbi@ti.com>

Anton's & Vivek's Samsung email addresses are no longer valid but
I added current Vivek's email address to Cc:.

BTW What is interesting is that the Exynos7 dts patch [2] has never
made it into upstream for some reason.  In the meantime however
Exynos5433 (similar to Exynos7 to some degree) became the user of
susp_clk.

[1] https://lkml.org/lkml/2014/11/21/247
[2] https://patchwork.kernel.org/patch/5355161/

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> thanks,
> -- Shuah
> 
> > 
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> > 
> >> thanks,
> >> -- Shuah
> >>
> >>>
> >>> thanks,
> >>> -- Shuah
> >>>
> >>>>
> >>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >>>>> ---
> >>>>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
> >>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> >>>>> index e27899b..f97a3d7 100644
> >>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
> >>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> >>>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>>>  	if (IS_ERR(exynos->susp_clk)) {
> >>>>>  		dev_info(dev, "no suspend clk specified\n");
> >>>>>  		exynos->susp_clk = NULL;
> >>>>> -	}
> >>>>> -	clk_prepare_enable(exynos->susp_clk);
> >>>>> +	} else
> >>>>> +		clk_prepare_enable(exynos->susp_clk);
> >>>>>  
> >>>>>  	if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
> >>>>>  		exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
> >>>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>>>  	regulator_disable(exynos->vdd33);
> >>>>>  err2:
> >>>>>  	clk_disable_unprepare(exynos->axius_clk);
> >>>>> -	clk_disable_unprepare(exynos->susp_clk);
> >>>>> +	if (exynos->susp_clk)
> >>>>> +		clk_disable_unprepare(exynos->susp_clk);
> >>>>>  	clk_disable_unprepare(exynos->clk);
> >>>>>  	return ret;
> >>>>>  }
> >>>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
> >>>>>  	platform_device_unregister(exynos->usb3_phy);
> >>>>>  
> >>>>>  	clk_disable_unprepare(exynos->axius_clk);
> >>>>> -	clk_disable_unprepare(exynos->susp_clk);
> >>>>> +	if (exynos->susp_clk)
> >>>>> +		clk_disable_unprepare(exynos->susp_clk);
> >>>>>  	clk_disable_unprepare(exynos->clk);
> >>>>>  
> >>>>>  	regulator_disable(exynos->vdd33);

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[vivek.gautam]

On 2017-01-10 22:39, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> On Tuesday, January 10, 2017 09:28:52 AM Shuah Khan wrote:
>> On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
>> >
>> > Hi,
>> >
>> > On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
>> >> On 01/10/2017 07:16 AM, Shuah Khan wrote:
>> >>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>> >>>>
>> >>>> Hi,
>> >>>>
>> >>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>> >>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>> >>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>> >>>>> error path only when susp_clk has been set from remove and probe error
>> >>>>> paths.
>> >>>>
>> >>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>> >>>> for NULL clock.  Also your patch changes susp_clk handling while
>> >>>> leaves axius_clk handling (which also can be NULL) untouched.
>> >>>>
>> >>>> Do you actually see some runtime problem with the current code?
>> >>>>
>> >>>> If not then the patch should probably be dropped.
>> >>>>
>> >>>> Best regards,
>> >>>> --
>> >>>> Bartlomiej Zolnierkiewicz
>> >>>> Samsung R&D Institute Poland
>> >>>> Samsung Electronics
>> >>>
>> >>> Hi Bartlomiej,
>> >>>
>> >>> I am seeing the "no suspend clk specified" message in dmesg.
>> >>> After that it sets the exynos->susp_clk = NULL and starts
>> >>> calling clk_prepare_enable(exynos->susp_clk);
>> >>>
>> >>> That can't be good. If you see the logic right above this
>> >>> one for exynos->clk, it returns error and fails the probe.
>> >>> This this case it doesn't, but tries to use null susp_clk.
>> >
>> > exynos->susp_clk is optional, exynos->clk is not.
>> 
>> Right. That is clear since we don't fail the probe.
>> 
>> >
>> >>> I believe this patch is necessary.
>> >>
>> >> Let me clarify this a bit further. Since we already know
>> >> susp_clk is null, with this patch we can avoid extra calls
>> >> to clk_prepare_enable() and clk_disable_unprepare().
>> >>
>> >> One can say, it also adds extra checks, hence I will let you
>> >> decide one way or the other. :)

Hi Shuah Khan,

Like Bartlomiej mentioned below, it is completely fair to call
clk_prepare_enable() with NULL clocks. That's how most of the
optional clocks are handled in the kernel. So, i don't think
there's any need of adding an additional check for the 
'exynos->susp_clk'.

The 'exynos->clk' is the main IP clock that is mandatory, and hence
the probe fails in case that is not present.

>> >
>> > I would prefer to leave the things as they are currently.
>> >
>> > The code in question is not performance sensitive so extra
>> > calls are not a problem.  No extra checks means less code.
>> >
>> > Also the current code seems to be more in line with the rest
>> > of the kernel.
>> 
>> What functionality is missing without the suspend clock? Would
>> it make sense to change the info. message to include what it
>> means. At the moment it doesn't anything more than "no suspend
>> clock" which is a very cryptic user visible message. It would be
>> helpful for it to also include what functionality is impacted.
> 
> Well, all I know is what the original commit descriptions says and
> that the commit itself comes from patchset adding Exynos7 USB 3.0
> support [1]:
> 
> commit 72d996fc7a01c2e4d581a15db7d001e2799ffb29
> Author: Vivek Gautam <gautam.vivek@samsung.com>
> Date:   Fri Nov 21 19:05:46 2014 +0530
> 
>     usb: dwc3: exynos: Add provision for suspend clock
> 
>     DWC3 controller on Exynos SoC series have separate control for
>     suspend clock which replaces pipe3_rx_pclk as clock source to
>     a small part of DWC3 core that operates when SS PHY is in its
>     lowest power state (P3) in states SS.disabled and U3.
> 
>     Suggested-by: Anton Tikhomirov <av.tikhomirov@samsung.com>
>     Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>     Signed-off-by: Felipe Balbi <balbi@ti.com>
> 

Hi Bartlomiej,

> Anton's & Vivek's Samsung email addresses are no longer valid but
> I added current Vivek's email address to Cc:.

Thanks for adding me to the thread.

> 
> BTW What is interesting is that the Exynos7 dts patch [2] has never
> made it into upstream for some reason.  In the meantime however
> Exynos5433 (similar to Exynos7 to some degree) became the user of
> susp_clk.

You are right. The exynos7 device tree patches could not make it to
upstream for some reasons. I think there are few guys looking at USB
for Exynos.
If there's something needed on Exynos7 USB side, i have added
Pankaj Dubey <pankaj.dubey@samsung.com> to this thread.

Hi Pankaj,
I am adding you to please help us with any future requirements
for exynos USB in the upstream.
Thanks!

> 
> [1] https://lkml.org/lkml/2014/11/21/247
> [2] https://patchwork.kernel.org/patch/5355161/
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics

[snip]


Best Regards
Vivek

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Anand Moon]

Hi Shuah,

On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
>>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
>>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>>>>>> error path only when susp_clk has been set from remove and probe error
>>>>>> paths.
>>>>>
>>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>>>>> for NULL clock.  Also your patch changes susp_clk handling while
>>>>> leaves axius_clk handling (which also can be NULL) untouched.
>>>>>
>>>>> Do you actually see some runtime problem with the current code?
>>>>>
>>>>> If not then the patch should probably be dropped.
>>>>>
>>>>> Best regards,
>>>>> --
>>>>> Bartlomiej Zolnierkiewicz
>>>>> Samsung R&D Institute Poland
>>>>> Samsung Electronics
>>>>
>>>> Hi Bartlomiej,
>>>>
>>>> I am seeing the "no suspend clk specified" message in dmesg.
>>>> After that it sets the exynos->susp_clk = NULL and starts
>>>> calling clk_prepare_enable(exynos->susp_clk);
>>>>
>>>> That can't be good. If you see the logic right above this
>>>> one for exynos->clk, it returns error and fails the probe.
>>>> This this case it doesn't, but tries to use null susp_clk.
>>
>> exynos->susp_clk is optional, exynos->clk is not.
>
> Right. That is clear since we don't fail the probe.
>
>>
>>>> I believe this patch is necessary.
>>>
>>> Let me clarify this a bit further. Since we already know
>>> susp_clk is null, with this patch we can avoid extra calls
>>> to clk_prepare_enable() and clk_disable_unprepare().
>>>
>>> One can say, it also adds extra checks, hence I will let you
>>> decide one way or the other. :)
>>
>> I would prefer to leave the things as they are currently.
>>
>> The code in question is not performance sensitive so extra
>> calls are not a problem.  No extra checks means less code.
>>
>> Also the current code seems to be more in line with the rest
>> of the kernel.
>
> What functionality is missing without the suspend clock? Would
> it make sense to change the info. message to include what it
> means. At the moment it doesn't anything more than "no suspend
> clock" which is a very cryptic user visible message. It would be
> helpful for it to also include what functionality is impacted.
>

Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.

Best Regards
-Anand

> thanks,
> -- Shuah
>
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>>> thanks,
>>> -- Shuah
>>>
>>>>
>>>> thanks,
>>>> -- Shuah
>>>>
>>>>>
>>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>>>> ---
>>>>>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>>>>>> index e27899b..f97a3d7 100644
>>>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>>>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>>>>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>>   if (IS_ERR(exynos->susp_clk)) {
>>>>>>           dev_info(dev, "no suspend clk specified\n");
>>>>>>           exynos->susp_clk = NULL;
>>>>>> - }
>>>>>> - clk_prepare_enable(exynos->susp_clk);
>>>>>> + } else
>>>>>> +         clk_prepare_enable(exynos->susp_clk);
>>>>>>
>>>>>>   if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
>>>>>>           exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
>>>>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>>>>>>   regulator_disable(exynos->vdd33);
>>>>>>  err2:
>>>>>>   clk_disable_unprepare(exynos->axius_clk);
>>>>>> - clk_disable_unprepare(exynos->susp_clk);
>>>>>> + if (exynos->susp_clk)
>>>>>> +         clk_disable_unprepare(exynos->susp_clk);
>>>>>>   clk_disable_unprepare(exynos->clk);
>>>>>>   return ret;
>>>>>>  }
>>>>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>>>>>>   platform_device_unregister(exynos->usb3_phy);
>>>>>>
>>>>>>   clk_disable_unprepare(exynos->axius_clk);
>>>>>> - clk_disable_unprepare(exynos->susp_clk);
>>>>>> + if (exynos->susp_clk)
>>>>>> +         clk_disable_unprepare(exynos->susp_clk);
>>>>>>   clk_disable_unprepare(exynos->clk);
>>>>>>
>>>>>>   regulator_disable(exynos->vdd33);
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, January 10, 2017 11:23:38 PM Anand Moon wrote:
> Hi Shuah,
> 
> On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> > On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
> >>
> >> Hi,
> >>
> >> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
> >>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
> >>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
> >>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
> >>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
> >>>>>> error path only when susp_clk has been set from remove and probe error
> >>>>>> paths.
> >>>>>
> >>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
> >>>>> for NULL clock.  Also your patch changes susp_clk handling while
> >>>>> leaves axius_clk handling (which also can be NULL) untouched.
> >>>>>
> >>>>> Do you actually see some runtime problem with the current code?
> >>>>>
> >>>>> If not then the patch should probably be dropped.
> >>>>>
> >>>>> Best regards,
> >>>>> --
> >>>>> Bartlomiej Zolnierkiewicz
> >>>>> Samsung R&D Institute Poland
> >>>>> Samsung Electronics
> >>>>
> >>>> Hi Bartlomiej,
> >>>>
> >>>> I am seeing the "no suspend clk specified" message in dmesg.
> >>>> After that it sets the exynos->susp_clk = NULL and starts
> >>>> calling clk_prepare_enable(exynos->susp_clk);
> >>>>
> >>>> That can't be good. If you see the logic right above this
> >>>> one for exynos->clk, it returns error and fails the probe.
> >>>> This this case it doesn't, but tries to use null susp_clk.
> >>
> >> exynos->susp_clk is optional, exynos->clk is not.
> >
> > Right. That is clear since we don't fail the probe.
> >
> >>
> >>>> I believe this patch is necessary.
> >>>
> >>> Let me clarify this a bit further. Since we already know
> >>> susp_clk is null, with this patch we can avoid extra calls
> >>> to clk_prepare_enable() and clk_disable_unprepare().
> >>>
> >>> One can say, it also adds extra checks, hence I will let you
> >>> decide one way or the other. :)
> >>
> >> I would prefer to leave the things as they are currently.
> >>
> >> The code in question is not performance sensitive so extra
> >> calls are not a problem.  No extra checks means less code.
> >>
> >> Also the current code seems to be more in line with the rest
> >> of the kernel.
> >
> > What functionality is missing without the suspend clock? Would
> > it make sense to change the info. message to include what it
> > means. At the moment it doesn't anything more than "no suspend
> > clock" which is a very cryptic user visible message. It would be
> > helpful for it to also include what functionality is impacted.
> >
> 
> Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform

Can you point me to the use of usbdrd30_axius_clk?

I cannot find in the upstream code.

> so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.

This is not so simple and we would probably need a new compatible for
Exynos5433 (it is currently using "samsung,exynos5250-dwusb3" one and
is not using axius_clk).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Best Regards
> -Anand
> 
> > thanks,
> > -- Shuah
> >
> >>
> >> Best regards,
> >> --
> >> Bartlomiej Zolnierkiewicz
> >> Samsung R&D Institute Poland
> >> Samsung Electronics
> >>
> >>> thanks,
> >>> -- Shuah
> >>>
> >>>>
> >>>> thanks,
> >>>> -- Shuah
> >>>>
> >>>>>
> >>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >>>>>> ---
> >>>>>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
> >>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> >>>>>> index e27899b..f97a3d7 100644
> >>>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
> >>>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> >>>>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>>>>   if (IS_ERR(exynos->susp_clk)) {
> >>>>>>           dev_info(dev, "no suspend clk specified\n");
> >>>>>>           exynos->susp_clk = NULL;
> >>>>>> - }
> >>>>>> - clk_prepare_enable(exynos->susp_clk);
> >>>>>> + } else
> >>>>>> +         clk_prepare_enable(exynos->susp_clk);
> >>>>>>
> >>>>>>   if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
> >>>>>>           exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
> >>>>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>>>>   regulator_disable(exynos->vdd33);
> >>>>>>  err2:
> >>>>>>   clk_disable_unprepare(exynos->axius_clk);
> >>>>>> - clk_disable_unprepare(exynos->susp_clk);
> >>>>>> + if (exynos->susp_clk)
> >>>>>> +         clk_disable_unprepare(exynos->susp_clk);
> >>>>>>   clk_disable_unprepare(exynos->clk);
> >>>>>>   return ret;
> >>>>>>  }
> >>>>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
> >>>>>>   platform_device_unregister(exynos->usb3_phy);
> >>>>>>
> >>>>>>   clk_disable_unprepare(exynos->axius_clk);
> >>>>>> - clk_disable_unprepare(exynos->susp_clk);
> >>>>>> + if (exynos->susp_clk)
> >>>>>> +         clk_disable_unprepare(exynos->susp_clk);
> >>>>>>   clk_disable_unprepare(exynos->clk);
> >>>>>>
> >>>>>>   regulator_disable(exynos->vdd33);

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Bartlomiej Zolnierkiewicz]

On Tuesday, January 10, 2017 07:03:57 PM Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Tuesday, January 10, 2017 11:23:38 PM Anand Moon wrote:
> > Hi Shuah,
> > 
> > On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> > > On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
> > >>
> > >> Hi,
> > >>
> > >> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
> > >>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
> > >>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
> > >>>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
> > >>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
> > >>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
> > >>>>>> error path only when susp_clk has been set from remove and probe error
> > >>>>>> paths.
> > >>>>>
> > >>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
> > >>>>> for NULL clock.  Also your patch changes susp_clk handling while
> > >>>>> leaves axius_clk handling (which also can be NULL) untouched.
> > >>>>>
> > >>>>> Do you actually see some runtime problem with the current code?
> > >>>>>
> > >>>>> If not then the patch should probably be dropped.
> > >>>>>
> > >>>>> Best regards,
> > >>>>> --
> > >>>>> Bartlomiej Zolnierkiewicz
> > >>>>> Samsung R&D Institute Poland
> > >>>>> Samsung Electronics
> > >>>>
> > >>>> Hi Bartlomiej,
> > >>>>
> > >>>> I am seeing the "no suspend clk specified" message in dmesg.
> > >>>> After that it sets the exynos->susp_clk = NULL and starts
> > >>>> calling clk_prepare_enable(exynos->susp_clk);
> > >>>>
> > >>>> That can't be good. If you see the logic right above this
> > >>>> one for exynos->clk, it returns error and fails the probe.
> > >>>> This this case it doesn't, but tries to use null susp_clk.
> > >>
> > >> exynos->susp_clk is optional, exynos->clk is not.
> > >
> > > Right. That is clear since we don't fail the probe.
> > >
> > >>
> > >>>> I believe this patch is necessary.
> > >>>
> > >>> Let me clarify this a bit further. Since we already know
> > >>> susp_clk is null, with this patch we can avoid extra calls
> > >>> to clk_prepare_enable() and clk_disable_unprepare().
> > >>>
> > >>> One can say, it also adds extra checks, hence I will let you
> > >>> decide one way or the other. :)
> > >>
> > >> I would prefer to leave the things as they are currently.
> > >>
> > >> The code in question is not performance sensitive so extra
> > >> calls are not a problem.  No extra checks means less code.
> > >>
> > >> Also the current code seems to be more in line with the rest
> > >> of the kernel.
> > >
> > > What functionality is missing without the suspend clock? Would
> > > it make sense to change the info. message to include what it
> > > means. At the moment it doesn't anything more than "no suspend
> > > clock" which is a very cryptic user visible message. It would be
> > > helpful for it to also include what functionality is impacted.
> > >
> > 
> > Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
> 
> Can you point me to the use of usbdrd30_axius_clk?
> 
> I cannot find in the upstream code.
> 
> > so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.
> 
> This is not so simple and we would probably need a new compatible for
> Exynos5433 (it is currently using "samsung,exynos5250-dwusb3" one and
> is not using axius_clk).

I also think that regardless of what is decided on making susp_clk
non-optional for some Exynos SoCs we should probably remove the debug
message as it doesn't bring useful information and may be confusing.

Shuah, can you take care of this?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Krzysztof Kozlowski]

On Tue, Jan 10, 2017 at 06:09:40PM +0100, Bartlomiej Zolnierkiewicz wrote:
> BTW What is interesting is that the Exynos7 dts patch [2] has never
> made it into upstream for some reason.  In the meantime however
> Exynos5433 (similar to Exynos7 to some degree) became the user of
> susp_clk.
> 
> [1] https://lkml.org/lkml/2014/11/21/247
> [2] https://patchwork.kernel.org/patch/5355161/
>

+Cc Alim and Pankaj,

Anyone would like to resend [2] after rebasing and testing? Interrupt
flags would have to be fixed and status=disabled added.

Best regards,
Krzysztof

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Anand Moon]

Hi Bartlomiej,

On 10 January 2017 at 23:33, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
>
> Hi,
>
> On Tuesday, January 10, 2017 11:23:38 PM Anand Moon wrote:
>> Hi Shuah,
>>
>> On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>> > On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
>> >>
>> >> Hi,
>> >>
>> >> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
>> >>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
>> >>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>> >>>>>
>> >>>>> Hi,
>> >>>>>
>> >>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>> >>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>> >>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>> >>>>>> error path only when susp_clk has been set from remove and probe error
>> >>>>>> paths.
>> >>>>>
>> >>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>> >>>>> for NULL clock.  Also your patch changes susp_clk handling while
>> >>>>> leaves axius_clk handling (which also can be NULL) untouched.
>> >>>>>
>> >>>>> Do you actually see some runtime problem with the current code?
>> >>>>>
>> >>>>> If not then the patch should probably be dropped.
>> >>>>>
>> >>>>> Best regards,
>> >>>>> --
>> >>>>> Bartlomiej Zolnierkiewicz
>> >>>>> Samsung R&D Institute Poland
>> >>>>> Samsung Electronics
>> >>>>
>> >>>> Hi Bartlomiej,
>> >>>>
>> >>>> I am seeing the "no suspend clk specified" message in dmesg.
>> >>>> After that it sets the exynos->susp_clk = NULL and starts
>> >>>> calling clk_prepare_enable(exynos->susp_clk);
>> >>>>
>> >>>> That can't be good. If you see the logic right above this
>> >>>> one for exynos->clk, it returns error and fails the probe.
>> >>>> This this case it doesn't, but tries to use null susp_clk.
>> >>
>> >> exynos->susp_clk is optional, exynos->clk is not.
>> >
>> > Right. That is clear since we don't fail the probe.
>> >
>> >>
>> >>>> I believe this patch is necessary.
>> >>>
>> >>> Let me clarify this a bit further. Since we already know
>> >>> susp_clk is null, with this patch we can avoid extra calls
>> >>> to clk_prepare_enable() and clk_disable_unprepare().
>> >>>
>> >>> One can say, it also adds extra checks, hence I will let you
>> >>> decide one way or the other. :)
>> >>
>> >> I would prefer to leave the things as they are currently.
>> >>
>> >> The code in question is not performance sensitive so extra
>> >> calls are not a problem.  No extra checks means less code.
>> >>
>> >> Also the current code seems to be more in line with the rest
>> >> of the kernel.
>> >
>> > What functionality is missing without the suspend clock? Would
>> > it make sense to change the info. message to include what it
>> > means. At the moment it doesn't anything more than "no suspend
>> > clock" which is a very cryptic user visible message. It would be
>> > helpful for it to also include what functionality is impacted.
>> >
>>
>> Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
>
> Can you point me to the use of usbdrd30_axius_clk?
>
> I cannot find in the upstream code.
>
>> so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.
>
> This is not so simple and we would probably need a new compatible for
> Exynos5433 (it is currently using "samsung,exynos5250-dwusb3" one and
> is not using axius_clk).

Opps: sorry for the noise, my result was based on simple grep.

Best Regards
-Anand

>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>> Best Regards
>> -Anand
>>
>> > thanks,
>> > -- Shuah
>> >
>> >>
>> >> Best regards,
>> >> --
>> >> Bartlomiej Zolnierkiewicz
>> >> Samsung R&D Institute Poland
>> >> Samsung Electronics
>> >>
>> >>> thanks,
>> >>> -- Shuah
>> >>>
>> >>>>
>> >>>> thanks,
>> >>>> -- Shuah
>> >>>>
>> >>>>>
>> >>>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> >>>>>> ---
>> >>>>>>  drivers/usb/dwc3/dwc3-exynos.c | 10 ++++++----
>> >>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> >>>>>>
>> >>>>>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
>> >>>>>> index e27899b..f97a3d7 100644
>> >>>>>> --- a/drivers/usb/dwc3/dwc3-exynos.c
>> >>>>>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
>> >>>>>> @@ -131,8 +131,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>> >>>>>>   if (IS_ERR(exynos->susp_clk)) {
>> >>>>>>           dev_info(dev, "no suspend clk specified\n");
>> >>>>>>           exynos->susp_clk = NULL;
>> >>>>>> - }
>> >>>>>> - clk_prepare_enable(exynos->susp_clk);
>> >>>>>> + } else
>> >>>>>> +         clk_prepare_enable(exynos->susp_clk);
>> >>>>>>
>> >>>>>>   if (of_device_is_compatible(node, "samsung,exynos7-dwusb3")) {
>> >>>>>>           exynos->axius_clk = devm_clk_get(dev, "usbdrd30_axius_clk");
>> >>>>>> @@ -196,7 +196,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
>> >>>>>>   regulator_disable(exynos->vdd33);
>> >>>>>>  err2:
>> >>>>>>   clk_disable_unprepare(exynos->axius_clk);
>> >>>>>> - clk_disable_unprepare(exynos->susp_clk);
>> >>>>>> + if (exynos->susp_clk)
>> >>>>>> +         clk_disable_unprepare(exynos->susp_clk);
>> >>>>>>   clk_disable_unprepare(exynos->clk);
>> >>>>>>   return ret;
>> >>>>>>  }
>> >>>>>> @@ -210,7 +211,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
>> >>>>>>   platform_device_unregister(exynos->usb3_phy);
>> >>>>>>
>> >>>>>>   clk_disable_unprepare(exynos->axius_clk);
>> >>>>>> - clk_disable_unprepare(exynos->susp_clk);
>> >>>>>> + if (exynos->susp_clk)
>> >>>>>> +         clk_disable_unprepare(exynos->susp_clk);
>> >>>>>>   clk_disable_unprepare(exynos->clk);
>> >>>>>>
>> >>>>>>   regulator_disable(exynos->vdd33);
>

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Shuah Khan]

On 01/10/2017 11:23 AM, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday, January 10, 2017 07:03:57 PM Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Tuesday, January 10, 2017 11:23:38 PM Anand Moon wrote:
>>> Hi Shuah,
>>>
>>> On 10 January 2017 at 21:58, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>>>> On 01/10/2017 09:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Tuesday, January 10, 2017 07:36:35 AM Shuah Khan wrote:
>>>>>> On 01/10/2017 07:16 AM, Shuah Khan wrote:
>>>>>>> On 01/10/2017 05:05 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Monday, January 09, 2017 07:21:31 PM Shuah Khan wrote:
>>>>>>>>> Fix dwc3_exynos_probe() to call clk_prepare_enable() only when suspend
>>>>>>>>> clock is specified. Call clk_disable_unprepare() from remove and probe
>>>>>>>>> error path only when susp_clk has been set from remove and probe error
>>>>>>>>> paths.
>>>>>>>>
>>>>>>>> It is legal to call clk_prepare_enable() and clk_disable_unprepare()
>>>>>>>> for NULL clock.  Also your patch changes susp_clk handling while
>>>>>>>> leaves axius_clk handling (which also can be NULL) untouched.
>>>>>>>>
>>>>>>>> Do you actually see some runtime problem with the current code?
>>>>>>>>
>>>>>>>> If not then the patch should probably be dropped.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> --
>>>>>>>> Bartlomiej Zolnierkiewicz
>>>>>>>> Samsung R&D Institute Poland
>>>>>>>> Samsung Electronics
>>>>>>>
>>>>>>> Hi Bartlomiej,
>>>>>>>
>>>>>>> I am seeing the "no suspend clk specified" message in dmesg.
>>>>>>> After that it sets the exynos->susp_clk = NULL and starts
>>>>>>> calling clk_prepare_enable(exynos->susp_clk);
>>>>>>>
>>>>>>> That can't be good. If you see the logic right above this
>>>>>>> one for exynos->clk, it returns error and fails the probe.
>>>>>>> This this case it doesn't, but tries to use null susp_clk.
>>>>>
>>>>> exynos->susp_clk is optional, exynos->clk is not.
>>>>
>>>> Right. That is clear since we don't fail the probe.
>>>>
>>>>>
>>>>>>> I believe this patch is necessary.
>>>>>>
>>>>>> Let me clarify this a bit further. Since we already know
>>>>>> susp_clk is null, with this patch we can avoid extra calls
>>>>>> to clk_prepare_enable() and clk_disable_unprepare().
>>>>>>
>>>>>> One can say, it also adds extra checks, hence I will let you
>>>>>> decide one way or the other. :)
>>>>>
>>>>> I would prefer to leave the things as they are currently.
>>>>>
>>>>> The code in question is not performance sensitive so extra
>>>>> calls are not a problem.  No extra checks means less code.
>>>>>
>>>>> Also the current code seems to be more in line with the rest
>>>>> of the kernel.
>>>>
>>>> What functionality is missing without the suspend clock? Would
>>>> it make sense to change the info. message to include what it
>>>> means. At the moment it doesn't anything more than "no suspend
>>>> clock" which is a very cryptic user visible message. It would be
>>>> helpful for it to also include what functionality is impacted.
>>>>
>>>
>>> Both usbdrd30_susp_clk and usbdrd30_axius_clk are used by exynos5433 platform
>>
>> Can you point me to the use of usbdrd30_axius_clk?
>>
>> I cannot find in the upstream code.
>>
>>> so moving the clk under compatible string "samsung,exynos7-dwusb3" make sense.
>>
>> This is not so simple and we would probably need a new compatible for
>> Exynos5433 (it is currently using "samsung,exynos5250-dwusb3" one and
>> is not using axius_clk).
> 
> I also think that regardless of what is decided on making susp_clk
> non-optional for some Exynos SoCs we should probably remove the debug
> message as it doesn't bring useful information and may be confusing.
> 
> Shuah, can you take care of this?

Yes. This message as it reads now is not only confusing, but also can
lead users to think something is wrong.

I can get rid of it or I could change it from info to debug and change
it to read:

"Optional Suspend clock isn't found. Diver operation isn't impacted"

thanks,
-- Shuah

> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Krzysztof Kozlowski]

On Tue, Jan 10, 2017 at 11:37:24AM -0700, Shuah Khan wrote:
> On 01/10/2017 11:23 AM, Bartlomiej Zolnierkiewicz wrote:
> > I also think that regardless of what is decided on making susp_clk
> > non-optional for some Exynos SoCs we should probably remove the debug
> > message as it doesn't bring useful information and may be confusing.
> > 
> > Shuah, can you take care of this?
> 
> Yes. This message as it reads now is not only confusing, but also can
> lead users to think something is wrong.
> 
> I can get rid of it or I could change it from info to debug and change
> it to read:
> 
> "Optional Suspend clock isn't found. Diver operation isn't impacted"

It is even more confusing. If the clock is required (by binding, by
hardware) - make it an error. If it is completely not important - do not
print anything. If it is optional but helpful (enabling clock gives
someything) then print something... but it is not that case.

Best regards,
Krzysztof

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[Shuah Khan]

On 01/10/2017 11:59 AM, Krzysztof Kozlowski wrote:
> On Tue, Jan 10, 2017 at 11:37:24AM -0700, Shuah Khan wrote:
>> On 01/10/2017 11:23 AM, Bartlomiej Zolnierkiewicz wrote:
>>> I also think that regardless of what is decided on making susp_clk
>>> non-optional for some Exynos SoCs we should probably remove the debug
>>> message as it doesn't bring useful information and may be confusing.
>>>
>>> Shuah, can you take care of this?
>>
>> Yes. This message as it reads now is not only confusing, but also can
>> lead users to think something is wrong.
>>
>> I can get rid of it or I could change it from info to debug and change
>> it to read:
>>
>> "Optional Suspend clock isn't found. Diver operation isn't impacted"
> 
> It is even more confusing. If the clock is required (by binding, by
> hardware) - make it an error. If it is completely not important - do not
> print anything. If it is optional but helpful (enabling clock gives
> someything) then print something... but it is not that case.
> 
> Best regards,
> Krzysztof
> 

Sounds fair. I will send a patch to remove the message.

-- Shuah

Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling

[pankaj.dubey]

Hi Krzysztof,

On Tuesday 10 January 2017 11:55 PM, Krzysztof Kozlowski wrote:
> On Tue, Jan 10, 2017 at 06:09:40PM +0100, Bartlomiej Zolnierkiewicz wrote:
>> BTW What is interesting is that the Exynos7 dts patch [2] has never
>> made it into upstream for some reason.  In the meantime however
>> Exynos5433 (similar to Exynos7 to some degree) became the user of
>> susp_clk.
>>
>> [1] https://lkml.org/lkml/2014/11/21/247
>> [2] https://patchwork.kernel.org/patch/5355161/
>>
> 
> +Cc Alim and Pankaj,
> 
> Anyone would like to resend [2] after rebasing and testing? Interrupt
> flags would have to be fixed and status=disabled added.
> 

Thanks for looping us into this thread.

Well I will be taking care of Exynos USB as Vivek has left Samsung.
I am currently working on adding Exynos7 USB support, as soon as patches
are ready for posting will post them. Will take care of review comments
given for Vivek's patchset [1].

Thanks,
Pankaj Dubey

> Best regards,
> Krzysztof
> 
>