[v8,07/11] clk: meson: a1: redesign Amlogic A1 PLL clock controller

Commit Message

Dmitry Rokosov Dec. 1, 2022, 10:56 p.m. UTC

Summary changes:
    - supported meson-a1-clkc common driver
    - inherited from the base clk-pll driver, implemented own version of
      init/enable/disable/enabled routines; rate calculating logic is
      fully the same
    - aligned CLKID-related definitions with CLKID list from order
      perspective to remove holes and permutations
    - corrected Kconfig dependencies and types
    - provided correct MODULE_AUTHORs() and MODULE_LICENSE()
    - optimized and fix up some clock relationships
    - removed unused register offset definitions (ANACTRL_* group)

Signed-off-by: Dmitry Rokosov <[e-mail]>
---
 drivers/clk/meson/Kconfig  |   5 +-
 drivers/clk/meson/a1-pll.c | 267 +++++++++++++++++++++++++------------
 drivers/clk/meson/a1-pll.h |  37 ++---
 3 files changed, 202 insertions(+), 107 deletions(-)

Comments

Jerome Brunet Dec. 2, 2022, 11:42 a.m. UTC | #1

On Fri 02 Dec 2022 at 01:56, Dmitry Rokosov <[e-mail]> wrote:

> Summary changes:
>     - supported meson-a1-clkc common driver
>     - inherited from the base clk-pll driver, implemented own version of
>       init/enable/disable/enabled routines; rate calculating logic is
>       fully the same
>     - aligned CLKID-related definitions with CLKID list from order
>       perspective to remove holes and permutations
>     - corrected Kconfig dependencies and types
>     - provided correct MODULE_AUTHORs() and MODULE_LICENSE()
>     - optimized and fix up some clock relationships
>     - removed unused register offset definitions (ANACTRL_* group)

This patch mix PLL stuff, factorization change, etc ...
In general, when your commit description is a list, it is a hint that
you are doing more than one thing in it. It is unlikely to be OK then

>
> Signed-off-by: Dmitry Rokosov <[e-mail]>
> ---
>  drivers/clk/meson/Kconfig  |   5 +-
>  drivers/clk/meson/a1-pll.c | 267 +++++++++++++++++++++++++------------
>  drivers/clk/meson/a1-pll.h |  37 ++---
>  3 files changed, 202 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 1c885541c3a9..deb273673ec1 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -104,10 +104,11 @@ config COMMON_CLK_AXG_AUDIO
>  	  aka axg, Say Y if you want audio subsystem to work.
>  
>  config COMMON_CLK_A1_PLL
> -	bool
> -	depends on ARCH_MESON
> +	tristate "Meson A1 SoC PLL controller support"
> +	depends on ARM64
>  	select COMMON_CLK_MESON_REGMAP
>  	select COMMON_CLK_MESON_PLL
> +	select COMMON_CLK_MESON_A1_CLKC
>  	help
>  	  Support for the PLL clock controller on Amlogic A113L device,
>  	  aka a1. Say Y if you want PLL to work.
> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
> index 69c1ca07d041..23487ca797b3 100644
> --- a/drivers/clk/meson/a1-pll.c
> +++ b/drivers/clk/meson/a1-pll.c
> @@ -2,15 +2,133 @@
>  /*
>   * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>   * Author: Jian Hu <[e-mail]>
> + *
> + * Copyright (c) 2022, 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-pll.h"
>  #include "clk-regmap.h"
>  
> +static inline
> +struct meson_a1_pll_data *meson_a1_pll_data(struct clk_regmap *clk)
> +{
> +	return (struct meson_a1_pll_data *)clk->data;
> +}
> +
> +static int meson_a1_pll_init(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
> +
> +	regmap_multi_reg_write(clk->map, pll->base.init_regs,
> +			       pll->base.init_count);
> +
> +	return 0;

Looks the the default init mostly

Looks like you are trying the handle the absence of the rst bit.
I'm pretty sure the hifi PLL of the SoC as one but you really don't want
to poke, this can be in the generic driver, with MESON_PARM_APPLICABLE()
test.

No need to redefine this

> +}
> +
> +static int meson_a1_pll_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
> +
> +	if (MESON_PARM_APPLICABLE(&pll->base.rst) &&
> +	    meson_parm_read(clk->map, &pll->base.rst))
> +		return 0;
> +
> +	if (!meson_parm_read(clk->map, &pll->base.en) ||
> +	    !meson_parm_read(clk->map, &pll->base.l))
> +		return 0;
> +

Same here, pretty sure rst is there and the generic function works but
if this update is required, it seems safe to do in the generic driver.

> +	return 1;
> +}
> +
> +static int meson_a1_pll_enable(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
> +
> +	/* Do nothing if the PLL is already enabled */
> +	if (clk_hw_is_enabled(hw))
> +		return 0;
> +
> +	/* Enable the pll */
> +	meson_parm_write(clk->map, &pll->base.en, 1);
> +
> +	/*
> +	 * 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
> +	 */
> +	usleep_range(10, 20);
> +	meson_parm_write(clk->map, &pll->current_en, 1);

