linux-kernel - Re: [RESEND] clk: imx: Refactor entire sccg pll clk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <155078304428.77512.12422358164048389195@swboyd.mtv.corp.google.com>
Date:   Thu, 21 Feb 2019 13:04:04 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Abel Vesa <abel.vesa@....com>, Aisheng Dong <aisheng.dong@....com>,
        Anson Huang <anson.huang@....com>,
        Fabio Estevam <fabio.estevam@....com>,
        Fabio Estevam <festevam@...il.com>,
        Jacky Bai <ping.bai@....com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Mike Turquette <mturquette@...libre.com>,
        Sascha Hauer <kernel@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>
Cc:     dl-linux-imx <linux-imx@....com>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Abel Vesa <abel.vesa@....com>
Subject: Re: [RESEND] clk: imx: Refactor entire sccg pll clk

Quoting Abel Vesa (2019-02-18 01:24:33)
> Make the entire combination of plls to be one single clock. The parents used
> for bypasses are specified each as an index in the parents list.
> The determine_rate does a lookup throughout all the possible combinations
> for all the divs and returns the best possible 'setup' which in turn is used
> by set_rate later to set up all the divs and bypasses.
> 
> Signed-off-by: Abel Vesa <abel.vesa@....com>
> Tested-by: Lucas Stach <l.stach@...gutronix.de>
> Acked-by: Lucas Stach <l.stach@...gutronix.de>
> ---
> 
> This is basically just a resend of the following:
> https://lkml.org/lkml/2019/1/29/874
> 
> There are no changes. Just added T-b and A-b tags from Lucas.
> 
> Waiting for review.

Spoke too soon. I've kicked this out of the tree. This doesn't compile
and spews a bunch of sparse warnings on arm32. Presumably because the
use of unsigned long was assuming 64 bits in size? Please write portable
code and fix these sparse warnings. Probably all the unsigned long usage
should be u64?

drivers/clk/imx/clk-sccg-pll.c:142:53: warning: decimal constant 2400000000 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here
drivers/clk/imx/clk-sccg-pll.c:199:48: warning: decimal constant 2400000000 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a warning) here
drivers/clk/imx/clk-sccg-pll.c:138:17: error: incompatible types in comparison expression (different type sizes)
drivers/clk/imx/clk-sccg-pll.c:144:25: error: incompatible types in comparison expression (different type sizes)
drivers/clk/imx/clk-sccg-pll.c:180:17: error: incompatible types in comparison expression (different type sizes)
drivers/clk/imx/clk-sccg-pll.c:217:17: error: incompatible types in comparison expression (different type sizes)
drivers/clk/imx/clk-sccg-pll.c:239:17: error: incompatible types in comparison expression (different type sizes)
drivers/clk/imx/clk-sccg-pll.c:138:17: warning: shift too big (32) for type unsigned long
drivers/clk/imx/clk-sccg-pll.c:144:25: warning: shift too big (32) for type unsigned long
drivers/clk/imx/clk-sccg-pll.c:180:17: warning: shift too big (32) for type unsigned long
drivers/clk/imx/clk-sccg-pll.c:217:17: warning: shift too big (32) for type long
drivers/clk/imx/clk-sccg-pll.c:239:17: warning: shift too big (32) for type unsigned long
In file included from arch/arm/include/asm/div64.h:127:0,
                 from include/linux/kernel.h:207,
                 from include/asm-generic/bug.h:18,
                 from arch/arm/include/asm/bug.h:60,
                 from include/linux/bug.h:5,
                 from include/linux/io.h:23,
                 from include/linux/clk-provider.h:9,
                 from drivers/clk/imx/clk-sccg-pll.c:11:
