From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S940565AbdAJSZa (ORCPT ); Tue, 10 Jan 2017 13:25:30 -0500 Received: from mailout4.samsung.com ([203.254.224.34]:40116 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762956AbdAJSXt (ORCPT ); Tue, 10 Jan 2017 13:23:49 -0500 X-AuditID: b6c32a3d-f793f6d000001fd5-be-587526b14cce From: Bartlomiej Zolnierkiewicz To: Shuah Khan Cc: Anand Moon , linux-arm-kernel , Felipe Balbi , Greg Kroah-Hartman , Kukjin Kim , Krzysztof Kozlowski , Javier Martinez Canillas , "linux-samsung-soc@vger.kernel.org" , Linux USB Mailing List , Linux Kernel Subject: Re: [PATCH] usb: dwc3-exynos fix unspecified suspend clk error handling Date: Tue, 10 Jan 2017 19:23:43 +0100 Message-id: <1659345.4CcVdVckN5@amdc3058> User-Agent: KMail/4.13.3 (Linux/3.13.0-96-generic; KDE/4.13.3; x86_64; ; ) In-reply-to: <2265318.ybtxfiNn3d@amdc3058> MIME-version: 1.0 Content-transfer-encoding: 7Bit Content-type: text/plain; charset=us-ascii X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrFKsWRmVeSWpSXmKPExsWy7bCmru5GtdIIgyUXbCyOtT1ht2hevJ7N 4s3bNUwW/Y9fM1ucP7+B3WLT42usFpd3zWGzmHF+H5PFomWtzBbrNt5it5j65QOLA7fHzll3 2T02repk89g/dw27x+Yl9R5b+oFCnzfJBbBFcdmkpOZklqUW6dslcGXc+vSJueCMXMXD6yIN jL/Euxg5OSQETCTuz/3BBGGLSVy4t56ti5GLQ0hgB6PEwg3/WSGcdiaJC49Xs8B0nG/7zgyR mMMocfbQOUYI5yujROPbBrAqNgEriYntqxhBbBEBDYknsyaBzWUWeMAssW/iNbAiYYEgiSev z4AtZxFQlXg9dQMziM0roClxs2cqG4gtKuAlsWVfO1gNp4CWxI6zvewQNYISPybfA5vDLCAv sW//VFYIW0fi7LF1YBdJCJxil+ifeAhoKAeQIyux6QCU6SIxv80M4hthiVfHt7BD2NISq/7d gobFdEaJ7b8lIMZsZpRYtXsCVJG1xOHjF6F28Um8+9rDCjGTV6KjTQjC9JBY9KYAotpRonFh IxMkfL4zSpxaeJp9AqP8LCQfzELywSwkHyxgZF7FKJZaUJybnlpsWGCpV5yYW1yal66XnJ+7 iRGcgrRsdzB+OedziFGAg1GJh9fidUmEEGtiWXFl7iFGCQ5mJRHeXyqlEUK8KYmVValF+fFF pTmpxYcYpTlYlMR5lzVaRwgJpCeWpGanphakFsFkmTg4pRoY+6eee1pg+vGANoeK7pEP/3ae bMkQif+rxLDGmDv1o2bKNb/OJ+82pV1eVMneJHbpXZxkUN2ajt2TSz1ju9u5LoQ6S29ueBA9 ddafq7uMdu6oDvr1QstoXfVaY7mzCq+tu1beKhda6Bxe6/19bx7DKwYR9V9/nj24VN+efPNG seu9Sxv4pQzOK7EUZyQaajEXFScCAKzWKsE9AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrLIsWRmVeSWpSXmKPExsVy+t9jQd2NaqURBjd/aFgca3vCbtG8eD2b xZu3a5gs+h+/ZrY4f34Du8Wmx9dYLS7vmsNmMeP8PiaLRctamS3WbbzFbjH1ywcWB26PnbPu sntsWtXJ5rF/7hp2j81L6j229AOFPm+SC2CLcrPJSE1MSS1SSM1Lzk/JzEu3VQoNcdO1UFLI S8xNtVWK0PUNCVJSKEvMKQXyjAzQgINzgHuwkr5dglvGrU+fmAvOyFU8vC7SwPhLvIuRk0NC wETifNt3ZghbTOLCvfVsXYxcHEICsxgl7nybzgaSEBL4yigx85sMiM0mYCUxsX0VI4gtIqAh 8WTWJLAGZoEnzBJXVv1iAUkICwRIvLjZCGazCKhKvJ66AWwDr4CmxM2eqWBDRQW8JLbsa2cC sTkFtCR2nO1lh9i8mFHi3ryNrBANghI/Jt8DG8QsIC+xb/9UVghbS2L9zuNMExiB7kQom4Wk bBaSsgWMzKsYJVILkguKk9JzDfNSy/WKE3OLS/PS9ZLzczcxgqPymdQOxoO73A8xCnAwKvHw PnhREiHEmlhWXJl7iFGCg1lJhPeXSmmEEG9KYmVValF+fFFpTmrxIUZToA8nMkuJJucDE0Ze SbyhibmJubGBhbmlpYmRkjhv4+xn4UIC6YklqdmpqQWpRTB9TBycUg2Mk23VplRJiH190W3I XWBQcE0ya2nWzrOKhnekrz97EbdzoXDVSaOrL9iye44eaj3pYrHknM7SfpNfsw8uzzp233SF 8pZ40yeVb2SzbOY6BJzl7PilwhBo94rPKOZvT2yVzzSOS86z0rNSnq+7yTZpgaQgz4ncYKub fR7R2db+C/rKhBxkMsKUWIozEg21mIuKEwEy/jc74AIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170110182345epcas1p3112f94aad300625ff1f75860c55a2b7e 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: 20170110182345epcas1p3112f94aad300625ff1f75860c55a2b7e X-RootMTR: 20170110182345epcas1p3112f94aad300625ff1f75860c55a2b7e References: <20170110022131.31042-1-shuahkh@osg.samsung.com> <2265318.ybtxfiNn3d@amdc3058> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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