this base.en vs current_en needs some explanation.

Again I think there is room to handle this through the pll driver

> +	usleep_range(40, 50);
> +
> +	meson_parm_write(clk->map, &pll->l_detect, 1);
> +	meson_parm_write(clk->map, &pll->l_detect, 0);
> +
> +	if (meson_clk_pll_wait_lock(hw))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static void meson_a1_pll_disable(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
> +
> +	/* Disable the pll */
> +	meson_parm_write(clk->map, &pll->base.en, 0);
> +
> +	/* Disable PLL internal self-adaption current module */
> +	meson_parm_write(clk->map, &pll->current_en, 0);
> +}
> +
> +/*
> + * A1 PLL clock controller driver is based on meson clk_pll driver,
> + * so some rate calculating routines are reused
> + */
> +static unsigned long meson_a1_pll_recalc_rate(struct clk_hw *hw,
> +					      unsigned long parent_rate)
> +{
> +	return meson_clk_pll_ops.recalc_rate(hw, parent_rate);
> +}
> +
> +static int meson_a1_pll_determine_rate(struct clk_hw *hw,
> +				       struct clk_rate_request *req)
> +{
> +	return meson_clk_pll_ops.determine_rate(hw, req);
> +}
> +
> +static int meson_a1_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long parent_rate)
> +{
> +	return meson_clk_pll_ops.set_rate(hw, rate, parent_rate);
> +}
> +

This really does not scale well

