From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935432AbdAJSEG (ORCPT ); Tue, 10 Jan 2017 13:04:06 -0500 Received: from mailout3.samsung.com ([203.254.224.33]:57599 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753978AbdAJSED (ORCPT ); Tue, 10 Jan 2017 13:04:03 -0500 X-AuditID: b6c32a36-f79d86d000003cda-1e-5875220f0bbc From: Bartlomiej Zolnierkiewicz To: Anand Moon Cc: Shuah Khan , 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:03:57 +0100 Message-id: <2265318.ybtxfiNn3d@amdc3058> User-Agent: KMail/4.13.3 (Linux/3.13.0-96-generic; KDE/4.13.3; x86_64; ; ) In-reply-to: MIME-version: 1.0 Content-transfer-encoding: 7Bit Content-type: text/plain; charset=us-ascii X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrNKsWRmVeSWpSXmKPExsWy7bCmri6/UmmEwcFWQ4tjbU/YLZoXr2ez ePN2DZNF/+PXzBbnz29gt9j0+BqrxeVdc9gsZpzfx2SxaFkrs8W6jbfYLaZ++cDiwO2xc9Zd do9NqzrZPPbPXcPusXlJvceWfqDQ501yAWxRXDYpqTmZZalF+nYJXBmP311lKriiXbFv+lHm BsYJyl2MnBwSAiYSnVtuskLYYhIX7q1n62Lk4hAS2MEoseDeEUYIp51JYt+ZAywwHc93zQPr EBJYzigxY4ENhP2VUWLejmoQm03ASmJi+ypGEFtEQE3iytMVrCCDmAUeMUt87/oGlhAWCJJ4 8voME4jNIqAqsfDdcrChvAKaEs/bjrGD2KICXhJb9rWD1XAKBEts7z3JDFEjKPFj8j2wg5gF 5CX27Z/KCmHrSJw9tg7sagmBU+wS01o/Av3DAeTISmw6wAzxgIvE4xezoJ4Rlnh1fAs7hC0t serfLSYIezqjxPbfEhBzNjNKrNo9AarIWuLw8YtQy/gk3n3tYYWYzyvR0SYEUeIhcf/OS6hd jhIHd86EhuIEJon3z3+xTmCUn4Xkh1lIfpiF5IcFjMyrGMVSC4pz01OLDQuM9IoTc4tL89L1 kvNzNzGCk5CW2Q7GRed8DjEKcDAq8fBavC6JEGJNLCuuzD3EKMHBrCTC66JQGiHEm5JYWZVa lB9fVJqTWnyIUZqDRUmcd3GjdYSQQHpiSWp2ampBahFMlomDU6qBMXYP61PHVHVB47O+AXdS N3lGT38kUXKJZ3ba9ez7LLp2go6MW05Pm5CqnFS/8H33YcWp0pUnVovI3rc/uMa9njUvfgfn 3Rbpzb+fifx5G3f4jfqsssIfrpa5Weni7vt++ApU8k19eGZXAu/vcBPhtq6zH4I9G4Ne/hd/ 171t1z0+ZfPJFU48SizFGYmGWsxFxYkAbZ1s+T4DAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrPIsWRmVeSWpSXmKPExsVy+t9jAV1+pdIIgxWXOCyOtT1ht2hevJ7N 4s3bNUwW/Y9fM1ucP7+B3WLT42usFpd3zWGzmHF+H5PFomWtzBbrNt5it5j65QOLA7fHzll3 2T02repk89g/dw27x+Yl9R5b+oFCnzfJBbBFudlkpCampBYppOYl56dk5qXbKoWGuOlaKCnk Jeam2ipF6PqGBCkplCXmlAJ5RgZowME5wD1YSd8uwS3j8burTAVXtCv2TT/K3MA4QbmLkZND QsBE4vmueawQtpjEhXvr2boYuTiEBJYySix/upMJwvnKKPF5QQtYFZuAlcTE9lWMILaIgJrE lacrWEGKmAWeMUu8n7SRGSQhLBAg8eJmIwuIzSKgKrHw3XKwZl4BTYnnbcfYQWxRAS+JLfva mUBsToFgiUfvnkNt28Mo8XT1XRaIBkGJH5PvgdnMAvIS+/ZPZYWwtSTW7zzONIFRYBaSsllI ymYhKVvAyLyKUSK1ILmgOCk91zAvtVyvODG3uDQvXS85P3cTIzgyn0ntYDy4y/0QowAHoxIP 74MXJRFCrIllxZW5hxglOJiVRHhdFEojhHhTEiurUovy44tKc1KLDzGaAn04kVlKNDkfmDTy SuINTcxNzI0NLMwtLU2MlMR5G2c/CxcSSE8sSc1OTS1ILYLpY+LglGpgZHNkZT8rvPpxUNP6 Xf6H3h9nTlzkN/Pt5WfM/CtqMlvu375Vt03hf7LDv7Stb0zLHszo0/YNeMqzvuS5vmG+9iv5 bD3vaj6X9qUsTfH521/X2Ym5eey35hL33rTH7o3O11Oi2o6ft79bOP+sDnNG1k/Bn1OUM2Ue H1ouelrdwr7vq/q9x5FblViKMxINtZiLihMBnp5w0OICAAA= X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170110180359epcas1p4729103fd575fe628bfd3ea7966ec4856 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: 20170110180359epcas1p4729103fd575fe628bfd3ea7966ec4856 X-RootMTR: 20170110180359epcas1p4729103fd575fe628bfd3ea7966ec4856 References: <20170110022131.31042-1-shuahkh@osg.samsung.com> <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 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). 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 > >>>>>> --- > >>>>>> 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);