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;
Date:   Mon, 29 May 2023 16:49:44 +0300
From:   Dmitry Rokosov <ddrokosov@sberdevices.ru>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>
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>
Subject: Re: [PATCH v13 4/6] clk: meson: a1: add Amlogic A1 PLL clock
 controller driver
Message-ID: <20230529134944.dh7isqg5ontfbtpa@CAB-WSD-L081021>
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>
 <CAFBinCCmNLQF_qV3k4kgDEAsemEsjL-GQtPex7Pmxrc2sHx30A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAFBinCCmNLQF_qV3k4kgDEAsemEsjL-GQtPex7Pmxrc2sHx30A@mail.gmail.com>
User-Agent: NeoMutt/20220415
Precedence: bulk

Hello Martin,

As I promised before, I'm now back with the vendor's reply regarding
AUDDDS. The information is still incomplete, but I have provided the
available details below. Please take a look.

On Sun, May 14, 2023 at 10:53:53PM +0200, Martin Blumenstingl wrote:
> Hi Dmitry.
> 
> On Thu, May 11, 2023 at 3:26 PM Dmitry Rokosov <ddrokosov@sberdevices.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 not
> > used anywhere. Unfortunately, as usual, the datasheet doesn't provide any
> > information or explanation about what it is. However, the driver has a few
> > 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 exported
> > GENCLK).
> > Jian did not include it in the first version of the PLL driver, and I have
> > decided not to change it either.
> >
> > I also noticed a few lines of AUDDDS initialization sequences in the vendor
> > driver, which may affect CPU clock objects (from my point of view).
> > However, they are currently under development, and I will try to figure it
> > out with Amlogic support.
> >
> > > > 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.
> >
> > 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 Amlogic
> - 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!

AUDDDS is a direct digital synthesizer used as a clock source.  It is
not used as default. You need to modify some clk registers in dts to
enable it. Basically, it is used for audio.

In A1 design, you can use AUD DDS as a clock source and it is not
necessary. It is not used in the Amlogic default setting.

According to the vendor, it is not necessary and is currently disabled
by default. While we don't have much information about AUDDDS, the
vendor suggests that it's not a commonly used clock object in A1
projects and it may be skipped if not needed.

Based on all above information, I suppose we can skip it now.

-- 
Thank you,
Dmitry