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] [day] [month] [year] [list]

Date:   Fri, 2 Dec 2022 21:20:35 +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

...

> >> > +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?
> 
> 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.

Okay, I see.
I will try to modify clk-pll driver in accurate way to support rst
optional bit and current self-adaptation optional IP.

...

-- 
Thank you,
Dmitry