[v9,2/5] clk: meson: a1: add Amlogic A1 PLL clock controller driver

Commit Message

Dmitry Rokosov March 1, 2023, 6:37 p.m. UTC

Introduce PLL clock controller for Amlogic A1 SoC family.

Signed-off-by: Jian Hu <[e-mail]>
Signed-off-by: Dmitry Rokosov <[e-mail]>
---
 drivers/clk/meson/Kconfig  |  10 +
 drivers/clk/meson/Makefile |   1 +
 drivers/clk/meson/a1-pll.c | 365 +++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/a1-pll.h |  47 +++++
 4 files changed, 423 insertions(+)
 create mode 100644 drivers/clk/meson/a1-pll.c
 create mode 100644 drivers/clk/meson/a1-pll.h

Comments

kernel test robot March 2, 2023, 10:02 a.m. UTC | #1

Hi Dmitry,

I love your patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Rokosov/clk-meson-add-support-for-A1-PLL-clock-ops/20230302-024110
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link:    https://lore.kernel.org/r/20230301183759.16163-3-ddrokosov%40sberdevices.ru
patch subject: [PATCH v9 2/5] clk: meson: a1: add Amlogic A1 PLL clock controller driver
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20230302/[e-mail]/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7f065968cd67b07045e9dda1d9b8fabb06f2100d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Dmitry-Rokosov/clk-meson-add-support-for-A1-PLL-clock-ops/20230302-024110
        git checkout 7f065968cd67b07045e9dda1d9b8fabb06f2100d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[e-mail]>
| Link: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All errors (new ones prefixed by >>):

>> drivers/clk/meson/a1-pll.c:13:10: fatal error: meson-a1-clkc.h: No such file or directory
      13 | #include "meson-a1-clkc.h"
         |          ^~~~~~~~~~~~~~~~~
   compilation terminated.


vim +13 drivers/clk/meson/a1-pll.c

  > 13	#include "meson-a1-clkc.h"
    14	#include "a1-pll.h"
    15	#include "clk-regmap.h"
    16

Jerome Brunet March 6, 2023, 11:17 a.m. UTC | #2

On Wed 01 Mar 2023 at 21:37, Dmitry Rokosov <[e-mail]> wrote:

> Introduce PLL clock controller for Amlogic A1 SoC family.
>
> Signed-off-by: Jian Hu <[e-mail]>
> Signed-off-by: Dmitry Rokosov <[e-mail]>
> ---
>  drivers/clk/meson/Kconfig  |  10 +
>  drivers/clk/meson/Makefile |   1 +
>  drivers/clk/meson/a1-pll.c | 365 +++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/a1-pll.h |  47 +++++
>  4 files changed, 423 insertions(+)
>  create mode 100644 drivers/clk/meson/a1-pll.c
>  create mode 100644 drivers/clk/meson/a1-pll.h
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index fc002c155bc3..f56da2a4b000 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -99,6 +99,16 @@ config COMMON_CLK_AXG_AUDIO
>  	  Support for the audio clock controller on AmLogic A113D devices,
>  	  aka axg, Say Y if you want audio subsystem to work.
>  
> +config COMMON_CLK_A1_PLL
> +	tristate "Meson A1 SoC PLL controller support"
> +	depends on ARM64
> +	select COMMON_CLK_MESON_REGMAP
> +	select COMMON_CLK_MESON_PLL
> +	help
> +	  Support for the PLL clock controller on Amlogic A113L based
> +	  device, A1 SoC Family. Say Y if you want A1 PLL clock controller
> +	  to work.
> +
>  config COMMON_CLK_G12A
>  	tristate "G12 and SM1 SoC clock controllers support"
>  	depends on ARM64
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 6eca2a406ee3..2f17f475a48f 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
>  
>  obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
> +obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
>  obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
> new file mode 100644
> index 000000000000..c565f9b2a8dd
> --- /dev/null
> +++ b/drivers/clk/meson/a1-pll.c
> @@ -0,0 +1,365 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + * Author: Jian Hu <[e-mail]>
> + *
> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <[e-mail]>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include "meson-a1-clkc.h"

As pointed out by the kernel robot, there is a problem here

