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 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
