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> <CAFBinCA3uZXzr3RgnWnKV5Qr-CPaZQX5joDg319i_cgzhLJy2g@mail.gmail.com>
 <20230425123304.xjmrkraybp2siwdw@CAB-WSD-L081021> <CAFBinCCqx1oHf+PcXBkeRYHnGQChbTTPRyD8SJU+ait+TG+AjQ@mail.gmail.com>
 <20230511132606.vk52yorf43alwgew@CAB-WSD-L081021>
In-Reply-To: <20230511132606.vk52yorf43alwgew@CAB-WSD-L081021>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Sun, 14 May 2023 22:53:53 +0200
Message-ID: <CAFBinCCmNLQF_qV3k4kgDEAsemEsjL-GQtPex7Pmxrc2sHx30A@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

Hi Dmitry.

On Thu, May 11, 2023 at 3:26=E2=80=AFPM Dmitry Rokosov <ddrokosov@sberdevic=
es.ru> wrote:
[...]
> > 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")?
> >
>
> To be honest, I have prepared A1 peripherals and A1 PLL drivers based on =
very
> poor Amlogic datasheets and custom 4.19-based vendor drivers.
> The vendor driver has an AUDDDS clock in the PLL clock part, but it is no=
t
> used anywhere. Unfortunately, as usual, the datasheet doesn't provide any
> information or explanation about what it is. However, the driver has a fe=
w
> lines of comments that indicate:
>
>     /*
>      * aud dds clock is not pll clock, not divider clock,
>      * No clock model can describe it.
>      * So we regard it as a gate, and the gate ops
>      * should realize lonely.
>      */
>
> Additionally, the vendor driver states that AUDDDS has a 49Mhz clock,
> but I do not see any relationship with other clocks (including the export=
ed
> GENCLK).
> Jian did not include it in the first version of the PLL driver, and I hav=
e
> decided not to change it either.
>
> I also noticed a few lines of AUDDDS initialization sequences in the vend=
or
> driver, which may affect CPU clock objects (from my point of view).
> However, they are currently under development, and I will try to figure i=
t
> out with Amlogic support.
>
> > > However, I do not believe this to be a significant issue. The clock D=
T
> > > bindings are organized to simplify the process of introducing new bin=
dings,
> > > 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 n=
umber,
> > > 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.
>
> Actually, AUDDDS has an xtal2dds parent clock, and if we need to have
> the AUDDDS clock in the PLL driver, we should add one more link between
> peripherals and PLL drivers.
>
> Let me know if you have any questions.
I have no questions - all I can say is that:
- I like your approach of clarifying details of the AUDDDS clock with Amlog=
ic
- and I fully agree with your conclusion that (depending on what
Amlogic says) we need one more link from the PLL driver to the AUDDDS
clock

Thank you for your persistence with this series, I'm sure it will pay
off in the long run!


Best regards,
Martin