2013-03-19 14:27:32

by Roger Quadros

[permalink] [raw]
Subject: [RFC][PATCH 0/2] Device tree support for OMAP4 SCRM clocks

Hi,

Based on the discussion in [1], I've implemented device tree
provider for the AUXCLKs on OMAP4.

Please let me know if there are any issues.

This is important to get USB Host support working on Panda with
device tree boot on 3.10.

Roger Quadros (2):
ARM: OMAP4: clock: Add device tree support for AUXCLKs
ARM: dts: omap4-panda: Provide PHY clock information

.../devicetree/bindings/clock/omap4-clock.txt | 32 ++++++++++++++++++
arch/arm/boot/dts/omap4-panda.dts | 2 +
arch/arm/boot/dts/omap4.dtsi | 5 +++
arch/arm/mach-omap2/board-generic.c | 2 +-
arch/arm/mach-omap2/cclock44xx_data.c | 35 ++++++++++++++++++++
arch/arm/mach-omap2/clock44xx.h | 1 +
arch/arm/mach-omap2/common.h | 9 +++++
arch/arm/mach-omap2/io.c | 6 +++
8 files changed, 91 insertions(+), 1 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt

--
1.7.4.1


2013-03-19 14:27:33

by Roger Quadros

[permalink] [raw]
Subject: [PATCH 2/2] ARM: dts: omap4-panda: Provide PHY clock information

The USB PHY needs AUXCLK3 to operate. Provide this information
in the device tree.

Signed-off-by: Roger Quadros <[email protected]>
---
arch/arm/boot/dts/omap4-panda.dts | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/omap4-panda.dts b/arch/arm/boot/dts/omap4-panda.dts
index cfc7683..2cd369b 100644
--- a/arch/arm/boot/dts/omap4-panda.dts
+++ b/arch/arm/boot/dts/omap4-panda.dts
@@ -85,6 +85,8 @@
compatible = "usb-nop-xceiv";
reset-supply = <&hsusb1_reset>;
vcc-supply = <&hsusb1_power>;
+ clocks = <&aux_clks 3>;
+ clock-names = "main_clk";
clock-frequency = <19200000>;
};
};
--
1.7.4.1

2013-03-19 14:28:43

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 0/2] Device tree support for OMAP4 SCRM clocks

On 03/19/2013 04:26 PM, Roger Quadros wrote:
> Hi,
>
> Based on the discussion in [1], I've implemented device tree
> provider for the AUXCLKs on OMAP4.

[1] - https://lkml.org/lkml/2013/3/12/241

cheers,
-roger

2013-03-19 14:27:31

by Roger Quadros

[permalink] [raw]
Subject: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Register a device tree clock provider for AUX clocks
on the OMAP4 SoC. Also provide the binding information.

Signed-off-by: Roger Quadros <[email protected]>
---
.../devicetree/bindings/clock/omap4-clock.txt | 32 ++++++++++++++++++
arch/arm/boot/dts/omap4.dtsi | 5 +++
arch/arm/mach-omap2/board-generic.c | 2 +-
arch/arm/mach-omap2/cclock44xx_data.c | 35 ++++++++++++++++++++
arch/arm/mach-omap2/clock44xx.h | 1 +
arch/arm/mach-omap2/common.h | 9 +++++
arch/arm/mach-omap2/io.c | 6 +++
7 files changed, 89 insertions(+), 1 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/omap4-clock.txt b/Documentation/devicetree/bindings/clock/omap4-clock.txt
new file mode 100644
index 0000000..9d5448b
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/omap4-clock.txt
@@ -0,0 +1,32 @@
+* Clock bindings for Texas Instruments OMAP4 SCRM clocks
+
+Required properties:
+- compatible: Should be "ti,omap4-scrm"
+- #clock-cells: Should be <1>
+
+The clock consumer should specify the desired clock by having the clock
+ID in its "clocks" phandle cell. The following is a full list of SCRM
+clocks and IDs.
+
+ Clock ID
+ ------------------
+ auxclk0_ck 0
+ auxclk1_ck 1
+ auxclk1_ck 1
+ auxclk1_ck 1
+ auxclk1_ck 1
+
+Example:
+
+aux_clks: scrmclks {
+ compatible = "ti,omap4-scrm";
+ #clock-cells = <1>;
+};
+
+hsusb1_phy: hsusb1_phy {
+ compatible = "usb-nop-xceiv";
+ reset-supply = <&hsusb1_reset>;
+ clocks = <&aux_clks 3>;
+ clock-names = "main_clk";
+ clock-frequency = <19200000>;
+};
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index b7db1a2..97de56c 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -101,6 +101,11 @@
ti,hwmods = "counter_32k";
};

+ aux_clks: scrmclks {
+ compatible = "ti,omap4-scrm";
+ #clock-cells = <1>;
+ };
+
omap4_pmx_core: [email protected] {
compatible = "ti,omap4-padconf", "pinctrl-single";
reg = <0x4a100040 0x0196>;
diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
index 0274ff7..23f2064 100644
--- a/arch/arm/mach-omap2/board-generic.c
+++ b/arch/arm/mach-omap2/board-generic.c
@@ -158,7 +158,7 @@ DT_MACHINE_START(OMAP4_DT, "Generic OMAP4 (Flattened Device Tree)")
.init_irq = omap_gic_of_init,
.init_machine = omap_generic_init,
.init_late = omap4430_init_late,
- .init_time = omap4_local_timer_init,
+ .init_time = omap4_init_time,
.dt_compat = omap4_boards_compat,
.restart = omap44xx_restart,
MACHINE_END
diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
index 3d58f33..bfc46c1 100644
--- a/arch/arm/mach-omap2/cclock44xx_data.c
+++ b/arch/arm/mach-omap2/cclock44xx_data.c
@@ -27,6 +27,7 @@
#include <linux/clk-private.h>
#include <linux/clkdev.h>
#include <linux/io.h>
+#include <linux/of.h>

#include "soc.h"
#include "iomap.h"
@@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
};

+static struct clk *scrm_clks[] = {
+ &auxclk0_ck,
+ &auxclk1_ck,
+ &auxclk2_ck,
+ &auxclk3_ck,
+ &auxclk4_ck,
+ &auxclk5_ck,
+};
+
+static struct clk_onecell_data scrm_data;
+
+#ifdef CONFIG_OF
+int __init omap4_clk_init_dt(void)
+{
+ struct device_node *np;
+
+ np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
+ if (np) {
+ scrm_data.clks = scrm_clks;
+ scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
+ of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
+ }
+
+ return 0;
+}
+
+#else
+
+int __init omap4_clk_init_dt(void)
+{
+ return 0;
+}
+#endif /* CONFIG_OF */
+
int __init omap4xxx_clk_init(void)
{
u32 cpu_clkflg;
diff --git a/arch/arm/mach-omap2/clock44xx.h b/arch/arm/mach-omap2/clock44xx.h
index 287a46f..6395f63 100644
--- a/arch/arm/mach-omap2/clock44xx.h
+++ b/arch/arm/mach-omap2/clock44xx.h
@@ -16,5 +16,6 @@
#define OMAP4430_REGM4XEN_MULT 4

int omap4xxx_clk_init(void);
+int omap4_clk_init_dt(void);

#endif
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index 0a6b9c7..1941d1c 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -98,6 +98,7 @@ void am35xx_init_early(void);
void ti81xx_init_early(void);
void am33xx_init_early(void);
void omap4430_init_early(void);
+void omap4_init_time(void);
void omap5_init_early(void);
void omap3_init_late(void); /* Do not use this one */
void omap4430_init_late(void);
@@ -143,6 +144,14 @@ static inline void omap44xx_restart(char mode, const char *cmd)
}
#endif

+#ifdef CONFIG_ARCH_OMAP4
+void omap4_init_time(void);
+#else
+static inline void omap4_init_time(void)
+{
+}
+#endif
+
/* This gets called from mach-omap2/io.c, do not call this */
void __init omap2_set_globals_tap(u32 class, void __iomem *tap);

diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 2c3fdd6..c504363 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -603,6 +603,12 @@ void __init omap4430_init_late(void)
omap4_pm_init();
omap2_clk_enable_autoidle_all();
}
+
+void __init omap4_init_time(void)
+{
+ omap4_clk_init_dt();
+ omap4_local_timer_init();
+}
#endif

#ifdef CONFIG_SOC_OMAP5
--
1.7.4.1

2013-03-19 14:30:42

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 03/19/2013 04:26 PM, Roger Quadros wrote:
> Register a device tree clock provider for AUX clocks
> on the OMAP4 SoC. Also provide the binding information.
>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> .../devicetree/bindings/clock/omap4-clock.txt | 32 ++++++++++++++++++
> arch/arm/boot/dts/omap4.dtsi | 5 +++
> arch/arm/mach-omap2/board-generic.c | 2 +-
> arch/arm/mach-omap2/cclock44xx_data.c | 35 ++++++++++++++++++++
> arch/arm/mach-omap2/clock44xx.h | 1 +
> arch/arm/mach-omap2/common.h | 9 +++++
> arch/arm/mach-omap2/io.c | 6 +++
> 7 files changed, 89 insertions(+), 1 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/omap4-clock.txt b/Documentation/devicetree/bindings/clock/omap4-clock.txt
> new file mode 100644
> index 0000000..9d5448b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/omap4-clock.txt
> @@ -0,0 +1,32 @@
> +* Clock bindings for Texas Instruments OMAP4 SCRM clocks
> +
> +Required properties:
> +- compatible: Should be "ti,omap4-scrm"
> +- #clock-cells: Should be <1>
> +
> +The clock consumer should specify the desired clock by having the clock
> +ID in its "clocks" phandle cell. The following is a full list of SCRM
> +clocks and IDs.
> +
> + Clock ID
> + ------------------
> + auxclk0_ck 0
> + auxclk1_ck 1
> + auxclk1_ck 1
> + auxclk1_ck 1
> + auxclk1_ck 1

Argh! should be 2,3,4,5

> +
> +Example:
> +
> +aux_clks: scrmclks {
> + compatible = "ti,omap4-scrm";
> + #clock-cells = <1>;
> +};
> +
> +hsusb1_phy: hsusb1_phy {
> + compatible = "usb-nop-xceiv";
> + reset-supply = <&hsusb1_reset>;
> + clocks = <&aux_clks 3>;
> + clock-names = "main_clk";
> + clock-frequency = <19200000>;
> +};
> diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
> index b7db1a2..97de56c 100644
> --- a/arch/arm/boot/dts/omap4.dtsi
> +++ b/arch/arm/boot/dts/omap4.dtsi
> @@ -101,6 +101,11 @@
> ti,hwmods = "counter_32k";
> };
>
> + aux_clks: scrmclks {
> + compatible = "ti,omap4-scrm";
> + #clock-cells = <1>;
> + };
> +
> omap4_pmx_core: [email protected] {
> compatible = "ti,omap4-padconf", "pinctrl-single";
> reg = <0x4a100040 0x0196>;
> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
> index 0274ff7..23f2064 100644
> --- a/arch/arm/mach-omap2/board-generic.c
> +++ b/arch/arm/mach-omap2/board-generic.c
> @@ -158,7 +158,7 @@ DT_MACHINE_START(OMAP4_DT, "Generic OMAP4 (Flattened Device Tree)")
> .init_irq = omap_gic_of_init,
> .init_machine = omap_generic_init,
> .init_late = omap4430_init_late,
> - .init_time = omap4_local_timer_init,
> + .init_time = omap4_init_time,
> .dt_compat = omap4_boards_compat,
> .restart = omap44xx_restart,
> MACHINE_END
> diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
> index 3d58f33..bfc46c1 100644
> --- a/arch/arm/mach-omap2/cclock44xx_data.c
> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
> @@ -27,6 +27,7 @@
> #include <linux/clk-private.h>
> #include <linux/clkdev.h>
> #include <linux/io.h>
> +#include <linux/of.h>
>
> #include "soc.h"
> #include "iomap.h"
> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
> };
>
> +static struct clk *scrm_clks[] = {
> + &auxclk0_ck,
> + &auxclk1_ck,
> + &auxclk2_ck,
> + &auxclk3_ck,
> + &auxclk4_ck,
> + &auxclk5_ck,
> +};
> +
> +static struct clk_onecell_data scrm_data;
> +
> +#ifdef CONFIG_OF
> +int __init omap4_clk_init_dt(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
> + if (np) {
> + scrm_data.clks = scrm_clks;
> + scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
> + of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
> + }
> +
> + return 0;
> +}
> +
> +#else
> +
> +int __init omap4_clk_init_dt(void)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_OF */
> +
> int __init omap4xxx_clk_init(void)
> {
> u32 cpu_clkflg;
> diff --git a/arch/arm/mach-omap2/clock44xx.h b/arch/arm/mach-omap2/clock44xx.h
> index 287a46f..6395f63 100644
> --- a/arch/arm/mach-omap2/clock44xx.h
> +++ b/arch/arm/mach-omap2/clock44xx.h
> @@ -16,5 +16,6 @@
> #define OMAP4430_REGM4XEN_MULT 4
>
> int omap4xxx_clk_init(void);
> +int omap4_clk_init_dt(void);
>
> #endif
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> index 0a6b9c7..1941d1c 100644
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -98,6 +98,7 @@ void am35xx_init_early(void);
> void ti81xx_init_early(void);
> void am33xx_init_early(void);
> void omap4430_init_early(void);
> +void omap4_init_time(void);
> void omap5_init_early(void);
> void omap3_init_late(void); /* Do not use this one */
> void omap4430_init_late(void);
> @@ -143,6 +144,14 @@ static inline void omap44xx_restart(char mode, const char *cmd)
> }
> #endif
>
> +#ifdef CONFIG_ARCH_OMAP4
> +void omap4_init_time(void);
> +#else
> +static inline void omap4_init_time(void)
> +{
> +}
> +#endif
> +
> /* This gets called from mach-omap2/io.c, do not call this */
> void __init omap2_set_globals_tap(u32 class, void __iomem *tap);
>
> diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
> index 2c3fdd6..c504363 100644
> --- a/arch/arm/mach-omap2/io.c
> +++ b/arch/arm/mach-omap2/io.c
> @@ -603,6 +603,12 @@ void __init omap4430_init_late(void)
> omap4_pm_init();
> omap2_clk_enable_autoidle_all();
> }
> +
> +void __init omap4_init_time(void)
> +{
> + omap4_clk_init_dt();
> + omap4_local_timer_init();
> +}
> #endif
>
> #ifdef CONFIG_SOC_OMAP5
>

2013-03-21 13:08:57

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

[]..

> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
> index 0274ff7..23f2064 100644
> --- a/arch/arm/mach-omap2/board-generic.c
> +++ b/arch/arm/mach-omap2/board-generic.c
> @@ -158,7 +158,7 @@ DT_MACHINE_START(OMAP4_DT, "Generic OMAP4 (Flattened Device Tree)")
> .init_irq = omap_gic_of_init,
> .init_machine = omap_generic_init,
> .init_late = omap4430_init_late,
> - .init_time = omap4_local_timer_init,
> + .init_time = omap4_init_time,
> .dt_compat = omap4_boards_compat,
> .restart = omap44xx_restart,
> MACHINE_END

[]..
> +#ifdef CONFIG_OF
> +int __init omap4_clk_init_dt(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
> + if (np) {
> + scrm_data.clks = scrm_clks;
> + scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
> + of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
> + }
> +
> + return 0;
> +}

[]..
> +
> +void __init omap4_init_time(void)
> +{
> + omap4_clk_init_dt();
> + omap4_local_timer_init();
> +}

I guess you did all this because of_clk_add_provider() needs
slab to be initialized. With the below patch[1], now clk inits
happen within .init_timer already, so none of this would
be needed.

[1] http://www.spinics.net/lists/arm-kernel/msg231288.html

2013-03-21 13:54:24

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 03/21/2013 03:08 PM, Rajendra Nayak wrote:
> []..
>
>> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
>> index 0274ff7..23f2064 100644
>> --- a/arch/arm/mach-omap2/board-generic.c
>> +++ b/arch/arm/mach-omap2/board-generic.c
>> @@ -158,7 +158,7 @@ DT_MACHINE_START(OMAP4_DT, "Generic OMAP4 (Flattened Device Tree)")
>> .init_irq = omap_gic_of_init,
>> .init_machine = omap_generic_init,
>> .init_late = omap4430_init_late,
>> - .init_time = omap4_local_timer_init,
>> + .init_time = omap4_init_time,
>> .dt_compat = omap4_boards_compat,
>> .restart = omap44xx_restart,
>> MACHINE_END
>
> []..
>> +#ifdef CONFIG_OF
>> +int __init omap4_clk_init_dt(void)
>> +{
>> + struct device_node *np;
>> +
>> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
>> + if (np) {
>> + scrm_data.clks = scrm_clks;
>> + scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
>> + of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
>> + }
>> +
>> + return 0;
>> +}
>
> []..
>> +
>> +void __init omap4_init_time(void)
>> +{
>> + omap4_clk_init_dt();
>> + omap4_local_timer_init();
>> +}
>
> I guess you did all this because of_clk_add_provider() needs
> slab to be initialized. With the below patch[1], now clk inits
> happen within .init_timer already, so none of this would
> be needed.
>
> [1] http://www.spinics.net/lists/arm-kernel/msg231288.html
>

Right. I can then call omap4_clk_init_dt() from within omap4xxx_clk_init().

Any comments about the main subject? Does the approach look fine?

cheers,
-roger

2013-03-21 14:05:22

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On Thursday 21 March 2013 07:24 PM, Roger Quadros wrote:
> On 03/21/2013 03:08 PM, Rajendra Nayak wrote:
>> []..
>>
>>> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
>>> index 0274ff7..23f2064 100644
>>> --- a/arch/arm/mach-omap2/board-generic.c
>>> +++ b/arch/arm/mach-omap2/board-generic.c
>>> @@ -158,7 +158,7 @@ DT_MACHINE_START(OMAP4_DT, "Generic OMAP4 (Flattened Device Tree)")
>>> .init_irq = omap_gic_of_init,
>>> .init_machine = omap_generic_init,
>>> .init_late = omap4430_init_late,
>>> - .init_time = omap4_local_timer_init,
>>> + .init_time = omap4_init_time,
>>> .dt_compat = omap4_boards_compat,
>>> .restart = omap44xx_restart,
>>> MACHINE_END
>>
>> []..
>>> +#ifdef CONFIG_OF
>>> +int __init omap4_clk_init_dt(void)
>>> +{
>>> + struct device_node *np;
>>> +
>>> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
>>> + if (np) {
>>> + scrm_data.clks = scrm_clks;
>>> + scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
>>> + of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> []..
>>> +
>>> +void __init omap4_init_time(void)
>>> +{
>>> + omap4_clk_init_dt();
>>> + omap4_local_timer_init();
>>> +}
>>
>> I guess you did all this because of_clk_add_provider() needs
>> slab to be initialized. With the below patch[1], now clk inits
>> happen within .init_timer already, so none of this would
>> be needed.
>>
>> [1] http://www.spinics.net/lists/arm-kernel/msg231288.html
>>
>
> Right. I can then call omap4_clk_init_dt() from within omap4xxx_clk_init().
>
> Any comments about the main subject? Does the approach look fine?

It looks fine, except for the fact that I was wondering if the clock
provider needs to restrict itself to SCRM.
Nishant Menon brought up a need for specifying the mpu clock source
from within DT, to be able to use a generic cpufreq driver.
It could be a provider (not specific to scrm, but having only scrm
clocks for now) which we could add clocks as and when we see a need for
them to be specified from DT.

Btw, you need to copy Paul Walmsley for any clock related patches as
he is the OMAP clock maintainer.

>
> cheers,
> -roger
>

2013-03-21 14:07:28

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

+Paul & Nishant

On 03/19/2013 04:26 PM, Roger Quadros wrote:
> Register a device tree clock provider for AUX clocks
> on the OMAP4 SoC. Also provide the binding information.
>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> .../devicetree/bindings/clock/omap4-clock.txt | 32 ++++++++++++++++++
> arch/arm/boot/dts/omap4.dtsi | 5 +++
> arch/arm/mach-omap2/board-generic.c | 2 +-
> arch/arm/mach-omap2/cclock44xx_data.c | 35 ++++++++++++++++++++
> arch/arm/mach-omap2/clock44xx.h | 1 +
> arch/arm/mach-omap2/common.h | 9 +++++
> arch/arm/mach-omap2/io.c | 6 +++
> 7 files changed, 89 insertions(+), 1 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/omap4-clock.txt b/Documentation/devicetree/bindings/clock/omap4-clock.txt
> new file mode 100644
> index 0000000..9d5448b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/omap4-clock.txt
> @@ -0,0 +1,32 @@
> +* Clock bindings for Texas Instruments OMAP4 SCRM clocks
> +
> +Required properties:
> +- compatible: Should be "ti,omap4-scrm"
> +- #clock-cells: Should be <1>
> +
> +The clock consumer should specify the desired clock by having the clock
> +ID in its "clocks" phandle cell. The following is a full list of SCRM
> +clocks and IDs.
> +
> + Clock ID
> + ------------------
> + auxclk0_ck 0
> + auxclk1_ck 1
> + auxclk1_ck 1
> + auxclk1_ck 1
> + auxclk1_ck 1
> +
> +Example:
> +
> +aux_clks: scrmclks {
> + compatible = "ti,omap4-scrm";
> + #clock-cells = <1>;
> +};
> +
> +hsusb1_phy: hsusb1_phy {
> + compatible = "usb-nop-xceiv";
> + reset-supply = <&hsusb1_reset>;
> + clocks = <&aux_clks 3>;
> + clock-names = "main_clk";
> + clock-frequency = <19200000>;
> +};
> diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
> index b7db1a2..97de56c 100644
> --- a/arch/arm/boot/dts/omap4.dtsi
> +++ b/arch/arm/boot/dts/omap4.dtsi
> @@ -101,6 +101,11 @@
> ti,hwmods = "counter_32k";
> };
>
> + aux_clks: scrmclks {
> + compatible = "ti,omap4-scrm";
> + #clock-cells = <1>;
> + };
> +
> omap4_pmx_core: [email protected] {
> compatible = "ti,omap4-padconf", "pinctrl-single";
> reg = <0x4a100040 0x0196>;
> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
> index 0274ff7..23f2064 100644
> --- a/arch/arm/mach-omap2/board-generic.c
> +++ b/arch/arm/mach-omap2/board-generic.c
> @@ -158,7 +158,7 @@ DT_MACHINE_START(OMAP4_DT, "Generic OMAP4 (Flattened Device Tree)")
> .init_irq = omap_gic_of_init,
> .init_machine = omap_generic_init,
> .init_late = omap4430_init_late,
> - .init_time = omap4_local_timer_init,
> + .init_time = omap4_init_time,
> .dt_compat = omap4_boards_compat,
> .restart = omap44xx_restart,
> MACHINE_END
> diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
> index 3d58f33..bfc46c1 100644
> --- a/arch/arm/mach-omap2/cclock44xx_data.c
> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
> @@ -27,6 +27,7 @@
> #include <linux/clk-private.h>
> #include <linux/clkdev.h>
> #include <linux/io.h>
> +#include <linux/of.h>
>
> #include "soc.h"
> #include "iomap.h"
> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
> };
>
> +static struct clk *scrm_clks[] = {
> + &auxclk0_ck,
> + &auxclk1_ck,
> + &auxclk2_ck,
> + &auxclk3_ck,
> + &auxclk4_ck,
> + &auxclk5_ck,
> +};
> +
> +static struct clk_onecell_data scrm_data;
> +
> +#ifdef CONFIG_OF
> +int __init omap4_clk_init_dt(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
> + if (np) {
> + scrm_data.clks = scrm_clks;
> + scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
> + of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
> + }
> +
> + return 0;
> +}
> +
> +#else
> +
> +int __init omap4_clk_init_dt(void)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_OF */
> +
> int __init omap4xxx_clk_init(void)
> {
> u32 cpu_clkflg;
> diff --git a/arch/arm/mach-omap2/clock44xx.h b/arch/arm/mach-omap2/clock44xx.h
> index 287a46f..6395f63 100644
> --- a/arch/arm/mach-omap2/clock44xx.h
> +++ b/arch/arm/mach-omap2/clock44xx.h
> @@ -16,5 +16,6 @@
> #define OMAP4430_REGM4XEN_MULT 4
>
> int omap4xxx_clk_init(void);
> +int omap4_clk_init_dt(void);
>
> #endif
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> index 0a6b9c7..1941d1c 100644
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -98,6 +98,7 @@ void am35xx_init_early(void);
> void ti81xx_init_early(void);
> void am33xx_init_early(void);
> void omap4430_init_early(void);
> +void omap4_init_time(void);
> void omap5_init_early(void);
> void omap3_init_late(void); /* Do not use this one */
> void omap4430_init_late(void);
> @@ -143,6 +144,14 @@ static inline void omap44xx_restart(char mode, const char *cmd)
> }
> #endif
>
> +#ifdef CONFIG_ARCH_OMAP4
> +void omap4_init_time(void);
> +#else
> +static inline void omap4_init_time(void)
> +{
> +}
> +#endif
> +
> /* This gets called from mach-omap2/io.c, do not call this */
> void __init omap2_set_globals_tap(u32 class, void __iomem *tap);
>
> diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
> index 2c3fdd6..c504363 100644
> --- a/arch/arm/mach-omap2/io.c
> +++ b/arch/arm/mach-omap2/io.c
> @@ -603,6 +603,12 @@ void __init omap4430_init_late(void)
> omap4_pm_init();
> omap2_clk_enable_autoidle_all();
> }
> +
> +void __init omap4_init_time(void)
> +{
> + omap4_clk_init_dt();
> + omap4_local_timer_init();
> +}
> #endif
>
> #ifdef CONFIG_SOC_OMAP5
>

