Received-SPF: pass (google.com: domain of linux-kernel+bounces-128139-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
References: <20240329205904.25002-1-ddrokosov@salutedevices.com>
 <20240329205904.25002-3-ddrokosov@salutedevices.com>
 <1j4jckjftk.fsf@starbuckisacylon.baylibre.com>
 <20240402121546.qrrc7r5un75464pb@CAB-WSD-L081021>
User-agent: mu4e 1.10.8; emacs 29.2
From: Jerome Brunet <jbrunet@baylibre.com>
To: Dmitry Rokosov <ddrokosov@salutedevices.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>, neil.armstrong@linaro.org,
 mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, khilman@baylibre.com,
 martin.blumenstingl@googlemail.com, kernel@salutedevices.com,
 rockosov@gmail.com, linux-amlogic@lists.infradead.org,
 linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v1 2/6] clk: meson: a1: pll: support 'syspll'
 general-purpose PLL for CPU clock
Date: Tue, 02 Apr 2024 16:27:17 +0200
In-reply-to: <20240402121546.qrrc7r5un75464pb@CAB-WSD-L081021>
Message-ID: <1jmsqbj0md.fsf@starbuckisacylon.baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain


On Tue 02 Apr 2024 at 15:15, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> On Tue, Apr 02, 2024 at 11:00:42AM +0200, Jerome Brunet wrote:
>> 
>> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>> 
>> > The 'syspll' PLL, also known as the system PLL, is a general and
>> > essential PLL responsible for generating the CPU clock frequency.
>> > With its wide-ranging capabilities, it is designed to accommodate
>> > frequencies within the range of 768MHz to 1536MHz.
>> >
>> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > ---
>> >  drivers/clk/meson/a1-pll.c | 78 ++++++++++++++++++++++++++++++++++++++
>> >  drivers/clk/meson/a1-pll.h |  6 +++
>> >  2 files changed, 84 insertions(+)
>> >
>> > diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
>> > index 60b2e53e7e51..02fd2d325cc6 100644
>> > --- a/drivers/clk/meson/a1-pll.c
>> > +++ b/drivers/clk/meson/a1-pll.c
>> > @@ -138,6 +138,81 @@ static struct clk_regmap hifi_pll = {
>> >  	},
>> >  };
>> >  
>> > +static const struct pll_mult_range sys_pll_mult_range = {
>> > +	.min = 32,
>> > +	.max = 64,
>> > +};
>> > +
>> > +/*
>> > + * We assume that the sys_pll_clk has already been set up by the low-level
>> > + * bootloaders as the main CPU PLL source. Therefore, it is not necessary to
>> > + * run the initialization sequence.
>> > + */
>> 
>> I see no reason to make such assumption.
>> This clock is no read-only, it apparently is able to re-lock so assuming
>> anything from the bootloader is just asking from trouble
>> 
>
> Indeed, I have implemented the following initialization sequence. I have
> dumped the bootloader setup and included it in the definition of my
> sys_pll. However, I have encountered an issue with the enable bit. If I
> leave the enable bit switched on by default, there is a possibility that
> the bootloader selects a fixed CPU clock while the sys_pll should be
> switched off. On the other hand, if I keep the enable bit switched off
> by default, the bootloader might configure the CPU clock to use sys_pll,
> resulting in the execution halting when the initialization sequence is
> run. This situation has led me to assume that we should place our trust
> in the bootloader setup.
>
> If you believe it is necessary to include the initialization sequence, I
> can prepare it with the sys_pll enabled by default.

I just noted your initial comment is misleading.

You could submit a patch to apply the init sequence only if the PLL is
not already enabled. Maybe even condition that to flag in the pll data
to avoid applying it to the other platforms for now.

