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@xxxxxxxxxxxxxxxxx> wrote: > > > On 3/18/24 13:17, Jerome Brunet wrote: > >> > >> On Sun 17 Mar 2024 at 17:17, Jan Dakinevich <jan.dakinevich@xxxxxxxxxxxxxxxxx> wrote: > >> > >>> On 3/15/24 11:58, Jerome Brunet wrote: > >>>> > >>>> On Fri 15 Mar 2024 at 02:21, Jan Dakinevich <jan.dakinevich@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx> > >>>>> --- > >>>>> 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@xxxxxxxxxxx/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