Re: [PATCH v8 07/11] clk: meson: a1: redesign Amlogic A1 PLL clock controller

[Jerome Brunet]

On Fri 02 Dec 2022 at 15:47, Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> wrote:

> On Fri, Dec 02, 2022 at 12:42:17PM +0100, Jerome Brunet wrote:
>> 
>> On Fri 02 Dec 2022 at 01:56, Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> wrote:
>> 
>> > Summary changes:
>> >     - supported meson-a1-clkc common driver
>> >     - inherited from the base clk-pll driver, implemented own version of
>> >       init/enable/disable/enabled routines; rate calculating logic is
>> >       fully the same
>> >     - aligned CLKID-related definitions with CLKID list from order
>> >       perspective to remove holes and permutations
>> >     - corrected Kconfig dependencies and types
>> >     - provided correct MODULE_AUTHORs() and MODULE_LICENSE()
>> >     - optimized and fix up some clock relationships
>> >     - removed unused register offset definitions (ANACTRL_* group)
>> 
>> This patch mix PLL stuff, factorization change, etc ...
>> In general, when your commit description is a list, it is a hint that
>> you are doing more than one thing in it. It is unlikely to be OK then
>
> It will be fixed by itself, when I'll squash patches.
>
>> > +static int meson_a1_pll_init(struct clk_hw *hw)
>> > +{
>> > +	struct clk_regmap *clk = to_clk_regmap(hw);
>> > +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
>> > +
>> > +	regmap_multi_reg_write(clk->map, pll->base.init_regs,
>> > +			       pll->base.init_count);
>> > +
>> > +	return 0;
>> 
>> Looks the the default init mostly
>> 
>> Looks like you are trying the handle the absence of the rst bit.
>> I'm pretty sure the hifi PLL of the SoC as one but you really don't want
>> to poke, this can be in the generic driver, with MESON_PARM_APPLICABLE()
>> test.
>> 
>> No need to redefine this
>> 
>
> I've redefined it, because in the previous v7 you mentioned that's
> not acceptable to mix init/enable/disable sequences between a1 pll and clk
> common pll driver:
>
> https://lore.kernel.org/linux-amlogic/1jd0ac5kpk.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
>
> Hmmm, looks like I've made a mistake. You meant only enable/disable
> callbacks...
>
> Anyway, it doesn't matter to me. I think both approaches are okay:
>     * clk-pll customization using MESON_PARM_APPLICABLE()
>     * custom callbacks implementation for some clk_ops like implemented in
>       this patchset.
>
> Please advise what's the best from you point of view?

It is a balance.

Everytime a new PLL comes up, it tends to treaded as a new ip block but,
most of the time after some digging and rework, we learn new things and
it ends up being compatible with the previous ones.

>From what I see here
* You are trying to make rst optional, that's fine. Do it with
  MESON_PARM_APPLICABLE() in the main driver. Still I would recommend to
  thorougly for this bit. I'm pretty sure the hifi pll has one.

* You add a new feature called current self-adaptation.
  This can be made optional too in the enable sequence.
  I would not be surprised to find out more PLL have that, even on
  earlier SoC.

>
>> > +}
>> > +
>> > +static int meson_a1_pll_is_enabled(struct clk_hw *hw)
>> > +{
>> > +	struct clk_regmap *clk = to_clk_regmap(hw);
>> > +	struct meson_a1_pll_data *pll = meson_a1_pll_data(clk);
>> > +
>> > +	if (MESON_PARM_APPLICABLE(&pll->base.rst) &&
>> > +	    meson_parm_read(clk->map, &pll->base.rst))
>> > +		return 0;
>> > +
>> > +	if (!meson_parm_read(clk->map, &pll->base.en) ||
>> > +	    !meson_parm_read(clk->map, &pll->base.l))
>> > +		return 0;
>> > +
>> 
>> Same here, pretty sure rst is there and the generic function works but
>> if this update is required, it seems safe to do in the generic driver.
>
> The same thing... in the v7 version you suggested to not touch clk-pll
> driver.
>
> https://lore.kernel.org/linux-amlogic/1jd0ac5kpk.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
>
> ...