RE: [PATCH 2/3] clk: imx: Add support for i.MX8MP clock driver

[Anson Huang <anson.huang@nxp.com>]

Hi, Stephen

> Subject: Re: [PATCH 2/3] clk: imx: Add support for i.MX8MP clock driver
> 
> Quoting Anson Huang (2019-12-26 18:12:27)
> > diff --git a/drivers/clk/imx/clk-imx8mp.c
> > b/drivers/clk/imx/clk-imx8mp.c new file mode 100644 index
> > 0000000..7f0d482
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-imx8mp.c
> > @@ -0,0 +1,767 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019 NXP.
> > + */
> > +
> > +#include <dt-bindings/clock/imx8mp-clock.h>
> > +#include <linux/clk.h>
> 
> Please include clk-provider.h as this is a clk provider. If possible, don't include
> clk.h as this shouldn't be a consumer.

The reason of including clk.h is to call of_clk_get_by_name() to get some clocks from DT, currently on i.MX8M
series SoC, we still NOT switch to hw based clock, so some hw based clock APIs is NOT able to be used.

> 
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> 
> Is this include used?

Will remove it in V2.

> 
> > +#include <linux/of_address.h>
> 
> Is this include used?

It is used by of_iomap for anatop_base below;

> 
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
> > +
> > +#include "clk.h"
> > +
> > +static u32 share_count_nand;
> > +static u32 share_count_media;
> > +
> > +static const char *pll_ref_sels[] = { "osc_24m", "dummy", "dummy",
> > +"dummy", };
> 
> Is it possible to make these const char * const foo[] arrays?

Will  improve it in V2.

> 
> > +static const char *audio_pll1_bypass_sels[] = {"audio_pll1",
> > +"audio_pll1_ref_sel", }; static const char *audio_pll2_bypass_sels[]
> > += {"audio_pll2", "audio_pll2_ref_sel", }; static const char
> > +*video_pll1_bypass_sels[] = {"video_pll1", "video_pll1_ref_sel", };
> [...]
> > +       clk_set_parent(clks[IMX8MP_AUDIO_PLL1_BYPASS],
> clks[IMX8MP_AUDIO_PLL1]);
> > +       clk_set_parent(clks[IMX8MP_AUDIO_PLL2_BYPASS],
> clks[IMX8MP_AUDIO_PLL2]);
> > +       clk_set_parent(clks[IMX8MP_VIDEO_PLL1_BYPASS],
> clks[IMX8MP_VIDEO_PLL1]);
> > +       clk_set_parent(clks[IMX8MP_DRAM_PLL_BYPASS],
> clks[IMX8MP_DRAM_PLL]);
> > +       clk_set_parent(clks[IMX8MP_GPU_PLL_BYPASS],
> clks[IMX8MP_GPU_PLL]);
> > +       clk_set_parent(clks[IMX8MP_VPU_PLL_BYPASS],
> clks[IMX8MP_VPU_PLL]);
> > +       clk_set_parent(clks[IMX8MP_ARM_PLL_BYPASS],
> clks[IMX8MP_ARM_PLL]);
> > +       clk_set_parent(clks[IMX8MP_SYS_PLL1_BYPASS],
> clks[IMX8MP_SYS_PLL1]);
> > +       clk_set_parent(clks[IMX8MP_SYS_PLL2_BYPASS],
> clks[IMX8MP_SYS_PLL2]);
> > +       clk_set_parent(clks[IMX8MP_SYS_PLL3_BYPASS],
> > + clks[IMX8MP_SYS_PLL3]);
> 
> These can't be done with assigned-clock-parents properties in DT?

Ah, yes, will remove them and put them in DT is necessary.

