diff mbox

[RFC,1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs

Message ID 1363703220-4777-2-git-send-email-rogerq@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Quadros March 19, 2013, 2:26 p.m. UTC
Register a device tree clock provider for AUX clocks
on the OMAP4 SoC. Also provide the binding information.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 .../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

Comments

Roger Quadros March 19, 2013, 2:30 p.m. UTC | #1
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 <rogerq@ti.com>
> ---
>  .../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: pinmux@4a100040 {
>  			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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajendra Nayak March 21, 2013, 1:08 p.m. UTC | #2
[]..

> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros March 21, 2013, 1:54 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajendra Nayak March 21, 2013, 2:04 p.m. UTC | #4
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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros March 21, 2013, 2:07 p.m. UTC | #5
+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 <rogerq@ti.com>
> ---
>  .../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: pinmux@4a100040 {
>  			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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros March 21, 2013, 2:11 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren April 3, 2013, 11:42 p.m. UTC | #7
Hi,

* Roger Quadros <rogerq@ti.com> [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 <rogerq@ti.com>
> ---
>  .../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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajendra Nayak April 4, 2013, 5:20 a.m. UTC | #8
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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros April 4, 2013, 7:35 a.m. UTC | #9
Hi,

On 04/04/2013 02:42 AM, Tony Lindgren wrote:
> Hi,
> 
> * Roger Quadros <rogerq@ti.com> [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 <rogerq@ti.com>
>> ---
>>  .../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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren April 4, 2013, 4:33 p.m. UTC | #10
* Rajendra Nayak <rnayak@ti.com> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren April 4, 2013, 4:41 p.m. UTC | #11
* Roger Quadros <rogerq@ti.com> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros April 5, 2013, 10:39 a.m. UTC | #12
On 04/04/2013 07:41 PM, Tony Lindgren wrote:
> * Roger Quadros <rogerq@ti.com> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren April 5, 2013, 3:58 p.m. UTC | #13
* Roger Quadros <rogerq@ti.com> [130405 03:44]:
> On 04/04/2013 07:41 PM, Tony Lindgren wrote:
> > * Roger Quadros <rogerq@ti.com> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko April 5, 2013, 5:56 p.m. UTC | #14
On 04/04/2013 07:41 PM, Tony Lindgren wrote:
> * Roger Quadros <rogerq@ti.com> [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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros April 9, 2013, 9:55 a.m. UTC | #15
On 04/05/2013 06:58 PM, Tony Lindgren wrote:
> * Roger Quadros <rogerq@ti.com> [130405 03:44]:
>> On 04/04/2013 07:41 PM, Tony Lindgren wrote:
>>> * Roger Quadros <rogerq@ti.com> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros April 9, 2013, 10:16 a.m. UTC | #16
On 04/05/2013 08:56 PM, Grygorii Strashko wrote:
> On 04/04/2013 07:41 PM, Tony Lindgren wrote:
>> * Roger Quadros <rogerq@ti.com> [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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren April 9, 2013, 4:49 p.m. UTC | #17
* Roger Quadros <rogerq@ti.com> [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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren April 9, 2013, 5:43 p.m. UTC | #18
* Tony Lindgren <tony@atomide.com> [130409 09:54]:
> * Roger Quadros <rogerq@ti.com> [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 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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: pinmux@4a100040 {
 			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