2013-03-21 14:11:50

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 03/21/2013 04:04 PM, Rajendra Nayak wrote:
> On Thursday 21 March 2013 07:24 PM, Roger Quadros wrote:
>> On 03/21/2013 03:08 PM, Rajendra Nayak wrote:
>>> []..
>>>
>>>> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
>>>> index 0274ff7..23f2064 100644
>>>> --- a/arch/arm/mach-omap2/board-generic.c
>>>> +++ b/arch/arm/mach-omap2/board-generic.c
>>>> @@ -158,7 +158,7 @@ DT_MACHINE_START(OMAP4_DT, "Generic OMAP4 (Flattened Device Tree)")
>>>> .init_irq = omap_gic_of_init,
>>>> .init_machine = omap_generic_init,
>>>> .init_late = omap4430_init_late,
>>>> - .init_time = omap4_local_timer_init,
>>>> + .init_time = omap4_init_time,
>>>> .dt_compat = omap4_boards_compat,
>>>> .restart = omap44xx_restart,
>>>> MACHINE_END
>>>
>>> []..
>>>> +#ifdef CONFIG_OF
>>>> +int __init omap4_clk_init_dt(void)
>>>> +{
>>>> + struct device_node *np;
>>>> +
>>>> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
>>>> + if (np) {
>>>> + scrm_data.clks = scrm_clks;
>>>> + scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
>>>> + of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>
>>> []..
>>>> +
>>>> +void __init omap4_init_time(void)
>>>> +{
>>>> + omap4_clk_init_dt();
>>>> + omap4_local_timer_init();
>>>> +}
>>>
>>> I guess you did all this because of_clk_add_provider() needs
>>> slab to be initialized. With the below patch[1], now clk inits
>>> happen within .init_timer already, so none of this would
>>> be needed.
>>>
>>> [1] http://www.spinics.net/lists/arm-kernel/msg231288.html
>>>
>>
>> Right. I can then call omap4_clk_init_dt() from within omap4xxx_clk_init().
>>
>> Any comments about the main subject? Does the approach look fine?
>
> It looks fine, except for the fact that I was wondering if the clock
> provider needs to restrict itself to SCRM.
> Nishant Menon brought up a need for specifying the mpu clock source
> from within DT, to be able to use a generic cpufreq driver.
> It could be a provider (not specific to scrm, but having only scrm
> clocks for now) which we could add clocks as and when we see a need for
> them to be specified from DT.

OK, I will revise the patch to not make it SCRM specific.

>
> Btw, you need to copy Paul Walmsley for any clock related patches as
> he is the OMAP clock maintainer.

OK. Thanks for letting know.

cheers,
-roger

2013-03-26 10:23:40

by Roger Quadros

[permalink] [raw]
Subject: [PATCH v2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Register a device tree clock provider for the clocks
on the OMAP4 SoC. Also provide the binding information.

For now we only provide AUXCLKs.

Signed-off-by: Roger Quadros <[email protected]>
---
.../devicetree/bindings/clock/omap4-clock.txt | 33 ++++++++++++++++
arch/arm/boot/dts/omap4.dtsi | 5 ++
arch/arm/mach-omap2/cclock44xx_data.c | 41 ++++++++++++++++++++
3 files changed, 79 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/omap4-clock.txt b/Documentation/devicetree/bindings/clock/omap4-clock.txt
new file mode 100644
index 0000000..2845a3f
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/omap4-clock.txt
@@ -0,0 +1,33 @@
+* Clock bindings for Texas Instruments OMAP4 clocks
+
+Required properties:
+- compatible: Should be "ti,omap4-clock"
+- #clock-cells: Should be <1>
+
+The clock consumer should specify the desired clock by having the clock
+ID in its "clocks" phandle cell. The following is a list of OMAP4 clocks
+and IDs.
+
+ Clock ID
+ ------------------
+ auxclk0_ck 0
+ auxclk1_ck 1
+ auxclk2_ck 2
+ auxclk3_ck 3
+ auxclk4_ck 4
+ auxclk5_ck 5
+
+Example:
+
+aux_clks: scrmclks {
+ compatible = "ti,omap4-clock";
+ #clock-cells = <1>;
+};
+
+hsusb1_phy: hsusb1_phy {
+ compatible = "usb-nop-xceiv";
+ reset-supply = <&hsusb1_reset>;
+ clocks = <&aux_clks 3>;
+ clock-names = "main_clk";
+ clock-frequency = <19200000>;
+};
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 739bb79..f27548a 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -101,6 +101,11 @@
ti,hwmods = "counter_32k";
};

+ clks: clocks {
+ compatible = "ti,omap4-clock";
+ #clock-cells = <1>;
+ };
+
omap4_pmx_core: [email protected] {
compatible = "ti,omap4-padconf", "pinctrl-single";
reg = <0x4a100040 0x0196>;
diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
index 3d58f33..a93617b 100644
--- a/arch/arm/mach-omap2/cclock44xx_data.c
+++ b/arch/arm/mach-omap2/cclock44xx_data.c
@@ -27,6 +27,7 @@
#include <linux/clk-private.h>
#include <linux/clkdev.h>
#include <linux/io.h>
+#include <linux/of.h>

#include "soc.h"
#include "iomap.h"
@@ -1663,6 +1664,44 @@ static struct omap_clk omap44xx_clks[] = {
CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
};

+/*
+ * List of clocks that can be referenced in device tree
+ * Must match with Documentation/devicetree/bindings/clock/omap4-clock.txt
+ */
+static struct clk *dt_clks[] = {
+ &auxclk0_ck,
+ &auxclk1_ck,
+ &auxclk2_ck,
+ &auxclk3_ck,
+ &auxclk4_ck,
+ &auxclk5_ck,
+};
+
+static struct clk_onecell_data clock_data;
+
+#ifdef CONFIG_OF
+int __init omap4_clk_init_dt(void)
+{
+ struct device_node *np;
+
+ np = of_find_compatible_node(NULL, NULL, "ti,omap4-clock");
+ if (np) {
+ clock_data.clks = dt_clks;
+ clock_data.clk_num = ARRAY_SIZE(dt_clks);
+ of_clk_add_provider(np, of_clk_src_onecell_get, &clock_data);
+ }
+
+ return 0;
+}
+
+#else
+
+int __init omap4_clk_init_dt(void)
+{
+ return 0;
+}
+#endif /* CONFIG_OF */
+
int __init omap4xxx_clk_init(void)
{
u32 cpu_clkflg;
@@ -1693,6 +1732,8 @@ int __init omap4xxx_clk_init(void)

omap2_clk_disable_autoidle_all();

+ omap4_clk_init_dt();
+
/*
* On OMAP4460 the ABE DPLL fails to turn on if in idle low-power
* state when turning the ABE clock domain. Workaround this by
--
1.7.4.1

2013-04-02 08:23:51

by Roger Quadros

[permalink] [raw]
Subject: Re: [PATCH v2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

+ Paul

On 03/26/2013 12:23 PM, Roger Quadros wrote:
> Register a device tree clock provider for the clocks
> on the OMAP4 SoC. Also provide the binding information.
>
> For now we only provide AUXCLKs.
>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> .../devicetree/bindings/clock/omap4-clock.txt | 33 ++++++++++++++++
> arch/arm/boot/dts/omap4.dtsi | 5 ++
> arch/arm/mach-omap2/cclock44xx_data.c | 41 ++++++++++++++++++++
> 3 files changed, 79 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/omap4-clock.txt b/Documentation/devicetree/bindings/clock/omap4-clock.txt
> new file mode 100644
> index 0000000..2845a3f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/omap4-clock.txt
> @@ -0,0 +1,33 @@
> +* Clock bindings for Texas Instruments OMAP4 clocks
> +
> +Required properties:
> +- compatible: Should be "ti,omap4-clock"
> +- #clock-cells: Should be <1>
> +
> +The clock consumer should specify the desired clock by having the clock
> +ID in its "clocks" phandle cell. The following is a list of OMAP4 clocks
> +and IDs.
> +
> + Clock ID
> + ------------------
> + auxclk0_ck 0
> + auxclk1_ck 1
> + auxclk2_ck 2
> + auxclk3_ck 3
> + auxclk4_ck 4
> + auxclk5_ck 5
> +
> +Example:
> +
> +aux_clks: scrmclks {
> + compatible = "ti,omap4-clock";
> + #clock-cells = <1>;
> +};
> +
> +hsusb1_phy: hsusb1_phy {
> + compatible = "usb-nop-xceiv";
> + reset-supply = <&hsusb1_reset>;
> + clocks = <&aux_clks 3>;
> + clock-names = "main_clk";
> + clock-frequency = <19200000>;
> +};
> diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
> index 739bb79..f27548a 100644
> --- a/arch/arm/boot/dts/omap4.dtsi
> +++ b/arch/arm/boot/dts/omap4.dtsi
> @@ -101,6 +101,11 @@
> ti,hwmods = "counter_32k";
> };
>
> + clks: clocks {
> + compatible = "ti,omap4-clock";
> + #clock-cells = <1>;
> + };
> +
> omap4_pmx_core: [email protected] {
> compatible = "ti,omap4-padconf", "pinctrl-single";
> reg = <0x4a100040 0x0196>;
> diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
> index 3d58f33..a93617b 100644
> --- a/arch/arm/mach-omap2/cclock44xx_data.c
> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
> @@ -27,6 +27,7 @@
> #include <linux/clk-private.h>
> #include <linux/clkdev.h>
> #include <linux/io.h>
> +#include <linux/of.h>
>
> #include "soc.h"
> #include "iomap.h"
> @@ -1663,6 +1664,44 @@ static struct omap_clk omap44xx_clks[] = {
> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
> };
>
> +/*
> + * List of clocks that can be referenced in device tree
> + * Must match with Documentation/devicetree/bindings/clock/omap4-clock.txt
> + */
> +static struct clk *dt_clks[] = {
> + &auxclk0_ck,
> + &auxclk1_ck,
> + &auxclk2_ck,
> + &auxclk3_ck,
> + &auxclk4_ck,
> + &auxclk5_ck,
> +};
> +
> +static struct clk_onecell_data clock_data;
> +
> +#ifdef CONFIG_OF
> +int __init omap4_clk_init_dt(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-clock");
> + if (np) {
> + clock_data.clks = dt_clks;
> + clock_data.clk_num = ARRAY_SIZE(dt_clks);
> + of_clk_add_provider(np, of_clk_src_onecell_get, &clock_data);
> + }
> +
> + return 0;
> +}
> +
> +#else
> +
> +int __init omap4_clk_init_dt(void)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_OF */
> +
> int __init omap4xxx_clk_init(void)
> {
> u32 cpu_clkflg;
> @@ -1693,6 +1732,8 @@ int __init omap4xxx_clk_init(void)
>
> omap2_clk_disable_autoidle_all();
>
> + omap4_clk_init_dt();
> +
> /*
> * On OMAP4460 the ABE DPLL fails to turn on if in idle low-power
> * state when turning the ABE clock domain. Workaround this by
>

2013-04-03 23:42:51

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Hi,

* Roger Quadros <[email protected]> [130319 07:31]:
> Register a device tree clock provider for AUX clocks
> on the OMAP4 SoC. Also provide the binding information.
>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> .../devicetree/bindings/clock/omap4-clock.txt | 32 ++++++++++++++++++
> arch/arm/boot/dts/omap4.dtsi | 5 +++
> arch/arm/mach-omap2/board-generic.c | 2 +-
> arch/arm/mach-omap2/cclock44xx_data.c | 35 ++++++++++++++++++++
> arch/arm/mach-omap2/clock44xx.h | 1 +
> arch/arm/mach-omap2/common.h | 9 +++++
> arch/arm/mach-omap2/io.c | 6 +++
> 7 files changed, 89 insertions(+), 1 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt
>
...

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/omap4-clock.txt

Is this really specific to omap4 and auxclk only?

Shouldn't it be just omap-clock.txt?

> --- a/arch/arm/mach-omap2/cclock44xx_data.c
> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
> @@ -27,6 +27,7 @@
> #include <linux/clk-private.h>
> #include <linux/clkdev.h>
> #include <linux/io.h>
> +#include <linux/of.h>
>
> #include "soc.h"
> #include "iomap.h"
> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
> };
>
> +static struct clk *scrm_clks[] = {
> + &auxclk0_ck,
> + &auxclk1_ck,
> + &auxclk2_ck,
> + &auxclk3_ck,
> + &auxclk4_ck,
> + &auxclk5_ck,
> +};

Hmm I don't like the idea of specifying the auxclk both in the
cclock44xx_data.c and in DT..

> +static struct clk_onecell_data scrm_data;
> +
> +#ifdef CONFIG_OF
> +int __init omap4_clk_init_dt(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
> + if (np) {
> + scrm_data.clks = scrm_clks;
> + scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
> + of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
> + }
> +
> + return 0;
> +}
> +
> +#else
> +
> +int __init omap4_clk_init_dt(void)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_OF */
> +
> int __init omap4xxx_clk_init(void)
> {
> u32 cpu_clkflg;

.. and I'm not too keen on adding driver specific stuff to this file.

How about just add a minimal drivers/clk/omap/clk-xyz.c that takes
the configuration from DT and is based on the binding we already have in
Documentation/devicetree/bindings/clock/clock-bindings.txt?

Then as we add new bindings there we can drop them from current
cclock44xx_data.c, no? That is after omap4 is DT only..

Regards,

Tony

2013-04-04 05:20:37

by Rajendra Nayak

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Hi Tony,

On Thursday 04 April 2013 05:12 AM, Tony Lindgren wrote:
> Hi,
>
[]..

>> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
>> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
>> };
>>
>> +static struct clk *scrm_clks[] = {
>> + &auxclk0_ck,
>> + &auxclk1_ck,
>> + &auxclk2_ck,
>> + &auxclk3_ck,
>> + &auxclk4_ck,
>> + &auxclk5_ck,
>> +};
>
> Hmm I don't like the idea of specifying the auxclk both in the
> cclock44xx_data.c and in DT..
>
>> +static struct clk_onecell_data scrm_data;
>> +
>> +#ifdef CONFIG_OF
>> +int __init omap4_clk_init_dt(void)
>> +{
>> + struct device_node *np;
>> +
>> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-scrm");
>> + if (np) {
>> + scrm_data.clks = scrm_clks;
>> + scrm_data.clk_num = ARRAY_SIZE(scrm_clks);
>> + of_clk_add_provider(np, of_clk_src_onecell_get, &scrm_data);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#else
>> +
>> +int __init omap4_clk_init_dt(void)
>> +{
>> + return 0;
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> int __init omap4xxx_clk_init(void)
>> {
>> u32 cpu_clkflg;
>
> .. and I'm not too keen on adding driver specific stuff to this file.
>
> How about just add a minimal drivers/clk/omap/clk-xyz.c that takes
> the configuration from DT and is based on the binding we already have in
> Documentation/devicetree/bindings/clock/clock-bindings.txt?
>
> Then as we add new bindings there we can drop them from current
> cclock44xx_data.c, no? That is after omap4 is DT only..

The patch just provides an alternative for clkdev mapping in case of DT.
Are you suggesting we move all *clock data* related to auxclks (and eventually
all clocks) into DT?
We have discussed this multiple times in the past, and moving 250 clock nodes
with each needing multiple register offsets, masks, shifts etc into DT makes it
completely un-readable. For me, having a way for devices to reference a clock that they
use for a device using DT makes sense, but not moving all clock data into dts files.

regards,
Rajendra

>
> Regards,
>
> Tony
>

2013-04-04 07:35:31

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Hi,

On 04/04/2013 02:42 AM, Tony Lindgren wrote:
> Hi,
>
> * Roger Quadros <[email protected]> [130319 07:31]:
>> Register a device tree clock provider for AUX clocks
>> on the OMAP4 SoC. Also provide the binding information.
>>
>> Signed-off-by: Roger Quadros <[email protected]>
>> ---
>> .../devicetree/bindings/clock/omap4-clock.txt | 32 ++++++++++++++++++
>> arch/arm/boot/dts/omap4.dtsi | 5 +++
>> arch/arm/mach-omap2/board-generic.c | 2 +-
>> arch/arm/mach-omap2/cclock44xx_data.c | 35 ++++++++++++++++++++
>> arch/arm/mach-omap2/clock44xx.h | 1 +
>> arch/arm/mach-omap2/common.h | 9 +++++
>> arch/arm/mach-omap2/io.c | 6 +++
>> 7 files changed, 89 insertions(+), 1 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt
>>
> ...
>
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/omap4-clock.txt
>
> Is this really specific to omap4 and auxclk only?
>
> Shouldn't it be just omap-clock.txt?

Yes, I've fixed this up in v2 of this patch.

>
>> --- a/arch/arm/mach-omap2/cclock44xx_data.c
>> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
>> @@ -27,6 +27,7 @@
>> #include <linux/clk-private.h>
>> #include <linux/clkdev.h>
>> #include <linux/io.h>
>> +#include <linux/of.h>
>>
>> #include "soc.h"
>> #include "iomap.h"
>> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
>> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
>> };
>>
>> +static struct clk *scrm_clks[] = {
>> + &auxclk0_ck,
>> + &auxclk1_ck,
>> + &auxclk2_ck,
>> + &auxclk3_ck,
>> + &auxclk4_ck,
>> + &auxclk5_ck,
>> +};
>
> Hmm I don't like the idea of specifying the auxclk both in the
> cclock44xx_data.c and in DT..

Right, but till we have all clocks moved to DT we only need this
approach for general purpose clocks that are not mapped to devices
by hwmod.

e.g. auxclk are required to be specified in DT nodes for USB PHY.
Without this we can't get USB host working on Panda.

As Rajendra points out, it seems moving entire clock data to DT is not
going to happen soon. So this is the simplistic way things can work.

cheers,
-roger

2013-04-04 16:33:56

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

* Rajendra Nayak <[email protected]> [130403 22:25]:
> On Thursday 04 April 2013 05:12 AM, Tony Lindgren wrote:
> >
> > How about just add a minimal drivers/clk/omap/clk-xyz.c that takes
> > the configuration from DT and is based on the binding we already have in
> > Documentation/devicetree/bindings/clock/clock-bindings.txt?
> >
> > Then as we add new bindings there we can drop them from current
> > cclock44xx_data.c, no? That is after omap4 is DT only..
>
> The patch just provides an alternative for clkdev mapping in case of DT.
> Are you suggesting we move all *clock data* related to auxclks (and eventually
> all clocks) into DT?

Well I think we should have a driver that can take any defined clock binding
from DT at least for boottime clocks. We need at least the timer clocks
early during the boot, and probably the root device like MMC or possibly
Ethernet depending on the board.

The rest of the huge amount of clocks we can just load from /lib/firmware
after mounting intramfs or root on MMC. As long as we can define any
boottime clock in DT also, loading the rest of the clock data from
/lib/firmware should not cause issues with booting.

And as we get the clocks moved, we can drop them from cclock44xx_data.c.
AFAIK the new driver can just clk_get the parent clocks so those can
stay in cclock44xx_data.c for now?

So basically we can do the same as we are already doing with pinctrl-single.c,
but with addition of loading large amounts of data from /lib/firmware.
And eventually we can do the same with hwmod too.

Regarding the readability issue, we now have preprocessing support for
.dts files merged AFAIK, so they can be as readable as data structures :)

> We have discussed this multiple times in the past, and moving 250 clock nodes
> with each needing multiple register offsets, masks, shifts etc into DT makes it
> completely un-readable. For me, having a way for devices to reference a clock that they
> use for a device using DT makes sense, but not moving all clock data into dts files.

Yes that's why we should also support loading clocks from /lib/firmware.
Naturally the parent clocks can be allocated by the clock driver(s) at
least initially.

But the main reason I think we should do this is so we get out of the
flame path with these huge data files for every new SoC.

Regards,

Tony

2013-04-04 16:41:46

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

* Roger Quadros <[email protected]> [130404 00:39]:
> On 04/04/2013 02:42 AM, Tony Lindgren wrote:
> >> --- a/arch/arm/mach-omap2/cclock44xx_data.c
> >> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
> >> @@ -27,6 +27,7 @@
> >> #include <linux/clk-private.h>
> >> #include <linux/clkdev.h>
> >> #include <linux/io.h>
> >> +#include <linux/of.h>
> >>
> >> #include "soc.h"
> >> #include "iomap.h"
> >> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
> >> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
> >> };
> >>
> >> +static struct clk *scrm_clks[] = {
> >> + &auxclk0_ck,
> >> + &auxclk1_ck,
> >> + &auxclk2_ck,
> >> + &auxclk3_ck,
> >> + &auxclk4_ck,
> >> + &auxclk5_ck,
> >> +};
> >
> > Hmm I don't like the idea of specifying the auxclk both in the
> > cclock44xx_data.c and in DT..
>
> Right, but till we have all clocks moved to DT we only need this
> approach for general purpose clocks that are not mapped to devices
> by hwmod.

For v3.10, let's just make sure that USB works with DT as then
after v3.10 we can make omap4 DT only and get rid of estimated
7K lines of code and data. I guess this is the last piece missing
for that, or are we also missing something else?

Can't you set up a clock alias for your device so it can find the
auxclk when requesting it with the dev entry?

For the DT clock driver if needed for v3.10, how about just do a
minimal drivers/clock/omap/ that uses the standard binding?
Then that driver can just do clk_get() from cclock44xx_data.c
for now? And then later on we'll just move all the clocks to a
combination of DT + /lib/firmware.

> e.g. auxclk are required to be specified in DT nodes for USB PHY.
> Without this we can't get USB host working on Panda.

OK. So if the USB PHY has a dev entry, can't you just set up a
clock alias in struct omap_clk omap44xx_clks[] for it?

> As Rajendra points out, it seems moving entire clock data to DT is not
> going to happen soon. So this is the simplistic way things can work.

Right but seems like we can get started there without moving
them all at once.

Regards,

Tony

2013-04-05 08:47:16

by Roger Quadros

[permalink] [raw]
Subject: Re: [PATCH v2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/02/2013 11:23 AM, Roger Quadros wrote:
> + Paul
>
> On 03/26/2013 12:23 PM, Roger Quadros wrote:
>> Register a device tree clock provider for the clocks
>> on the OMAP4 SoC. Also provide the binding information.
>>
>> For now we only provide AUXCLKs.

NOTE: this patch depends on
https://patchwork.kernel.org/patch/2312211/

>>
>> Signed-off-by: Roger Quadros <[email protected]>
>> ---
>> .../devicetree/bindings/clock/omap4-clock.txt | 33 ++++++++++++++++
>> arch/arm/boot/dts/omap4.dtsi | 5 ++
>> arch/arm/mach-omap2/cclock44xx_data.c | 41 ++++++++++++++++++++
>> 3 files changed, 79 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt
>>

cheers,
-roger

2013-04-05 08:48:43

by Nishanth Menon

[permalink] [raw]
Subject: Re: [PATCH v2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 11:47-20130405, Roger Quadros wrote:
> On 04/02/2013 11:23 AM, Roger Quadros wrote:
> > + Paul
> >
> > On 03/26/2013 12:23 PM, Roger Quadros wrote:
> >> Register a device tree clock provider for the clocks
> >> on the OMAP4 SoC. Also provide the binding information.
> >>
> >> For now we only provide AUXCLKs.
>
> NOTE: this patch depends on
> https://patchwork.kernel.org/patch/2312211/
>
> >>
> >> Signed-off-by: Roger Quadros <[email protected]>
> >> ---
> >> .../devicetree/bindings/clock/omap4-clock.txt | 33 ++++++++++++++++
> >> arch/arm/boot/dts/omap4.dtsi | 5 ++
> >> arch/arm/mach-omap2/cclock44xx_data.c | 41 ++++++++++++++++++++
> >> 3 files changed, 79 insertions(+), 0 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt
If we are doing this, we might as well do for all platforms(OMAP3-5,
AM..) in an series, no?

--
Regards,
Nishanth Menon

2013-04-05 08:51:10

by Roger Quadros

[permalink] [raw]
Subject: Re: [PATCH v2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/05/2013 11:48 AM, Nishanth Menon wrote:
> On 11:47-20130405, Roger Quadros wrote:
>> On 04/02/2013 11:23 AM, Roger Quadros wrote:
>>> + Paul
>>>
>>> On 03/26/2013 12:23 PM, Roger Quadros wrote:
>>>> Register a device tree clock provider for the clocks
>>>> on the OMAP4 SoC. Also provide the binding information.
>>>>
>>>> For now we only provide AUXCLKs.
>>
>> NOTE: this patch depends on
>> https://patchwork.kernel.org/patch/2312211/
>>
>>>>
>>>> Signed-off-by: Roger Quadros <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/clock/omap4-clock.txt | 33 ++++++++++++++++
>>>> arch/arm/boot/dts/omap4.dtsi | 5 ++
>>>> arch/arm/mach-omap2/cclock44xx_data.c | 41 ++++++++++++++++++++
>>>> 3 files changed, 79 insertions(+), 0 deletions(-)
>>>> create mode 100644 Documentation/devicetree/bindings/clock/omap4-clock.txt
> If we are doing this, we might as well do for all platforms(OMAP3-5,
> AM..) in an series, no?
>
Yes, once we agree on the approach, I can write it for all OMAPs.

cheers,
-roger

2013-04-05 10:39:56

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/04/2013 07:41 PM, Tony Lindgren wrote:
> * Roger Quadros <[email protected]> [130404 00:39]:
>> On 04/04/2013 02:42 AM, Tony Lindgren wrote:
>>>> --- a/arch/arm/mach-omap2/cclock44xx_data.c
>>>> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
>>>> @@ -27,6 +27,7 @@
>>>> #include <linux/clk-private.h>
>>>> #include <linux/clkdev.h>
>>>> #include <linux/io.h>
>>>> +#include <linux/of.h>
>>>>
>>>> #include "soc.h"
>>>> #include "iomap.h"
>>>> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
>>>> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
>>>> };
>>>>
>>>> +static struct clk *scrm_clks[] = {
>>>> + &auxclk0_ck,
>>>> + &auxclk1_ck,
>>>> + &auxclk2_ck,
>>>> + &auxclk3_ck,
>>>> + &auxclk4_ck,
>>>> + &auxclk5_ck,
>>>> +};
>>>
>>> Hmm I don't like the idea of specifying the auxclk both in the
>>> cclock44xx_data.c and in DT..
>>
>> Right, but till we have all clocks moved to DT we only need this
>> approach for general purpose clocks that are not mapped to devices
>> by hwmod.
>
> For v3.10, let's just make sure that USB works with DT as then
> after v3.10 we can make omap4 DT only and get rid of estimated
> 7K lines of code and data. I guess this is the last piece missing
> for that, or are we also missing something else?

For panda we just need a way to map the auxclk to the USB PHY device
and the relevant dts data to get USB host working with DT.
Beagle USB host should work already with DT without any changes.

>
> Can't you set up a clock alias for your device so it can find the
> auxclk when requesting it with the dev entry?
>

which clock is mapped to which PHY device depends on board design
and that cannot be per-determined at one place. This information
needs to come from the board.dts file.

There was an earlier attempt to provide a way of building clock aliases
at runtime from device tree [1], but the current approach is way better

[1]
https://lkml.org/lkml/2013/3/12/241

> For the DT clock driver if needed for v3.10, how about just do a
> minimal drivers/clock/omap/ that uses the standard binding?
> Then that driver can just do clk_get() from cclock44xx_data.c

I don't understand how to do it and why it is better than the current
approach.

How can that driver do clk_get() from cclock44xx_data.c?
from where does it get the clk_id to pass into clk_get()?

> for now? And then later on we'll just move all the clocks to a
> combination of DT + /lib/firmware.

What is the benefit of moving clock data to /lib/firmware? We could
as well just move it to DT only, no?

>
>> e.g. auxclk are required to be specified in DT nodes for USB PHY.
>> Without this we can't get USB host working on Panda.
>
> OK. So if the USB PHY has a dev entry, can't you just set up a
> clock alias in struct omap_clk omap44xx_clks[] for it?

I've explained why this can't be done above.

>
>> As Rajendra points out, it seems moving entire clock data to DT is not
>> going to happen soon. So this is the simplistic way things can work.
>
> Right but seems like we can get started there without moving
> them all at once.
>
What if we provide a complete clock list instead of only auxclks in
dt_clks[]?

This approach is similar to arch/arm/mach-imx/clk-imx35.c

Device drivers can then use them as they migrate to DT. Then later
we could migrate clock data to DT, without impacting device drivers.

cheers,
-roger

2013-04-05 15:59:00

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

* Roger Quadros <[email protected]> [130405 03:44]:
> On 04/04/2013 07:41 PM, Tony Lindgren wrote:
> > * Roger Quadros <[email protected]> [130404 00:39]:
> >> On 04/04/2013 02:42 AM, Tony Lindgren wrote:
> > For v3.10, let's just make sure that USB works with DT as then
> > after v3.10 we can make omap4 DT only and get rid of estimated
> > 7K lines of code and data. I guess this is the last piece missing
> > for that, or are we also missing something else?
>
> For panda we just need a way to map the auxclk to the USB PHY device
> and the relevant dts data to get USB host working with DT.
> Beagle USB host should work already with DT without any changes.
>
> >
> > Can't you set up a clock alias for your device so it can find the
> > auxclk when requesting it with the dev entry?
> >
>
> which clock is mapped to which PHY device depends on board design
> and that cannot be per-determined at one place. This information
> needs to come from the board.dts file.

OK that makes sense.

> There was an earlier attempt to provide a way of building clock aliases
> at runtime from device tree [1], but the current approach is way better
>
> [1]
> https://lkml.org/lkml/2013/3/12/241
>
> > For the DT clock driver if needed for v3.10, how about just do a
> > minimal drivers/clock/omap/ that uses the standard binding?
> > Then that driver can just do clk_get() from cclock44xx_data.c
>
> I don't understand how to do it and why it is better than the current
> approach.

Well your approach is fine as a first step moving all the clock
code, but it needs to be a real driver under drivers/clock/omap.
And the DT binding needs to stay the same for the driver(s) in the
long term as we start moving clocks to DT + /lib/firmware.

If this all is too late for v3.10, I suggest you just set up the
right clock alias for panda with machine_is_compatible flag in
board-generic.c so we get EHCI working with DT for v3.10. Then
it's easy to to deal with it properly for v3.11.

> How can that driver do clk_get() from cclock44xx_data.c?
> from where does it get the clk_id to pass into clk_get()?

Can't you just use the clock name there to get it?

> > for now? And then later on we'll just move all the clocks to a
> > combination of DT + /lib/firmware.
>
> What is the benefit of moving clock data to /lib/firmware? We could
> as well just move it to DT only, no?

DT only clocks option is naturally available with this too. It
just gets easily inefficient with such a huge number of clocks.

> >> e.g. auxclk are required to be specified in DT nodes for USB PHY.
> >> Without this we can't get USB host working on Panda.
> >
> > OK. So if the USB PHY has a dev entry, can't you just set up a
> > clock alias in struct omap_clk omap44xx_clks[] for it?
>
> I've explained why this can't be done above.

Yes I understand now, the clock is board specific.

> >> As Rajendra points out, it seems moving entire clock data to DT is not
> >> going to happen soon. So this is the simplistic way things can work.
> >
> > Right but seems like we can get started there without moving
> > them all at once.
> >
> What if we provide a complete clock list instead of only auxclks in
> dt_clks[]?
>
> This approach is similar to arch/arm/mach-imx/clk-imx35.c
>
> Device drivers can then use them as they migrate to DT. Then later
> we could migrate clock data to DT, without impacting device drivers.

As long as the binding stays the same in the long run too, this
clock remapping approach is just fine as a starting point. And
the driver needs to go to drivers/clock/omap. But in the long run
we just want to get the huge amounts static data out of the kernel
for clocks and hwmod data to fix things for good.

Regards,

Tony

2013-04-05 17:57:52

by Grygorii Strashko

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/04/2013 07:41 PM, Tony Lindgren wrote:
> * Roger Quadros <[email protected]> [130404 00:39]:
>> On 04/04/2013 02:42 AM, Tony Lindgren wrote:
>>>> --- a/arch/arm/mach-omap2/cclock44xx_data.c
>>>> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
>>>> @@ -27,6 +27,7 @@
>>>> #include <linux/clk-private.h>
>>>> #include <linux/clkdev.h>
>>>> #include <linux/io.h>
>>>> +#include <linux/of.h>
>>>>
>>>> #include "soc.h"
>>>> #include "iomap.h"
>>>> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
>>>> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
>>>> };
>>>>
>>>> +static struct clk *scrm_clks[] = {
>>>> + &auxclk0_ck,
>>>> + &auxclk1_ck,
>>>> + &auxclk2_ck,
>>>> + &auxclk3_ck,
>>>> + &auxclk4_ck,
>>>> + &auxclk5_ck,
>>>> +};
>>> Hmm I don't like the idea of specifying the auxclk both in the
>>> cclock44xx_data.c and in DT..
>> Right, but till we have all clocks moved to DT we only need this
>> approach for general purpose clocks that are not mapped to devices
>> by hwmod.
> For v3.10, let's just make sure that USB works with DT as then
> after v3.10 we can make omap4 DT only and get rid of estimated
> 7K lines of code and data. I guess this is the last piece missing
> for that, or are we also missing something else?
>
> Can't you set up a clock alias for your device so it can find the
> auxclk when requesting it with the dev entry?
>
> For the DT clock driver if needed for v3.10, how about just do a
> minimal drivers/clock/omap/ that uses the standard binding?
> Then that driver can just do clk_get() from cclock44xx_data.c
> for now? And then later on we'll just move all the clocks to a
> combination of DT + /lib/firmware.
>
Hi Roger, Rajendra, All

Sorry that disturbing you.

I'm supporting Android OMAP4 kernels (K3.0/K3.4) and like to clarify few
points regarding this approach (having into account possible future
migrations
on newer Kernels and OMAP5).
If I understand everything right, this patch series allows to create
clock binding
in DT using following syntax: clocks = <&aux_clks 3>
- does it means that in worst case there will be ~200 clock IDs defined?
- does it means that clock nodes binding using phandles
(human-friendly notation)
isn't going to be supported?
for example:
clocks = <&sys_clkin_ck>
clocks = <&dss_sys_clk &dss_tv_clk &dss_dss_clk>)

I was horrified to think about the problems of this approach support
(in case if there would be more then ~30 IDs) - just miss with on digit
and weeks of debugging would be guaranteed.

Please, say me that i'm wrong.
And why clock DT data can't be auto-generated like all other OMAP data
to close this questions?

Thanks.

>> e.g. auxclk are required to be specified in DT nodes for USB PHY.
>> Without this we can't get USB host working on Panda.
> OK. So if the USB PHY has a dev entry, can't you just set up a
> clock alias in struct omap_clk omap44xx_clks[] for it?
>
>> As Rajendra points out, it seems moving entire clock data to DT is not
>> going to happen soon. So this is the simplistic way things can work.
> Right but seems like we can get started there without moving
> them all at once.
>
> Regards,
>
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-04-09 09:55:43

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/05/2013 06:58 PM, Tony Lindgren wrote:
> * Roger Quadros <[email protected]> [130405 03:44]:
>> On 04/04/2013 07:41 PM, Tony Lindgren wrote:
>>> * Roger Quadros <[email protected]> [130404 00:39]:
>>>> On 04/04/2013 02:42 AM, Tony Lindgren wrote:
>>> For v3.10, let's just make sure that USB works with DT as then
>>> after v3.10 we can make omap4 DT only and get rid of estimated
>>> 7K lines of code and data. I guess this is the last piece missing
>>> for that, or are we also missing something else?
>>
>> For panda we just need a way to map the auxclk to the USB PHY device
>> and the relevant dts data to get USB host working with DT.
>> Beagle USB host should work already with DT without any changes.
>>
>>>
>>> Can't you set up a clock alias for your device so it can find the
>>> auxclk when requesting it with the dev entry?
>>>
>>
>> which clock is mapped to which PHY device depends on board design
>> and that cannot be per-determined at one place. This information
>> needs to come from the board.dts file.
>
> OK that makes sense.
>
>> There was an earlier attempt to provide a way of building clock aliases
>> at runtime from device tree [1], but the current approach is way better
>>
>> [1]
>> https://lkml.org/lkml/2013/3/12/241
>>
>>> For the DT clock driver if needed for v3.10, how about just do a
>>> minimal drivers/clock/omap/ that uses the standard binding?
>>> Then that driver can just do clk_get() from cclock44xx_data.c
>>
>> I don't understand how to do it and why it is better than the current
>> approach.
>
> Well your approach is fine as a first step moving all the clock
> code, but it needs to be a real driver under drivers/clock/omap.
> And the DT binding needs to stay the same for the driver(s) in the
> long term as we start moving clocks to DT + /lib/firmware.

The code needs to be there were the clock structs are defined.
Currently they are in arch/arm/mach-omap2/cclock44xx_data.c for OMAP4.

>
> If this all is too late for v3.10, I suggest you just set up the
> right clock alias for panda with machine_is_compatible flag in
> board-generic.c so we get EHCI working with DT for v3.10. Then
> it's easy to to deal with it properly for v3.11.

OK, let's do it this way for Panda for 3.10.

>
>> How can that driver do clk_get() from cclock44xx_data.c?
>> from where does it get the clk_id to pass into clk_get()?
>
> Can't you just use the clock name there to get it?

In device tree we don't pass around clock names. You can either get
a phandle or an index to the clock.

e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt

>
>>> for now? And then later on we'll just move all the clocks to a
>>> combination of DT + /lib/firmware.
>>
>> What is the benefit of moving clock data to /lib/firmware? We could
>> as well just move it to DT only, no?
>
> DT only clocks option is naturally available with this too. It
> just gets easily inefficient with such a huge number of clocks.
>

OK.
>>>> e.g. auxclk are required to be specified in DT nodes for USB PHY.
>>>> Without this we can't get USB host working on Panda.
>>>
>>> OK. So if the USB PHY has a dev entry, can't you just set up a
>>> clock alias in struct omap_clk omap44xx_clks[] for it?
>>
>> I've explained why this can't be done above.
>
> Yes I understand now, the clock is board specific.
>
>>>> As Rajendra points out, it seems moving entire clock data to DT is not
>>>> going to happen soon. So this is the simplistic way things can work.
>>>
>>> Right but seems like we can get started there without moving
>>> them all at once.
>>>
>> What if we provide a complete clock list instead of only auxclks in
>> dt_clks[]?
>>
>> This approach is similar to arch/arm/mach-imx/clk-imx35.c
>>
>> Device drivers can then use them as they migrate to DT. Then later
>> we could migrate clock data to DT, without impacting device drivers.
>
> As long as the binding stays the same in the long run too, this
> clock remapping approach is just fine as a starting point. And
> the driver needs to go to drivers/clock/omap. But in the long run
> we just want to get the huge amounts static data out of the kernel
> for clocks and hwmod data to fix things for good.

In that case we need to identify what clocks need to be supported.
If it is all (~200) of them, is this method good enough?

cheers,
-roger

2013-04-09 10:16:45

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/05/2013 08:56 PM, Grygorii Strashko wrote:
> On 04/04/2013 07:41 PM, Tony Lindgren wrote:
>> * Roger Quadros <[email protected]> [130404 00:39]:
>>> On 04/04/2013 02:42 AM, Tony Lindgren wrote:
>>>>> --- a/arch/arm/mach-omap2/cclock44xx_data.c
>>>>> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
>>>>> @@ -27,6 +27,7 @@
>>>>> #include <linux/clk-private.h>
>>>>> #include <linux/clkdev.h>
>>>>> #include <linux/io.h>
>>>>> +#include <linux/of.h>
>>>>> #include "soc.h"
>>>>> #include "iomap.h"
>>>>> @@ -1663,6 +1664,40 @@ static struct omap_clk omap44xx_clks[] = {
>>>>> CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
>>>>> };
>>>>> +static struct clk *scrm_clks[] = {
>>>>> + &auxclk0_ck,
>>>>> + &auxclk1_ck,
>>>>> + &auxclk2_ck,
>>>>> + &auxclk3_ck,
>>>>> + &auxclk4_ck,
>>>>> + &auxclk5_ck,
>>>>> +};
>>>> Hmm I don't like the idea of specifying the auxclk both in the
>>>> cclock44xx_data.c and in DT..
>>> Right, but till we have all clocks moved to DT we only need this
>>> approach for general purpose clocks that are not mapped to devices
>>> by hwmod.
>> For v3.10, let's just make sure that USB works with DT as then
>> after v3.10 we can make omap4 DT only and get rid of estimated
>> 7K lines of code and data. I guess this is the last piece missing
>> for that, or are we also missing something else?
>>
>> Can't you set up a clock alias for your device so it can find the
>> auxclk when requesting it with the dev entry?
>>
>> For the DT clock driver if needed for v3.10, how about just do a
>> minimal drivers/clock/omap/ that uses the standard binding?
>> Then that driver can just do clk_get() from cclock44xx_data.c
>> for now? And then later on we'll just move all the clocks to a
>> combination of DT + /lib/firmware.
>>
> Hi Roger, Rajendra, All
>
> Sorry that disturbing you.

Hi Grygorri,

Nothing to disturb at all ;).

>
> I'm supporting Android OMAP4 kernels (K3.0/K3.4) and like to clarify few
> points regarding this approach (having into account possible future migrations
> on newer Kernels and OMAP5).
> If I understand everything right, this patch series allows to create clock binding
> in DT using following syntax: clocks = <&aux_clks 3>

Actually in v2 of the patch this would be clocks = <&clks 3>

> - does it means that in worst case there will be ~200 clock IDs defined?

I'm afraid so yes. But when I wrote this I was only thinking of generic clocks, i.e. AUXCLKS.
So when we start talking of all clocks we might need to reconsider the approach.

> - does it means that clock nodes binding using phandles (human-friendly notation)
> isn't going to be supported?
> for example:
> clocks = <&sys_clkin_ck>
> clocks = <&dss_sys_clk &dss_tv_clk &dss_dss_clk>)

This would depend if we define the clock nodes within DT or not. From what Tony
mentioned (i.e. inefficiency) it seems that the clock nodes won't be defined
in the device tree. Then we are left with using an index.
>
> I was horrified to think about the problems of this approach support
> (in case if there would be more then ~30 IDs) - just miss with on digit
> and weeks of debugging would be guaranteed.
>
> Please, say me that i'm wrong.

It is still not written in stone so if you have a better idea we could
go that route.

cheers,
-roger

> And why clock DT data can't be auto-generated like all other OMAP data
> to close this questions?
>
> Thanks.
>
>>> e.g. auxclk are required to be specified in DT nodes for USB PHY.
>>> Without this we can't get USB host working on Panda.
>> OK. So if the USB PHY has a dev entry, can't you just set up a
>> clock alias in struct omap_clk omap44xx_clks[] for it?
>>
>>> As Rajendra points out, it seems moving entire clock data to DT is not
>>> going to happen soon. So this is the simplistic way things can work.
>> Right but seems like we can get started there without moving
>> them all at once.
>>
>> Regards,
>>
>> Tony
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2013-04-09 16:49:41

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

* Roger Quadros <[email protected]> [130409 03:00]:
> On 04/05/2013 06:58 PM, Tony Lindgren wrote:
> >
> > Well your approach is fine as a first step moving all the clock
> > code, but it needs to be a real driver under drivers/clock/omap.
> > And the DT binding needs to stay the same for the driver(s) in the
> > long term as we start moving clocks to DT + /lib/firmware.
>
> The code needs to be there were the clock structs are defined.
> Currently they are in arch/arm/mach-omap2/cclock44xx_data.c for OMAP4.

But if you do just a passthrough driver then that should not
be needed.

> > If this all is too late for v3.10, I suggest you just set up the
> > right clock alias for panda with machine_is_compatible flag in
> > board-generic.c so we get EHCI working with DT for v3.10. Then
> > it's easy to to deal with it properly for v3.11.
>
> OK, let's do it this way for Panda for 3.10.

Yes otherwise we'll be delaying omap4 DT conversion again.

> >> How can that driver do clk_get() from cclock44xx_data.c?
> >> from where does it get the clk_id to pass into clk_get()?
> >
> > Can't you just use the clock name there to get it?
>
> In device tree we don't pass around clock names. You can either get
> a phandle or an index to the clock.
>
> e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt

Yes I understand that. But the driver/clock/omap driver can just
remap the DT device initially so the board specific clock is
found from the clock alias table. Basically initially a passthrough
driver that can be enhanced to parse DT clock bindings and load
data from /lib/firmware.

> > As long as the binding stays the same in the long run too, this
> > clock remapping approach is just fine as a starting point. And
> > the driver needs to go to drivers/clock/omap. But in the long run
> > we just want to get the huge amounts static data out of the kernel
> > for clocks and hwmod data to fix things for good.
>
> In that case we need to identify what clocks need to be supported.
> If it is all (~200) of them, is this method good enough?

We should support any clock we need for booting the device with
just DT bindings to get timers, console and rootfs working. Then
we just need to load the complete set from /lib/firmware.

It seems that the binding can be the same for all the clocks.
For now, we can just use the standard clock binding and do the
remapping in the clock driver.

Regards,

Tony

2013-04-09 17:43:25

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

* Tony Lindgren <[email protected]> [130409 09:54]:
> * Roger Quadros <[email protected]> [130409 03:00]:
> > On 04/05/2013 06:58 PM, Tony Lindgren wrote:
> > >
> > > Can't you just use the clock name there to get it?
> >
> > In device tree we don't pass around clock names. You can either get
> > a phandle or an index to the clock.
> >
> > e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
>
> Yes I understand that. But the driver/clock/omap driver can just
> remap the DT device initially so the board specific clock is
> found from the clock alias table. Basically initially a passthrough
> driver that can be enhanced to parse DT clock bindings and load
> data from /lib/firmware.

Actually probably the driver/clock/omap can even do even less
initially. There probably even no need to remap clocks there.

As long as the DT clock driver understands that a board specific
auxclk is specified in the DT it can just call clk_add_alias() so
the driver will get the right auxclk from cclock44xx_data.c.

Then other features can be added later on like to allocate a
clock entirely based on the binding etc.

Regards,

Tony

2013-04-09 20:49:23

by Nishanth Menon

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 10:43-20130409, Tony Lindgren wrote:
> * Tony Lindgren <[email protected]> [130409 09:54]:
> > * Roger Quadros <[email protected]> [130409 03:00]:
> > > On 04/05/2013 06:58 PM, Tony Lindgren wrote:
> > > >
> > > > Can't you just use the clock name there to get it?
> > >
> > > In device tree we don't pass around clock names. You can either get
> > > a phandle or an index to the clock.
> > >
> > > e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
> >
> > Yes I understand that. But the driver/clock/omap driver can just
> > remap the DT device initially so the board specific clock is
> > found from the clock alias table. Basically initially a passthrough
> > driver that can be enhanced to parse DT clock bindings and load
> > data from /lib/firmware.
>
> Actually probably the driver/clock/omap can even do even less
> initially. There probably even no need to remap clocks there.
>
> As long as the DT clock driver understands that a board specific
> auxclk is specified in the DT it can just call clk_add_alias() so
> the driver will get the right auxclk from cclock44xx_data.c.
>
> Then other features can be added later on like to allocate a
> clock entirely based on the binding etc.
I did try to have an implementation for cpufreq using clock nodes.
unfortunately, device tree wont let me have arguments of strings :(
So, I am unable to do clock = <&clk mpu_dpll>;
instead, I am forced to do clock = <&clk 249>;

Here is an attempt on beagleXM - adds every clock node to the list.
Tons of un-necessary prints added to give an idea - see log:
http://pastebin.com/F9A2zSTr

Would an cleaned up version be good enough as a step #1 of transition?

>From 7d373bdb9e9549c1b6ba1775a8dfd96ebe78abfb Mon Sep 17 00:00:00 2001
From: Nishanth Menon <[email protected]>
Date: Tue, 26 Mar 2013 10:23:27 +0000
Subject: [PATCH] OMAP: add devicetree support for clock nodes.

Dummy patch based on Roger's original idea

Nyet-Signed-off-by: Nishanth Menon <[email protected]>
---
arch/arm/boot/dts/omap3.dtsi | 5 ++
arch/arm/boot/dts/omap34xx.dtsi | 2 +
arch/arm/mach-omap2/cclock3xxx_data.c | 3 +-
arch/arm/mach-omap2/cclock44xx_data.c | 3 +-
arch/arm/mach-omap2/pm.c | 11 +++-
drivers/clk/Kconfig | 6 ++
drivers/clk/Makefile | 2 +
drivers/clk/ti.c | 100 +++++++++++++++++++++++++++++++++
include/linux/clk/ti.h | 30 ++++++++++
9 files changed, 157 insertions(+), 5 deletions(-)
create mode 100644 drivers/clk/ti.c
create mode 100644 include/linux/clk/ti.h

diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index 3344f05..a08990d 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -73,6 +73,11 @@
ti,hwmods = "counter_32k";
};

+ clks: clocks {
+ compatible = "ti,clock";
+ #clock-cells = <1>;
+ };
+
intc: [email protected] {
compatible = "ti,omap2-intc";
interrupt-controller;
diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
index 75ed4ae..93c2621 100644
--- a/arch/arm/boot/dts/omap34xx.dtsi
+++ b/arch/arm/boot/dts/omap34xx.dtsi
@@ -23,6 +23,8 @@
600000 1350000
>;
clock-latency = <300000>; /* From legacy driver */
+ clocks = <&clks 249>; /* index to cpufreq_ck */
+ clock-names = "cpu";
};
};
};
diff --git a/arch/arm/mach-omap2/cclock3xxx_data.c b/arch/arm/mach-omap2/cclock3xxx_data.c
index 4579c3c..d5d5ef5 100644
--- a/arch/arm/mach-omap2/cclock3xxx_data.c
+++ b/arch/arm/mach-omap2/cclock3xxx_data.c
@@ -22,6 +22,7 @@
#include <linux/clk-private.h>
#include <linux/list.h>
#include <linux/io.h>
+#include <linux/clk/ti.h>

#include "soc.h"
#include "iomap.h"
@@ -3574,7 +3575,7 @@ int __init omap3xxx_clk_init(void)
for (c = omap3xxx_clks; c < omap3xxx_clks + ARRAY_SIZE(omap3xxx_clks);
c++)
if (c->cpu & cpu_clkflg) {
- clkdev_add(&c->lk);
+ ti_clk_node_add(&c->lk);
if (!__clk_init(NULL, c->lk.clk))
omap2_init_clk_hw_omap_clocks(c->lk.clk);
}
diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
index 0c6834a..338ef64 100644
--- a/arch/arm/mach-omap2/cclock44xx_data.c
+++ b/arch/arm/mach-omap2/cclock44xx_data.c
@@ -27,6 +27,7 @@
#include <linux/clk-private.h>
#include <linux/clkdev.h>
#include <linux/io.h>
+#include <linux/clk/ti.h>

#include "soc.h"
#include "iomap.h"
@@ -1697,7 +1698,7 @@ int __init omap4xxx_clk_init(void)
for (c = omap44xx_clks; c < omap44xx_clks + ARRAY_SIZE(omap44xx_clks);
c++) {
if (c->cpu & cpu_clkflg) {
- clkdev_add(&c->lk);
+ ti_clk_node_add(&c->lk);
if (!__clk_init(NULL, c->lk.clk))
omap2_init_clk_hw_omap_clocks(c->lk.clk);
}
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 8d15f9a..6cf95160 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -267,7 +267,12 @@ static void __init omap4_init_voltages(void)

static inline void omap_init_cpufreq(void)
{
- struct platform_device_info devinfo = { .name = "omap-cpufreq", };
+ struct platform_device_info devinfo = { };
+
+ if (!of_have_populated_dt())
+ devinfo.name = "omap-cpufreq";
+ else
+ devinfo.name = "cpufreq-cpu0";
platform_device_register_full(&devinfo);
}

@@ -301,9 +306,9 @@ int __init omap2_common_pm_late_init(void)
/* Smartreflex device init */
omap_devinit_smartreflex();

- /* cpufreq dummy device instantiation */
- omap_init_cpufreq();
}
+ /* cpufreq dummy device instantiation */
+ omap_init_cpufreq();

#ifdef CONFIG_SUSPEND
suspend_set_ops(&omap_pm_ops);
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index a47e6ee..03c6e48 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -55,6 +55,12 @@ config COMMON_CLK_MAX77686
---help---
This driver supports Maxim 77686 crystal oscillator clock.

+config COMMON_CLK_TI
+ tristate "Clock driver for TI SoCs"
+ depends on ARCH_OMAP && OF
+ ---help---
+ Fill me up.. some generic statement ofcourse (lets start with OMAP)
+
config CLK_TWL6040
tristate "External McPDM functional clock from twl6040"
depends on TWL6040_CORE
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 300d477..9621815 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -30,6 +30,8 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/

obj-$(CONFIG_X86) += x86/

+obj-$(CONFIG_COMMON_CLK_TI) += ti.o
+
# Chip specific
obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
diff --git a/drivers/clk/ti.c b/drivers/clk/ti.c
new file mode 100644
index 0000000..c747381
--- /dev/null
+++ b/drivers/clk/ti.c
@@ -0,0 +1,100 @@
+/*
+ * TI Clock node provider
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Nishanth Menon
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/clk-private.h>
+#include <linux/clkdev.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk/ti.h>
+
+struct ti_clk {
+ struct clk_lookup *lk;
+ struct list_head node;
+};
+
+static LIST_HEAD(ti_clk_list);
+
+static struct clk *ti_bypass_clk_get_by_name(struct of_phandle_args *clkspec,
+ void *data)
+{
+ struct ti_clk *c;
+#if 0
+ /* Eww.. IF ONLY phandle arguments could be string */
+ char clk_name[32]; /* 32 is max size of property name */
+
+ char *name = clkspec->args[0];
+
+ snprintf(clk_name, 32, "%s_ck", name);
+ list_for_each_entry(c, &ti_clk_list, node) {
+ int r = strncmp(c->lk->conn_id, clk_name, 32);
+ pr_err("%s: searching %s in %s - %d\n",
+ __func__, clk_name, c->lk->con_id, r);
+ if (!r) {
+ pr_err("%s: found it!\n", __func__);
+ return c->lk.clk;
+ }
+ }
+#else
+ /* Use integer indexing into clkdev list! Sigh.. */
+ int idx = clkspec->args[0];
+ int cindex = 1;
+
+ list_for_each_entry(c, &ti_clk_list, node) {
+ int r = (idx == cindex) ? 0 : 1;
+ pr_err("%s: searching index search = %d in %d in %s - %d\n",
+ __func__, idx, cindex, c->lk->con_id, r);
+ if (!r) {
+ pr_err("%s: found it!\n", __func__);
+ return c->lk->clk;
+ }
+ cindex++;
+ }
+#endif
+
+ pr_err("%s: ran out of options\n", __func__);
+ return ERR_PTR(-ENODEV);
+}
+
+static void __init ti_clock_init(struct device_node *node)
+{
+
+ pr_err("%s: START\n", __func__);
+ of_clk_add_provider(node, ti_bypass_clk_get_by_name, NULL);
+ pr_err("%s: END\n", __func__);
+}
+CLK_OF_DECLARE(ti_clk, "ti,clock", ti_clock_init);
+
+void __init ti_clk_node_add(struct clk_lookup *lk)
+{
+ struct ti_clk *c;
+ static bool of_added;
+
+ c = kzalloc(sizeof(struct ti_clk), GFP_KERNEL);
+ if (!c) {
+ pr_err("%s: No memory!! cannot add clk node!\n", __func__);
+ return;
+ }
+ clkdev_add(lk);
+ c->lk = lk;
+ list_add_tail(&c->node, &ti_clk_list);
+ pr_err("%s: Added clock node %s\n", __func__, lk->con_id);
+ if (!of_added) {
+ of_clk_init(NULL);
+ of_added = true;
+ }
+};
+
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
new file mode 100644
index 0000000..eb502a8
--- /dev/null
+++ b/include/linux/clk/ti.h
@@ -0,0 +1,30 @@
+/*
+ * TI Clock node provider header
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Nishanth Menon
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#ifndef __TI_CLK_H
+#define __TI_CLK_H
+
+#include <linux/clkdev.h>
+
+#ifdef CONFIG_OF
+extern void ti_clk_node_add(struct clk_lookup *lk);
+#else
+static inline void ti_clk_node_add(struct clk_lookup *lk)
+{
+ clkdev_add(lk);
+}
+#endif /* CONFIG_OF */
+
+#endif /* __TI_CLK_H */
--
1.7.9.5


--
Regards,
Nishanth Menon

2013-04-09 21:54:37

by Nishanth Menon

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 15:49-20130409, Nishanth Menon wrote:
> On 10:43-20130409, Tony Lindgren wrote:
> > * Tony Lindgren <[email protected]> [130409 09:54]:
> > > * Roger Quadros <[email protected]> [130409 03:00]:
> > > > On 04/05/2013 06:58 PM, Tony Lindgren wrote:
> > > > >
> > > > > Can't you just use the clock name there to get it?
> > > >
> > > > In device tree we don't pass around clock names. You can either get
> > > > a phandle or an index to the clock.
> > > >
> > > > e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
> > >
> > > Yes I understand that. But the driver/clock/omap driver can just
> > > remap the DT device initially so the board specific clock is
> > > found from the clock alias table. Basically initially a passthrough
> > > driver that can be enhanced to parse DT clock bindings and load
> > > data from /lib/firmware.
> >
> > Actually probably the driver/clock/omap can even do even less
> > initially. There probably even no need to remap clocks there.
> >
> > As long as the DT clock driver understands that a board specific
> > auxclk is specified in the DT it can just call clk_add_alias() so
> > the driver will get the right auxclk from cclock44xx_data.c.
> >
> > Then other features can be added later on like to allocate a
> > clock entirely based on the binding etc.
> I did try to have an implementation for cpufreq using clock nodes.
> unfortunately, device tree wont let me have arguments of strings :(
> So, I am unable to do clock = <&clk mpu_dpll>;
> instead, I am forced to do clock = <&clk 249>;
>
> Here is an attempt on beagleXM - adds every clock node to the list.
> Tons of un-necessary prints added to give an idea - see log:
> http://pastebin.com/F9A2zSTr
>
> Would an cleaned up version be good enough as a step #1 of transition?
>
Approach #2:
Here is yet another revision -> here I am trying to avoid the risk of
folks messing up indexing. for example: using an older DTB with newer
kernel, clocks being inserted into existing list etc. to prevent these,
we add an "DT_ID" for omap clock nodes, and use it to uniquely identify
the clock node. We try to minimize(not avoidable with integer indexing)
mistakes during development/productization.

>From 2b576affdc6f6bf0b51ebf9b85ef4319357a7994 Mon Sep 17 00:00:00 2001
From: Nishanth Menon <[email protected]>
Date: Tue, 26 Mar 2013 10:23:27 +0000
Subject: [RFC PATCH V2] OMAP: add devicetree support for clock nodes.(rev 2)

Dummy patch based on Roger's original idea
this time have lesser possibiltiy of screwing up indices. instead
use a specific integer ID to uniquely (TI SoC wide) identify an clock.
on the flip side, we do not all make clock nodes to be accesible from DT
clock indexing.

Nyet-Signed-off-by: Nishanth Menon <[email protected]>
---
.../devicetree/bindings/clock/ti,clock.txt | 48 +++++++++++
arch/arm/boot/dts/omap3.dtsi | 5 ++
arch/arm/boot/dts/omap34xx.dtsi | 2 +
arch/arm/boot/dts/omap4.dtsi | 5 ++
arch/arm/boot/dts/omap443x.dtsi | 2 +
arch/arm/mach-omap2/cclock3xxx_data.c | 5 +-
arch/arm/mach-omap2/cclock44xx_data.c | 5 +-
arch/arm/mach-omap2/clock.h | 12 +++
arch/arm/mach-omap2/pm.c | 11 ++-
drivers/clk/Kconfig | 6 ++
drivers/clk/Makefile | 2 +
drivers/clk/ti.c | 85 ++++++++++++++++++++
include/linux/clk/ti.h | 30 +++++++
13 files changed, 211 insertions(+), 7 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/ti,clock.txt
create mode 100644 drivers/clk/ti.c
create mode 100644 include/linux/clk/ti.h

diff --git a/Documentation/devicetree/bindings/clock/ti,clock.txt b/Documentation/devicetree/bindings/clock/ti,clock.txt
new file mode 100644
index 0000000..53c947c
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti,clock.txt
@@ -0,0 +1,48 @@
+* Clock bindings for Texas Instruments clocks
+
+Required properties:
+- compatible: Should be "ti,clock"
+- #clock-cells: Should be <1>
+
+The clock consumer should specify the desired clock by having the clock
+ID in its "clocks" phandle cell. The following is a list of ID reservations
+across TI SoCs
+
+ Clock ID
+ ------------------
+ cpu clock 1
+
+The definition is used by SoC clock data using CLKDT() macro
+(example of OMAP2+).
+
+Example Steps:
+/* step 1: definiting an SoC nodes compatible with ti clocks -skip if already done */
+clks: clocks {
+ compatible = "ti,clock";
+ #clock-cells = <1>;
+};
+
+/* step 2: Modify the bindings documentation to reserve an ID */
+ auxclk0_ck 2
+ auxclk1_ck 3
+ auxclk2_ck 4
+ auxclk3_ck 5
+ auxclk4_ck 6
+ auxclk5_ck 7
+etc. use an unique number not conflicting with previous reservations.
+WARNING: DONOT insert values - this breaks backward compatibility.
+/* Step 3: replace in clock data file */
+ CLK(NULL, "auxclk0_ck", &auxclk0_ck, CK_443X),
+with
+ CLKDT(2, NULL, "auxclk0_ck", &auxclk0_ck, CK_443X),
+
+/* step 4: in device tree entry, now we can use index 2 to uniquely identify auxclk0 clock */
+
+hsusb1_phy: hsusb1_phy {
+ compatible = "usb-nop-xceiv";
+ reset-supply = <&hsusb1_reset>;
+ clocks = <&aux_clks 2>;
+ clock-names = "main_clk";
+ clock-frequency = <19200000>;
+};
+
diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index 3344f05..a08990d 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -73,6 +73,11 @@
ti,hwmods = "counter_32k";
};

+ clks: clocks {
+ compatible = "ti,clock";
+ #clock-cells = <1>;
+ };
+
intc: [email protected] {
compatible = "ti,omap2-intc";
interrupt-controller;
diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
index 75ed4ae..de59fb8 100644
--- a/arch/arm/boot/dts/omap34xx.dtsi
+++ b/arch/arm/boot/dts/omap34xx.dtsi
@@ -23,6 +23,8 @@
600000 1350000
>;
clock-latency = <300000>; /* From legacy driver */
+ clocks = <&clks 1>;
+ clock-names = "cpu";
};
};
};
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 3329140..ad18d6a 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -106,6 +106,11 @@
ti,hwmods = "counter_32k";
};