> 
> > +
> > +       clks[IMX8MP_AUDIO_PLL1_OUT] = imx_clk_gate("audio_pll1_out",
> "audio_pll1_bypass", base, 13);
> > +       clks[IMX8MP_AUDIO_PLL2_OUT] = imx_clk_gate("audio_pll2_out",
> "audio_pll2_bypass", base + 0x14, 13);
> > +       clks[IMX8MP_VIDEO_PLL1_OUT] = imx_clk_gate("video_pll1_out",
> "video_pll1_bypass", base + 0x28, 13);
> > +       clks[IMX8MP_DRAM_PLL_OUT] = imx_clk_gate("dram_pll_out",
> "dram_pll_bypass", base + 0x50, 13);
> > +       clks[IMX8MP_GPU_PLL_OUT] = imx_clk_gate("gpu_pll_out",
> "gpu_pll_bypass", base + 0x64, 11);
> > +       clks[IMX8MP_VPU_PLL_OUT] = imx_clk_gate("vpu_pll_out",
> "vpu_pll_bypass", base + 0x74, 11);
> > +       clks[IMX8MP_ARM_PLL_OUT] = imx_clk_gate("arm_pll_out",
> "arm_pll_bypass", base + 0x84, 11);
> > +       clks[IMX8MP_SYS_PLL1_OUT] = imx_clk_gate("sys_pll1_out",
> "sys_pll1_bypass", base + 0x94, 11);
> > +       clks[IMX8MP_SYS_PLL2_OUT] = imx_clk_gate("sys_pll2_out",
> "sys_pll2_bypass", base + 0x104, 11);
> > +       clks[IMX8MP_SYS_PLL3_OUT] = imx_clk_gate("sys_pll3_out",
> > + "sys_pll3_bypass", base + 0x114, 11);
> 
> Any reason why we can't get back clk_hw pointers instead and register a hw
> based provider?

Because i.MX8M series SoCs are still NOT using hw based clock implementation, some of
the APIs are shared, like imx_clk_pll14xx() and imx8m_clk_composite() etc., so I think it is
better to keep them(i.MX8MQ/i.MX8MM/i.MX8MN/i.MX8MP) aligned, and I will find a chance soon
to do a patch series to switch all of them to hw based clock, does it make sense to you? 

