From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S940423AbdAJRJv (ORCPT ); Tue, 10 Jan 2017 12:09:51 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:47966 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S938997AbdAJRJq (ORCPT ); Tue, 10 Jan 2017 12:09:46 -0500 X-AuditID: b6c32a3c-f79826d000002380-35-587515562488 From: Bartlomiej Zolnierkiewicz To: Shuah Khan Cc: linux-arm-kernel@lists.infradead.org, balbi@kernel.org, gregkh@linuxfoundation.org, kgene@kernel.org, krzk@kernel.org, javier@osg.samsung.com, linux-samsung-soc@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Vivek Gautam Subject: Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling Date: Tue, 10 Jan 2017 18:09:40 +0100 Message-id: <5156028.4V9iLD3Qkj@amdc3058> User-Agent: KMail/4.13.3 (Linux/3.13.0-96-generic; KDE/4.13.3; x86_64; ; ) In-reply-to: <28815306-e220-4847-99ce-b058baae0d07@osg.samsung.com> MIME-version: 1.0 Content-transfer-encoding: 7Bit Content-type: text/plain; charset=us-ascii X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrBKsWRmVeSWpSXmKPExsWy7bCmrm6YaGmEwZENJhbH2p6wWzQvXs9m 8ebtGiaL/sevmS3On9/AbrHp8TVWi8u75rBZzDi/j8li0bJWZoupXz6wWJz4uYPZgdvjcl8v k8emVZ1sHvvnrmH32Lyk3mNL/112j8+b5ALYorhsUlJzMstSi/TtErgylhwrLDikX7FtcnwD 41K1LkYODgkBE4mXN7m7GDmBTDGJC/fWs3UxcnEICexglHh87BUThNPOJLHq81cWiCoTibd/ ljBCJOYwSpz40cQO4XxllJj/6BEbSBWbgJXExPZVjCC2iICGxJNZk8DmMgssZJI4d/oC2Chh gSCJJ6/PMIHYLAKqEo3H1rKD2LwCmhI7P6wGqxEV8JLYsq8drIZTwFli/aatzBA1ghI/Jt8D q2EWkJfYt38qK4StI3H22DpGiFOPsUvsesYP8aesxKYDzBCmi8SntYoQFcISr45vYYewpSVW /bvFBGFPZ5TY/lsC5GQJgc2MEqt2T4AqspY4fPwi1Co+iXdfe1ghZvJKdLQJQZR4SDS9WwF1 gaPE5NszWSDh85JRYuGzCWwTGOVnIflgFpIPZiH5YAEj8ypGsdSC4tz01GLDAgu94sTc4tK8 dL3k/NxNjOD0o2Wzg/HSOZ9DjAIcjEo8vBPelkQIsSaWFVfmHmKU4GBWEuHVFCqNEOJNSays Si3Kjy8qzUktPsQozcGiJM67rNE6QkggPbEkNTs1tSC1CCbLxMEp1cCoJ52uw2V+m2+Kx+3T F0KNU/9/v3331r9z3Ovnz9t+aJr3o8h9HaYzrn86eE5SQyg0LLhLq9J+MWsq6+vYUL4nFaJ/ PcIUSq5zrF2/U7Ah4NyNzPkXUz9a93alHBH+IRvguEuhrlOO8138gn/8Tzcy5JaV9FT1Jr2y m3G5itWu3nxu5rvZ6vJKLMUZiYZazEXFiQAOrHTaOwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrDIsWRmVeSWpSXmKPExsVy+t9jQd0w0dIIg4sT5CyOtT1ht2hevJ7N 4s3bNUwW/Y9fM1ucP7+B3WLT42usFpd3zWGzmHF+H5PFomWtzBZTv3xgsTjxcwezA7fH5b5e Jo9NqzrZPPbPXcPusXlJvceW/rvsHp83yQWwRbnZZKQmpqQWKaTmJeenZOal2yqFhrjpWigp 5CXmptoqRej6hgQpKZQl5pQCeUYGaMDBOcA9WEnfLsEtY8mxwoJD+hXbJsc3MC5V62Lk5JAQ MJF4+2cJI4QtJnHh3no2EFtIYBajxORHhl2MXED2V0aJHTPngSXYBKwkJravAmsQEdCQeDJr EhtIEbPAQiaJ6fNegxUJCwRIvLjZyAJiswioSjQeW8sOYvMKaErs/LAaLC4q4CWxZV87E4jN KeAssX7TVmaIbVMZJW7s38IC0SAo8WPyPTCbWUBeYt/+qawQtpbE+p3HmSYwAt2JUDYLSdks JGULGJlXMUqkFiQXFCel5xrmpZbrFSfmFpfmpesl5+duYgTH5DOpHYwHd7kfYhTgYFTi4X3w oiRCiDWxrLgy9xCjBAezkgivplBphBBvSmJlVWpRfnxRaU5q8SFGU6APJzJLiSbnA9NFXkm8 oYm5ibmxgYW5paWJkZI4b+PsZ+FCAumJJanZqakFqUUwfUwcnFINjLrB89g+vVPJOXposaXC bYaMtr+T+TM7w8x35HM/XCITlzFr68bleS8vL/p5a4PiG48Wt6p404+Kv29ItdxZJWms47sk +rOZumugkGPhvPtflkzkfuUqdXzOrbYHHIJccscOBFuEtga/urpBIMhZgVP+rniF7p34W/Zp K5weZh4Uvi2qc0+lXomlOCPRUIu5qDgRAM3tGa/fAgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170110170942epcas1p3decee28366d7d42b70e7b0669b97326f X-Msg-Generator: CA X-Sender-IP: 203.254.230.26 X-Local-Sender: =?UTF-8?B?QmFydGxvbWllaiBab2xuaWVya2lld2ljehtTUlBPTC1LZXJu?= =?UTF-8?B?ZWwgKFRQKRvsgrzshLHsoITsnpAbU2VuaW9yIFNvZnR3YXJlIEVuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?QmFydGxvbWllaiBab2xuaWVya2lld2ljehtTUlBPTC1LZXJu?= =?UTF-8?B?ZWwgKFRQKRtTYW1zdW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBF?= =?UTF-8?B?bmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 101P X-HopCount: 7 X-CMS-RootMailID: 20170110170942epcas1p3decee28366d7d42b70e7b0669b97326f X-RootMTR: 20170110170942epcas1p3decee28366d7d42b70e7b0669b97326f References: <20170110022131.31042-1-shuahkh@osg.samsung.com> <8784459.rxWqZlDLVg@amdc3058> <28815306-e220-4847-99ce-b058baae0d07@osg.samsung.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 Signed-off-by: Vivek Gautam Signed-off-by: Felipe Balbi 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 > >>>>> --- > >>>>> 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);