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?
> > +}
> > +
> > +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/
...
--
Thank you,
Dmitry
