diff mbox series

[RESEND,v3,1/3] dt-bindings: clock: Add bindings for Canaan K230 clock controller

Message ID 20250203-b4-k230-clk-v3-1-362c79124572@zohomail.com (mailing list archive)
State New
Headers show
Series riscv: canaan: Add support for K230-Canmv clock | expand

Checks

Context Check Description
bjorn/pre-ci_am fail Failed to apply series

Commit Message

Xukai Wang Feb. 3, 2025, 2:49 p.m. UTC 
This patch adds the Device Tree binding for the clock controller
on Canaan k230. The binding defines the new clocks available and
the required properties to configure them correctly.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Xukai Wang <kingxukai@zohomail.com>
---
 .../devicetree/bindings/clock/canaan,k230-clk.yaml | 43 +++++++++++++++++++
 include/dt-bindings/clock/canaan,k230-clk.h        | 49 ++++++++++++++++++++++
 2 files changed, 92 insertions(+)

Comments

Samuel Holland Feb. 3, 2025, 8:24 p.m. UTC | #1 
On 2025-02-03 8:49 AM, Xukai Wang wrote:
> This patch adds the Device Tree binding for the clock controller
> on Canaan k230. The binding defines the new clocks available and
> the required properties to configure them correctly.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Xukai Wang <kingxukai@zohomail.com>
> ---
>  .../devicetree/bindings/clock/canaan,k230-clk.yaml | 43 +++++++++++++++++++
>  include/dt-bindings/clock/canaan,k230-clk.h        | 49 ++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml b/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..d7220fa30e4699a68fa5279c04abc63c1905fa4a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/canaan,k230-clk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Canaan Kendryte K230 Clock
> +
> +maintainers:
> +  - Xukai Wang <kingxukai@zohomail.com>
> +
> +properties:
> +  compatible:
> +    const: canaan,k230-clk
> +
> +  reg:
> +    items:
> +      - description: PLL control registers.
> +      - description: Sysclk control registers.

From the way the driver is structured, this looks rather like two separate
hardware blocks, not two groups of registers for a single hardware block. For
example, the driver registers two clock providers for the same DT node, with
overlapping indexes. This doesn't work. Either you need two separate DT nodes --
one for the PLLs and another for the sysclks -- or you need to include the PLLs
in the binding header below at non-overlapping indexes.

Regards,
Samuel

> +
> +  clocks:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clock-controller@91102000 {
> +        compatible = "canaan,k230-clk";
> +        reg = <0x91102000 0x1000>,
> +              <0x91100000 0x1000>;
> +        clocks = <&osc24m>;
> +        #clock-cells = <1>;
> +    };
> diff --git a/include/dt-bindings/clock/canaan,k230-clk.h b/include/dt-bindings/clock/canaan,k230-clk.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..47d966fda5771615dad8ade64eeec42a9b27696e
> --- /dev/null
> +++ b/include/dt-bindings/clock/canaan,k230-clk.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Kendryte Canaan K230 Clock Drivers
> + *
> + * Author: Xukai Wang <kingxukai@zohomail.com>
> + */
> +
> +#ifndef CLOCK_K230_CLK_H
> +#define CLOCK_K230_CLK_H
> +
> +/* Kendryte K230 SoC clock identifiers (arbitrary values). */
> +#define K230_CPU0_SRC			0
> +#define K230_CPU0_ACLK			1
> +#define K230_CPU0_PLIC			2
> +#define K230_CPU0_NOC_DDRCP4		3
> +#define K230_CPU0_PCLK			4
> +#define K230_PMU_PCLK			5
> +#define K230_HS_HCLK_HIGH_SRC		6
> +#define K230_HS_HCLK_HIGH_GATE		7
> +#define K230_HS_HCLK_SRC		8
> +#define K230_HS_SD0_HS_AHB_GAT		9
> +#define K230_HS_SD1_HS_AHB_GAT		10
> +#define K230_HS_SSI1_HS_AHB_GA		11
> +#define K230_HS_SSI2_HS_AHB_GA		12
> +#define K230_HS_USB0_HS_AHB_GA		13
> +#define K230_HS_USB1_HS_AHB_GA		14
> +#define K230_HS_SSI0_AXI15		15
> +#define K230_HS_SSI1			16
> +#define K230_HS_SSI2			17
> +#define K230_HS_QSPI_AXI_SRC		18
> +#define K230_HS_SSI1_ACLK_GATE		19
> +#define K230_HS_SSI2_ACLK_GATE		20
> +#define K230_HS_SD_CARD_SRC		21
> +#define K230_HS_SD0_CARD_TX		22
> +#define K230_HS_SD1_CARD_TX		23
> +#define K230_HS_SD_AXI_SRC		24
> +#define K230_HS_SD0_AXI_GATE		25
> +#define K230_HS_SD1_AXI_GATE		26
> +#define K230_HS_SD0_BASE_GATE		27
> +#define K230_HS_SD1_BASE_GATE		28
> +#define K230_HS_OSPI_SRC		29
> +#define K230_HS_USB_REF_50M		30
> +#define K230_HS_SD_TIMER_SRC		31
> +#define K230_HS_SD0_TIMER_GATE		32
> +#define K230_HS_SD1_TIMER_GATE		33
> +#define K230_HS_USB0_REFERENCE		34
> +#define K230_HS_USB1_REFERENCE		35
> +
> +#endif /* CLOCK_K230_CLK_H */
>
Xukai Wang Feb. 4, 2025, 5:06 p.m. UTC | #2 
On 2025/2/4 04:24, Samuel Holland wrote:
> On 2025-02-03 8:49 AM, Xukai Wang wrote:
>> This patch adds the Device Tree binding for the clock controller
>> on Canaan k230. The binding defines the new clocks available and
>> the required properties to configure them correctly.
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Signed-off-by: Xukai Wang <kingxukai@zohomail.com>
>> ---
>>  .../devicetree/bindings/clock/canaan,k230-clk.yaml | 43 +++++++++++++++++++
>>  include/dt-bindings/clock/canaan,k230-clk.h        | 49 ++++++++++++++++++++++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml b/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..d7220fa30e4699a68fa5279c04abc63c1905fa4a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml
>> @@ -0,0 +1,43 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/canaan,k230-clk.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Canaan Kendryte K230 Clock
>> +
>> +maintainers:
>> +  - Xukai Wang <kingxukai@zohomail.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: canaan,k230-clk
>> +
>> +  reg:
>> +    items:
>> +      - description: PLL control registers.
>> +      - description: Sysclk control registers.
> From the way the driver is structured, this looks rather like two separate
> hardware blocks, not two groups of registers for a single hardware block. For
> example, the driver registers two clock providers for the same DT node, with
> overlapping indexes. This doesn't work. Either you need two separate DT nodes --
> one for the PLLs and another for the sysclks -- or you need to include the PLLs
> in the binding header below at non-overlapping indexes.
>
> Regards,
> Samuel
Thank you for your feedback.

