Received-SPF: pass (google.com: domain of linux-kernel+bounces-106756-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <d4cfef9e-3cae-4f1a-90b3-33d5707596f9@salutedevices.com>
Date: Tue, 19 Mar 2024 01:35:12 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 01/25] clk: meson: a1: restrict an amount of 'hifi_pll'
 params
Content-Language: en-US
To: Jerome Brunet <jbrunet@baylibre.com>
CC: Neil Armstrong <neil.armstrong@linaro.org>, Michael Turquette
	<mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Rob Herring
	<robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, Philipp Zabel <p.zabel@pengutronix.de>,
	Kevin Hilman <khilman@baylibre.com>, Martin Blumenstingl
	<martin.blumenstingl@googlemail.com>, Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Linus Walleij <linus.walleij@linaro.org>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.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>, <alsa-devel@alsa-project.org>,
	<linux-sound@vger.kernel.org>, <linux-gpio@vger.kernel.org>,
	<kernel@salutedevices.com>
References: <20240314232201.2102178-1-jan.dakinevich@salutedevices.com>
 <20240314232201.2102178-2-jan.dakinevich@salutedevices.com>
 <1j8r2jj24k.fsf@starbuckisacylon.baylibre.com>
 <cbfd9c66-cca5-49f5-9468-43710c48518e@salutedevices.com>
 <1jedc7hlg4.fsf@starbuckisacylon.baylibre.com>
From: Jan Dakinevich <jan.dakinevich@salutedevices.com>
In-Reply-To: <1jedc7hlg4.fsf@starbuckisacylon.baylibre.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 3/18/24 13:17, Jerome Brunet wrote:
> 
> On Sun 17 Mar 2024 at 17:17, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
> 
>> On 3/15/24 11:58, Jerome Brunet wrote:
>>>
>>> On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
>>>
>>>> Existing values were insufficient to produce accurate clock for audio
>>>> devices. New values are safe and most suitable to produce 48000Hz sample
>>>> rate.
>>>
>>> The hifi pll is not about 48k only. I see no reason to restrict the PLL
>>> to a single setting.
>>>> You've provided no justification why the PLL driver can't reach the same
>>> setting for 48k. The setting below is just the crude part. the fine
>>> tuning is done done with the frac parameter so I doubt this provides a
>>> more accurate rate.
>>>
>>
>> You are right, it is not about 48k only. However, there are two issues.
>>
>> First, indeed, I could just extend the range of multipliers to 1..255.
> 
> Why 1..255 ? This is not what I'm pointing out
> 
> According to the datasheet - the range is 32 - 64, as currently
> set in the driver.
> 

Could you point where in the doc the range 32..64 is documented?
Documentation that I have may be not so complete, but I don't see there
any mention about it.

Anyway, range 32..64 of multipliers is not enough to produce accurate
clock, and a need 128 for 48kHz.

> The change you have provided request a multipler of 128/5 = 25,6
> If you put assigned-rate = 614400000 in DT, I see no reason can find the
> same solution on its own.
> 

The reasoning is following. I don't know why 32..64 range was declared
for this clock, and whether it would be safe to extend it and include
128, which is required for 48kHz. But I know, that multiplier=128 is
safe and works fine (together divider=5).

>> But I am unsure if hifi_pll is able to handle whole range of
>> mulptipliers. The value 128 is taken from Amlogic's branch, and I am
>> pretty sure that it works.
> 
>>
>> Second, unfortunately frac parameter currently doesn't work. When frac
>> is used enabling of hifi_pll fails in meson_clk_pll_wait_lock(). I see
>> it when try to use 44100Hz and multipliers' range is set to 1..255. So,
>> support of other rates than 48k requires extra effort.
> 
> Then your change is even more problematic because it certainly does not
> disable frac ... which you say is broken.
> 
> That parameter should be removed with a proper comment explaining why
> you are disabling it. That type a limitation / known issue should be
> mentionned in your change.
> 

Handling of frac should not be removed, it should be fixed to achieve
another rates. But that is not the goal of this commit.


>>
>>>>
>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>>>> ---
>>>>  drivers/clk/meson/a1-pll.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
>>>> index 4325e8a6a3ef..00e06d03445b 100644
>>>> --- a/drivers/clk/meson/a1-pll.c
>>>> +++ b/drivers/clk/meson/a1-pll.c
>>>> @@ -74,9 +74,9 @@ static struct clk_regmap fixed_pll = {
>>>>  	},
>>>>  };
>>>>  
>>>> -static const struct pll_mult_range hifi_pll_mult_range = {
>>>> -	.min = 32,
>>>> -	.max = 64,
>>>> +static const struct pll_params_table hifi_pll_params_table[] = {
>>>> +	PLL_PARAMS(128, 5),
>>>> +	{ },
>>>>  };
>>>>  
>>>>  static const struct reg_sequence hifi_init_regs[] = {
>>>> @@ -124,7 +124,7 @@ static struct clk_regmap hifi_pll = {
>>>>  			.shift   = 6,
>>>>  			.width   = 1,
>>>>  		},
>>>> -		.range = &hifi_pll_mult_range,
>>>> +		.table = hifi_pll_params_table,
>>>>  		.init_regs = hifi_init_regs,
>>>>  		.init_count = ARRAY_SIZE(hifi_init_regs),
>>>>  	},
>>>
>>>
> 
> 

-- 
Best regards
Jan Dakinevich