Add documentation and dt bindings for the Amlogic A1 Peripherals clock
controller.
A1 PLL clock controller has references to A1 Peripherals clock
controller objects, so reflect them in the schema.
Signed-off-by: Jian Hu <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
.../clock/amlogic,a1-peripherals-clkc.yaml | 73 +++++++++++
.../bindings/clock/amlogic,a1-pll-clkc.yaml | 5 +-
.../clock/amlogic,a1-peripherals-clkc.h | 115 ++++++++++++++++++
3 files changed, 191 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
create mode 100644 include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
new file mode 100644
index 000000000000..6d84cee1bd75
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,a1-peripherals-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic A1 Peripherals Clock Control Unit
+
+maintainers:
+ - Neil Armstrong <[email protected]>
+ - Jerome Brunet <[email protected]>
+ - Jian Hu <[email protected]>
+ - Dmitry Rokosov <[email protected]>
+
+properties:
+ compatible:
+ const: amlogic,a1-peripherals-clkc
+
+ '#clock-cells':
+ const: 1
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: input fixed pll div2
+ - description: input fixed pll div3
+ - description: input fixed pll div5
+ - description: input fixed pll div7
+ - description: input hifi pll
+ - description: input oscillator (usually at 24MHz)
+
+ clock-names:
+ items:
+ - const: fclk_div2
+ - const: fclk_div3
+ - const: fclk_div5
+ - const: fclk_div7
+ - const: hifi_pll
+ - const: xtal
+
+required:
+ - compatible
+ - '#clock-cells'
+ - reg
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/amlogic,a1-pll-clkc.h>
+ apb {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ clock-controller@800 {
+ compatible = "amlogic,a1-peripherals-clkc";
+ reg = <0 0x800 0 0x104>;
+ #clock-cells = <1>;
+ clocks = <&clkc_pll CLKID_FCLK_DIV2>,
+ <&clkc_pll CLKID_FCLK_DIV3>,
+ <&clkc_pll CLKID_FCLK_DIV5>,
+ <&clkc_pll CLKID_FCLK_DIV7>,
+ <&clkc_pll CLKID_HIFI_PLL>,
+ <&xtal>;
+ clock-names = "fclk_div2", "fclk_div3",
+ "fclk_div5", "fclk_div7",
+ "hifi_pll", "xtal";
+ };
+ };
diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
index 5c6fa620a63c..a59b188a8bf5 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
@@ -43,6 +43,7 @@ additionalProperties: false
examples:
- |
+ #include <dt-bindings/clock/amlogic,a1-peripherals-clkc.h>
apb {
#address-cells = <2>;
#size-cells = <2>;
@@ -51,8 +52,8 @@ examples:
compatible = "amlogic,a1-pll-clkc";
reg = <0 0x7c80 0 0x18c>;
#clock-cells = <1>;
- clocks = <&clkc_periphs_fixpll_in>,
- <&clkc_periphs_hifipll_in>;
+ clocks = <&clkc_periphs CLKID_FIXPLL_IN>,
+ <&clkc_periphs CLKID_HIFIPLL_IN>;
clock-names = "fixpll_in", "hifipll_in";
};
};
diff --git a/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
new file mode 100644
index 000000000000..ff2730f398a6
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
@@ -0,0 +1,115 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ * Author: Jian Hu <[email protected]>
+ *
+ * Copyright (c) 2023, SberDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <[email protected]>
+ */
+
+#ifndef __A1_PERIPHERALS_CLKC_H
+#define __A1_PERIPHERALS_CLKC_H
+
+#define CLKID_FIXPLL_IN 1
+#define CLKID_USB_PHY_IN 2
+#define CLKID_USB_CTRL_IN 3
+#define CLKID_HIFIPLL_IN 4
+#define CLKID_SYSPLL_IN 5
+#define CLKID_DDS_IN 6
+#define CLKID_SYS 7
+#define CLKID_CLKTREE 8
+#define CLKID_RESET_CTRL 9
+#define CLKID_ANALOG_CTRL 10
+#define CLKID_PWR_CTRL 11
+#define CLKID_PAD_CTRL 12
+#define CLKID_SYS_CTRL 13
+#define CLKID_TEMP_SENSOR 14
+#define CLKID_AM2AXI_DIV 15
+#define CLKID_SPICC_B 16
+#define CLKID_SPICC_A 17
+#define CLKID_MSR 18
+#define CLKID_AUDIO 19
+#define CLKID_JTAG_CTRL 20
+#define CLKID_SARADC_EN 21
+#define CLKID_PWM_EF 22
+#define CLKID_PWM_CD 23
+#define CLKID_PWM_AB 24
+#define CLKID_CEC 25
+#define CLKID_I2C_S 26
+#define CLKID_IR_CTRL 27
+#define CLKID_I2C_M_D 28
+#define CLKID_I2C_M_C 29
+#define CLKID_I2C_M_B 30
+#define CLKID_I2C_M_A 31
+#define CLKID_ACODEC 32
+#define CLKID_OTP 33
+#define CLKID_SD_EMMC_A 34
+#define CLKID_USB_PHY 35
+#define CLKID_USB_CTRL 36
+#define CLKID_SYS_DSPB 37
+#define CLKID_SYS_DSPA 38
+#define CLKID_DMA 39
+#define CLKID_IRQ_CTRL 40
+#define CLKID_NIC 41
+#define CLKID_GIC 42
+#define CLKID_UART_C 43
+#define CLKID_UART_B 44
+#define CLKID_UART_A 45
+#define CLKID_SYS_PSRAM 46
+#define CLKID_RSA 47
+#define CLKID_CORESIGHT 48
+#define CLKID_AM2AXI_VAD 49
+#define CLKID_AUDIO_VAD 50
+#define CLKID_AXI_DMC 51
+#define CLKID_AXI_PSRAM 52
+#define CLKID_RAMB 53
+#define CLKID_RAMA 54
+#define CLKID_AXI_SPIFC 55
+#define CLKID_AXI_NIC 56
+#define CLKID_AXI_DMA 57
+#define CLKID_CPU_CTRL 58
+#define CLKID_ROM 59
+#define CLKID_PROC_I2C 60
+#define CLKID_DSPA_EN 63
+#define CLKID_DSPA_EN_NIC 64
+#define CLKID_DSPB_EN 65
+#define CLKID_DSPB_EN_NIC 66
+#define CLKID_RTC 67
+#define CLKID_CECA_32K 68
+#define CLKID_CECB_32K 69
+#define CLKID_24M 70
+#define CLKID_12M 71
+#define CLKID_FCLK_DIV2_DIVN 72
+#define CLKID_GEN 73
+#define CLKID_SARADC 75
+#define CLKID_PWM_A 76
+#define CLKID_PWM_B 77
+#define CLKID_PWM_C 78
+#define CLKID_PWM_D 79
+#define CLKID_PWM_E 80
+#define CLKID_PWM_F 81
+#define CLKID_SPICC 82
+#define CLKID_TS 83
+#define CLKID_SPIFC 84
+#define CLKID_USB_BUS 85
+#define CLKID_SD_EMMC 86
+#define CLKID_PSRAM 87
+#define CLKID_DMC 88
+#define CLKID_DSPA_A_SEL 95
+#define CLKID_DSPA_B_SEL 98
+#define CLKID_DSPB_A_SEL 101
+#define CLKID_DSPB_B_SEL 104
+#define CLKID_CECB_32K_SEL_PRE 113
+#define CLKID_CECB_32K_SEL 114
+#define CLKID_CECA_32K_SEL_PRE 117
+#define CLKID_CECA_32K_SEL 118
+#define CLKID_GEN_SEL 121
+#define CLKID_PWM_A_SEL 124
+#define CLKID_PWM_B_SEL 126
+#define CLKID_PWM_C_SEL 128
+#define CLKID_PWM_D_SEL 130
+#define CLKID_PWM_E_SEL 132
+#define CLKID_PWM_F_SEL 134
+#define CLKID_SD_EMMC_SEL2 147
+
+#endif /* __A1_PERIPHERALS_CLKC_H */
--
2.36.0
Hi Krzysztof and Dmitry,
On Wed, May 17, 2023 at 3:33 PM Dmitry Rokosov <[email protected]> wrote:
[...]
> + clocks:
> + items:
> + - description: input fixed pll div2
> + - description: input fixed pll div3
> + - description: input fixed pll div5
> + - description: input fixed pll div7
> + - description: input hifi pll
> + - description: input oscillator (usually at 24MHz)
> +
> + clock-names:
> + items:
> + - const: fclk_div2
> + - const: fclk_div3
> + - const: fclk_div5
> + - const: fclk_div7
> + - const: hifi_pll
> + - const: xtal
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)?
Best regards,
Martin
Martin,
On Fri, May 19, 2023 at 11:09:29PM +0200, Martin Blumenstingl wrote:
> Hi Krzysztof and Dmitry,
>
> On Wed, May 17, 2023 at 3:33 PM Dmitry Rokosov <[email protected]> wrote:
> [...]
> > + clocks:
> > + items:
> > + - description: input fixed pll div2
> > + - description: input fixed pll div3
> > + - description: input fixed pll div5
> > + - description: input fixed pll div7
> > + - description: input hifi pll
> > + - description: input oscillator (usually at 24MHz)
> > +
> > + clock-names:
> > + items:
> > + - const: fclk_div2
> > + - const: fclk_div3
> > + - const: fclk_div5
> > + - const: fclk_div7
> > + - const: hifi_pll
> > + - const: xtal
> 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.
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.
--
Thank you,
Dmitry
Hi Dmitry,
On Mon, May 22, 2023 at 3:00 PM Dmitry Rokosov <[email protected]> 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)
> 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
On Mon 29 May 2023 at 22:38, Martin Blumenstingl <[email protected]> wrote:
> Hi Dmitry,
>
> On Mon, May 22, 2023 at 3:00 PM Dmitry Rokosov <[email protected]> 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
Yo,
On Mon, May 29, 2023 at 10:38:33PM +0200, Martin Blumenstingl wrote:
> On Mon, May 22, 2023 at 3:00 PM Dmitry Rokosov <[email protected]> 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)
If I am understanding correctly, this series implements the child
controller and a parent, which is unimplemented, provides the child with
sys_pll_div16.
The thing I am missing is whether the child controller has some outputs
that depend on this sys_pll_div16 input & whether those are documented
in this series. Regardless, you should be able to add more output clocks
without compatibility issues.
> > 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?
Perhaps Krzysztof will disagree with me, but my take on it would be that
the binding should describe the individual clock controller in its
totality, but the driver can choose to only implement a subset of it.
If you define the binding as only needing N inputs, but then later
expand it to having N+M inputs, the driver will have to support N & N+M
input clocks to preserve compatibility.
If you define it as needing N+M inputs from the beginning, but only use
N, there is no issue with backwards compatibility when you later use
them all.
> 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)
I would be inclined to do the same thing in the dts as the example,
and make up a fixed-frequency clock and use it to plug the hole.
When you have bindings etc written for the clock controller providing
that clock, the fixed-frequency clock could be swapped out for the real
one.
> 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.
I *think* that what I've just said should not get in the way of such a
timeline, as it would only involve a "small" change to the dt-binding,
but not require additional bindings or driver.
Cheers,
Conor.
Hello Conor, Martin and Jerome,
Thank you so much for the detailed information and thoughts you shared
about the important process of changing DT bindings. Please find my
comments below.
On Tue, May 30, 2023 at 10:34:07AM +0100, Conor Dooley wrote:
> Yo,
>
> On Mon, May 29, 2023 at 10:38:33PM +0200, Martin Blumenstingl wrote:
> > On Mon, May 22, 2023 at 3:00 PM Dmitry Rokosov <[email protected]> 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)
>
> If I am understanding correctly, this series implements the child
> controller and a parent, which is unimplemented, provides the child with
> sys_pll_div16.
> The thing I am missing is whether the child controller has some outputs
> that depend on this sys_pll_div16 input & whether those are documented
> in this series. Regardless, you should be able to add more output clocks
> without compatibility issues.
>
> > > 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?
>
> Perhaps Krzysztof will disagree with me, but my take on it would be that
> the binding should describe the individual clock controller in its
> totality, but the driver can choose to only implement a subset of it.
>
> If you define the binding as only needing N inputs, but then later
> expand it to having N+M inputs, the driver will have to support N & N+M
> input clocks to preserve compatibility.
> If you define it as needing N+M inputs from the beginning, but only use
> N, there is no issue with backwards compatibility when you later use
> them all.
>
In short, I agree with Jerome's position, which he shared in the
parallel email:
https://lore.kernel.org/all/[email protected]/
Below I will try to explain my position.
Please correct me if I am mistaken, but it appears we are currently
discussing a scenario where a new kernel X+1 image version is flashed
onto a device with a device tree from kernel X-1 image version, and
whether a new driver with clock object connections can function on the
previous Device Tree version.
If I'm grasping the issue correctly, it seems that we have four
potential scenarios at play here:
1) we may have altered the relationships between internal clock objects;
2) we may have introduced new output clock connections;
3) we may have introduced new input clock connections;
4) we may have removed existing connections from the driver.
It seems that options 1 and 4 are straightforward and easy to
understand.
The 4 case is fully prohibited. We cannot remove
any object or connection from the clock driver if another consumer uses
it in the previous Device Tree versions. It is also important to note
that any internal changes within the driver do not affect the device
tree connections, as this is already clear.
The second and third cases are more complex.
For instance, let's say we are preparing a new patchset for an existing
clock driver, such as Peripherals, where we are adding an Audio clock
that is handled by an MMIO register inside Peripherals clock driver. X-1
dts does not recognize this new clock and doesn't have any connection to
any node or to any other clock driver, thereby eliminating any backwards
compatibility issues.
As for new input clock connections, such as the cpu_clock
(sys_pll_div16), these are handled by clock muxing abstraction, allowing
CCF to find the clock object by fw.name and returning -ENOENT if the
connection is missing without breaking any CCF flow. It happens in the
kernel function clk_core_fill_parent_index()
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L424
Despite not having the connection for the new input in the old Device
Tree version, this will not break kernel boot flow and workflow, and the
new clock object just would not be utilized.
Based on the presented arguments, I fully agree with Jerome's position.
We can add new connections and objects in new driver versions, but their
removal is prohibited.
If it's alright with you, I would prefer to keep the Peripherals and PLL
clock driver and their bindings as they are, and continue with the CPU
and Audio clock controllers in a separate patch series. Would that be
feasible for you?
> > 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)
>
> I would be inclined to do the same thing in the dts as the example,
> and make up a fixed-frequency clock and use it to plug the hole.
> When you have bindings etc written for the clock controller providing
> that clock, the fixed-frequency clock could be swapped out for the real
> one.
>
> > 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.
>
> I *think* that what I've just said should not get in the way of such a
> timeline, as it would only involve a "small" change to the dt-binding,
> but not require additional bindings or driver.
>
> Cheers,
> Conor.
>
--
Thank you,
Dmitry
УВЕДОМЛЕНИЕ О КОНФИДЕНЦИАЛЬНОСТИ: Это электронное сообщение и любые документы, приложенные к нему, содержат конфиденциальную информацию. Настоящим уведомляем Вас о том, что если это сообщение не предназначено Вам, использование, копирование, распространение информации, содержащейся в настоящем сообщении, а также осуществление любых действий на основе этой информации, строго запрещено. Если Вы получили это сообщение по ошибке, пожалуйста, сообщите об этом отправителю по электронной почте и удалите это сообщение.
CONFIDENTIALITY NOTICE: This email and any files attached to it are confidential. If you are not the intended recipient you are notified that using, copying, distributing or taking any action in reliance on the contents of this information is strictly prohibited. If you have received this email in error please notify the sender and delete this email.
Hi,
On Tue, May 30, 2023 at 6:03 PM Dmitry Rokosov <[email protected]> wrote:
[...]
> > If I am understanding correctly, this series implements the child
> > controller and a parent, which is unimplemented, provides the child with
> > sys_pll_div16.
> > The thing I am missing is whether the child controller has some outputs
> > that depend on this sys_pll_div16 input & whether those are documented
> > in this series. Regardless, you should be able to add more output clocks
> > without compatibility issues.
Conor, the short answer is yes, the "gen_sel" mux (see patch 6/6 from
this series, which is then part of a clock tree that's an output of
the peripheral clock controller) uses sys_pll_div16 as input.
Dmitry goes into more details below.
[...]
> As for new input clock connections, such as the cpu_clock
> (sys_pll_div16), these are handled by clock muxing abstraction, allowing
> CCF to find the clock object by fw.name and returning -ENOENT if the
> connection is missing without breaking any CCF flow. It happens in the
> kernel function clk_core_fill_parent_index()
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L424
> Despite not having the connection for the new input in the old Device
> Tree version, this will not break kernel boot flow and workflow, and the
> new clock object just would not be utilized.
>
> Based on the presented arguments, I fully agree with Jerome's position.
> We can add new connections and objects in new driver versions, but their
> removal is prohibited.
>
> If it's alright with you, I would prefer to keep the Peripherals and PLL
> clock driver and their bindings as they are, and continue with the CPU
> and Audio clock controllers in a separate patch series. Would that be
> feasible for you?
To me this sounds good!
Best regards,
Martin