Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230405195927.13487-1-ddrokosov@sberdevices.ru> <20230405195927.13487-5-ddrokosov@sberdevices.ru>
In-Reply-To: <20230405195927.13487-5-ddrokosov@sberdevices.ru>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Sun, 23 Apr 2023 23:12:27 +0200
Message-ID: <CAFBinCA3uZXzr3RgnWnKV5Qr-CPaZQX5joDg319i_cgzhLJy2g@mail.gmail.com>
Subject: Re: [PATCH v13 4/6] clk: meson: a1: add Amlogic A1 PLL clock
 controller driver
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc:     neil.armstrong@linaro.org, jbrunet@baylibre.com,
        mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, khilman@baylibre.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hello Dmitry,

currently Jerome is busy so I am trying to continue where he left off.
I have followed the previous iterations a bit but may have missed some
details. So apologies if I'm repeating some questions that Jerome
previously asked.

On Wed, Apr 5, 2023 at 9:59=E2=80=AFPM Dmitry Rokosov <ddrokosov@sberdevice=
s.ru> wrote:
[...]
> +config COMMON_CLK_A1_PLL
> +       tristate "Meson A1 SoC PLL controller support"
Should this be "Amlogic A1 SoC PLL controller support"?
My understanding is that the "meson" name was dropped for this
generation of SoCs.

[...]
> +static const struct of_device_id a1_pll_clkc_match_table[] =3D {
> +       { .compatible =3D "amlogic,a1-pll-clkc", },
> +       {},
nit-pick: please drop the comma after {}
This empty entry is a sentinel, no other entries are supposed to come
after this - so a trailing comma is not necessary.

[...]
> +/* PLL register offset */
> +#define ANACTRL_FIXPLL_CTRL0   0x0
> +#define ANACTRL_FIXPLL_CTRL1   0x4
> +#define ANACTRL_FIXPLL_STS     0x14
> +#define ANACTRL_HIFIPLL_CTRL0  0xc0
> +#define ANACTRL_HIFIPLL_CTRL1  0xc4
> +#define ANACTRL_HIFIPLL_CTRL2  0xc8
> +#define ANACTRL_HIFIPLL_CTRL3  0xcc
> +#define ANACTRL_HIFIPLL_CTRL4  0xd0
> +#define ANACTRL_HIFIPLL_STS    0xd4
Here I have a question that will potentially affect patch 3/6
("dt-bindings: clock: meson: add A1 PLL clock controller bindings").
In the cover-letter you mentioned that quite a few clocks have been omitted=
.
Any dt-bindings that we create need to be stable going forward. That
means: the dt-bindings will always need to describe what the hardware
is capable of, not what the driver implements.
So my question is: do we have all needed inputs described in the
dt-bindings (even though we're omitting quite a few registers here
that will only be added/used in the future)?
Older SoCs require (temporarily) using the XTAL clock for CPU clock
tree changes. To make a long story short: I'm wondering if - at least
- the XTAL clock input is missing.

PS: I don't have an A1 datasheet nor a vendor kernel source (and even
less a board for testing). So I can't verify any of this myself and
I'm asking questions instead.


Best regards,
Martin