| Message ID | 20170110022131.31042-1-shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);
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); >
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); >> >
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
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);
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); >
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);
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
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
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);
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
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
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); >
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 >
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
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
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 > >
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);
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(-)