Re: [PATCH v13 4/6] clk: meson: a1: add Amlogic A1 PLL clock controller driver

[Martin Blumenstingl]

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 PM Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> 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[] = {
> +       { .compatible = "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