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=-5.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SIGNED_OFF_BY,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 A8B6BC43603 for ; Thu, 12 Dec 2019 03:45:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6A51721655 for ; Thu, 12 Dec 2019 03:45:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=nvidia.com header.i=@nvidia.com header.b="GhnPwrUR" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727414AbfLLDpg (ORCPT ); Wed, 11 Dec 2019 22:45:36 -0500 Received: from hqnvemgate25.nvidia.com ([216.228.121.64]:14482 "EHLO hqnvemgate25.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727407AbfLLDpg (ORCPT ); Wed, 11 Dec 2019 22:45:36 -0500 Received: from hqpgpgate102.nvidia.com (Not Verified[216.228.121.13]) by hqnvemgate25.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Wed, 11 Dec 2019 19:45:27 -0800 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate102.nvidia.com (PGP Universal service); Wed, 11 Dec 2019 19:45:34 -0800 X-PGP-Universal: processed; by hqpgpgate102.nvidia.com on Wed, 11 Dec 2019 19:45:34 -0800 Received: from [10.2.169.141] (10.124.1.5) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Thu, 12 Dec 2019 03:45:32 +0000 Subject: Re: [PATCH v3 03/15] soc: tegra: Add Tegra PMC clock registrations into PMC driver To: Dmitry Osipenko , , , , , CC: , , , , , , , , , , , , , , , , , , , , , , , , , , References: <1575600535-26877-1-git-send-email-skomatineni@nvidia.com> <1575600535-26877-4-git-send-email-skomatineni@nvidia.com> <7cf4ff77-2f33-4ee5-0e09-5aa6aef3e8be@gmail.com>

<288a1701-def6-d628-26bc-a305f817bdb1@gmail.com> <78644d45-2ae3-121f-99fc-0a46f205907d@nvidia.com>

<49da77dc-b346-68eb-9ef8-42cfb3221489@nvidia.com> <3f1c9325-3017-62be-1e3b-82fd28540fdf@nvidia.com> <6fcbff3d-8695-7cd0-60de-6eb523b6964c@gmail.com> <8eb792ad-cded-05cc-93fc-763be7ee66aa@nvidia.com> <02109d70-2747-c246-5401-69a2d5c84771@gmail.com> From: Sowjanya Komatineni Message-ID: <01bf40ae-393d-3cb1-9ba2-acdd10385cb8@nvidia.com> Date: Wed, 11 Dec 2019 19:45:31 -0800 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: <02109d70-2747-c246-5401-69a2d5c84771@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=1576122327; bh=b3mpMik1+G+W/AUoZ42C8rOMX06OvOzuq5SW7MV23vw=; 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=GhnPwrURo5xknD/DovjGmU2T1L4uhPI2j1fi3ucoSj6metnxynyN2P50ADkaZeZiS cnbJvNdXdStUriV1HdP3eBJ0LF6aO5crcwwx2GQKbKuCkwYYJ5L6FKqV4NxoJ3FIwC wnh2wY2tl4Z0St9uwCsm5+XUE6wAwCegWifsIjF0yKMTm7jRYGnddxL3B/dtFY82Ik 1W76l6bprmJV9O7PhYYXENr5GJSwdiwR9Piic7luP/O+VpRuumVTkPzA1TRw1NXOcW VGmCH8f1S+Lh2Av9X0rrdTnGH6xzpytpIwxhwBHyDDhxvifSNzH1IIKyH2ANSYBcHU wB18HrMr+fOag== Sender: linux-clk-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-clk@vger.kernel.org On 12/11/19 5:39 PM, Dmitry Osipenko wrote: > 11.12.2019 21:50, Sowjanya Komatineni =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >> On 12/10/19 5:06 PM, Sowjanya Komatineni wrote: >>> On 12/10/19 9:41 AM, Dmitry Osipenko wrote: >>>> 10.12.2019 19:53, Sowjanya Komatineni =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>>>> On 12/9/19 3:03 PM, Sowjanya Komatineni wrote: >>>>>> On 12/9/19 12:46 PM, Sowjanya Komatineni wrote: >>>>>>> On 12/9/19 12:12 PM, Dmitry Osipenko wrote: >>>>>>>> 08.12.2019 00:36, Sowjanya Komatineni =D0=BF=D0=B8=D1=88=D0=B5=D1= =82: >>>>>>>>> On 12/7/19 11:59 AM, Sowjanya Komatineni wrote: >>>>>>>>>> On 12/7/19 8:00 AM, Dmitry Osipenko wrote: >>>>>>>>>>> 07.12.2019 18:53, Dmitry Osipenko =D0=BF=D0=B8=D1=88=D0=B5=D1= =82: >>>>>>>>>>>> 07.12.2019 18:47, Dmitry Osipenko =D0=BF=D0=B8=D1=88=D0=B5=D1= =82: >>>>>>>>>>>>> 07.12.2019 17:28, Dmitry Osipenko =D0=BF=D0=B8=D1=88=D0=B5=D1= =82: >>>>>>>>>>>>>> 06.12.2019 05:48, Sowjanya Komatineni =D0=BF=D0=B8=D1=88=D0= =B5=D1=82: >>>>>>>>>>>>>>> Tegra210 and prior Tegra PMC has clk_out_1, clk_out_2, >>>>>>>>>>>>>>> clk_out_3 >>>>>>>>>>>>>>> with >>>>>>>>>>>>>>> mux and gate for each of these clocks. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Currently these PMC clocks are registered by Tegra clock >>>>>>>>>>>>>>> driver >>>>>>>>>>>>>>> using >>>>>>>>>>>>>>> clk_register_mux and clk_register_gate by passing PMC base >>>>>>>>>>>>>>> address >>>>>>>>>>>>>>> and register offsets and PMC programming for these clocks >>>>>>>>>>>>>>> happens >>>>>>>>>>>>>>> through direct PMC access by the clock driver. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> With this, when PMC is in secure mode any direct PMC access >>>>>>>>>>>>>>> from the >>>>>>>>>>>>>>> non-secure world does not go through and these clocks will >>>>>>>>>>>>>>> not be >>>>>>>>>>>>>>> functional. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This patch adds these clocks registration with PMC as a clo= ck >>>>>>>>>>>>>>> provider >>>>>>>>>>>>>>> for these clocks. clk_ops callback implementations for thes= e >>>>>>>>>>>>>>> clocks >>>>>>>>>>>>>>> uses tegra_pmc_readl and tegra_pmc_writel which supports PM= C >>>>>>>>>>>>>>> programming >>>>>>>>>>>>>>> in secure mode and non-secure mode. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni >>>>>>>>>>>>>>> --- >>>>>>>>>>>>> [snip] >>>>>>>>>>>>> >>>>>>>>>>>>>>> + >>>>>>>>>>>>>>> +static const struct clk_ops pmc_clk_gate_ops =3D { >>>>>>>>>>>>>>> +=C2=A0=C2=A0=C2=A0 .is_enabled =3D pmc_clk_is_enabled, >>>>>>>>>>>>>>> +=C2=A0=C2=A0=C2=A0 .enable =3D pmc_clk_enable, >>>>>>>>>>>>>>> +=C2=A0=C2=A0=C2=A0 .disable =3D pmc_clk_disable, >>>>>>>>>>>>>>> +}; >>>>>>>>>>>>>> What's the benefit of separating GATE from the MUX? >>>>>>>>>>>>>> >>>>>>>>>>>>>> I think it could be a single clock. >>>>>>>>>>>>> According to TRM: >>>>>>>>>>>>> >>>>>>>>>>>>> 1. GATE and MUX are separate entities. >>>>>>>>>>>>> >>>>>>>>>>>>> 2. GATE is the parent of MUX (see PMC's CLK_OUT paths diagram >>>>>>>>>>>>> in TRM). >>>>>>>>>>>>> >>>>>>>>>>>>> 3. PMC doesn't gate EXTPERIPH clock but could "force-enable" >>>>>>>>>>>>> it, >>>>>>>>>>>>> correct? >>>>>>>>> Was following existing clk-tegra-pmc as I am not sure of reason f= or >>>>>>>>> having these clocks registered as separate mux and gate clocks. >>>>>>>>> >>>>>>>>> Yes, PMC clocks can be registered as single clock and can use >>>>>>>>> clk_ops >>>>>>>>> for set/get parent and enable/disable. >>>>>>>>> >>>>>>>>> enable/disable of PMC clocks is for force-enable to force the >>>>>>>>> clock to >>>>>>>>> run regardless of ACCEPT_REQ or INVERT_REQ. >>>>>>>>> >>>>>>>>>>>> 4. clk_m_div2/4 are internal PMC OSC dividers and thus these >>>>>>>>>>>> clocks >>>>>>>>>>>> should belong to PMC. >>>>>>>>>>> Also, it should be "osc" and not "clk_m". >>>>>>>>>> I followed the same parents as it were in existing clk-tegra-pmc >>>>>>>>>> driver. >>>>>>>>>> >>>>>>>>>> Yeah they are wrong and they should be from osc and not clk_m. >>>>>>>>>> >>>>>>>>>> Will fix in next version. >>>>>>>>>> >>>>>> Reg clk_m_div2/3, they are dividers at OSC pad and not really intern= al >>>>>> to PMC block. >>>>>> >>>>>> current clock driver creates clk_m_div clocks which should actually = be >>>>>> osc_div2/osc_div4 clocks with osc as parent. >>>>>> >>>>>> There are no clk_m_div2 and clk_m_div4 from clk_m >>>>>> >>>>>> Will fix this in next version. >>>>>> >>>>>>>> Could you please describe the full EXTPERIPH clock topology and >>>>>>>> how the >>>>>>>> pinmux configuration is related to it all? >>>>>>>> >>>>>>>> What is internal to the Tegra chip and what are the external >>>>>>>> outputs? >>>>>>>> >>>>>>>> Is it possible to bypass PMC on T30+ for the EXTPERIPH clocks? >>>>>>> PMC CLK1/2/3 possible sources are OSC_DIV1, OSC_DIV2, OSC_DIV4, >>>>>>> EXTPERIPH from CAR. >>>>>>> >>>>>>> OSC_DIV1/2/4 are with internal dividers at the OSC Pads >>>>>>> >>>>>>> EXTPERIPH is from CAR and it has reset and enable controls along wi= th >>>>>>> clock source selections to choose one of the PLLA_OUT0, CLK_S, >>>>>>> PLLP_OUT0, CLK_M, PLLE_OUT0 >>>>>>> >>>>>>> So, PMC CLK1/2/4 possible parents are OSC_DIV1, OSC_DIV2, OSC_DIV4, >>>>>>> EXTERN. >>>>>>> >>>>>>> >>>>>>> CLK1/2/3 also has Pinmux to route EXTPERIPH output on to these pins= . >>>>>>> >>>>>>> >>>>>>> When EXTERN output clock is selected for these PMC clocks thru >>>>>>> CLKx_SRC_SEL, output clock is from driver by EXTPERIPH from CAR via >>>>>>> Pinmux logic or driven as per CLKx_SRC_SEL bypassing pinmux based o= n >>>>>>> CLKx_ACCEPT_REQ bit. >>>>>>> >>>>>>> >>>>>>> PMC Clock control register has bit CLKx_ACCEPT_REQ >>>>>>> When CLKx_ACCEPT_REQ =3D 0, output clock driver is from by EXTPERIP= H >>>>>>> through the pinmux >>>>>>> When CLKx_ACCEPT_REQ =3D 1, output clock is based on CLKx_SRC_SEL b= its >>>>>>> (OSC_DIV1/2/4 and EXTPERIPH clock bypassing the pinmux) >>>>>>> >>>>>>> FORCE_EN bit in PMC CLock control register forces the clock to run >>>>>>> regardless of this. >>>>> PMC clock gate is based on the state of CLKx_ACCEPT_REQ and FORCE_EN >>>>> like explained above. >>>>> >>>>> CLKx_ACCEPT_REQ is 0 default and FORCE_EN acts as gate to >>>>> enable/disable >>>>> EXTPERIPH clock output to PMC CLK_OUT_1/2/3. >>>> [and to enable OSC as well] >>>> >>>>> So I believe we need to register as MUX and Gate rather than as a >>>>> single >>>>> clock. Please confirm. >>>> 1. The force-enabling is applied to both OSC and EXTERN sources of >>>> PMC_CLK_OUT_x by PMC at once. >>>> >>>> 2. Both of PMC's force-enabling and OSC/EXTERN selection is internal >>>> to PMC. >>>> >>>> Should be better to define it as a single "pmc_clk_out_x". I don't see >>>> any good reasons for differentiating PMC's Gate from the MUX, it's a >>>> single hardware unit from a point of view of the rest of the system. >>>> >>>> Peter, do you have any objections? >>> We added fallback option for audio mclk and also added check for >>> assigned-clock-parents dt property in audio driver and if its not then >>> we do parent init configuration in audio driver. >>> >>> Current clock driver creates 2 separate clocks clk_out_1_mux and >>> clk_out_1 for each pmc clock in clock driver and uses extern1 as >>> parent to clk_out_1_mux and clk_out_1_mux is parent to clk_out_1. >>> >>> With change of registering each pmc clock as a single clock, when we >>> do parent init assignment in audio driver when >>> assigned-clock-properties are not used in DT (as we removed parent >>> inits for extern and clk_outs from clock driver), we should still try >>> to get clock based on clk_out_1_mux as parent assignment of extern1 is >>> for clk_out_1_mux as per existing clock tree. >>> >>> clk_out_1_mux clock retrieve will fail with this change of single >>> clock when any new platform device tree doesn't specify >>> assigned-clock-parents properties and tegra_asoc_utils_init fails. > You made the PMC/CaR changes before the audio changes, the clk_out_1_mux > won't exist for the audio driver patches. > > If you care about bisect-ability of the patches, then the clock and > audio changes need to be done in a single patch. But I don't think that > it's worthwhile. > >>> With single clock, extern1 is the parent for clk_out_1 and with >>> separate clocks for mux and gate, extern1 is the parent for >>> clk_out_1_mux. >> If we move to single clock now, it need one more additional fallback >> implementation in audio driver during parent configuration as >> clk_out_1_mux will not be there with single clock change and old/current >> kernel has it as it uses separate clocks for pmc mux and gate. > Why additional fallback? Additional to what? > >> Also, with single clock for both PMC mux and gate now, new DT should use >> extern1 as parent to CLK_OUT_1 as CLK_OUT_1_MUX will not be there old >> PMC dt-bindings has separate clocks for MUX (CLK_OUT_1_MUX) and gate >> (CLK_OUT_1) >> >> DT bindings will not be compatible b/w old and new changes if we move to >> Single PMC clock now. > Sorry, I don't understand what you're meaning by the "new changes". > >> Should we go with same separate clocks to have it compatible to avoid >> all this? >> The reason we added mclk fallback and also for doing parent=20 configuration based on presence of assigned-clock-parents property is to=20 have old dt compatible with new kernel and also to have new dt=20 compatible with old kernel. So the point I was mentioning is to have new DT to work with old kernel,=20 setting extern1 as parent to clk_out_1 (with single pmc clock) through=20 assigned-clock-parents in DT will fail as old kernel has mux and gate as=20 separate clocks and parent configuration is for mux clock (clk_out_1_mux)