Re: [PATCH 01/25] clk: meson: a1: restrict an amount of 'hifi_pll' params

[Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxxxxx>]

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