> +#include "a1-pll.h"
> +#include "clk-regmap.h"
> +
> +static struct clk_regmap fixed_pll_dco = {
> +	.data = &(struct meson_clk_pll_data){
> +		.en = {
> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
> +			.shift   = 28,
> +			.width   = 1,
> +		},
> +		.m = {
> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
> +			.shift   = 0,
> +			.width   = 8,
> +		},
> +		.n = {
> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
> +			.shift   = 10,
> +			.width   = 5,
> +		},
> +		.frac = {
> +			.reg_off = ANACTRL_FIXPLL_CTRL1,
> +			.shift   = 0,
> +			.width   = 19,
> +		},
> +		.l = {
> +			.reg_off = ANACTRL_FIXPLL_STS,
> +			.shift   = 31,
> +			.width   = 1,
> +		},
> +		.rst = {
> +			.reg_off = ANACTRL_FIXPLL_CTRL0,
> +			.shift   = 29,
> +			.width   = 1,
> +		},
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fixed_pll_dco",
> +		.ops = &meson_clk_pll_ro_ops,
> +		.parent_data = &(const struct clk_parent_data) {
> +			.fw_name = "fixpll_in",
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap fixed_pll = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = ANACTRL_FIXPLL_CTRL0,
> +		.bit_idx = 20,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "fixed_pll",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fixed_pll_dco.hw
> +		},
> +		.num_parents = 1,
> +		/*
> +		 * It is enough that the fdiv leaf has critical flag,
> +		 * No critical or unused flag here.
> +		 */

The comment is not useful

> +	},
> +};
> +
> +static const struct pll_mult_range hifi_pll_mult_range = {
> +	.min = 32,
> +	.max = 64,
> +};
> +
> +static const struct reg_sequence hifi_init_regs[] = {
> +	{ .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
> +	{ .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
> +	{ .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
> +	{ .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
> +	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },

This last poke should not bits otherwise handled by parms.
This is a rate init in disguise.

> +};
> +
> +static struct clk_regmap hifi_pll = {
> +	.data = &(struct meson_clk_pll_data){
> +		.en = {
> +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> +			.shift   = 28,
> +			.width   = 1,
> +		},
> +		.m = {
> +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> +			.shift   = 0,
> +			.width   = 8,
> +		},
> +		.n = {
> +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> +			.shift   = 10,
> +			.width   = 5,
> +		},
> +		.frac = {
> +			.reg_off = ANACTRL_HIFIPLL_CTRL1,
> +			.shift   = 0,
> +			.width   = 19,
> +		},
> +		.l = {
> +			.reg_off = ANACTRL_HIFIPLL_STS,
> +			.shift   = 31,
> +			.width   = 1,
> +		},
> +		.current_en = {
> +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> +			.shift   = 26,
> +			.width   = 1,
> +		},
> +		.l_detect = {

What is this ?

> +			.reg_off = ANACTRL_HIFIPLL_CTRL2,
> +			.shift   = 6,
> +			.width   = 1,
> +		},
> +		.range = &hifi_pll_mult_range,
> +		.init_regs = hifi_init_regs,
> +		.init_count = ARRAY_SIZE(hifi_init_regs),
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "hifi_pll",
> +		.ops = &meson_clk_pll_ops,
> +		.parent_data = &(const struct clk_parent_data) {
> +			.fw_name = "hifipll_in",
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_fixed_factor fclk_div2_div = {
> +	.mult = 1,
> +	.div = 2,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div2_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fixed_pll.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap fclk_div2 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = ANACTRL_FIXPLL_CTRL0,
> +		.bit_idx = 21,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div2",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fclk_div2_div.hw
> +		},
> +		.num_parents = 1,
> +		/*
> +		 * This clock is used by DDR clock in BL2 firmware
> +		 * and is required by the platform to operate correctly.
> +		 * Until the following condition are met, we need this clock to
> +		 * be marked as critical:
> +		 * a) Mark the clock used by a firmware resource, if possible
> +		 * b) CCF has a clock hand-off mechanism to make the sure the
> +		 *    clock stays on until the proper driver comes along
> +		 */
> +		.flags = CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static struct clk_fixed_factor fclk_div3_div = {
> +	.mult = 1,
> +	.div = 3,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div3_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fixed_pll.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap fclk_div3 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = ANACTRL_FIXPLL_CTRL0,
> +		.bit_idx = 22,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div3",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fclk_div3_div.hw
> +		},
> +		.num_parents = 1,
> +		/*
> +		 * This clock is used by APB bus which is set in boot ROM code
> +		 * and is required by the platform to operate correctly.
> +		 * About critical, refer to fclk_div2.

This last line is not useful. Same for other occurences

> +		 */
> +		.flags = CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static struct clk_fixed_factor fclk_div5_div = {
> +	.mult = 1,
> +	.div = 5,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div5_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fixed_pll.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap fclk_div5 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = ANACTRL_FIXPLL_CTRL0,
> +		.bit_idx = 23,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div5",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fclk_div5_div.hw
> +		},
> +		.num_parents = 1,
> +		/*
> +		 * This clock is used by AXI bus which setted in Romcode
> +		 * and is required by the platform to operate correctly.
> +		 * About critical, refer to fclk_div2.
> +		 */
> +		.flags = CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static struct clk_fixed_factor fclk_div7_div = {
> +	.mult = 1,
> +	.div = 7,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div7_div",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fixed_pll.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +static struct clk_regmap fclk_div7 = {
> +	.data = &(struct clk_regmap_gate_data){
> +		.offset = ANACTRL_FIXPLL_CTRL0,
> +		.bit_idx = 24,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "fclk_div7",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&fclk_div7_div.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};
> +
> +/* Array of all clocks provided by this provider */
> +static struct clk_hw_onecell_data a1_pll_hw_onecell_data = {
> +	.hws = {
> +		[CLKID_FIXED_PLL_DCO]	= &fixed_pll_dco.hw,
> +		[CLKID_FIXED_PLL]	= &fixed_pll.hw,
> +		[CLKID_FCLK_DIV2_DIV]	= &fclk_div2_div.hw,
> +		[CLKID_FCLK_DIV3_DIV]	= &fclk_div3_div.hw,
> +		[CLKID_FCLK_DIV5_DIV]	= &fclk_div5_div.hw,
> +		[CLKID_FCLK_DIV7_DIV]	= &fclk_div7_div.hw,
> +		[CLKID_FCLK_DIV2]	= &fclk_div2.hw,
> +		[CLKID_FCLK_DIV3]	= &fclk_div3.hw,
> +		[CLKID_FCLK_DIV5]	= &fclk_div5.hw,
> +		[CLKID_FCLK_DIV7]	= &fclk_div7.hw,
> +		[CLKID_HIFI_PLL]	= &hifi_pll.hw,
> +		[NR_PLL_CLKS]		= NULL,
> +	},
> +	.num = NR_PLL_CLKS,
> +};
> +
> +static struct clk_regmap *const a1_pll_regmaps[] = {
> +	&fixed_pll_dco,
> +	&fixed_pll,
> +	&fclk_div2,
> +	&fclk_div3,
> +	&fclk_div5,
> +	&fclk_div7,
> +	&hifi_pll,
> +};
> +
> +static struct regmap_config a1_pll_regmap_cfg = {
> +	.reg_bits   = 32,
> +	.val_bits   = 32,
> +	.reg_stride = 4,
> +};
> +
> +static int meson_a1_pll_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct clk_hw *hw;
> +	void __iomem *base;
> +	struct regmap *map;
> +	int clkid, i, err;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return dev_err_probe(dev, PTR_ERR(base),
> +				     "can't ioremap resource\n");
> +
> +	map = devm_regmap_init_mmio(dev, base, &a1_pll_regmap_cfg);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map),
> +				     "can't init regmap mmio region\n");
> +
> +	/* Populate regmap for the regmap backed clocks */
> +	for (i = 0; i < ARRAY_SIZE(a1_pll_regmaps); i++)
> +		a1_pll_regmaps[i]->map = map;
> +
> +	for (clkid = 0; clkid < a1_pll_hw_onecell_data.num; clkid++) {
> +		hw = a1_pll_hw_onecell_data.hws[clkid];
> +		err = devm_clk_hw_register(dev, hw);
> +		if (err)
> +			return dev_err_probe(dev, err,
> +					     "clock registration failed\n");
> +	}
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					   &a1_pll_hw_onecell_data);
> +}
> +
> +#ifdef CONFIG_OF

This config is selected by ARM64 which this driver depends on

> +static const struct of_device_id a1_pll_clkc_match_table[] = {
> +	{ .compatible = "amlogic,a1-pll-clkc", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, a1_pll_clkc_match_table);
> +#endif /* CONFIG_OF */
> +
> +static struct platform_driver a1_pll_clkc_driver = {
> +	.probe = meson_a1_pll_probe,
> +	.driver = {
> +		.name = "a1-pll-clkc",
> +		.of_match_table = of_match_ptr(a1_pll_clkc_match_table),
> +	},
> +};
> +
> +module_platform_driver(a1_pll_clkc_driver);
> +MODULE_AUTHOR("Jian Hu <[e-mail]>");
> +MODULE_AUTHOR("Dmitry Rokosov <[e-mail]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/meson/a1-pll.h b/drivers/clk/meson/a1-pll.h
> new file mode 100644
> index 000000000000..de2eebce98af
> --- /dev/null
> +++ b/drivers/clk/meson/a1-pll.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Amlogic Meson-A1 PLL Clock Controller internals
> + *
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + * Author: Jian Hu <[e-mail]>
> + *
> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <[e-mail]>
> + */
> +
> +#ifndef __A1_PLL_H
> +#define __A1_PLL_H
> +
> +#include "clk-pll.h"
> +
> +/* PLL register offset */
> +#define ANACTRL_FIXPLL_CTRL0	0x0
> +#define ANACTRL_FIXPLL_CTRL1	0x4
> +#define ANACTRL_FIXPLL_STS	0x14
> +#define ANACTRL_HIFIPLL_CTRL0	0xc0
> +#define ANACTRL_HIFIPLL_CTRL1	0xc4
> +#define ANACTRL_HIFIPLL_CTRL2	0xc8
> +#define ANACTRL_HIFIPLL_CTRL3	0xcc
> +#define ANACTRL_HIFIPLL_CTRL4	0xd0
> +#define ANACTRL_HIFIPLL_STS	0xd4
> +
> +/*
> + * CLKID index values
> + *
> + * These indices are entirely contrived and do not map onto the hardware.
> + * It has now been decided to expose everything by default in the DT header:
> + * include/dt-bindings/clock/a1-pll-clkc.h. Only the clocks ids we don't want
> + * to expose, such as the internal muxes and dividers of composite clocks,
> + * will remain defined here.
> + */
> +#define CLKID_FIXED_PLL_DCO	0
> +#define CLKID_FCLK_DIV2_DIV	2
> +#define CLKID_FCLK_DIV3_DIV	3
> +#define CLKID_FCLK_DIV5_DIV	4
> +#define CLKID_FCLK_DIV7_DIV	5
> +#define NR_PLL_CLKS		11
> +
> +/* include the CLKIDs that have been made part of the DT binding */
> +#include <dt-bindings/clock/a1-pll-clkc.h>
> +
> +#endif /* __A1_PLL_H */

Dmitry Rokosov March 6, 2023, 8:05 p.m. UTC | #3

On Mon, Mar 06, 2023 at 12:17:23PM +0100, Jerome Brunet wrote:
> 
> On Wed 01 Mar 2023 at 21:37, Dmitry Rokosov <[e-mail]> wrote:
> 
> > Introduce PLL clock controller for Amlogic A1 SoC family.
> >
> > Signed-off-by: Jian Hu <[e-mail]>
> > Signed-off-by: Dmitry Rokosov <[e-mail]>
> > ---
> >  drivers/clk/meson/Kconfig  |  10 +
> >  drivers/clk/meson/Makefile |   1 +
> >  drivers/clk/meson/a1-pll.c | 365 +++++++++++++++++++++++++++++++++++++
> >  drivers/clk/meson/a1-pll.h |  47 +++++
> >  4 files changed, 423 insertions(+)
> >  create mode 100644 drivers/clk/meson/a1-pll.c
> >  create mode 100644 drivers/clk/meson/a1-pll.h
> >

[...]

> > diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
> > new file mode 100644
> > index 000000000000..c565f9b2a8dd
> > --- /dev/null
> > +++ b/drivers/clk/meson/a1-pll.c
> > @@ -0,0 +1,365 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> > + * Author: Jian Hu <[e-mail]>
> > + *
> > + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> > + * Author: Dmitry Rokosov <[e-mail]>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include "meson-a1-clkc.h"
> 
> As pointed out by the kernel robot, there is a problem here
> 

My fault. Really sorry for that.

[...]

> > +static struct clk_regmap fixed_pll = {
> > +	.data = &(struct clk_regmap_gate_data){
> > +		.offset = ANACTRL_FIXPLL_CTRL0,
> > +		.bit_idx = 20,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "fixed_pll",
> > +		.ops = &clk_regmap_gate_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&fixed_pll_dco.hw
> > +		},
> > +		.num_parents = 1,
> > +		/*
> > +		 * It is enough that the fdiv leaf has critical flag,
> > +		 * No critical or unused flag here.
> > +		 */
> 
> The comment is not useful
> 

OK

> > +	},
> > +};
> > +
> > +static const struct pll_mult_range hifi_pll_mult_range = {
> > +	.min = 32,
> > +	.max = 64,
> > +};
> > +
> > +static const struct reg_sequence hifi_init_regs[] = {
> > +	{ .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
> > +	{ .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
> > +	{ .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
> > +	{ .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
> > +	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
> 
> This last poke should not bits otherwise handled by parms.
> This is a rate init in disguise.
> 

I believe, you are talking about hifi_pll clk_regmap conflicts with
hifi_init_regs. The above init sequence shouldn't affect pll regmap setup,
it doesn't touch them (we assume that default bit values are all zero):

    .en = {
        .reg_off = ANACTRL_HIFIPLL_CTRL0,
        .shift   = 28,
        .width   = 1,
    },
    // init_value = 0x01f18440
    // en_mask    = 0x10000000

    .m = {
        .reg_off = ANACTRL_HIFIPLL_CTRL0,
        .shift   = 0,
        .width   = 8,
    },
    // init_value = 0x01f18440
    // m_mask     = 0x0000000f

    .n = {
        .reg_off = ANACTRL_HIFIPLL_CTRL0,
        .shift   = 10,
        .width   = 5,
    },
    // init_value = 0x01f18440
    // n_mask     = 0x00007c00
                           ^
                    oops, one overlap
                    but why we can't set init value for pre_sel?

    .frac = {
        .reg_off = ANACTRL_HIFIPLL_CTRL1,
        .shift   = 0,
        .width   = 19,
    },
    // init_value = 0x01800000
    // frac_mask  = 0x0007ffff

    .current_en = {
        .reg_off = ANACTRL_HIFIPLL_CTRL0,
        .shift   = 26,
        .width   = 1,
    },
    // init_value      = 0x01f18440
    // current_en_mask = 0x04000000

    .l_detect = {
        .reg_off = ANACTRL_HIFIPLL_CTRL2,
        .shift   = 6,
        .width   = 1,
    },
    // init_value    = 0x00001100
    // l_detect_mask = 0x00000040

> > +};
> > +
> > +static struct clk_regmap hifi_pll = {
> > +	.data = &(struct meson_clk_pll_data){
> > +		.en = {
> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> > +			.shift   = 28,
> > +			.width   = 1,
> > +		},
> > +		.m = {
> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> > +			.shift   = 0,
> > +			.width   = 8,
> > +		},
> > +		.n = {
> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> > +			.shift   = 10,
> > +			.width   = 5,
> > +		},
> > +		.frac = {
> > +			.reg_off = ANACTRL_HIFIPLL_CTRL1,
> > +			.shift   = 0,
> > +			.width   = 19,
> > +		},
> > +		.l = {
> > +			.reg_off = ANACTRL_HIFIPLL_STS,
> > +			.shift   = 31,
> > +			.width   = 1,
> > +		},
> > +		.current_en = {
> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> > +			.shift   = 26,
> > +			.width   = 1,
> > +		},
> > +		.l_detect = {
> 
> What is this ?
> 

Lock detection module.

This is IP module included to new PLL power-on sequence. From clk-pll.c
patchset:

/*
 * Compared with the previous SoCs, self-adaption current module
 * is newly added for A1, keep the new power-on sequence to enable the
 * PLL. The sequence is:
 * 1. enable the pll, delay for 10us
 * 2. enable the pll self-adaption current module, delay for 40us
 * 3. enable the lock detect module
 */

[...]

> > +static struct clk_regmap fclk_div3 = {
> > +	.data = &(struct clk_regmap_gate_data){
> > +		.offset = ANACTRL_FIXPLL_CTRL0,
> > +		.bit_idx = 22,
> > +	},
> > +	.hw.init = &(struct clk_init_data){
> > +		.name = "fclk_div3",
> > +		.ops = &clk_regmap_gate_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&fclk_div3_div.hw
> > +		},
> > +		.num_parents = 1,
> > +		/*
> > +		 * This clock is used by APB bus which is set in boot ROM code
> > +		 * and is required by the platform to operate correctly.
> > +		 * About critical, refer to fclk_div2.
> 
> This last line is not useful. Same for other occurences
> 

Good point. Copy-paste detected :-)

[...]

> > +static int meson_a1_pll_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct clk_hw *hw;
> > +	void __iomem *base;
> > +	struct regmap *map;
> > +	int clkid, i, err;
> > +
> > +	base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(base))
> > +		return dev_err_probe(dev, PTR_ERR(base),
> > +				     "can't ioremap resource\n");
> > +
> > +	map = devm_regmap_init_mmio(dev, base, &a1_pll_regmap_cfg);
> > +	if (IS_ERR(map))
> > +		return dev_err_probe(dev, PTR_ERR(map),
> > +				     "can't init regmap mmio region\n");
> > +
> > +	/* Populate regmap for the regmap backed clocks */
> > +	for (i = 0; i < ARRAY_SIZE(a1_pll_regmaps); i++)
> > +		a1_pll_regmaps[i]->map = map;
> > +
> > +	for (clkid = 0; clkid < a1_pll_hw_onecell_data.num; clkid++) {
> > +		hw = a1_pll_hw_onecell_data.hws[clkid];
> > +		err = devm_clk_hw_register(dev, hw);
> > +		if (err)
> > +			return dev_err_probe(dev, err,
> > +					     "clock registration failed\n");
> > +	}
> > +
> > +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> > +					   &a1_pll_hw_onecell_data);
> > +}
> > +
> > +#ifdef CONFIG_OF
> 
> This config is selected by ARM64 which this driver depends on
> 

Make sense, thanks a lot!

[...]

Jerome Brunet March 9, 2023, 2:20 p.m. UTC | #4

On Mon 06 Mar 2023 at 23:05, Dmitry Rokosov <[e-mail]> wrote:

> On Mon, Mar 06, 2023 at 12:17:23PM +0100, Jerome Brunet wrote:
>> 
>> On Wed 01 Mar 2023 at 21:37, Dmitry Rokosov <[e-mail]> wrote:
>> 
>> > Introduce PLL clock controller for Amlogic A1 SoC family.
>> >
>> > Signed-off-by: Jian Hu <[e-mail]>
>> > Signed-off-by: Dmitry Rokosov <[e-mail]>
>> > ---
>> >  drivers/clk/meson/Kconfig  |  10 +
>> >  drivers/clk/meson/Makefile |   1 +
>> >  drivers/clk/meson/a1-pll.c | 365 +++++++++++++++++++++++++++++++++++++
>> >  drivers/clk/meson/a1-pll.h |  47 +++++
>> >  4 files changed, 423 insertions(+)
>> >  create mode 100644 drivers/clk/meson/a1-pll.c
>> >  create mode 100644 drivers/clk/meson/a1-pll.h
>> >
>
> [...]
>
>> > diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
>> > new file mode 100644
>> > index 000000000000..c565f9b2a8dd
>> > --- /dev/null
>> > +++ b/drivers/clk/meson/a1-pll.c
>> > @@ -0,0 +1,365 @@
>> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> > +/*
>> > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> > + * Author: Jian Hu <[e-mail]>
>> > + *
>> > + * Copyright (c) 2023, SberDevices. All Rights Reserved.
>> > + * Author: Dmitry Rokosov <[e-mail]>
>> > + */
>> > +
>> > +#include <linux/clk-provider.h>
>> > +#include <linux/of_device.h>
>> > +#include <linux/platform_device.h>
>> > +#include "meson-a1-clkc.h"
>> 
>> As pointed out by the kernel robot, there is a problem here
>> 
>
> My fault. Really sorry for that.
>
> [...]
>
>> > +static struct clk_regmap fixed_pll = {
>> > +	.data = &(struct clk_regmap_gate_data){
>> > +		.offset = ANACTRL_FIXPLL_CTRL0,
>> > +		.bit_idx = 20,
>> > +	},
>> > +	.hw.init = &(struct clk_init_data) {
>> > +		.name = "fixed_pll",
>> > +		.ops = &clk_regmap_gate_ops,
>> > +		.parent_hws = (const struct clk_hw *[]) {
>> > +			&fixed_pll_dco.hw
>> > +		},
>> > +		.num_parents = 1,
>> > +		/*
>> > +		 * It is enough that the fdiv leaf has critical flag,
>> > +		 * No critical or unused flag here.
>> > +		 */
>> 
>> The comment is not useful
>> 
>
> OK
>
>> > +	},
>> > +};
>> > +
>> > +static const struct pll_mult_range hifi_pll_mult_range = {
>> > +	.min = 32,
>> > +	.max = 64,
>> > +};
>> > +
>> > +static const struct reg_sequence hifi_init_regs[] = {
>> > +	{ .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
>> > +	{ .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>> > +	{ .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
>> > +	{ .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
>> > +	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
>> 
>> This last poke should not bits otherwise handled by parms.
>> This is a rate init in disguise.
>> 
>
> I believe, you are talking about hifi_pll clk_regmap conflicts with
> hifi_init_regs. The above init sequence shouldn't affect pll regmap setup,
> it doesn't touch them (we assume that default bit values are all zero):
>
>     .en = {
>         .reg_off = ANACTRL_HIFIPLL_CTRL0,
>         .shift   = 28,
>         .width   = 1,
>     },
>     // init_value = 0x01f18440
>     // en_mask    = 0x10000000
>
>     .m = {
>         .reg_off = ANACTRL_HIFIPLL_CTRL0,
>         .shift   = 0,
>         .width   = 8,
>     },
>     // init_value = 0x01f18440
>     // m_mask     = 0x0000000f

mask is 0xff with width 8

>
>     .n = {
>         .reg_off = ANACTRL_HIFIPLL_CTRL0,
>         .shift   = 10,
>         .width   = 5,
>     },
>     // init_value = 0x01f18440
>     // n_mask     = 0x00007c00
>                            ^
>                     oops, one overlap
>                     but why we can't set init value for pre_sel?
>
>     .frac = {
>         .reg_off = ANACTRL_HIFIPLL_CTRL1,
>         .shift   = 0,
>         .width   = 19,
>     },
>     // init_value = 0x01800000
>     // frac_mask  = 0x0007ffff
>
>     .current_en = {
>         .reg_off = ANACTRL_HIFIPLL_CTRL0,
>         .shift   = 26,
>         .width   = 1,
>     },
>     // init_value      = 0x01f18440
>     // current_en_mask = 0x04000000
>
>     .l_detect = {
>         .reg_off = ANACTRL_HIFIPLL_CTRL2,
>         .shift   = 6,
>         .width   = 1,
>     },
>     // init_value    = 0x00001100
>     // l_detect_mask = 0x00000040
>
>> > +};
>> > +
>> > +static struct clk_regmap hifi_pll = {
>> > +	.data = &(struct meson_clk_pll_data){
>> > +		.en = {
>> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
>> > +			.shift   = 28,
>> > +			.width   = 1,
>> > +		},
>> > +		.m = {
>> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
>> > +			.shift   = 0,
>> > +			.width   = 8,
>> > +		},
>> > +		.n = {
>> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
>> > +			.shift   = 10,
>> > +			.width   = 5,
>> > +		},
>> > +		.frac = {
>> > +			.reg_off = ANACTRL_HIFIPLL_CTRL1,
>> > +			.shift   = 0,
>> > +			.width   = 19,
>> > +		},
>> > +		.l = {
>> > +			.reg_off = ANACTRL_HIFIPLL_STS,
>> > +			.shift   = 31,
>> > +			.width   = 1,
>> > +		},
>> > +		.current_en = {
>> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
>> > +			.shift   = 26,
>> > +			.width   = 1,
>> > +		},
>> > +		.l_detect = {
>> 
>> What is this ?
>> 
>
> Lock detection module.
>
> This is IP module included to new PLL power-on sequence. From clk-pll.c
> patchset:
>
> /*
>  * Compared with the previous SoCs, self-adaption current module
>  * is newly added for A1, keep the new power-on sequence to enable the
>  * PLL. The sequence is:
>  * 1. enable the pll, delay for 10us
>  * 2. enable the pll self-adaption current module, delay for 40us
>  * 3. enable the lock detect module
>  */

Ok. I missed this is the PLL driver

>
> [...]
>
>> > +static struct clk_regmap fclk_div3 = {
>> > +	.data = &(struct clk_regmap_gate_data){
>> > +		.offset = ANACTRL_FIXPLL_CTRL0,
>> > +		.bit_idx = 22,
>> > +	},
>> > +	.hw.init = &(struct clk_init_data){
>> > +		.name = "fclk_div3",
>> > +		.ops = &clk_regmap_gate_ops,
>> > +		.parent_hws = (const struct clk_hw *[]) {
>> > +			&fclk_div3_div.hw
>> > +		},
>> > +		.num_parents = 1,
>> > +		/*
>> > +		 * This clock is used by APB bus which is set in boot ROM code
>> > +		 * and is required by the platform to operate correctly.
>> > +		 * About critical, refer to fclk_div2.
>> 
>> This last line is not useful. Same for other occurences
>> 
>
> Good point. Copy-paste detected :-)
>
> [...]
>
>> > +static int meson_a1_pll_probe(struct platform_device *pdev)
>> > +{
>> > +	struct device *dev = &pdev->dev;
>> > +	struct clk_hw *hw;
>> > +	void __iomem *base;
>> > +	struct regmap *map;
>> > +	int clkid, i, err;
>> > +
>> > +	base = devm_platform_ioremap_resource(pdev, 0);
>> > +	if (IS_ERR(base))
>> > +		return dev_err_probe(dev, PTR_ERR(base),
>> > +				     "can't ioremap resource\n");
>> > +
>> > +	map = devm_regmap_init_mmio(dev, base, &a1_pll_regmap_cfg);
>> > +	if (IS_ERR(map))
>> > +		return dev_err_probe(dev, PTR_ERR(map),
>> > +				     "can't init regmap mmio region\n");
>> > +
>> > +	/* Populate regmap for the regmap backed clocks */
>> > +	for (i = 0; i < ARRAY_SIZE(a1_pll_regmaps); i++)
>> > +		a1_pll_regmaps[i]->map = map;
>> > +
>> > +	for (clkid = 0; clkid < a1_pll_hw_onecell_data.num; clkid++) {
>> > +		hw = a1_pll_hw_onecell_data.hws[clkid];
>> > +		err = devm_clk_hw_register(dev, hw);
>> > +		if (err)
>> > +			return dev_err_probe(dev, err,
>> > +					     "clock registration failed\n");
>> > +	}
>> > +
>> > +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> > +					   &a1_pll_hw_onecell_data);
>> > +}
>> > +
>> > +#ifdef CONFIG_OF
>> 
>> This config is selected by ARM64 which this driver depends on
>> 
>
> Make sense, thanks a lot!
>
> [...]

Dmitry Rokosov March 9, 2023, 6:28 p.m. UTC | #5

On Thu, Mar 09, 2023 at 03:20:23PM +0100, Jerome Brunet wrote:
> 
> On Mon 06 Mar 2023 at 23:05, Dmitry Rokosov <[e-mail]> wrote:
> 
> > On Mon, Mar 06, 2023 at 12:17:23PM +0100, Jerome Brunet wrote:
> >> 
> >> On Wed 01 Mar 2023 at 21:37, Dmitry Rokosov <[e-mail]> wrote:
> >> 
> >> > Introduce PLL clock controller for Amlogic A1 SoC family.
> >> >
> >> > Signed-off-by: Jian Hu <[e-mail]>
> >> > Signed-off-by: Dmitry Rokosov <[e-mail]>

[...]

> >> > +	},
> >> > +};
> >> > +
> >> > +static const struct pll_mult_range hifi_pll_mult_range = {
> >> > +	.min = 32,
> >> > +	.max = 64,
> >> > +};
> >> > +
> >> > +static const struct reg_sequence hifi_init_regs[] = {
> >> > +	{ .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
> >> > +	{ .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
> >> > +	{ .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
> >> > +	{ .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
> >> > +	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
> >> 
> >> This last poke should not bits otherwise handled by parms.
> >> This is a rate init in disguise.
> >> 
> >
> > I believe, you are talking about hifi_pll clk_regmap conflicts with
> > hifi_init_regs. The above init sequence shouldn't affect pll regmap setup,
> > it doesn't touch them (we assume that default bit values are all zero):
> >
> >     .en = {
> >         .reg_off = ANACTRL_HIFIPLL_CTRL0,
> >         .shift   = 28,
> >         .width   = 1,
> >     },
> >     // init_value = 0x01f18440
> >     // en_mask    = 0x10000000
> >
> >     .m = {
> >         .reg_off = ANACTRL_HIFIPLL_CTRL0,
> >         .shift   = 0,
> >         .width   = 8,
> >     },
> >     // init_value = 0x01f18440
> >     // m_mask     = 0x0000000f
> 
> mask is 0xff with width 8
> 

Ah, you're right. Anyway, I think this is just init value and it's okay
to set it during initialization and rewrite after in parameter
propagation stage.

> >
> >     .n = {
> >         .reg_off = ANACTRL_HIFIPLL_CTRL0,
> >         .shift   = 10,
> >         .width   = 5,
> >     },
> >     // init_value = 0x01f18440
> >     // n_mask     = 0x00007c00
> >                            ^
> >                     oops, one overlap
> >                     but why we can't set init value for pre_sel?
> >
> >     .frac = {
> >         .reg_off = ANACTRL_HIFIPLL_CTRL1,
> >         .shift   = 0,
> >         .width   = 19,
> >     },
> >     // init_value = 0x01800000
> >     // frac_mask  = 0x0007ffff
> >
> >     .current_en = {
> >         .reg_off = ANACTRL_HIFIPLL_CTRL0,
> >         .shift   = 26,
> >         .width   = 1,
> >     },
> >     // init_value      = 0x01f18440
> >     // current_en_mask = 0x04000000
> >
> >     .l_detect = {
> >         .reg_off = ANACTRL_HIFIPLL_CTRL2,
> >         .shift   = 6,
> >         .width   = 1,
> >     },
> >     // init_value    = 0x00001100
> >     // l_detect_mask = 0x00000040
> >
> >> > +};
> >> > +
> >> > +static struct clk_regmap hifi_pll = {
> >> > +	.data = &(struct meson_clk_pll_data){
> >> > +		.en = {
> >> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> >> > +			.shift   = 28,
> >> > +			.width   = 1,
> >> > +		},
> >> > +		.m = {
> >> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> >> > +			.shift   = 0,
> >> > +			.width   = 8,
> >> > +		},
> >> > +		.n = {
> >> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> >> > +			.shift   = 10,
> >> > +			.width   = 5,
> >> > +		},
> >> > +		.frac = {
> >> > +			.reg_off = ANACTRL_HIFIPLL_CTRL1,
> >> > +			.shift   = 0,
> >> > +			.width   = 19,
> >> > +		},
> >> > +		.l = {
> >> > +			.reg_off = ANACTRL_HIFIPLL_STS,
> >> > +			.shift   = 31,
> >> > +			.width   = 1,
> >> > +		},
> >> > +		.current_en = {
> >> > +			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> >> > +			.shift   = 26,
> >> > +			.width   = 1,
> >> > +		},
> >> > +		.l_detect = {
> >> 
> >> What is this ?
> >> 
> >
> > Lock detection module.
> >
> > This is IP module included to new PLL power-on sequence. From clk-pll.c
> > patchset:
> >
> > /*
> >  * Compared with the previous SoCs, self-adaption current module
> >  * is newly added for A1, keep the new power-on sequence to enable the
> >  * PLL. The sequence is:
> >  * 1. enable the pll, delay for 10us
> >  * 2. enable the pll self-adaption current module, delay for 40us
> >  * 3. enable the lock detect module
> >  */
> 
> Ok. I missed this is the PLL driver
> 

No problem.

[...]

Jerome Brunet March 13, 2023, 9:18 a.m. UTC | #6

On Thu 09 Mar 2023 at 21:28, Dmitry Rokosov <[e-mail]> wrote:

>> >> 
>> >> This last poke should not bits otherwise handled by parms.
>> >> This is a rate init in disguise.
>> >> 
>> >
>> > I believe, you are talking about hifi_pll clk_regmap conflicts with
>> > hifi_init_regs. The above init sequence shouldn't affect pll regmap setup,
>> > it doesn't touch them (we assume that default bit values are all zero):
>> >
>> >     .en = {
>> >         .reg_off = ANACTRL_HIFIPLL_CTRL0,
>> >         .shift   = 28,
>> >         .width   = 1,
>> >     },
>> >     // init_value = 0x01f18440
>> >     // en_mask    = 0x10000000
>> >
>> >     .m = {
>> >         .reg_off = ANACTRL_HIFIPLL_CTRL0,
>> >         .shift   = 0,
>> >         .width   = 8,
>> >     },
>> >     // init_value = 0x01f18440
>> >     // m_mask     = 0x0000000f
>> 
>> mask is 0xff with width 8
>> 
>
> Ah, you're right. Anyway, I think this is just init value and it's okay
> to set it during initialization and rewrite after in parameter
> propagation stage.
>

... But the magic pokes are there only to initialize the unmanaged part
of the clock regs. I'd like it to be clear and stay that way.

So please, clear the managed fields from the initial poke table.

Dmitry Rokosov March 13, 2023, 10:25 a.m. UTC | #7

On Mon, Mar 13, 2023 at 10:18:02AM +0100, Jerome Brunet wrote:
> 
> On Thu 09 Mar 2023 at 21:28, Dmitry Rokosov <[e-mail]> wrote:
> 
> >> >> 
> >> >> This last poke should not bits otherwise handled by parms.
> >> >> This is a rate init in disguise.
> >> >> 
> >> >
> >> > I believe, you are talking about hifi_pll clk_regmap conflicts with
> >> > hifi_init_regs. The above init sequence shouldn't affect pll regmap setup,
> >> > it doesn't touch them (we assume that default bit values are all zero):
> >> >
> >> >     .en = {
> >> >         .reg_off = ANACTRL_HIFIPLL_CTRL0,
> >> >         .shift   = 28,
> >> >         .width   = 1,
> >> >     },
> >> >     // init_value = 0x01f18440
> >> >     // en_mask    = 0x10000000
> >> >
> >> >     .m = {
> >> >         .reg_off = ANACTRL_HIFIPLL_CTRL0,
> >> >         .shift   = 0,
> >> >         .width   = 8,
> >> >     },
> >> >     // init_value = 0x01f18440
> >> >     // m_mask     = 0x0000000f
> >> 
> >> mask is 0xff with width 8
> >> 
> >
> > Ah, you're right. Anyway, I think this is just init value and it's okay
> > to set it during initialization and rewrite after in parameter
> > propagation stage.
> >
> 
> ... But the magic pokes are there only to initialize the unmanaged part
> of the clock regs. I'd like it to be clear and stay that way.
> 
> So please, clear the managed fields from the initial poke table.

I've double checked hifi_pll clk. In the my current configuration no any
clks inherited from it. Therefore its 'enable_count' equals to 0.
And of course in the such situation the rate must be zeroed as well.
It means you are right at all. I'll remove pre_sel and fbkdiv hifi_pll
pre-setup in the next version.
Thank you for hunted down!

Patch

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index fc002c155bc3..f56da2a4b000 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -99,6 +99,16 @@  config COMMON_CLK_AXG_AUDIO
 	  Support for the audio clock controller on AmLogic A113D devices,
 	  aka axg, Say Y if you want audio subsystem to work.
 
+config COMMON_CLK_A1_PLL
+	tristate "Meson A1 SoC PLL controller support"
+	depends on ARM64
+	select COMMON_CLK_MESON_REGMAP
+	select COMMON_CLK_MESON_PLL
+	help
+	  Support for the PLL clock controller on Amlogic A113L based
+	  device, A1 SoC Family. Say Y if you want A1 PLL clock controller
+	  to work.
+
 config COMMON_CLK_G12A
 	tristate "G12 and SM1 SoC clock controllers support"
 	depends on ARM64
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 6eca2a406ee3..2f17f475a48f 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -16,6 +16,7 @@  obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
 
 obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
 obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
+obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
 obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
 obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
 obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
new file mode 100644
index 000000000000..c565f9b2a8dd
--- /dev/null
+++ b/drivers/clk/meson/a1-pll.c
@@ -0,0 +1,365 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ * Author: Jian Hu <[e-mail]>
+ *
+ * Copyright (c) 2023, SberDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <[e-mail]>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include "meson-a1-clkc.h"
+#include "a1-pll.h"
+#include "clk-regmap.h"
+
+static struct clk_regmap fixed_pll_dco = {
+	.data = &(struct meson_clk_pll_data){
+		.en = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 28,
+			.width   = 1,
+		},
+		.m = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 0,
+			.width   = 8,
+		},
+		.n = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 10,
+			.width   = 5,
+		},
+		.frac = {
+			.reg_off = ANACTRL_FIXPLL_CTRL1,
+			.shift   = 0,
+			.width   = 19,
+		},
+		.l = {
+			.reg_off = ANACTRL_FIXPLL_STS,
+			.shift   = 31,
+			.width   = 1,
+		},
+		.rst = {
+			.reg_off = ANACTRL_FIXPLL_CTRL0,
+			.shift   = 29,
+			.width   = 1,
+		},
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fixed_pll_dco",
+		.ops = &meson_clk_pll_ro_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "fixpll_in",
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap fixed_pll = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL0,
+		.bit_idx = 20,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "fixed_pll",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fixed_pll_dco.hw
+		},
+		.num_parents = 1,
+		/*
+		 * It is enough that the fdiv leaf has critical flag,
+		 * No critical or unused flag here.
+		 */
+	},
+};
+
+static const struct pll_mult_range hifi_pll_mult_range = {
+	.min = 32,
+	.max = 64,
+};
+
+static const struct reg_sequence hifi_init_regs[] = {
+	{ .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
+	{ .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
+};
+
+static struct clk_regmap hifi_pll = {
+	.data = &(struct meson_clk_pll_data){
+		.en = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 28,
+			.width   = 1,
+		},
+		.m = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 0,
+			.width   = 8,
+		},
+		.n = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 10,
+			.width   = 5,
+		},
+		.frac = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL1,
+			.shift   = 0,
+			.width   = 19,
+		},
+		.l = {
+			.reg_off = ANACTRL_HIFIPLL_STS,
+			.shift   = 31,
+			.width   = 1,
+		},
+		.current_en = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL0,
+			.shift   = 26,
+			.width   = 1,
+		},
+		.l_detect = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL2,
+			.shift   = 6,
+			.width   = 1,
+		},
+		.range = &hifi_pll_mult_range,
+		.init_regs = hifi_init_regs,
+		.init_count = ARRAY_SIZE(hifi_init_regs),
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "hifi_pll",
+		.ops = &meson_clk_pll_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "hifipll_in",
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_fixed_factor fclk_div2_div = {
+	.mult = 1,
+	.div = 2,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div2_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fixed_pll.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap fclk_div2 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL0,
+		.bit_idx = 21,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div2",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fclk_div2_div.hw
+		},
+		.num_parents = 1,
+		/*
+		 * This clock is used by DDR clock in BL2 firmware
+		 * and is required by the platform to operate correctly.
+		 * Until the following condition are met, we need this clock to
+		 * be marked as critical:
+		 * a) Mark the clock used by a firmware resource, if possible
+		 * b) CCF has a clock hand-off mechanism to make the sure the
+		 *    clock stays on until the proper driver comes along
+		 */
+		.flags = CLK_IS_CRITICAL,
+	},
+};
+
+static struct clk_fixed_factor fclk_div3_div = {
+	.mult = 1,
+	.div = 3,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div3_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fixed_pll.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap fclk_div3 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL0,
+		.bit_idx = 22,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div3",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fclk_div3_div.hw
+		},
+		.num_parents = 1,
+		/*
+		 * This clock is used by APB bus which is set in boot ROM code
+		 * and is required by the platform to operate correctly.
+		 * About critical, refer to fclk_div2.
+		 */
+		.flags = CLK_IS_CRITICAL,
+	},
+};
+
+static struct clk_fixed_factor fclk_div5_div = {
+	.mult = 1,
+	.div = 5,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div5_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fixed_pll.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap fclk_div5 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL0,
+		.bit_idx = 23,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div5",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fclk_div5_div.hw
+		},
+		.num_parents = 1,
+		/*
+		 * This clock is used by AXI bus which setted in Romcode
+		 * and is required by the platform to operate correctly.
+		 * About critical, refer to fclk_div2.
+		 */
+		.flags = CLK_IS_CRITICAL,
+	},
+};
+
+static struct clk_fixed_factor fclk_div7_div = {
+	.mult = 1,
+	.div = 7,
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div7_div",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fixed_pll.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap fclk_div7 = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = ANACTRL_FIXPLL_CTRL0,
+		.bit_idx = 24,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "fclk_div7",
+		.ops = &clk_regmap_gate_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&fclk_div7_div.hw
+		},
+		.num_parents = 1,
+	},
+};
+
+/* Array of all clocks provided by this provider */
+static struct clk_hw_onecell_data a1_pll_hw_onecell_data = {
+	.hws = {
+		[CLKID_FIXED_PLL_DCO]	= &fixed_pll_dco.hw,
+		[CLKID_FIXED_PLL]	= &fixed_pll.hw,
+		[CLKID_FCLK_DIV2_DIV]	= &fclk_div2_div.hw,
+		[CLKID_FCLK_DIV3_DIV]	= &fclk_div3_div.hw,
+		[CLKID_FCLK_DIV5_DIV]	= &fclk_div5_div.hw,
+		[CLKID_FCLK_DIV7_DIV]	= &fclk_div7_div.hw,
+		[CLKID_FCLK_DIV2]	= &fclk_div2.hw,
+		[CLKID_FCLK_DIV3]	= &fclk_div3.hw,
+		[CLKID_FCLK_DIV5]	= &fclk_div5.hw,
+		[CLKID_FCLK_DIV7]	= &fclk_div7.hw,
+		[CLKID_HIFI_PLL]	= &hifi_pll.hw,
+		[NR_PLL_CLKS]		= NULL,
+	},
+	.num = NR_PLL_CLKS,
+};
+
+static struct clk_regmap *const a1_pll_regmaps[] = {
+	&fixed_pll_dco,
+	&fixed_pll,
+	&fclk_div2,
+	&fclk_div3,
+	&fclk_div5,
+	&fclk_div7,
+	&hifi_pll,
+};
+
+static struct regmap_config a1_pll_regmap_cfg = {
+	.reg_bits   = 32,
+	.val_bits   = 32,
+	.reg_stride = 4,
+};
+
+static int meson_a1_pll_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk_hw *hw;
+	void __iomem *base;
+	struct regmap *map;
+	int clkid, i, err;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return dev_err_probe(dev, PTR_ERR(base),
+				     "can't ioremap resource\n");
+
+	map = devm_regmap_init_mmio(dev, base, &a1_pll_regmap_cfg);
+	if (IS_ERR(map))
+		return dev_err_probe(dev, PTR_ERR(map),
+				     "can't init regmap mmio region\n");
+
+	/* Populate regmap for the regmap backed clocks */
+	for (i = 0; i < ARRAY_SIZE(a1_pll_regmaps); i++)
+		a1_pll_regmaps[i]->map = map;
+
+	for (clkid = 0; clkid < a1_pll_hw_onecell_data.num; clkid++) {
+		hw = a1_pll_hw_onecell_data.hws[clkid];
+		err = devm_clk_hw_register(dev, hw);
+		if (err)
+			return dev_err_probe(dev, err,
+					     "clock registration failed\n");
+	}
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   &a1_pll_hw_onecell_data);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id a1_pll_clkc_match_table[] = {
+	{ .compatible = "amlogic,a1-pll-clkc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, a1_pll_clkc_match_table);
+#endif /* CONFIG_OF */
+
+static struct platform_driver a1_pll_clkc_driver = {
+	.probe = meson_a1_pll_probe,
+	.driver = {
+		.name = "a1-pll-clkc",
+		.of_match_table = of_match_ptr(a1_pll_clkc_match_table),
+	},
+};
+
+module_platform_driver(a1_pll_clkc_driver);
+MODULE_AUTHOR("Jian Hu <[e-mail]>");
+MODULE_AUTHOR("Dmitry Rokosov <[e-mail]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/clk/meson/a1-pll.h b/drivers/clk/meson/a1-pll.h
new file mode 100644
index 000000000000..de2eebce98af
--- /dev/null
+++ b/drivers/clk/meson/a1-pll.h
@@ -0,0 +1,47 @@ 
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Amlogic Meson-A1 PLL Clock Controller internals
+ *
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ * Author: Jian Hu <[e-mail]>
+ *
+ * Copyright (c) 2023, SberDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <[e-mail]>
+ */
+
+#ifndef __A1_PLL_H
+#define __A1_PLL_H
+
+#include "clk-pll.h"
+
+/* PLL register offset */
+#define ANACTRL_FIXPLL_CTRL0	0x0
+#define ANACTRL_FIXPLL_CTRL1	0x4
+#define ANACTRL_FIXPLL_STS	0x14
+#define ANACTRL_HIFIPLL_CTRL0	0xc0
+#define ANACTRL_HIFIPLL_CTRL1	0xc4
+#define ANACTRL_HIFIPLL_CTRL2	0xc8
+#define ANACTRL_HIFIPLL_CTRL3	0xcc
+#define ANACTRL_HIFIPLL_CTRL4	0xd0
+#define ANACTRL_HIFIPLL_STS	0xd4
+
+/*
+ * CLKID index values
+ *
+ * These indices are entirely contrived and do not map onto the hardware.
+ * It has now been decided to expose everything by default in the DT header:
+ * include/dt-bindings/clock/a1-pll-clkc.h. Only the clocks ids we don't want
+ * to expose, such as the internal muxes and dividers of composite clocks,
+ * will remain defined here.
+ */
+#define CLKID_FIXED_PLL_DCO	0
+#define CLKID_FCLK_DIV2_DIV	2
+#define CLKID_FCLK_DIV3_DIV	3
+#define CLKID_FCLK_DIV5_DIV	4
+#define CLKID_FCLK_DIV7_DIV	5
+#define NR_PLL_CLKS		11
+
+/* include the CLKIDs that have been made part of the DT binding */
+#include <dt-bindings/clock/a1-pll-clkc.h>
+
+#endif /* __A1_PLL_H */