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

[Dmitry Rokosov]

Hello Martin,

On Sun, Apr 23, 2023 at 11:12:27PM +0200, Martin Blumenstingl wrote:
> 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.
> 

Thank you very much for your effort! Please feel free to ask any
questions you may have, without any hesitation.

> 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.
> 
> [...]

Yep, that's good point. Will change it in the next version.

> > +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.
> 
> [...]

OK

> > +/* 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.

The Amlogic A1 clock engine comprises four clock controllers for
peripherals, PLL, CPU, and audio. While the first two have been
introduced in the current patch series, the last two will be sent in the
next iteration.
Presently, the PLL controller driver includes all the required bindings,
and the peripherals controller driver has all bindings except for the
CPU-related clock.
However, I do not believe this to be a significant issue. The clock DT
bindings are organized to simplify the process of introducing new bindings,
whether public or private. For instance, we may add new bindings to
include/dt-bindings at the end of the list and increase the overall number,
without disrupting the DT bindings ABI (the old numbers will remain
unchanged).

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

-- 
Thank you,
Dmitry