This SoC is similar to Hi3798CV200 with a few more clocks in CRG module.
Note this driver is still ongoing, many clocks are not registered in the
driver now. Feedback is welcomed, especially from HiSilicon people.
Signed-off-by: Yang Xiwen <[email protected]>
---
Changes in v2:
- s/dt-binding/dt-bindings in commit logs: (Krzysztof Kozlowski)
- fix bot error by adding "hisilicon,hisi-sdmmc-dll" to syscon.yaml (Rob
Herring)
- hi3798mv200-crg: assign fixed rate parents to some gates
- hi3798mv200-crg: s/ETH/FEMAC, add GMAC ctrl clock
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Yang Xiwen (5):
dt-bindings: clock: histb-clock: Add missing common clock and Hi3798MV200 specific clock definition
clk: hisilicon: add CRG driver for Hi3798MV200 SoC
dt-bindings: clock: merge all hisilicon clock bindings to hisilicon,clock-reset-generator
dt-bindings: mfd: syscon: Add hisilicon,sdmmc-sap-dll compatible
dt-bindings: clock: hisilicon,clock-reset-controller: add Hi3798MV200 SoC support
.../devicetree/bindings/clock/hi3660-clock.txt | 47 ---
.../devicetree/bindings/clock/hi3670-clock.txt | 43 --
.../devicetree/bindings/clock/hi6220-clock.txt | 52 ---
.../devicetree/bindings/clock/hisi-crg.txt | 50 ---
.../clock/hisilicon,clock-reset-generator.yaml | 175 +++++++++
.../clock/hisilicon,hi3559av100-clock.yaml | 59 ---
Documentation/devicetree/bindings/mfd/syscon.yaml | 1 +
drivers/clk/hisilicon/Kconfig | 8 +
drivers/clk/hisilicon/Makefile | 1 +
drivers/clk/hisilicon/crg-hi3798mv200.c | 436 +++++++++++++++++++++
include/dt-bindings/clock/histb-clock.h | 21 +
11 files changed, 642 insertions(+), 251 deletions(-)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240216-clk-mv200-cc8cae396ee0
Best regards,
--
Yang Xiwen <[email protected]>
From: Yang Xiwen <[email protected]>
According to the datasheet, some clocks are missing, add their
definitions first.
Some aliases for hi3798mv200 are also introduced.
Signed-off-by: Yang Xiwen <[email protected]>
---
include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
index e64e5770ada6..68a53053586a 100644
--- a/include/dt-bindings/clock/histb-clock.h
+++ b/include/dt-bindings/clock/histb-clock.h
@@ -58,6 +58,27 @@
#define HISTB_USB3_UTMI_CLK1 48
#define HISTB_USB3_PIPE_CLK1 49
#define HISTB_USB3_SUSPEND_CLK1 50
+#define HISTB_SDIO1_BIU_CLK 51
+#define HISTB_SDIO1_CIU_CLK 52
+#define HISTB_SDIO1_DRV_CLK 53
+#define HISTB_SDIO1_SAMPLE_CLK 54
+#define HISTB_ETH0_PHY_CLK 55
+#define HISTB_ETH1_PHY_CLK 56
+#define HISTB_WDG0_CLK 57
+#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
+#define HISTB_USB2_UTMI1_CLK 58
+#define HISTB_USB3_REF_CLK 59
+#define HISTB_USB3_GM_CLK 60
+#define HISTB_USB3_GS_CLK 61
+
+/* Hi3798MV200 specific clocks */
+
+// reuse clocks of histb
+#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK
+#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK
+#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK
+#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK
+#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK
/* clocks provided by mcu CRG */
#define HISTB_MCE_CLK 1
--
2.43.0
From: Yang Xiwen <[email protected]>
This SoC is similar to Hi3798CV200.
Also document the specific DLL regs and add an example for it.
Signed-off-by: Yang Xiwen <[email protected]>
---
.../clock/hisilicon,clock-reset-generator.yaml | 36 ++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
index d37cd892473e..8ee844574eda 100644
--- a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
+++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
@@ -44,12 +44,17 @@ properties:
- hisilicon,hi3519-crg
- hisilicon,hi3798cv200-crg
- hisilicon,hi3798cv200-sysctrl
+ - hisilicon,hi3798mv200-crg
+ - hisilicon,hi3798mv200-sysctrl
- const: syscon
- const: simple-mfd
reg:
maxItems: 1
+ ranges:
+ maxItems: 1
+
'#clock-cells':
const: 1
@@ -87,6 +92,12 @@ properties:
description: |
Reset controller for Hi3798CV200 GMAC module
+patternProperties:
+ '.*-dll@[0-9a-f]+':
+ type: object
+ description: |
+ eMMC/SD delay-locked-loop (DLL) register subnode
+
required:
- compatible
- '#clock-cells'
@@ -137,3 +148,28 @@ examples:
#clock-cells = <1>;
};
};
+ - |
+ crg: clock-reset-controller@8a22000 {
+ compatible = "hisilicon,hi3798mv200-crg", "syscon", "simple-mfd";
+ reg = <0x8a22000 0x1000>;
+ ranges = <0x0 0x8a22000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ #clock-cells = <1>;
+ #reset-cells = <2>;
+
+ emmc_sap_dll: sap-dll@39c {
+ compatible = "hisilicon,sdmmc-sap-dll", "syscon", "simple-mfd";
+ reg = <0x39c 0x8>;
+ };
+
+ sdio0_sap_dll: sap-dll@3a4 {
+ compatible = "hisilicon,sdmmc-sap-dll", "syscon", "simple-mfd";
+ reg = <0x3a4 0x8>;
+ };
+
+ sdio1_sap_dll: sap-dll@3ac {
+ compatible = "hisilicon,sdmmc-sap-dll", "syscon", "simple-mfd";
+ reg = <0x3ac 0x8>;
+ };
+ };
--
2.43.0
From: Yang Xiwen <[email protected]>
We don't need so many separated and duplicated dt-binding files. Merge
them all and convert them to YAML.
Signed-off-by: Yang Xiwen <[email protected]>
---
.../devicetree/bindings/clock/hi3660-clock.txt | 47 -------
.../devicetree/bindings/clock/hi3670-clock.txt | 43 -------
.../devicetree/bindings/clock/hi6220-clock.txt | 52 --------
.../devicetree/bindings/clock/hisi-crg.txt | 50 --------
.../clock/hisilicon,clock-reset-generator.yaml | 139 +++++++++++++++++++++
.../clock/hisilicon,hi3559av100-clock.yaml | 59 ---------
6 files changed, 139 insertions(+), 251 deletions(-)
diff --git a/Documentation/devicetree/bindings/clock/hi3660-clock.txt b/Documentation/devicetree/bindings/clock/hi3660-clock.txt
deleted file mode 100644
index 946da7cee54f..000000000000
--- a/Documentation/devicetree/bindings/clock/hi3660-clock.txt
+++ /dev/null
@@ -1,47 +0,0 @@
-* Hisilicon Hi3660 Clock Controller
-
-The Hi3660 clock controller generates and supplies clock to various
-controllers within the Hi3660 SoC.
-
-Required Properties:
-
-- compatible: the compatible should be one of the following strings to
- indicate the clock controller functionality.
-
- - "hisilicon,hi3660-crgctrl"
- - "hisilicon,hi3660-pctrl"
- - "hisilicon,hi3660-pmuctrl"
- - "hisilicon,hi3660-sctrl"
- - "hisilicon,hi3660-iomcu"
- - "hisilicon,hi3660-stub-clk"
-
-- reg: physical base address of the controller and length of memory mapped
- region.
-
-- #clock-cells: should be 1.
-
-Optional Properties:
-
-- mboxes: Phandle to the mailbox for sending message to MCU.
- (See: ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
-
-Each clock is assigned an identifier and client nodes use this identifier
-to specify the clock which they consume.
-
-All these identifier could be found in <dt-bindings/clock/hi3660-clock.h>.
-
-Examples:
- crg_ctrl: clock-controller@fff35000 {
- compatible = "hisilicon,hi3660-crgctrl", "syscon";
- reg = <0x0 0xfff35000 0x0 0x1000>;
- #clock-cells = <1>;
- };
-
- uart0: serial@fdf02000 {
- compatible = "arm,pl011", "arm,primecell";
- reg = <0x0 0xfdf02000 0x0 0x1000>;
- interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&crg_ctrl HI3660_CLK_MUX_UART0>,
- <&crg_ctrl HI3660_PCLK>;
- clock-names = "uartclk", "apb_pclk";
- };
diff --git a/Documentation/devicetree/bindings/clock/hi3670-clock.txt b/Documentation/devicetree/bindings/clock/hi3670-clock.txt
deleted file mode 100644
index 66f3697eca78..000000000000
--- a/Documentation/devicetree/bindings/clock/hi3670-clock.txt
+++ /dev/null
@@ -1,43 +0,0 @@
-* Hisilicon Hi3670 Clock Controller
-
-The Hi3670 clock controller generates and supplies clock to various
-controllers within the Hi3670 SoC.
-
-Required Properties:
-
-- compatible: the compatible should be one of the following strings to
- indicate the clock controller functionality.
-
- - "hisilicon,hi3670-crgctrl"
- - "hisilicon,hi3670-pctrl"
- - "hisilicon,hi3670-pmuctrl"
- - "hisilicon,hi3670-sctrl"
- - "hisilicon,hi3670-iomcu"
- - "hisilicon,hi3670-media1-crg"
- - "hisilicon,hi3670-media2-crg"
-
-- reg: physical base address of the controller and length of memory mapped
- region.
-
-- #clock-cells: should be 1.
-
-Each clock is assigned an identifier and client nodes use this identifier
-to specify the clock which they consume.
-
-All these identifier could be found in <dt-bindings/clock/hi3670-clock.h>.
-
-Examples:
- crg_ctrl: clock-controller@fff35000 {
- compatible = "hisilicon,hi3670-crgctrl", "syscon";
- reg = <0x0 0xfff35000 0x0 0x1000>;
- #clock-cells = <1>;
- };
-
- uart0: serial@fdf02000 {
- compatible = "arm,pl011", "arm,primecell";
- reg = <0x0 0xfdf02000 0x0 0x1000>;
- interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&crg_ctrl HI3670_CLK_GATE_UART0>,
- <&crg_ctrl HI3670_PCLK>;
- clock-names = "uartclk", "apb_pclk";
- };
diff --git a/Documentation/devicetree/bindings/clock/hi6220-clock.txt b/Documentation/devicetree/bindings/clock/hi6220-clock.txt
deleted file mode 100644
index 17ac4a3dd26a..000000000000
--- a/Documentation/devicetree/bindings/clock/hi6220-clock.txt
+++ /dev/null
@@ -1,52 +0,0 @@
-* Hisilicon Hi6220 Clock Controller
-
-Clock control registers reside in different Hi6220 system controllers,
-please refer the following document to know more about the binding rules
-for these system controllers:
-
-Documentation/devicetree/bindings/arm/hisilicon/hisilicon.yaml
-
-Required Properties:
-
-- compatible: the compatible should be one of the following strings to
- indicate the clock controller functionality.
-
- - "hisilicon,hi6220-acpu-sctrl"
- - "hisilicon,hi6220-aoctrl"
- - "hisilicon,hi6220-sysctrl"
- - "hisilicon,hi6220-mediactrl"
- - "hisilicon,hi6220-pmctrl"
- - "hisilicon,hi6220-stub-clk"
-
-- reg: physical base address of the controller and length of memory mapped
- region.
-
-- #clock-cells: should be 1.
-
-Optional Properties:
-
-- hisilicon,hi6220-clk-sram: phandle to the syscon managing the SoC internal sram;
- the driver need use the sram to pass parameters for frequency change.
-
-- mboxes: use the label reference for the mailbox as the first parameter, the
- second parameter is the channel number.
-
-Example 1:
- sys_ctrl: sys_ctrl@f7030000 {
- compatible = "hisilicon,hi6220-sysctrl", "syscon";
- reg = <0x0 0xf7030000 0x0 0x2000>;
- #clock-cells = <1>;
- };
-
-Example 2:
- stub_clock: stub_clock {
- compatible = "hisilicon,hi6220-stub-clk";
- hisilicon,hi6220-clk-sram = <&sram>;
- #clock-cells = <1>;
- mboxes = <&mailbox 1>;
- };
-
-Each clock is assigned an identifier and client nodes use this identifier
-to specify the clock which they consume.
-
-All these identifier could be found in <dt-bindings/clock/hi6220-clock.h>.
diff --git a/Documentation/devicetree/bindings/clock/hisi-crg.txt b/Documentation/devicetree/bindings/clock/hisi-crg.txt
deleted file mode 100644
index cc60b3d423f3..000000000000
--- a/Documentation/devicetree/bindings/clock/hisi-crg.txt
+++ /dev/null
@@ -1,50 +0,0 @@
-* HiSilicon Clock and Reset Generator(CRG)
-
-The CRG module provides clock and reset signals to various
-modules within the SoC.
-
-This binding uses the following bindings:
- Documentation/devicetree/bindings/clock/clock-bindings.txt
- Documentation/devicetree/bindings/reset/reset.txt
-
-Required Properties:
-
-- compatible: should be one of the following.
- - "hisilicon,hi3516cv300-crg"
- - "hisilicon,hi3516cv300-sysctrl"
- - "hisilicon,hi3519-crg"
- - "hisilicon,hi3798cv200-crg"
- - "hisilicon,hi3798cv200-sysctrl"
-
-- reg: physical base address of the controller and length of memory mapped
- region.
-
-- #clock-cells: should be 1.
-
-Each clock is assigned an identifier and client nodes use this identifier
-to specify the clock which they consume.
-
-All these identifier could be found in <dt-bindings/clock/hi3519-clock.h>.
-
-- #reset-cells: should be 2.
-
-A reset signal can be controlled by writing a bit register in the CRG module.
-The reset specifier consists of two cells. The first cell represents the
-register offset relative to the base address. The second cell represents the
-bit index in the register.
-
-Example: CRG nodes
-CRG: clock-reset-controller@12010000 {
- compatible = "hisilicon,hi3519-crg";
- reg = <0x12010000 0x10000>;
- #clock-cells = <1>;
- #reset-cells = <2>;
-};
-
-Example: consumer nodes
-i2c0: i2c@12110000 {
- compatible = "hisilicon,hi3519-i2c";
- reg = <0x12110000 0x1000>;
- clocks = <&CRG HI3519_I2C0_RST>;
- resets = <&CRG 0xe4 0>;
-};
diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
new file mode 100644
index 000000000000..d37cd892473e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/hisilicon,clock-reset-generator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon SOC Clock and Reset Generator (CRG) module
+
+maintainers:
+ - Yang Xiwen <[email protected]>
+
+description: |
+ Hisilicon SOC clock control module which supports the clocks, resets and
+ power domains on various SoCs.
+
+properties:
+ compatible:
+ minItems: 1
+ items:
+ - enum:
+ - hisilicon,hi3559av100-clock
+ - hisilicon,hi3559av100-shub-clock
+ - hisilicon,hi3660-crgctrl
+ - hisilicon,hi3660-pctrl
+ - hisilicon,hi3660-pmuctrl
+ - hisilicon,hi3660-sctrl
+ - hisilicon,hi3660-iomcu
+ - hisilicon,hi3660-stub-clk
+ - hisilicon,hi3670-crgctrl
+ - hisilicon,hi3670-pctrl
+ - hisilicon,hi3670-pmuctrl
+ - hisilicon,hi3670-sctrl
+ - hisilicon,hi3670-iomcu
+ - hisilicon,hi3670-media1-crg
+ - hisilicon,hi3670-media2-crg
+ - hisilicon,hi6220-acpu-sctrl
+ - hisilicon,hi6220-aoctrl
+ - hisilicon,hi6220-sysctrl
+ - hisilicon,hi6220-mediactrl
+ - hisilicon,hi6220-pmctrl
+ - hisilicon,hi6220-stub-clk
+ - hisilicon,hi3516cv300-crg
+ - hisilicon,hi3516cv300-sysctrl
+ - hisilicon,hi3519-crg
+ - hisilicon,hi3798cv200-crg
+ - hisilicon,hi3798cv200-sysctrl
+ - const: syscon
+ - const: simple-mfd
+
+ reg:
+ maxItems: 1
+
+ '#clock-cells':
+ const: 1
+
+ '#reset-cells':
+ enum: [1, 2]
+ description: |
+ First cell is reset request register offset.
+ Second cell is bit offset in reset request register.
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 1
+
+ mboxes:
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+ description: |
+ Phandle to the mailbox for sending msg to MCU
+ (See ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
+
+ mbox-names:
+ $ref: /schemas/types.yaml#/definitions/string-array
+ description: |
+ Names of the mailboxes.
+
+ hisilicon,hi6220-clk-sram:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: |
+ Phandle to the syscon managing the SoC internal sram
+ the driver needs using the sram to pass parameters for frequency change.
+
+ reset-controller:
+ type: object
+ description: |
+ Reset controller for Hi3798CV200 GMAC module
+
+required:
+ - compatible
+ - '#clock-cells'
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ not:
+ contains:
+ enum:
+ - hisilicon,hi3798cv200-crg
+ then:
+ properties:
+ reset-controller: false
+ - oneOf:
+ - required:
+ - hisilicon,hi6220-clk-sram
+ - required:
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/hi3559av100-clock.h>
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ clock-controller@12010000 {
+ compatible = "hisilicon,hi3559av100-clock";
+ #clock-cells = <1>;
+ #reset-cells = <2>;
+ reg = <0x0 0x12010000 0x0 0x10000>;
+ };
+ };
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/hi3660-clock.h>
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ clock-controller@fff35000 {
+ compatible = "hisilicon,hi3660-crgctrl", "syscon";
+ reg = <0x0 0xfff35000 0x0 0x1000>;
+ #clock-cells = <1>;
+ };
+ };
diff --git a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml b/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
deleted file mode 100644
index 3ceb29cec704..000000000000
--- a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
+++ /dev/null
@@ -1,59 +0,0 @@
-# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/clock/hisilicon,hi3559av100-clock.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: Hisilicon SOC Clock for HI3559AV100
-
-maintainers:
- - Dongjiu Geng <[email protected]>
-
-description: |
- Hisilicon SOC clock control module which supports the clocks, resets and
- power domains on HI3559AV100.
-
- See also:
- dt-bindings/clock/hi3559av100-clock.h
-
-properties:
- compatible:
- enum:
- - hisilicon,hi3559av100-clock
- - hisilicon,hi3559av100-shub-clock
-
- reg:
- minItems: 1
- maxItems: 2
-
- '#clock-cells':
- const: 1
-
- '#reset-cells':
- const: 2
- description: |
- First cell is reset request register offset.
- Second cell is bit offset in reset request register.
-
-required:
- - compatible
- - reg
- - '#clock-cells'
- - '#reset-cells'
-
-additionalProperties: false
-
-examples:
- - |
- soc {
- #address-cells = <2>;
- #size-cells = <2>;
-
- clock-controller@12010000 {
- compatible = "hisilicon,hi3559av100-clock";
- #clock-cells = <1>;
- #reset-cells = <2>;
- reg = <0x0 0x12010000 0x0 0x10000>;
- };
- };
-...
--
2.43.0
From: Yang Xiwen <[email protected]>
Add CRG driver for Hi3798MV200 SoC. CRG(Clock and Reset
Generator) module generates clock and reset signals used
by other module blocks on SoC.
Only currently used clocks are added. Clocks without mainline
user are omitted.
Signed-off-by: Yang Xiwen <[email protected]>
---
drivers/clk/hisilicon/Kconfig | 8 +
drivers/clk/hisilicon/Makefile | 1 +
drivers/clk/hisilicon/crg-hi3798mv200.c | 436 ++++++++++++++++++++++++++++++++
3 files changed, 445 insertions(+)
diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
index c1ec75aa4ccd..fab8059240b7 100644
--- a/drivers/clk/hisilicon/Kconfig
+++ b/drivers/clk/hisilicon/Kconfig
@@ -45,6 +45,14 @@ config COMMON_CLK_HI3798CV200
help
Build the clock driver for hi3798cv200.
+config COMMON_CLK_HI3798MV200
+ tristate "Hi3798MV200 Clock Driver"
+ depends on ARCH_HISI || COMPILE_TEST
+ select RESET_HISI
+ default ARCH_HISI
+ help
+ Build the clock driver for hi3798mv200.
+
config COMMON_CLK_HI6220
bool "Hi6220 Clock Driver"
depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
index 2978e56cb876..7acb63e909bd 100644
--- a/drivers/clk/hisilicon/Makefile
+++ b/drivers/clk/hisilicon/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_COMMON_CLK_HI3559A) += clk-hi3559a.o
obj-$(CONFIG_COMMON_CLK_HI3660) += clk-hi3660.o
obj-$(CONFIG_COMMON_CLK_HI3670) += clk-hi3670.o
obj-$(CONFIG_COMMON_CLK_HI3798CV200) += crg-hi3798cv200.o
+obj-$(CONFIG_COMMON_CLK_HI3798MV200) += crg-hi3798mv200.o
obj-$(CONFIG_COMMON_CLK_HI6220) += clk-hi6220.o
obj-$(CONFIG_RESET_HISI) += reset.o
obj-$(CONFIG_STUB_CLK_HI6220) += clk-hi6220-stub.o
diff --git a/drivers/clk/hisilicon/crg-hi3798mv200.c b/drivers/clk/hisilicon/crg-hi3798mv200.c
new file mode 100644
index 000000000000..756deed9303f
--- /dev/null
+++ b/drivers/clk/hisilicon/crg-hi3798mv200.c
@@ -0,0 +1,436 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hi3798MV200 Clock and Reset Generator Driver
+ *
+ * Copyright (c) 2024 Yang Xiwen <[email protected]>
+ * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
+ */
+
+#include <dt-bindings/clock/histb-clock.h>
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include "clk.h"
+#include "crg.h"
+#include "reset.h"
+
+/* hi3798MV200 core CRG */
+#define HI3798MV200_INNER_CLK_OFFSET 64
+#define HI3798MV200_FIXED_3M 65
+#define HI3798MV200_FIXED_12M 66
+#define HI3798MV200_FIXED_24M 67
+#define HI3798MV200_FIXED_25M 68
+#define HI3798MV200_FIXED_27M 69
+#define HI3798MV200_FIXED_48M 70
+#define HI3798MV200_FIXED_50M 71
+#define HI3798MV200_FIXED_54M 72
+#define HI3798MV200_FIXED_60M 73
+#define HI3798MV200_FIXED_75M 74
+#define HI3798MV200_FIXED_100M 75
+#define HI3798MV200_FIXED_125M 76
+#define HI3798MV200_FIXED_150M 77
+#define HI3798MV200_FIXED_166P5M 78
+#define HI3798MV200_FIXED_200M 79
+#define HI3798MV200_MMC_MUX 80
+#define HI3798MV200_SDIO0_MUX 81
+#define HI3798MV200_SDIO1_MUX 82
+#define HI3798MV200_COMBPHY0_MUX 83
+#define HI3798MV200_FEMAC_MUX 84
+#define HI3798MV200_GMAC_MUX 85
+
+#define HI3798MV200_CRG_NR_CLKS 128
+
+static const struct hisi_fixed_rate_clock hi3798mv200_fixed_rate_clks[] = {
+ { HISTB_OSC_CLK, "clk_osc", NULL, 0, 24000000, },
+ { HISTB_APB_CLK, "clk_apb", NULL, 0, 100000000, },
+ { HISTB_AHB_CLK, "clk_ahb", NULL, 0, 200000000, },
+ { HI3798MV200_FIXED_3M, "3m", NULL, 0, 3000000, },
+ { HI3798MV200_FIXED_12M, "12m", NULL, 0, 12000000, },
+ { HI3798MV200_FIXED_24M, "24m", NULL, 0, 24000000, },
+ { HI3798MV200_FIXED_25M, "25m", NULL, 0, 25000000, },
+ { HI3798MV200_FIXED_27M, "27m", NULL, 0, 27000000, },
+ { HI3798MV200_FIXED_48M, "48m", NULL, 0, 48000000, },
+ { HI3798MV200_FIXED_50M, "50m", NULL, 0, 50000000, },
+ { HI3798MV200_FIXED_54M, "54m", NULL, 0, 54000000, },
+ { HI3798MV200_FIXED_60M, "60m", NULL, 0, 60000000, },
+ { HI3798MV200_FIXED_75M, "75m", NULL, 0, 75000000, },
+ { HI3798MV200_FIXED_100M, "100m", NULL, 0, 100000000, },
+ { HI3798MV200_FIXED_125M, "125m", NULL, 0, 125000000, },
+ { HI3798MV200_FIXED_150M, "150m", NULL, 0, 150000000, },
+ { HI3798MV200_FIXED_166P5M, "166p5m", NULL, 0, 165000000, },
+ { HI3798MV200_FIXED_200M, "200m", NULL, 0, 200000000, },
+};
+
+static const char *const mmc_mux_p[] = {
+ "100m", "50m", "25m", "200m", "150m" };
+static u32 mmc_mux_table[] = {0, 1, 2, 3, 6};
+
+static const char *const comphy_mux_p[] = {
+ "25m", "100m"};
+static u32 comphy_mux_table[] = {0, 1};
+
+static const char *const sdio_mux_p[] = {
+ "100m", "50m", "150m", "25m" };
+static u32 sdio_mux_table[] = {0, 1, 2, 3};
+
+static const char *const femac_mux_p[] = {
+ "54m", "27m" };
+static const char *const gmac_mux_p[] = {
+ "125m", "75m" };
+static u32 eth_mux_table[] = {0, 1};
+
+static struct hisi_mux_clock hi3798mv200_mux_clks[] = {
+ { HI3798MV200_MMC_MUX, "mmc_mux", mmc_mux_p, ARRAY_SIZE(mmc_mux_p),
+ 0, 0xa0, 8, 3, CLK_MUX_ROUND_CLOSEST, mmc_mux_table, },
+ { HI3798MV200_COMBPHY0_MUX, "combphy0_mux", comphy_mux_p,
+ ARRAY_SIZE(comphy_mux_p), 0, 0x188, 3, 1, 0, comphy_mux_table, },
+ { HI3798MV200_SDIO0_MUX, "sdio0_mux", sdio_mux_p, ARRAY_SIZE(sdio_mux_p),
+ 0, 0x9c, 8, 2, CLK_MUX_ROUND_CLOSEST, sdio_mux_table, },
+ { HI3798MV200_SDIO1_MUX, "sdio1_mux", sdio_mux_p, ARRAY_SIZE(sdio_mux_p),
+ 0, 0x28c, 8, 2, CLK_MUX_ROUND_CLOSEST, sdio_mux_table, },
+ { HI3798MV200_FEMAC_MUX, "femac_mux", femac_mux_p, ARRAY_SIZE(femac_mux_p),
+ 0, 0xd0, 2, 1, 0, eth_mux_table, },
+ { HI3798MV200_GMAC_MUX, "gmac_mux", gmac_mux_p, ARRAY_SIZE(gmac_mux_p),
+ 0, 0xcc, 7, 1, 0, eth_mux_table, },
+};
+
+static u32 mmc_phase_regvals[] = {0, 1, 2, 3, 4, 5, 6, 7};
+static u32 mmc_phase_degrees[] = {0, 45, 90, 135, 180, 225, 270, 315};
+
+static struct hisi_phase_clock hi3798mv200_phase_clks[] = {
+ { HISTB_SDIO0_SAMPLE_CLK, "sdio0_sample", "clk_sdio0_ciu",
+ 0, 0x9c, 12, 3, mmc_phase_degrees,
+ mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+ { HISTB_SDIO0_DRV_CLK, "sdio0_drive", "clk_sdio0_ciu",
+ 0, 0x9c, 16, 3, mmc_phase_degrees,
+ mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+ { HISTB_SDIO1_SAMPLE_CLK, "sdio1_sample", "clk_sdio1_ciu",
+ 0, 0x28c, 12, 3, mmc_phase_degrees,
+ mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+ { HISTB_SDIO1_DRV_CLK, "sdio1_drive", "clk_sdio1_ciu",
+ 0, 0x28c, 16, 3, mmc_phase_degrees,
+ mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+ { HISTB_MMC_SAMPLE_CLK, "mmc_sample", "clk_mmc_ciu",
+ 0, 0xa0, 12, 3, mmc_phase_degrees,
+ mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+ { HISTB_MMC_DRV_CLK, "mmc_drive", "clk_mmc_ciu",
+ 0, 0xa0, 16, 3, mmc_phase_degrees,
+ mmc_phase_regvals, ARRAY_SIZE(mmc_phase_regvals) },
+};
+
+static const struct hisi_gate_clock hi3798mv200_gate_clks[] = {
+ /* UART */
+ { HISTB_UART2_CLK, "clk_uart2", "75m",
+ CLK_SET_RATE_PARENT, 0x68, 4, 0, },
+ { HISTB_UART3_CLK, "clk_uart3", "75m",
+ CLK_SET_RATE_PARENT, 0x68, 6, 0, },
+ /* SPI */
+ { HISTB_SPI0_CLK, "clk_spi0", "clk_apb",
+ CLK_SET_RATE_PARENT, 0x70, 0, 0, },
+ /* I2C */
+ { HISTB_I2C0_CLK, "clk_i2c0", "clk_apb",
+ CLK_SET_RATE_PARENT, 0x6c, 4, 0, },
+ { HISTB_I2C1_CLK, "clk_i2c1", "clk_apb",
+ CLK_SET_RATE_PARENT, 0x6c, 8, 0, },
+ { HISTB_I2C2_CLK, "clk_i2c2", "clk_apb",
+ CLK_SET_RATE_PARENT, 0x6c, 12, 0, },
+ /* SDIO */
+ { HISTB_SDIO0_BIU_CLK, "clk_sdio0_biu", "200m",
+ CLK_SET_RATE_PARENT, 0x9c, 0, 0, },
+ { HISTB_SDIO0_CIU_CLK, "clk_sdio0_ciu", "sdio0_mux",
+ CLK_SET_RATE_PARENT, 0x9c, 1, 0, },
+ { HISTB_SDIO1_BIU_CLK, "clk_sdio1_biu", "200m",
+ CLK_SET_RATE_PARENT, 0x28c, 0, 0, },
+ { HISTB_SDIO1_CIU_CLK, "clk_sdio1_ciu", "sdio1_mux",
+ CLK_SET_RATE_PARENT, 0x28c, 1, 0, },
+ /* EMMC */
+ { HISTB_MMC_BIU_CLK, "clk_mmc_biu", "200m",
+ CLK_SET_RATE_PARENT, 0xa0, 0, 0, },
+ { HISTB_MMC_CIU_CLK, "clk_mmc_ciu", "mmc_mux",
+ CLK_SET_RATE_PARENT, 0xa0, 1, 0, },
+ /* Ethernet */
+ { HI3798MV200_GMAC_CLK, "clk_gmac", "gmac_mux",
+ CLK_SET_RATE_PARENT, 0xcc, 2, 0, },
+ { HI3798MV200_GMACIF_CLK, "clk_gmacif", "clk_ahb",
+ CLK_SET_RATE_PARENT, 0xcc, 0, 0, },
+ { HI3798MV200_FEMAC_CLK, "clk_femac", "femac_mux",
+ CLK_SET_RATE_PARENT, 0xd0, 1, 0, },
+ { HI3798MV200_FEMACIF_CLK, "clk_femacif", "clk_ahb",
+ CLK_SET_RATE_PARENT, 0xd0, 0, 0, },
+ { HI3798MV200_FEPHY_CLK, "clk_fephy", "25m",
+ CLK_SET_RATE_PARENT, 0x388, 0, 0, },
+ /* COMBPHY0 */
+ { HISTB_COMBPHY0_CLK, "clk_combphy0", "combphy0_mux",
+ CLK_SET_RATE_PARENT, 0x188, 0, 0, },
+ /* USB2 */
+ { HISTB_USB2_BUS_CLK, "clk_u2_bus", "clk_ahb",
+ CLK_SET_RATE_PARENT, 0xb8, 0, 0, },
+ { HISTB_USB2_PHY_CLK, "clk_u2_phy", "60m",
+ CLK_SET_RATE_PARENT, 0xb8, 4, 0, },
+ { HISTB_USB2_12M_CLK, "clk_u2_12m", "12m",
+ CLK_SET_RATE_PARENT, 0xb8, 2, 0 },
+ { HISTB_USB2_48M_CLK, "clk_u2_48m", "48m",
+ CLK_SET_RATE_PARENT, 0xb8, 1, 0 },
+ { HISTB_USB2_UTMI0_CLK, "clk_u2_utmi0", "60m",
+ CLK_SET_RATE_PARENT, 0xb8, 5, 0 },
+ { HISTB_USB2_UTMI1_CLK, "clk_u2_utmi1", "60m",
+ CLK_SET_RATE_PARENT, 0xb8, 6, 0 },
+ { HISTB_USB2_OTG_UTMI_CLK, "clk_u2_otg_utmi", "60m",
+ CLK_SET_RATE_PARENT, 0xb8, 3, 0 },
+ { HISTB_USB2_PHY1_REF_CLK, "clk_u2_phy1_ref", "24m",
+ CLK_SET_RATE_PARENT, 0xbc, 0, 0 },
+ { HISTB_USB2_PHY2_REF_CLK, "clk_u2_phy2_ref", "24m",
+ CLK_SET_RATE_PARENT, 0xbc, 2, 0 },
+ /* USB3 bus */
+ { HISTB_USB3_GM_CLK, "clk_u3_gm", "clk_ahb",
+ CLK_SET_RATE_PARENT, 0xb0, 6, 0 },
+ { HISTB_USB3_GS_CLK, "clk_u3_gs", "clk_ahb",
+ CLK_SET_RATE_PARENT, 0xb0, 5, 0 },
+ { HISTB_USB3_BUS_CLK, "clk_u3_bus", "clk_ahb",
+ CLK_SET_RATE_PARENT, 0xb0, 0, 0 },
+ /* USB3 ctrl */
+ { HISTB_USB3_SUSPEND_CLK, "clk_u3_suspend", NULL,
+ CLK_SET_RATE_PARENT, 0xb0, 2, 0 },
+ { HISTB_USB3_PIPE_CLK, "clk_u3_pipe", NULL,
+ CLK_SET_RATE_PARENT, 0xb0, 3, 0 },
+ { HISTB_USB3_REF_CLK, "clk_u3_ref", "125m",
+ CLK_SET_RATE_PARENT, 0xb0, 1, 0 },
+ { HISTB_USB3_UTMI_CLK, "clk_u3_utmi", "60m",
+ CLK_SET_RATE_PARENT, 0xb0, 4, 0 },
+ /* Watchdog */
+ { HISTB_WDG0_CLK, "clk_wdg0", "24m",
+ CLK_SET_RATE_PARENT, 0x178, 0, 0 },
+};
+
+static struct hisi_clock_data *hi3798mv200_clk_register(
+ struct platform_device *pdev)
+{
+ struct hisi_clock_data *clk_data;
+ int ret;
+
+ clk_data = hisi_clk_alloc(pdev, HI3798MV200_CRG_NR_CLKS);
+ if (!clk_data)
+ return ERR_PTR(-ENOMEM);
+
+ /* hisi_phase_clock is resource managed */
+ ret = hisi_clk_register_phase(&pdev->dev,
+ hi3798mv200_phase_clks,
+ ARRAY_SIZE(hi3798mv200_phase_clks),
+ clk_data);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = hisi_clk_register_fixed_rate(hi3798mv200_fixed_rate_clks,
+ ARRAY_SIZE(hi3798mv200_fixed_rate_clks),
+ clk_data);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = hisi_clk_register_mux(hi3798mv200_mux_clks,
+ ARRAY_SIZE(hi3798mv200_mux_clks),
+ clk_data);
+ if (ret)
+ goto unregister_fixed_rate;
+
+ ret = hisi_clk_register_gate(hi3798mv200_gate_clks,
+ ARRAY_SIZE(hi3798mv200_gate_clks),
+ clk_data);
+ if (ret)
+ goto unregister_mux;
+
+ ret = of_clk_add_provider(pdev->dev.of_node,
+ of_clk_src_onecell_get, &clk_data->clk_data);
+ if (ret)
+ goto unregister_gate;
+
+ return clk_data;
+
+unregister_gate:
+ hisi_clk_unregister_gate(hi3798mv200_gate_clks,
+ ARRAY_SIZE(hi3798mv200_gate_clks),
+ clk_data);
+unregister_mux:
+ hisi_clk_unregister_mux(hi3798mv200_mux_clks,
+ ARRAY_SIZE(hi3798mv200_mux_clks),
+ clk_data);
+unregister_fixed_rate:
+ hisi_clk_unregister_fixed_rate(hi3798mv200_fixed_rate_clks,
+ ARRAY_SIZE(hi3798mv200_fixed_rate_clks),
+ clk_data);
+ return ERR_PTR(ret);
+}
+
+static void hi3798mv200_clk_unregister(struct platform_device *pdev)
+{
+ struct hisi_crg_dev *crg = platform_get_drvdata(pdev);
+
+ of_clk_del_provider(pdev->dev.of_node);
+
+ hisi_clk_unregister_gate(hi3798mv200_gate_clks,
+ ARRAY_SIZE(hi3798mv200_gate_clks),
+ crg->clk_data);
+ hisi_clk_unregister_mux(hi3798mv200_mux_clks,
+ ARRAY_SIZE(hi3798mv200_mux_clks),
+ crg->clk_data);
+ hisi_clk_unregister_fixed_rate(hi3798mv200_fixed_rate_clks,
+ ARRAY_SIZE(hi3798mv200_fixed_rate_clks),
+ crg->clk_data);
+}
+
+static const struct hisi_crg_funcs hi3798mv200_crg_funcs = {
+ .register_clks = hi3798mv200_clk_register,
+ .unregister_clks = hi3798mv200_clk_unregister,
+};
+
+/* hi3798MV200 sysctrl CRG */
+
+#define HI3798MV200_SYSCTRL_INNER_CLK_OFFSET 16
+#define HI3798MV200_UART0_MUX 17
+
+#define HI3798MV200_SYSCTRL_NR_CLKS 32
+
+static const char *const uart0_mux[] = {
+ "3m", "75m" };
+static u32 uart0_mux_table[] = {0, 1};
+
+static const struct hisi_mux_clock hi3798mv200_sysctrl_mux_clks[] = {
+ { HI3798MV200_UART0_MUX, "uart0_mux", uart0_mux, ARRAY_SIZE(uart0_mux),
+ CLK_SET_RATE_PARENT, 0x48, 29, 1, 0, uart0_mux_table, },
+};
+
+static const struct hisi_gate_clock hi3798mv200_sysctrl_gate_clks[] = {
+ { HISTB_IR_CLK, "clk_ir", "24m",
+ CLK_SET_RATE_PARENT, 0x48, 4, 0, },
+ { HISTB_TIMER01_CLK, "clk_timer01", "24m",
+ CLK_SET_RATE_PARENT, 0x48, 6, 0, },
+ { HISTB_UART0_CLK, "clk_uart0", "uart0_mux",
+ CLK_SET_RATE_PARENT, 0x48, 12, 0, },
+};
+
+static struct hisi_clock_data *hi3798mv200_sysctrl_clk_register(
+ struct platform_device *pdev)
+{
+ struct hisi_clock_data *clk_data;
+ int ret;
+
+ clk_data = hisi_clk_alloc(pdev, HI3798MV200_SYSCTRL_NR_CLKS);
+ if (!clk_data)
+ return ERR_PTR(-ENOMEM);
+
+ ret = hisi_clk_register_mux(hi3798mv200_sysctrl_mux_clks,
+ ARRAY_SIZE(hi3798mv200_sysctrl_mux_clks),
+ clk_data);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = hisi_clk_register_gate(hi3798mv200_sysctrl_gate_clks,
+ ARRAY_SIZE(hi3798mv200_sysctrl_gate_clks),
+ clk_data);
+ if (ret)
+ goto unregister_mux;
+
+ ret = of_clk_add_provider(pdev->dev.of_node,
+ of_clk_src_onecell_get, &clk_data->clk_data);
+ if (ret)
+ goto unregister_gate;
+
+ return clk_data;
+
+unregister_gate:
+ hisi_clk_unregister_gate(hi3798mv200_sysctrl_gate_clks,
+ ARRAY_SIZE(hi3798mv200_sysctrl_gate_clks),
+ clk_data);
+unregister_mux:
+ hisi_clk_unregister_mux(hi3798mv200_sysctrl_mux_clks,
+ ARRAY_SIZE(hi3798mv200_sysctrl_mux_clks),
+ clk_data);
+ return ERR_PTR(ret);
+}
+
+static void hi3798mv200_sysctrl_clk_unregister(struct platform_device *pdev)
+{
+ struct hisi_crg_dev *crg = platform_get_drvdata(pdev);
+
+ of_clk_del_provider(pdev->dev.of_node);
+
+ hisi_clk_unregister_gate(hi3798mv200_sysctrl_gate_clks,
+ ARRAY_SIZE(hi3798mv200_sysctrl_gate_clks),
+ crg->clk_data);
+ hisi_clk_unregister_mux(hi3798mv200_sysctrl_mux_clks,
+ ARRAY_SIZE(hi3798mv200_sysctrl_mux_clks),
+ crg->clk_data);
+}
+
+static const struct hisi_crg_funcs hi3798mv200_sysctrl_funcs = {
+ .register_clks = hi3798mv200_sysctrl_clk_register,
+ .unregister_clks = hi3798mv200_sysctrl_clk_unregister,
+};
+
+static const struct of_device_id hi3798mv200_crg_match_table[] = {
+ { .compatible = "hisilicon,hi3798mv200-crg",
+ .data = &hi3798mv200_crg_funcs },
+ { .compatible = "hisilicon,hi3798mv200-sysctrl",
+ .data = &hi3798mv200_sysctrl_funcs },
+ { }
+};
+MODULE_DEVICE_TABLE(of, hi3798mv200_crg_match_table);
+
+static int hi3798mv200_crg_probe(struct platform_device *pdev)
+{
+ struct hisi_crg_dev *crg;
+
+ crg = devm_kmalloc(&pdev->dev, sizeof(*crg), GFP_KERNEL);
+ if (!crg)
+ return -ENOMEM;
+
+ crg->funcs = of_device_get_match_data(&pdev->dev);
+ if (!crg->funcs)
+ return -ENOENT;
+
+ crg->rstc = hisi_reset_init(pdev);
+ if (!crg->rstc)
+ return -ENOMEM;
+
+ crg->clk_data = crg->funcs->register_clks(pdev);
+ if (IS_ERR(crg->clk_data)) {
+ hisi_reset_exit(crg->rstc);
+ return PTR_ERR(crg->clk_data);
+ }
+
+ platform_set_drvdata(pdev, crg);
+ return 0;
+}
+
+static int hi3798mv200_crg_remove(struct platform_device *pdev)
+{
+ struct hisi_crg_dev *crg = platform_get_drvdata(pdev);
+
+ hisi_reset_exit(crg->rstc);
+ crg->funcs->unregister_clks(pdev);
+ return 0;
+}
+
+static struct platform_driver hi3798mv200_crg_driver = {
+ .probe = hi3798mv200_crg_probe,
+ .remove = hi3798mv200_crg_remove,
+ .driver = {
+ .name = "hi3798mv200-crg",
+ .of_match_table = hi3798mv200_crg_match_table,
+ },
+};
+
+static int __init hi3798mv200_crg_init(void)
+{
+ return platform_driver_register(&hi3798mv200_crg_driver);
+}
+core_initcall(hi3798mv200_crg_init);
+
+static void __exit hi3798mv200_crg_exit(void)
+{
+ platform_driver_unregister(&hi3798mv200_crg_driver);
+}
+module_exit(hi3798mv200_crg_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("HiSilicon Hi3798MV200 CRG Driver");
--
2.43.0
From: Yang Xiwen <[email protected]>
Add hisilicon,sdmmc-sap-dll compatible. This is a 8 bytes range with two
registers. Mainly used for precise sample phase selection during eMMC
tuning.
Signed-off-by: Yang Xiwen <[email protected]>
---
Documentation/devicetree/bindings/mfd/syscon.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 084b5c2a2a3c..c685d4b36ea4 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -47,6 +47,7 @@ properties:
- hisilicon,hi6220-sramctrl
- hisilicon,pcie-sas-subctrl
- hisilicon,peri-subctrl
+ - hisilicon,sdmmc-sap-dll
- hpe,gxp-sysreg
- intel,lgm-syscon
- loongson,ls1b-syscon
--
2.43.0
On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
>
> According to the datasheet, some clocks are missing, add their
> definitions first.
>
> Some aliases for hi3798mv200 are also introduced.
>
> Signed-off-by: Yang Xiwen <[email protected]>
> ---
> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
> index e64e5770ada6..68a53053586a 100644
> --- a/include/dt-bindings/clock/histb-clock.h
> +++ b/include/dt-bindings/clock/histb-clock.h
> @@ -58,6 +58,27 @@
> #define HISTB_USB3_UTMI_CLK1 48
> #define HISTB_USB3_PIPE_CLK1 49
> #define HISTB_USB3_SUSPEND_CLK1 50
> +#define HISTB_SDIO1_BIU_CLK 51
> +#define HISTB_SDIO1_CIU_CLK 52
> +#define HISTB_SDIO1_DRV_CLK 53
> +#define HISTB_SDIO1_SAMPLE_CLK 54
> +#define HISTB_ETH0_PHY_CLK 55
> +#define HISTB_ETH1_PHY_CLK 56
> +#define HISTB_WDG0_CLK 57
> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
Why? It's anyway placed oddly, the entries are ordered by number/value.
> +#define HISTB_USB2_UTMI1_CLK 58
> +#define HISTB_USB3_REF_CLK 59
> +#define HISTB_USB3_GM_CLK 60
> +#define HISTB_USB3_GS_CLK 61
> +
> +/* Hi3798MV200 specific clocks */
> +
> +// reuse clocks of histb
Don't mix comment styles.
> +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK
> +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK
> +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK
> +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK
> +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK
I don't understand what do you want to achieve here. Clock IDs start
from 0 or 1.
Best regards,
Krzysztof
On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
> +
> +static const struct hisi_crg_funcs hi3798mv200_sysctrl_funcs = {
> + .register_clks = hi3798mv200_sysctrl_clk_register,
> + .unregister_clks = hi3798mv200_sysctrl_clk_unregister,
> +};
> +
> +static const struct of_device_id hi3798mv200_crg_match_table[] = {
> + { .compatible = "hisilicon,hi3798mv200-crg",
> + .data = &hi3798mv200_crg_funcs },
> + { .compatible = "hisilicon,hi3798mv200-sysctrl",
> + .data = &hi3798mv200_sysctrl_funcs },
These are undocumented compatibles. Run checkpatch or properly order
your patchset.
Best regards,
Krzysztof
On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <[email protected]>
>>
>> According to the datasheet, some clocks are missing, add their
>> definitions first.
>>
>> Some aliases for hi3798mv200 are also introduced.
>>
>> Signed-off-by: Yang Xiwen <[email protected]>
>> ---
>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>> index e64e5770ada6..68a53053586a 100644
>> --- a/include/dt-bindings/clock/histb-clock.h
>> +++ b/include/dt-bindings/clock/histb-clock.h
>> @@ -58,6 +58,27 @@
>> #define HISTB_USB3_UTMI_CLK1 48
>> #define HISTB_USB3_PIPE_CLK1 49
>> #define HISTB_USB3_SUSPEND_CLK1 50
>> +#define HISTB_SDIO1_BIU_CLK 51
>> +#define HISTB_SDIO1_CIU_CLK 52
>> +#define HISTB_SDIO1_DRV_CLK 53
>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>> +#define HISTB_ETH0_PHY_CLK 55
>> +#define HISTB_ETH1_PHY_CLK 56
>> +#define HISTB_WDG0_CLK 57
>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
> Why? It's anyway placed oddly, the entries are ordered by number/value.
>
>> +#define HISTB_USB2_UTMI1_CLK 58
>> +#define HISTB_USB3_REF_CLK 59
>> +#define HISTB_USB3_GM_CLK 60
>> +#define HISTB_USB3_GS_CLK 61
>> +
>> +/* Hi3798MV200 specific clocks */
>> +
>> +// reuse clocks of histb
> Don't mix comment styles.
>
>> +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK
>> +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK
>> +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK
>> +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK
>> +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK
> I don't understand what do you want to achieve here. Clock IDs start
> from 0 or 1.
They are aliases. A friendlier name compared to ETH0/1.
>
>
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 2/20/2024 6:11 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>> +
>> +static const struct hisi_crg_funcs hi3798mv200_sysctrl_funcs = {
>> + .register_clks = hi3798mv200_sysctrl_clk_register,
>> + .unregister_clks = hi3798mv200_sysctrl_clk_unregister,
>> +};
>> +
>> +static const struct of_device_id hi3798mv200_crg_match_table[] = {
>> + { .compatible = "hisilicon,hi3798mv200-crg",
>> + .data = &hi3798mv200_crg_funcs },
>> + { .compatible = "hisilicon,hi3798mv200-sysctrl",
>> + .data = &hi3798mv200_sysctrl_funcs },
> These are undocumented compatibles. Run checkpatch or properly order
> your patchset.
It's in patch 5. You mean binding patch first and then driver?
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
>
> This SoC is similar to Hi3798CV200.
>
> Also document the specific DLL regs and add an example for it.
>
> Signed-off-by: Yang Xiwen <[email protected]>
> ---
> .../clock/hisilicon,clock-reset-generator.yaml | 36 ++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> index d37cd892473e..8ee844574eda 100644
> --- a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> +++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> @@ -44,12 +44,17 @@ properties:
> - hisilicon,hi3519-crg
> - hisilicon,hi3798cv200-crg
> - hisilicon,hi3798cv200-sysctrl
> + - hisilicon,hi3798mv200-crg
> + - hisilicon,hi3798mv200-sysctrl
> - const: syscon
> - const: simple-mfd
>
> reg:
> maxItems: 1
>
> + ranges:
> + maxItems: 1
> +
> '#clock-cells':
> const: 1
>
> @@ -87,6 +92,12 @@ properties:
> description: |
> Reset controller for Hi3798CV200 GMAC module
>
> +patternProperties:
> + '.*-dll@[0-9a-f]+':
> + type: object
> + description: |
> + eMMC/SD delay-locked-loop (DLL) register subnode
NAK, now all of the syscons have the DLL node?
Best regards,
Krzysztof
On 20/02/2024 11:12, Yang Xiwen wrote:
> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <[email protected]>
>>>
>>> According to the datasheet, some clocks are missing, add their
>>> definitions first.
>>>
>>> Some aliases for hi3798mv200 are also introduced.
>>>
>>> Signed-off-by: Yang Xiwen <[email protected]>
>>> ---
>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>> 1 file changed, 21 insertions(+)
>>>
>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>> index e64e5770ada6..68a53053586a 100644
>>> --- a/include/dt-bindings/clock/histb-clock.h
>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>> @@ -58,6 +58,27 @@
>>> #define HISTB_USB3_UTMI_CLK1 48
>>> #define HISTB_USB3_PIPE_CLK1 49
>>> #define HISTB_USB3_SUSPEND_CLK1 50
>>> +#define HISTB_SDIO1_BIU_CLK 51
>>> +#define HISTB_SDIO1_CIU_CLK 52
>>> +#define HISTB_SDIO1_DRV_CLK 53
>>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>>> +#define HISTB_ETH0_PHY_CLK 55
>>> +#define HISTB_ETH1_PHY_CLK 56
>>> +#define HISTB_WDG0_CLK 57
>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>>
>>> +#define HISTB_USB2_UTMI1_CLK 58
>>> +#define HISTB_USB3_REF_CLK 59
>>> +#define HISTB_USB3_GM_CLK 60
>>> +#define HISTB_USB3_GS_CLK 61
>>> +
>>> +/* Hi3798MV200 specific clocks */
>>> +
>>> +// reuse clocks of histb
>> Don't mix comment styles.
>>
>>> +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK
>>> +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK
>>> +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK
>>> +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK
>>> +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK
>> I don't understand what do you want to achieve here. Clock IDs start
>> from 0 or 1.
> They are aliases. A friendlier name compared to ETH0/1.
Fix your email client, so it will not remove line breaks before/after
quotes. Your email client makes it unreadable.
Aliases do not bind anything, so you can drop these.
Best regards,
Krzysztof
On 2/20/2024 6:15 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <[email protected]>
>>
>> This SoC is similar to Hi3798CV200.
>>
>> Also document the specific DLL regs and add an example for it.
>>
>> Signed-off-by: Yang Xiwen <[email protected]>
>> ---
>> .../clock/hisilicon,clock-reset-generator.yaml | 36 ++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> index d37cd892473e..8ee844574eda 100644
>> --- a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> +++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> @@ -44,12 +44,17 @@ properties:
>> - hisilicon,hi3519-crg
>> - hisilicon,hi3798cv200-crg
>> - hisilicon,hi3798cv200-sysctrl
>> + - hisilicon,hi3798mv200-crg
>> + - hisilicon,hi3798mv200-sysctrl
>> - const: syscon
>> - const: simple-mfd
>>
>> reg:
>> maxItems: 1
>>
>> + ranges:
>> + maxItems: 1
>> +
>> '#clock-cells':
>> const: 1
>>
>> @@ -87,6 +92,12 @@ properties:
>> description: |
>> Reset controller for Hi3798CV200 GMAC module
>>
>> +patternProperties:
>> + '.*-dll@[0-9a-f]+':
>> + type: object
>> + description: |
>> + eMMC/SD delay-locked-loop (DLL) register subnode
> NAK, now all of the syscons have the DLL node?
Oops. Forgot to remove this. it should be removed since it's now not
used anymore.
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 20/02/2024 11:14, Yang Xiwen wrote:
> On 2/20/2024 6:11 PM, Krzysztof Kozlowski wrote:
>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>> +
>>> +static const struct hisi_crg_funcs hi3798mv200_sysctrl_funcs = {
>>> + .register_clks = hi3798mv200_sysctrl_clk_register,
>>> + .unregister_clks = hi3798mv200_sysctrl_clk_unregister,
>>> +};
>>> +
>>> +static const struct of_device_id hi3798mv200_crg_match_table[] = {
>>> + { .compatible = "hisilicon,hi3798mv200-crg",
>>> + .data = &hi3798mv200_crg_funcs },
>>> + { .compatible = "hisilicon,hi3798mv200-sysctrl",
>>> + .data = &hi3798mv200_sysctrl_funcs },
>> These are undocumented compatibles. Run checkpatch or properly order
>> your patchset.
> It's in patch 5. You mean binding patch first and then driver?
https://elixir.bootlin.com/linux/v6.7/source/Documentation/devicetree/bindings/submitting-patches.rst
Best regards,
Krzysztof
On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
>
> We don't need so many separated and duplicated dt-binding files. Merge
> them all and convert them to YAML.
What was exactly duplicated? You created unspecific, lose binding...
Why this is RFC? RFC means we should not review.
>
> Signed-off-by: Yang Xiwen <[email protected]>
> ---
> .../devicetree/bindings/clock/hi3660-clock.txt | 47 -------
> .../devicetree/bindings/clock/hi3670-clock.txt | 43 -------
> .../devicetree/bindings/clock/hi6220-clock.txt | 52 --------
> .../devicetree/bindings/clock/hisi-crg.txt | 50 --------
> .../clock/hisilicon,clock-reset-generator.yaml | 139 +++++++++++++++++++++
> .../clock/hisilicon,hi3559av100-clock.yaml | 59 ---------
> 6 files changed, 139 insertions(+), 251 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> new file mode 100644
> index 000000000000..d37cd892473e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/hisilicon,clock-reset-generator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hisilicon SOC Clock and Reset Generator (CRG) module
> +
> +maintainers:
> + - Yang Xiwen <[email protected]>
> +
> +description: |
> + Hisilicon SOC clock control module which supports the clocks, resets and
> + power domains on various SoCs.
> +
> +properties:
> + compatible:
> + minItems: 1
No, it does not work like that. Compatibles are fixed, not fluid. It's
quite a hint that your merging is wrong approach.
> + items:
> + - enum:
> + - hisilicon,hi3559av100-clock
> + - hisilicon,hi3559av100-shub-clock
> + - hisilicon,hi3660-crgctrl
> + - hisilicon,hi3660-pctrl
> + - hisilicon,hi3660-pmuctrl
> + - hisilicon,hi3660-sctrl
> + - hisilicon,hi3660-iomcu
> + - hisilicon,hi3660-stub-clk
> + - hisilicon,hi3670-crgctrl
> + - hisilicon,hi3670-pctrl
> + - hisilicon,hi3670-pmuctrl
> + - hisilicon,hi3670-sctrl
> + - hisilicon,hi3670-iomcu
> + - hisilicon,hi3670-media1-crg
> + - hisilicon,hi3670-media2-crg
> + - hisilicon,hi6220-acpu-sctrl
> + - hisilicon,hi6220-aoctrl
> + - hisilicon,hi6220-sysctrl
> + - hisilicon,hi6220-mediactrl
> + - hisilicon,hi6220-pmctrl
> + - hisilicon,hi6220-stub-clk
> + - hisilicon,hi3516cv300-crg
> + - hisilicon,hi3516cv300-sysctrl
> + - hisilicon,hi3519-crg
> + - hisilicon,hi3798cv200-crg
> + - hisilicon,hi3798cv200-sysctrl
> + - const: syscon
> + - const: simple-mfd
> +
> + reg:
> + maxItems: 1
> +
> + '#clock-cells':
> + const: 1
> +
> + '#reset-cells':
> + enum: [1, 2]
> + description: |
Previous bindings has only 2. Your patch is difficult to review and
understand.
> + First cell is reset request register offset.
> + Second cell is bit offset in reset request register.
All of these are reset controllers? I doubt.
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 1
All of these have children? No, sorry, but this merging does not make
any sense.
> +
> + mboxes:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description: |
> + Phandle to the mailbox for sending msg to MCU
> + (See ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
> +
> + mbox-names:
> + $ref: /schemas/types.yaml#/definitions/string-array
> + description: |
> + Names of the mailboxes.
> +
> + hisilicon,hi6220-clk-sram:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: |
> + Phandle to the syscon managing the SoC internal sram
> + the driver needs using the sram to pass parameters for frequency change.
> +
> + reset-controller:
> + type: object
> + description: |
> + Reset controller for Hi3798CV200 GMAC module
> +
> +required:
> + - compatible
> + - '#clock-cells'
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + not:
> + contains:
> + enum:
> + - hisilicon,hi3798cv200-crg
> + then:
> + properties:
> + reset-controller: false
> + - oneOf:
> + - required:
> + - hisilicon,hi6220-clk-sram
> + - required:
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/hi3559av100-clock.h>
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + clock-controller@12010000 {
> + compatible = "hisilicon,hi3559av100-clock";
> + #clock-cells = <1>;
> + #reset-cells = <2>;
> + reg = <0x0 0x12010000 0x0 0x10000>;
> + };
> + };
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/clock/hi3660-clock.h>
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + clock-controller@fff35000 {
> + compatible = "hisilicon,hi3660-crgctrl", "syscon";
> + reg = <0x0 0xfff35000 0x0 0x1000>;
> + #clock-cells = <1>;
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml b/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
> deleted file mode 100644
> index 3ceb29cec704..000000000000
> --- a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
> +++ /dev/null
> @@ -1,59 +0,0 @@
> -# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> -%YAML 1.2
> ----
> -$id: http://devicetree.org/schemas/clock/hisilicon,hi3559av100-clock.yaml#
> -$schema: http://devicetree.org/meta-schemas/core.yaml#
NAK, not related patch.
Please split all your patches into logical chunks.
Please read submitting-patches *BEFORE SENDING* further submissions.
Best regards,
Krzysztof
On 20/02/2024 11:52, Yang Xiwen wrote:
> On 2/20/2024 6:14 PM, Krzysztof Kozlowski wrote:
>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <[email protected]>
>>>
>>> We don't need so many separated and duplicated dt-binding files. Merge
>>> them all and convert them to YAML.
>> What was exactly duplicated? You created unspecific, lose binding...
>
> You can take at the drivers at drivers/clk/hisilicon. All of them use
> the same sets of APIs to register a few clocks and resets. That's why i
> think they should be merged.
Drivers don't really matter for bindings. That's not a valid argument.
Creating invalid combinations and lose bindings is not the answer to
duplication of few parts.
Best regards,
Krzysztof
On 2/20/2024 6:14 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <[email protected]>
>>
>> We don't need so many separated and duplicated dt-binding files. Merge
>> them all and convert them to YAML.
> What was exactly duplicated? You created unspecific, lose binding...
You can take at the drivers at drivers/clk/hisilicon. All of them use
the same sets of APIs to register a few clocks and resets. That's why i
think they should be merged.
>
> Why this is RFC? RFC means we should not review.
>
>> Signed-off-by: Yang Xiwen <[email protected]>
>> ---
>> .../devicetree/bindings/clock/hi3660-clock.txt | 47 -------
>> .../devicetree/bindings/clock/hi3670-clock.txt | 43 -------
>> .../devicetree/bindings/clock/hi6220-clock.txt | 52 --------
>> .../devicetree/bindings/clock/hisi-crg.txt | 50 --------
>> .../clock/hisilicon,clock-reset-generator.yaml | 139 +++++++++++++++++++++
>> .../clock/hisilicon,hi3559av100-clock.yaml | 59 ---------
>> 6 files changed, 139 insertions(+), 251 deletions(-)
>>
>
>> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> new file mode 100644
>> index 000000000000..d37cd892473e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/hisilicon,clock-reset-generator.yaml
>> @@ -0,0 +1,139 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/hisilicon,clock-reset-generator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Hisilicon SOC Clock and Reset Generator (CRG) module
>> +
>> +maintainers:
>> + - Yang Xiwen <[email protected]>
>> +
>> +description: |
>> + Hisilicon SOC clock control module which supports the clocks, resets and
>> + power domains on various SoCs.
>> +
>> +properties:
>> + compatible:
>> + minItems: 1
> No, it does not work like that. Compatibles are fixed, not fluid. It's
> quite a hint that your merging is wrong approach.
>
>
>> + items:
>> + - enum:
>> + - hisilicon,hi3559av100-clock
>> + - hisilicon,hi3559av100-shub-clock
>> + - hisilicon,hi3660-crgctrl
>> + - hisilicon,hi3660-pctrl
>> + - hisilicon,hi3660-pmuctrl
>> + - hisilicon,hi3660-sctrl
>> + - hisilicon,hi3660-iomcu
>> + - hisilicon,hi3660-stub-clk
>> + - hisilicon,hi3670-crgctrl
>> + - hisilicon,hi3670-pctrl
>> + - hisilicon,hi3670-pmuctrl
>> + - hisilicon,hi3670-sctrl
>> + - hisilicon,hi3670-iomcu
>> + - hisilicon,hi3670-media1-crg
>> + - hisilicon,hi3670-media2-crg
>> + - hisilicon,hi6220-acpu-sctrl
>> + - hisilicon,hi6220-aoctrl
>> + - hisilicon,hi6220-sysctrl
>> + - hisilicon,hi6220-mediactrl
>> + - hisilicon,hi6220-pmctrl
>> + - hisilicon,hi6220-stub-clk
>> + - hisilicon,hi3516cv300-crg
>> + - hisilicon,hi3516cv300-sysctrl
>> + - hisilicon,hi3519-crg
>> + - hisilicon,hi3798cv200-crg
>> + - hisilicon,hi3798cv200-sysctrl
>> + - const: syscon
>> + - const: simple-mfd
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + '#clock-cells':
>> + const: 1
>> +
>> + '#reset-cells':
>> + enum: [1, 2]
>> + description: |
> Previous bindings has only 2. Your patch is difficult to review and
> understand.
>
>> + First cell is reset request register offset.
>> + Second cell is bit offset in reset request register.
> All of these are reset controllers? I doubt.
>
>> +
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 1
> All of these have children? No, sorry, but this merging does not make
> any sense.
>
>> +
>> + mboxes:
>> + $ref: /schemas/types.yaml#/definitions/phandle-array
>> + description: |
>> + Phandle to the mailbox for sending msg to MCU
>> + (See ../mailbox/hisilicon,hi3660-mailbox.txt for more info)
>> +
>> + mbox-names:
>> + $ref: /schemas/types.yaml#/definitions/string-array
>> + description: |
>> + Names of the mailboxes.
>> +
>> + hisilicon,hi6220-clk-sram:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: |
>> + Phandle to the syscon managing the SoC internal sram
>> + the driver needs using the sram to pass parameters for frequency change.
>> +
>> + reset-controller:
>> + type: object
>> + description: |
>> + Reset controller for Hi3798CV200 GMAC module
>> +
>> +required:
>> + - compatible
>> + - '#clock-cells'
>> +
>> +allOf:
>> + - if:
>> + properties:
>> + compatible:
>> + not:
>> + contains:
>> + enum:
>> + - hisilicon,hi3798cv200-crg
>> + then:
>> + properties:
>> + reset-controller: false
>> + - oneOf:
>> + - required:
>> + - hisilicon,hi6220-clk-sram
>> + - required:
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/clock/hi3559av100-clock.h>
>> + soc {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> +
>> + clock-controller@12010000 {
>> + compatible = "hisilicon,hi3559av100-clock";
>> + #clock-cells = <1>;
>> + #reset-cells = <2>;
>> + reg = <0x0 0x12010000 0x0 0x10000>;
>> + };
>> + };
>> + - |
>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>> + #include <dt-bindings/clock/hi3660-clock.h>
>> + soc {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> +
>> + clock-controller@fff35000 {
>> + compatible = "hisilicon,hi3660-crgctrl", "syscon";
>> + reg = <0x0 0xfff35000 0x0 0x1000>;
>> + #clock-cells = <1>;
>> + };
>> + };
>> diff --git a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml b/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
>> deleted file mode 100644
>> index 3ceb29cec704..000000000000
>> --- a/Documentation/devicetree/bindings/clock/hisilicon,hi3559av100-clock.yaml
>> +++ /dev/null
>> @@ -1,59 +0,0 @@
>> -# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> -%YAML 1.2
>> ----
>> -$id: http://devicetree.org/schemas/clock/hisilicon,hi3559av100-clock.yaml#
>> -$schema: http://devicetree.org/meta-schemas/core.yaml#
> NAK, not related patch.
Okay. I'll revert most of the changes here. Maybe i should only convert
hisi-crg.txt to yaml. That's what i really cares.
>
> Please split all your patches into logical chunks.
>
> Please read submitting-patches *BEFORE SENDING* further submissions.
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <[email protected]>
>>
>> According to the datasheet, some clocks are missing, add their
>> definitions first.
>>
>> Some aliases for hi3798mv200 are also introduced.
>>
>> Signed-off-by: Yang Xiwen <[email protected]>
>> ---
>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>> index e64e5770ada6..68a53053586a 100644
>> --- a/include/dt-bindings/clock/histb-clock.h
>> +++ b/include/dt-bindings/clock/histb-clock.h
>> @@ -58,6 +58,27 @@
>> #define HISTB_USB3_UTMI_CLK1 48
>> #define HISTB_USB3_PIPE_CLK1 49
>> #define HISTB_USB3_SUSPEND_CLK1 50
>> +#define HISTB_SDIO1_BIU_CLK 51
>> +#define HISTB_SDIO1_CIU_CLK 52
>> +#define HISTB_SDIO1_DRV_CLK 53
>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>> +#define HISTB_ETH0_PHY_CLK 55
>> +#define HISTB_ETH1_PHY_CLK 56
>> +#define HISTB_WDG0_CLK 57
>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
> Why? It's anyway placed oddly, the entries are ordered by number/value.
So this is somewhat broken at the beginning. It named after
histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
after it and increment all the indexes after it? Then the diff would be
very ugly.
>
>> +#define HISTB_USB2_UTMI1_CLK 58
>> +#define HISTB_USB3_REF_CLK 59
>> +#define HISTB_USB3_GM_CLK 60
>> +#define HISTB_USB3_GS_CLK 61
>> +
>> +/* Hi3798MV200 specific clocks */
>> +
>> +// reuse clocks of histb
> Don't mix comment styles.
>
>> +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK
>> +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK
>> +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK
>> +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK
>> +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK
> I don't understand what do you want to achieve here. Clock IDs start
> from 0 or 1.
>
>
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 20/02/2024 15:06, Yang Xiwen wrote:
> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <[email protected]>
>>>
>>> According to the datasheet, some clocks are missing, add their
>>> definitions first.
>>>
>>> Some aliases for hi3798mv200 are also introduced.
>>>
>>> Signed-off-by: Yang Xiwen <[email protected]>
>>> ---
>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>> 1 file changed, 21 insertions(+)
>>>
>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>> index e64e5770ada6..68a53053586a 100644
>>> --- a/include/dt-bindings/clock/histb-clock.h
>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>> @@ -58,6 +58,27 @@
>>> #define HISTB_USB3_UTMI_CLK1 48
>>> #define HISTB_USB3_PIPE_CLK1 49
>>> #define HISTB_USB3_SUSPEND_CLK1 50
>>> +#define HISTB_SDIO1_BIU_CLK 51
>>> +#define HISTB_SDIO1_CIU_CLK 52
>>> +#define HISTB_SDIO1_DRV_CLK 53
>>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>>> +#define HISTB_ETH0_PHY_CLK 55
>>> +#define HISTB_ETH1_PHY_CLK 56
>>> +#define HISTB_WDG0_CLK 57
>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>
>
> So this is somewhat broken at the beginning. It named after
> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
>
>
> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
> after it and increment all the indexes after it? Then the diff would be
> very ugly.
I still don't understand what is the problem you are trying to solve
here. Your commit msg says add missing ID, but that ID -
HISTB_USB2_UTMI_CLK - is already there.
I also do not get why there is a need to rename anything.
Best regards,
Krzysztof
On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote:
> On 20/02/2024 15:06, Yang Xiwen wrote:
>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>>> From: Yang Xiwen <[email protected]>
>>>>
>>>> According to the datasheet, some clocks are missing, add their
>>>> definitions first.
>>>>
>>>> Some aliases for hi3798mv200 are also introduced.
>>>>
>>>> Signed-off-by: Yang Xiwen <[email protected]>
>>>> ---
>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>>> 1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>>> index e64e5770ada6..68a53053586a 100644
>>>> --- a/include/dt-bindings/clock/histb-clock.h
>>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>>> @@ -58,6 +58,27 @@
>>>> #define HISTB_USB3_UTMI_CLK1 48
>>>> #define HISTB_USB3_PIPE_CLK1 49
>>>> #define HISTB_USB3_SUSPEND_CLK1 50
>>>> +#define HISTB_SDIO1_BIU_CLK 51
>>>> +#define HISTB_SDIO1_CIU_CLK 52
>>>> +#define HISTB_SDIO1_DRV_CLK 53
>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>>>> +#define HISTB_ETH0_PHY_CLK 55
>>>> +#define HISTB_ETH1_PHY_CLK 56
>>>> +#define HISTB_WDG0_CLK 57
>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
>>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>>
>> So this is somewhat broken at the beginning. It named after
>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
>>
>>
>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
>> after it and increment all the indexes after it? Then the diff would be
>> very ugly.
> I still don't understand what is the problem you are trying to solve
> here. Your commit msg says add missing ID, but that ID -
> HISTB_USB2_UTMI_CLK - is already there.
>
> I also do not get why there is a need to rename anything.
Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200.
UTMI1 is missing here. For other HiSTB SoCs, there could be even more.
If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without
renaming it to UTMI0. Just like all the other clocks. E.g.
I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK.
>
>
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 20/02/2024 17:19, Yang Xiwen wrote:
> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote:
>> On 20/02/2024 15:06, Yang Xiwen wrote:
>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>>>> From: Yang Xiwen <[email protected]>
>>>>>
>>>>> According to the datasheet, some clocks are missing, add their
>>>>> definitions first.
>>>>>
>>>>> Some aliases for hi3798mv200 are also introduced.
>>>>>
>>>>> Signed-off-by: Yang Xiwen <[email protected]>
>>>>> ---
>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>>>> 1 file changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>>>> index e64e5770ada6..68a53053586a 100644
>>>>> --- a/include/dt-bindings/clock/histb-clock.h
>>>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>>>> @@ -58,6 +58,27 @@
>>>>> #define HISTB_USB3_UTMI_CLK1 48
>>>>> #define HISTB_USB3_PIPE_CLK1 49
>>>>> #define HISTB_USB3_SUSPEND_CLK1 50
>>>>> +#define HISTB_SDIO1_BIU_CLK 51
>>>>> +#define HISTB_SDIO1_CIU_CLK 52
>>>>> +#define HISTB_SDIO1_DRV_CLK 53
>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>>>>> +#define HISTB_ETH0_PHY_CLK 55
>>>>> +#define HISTB_ETH1_PHY_CLK 56
>>>>> +#define HISTB_WDG0_CLK 57
>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
>>>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>>>
>>> So this is somewhat broken at the beginning. It named after
>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
>>>
>>>
>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
>>> after it and increment all the indexes after it? Then the diff would be
>>> very ugly.
>> I still don't understand what is the problem you are trying to solve
>> here. Your commit msg says add missing ID, but that ID -
>> HISTB_USB2_UTMI_CLK - is already there.
>>
>> I also do not get why there is a need to rename anything.
>
>
> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200.
> UTMI1 is missing here. For other HiSTB SoCs, there could be even more.
My comment was under UTMI0. We do not talk about UTMI1...
>
>
> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without
> renaming it to UTMI0. Just like all the other clocks. E.g.
> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK.
Then place it next to old name and explain why it is deprecated with
comment.
Best regards,
Krzysztof
On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote:
> On 20/02/2024 17:19, Yang Xiwen wrote:
>> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote:
>>> On 20/02/2024 15:06, Yang Xiwen wrote:
>>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>>>>> From: Yang Xiwen <[email protected]>
>>>>>>
>>>>>> According to the datasheet, some clocks are missing, add their
>>>>>> definitions first.
>>>>>>
>>>>>> Some aliases for hi3798mv200 are also introduced.
>>>>>>
>>>>>> Signed-off-by: Yang Xiwen <[email protected]>
>>>>>> ---
>>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>>>>> 1 file changed, 21 insertions(+)
>>>>>>
>>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>>>>> index e64e5770ada6..68a53053586a 100644
>>>>>> --- a/include/dt-bindings/clock/histb-clock.h
>>>>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>>>>> @@ -58,6 +58,27 @@
>>>>>> #define HISTB_USB3_UTMI_CLK1 48
>>>>>> #define HISTB_USB3_PIPE_CLK1 49
>>>>>> #define HISTB_USB3_SUSPEND_CLK1 50
>>>>>> +#define HISTB_SDIO1_BIU_CLK 51
>>>>>> +#define HISTB_SDIO1_CIU_CLK 52
>>>>>> +#define HISTB_SDIO1_DRV_CLK 53
>>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>>>>>> +#define HISTB_ETH0_PHY_CLK 55
>>>>>> +#define HISTB_ETH1_PHY_CLK 56
>>>>>> +#define HISTB_WDG0_CLK 57
>>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
>>>>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>>>> So this is somewhat broken at the beginning. It named after
>>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
>>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
>>>>
>>>>
>>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
>>>> after it and increment all the indexes after it? Then the diff would be
>>>> very ugly.
>>> I still don't understand what is the problem you are trying to solve
>>> here. Your commit msg says add missing ID, but that ID -
>>> HISTB_USB2_UTMI_CLK - is already there.
>>>
>>> I also do not get why there is a need to rename anything.
>>
>> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200.
>> UTMI1 is missing here. For other HiSTB SoCs, there could be even more.
> My comment was under UTMI0. We do not talk about UTMI1...
>
>>
>> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without
>> renaming it to UTMI0. Just like all the other clocks. E.g.
>> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK.
> Then place it next to old name and explain why it is deprecated with
> comment.
Do we need to keep the old name? I can fix all the users (only
hi3798cv200.dtsi) in next version and drop this name directly. Is that
okay? Should i insert UTMI1_CLK to the middle and re-index all the
macros after it? Or simply add it to the tail?
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 20/02/2024 17:31, Yang Xiwen wrote:
> On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote:
>> On 20/02/2024 17:19, Yang Xiwen wrote:
>>> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote:
>>>> On 20/02/2024 15:06, Yang Xiwen wrote:
>>>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>>>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>>>>>> From: Yang Xiwen <[email protected]>
>>>>>>>
>>>>>>> According to the datasheet, some clocks are missing, add their
>>>>>>> definitions first.
>>>>>>>
>>>>>>> Some aliases for hi3798mv200 are also introduced.
>>>>>>>
>>>>>>> Signed-off-by: Yang Xiwen <[email protected]>
>>>>>>> ---
>>>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>>>>>> 1 file changed, 21 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>>>>>> index e64e5770ada6..68a53053586a 100644
>>>>>>> --- a/include/dt-bindings/clock/histb-clock.h
>>>>>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>>>>>> @@ -58,6 +58,27 @@
>>>>>>> #define HISTB_USB3_UTMI_CLK1 48
>>>>>>> #define HISTB_USB3_PIPE_CLK1 49
>>>>>>> #define HISTB_USB3_SUSPEND_CLK1 50
>>>>>>> +#define HISTB_SDIO1_BIU_CLK 51
>>>>>>> +#define HISTB_SDIO1_CIU_CLK 52
>>>>>>> +#define HISTB_SDIO1_DRV_CLK 53
>>>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>>>>>>> +#define HISTB_ETH0_PHY_CLK 55
>>>>>>> +#define HISTB_ETH1_PHY_CLK 56
>>>>>>> +#define HISTB_WDG0_CLK 57
>>>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
>>>>>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>>>>> So this is somewhat broken at the beginning. It named after
>>>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
>>>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
>>>>>
>>>>>
>>>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
>>>>> after it and increment all the indexes after it? Then the diff would be
>>>>> very ugly.
>>>> I still don't understand what is the problem you are trying to solve
>>>> here. Your commit msg says add missing ID, but that ID -
>>>> HISTB_USB2_UTMI_CLK - is already there.
>>>>
>>>> I also do not get why there is a need to rename anything.
>>>
>>> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200.
>>> UTMI1 is missing here. For other HiSTB SoCs, there could be even more.
>> My comment was under UTMI0. We do not talk about UTMI1...
>>
>>>
>>> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without
>>> renaming it to UTMI0. Just like all the other clocks. E.g.
>>> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK.
>> Then place it next to old name and explain why it is deprecated with
>> comment.
>
>
> Do we need to keep the old name? I can fix all the users (only
> hi3798cv200.dtsi) in next version and drop this name directly. Is that
All users in all projects? That might be tricky. And even for Linux
kernel, how can you do it in a bisectable way? Just keep old name.
> okay? Should i insert UTMI1_CLK to the middle and re-index all the
> macros after it? Or simply add it to the tail?
Bindings and header constants are ABI, so you cannot change them.
Best regards,
Krzysztof
On 2/21/2024 1:06 AM, Krzysztof Kozlowski wrote:
> On 20/02/2024 17:31, Yang Xiwen wrote:
>> On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote:
>>> On 20/02/2024 17:19, Yang Xiwen wrote:
>>>> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote:
>>>>> On 20/02/2024 15:06, Yang Xiwen wrote:
>>>>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>>>>>>> From: Yang Xiwen <[email protected]>
>>>>>>>>
>>>>>>>> According to the datasheet, some clocks are missing, add their
>>>>>>>> definitions first.
>>>>>>>>
>>>>>>>> Some aliases for hi3798mv200 are also introduced.
>>>>>>>>
>>>>>>>> Signed-off-by: Yang Xiwen <[email protected]>
>>>>>>>> ---
>>>>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>>>>>>> 1 file changed, 21 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>>>>>>> index e64e5770ada6..68a53053586a 100644
>>>>>>>> --- a/include/dt-bindings/clock/histb-clock.h
>>>>>>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>>>>>>> @@ -58,6 +58,27 @@
>>>>>>>> #define HISTB_USB3_UTMI_CLK1 48
>>>>>>>> #define HISTB_USB3_PIPE_CLK1 49
>>>>>>>> #define HISTB_USB3_SUSPEND_CLK1 50
>>>>>>>> +#define HISTB_SDIO1_BIU_CLK 51
>>>>>>>> +#define HISTB_SDIO1_CIU_CLK 52
>>>>>>>> +#define HISTB_SDIO1_DRV_CLK 53
>>>>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>>>>>>>> +#define HISTB_ETH0_PHY_CLK 55
>>>>>>>> +#define HISTB_ETH1_PHY_CLK 56
>>>>>>>> +#define HISTB_WDG0_CLK 57
>>>>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
>>>>>>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>>>>>> So this is somewhat broken at the beginning. It named after
>>>>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
>>>>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
>>>>>>
>>>>>>
>>>>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
>>>>>> after it and increment all the indexes after it? Then the diff would be
>>>>>> very ugly.
>>>>> I still don't understand what is the problem you are trying to solve
>>>>> here. Your commit msg says add missing ID, but that ID -
>>>>> HISTB_USB2_UTMI_CLK - is already there.
>>>>>
>>>>> I also do not get why there is a need to rename anything.
>>>> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200.
>>>> UTMI1 is missing here. For other HiSTB SoCs, there could be even more.
>>> My comment was under UTMI0. We do not talk about UTMI1...
>>>
>>>> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without
>>>> renaming it to UTMI0. Just like all the other clocks. E.g.
>>>> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK.
>>> Then place it next to old name and explain why it is deprecated with
>>> comment.
>>
>> Do we need to keep the old name? I can fix all the users (only
>> hi3798cv200.dtsi) in next version and drop this name directly. Is that
> All users in all projects? That might be tricky. And even for Linux
> kernel, how can you do it in a bisectable way? Just keep old name.
>
>
>> okay? Should i insert UTMI1_CLK to the middle and re-index all the
>> macros after it? Or simply add it to the tail?
> Bindings and header constants are ABI, so you cannot change them.
This file should be renamed to hi3798cv200-clock.h, it shouldn't be
called histb-clock.h from the beginning. Now I have to workaround this
in a dirty way. What if another HiSTB SoC has 3 or more UTMI_CLKs? Do we
need to add more definitions to the end of the file? The file is gonna
to be more and more unreadable with scattered clock definitions.
Do you think it's acceptable to create a new header file instead? I
think we don't need a generic histb-clock.h. Each SoC should maintain
their own clock indexes header file. Maybe we can rename it to
hi3798cv200-clock.h and include it from a new histb-clock.h (also mark
this generic header file deprecated and only for hi3798cv200). Then I'll
create hi3798mv200-clock.h header file instead. So we don't have to
workaround this.
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 20/02/2024 18:29, Yang Xiwen wrote:
> On 2/21/2024 1:06 AM, Krzysztof Kozlowski wrote:
>> On 20/02/2024 17:31, Yang Xiwen wrote:
>>> On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote:
>>>> On 20/02/2024 17:19, Yang Xiwen wrote:
>>>>> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote:
>>>>>> On 20/02/2024 15:06, Yang Xiwen wrote:
>>>>>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote:
>>>>>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote:
>>>>>>>>> From: Yang Xiwen <[email protected]>
>>>>>>>>>
>>>>>>>>> According to the datasheet, some clocks are missing, add their
>>>>>>>>> definitions first.
>>>>>>>>>
>>>>>>>>> Some aliases for hi3798mv200 are also introduced.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Yang Xiwen <[email protected]>
>>>>>>>>> ---
>>>>>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++
>>>>>>>>> 1 file changed, 21 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h
>>>>>>>>> index e64e5770ada6..68a53053586a 100644
>>>>>>>>> --- a/include/dt-bindings/clock/histb-clock.h
>>>>>>>>> +++ b/include/dt-bindings/clock/histb-clock.h
>>>>>>>>> @@ -58,6 +58,27 @@
>>>>>>>>> #define HISTB_USB3_UTMI_CLK1 48
>>>>>>>>> #define HISTB_USB3_PIPE_CLK1 49
>>>>>>>>> #define HISTB_USB3_SUSPEND_CLK1 50
>>>>>>>>> +#define HISTB_SDIO1_BIU_CLK 51
>>>>>>>>> +#define HISTB_SDIO1_CIU_CLK 52
>>>>>>>>> +#define HISTB_SDIO1_DRV_CLK 53
>>>>>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54
>>>>>>>>> +#define HISTB_ETH0_PHY_CLK 55
>>>>>>>>> +#define HISTB_ETH1_PHY_CLK 56
>>>>>>>>> +#define HISTB_WDG0_CLK 57
>>>>>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK
>>>>>>>> Why? It's anyway placed oddly, the entries are ordered by number/value.
>>>>>>> So this is somewhat broken at the beginning. It named after
>>>>>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For
>>>>>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock.
>>>>>>>
>>>>>>>
>>>>>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK
>>>>>>> after it and increment all the indexes after it? Then the diff would be
>>>>>>> very ugly.
>>>>>> I still don't understand what is the problem you are trying to solve
>>>>>> here. Your commit msg says add missing ID, but that ID -
>>>>>> HISTB_USB2_UTMI_CLK - is already there.
>>>>>>
>>>>>> I also do not get why there is a need to rename anything.
>>>>> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200.
>>>>> UTMI1 is missing here. For other HiSTB SoCs, there could be even more.
>>>> My comment was under UTMI0. We do not talk about UTMI1...
>>>>
>>>>> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without
>>>>> renaming it to UTMI0. Just like all the other clocks. E.g.
>>>>> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK.
>>>> Then place it next to old name and explain why it is deprecated with
>>>> comment.
>>>
>>> Do we need to keep the old name? I can fix all the users (only
>>> hi3798cv200.dtsi) in next version and drop this name directly. Is that
>> All users in all projects? That might be tricky. And even for Linux
>> kernel, how can you do it in a bisectable way? Just keep old name.
>>
>>
>>> okay? Should i insert UTMI1_CLK to the middle and re-index all the
>>> macros after it? Or simply add it to the tail?
>> Bindings and header constants are ABI, so you cannot change them.
>
>
> This file should be renamed to hi3798cv200-clock.h, it shouldn't be
> called histb-clock.h from the beginning. Now I have to workaround this
> in a dirty way. What if another HiSTB SoC has 3 or more UTMI_CLKs? Do we
> need to add more definitions to the end of the file? The file is gonna
That's not a big problem, but indeed shows poor design of the driver and
bindings.
> to be more and more unreadable with scattered clock definitions.
>
>
> Do you think it's acceptable to create a new header file instead? I
This depends on the purpose of it. In general every SoC follows that
concept - binding headers are per given clock controller, not even per SoC.
> think we don't need a generic histb-clock.h. Each SoC should maintain
> their own clock indexes header file. Maybe we can rename it to
> hi3798cv200-clock.h and include it from a new histb-clock.h (also mark
> this generic header file deprecated and only for hi3798cv200). Then I'll
> create hi3798mv200-clock.h header file instead. So we don't have to
> workaround this.
Best regards,
Krzysztof