From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 49440C76195 for ; Tue, 16 Jul 2019 21:12:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 15D7A2173E for ; Tue, 16 Jul 2019 21:12:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=nvidia.com header.i=@nvidia.com header.b="heGqTnpR" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388860AbfGPVMm (ORCPT ); Tue, 16 Jul 2019 17:12:42 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:8212 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728294AbfGPVMm (ORCPT ); Tue, 16 Jul 2019 17:12:42 -0400 Received: from hqpgpgate101.nvidia.com (Not Verified[216.228.121.13]) by hqemgate14.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Tue, 16 Jul 2019 14:12:39 -0700 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate101.nvidia.com (PGP Universal service); Tue, 16 Jul 2019 14:12:39 -0700 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Tue, 16 Jul 2019 14:12:39 -0700 Received: from [10.2.164.12] (10.124.1.5) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Tue, 16 Jul 2019 21:12:37 +0000 Subject: Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks To: Dmitry Osipenko , Peter De Schrijver , Joseph Lo CC: , , , , , , , , , , , , , , , , , , , References:

<3938092a-bbc7-b304-641d-31677539598d@nvidia.com> <932d4d50-120c-9191-6a9a-23bf9c96633b@nvidia.com> <0ee055ad-d397-32e5-60ee-d62c14c6f77b@gmail.com> <86fc07d5-ab2e-a52a-a570-b1dfff4c20fe@nvidia.com> <20190716083701.225f0fd9@dimatab> <21266e4f-16b1-4c87-067a-16c07c803b6e@nvidia.com> <20190716080610.GE12715@pdeschrijver-desktop.Nvidia.com> <72b5df8c-8acb-d0d0-ebcf-b406e8404973@nvidia.com> <2b701832-5548-7c83-7c17-05cc2f1470c8@nvidia.com> <76e341be-6f38-2bc1-048e-1aa6883f9b88@gmail.com> <0706576a-ce61-1cf3-bed1-05f54a1e2489@nvidia.com> <5b2945c5-fcb2-2ac0-2bf2-df869dc9c713@gmail.com> <27641e30-fdd1-e53a-206d-71e1f23343fd@gmail.com> From: Sowjanya Komatineni Message-ID: <10c4b9a2-a857-d124-c22d-7fd71a473079@nvidia.com> Date: Tue, 16 Jul 2019 14:12:36 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <27641e30-fdd1-e53a-206d-71e1f23343fd@gmail.com> X-Originating-IP: [10.124.1.5] X-ClientProxiedBy: HQMAIL105.nvidia.com (172.20.187.12) To HQMAIL107.nvidia.com (172.20.187.13) Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: quoted-printable Content-Language: en-US DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1563311559; bh=jSr6oXibI3bnE25r/sq4SDRAKfRgY+SLeqGAJmMGsEw=; h=X-PGP-Universal:Subject:To:CC:References:From:Message-ID:Date: User-Agent:MIME-Version:In-Reply-To:X-Originating-IP: X-ClientProxiedBy:Content-Type:Content-Transfer-Encoding: Content-Language; b=heGqTnpR1H30+rrY7pnpIvaesWuyXd3eHZC4RHjFVblRgflphHqJhupSrjzvOyGIN 4KrNWWff5q9i4WA40g2rZxWleS4Jj9yDyLlQeypcoP7GOrbSLGi9tVYWzDpdcwNwI0 /wJadFL12aiYzhA69lPywFNDE8LYr/cDenPFEzEevu3v0Z7DiDw01bFPpYlt/2LQQX /ea53YbUge9b95wNWXj39Xo4z90Fn15nh8AHDvxFxC5HNedElPowW3af5jeIj5e7Q/ 1KMG/Y0zEH5WKk6tWnmY0UoVLZ0EBW6NjGjDTPnF+B9DfdSvUYVudSi9ymwGQ0fO3V qGoXnrcBOh29A== Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 7/16/19 1:47 PM, Dmitry Osipenko wrote: > 16.07.2019 22:26, Sowjanya Komatineni =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >> On 7/16/19 11:43 AM, Dmitry Osipenko wrote: >>> 16.07.2019 21:30, Sowjanya Komatineni =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote: >>>>> 16.07.2019 21:19, Sowjanya Komatineni =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote: >>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote: >>>>>>>> 16.07.2019 11:06, Peter De Schrijver =D0=BF=D0=B8=D1=88=D0=B5=D1= =82: >>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote: >>>>>>>>>>> OK, Will add to CPUFreq driver... >>>>>>>>>>>> The other thing that also need attention is that T124 CPUFreq >>>>>>>>>>>> driver >>>>>>>>>>>> implicitly relies on DFLL driver to be probed first, which is >>>>>>>>>>>> icky. >>>>>>>>>>>> >>>>>>>>>>> Should I add check for successful dfll clk register explicitly = in >>>>>>>>>>> CPUFreq driver probe and defer till dfll clk registers? >>>>>>>> Probably you should use the "device links". See [1][2] for the >>>>>>>> example. >>>>>>>> >>>>>>>> [1] >>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/teg= ra/dc.c#L2383 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> [2] >>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html >>>>>>>> >>>>>>>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. >>>>>>>> And >>>>>>>> use of_find_device_by_node() to get the DFLL's device, see [3]. >>>>>>>> >>>>>>>> [3] >>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.gi= t/tree/drivers/devfreq/tegra20-devfreq.c#n100 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> Will go thru and add... >>>>> Looks like I initially confused this case with getting orphaned clock= . >>>>> I'm now seeing that the DFLL driver registers the clock and then >>>>> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is >>>>> probed, hence everything should be fine as-is and there is no real ne= ed >>>>> for the 'device link'. Sorry for the confusion! >>>>> >>>>>>>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL >>>>>>>>>> part. >>>>>>>>>> >>>>>>>>>> As you know it, the DFLL clock is one of the CPU clock sources a= nd >>>>>>>>>> integrated with DVFS control logic with the regulator. We will n= ot >>>>>>>>>> switch >>>>>>>>>> CPU to other clock sources once we switched to DFLL. Because the >>>>>>>>>> CPU has >>>>>>>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP >>>>>>>>>> table >>>>>>>>>> you see >>>>>>>>>> in the driver.). We shouldn't reparent it to other sources with >>>>>>>>>> unknew >>>>>>>>>> freq/volt pair. That's not guaranteed to work. We allow >>>>>>>>>> switching to >>>>>>>>>> open-loop mode but different sources. >>>>>>>> Okay, then the CPUFreq driver will have to enforce DFLL freq to >>>>>>>> PLLP's >>>>>>>> rate before switching to PLLP in order to have a proper CPU voltag= e. >>>>>>> PLLP freq is safe to work for any CPU voltage. So no need to enforc= e >>>>>>> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during >>>>>>> suspend >>>>>>> >>>>>> Sorry, please ignore my above comment. During suspend, need to chang= e >>>>>> CCLK_G source to PLLP when dfll is in closed loop mode first and the= n >>>>>> dfll need to be set to open loop. >>>>> Okay. >>>>> >>>>>>>>>> And I don't exactly understand why we need to switch to PLLP in >>>>>>>>>> CPU >>>>>>>>>> idle >>>>>>>>>> driver. Just keep it on CL-DVFS mode all the time. >>>>>>>>>> >>>>>>>>>> In SC7 entry, the dfll suspend function moves it the open-loop >>>>>>>>>> mode. That's >>>>>>>>>> all. The sc7-entryfirmware will handle the rest of the sequence = to >>>>>>>>>> turn off >>>>>>>>>> the CPU power. >>>>>>>>>> >>>>>>>>>> In SC7 resume, the warmboot code will handle the sequence to >>>>>>>>>> turn on >>>>>>>>>> regulator and power up the CPU cluster. And leave it on PLL_P. >>>>>>>>>> After >>>>>>>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock >>>>>>>>>> policy (CPU >>>>>>>>>> runs on DFLL open-loop mode) and then moving to close-loop mode. >>>>>>>> The DFLL is re-inited after switching CCLK to DFLL parent during o= f >>>>>>>> the >>>>>>>> early clocks-state restoring by CaR driver. Hence instead of havin= g >>>>>>>> odd >>>>>>>> hacks in the CaR driver, it is much nicer to have a proper >>>>>>>> suspend-resume sequencing of the device drivers. In this case >>>>>>>> CPUFreq >>>>>>>> driver is the driver that enables DFLL and switches CPU to that >>>>>>>> clock >>>>>>>> source, which means that this driver is also should be >>>>>>>> responsible for >>>>>>>> management of the DFLL's state during of suspend/resume process. I= f >>>>>>>> CPUFreq driver disables DFLL during suspend and re-enables it duri= ng >>>>>>>> resume, then looks like the CaR driver hacks around DFLL are not >>>>>>>> needed. >>>>>>>> >>>>>>>>>> The DFLL part looks good to me. BTW, change the patch subject to >>>>>>>>>> "Add >>>>>>>>>> suspend-resume support" seems more appropriate to me. >>>>>>>>>> >>>>>>>>> To clarify this, the sequences for DFLL use are as follows >>>>>>>>> (assuming >>>>>>>>> all >>>>>>>>> required DFLL hw configuration has been done) >>>>>>>>> >>>>>>>>> Switch to DFLL: >>>>>>>>> 0) Save current parent and frequency >>>>>>>>> 1) Program DFLL to open loop mode >>>>>>>>> 2) Enable DFLL >>>>>>>>> 3) Change cclk_g parent to DFLL >>>>>>>>> For OVR regulator: >>>>>>>>> 4) Change PWM output pin from tristate to output >>>>>>>>> 5) Enable DFLL PWM output >>>>>>>>> For I2C regulator: >>>>>>>>> 4) Enable DFLL I2C output >>>>>>>>> 6) Program DFLL to closed loop mode >>>>>>>>> >>>>>>>>> Switch away from DFLL: >>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU frequency is ok for an= y >>>>>>>>> vdd_cpu voltage >>>>>>>>> 1) Program DFLL to open loop mode >>>>>>>>> >>>>>> I see during switch away from DFLL (suspend), cclk_g parent is not >>>>>> changed to PLLP before changing dfll to open loop mode. >>>>>> >>>>>> Will add this ... >>>>> The CPUFreq driver switches parent to PLLP during the probe, similar >>>>> should be done on suspend. >>>>> >>>>> I'm also wondering if it's always safe to switch to PLLP in the probe= . >>>>> If CPU is running on a lower freq than PLLP, then some other more >>>>> appropriate intermediate parent should be selected. >>>>> >>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher ra= te >>>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable >>>> should be safe. >>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLL= P >>> which CCLKG supports, the PLLP_OUT4. >>> >>> Probably, realistically, CPU is always running off a fast PLLX during >>> boot, but I'm wondering what may happen on KEXEC. I guess ideally >>> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL >>> on a reboot, but likely that there are other clock-related problems as >>> well that may break KEXEC and thus it is not very important at the >>> moment. >>> >>> [snip] >> During bootup CPUG sources from PLL_X. By PLL_P source above I meant >> PLL_P_OUT4. >> >> As per clock policies, PLL_X is always used for high freq like >800Mhz >> and for low frequency it will be sourced from PLLP. > Alright, then please don't forget to pre-initialize PLLP_OUT4 rate to a > reasonable value using tegra_clk_init_table or assigned-clocks. PLLP_OUT4 rate update is not needed as it is safe to run at 408Mhz=20 because it is below fmax @ Vmin