From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965688AbdAJQFj (ORCPT ); Tue, 10 Jan 2017 11:05:39 -0500 Received: from mailout1.samsung.com ([203.254.224.24]:38611 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935492AbdAJQFg (ORCPT ); Tue, 10 Jan 2017 11:05:36 -0500 X-AuditID: b6c32a35-f79956d000002021-72-5875064c7531 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 Subject: Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling Date: Tue, 10 Jan 2017 17:05:30 +0100 Message-id: <8784459.rxWqZlDLVg@amdc3058> User-Agent: KMail/4.13.3 (Linux/3.13.0-96-generic; KDE/4.13.3; x86_64; ; ) In-reply-to: <3a113696-d0cd-8944-4acd-c87646fbee15@osg.samsung.com> MIME-version: 1.0 Content-transfer-encoding: 7Bit Content-type: text/plain; charset=us-ascii X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrMKsWRmVeSWpSXmKPExsWy7bCmvq4PW2mEwcmDhhbH2p6wWzQvXs9m 8ebtGiaL/sevmS3On9/AbrHp8TVWi8u75rBZzDi/j8li0bJWZoupXz6wOHB5bFrVyeaxf+4a do/NS+o9tvTfZff4vEkugDWKyyYlNSezLLVI3y6BK6Pz4g22gmdyFXv6j7A0MH6W6GLk5JAQ MJH4uvkLO4QtJnHh3nq2LkYuDiGBHYwSR08/YIdw2pkkJjT1McF0LNp3kwkisZxRYvmx1VBV Xxkluk7OZgSpYhOwkpjYvgrMFhHQkHgyaxLYXGaBz4wS/46/ZAFJCAsESTx5fQZsLIuAqsSU 6UfBDuEV0JTY1PCNGcQWFfCS2LKvHayGU8BZomf1J1aIGkGJH5Pvgc1hFpCX2Ld/KiuErSNx 9tg6RpBlEgL72CUW/d0AlOAAcmQlNh1ghnjBReL6324WCFtY4tXxLdAAkJZY9e8W1JvTGSW2 /5aAmLOZUWLV7glQRdYSh49fhFrGJ/Huaw/UfF6JjjYhiBIPiYlHr0LNd5T4dmUrNFCbmCQW vd7HMoFRfhaSH2Yh+WEWkh8WMDKvYhRLLSjOTU8tNiww1CtOzC0uzUvXS87P3cQITjpapjsY p5zzOcQowMGoxMNr8bokQog1say4MvcQowQHs5IILx9LaYQQb0piZVVqUX58UWlOavEhRmkO FiVx3sWN1hFCAumJJanZqakFqUUwWSYOTqkGxuAWZzVW17ryv2uDXJiP63zbnqd9ytFl95E5 4a55f03vBvj9+nL2VZRUk7tg6V+Fz1+0r82JOeDlffHSvbYd11Z5VPqJltUsN7WrLF/Q3uhq sWntjII/rP9T1aew+F3V/utZ/GFD9Mboc9f4WqT+Gy9f6TAtued419U1HnnTKgSdH3E2y1+a rsRSnJFoqMVcVJwIABlvHsY2AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrKIsWRmVeSWpSXmKPExsVy+t9jAV0fttIIg0frFCyOtT1ht2hevJ7N 4s3bNUwW/Y9fM1ucP7+B3WLT42usFpd3zWGzmHF+H5PFomWtzBZTv3xgceDy2LSqk81j/9w1 7B6bl9R7bOm/y+7xeZNcAGuUm01GamJKapFCal5yfkpmXrqtUmiIm66FkkJeYm6qrVKErm9I kJJCWWJOKZBnZIAGHJwD3IOV9O0S3DI6L95gK3gmV7Gn/whLA+NniS5GTg4JAROJRftuMkHY YhIX7q1n62Lk4hASWMoosf7yBxYI5yujxLbJ9xlBqtgErCQmtq8Cs0UENCSezJoE1sEs8JlR 4tCin2wgCWGBAIkXNxtZQGwWAVWJKdOPsoPYvAKaEpsavjGD2KICXhJb9rWDreYUcJboWf2J FWLbOkaJHR92MEI0CEr8mHwPbBCzgLzEvv1TWSFsLYn1O48zTWAUmIWkbBaSsllIyhYwMq9i lEgtSC4oTkrPNcxLLdcrTswtLs1L10vOz93ECI7CZ1I7GA/ucj/EKMDBqMTD++BFSYQQa2JZ cWXuIUYJDmYlEV4+ltIIId6UxMqq1KL8+KLSnNTiQ4ymQB9OZJYSTc4HJoi8knhDE3MTc2MD C3NLSxMjJXHextnPwoUE0hNLUrNTUwtSi2D6mDg4pRoYE21TL2w0Z7TOd/qq3pxpw7G7aG9X nM+xUyX3tnXpSP31EeiaK19qHvtso+xMzqjkf9GhrT82GK24+VnOKL9Pf2r82zTWE3EioXsz b/uFL/6r0l/GvdOx6gnnhM175Kd/VJrsLuvRXhh70zhdTkQyPMKlbS/rJEbj9u12CpnFPRHO Cl+vMSuxFGckGmoxFxUnAgD1bDt42AIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170110160532epcas1p198cef8317dcbc8740366d06b38e7465f 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: 20170110160532epcas1p198cef8317dcbc8740366d06b38e7465f X-RootMTR: 20170110160532epcas1p198cef8317dcbc8740366d06b38e7465f References: <20170110022131.31042-1-shuahkh@osg.samsung.com> <2e4a5e53-6603-13b8-0302-dc8093978013@osg.samsung.com> <3a113696-d0cd-8944-4acd-c87646fbee15@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 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 > >>> --- > >>> 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);