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

[Martin Blumenstingl]

Hello Dmitry,

(I'm aware you already posted a v14 - but I'm still replying here to
continue the discussion on one question I had to keep the context)

On Tue, Apr 25, 2023 at 2:33 PM Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> wrote:
[...]
> > > +/* 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.
I (think that I) understand this part.

> Presently, the PLL controller driver includes all the required bindings,
> and the peripherals controller driver has all bindings except for the
> CPU-related clock.
Let's stick to the PLL controller bindings for the next part.
My understanding is that the PLL clock controller registers
(ANACTRL_*) are managing the following clocks:
- fixed_pll
- sys_pll
- hifi_pll
- whatever "AUDDDS" is
- and some miscellaneous registers like ANACTRL_POR_CNTL and
ANACTRL_MISCTOP_CTRL0

I *think* you got the dt-bindings correct:
Even though the driver part does not support the hifi_pll yet, this IP
block seems to have a "hifipll_in" clock input.
Since the dt-bindings describes the hardware it may describe (for
example) clock inputs that are not used by the driver yet.

If you agree with my statement from above I'll be able to make my
original question more specific:
Since we know that we have all the required inputs for fixed_pll,
sys_pll and hifi_pll - do you know what AUDDDS is and whether it
requires any specific clock inputs (other than "fixpll_in" and
"hifipll_in")?

> 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).
Yep, this part is clear to me. I should have been more specific that I
was asking about the inputs that are described in the .yaml file, not
the clock IDs.


Best regards,
Martin