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;
References: <20230517133309.9874-1-ddrokosov@sberdevices.ru>
 <20230517133309.9874-6-ddrokosov@sberdevices.ru>
 <CAFBinCC3kQ9Nz3R2W-Qj9tbPJfS8JsB_4AkmPgS6xpQ96DBy2w@mail.gmail.com>
 <20230522130033.a47vlybocme66rev@CAB-WSD-L081021>
 <CAFBinCAk9+Km3BssA8d8nc_Z_GbhY87FD3qQRpZ2k7ChKt7TBg@mail.gmail.com>
User-agent: mu4e 1.8.13; emacs 28.2
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Dmitry Rokosov <ddrokosov@sberdevices.ru>,
        Conor Dooley <conor.dooley@microchip.com>,
        krzysztof.kozlowski+dt@linaro.org
Cc:     neil.armstrong@linaro.org, mturquette@baylibre.com,
        sboyd@kernel.org, robh+dt@kernel.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 v15 5/6] dt-bindings: clock: meson: add A1 Peripherals
 clock controller bindings
Date:   Tue, 30 May 2023 10:56:52 +0200
In-reply-to: <CAFBinCAk9+Km3BssA8d8nc_Z_GbhY87FD3qQRpZ2k7ChKt7TBg@mail.gmail.com>
Message-ID: <1jmt1m42m1.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On Mon 29 May 2023 at 22:38, Martin Blumenstingl <martin.blumenstingl@googl=
email.com> wrote:

> Hi Dmitry,
>
> On Mon, May 22, 2023 at 3:00=E2=80=AFPM Dmitry Rokosov <ddrokosov@sberdev=
ices.ru> wrote:
> [...]
>> > This IP block has at least one additional input called "sys_pll_div16".
>> > My understanding is that the "sys_pll_div16" clock is generated by the
>> > CPU clock controller. Support for the CPU clock controller
>> > (dt-bindings and a driver) will be added at a later time by Dmitry.
>> > How can we manage incrementally implementing the clock controllers?
>> > From a hardware perspective the "sys_pll_div16" input is mandatory.
>> > How to manage this in the .dts patches then (for example: does this
>> > mean that Dmitry can only add the clock controller to the .dts when
>> > all clock controller bindings have been implemented - or is there
>> > another way)?
>>
>> You're absolutely right: currently, not all inputs are supported because
>> the CPU clock controller isn't ready yet =E2=80=93 I'm working on it at =
the
>> moment.
>>
>> I understand your concerns about bindings and schema description, but
>> there is an issue to be considered. I'm developing the entire clock
>> controller A1 subsystem incrementally in three stages: peripherals and
>> PLL, CPU, and Audio. This is because the CPU can operate at a static
>> frequency and voltage, and the board boots normally without the CPU
>> clock controller, thermal sensor, and OPP table. Audio is also
>> important, but it's optional. On the other hand, without setting up the
>> peripherals and PLL controllers, the board won't function because
>> they're fundamental.
> I understand your approach and I like it (without that incremental
> approach you would probably be looking at a series with 15-20
> patches).
>
> Maybe the dt-binding maintainers have a suggestion for us here?
> Let me try to summarize the issue in a few bullet points:
> - There's (at least) four clock controllers on the Amlogic A1 SoC
> - Some of these clock controllers take the outputs of another clock
> controller as inputs
> - In this series patch the peripheral clock controller has an input
> called "sys_pll_div16"
> - The clock controller which provides the "sys_pll_div16" clock is not
> implemented yet (my understanding is that implementing it and adding
> it to this series is not easy: it would add even more patches that
> need to be reviewed and in general it's a tricky clock controller to
> implement as it manages the CPU clocks)

IMO, if you just add another const later on, it is fine. I would not make
any existing DT invalid as a result. The problem would be if you removed
an entry

That's my understanding at least

>
>> Right now, we're in the first stage of the plan. Unfortunately, I can't
>> disclose the exact names and number of clock bindings for the CPU and
>> Audio, as they're still in development and only exist in my head or
>> draft versions.
>>
>> If possible, I'd prefer to provide the new bindings and connections once
>> all the appropriate drivers are finalized.
> Question to Conor and Krzysztof (assuming you read my summary above):
> Is it fine that Dmitry adds additional inputs to the peripheral clock
> controller binding in later patches?
> If not: how can we proceed in case we need to add them now (the
> dt-binding example is the easy part for me as we can just make up a
> phandle like &sys_pll_div16_clk and use that - but this can't work
> when Dmitry tries to add the clock controller to meson-a1.dtsi)
>
> PS: Dmitry is trying to get this series into Linux 6.5. As far as I
> remember the common clock maintainers don't take pull requests with
> new features after -rc6 (which is in less than two weeks).
> So time is getting a bit short and for me this is the very last
> outstanding question. If you say that it's fine to add clocks later on
> this will immediately get my Reviewed-by.
>
>
> Best regards,
> Martin