2020年8月11日(火) 18:45 Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>: > > Hi Tomasz, > > On 11.08.2020 14:59, Tomasz Figa wrote: > > 2020年8月11日(火) 13:25 Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>: > >> > >> 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 object > >> code size increase due to inlining. > >> > >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> > >> --- > >> 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-pll.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 = 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] [<c0111e9c>] (unwind_backtrace) from [<c010d0ec>] (show_stack+0x10/0x14) > [ 0.000000] [<c010d0ec>] (show_stack) from [<c051d890>] (dump_stack+0xac/0xd8) > [ 0.000000] [<c051d890>] (dump_stack) from [<c0578d94>] (samsung_pll_lock_wait+0x14/0x174) > [ 0.000000] [<c0578d94>] (samsung_pll_lock_wait) from [<c057319c>] (clk_change_rate+0x1a8/0x8ac) > [ 0.000000] [<c057319c>] (clk_change_rate) from [<c0573aec>] (clk_core_set_rate_nolock+0x24c/0x268) > [ 0.000000] [<c0573aec>] (clk_core_set_rate_nolock) from [<c0573b38>] (clk_set_rate+0x30/0x64) > [ 0.000000] [<c0573b38>] (clk_set_rate) from [<c0577df8>] (of_clk_set_defaults+0x214/0x384) > [ 0.000000] [<c0577df8>] (of_clk_set_defaults) from [<c0572f34>] (of_clk_add_hw_provider+0x98/0xd8) > [ 0.000000] [<c0572f34>] (of_clk_add_hw_provider) from [<c1120278>] (samsung_clk_of_add_provider+0x1c/0x30) > [ 0.000000] [<c1120278>] (samsung_clk_of_add_provider) from [<c1121844>] (exynos5250_clk_of_clk_init_driver+0x1f4/0x240) > [ 0.000000] [<c1121844>] (exynos5250_clk_of_clk_init_driver) from [<c11200d0>] (of_clk_init+0x16c/0x218) > [ 0.000000] [<c11200d0>] (of_clk_init) from [<c1104bdc>] (time_init+0x24/0x30) > [ 0.000000] [<c1104bdc>] (time_init) from [<c1100d20>] (start_kernel+0x3b0/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] [<c1100d20>] (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=1700000000 > [ 0.000000] Switching to timer-based delay loop, resolution 41ns > [ 0.000000] clocksource: mct-frc: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns > [ 0.000003] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns > [ 0.000032] genirq: irq_chip COMBINER did not update eff. affinity mask 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 every 4398046511097ns > > -- > Regards, > Sylwester