drivers/clk/imx/clk-sccg-pll.c: In function 'clk_sccg_divq_lookup':
include/asm-generic/div64.h:222:28: warning: comparison of distinct pointer types lacks a cast
  (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \  
                            ^

> @@ -347,14 +319,12 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>         clks[IMX8MQ_AUDIO_PLL2_BYPASS] = imx_clk_mux("audio_pll2_bypass", base + 0x8, 14, 1, audio_pll2_bypass_sels, ARRAY_SIZE(audio_pll2_bypass_sels));
>         clks[IMX8MQ_VIDEO_PLL1_BYPASS] = imx_clk_mux("video_pll1_bypass", base + 0x10, 14, 1, video_pll1_bypass_sels, ARRAY_SIZE(video_pll1_bypass_sels));
>  
> -       clks[IMX8MQ_SYS1_PLL1_OUT] = imx_clk_mux("sys1_pll1_out", base + 0x30, 5, 1, sys1_pll1_out_sels, ARRAY_SIZE(sys1_pll1_out_sels));
> -       clks[IMX8MQ_SYS2_PLL1_OUT] = imx_clk_mux("sys2_pll1_out", base + 0x3c, 5, 1, sys2_pll1_out_sels, ARRAY_SIZE(sys2_pll1_out_sels));
> -       clks[IMX8MQ_SYS3_PLL1_OUT] = imx_clk_mux("sys3_pll1_out", base + 0x48, 5, 1, sys3_pll1_out_sels, ARRAY_SIZE(sys3_pll1_out_sels));
> -       clks[IMX8MQ_DRAM_PLL1_OUT] = imx_clk_mux("dram_pll1_out", base + 0x60, 5, 1, dram_pll1_out_sels, ARRAY_SIZE(dram_pll1_out_sels));
> -       clks[IMX8MQ_SYS1_PLL2_OUT] = imx_clk_mux("sys1_pll2_out", base + 0x30, 4, 1, sys1_pll2_out_sels, ARRAY_SIZE(sys1_pll2_out_sels));
> -       clks[IMX8MQ_SYS2_PLL2_OUT] = imx_clk_mux("sys2_pll2_out", base + 0x3c, 4, 1, sys2_pll2_out_sels, ARRAY_SIZE(sys2_pll2_out_sels));
> -       clks[IMX8MQ_SYS3_PLL2_OUT] = imx_clk_mux("sys3_pll2_out", base + 0x48, 4, 1, sys3_pll2_out_sels, ARRAY_SIZE(sys3_pll2_out_sels));
> -       clks[IMX8MQ_DRAM_PLL2_OUT] = imx_clk_mux("dram_pll2_out", base + 0x60, 4, 1, dram_pll2_out_sels, ARRAY_SIZE(dram_pll2_out_sels));
> +       /* unbypass all the plls */
> +       clk_set_parent(clks[IMX8MQ_GPU_PLL_BYPASS], clks[IMX8MQ_GPU_PLL]);
> +       clk_set_parent(clks[IMX8MQ_VPU_PLL_BYPASS], clks[IMX8MQ_VPU_PLL]);
> +       clk_set_parent(clks[IMX8MQ_AUDIO_PLL1_BYPASS], clks[IMX8MQ_AUDIO_PLL1]);
> +       clk_set_parent(clks[IMX8MQ_AUDIO_PLL2_BYPASS], clks[IMX8MQ_AUDIO_PLL2]);
> +       clk_set_parent(clks[IMX8MQ_VIDEO_PLL1_BYPASS], clks[IMX8MQ_VIDEO_PLL1]);

Why? Can you just write the hardware registers directly instead of going
through the clk APIs during this clk registration routine? While it's
obviously easier to just call consumer APIs, it makes it weird because
this is a clk provider driver which 1) shouldn't use the consumer APIs
and 2) makes probe ordering and other things complicated to figure out.
Plus one more extra, it means that changing this code to use the clk_hw
APIs is harder.

>  
>         /* PLL OUT GATE */
>         clks[IMX8MQ_ARM_PLL_OUT] = imx_clk_gate("arm_pll_out", "arm_pll_bypass", base + 0x28, 21);