Received: by 2002:a05:6a10:a0d1:0:0:0:0 with SMTP id j17csp580526pxa; Tue, 11 Aug 2020 09:55:01 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyIqYYPeHNhmy1ViiG0gS3ZYWn+jaBapWCjVKg3cB6PWboOkCgMmCKjIVgZNhxMd5YzSGj2 X-Received: by 2002:a17:906:f24c:: with SMTP id gy12mr26800681ejb.275.1597164901578; Tue, 11 Aug 2020 09:55:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1597164901; cv=none; d=google.com; s=arc-20160816; b=LDUJ5O+v2/BmJ0aRykNWdPMjGAiRs0bsWoB6yol3ihSmuyJ9ozDGpfx6O2U42SHBi7 zZvb/kaCtqIxyCm/A3qvJbabkRAZCctiM2aNL9loT1fjF3IoDIViM8zsIwu/8DgBcCVP X/VVpEEXHpVaLTdxDbU9PjEPmpgbT+Se6R9kiIZSl1U6rfRAhXFbgBXaYlGPxbChfcaZ bjHBQ7D2B7OBOLt4cu7yGP7auWkudz511ZCWkjkTeTAsAcjCHRI1o4ZlqF8MiiYlGNvv oBCwvFKly7I/r7BV0UjkdINlYrm7GBeU8UHvc0oY4nbNo7sj5oYVf6fCMsXF7Yyyze6x WzCQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=nZ/k9Byto1826yL422aEJoOrFWeBEVpscC6FOqMA7kA=; b=kKn4JD+zHQU+L4JODrN89EamNihKrXKwpFY4tq4//83jGkAEZidNNYfnH+2kEPGoI1 M3GkvRCwpJQdzNKX9TfFmb/ovXl+s/nSouwnWw4+pXtKc+yg19GcSS5BicbAQPho4HV/ eTgG14Qnk4B7apjSAztSVB0Ls+hxhTZ5UF3mTEb03PFI550Rn8k7xmDEEtNeAgKCJ0Fc nq0cz+UETfFa+E3CvkPJdvkL8PGGUITbRzE2gpxNb/6yZ0ORDkaEoQUExkm9XkInwyjI VCwGd8mf2JqEdKhj6qRWYxpooXX2B1Mumt6wJkwA3/jsZLTv18wnLeXE4NtgNPUAgaPM M+QA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=ME5nK4ym; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id b1si14276614ejb.647.2020.08.11.09.54.36; Tue, 11 Aug 2020 09:55:01 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=ME5nK4ym; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729124AbgHKQx6 (ORCPT + 99 others); Tue, 11 Aug 2020 12:53:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44848 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729046AbgHKQx5 (ORCPT ); Tue, 11 Aug 2020 12:53:57 -0400 Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6CDD8C06174A; Tue, 11 Aug 2020 09:53:57 -0700 (PDT) Received: by mail-io1-xd42.google.com with SMTP id z6so13403732iow.6; Tue, 11 Aug 2020 09:53:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=nZ/k9Byto1826yL422aEJoOrFWeBEVpscC6FOqMA7kA=; b=ME5nK4ymcdbDyMLRTGXIrkRc9MfpLvEQIypfQ6i7SLtjdscQ9yqRf5etWxP3lgzQqN WCkmcuPrpUvn6+F/UgAMbFdy2ioyoNDmVgzF0QS1FGxqFSAOkkCSLmTLJsdgqovV43Yh lsIttJxqv6oi4sSUl0Jj4ABVh2hC12n5rb5i/i9u8sW8J2LZu5FhSndWAhGTrcXLtaTl kgCRiaY8APBt85uH9U77xmouwnOv60WHMEhmUTAFiGFFq1Tnhw4PgDWonLsxmE2+b0h0 lHKQSvaHkAmweqs6ezAoljmvb69FbqQsgTLRB7GGh0/+fuMNJayFdmoUOqa7vUawHp8i jKcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=nZ/k9Byto1826yL422aEJoOrFWeBEVpscC6FOqMA7kA=; b=Qdm9/EQH1JiG8GMHN+CxNKoZZ+vy6pRShArcbaJ4g5OWupAv6DFevb0b8rBU+9sUwe mN69v5JpeWkDIboSBcgFzVOnjDrEZ3BZyuuj1MPTlbZgpBEZ6JU99nvIcsfUs1OBafE3 UJ+URG/sIe/GBKjMoOkCGvtCA8Q/kaPKjB2sV+1UbwhnRvHB4DwvzsMypydevX3ryqYU RFskPsZqyBDZQPxgDV3zEAYc7cU1Q0OJiPKDKI8Cz+vBrQKvu/zRp4L/7FfvEQA5PD7t BCnRzfpINTYCYT6eA17A/Cpo9++ZYFba2u4PaFQXshoWkrZE0+4o8ORh2qOMdadeLzRC cHTw== X-Gm-Message-State: AOAM530xkVNveqhttKljVQ5JxhBIzt16Hp+bwgCJBDeZu2ucmF9an5zg m1N9RERTtQjfTjhl8rAjHfw2k82IW6nQh3HFU9Q= X-Received: by 2002:a6b:b888:: with SMTP id i130mr23575515iof.182.1597164836425; Tue, 11 Aug 2020 09:53:56 -0700 (PDT) MIME-Version: 1.0 References: <20200811112507.24418-1-s.nawrocki@samsung.com> <66c7330e-507e-d81f-1cb1-b509bf54d050@samsung.com> In-Reply-To: <66c7330e-507e-d81f-1cb1-b509bf54d050@samsung.com> From: Tomasz Figa Date: Tue, 11 Aug 2020 18:53:44 +0200 Message-ID: Subject: Re: [PATCH v2] clk: samsung: Prevent potential endless loop in the PLL set_rate ops To: Sylwester Nawrocki Cc: "open list:COMMON CLK FRAMEWORK" , Chanwoo Choi , Stephen Boyd , Mike Turquette , "moderated list:SAMSUNG SOC CLOCK DRIVERS" , linux-kernel , Bartlomiej Zolnierkiewicz , Marek Szyprowski Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2020=E5=B9=B48=E6=9C=8811=E6=97=A5(=E7=81=AB) 18:45 Sylwester Nawrocki : > > Hi Tomasz, > > On 11.08.2020 14:59, Tomasz Figa wrote: > > 2020=E5=B9=B48=E6=9C=8811=E6=97=A5(=E7=81=AB) 13:25 Sylwester Nawrocki = : > >> > >> In the .set_rate callback for some PLLs there is a loop polling state > >> of the PLL lock bit and it may become an endless loop when something > >> goes wrong with the PLL. For some PLLs there is already (a duplicated) > >> code for polling with timeout. This patch replaces that code with > >> the readl_relaxed_poll_timeout_atomic() macro and moves it to a common > >> helper function, which is then used for all the PLLs. The downside > >> of switching to the common macro is that we drop the cpu_relax() call. > > > > Tbh. I'm not sure what effect was exactly expected from cpu_relax() in > > the functions which already had timeout handling. Could someone shed > > some light on this? > > > >> Using a common helper function rather than the macro directly allows > >> to avoid repeating the error message in the code and to avoid the obje= ct > >> code size increase due to inlining. > >> > >> Signed-off-by: Sylwester Nawrocki > >> --- > >> Changes for v2: > >> - use common readl_relaxed_poll_timeout_atomic() macro > >> --- > >> drivers/clk/samsung/clk-pll.c | 92 +++++++++++++++-------------------= --------- > >> 1 file changed, 32 insertions(+), 60 deletions(-) > >> > >> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-p= ll.c > >> index ac70ad7..c3c1efe 100644 > >> --- a/drivers/clk/samsung/clk-pll.c > >> +++ b/drivers/clk/samsung/clk-pll.c > >> @@ -9,13 +9,14 @@ > > >> -#define PLL_TIMEOUT_MS 10 > >> +#define PLL_TIMEOUT_US 10000U > > > > I'm also wondering if 10ms is the universal value that would cover the > > oldest PLLs as well, but my loose recollection is that they should > > still lock much faster than that. Could you double check that in the > > documentation? > > Thanks for your comments. > > The oldest PLLs have a hard coded 300 us waiting time for PLL lock and > are not affected by the patch. > I have checked some of the PLLs and maximum observed lock time was around > 370 us and most of the time it was just a few us. > > We calculate the lock time in each set_rate op, in the oscillator cycle > units, as a product of current P divider value and a constant PLL type > specific LOCK_FACTOR. Maximum possible P value is 64, maximum possible > LOCK_FACTOR is 3000. Assuming minimum VCO frequency of 24 MHz (which > I think will usually be much higher than that) maximum lock time > would be (64 x 3000) / 24 MHz =3D 8 ms. I think we can leave the current > 10 ms value. Sounds good to me. Thanks! > > But there is other issue, it seems we can't really use the ktime API > in the set_rate callbacks, as these could be called early, before the > clocksource is initialized and ktime doesn't work yet. Below trace > is from a dump_stack() added to the samsung_pll_lock_wait() callback. > The PLL rate setting is triggered by assigned-clock* properties in > the clock supplier node. > I think we need to switch to a simple udelay() loop, as is done in > clk-tegra210 driver for instance. > > [ 0.000000] Hardware name: Samsung Exynos (Flattened Device Tree) > [ 0.000000] [] (unwind_backtrace) from [] (show_st= ack+0x10/0x14) > [ 0.000000] [] (show_stack) from [] (dump_stack+0x= ac/0xd8) > [ 0.000000] [] (dump_stack) from [] (samsung_pll_l= ock_wait+0x14/0x174) > [ 0.000000] [] (samsung_pll_lock_wait) from [] (cl= k_change_rate+0x1a8/0x8ac) > [ 0.000000] [] (clk_change_rate) from [] (clk_core= _set_rate_nolock+0x24c/0x268) > [ 0.000000] [] (clk_core_set_rate_nolock) from [] = (clk_set_rate+0x30/0x64) > [ 0.000000] [] (clk_set_rate) from [] (of_clk_set_= defaults+0x214/0x384) > [ 0.000000] [] (of_clk_set_defaults) from [] (of_c= lk_add_hw_provider+0x98/0xd8) > [ 0.000000] [] (of_clk_add_hw_provider) from [] (s= amsung_clk_of_add_provider+0x1c/0x30) > [ 0.000000] [] (samsung_clk_of_add_provider) from [] (exynos5250_clk_of_clk_init_driver+0x1f4/0x240) > [ 0.000000] [] (exynos5250_clk_of_clk_init_driver) from [] (of_clk_init+0x16c/0x218) > [ 0.000000] [] (of_clk_init) from [] (time_init+0x= 24/0x30) > [ 0.000000] [] (time_init) from [] (start_kernel+0= x3b0/0x520) Yeah... I should've thought about this. Interestingly enough, some of the existing implementations in drivers/clk/samsung/clk-pll.c use the ktime API. I guess they are lucky enough not to be called too early, i.e. are not needed for the initialization of timers. Best regards, Tomasz > [ 0.000000] [] (start_kernel) from [<00000000>] (0x0) > [ 0.000000] samsung_pll_lock_wait: PLL fout_epll, lock time: 0 us, ret= : 0 > [ 0.000000] Exynos5250: clock setup completed, armclk=3D1700000000 > [ 0.000000] Switching to timer-based delay loop, resolution 41ns > [ 0.000000] clocksource: mct-frc: mask: 0xffffffff max_cycles: 0xfffff= fff, max_idle_ns: 79635851949 ns > [ 0.000003] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps ever= y 89478484971ns > [ 0.000032] genirq: irq_chip COMBINER did not update eff. affinity mas= k of irq 49 > [ 0.000523] arch_timer: cp15 timer(s) running at 24.00MHz (virt). > [ 0.000536] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_= cycles: 0x588fe9dc0, max_idle_ns: 440795202592 ns > [ 0.000551] sched_clock: 56 bits at 24MHz, resolution 41ns, wraps ever= y 4398046511097ns > > -- > Regards, > Sylwester