+ clks: clocks {
+ compatible = "ti,clock";
+ #clock-cells = <1>;
+ };
+
omap4_pmx_core: pinmux[email protected] {
compatible = "ti,omap4-padconf", "pinctrl-single";
reg = <0x4a100040 0x0196>;
diff --git a/arch/arm/boot/dts/omap443x.dtsi b/arch/arm/boot/dts/omap443x.dtsi
index cccf39a..5bfc02b 100644
--- a/arch/arm/boot/dts/omap443x.dtsi
+++ b/arch/arm/boot/dts/omap443x.dtsi
@@ -22,6 +22,8 @@
1008000 1375000
>;
clock-latency = <300000>; /* From legacy driver */
+ clocks = <&clks 1>;
+ clock-names = "cpu";
};
};
};
diff --git a/arch/arm/mach-omap2/cclock3xxx_data.c b/arch/arm/mach-omap2/cclock3xxx_data.c
index 4579c3c..2f2932b 100644
--- a/arch/arm/mach-omap2/cclock3xxx_data.c
+++ b/arch/arm/mach-omap2/cclock3xxx_data.c
@@ -22,6 +22,7 @@
#include <linux/clk-private.h>
#include <linux/list.h>
#include <linux/io.h>
+#include <linux/clk/ti.h>

