add clk-hi3660
Subsystems (2)
cover add clk-hi3660
Zhangfei Gao (2):
dt-bindings: Document the hi3660 clock bindings
clk: hi3660: Clock driver support for Hisilicon hi3660 SoC
.../devicetree/bindings/clock/hi3660-clock.txt | 25 + drivers/clk/hisilicon/Kconfig | 7 + drivers/clk/hisilicon/Makefile | 1 + drivers/clk/hisilicon/clk-hi3660.c | 601 +++++++++++++++++++++ include/dt-bindings/clock/hi3660-clock.h | 194 +++++++ 5 files changed, 828 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/hi3660-clock.txt create mode 100644 drivers/clk/hisilicon/clk-hi3660.c create mode 100644 include/dt-bindings/clock/hi3660-clock.h
Signature · 1 line
2.7.4
PATCH 1/2 dt-bindings: Document the hi3660 clock bindings
Add DT bindings documentation for hi3660 SoC clock.
--- .../devicetree/bindings/clock/hi3660-clock.txt | 42 ++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/hi3660-clock.txt
Documentation/devicetree/bindings/clock/hi3660-clock.txt new file +42 -0
@@ -0,0 +1,42 @@ +* 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" + +- 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/hi3660-clock.h>. + +Examples: + crg_ctrl: crg_ctrl at fff35000 { + compatible = "hisilicon,hi3660-crgctrl", "syscon"; + reg = <0x0 0xfff35000 0x0 0x1000>; + #clock-cells = <1>; + }; + + uart0: uart at 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"; + status = "disabled"; + };
Signature · 1 line
2.7.4
Rob Herring
maintainerPATCH 1/2 dt-bindings: Document the hi3660 clock bindings
Quoted 32 lines · Zhangfei Gao
Add DT bindings documentation for hi3660 SoC clock.
Signed-off-by Zhangfei Gao <[email protected]>--- .../devicetree/bindings/clock/hi3660-clock.txt | 42 ++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/hi3660-clock.txtDocumentation/devicetree/bindings/clock/hi3660-clock.txt new file +18
-0@@ -0,0 +1,42 @@ +* 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" + +- reg: physical base address of the controller and length of memory mapped + region.
Documentation/devicetree/bindings/clock/hi3660-clock.txt line 28
Quoted 10 lines
Documentation/devicetree/bindings/clock/hi3660-clock.txt new file +10
-0+ +- #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/hi3660-clock.h>. + +Examples: + crg_ctrl: crg_ctrl at fff35000 {
clock-controller at ...
Documentation/devicetree/bindings/clock/hi3660-clock.txt line 34
Quoted 6 lines
Documentation/devicetree/bindings/clock/hi3660-clock.txt new file +6
-0+ compatible = "hisilicon,hi3660-crgctrl", "syscon"; + reg = <0x0 0xfff35000 0x0 0x1000>; + #clock-cells = <1>; + }; + + uart0: uart at fdf02000 {
serial at ...
Quoted 11 lines
Documentation/devicetree/bindings/clock/hi3660-clock.txt new file +8
-0+ 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"; + status = "disabled"; + };Signature · 1 line
2.7.4
PATCH 2/2 clk: hi3660: Clock driver support for Hisilicon hi3660 SoC
In reply to Zhangfei Gao--- drivers/clk/hisilicon/Kconfig | 7 + drivers/clk/hisilicon/Makefile | 1 + drivers/clk/hisilicon/clk-hi3660.c | 601 +++++++++++++++++++++++++++++++ include/dt-bindings/clock/hi3660-clock.h | 194 ++++++++++ 4 files changed, 803 insertions(+) create mode 100644 drivers/clk/hisilicon/clk-hi3660.c create mode 100644 include/dt-bindings/clock/hi3660-clock.h
drivers/clk/hisilicon/Kconfig +7 -0
@@ -6,6 +6,13 @@ config COMMON_CLK_HI3519 help Build the clock driver for hi3519. +config COMMON_CLK_HI3660 + bool "Hi3660 Clock Driver" + depends on ARCH_HISI || COMPILE_TEST + default ARCH_HISI + help + Build the Hisilicon Hi3660 clock driver based on the common clock framework. + config COMMON_CLK_HI6220 bool "Hi6220 Clock Driver" depends on ARCH_HISI || COMPILE_TEST
drivers/clk/hisilicon/Makefile +1 -0
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_HI3xxx) += clk-hi3620.o obj-$(CONFIG_ARCH_HIP04) += clk-hip04.o obj-$(CONFIG_ARCH_HIX5HD2) += clk-hix5hd2.o obj-$(CONFIG_COMMON_CLK_HI3519) += clk-hi3519.o +obj-$(CONFIG_COMMON_CLK_HI3660) += clk-hi3660.o obj-$(CONFIG_COMMON_CLK_HI6220) += clk-hi6220.o obj-$(CONFIG_RESET_HISI) += reset.o obj-$(CONFIG_STUB_CLK_HI6220) += clk-hi6220-stub.o
drivers/clk/hisilicon/clk-hi3660.c new file +601 -0
601 lines not on this page · raw message
include/dt-bindings/clock/hi3660-clock.h new file +194 -0
194 lines not on this page · raw message
Signature · 1 line
2.7.4
PATCH 2/2 clk: hi3660: Clock driver support for Hisilicon hi3660 SoC
Quoted 1 line · Zhangfei Gao
Signed-off-by Zhangfei Gao <[email protected]>
Add some commit text here?
Quoted 9 lines
drivers/clk/hisilicon/clk-hi3660.c new file +3
-0@@ -0,0 +1,601 @@ +/* +* Copyright (c) 2016-2017 Linaro Ltd. +* Copyright (c) 2016-2017 HiSilicon Technologies Co., Ltd.
drivers/clk/hisilicon/clk-hi3660.c line 13
Quoted 10 lines
drivers/clk/hisilicon/clk-hi3660.c new file +10
-0+* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +*/ + +#include <dt-bindings/clock/hi3660-clock.h> +#include <linux/clk-provider.h> +#include <linux/module.h>
This isn't needed.
Quoted 4 lines
drivers/clk/hisilicon/clk-hi3660.c new file +4
-0+#include <linux/of_device.h> +#include <linux/platform_device.h> +#include "clk.h" +
[...]
drivers/clk/hisilicon/clk-hi3660.c line 561
Quoted 13 lines
drivers/clk/hisilicon/clk-hi3660.c new file +13
-0+ +static int hi3660_clk_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct device_node *np = pdev->dev.of_node; +const struct of_device_id *of_id; +enum hi3660_clk_type type; + + of_id =of_match_device (hi3660_clk_match_table, dev); +if (!of_id) +return -EINVAL ; + + type = (enum hi3660_clk_type )of_id->data;
Use of_device_get_match_data() instead please.
Quoted 7 lines
drivers/clk/hisilicon/clk-hi3660.c new file +7
-0+ +switch (type) { +case HI3660_CRGCTRL : +hi3660_clk_crgctrl_init (np); +break ; +case HI3660_PCTRL : +hi3660_clk_pctrl_init (np);
drivers/clk/hisilicon/clk-hi3660.c line 578
Quoted 10 lines
drivers/clk/hisilicon/clk-hi3660.c new file +10
-0+break ; +case HI3660_PMUCTRL : +hi3660_clk_pmuctrl_init (np); +break ; +case HI3660_SCTRL : +hi3660_clk_sctrl_init (np); +break ; +case HI3660_IOMCU : +hi3660_clk_iomcu_init (np); +break ;
This "multi-device" driver design is sort of odd. Why not have different files and struct drivers for the different devices in the system that are clock controllers? I don't really understand why we're controlling the devices with one struct driver instance. Is something shared between the devices?
Quoted 5 lines
drivers/clk/hisilicon/clk-hi3660.c new file +5
-0+default : +break ; + } +return 0 ; +}
[...]
drivers/clk/hisilicon/clk-hi3660.c line 601
Quoted 4 lines
drivers/clk/hisilicon/clk-hi3660.c new file +4
-0+ +MODULE_LICENSE ("GPL" ); +MODULE_ALIAS ("platform:hi3660-clk" ); +MODULE_DESCRIPTION ("HiSilicon Hi3660 Clock Driver" );
You can drop these MODULE_* things as they're not going to be used in builtin only code.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 2/2 clk: hi3660: Clock driver support for Hisilicon hi3660 SoC
Hi, Stephen
Thanks for the suggestion.
Quoted 3 lines · Stephen Boyd
Quoted 1 line · Zhangfei Gao
Signed-off-by Zhangfei Gao <[email protected]>Add some commit text here?
OK
Quoted 27 lines
Quoted 10 lines
drivers/clk/hisilicon/clk-hi3660.c new file +3
-0@@ -0,0 +1,601 @@ +/* +* Copyright (c) 2016-2017 Linaro Ltd. +* Copyright (c) 2016-2017 HiSilicon Technologies Co., Ltd. drivers/clk/hisilicon/clk-hi3660.c line 13
Quoted 10 lines
drivers/clk/hisilicon/clk-hi3660.c new file +10
-0+* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +*/ + +#include <dt-bindings/clock/hi3660-clock.h> +#include <linux/clk-provider.h> +#include <linux/module.h> This isn't needed.
Quoted 4 lines
drivers/clk/hisilicon/clk-hi3660.c new file +4
-0+#include <linux/of_device.h> +#include <linux/platform_device.h> +#include "clk.h" +[...]
Quoted 14 lines
drivers/clk/hisilicon/clk-hi3660.c line 561
Quoted 13 lines
drivers/clk/hisilicon/clk-hi3660.c new file +13
-0+ +static int hi3660_clk_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct device_node *np = pdev->dev.of_node; +const struct of_device_id *of_id; +enum hi3660_clk_type type; + + of_id =of_match_device (hi3660_clk_match_table, dev); +if (!of_id) +return -EINVAL ; + + type = (enum hi3660_clk_type )of_id->data;Use
of_device_get_match_data()instead please.
Yes, this is simpler.
Quoted 15 lines
Quoted 15 lines
drivers/clk/hisilicon/clk-hi3660.c new file +14
-0+ +switch (type) { +case HI3660_CRGCTRL : +hi3660_clk_crgctrl_init (np); +break ; +case HI3660_PCTRL : +hi3660_clk_pctrl_init (np); +break ; +case HI3660_PMUCTRL : +hi3660_clk_pmuctrl_init (np); +break ; +case HI3660_SCTRL : +hi3660_clk_sctrl_init (np); +break ;
Quoted 8 lines
drivers/clk/hisilicon/clk-hi3660.c line 578
Quoted 3 lines
drivers/clk/hisilicon/clk-hi3660.c new file +3
-0+case HI3660_IOMCU : +hi3660_clk_iomcu_init (np); +break ;This "multi-device" driver design is sort of odd. Why not have different files and struct drivers for the different devices in the system that are clock controllers? I don't really understand why we're controlling the devices with one struct driver instance. Is something shared between the devices?
Do you mean put in different .c / drivers?
They have to be put in the same file, since the parent / child relate to each other.
They are for the same chip, but some put in different region for privilege control.
Quoted 7 lines
Quoted 6 lines
drivers/clk/hisilicon/clk-hi3660.c new file +5
-0+default : +break ; + } +return 0 ; +}[...]
Quoted 7 lines
drivers/clk/hisilicon/clk-hi3660.c line 601
Quoted 4 lines
drivers/clk/hisilicon/clk-hi3660.c new file +4
-0+ +MODULE_LICENSE ("GPL" ); +MODULE_ALIAS ("platform:hi3660-clk" ); +MODULE_DESCRIPTION ("HiSilicon Hi3660 Clock Driver" );You can drop these MODULE_* things as they're not going to be used in builtin only code.
ok, got it.
Thanks
PATCH 2/2 clk: hi3660: Clock driver support for Hisilicon hi3660 SoC
Quoted 26 lines · zhangfei
Quoted 17 lines · Stephen Boyd
Quoted 16 lines · Zhangfei Gao
Signed-off-by Zhangfei Gao <[email protected]>drivers/clk/hisilicon/clk-hi3660.c new file +14
-0+ +switch (type) { +case HI3660_CRGCTRL : +hi3660_clk_crgctrl_init (np); +break ; +case HI3660_PCTRL : +hi3660_clk_pctrl_init (np); +break ; +case HI3660_PMUCTRL : +hi3660_clk_pmuctrl_init (np); +break ; +case HI3660_SCTRL : +hi3660_clk_sctrl_init (np); +break ;Quoted 8 lines
drivers/clk/hisilicon/clk-hi3660.c line 578
Quoted 3 lines
drivers/clk/hisilicon/clk-hi3660.c new file +3
-0+case HI3660_IOMCU : +hi3660_clk_iomcu_init (np); +break ;This "multi-device" driver design is sort of odd. Why not have different files and struct drivers for the different devices in the system that are clock controllers? I don't really understand why we're controlling the devices with one struct driver instance. Is something shared between the devices?
Quoted 1 line
Do you mean put in different .c / drivers?
Yes.
Quoted 2 lines
They have to be put in the same file, since the parent / child relate to each other.
We handle clk parent/child relationships through strings. So why does that mean we need to put these in the same file with the same struct driver?
Quoted 2 lines
They are for the same chip, but some put in different region for privilege control.
Ok.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 2/2 clk: hi3660: Clock driver support for Hisilicon hi3660 SoC
Hi, Stephen
Quoted 26 lines · Stephen Boyd
Quoted 25 lines · zhangfei
Quoted 24 lines · Stephen Boyd
Quoted 8 lines · Zhangfei Gao
Signed-off-by Zhangfei Gao <[email protected]>drivers/clk/hisilicon/clk-hi3660.c new file +7
-0+ +switch (type) { +case HI3660_CRGCTRL : +hi3660_clk_crgctrl_init (np); +break ; +case HI3660_PCTRL : +hi3660_clk_pctrl_init (np);drivers/clk/hisilicon/clk-hi3660.c line 578
Quoted 10 lines
drivers/clk/hisilicon/clk-hi3660.c new file +10
-0+break ; +case HI3660_PMUCTRL : +hi3660_clk_pmuctrl_init (np); +break ; +case HI3660_SCTRL : +hi3660_clk_sctrl_init (np); +break ; +case HI3660_IOMCU : +hi3660_clk_iomcu_init (np); +break ;This "multi-device" driver design is sort of odd. Why not have different files and struct drivers for the different devices in the system that are clock controllers? I don't really understand why we're controlling the devices with one struct driver instance. Is something shared between the devices?
Quoted 8 lines
Quoted 1 line
Do you mean put in different .c / drivers?
Yes.
Quoted 2 lines
They have to be put in the same file, since the parent / child relate to each other.
We handle clk parent/child relationships through strings. So why does that mean we need to put these in the same file with the same struct driver?
Yes, you are right.
But we still prefer to put in the same file for as a hi3660-clock driver, since it is easy to maintain.
There may too many files if each chip split to small pieces, there may many chips for hisilicon in the futures.
And some clocks has less entries, like hi3660_pmu_gate_clks only have one entry.
Also put in one file is easy to find parent/child relation, so easier to find bugs.
After all, they just put in different region for privilege control, no other special reason.
What do you think.
Thanks
Quoted 5 lines
Quoted 3 lines
They are for the same chip, but some put in different region for privilege control.
Ok.