* [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling @ 2017-01-10 2:21 Shuah Khan 2017-01-10 11:20 ` Sergei Shtylyov [not found] ` <CGME20170110120605epcas1p1c26f7f90fca4765ab9cc339c51d11347@epcas1p1.samsung.com> 0 siblings, 2 replies; 19+ messages in thread From: Shuah Khan @ 2017-01-10 2:21 UTC (permalink / raw) To: balbi, gregkh, kgene, krzk, javier Cc: Shuah Khan, linux-usb, linux-arm-kernel, linux-samsung-soc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 2:21 [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling Shuah Khan @ 2017-01-10 11:20 ` Sergei Shtylyov 2017-01-10 14:38 ` Shuah Khan [not found] ` <CGME20170110120605epcas1p1c26f7f90fca4765ab9cc339c51d11347@epcas1p1.samsung.com> 1 sibling, 1 reply; 19+ messages in thread From: Sergei Shtylyov @ 2017-01-10 11:20 UTC (permalink / raw) To: Shuah Khan, balbi, gregkh, kgene, krzk, javier Cc: linux-usb, linux-arm-kernel, linux-samsung-soc, linux-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 11:20 ` Sergei Shtylyov @ 2017-01-10 14:38 ` Shuah Khan 0 siblings, 0 replies; 19+ messages in thread From: Shuah Khan @ 2017-01-10 14:38 UTC (permalink / raw) To: Sergei Shtylyov, balbi, gregkh, kgene, krzk, javier Cc: linux-usb, linux-arm-kernel, linux-samsung-soc, linux-kernel, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CGME20170110120605epcas1p1c26f7f90fca4765ab9cc339c51d11347@epcas1p1.samsung.com>]
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling [not found] ` <CGME20170110120605epcas1p1c26f7f90fca4765ab9cc339c51d11347@epcas1p1.samsung.com> @ 2017-01-10 12:05 ` Bartlomiej Zolnierkiewicz 2017-01-10 14:16 ` Shuah Khan 0 siblings, 1 reply; 19+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2017-01-10 12:05 UTC (permalink / raw) To: linux-arm-kernel Cc: Shuah Khan, balbi, gregkh, kgene, krzk, javier, linux-samsung-soc, linux-usb, linux-kernel 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); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 12:05 ` Bartlomiej Zolnierkiewicz @ 2017-01-10 14:16 ` Shuah Khan 2017-01-10 14:36 ` Shuah Khan 0 siblings, 1 reply; 19+ messages in thread From: Shuah Khan @ 2017-01-10 14:16 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, linux-arm-kernel Cc: balbi, gregkh, kgene, krzk, javier, linux-samsung-soc, linux-usb, linux-kernel, 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); > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 14:16 ` Shuah Khan @ 2017-01-10 14:36 ` Shuah Khan [not found] ` <CGME20170110160532epcas1p198cef8317dcbc8740366d06b38e7465f@epcas1p1.samsung.com> 0 siblings, 1 reply; 19+ messages in thread From: Shuah Khan @ 2017-01-10 14:36 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, linux-arm-kernel Cc: balbi, gregkh, kgene, krzk, javier, linux-samsung-soc, linux-usb, linux-kernel, 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); >> > ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CGME20170110160532epcas1p198cef8317dcbc8740366d06b38e7465f@epcas1p1.samsung.com>]
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling [not found] ` <CGME20170110160532epcas1p198cef8317dcbc8740366d06b38e7465f@epcas1p1.samsung.com> @ 2017-01-10 16:05 ` Bartlomiej Zolnierkiewicz 2017-01-10 16:28 ` Shuah Khan 0 siblings, 1 reply; 19+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2017-01-10 16:05 UTC (permalink / raw) To: Shuah Khan Cc: linux-arm-kernel, balbi, gregkh, kgene, krzk, javier, linux-samsung-soc, linux-usb, linux-kernel 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); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 16:05 ` Bartlomiej Zolnierkiewicz @ 2017-01-10 16:28 ` Shuah Khan [not found] ` <CGME20170110170942epcas1p3decee28366d7d42b70e7b0669b97326f@epcas1p3.samsung.com> 2017-01-10 17:53 ` Anand Moon 0 siblings, 2 replies; 19+ messages in thread From: Shuah Khan @ 2017-01-10 16:28 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: linux-arm-kernel, balbi, gregkh, kgene, krzk, javier, linux-samsung-soc, linux-usb, linux-kernel, 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); > ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CGME20170110170942epcas1p3decee28366d7d42b70e7b0669b97326f@epcas1p3.samsung.com>]
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling [not found] ` <CGME20170110170942epcas1p3decee28366d7d42b70e7b0669b97326f@epcas1p3.samsung.com> @ 2017-01-10 17:09 ` Bartlomiej Zolnierkiewicz 2017-01-10 17:49 ` vivek.gautam 2017-01-10 18:25 ` Krzysztof Kozlowski 0 siblings, 2 replies; 19+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2017-01-10 17:09 UTC (permalink / raw) To: Shuah Khan Cc: linux-arm-kernel, balbi, gregkh, kgene, krzk, javier, linux-samsung-soc, linux-usb, linux-kernel, Vivek Gautam 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); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 17:09 ` Bartlomiej Zolnierkiewicz @ 2017-01-10 17:49 ` vivek.gautam 2017-01-10 18:25 ` Krzysztof Kozlowski 1 sibling, 0 replies; 19+ messages in thread From: vivek.gautam @ 2017-01-10 17:49 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Shuah Khan Cc: linux-arm-kernel, balbi, gregkh, kgene, krzk, javier, linux-samsung-soc, linux-usb, linux-kernel, pankaj.dubey 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 17:09 ` Bartlomiej Zolnierkiewicz 2017-01-10 17:49 ` vivek.gautam @ 2017-01-10 18:25 ` Krzysztof Kozlowski 2017-01-11 2:43 ` pankaj.dubey 1 sibling, 1 reply; 19+ messages in thread From: Krzysztof Kozlowski @ 2017-01-10 18:25 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Shuah Khan, linux-arm-kernel, balbi, gregkh, kgene, krzk, javier, linux-samsung-soc, linux-usb, linux-kernel, Vivek Gautam, Pankaj Dubey, Alim Akhtar 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 18:25 ` Krzysztof Kozlowski @ 2017-01-11 2:43 ` pankaj.dubey 0 siblings, 0 replies; 19+ messages in thread From: pankaj.dubey @ 2017-01-11 2:43 UTC (permalink / raw) To: Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz Cc: Shuah Khan, linux-arm-kernel, balbi, gregkh, kgene, javier, linux-samsung-soc, linux-usb, linux-kernel, Vivek Gautam, Alim Akhtar 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 > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 16:28 ` Shuah Khan [not found] ` <CGME20170110170942epcas1p3decee28366d7d42b70e7b0669b97326f@epcas1p3.samsung.com> @ 2017-01-10 17:53 ` Anand Moon [not found] ` <CGME20170110180359epcas1p4729103fd575fe628bfd3ea7966ec4856@epcas1p4.samsung.com> 1 sibling, 1 reply; 19+ messages in thread From: Anand Moon @ 2017-01-10 17:53 UTC (permalink / raw) To: Shuah Khan Cc: Bartlomiej Zolnierkiewicz, linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc, Linux USB Mailing List, Linux Kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CGME20170110180359epcas1p4729103fd575fe628bfd3ea7966ec4856@epcas1p4.samsung.com>]
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling [not found] ` <CGME20170110180359epcas1p4729103fd575fe628bfd3ea7966ec4856@epcas1p4.samsung.com> @ 2017-01-10 18:03 ` Bartlomiej Zolnierkiewicz [not found] ` <CGME20170110182345epcas1p3112f94aad300625ff1f75860c55a2b7e@epcas1p3.samsung.com> 2017-01-10 18:36 ` Anand Moon 0 siblings, 2 replies; 19+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2017-01-10 18:03 UTC (permalink / raw) To: Anand Moon Cc: Shuah Khan, linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc, Linux USB Mailing List, Linux Kernel 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); ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <CGME20170110182345epcas1p3112f94aad300625ff1f75860c55a2b7e@epcas1p3.samsung.com>]
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling [not found] ` <CGME20170110182345epcas1p3112f94aad300625ff1f75860c55a2b7e@epcas1p3.samsung.com> @ 2017-01-10 18:23 ` Bartlomiej Zolnierkiewicz 2017-01-10 18:37 ` Shuah Khan 0 siblings, 1 reply; 19+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2017-01-10 18:23 UTC (permalink / raw) To: Shuah Khan Cc: Anand Moon, linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc, Linux USB Mailing List, Linux Kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 18:23 ` Bartlomiej Zolnierkiewicz @ 2017-01-10 18:37 ` Shuah Khan 2017-01-10 18:59 ` Krzysztof Kozlowski 0 siblings, 1 reply; 19+ messages in thread From: Shuah Khan @ 2017-01-10 18:37 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Anand Moon, linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc, Linux USB Mailing List, Linux Kernel, 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 18:37 ` Shuah Khan @ 2017-01-10 18:59 ` Krzysztof Kozlowski 2017-01-10 19:20 ` Shuah Khan 0 siblings, 1 reply; 19+ messages in thread From: Krzysztof Kozlowski @ 2017-01-10 18:59 UTC (permalink / raw) To: Shuah Khan Cc: Bartlomiej Zolnierkiewicz, Anand Moon, linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc, Linux USB Mailing List, Linux Kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 18:59 ` Krzysztof Kozlowski @ 2017-01-10 19:20 ` Shuah Khan 0 siblings, 0 replies; 19+ messages in thread From: Shuah Khan @ 2017-01-10 19:20 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Bartlomiej Zolnierkiewicz, Anand Moon, linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, Kukjin Kim, Javier Martinez Canillas, linux-samsung-soc, Linux USB Mailing List, Linux Kernel, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling 2017-01-10 18:03 ` Bartlomiej Zolnierkiewicz [not found] ` <CGME20170110182345epcas1p3112f94aad300625ff1f75860c55a2b7e@epcas1p3.samsung.com> @ 2017-01-10 18:36 ` Anand Moon 1 sibling, 0 replies; 19+ messages in thread From: Anand Moon @ 2017-01-10 18:36 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Shuah Khan, linux-arm-kernel, Felipe Balbi, Greg Kroah-Hartman, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc, Linux USB Mailing List, Linux Kernel 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); > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-01-11 2:40 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-10 2:21 [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling Shuah Khan 2017-01-10 11:20 ` Sergei Shtylyov 2017-01-10 14:38 ` Shuah Khan [not found] ` <CGME20170110120605epcas1p1c26f7f90fca4765ab9cc339c51d11347@epcas1p1.samsung.com> 2017-01-10 12:05 ` Bartlomiej Zolnierkiewicz 2017-01-10 14:16 ` Shuah Khan 2017-01-10 14:36 ` Shuah Khan [not found] ` <CGME20170110160532epcas1p198cef8317dcbc8740366d06b38e7465f@epcas1p1.samsung.com> 2017-01-10 16:05 ` Bartlomiej Zolnierkiewicz 2017-01-10 16:28 ` Shuah Khan [not found] ` <CGME20170110170942epcas1p3decee28366d7d42b70e7b0669b97326f@epcas1p3.samsung.com> 2017-01-10 17:09 ` Bartlomiej Zolnierkiewicz 2017-01-10 17:49 ` vivek.gautam 2017-01-10 18:25 ` Krzysztof Kozlowski 2017-01-11 2:43 ` pankaj.dubey 2017-01-10 17:53 ` Anand Moon [not found] ` <CGME20170110180359epcas1p4729103fd575fe628bfd3ea7966ec4856@epcas1p4.samsung.com> 2017-01-10 18:03 ` Bartlomiej Zolnierkiewicz [not found] ` <CGME20170110182345epcas1p3112f94aad300625ff1f75860c55a2b7e@epcas1p3.samsung.com> 2017-01-10 18:23 ` Bartlomiej Zolnierkiewicz 2017-01-10 18:37 ` Shuah Khan 2017-01-10 18:59 ` Krzysztof Kozlowski 2017-01-10 19:20 ` Shuah Khan 2017-01-10 18:36 ` Anand Moon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).