#include "soc.h"
#include "iomap.h"
@@ -3501,7 +3502,7 @@ static struct omap_clk omap3xxx_clks[] = {
CLK(NULL, "uart4_ick", &uart4_ick_am35xx, CK_AM35XX),
CLK(NULL, "timer_32k_ck", &omap_32k_fck, CK_3XXX),
CLK(NULL, "timer_sys_ck", &sys_ck, CK_3XXX),
- CLK(NULL, "cpufreq_ck", &dpll1_ck, CK_3XXX),
+ CLKDT(1, NULL, "cpufreq_ck", &dpll1_ck, CK_3XXX),
};

static const char *enable_init_clks[] = {
@@ -3574,7 +3575,7 @@ int __init omap3xxx_clk_init(void)
for (c = omap3xxx_clks; c < omap3xxx_clks + ARRAY_SIZE(omap3xxx_clks);
c++)
if (c->cpu & cpu_clkflg) {
- clkdev_add(&c->lk);
+ ti_clk_node_add(&c->lk, c->dt_clkid);
if (!__clk_init(NULL, c->lk.clk))
omap2_init_clk_hw_omap_clocks(c->lk.clk);
}
diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
index 0c6834a..74d4e8c 100644
--- a/arch/arm/mach-omap2/cclock44xx_data.c
+++ b/arch/arm/mach-omap2/cclock44xx_data.c
@@ -27,6 +27,7 @@
#include <linux/clk-private.h>
#include <linux/clkdev.h>
#include <linux/io.h>
+#include <linux/clk/ti.h>

#include "soc.h"
#include "iomap.h"
@@ -1672,7 +1673,7 @@ static struct omap_clk omap44xx_clks[] = {
CLK("4013a000.timer", "timer_sys_ck", &syc_clk_div_ck, CK_443X),
CLK("4013c000.timer", "timer_sys_ck", &syc_clk_div_ck, CK_443X),
CLK("4013e000.timer", "timer_sys_ck", &syc_clk_div_ck, CK_443X),
- CLK(NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
+ CLKDT(1, NULL, "cpufreq_ck", &dpll_mpu_ck, CK_443X),
};

int __init omap4xxx_clk_init(void)
@@ -1697,7 +1698,7 @@ int __init omap4xxx_clk_init(void)
for (c = omap44xx_clks; c < omap44xx_clks + ARRAY_SIZE(omap44xx_clks);
c++) {
if (c->cpu & cpu_clkflg) {
- clkdev_add(&c->lk);
+ ti_clk_node_add(&c->lk, c->dt_clkid);
if (!__clk_init(NULL, c->lk.clk))
omap2_init_clk_hw_omap_clocks(c->lk.clk);
}
diff --git a/arch/arm/mach-omap2/clock.h b/arch/arm/mach-omap2/clock.h
index 60ddd86..9a7c95b 100644
--- a/arch/arm/mach-omap2/clock.h
+++ b/arch/arm/mach-omap2/clock.h
@@ -23,10 +23,22 @@
#include <linux/clk-provider.h>

struct omap_clk {
+ u16 dt_clkid;
u16 cpu;
struct clk_lookup lk;
};

+#define CLKDT(id, dev, con, ck, cp) \
+ { \
+ .dt_clkid = id, \
+ .cpu = cp, \
+ .lk = { \
+ .dev_id = dev, \
+ .con_id = con, \
+ .clk = ck, \
+ }, \
+ }
+
#define CLK(dev, con, ck, cp) \
{ \
.cpu = cp, \
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index a47e6ee..03c6e48 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -55,6 +55,12 @@ config COMMON_CLK_MAX77686
---help---
This driver supports Maxim 77686 crystal oscillator clock.

+config COMMON_CLK_TI
+ tristate "Clock driver for TI SoCs"
+ depends on ARCH_OMAP && OF
+ ---help---
+ Fill me up.. some generic statement ofcourse (lets start with OMAP)
+
config CLK_TWL6040
tristate "External McPDM functional clock from twl6040"
depends on TWL6040_CORE
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 300d477..9621815 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -30,6 +30,8 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/

obj-$(CONFIG_X86) += x86/

+obj-$(CONFIG_COMMON_CLK_TI) += ti.o
+
# Chip specific
obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
diff --git a/drivers/clk/ti.c b/drivers/clk/ti.c
new file mode 100644
index 0000000..56b8754
--- /dev/null
+++ b/drivers/clk/ti.c
@@ -0,0 +1,85 @@
+/*
+ * TI Clock node provider
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Nishanth Menon
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/clk-private.h>
+#include <linux/clkdev.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk/ti.h>
+
+struct ti_clk {
+ struct clk_lookup *lk;
+ u16 id;
+ struct list_head node;
+};
+
+static LIST_HEAD(ti_clk_list);
+
+static struct clk *ti_bypass_clk_get_by_name(struct of_phandle_args *clkspec,
+ void *data)
+{
+ struct ti_clk *c;
+ int idx = clkspec->args[0];
+
+ list_for_each_entry(c, &ti_clk_list, node) {
+ if (idx == c->id)
+ return c->lk->clk;
+ }
+
+ pr_err("%s: Clock ID %d not found in list\n", __func__, idx);
+ return ERR_PTR(-ENODEV);
+}
+
+static void __init ti_clock_init(struct device_node *node)
+{
+
+ pr_err("%s: START\n", __func__);
+ of_clk_add_provider(node, ti_bypass_clk_get_by_name, NULL);
+ pr_err("%s: END\n", __func__);
+}
+CLK_OF_DECLARE(ti_clk, "ti,clock", ti_clock_init);
+
+void __init ti_clk_node_add(struct clk_lookup *lk, u16 id)
+{
+ struct ti_clk *c;
+ static bool of_added;
+
+ if (lk->dev_id || !id)
+ goto out;
+
+ /* No lists if we did not boot supported by DT entries */
+ if (!of_have_populated_dt())
+ goto out;
+
+ c = kzalloc(sizeof(struct ti_clk), GFP_KERNEL);
+ if (!c) {
+ pr_err("%s: No memory!! cannot add clk node for DT!\n",
+ __func__);
+ /* lets try atleast an regular clock node */
+ goto out;
+ }
+ c->lk = lk;
+ c->id = id;
+ list_add_tail(&c->node, &ti_clk_list);
+ pr_err("%s: Added clock node %s\n", __func__, lk->con_id);
+ if (!of_added) {
+ of_clk_init(NULL);
+ of_added = true;
+ }
+out:
+ clkdev_add(lk);
+};
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
new file mode 100644
index 0000000..ebed98a
--- /dev/null
+++ b/include/linux/clk/ti.h
@@ -0,0 +1,30 @@
+/*
+ * TI Clock node provider header
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Nishanth Menon
+ *
+ * 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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#ifndef __TI_CLK_H
+#define __TI_CLK_H
+
+#include <linux/clkdev.h>
+
+#ifdef CONFIG_OF
+extern void ti_clk_node_add(struct clk_lookup *lk, u16 id);
+#else
+static inline void ti_clk_node_add(struct clk_lookup *lk, u16 id)
+{
+ clkdev_add(lk);
+}
+#endif /* CONFIG_OF */
+
+#endif /* __TI_CLK_H */
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 8d15f9a..6cf95160 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -267,7 +267,12 @@ static void __init omap4_init_voltages(void)

static inline void omap_init_cpufreq(void)
{
- struct platform_device_info devinfo = { .name = "omap-cpufreq", };
+ struct platform_device_info devinfo = { };
+
+ if (!of_have_populated_dt())
+ devinfo.name = "omap-cpufreq";
+ else
+ devinfo.name = "cpufreq-cpu0";
platform_device_register_full(&devinfo);
}

@@ -301,9 +306,9 @@ int __init omap2_common_pm_late_init(void)
/* Smartreflex device init */
omap_devinit_smartreflex();

- /* cpufreq dummy device instantiation */
- omap_init_cpufreq();
}
+ /* cpufreq dummy device instantiation */
+ omap_init_cpufreq();

#ifdef CONFIG_SUSPEND
suspend_set_ops(&omap_pm_ops);
--
1.7.9.5

--
Regards,
Nishanth Menon

2013-04-09 22:22:38

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

* Nishanth Menon <[email protected]> [130409 13:53]:
> I did try to have an implementation for cpufreq using clock nodes.
> unfortunately, device tree wont let me have arguments of strings :(
> So, I am unable to do clock = <&clk mpu_dpll>;
> instead, I am forced to do clock = <&clk 249>;

It seems that you should have a separate clock defines for each
clock instead. That way we can later on populate that with
the clock specific data.

> Here is an attempt on beagleXM - adds every clock node to the list.
> Tons of un-necessary prints added to give an idea - see log:
> http://pastebin.com/F9A2zSTr
>
> Would an cleaned up version be good enough as a step #1 of transition?

Well I would make it even simpler initially. Just a standard .dts
clock defined that gets parsed by drivers/clock/ driver that just
calls clk_add_alias(). Then the consumer driver can just do clk_get()
and and the right clock is found, see below.

> --- a/arch/arm/boot/dts/omap3.dtsi
> +++ b/arch/arm/boot/dts/omap3.dtsi
> @@ -73,6 +73,11 @@
> ti,hwmods = "counter_32k";
> };
>
> + clks: clocks {
> + compatible = "ti,clock";
> + #clock-cells = <1>;
> + };
> +
> intc: [email protected] {
> compatible = "ti,omap2-intc";
> interrupt-controller;

There should be a separate entry for each clock defined,
like auxclk1 in the USB case assuming the clock being used
is aux clock #1. I doubt that we want to map all the auxclks
as a single clock as they are separate clocks AFAIK.

> --- a/arch/arm/boot/dts/omap34xx.dtsi
> +++ b/arch/arm/boot/dts/omap34xx.dtsi
> @@ -23,6 +23,8 @@
> 600000 1350000
> >;
> clock-latency = <300000>; /* From legacy driver */
> + clocks = <&clks 249>; /* index to cpufreq_ck */
> + clock-names = "cpu";
> };
> };
> };

