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@xxxxxxxxxxxxxxx> 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
>