>
>> > +static struct clk_regmap sys_pll = {
>> > +	.data = &(struct meson_clk_pll_data){
>> > +		.en = {
>> > +			.reg_off = ANACTRL_SYSPLL_CTRL0,
>> > +			.shift   = 28,
>> > +			.width   = 1,
>> > +		},
>> > +		.m = {
>> > +			.reg_off = ANACTRL_SYSPLL_CTRL0,
>> > +			.shift   = 0,
>> > +			.width   = 8,
>> > +		},
>> > +		.n = {
>> > +			.reg_off = ANACTRL_SYSPLL_CTRL0,
>> > +			.shift   = 10,
>> > +			.width   = 5,
>> > +		},
>> > +		.frac = {
>> > +			.reg_off = ANACTRL_SYSPLL_CTRL1,
>> > +			.shift   = 0,
>> > +			.width   = 19,
>> > +		},
>> > +		.l = {
>> > +			.reg_off = ANACTRL_SYSPLL_STS,
>> > +			.shift   = 31,
>> > +			.width   = 1,
>> > +		},
>> > +		.current_en = {
>> > +			.reg_off = ANACTRL_SYSPLL_CTRL0,
>> > +			.shift   = 26,
>> > +			.width   = 1,
>> > +		},
>> > +		.l_detect = {
>> > +			.reg_off = ANACTRL_SYSPLL_CTRL2,
>> > +			.shift   = 6,
>> > +			.width   = 1,
>> > +		},
>> > +		.range = &sys_pll_mult_range,
>> > +	},
>> > +	.hw.init = &(struct clk_init_data){
>> > +		.name = "sys_pll",
>> > +		.ops = &meson_clk_pll_ops,
>> > +		.parent_names = (const char *[]){ "syspll_in" },
>> > +		.num_parents = 1,
>> > +		/*
>> > +		 * This clock is used as the main CPU PLL source in low-level
>> > +		 * bootloaders, and it is necessary to mark it as critical.
>> > +		 */
>> > +		.flags = CLK_IS_CRITICAL,
>> 
>> No I don't think so. Downstream consumer maybe critical but that one is
>> not, unless it is read-only.
>> 
>> A CPU pll, like on the g12 family, is unlikely to be read-only since the
>> PLL will need to relock to change rates. During this phase, there will
>> be no reate coming from the PLL so the PLL is not critical and you must
>> be able to "park" your CPU an another clock while poking this one
>> 
>
> Initially, I tagged it with CLK_IS_CRITICAL because I observed in the
> kernel start that CCF disables it.
> However, upon further understanding,
> I realized that this happened due to other reasons. I believe that if I
> provide an init sequence where sys_pll is enabled by default, CCF will
> not disable this clock.
>
>> > +	},
>> > +};
>> > +
>> > +static struct clk_fixed_factor sys_pll_div16 = {
>> > +	.mult = 1,
>> > +	.div = 16,
>> > +	.hw.init = &(struct clk_init_data){
>> > +		.name = "sys_pll_div16",
>> > +		.ops = &clk_fixed_factor_ops,
>> > +		.parent_hws = (const struct clk_hw *[]) {
>> > +			&sys_pll.hw
>> > +		},
>> > +		.num_parents = 1,
>> > +	},
>> > +};
>> > +
>> >  static struct clk_fixed_factor fclk_div2_div = {
>> >  	.mult = 1,
>> >  	.div = 2,
>> > @@ -283,6 +358,8 @@ static struct clk_hw *a1_pll_hw_clks[] = {
>> >  	[CLKID_FCLK_DIV5]	= &fclk_div5.hw,
>> >  	[CLKID_FCLK_DIV7]	= &fclk_div7.hw,
>> >  	[CLKID_HIFI_PLL]	= &hifi_pll.hw,
>> > +	[CLKID_SYS_PLL]		= &sys_pll.hw,
>> > +	[CLKID_SYS_PLL_DIV16]	= &sys_pll_div16.hw,
>> >  };
>> >  
>> >  static struct clk_regmap *const a1_pll_regmaps[] = {
>> > @@ -293,6 +370,7 @@ static struct clk_regmap *const a1_pll_regmaps[] = {
>> >  	&fclk_div5,
>> >  	&fclk_div7,
>> >  	&hifi_pll,
>> > +	&sys_pll,
>> >  };
>> >  
>> >  static struct regmap_config a1_pll_regmap_cfg = {
>> > diff --git a/drivers/clk/meson/a1-pll.h b/drivers/clk/meson/a1-pll.h
>> > index 4be17b2bf383..666d9b2137e9 100644
>> > --- a/drivers/clk/meson/a1-pll.h
>> > +++ b/drivers/clk/meson/a1-pll.h
>> > @@ -18,6 +18,12 @@
>> >  #define ANACTRL_FIXPLL_CTRL0	0x0
>> >  #define ANACTRL_FIXPLL_CTRL1	0x4
>> >  #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


-- 
Jerome