Then in the consumer driver you would just have:

clocks = <&auxclk1 0>;

for the USB case, then something else for the cpufreq driver.

> --- a/arch/arm/mach-omap2/cclock3xxx_data.c
> +++ b/arch/arm/mach-omap2/cclock3xxx_data.c
> @@ -22,6 +22,7 @@
> #include <linux/clk-private.h>
> #include <linux/list.h>
> #include <linux/io.h>
> +#include <linux/clk/ti.h>
>
> #include "soc.h"
> #include "iomap.h"
> @@ -3574,7 +3575,7 @@ int __init omap3xxx_clk_init(void)
> for (c = omap3xxx_clks; c < omap3xxx_clks + ARRAY_SIZE(omap3xxx_clks);
> c++)
> if (c->cpu & cpu_clkflg) {
> - clkdev_add(&c->lk);
> + ti_clk_node_add(&c->lk);
> if (!__clk_init(NULL, c->lk.clk))
> omap2_init_clk_hw_omap_clocks(c->lk.clk);
> }

AFAIK no need to tinkering with the clockxxxx_data.c files.

> --- /dev/null
> +++ b/drivers/clk/ti.c
> @@ -0,0 +1,100 @@
> +/*
> + * TI Clock node provider
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> + * Nishanth Menon
> + *
> + * 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.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/clk-private.h>
> +#include <linux/clkdev.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/clk/ti.h>

Then this can be just a minimal DT device driver that initially just
calls clk_add_alias() so the right clock is found when the driver
does clk_get(). Probably should be drivers/clock/omap/clk.c.

Regards,

Tony

2013-04-10 08:06:46

by Mike Turquette

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Quoting Nishanth Menon (2013-04-09 13:49:00)
> On 10:43-20130409, Tony Lindgren wrote:
> > * Tony Lindgren <[email protected]> [130409 09:54]:
> > > * Roger Quadros <[email protected]> [130409 03:00]:
> > > > On 04/05/2013 06:58 PM, Tony Lindgren wrote:
> > > > >
> > > > > Can't you just use the clock name there to get it?
> > > >
> > > > In device tree we don't pass around clock names. You can either get
> > > > a phandle or an index to the clock.
> > > >
> > > > e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
> > >
> > > Yes I understand that. But the driver/clock/omap driver can just
> > > remap the DT device initially so the board specific clock is
> > > found from the clock alias table. Basically initially a passthrough
> > > driver that can be enhanced to parse DT clock bindings and load
> > > data from /lib/firmware.
> >
> > Actually probably the driver/clock/omap can even do even less
> > initially. There probably even no need to remap clocks there.
> >
> > As long as the DT clock driver understands that a board specific
> > auxclk is specified in the DT it can just call clk_add_alias() so
> > the driver will get the right auxclk from cclock44xx_data.c.
> >
> > Then other features can be added later on like to allocate a
> > clock entirely based on the binding etc.
> I did try to have an implementation for cpufreq using clock nodes.
> unfortunately, device tree wont let me have arguments of strings :(
> So, I am unable to do clock = <&clk mpu_dpll>;
> instead, I am forced to do clock = <&clk 249>;
>

See http://article.gmane.org/gmane.linux.ports.arm.kernel/229034

Regards,
Mike

> Here is an attempt on beagleXM - adds every clock node to the list.
> Tons of un-necessary prints added to give an idea - see log:
> http://pastebin.com/F9A2zSTr
>
> Would an cleaned up version be good enough as a step #1 of transition?
>
> From 7d373bdb9e9549c1b6ba1775a8dfd96ebe78abfb Mon Sep 17 00:00:00 2001
> From: Nishanth Menon <[email protected]>
> Date: Tue, 26 Mar 2013 10:23:27 +0000
> Subject: [PATCH] OMAP: add devicetree support for clock nodes.
>
> Dummy patch based on Roger's original idea
>
> Nyet-Signed-off-by: Nishanth Menon <[email protected]>
> ---
> arch/arm/boot/dts/omap3.dtsi | 5 ++
> arch/arm/boot/dts/omap34xx.dtsi | 2 +
> arch/arm/mach-omap2/cclock3xxx_data.c | 3 +-
> arch/arm/mach-omap2/cclock44xx_data.c | 3 +-
> arch/arm/mach-omap2/pm.c | 11 +++-
> drivers/clk/Kconfig | 6 ++
> drivers/clk/Makefile | 2 +
> drivers/clk/ti.c | 100 +++++++++++++++++++++++++++++++++
> include/linux/clk/ti.h | 30 ++++++++++
> 9 files changed, 157 insertions(+), 5 deletions(-)
> create mode 100644 drivers/clk/ti.c
> create mode 100644 include/linux/clk/ti.h
>
> diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
> index 3344f05..a08990d 100644
> --- a/arch/arm/boot/dts/omap3.dtsi
> +++ b/arch/arm/boot/dts/omap3.dtsi
> @@ -73,6 +73,11 @@
> ti,hwmods = "counter_32k";
> };
>
> + clks: clocks {
> + compatible = "ti,clock";
> + #clock-cells = <1>;
> + };
> +
> intc: [email protected] {
> compatible = "ti,omap2-intc";
> interrupt-controller;
> diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
> index 75ed4ae..93c2621 100644
> --- a/arch/arm/boot/dts/omap34xx.dtsi
> +++ b/arch/arm/boot/dts/omap34xx.dtsi
> @@ -23,6 +23,8 @@
> 600000 1350000
> >;
> clock-latency = <300000>; /* From legacy driver */
> + clocks = <&clks 249>; /* index to cpufreq_ck */
> + clock-names = "cpu";
> };
> };
> };
> diff --git a/arch/arm/mach-omap2/cclock3xxx_data.c b/arch/arm/mach-omap2/cclock3xxx_data.c
> index 4579c3c..d5d5ef5 100644
> --- a/arch/arm/mach-omap2/cclock3xxx_data.c
> +++ b/arch/arm/mach-omap2/cclock3xxx_data.c
> @@ -22,6 +22,7 @@
> #include <linux/clk-private.h>
> #include <linux/list.h>
> #include <linux/io.h>
> +#include <linux/clk/ti.h>
>
> #include "soc.h"
> #include "iomap.h"
> @@ -3574,7 +3575,7 @@ int __init omap3xxx_clk_init(void)
> for (c = omap3xxx_clks; c < omap3xxx_clks + ARRAY_SIZE(omap3xxx_clks);
> c++)
> if (c->cpu & cpu_clkflg) {
> - clkdev_add(&c->lk);
> + ti_clk_node_add(&c->lk);
> if (!__clk_init(NULL, c->lk.clk))
> omap2_init_clk_hw_omap_clocks(c->lk.clk);
> }
> diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
> index 0c6834a..338ef64 100644
> --- a/arch/arm/mach-omap2/cclock44xx_data.c
> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
> @@ -27,6 +27,7 @@
> #include <linux/clk-private.h>
> #include <linux/clkdev.h>
> #include <linux/io.h>
> +#include <linux/clk/ti.h>
>
> #include "soc.h"
> #include "iomap.h"
> @@ -1697,7 +1698,7 @@ int __init omap4xxx_clk_init(void)
> for (c = omap44xx_clks; c < omap44xx_clks + ARRAY_SIZE(omap44xx_clks);
> c++) {
> if (c->cpu & cpu_clkflg) {
> - clkdev_add(&c->lk);
> + ti_clk_node_add(&c->lk);
> if (!__clk_init(NULL, c->lk.clk))
> omap2_init_clk_hw_omap_clocks(c->lk.clk);
> }
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index 8d15f9a..6cf95160 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -267,7 +267,12 @@ static void __init omap4_init_voltages(void)
>
> static inline void omap_init_cpufreq(void)
> {
> - struct platform_device_info devinfo = { .name = "omap-cpufreq", };
> + struct platform_device_info devinfo = { };
> +
> + if (!of_have_populated_dt())
> + devinfo.name = "omap-cpufreq";
> + else
> + devinfo.name = "cpufreq-cpu0";
> platform_device_register_full(&devinfo);
> }
>
> @@ -301,9 +306,9 @@ int __init omap2_common_pm_late_init(void)
> /* Smartreflex device init */
> omap_devinit_smartreflex();
>
> - /* cpufreq dummy device instantiation */
> - omap_init_cpufreq();
> }
> + /* cpufreq dummy device instantiation */
> + omap_init_cpufreq();
>
> #ifdef CONFIG_SUSPEND
> suspend_set_ops(&omap_pm_ops);
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index a47e6ee..03c6e48 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -55,6 +55,12 @@ config COMMON_CLK_MAX77686
> ---help---
> This driver supports Maxim 77686 crystal oscillator clock.
>
> +config COMMON_CLK_TI
> + tristate "Clock driver for TI SoCs"
> + depends on ARCH_OMAP && OF
> + ---help---
> + Fill me up.. some generic statement ofcourse (lets start with OMAP)
> +
> config CLK_TWL6040
> tristate "External McPDM functional clock from twl6040"
> depends on TWL6040_CORE
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 300d477..9621815 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -30,6 +30,8 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/
>
> obj-$(CONFIG_X86) += x86/
>
> +obj-$(CONFIG_COMMON_CLK_TI) += ti.o
> +
> # Chip specific
> obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
> obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
> diff --git a/drivers/clk/ti.c b/drivers/clk/ti.c
> new file mode 100644
> index 0000000..c747381
> --- /dev/null
> +++ b/drivers/clk/ti.c
> @@ -0,0 +1,100 @@
> +/*
> + * TI Clock node provider
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> + * Nishanth Menon
> + *
> + * 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.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/clk-private.h>
> +#include <linux/clkdev.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/clk/ti.h>
> +
> +struct ti_clk {
> + struct clk_lookup *lk;
> + struct list_head node;
> +};
> +
> +static LIST_HEAD(ti_clk_list);
> +
> +static struct clk *ti_bypass_clk_get_by_name(struct of_phandle_args *clkspec,
> + void *data)
> +{
> + struct ti_clk *c;
> +#if 0
> + /* Eww.. IF ONLY phandle arguments could be string */
> + char clk_name[32]; /* 32 is max size of property name */
> +
> + char *name = clkspec->args[0];
> +
> + snprintf(clk_name, 32, "%s_ck", name);
> + list_for_each_entry(c, &ti_clk_list, node) {
> + int r = strncmp(c->lk->conn_id, clk_name, 32);
> + pr_err("%s: searching %s in %s - %d\n",
> + __func__, clk_name, c->lk->con_id, r);
> + if (!r) {
> + pr_err("%s: found it!\n", __func__);
> + return c->lk.clk;
> + }
> + }
> +#else
> + /* Use integer indexing into clkdev list! Sigh.. */
> + int idx = clkspec->args[0];
> + int cindex = 1;
> +
> + list_for_each_entry(c, &ti_clk_list, node) {
> + int r = (idx == cindex) ? 0 : 1;
> + pr_err("%s: searching index search = %d in %d in %s - %d\n",
> + __func__, idx, cindex, c->lk->con_id, r);
> + if (!r) {
> + pr_err("%s: found it!\n", __func__);
> + return c->lk->clk;
> + }
> + cindex++;
> + }
> +#endif
> +
> + pr_err("%s: ran out of options\n", __func__);
> + return ERR_PTR(-ENODEV);
> +}
> +
> +static void __init ti_clock_init(struct device_node *node)
> +{
> +
> + pr_err("%s: START\n", __func__);
> + of_clk_add_provider(node, ti_bypass_clk_get_by_name, NULL);
> + pr_err("%s: END\n", __func__);
> +}
> +CLK_OF_DECLARE(ti_clk, "ti,clock", ti_clock_init);
> +
> +void __init ti_clk_node_add(struct clk_lookup *lk)
> +{
> + struct ti_clk *c;
> + static bool of_added;
> +
> + c = kzalloc(sizeof(struct ti_clk), GFP_KERNEL);
> + if (!c) {
> + pr_err("%s: No memory!! cannot add clk node!\n", __func__);
> + return;
> + }
> + clkdev_add(lk);
> + c->lk = lk;
> + list_add_tail(&c->node, &ti_clk_list);
> + pr_err("%s: Added clock node %s\n", __func__, lk->con_id);
> + if (!of_added) {
> + of_clk_init(NULL);
> + of_added = true;
> + }
> +};
> +
> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
> new file mode 100644
> index 0000000..eb502a8
> --- /dev/null
> +++ b/include/linux/clk/ti.h
> @@ -0,0 +1,30 @@
> +/*
> + * TI Clock node provider header
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> + * Nishanth Menon
> + *
> + * 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.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#ifndef __TI_CLK_H
> +#define __TI_CLK_H
> +
> +#include <linux/clkdev.h>
> +
> +#ifdef CONFIG_OF
> +extern void ti_clk_node_add(struct clk_lookup *lk);
> +#else
> +static inline void ti_clk_node_add(struct clk_lookup *lk)
> +{
> + clkdev_add(lk);
> +}
> +#endif /* CONFIG_OF */
> +
> +#endif /* __TI_CLK_H */
> --
> 1.7.9.5
>
>
> --
> Regards,
> Nishanth Menon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-04-10 10:56:22

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/10/2013 11:06 AM, Mike Turquette wrote:
> Quoting Nishanth Menon (2013-04-09 13:49:00)
>> On 10:43-20130409, Tony Lindgren wrote:
>>> * Tony Lindgren <[email protected]> [130409 09:54]:
>>>> * Roger Quadros <[email protected]> [130409 03:00]:
>>>>> On 04/05/2013 06:58 PM, Tony Lindgren wrote:
>>>>>>
>>>>>> Can't you just use the clock name there to get it?
>>>>>
>>>>> In device tree we don't pass around clock names. You can either get
>>>>> a phandle or an index to the clock.
>>>>>
>>>>> e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
>>>>
>>>> Yes I understand that. But the driver/clock/omap driver can just
>>>> remap the DT device initially so the board specific clock is
>>>> found from the clock alias table. Basically initially a passthrough
>>>> driver that can be enhanced to parse DT clock bindings and load
>>>> data from /lib/firmware.
>>>
>>> Actually probably the driver/clock/omap can even do even less
>>> initially. There probably even no need to remap clocks there.
>>>
>>> As long as the DT clock driver understands that a board specific
>>> auxclk is specified in the DT it can just call clk_add_alias() so
>>> the driver will get the right auxclk from cclock44xx_data.c.
>>>
>>> Then other features can be added later on like to allocate a
>>> clock entirely based on the binding etc.
>> I did try to have an implementation for cpufreq using clock nodes.
>> unfortunately, device tree wont let me have arguments of strings :(
>> So, I am unable to do clock = <&clk mpu_dpll>;
>> instead, I am forced to do clock = <&clk 249>;
>>
>
> See http://article.gmane.org/gmane.linux.ports.arm.kernel/229034
>

Awesome. Thanks for pointing this out Mike.

Now all we need to do is create a named define for each clock index in the
header file.

cheers,
-roger

>
>> Here is an attempt on beagleXM - adds every clock node to the list.
>> Tons of un-necessary prints added to give an idea - see log:
>> http://pastebin.com/F9A2zSTr
>>
>> Would an cleaned up version be good enough as a step #1 of transition?
>>
>> From 7d373bdb9e9549c1b6ba1775a8dfd96ebe78abfb Mon Sep 17 00:00:00 2001
>> From: Nishanth Menon <[email protected]>
>> Date: Tue, 26 Mar 2013 10:23:27 +0000
>> Subject: [PATCH] OMAP: add devicetree support for clock nodes.
>>
>> Dummy patch based on Roger's original idea
>>
>> Nyet-Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> arch/arm/boot/dts/omap3.dtsi | 5 ++
>> arch/arm/boot/dts/omap34xx.dtsi | 2 +
>> arch/arm/mach-omap2/cclock3xxx_data.c | 3 +-
>> arch/arm/mach-omap2/cclock44xx_data.c | 3 +-
>> arch/arm/mach-omap2/pm.c | 11 +++-
>> drivers/clk/Kconfig | 6 ++
>> drivers/clk/Makefile | 2 +
>> drivers/clk/ti.c | 100 +++++++++++++++++++++++++++++++++
>> include/linux/clk/ti.h | 30 ++++++++++
>> 9 files changed, 157 insertions(+), 5 deletions(-)
>> create mode 100644 drivers/clk/ti.c
>> create mode 100644 include/linux/clk/ti.h
>>
>> diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
>> index 3344f05..a08990d 100644
>> --- a/arch/arm/boot/dts/omap3.dtsi
>> +++ b/arch/arm/boot/dts/omap3.dtsi
>> @@ -73,6 +73,11 @@
>> ti,hwmods = "counter_32k";
>> };
>>
>> + clks: clocks {
>> + compatible = "ti,clock";
>> + #clock-cells = <1>;
>> + };
>> +
>> intc: [email protected] {
>> compatible = "ti,omap2-intc";
>> interrupt-controller;
>> diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
>> index 75ed4ae..93c2621 100644
>> --- a/arch/arm/boot/dts/omap34xx.dtsi
>> +++ b/arch/arm/boot/dts/omap34xx.dtsi
>> @@ -23,6 +23,8 @@
>> 600000 1350000
>> >;
>> clock-latency = <300000>; /* From legacy driver */
>> + clocks = <&clks 249>; /* index to cpufreq_ck */
>> + clock-names = "cpu";
>> };
>> };
>> };
>> diff --git a/arch/arm/mach-omap2/cclock3xxx_data.c b/arch/arm/mach-omap2/cclock3xxx_data.c
>> index 4579c3c..d5d5ef5 100644
>> --- a/arch/arm/mach-omap2/cclock3xxx_data.c
>> +++ b/arch/arm/mach-omap2/cclock3xxx_data.c
>> @@ -22,6 +22,7 @@
>> #include <linux/clk-private.h>
>> #include <linux/list.h>
>> #include <linux/io.h>
>> +#include <linux/clk/ti.h>
>>
>> #include "soc.h"
>> #include "iomap.h"
>> @@ -3574,7 +3575,7 @@ int __init omap3xxx_clk_init(void)
>> for (c = omap3xxx_clks; c < omap3xxx_clks + ARRAY_SIZE(omap3xxx_clks);
>> c++)
>> if (c->cpu & cpu_clkflg) {
>> - clkdev_add(&c->lk);
>> + ti_clk_node_add(&c->lk);
>> if (!__clk_init(NULL, c->lk.clk))
>> omap2_init_clk_hw_omap_clocks(c->lk.clk);
>> }
>> diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c
>> index 0c6834a..338ef64 100644
>> --- a/arch/arm/mach-omap2/cclock44xx_data.c
>> +++ b/arch/arm/mach-omap2/cclock44xx_data.c
>> @@ -27,6 +27,7 @@
>> #include <linux/clk-private.h>
>> #include <linux/clkdev.h>
>> #include <linux/io.h>
>> +#include <linux/clk/ti.h>
>>
>> #include "soc.h"
>> #include "iomap.h"
>> @@ -1697,7 +1698,7 @@ int __init omap4xxx_clk_init(void)
>> for (c = omap44xx_clks; c < omap44xx_clks + ARRAY_SIZE(omap44xx_clks);
>> c++) {
>> if (c->cpu & cpu_clkflg) {
>> - clkdev_add(&c->lk);
>> + ti_clk_node_add(&c->lk);
>> if (!__clk_init(NULL, c->lk.clk))
>> omap2_init_clk_hw_omap_clocks(c->lk.clk);
>> }
>> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
>> index 8d15f9a..6cf95160 100644
>> --- a/arch/arm/mach-omap2/pm.c
>> +++ b/arch/arm/mach-omap2/pm.c
>> @@ -267,7 +267,12 @@ static void __init omap4_init_voltages(void)
>>
>> static inline void omap_init_cpufreq(void)
>> {
>> - struct platform_device_info devinfo = { .name = "omap-cpufreq", };
>> + struct platform_device_info devinfo = { };
>> +
>> + if (!of_have_populated_dt())
>> + devinfo.name = "omap-cpufreq";
>> + else
>> + devinfo.name = "cpufreq-cpu0";
>> platform_device_register_full(&devinfo);
>> }
>>
>> @@ -301,9 +306,9 @@ int __init omap2_common_pm_late_init(void)
>> /* Smartreflex device init */
>> omap_devinit_smartreflex();
>>
>> - /* cpufreq dummy device instantiation */
>> - omap_init_cpufreq();
>> }
>> + /* cpufreq dummy device instantiation */
>> + omap_init_cpufreq();
>>
>> #ifdef CONFIG_SUSPEND
>> suspend_set_ops(&omap_pm_ops);
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index a47e6ee..03c6e48 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -55,6 +55,12 @@ config COMMON_CLK_MAX77686
>> ---help---
>> This driver supports Maxim 77686 crystal oscillator clock.
>>
>> +config COMMON_CLK_TI
>> + tristate "Clock driver for TI SoCs"
>> + depends on ARCH_OMAP && OF
>> + ---help---
>> + Fill me up.. some generic statement ofcourse (lets start with OMAP)
>> +
>> config CLK_TWL6040
>> tristate "External McPDM functional clock from twl6040"
>> depends on TWL6040_CORE
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 300d477..9621815 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -30,6 +30,8 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/
>>
>> obj-$(CONFIG_X86) += x86/
>>
>> +obj-$(CONFIG_COMMON_CLK_TI) += ti.o
>> +
>> # Chip specific
>> obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
>> obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
>> diff --git a/drivers/clk/ti.c b/drivers/clk/ti.c
>> new file mode 100644
>> index 0000000..c747381
>> --- /dev/null
>> +++ b/drivers/clk/ti.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * TI Clock node provider
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
>> + * Nishanth Menon
>> + *
>> + * 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.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/clk-private.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/clk/ti.h>
>> +
>> +struct ti_clk {
>> + struct clk_lookup *lk;
>> + struct list_head node;
>> +};
>> +
>> +static LIST_HEAD(ti_clk_list);
>> +
>> +static struct clk *ti_bypass_clk_get_by_name(struct of_phandle_args *clkspec,
>> + void *data)
>> +{
>> + struct ti_clk *c;
>> +#if 0
>> + /* Eww.. IF ONLY phandle arguments could be string */
>> + char clk_name[32]; /* 32 is max size of property name */
>> +
>> + char *name = clkspec->args[0];
>> +
>> + snprintf(clk_name, 32, "%s_ck", name);
>> + list_for_each_entry(c, &ti_clk_list, node) {
>> + int r = strncmp(c->lk->conn_id, clk_name, 32);
>> + pr_err("%s: searching %s in %s - %d\n",
>> + __func__, clk_name, c->lk->con_id, r);
>> + if (!r) {
>> + pr_err("%s: found it!\n", __func__);
>> + return c->lk.clk;
>> + }
>> + }
>> +#else
>> + /* Use integer indexing into clkdev list! Sigh.. */
>> + int idx = clkspec->args[0];
>> + int cindex = 1;
>> +
>> + list_for_each_entry(c, &ti_clk_list, node) {
>> + int r = (idx == cindex) ? 0 : 1;
>> + pr_err("%s: searching index search = %d in %d in %s - %d\n",
>> + __func__, idx, cindex, c->lk->con_id, r);
>> + if (!r) {
>> + pr_err("%s: found it!\n", __func__);
>> + return c->lk->clk;
>> + }
>> + cindex++;
>> + }
>> +#endif
>> +
>> + pr_err("%s: ran out of options\n", __func__);
>> + return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static void __init ti_clock_init(struct device_node *node)
>> +{
>> +
>> + pr_err("%s: START\n", __func__);
>> + of_clk_add_provider(node, ti_bypass_clk_get_by_name, NULL);
>> + pr_err("%s: END\n", __func__);
>> +}
>> +CLK_OF_DECLARE(ti_clk, "ti,clock", ti_clock_init);
>> +
>> +void __init ti_clk_node_add(struct clk_lookup *lk)
>> +{
>> + struct ti_clk *c;
>> + static bool of_added;
>> +
>> + c = kzalloc(sizeof(struct ti_clk), GFP_KERNEL);
>> + if (!c) {
>> + pr_err("%s: No memory!! cannot add clk node!\n", __func__);
>> + return;
>> + }
>> + clkdev_add(lk);
>> + c->lk = lk;
>> + list_add_tail(&c->node, &ti_clk_list);
>> + pr_err("%s: Added clock node %s\n", __func__, lk->con_id);
>> + if (!of_added) {
>> + of_clk_init(NULL);
>> + of_added = true;
>> + }
>> +};
>> +
>> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
>> new file mode 100644
>> index 0000000..eb502a8
>> --- /dev/null
>> +++ b/include/linux/clk/ti.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * TI Clock node provider header
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
>> + * Nishanth Menon
>> + *
>> + * 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.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +#ifndef __TI_CLK_H
>> +#define __TI_CLK_H
>> +
>> +#include <linux/clkdev.h>
>> +
>> +#ifdef CONFIG_OF
>> +extern void ti_clk_node_add(struct clk_lookup *lk);
>> +#else
>> +static inline void ti_clk_node_add(struct clk_lookup *lk)
>> +{
>> + clkdev_add(lk);
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> +#endif /* __TI_CLK_H */
>> --
>> 1.7.9.5
>>
>>
>> --
>> Regards,
>> Nishanth Menon
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-04-10 11:04:54

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/10/2013 12:54 AM, Nishanth Menon wrote:
> On 15:49-20130409, Nishanth Menon wrote:
>> On 10:43-20130409, Tony Lindgren wrote:
>>> * Tony Lindgren <[email protected]> [130409 09:54]:
>>>> * Roger Quadros <[email protected]> [130409 03:00]:
>>>>> On 04/05/2013 06:58 PM, Tony Lindgren wrote:
>>>>>>
>>>>>> Can't you just use the clock name there to get it?
>>>>>
>>>>> In device tree we don't pass around clock names. You can either get
>>>>> a phandle or an index to the clock.
>>>>>
>>>>> e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
>>>>
>>>> Yes I understand that. But the driver/clock/omap driver can just
>>>> remap the DT device initially so the board specific clock is
>>>> found from the clock alias table. Basically initially a passthrough
>>>> driver that can be enhanced to parse DT clock bindings and load
>>>> data from /lib/firmware.
>>>
>>> Actually probably the driver/clock/omap can even do even less
>>> initially. There probably even no need to remap clocks there.
>>>
>>> As long as the DT clock driver understands that a board specific
>>> auxclk is specified in the DT it can just call clk_add_alias() so
>>> the driver will get the right auxclk from cclock44xx_data.c.
>>>
>>> Then other features can be added later on like to allocate a
>>> clock entirely based on the binding etc.
>> I did try to have an implementation for cpufreq using clock nodes.
>> unfortunately, device tree wont let me have arguments of strings :(
>> So, I am unable to do clock = <&clk mpu_dpll>;
>> instead, I am forced to do clock = <&clk 249>;
>>
>> Here is an attempt on beagleXM - adds every clock node to the list.
>> Tons of un-necessary prints added to give an idea - see log:
>> http://pastebin.com/F9A2zSTr
>>
>> Would an cleaned up version be good enough as a step #1 of transition?
>>
> Approach #2:
> Here is yet another revision -> here I am trying to avoid the risk of
> folks messing up indexing. for example: using an older DTB with newer
> kernel, clocks being inserted into existing list etc. to prevent these,

Why do you need to worry about users using old DTB with new kernel.
That is entirely the user's fault no?

> we add an "DT_ID" for omap clock nodes, and use it to uniquely identify
> the clock node. We try to minimize(not avoidable with integer indexing)
> mistakes during development/productization.

cheers,
-roger

2013-04-10 17:39:39

by Nishanth Menon

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 13:55-20130410, Roger Quadros wrote:
> On 04/10/2013 11:06 AM, Mike Turquette wrote:
> > Quoting Nishanth Menon (2013-04-09 13:49:00)
> >> On 10:43-20130409, Tony Lindgren wrote:
> >>> * Tony Lindgren <[email protected]> [130409 09:54]:
> >>>> * Roger Quadros <[email protected]> [130409 03:00]:
> >>>>> On 04/05/2013 06:58 PM, Tony Lindgren wrote:
> >>>>>>
> >>>>>> Can't you just use the clock name there to get it?
> >>>>>
> >>>>> In device tree we don't pass around clock names. You can either get
> >>>>> a phandle or an index to the clock.
> >>>>>
> >>>>> e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
> >>>>
> >>>> Yes I understand that. But the driver/clock/omap driver can just
> >>>> remap the DT device initially so the board specific clock is
> >>>> found from the clock alias table. Basically initially a passthrough
> >>>> driver that can be enhanced to parse DT clock bindings and load
> >>>> data from /lib/firmware.
> >>>
> >>> Actually probably the driver/clock/omap can even do even less
> >>> initially. There probably even no need to remap clocks there.
> >>>
> >>> As long as the DT clock driver understands that a board specific
> >>> auxclk is specified in the DT it can just call clk_add_alias() so
> >>> the driver will get the right auxclk from cclock44xx_data.c.
> >>>
> >>> Then other features can be added later on like to allocate a
> >>> clock entirely based on the binding etc.
> >> I did try to have an implementation for cpufreq using clock nodes.
> >> unfortunately, device tree wont let me have arguments of strings :(
> >> So, I am unable to do clock = <&clk mpu_dpll>;
> >> instead, I am forced to do clock = <&clk 249>;
> >>
> >
> > See http://article.gmane.org/gmane.linux.ports.arm.kernel/229034
> >
>
> Awesome. Thanks for pointing this out Mike.
>
> Now all we need to do is create a named define for each clock index in the
> header file.
Approach #3: Thanks to Tony for collaborating on this:
Works for cpufreq-cpu0 - additional patches:
http://pastebin.com/GHnTRVJf, http://pastebin.com/FZS89J6L (tested on
beagleXM)
Work for USB - http://pastebin.com/aJpDnXci - thanks Roger for testing
this.
Details in the patch below (Tony, I have added you as collaborator for
helping in getting this working-clk_add_alias was'nt needed in the
internal patch discussion we had - I have taken a bit of freedom in
adding your contributions to the patch below)

Folks, this does seem to be the best compromise we can achieve at this
point in time. feedback on this approach is much appreciated - if folks
are ok, I can post this as an formal patch series.

>From 130a41821bf57081ca45ef654029175d173135e6 Mon Sep 17 00:00:00 2001
From: Nishanth Menon <[email protected]>
Date: Tue, 9 Apr 2013 19:26:40 -0500
Subject: [RFC PATCH] clk: OMAP: introduce device tree binding to kernel clock
data

OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c.
However, this presents an obstacle for using these clock nodes in
Device Tree definitions. There are many possible approaches to this
problem as discussed in the following thread:
http://marc.info/?t=136370325600009&r=1&w=2
Highlights of the options:
a) device specific clk_add_alias:
cons: driver handling required
b) using an generic clk node and indexing to reach the clock required.
This is similar in approach taken by tegra and few other platforms.
example clock = <&clk 5>;
cons: potential to have mismatches in indexed table and associated
dtb data. In addition, managing continued documentation in bindings
as clock indexing increases. Even though readability angle could be
improved by using preprocessing of DT using macros, indexed approach
is inherently risky from cases like the following:
clk indexes in kernel:
1 - mpu_dpll
2 - aux_clk1
3 - core_clk
DT entry for peripheral x uses <&clk 2>, kernel updates to:
1 - mpu_dpll
2 - per_dpll
3 - aux_clk1
4 - core_clk
using the old dtb(or dts missing an update), on new kernel which has
updated indices will result in per_dpll now controlled for peripheral
X without warning or any potential error detection and warning.

Even though we can claim this is user error, such errors are hard to
track down and fix.

An alternate approach introduced here is to introduce device tree bindings
corresponding to the clock nodes required in DT definition for SoC which
automatically maps back to the definitions in cclockXYZ_data.c.

The driver introduced here to do this mapping will eventually be the
place where the clock handling will migrate to. We need to consider this
angle as well so that the solution will be an valid transition point for
moving the clock data out of kernel image (into device tree or firmware load
etc..).

Overall strategy introduced here is simple: an clock node described in
device tree blob is used to identify the exact clock provided in the SoC
specific data. This is then linked back using of_clk_add_provider to the
device node to be accessible by of_clk_get.

Based on discussion contributions from Roger Quadros, Grygorii Strashko
and others.

[[email protected]: co-developed]
Signed-off-by: Tony Lindgren <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
.../devicetree/bindings/clock/omap-clock.txt | 40 +++++++++
drivers/clk/Makefile | 1 +
drivers/clk/omap/Makefile | 1 +
drivers/clk/omap/clk.c | 94 ++++++++++++++++++++
4 files changed, 136 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/omap-clock.txt
create mode 100644 drivers/clk/omap/Makefile
create mode 100644 drivers/clk/omap/clk.c

diff --git a/Documentation/devicetree/bindings/clock/omap-clock.txt b/Documentation/devicetree/bindings/clock/omap-clock.txt
new file mode 100644
index 0000000..07e3ff8
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/omap-clock.txt
@@ -0,0 +1,40 @@
+Device Tree Clock bindings for Texas Instrument's OMAP compatible platforms
+
+This binding is a work-in-progress, and meant to be stage #1 of transitioning
+OMAP clock data out of kernel image.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "ti,omap-clock"
+- #clock-cells : from common clock binding; shall be set to 0.
+NOTE:
+node name should map to clock database in arch/arm/mach-omap2/cclock<SoC>_data.c
+Since all clocks are described with _ck, the node name is optimized to drop the
+usage of _ck. For example, a clock called dpll1_ck will be defined as dpll1.
+
+Example #1: describing clock node for CPU on OMAP34xx platform:
+Ref: arch/arm/mach-omap2/cclock3xxx_data.c
+describes the CPU clock to be as follows
+ CLK(NULL, "dpll1_ck", &dpll1_ck, CK_3XXX),
+Corresponding binding will be:
+ dpll1: dpll1 {
+ #clock-cells = <0>;
+ compatible = "ti,omap-clock";
+ };
+And it's usage will be:
+ clocks = <&dpll1>;
+
+Example #2: describing clock node for auxilary clock #3 on OMAP443x platform:
+Ref: arch/arm/mach-omap2/cclock44xx_data.c
+describes the auxclk3 clock to be as follows:
+ CLK(NULL, "auxclk3_ck", &auxclk3_ck, CK_443X),
+Corresponding binding will be:
+ auxclk3: auxclk3 {
+ #clock-cells = <1>;
+ compatible = "ti,omap-clock";
+ };
+And it's usage will be:
+ clocks = <&auxclk3>;
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 300d477..a0209d7 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_ARCH_U8500) += ux500/
obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
obj-$(CONFIG_ARCH_ZYNQ) += clk-zynq.o
obj-$(CONFIG_ARCH_TEGRA) += tegra/
+obj-$(CONFIG_ARCH_OMAP) += omap/