You mentioned the issue of registering two clock providers on the same
DT node with overlapping indexes, which was indeed my oversight.

The actual situation is that PLLs are only used for internal
configuration by PLL_DIVs, and since PLL_DIVs do not be directly invoked
using `clock=<PLL_DIVs Index>` in the DT node, there is no need to use
`devm_of_clk_hw_add_provider` to add them as clock providers.

Therefore, I will remove the code that registers PLL_DIVs as clock
providers ant its `onecell_get` function.

Moreover, since PLLs are only used internally by PLL_DIVs and do not
need to be exposed as clock providers externally, 

so I think there is no need to modify the implementation to separate
them into PLLs and sysclk.

If there's anything I might have overlooked or misunderstood, please
feel free to point it out.

>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  '#clock-cells':
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - '#clock-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    clock-controller@91102000 {
>> +        compatible = "canaan,k230-clk";
>> +        reg = <0x91102000 0x1000>,
>> +              <0x91100000 0x1000>;
>> +        clocks = <&osc24m>;
>> +        #clock-cells = <1>;
>> +    };
>> diff --git a/include/dt-bindings/clock/canaan,k230-clk.h b/include/dt-bindings/clock/canaan,k230-clk.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..47d966fda5771615dad8ade64eeec42a9b27696e
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/canaan,k230-clk.h
>> @@ -0,0 +1,49 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>> +/*
>> + * Kendryte Canaan K230 Clock Drivers
>> + *
>> + * Author: Xukai Wang <kingxukai@zohomail.com>
>> + */
>> +
>> +#ifndef CLOCK_K230_CLK_H
>> +#define CLOCK_K230_CLK_H
>> +
>> +/* Kendryte K230 SoC clock identifiers (arbitrary values). */
>> +#define K230_CPU0_SRC			0
>> +#define K230_CPU0_ACLK			1
>> +#define K230_CPU0_PLIC			2
>> +#define K230_CPU0_NOC_DDRCP4		3
>> +#define K230_CPU0_PCLK			4
>> +#define K230_PMU_PCLK			5
>> +#define K230_HS_HCLK_HIGH_SRC		6
>> +#define K230_HS_HCLK_HIGH_GATE		7
>> +#define K230_HS_HCLK_SRC		8
>> +#define K230_HS_SD0_HS_AHB_GAT		9
>> +#define K230_HS_SD1_HS_AHB_GAT		10
>> +#define K230_HS_SSI1_HS_AHB_GA		11
>> +#define K230_HS_SSI2_HS_AHB_GA		12
>> +#define K230_HS_USB0_HS_AHB_GA		13
>> +#define K230_HS_USB1_HS_AHB_GA		14
>> +#define K230_HS_SSI0_AXI15		15
>> +#define K230_HS_SSI1			16
>> +#define K230_HS_SSI2			17
>> +#define K230_HS_QSPI_AXI_SRC		18
>> +#define K230_HS_SSI1_ACLK_GATE		19
>> +#define K230_HS_SSI2_ACLK_GATE		20
>> +#define K230_HS_SD_CARD_SRC		21
>> +#define K230_HS_SD0_CARD_TX		22
>> +#define K230_HS_SD1_CARD_TX		23
>> +#define K230_HS_SD_AXI_SRC		24
>> +#define K230_HS_SD0_AXI_GATE		25
>> +#define K230_HS_SD1_AXI_GATE		26
>> +#define K230_HS_SD0_BASE_GATE		27
>> +#define K230_HS_SD1_BASE_GATE		28
>> +#define K230_HS_OSPI_SRC		29
>> +#define K230_HS_USB_REF_50M		30
>> +#define K230_HS_SD_TIMER_SRC		31
>> +#define K230_HS_SD0_TIMER_GATE		32
>> +#define K230_HS_SD1_TIMER_GATE		33
>> +#define K230_HS_USB0_REFERENCE		34
>> +#define K230_HS_USB1_REFERENCE		35
>> +
>> +#endif /* CLOCK_K230_CLK_H */
>>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml b/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..d7220fa30e4699a68fa5279c04abc63c1905fa4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/canaan,k230-clk.yaml
@@ -0,0 +1,43 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/canaan,k230-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Canaan Kendryte K230 Clock
+
+maintainers:
+  - Xukai Wang <kingxukai@zohomail.com>
+
+properties:
+  compatible:
+    const: canaan,k230-clk
+
+  reg:
+    items:
+      - description: PLL control registers.
+      - description: Sysclk control registers.
+
+  clocks:
+    maxItems: 1
+
+  '#clock-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-controller@91102000 {
+        compatible = "canaan,k230-clk";
+        reg = <0x91102000 0x1000>,
+              <0x91100000 0x1000>;
+        clocks = <&osc24m>;
+        #clock-cells = <1>;
+    };
diff --git a/include/dt-bindings/clock/canaan,k230-clk.h b/include/dt-bindings/clock/canaan,k230-clk.h
new file mode 100644
index 0000000000000000000000000000000000000000..47d966fda5771615dad8ade64eeec42a9b27696e
--- /dev/null
+++ b/include/dt-bindings/clock/canaan,k230-clk.h
@@ -0,0 +1,49 @@ 
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Kendryte Canaan K230 Clock Drivers
+ *
+ * Author: Xukai Wang <kingxukai@zohomail.com>
+ */
+
+#ifndef CLOCK_K230_CLK_H
+#define CLOCK_K230_CLK_H
+
+/* Kendryte K230 SoC clock identifiers (arbitrary values). */
+#define K230_CPU0_SRC			0
+#define K230_CPU0_ACLK			1
+#define K230_CPU0_PLIC			2
+#define K230_CPU0_NOC_DDRCP4		3
+#define K230_CPU0_PCLK			4
+#define K230_PMU_PCLK			5
+#define K230_HS_HCLK_HIGH_SRC		6
+#define K230_HS_HCLK_HIGH_GATE		7
+#define K230_HS_HCLK_SRC		8
+#define K230_HS_SD0_HS_AHB_GAT		9
+#define K230_HS_SD1_HS_AHB_GAT		10
+#define K230_HS_SSI1_HS_AHB_GA		11
+#define K230_HS_SSI2_HS_AHB_GA		12
+#define K230_HS_USB0_HS_AHB_GA		13
+#define K230_HS_USB1_HS_AHB_GA		14
+#define K230_HS_SSI0_AXI15		15
+#define K230_HS_SSI1			16
+#define K230_HS_SSI2			17
+#define K230_HS_QSPI_AXI_SRC		18
+#define K230_HS_SSI1_ACLK_GATE		19
+#define K230_HS_SSI2_ACLK_GATE		20
+#define K230_HS_SD_CARD_SRC		21
+#define K230_HS_SD0_CARD_TX		22
+#define K230_HS_SD1_CARD_TX		23
+#define K230_HS_SD_AXI_SRC		24
+#define K230_HS_SD0_AXI_GATE		25
+#define K230_HS_SD1_AXI_GATE		26
+#define K230_HS_SD0_BASE_GATE		27
+#define K230_HS_SD1_BASE_GATE		28
+#define K230_HS_OSPI_SRC		29
+#define K230_HS_USB_REF_50M		30
+#define K230_HS_SD_TIMER_SRC		31
+#define K230_HS_SD0_TIMER_GATE		32
+#define K230_HS_SD1_TIMER_GATE		33
+#define K230_HS_USB0_REFERENCE		34
+#define K230_HS_USB1_REFERENCE		35
+
+#endif /* CLOCK_K230_CLK_H */