> +static const struct clk_ops meson_a1_pll_ops = {
> +	.init		= meson_a1_pll_init,
> +	.recalc_rate	= meson_a1_pll_recalc_rate,
> +	.determine_rate	= meson_a1_pll_determine_rate,
> +	.set_rate	= meson_a1_pll_set_rate,
> +	.is_enabled	= meson_a1_pll_is_enabled,
> +	.enable		= meson_a1_pll_enable,
> +	.disable	= meson_a1_pll_disable
> +};
> +
> +static const struct clk_ops meson_a1_pll_ro_ops = {
> +	.recalc_rate	= meson_a1_pll_recalc_rate,
> +	.is_enabled	= meson_a1_pll_is_enabled,
> +};
> +
>  static struct clk_regmap a1_fixed_pll_dco = {
>  	.data = &(struct meson_clk_pll_data){
>  		.en = {
> @@ -46,7 +164,7 @@ static struct clk_regmap a1_fixed_pll_dco = {
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "fixed_pll_dco",
> -		.ops = &meson_clk_pll_ro_ops,
> +		.ops = &meson_a1_pll_ro_ops,
>  		.parent_data = &(const struct clk_parent_data) {
>  			.fw_name = "xtal_fixpll",
>  		},
> @@ -87,31 +205,36 @@ static const struct reg_sequence a1_hifi_init_regs[] = {
>  };
>  
>  static struct clk_regmap a1_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,
> +	.data = &(struct meson_a1_pll_data){
> +		.base = {
> +			.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,
> +			},
> +			.range = &a1_hifi_pll_mult_range,
> +			.init_regs = a1_hifi_init_regs,
> +			.init_count = ARRAY_SIZE(a1_hifi_init_regs),
>  		},
>  		.current_en = {
>  			.reg_off = ANACTRL_HIFIPLL_CTRL0,
> @@ -123,13 +246,10 @@ static struct clk_regmap a1_hifi_pll = {
>  			.shift   = 6,
>  			.width   = 1,
>  		},
> -		.range = &a1_hifi_pll_mult_range,
> -		.init_regs = a1_hifi_init_regs,
> -		.init_count = ARRAY_SIZE(a1_hifi_init_regs),
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "hifi_pll",
> -		.ops = &meson_clk_pll_ops,
> +		.ops = &meson_a1_pll_ops,
>  		.parent_data = &(const struct clk_parent_data) {
>  			.fw_name = "xtal_hifipll",
>  		},
> @@ -276,15 +396,15 @@ static struct clk_hw_onecell_data a1_pll_hw_onecell_data = {
>  	.hws = {
>  		[CLKID_FIXED_PLL_DCO]		= &a1_fixed_pll_dco.hw,
>  		[CLKID_FIXED_PLL]		= &a1_fixed_pll.hw,
> -		[CLKID_HIFI_PLL]		= &a1_hifi_pll.hw,
> -		[CLKID_FCLK_DIV2]		= &a1_fclk_div2.hw,
> -		[CLKID_FCLK_DIV3]		= &a1_fclk_div3.hw,
> -		[CLKID_FCLK_DIV5]		= &a1_fclk_div5.hw,
> -		[CLKID_FCLK_DIV7]		= &a1_fclk_div7.hw,
>  		[CLKID_FCLK_DIV2_DIV]		= &a1_fclk_div2_div.hw,
>  		[CLKID_FCLK_DIV3_DIV]		= &a1_fclk_div3_div.hw,
>  		[CLKID_FCLK_DIV5_DIV]		= &a1_fclk_div5_div.hw,
>  		[CLKID_FCLK_DIV7_DIV]		= &a1_fclk_div7_div.hw,
> +		[CLKID_FCLK_DIV2]		= &a1_fclk_div2.hw,
> +		[CLKID_FCLK_DIV3]		= &a1_fclk_div3.hw,
> +		[CLKID_FCLK_DIV5]		= &a1_fclk_div5.hw,
> +		[CLKID_FCLK_DIV7]		= &a1_fclk_div7.hw,
> +		[CLKID_HIFI_PLL]		= &a1_hifi_pll.hw,

I get you are trying to do but keep the ID order here 

>  		[NR_PLL_CLKS]			= NULL,
>  	},
>  	.num = NR_PLL_CLKS,
> @@ -293,68 +413,39 @@ static struct clk_hw_onecell_data a1_pll_hw_onecell_data = {
>  static struct clk_regmap *const a1_pll_regmaps[] = {
>  	&a1_fixed_pll_dco,
>  	&a1_fixed_pll,
> -	&a1_hifi_pll,
>  	&a1_fclk_div2,
>  	&a1_fclk_div3,
>  	&a1_fclk_div5,
>  	&a1_fclk_div7,
> +	&a1_hifi_pll,

?

>  };
>  
> -static struct regmap_config clkc_regmap_config = {
> -	.reg_bits       = 32,
> -	.val_bits       = 32,
> -	.reg_stride     = 4,
> +static const struct meson_a1_clkc_data a1_pll_clkc __maybe_unused = {
> +	.hw = &a1_pll_hw_onecell_data,
> +	.regs = a1_pll_regmaps,
> +	.num_regs = ARRAY_SIZE(a1_pll_regmaps),
>  };
>  
> -static int meson_a1_pll_probe(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	struct resource *res;
> -	void __iomem *base;
> -	struct regmap *map;
> -	int ret, i;
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -	base = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(base))
> -		return PTR_ERR(base);
> -
> -	map = devm_regmap_init_mmio(dev, base, &clkc_regmap_config);
> -	if (IS_ERR(map))
> -		return PTR_ERR(map);
> -
> -	/* Populate regmap for the regmap backed clocks */
> -	for (i = 0; i < ARRAY_SIZE(a1_pll_regmaps); i++)
> -		a1_pll_regmaps[i]->map = map;
> -
> -	for (i = 0; i < a1_pll_hw_onecell_data.num; i++) {
> -		/* array might be sparse */
> -		if (!a1_pll_hw_onecell_data.hws[i])
> -			continue;
> -
> -		ret = devm_clk_hw_register(dev, a1_pll_hw_onecell_data.hws[i]);
> -		if (ret) {
> -			dev_err(dev, "Clock registration failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> -					   &a1_pll_hw_onecell_data);
> -}
> -
> -static const struct of_device_id clkc_match_table[] = {
> -	{ .compatible = "amlogic,a1-pll-clkc", },
> -	{}
> +#ifdef CONFIG_OF
> +static const struct of_device_id a1_pll_clkc_match_table[] = {
> +	{
> +		.compatible = "amlogic,a1-pll-clkc",
> +		.data = &a1_pll_clkc,
> +	},
> +	{},
>  };
> +MODULE_DEVICE_TABLE(of, a1_pll_clkc_match_table);
> +#endif /* CONFIG_OF */
>  
> -static struct platform_driver a1_pll_driver = {
> -	.probe		= meson_a1_pll_probe,
> -	.driver		= {
> -		.name	= "a1-pll-clkc",
> -		.of_match_table = clkc_match_table,
> +static struct platform_driver a1_pll_clkc_driver = {
> +	.probe = meson_a1_clkc_probe,
> +	.driver = {
> +		.name = "a1-pll-clkc",
> +		.of_match_table = of_match_ptr(a1_pll_clkc_match_table),
>  	},
>  };
>  
> -builtin_platform_driver(a1_pll_driver);
> +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
> index 8ded267061ad..2ff5a2042a97 100644
> --- a/drivers/clk/meson/a1-pll.h
> +++ b/drivers/clk/meson/a1-pll.h
> @@ -1,38 +1,29 @@
>  /* 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) 2022, 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_CTRL2		0x8
> -#define ANACTRL_FIXPLL_CTRL3		0xc
> -#define ANACTRL_FIXPLL_CTRL4		0x10
>  #define ANACTRL_FIXPLL_STS		0x14
> -#define ANACTRL_SYSPLL_CTRL0		0x80
> -#define ANACTRL_SYSPLL_CTRL1		0x84
> -#define ANACTRL_SYSPLL_CTRL2		0x88
> -#define ANACTRL_SYSPLL_CTRL3		0x8c
> -#define ANACTRL_SYSPLL_CTRL4		0x90
> -#define ANACTRL_SYSPLL_STS		0x94
>  #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
> -#define ANACTRL_AUDDDS_CTRL0		0x100
> -#define ANACTRL_AUDDDS_CTRL1		0x104
> -#define ANACTRL_AUDDDS_CTRL2		0x108
> -#define ANACTRL_AUDDDS_CTRL3		0x10c
> -#define ANACTRL_AUDDDS_CTRL4		0x110
> -#define ANACTRL_AUDDDS_STS		0x114
> -#define ANACTRL_MISCTOP_CTRL0		0x140
> -#define ANACTRL_POR_CNTL		0x188
>  
>  /*
>   * CLKID index values
> @@ -53,4 +44,16 @@
>  /* include the CLKIDs that have been made part of the DT binding */
>  #include <dt-bindings/clock/a1-pll-clkc.h>
>  
> +/**
> + * struct meson_a1_pll_data - A1 PLL state
> + * @base: Basic CLK PLL state
> + * @current_en: Enable or disable the PLL self-adaption current module
> + * @l_detect: Enable or disable the lock detect module
> + */
> +struct meson_a1_pll_data {
> +	struct meson_clk_pll_data base;
> +	struct parm current_en;
> +	struct parm l_detect;
> +};
> +
>  #endif /* __A1_PLL_H */

Dmitry Rokosov Dec. 2, 2022, 12:47 p.m. UTC | #2

On Fri, Dec 02, 2022 at 12:42:17PM +0100, Jerome Brunet wrote:
> 
> On Fri 02 Dec 2022 at 01:56, Dmitry Rokosov <[e-mail]> wrote:
> 
> > Summary changes:
> >     - supported meson-a1-clkc common driver
> >     - inherited from the base clk-pll driver, implemented own version of
> >       init/enable/disable/enabled routines; rate calculating logic is
> >       fully the same
> >     - aligned CLKID-related definitions with CLKID list from order
> >       perspective to remove holes and permutations
> >     - corrected Kconfig dependencies and types
> >     - provided correct MODULE_AUTHORs() and MODULE_LICENSE()
> >     - optimized and fix up some clock relationships
> >     - removed unused register offset definitions (ANACTRL_* group)
> 
> This patch mix PLL stuff, factorization change, etc ...
> In general, when your commit description is a list, it is a hint that
> you are doing more than one thing in it. It is unlikely to be OK then

It will be fixed by itself, when I'll squash patches.

> > +static int meson_a1_pll_init(struct clk_hw *hw)
> > +{
> > +	struct clk_regmap *clk = to_clk_regmap(hw);
> > +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
> > +
> > +	regmap_multi_reg_write(clk->map, pll->base.init_regs,
> > +			       pll->base.init_count);
> > +
> > +	return 0;
> 
> Looks the the default init mostly
> 
> Looks like you are trying the handle the absence of the rst bit.
> I'm pretty sure the hifi PLL of the SoC as one but you really don't want
> to poke, this can be in the generic driver, with MESON_PARM_APPLICABLE()
> test.
> 
> No need to redefine this
> 

I've redefined it, because in the previous v7 you mentioned that's
not acceptable to mix init/enable/disable sequences between a1 pll and clk
common pll driver:

https://lore.kernel.org/linux-amlogic/[e-mail]/

Hmmm, looks like I've made a mistake. You meant only enable/disable
callbacks...

Anyway, it doesn't matter to me. I think both approaches are okay:
    * clk-pll customization using MESON_PARM_APPLICABLE()
    * custom callbacks implementation for some clk_ops like implemented in
      this patchset.

Please advise what's the best from you point of view?

> > +}
> > +
> > +static int meson_a1_pll_is_enabled(struct clk_hw *hw)
> > +{
> > +	struct clk_regmap *clk = to_clk_regmap(hw);
> > +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
> > +
> > +	if (MESON_PARM_APPLICABLE(&pll->base.rst) &&
> > +	    meson_parm_read(clk->map, &pll->base.rst))
> > +		return 0;
> > +
> > +	if (!meson_parm_read(clk->map, &pll->base.en) ||
> > +	    !meson_parm_read(clk->map, &pll->base.l))
> > +		return 0;
> > +
> 
> Same here, pretty sure rst is there and the generic function works but
> if this update is required, it seems safe to do in the generic driver.

The same thing... in the v7 version you suggested to not touch clk-pll
driver.

https://lore.kernel.org/linux-amlogic/[e-mail]/

...

Jerome Brunet Dec. 2, 2022, 12:49 p.m. UTC | #3

On Fri 02 Dec 2022 at 15:47, Dmitry Rokosov <[e-mail]> wrote:

> On Fri, Dec 02, 2022 at 12:42:17PM +0100, Jerome Brunet wrote:
>> 
>> On Fri 02 Dec 2022 at 01:56, Dmitry Rokosov <[e-mail]> wrote:
>> 
>> > Summary changes:
>> >     - supported meson-a1-clkc common driver
>> >     - inherited from the base clk-pll driver, implemented own version of
>> >       init/enable/disable/enabled routines; rate calculating logic is
>> >       fully the same
>> >     - aligned CLKID-related definitions with CLKID list from order
>> >       perspective to remove holes and permutations
>> >     - corrected Kconfig dependencies and types
>> >     - provided correct MODULE_AUTHORs() and MODULE_LICENSE()
>> >     - optimized and fix up some clock relationships
>> >     - removed unused register offset definitions (ANACTRL_* group)
>> 
>> This patch mix PLL stuff, factorization change, etc ...
>> In general, when your commit description is a list, it is a hint that
>> you are doing more than one thing in it. It is unlikely to be OK then
>
> It will be fixed by itself, when I'll squash patches.
>
>> > +static int meson_a1_pll_init(struct clk_hw *hw)
>> > +{
>> > +	struct clk_regmap *clk = to_clk_regmap(hw);
>> > +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
>> > +
>> > +	regmap_multi_reg_write(clk->map, pll->base.init_regs,
>> > +			       pll->base.init_count);
>> > +
>> > +	return 0;
>> 
>> Looks the the default init mostly
>> 
>> Looks like you are trying the handle the absence of the rst bit.
>> I'm pretty sure the hifi PLL of the SoC as one but you really don't want
>> to poke, this can be in the generic driver, with MESON_PARM_APPLICABLE()
>> test.
>> 
>> No need to redefine this
>> 
>
> I've redefined it, because in the previous v7 you mentioned that's
> not acceptable to mix init/enable/disable sequences between a1 pll and clk
> common pll driver:
>
> https://lore.kernel.org/linux-amlogic/[e-mail]/
>
> Hmmm, looks like I've made a mistake. You meant only enable/disable
> callbacks...
>
> Anyway, it doesn't matter to me. I think both approaches are okay:
>     * clk-pll customization using MESON_PARM_APPLICABLE()
>     * custom callbacks implementation for some clk_ops like implemented in
>       this patchset.
>
> Please advise what's the best from you point of view?

It is a balance.

Everytime a new PLL comes up, it tends to treaded as a new ip block but,
most of the time after some digging and rework, we learn new things and
it ends up being compatible with the previous ones.

From what I see here
* You are trying to make rst optional, that's fine. Do it with
  MESON_PARM_APPLICABLE() in the main driver. Still I would recommend to
  thorougly for this bit. I'm pretty sure the hifi pll has one.

* You add a new feature called current self-adaptation.
  This can be made optional too in the enable sequence.
  I would not be surprised to find out more PLL have that, even on
  earlier SoC.

>
>> > +}
>> > +
>> > +static int meson_a1_pll_is_enabled(struct clk_hw *hw)
>> > +{
>> > +	struct clk_regmap *clk = to_clk_regmap(hw);
>> > +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
>> > +
>> > +	if (MESON_PARM_APPLICABLE(&pll->base.rst) &&
>> > +	    meson_parm_read(clk->map, &pll->base.rst))
>> > +		return 0;
>> > +
>> > +	if (!meson_parm_read(clk->map, &pll->base.en) ||
>> > +	    !meson_parm_read(clk->map, &pll->base.l))
>> > +		return 0;
>> > +
>> 
>> Same here, pretty sure rst is there and the generic function works but
>> if this update is required, it seems safe to do in the generic driver.
>
> The same thing... in the v7 version you suggested to not touch clk-pll
> driver.
>
> https://lore.kernel.org/linux-amlogic/[e-mail]/
>
> ...

Dmitry Rokosov Dec. 2, 2022, 6:20 p.m. UTC | #4

...

> >> > +static int meson_a1_pll_init(struct clk_hw *hw)
> >> > +{
> >> > +	struct clk_regmap *clk = to_clk_regmap(hw);
> >> > +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
> >> > +
> >> > +	regmap_multi_reg_write(clk->map, pll->base.init_regs,
> >> > +			       pll->base.init_count);
> >> > +
> >> > +	return 0;
> >> 
> >> Looks the the default init mostly
> >> 
> >> Looks like you are trying the handle the absence of the rst bit.
> >> I'm pretty sure the hifi PLL of the SoC as one but you really don't want
> >> to poke, this can be in the generic driver, with MESON_PARM_APPLICABLE()
> >> test.
> >> 
> >> No need to redefine this
> >> 
> >
> > I've redefined it, because in the previous v7 you mentioned that's
> > not acceptable to mix init/enable/disable sequences between a1 pll and clk
> > common pll driver:
> >
> > https://lore.kernel.org/linux-amlogic/[e-mail]/
> >
> > Hmmm, looks like I've made a mistake. You meant only enable/disable
> > callbacks...
> >
> > Anyway, it doesn't matter to me. I think both approaches are okay:
> >     * clk-pll customization using MESON_PARM_APPLICABLE()
> >     * custom callbacks implementation for some clk_ops like implemented in
> >       this patchset.
> >
> > Please advise what's the best from you point of view?
> 
> It is a balance.
> 
> Everytime a new PLL comes up, it tends to treaded as a new ip block but,
> most of the time after some digging and rework, we learn new things and
> it ends up being compatible with the previous ones.
> 
> From what I see here
> * You are trying to make rst optional, that's fine. Do it with
>   MESON_PARM_APPLICABLE() in the main driver. Still I would recommend to
>   thorougly for this bit. I'm pretty sure the hifi pll has one.
> 
> * You add a new feature called current self-adaptation.
>   This can be made optional too in the enable sequence.
>   I would not be surprised to find out more PLL have that, even on
>   earlier SoC.

Okay, I see.
I will try to modify clk-pll driver in accurate way to support rst
optional bit and current self-adaptation optional IP.

...

Patch

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index 1c885541c3a9..deb273673ec1 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -104,10 +104,11 @@  config COMMON_CLK_AXG_AUDIO
 	  aka axg, Say Y if you want audio subsystem to work.
 
 config COMMON_CLK_A1_PLL
-	bool
-	depends on ARCH_MESON
+	tristate "Meson A1 SoC PLL controller support"
+	depends on ARM64
 	select COMMON_CLK_MESON_REGMAP
 	select COMMON_CLK_MESON_PLL
+	select COMMON_CLK_MESON_A1_CLKC
 	help
 	  Support for the PLL clock controller on Amlogic A113L device,
 	  aka a1. Say Y if you want PLL to work.
diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
index 69c1ca07d041..23487ca797b3 100644
--- a/drivers/clk/meson/a1-pll.c
+++ b/drivers/clk/meson/a1-pll.c
@@ -2,15 +2,133 @@ 
 /*
  * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
  * Author: Jian Hu <[e-mail]>
+ *
+ * Copyright (c) 2022, 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-pll.h"
 #include "clk-regmap.h"
 
+static inline
+struct meson_a1_pll_data *meson_a1_pll_data(struct clk_regmap *clk)
+{
+	return (struct meson_a1_pll_data *)clk->data;
+}
+
+static int meson_a1_pll_init(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
+
+	regmap_multi_reg_write(clk->map, pll->base.init_regs,
+			       pll->base.init_count);
+
+	return 0;
+}
+
+static int meson_a1_pll_is_enabled(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
+
+	if (MESON_PARM_APPLICABLE(&pll->base.rst) &&
+	    meson_parm_read(clk->map, &pll->base.rst))
+		return 0;
+
+	if (!meson_parm_read(clk->map, &pll->base.en) ||
+	    !meson_parm_read(clk->map, &pll->base.l))
+		return 0;
+
+	return 1;
+}
+
+static int meson_a1_pll_enable(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
+
+	/* Do nothing if the PLL is already enabled */
+	if (clk_hw_is_enabled(hw))
+		return 0;
+
+	/* Enable the pll */
+	meson_parm_write(clk->map, &pll->base.en, 1);
+
+	/*
+	 * 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
+	 */
+	usleep_range(10, 20);
+	meson_parm_write(clk->map, &pll->current_en, 1);
+	usleep_range(40, 50);
+
+	meson_parm_write(clk->map, &pll->l_detect, 1);
+	meson_parm_write(clk->map, &pll->l_detect, 0);
+
+	if (meson_clk_pll_wait_lock(hw))
+		return -EIO;
+
+	return 0;
+}
+
+static void meson_a1_pll_disable(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
+
+	/* Disable the pll */
+	meson_parm_write(clk->map, &pll->base.en, 0);
+
+	/* Disable PLL internal self-adaption current module */
+	meson_parm_write(clk->map, &pll->current_en, 0);
+}
+
+/*
+ * A1 PLL clock controller driver is based on meson clk_pll driver,
+ * so some rate calculating routines are reused
+ */
+static unsigned long meson_a1_pll_recalc_rate(struct clk_hw *hw,
+					      unsigned long parent_rate)
+{
+	return meson_clk_pll_ops.recalc_rate(hw, parent_rate);
+}
+
+static int meson_a1_pll_determine_rate(struct clk_hw *hw,
+				       struct clk_rate_request *req)
+{
+	return meson_clk_pll_ops.determine_rate(hw, req);
+}
+
+static int meson_a1_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long parent_rate)
+{
+	return meson_clk_pll_ops.set_rate(hw, rate, parent_rate);
+}
+
+static const struct clk_ops meson_a1_pll_ops = {
+	.init		= meson_a1_pll_init,
+	.recalc_rate	= meson_a1_pll_recalc_rate,
+	.determine_rate	= meson_a1_pll_determine_rate,
+	.set_rate	= meson_a1_pll_set_rate,
+	.is_enabled	= meson_a1_pll_is_enabled,
+	.enable		= meson_a1_pll_enable,
+	.disable	= meson_a1_pll_disable
+};
+
+static const struct clk_ops meson_a1_pll_ro_ops = {
+	.recalc_rate	= meson_a1_pll_recalc_rate,
+	.is_enabled	= meson_a1_pll_is_enabled,
+};
+
 static struct clk_regmap a1_fixed_pll_dco = {
 	.data = &(struct meson_clk_pll_data){
 		.en = {
@@ -46,7 +164,7 @@  static struct clk_regmap a1_fixed_pll_dco = {
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "fixed_pll_dco",
-		.ops = &meson_clk_pll_ro_ops,
+		.ops = &meson_a1_pll_ro_ops,
 		.parent_data = &(const struct clk_parent_data) {
 			.fw_name = "xtal_fixpll",
 		},
@@ -87,31 +205,36 @@  static const struct reg_sequence a1_hifi_init_regs[] = {
 };
 
 static struct clk_regmap a1_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,
+	.data = &(struct meson_a1_pll_data){
+		.base = {
+			.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,
+			},
+			.range = &a1_hifi_pll_mult_range,
+			.init_regs = a1_hifi_init_regs,
+			.init_count = ARRAY_SIZE(a1_hifi_init_regs),
 		},
 		.current_en = {
 			.reg_off = ANACTRL_HIFIPLL_CTRL0,
@@ -123,13 +246,10 @@  static struct clk_regmap a1_hifi_pll = {
 			.shift   = 6,
 			.width   = 1,
 		},
-		.range = &a1_hifi_pll_mult_range,
-		.init_regs = a1_hifi_init_regs,
-		.init_count = ARRAY_SIZE(a1_hifi_init_regs),
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "hifi_pll",
-		.ops = &meson_clk_pll_ops,
+		.ops = &meson_a1_pll_ops,
 		.parent_data = &(const struct clk_parent_data) {
 			.fw_name = "xtal_hifipll",
 		},
@@ -276,15 +396,15 @@  static struct clk_hw_onecell_data a1_pll_hw_onecell_data = {
 	.hws = {
 		[CLKID_FIXED_PLL_DCO]		= &a1_fixed_pll_dco.hw,
 		[CLKID_FIXED_PLL]		= &a1_fixed_pll.hw,
-		[CLKID_HIFI_PLL]		= &a1_hifi_pll.hw,
-		[CLKID_FCLK_DIV2]		= &a1_fclk_div2.hw,
-		[CLKID_FCLK_DIV3]		= &a1_fclk_div3.hw,
-		[CLKID_FCLK_DIV5]		= &a1_fclk_div5.hw,
-		[CLKID_FCLK_DIV7]		= &a1_fclk_div7.hw,
 		[CLKID_FCLK_DIV2_DIV]		= &a1_fclk_div2_div.hw,
 		[CLKID_FCLK_DIV3_DIV]		= &a1_fclk_div3_div.hw,
 		[CLKID_FCLK_DIV5_DIV]		= &a1_fclk_div5_div.hw,
 		[CLKID_FCLK_DIV7_DIV]		= &a1_fclk_div7_div.hw,
+		[CLKID_FCLK_DIV2]		= &a1_fclk_div2.hw,
+		[CLKID_FCLK_DIV3]		= &a1_fclk_div3.hw,
+		[CLKID_FCLK_DIV5]		= &a1_fclk_div5.hw,
+		[CLKID_FCLK_DIV7]		= &a1_fclk_div7.hw,
+		[CLKID_HIFI_PLL]		= &a1_hifi_pll.hw,
 		[NR_PLL_CLKS]			= NULL,
 	},
 	.num = NR_PLL_CLKS,
@@ -293,68 +413,39 @@  static struct clk_hw_onecell_data a1_pll_hw_onecell_data = {
 static struct clk_regmap *const a1_pll_regmaps[] = {
 	&a1_fixed_pll_dco,
 	&a1_fixed_pll,
-	&a1_hifi_pll,
 	&a1_fclk_div2,
 	&a1_fclk_div3,
 	&a1_fclk_div5,
 	&a1_fclk_div7,
+	&a1_hifi_pll,
 };
 
-static struct regmap_config clkc_regmap_config = {
-	.reg_bits       = 32,
-	.val_bits       = 32,
-	.reg_stride     = 4,
+static const struct meson_a1_clkc_data a1_pll_clkc __maybe_unused = {
+	.hw = &a1_pll_hw_onecell_data,
+	.regs = a1_pll_regmaps,
+	.num_regs = ARRAY_SIZE(a1_pll_regmaps),
 };
 
-static int meson_a1_pll_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct resource *res;
-	void __iomem *base;
-	struct regmap *map;
-	int ret, i;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-	base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	map = devm_regmap_init_mmio(dev, base, &clkc_regmap_config);
-	if (IS_ERR(map))
-		return PTR_ERR(map);
-
-	/* Populate regmap for the regmap backed clocks */
-	for (i = 0; i < ARRAY_SIZE(a1_pll_regmaps); i++)
-		a1_pll_regmaps[i]->map = map;
-
-	for (i = 0; i < a1_pll_hw_onecell_data.num; i++) {
-		/* array might be sparse */
-		if (!a1_pll_hw_onecell_data.hws[i])
-			continue;
-
-		ret = devm_clk_hw_register(dev, a1_pll_hw_onecell_data.hws[i]);
-		if (ret) {
-			dev_err(dev, "Clock registration failed\n");
-			return ret;
-		}
-	}
-
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
-					   &a1_pll_hw_onecell_data);
-}
-
-static const struct of_device_id clkc_match_table[] = {
-	{ .compatible = "amlogic,a1-pll-clkc", },
-	{}
+#ifdef CONFIG_OF
+static const struct of_device_id a1_pll_clkc_match_table[] = {
+	{
+		.compatible = "amlogic,a1-pll-clkc",
+		.data = &a1_pll_clkc,
+	},
+	{},
 };
+MODULE_DEVICE_TABLE(of, a1_pll_clkc_match_table);
+#endif /* CONFIG_OF */
 
-static struct platform_driver a1_pll_driver = {
-	.probe		= meson_a1_pll_probe,
-	.driver		= {
-		.name	= "a1-pll-clkc",
-		.of_match_table = clkc_match_table,
+static struct platform_driver a1_pll_clkc_driver = {
+	.probe = meson_a1_clkc_probe,
+	.driver = {
+		.name = "a1-pll-clkc",
+		.of_match_table = of_match_ptr(a1_pll_clkc_match_table),
 	},
 };
 
-builtin_platform_driver(a1_pll_driver);
+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
index 8ded267061ad..2ff5a2042a97 100644
--- a/drivers/clk/meson/a1-pll.h
+++ b/drivers/clk/meson/a1-pll.h
@@ -1,38 +1,29 @@ 
 /* 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) 2022, 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_CTRL2		0x8
-#define ANACTRL_FIXPLL_CTRL3		0xc
-#define ANACTRL_FIXPLL_CTRL4		0x10
 #define ANACTRL_FIXPLL_STS		0x14
-#define ANACTRL_SYSPLL_CTRL0		0x80
-#define ANACTRL_SYSPLL_CTRL1		0x84
-#define ANACTRL_SYSPLL_CTRL2		0x88
-#define ANACTRL_SYSPLL_CTRL3		0x8c
-#define ANACTRL_SYSPLL_CTRL4		0x90
-#define ANACTRL_SYSPLL_STS		0x94
 #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
-#define ANACTRL_AUDDDS_CTRL0		0x100
-#define ANACTRL_AUDDDS_CTRL1		0x104
-#define ANACTRL_AUDDDS_CTRL2		0x108
-#define ANACTRL_AUDDDS_CTRL3		0x10c
-#define ANACTRL_AUDDDS_CTRL4		0x110
-#define ANACTRL_AUDDDS_STS		0x114
-#define ANACTRL_MISCTOP_CTRL0		0x140
-#define ANACTRL_POR_CNTL		0x188
 
 /*
  * CLKID index values
@@ -53,4 +44,16 @@ 
 /* include the CLKIDs that have been made part of the DT binding */
 #include <dt-bindings/clock/a1-pll-clkc.h>
 
+/**
+ * struct meson_a1_pll_data - A1 PLL state
+ * @base: Basic CLK PLL state
+ * @current_en: Enable or disable the PLL self-adaption current module
+ * @l_detect: Enable or disable the lock detect module
+ */
+struct meson_a1_pll_data {
+	struct meson_clk_pll_data base;
+	struct parm current_en;
+	struct parm l_detect;
+};
+
 #endif /* __A1_PLL_H */