obj-$(CONFIG_X86) += x86/

diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile
new file mode 100644
index 0000000..8195931
--- /dev/null
+++ b/drivers/clk/omap/Makefile
@@ -0,0 +1 @@
+obj-y += clk.o
diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c
new file mode 100644
index 0000000..63a4cce
--- /dev/null
+++ b/drivers/clk/omap/clk.c
@@ -0,0 +1,94 @@
+/*
+ * Texas Instruments OMAP Clock driver
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Nishanth Menon
+ * Tony Lindgren <[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.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clkdev.h>
+#include <linux/clk-private.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+
+static const struct of_device_id omap_clk_of_match[] = {
+ {.compatible = "ti,omap-clock",},
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, omap_clk_of_match);
+
+/**
+ * omap_clk_probe() - create link from DT definition to clock data
+ * @pdev: device node
+ *
+ * REVISIT: We are now assuming the following:
+ * 1. omap clock names end with _ck
+ * 2. omap clocks don't get removed
+ */
+static int omap_clk_probe(struct platform_device *pdev)
+{
+ struct clk *clk;
+ int res;
+
+ const struct of_device_id *match;
+ struct device_node *np = pdev->dev.of_node;
+ char clk_name[32];
+
+ match = of_match_device(omap_clk_of_match, &pdev->dev);
+
+ /* Set up things so consumer can call clk_get() with name */
+ snprintf(clk_name, 32, "%s_ck", np->name);
+ clk = clk_get(NULL, clk_name);
+ if (IS_ERR(clk)) {
+ res = PTR_ERR(clk);
+ dev_err(&pdev->dev, "could not get clock %s (%d)\n",
+ clk_name, res);
+ goto out;
+ }
+
+ /* This allows the driver to of_clk_get() */
+ res = of_clk_add_provider(np, of_clk_src_simple_get, clk);
+ if (res)
+ dev_err(&pdev->dev, "could not add provider for %s (%d)\n",
+ clk_name, res);
+
+ clk_put(clk);
+out:
+ return res;
+}
+
+MODULE_ALIAS("platform:omap_clk");
+
+/* We assume here that OMAP clocks will not be removed */
+static struct platform_driver omap_clk_driver = {
+ .probe = omap_clk_probe,
+ .driver = {
+ .name = "omap_clk",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(omap_clk_of_match),
+ },
+};
+
+static int __init omap_clk_init(void)
+{
+ return platform_driver_register(&omap_clk_driver);
+}
+
+arch_initcall(omap_clk_init);
+
+MODULE_DESCRIPTION("OMAP Clock driver");
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5

