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/
>
> ...
