On Mon 29 May 2023 at 22:38, Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx> wrote: > Hi Dmitry, > > On Mon, May 22, 2023 at 3:00 PM Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> 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 – 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