--
Regards,
Nishanth Menon

2013-04-10 18:49:28

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

* Nishanth Menon <[email protected]> [130410 10:44]:
> Details in the patch below (Tony, I have added you as collaborator for
> helping in getting this working-clk_add_alias was'nt needed in the
> internal patch discussion we had - I have taken a bit of freedom in
> adding your contributions to the patch below)

OK thanks. Noticed few minor things, see below.

> Folks, this does seem to be the best compromise we can achieve at this
> point in time. feedback on this approach is much appreciated - if folks
> are ok, I can post this as an formal patch series.
>
> From 130a41821bf57081ca45ef654029175d173135e6 Mon Sep 17 00:00:00 2001
> From: Nishanth Menon <[email protected]>
> Date: Tue, 9 Apr 2013 19:26:40 -0500
> Subject: [RFC PATCH] clk: OMAP: introduce device tree binding to kernel clock
> data
>
> OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c.
> However, this presents an obstacle for using these clock nodes in
> Device Tree definitions. There are many possible approaches to this
> problem as discussed in the following thread:
> http://marc.info/?t=136370325600009&r=1&w=2

It might be worth clarifying that this is especially for the board
specific clocks initially. The fixed clocks are currently found via
the clock aliases table.

> Highlights of the options:
> a) device specific clk_add_alias:
> cons: driver handling required
> b) using an generic clk node and indexing to reach the clock required.
> This is similar in approach taken by tegra and few other platforms.
> example clock = <&clk 5>;
> cons: potential to have mismatches in indexed table and associated
> dtb data. In addition, managing continued documentation in bindings
> as clock indexing increases. Even though readability angle could be
> improved by using preprocessing of DT using macros, indexed approach
> is inherently risky from cases like the following:
> clk indexes in kernel:
> 1 - mpu_dpll
> 2 - aux_clk1
> 3 - core_clk
> DT entry for peripheral x uses <&clk 2>, kernel updates to:
> 1 - mpu_dpll
> 2 - per_dpll
> 3 - aux_clk1
> 4 - core_clk
> using the old dtb(or dts missing an update), on new kernel which has
> updated indices will result in per_dpll now controlled for peripheral
> X without warning or any potential error detection and warning.
>
> Even though we can claim this is user error, such errors are hard to
> track down and fix.
>
> An alternate approach introduced here is to introduce device tree bindings
> corresponding to the clock nodes required in DT definition for SoC which
> automatically maps back to the definitions in cclockXYZ_data.c.
>
> The driver introduced here to do this mapping will eventually be the
> place where the clock handling will migrate to. We need to consider this
> angle as well so that the solution will be an valid transition point for
> moving the clock data out of kernel image (into device tree or firmware load
> etc..).
>
> Overall strategy introduced here is simple: an clock node described in
> device tree blob is used to identify the exact clock provided in the SoC
> specific data. This is then linked back using of_clk_add_provider to the
> device node to be accessible by of_clk_get.
>
> Based on discussion contributions from Roger Quadros, Grygorii Strashko
> and others.
>
> [[email protected]: co-developed]
> Signed-off-by: Tony Lindgren <[email protected]>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> .../devicetree/bindings/clock/omap-clock.txt | 40 +++++++++
> drivers/clk/Makefile | 1 +
> drivers/clk/omap/Makefile | 1 +
> drivers/clk/omap/clk.c | 94 ++++++++++++++++++++
> 4 files changed, 136 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/omap-clock.txt
> create mode 100644 drivers/clk/omap/Makefile
> create mode 100644 drivers/clk/omap/clk.c
>
> diff --git a/Documentation/devicetree/bindings/clock/omap-clock.txt b/Documentation/devicetree/bindings/clock/omap-clock.txt
> new file mode 100644
> index 0000000..07e3ff8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/omap-clock.txt
> @@ -0,0 +1,40 @@
> +Device Tree Clock bindings for Texas Instrument's OMAP compatible platforms
> +
> +This binding is a work-in-progress, and meant to be stage #1 of transitioning
> +OMAP clock data out of kernel image.

As it's using the common clock binding, this will be supported in the
long run too. So maybe replace work-in-progress with initial minimal
binding that can be enhanced later on.

> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "ti,omap-clock"
> +- #clock-cells : from common clock binding; shall be set to 0.
> +NOTE:
> +node name should map to clock database in arch/arm/mach-omap2/cclock<SoC>_data.c
> +Since all clocks are described with _ck, the node name is optimized to drop the
> +usage of _ck. For example, a clock called dpll1_ck will be defined as dpll1.
> +
> +Example #1: describing clock node for CPU on OMAP34xx platform:
> +Ref: arch/arm/mach-omap2/cclock3xxx_data.c
> +describes the CPU clock to be as follows
> + CLK(NULL, "dpll1_ck", &dpll1_ck, CK_3XXX),
> +Corresponding binding will be:
> + dpll1: dpll1 {
> + #clock-cells = <0>;
> + compatible = "ti,omap-clock";
> + };
> +And it's usage will be:
> + clocks = <&dpll1>;
> +
> +Example #2: describing clock node for auxilary clock #3 on OMAP443x platform:
> +Ref: arch/arm/mach-omap2/cclock44xx_data.c
> +describes the auxclk3 clock to be as follows:
> + CLK(NULL, "auxclk3_ck", &auxclk3_ck, CK_443X),
> +Corresponding binding will be:
> + auxclk3: auxclk3 {
> + #clock-cells = <1>;
> + compatible = "ti,omap-clock";
> + };
> +And it's usage will be:
> + clocks = <&auxclk3>;

The #clock-cells in the auxclk3 example should also be 0 instead of 1
AFAIK. We should only use #clock-cells = <1> when the same physical
clock provides multiple outputs. I believe the auxclocks are all separate,
but that needs to be verified.

> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 300d477..a0209d7 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_ARCH_U8500) += ux500/
> obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
> obj-$(CONFIG_ARCH_ZYNQ) += clk-zynq.o
> obj-$(CONFIG_ARCH_TEGRA) += tegra/
> +obj-$(CONFIG_ARCH_OMAP) += omap/
>
> obj-$(CONFIG_X86) += x86/
>
> diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile
> new file mode 100644
> index 0000000..8195931
> --- /dev/null
> +++ b/drivers/clk/omap/Makefile
> @@ -0,0 +1 @@
> +obj-y += clk.o
> diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c
> new file mode 100644
> index 0000000..63a4cce
> --- /dev/null
> +++ b/drivers/clk/omap/clk.c
> @@ -0,0 +1,94 @@
> +/*
> + * Texas Instruments OMAP Clock driver
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> + * Nishanth Menon
> + * Tony Lindgren <[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.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clkdev.h>
> +#include <linux/clk-private.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +
> +static const struct of_device_id omap_clk_of_match[] = {
> + {.compatible = "ti,omap-clock",},
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, omap_clk_of_match);
> +
> +/**
> + * omap_clk_probe() - create link from DT definition to clock data
> + * @pdev: device node
> + *
> + * REVISIT: We are now assuming the following:
> + * 1. omap clock names end with _ck
> + * 2. omap clocks don't get removed
> + */
> +static int omap_clk_probe(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + int res;
> +
> + const struct of_device_id *match;
> + struct device_node *np = pdev->dev.of_node;
> + char clk_name[32];
> +
> + match = of_match_device(omap_clk_of_match, &pdev->dev);
> +
> + /* Set up things so consumer can call clk_get() with name */
> + snprintf(clk_name, 32, "%s_ck", np->name);
> + clk = clk_get(NULL, clk_name);
> + if (IS_ERR(clk)) {
> + res = PTR_ERR(clk);
> + dev_err(&pdev->dev, "could not get clock %s (%d)\n",
> + clk_name, res);
> + goto out;
> + }

It seems that at least for now we can assume the naming will stay
that way, then if we need other rules for finding clocks, we can
add omap_match_clock() function or something.

> + /* This allows the driver to of_clk_get() */
> + res = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> + if (res)
> + dev_err(&pdev->dev, "could not add provider for %s (%d)\n",
> + clk_name, res);
> +
> + clk_put(clk);
> +out:
> + return res;
> +}

We can avoid the concern of storing the struct clk * and do the
look up lazily on consumer driver probe by setting a dummy struct
clk * here. Then replace of_clk_src_simple_get() with a custom
omap_clk_src_get() that does the lookup and replaces the struct
clk * with the real one.

> +MODULE_ALIAS("platform:omap_clk");
> +
> +/* We assume here that OMAP clocks will not be removed */
> +static struct platform_driver omap_clk_driver = {
> + .probe = omap_clk_probe,
> + .driver = {
> + .name = "omap_clk",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(omap_clk_of_match),
> + },
> +};
> +
> +static int __init omap_clk_init(void)
> +{
> + return platform_driver_register(&omap_clk_driver);
> +}
> +
> +arch_initcall(omap_clk_init);
>
> +MODULE_DESCRIPTION("OMAP Clock driver");
> +MODULE_AUTHOR("Texas Instruments Inc.");
> +MODULE_LICENSE("GPL v2");

Regards,

Tony

2013-04-10 19:19:17

by Nishanth Menon

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Hi Tony,
On Wed, Apr 10, 2013 at 1:49 PM, Tony Lindgren <[email protected]> wrote:
> * Nishanth Menon <[email protected]> [130410 10:44]:
>> Details in the patch below (Tony, I have added you as collaborator for
>> helping in getting this working-clk_add_alias was'nt needed in the
>> internal patch discussion we had - I have taken a bit of freedom in
>> adding your contributions to the patch below)
>
> OK thanks. Noticed few minor things, see below.
Thanks on the checkup
>
>> Folks, this does seem to be the best compromise we can achieve at this
>> point in time. feedback on this approach is much appreciated - if folks
>> are ok, I can post this as an formal patch series.
>>
>> From 130a41821bf57081ca45ef654029175d173135e6 Mon Sep 17 00:00:00 2001
>> From: Nishanth Menon <[email protected]>
>> Date: Tue, 9 Apr 2013 19:26:40 -0500
>> Subject: [RFC PATCH] clk: OMAP: introduce device tree binding to kernel clock
>> data
>>
>> OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c.
>> However, this presents an obstacle for using these clock nodes in
>> Device Tree definitions. There are many possible approaches to this
>> problem as discussed in the following thread:
>> http://marc.info/?t=136370325600009&r=1&w=2
>
> It might be worth clarifying that this is especially for the board
> specific clocks initially. The fixed clocks are currently found via
> the clock aliases table.
Ack.
[...]
>
>> +Example #2: describing clock node for auxilary clock #3 on OMAP443x platform:
>> +Ref: arch/arm/mach-omap2/cclock44xx_data.c
>> +describes the auxclk3 clock to be as follows:
>> + CLK(NULL, "auxclk3_ck", &auxclk3_ck, CK_443X),
>> +Corresponding binding will be:
>> + auxclk3: auxclk3 {
>> + #clock-cells = <1>;
>> + compatible = "ti,omap-clock";
>> + };
>> +And it's usage will be:
>> + clocks = <&auxclk3>;
>
> The #clock-cells in the auxclk3 example should also be 0 instead of 1
> AFAIK. We should only use #clock-cells = <1> when the same physical
> clock provides multiple outputs. I believe the auxclocks are all separate,
> but that needs to be verified.
Oops.. my bad. you are correct here - auxclk is single output. all of them.
will fix.
[...]
>> +static int omap_clk_probe(struct platform_device *pdev)
>> +{
>> + struct clk *clk;
>> + int res;
>> +
>> + const struct of_device_id *match;
>> + struct device_node *np = pdev->dev.of_node;
>> + char clk_name[32];
>> +
>> + match = of_match_device(omap_clk_of_match, &pdev->dev);
>> +
>> + /* Set up things so consumer can call clk_get() with name */
>> + snprintf(clk_name, 32, "%s_ck", np->name);
>> + clk = clk_get(NULL, clk_name);
>> + if (IS_ERR(clk)) {
>> + res = PTR_ERR(clk);
>> + dev_err(&pdev->dev, "could not get clock %s (%d)\n",
>> + clk_name, res);
>> + goto out;
>> + }
>
> It seems that at least for now we can assume the naming will stay
> that way, then if we need other rules for finding clocks, we can
> add omap_match_clock() function or something.
ack.
>
>> + /* This allows the driver to of_clk_get() */
>> + res = of_clk_add_provider(np, of_clk_src_simple_get, clk);
>> + if (res)
>> + dev_err(&pdev->dev, "could not add provider for %s (%d)\n",
>> + clk_name, res);
>> +
>> + clk_put(clk);
>> +out:
>> + return res;
>> +}
>
> We can avoid the concern of storing the struct clk * and do the
> look up lazily on consumer driver probe by setting a dummy struct
> clk * here. Then replace of_clk_src_simple_get() with a custom
> omap_clk_src_get() that does the lookup and replaces the struct
> clk * with the real one.
Hmm.. this is interesting. will give it a try. Thanks on the suggestion.

Regards,
Nishanth Menon

2013-04-10 20:21:29

by Tony Lindgren

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

* Nishanth Menon <[email protected]> [130410 12:23]:
> On Wed, Apr 10, 2013 at 1:49 PM, Tony Lindgren <[email protected]> wrote:
> >
> > We can avoid the concern of storing the struct clk * and do the
> > look up lazily on consumer driver probe by setting a dummy struct
> > clk * here. Then replace of_clk_src_simple_get() with a custom
> > omap_clk_src_get() that does the lookup and replaces the struct
> > clk * with the real one.
> Hmm.. this is interesting. will give it a try. Thanks on the suggestion.

Setting the struct clk * to NULL initially might work too, but that
needs to be checked.

Regards,

Tony

2013-04-11 07:49:19

by Roger Quadros

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/10/2013 08:39 PM, Nishanth Menon wrote:
> On 13:55-20130410, Roger Quadros wrote:
>> On 04/10/2013 11:06 AM, Mike Turquette wrote:
>>> Quoting Nishanth Menon (2013-04-09 13:49:00)
>>>> On 10:43-20130409, Tony Lindgren wrote:
>>>>> * Tony Lindgren <[email protected]> [130409 09:54]:
>>>>>> * Roger Quadros <[email protected]> [130409 03:00]:
>>>>>>> On 04/05/2013 06:58 PM, Tony Lindgren wrote:
>>>>>>>>
>>>>>>>> Can't you just use the clock name there to get it?
>>>>>>>
>>>>>>> In device tree we don't pass around clock names. You can either get
>>>>>>> a phandle or an index to the clock.
>>>>>>>
>>>>>>> e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
>>>>>>
>>>>>> Yes I understand that. But the driver/clock/omap driver can just
>>>>>> remap the DT device initially so the board specific clock is
>>>>>> found from the clock alias table. Basically initially a passthrough
>>>>>> driver that can be enhanced to parse DT clock bindings and load
>>>>>> data from /lib/firmware.
>>>>>
>>>>> Actually probably the driver/clock/omap can even do even less
>>>>> initially. There probably even no need to remap clocks there.
>>>>>
>>>>> As long as the DT clock driver understands that a board specific
>>>>> auxclk is specified in the DT it can just call clk_add_alias() so
>>>>> the driver will get the right auxclk from cclock44xx_data.c.
>>>>>
>>>>> Then other features can be added later on like to allocate a
>>>>> clock entirely based on the binding etc.
>>>> I did try to have an implementation for cpufreq using clock nodes.
>>>> unfortunately, device tree wont let me have arguments of strings :(
>>>> So, I am unable to do clock = <&clk mpu_dpll>;
>>>> instead, I am forced to do clock = <&clk 249>;
>>>>
>>>
>>> See http://article.gmane.org/gmane.linux.ports.arm.kernel/229034
>>>
>>
>> Awesome. Thanks for pointing this out Mike.
>>
>> Now all we need to do is create a named define for each clock index in the
>> header file.
> Approach #3: Thanks to Tony for collaborating on this:
> Works for cpufreq-cpu0 - additional patches:
> http://pastebin.com/GHnTRVJf, http://pastebin.com/FZS89J6L (tested on
> beagleXM)
> Work for USB - http://pastebin.com/aJpDnXci - thanks Roger for testing
> this.
> Details in the patch below (Tony, I have added you as collaborator for
> helping in getting this working-clk_add_alias was'nt needed in the
> internal patch discussion we had - I have taken a bit of freedom in
> adding your contributions to the patch below)
>
> Folks, this does seem to be the best compromise we can achieve at this
> point in time. feedback on this approach is much appreciated - if folks
> are ok, I can post this as an formal patch series.

