Received-SPF: pass (google.com: domain of linux-kernel+bounces-107610-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 19 Mar 2024 16:53:11 +0300
From: Dmitry Rokosov <ddrokosov@salutedevices.com>
To: Jerome Brunet <jbrunet@baylibre.com>
CC: Jan Dakinevich <jan.dakinevich@salutedevices.com>, 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>
Subject: Re: [PATCH 01/25] clk: meson: a1: restrict an amount of 'hifi_pll'
 params
Message-ID: <20240319135311.xjpgkvgyczq4eymi@CAB-WSD-L081021>
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>
 <d4cfef9e-3cae-4f1a-90b3-33d5707596f9@salutedevices.com>
 <1jsf0mfwwg.fsf@starbuckisacylon.baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1jsf0mfwwg.fsf@starbuckisacylon.baylibre.com>
User-Agent: NeoMutt/20220415

On Tue, Mar 19, 2024 at 09:21:27AM +0100, Jerome Brunet wrote:
> 
> On Tue 19 Mar 2024 at 01:35, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
> 
> > 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.
> 
> A1 datasheet v0.4 - Section 7.6.3.2
> 
> >
> >> 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).
> 
> You have not answer my remark.
> Mainline does not do everything like the AML SDK does. Saying you are
> copying it because you know it works (in your opinion) is not good
> enough.
> 
> I'm telling you that your hack is not necessary and so far, you have not
> demonstrated that it is.
> 
> Also the multiplier range in m/n, not m alone.
> 
> >
> >>> 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.
> 
> You argued that frac was broken and that was partly why you introduced
> this work around. I'm telling you this approach is incorrect.
> 
> So either :
> * Remove frac for now, until it is fixed, because it is broken and add
>   comment clearly explaining that quirk.
> * Or fix it now.
> 
> Your choice.
> 
> >
> >
> >>>
> >>>>>
> >>>>> 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),
> >>>>>  	},
> >>>>
> >>>>
> >> 
> >> 
> 
> 
> -- 
> Jerome

BTW, here Amlogic already mentioned all possible output audio rates for
which hifipll can be used:

https://lore.kernel.org/all/1569411888-98116-1-git-send-email-jian.hu@amlogic.com/T/#md7083b4f851ab97dfce43f8f6a3b266eb49ed329

```
The audio working frequency are 44.1khz, 48khz and 192khz.

614.4M can meet the three frequency.

after the hifi pll, there are two dividers in Audio clock.

614.4M/3200 = 192khz

614.4M/12800 = 48khz

614,4M/13932 = 44.0999khz
```

-- 
Thank you,
Dmitry