Enable Hisilicon Hi3620 SoC
Subsystems (5)
v7 cover Enable Hisilicon Hi3620 SoC
Changelog:
v7:
1. Add hi3xxx_defconfig.
2. Use reg property in clock node.
3. Drop origin clock divider table.
4. Reuse clock divider register helper.
5. Reuse clock gate register helper.
6. Append CLK_GATE_SEPERATED_REG flag in order to support Hisilicon Hi3620 SoC.
7. Rebase DEBUG_LL for Hi3xxx.
8. Add more clock node in DTS file.
v6:
1. Remove hisilicon string from properties in clock driver.
2. Replace array by pointer in clock driver. Since only sctrl parent node exists at this time.
v5:
1. Remove HIWORD clk patches since they're merged into clk git tree.
2. Set hisilicon,clk-reset property of clkgate node is optional.
3. Update on commandline args in DTS file. Remove earlyprintk, mem, nfs.
4. Move gpio-keys out of amba node in DTS file.
v4:
1. Add clk gate with HIWORD mask for Rockchip.
2. Update comments and code of HIWORD flags for mux/divider.
3. Append a mux without HIWORD mask in Hisilicon 3620.
4. Fix the pinmux setting in Hi4511.
v3:
1. Use clk_register_mux_table().
v2:
1. Reuse mux & divider driver. So append CLK_MUX_HIWORD_MASK & CLK_DIVIDER_HIWORD_MASK for Hi3620 SoC.
2. Fix system timer running too fast because wrong divider is choosen.
3. Remove .init_irq in DT machine descriptor.
PATCH v7 1/11 ARM: debug: support debug ll on hisilicon soc
Support UART0 debug ll on Hisilicon Hi3620 SoC & Hi3716 SoC.
--- arch/arm/Kconfig.debug | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
arch/arm/Kconfig.debug +19 -0
@@ -194,6 +194,22 @@ choice Say Y here if you want kernel low-level debugging support on HI3716 UART. + config DEBUG_HI3620_UART + bool "Hisilicon HI3620 Debug UART" + depends on ARCH_HI3xxx + select DEBUG_UART_PL01X + help + Say Y here if you want kernel low-level debugging support + on HI3620 UART. + + config DEBUG_HI3716_UART + bool "Hisilicon Hi3716 Debug UART" + depends on ARCH_HI3xxx + select DEBUG_UART_PL01X + help + Say Y here if you want kernel low-level debugging support + on HI3716 UART. + config DEBUG_HIGHBANK_UART bool "Kernel low-level debugging messages via Highbank UART" depends on ARCH_HIGHBANK
@@ -980,6 +996,8 @@ config DEBUG_UART_PHYS default 0xf1012000 if DEBUG_MVEBU_UART_ALTERNATE default 0xf1012000 if ARCH_DOVE || ARCH_KIRKWOOD || ARCH_MV78XX0 || \ ARCH_ORION5X + default 0xf8b00000 if DEBUG_HI3716_UART + default 0xfcb00000 if DEBUG_HI3620_UART default 0xfe800000 if ARCH_IOP32X default 0xffc02000 if DEBUG_SOCFPGA_UART default 0xffd82340 if ARCH_IOP13XX
@@ -1016,6 +1034,7 @@ config DEBUG_UART_VIRT default 0xfe100000 if DEBUG_IMX23_UART || DEBUG_IMX28_UART default 0xfe230000 if DEBUG_PICOXCELL_UART default 0xfe800000 if ARCH_IOP32X + default 0xfeb00000 if DEBUG_HI3620_UART || DEBUG_HI3716_UART default 0xfeb24000 if DEBUG_RK3X_UART0 default 0xfeb26000 if DEBUG_RK3X_UART1 default 0xfeb30c00 if DEBUG_KEYSTONE_UART0
Signature · 1 line
1.8.1.2
PATCH v7 2/11 clk: hi3xxx: add clock support
In reply to Haojian ZhuangAdd clock support with device tree on Hisilicon SoC.
--- .../devicetree/bindings/clock/hisilicon.txt | 118 +++++++++ drivers/clk/Makefile | 1 + drivers/clk/clk-hi3xxx.c | 272 +++++++++++++++++++++ 3 files changed, 391 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/hisilicon.txt create mode 100644 drivers/clk/clk-hi3xxx.c
Documentation/devicetree/bindings/clock/hisilicon.txt new file +118 -0
@@ -0,0 +1,118 @@ +Device Tree Clock bindings for arch-hi3xxx + +This binding uses the common clock binding[1]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +All the mux, gate & divider are in clock container of DTS file. + +Required properties for mux clocks: + - compatible : shall be "hisilicon,clk-mux". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock. + - clock-output-names : shall be reference name. + - #clock-cells : from common clock binding; shall be set to 0. + - reg : the mux register address. It should be the offset of the container. + - clkmux-mask : mask bits of the mux register. + - clkmux-table : array of mux select bits. + +Optional properties for mux clocks: + - clkmux-hiword-mask : indicates that the bit[31:16] are the hiword mask + of mux selected bits (bit[15:0]). The bit[15:0] is valid only when + bit[31:16] is set. + + + +Required properties for gate clocks: + - compatible : shall be "hisilicon,clk-gate". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock. + - clock-output-names : shall be reference name. + - #clock-cells : from common clock binding; shall be set to 0. + - reg : the mux register address. It should be the offset of the container. + - clkgate : bit index to control the clock gate in the gate register. + +Optional properties for gate clocks: + - clkgate-inverted : it indicates that setting 0 could enable the clock gate + and setting 1 could disable the clock gate. + - clkgate-seperated-reg : it indicates that there're three continuous + registers (enable, disable & status) for the same clock gate. + + + +Required properties for divider clocks: + - compatible : shall be "hisilicon,clk-div". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock. + - clock-output-names : shall be reference name. + - reg : the divider register address. It should be the offset of the + container. + - clkdiv-mask : mask bits of the divider register. + - clkdiv-min : the minimum divider of the clock divider. + - clkdiv-max : the maximum divider of the clock divider. + +Optional properties for divider clocks: + - clkdiv-hiword-mask : indicates that the bit[31:16] are the hiword mask + of divider selected bits (bit[15:0]). The bit[15:0] is valid only when + bit[31:16] is set. + + + +For example: + sctrl: sctrl at fc802000 { + compatible = "hisilicon,sctrl"; + reg = <0xfc802000 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + + + timer0_mux: timer0_mux { + compatible = "hisilicon,clk-mux"; + #clock-cells = <0>; + clocks = <&osc32k &timerclk01>; + clock-output-names = "timer0_mux"; + reg = <0>; + clkmux-mask = <0x8000>; + clkmux-table = <0 0x8000>; + }; + uart0_mux: uart0_mux { + compatible = "hisilicon,clk-mux"; + #clock-cells = <0>; + clocks = <&osc26m &pclk>; + clock-output-names = "uart0_mux"; + reg = <0x100>; + clkmux-mask = <0x80>; + /* each item value */ + clkmux-table = <0 0x80>; + clkmux-hiword-mask; + }; + timclk0: timclk0 { + compatible = "hisilicon,clk-gate"; + #clock-cells = <0>; + clocks = <&timer0_mux>; + clock-output-names = "timclk0"; + clkgate-inverted; + reg = <0>; + clkgate = <16>; + }; + timerclk01: timerclk01 { + compatible = "hisilicon,clk-gate"; + #clock-cells = <0>; + clocks = <&timer_rclk01>; + clock-output-names = "timerclk01"; + reg = <0x20>; + clkgate = <0>; + clkgate-seperated-reg; + }; + mmc1_div: mmc1_div { + compatible = "hisilicon,clk-div"; + #clock-cells = <0>; + clocks = <&mmc1_mux>; + clock-output-names = "mmc1_div"; + reg = <0x108>; + clkdiv-mask = <0x1e0>; + clkdiv-min = <1>; + clkdiv-max = <16>; + clkdiv-hiword-mask; + }; + };
drivers/clk/Makefile +1 -0
@@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK) += clk-composite.o # SoCs specific obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o obj-$(CONFIG_ARCH_NOMADIK) += clk-nomadik.o +obj-$(CONFIG_ARCH_HI3xxx) += clk-hi3xxx.o obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o obj-$(CONFIG_ARCH_NSPIRE) += clk-nspire.o obj-$(CONFIG_ARCH_MXS) += mxs/
drivers/clk/clk-hi3xxx.c new file +272 -0
272 lines not on this page · raw message
Signature · 1 line
1.8.1.2
PATCH v7 2/11 clk: hi3xxx: add clock support
Quoting Haojian Zhuang (2013-08-19 19:31:04)
Quoted 30 lines
Add clock support with device tree on Hisilicon SoC.
Signed-off-by Haojian Zhuang <[email protected]>Cc Mike Turquette <[email protected]>--- .../devicetree/bindings/clock/hisilicon.txt | 118 +++++++++ drivers/clk/Makefile | 1 + drivers/clk/clk-hi3xxx.c | 272 +++++++++++++++++++++ 3 files changed, 391 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/hisilicon.txt create mode 100644 drivers/clk/clk-hi3xxx.cDocumentation/devicetree/bindings/clock/hisilicon.txt new file +12
-0@@ -0,0 +1,118 @@ +Device Tree Clock bindings for arch-hi3xxx + +This binding uses the common clock binding[1]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +All the mux, gate & divider are in clock container of DTS file. + +Required properties for mux clocks: + - compatible : shall be "hisilicon,clk-mux". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock.
Documentation/devicetree/bindings/clock/hisilicon.txt line 22
Quoted 10 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +10
-0+ - clock-output-names : shall be reference name. + - #clock-cells : from common clock binding; shall be set to 0. + - reg : the mux register address. It should be the offset of the container. + - clkmux-mask : mask bits of the mux register. + - clkmux-table : array of mux select bits. + +Optional properties for mux clocks: + - clkmux-hiword-mask : indicates that the bit[31:16] are the hiword mask + of mux selected bits (bit[15:0]). The bit[15:0] is valid only when + bit[31:16] is set.
Instead of putting this as a flag I'm tempted to say this should be a separate compatible string. Any thoughts from the DT experts?
Quoted 7 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +7
-0+ + + +Required properties for gate clocks: + - compatible : shall be "hisilicon,clk-gate". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock.
Documentation/devicetree/bindings/clock/hisilicon.txt line 39
Quoted 10 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +10
-0+ - clock-output-names : shall be reference name. + - #clock-cells : from common clock binding; shall be set to 0. + - reg : the mux register address. It should be the offset of the container. + - clkgate : bit index to control the clock gate in the gate register. + +Optional properties for gate clocks: + - clkgate-inverted : it indicates that setting 0 could enable the clock gate + and setting 1 could disable the clock gate. + - clkgate-seperated-reg : it indicates that there're three continuous + registers (enable, disable & status) for the same clock gate.
I responded to this in the other thread but clkgate-separated-reg is a bad idea. You'll need your own gate clock type which handles this instead of pushing it into the common gate implementation. Use the existing 'reg' property to list the registers needed by this clock.
Documentation/devicetree/bindings/clock/hisilicon.txt line 52
Quoted 13 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +13
-0+ + + +Required properties for divider clocks: + - compatible : shall be "hisilicon,clk-div". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock. + - clock-output-names : shall be reference name. + - reg : the divider register address. It should be the offset of the + container. + - clkdiv-mask : mask bits of the divider register. + - clkdiv-min : the minimum divider of the clock divider. + - clkdiv-max : the maximum divider of the clock divider.
Are these details necessary? Do the mask, min & max values change from clock to clock? It's nicer to have these in the driver if possible. This looks like building a binding from a struct, which is considered bad.
Documentation/devicetree/bindings/clock/hisilicon.txt line 57
Quoted 5 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +5
-0+ +Optional properties for divider clocks: + - clkdiv-hiword-mask : indicates that the bit[31:16] are the hiword mask + of divider selected bits (bit[15:0]). The bit[15:0] is valid only when + bit[31:16] is set.
Same here. Again you might need a different compatible string for these types of clocks.
Regards,
Mike
Quoted 353 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +61
-0+ + + +For example: + sctrl: sctrl at fc802000 { + compatible = "hisilicon,sctrl"; + reg = <0xfc802000 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + + + timer0_mux: timer0_mux { + compatible = "hisilicon,clk-mux"; + #clock-cells = <0>; + clocks = <&osc32k &timerclk01>; + clock-output-names = "timer0_mux"; + reg = <0>; + clkmux-mask = <0x8000>; + clkmux-table = <0 0x8000>; + }; + uart0_mux: uart0_mux { + compatible = "hisilicon,clk-mux"; + #clock-cells = <0>; + clocks = <&osc26m &pclk>; + clock-output-names = "uart0_mux"; + reg = <0x100>; + clkmux-mask = <0x80>; + /* each item value */ + clkmux-table = <0 0x80>; + clkmux-hiword-mask; + }; + timclk0: timclk0 { + compatible = "hisilicon,clk-gate"; + #clock-cells = <0>; + clocks = <&timer0_mux>; + clock-output-names = "timclk0"; + clkgate-inverted; + reg = <0>; + clkgate = <16>; + }; + timerclk01: timerclk01 { + compatible = "hisilicon,clk-gate"; + #clock-cells = <0>; + clocks = <&timer_rclk01>; + clock-output-names = "timerclk01"; + reg = <0x20>; + clkgate = <0>; + clkgate-seperated-reg; + }; + mmc1_div: mmc1_div { + compatible = "hisilicon,clk-div"; + #clock-cells = <0>; + clocks = <&mmc1_mux>; + clock-output-names = "mmc1_div"; + reg = <0x108>; + clkdiv-mask = <0x1e0>; + clkdiv-min = <1>; + clkdiv-max = <16>; + clkdiv-hiword-mask; + }; + };drivers/clk/Makefile +1
-0@@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK) += clk-composite.o # SoCs specific obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o obj-$(CONFIG_ARCH_NOMADIK) += clk-nomadik.o +obj-$(CONFIG_ARCH_HI3xxx) += clk-hi3xxx.o obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o obj-$(CONFIG_ARCH_NSPIRE) += clk-nspire.o obj-$(CONFIG_ARCH_MXS) += mxs/drivers/clk/clk-hi3xxx.c new file +272
-0@@ -0,0 +1,272 @@ +/* +* Hisilicon Hi3xxx clock driver +* +* Copyright (c) 2012-2013 Hisilicon Limited. +* Copyright (c) 2012-2013 Linaro Limited. +* +* Author: Haojian Zhuang <[email protected]> +* Xin Li <[email protected]> +* +* 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. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program; if not, write to the Free Software Foundation, Inc., +* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +* +*/ + +#include <linux/kernel.h> +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/slab.h> +#include <linux/clk.h> + +struct hi3620_periclk { +struct clk_hw hw; +void __iomem *enable;/* enable register */ +u32 ebits;/* bits in enable/disable register */ +spinlock_t *lock; +}; + +static void __iomem *hi3xxx_clk_base =NULL ; + +static DEFINE_SPINLOCK (hi3xxx_clk_lock); + +static const struct of_device_id hi3xxx_of_match[] = { + { .compatible ="hisilicon,sctrl" }, +}; + +static void __iomem __init *hi3xxx_init_clocks (struct device_node *np) +{ +struct device_node *parent; +const struct of_device_id *match; +void __iomem *ret =NULL ; + + parent =of_get_parent (np); +if (!parent) { +pr_warn ("Can't find parent node of these clocks\n" ); + goto out; + } + match =of_match_node (hi3xxx_of_match, parent); +if (!match) { +pr_warn ("Can't find the right parent\n" ); + goto out; + } + +if (!hi3xxx_clk_base) { + ret =of_iomap (parent,0 ); +WARN_ON (!ret); + hi3xxx_clk_base = ret; + }else { + ret = hi3xxx_clk_base; + } +out: +return ret; +} + +static void __init hi3xxx_clkgate_setup (struct device_node *np) +{ +struct clk *clk; +const char *clk_name, **parent_names, *name; +const __be32 *prop; +unsigned long flags =0 ; +u32 bit_idx; +void __iomem *base, *reg; + + base =hi3xxx_init_clocks (np); +if (!base) + goto err; +if (of_property_read_string (np,"clock-output-names" , &clk_name)) + goto err; +if (of_property_read_u32 (np,"clkgate" , &bit_idx)) + goto err; + prop =of_get_address (np,0 ,NULL ,NULL ); +if (!prop) + goto err; + reg = base +be32_to_cpu (*prop); +if (of_property_read_bool (np,"clkgate-inverted" )) + flags |=CLK_GATE_SET_TO_DISABLE ; +if (of_property_read_bool (np,"clkgate-seperated-reg" )) + flags |=CLK_GATE_SEPERATED_REG ; +/* gate only has the fixed parent */ + parent_names =kzalloc (sizeof (char *),GFP_KERNEL ); +if (!parent_names) + goto err; + parent_names[0 ] =of_clk_get_parent_name (np,0 ); + + clk =clk_register_gate (NULL , clk_name, parent_names[0 ],0 , reg, + (u8 )bit_idx, flags, &hi3xxx_clk_lock); +if (IS_ERR (clk)) + goto err_clk; +if (!of_property_read_string (np,"clock-names" , &name)) +clk_register_clkdev (clk, name,NULL ); +of_clk_add_provider (np, of_clk_src_simple_get, clk); +return ; +err_clk: +kfree (parent_names); +err: +pr_err ("Fail on registering hi3xxx clkgate node.\n" ); +} +CLK_OF_DECLARE(hi3xxx_gate, "hisilicon,clk-gate", hi3xxx_clkgate_setup) + +static int __init hi3xxx_parse_mux (struct device_node *np, +u8 *num_parents, +u32 *table) +{ +int i, cnt, ret; + +/* get the count of items in mux */ +for (i =0 , cnt =0 ; ; i++, cnt++) { +/* parent's #clock-cells property is always 0 */ +if (!of_parse_phandle (np,"clocks" , i)) +break ; + } + +for (i =0 ; i < cnt; i++) { +if (!of_clk_get_parent_name (np, i)) +return -ENOENT ; + } + *num_parents = cnt; + table =kzalloc (sizeof (u32 *) * cnt,GFP_KERNEL ); +if (!table) +return -ENOMEM ; + ret =of_property_read_u32_array (np,"clkmux-table" , + table, cnt); +if (ret) + goto err; +return 0 ; +err: +kfree (table); +return ret; +} + +static void __init hi3xxx_clkmux_setup (struct device_node *np) +{ +struct clk *clk; +const char *clk_name, **parent_names =NULL ; +const __be32 *prop; +u32 mask, *table =NULL ; +u8 num_parents, shift, clk_mux_flags =0 ; +void __iomem *reg, *base; +int i, ret; + + base =hi3xxx_init_clocks (np); +if (!base) + goto err; +if (of_property_read_string (np,"clock-output-names" , &clk_name)) + goto err; + prop =of_get_address (np,0 ,NULL ,NULL ); +if (!prop) + goto err; + reg = base +be32_to_cpu (*prop); +if (of_property_read_u32 (np,"clkmux-mask" , &mask)) + goto err; +if (of_property_read_bool (np,"clkmux-hiword-mask" )) + clk_mux_flags =CLK_MUX_HIWORD_MASK ; + + ret =hi3xxx_parse_mux (np, &num_parents, table); +if (ret) + goto err; + + parent_names =kzalloc (sizeof (char *) * num_parents,GFP_KERNEL ); +if (!parent_names) + goto err_table; +for (i =0 ; i < num_parents; i++) + parent_names[i] =of_clk_get_parent_name (np, i); + + shift =ffs (mask) -1 ; + mask = mask >> shift; + clk =clk_register_mux_table (NULL , clk_name, parent_names, num_parents, +CLK_SET_RATE_PARENT , reg, shift, mask, + clk_mux_flags, table, &hi3xxx_clk_lock); +if (IS_ERR (clk)) + goto err_clk; +of_clk_add_provider (np, of_clk_src_simple_get, clk); + +return ; +err_clk: +kfree (parent_names); +err_table: +kfree (table); +err: +pr_err ("Fail on registering hi3xxx clkmux node.\n" ); +} +CLK_OF_DECLARE(hi3xxx_mux, "hisilicon,clk-mux", hi3xxx_clkmux_setup) + +static void __init hi3xxx_clkdiv_setup (struct device_node *np,int mode) +{ +struct clk *clk; +const char *clk_name, **parent_names; +const __be32 *prop; +struct clk_div_table *table; +unsigned int table_num; +u32 min, max, mask; +u8 shift, width, clk_div_flags =0 ; +void __iomem *reg, *base; +int i; + + base =hi3xxx_init_clocks (np); +if (!base) + goto err; + +if (of_property_read_string (np,"clock-output-names" , &clk_name)) + goto err; +if (of_property_read_u32 (np,"clkdiv-mask" , &mask)) + goto err; +if (of_property_read_u32 (np,"clkdiv-min" , &min)) + goto err; +if (of_property_read_u32 (np,"clkdiv-max" , &max)) + goto err; +if (of_property_read_bool (np,"clkdiv-hiword-mask" )) + clk_div_flags =CLK_DIVIDER_HIWORD_MASK ; + + prop =of_get_address (np,0 ,NULL ,NULL ); +if (!prop) + goto err; + reg = base +be32_to_cpu (*prop); + + table_num = max - min +1 ; + table =kzalloc (sizeof (*table) * table_num,GFP_KERNEL ); +if (!table) + goto err; + +for (i =0 ; i < table_num; i++) { + table[i].div = min + i; + table[i].val = table[i].div -1 ; + } + +/* gate only has the fixed parent */ + parent_names =kzalloc (sizeof (char *),GFP_KERNEL ); +if (!parent_names) + goto err_par; + parent_names[0 ] =of_clk_get_parent_name (np,0 ); + shift =ffs (mask) -1 ; + width =fls (mask) -ffs (mask) +1 ; + clk =clk_register_divider_table (NULL , clk_name, parent_names[0 ],0 , + reg, shift, width, clk_div_flags, + table, &hi3xxx_clk_lock); +if (IS_ERR (clk)) + goto err_clk; +of_clk_add_provider (np, of_clk_src_simple_get, clk); +return ; +err_clk: +kfree (parent_names); +err_par: +kfree (table); +err: +pr_err ("Fail on registering hi3xxx clkdiv node\n" ); +} +CLK_OF_DECLARE (hi3xxx_div,"hisilicon,clk-div" , hi3xxx_clkdiv_setup)Signature · 1 line
1.8.1.2
PATCH v7 2/11 clk: hi3xxx: add clock support
Quoted 36 lines · Mike Turquette
Quoting Haojian Zhuang (2013-08-19 19:31:04)
Quoted 35 lines
Add clock support with device tree on Hisilicon SoC.
Signed-off-by Haojian Zhuang <[email protected]>Cc Mike Turquette <[email protected]>--- .../devicetree/bindings/clock/hisilicon.txt | 118 +++++++++ drivers/clk/Makefile | 1 + drivers/clk/clk-hi3xxx.c | 272 +++++++++++++++++++++ 3 files changed, 391 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/hisilicon.txt create mode 100644 drivers/clk/clk-hi3xxx.cDocumentation/devicetree/bindings/clock/hisilicon.txt new file +17
-0@@ -0,0 +1,118 @@ +Device Tree Clock bindings for arch-hi3xxx + +This binding uses the common clock binding[1]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +All the mux, gate & divider are in clock container of DTS file. + +Required properties for mux clocks: + - compatible : shall be "hisilicon,clk-mux". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock. + - clock-output-names : shall be reference name. + - #clock-cells : from common clock binding; shall be set to 0. + - reg : the mux register address. It should be the offset of the container. + - clkmux-mask : mask bits of the mux register. + - clkmux-table : array of mux select bits.
Quoted 9 lines
Documentation/devicetree/bindings/clock/hisilicon.txt line 22
Quoted 5 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +5
-0+ +Optional properties for mux clocks: + - clkmux-hiword-mask : indicates that the bit[31:16] are the hiword mask + of mux selected bits (bit[15:0]). The bit[15:0] is valid only when + bit[31:16] is set.Instead of putting this as a flag I'm tempted to say this should be a separate compatible string. Any thoughts from the DT experts?
Since they're different divider types or mux types (with or without HIWORD) in the same dts, it'll only make people hard to check the node with compatible name.
So I hope to use the same compatible name with different property. It could be easy on checking them with register table.
In this driver, I'll only use one compatible string for one clock type on one SoC.
Since it's easier to developers on tracking with register tables. Otherwise, they
frustrated that there're too much compatible string on the same clock type. It only
increased unnecessary time on understanding the driver.
Quoted 14 lines
Quoted 14 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +14
-0+ + + +Required properties for gate clocks: + - compatible : shall be "hisilicon,clk-gate". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock. + - clock-output-names : shall be reference name. + - #clock-cells : from common clock binding; shall be set to 0. + - reg : the mux register address. It should be the offset of the container. + - clkgate : bit index to control the clock gate in the gate register. + +Optional properties for gate clocks: + - clkgate-inverted : it indicates that setting 0 could enable the clock gate
Quoted 9 lines
Documentation/devicetree/bindings/clock/hisilicon.txt line 39
Quoted 3 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +3
-0+ and setting 1 could disable the clock gate. + - clkgate-seperated-reg : it indicates that there're three continuous + registers (enable, disable & status) for the same clock gate.I responded to this in the other thread but clkgate-separated-reg is a bad idea. You'll need your own gate clock type which handles this instead of pushing it into the common gate implementation. Use the existing 'reg' property to list the registers needed by this clock.
OK. I'll use my own gate clock.
Quoted 9 lines
Quoted 9 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +9
-0+ + + +Required properties for divider clocks: + - compatible : shall be "hisilicon,clk-div". + - clocks : shall be the input parent clock phandle for the clock. This should + be the reference clock. + - clock-output-names : shall be reference name. + - reg : the divider register address. It should be the offset of the
Quoted 9 lines
Documentation/devicetree/bindings/clock/hisilicon.txt line 52
Quoted 4 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +4
-0+ container. + - clkdiv-mask : mask bits of the divider register. + - clkdiv-min : the minimum divider of the clock divider. + - clkdiv-max : the maximum divider of the clock divider.Are these details necessary? Do the mask, min & max values change from clock to clock? It's nicer to have these in the driver if possible. This looks like building a binding from a struct, which is considered bad.
Since there're too much divider values for one divider, likes 32 or 64. Then I abandon to use clkdiv-table property. Because those divider values are continuous
ascending, I use min & max instead. It could make the node simpler.
Quoted 8 lines
Documentation/devicetree/bindings/clock/hisilicon.txt line 57
Quoted 5 lines
Documentation/devicetree/bindings/clock/hisilicon.txt new file +5
-0+ +Optional properties for divider clocks: + - clkdiv-hiword-mask : indicates that the bit[31:16] are the hiword mask + of divider selected bits (bit[15:0]). The bit[15:0] is valid only when + bit[31:16] is set.Same here. Again you might need a different compatible string for these types of clocks.
Since they're different divider types or mux types (with or without HIWORD) in the same dts, it'll only make people hard to check the node with compatible name.
So I hope to use the same compatible name with different property. It could be easy on checking them with register table.
Regards
Haojian
Quoted 4 lines
Regards,
Mike
PATCH v7 3/11 clk: gate: add CLK_GATE_SEPERATED_REG flag
In reply to Haojian ZhuangIn Hisilicon Hi3620 SoC, there're two kinds of clock gates.
1. The same bit in one register controls enabling, disabling or reading status of the clock gate. It's the normal clock gate register.
2. The same bit in three continuous registers control enabling, disabling or reading status of the clock gate. Since these three registers are enabling, disabling and reading status of clock gate.
Now reuse common clock gate driver to support the second case with the new CLK_GATE_SEPERATED_REG flag.
--- drivers/clk/clk-gate.c | 18 +++++++++++++++--- include/linux/clk-provider.h | 7 +++++++ 2 files changed, 22 insertions(+), 3 deletions(-)
drivers/clk/clk-gate.c +15 -3
@@ -58,7 +58,10 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)if (set) reg |=BIT (gate->bit_idx); }else { - reg =readl (gate->reg); +if (gate->flags &CLK_GATE_SEPERATED_REG ) + reg =0 ; +else + reg =readl (gate->reg);if (set) reg |=BIT (gate->bit_idx);
@@ -66,7 +69,13 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable) reg &= ~BIT (gate->bit_idx); } -writel (reg, gate->reg); +if (gate->flags &CLK_GATE_SEPERATED_REG ) { +if (enable) +writel (reg, gate->reg +CLK_GATE_ENABLE_REG ); +else +writel (reg, gate->reg +CLK_GATE_DISABLE_REG ); + }else +writel (reg, gate->reg);if (gate->lock)spin_unlock_irqrestore (gate->lock, flags);
@@ -89,7 +98,10 @@ static int clk_gate_is_enabled(struct clk_hw *hw)u32 reg;struct clk_gate *gate =to_clk_gate (hw); - reg =readl (gate->reg); +if (gate->flags &CLK_GATE_SEPERATED_REG ) + reg =readl (gate->reg +CLK_GATE_STATUS_REG ); +else + reg =readl (gate->reg);/* if a set bit disables this clk, flip it before masking */ if (gate->flags &CLK_GATE_SET_TO_DISABLE )
include/linux/clk-provider.h +7 -0
@@ -214,6 +214,8 @@ void of_fixed_clk_setup(struct device_node *np);* of this register, and mask of gate bits are in higher 16-bit of this * register. While setting the gate bits, higher 16-bit should also be * updated to indicate changing gate bits. +* CLK_GATE_SEPERATED_REG - The enable, disable & status register are three +* seperated registers. */ struct clk_gate {struct clk_hw hw;
@@ -225,6 +227,11 @@ struct clk_gate {#define CLK_GATE_SET_TO_DISABLE BIT(0)#define CLK_GATE_HIWORD_MASK BIT(1) +#define CLK_GATE_SEPERATED_REG BIT(2) + +#define CLK_GATE_ENABLE_REG (0x00) +#define CLK_GATE_DISABLE_REG (0x04) +#define CLK_GATE_STATUS_REG (0x08)extern const struct clk_ops clk_gate_ops; struct clk *clk_register_gate(struct device *dev, const char *name,
Signature · 1 line
1.8.1.2
PATCH v7 3/11 clk: gate: add CLK_GATE_SEPERATED_REG flag
Quoting Haojian Zhuang (2013-08-19 19:31:05)
Quoted 5 lines
In Hisilicon Hi3620 SoC, there're two kinds of clock gates.
1. The same bit in one register controls enabling, disabling or reading status of the clock gate. It's the normal clock gate register.
Quoted 8 lines
2. The same bit in three continuous registers control enabling, disabling or reading status of the clock gate. Since these three registers are enabling, disabling and reading status of clock gate.
Now reuse common clock gate driver to support the second case with the new
CLK_GATE_SEPERATED_REGflag.Signed-off-by Haojian Zhuang <[email protected]>
Hi Haojian,
This is too implementation-specific to be pushed into the common gate clock. Instead you should write a custom gate implementation that uses these three registers without any flag. Your DT binding for this new clock type should list those three register addresses via the 'reg' property.
Regards,
Mike
Quoted 75 lines
--- drivers/clk/clk-gate.c | 18 +++++++++++++++--- include/linux/clk-provider.h | 7 +++++++ 2 files changed, 22 insertions(+), 3 deletions(-)drivers/clk/clk-gate.c +15
-3@@ -58,7 +58,10 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)if (set) reg |=BIT (gate->bit_idx); }else { - reg =readl (gate->reg); +if (gate->flags &CLK_GATE_SEPERATED_REG ) + reg =0 ; +else + reg =readl (gate->reg);if (set) reg |=BIT (gate->bit_idx);@@ -66,7 +69,13 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable) reg &= ~BIT (gate->bit_idx); } -writel (reg, gate->reg); +if (gate->flags &CLK_GATE_SEPERATED_REG ) { +if (enable) +writel (reg, gate->reg +CLK_GATE_ENABLE_REG ); +else +writel (reg, gate->reg +CLK_GATE_DISABLE_REG ); + }else +writel (reg, gate->reg);if (gate->lock)spin_unlock_irqrestore (gate->lock, flags);@@ -89,7 +98,10 @@ static int clk_gate_is_enabled(struct clk_hw *hw)u32 reg;struct clk_gate *gate =to_clk_gate (hw); - reg =readl (gate->reg); +if (gate->flags &CLK_GATE_SEPERATED_REG ) + reg =readl (gate->reg +CLK_GATE_STATUS_REG ); +else + reg =readl (gate->reg);/* if a set bit disables this clk, flip it before masking */ if (gate->flags &CLK_GATE_SET_TO_DISABLE )include/linux/clk-provider.h +7
-0@@ -214,6 +214,8 @@ void of_fixed_clk_setup(struct device_node *np);* of this register, and mask of gate bits are in higher 16-bit of this * register. While setting the gate bits, higher 16-bit should also be * updated to indicate changing gate bits. +* CLK_GATE_SEPERATED_REG - The enable, disable & status register are three +* seperated registers. */ struct clk_gate {struct clk_hw hw;@@ -225,6 +227,11 @@ struct clk_gate {#define CLK_GATE_SET_TO_DISABLE BIT(0)#define CLK_GATE_HIWORD_MASK BIT(1) +#define CLK_GATE_SEPERATED_REG BIT(2) + +#define CLK_GATE_ENABLE_REG (0x00) +#define CLK_GATE_DISABLE_REG (0x04) +#define CLK_GATE_STATUS_REG (0x08)extern const struct clk_ops clk_gate_ops; struct clk *clk_register_gate(struct device *dev, const char *name,Signature · 1 line
1.8.1.2
PATCH v7 4/11 ARM: hi3xxx: add board support with device tree
In reply to Haojian ZhuangAdd board support with device tree for Hisilicon Hi36xx/Hi37xx platform.
Changelog:
v3:
1. Remove .map_io() in DT machine descriptor. Since debug_ll_io_init() is called by default.
2. Remove .init_machine() in DT machine descriptor. Since of_platform_populate() is called by default in DT mode.
v2:
1. Remove .init_irq() in DT machine descriptor. Since irqchip_init() is called by default in DT mode.
--- .../bindings/arm/hisilicon/hisilicon.txt | 10 ++++++ arch/arm/Kconfig | 2 ++ arch/arm/Makefile | 1 + arch/arm/mach-hi3xxx/Kconfig | 12 ++++++++ arch/arm/mach-hi3xxx/Makefile | 5 +++ arch/arm/mach-hi3xxx/hi3xxx.c | 36 ++++++++++++++++++++++ 6 files changed, 66 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt create mode 100644 arch/arm/mach-hi3xxx/Kconfig create mode 100644 arch/arm/mach-hi3xxx/Makefile create mode 100644 arch/arm/mach-hi3xxx/hi3xxx.c
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt new file +10 -0
@@ -0,0 +1,10 @@ +Hisilicon Platforms Device Tree Bindings +---------------------------------------------------- + +Hi3716 Development Board +Required root node properties: + - compatible = "hisilicon,hi3716-dkb"; + +Hi4511 Board +Required root node properties: + - compatible = "hisilicon,hi3620-hi4511";
arch/arm/Kconfig +2 -0
@@ -943,6 +943,8 @@ source "arch/arm/mach-footbridge/Kconfig" source "arch/arm/mach-gemini/Kconfig" +source "arch/arm/mach-hi3xxx/Kconfig" + source "arch/arm/mach-highbank/Kconfig" source "arch/arm/mach-integrator/Kconfig"
arch/arm/Makefile +1 -0
@@ -155,6 +155,7 @@ machine-$(CONFIG_ARCH_EBSA110) += ebsa110 machine-$(CONFIG_ARCH_EP93XX) += ep93xx machine-$(CONFIG_ARCH_EXYNOS) += exynos machine-$(CONFIG_ARCH_GEMINI) += gemini +machine-$(CONFIG_ARCH_HI3xxx) += hi3xxx machine-$(CONFIG_ARCH_HIGHBANK) += highbank machine-$(CONFIG_ARCH_INTEGRATOR) += integrator machine-$(CONFIG_ARCH_IOP13XX) += iop13xx
arch/arm/mach-hi3xxx/Kconfig new file +12 -0
@@ -0,0 +1,12 @@ +config ARCH_HI3xxx + bool "Hisilicon Hi36xx/Hi37xx family" if ARCH_MULTI_V7 + select ARM_AMBA + select ARM_GIC + select ARM_TIMER_SP804 + select CACHE_L2X0 + select CLKSRC_OF + select GENERIC_CLOCKEVENTS + select PINCTRL + select PINCTRL_SINGLE + help + Support for Hisilicon Hi36xx/Hi37xx processor family
arch/arm/mach-hi3xxx/Makefile new file +5 -0
@@ -0,0 +1,5 @@ +# +# Makefile for Hisilicon Hi36xx/Hi37xx processors line +# + +obj-y += hi3xxx.o
arch/arm/mach-hi3xxx/hi3xxx.c new file +36 -0
@@ -0,0 +1,36 @@ +/* +* (Hisilicon's Hi36xx/Hi37xx SoC based) flattened device tree enabled machine +* +* Copyright (c) 2012-2013 Hisilicon Ltd. +* Copyright (c) 2012-2013 Linaro Ltd. +* +* Author: Haojian Zhuang <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ + +#include <linux/clk-provider.h> +#include <linux/clocksource.h> +#include <linux/irqchip.h> +#include <linux/of_platform.h> + +#include <asm/mach/arch.h> +#include <asm/mach/map.h> + +static void __init hi3xxx_timer_init (void ) +{ +of_clk_init (NULL ); +clocksource_of_init (); +} + +static const char *hs_compat[]__initdata = { +"hisilicon,hi3620-hi4511" , +NULL , +}; + +DT_MACHINE_START (HI3xxx,"Hisilicon Hi36xx/Hi37xx (Flattened Device Tree)" ) + .init_time = hi3xxx_timer_init, + .dt_compat = hs_compat, +MACHINE_END
Signature · 1 line
1.8.1.2
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
In reply to Haojian ZhuangEnable Hisilicon Hi4511 development platform with device tree support.
--- arch/arm/boot/dts/Makefile | 1 + arch/arm/boot/dts/hi3620.dtsi | 1645 +++++++++++++++++++++++++++++++++++++++++ arch/arm/boot/dts/hi4511.dts | 648 ++++++++++++++++ 3 files changed, 2294 insertions(+) create mode 100644 arch/arm/boot/dts/hi3620.dtsi create mode 100644 arch/arm/boot/dts/hi4511.dts
arch/arm/boot/dts/Makefile +1 -0
@@ -64,6 +64,7 @@ dtb-$(CONFIG_ARCH_EXYNOS) += exynos4210-origen.dtb \ exynos5420-smdk5420.dtb \ exynos5440-sd5v1.dtb \ exynos5440-ssdk5440.dtb +dtb-$(CONFIG_ARCH_HI3xxx) += hi4511.dtb dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb \ ecx-2000.dtb dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
arch/arm/boot/dts/hi3620.dtsi new file +1645 -0
1,645 lines not on this page · raw message
arch/arm/boot/dts/hi4511.dts new file +648 -0
648 lines not on this page · raw message
Signature · 1 line
1.8.1.2
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>
There seems to be use of several new vendor-specific bindings here that are not documented anywhere:
hisilicon,sctrl
hisilicon,pmctrl
hisilicon,clk-mux
hisilicon,clk-div
hisilicon,clk-gate
Not only that, the clock-related compatible properties used here are different than the ones specificed PATCH 10/11 of this series where you document some clock bindings.
also...
[...]
Quoted 19 lines
arch/arm/boot/dts/hi3620.dtsi new file +19
-0+/include/ "skeleton.dtsi" + +/ { + aliases { + serial0 = &uart0; + serial1 = &uart1; + serial2 = &uart2; + serial3 = &uart3; + serial4 = &uart4; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu0: cpu at 0 { + device_type = "cpu"; + compatible = "arm,cortex-a9"; + reg = <0x0>;
arch/arm/boot/dts/hi3620.dtsi line 42
Quoted 10 lines
arch/arm/boot/dts/hi3620.dtsi new file +10
-0+ next-level-cache = <&L2>; + }; + }; + + osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };
...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
Kevin
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
Quoted 7 lines · Kevin Hilman
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>
...
Quoted 33 lines
Quoted 19 lines
arch/arm/boot/dts/hi3620.dtsi new file +19
-0+/include/ "skeleton.dtsi" + +/ { + aliases { + serial0 = &uart0; + serial1 = &uart1; + serial2 = &uart2; + serial3 = &uart3; + serial4 = &uart4; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu0: cpu at 0 { + device_type = "cpu"; + compatible = "arm,cortex-a9"; + reg = <0x0>;arch/arm/boot/dts/hi3620.dtsi line 42
Quoted 10 lines
arch/arm/boot/dts/hi3620.dtsi new file +10
-0+ next-level-cache = <&L2>; + }; + }; + + osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
Quoted 2 lines
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
I would expect all the clock node names to be just "clock", since the node names should describe the type of device not their identity (i.e. clock name).
In turn, this means that each clock node name needs to use a unit address ("@nnn") to make them unique. In turn, this means they must have a reg property since the unit address must match the first entry in the reg property.
Now I assume these clocks don't have any memory-mapped IO registers, so they would need an arbitrary reg value rather than a real one. So it doesn't make sense to place them directly under the root DT node, since their reg values would make no sense within the context of the CPU-visible MMIO space that the root node describes.
In this case, it's typical to put all the clock nodes into e.g. a /clocks node, since that node can introduce a separate numbering-space for clocks. For example, I'd expect something like:
clocks {
#address-cells = <1>;
#size-cells = <0>;
osc32k: clock at 0 {
compatible = "fixed-clock";
reg = <0>;
#clock-cells = <0>;
clock-frequency = <32768>;
clock-output-names = "osc32khz";
};
osc26m: clock at 1 {
compatible = "fixed-clock";
reg = <1>;
#clock-cells = <0>;
clock-frequency = <26000000>;
clock-output-names = "osc26mhz";
};
...
};However, it also depends on what is actually providing those clocks. If every one of them is some standalone device on the board (e.g. a crystal), then just dumping them all in /clocks makes sense. However, if the clocks are provided by some on-SoC clock module, then I'd likely expect the clocks to be contained within the DT node that represents that clock module, which presumably does have some registers, and hence could be a direct child of the root node. For example, I wonder if the following is more accurate:
sctrl: sctrl at fc802000 {
compatible = "hisilicon,sctrl";
reg = <0xfc802000 0x1000>;
#address-cells = <1>;
#size-cells = <0>;
osc32k: clock at 0 {
compatible = "fixed-clock";
reg = <0>;
#clock-cells = <0>;
clock-frequency = <32768>;
clock-output-names = "osc32khz";
};
osc26m: clock at 1 {
compatible = "fixed-clock";
reg = <1>;
#clock-cells = <0>;
clock-frequency = <26000000>;
clock-output-names = "osc26mhz";
};
...
};... since I see there are already quite a few clocks inside the sctrl node.
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
Quoted 45 lines · Stephen Warren
Quoted 7 lines · Kevin Hilman
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>...
Quoted 33 lines
Quoted 19 lines
arch/arm/boot/dts/hi3620.dtsi new file +19
-0+/include/ "skeleton.dtsi" + +/ { + aliases { + serial0 = &uart0; + serial1 = &uart1; + serial2 = &uart2; + serial3 = &uart3; + serial4 = &uart4; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu0: cpu at 0 { + device_type = "cpu"; + compatible = "arm,cortex-a9"; + reg = <0x0>;arch/arm/boot/dts/hi3620.dtsi line 42
Quoted 10 lines
arch/arm/boot/dts/hi3620.dtsi new file +10
-0+ next-level-cache = <&L2>; + }; + }; + + osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
Quoted 2 lines
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
Quoted 8 lines
I would expect all the clock node names to be just "clock", since the node names should describe the type of device not their identity (i.e. clock name).
In turn, this means that each clock node name needs to use a unit address ("@nnn") to make them unique. In turn, this means they must have a reg property since the unit address must match the first entry in the reg property.
No, it's really bad on using a unit address. The register always contains multiple mux or gate or divider. It would cause duplicated unit address.
I tried to use index number also. And it's also bad to append new clock nodes. So I use the label name instead.
Quoted 24 lines
Now I assume these clocks don't have any memory-mapped IO registers, so they would need an arbitrary reg value rather than a real one. So it doesn't make sense to place them directly under the root DT node, since their reg values would make no sense within the context of the CPU-visible MMIO space that the root node describes.
In this case, it's typical to put all the clock nodes into e.g. a /clocks node, since that node can introduce a separate numbering-space for clocks. For example, I'd expect something like:
clocks { #address-cells = <1>; #size-cells = <0>; osc32k: clock at 0 { compatible = "fixed-clock"; reg = <0>; #clock-cells = <0>; clock-frequency = <32768>; clock-output-names = "osc32khz"; }; osc26m: clock at 1 {
Quoted 8 lines
compatible = "fixed-clock";
reg = <1>;
#clock-cells = <0>;
clock-frequency = <26000000>;
clock-output-names = "osc26mhz";
};
...
};Those fixed-clock doesn't contain reg property. Since it needs not to access any clock register. It only provides the clock rate those child clock node.
Quoted 34 lines
However, it also depends on what is actually providing those clocks. If every one of them is some standalone device on the board (e.g. a crystal), then just dumping them all in /clocks makes sense. However, if the clocks are provided by some on-SoC clock module, then I'd likely expect the clocks to be contained within the DT node that represents that clock module, which presumably does have some registers, and hence could be a direct child of the root node. For example, I wonder if the following is more accurate:
sctrl: sctrl at fc802000 { compatible = "hisilicon,sctrl"; reg = <0xfc802000 0x1000>; #address-cells = <1>; #size-cells = <0>; osc32k: clock at 0 { compatible = "fixed-clock"; reg = <0>; #clock-cells = <0>; clock-frequency = <32768>; clock-output-names = "osc32khz"; }; osc26m: clock at 1 { compatible = "fixed-clock"; reg = <1>; #clock-cells = <0>; clock-frequency = <26000000>; clock-output-names = "osc26mhz"; }; ... };
Quoted 1 line
... since I see there are already quite a few clocks inside the sctrl node.
I can move all others clock nodes into clocks node, likes osc32k, osc26m. Since they're not belong to sctrl register bank. And I also move all clock nodes into a new dtsi file to make it more clearly.
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
Quoted 28 lines · Haojian Zhuang
Quoted 27 lines · Stephen Warren
Quoted 7 lines · Kevin Hilman
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>...
Quoted 7 lines
arch/arm/boot/dts/hi3620.dtsi line 42
Quoted 6 lines
arch/arm/boot/dts/hi3620.dtsi new file +6
-0+ osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };Quoted 5 lines
...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
I would expect all the clock node names to be just "clock", since the node names should describe the type of device not their identity (i.e. clock name).
Quoted 7 lines
Quoted 4 lines
In turn, this means that each clock node name needs to use a unit address ("@nnn") to make them unique. In turn, this means they must have a reg property since the unit address must match the first entry in the reg property.
No, it's really bad on using a unit address. The register always contains multiple mux or gate or divider. It would cause duplicated unit address.
There shouldn't be multiple nodes with identical reg values; if that's happening, then it seems like the mapping of nodes to HW is incorrect.
Each HW block should have 1 DT node. That way, the reg values won't collide.
Quoted 20 lines
I tried to use index number also. And it's also bad to append new clock nodes. So I use the label name instead.
Quoted 17 lines
Now I assume these clocks don't have any memory-mapped IO registers, so they would need an arbitrary reg value rather than a real one. So it doesn't make sense to place them directly under the root DT node, since their reg values would make no sense within the context of the CPU-visible MMIO space that the root node describes.
In this case, it's typical to put all the clock nodes into e.g. a /clocks node, since that node can introduce a separate numbering-space for clocks. For example, I'd expect something like:
clocks { #address-cells = <1>; #size-cells = <0>; osc32k: clock at 0 { compatible = "fixed-clock"; reg = <0>;
...
Quoted 3 lines
Quoted 3 lines
osc26m: clock at 1 { compatible = "fixed-clock"; reg = <1>;
...
Quoted 2 lines
Those fixed-clock doesn't contain reg property. Since it needs not to access any clock register. It only provides the clock rate those child clock node.
Inside the clocks node, the reg property is just a dummy value.
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
Quoted 34 lines · Stephen Warren
Quoted 33 lines · Haojian Zhuang
Quoted 31 lines · Stephen Warren
Quoted 7 lines · Kevin Hilman
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>...
Quoted 7 lines
arch/arm/boot/dts/hi3620.dtsi line 42
Quoted 6 lines
arch/arm/boot/dts/hi3620.dtsi new file +6
-0+ osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };Quoted 5 lines
...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
I would expect all the clock node names to be just "clock", since the node names should describe the type of device not their identity (i.e. clock name).
In turn, this means that each clock node name needs to use a unit address ("@nnn") to make them unique. In turn, this means they must have a reg property since the unit address must match the first entry in the reg property.
Quoted 8 lines
Quoted 2 lines
No, it's really bad on using a unit address. The register always contains multiple mux or gate or divider. It would cause duplicated unit address.
There shouldn't be multiple nodes with identical reg values; if that's happening, then it seems like the mapping of nodes to HW is incorrect.
Each HW block should have 1 DT node. That way, the reg values won't collide.
At here, I emphasize each clock node is one clock node. They are organized in tree. The same register integrates multiple clock gate/clock mux/clock divider. If each clock node is depend on reg, maybe it's not easy to read and the clock driver will be more complex.
Quoted 24 lines
Quoted 20 lines
I tried to use index number also. And it's also bad to append new clock nodes. So I use the label name instead.
Quoted 17 lines
Now I assume these clocks don't have any memory-mapped IO registers, so they would need an arbitrary reg value rather than a real one. So it doesn't make sense to place them directly under the root DT node, since their reg values would make no sense within the context of the CPU-visible MMIO space that the root node describes.
In this case, it's typical to put all the clock nodes into e.g. a /clocks node, since that node can introduce a separate numbering-space for clocks. For example, I'd expect something like:
clocks { #address-cells = <1>; #size-cells = <0>; osc32k: clock at 0 { compatible = "fixed-clock"; reg = <0>;...
Quoted 3 lines
Quoted 3 lines
osc26m: clock at 1 { compatible = "fixed-clock"; reg = <1>;
Quoted 6 lines
...
Quoted 2 lines
Those fixed-clock doesn't contain reg property. Since it needs not to access any clock register. It only provides the clock rate those child clock node.
Inside the clocks node, the reg property is just a dummy value.
Is a dummy value helpful? I don't think so.
Regards
Haojian
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
Quoted 41 lines · Haojian Zhuang
Quoted 40 lines · Stephen Warren
Quoted 28 lines · Haojian Zhuang
Quoted 27 lines · Stephen Warren
Quoted 7 lines · Kevin Hilman
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>...
Quoted 7 lines
arch/arm/boot/dts/hi3620.dtsi line 42
Quoted 6 lines
arch/arm/boot/dts/hi3620.dtsi new file +6
-0+ osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };Quoted 5 lines
...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
I would expect all the clock node names to be just "clock", since the node names should describe the type of device not their identity (i.e. clock name).
Quoted 7 lines
Quoted 4 lines
In turn, this means that each clock node name needs to use a unit address ("@nnn") to make them unique. In turn, this means they must have a reg property since the unit address must match the first entry in the reg property.
No, it's really bad on using a unit address. The register always contains multiple mux or gate or divider. It would cause duplicated unit address.
There shouldn't be multiple nodes with identical reg values; if that's happening, then it seems like the mapping of nodes to HW is incorrect.
Quoted 6 lines
Quoted 1 line
Each HW block should have 1 DT node. That way, the reg values won't collide.
At here, I emphasize each clock node is one clock node. They are organized in tree. The same register integrates multiple clock gate/clock mux/clock divider. If each clock node is depend on reg, maybe it's not easy to read and the clock driver will be more complex.
If there's a single HW block (or single register) that provides multiple clocks, there should be a single DT node and a single device that provides multiple clocks.
Quoted 24 lines
Quoted 24 lines
Quoted 20 lines
I tried to use index number also. And it's also bad to append new clock nodes. So I use the label name instead.
Quoted 17 lines
Now I assume these clocks don't have any memory-mapped IO registers, so they would need an arbitrary reg value rather than a real one. So it doesn't make sense to place them directly under the root DT node, since their reg values would make no sense within the context of the CPU-visible MMIO space that the root node describes.
In this case, it's typical to put all the clock nodes into e.g. a /clocks node, since that node can introduce a separate numbering-space for clocks. For example, I'd expect something like:
clocks { #address-cells = <1>; #size-cells = <0>; osc32k: clock at 0 { compatible = "fixed-clock"; reg = <0>;...
Quoted 3 lines
Quoted 3 lines
osc26m: clock at 1 { compatible = "fixed-clock"; reg = <1>;
Quoted 8 lines
Quoted 6 lines
...
Quoted 2 lines
Those fixed-clock doesn't contain reg property. Since it needs not to access any clock register. It only provides the clock rate those child clock node.
Inside the clocks node, the reg property is just a dummy value.
Is a dummy value helpful? I don't think so.
It's not a matter of whether it's helpful; it's just how DT works.
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
Quoted 79 lines · Stephen Warren
Quoted 41 lines · Haojian Zhuang
Quoted 40 lines · Stephen Warren
Quoted 28 lines · Haojian Zhuang
Quoted 27 lines · Stephen Warren
Quoted 7 lines · Kevin Hilman
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>...
Quoted 7 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };Quoted 5 lines
...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
I would expect all the clock node names to be just "clock", since the node names should describe the type of device not their identity (i.e. clock name).
Quoted 7 lines
Quoted 4 lines
In turn, this means that each clock node name needs to use a unit address ("@nnn") to make them unique. In turn, this means they must have a reg property since the unit address must match the first entry in the reg property.
No, it's really bad on using a unit address. The register always contains multiple mux or gate or divider. It would cause duplicated unit address.
There shouldn't be multiple nodes with identical reg values; if that's happening, then it seems like the mapping of nodes to HW is incorrect.
Quoted 6 lines
Quoted 1 line
Each HW block should have 1 DT node. That way, the reg values won't collide.
At here, I emphasize each clock node is one clock node. They are organized in tree. The same register integrates multiple clock gate/clock mux/clock divider. If each clock node is depend on reg, maybe it's not easy to read and the clock driver will be more complex.
If there's a single HW block (or single register) that provides multiple clocks, there should be a single DT node and a single device that provides multiple clocks.
Quoted 26 lines
Quoted 26 lines
Quoted 20 lines
I tried to use index number also. And it's also bad to append new clock nodes. So I use the label name instead.
Quoted 17 lines
Now I assume these clocks don't have any memory-mapped IO registers, so they would need an arbitrary reg value rather than a real one. So it doesn't make sense to place them directly under the root DT node, since their reg values would make no sense within the context of the CPU-visible MMIO space that the root node describes.
In this case, it's typical to put all the clock nodes into e.g. a /clocks node, since that node can introduce a separate numbering-space for clocks. For example, I'd expect something like:
clocks { #address-cells = <1>; #size-cells = <0>; osc32k: clock at 0 { compatible = "fixed-clock"; reg = <0>;...
Quoted 3 lines
Quoted 3 lines
osc26m: clock at 1 { compatible = "fixed-clock"; reg = <1>;...
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Those fixed-clock doesn't contain reg property. Since it needs not to access any clock register. It only provides the clock rate those child clock node.
Inside the clocks node, the reg property is just a dummy value.
Is a dummy value helpful? I don't think so.
It's not a matter of whether it's helpful; it's just how DT works.
I copied some texts from device tree spec (Page 87. file name:
Power_ePAPER_APPROVED_v1.0.pdf).
Node and property definitions
Device tree nodes are defined with a node name and unit address
with braces marking the start and
end of the node definition. They may be preceded by a label.
[label:] node-name[@unit-address] {
[properties definitions]
[child nodes]
}We can see that "unit-address" is optional.
I also copied some texts from $LINUX/Documentation/devicetree/usage-model.txt
Structurally, the DT is a tree, or acyclic graph with named nodes, and nodes may have an arbitrary number of named properties encapsulating arbitrary data. A mechanism also exists to create arbitrary links from one node to another outside of the natural tree structure.
Conceptually, a common set of usage conventions, called 'bindings', is defined for how data should appear in the tree to describe typical hardware characteristics including data busses, interrupt lines, GPIO connections, and peripheral devices.
I define each clock as one node although multiple clocks share one register, because it could
share clock driver as much as possible. Whether they're sharing one register or not, they
can be distinguished by clock types. I think that encapsulating them into one single
node could only make clock driver complex. Sometimes one clock is using a single register,
and sometime some clocks are sharing a single register. If the clock driver wants to support
both, it needs to add more logic to check. And I don't think that abstracting clock breaks
any law in DT.
Regards
Haojian
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
Quoted 97 lines · Haojian Zhuang
Quoted 79 lines · Stephen Warren
Quoted 41 lines · Haojian Zhuang
Quoted 40 lines · Stephen Warren
Quoted 28 lines · Haojian Zhuang
Quoted 27 lines · Stephen Warren
Quoted 7 lines · Kevin Hilman
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>...
Quoted 7 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };Quoted 5 lines
...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
I would expect all the clock node names to be just "clock", since the node names should describe the type of device not their identity (i.e. clock name).
Quoted 7 lines
Quoted 4 lines
In turn, this means that each clock node name needs to use a unit address ("@nnn") to make them unique. In turn, this means they must have a reg property since the unit address must match the first entry in the reg property.
No, it's really bad on using a unit address. The register always contains multiple mux or gate or divider. It would cause duplicated unit address.
There shouldn't be multiple nodes with identical reg values; if that's happening, then it seems like the mapping of nodes to HW is incorrect.
Quoted 6 lines
Quoted 1 line
Each HW block should have 1 DT node. That way, the reg values won't collide.
At here, I emphasize each clock node is one clock node. They are organized in tree. The same register integrates multiple clock gate/clock mux/clock divider. If each clock node is depend on reg, maybe it's not easy to read and the clock driver will be more complex.
If there's a single HW block (or single register) that provides multiple clocks, there should be a single DT node and a single device that provides multiple clocks.
Quoted 26 lines
Quoted 26 lines
Quoted 20 lines
I tried to use index number also. And it's also bad to append new clock nodes. So I use the label name instead.
Quoted 17 lines
Now I assume these clocks don't have any memory-mapped IO registers, so they would need an arbitrary reg value rather than a real one. So it doesn't make sense to place them directly under the root DT node, since their reg values would make no sense within the context of the CPU-visible MMIO space that the root node describes.
In this case, it's typical to put all the clock nodes into e.g. a /clocks node, since that node can introduce a separate numbering-space for clocks. For example, I'd expect something like:
clocks { #address-cells = <1>; #size-cells = <0>; osc32k: clock at 0 { compatible = "fixed-clock"; reg = <0>;...
Quoted 3 lines
Quoted 3 lines
osc26m: clock at 1 { compatible = "fixed-clock"; reg = <1>;...
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Those fixed-clock doesn't contain reg property. Since it needs not to access any clock register. It only provides the clock rate those child clock node.
Inside the clocks node, the reg property is just a dummy value.
Is a dummy value helpful? I don't think so.
It's not a matter of whether it's helpful; it's just how DT works.
I copied some texts from device tree spec (Page 87. file name:
Power_ePAPER_APPROVED_v1.0.pdf).Node and property definitions
Device tree nodes are defined with a node name and unit address
with braces marking the start and
Quoted 8 lines
end of the node definition. They may be preceded by a label. [label:] node-name[@unit-address] { [properties definitions] [child nodes] }We can see that "unit-address" is optional.
It's optional syntactically, yes.
But,
a) Nodes must be named after their type, not their identity, so then you end up with multiple nodes named "clock", all/many of which are children of the same parent.
b) Nodes must have unique names within their parent, so you can unambiguously specify which node you're accessing.
If (b) is not satisfied by node name alone, adding the unit address to the node name is the way you make it unique.
Quoted 12 lines
I also copied some texts from
$LINUX/Documentation/devicetree/usage-model.txtStructurally, the DT is a tree, or acyclic graph with named nodes, and nodes may have an arbitrary number of named properties encapsulating arbitrary data. A mechanism also exists to create arbitrary links from one node to another outside of the natural tree structure.
Conceptually, a common set of usage conventions, called 'bindings', is defined for how data should appear in the tree to describe typical hardware characteristics including data busses, interrupt lines, GPIO connections, and peripheral devices.
Quoted 13 lines
I define each clock as one node although multiple clocks share one register, because it could
share clock driver as much as possible. Whether they're sharing one register or not, they
can be distinguished by clock types. I think that encapsulating them into one single
node could only make clock driver complex. Sometimes one clock is using a single register,
and sometime some clocks are sharing a single register. If the clock driver wants to support
both, it needs to add more logic to check. And I don't think that abstracting clock breaks
any law in DT.
The DT structure should be driven purely by the HW structure. The driver structure should have no influence over the DT structure. If there's a single HW module that exposes N clocks, there should be a single DT node that's a clock provider, which uses #clock-cells>0 to identify the clocks that consumers use.
PATCH v7 5/11 ARM: dts: enable hi4511 with device tree
In reply to Kevin HilmanQuoted 8 lines · Kevin Hilman
[+ DT maintainers]
Quoted 3 lines · Haojian Zhuang
Enable Hisilicon Hi4511 development platform with device tree support.
Signed-off-by Haojian Zhuang <[email protected]>
Quoted 9 lines
There seems to be use of several new vendor-specific bindings here that are not documented anywhere:
hisilicon,sctrl
hisilicon,pmctrl
hisilicon,clk-mux
hisilicon,clk-div
hisilicon,clk-gate
Those are in clock bindings with clock patches. Since you're always requesting those patch going through Mike's tree. I didn't pick them up into this patch set.
Quoted 3 lines
Not only that, the clock-related compatible properties used here are different than the ones specificed PATCH 10/11 of this series where you document some clock bindings.
No. #10 is only for Hi3716. It's a little different on hi3xxx. But now I dropped this patch in my pull request.
Since there's still some comments on the whole v7 patchset, I'll update with a v8.
Quoted 42 lines
also...
[...]
Quoted 19 lines
arch/arm/boot/dts/hi3620.dtsi new file +19
-0+/include/ "skeleton.dtsi" + +/ { + aliases { + serial0 = &uart0; + serial1 = &uart1; + serial2 = &uart2; + serial3 = &uart3; + serial4 = &uart4; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu0: cpu at 0 { + device_type = "cpu"; + compatible = "arm,cortex-a9"; + reg = <0x0>;arch/arm/boot/dts/hi3620.dtsi line 42
Quoted 10 lines
arch/arm/boot/dts/hi3620.dtsi new file +10
-0+ next-level-cache = <&L2>; + }; + }; + + osc32k: osc32k { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "osc32khz"; + };...seems many of the recent users of clocks have grouped them into a clocks {} grouping on a "simple-bus".
DT folks: is there a rule of thumb on how whether these fixed clocks should be grouped on a simple bus?
Kevin
PATCH v7 6/11 ARM: config: enable hi3xxx in multi_v7_defconfig
In reply to Haojian ZhuangEnable ARCH_HI3xxx in multi_v7_defconfig.
--- arch/arm/configs/multi_v7_defconfig | 1 + 1 file changed, 1 insertion(+)
arch/arm/configs/multi_v7_defconfig +1 -0
@@ -8,6 +8,7 @@ CONFIG_MACH_ARMADA_XP=y CONFIG_ARCH_BCM=y CONFIG_GPIO_PCA953X=y CONFIG_ARCH_HIGHBANK=y +CONFIG_ARCH_HI3xxx=y CONFIG_ARCH_KEYSTONE=y CONFIG_ARCH_MXC=y CONFIG_MACH_IMX51_DT=y
Signature · 1 line
1.8.1.2
PATCH v7 7/11 ARM: config: add defconfig for Hi3xxx
In reply to Haojian ZhuangAdd default config for arch-hi3xxx. It's used for Hisilicon Hi3xxx SoC.
--- arch/arm/configs/hi3xxx_defconfig | 56 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 arch/arm/configs/hi3xxx_defconfig
arch/arm/configs/hi3xxx_defconfig new file +56 -0
@@ -0,0 +1,56 @@ +CONFIG_IRQ_DOMAIN_DEBUG=y +CONFIG_NO_HZ=y +CONFIG_HIGH_RES_TIMERS=y +CONFIG_BLK_DEV_INITRD=y +CONFIG_RD_LZMA=y +CONFIG_ARCH_HI3xxx=y +CONFIG_SMP=y +CONFIG_PREEMPT=y +CONFIG_AEABI=y +CONFIG_ARM_APPENDED_DTB=y +CONFIG_NET=y +CONFIG_UNIX=y +CONFIG_INET=y +CONFIG_IP_PNP=y +CONFIG_IP_PNP_DHCP=y +CONFIG_DEVTMPFS=y +CONFIG_DEVTMPFS_MOUNT=y +CONFIG_BLK_DEV_SD=y +CONFIG_ATA=y +CONFIG_SATA_AHCI_PLATFORM=y +CONFIG_NETDEVICES=y +CONFIG_SERIAL_AMBA_PL011=y +CONFIG_SERIAL_AMBA_PL011_CONSOLE=y +CONFIG_SERIAL_OF_PLATFORM=y +CONFIG_I2C_DESIGNWARE_PLATFORM=y +CONFIG_SPI=y +CONFIG_SPI_PL022=y +CONFIG_PINCTRL_SINGLE=y +CONFIG_GPIO_GENERIC_PLATFORM=y +CONFIG_REGULATOR_GPIO=y +CONFIG_DRM=y +CONFIG_FB_SIMPLE=y +CONFIG_USB=y +CONFIG_USB_XHCI_HCD=y +CONFIG_USB_EHCI_HCD=y +CONFIG_USB_EHCI_MXC=y +CONFIG_USB_EHCI_HCD_PLATFORM=y +CONFIG_USB_STORAGE=y +CONFIG_NOP_USB_XCEIV=y +CONFIG_MMC=y +CONFIG_RTC_CLASS=y +CONFIG_RTC_DRV_PL031=y +CONFIG_DMADEVICES=y +CONFIG_DW_DMAC=y +CONFIG_PL330_DMA=y +CONFIG_PWM=y +CONFIG_EXT4_FS=y +CONFIG_TMPFS=y +CONFIG_NFS_FS=y +CONFIG_NFS_V3_ACL=y +CONFIG_NFS_V4=y +CONFIG_ROOT_NFS=y +CONFIG_PRINTK_TIME=y +CONFIG_DEBUG_FS=y +CONFIG_DEBUG_KERNEL=y +CONFIG_LOCKUP_DETECTOR=y
Signature · 1 line
1.8.1.2
PATCH v7 8/11 ARM: hi3xxx: add smp support
In reply to Haojian ZhuangFrom: Zhangfei Gao <[email protected]>
Enable SMP support on hi3xxx platform
--- .../bindings/arm/hisilicon/hisilicon.txt | 47 ++++++++++++++++ arch/arm/boot/dts/hi3620.dtsi | 28 +++++++++- arch/arm/boot/dts/hi4511.dts | 2 +- arch/arm/mach-hi3xxx/Kconfig | 2 + arch/arm/mach-hi3xxx/Makefile | 3 +- arch/arm/mach-hi3xxx/core.h | 14 +++++ arch/arm/mach-hi3xxx/hi3xxx.c | 5 ++ arch/arm/mach-hi3xxx/platsmp.c | 43 ++++++++++++++ arch/arm/mach-hi3xxx/system.c | 65 ++++++++++++++++++++++ 9 files changed, 206 insertions(+), 3 deletions(-) create mode 100644 arch/arm/mach-hi3xxx/core.h create mode 100644 arch/arm/mach-hi3xxx/platsmp.c create mode 100644 arch/arm/mach-hi3xxx/system.c
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +47 -0
@@ -8,3 +8,50 @@ Required root node properties: Hi4511 Board Required root node properties: - compatible = "hisilicon,hi3620-hi4511"; + + +Hisilicon sctrl resiter + +Required properties: +- compatible : "hisilicon,sctrl" +- reg : Address and size of sysctrl. + +Optional properties: +- smp_reg : offset in sysctrl for notifying slave cpu booting + cpu 1, reg; + cpu 2, reg + 0x4; + cpu 3, reg + 0x8; + If reg value is not zero, cpun exit wfi and go +- resume_reg : offset in sysctrl for notifying cpu0 when resume +- reset_reg : offset in sysctrl for system reset + +Example: + hi3716: + sctrl at f8000000 { + compatible = "hisilicon,sctrl"; + reg = <0xf8000000 0x1000>; + smp_reg = <0xc0>; + reboot_reg = <0x4>; + }; + + hi3620: + sctrl at fc802000 { + compatible = "hisilicon,sctrl"; + reg = <0xfc802000 0x1000>; + smp_reg = <0x31c>; + resume_reg = <0x308>; + reboot_reg = <0x4>; + }; + +Hi3716 cpuctrl register +Required properties: +- compatible : "hisilicon,cpuctrl" +- reg : Address and size of cpuctrl. + +Hi3716 specific cpuctrl register, control hotplug info etc. +Example: + hi3716: + cpuctrl at f8a22000 { + compatible = "hisilicon,cpuctrl"; + reg = <0xf8a22000 0x2000>; + };
arch/arm/boot/dts/hi3620.dtsi +27 -1
@@ -32,6 +32,24 @@ reg = <0x0>; next-level-cache = <&L2>; }; + cpu1: cpu at 1 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <1>; + next-level-cache = <&L2>; + }; + cpu2: cpu at 2 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <2>; + next-level-cache = <&L2>; + }; + cpu3: cpu at 3 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <3>; + next-level-cache = <&L2>; + }; }; osc32k: osc32k {
@@ -130,7 +148,9 @@ reg = <0xfc802000 0x1000>; #address-cells = <1>; #size-cells = <0>; - + smp_reg = <0x31c>; + resume_reg = <0x308>; + reboot_reg = <0x4>; timer0_mux: timer0_mux { compatible = "hisilicon,clk-mux";
@@ -1242,6 +1262,12 @@ status = "disabled"; }; + timer5: timer at fc000600 { + compatible = "arm,cortex-a9-twd-timer"; + reg = <0xfc000600 0x20>; + interrupts = <1 13 0xf01>; + }; + uart0: uart at fcb00000 { compatible = "arm,pl011", "arm,primecell"; reg = <0xfcb00000 0x1000>;
arch/arm/boot/dts/hi4511.dts +1 -1
@@ -15,7 +15,7 @@ compatible = "hisilicon,hi3620-hi4511"; chosen { - bootargs = "console=ttyAMA0,115200 root=/dev/ram0 earlyprintk"; + bootargs = "console=ttyAMA0,115200 root=/dev/ram0 nr_cpus=4 earlyprintk"; }; memory {
arch/arm/mach-hi3xxx/Kconfig +2 -0
@@ -6,6 +6,8 @@ config ARCH_HI3xxx select CACHE_L2X0 select CLKSRC_OF select GENERIC_CLOCKEVENTS + select HAVE_ARM_SCU + select HAVE_SMP select PINCTRL select PINCTRL_SINGLE help
arch/arm/mach-hi3xxx/Makefile +2 -1
@@ -2,4 +2,5 @@ # Makefile for Hisilicon Hi36xx/Hi37xx processors line # -obj-y += hi3xxx.o +obj-y += hi3xxx.o system.o +obj-$(CONFIG_SMP) += platsmp.o
arch/arm/mach-hi3xxx/core.h new file +14 -0
@@ -0,0 +1,14 @@ +#ifndef __HISILICON_CORE_H +#define __HISILICON_CORE_H + +#include <linux/init.h> +#include <linux/reboot.h> + +extern void hs_set_cpu_jump (int cpu,void *jump_addr); +extern int hs_get_cpu_jump (int cpu); +extern void secondary_startup (void ); +extern void hs_map_io (void ); +extern void hs_restart (enum reboot_mode ,const char *cmd); +extern struct smp_operations hs_smp_ops; + +#endif
arch/arm/mach-hi3xxx/hi3xxx.c +5 -0
@@ -19,8 +19,11 @@#include <asm/mach/arch.h> #include <asm/mach/map.h> +#include "core.h" +static void __init hi3xxx_timer_init (void ) { +hs_map_io ();of_clk_init (NULL );clocksource_of_init (); }
@@ -33,4 +36,6 @@ static const char *hs_compat[] __initdata = {DT_MACHINE_START (HI3xxx,"Hisilicon Hi36xx/Hi37xx (Flattened Device Tree)" ) .init_time = hi3xxx_timer_init, .dt_compat = hs_compat, + .smp = smp_ops(hs_smp_ops), + .restart = hs_restart,MACHINE_END
arch/arm/mach-hi3xxx/platsmp.c new file +43 -0
43 lines not on this page · raw message
arch/arm/mach-hi3xxx/system.c new file +65 -0
65 lines not on this page · raw message
Signature · 1 line
1.8.1.2
PATCH v7 8/11 ARM: hi3xxx: add smp support
Hi,
Some comments below.
Quoted 27 lines · Haojian Zhuang
From: Zhangfei Gao <[email protected]>
Enable SMP support on hi3xxx platform
Signed-off-by Zhangfei Gao <[email protected]>Tested-by Zhang Mingjun <[email protected]>Tested-by Li Xin <[email protected]>--- .../bindings/arm/hisilicon/hisilicon.txt | 47 ++++++++++++++++ arch/arm/boot/dts/hi3620.dtsi | 28 +++++++++- arch/arm/boot/dts/hi4511.dts | 2 +- arch/arm/mach-hi3xxx/Kconfig | 2 + arch/arm/mach-hi3xxx/Makefile | 3 +- arch/arm/mach-hi3xxx/core.h | 14 +++++ arch/arm/mach-hi3xxx/hi3xxx.c | 5 ++ arch/arm/mach-hi3xxx/platsmp.c | 43 ++++++++++++++ arch/arm/mach-hi3xxx/system.c | 65 ++++++++++++++++++++++ 9 files changed, 206 insertions(+), 3 deletions(-) create mode 100644 arch/arm/mach-hi3xxx/core.h create mode 100644 arch/arm/mach-hi3xxx/platsmp.c create mode 100644 arch/arm/mach-hi3xxx/system.cDocumentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +0
-0
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 13
Quoted 6 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +3
-0@@ -8,3 +8,50 @@ Required root node properties: Hi4511 Board Required root node properties: - compatible = "hisilicon,hi3620-hi4511"; + + +Hisilicon sctrl resiter
Resiter seems like odd naming here.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 17
Quoted 4 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +4
-0+ +Required properties: +- compatible : "hisilicon,sctrl" +- reg : Address and size of sysctrl.
sysctrl or sctrl? sysctrl isn't really too long, might as well use that.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 26
Quoted 9 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +9
-0+ +Optional properties: +- smp_reg : offset in sysctrl for notifying slave cpu booting + cpu 1, reg; + cpu 2, reg + 0x4; + cpu 3, reg + 0x8; + If reg value is not zero, cpun exit wfi and go +- resume_reg : offset in sysctrl for notifying cpu0 when resume +- reset_reg : offset in sysctrl for system reset
Underscores are usually not used in properties, you use dashes instead.
Anyway, I suggest defining those as the different cells in a regular 'reg' property instead.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 30
Quoted 4 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +4
-0+ +Example: + hi3716: + sctrl at f8000000 {
Labels are usually not done on a separate line before.
Common node names would be more clear-text than this. Maybe "system-controller" in this case.
Quoted 11 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +11
-0+ compatible = "hisilicon,sctrl"; + reg = <0xf8000000 0x1000>; + smp_reg = <0xc0>; + reboot_reg = <0x4>; + }; + + hi3620: + sctrl at fc802000 { + compatible = "hisilicon,sctrl"; + reg = <0xfc802000 0x1000>; + smp_reg = <0x31c>;
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 51
Quoted 10 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +10
-0+ resume_reg = <0x308>; + reboot_reg = <0x4>; + }; + +Hi3716 cpuctrl register +Required properties: +- compatible : "hisilicon,cpuctrl" +- reg : Address and size of cpuctrl. + +Hi3716 specific cpuctrl register, control hotplug info etc.
Controlling hotplug _info_? Seems like odd wording here.
Quoted 11 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +6
-0+Example: + hi3716: + cpuctrl at f8a22000 { + compatible = "hisilicon,cpuctrl"; + reg = <0xf8a22000 0x2000>; + };arch/arm/boot/dts/hi3620.dtsi +0
-0
arch/arm/boot/dts/hi3620.dtsi line 35
Quoted 4 lines
arch/arm/boot/dts/hi3620.dtsi +1
-0@@ -32,6 +32,24 @@ reg = <0x0>; next-level-cache = <&L2>; }; + cpu1: cpu at 1 {
Do you need labels here? I don't think these are referred to as phandles anywhere?
Quoted 49 lines
arch/arm/boot/dts/hi3620.dtsi +26
-1+ compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <1>; + next-level-cache = <&L2>; + }; + cpu2: cpu at 2 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <2>; + next-level-cache = <&L2>; + }; + cpu3: cpu at 3 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <3>; + next-level-cache = <&L2>; + }; }; osc32k: osc32k {@@ -130,7 +148,9 @@ reg = <0xfc802000 0x1000>; #address-cells = <1>; #size-cells = <0>; - + smp_reg = <0x31c>; + resume_reg = <0x308>; + reboot_reg = <0x4>; timer0_mux: timer0_mux { compatible = "hisilicon,clk-mux";@@ -1242,6 +1262,12 @@ status = "disabled"; }; + timer5: timer at fc000600 { + compatible = "arm,cortex-a9-twd-timer"; + reg = <0xfc000600 0x20>; + interrupts = <1 13 0xf01>; + }; + uart0: uart at fcb00000 { compatible = "arm,pl011", "arm,primecell"; reg = <0xfcb00000 0x1000>;arch/arm/boot/dts/hi4511.dts +0
-0
arch/arm/boot/dts/hi4511.dts line 18
Quoted 5 lines
arch/arm/boot/dts/hi4511.dts +1
-1@@ -15,7 +15,7 @@ compatible = "hisilicon,hi3620-hi4511"; chosen { - bootargs = "console=ttyAMA0,115200 root=/dev/ram0 earlyprintk"; + bootargs = "console=ttyAMA0,115200 root=/dev/ram0 nr_cpus=4 earlyprintk";
nr_cpus should not be needed on the command line.
Quoted 33 lines
arch/arm/boot/dts/hi4511.dts +0
-0}; memory {arch/arm/mach-hi3xxx/Kconfig +2
-0@@ -6,6 +6,8 @@ config ARCH_HI3xxx select CACHE_L2X0 select CLKSRC_OF select GENERIC_CLOCKEVENTS + select HAVE_ARM_SCU + select HAVE_SMP select PINCTRL select PINCTRL_SINGLE helparch/arm/mach-hi3xxx/Makefile +2
-1@@ -2,4 +2,5 @@ # Makefile for Hisilicon Hi36xx/Hi37xx processors line # -obj-y += hi3xxx.o +obj-y += hi3xxx.o system.o +obj-$(CONFIG_SMP) += platsmp.oarch/arm/mach-hi3xxx/core.h new file +0
-0
arch/arm/mach-hi3xxx/core.h line 4
Quoted 4 lines
arch/arm/mach-hi3xxx/core.h new file +4
-0@@ -0,0 +1,14 @@ +#ifndef __HISILICON_CORE_H +#define __HISILICON_CORE_H + +#include <linux/init.h>
I don't think you need linux/init.h here?
Quoted 156 lines
arch/arm/mach-hi3xxx/core.h new file +10
-0+#include <linux/reboot.h> + +extern void hs_set_cpu_jump (int cpu,void *jump_addr); +extern int hs_get_cpu_jump (int cpu); +extern void secondary_startup (void ); +extern void hs_map_io (void ); +extern void hs_restart (enum reboot_mode ,const char *cmd); +extern struct smp_operations hs_smp_ops; + +#endif arch/arm/mach-hi3xxx/hi3xxx.c +5
-0@@ -19,8 +19,11 @@#include <asm/mach/arch.h> #include <asm/mach/map.h> +#include "core.h" +static void __init hi3xxx_timer_init (void ) { +hs_map_io ();of_clk_init (NULL );clocksource_of_init (); }@@ -33,4 +36,6 @@ static const char *hs_compat[] __initdata = {DT_MACHINE_START (HI3xxx,"Hisilicon Hi36xx/Hi37xx (Flattened Device Tree)" ) .init_time = hi3xxx_timer_init, .dt_compat = hs_compat, + .smp = smp_ops(hs_smp_ops), + .restart = hs_restart,MACHINE_END arch/arm/mach-hi3xxx/platsmp.c new file +43
-0@@ -0,0 +1,43 @@ +/* +* Copyright (c) 2013 Linaro Ltd. +* Copyright (c) 2013 Hisilicon Limited. +* Based on platsmp.c, Copyright (C) 2002 ARM Ltd. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +*/ +#include <linux/smp.h> +#include <linux/io.h> +#include <asm/smp_scu.h> + +#include "core.h" + +static void __init hs_smp_prepare_cpus (unsigned int max_cpus) +{ +unsigned long base; +void __iomem *scu_base; + +if (scu_a9_has_base ()) { + base =scu_a9_get_base (); + scu_base =ioremap (base,SZ_4K ); +if (!scu_base) { +pr_err ("ioremap(scu_base) failed\n" ); +return ; + } +scu_enable (scu_base); +iounmap (scu_base); + } +} + +static int hs_boot_secondary (unsigned int cpu,struct task_struct *idle) +{ +hs_set_cpu_jump (cpu, secondary_startup); +arch_send_wakeup_ipi_mask (cpumask_of (cpu)); +return 0 ; +} + +struct smp_operations hs_smp_ops__initdata = { + .smp_prepare_cpus = hs_smp_prepare_cpus, + .smp_boot_secondary = hs_boot_secondary, +};arch/arm/mach-hi3xxx/system.c new file +65
-0@@ -0,0 +1,65 @@ +/* +* Copyright (c) 2013 Linaro Ltd. +* Copyright (c) 2013 Hisilicon Limited. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +*/ +#include <linux/io.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> + +#include <asm/proc-fns.h> +#include <asm/smp_plat.h> + +#include "core.h" + +static void __iomem *hs_sctrl_base; +static int hs_smp_reg; +static int hs_resume_reg; +static int hs_reboot_reg; + +void hs_map_io (void ) +{ +struct device_node *np; + + np =of_find_compatible_node (NULL ,NULL ,"hisilicon,sctrl" ); +if (np) { + hs_sctrl_base =of_iomap (np,0 ); +if (!hs_sctrl_base) +pr_err ("of_iomap(sctrl_base) failed\n" ); +of_property_read_u32 (np,"smp_reg" , &hs_smp_reg); +of_property_read_u32 (np,"resume_reg" , &hs_resume_reg); +of_property_read_u32 (np,"reboot_reg" , &hs_reboot_reg); + } +} + +void hs_set_cpu_jump (int cpu,void *jump_addr) +{ +int offset = hs_smp_reg; + + cpu =cpu_logical_map (cpu); +if (cpu >0 ) + offset +=0x04 * (cpu -1 ); +writel_relaxed (virt_to_phys (jump_addr), hs_sctrl_base + offset); +} + +int hs_get_cpu_jump (int cpu) +{ +int offset = hs_smp_reg; + + cpu =cpu_logical_map (cpu); +if (cpu >0 ) + offset +=0x04 * (cpu -1 ); +return readl_relaxed (hs_sctrl_base + offset); +} + +void hs_restart (enum reboot_mode mode,const char *cmd) +{ +writel_relaxed (0xdeadbeef , hs_sctrl_base + hs_reboot_reg); + +while (1 ) +cpu_do_idle (); +} +Signature · 1 line
1.8.1.2
PATCH v7 8/11 ARM: hi3xxx: add smp support
Thanks Olof, will update with your comments.
Quoted 34 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 13
Quoted 12 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +3
-0@@ -8,3 +8,50 @@ Required root node properties: Hi4511 Board Required root node properties: - compatible = "hisilicon,hi3620-hi4511"; + + +Hisilicon sctrl resiterResiter seems like odd naming here.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 17
Quoted 4 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +4
-0+ +Required properties: +- compatible : "hisilicon,sctrl" +- reg : Address and size of sysctrl.sysctrl or sctrl? sysctrl isn't really too long, might as well use that.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 26
Quoted 9 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +9
-0+ +Optional properties: +- smp_reg : offset in sysctrl for notifying slave cpu booting + cpu 1, reg; + cpu 2, reg + 0x4; + cpu 3, reg + 0x8; + If reg value is not zero, cpun exit wfi and go +- resume_reg : offset in sysctrl for notifying cpu0 when resume +- reset_reg : offset in sysctrl for system resetUnderscores are usually not used in properties, you use dashes instead.
Quoted 2 lines
Anyway, I suggest defining those as the different cells in a regular 'reg' property instead.
Will update with
Required properties:
- compatible : "hisilicon,sysctrl"
- reg : Register address and size
Optional properties:
- smp-off : offset in sysctrl for notifying slave cpu booting
cpu 1, reg;
cpu 2, reg + 0x4;
cpu 3, reg + 0x8;
If reg value is not zero, cpun exit wfi and go- resume-off : offset in sysctrl for notifying cpu0 when resume
- reboot-off : offset in sysctrl for system reboot
Quoted 10 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 30
Quoted 5 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +4
-0+ +Example: + hi3716: + sctrl at f8000000 {Labels are usually not done on a separate line before.
Common node names would be more clear-text than this. Maybe "system-controller" in this case.
will update with
sysctrl: system-controller at f8000000 {
compatible = "hisilicon,sysctrl";
reg = <0xf8000000 0x1000>;
smp-off = <0xc0>;
reboot-off = <0x4>;
};Quoted 17 lines
Quoted 17 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +15
-0+ compatible = "hisilicon,sctrl"; + reg = <0xf8000000 0x1000>; + smp_reg = <0xc0>; + reboot_reg = <0x4>; + }; + + hi3620: + sctrl at fc802000 { + compatible = "hisilicon,sctrl"; + reg = <0xfc802000 0x1000>; + smp_reg = <0x31c>; + resume_reg = <0x308>; + reboot_reg = <0x4>; + }; +
Quoted 8 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 51
Quoted 6 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +6
-0+Hi3716 cpuctrl register +Required properties: +- compatible : "hisilicon,cpuctrl" +- reg : Address and size of cpuctrl. + +Hi3716 specific cpuctrl register, control hotplug info etc.Controlling hotplug _info_? Seems like odd wording here.
Update to
Hi3716 cpuctrl register
Hi3716 specific cpuctrl register, control power & clock of cpu and module.
Quoted 101 lines
Quoted 12 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +6
-0+Example: + hi3716: + cpuctrl at f8a22000 { + compatible = "hisilicon,cpuctrl"; + reg = <0xf8a22000 0x2000>; + };arch/arm/boot/dts/hi3620.dtsi +0
-0arch/arm/boot/dts/hi3620.dtsi line 35
Quoted 4 lines
arch/arm/boot/dts/hi3620.dtsi +1
-0@@ -32,6 +32,24 @@ reg = <0x0>; next-level-cache = <&L2>; }; + cpu1: cpu at 1 {Do you need labels here? I don't think these are referred to as phandles anywhere?
Quoted 49 lines
arch/arm/boot/dts/hi3620.dtsi +26
-1+ compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <1>; + next-level-cache = <&L2>; + }; + cpu2: cpu at 2 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <2>; + next-level-cache = <&L2>; + }; + cpu3: cpu at 3 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + reg = <3>; + next-level-cache = <&L2>; + }; }; osc32k: osc32k {@@ -130,7 +148,9 @@ reg = <0xfc802000 0x1000>; #address-cells = <1>; #size-cells = <0>; - + smp_reg = <0x31c>; + resume_reg = <0x308>; + reboot_reg = <0x4>; timer0_mux: timer0_mux { compatible = "hisilicon,clk-mux";@@ -1242,6 +1262,12 @@ status = "disabled"; }; + timer5: timer at fc000600 { + compatible = "arm,cortex-a9-twd-timer"; + reg = <0xfc000600 0x20>; + interrupts = <1 13 0xf01>; + }; + uart0: uart at fcb00000 { compatible = "arm,pl011", "arm,primecell"; reg = <0xfcb00000 0x1000>;arch/arm/boot/dts/hi4511.dts +0
-0arch/arm/boot/dts/hi4511.dts line 18
Quoted 5 lines
arch/arm/boot/dts/hi4511.dts +1
-1@@ -15,7 +15,7 @@ compatible = "hisilicon,hi3620-hi4511"; chosen { - bootargs = "console=ttyAMA0,115200 root=/dev/ram0 earlyprintk"; + bootargs = "console=ttyAMA0,115200 root=/dev/ram0 nr_cpus=4 earlyprintk";
nr_cpusshould not be needed on the command line.Quoted 24 lines
arch/arm/boot/dts/hi4511.dts +0
-0}; memory {arch/arm/mach-hi3xxx/Kconfig +2
-0@@ -6,6 +6,8 @@ config ARCH_HI3xxx select CACHE_L2X0 select CLKSRC_OF select GENERIC_CLOCKEVENTS + select HAVE_ARM_SCU + select HAVE_SMP select PINCTRL select PINCTRL_SINGLE helparch/arm/mach-hi3xxx/Makefile +0
-0@@ -2,4 +2,5 @@ # Makefile for Hisilicon Hi36xx/Hi37xx processors line #
Quoted 16 lines
arch/arm/mach-hi3xxx/core.h line 4
Quoted 13 lines
arch/arm/mach-hi3xxx/Makefile +2
-1-obj-y += hi3xxx.o +obj-y += hi3xxx.o system.o +obj-$(CONFIG_SMP) += platsmp.oarch/arm/mach-hi3xxx/core.h new file +4
-0@@ -0,0 +1,14 @@ +#ifndef __HISILICON_CORE_H +#define __HISILICON_CORE_H + +#include <linux/init.h> I don't think you need
linux/init.hhere?
Thanks, definitely true.
PATCH v7 8/11 ARM: hi3xxx: add smp support
Quoted 45 lines · zhangfei gao
Thanks Olof, will update with your comments.
Quoted 34 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 13
Quoted 12 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +3
-0@@ -8,3 +8,50 @@ Required root node properties: Hi4511 Board Required root node properties: - compatible = "hisilicon,hi3620-hi4511"; + + +Hisilicon sctrl resiterResiter seems like odd naming here.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 17
Quoted 4 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +4
-0+ +Required properties: +- compatible : "hisilicon,sctrl" +- reg : Address and size of sysctrl.sysctrl or sctrl? sysctrl isn't really too long, might as well use that.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 26
Quoted 9 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +9
-0+ +Optional properties: +- smp_reg : offset in sysctrl for notifying slave cpu booting + cpu 1, reg; + cpu 2, reg + 0x4; + cpu 3, reg + 0x8; + If reg value is not zero, cpun exit wfi and go +- resume_reg : offset in sysctrl for notifying cpu0 when resume +- reset_reg : offset in sysctrl for system resetUnderscores are usually not used in properties, you use dashes instead.
Quoted 2 lines
Anyway, I suggest defining those as the different cells in a regular 'reg' property instead.
Will update with
Required properties:
- compatible : "hisilicon,sysctrl"
- reg : Register address and size
Quoted 8 lines
Optional properties:
- smp-off : offset in sysctrl for notifying slave cpu bootingcpu 1, reg; cpu 2, reg + 0x4; cpu 3, reg + 0x8; If reg value is not zero, cpun exit wfi and go- resume-off : offset in sysctrl for notifying cpu0 when resume
- reboot-off : offset in sysctrl for system reboot
Unabbreviated "offset" would be better than "off" here.
Do you even need this in the device tree? Do you have different implementations of this IP that has the same programming model but just with a couple of different offsets?
-Olof
PATCH v7 8/11 ARM: hi3xxx: add smp support
Quoted 55 lines · Olof Johansson
Quoted 45 lines · zhangfei gao
Thanks Olof, will update with your comments.
Quoted 34 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 13
Quoted 12 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +3
-0@@ -8,3 +8,50 @@ Required root node properties: Hi4511 Board Required root node properties: - compatible = "hisilicon,hi3620-hi4511"; + + +Hisilicon sctrl resiterResiter seems like odd naming here.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 17
Quoted 4 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +4
-0+ +Required properties: +- compatible : "hisilicon,sctrl" +- reg : Address and size of sysctrl.sysctrl or sctrl? sysctrl isn't really too long, might as well use that.
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt Required root node properties: · line 26
Quoted 9 lines
Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +9
-0+ +Optional properties: +- smp_reg : offset in sysctrl for notifying slave cpu booting + cpu 1, reg; + cpu 2, reg + 0x4; + cpu 3, reg + 0x8; + If reg value is not zero, cpun exit wfi and go +- resume_reg : offset in sysctrl for notifying cpu0 when resume +- reset_reg : offset in sysctrl for system resetUnderscores are usually not used in properties, you use dashes instead.
Quoted 2 lines
Anyway, I suggest defining those as the different cells in a regular 'reg' property instead.
Will update with
Required properties:
- compatible : "hisilicon,sysctrl"
- reg : Register address and sizeQuoted 8 lines
Optional properties:
- smp-off : offset in sysctrl for notifying slave cpu bootingcpu 1, reg; cpu 2, reg + 0x4; cpu 3, reg + 0x8; If reg value is not zero, cpun exit wfi and go- resume-off : offset in sysctrl for notifying cpu0 when resume
- reboot-off : offset in sysctrl for system reboot
Quoted 1 line
Unabbreviated "offset" would be better than "off" here.
:(
Have updated with off in Haojian's v9, since find such vector in /Document/devicetree
Do I need update to offset?
Will update to offset if there are newer version.
Quoted 4 lines
Do you even need this in the device tree? Do you have different implementations of this IP that has the same programming model but just with a couple of different offsets?
Yes, they are different offsets value in hi3716 and hi3620. Same implementation, only different offset value in different soc.
Thanks
PATCH v7 9/11 ARM: hi3xxx: add hotplug support
In reply to Haojian ZhuangFrom: Zhangfei Gao <[email protected]>
Enable hotplug support on hi3xxx platform
How to test:
cat proc/interrupts
echo 0 > /sys/devices/system/cpu/cpuX/online
cat proc/interrupts
echo 1 > /sys/devices/system/cpu/cpuX/online
--- arch/arm/mach-hi3xxx/Makefile | 1 + arch/arm/mach-hi3xxx/core.h | 5 ++ arch/arm/mach-hi3xxx/hi3xxx.c | 1 + arch/arm/mach-hi3xxx/hotplug.c | 154 +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-hi3xxx/platsmp.c | 5 ++ 5 files changed, 166 insertions(+) create mode 100644 arch/arm/mach-hi3xxx/hotplug.c
arch/arm/mach-hi3xxx/Makefile +1 -0
@@ -4,3 +4,4 @@ obj-y += hi3xxx.o system.o obj-$(CONFIG_SMP) += platsmp.o +obj-$(CONFIG_HOTPLUG_CPU) += hotplug.o
arch/arm/mach-hi3xxx/core.h +5 -0
@@ -11,4 +11,9 @@ extern void hs_map_io(void);extern void hs_restart (enum reboot_mode ,const char *cmd);extern struct smp_operations hs_smp_ops; +extern void __init hs_hotplug_init (void ); +extern void hs_cpu_die (unsigned int cpu); +extern int hs_cpu_kill (unsigned int cpu); +extern void hs_set_cpu (int cpu,bool enable); +#endif
arch/arm/mach-hi3xxx/hi3xxx.c +1 -0
@@ -24,6 +24,7 @@static void __init hi3xxx_timer_init (void ) {hs_map_io (); +hs_hotplug_init ();of_clk_init (NULL );clocksource_of_init (); }
arch/arm/mach-hi3xxx/hotplug.c new file +154 -0
154 lines not on this page · raw message
arch/arm/mach-hi3xxx/platsmp.c +5 -0
@@ -32,6 +32,7 @@ static void __init hs_smp_prepare_cpus(unsigned int max_cpus)static int hs_boot_secondary (unsigned int cpu,struct task_struct *idle) { +hs_set_cpu (cpu, true);hs_set_cpu_jump (cpu, secondary_startup);arch_send_wakeup_ipi_mask (cpumask_of (cpu));return 0 ;
@@ -40,4 +41,8 @@ static int hs_boot_secondary(unsigned int cpu, struct task_struct *idle)struct smp_operations hs_smp_ops__initdata = { .smp_prepare_cpus = hs_smp_prepare_cpus, .smp_boot_secondary = hs_boot_secondary, +#ifdef CONFIG_HOTPLUG_CPU + .cpu_die = hs_cpu_die, + .cpu_kill = hs_cpu_kill, +#endif };
Signature · 1 line
1.8.1.2
PATCH v7 9/11 ARM: hi3xxx: add hotplug support
Hi,
Some comments below.
Quoted 87 lines · Haojian Zhuang
From: Zhangfei Gao <[email protected]>
Enable hotplug support on hi3xxx platform
How to test:
cat proc/interrupts
echo 0 >/sys/devices/system/cpu/cpuX/online
cat proc/interrupts
echo 1 >/sys/devices/system/cpu/cpuX/onlineSigned-off-by Zhangfei Gao <[email protected]>Tested-by Zhang Mingjun <[email protected]>--- arch/arm/mach-hi3xxx/Makefile | 1 + arch/arm/mach-hi3xxx/core.h | 5 ++ arch/arm/mach-hi3xxx/hi3xxx.c | 1 + arch/arm/mach-hi3xxx/hotplug.c | 154 +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-hi3xxx/platsmp.c | 5 ++ 5 files changed, 166 insertions(+) create mode 100644 arch/arm/mach-hi3xxx/hotplug.carch/arm/mach-hi3xxx/Makefile +1
-0@@ -4,3 +4,4 @@ obj-y += hi3xxx.o system.o obj-$(CONFIG_SMP) += platsmp.o +obj-$(CONFIG_HOTPLUG_CPU) += hotplug.oarch/arm/mach-hi3xxx/core.h +5
-0@@ -11,4 +11,9 @@ extern void hs_map_io(void);extern void hs_restart (enum reboot_mode ,const char *cmd);extern struct smp_operations hs_smp_ops; +extern void __init hs_hotplug_init (void ); +extern void hs_cpu_die (unsigned int cpu); +extern int hs_cpu_kill (unsigned int cpu); +extern void hs_set_cpu (int cpu,bool enable); +#endif arch/arm/mach-hi3xxx/hi3xxx.c +1
-0@@ -24,6 +24,7 @@static void __init hi3xxx_timer_init (void ) {hs_map_io (); +hs_hotplug_init ();of_clk_init (NULL );clocksource_of_init (); }arch/arm/mach-hi3xxx/hotplug.c new file +25
-0@@ -0,0 +1,154 @@ +/* +* Copyright (c) 2013 Linaro Ltd. +* Copyright (c) 2013 Hisilicon Limited. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +*/ +#include <linux/cpu.h> +#include <linux/io.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <asm/cacheflush.h> +#include <asm/smp_plat.h> +#include "core.h" + +enum { +HI3620_CTRL , +HI3716_CTRL , +}; + +static void __iomem *ctrl_base; +static int id; + +void hs_set_cpu (int cpu,bool enable)
arch/arm/mach-hi3xxx/hotplug.c line 35
Quoted 10 lines
arch/arm/mach-hi3xxx/hotplug.c new file +10
-0+{ +u32 val =0 ; + +if (!ctrl_base) +return ; + +if (id ==HI3620_CTRL ) { +if (enable) { +/* MTCMOS */ +writel_relaxed ((0x01 << (cpu +3 )), ctrl_base +0xD0 );
0xD0 should be 0xd0 (we usually don't use all-caps hex)
arch/arm/mach-hi3xxx/hotplug.c line 37
Quoted 2 lines
arch/arm/mach-hi3xxx/hotplug.c new file +2
-0+writel_relaxed ((0x1011 << cpu), ctrl_base +0x414 ); +writel_relaxed ((0x401011 << cpu), ctrl_base +0x410 );
You can skip the outermost parens here.
These numbers look pretty magic. Can you make it slightly easier for someone reading the code to figure out what's going on?
arch/arm/mach-hi3xxx/hotplug.c line 40
Quoted 2 lines
arch/arm/mach-hi3xxx/hotplug.c new file +2
-0+/* ISO disable */ +writel ((0x01 << (cpu +3 )), ctrl_base +0x0C4 );
0x0C4 should be 0xc4. Also, it's clearer if you use 0x8 << cpu instead.
arch/arm/mach-hi3xxx/hotplug.c line 44
Quoted 4 lines
arch/arm/mach-hi3xxx/hotplug.c new file +4
-0+ +/* WFI Mask */ + val =readl (ctrl_base +0x200 ); + val &= ~(0x1 << (cpu+28 ));
Same here, don't use cpu + 28, modify the constant instead. Same for the other occurrances below.
Quoted 40 lines
arch/arm/mach-hi3xxx/hotplug.c new file +40
-0+writel (val, ctrl_base +0x200 ); + +/* Enable core */ +writel_relaxed ((0x01 << cpu), ctrl_base +0xf4 ); +/* Unreset */ +writel_relaxed ((0x401011 << cpu), ctrl_base +0x414 ); + }else { +/* iso enable */ +writel_relaxed ((0x01 << (cpu +3 )), ctrl_base +0xC0 ); + +/* MTCMOS */ +writel_relaxed ((0x01 << (cpu +3 )), ctrl_base +0xD4 ); + +/* wfi mask */ + val =readl_relaxed (ctrl_base +0x200 ); + val |= (0x1 << (cpu+28 )); +writel_relaxed (val, ctrl_base +0x200 ); + +/* disable core*/ +writel_relaxed ((0x01 << cpu), ctrl_base +0xf8 ); +/* Reset */ +writel_relaxed ((0x401011 << cpu), ctrl_base +0x410 ); + } + }else if (id ==HI3716_CTRL ) { +if (enable) { +/* power on cpu1 */ + val =readl_relaxed (ctrl_base +0x1000 ); + val &= ~(0x1 <<8 ); + val |= (0x1 <<7 ); + val &= ~(0x1 <<3 ); +writel_relaxed (val, ctrl_base +0x1000 ); + +/* unreset */ + val =readl_relaxed (ctrl_base +0x50 ); + val &= ~(0x1 <<17 ); +writel_relaxed (val, ctrl_base +0x50 ); + }else { +/* power down cpu1 */ + val =readl_relaxed (ctrl_base +0x1000 ); + val &= ~(0x1 <<8 );
arch/arm/mach-hi3xxx/hotplug.c line 94
Quoted 10 lines
arch/arm/mach-hi3xxx/hotplug.c new file +10
-0+ val |= (0x1 <<7 ); + val |= (0x1 <<3 ); +writel_relaxed (val, ctrl_base +0x1000 ); + +/* reset */ + val =readl_relaxed (ctrl_base +0x50 ); + val |= (0x1 <<17 ); +writel_relaxed (val, ctrl_base +0x50 ); + } + }
So the function above really shares no code path between the different SoCs. Instead of doing it all in nested if/else bodies, please split it up in two functions, one for each SoC. You can still keep the enable/disable paths under if/else though.
arch/arm/mach-hi3xxx/hotplug.c line 104
Quoted 10 lines
arch/arm/mach-hi3xxx/hotplug.c new file +10
-0+} + +void __init hs_hotplug_init (void ) +{ +struct device_node *node; + + node =of_find_compatible_node (NULL ,NULL ,"hisilicon,cpuctrl" ); +if (node) { + ctrl_base =of_iomap (node,0 ); + id =HI3716_CTRL ;
You should use something more specific than just hisilicon,cpuctrl here, if it's truly tied to the SoC (i.e. this ID here).
hisilicon,hi3716-cpuctrl seems appropriate. Note that the bindings need to be revised for this.
arch/arm/mach-hi3xxx/hotplug.c line 110
Quoted 6 lines
arch/arm/mach-hi3xxx/hotplug.c new file +6
-0+return ; + } + node =of_find_compatible_node (NULL ,NULL ,"hisilicon,sctrl" ); +if (node) { + ctrl_base =of_iomap (node,0 ); + id =HI3620_CTRL ;
Same here.
arch/arm/mach-hi3xxx/hotplug.c line 123
Quoted 13 lines
arch/arm/mach-hi3xxx/hotplug.c new file +13
-0+return ; + } +} + +static inline void cpu_enter_lowpower (void ) +{ +unsigned int v; + +flush_cache_all (); + asm volatile( +/* +* Turn off coherency and L1 D-cache +*/
Move the comment up above the asm volatile
Quoted 55 lines
arch/arm/mach-hi3xxx/hotplug.c new file +31
-0+" mrc p15, 0, %0, c1, c0, 1\n" +" bic %0, %0, #0x40\n" +" mcr p15, 0, %0, c1, c0, 1\n" +" mrc p15, 0, %0, c1, c0, 0\n" +" bic %0, %0, #0x04\n" +" mcr p15, 0, %0, c1, c0, 0\n" + : "=&r" (v) + : "r" (0) + : "cc"); +} + +void hs_cpu_die(unsigned int cpu) +{ +cpu_enter_lowpower (); +hs_set_cpu_jump (cpu,phys_to_virt (0 )); +cpu_do_idle (); + +/* We should have never returned from idle */ +panic ("cpu %d unexpectedly exit from shutdown\n" , cpu); +} + +int hs_cpu_kill (unsigned int cpu) +{ +unsigned long timeout = jiffies +msecs_to_jiffies (50 ); + +while (hs_get_cpu_jump (cpu)) +if (time_after (jiffies, timeout)) +return 0 ; +hs_set_cpu (cpu, false); +return 1 ; +}arch/arm/mach-hi3xxx/platsmp.c +5
-0@@ -32,6 +32,7 @@ static void __init hs_smp_prepare_cpus(unsigned int max_cpus)static int hs_boot_secondary (unsigned int cpu,struct task_struct *idle) { +hs_set_cpu (cpu, true);hs_set_cpu_jump (cpu, secondary_startup);arch_send_wakeup_ipi_mask (cpumask_of (cpu));return 0 ;@@ -40,4 +41,8 @@ static int hs_boot_secondary(unsigned int cpu, struct task_struct *idle)struct smp_operations hs_smp_ops__initdata = { .smp_prepare_cpus = hs_smp_prepare_cpus, .smp_boot_secondary = hs_boot_secondary, +#ifdef CONFIG_HOTPLUG_CPU + .cpu_die = hs_cpu_die, + .cpu_kill = hs_cpu_kill, +#endif };Signature · 1 line
1.8.1.2
PATCH v7 9/11 ARM: hi3xxx: add hotplug support
Dear Olof
Thanks for the good suggestion.
All the comments make sense, will update in new version.
Quoted 109 lines
arch/arm/mach-hi3xxx/hotplug.c line 35
Quoted 11 lines
arch/arm/mach-hi3xxx/hotplug.c new file +11
-0+void hs_set_cpu (int cpu,bool enable) +{ +u32 val =0 ; + +if (!ctrl_base) +return ; + +if (id ==HI3620_CTRL ) { +if (enable) { +/* MTCMOS */ +writel_relaxed ((0x01 << (cpu +3 )), ctrl_base +0xD0 );
0xD0should be0xd0(we usually don't use all-caps hex)Review
Quoted 2 lines
(quoted diff) +2
-0+ writel_relaxed((0x1011 << cpu), ctrl_base + 0x414); + writel_relaxed((0x401011 << cpu), ctrl_base + 0x410);You can skip the outermost parens here.
These numbers look pretty magic. Can you make it slightly easier for someone reading the code to figure out what's going on?
arch/arm/mach-hi3xxx/hotplug.c line 40
Quoted 2 lines
arch/arm/mach-hi3xxx/hotplug.c new file +2
-0+/* ISO disable */ +writel ((0x01 << (cpu +3 )), ctrl_base +0x0C4 );
0x0C4should be0xc4. Also, it's clearer if you use0x8<< cpu instead.arch/arm/mach-hi3xxx/hotplug.c line 44
Quoted 4 lines
arch/arm/mach-hi3xxx/hotplug.c new file +4
-0+ +/* WFI Mask */ + val =readl (ctrl_base +0x200 ); + val &= ~(0x1 << (cpu+28 ));Same here, don't use cpu + 28, modify the constant instead. Same for the other occurrances below.
Quoted 40 lines
arch/arm/mach-hi3xxx/hotplug.c new file +40
-0+writel (val, ctrl_base +0x200 ); + +/* Enable core */ +writel_relaxed ((0x01 << cpu), ctrl_base +0xf4 ); +/* Unreset */ +writel_relaxed ((0x401011 << cpu), ctrl_base +0x414 ); + }else { +/* iso enable */ +writel_relaxed ((0x01 << (cpu +3 )), ctrl_base +0xC0 ); + +/* MTCMOS */ +writel_relaxed ((0x01 << (cpu +3 )), ctrl_base +0xD4 ); + +/* wfi mask */ + val =readl_relaxed (ctrl_base +0x200 ); + val |= (0x1 << (cpu+28 )); +writel_relaxed (val, ctrl_base +0x200 ); + +/* disable core*/ +writel_relaxed ((0x01 << cpu), ctrl_base +0xf8 ); +/* Reset */ +writel_relaxed ((0x401011 << cpu), ctrl_base +0x410 ); + } + }else if (id ==HI3716_CTRL ) { +if (enable) { +/* power on cpu1 */ + val =readl_relaxed (ctrl_base +0x1000 ); + val &= ~(0x1 <<8 ); + val |= (0x1 <<7 ); + val &= ~(0x1 <<3 ); +writel_relaxed (val, ctrl_base +0x1000 ); + +/* unreset */ + val =readl_relaxed (ctrl_base +0x50 ); + val &= ~(0x1 <<17 ); +writel_relaxed (val, ctrl_base +0x50 ); + }else { +/* power down cpu1 */ + val =readl_relaxed (ctrl_base +0x1000 ); + val &= ~(0x1 <<8 );arch/arm/mach-hi3xxx/hotplug.c line 94
Quoted 10 lines
arch/arm/mach-hi3xxx/hotplug.c new file +10
-0+ val |= (0x1 <<7 ); + val |= (0x1 <<3 ); +writel_relaxed (val, ctrl_base +0x1000 ); + +/* reset */ + val =readl_relaxed (ctrl_base +0x50 ); + val |= (0x1 <<17 ); +writel_relaxed (val, ctrl_base +0x50 ); + } + }So the function above really shares no code path between the different SoCs. Instead of doing it all in nested if/else bodies, please split it up in two functions, one for each SoC. You can still keep the enable/disable paths under if/else though.
arch/arm/mach-hi3xxx/hotplug.c line 104
Quoted 10 lines
arch/arm/mach-hi3xxx/hotplug.c new file +10
-0+} + +void __init hs_hotplug_init (void ) +{ +struct device_node *node; + + node =of_find_compatible_node (NULL ,NULL ,"hisilicon,cpuctrl" ); +if (node) { + ctrl_base =of_iomap (node,0 ); + id =HI3716_CTRL ;You should use something more specific than just hisilicon,cpuctrl here, if it's truly tied to the SoC (i.e. this ID here).
hisilicon,hi3716-cpuctrl seems appropriate. Note that the bindings need to be revised for this.
Quoted 8 lines
arch/arm/mach-hi3xxx/hotplug.c line 110
Quoted 6 lines
arch/arm/mach-hi3xxx/hotplug.c new file +6
-0+return ; + } + node =of_find_compatible_node (NULL ,NULL ,"hisilicon,sctrl" ); +if (node) { + ctrl_base =of_iomap (node,0 ); + id =HI3620_CTRL ;Same here.
Will use "hisilicon,hi3716-cpuctrl", which is specific to hi3716, and we use this as flag to indicate hi3716 soc
Will use "hisilicon,sysctrl", however, it exists on both hi3620 and hi3716. So use as a flag of hi3620 after check hi3716.
Quoted 17 lines
Quoted 4 lines
arch/arm/mach-hi3xxx/hotplug.c new file +3
-0+return ; + } +}arch/arm/mach-hi3xxx/hotplug.c line 123
Quoted 10 lines
arch/arm/mach-hi3xxx/hotplug.c new file +10
-0+ +static inline void cpu_enter_lowpower (void ) +{ +unsigned int v; + +flush_cache_all (); + asm volatile( +/* +* Turn off coherency and L1 D-cache +*/ Move the comment up above the asm volatile
Thanks
PATCH v7 10/11 ARM: hi3xxx: add clk-hi3716
In reply to Haojian ZhuangFrom: Zhangfei Gao <[email protected]>
--- Documentation/devicetree/bindings/clock/hi3716.txt | 121 ++++++++++ drivers/clk/Makefile | 2 +- drivers/clk/clk-hi3716.c | 268 +++++++++++++++++++++ 3 files changed, 390 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/hi3716.txt create mode 100644 drivers/clk/clk-hi3716.c
Documentation/devicetree/bindings/clock/hi3716.txt new file +121 -0
@@ -0,0 +1,121 @@ +Device Tree Clock bindings for hi3716 + +This binding uses the common clock binding[1]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +Clock control register +Required properties: +- compatible : "hisilicon,clkbase" +- reg : Address and size of clkbase. + +Device Clocks + +Device clocks are required to have one or both of the following sets of +properties: + + +Gated device clocks: + +Required properties: +- compatible : "hisilicon,hi3716-clk-gate" +- gate-reg : shall be the register offset from clkbase and enable bit, reset bit +- clock-output-names : shall be reference name + + +Mux device clocks: + +Required properties: +- compatible : "hisilicon,hi3716-clk-mux" +- mux-reg : shall be the register offset from clkbase and mux_shift mux_width +- mux-table : shall be reg value to be choose clocks accordingly +- clock-output-names : shall be reference name + + +Fixed divisor device clocks: + +Required properties: +- compatible : "hisilicon,hi3716-fixed-divider" +- div-table : shall be divisor sequence +- clock-output-names : shall be reference name according to divisor + + +Fixed pll clocks: + +Required properties: +- compatible : "hisilicon,hi3716-fixed-pll" +- clock-frequency : shall be output clock frequence sequence +- clock-output-names : shall be reference name according to clock-frequnce + +For example: + clkbase at f8a22000 { + compatible = "hisilicon,clkbase"; + reg = <0xf8a22000 0x1000>; + }; + + osc24m: osc { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <24000000>; + clock-output-names = "osc24mhz"; + }; + + bpll: bpll { + compatible = "hisilicon,hi3716-fixed-pll"; + #clock-cells = <1>; + clocks = <&osc24m>; + clock-frequency = <1200000000>, + <600000000>, + <300000000>, + <200000000>, + <150000000>; + clock-output-names = "bpll_fout0", + "bpll_fout1", + "bpll_fout2", + "bpll_fout3", + "bpll_fout4"; + }; + + bpll_fout0_div: bpll_fout0_div {/* 1200Mhz */ + compatible = "hisilicon,hi3716-fixed-divider"; + #clock-cells = <1>; + clocks = <&bpll 0>; + div-table = <3 14 25 50>; + clock-output-names = "fout0_400m", "fout0_86m", + "fout0_48m", "fout0_24m"; + }; + + bpll_fout3_div: bpll_fout3_div { + compatible = "hisilicon,hi3716-fixed-divider"; + #clock-cells = <1>; + clocks = <&bpll 3>; + div-table = <2 4 5 8>; + clock-output-names = "fout3_100m", "fout3_50m", + "fout3_40m", "fout3_25m"; + }; + + clk_sfc_mux: clk_sfc_mux { + compatible = "hisilicon,hi3716-clk-mux"; + #clock-cells = <0>; + /* clks: 24M 75M 100M 150M 200M */ + clocks = <&osc24m>, <&bpll_fout2_div>, + <&bpll_fout3_div 0>, <&bpll 4>, <&bpll 3>; + + /* offset mux_shift mux_width */ + mux-reg = <0x5c 8 3>; + /* mux reg value to choose clks */ + mux-table = <0 7 6 4 5>; + + clock-output-names = "sfc_mux"; + }; + + clk_sfc: clk_sfc { + compatible = "hisilicon,hi3716-clk-gate"; + #clock-cells = <0>; + clocks = <&clk_sfc_mux>; + + /* offset, enable, reset */ + gate-reg = <0x5c 0 4>; + + clock-output-names = "sfc"; + };
drivers/clk/Makefile +1 -1
@@ -12,7 +12,7 @@ obj-$(CONFIG_COMMON_CLK) += clk-composite.o # SoCs specific obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o obj-$(CONFIG_ARCH_NOMADIK) += clk-nomadik.o -obj-$(CONFIG_ARCH_HI3xxx) += clk-hi3xxx.o +obj-$(CONFIG_ARCH_HI3xxx) += clk-hi3xxx.o clk-hi3716.o obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o obj-$(CONFIG_ARCH_NSPIRE) += clk-nspire.o obj-$(CONFIG_ARCH_MXS) += mxs/
drivers/clk/clk-hi3716.c new file +268 -0
268 lines not on this page · raw message
Signature · 1 line
1.8.1.2
PATCH v7 10/11 ARM: hi3xxx: add clk-hi3716
Quoting Haojian Zhuang (2013-08-19 19:31:12)
Quoted 30 lines
From: Zhangfei Gao <[email protected]>
Signed-off-by Zhangfei Gao <[email protected]>Signed-off-by Zhang Mingjun <[email protected]>--- Documentation/devicetree/bindings/clock/hi3716.txt | 121 ++++++++++ drivers/clk/Makefile | 2 +- drivers/clk/clk-hi3716.c | 268 +++++++++++++++++++++ 3 files changed, 390 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/hi3716.txt create mode 100644 drivers/clk/clk-hi3716.cDocumentation/devicetree/bindings/clock/hi3716.txt new file +12
-0@@ -0,0 +1,121 @@ +Device Tree Clock bindings for hi3716 + +This binding uses the common clock binding[1]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +Clock control register +Required properties: +- compatible : "hisilicon,clkbase" +- reg : Address and size of clkbase. + +Device Clocks
Documentation/devicetree/bindings/clock/hi3716.txt line 22
Quoted 10 lines
Documentation/devicetree/bindings/clock/hi3716.txt new file +10
-0+ +Device clocks are required to have one or both of the following sets of +properties: + + +Gated device clocks: + +Required properties: +- compatible : "hisilicon,hi3716-clk-gate" +- gate-reg : shall be the register offset from clkbase and enable bit, reset bit
The 'reg' property is standard and well understood. Best not to try and re-invent that with gate-reg.
Instead put the register address in the 'reg' property and the other bits in separate properties.
The same goes for the *-reg properties below.
<snip>
Quoted 26 lines
Documentation/devicetree/bindings/clock/hi3716.txt new file +26
-0+static int hi3716_clkgate_prepare(struct clk_hw *hw) +{ + struct hi3716_clk *clk = to_clk_hi3716(hw); + unsigned long flags = 0; + u32 reg; + + spin_lock_irqsave(&_lock, flags); + + reg = readl_relaxed(clk->reg); + reg &= ~BIT(clk->reset_bit); + writel_relaxed(reg, clk->reg); + + spin_unlock_irqrestore(&_lock, flags); + + return 0; +} + +static void hi3716_clkgate_unprepare(struct clk_hw *hw) +{ + struct hi3716_clk *clk = to_clk_hi3716(hw); + unsigned long flags = 0; + u32 reg; + + spin_lock_irqsave(&_lock, flags); + + reg = readl_relaxed(clk->reg);
Documentation/devicetree/bindings/clock/hi3716.txt line 58
Quoted 10 lines
Documentation/devicetree/bindings/clock/hi3716.txt new file +10
-0+ reg |= BIT(clk->reset_bit); + writel_relaxed(reg, clk->reg); + + spin_unlock_irqrestore(&_lock, flags); +} + +static struct clk_ops hi3716_clkgate_ops = { + .prepare = hi3716_clkgate_prepare, + .unprepare = hi3716_clkgate_unprepare, +};
Why .prepare & .unprepare instead of .enable & .disable?
Regards,
Mike
PATCH v7 10/11 ARM: hi3xxx: add clk-hi3716
Quoted 46 lines · Mike Turquette
Quoting Haojian Zhuang (2013-08-19 19:31:12)
Quoted 32 lines
From: Zhangfei Gao <[email protected]>
Signed-off-by Zhangfei Gao <[email protected]>Signed-off-by Zhang Mingjun <[email protected]>---
drivers/clk/clk-hi3716.c new file +26
-0+static int hi3716_clkgate_prepare (struct clk_hw *hw) +{ +struct hi3716_clk *clk =to_clk_hi3716 (hw); +unsigned long flags =0 ; +u32 reg; + +spin_lock_irqsave (&_lock, flags); + + reg =readl_relaxed (clk->reg); + reg &= ~BIT (clk->reset_bit); +writel_relaxed (reg, clk->reg); + +spin_unlock_irqrestore (&_lock, flags); + +return 0 ; +} + +static void hi3716_clkgate_unprepare (struct clk_hw *hw) +{ +struct hi3716_clk *clk =to_clk_hi3716 (hw); +unsigned long flags =0 ; +u32 reg; + +spin_lock_irqsave (&_lock, flags); + + reg =readl_relaxed (clk->reg);drivers/clk/clk-hi3716.c line 83
Quoted 10 lines
drivers/clk/clk-hi3716.c new file +10
-0+ reg |=BIT (clk->reset_bit); +writel_relaxed (reg, clk->reg); + +spin_unlock_irqrestore (&_lock, flags); +} + +static struct clk_ops hi3716_clkgate_ops = { + .prepare = hi3716_clkgate_prepare, + .unprepare = hi3716_clkgate_unprepare, +};Why .prepare & .unprepare instead of .enable & .disable?
Quoted 2 lines
Regards,
Mike
Thanks Mike for the review
the .enable & .disable is directly use clk_gate_ops.
+ hi3716_clkgate_ops.enable = clk_gate_ops.enable; + hi3716_clkgate_ops.disable = clk_gate_ops.disable; + hi3716_clkgate_ops.is_enabled = clk_gate_ops.is_enabled; + p_clk->gate.bit_idx = array[1];
prepare & unprepare is handle reset bit, while enable & disable is handle enable bit.
We have to extend since clk_gate_ops does not consider prepare & unprepare, otherwise it would be simpler.
Thanks
PATCH v7 10/11 ARM: hi3xxx: add clk-hi3716
Quoting zhangfei gao (2013-08-21 18:19:33)
Quoted 59 lines · Mike Turquette
Quoted 46 lines
Quoting Haojian Zhuang (2013-08-19 19:31:12)
Quoted 32 lines
From: Zhangfei Gao <[email protected]>
Signed-off-by Zhangfei Gao <[email protected]>Signed-off-by Zhang Mingjun <[email protected]>---
drivers/clk/clk-hi3716.c new file +26
-0+static int hi3716_clkgate_prepare (struct clk_hw *hw) +{ +struct hi3716_clk *clk =to_clk_hi3716 (hw); +unsigned long flags =0 ; +u32 reg; + +spin_lock_irqsave (&_lock, flags); + + reg =readl_relaxed (clk->reg); + reg &= ~BIT (clk->reset_bit); +writel_relaxed (reg, clk->reg); + +spin_unlock_irqrestore (&_lock, flags); + +return 0 ; +} + +static void hi3716_clkgate_unprepare (struct clk_hw *hw) +{ +struct hi3716_clk *clk =to_clk_hi3716 (hw); +unsigned long flags =0 ; +u32 reg; + +spin_lock_irqsave (&_lock, flags); + + reg =readl_relaxed (clk->reg);drivers/clk/clk-hi3716.c line 83
Quoted 10 lines
drivers/clk/clk-hi3716.c new file +10
-0+ reg |=BIT (clk->reset_bit); +writel_relaxed (reg, clk->reg); + +spin_unlock_irqrestore (&_lock, flags); +} + +static struct clk_ops hi3716_clkgate_ops = { + .prepare = hi3716_clkgate_prepare, + .unprepare = hi3716_clkgate_unprepare, +};Why .prepare & .unprepare instead of .enable & .disable?
Quoted 2 lines
Regards,
MikeThanks Mike for the review
the .enable & .disable is directly use
clk_gate_ops.drivers/clk/clk-hi3716.c new file +4
-0+ hi3716_clkgate_ops.enable = clk_gate_ops.enable; + hi3716_clkgate_ops.disable = clk_gate_ops.disable; + hi3716_clkgate_ops.is_enabled = clk_gate_ops.is_enabled; + p_clk->gate.bit_idx = array[1 ];
Quoted 4 lines
prepare & unprepare is handle reset bit, while enable & disable is handle enable bit.
We have to extend sinceclk_gate_opsdoes not consider prepare & unprepare, otherwise it would be simpler.
I understand why you made this choice from the perspective of re-using the existing gate-clock implementation. What I meant in my question is whether or not handling the reset bit in the .prepare/.unprepare is the right thing.
For instance if you called clk_enable or clk_disable from within an interrupt handler would you want to toggle the reset bit in that instance?
Regards,
Mike
Quoted 2 lines
Thanks
PATCH v7 10/11 ARM: hi3xxx: add clk-hi3716
Quoted 65 lines · Mike Turquette
Quoting zhangfei gao (2013-08-21 18:19:33)
Quoted 59 lines · Mike Turquette
Quoted 46 lines
Quoting Haojian Zhuang (2013-08-19 19:31:12)
Quoted 32 lines
From: Zhangfei Gao <[email protected]>
Signed-off-by Zhangfei Gao <[email protected]>Signed-off-by Zhang Mingjun <[email protected]>---
drivers/clk/clk-hi3716.c new file +26
-0+static int hi3716_clkgate_prepare (struct clk_hw *hw) +{ +struct hi3716_clk *clk =to_clk_hi3716 (hw); +unsigned long flags =0 ; +u32 reg; + +spin_lock_irqsave (&_lock, flags); + + reg =readl_relaxed (clk->reg); + reg &= ~BIT (clk->reset_bit); +writel_relaxed (reg, clk->reg); + +spin_unlock_irqrestore (&_lock, flags); + +return 0 ; +} + +static void hi3716_clkgate_unprepare (struct clk_hw *hw) +{ +struct hi3716_clk *clk =to_clk_hi3716 (hw); +unsigned long flags =0 ; +u32 reg; + +spin_lock_irqsave (&_lock, flags); + + reg =readl_relaxed (clk->reg);drivers/clk/clk-hi3716.c line 83
Quoted 10 lines
drivers/clk/clk-hi3716.c new file +10
-0+ reg |=BIT (clk->reset_bit); +writel_relaxed (reg, clk->reg); + +spin_unlock_irqrestore (&_lock, flags); +} + +static struct clk_ops hi3716_clkgate_ops = { + .prepare = hi3716_clkgate_prepare, + .unprepare = hi3716_clkgate_unprepare, +};Why .prepare & .unprepare instead of .enable & .disable?
Quoted 2 lines
Regards,
MikeThanks Mike for the review
the .enable & .disable is directly use
clk_gate_ops.drivers/clk/clk-hi3716.c new file +4
-0+ hi3716_clkgate_ops.enable = clk_gate_ops.enable; + hi3716_clkgate_ops.disable = clk_gate_ops.disable; + hi3716_clkgate_ops.is_enabled = clk_gate_ops.is_enabled; + p_clk->gate.bit_idx = array[1 ];Quoted 4 lines
prepare & unprepare is handle reset bit, while enable & disable is handle enable bit.
We have to extend sinceclk_gate_opsdoes not consider prepare & unprepare, otherwise it would be simpler.
Quoted 9 lines
I understand why you made this choice from the perspective of re-using the existing gate-clock implementation. What I meant in my question is whether or not handling the reset bit in the .prepare/.unprepare is the right thing.
For instance if you called
clk_enableorclk_disablefrom within an interrupt handler would you want to toggle the reset bit in that instance?
No. We don't want to access reset bit for clk_enable() & clk_disable().
We don't know what issue will occur if we always control reset & unreset with enabling/disabling.
Regards
Haojian
PATCH v7 10/11 ARM: hi3xxx: add clk-hi3716
Quoting Haojian Zhuang (2013-08-22 00:50:52)
Quoted 76 lines · Mike Turquette
Quoted 65 lines
Quoting zhangfei gao (2013-08-21 18:19:33)
Quoted 59 lines · Mike Turquette
Quoted 46 lines
Quoting Haojian Zhuang (2013-08-19 19:31:12)
Quoted 32 lines
From: Zhangfei Gao <[email protected]>
Signed-off-by Zhangfei Gao <[email protected]>Signed-off-by Zhang Mingjun <[email protected]>---
drivers/clk/clk-hi3716.c new file +26
-0+static int hi3716_clkgate_prepare (struct clk_hw *hw) +{ +struct hi3716_clk *clk =to_clk_hi3716 (hw); +unsigned long flags =0 ; +u32 reg; + +spin_lock_irqsave (&_lock, flags); + + reg =readl_relaxed (clk->reg); + reg &= ~BIT (clk->reset_bit); +writel_relaxed (reg, clk->reg); + +spin_unlock_irqrestore (&_lock, flags); + +return 0 ; +} + +static void hi3716_clkgate_unprepare (struct clk_hw *hw) +{ +struct hi3716_clk *clk =to_clk_hi3716 (hw); +unsigned long flags =0 ; +u32 reg; + +spin_lock_irqsave (&_lock, flags); + + reg =readl_relaxed (clk->reg);drivers/clk/clk-hi3716.c line 83
Quoted 10 lines
drivers/clk/clk-hi3716.c new file +10
-0+ reg |=BIT (clk->reset_bit); +writel_relaxed (reg, clk->reg); + +spin_unlock_irqrestore (&_lock, flags); +} + +static struct clk_ops hi3716_clkgate_ops = { + .prepare = hi3716_clkgate_prepare, + .unprepare = hi3716_clkgate_unprepare, +};Why .prepare & .unprepare instead of .enable & .disable?
Quoted 2 lines
Regards,
MikeThanks Mike for the review
the .enable & .disable is directly use
clk_gate_ops.drivers/clk/clk-hi3716.c new file +4
-0+ hi3716_clkgate_ops.enable = clk_gate_ops.enable; + hi3716_clkgate_ops.disable = clk_gate_ops.disable; + hi3716_clkgate_ops.is_enabled = clk_gate_ops.is_enabled; + p_clk->gate.bit_idx = array[1 ];Quoted 4 lines
prepare & unprepare is handle reset bit, while enable & disable is handle enable bit.
We have to extend sinceclk_gate_opsdoes not consider prepare & unprepare, otherwise it would be simpler.Quoted 9 lines
I understand why you made this choice from the perspective of re-using the existing gate-clock implementation. What I meant in my question is whether or not handling the reset bit in the .prepare/.unprepare is the right thing.
For instance if you called
clk_enableorclk_disablefrom within an interrupt handler would you want to toggle the reset bit in that instance?
Quoted 4 lines
No. We don't want to access reset bit for
clk_enable()&clk_disable().We don't know what issue will occur if we always control reset & unreset with enabling/disabling.
Ok then your use of .prepare & .unprepare sounds correct to me.
Thanks,
Mike
Quoted 3 lines
Regards
Haojian
PATCH v7 11/11 ARM: hi3xxx: support hi3716-dkb board
In reply to Haojian ZhuangFrom: Zhangfei Gao <[email protected]>
--- arch/arm/boot/dts/hi3716-dkb.dts | 37 ++++++ arch/arm/boot/dts/hi3716.dtsi | 275 +++++++++++++++++++++++++++++++++++++++ arch/arm/mach-hi3xxx/hi3xxx.c | 1 + 3 files changed, 313 insertions(+) create mode 100644 arch/arm/boot/dts/hi3716-dkb.dts create mode 100644 arch/arm/boot/dts/hi3716.dtsi
arch/arm/boot/dts/hi3716-dkb.dts new file +37 -0
@@ -0,0 +1,37 @@ +/* + * Copyright (c) 2013 Linaro Ltd. + * Copyright (c) 2013 Hisilicon Limited. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * publishhed by the Free Software Foundation. + */ + +/dts-v1/; +/include/ "hi3716.dtsi" + +/ { + model = "Hisilicon Hi3716 Development Board"; + compatible = "hisilicon,hi3716"; + + chosen { + bootargs = "console=ttyAMA0,115200"; + }; + + memory { + device_type = "memory"; + reg = <0x00000000 0x80000000>; + }; + + soc { + amba { + timer0: timer at f8002000 { + status = "okay"; + }; + + uart0: uart at f8b00000 { + status = "okay"; + }; + }; + }; +};
arch/arm/boot/dts/hi3716.dtsi new file +275 -0
275 lines not on this page · raw message
arch/arm/mach-hi3xxx/hi3xxx.c +1 -0
@@ -31,6 +31,7 @@ static void __init hi3xxx_timer_init(void)static const char *hs_compat[]__initdata = {"hisilicon,hi3620-hi4511" , +"hisilicon,hi3716" ,NULL , };
Signature · 1 line
1.8.1.2