This looks fine to me. Minor comments below.

>
> From 130a41821bf57081ca45ef654029175d173135e6 Mon Sep 17 00:00:00 2001
> From: Nishanth Menon <[email protected]>
> Date: Tue, 9 Apr 2013 19:26:40 -0500
> Subject: [RFC PATCH] clk: OMAP: introduce device tree binding to kernel clock
> data
>
> OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c.
> However, this presents an obstacle for using these clock nodes in
> Device Tree definitions. There are many possible approaches to this
> problem as discussed in the following thread:
> http://marc.info/?t=136370325600009&r=1&w=2
> Highlights of the options:
> a) device specific clk_add_alias:
> cons: driver handling required
> b) using an generic clk node and indexing to reach the clock required.
> This is similar in approach taken by tegra and few other platforms.
> example clock = <&clk 5>;
> cons: potential to have mismatches in indexed table and associated
> dtb data. In addition, managing continued documentation in bindings
> as clock indexing increases. Even though readability angle could be
> improved by using preprocessing of DT using macros, indexed approach
> is inherently risky from cases like the following:
> clk indexes in kernel:
> 1 - mpu_dpll
> 2 - aux_clk1
> 3 - core_clk
> DT entry for peripheral x uses <&clk 2>, kernel updates to:
> 1 - mpu_dpll
> 2 - per_dpll
> 3 - aux_clk1
> 4 - core_clk
> using the old dtb(or dts missing an update), on new kernel which has
> updated indices will result in per_dpll now controlled for peripheral
> X without warning or any potential error detection and warning.
>
> Even though we can claim this is user error, such errors are hard to
> track down and fix.
>
> An alternate approach introduced here is to introduce device tree bindings
> corresponding to the clock nodes required in DT definition for SoC which
> automatically maps back to the definitions in cclockXYZ_data.c.
>
> The driver introduced here to do this mapping will eventually be the
> place where the clock handling will migrate to. We need to consider this
> angle as well so that the solution will be an valid transition point for
> moving the clock data out of kernel image (into device tree or firmware load
> etc..).
>
> Overall strategy introduced here is simple: an clock node described in

typo: "an"->"a"

> device tree blob is used to identify the exact clock provided in the SoC
> specific data. This is then linked back using of_clk_add_provider to the
> device node to be accessible by of_clk_get.
>
> Based on discussion contributions from Roger Quadros, Grygorii Strashko
> and others.
>
> [[email protected]: co-developed]
> Signed-off-by: Tony Lindgren <[email protected]>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> .../devicetree/bindings/clock/omap-clock.txt | 40 +++++++++
> drivers/clk/Makefile | 1 +
> drivers/clk/omap/Makefile | 1 +
> drivers/clk/omap/clk.c | 94 ++++++++++++++++++++
> 4 files changed, 136 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/omap-clock.txt
> create mode 100644 drivers/clk/omap/Makefile
> create mode 100644 drivers/clk/omap/clk.c
>
> diff --git a/Documentation/devicetree/bindings/clock/omap-clock.txt b/Documentation/devicetree/bindings/clock/omap-clock.txt
> new file mode 100644
> index 0000000..07e3ff8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/omap-clock.txt
> @@ -0,0 +1,40 @@
> +Device Tree Clock bindings for Texas Instrument's OMAP compatible platforms
> +
> +This binding is a work-in-progress, and meant to be stage #1 of transitioning
> +OMAP clock data out of kernel image.
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "ti,omap-clock"
> +- #clock-cells : from common clock binding; shall be set to 0.
> +NOTE:
> +node name should map to clock database in arch/arm/mach-omap2/cclock<SoC>_data.c
> +Since all clocks are described with _ck, the node name is optimized to drop the
> +usage of _ck. For example, a clock called dpll1_ck will be defined as dpll1.
> +
> +Example #1: describing clock node for CPU on OMAP34xx platform:
> +Ref: arch/arm/mach-omap2/cclock3xxx_data.c
> +describes the CPU clock to be as follows
> + CLK(NULL, "dpll1_ck", &dpll1_ck, CK_3XXX),
> +Corresponding binding will be:
> + dpll1: dpll1 {
> + #clock-cells = <0>;
> + compatible = "ti,omap-clock";
> + };
> +And it's usage will be:
> + clocks = <&dpll1>;
> +
> +Example #2: describing clock node for auxilary clock #3 on OMAP443x platform:
> +Ref: arch/arm/mach-omap2/cclock44xx_data.c
> +describes the auxclk3 clock to be as follows:
> + CLK(NULL, "auxclk3_ck", &auxclk3_ck, CK_443X),
> +Corresponding binding will be:
> + auxclk3: auxclk3 {
> + #clock-cells = <1>;
> + compatible = "ti,omap-clock";
> + };
> +And it's usage will be:
> + clocks = <&auxclk3>;
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 300d477..a0209d7 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_ARCH_U8500) += ux500/
> obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
> obj-$(CONFIG_ARCH_ZYNQ) += clk-zynq.o
> obj-$(CONFIG_ARCH_TEGRA) += tegra/
> +obj-$(CONFIG_ARCH_OMAP) += omap/
>
> obj-$(CONFIG_X86) += x86/
>
> diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile
> new file mode 100644
> index 0000000..8195931
> --- /dev/null
> +++ b/drivers/clk/omap/Makefile
> @@ -0,0 +1 @@
> +obj-y += clk.o
> diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c
> new file mode 100644
> index 0000000..63a4cce
> --- /dev/null
> +++ b/drivers/clk/omap/clk.c
> @@ -0,0 +1,94 @@
> +/*
> + * Texas Instruments OMAP Clock driver
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> + * Nishanth Menon

missing e-mail id.

> + * Tony Lindgren <[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.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clkdev.h>
> +#include <linux/clk-private.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +
> +static const struct of_device_id omap_clk_of_match[] = {
> + {.compatible = "ti,omap-clock",},
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, omap_clk_of_match);
> +
Is this driver ever going to be a loadable module? If not then you don't need this.

> +/**
> + * omap_clk_probe() - create link from DT definition to clock data
> + * @pdev: device node
> + *
> + * REVISIT: We are now assuming the following:
> + * 1. omap clock names end with _ck
> + * 2. omap clocks don't get removed
> + */
> +static int omap_clk_probe(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + int res;
> +
> + const struct of_device_id *match;
> + struct device_node *np = pdev->dev.of_node;
> + char clk_name[32];
> +
> + match = of_match_device(omap_clk_of_match, &pdev->dev);
> +
> + /* Set up things so consumer can call clk_get() with name */
> + snprintf(clk_name, 32, "%s_ck", np->name);
> + clk = clk_get(NULL, clk_name);
> + if (IS_ERR(clk)) {
> + res = PTR_ERR(clk);
> + dev_err(&pdev->dev, "could not get clock %s (%d)\n",
> + clk_name, res);
> + goto out;
> + }
> +
> + /* This allows the driver to of_clk_get() */
> + res = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> + if (res)
> + dev_err(&pdev->dev, "could not add provider for %s (%d)\n",
> + clk_name, res);
> +
> + clk_put(clk);
> +out:
> + return res;
> +}
> +
> +MODULE_ALIAS("platform:omap_clk");

ditto.

> +
> +/* We assume here that OMAP clocks will not be removed */
> +static struct platform_driver omap_clk_driver = {
> + .probe = omap_clk_probe,
> + .driver = {
> + .name = "omap_clk",
> + .owner = THIS_MODULE,

same here.

> + .of_match_table = of_match_ptr(omap_clk_of_match),
> + },
> +};
> +
> +static int __init omap_clk_init(void)
> +{
> + return platform_driver_register(&omap_clk_driver);
> +}
> +
> +arch_initcall(omap_clk_init);
> +
> +MODULE_DESCRIPTION("OMAP Clock driver");
> +MODULE_AUTHOR("Texas Instruments Inc.");
> +MODULE_LICENSE("GPL v2");
>

cheers,
-roger

2013-04-11 09:05:07

by Grygorii Strashko

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On 04/11/2013 10:48 AM, Roger Quadros wrote:
> On 04/10/2013 08:39 PM, Nishanth Menon wrote:
>> On 13:55-20130410, Roger Quadros wrote:
>>> On 04/10/2013 11:06 AM, Mike Turquette wrote:
>>>> Quoting Nishanth Menon (2013-04-09 13:49:00)
>>>>> On 10:43-20130409, Tony Lindgren wrote:
>>>>>> * Tony Lindgren <[email protected]> [130409 09:54]:
>>>>>>> * Roger Quadros <[email protected]> [130409 03:00]:
>>>>>>>> On 04/05/2013 06:58 PM, Tony Lindgren wrote:
>>>>>>>>> Can't you just use the clock name there to get it?
>>>>>>>> In device tree we don't pass around clock names. You can either get
>>>>>>>> a phandle or an index to the clock.
>>>>>>>>
>>>>>>>> e.g. Documentation/devicetree/bindings/clock/imx31-clock.txt
>>>>>>> Yes I understand that. But the driver/clock/omap driver can just
>>>>>>> remap the DT device initially so the board specific clock is
>>>>>>> found from the clock alias table. Basically initially a passthrough
>>>>>>> driver that can be enhanced to parse DT clock bindings and load
>>>>>>> data from /lib/firmware.
>>>>>> Actually probably the driver/clock/omap can even do even less
>>>>>> initially. There probably even no need to remap clocks there.
>>>>>>
>>>>>> As long as the DT clock driver understands that a board specific
>>>>>> auxclk is specified in the DT it can just call clk_add_alias() so
>>>>>> the driver will get the right auxclk from cclock44xx_data.c.
>>>>>>
>>>>>> Then other features can be added later on like to allocate a
>>>>>> clock entirely based on the binding etc.
>>>>> I did try to have an implementation for cpufreq using clock nodes.
>>>>> unfortunately, device tree wont let me have arguments of strings :(
>>>>> So, I am unable to do clock = <&clk mpu_dpll>;
>>>>> instead, I am forced to do clock = <&clk 249>;
>>>>>
>>>> See http://article.gmane.org/gmane.linux.ports.arm.kernel/229034
>>>>
>>> Awesome. Thanks for pointing this out Mike.
>>>
>>> Now all we need to do is create a named define for each clock index in the
>>> header file.
>> Approach #3: Thanks to Tony for collaborating on this:
>> Works for cpufreq-cpu0 - additional patches:
>> http://pastebin.com/GHnTRVJf, http://pastebin.com/FZS89J6L (tested on
>> beagleXM)
>> Work for USB - http://pastebin.com/aJpDnXci - thanks Roger for testing
>> this.
>> Details in the patch below (Tony, I have added you as collaborator for
>> helping in getting this working-clk_add_alias was'nt needed in the
>> internal patch discussion we had - I have taken a bit of freedom in
>> adding your contributions to the patch below)
>>
>> Folks, this does seem to be the best compromise we can achieve at this
>> point in time. feedback on this approach is much appreciated - if folks
>> are ok, I can post this as an formal patch series.
> This looks fine to me. Minor comments below.

I like it. No IDs and can add clocks support in DT as needed.

>
>> From 130a41821bf57081ca45ef654029175d173135e6 Mon Sep 17 00:00:00 2001
>> From: Nishanth Menon <[email protected]>
>> Date: Tue, 9 Apr 2013 19:26:40 -0500
>> Subject: [RFC PATCH] clk: OMAP: introduce device tree binding to kernel clock
>> data
>>
>> OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c.
>> However, this presents an obstacle for using these clock nodes in
>> Device Tree definitions. There are many possible approaches to this
>> problem as discussed in the following thread:
>> http://marc.info/?t=136370325600009&r=1&w=2
>> Highlights of the options:
>> a) device specific clk_add_alias:
>> cons: driver handling required
>> b) using an generic clk node and indexing to reach the clock required.
>> This is similar in approach taken by tegra and few other platforms.
>> example clock = <&clk 5>;
>> cons: potential to have mismatches in indexed table and associated
>> dtb data. In addition, managing continued documentation in bindings
>> as clock indexing increases. Even though readability angle could be
>> improved by using preprocessing of DT using macros, indexed approach
>> is inherently risky from cases like the following:
>> clk indexes in kernel:
>> 1 - mpu_dpll
>> 2 - aux_clk1
>> 3 - core_clk
>> DT entry for peripheral x uses <&clk 2>, kernel updates to:
>> 1 - mpu_dpll
>> 2 - per_dpll
>> 3 - aux_clk1
>> 4 - core_clk
>> using the old dtb(or dts missing an update), on new kernel which has
>> updated indices will result in per_dpll now controlled for peripheral
>> X without warning or any potential error detection and warning.
>>
>> Even though we can claim this is user error, such errors are hard to
>> track down and fix.
>>
>> An alternate approach introduced here is to introduce device tree bindings
>> corresponding to the clock nodes required in DT definition for SoC which
>> automatically maps back to the definitions in cclockXYZ_data.c.
>>
>> The driver introduced here to do this mapping will eventually be the
>> place where the clock handling will migrate to. We need to consider this
>> angle as well so that the solution will be an valid transition point for
>> moving the clock data out of kernel image (into device tree or firmware load
>> etc..).
>>
>> Overall strategy introduced here is simple: an clock node described in
> typo: "an"->"a"
>
>> device tree blob is used to identify the exact clock provided in the SoC
>> specific data. This is then linked back using of_clk_add_provider to the
>> device node to be accessible by of_clk_get.
>>
>> Based on discussion contributions from Roger Quadros, Grygorii Strashko
>> and others.
>>
>> [[email protected]: co-developed]
>> Signed-off-by: Tony Lindgren <[email protected]>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> .../devicetree/bindings/clock/omap-clock.txt | 40 +++++++++
>> drivers/clk/Makefile | 1 +
>> drivers/clk/omap/Makefile | 1 +
>> drivers/clk/omap/clk.c | 94 ++++++++++++++++++++
>> 4 files changed, 136 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/omap-clock.txt
>> create mode 100644 drivers/clk/omap/Makefile
>> create mode 100644 drivers/clk/omap/clk.c
>>
>> diff --git a/Documentation/devicetree/bindings/clock/omap-clock.txt b/Documentation/devicetree/bindings/clock/omap-clock.txt
>> new file mode 100644
>> index 0000000..07e3ff8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/omap-clock.txt
>> @@ -0,0 +1,40 @@
>> +Device Tree Clock bindings for Texas Instrument's OMAP compatible platforms
>> +
>> +This binding is a work-in-progress, and meant to be stage #1 of transitioning
>> +OMAP clock data out of kernel image.
>> +
>> +This binding uses the common clock binding[1].
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible : shall be "ti,omap-clock"
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +NOTE:
>> +node name should map to clock database in arch/arm/mach-omap2/cclock<SoC>_data.c
>> +Since all clocks are described with _ck, the node name is optimized to drop the
>> +usage of _ck. For example, a clock called dpll1_ck will be defined as dpll1.
>> +
>> +Example #1: describing clock node for CPU on OMAP34xx platform:
>> +Ref: arch/arm/mach-omap2/cclock3xxx_data.c
>> +describes the CPU clock to be as follows
>> + CLK(NULL, "dpll1_ck", &dpll1_ck, CK_3XXX),
>> +Corresponding binding will be:
>> + dpll1: dpll1 {
>> + #clock-cells = <0>;
>> + compatible = "ti,omap-clock";
>> + };
>> +And it's usage will be:
>> + clocks = <&dpll1>;
>> +
>> +Example #2: describing clock node for auxilary clock #3 on OMAP443x platform:
>> +Ref: arch/arm/mach-omap2/cclock44xx_data.c
>> +describes the auxclk3 clock to be as follows:
>> + CLK(NULL, "auxclk3_ck", &auxclk3_ck, CK_443X),
>> +Corresponding binding will be:
>> + auxclk3: auxclk3 {
>> + #clock-cells = <1>;
>> + compatible = "ti,omap-clock";
>> + };
>> +And it's usage will be:
>> + clocks = <&auxclk3>;
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 300d477..a0209d7 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -27,6 +27,7 @@ obj-$(CONFIG_ARCH_U8500) += ux500/
>> obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
>> obj-$(CONFIG_ARCH_ZYNQ) += clk-zynq.o
>> obj-$(CONFIG_ARCH_TEGRA) += tegra/
>> +obj-$(CONFIG_ARCH_OMAP) += omap/
>>
>> obj-$(CONFIG_X86) += x86/
>>
>> diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile
>> new file mode 100644
>> index 0000000..8195931
>> --- /dev/null
>> +++ b/drivers/clk/omap/Makefile
>> @@ -0,0 +1 @@
>> +obj-y += clk.o
>> diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c
>> new file mode 100644
>> index 0000000..63a4cce
>> --- /dev/null
>> +++ b/drivers/clk/omap/clk.c
>> @@ -0,0 +1,94 @@
>> +/*
>> + * Texas Instruments OMAP Clock driver
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
>> + * Nishanth Menon
> missing e-mail id.
>
>> + * Tony Lindgren <[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.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clkdev.h>
>> +#include <linux/clk-private.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/string.h>
>> +
>> +static const struct of_device_id omap_clk_of_match[] = {
>> + {.compatible = "ti,omap-clock",},
>> + {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, omap_clk_of_match);
>> +
> Is this driver ever going to be a loadable module? If not then you don't need this.
>
>> +/**
>> + * omap_clk_probe() - create link from DT definition to clock data
>> + * @pdev: device node
>> + *
>> + * REVISIT: We are now assuming the following:
>> + * 1. omap clock names end with _ck
>> + * 2. omap clocks don't get removed
>> + */
>> +static int omap_clk_probe(struct platform_device *pdev)
>> +{
>> + struct clk *clk;
>> + int res;
>> +
>> + const struct of_device_id *match;
>> + struct device_node *np = pdev->dev.of_node;
>> + char clk_name[32];
>> +
>> + match = of_match_device(omap_clk_of_match, &pdev->dev);
>> +
>> + /* Set up things so consumer can call clk_get() with name */
>> + snprintf(clk_name, 32, "%s_ck", np->name);
>> + clk = clk_get(NULL, clk_name);
>> + if (IS_ERR(clk)) {
>> + res = PTR_ERR(clk);
>> + dev_err(&pdev->dev, "could not get clock %s (%d)\n",
>> + clk_name, res);
>> + goto out;
>> + }
>> +
>> + /* This allows the driver to of_clk_get() */
>> + res = of_clk_add_provider(np, of_clk_src_simple_get, clk);
>> + if (res)
>> + dev_err(&pdev->dev, "could not add provider for %s (%d)\n",
>> + clk_name, res);
>> +
>> + clk_put(clk);
>> +out:
>> + return res;
>> +}
>> +
>> +MODULE_ALIAS("platform:omap_clk");
> ditto.
>
>> +
>> +/* We assume here that OMAP clocks will not be removed */
>> +static struct platform_driver omap_clk_driver = {
>> + .probe = omap_clk_probe,
>> + .driver = {
>> + .name = "omap_clk",
>> + .owner = THIS_MODULE,
> same here.
>
>> + .of_match_table = of_match_ptr(omap_clk_of_match),
>> + },
>> +};
>> +
>> +static int __init omap_clk_init(void)
>> +{
>> + return platform_driver_register(&omap_clk_driver);
>> +}
>> +
>> +arch_initcall(omap_clk_init);
>> +
>> +MODULE_DESCRIPTION("OMAP Clock driver");
>> +MODULE_AUTHOR("Texas Instruments Inc.");
>> +MODULE_LICENSE("GPL v2");
>>
> cheers,
> -roger

-grygorii

2013-04-11 18:46:56

by Mike Turquette

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Quoting Nishanth Menon (2013-04-10 10:39:21)
> diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c
> new file mode 100644
> index 0000000..63a4cce
> --- /dev/null
> +++ b/drivers/clk/omap/clk.c
> @@ -0,0 +1,94 @@
> +/*
> + * Texas Instruments OMAP Clock driver
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> + * Nishanth Menon
> + * Tony Lindgren <[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.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clkdev.h>
> +#include <linux/clk-private.h>

Please use clk-provider.h. Otherwise this looks like an OK transitional
solution. Hopefully this will be replaced with a more legitimate clock
driver for 3.11.

Regards,
Mike

2013-04-11 22:41:02

by Nishanth Menon

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On Thu, Apr 11, 2013 at 1:46 PM, Mike Turquette <[email protected]> wrote:
> Quoting Nishanth Menon (2013-04-10 10:39:21)
>> diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c
>> new file mode 100644
>> index 0000000..63a4cce
>> --- /dev/null
>> +++ b/drivers/clk/omap/clk.c
>> @@ -0,0 +1,94 @@
>> +/*
>> + * Texas Instruments OMAP Clock driver
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
>> + * Nishanth Menon
>> + * Tony Lindgren <[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.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clkdev.h>
>> +#include <linux/clk-private.h>
>
> Please use clk-provider.h. Otherwise this looks like an OK transitional
k. thanks for checking up on this. will update.
> solution. Hopefully this will be replaced with a more legitimate clock
> driver for 3.11.
I hope so too. At least we should start debate with the direction we'd
like to take and start migrating towards that purpose.

Regards,
Nishanth Menon

2013-04-11 22:45:12

by Nishanth Menon

[permalink] [raw]
Subject: Re: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

On Thu, Apr 11, 2013 at 2:48 AM, Roger Quadros <[email protected]> wrote:
> On 04/10/2013 08:39 PM, Nishanth Menon wrote:
>> On 13:55-20130410, Roger Quadros wrote:
>>> On 04/10/2013 11:06 AM, Mike Turquette wrote:
>>>> Quoting Nishanth Menon (2013-04-09 13:49:00)
>> Folks, this does seem to be the best compromise we can achieve at this
>> point in time. feedback on this approach is much appreciated - if folks
>> are ok, I can post this as an formal patch series.
>
> This looks fine to me. Minor comments below.
Thanks on the review. will fix in my next post
Regards,
Nishanth Menon