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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221202124730.uckax232hnjqg26a@CAB-WSD-L081021>
Date:   Fri, 2 Dec 2022 15:47:30 +0300
From:   Dmitry Rokosov <ddrokosov@...rdevices.ru>
To:     Jerome Brunet <jbrunet@...libre.com>
CC:     <neil.armstrong@...aro.org>, <mturquette@...libre.com>,
        <sboyd@...nel.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <khilman@...libre.com>,
        <martin.blumenstingl@...glemail.com>, <jian.hu@...ogic.com>,
        <kernel@...rdevices.ru>, <rockosov@...il.com>,
        <linux-amlogic@...ts.infradead.org>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v8 07/11] clk: meson: a1: redesign Amlogic A1 PLL clock
 controller

On Fri, Dec 02, 2022 at 12:42:17PM +0100, Jerome Brunet wrote:
> 
> On Fri 02 Dec 2022 at 01:56, Dmitry Rokosov <ddrokosov@...rdevices.ru> 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@starbuckisacylon.baylibre.com/

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@starbuckisacylon.baylibre.com/

...

-- 
Thank you,
Dmitry