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@xxxxxxxxxx> >> Signed-off-by: Xukai Wang <kingxukai@xxxxxxxxxxxx> >> --- >> .../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@xxxxxxxxxxxx> >> + >> +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@xxxxxxxxxxxx> >> + */ >> + >> +#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 */ >>