> 
> > +
> > +       clks[IMX8MP_SYS_PLL1_40M] = imx_clk_fixed_factor("sys_pll1_40m",
> "sys_pll1_out", 1, 20);
> > +       clks[IMX8MP_SYS_PLL1_80M] = imx_clk_fixed_factor("sys_pll1_80m",
> "sys_pll1_out", 1, 10);
> > +       clks[IMX8MP_SYS_PLL1_100M] =
> imx_clk_fixed_factor("sys_pll1_100m", "sys_pll1_out", 1, 8);
> > +       clks[IMX8MP_SYS_PLL1_133M] =
> imx_clk_fixed_factor("sys_pll1_133m", "sys_pll1_out", 1, 6);
> > +       clks[IMX8MP_SYS_PLL1_160M] =
> imx_clk_fixed_factor("sys_pll1_160m", "sys_pll1_out", 1, 5);
> > +       clks[IMX8MP_SYS_PLL1_200M] =
> imx_clk_fixed_factor("sys_pll1_200m", "sys_pll1_out", 1, 4);
> > +       clks[IMX8MP_SYS_PLL1_266M] =
> imx_clk_fixed_factor("sys_pll1_266m", "sys_pll1_out", 1, 3);
> > +       clks[IMX8MP_SYS_PLL1_400M] =
> imx_clk_fixed_factor("sys_pll1_400m", "sys_pll1_out", 1, 2);
> > +       clks[IMX8MP_SYS_PLL1_800M] =
> > + imx_clk_fixed_factor("sys_pll1_800m", "sys_pll1_out", 1, 1);
> > +
> > +       clks[IMX8MP_SYS_PLL2_50M] = imx_clk_fixed_factor("sys_pll2_50m",
> "sys_pll2_out", 1, 20);
> > +       clks[IMX8MP_SYS_PLL2_100M] =
> imx_clk_fixed_factor("sys_pll2_100m", "sys_pll2_out", 1, 10);
> > +       clks[IMX8MP_SYS_PLL2_125M] =
> imx_clk_fixed_factor("sys_pll2_125m", "sys_pll2_out", 1, 8);
> > +       clks[IMX8MP_SYS_PLL2_166M] =
> imx_clk_fixed_factor("sys_pll2_166m", "sys_pll2_out", 1, 6);
> > +       clks[IMX8MP_SYS_PLL2_200M] =
> imx_clk_fixed_factor("sys_pll2_200m", "sys_pll2_out", 1, 5);
> > +       clks[IMX8MP_SYS_PLL2_250M] =
> imx_clk_fixed_factor("sys_pll2_250m", "sys_pll2_out", 1, 4);
> > +       clks[IMX8MP_SYS_PLL2_333M] =
> imx_clk_fixed_factor("sys_pll2_333m", "sys_pll2_out", 1, 3);
> > +       clks[IMX8MP_SYS_PLL2_500M] =
> imx_clk_fixed_factor("sys_pll2_500m", "sys_pll2_out", 1, 2);
> > +       clks[IMX8MP_SYS_PLL2_1000M] =
> > + imx_clk_fixed_factor("sys_pll2_1000m", "sys_pll2_out", 1, 1);
> > +
> > +       np = dev->of_node;
> > +       base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (WARN_ON(IS_ERR(base))) {
> > +               ret = PTR_ERR(base);
> > +               goto unregister_clks;
> 
> Why not ioremap first so we don't have to unwind clk registration on failure?

Yes, I will do it in V2.

> 
> > +       }
> > +
> > +       clks[IMX8MP_CLK_A53_SRC] = imx_clk_mux2("arm_a53_src", base +
> 0x8000, 24, 3, imx8mp_a53_sels, ARRAY_SIZE(imx8mp_a53_sels));
> > +       clks[IMX8MP_CLK_M7_SRC] = imx_clk_mux2("arm_m7_src", base +
> 0x8080, 24, 3, imx8mp_m7_sels, ARRAY_SIZE(imx8mp_m7_sels));
> > +       clks[IMX8MP_CLK_ML_SRC] = imx_clk_mux2("ml_src", base + 0x8100,
> 24, 3, imx8mp_ml_sels, ARRAY_SIZE(imx8mp_ml_sels));
> > +       clks[IMX8MP_CLK_GPU3D_CORE_SRC] =
> imx_clk_mux2("gpu3d_core_src", base + 0x8180, 24, 3,
> imx8mp_gpu3d_core_sels, ARRAY_SIZE(imx8mp_gpu3d_core_sels));
> > +       clks[IMX8MP_CLK_GPU3D_SHADER_SRC] =
> imx_clk_mux2("gpu3d_shader_src", base + 0x8200, 24, 3,
> imx8mp_gpu3d_shader_sels, ARRAY_SIZE(imx8mp_gpu3d_shader_sels));
> > +       clks[IMX8MP_CLK_GPU2D_SRC] = imx_clk_mux2("gpu2d_src", base +
> 0x8280, 24, 3, imx8mp_gpu2d_sels, ARRAY_SIZE(imx8mp_gpu2d_sels));
> > +       clks[IMX8MP_CLK_AUDIO_AXI_SRC] = imx_clk_mux2("audio_axi_src",
> > + base + 0x8300, 24, 3, imx8mp_audio_axi_sels,
> > + ARRAY_SIZE(imx8mp_audio_axi_sels));
> [...]
> > +
> > +       imx_register_uart_clocks(uart_clks);
> > +
> > +       return 0;
> > +
> > +unregister_clks:
> > +       imx_unregister_clocks(clks, ARRAY_SIZE(clks));
> > +
> > +       return ret;
> > +}
> > +
> > +static const struct of_device_id imx8mp_clk_of_match[] = {
> > +       { .compatible = "fsl,imx8mp-ccm" },
> > +       { /* Sentinel */ },
> 
> Please drop the comma after sentinel so that nothing can go after it.

Will do it in V2.

Thanks,
Anson