From: Jerome Brunet <jbrunet@baylibre.com>
To: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc: neil.armstrong@linaro.org, mturquette@baylibre.com,
sboyd@kernel.org, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, khilman@baylibre.com,
martin.blumenstingl@googlemail.com, jian.hu@amlogic.com,
kernel@sberdevices.ru, rockosov@gmail.com,
linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v8 07/11] clk: meson: a1: redesign Amlogic A1 PLL clock controller
Date: Fri, 02 Dec 2022 13:49:59 +0100 [thread overview]
Message-ID: <1jh6ye6js6.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <20221202124730.uckax232hnjqg26a@CAB-WSD-L081021>
On Fri 02 Dec 2022 at 15:47, Dmitry Rokosov <ddrokosov@sberdevices.ru> 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@sberdevices.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?
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@starbuckisacylon.baylibre.com/
>
> ...
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
next prev parent reply other threads:[~2022-12-02 13:02 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-01 22:56 [PATCH v8 00/11] add Amlogic A1 clock controller drivers Dmitry Rokosov
2022-12-01 22:56 ` [PATCH v8 01/11] dt-bindings: clock: meson: add A1 PLL clock controller bindings Dmitry Rokosov
2022-12-02 4:10 ` Rob Herring
2022-12-02 9:51 ` Dmitry Rokosov
2022-12-02 10:39 ` Krzysztof Kozlowski
2022-12-02 11:04 ` Jerome Brunet
2022-12-02 11:16 ` Krzysztof Kozlowski
2022-12-02 11:28 ` Dmitry Rokosov
2022-12-02 13:36 ` neil.armstrong
2022-12-02 10:42 ` Krzysztof Kozlowski
2022-12-02 11:18 ` Dmitry Rokosov
2022-12-02 11:11 ` Jerome Brunet
2022-12-02 12:26 ` Dmitry Rokosov
2022-12-01 22:56 ` [PATCH v8 02/11] clk: meson: a1: add support for Amlogic A1 PLL clock driver Dmitry Rokosov
2022-12-02 11:16 ` Jerome Brunet
2022-12-02 11:31 ` Dmitry Rokosov
2022-12-01 22:56 ` [PATCH v8 03/11] dt-bindings: clock: meson: add A1 peripheral clock controller bindings Dmitry Rokosov
2022-12-02 4:10 ` Rob Herring
2022-12-02 9:49 ` Dmitry Rokosov
2022-12-02 10:39 ` Krzysztof Kozlowski
2022-12-02 10:58 ` Dmitry Rokosov
2022-12-02 10:43 ` Krzysztof Kozlowski
2022-12-01 22:56 ` [PATCH v8 04/11] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver Dmitry Rokosov
2022-12-02 11:19 ` Jerome Brunet
2022-12-02 12:33 ` Dmitry Rokosov
2022-12-01 22:56 ` [PATCH v8 05/11] clk: meson: pll: export meson_clk_pll_wait_lock symbol Dmitry Rokosov
2022-12-01 22:56 ` [PATCH v8 06/11] clk: meson: introduce a1-clkc common driver for all A1 clock controllers Dmitry Rokosov
2022-12-02 11:36 ` Jerome Brunet
2022-12-02 11:58 ` Dmitry Rokosov
2022-12-01 22:56 ` [PATCH v8 07/11] clk: meson: a1: redesign Amlogic A1 PLL clock controller Dmitry Rokosov
2022-12-02 11:42 ` Jerome Brunet
2022-12-02 12:47 ` Dmitry Rokosov
2022-12-02 12:49 ` Jerome Brunet [this message]
2022-12-02 18:20 ` Dmitry Rokosov
2022-12-01 22:57 ` [PATCH v8 08/11] dt-bindings: clock: meson: fixup A1 PLL clkc dtb_check errors Dmitry Rokosov
2022-12-02 10:40 ` Krzysztof Kozlowski
2022-12-01 22:57 ` [PATCH v8 09/11] clk: meson: redesign A1 Peripherals CLK controller Dmitry Rokosov
2022-12-02 12:01 ` Jerome Brunet
2022-12-02 12:10 ` Dmitry Rokosov
2022-12-01 22:57 ` [PATCH v8 10/11] dt-bindings: clock: meson: fixup A1 peripherals clkc dtb_check errors Dmitry Rokosov
2022-12-02 10:40 ` Krzysztof Kozlowski
2022-12-01 22:57 ` [PATCH v8 11/11] arm64: dts: meson: a1: introduce PLL and Peripherals clk controllers Dmitry Rokosov
2022-12-02 10:43 ` Krzysztof Kozlowski
2022-12-02 12:03 ` Jerome Brunet
2022-12-02 13:37 ` neil.armstrong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1jh6ye6js6.fsf@starbuckisacylon.baylibre.com \
--to=jbrunet@baylibre.com \
--cc=ddrokosov@sberdevices.ru \
--cc=devicetree@vger.kernel.org \
--cc=jian.hu@amlogic.com \
--cc=kernel@sberdevices.ru \
--cc=khilman@baylibre.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.blumenstingl@googlemail.com \
--cc=mturquette@baylibre.com \
--cc=neil.armstrong@linaro.org \
--cc=robh+dt@kernel.org \
--cc=rockosov@gmail.com \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).