[PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
add it to tegra20.dtsi, and configure it for the board in tegra-
seaboard.dts.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2:
* Remove clock-names, clock-output-names properties; Tegra will solely
  use integer IDs for clocks in DT.
* Fixed typo in compatible flag
* Resolve FIXME re: multiple clocks with the same "reset ID"; give each
  unique clock an ID, and ignore the reset bits, since this is purely a
  clock binding. Code (e.g. U-Boot) that wants to use this to determine
  CAR reset bit numbers would need a table to convert from the clock IDs
  in this binding to the related reset bit number, if any. In general, I
  think that's true, and the U-Boot code that handles "peripheral IDs"
  should be reworked to handle "clocks", the peripheral clocks being a
  subset of all clocks.
* Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
  etc.
* Separate tegra-seaboard.dts usage example into a separate patch.

This patch semantically relies on Grant Likely's common clock binding patch
series. Once that's finalized, this patch could be checked into the kernel
provided there are no relevant changes to Grant's patches. I believe that
Simon Glass wants to start using this within U-Boot ASAP though.
---
 .../bindings/clock/nvidia,tegra20-car.txt          |  164 ++++++++++++++++++++
 arch/arm/boot/dts/tegra20.dtsi                     |    6 +
 2 files changed, 170 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt

diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
new file mode 100644
index 0000000..acce2d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
@@ -0,0 +1,164 @@
+NVIDIA Tegra20 Clock And Reset Controller
+
+This binding uses the common clock binding:
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+The CAR (Clock And Reset) Controller on Tegra is the HW module responsible
+for muxing and gating Tegra's clocks, and setting their rates.
+
+Required properties :
+- compatible : Should be "nvidia,<chip>-car"
+- reg : Should contain CAR registers location and length
+- clocks : Should contain phandle and clock specifiers for two clocks:
+  the 32 KHz "32k_in", and the board-specific oscillator "osc".
+- #clock-cells : Should be 1.
+  In clock consumers, this cell represents the clock ID exposed by the CAR.
+
+  The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
+  registers. Later, subsequent IDs will be assigned to all other clocks the
+  CAR controls; mainly the PLLs.
+
+    0	osc
+    1	clk_32k		a/k/a clk_s
+    2	clk_m
+    3	sclk
+    4	cclk
+    5	hclk
+    6	pclk
+    7	blink
+    8	pll_a
+    9	pll_a_out0
+    10	pll_c
+    11	pll_c_out1
+    12	pll_d
+    13	pll_e
+    14	pll_m
+    15	pll_m_out1
+    16	pll_p
+    17	pll_p_out1
+    18	pll_p_out2
+    19	pll_p_out3
+    20	pll_p_out4
+    21	pll_s
+    22	pll_u
+    23	pll_x
+    24	audio		a/k/a audio_sync_clk
+    25	audio_2x	a/k/a audio_2x_sync_clk
+    26	cpu
+    27	cop		a/k/a avp
+    28	ac97
+    29	rtc
+    30	tmr
+    31	uart1
+    32	uart2
+    33	gpio
+    34	sdmmc2
+    35	spdif_out
+    36	spdif_in
+    37	i2s1
+    38	i2c1
+    39	ndflash
+    40	sdmmc1
+    41	sdmmc4
+    42	twc
+    43	pwm
+    44	i2s2
+    45	epp
+    46	vi
+    47	vi_sensor
+    48	2d
+    49	usbd
+    50	isp
+    51	3d
+    52	ide
+    53	disp2
+    54	disp1
+    55	host1x
+    56	vcp
+    57	cache2
+    58	mem
+    59	ahbdma
+    60	apbdma
+    61	kbc
+    62	stat_mon
+    63	pmc
+    64	fuse
+    65	kfuse
+    66	sbc1
+    67	snor
+    68	spi
+    69	sbc2
+    70	xio
+    71	sbc3
+    72	dvc
+    73	dsi
+    74	cve
+    75	tvo
+    76	mipi
+    77	hdmi
+    78	csi
+    79	tvdac
+    80	i2c2
+    81	uart3
+    82	emc
+    83	usb2
+    84	usb3
+    85	mpe
+    86	vde
+    87	bsea
+    88	bsev
+    89	speedo
+    90	uart4
+    91	uart5
+    92	i2c3
+    93	sbc4
+    94	sdmmc3
+    95	pcie
+    96	owr
+    97	afi
+    98	csite
+    99	avpucq
+    100	la
+    101	irama
+    102	iramb
+    103	iramc
+    104	iramd
+    105	cram2
+    106	clk_d
+    107	sus
+    108	cdev1
+    109	cdev2
+
+Example:
+
+clocks {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	clk_32k: clock@0 {
+		compatible = "fixed-clock";
+		reg = <0>;
+		#clock-cells = <0>;
+		clock-frequency = <32768>;
+	};
+
+	osc: clock@1 {
+		compatible = "fixed-clock";
+		reg = <1>;
+		#clock-cells = <0>;
+		clock-frequency = <12000000>;
+	};
+};
+
+tegra_car: clock@60006000 {
+	compatible = "nvidia,tegra20-car";
+	reg = <0x60006000 1000>;
+	clocks = <&clk_32k> <&osc>;
+	clock-names = "32k_in", "osc";
+	#clock-cells = <1>;
+};
+
+usb@c5004000 {
+	...
+	clocks = <&tegra_car 58>; /* usb2 */
+};
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 3da7afd..8208660 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -4,6 +4,12 @@
 	compatible = "nvidia,tegra20";
 	interrupt-parent = <&intc>;
 
+	tegra_car: clock@60006000 {
+		compatible = "nvidia,tegra20-car";
+		reg = <0x60006000 1000>;
+		#clock-cells = <1>;
+	};
+
 	intc: interrupt-controller@50041000 {
 		compatible = "arm,cortex-a9-gic";
 		interrupt-controller;
-- 
1.7.0.4

[U-Boot] [PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
add it to tegra20.dtsi, and configure it for the board in tegra-
seaboard.dts.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2:
* Remove clock-names, clock-output-names properties; Tegra will solely
  use integer IDs for clocks in DT.
* Fixed typo in compatible flag
* Resolve FIXME re: multiple clocks with the same "reset ID"; give each
  unique clock an ID, and ignore the reset bits, since this is purely a
  clock binding. Code (e.g. U-Boot) that wants to use this to determine
  CAR reset bit numbers would need a table to convert from the clock IDs
  in this binding to the related reset bit number, if any. In general, I
  think that's true, and the U-Boot code that handles "peripheral IDs"
  should be reworked to handle "clocks", the peripheral clocks being a
  subset of all clocks.
* Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
  etc.
* Separate tegra-seaboard.dts usage example into a separate patch.

This patch semantically relies on Grant Likely's common clock binding patch
series. Once that's finalized, this patch could be checked into the kernel
provided there are no relevant changes to Grant's patches. I believe that
Simon Glass wants to start using this within U-Boot ASAP though.
---
 .../bindings/clock/nvidia,tegra20-car.txt          |  164 ++++++++++++++++++++
 arch/arm/boot/dts/tegra20.dtsi                     |    6 +
 2 files changed, 170 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt

diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
new file mode 100644
index 0000000..acce2d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
@@ -0,0 +1,164 @@
+NVIDIA Tegra20 Clock And Reset Controller
+
+This binding uses the common clock binding:
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+The CAR (Clock And Reset) Controller on Tegra is the HW module responsible
+for muxing and gating Tegra's clocks, and setting their rates.
+
+Required properties :
+- compatible : Should be "nvidia,<chip>-car"
+- reg : Should contain CAR registers location and length
+- clocks : Should contain phandle and clock specifiers for two clocks:
+  the 32 KHz "32k_in", and the board-specific oscillator "osc".
+- #clock-cells : Should be 1.
+  In clock consumers, this cell represents the clock ID exposed by the CAR.
+
+  The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
+  registers. Later, subsequent IDs will be assigned to all other clocks the
+  CAR controls; mainly the PLLs.
+
+    0	osc
+    1	clk_32k		a/k/a clk_s
+    2	clk_m
+    3	sclk
+    4	cclk
+    5	hclk
+    6	pclk
+    7	blink
+    8	pll_a
+    9	pll_a_out0
+    10	pll_c
+    11	pll_c_out1
+    12	pll_d
+    13	pll_e
+    14	pll_m
+    15	pll_m_out1
+    16	pll_p
+    17	pll_p_out1
+    18	pll_p_out2
+    19	pll_p_out3
+    20	pll_p_out4
+    21	pll_s
+    22	pll_u
+    23	pll_x
+    24	audio		a/k/a audio_sync_clk
+    25	audio_2x	a/k/a audio_2x_sync_clk
+    26	cpu
+    27	cop		a/k/a avp
+    28	ac97
+    29	rtc
+    30	tmr
+    31	uart1
+    32	uart2
+    33	gpio
+    34	sdmmc2
+    35	spdif_out
+    36	spdif_in
+    37	i2s1
+    38	i2c1
+    39	ndflash
+    40	sdmmc1
+    41	sdmmc4
+    42	twc
+    43	pwm
+    44	i2s2
+    45	epp
+    46	vi
+    47	vi_sensor
+    48	2d
+    49	usbd
+    50	isp
+    51	3d
+    52	ide
+    53	disp2
+    54	disp1
+    55	host1x
+    56	vcp
+    57	cache2
+    58	mem
+    59	ahbdma
+    60	apbdma
+    61	kbc
+    62	stat_mon
+    63	pmc
+    64	fuse
+    65	kfuse
+    66	sbc1
+    67	snor
+    68	spi
+    69	sbc2
+    70	xio
+    71	sbc3
+    72	dvc
+    73	dsi
+    74	cve
+    75	tvo
+    76	mipi
+    77	hdmi
+    78	csi
+    79	tvdac
+    80	i2c2
+    81	uart3
+    82	emc
+    83	usb2
+    84	usb3
+    85	mpe
+    86	vde
+    87	bsea
+    88	bsev
+    89	speedo
+    90	uart4
+    91	uart5
+    92	i2c3
+    93	sbc4
+    94	sdmmc3
+    95	pcie
+    96	owr
+    97	afi
+    98	csite
+    99	avpucq
+    100	la
+    101	irama
+    102	iramb
+    103	iramc
+    104	iramd
+    105	cram2
+    106	clk_d
+    107	sus
+    108	cdev1
+    109	cdev2
+
+Example:
+
+clocks {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	clk_32k: clock at 0 {
+		compatible = "fixed-clock";
+		reg = <0>;
+		#clock-cells = <0>;
+		clock-frequency = <32768>;
+	};
+
+	osc: clock at 1 {
+		compatible = "fixed-clock";
+		reg = <1>;
+		#clock-cells = <0>;
+		clock-frequency = <12000000>;
+	};
+};
+
+tegra_car: clock at 60006000 {
+	compatible = "nvidia,tegra20-car";
+	reg = <0x60006000 1000>;
+	clocks = <&clk_32k> <&osc>;
+	clock-names = "32k_in", "osc";
+	#clock-cells = <1>;
+};
+
+usb at c5004000 {
+	...
+	clocks = <&tegra_car 58>; /* usb2 */
+};
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 3da7afd..8208660 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -4,6 +4,12 @@
 	compatible = "nvidia,tegra20";
 	interrupt-parent = <&intc>;
 
+	tegra_car: clock at 60006000 {
+		compatible = "nvidia,tegra20-car";
+		reg = <0x60006000 1000>;
+		#clock-cells = <1>;
+	};
+
 	intc: interrupt-controller at 50041000 {
 		compatible = "arm,cortex-a9-gic";
 		interrupt-controller;
-- 
1.7.0.4

[PATCH 2/2] ARM: tegra: Add clock binding to tegra-seabaord.dts

[Stephen Warren]

This provides an example of how to use the Tegra CAR clock binding in a
board .dts file.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
I think it makes sense to check the previous patch into the Linux kernel,
since it's mainly just defining what the binding is. However, this patch
is just an example and needs to be reworked to cover all boards, and add
a PMU node for the true clk_32k source. I'm providing this patch mostly
as an example that Simon Glass can use within U-Boot in the interim.
---
 arch/arm/boot/dts/tegra-seaboard.dts |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
index b55a02e..06a3cb1 100644
--- a/arch/arm/boot/dts/tegra-seaboard.dts
+++ b/arch/arm/boot/dts/tegra-seaboard.dts
@@ -11,6 +11,30 @@
 		reg = < 0x00000000 0x40000000 >;
 	};
 
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/* This will eventually be a clock output from a PMU IC */
+		clk_32k: clock@0 {
+			compatible = "fixed-clock";
+			reg = <0>;
+			#clock-cells = <0>;
+			clock-frequency = <32768>;
+		};
+
+		osc: clock@1 {
+			compatible = "fixed-clock";
+			reg = <1>;
+			#clock-cells = <0>;
+			clock-frequency = <12000000>;
+		};
+	};
+
+	clock@60006000 {
+		clocks = <&clk_32k> <&osc>;
+	};
+
 	i2c@7000c000 {
 		clock-frequency = <400000>;
 	};
-- 
1.7.0.4

[U-Boot] [PATCH 2/2] ARM: tegra: Add clock binding to tegra-seabaord.dts

[Stephen Warren]

This provides an example of how to use the Tegra CAR clock binding in a
board .dts file.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
I think it makes sense to check the previous patch into the Linux kernel,
since it's mainly just defining what the binding is. However, this patch
is just an example and needs to be reworked to cover all boards, and add
a PMU node for the true clk_32k source. I'm providing this patch mostly
as an example that Simon Glass can use within U-Boot in the interim.
---
 arch/arm/boot/dts/tegra-seaboard.dts |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
index b55a02e..06a3cb1 100644
--- a/arch/arm/boot/dts/tegra-seaboard.dts
+++ b/arch/arm/boot/dts/tegra-seaboard.dts
@@ -11,6 +11,30 @@
 		reg = < 0x00000000 0x40000000 >;
 	};
 
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/* This will eventually be a clock output from a PMU IC */
+		clk_32k: clock at 0 {
+			compatible = "fixed-clock";
+			reg = <0>;
+			#clock-cells = <0>;
+			clock-frequency = <32768>;
+		};
+
+		osc: clock at 1 {
+			compatible = "fixed-clock";
+			reg = <1>;
+			#clock-cells = <0>;
+			clock-frequency = <12000000>;
+		};
+	};
+
+	clock at 60006000 {
+		clocks = <&clk_32k> <&osc>;
+	};
+
 	i2c at 7000c000 {
 		clock-frequency = <400000>;
 	};
-- 
1.7.0.4

Re: [PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Simon Glass]

Hi Stephen,

On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
> add it to tegra20.dtsi, and configure it for the board in tegra-
> seaboard.dts.
>
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2:
> * Remove clock-names, clock-output-names properties; Tegra will solely
>  use integer IDs for clocks in DT.
> * Fixed typo in compatible flag
> * Resolve FIXME re: multiple clocks with the same "reset ID"; give each
>  unique clock an ID, and ignore the reset bits, since this is purely a
>  clock binding. Code (e.g. U-Boot) that wants to use this to determine
>  CAR reset bit numbers would need a table to convert from the clock IDs
>  in this binding to the related reset bit number, if any. In general, I
>  think that's true, and the U-Boot code that handles "peripheral IDs"
>  should be reworked to handle "clocks", the peripheral clocks being a
>  subset of all clocks.

The clock enable and reset enable bits use the same numbering. I think
you have invented a third which is an arbitrary number which doesn't
correspond to anything in hardware. That makes sense for pin mux
function perhaps, since the indirection provides a useful concept of
function number, but here I can't see the benefit.

> * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
>  etc.
> * Separate tegra-seaboard.dts usage example into a separate patch.
>
> This patch semantically relies on Grant Likely's common clock binding patch
> series. Once that's finalized, this patch could be checked into the kernel
> provided there are no relevant changes to Grant's patches. I believe that
> Simon Glass wants to start using this within U-Boot ASAP though.

As I may have said I am really not keen on the idea of having a table
just to use it in U-Boot.

> ---
>  .../bindings/clock/nvidia,tegra20-car.txt          |  164 ++++++++++++++++++++
>  arch/arm/boot/dts/tegra20.dtsi                     |    6 +
>  2 files changed, 170 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> new file mode 100644
> index 0000000..acce2d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> @@ -0,0 +1,164 @@
> +NVIDIA Tegra20 Clock And Reset Controller
> +
> +This binding uses the common clock binding:
> +Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +The CAR (Clock And Reset) Controller on Tegra is the HW module responsible
> +for muxing and gating Tegra's clocks, and setting their rates.
> +
> +Required properties :
> +- compatible : Should be "nvidia,<chip>-car"
> +- reg : Should contain CAR registers location and length
> +- clocks : Should contain phandle and clock specifiers for two clocks:
> +  the 32 KHz "32k_in", and the board-specific oscillator "osc".
> +- #clock-cells : Should be 1.
> +  In clock consumers, this cell represents the clock ID exposed by the CAR.
> +
> +  The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
> +  registers. Later, subsequent IDs will be assigned to all other clocks the
> +  CAR controls; mainly the PLLs.

Are you sure? The ordering doesn't seem to fit with U-Boot's enum
periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file
follows along with the hardware.

> +
> +    0  osc
> +    1  clk_32k         a/k/a clk_s
> +    2  clk_m
> +    3  sclk
> +    4  cclk
> +    5  hclk
> +    6  pclk
> +    7  blink
> +    8  pll_a
> +    9  pll_a_out0
> +    10 pll_c
> +    11 pll_c_out1
> +    12 pll_d
> +    13 pll_e
> +    14 pll_m
> +    15 pll_m_out1
> +    16 pll_p
> +    17 pll_p_out1
> +    18 pll_p_out2
> +    19 pll_p_out3
> +    20 pll_p_out4
> +    21 pll_s
> +    22 pll_u
> +    23 pll_x
> +    24 audio           a/k/a audio_sync_clk
> +    25 audio_2x        a/k/a audio_2x_sync_clk
> +    26 cpu
> +    27 cop             a/k/a avp
> +    28 ac97
> +    29 rtc
> +    30 tmr
> +    31 uart1
> +    32 uart2
> +    33 gpio
> +    34 sdmmc2
> +    35 spdif_out
> +    36 spdif_in
> +    37 i2s1
> +    38 i2c1
> +    39 ndflash
> +    40 sdmmc1
> +    41 sdmmc4
> +    42 twc
> +    43 pwm
> +    44 i2s2
> +    45 epp
> +    46 vi
> +    47 vi_sensor
> +    48 2d
> +    49 usbd
> +    50 isp
> +    51 3d
> +    52 ide
> +    53 disp2
> +    54 disp1
> +    55 host1x
> +    56 vcp
> +    57 cache2
> +    58 mem
> +    59 ahbdma
> +    60 apbdma
> +    61 kbc
> +    62 stat_mon
> +    63 pmc
> +    64 fuse
> +    65 kfuse
> +    66 sbc1
> +    67 snor
> +    68 spi
> +    69 sbc2
> +    70 xio
> +    71 sbc3
> +    72 dvc
> +    73 dsi
> +    74 cve
> +    75 tvo
> +    76 mipi
> +    77 hdmi
> +    78 csi
> +    79 tvdac
> +    80 i2c2
> +    81 uart3
> +    82 emc
> +    83 usb2
> +    84 usb3
> +    85 mpe
> +    86 vde
> +    87 bsea
> +    88 bsev
> +    89 speedo
> +    90 uart4
> +    91 uart5
> +    92 i2c3
> +    93 sbc4
> +    94 sdmmc3
> +    95 pcie
> +    96 owr
> +    97 afi
> +    98 csite
> +    99 avpucq
> +    100        la
> +    101        irama
> +    102        iramb
> +    103        iramc
> +    104        iramd
> +    105        cram2
> +    106        clk_d
> +    107        sus
> +    108        cdev1
> +    109        cdev2
> +
> +Example:
> +
> +clocks {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +
> +       clk_32k: clock@0 {
> +               compatible = "fixed-clock";
> +               reg = <0>;
> +               #clock-cells = <0>;
> +               clock-frequency = <32768>;
> +       };
> +
> +       osc: clock@1 {
> +               compatible = "fixed-clock";
> +               reg = <1>;
> +               #clock-cells = <0>;
> +               clock-frequency = <12000000>;
> +       };
> +};
> +
> +tegra_car: clock@60006000 {
> +       compatible = "nvidia,tegra20-car";
> +       reg = <0x60006000 1000>;
> +       clocks = <&clk_32k> <&osc>;
> +       clock-names = "32k_in", "osc";
> +       #clock-cells = <1>;
> +};
> +
> +usb@c5004000 {
> +       ...
> +       clocks = <&tegra_car 58>; /* usb2 */
> +};
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 3da7afd..8208660 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -4,6 +4,12 @@
>        compatible = "nvidia,tegra20";
>        interrupt-parent = <&intc>;
>
> +       tegra_car: clock@60006000 {
> +               compatible = "nvidia,tegra20-car";
> +               reg = <0x60006000 1000>;
> +               #clock-cells = <1>;
> +       };
> +
>        intc: interrupt-controller@50041000 {
>                compatible = "arm,cortex-a9-gic";
>                interrupt-controller;
> --
> 1.7.0.4
>

Regards,
Simon

[U-Boot] [PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Simon Glass]

Hi Stephen,

On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
> add it to tegra20.dtsi, and configure it for the board in tegra-
> seaboard.dts.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v2:
> * Remove clock-names, clock-output-names properties; Tegra will solely
> ?use integer IDs for clocks in DT.
> * Fixed typo in compatible flag
> * Resolve FIXME re: multiple clocks with the same "reset ID"; give each
> ?unique clock an ID, and ignore the reset bits, since this is purely a
> ?clock binding. Code (e.g. U-Boot) that wants to use this to determine
> ?CAR reset bit numbers would need a table to convert from the clock IDs
> ?in this binding to the related reset bit number, if any. In general, I
> ?think that's true, and the U-Boot code that handles "peripheral IDs"
> ?should be reworked to handle "clocks", the peripheral clocks being a
> ?subset of all clocks.

The clock enable and reset enable bits use the same numbering. I think
you have invented a third which is an arbitrary number which doesn't
correspond to anything in hardware. That makes sense for pin mux
function perhaps, since the indirection provides a useful concept of
function number, but here I can't see the benefit.

> * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
> ?etc.
> * Separate tegra-seaboard.dts usage example into a separate patch.
>
> This patch semantically relies on Grant Likely's common clock binding patch
> series. Once that's finalized, this patch could be checked into the kernel
> provided there are no relevant changes to Grant's patches. I believe that
> Simon Glass wants to start using this within U-Boot ASAP though.

As I may have said I am really not keen on the idea of having a table
just to use it in U-Boot.

> ---
> ?.../bindings/clock/nvidia,tegra20-car.txt ? ? ? ? ?| ?164 ++++++++++++++++++++
> ?arch/arm/boot/dts/tegra20.dtsi ? ? ? ? ? ? ? ? ? ? | ? ?6 +
> ?2 files changed, 170 insertions(+), 0 deletions(-)
> ?create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> new file mode 100644
> index 0000000..acce2d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> @@ -0,0 +1,164 @@
> +NVIDIA Tegra20 Clock And Reset Controller
> +
> +This binding uses the common clock binding:
> +Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +The CAR (Clock And Reset) Controller on Tegra is the HW module responsible
> +for muxing and gating Tegra's clocks, and setting their rates.
> +
> +Required properties :
> +- compatible : Should be "nvidia,<chip>-car"
> +- reg : Should contain CAR registers location and length
> +- clocks : Should contain phandle and clock specifiers for two clocks:
> + ?the 32 KHz "32k_in", and the board-specific oscillator "osc".
> +- #clock-cells : Should be 1.
> + ?In clock consumers, this cell represents the clock ID exposed by the CAR.
> +
> + ?The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
> + ?registers. Later, subsequent IDs will be assigned to all other clocks the
> + ?CAR controls; mainly the PLLs.

Are you sure? The ordering doesn't seem to fit with U-Boot's enum
periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file
follows along with the hardware.

> +
> + ? ?0 ?osc
> + ? ?1 ?clk_32k ? ? ? ? a/k/a clk_s
> + ? ?2 ?clk_m
> + ? ?3 ?sclk
> + ? ?4 ?cclk
> + ? ?5 ?hclk
> + ? ?6 ?pclk
> + ? ?7 ?blink
> + ? ?8 ?pll_a
> + ? ?9 ?pll_a_out0
> + ? ?10 pll_c
> + ? ?11 pll_c_out1
> + ? ?12 pll_d
> + ? ?13 pll_e
> + ? ?14 pll_m
> + ? ?15 pll_m_out1
> + ? ?16 pll_p
> + ? ?17 pll_p_out1
> + ? ?18 pll_p_out2
> + ? ?19 pll_p_out3
> + ? ?20 pll_p_out4
> + ? ?21 pll_s
> + ? ?22 pll_u
> + ? ?23 pll_x
> + ? ?24 audio ? ? ? ? ? a/k/a audio_sync_clk
> + ? ?25 audio_2x ? ? ? ?a/k/a audio_2x_sync_clk
> + ? ?26 cpu
> + ? ?27 cop ? ? ? ? ? ? a/k/a avp
> + ? ?28 ac97
> + ? ?29 rtc
> + ? ?30 tmr
> + ? ?31 uart1
> + ? ?32 uart2
> + ? ?33 gpio
> + ? ?34 sdmmc2
> + ? ?35 spdif_out
> + ? ?36 spdif_in
> + ? ?37 i2s1
> + ? ?38 i2c1
> + ? ?39 ndflash
> + ? ?40 sdmmc1
> + ? ?41 sdmmc4
> + ? ?42 twc
> + ? ?43 pwm
> + ? ?44 i2s2
> + ? ?45 epp
> + ? ?46 vi
> + ? ?47 vi_sensor
> + ? ?48 2d
> + ? ?49 usbd
> + ? ?50 isp
> + ? ?51 3d
> + ? ?52 ide
> + ? ?53 disp2
> + ? ?54 disp1
> + ? ?55 host1x
> + ? ?56 vcp
> + ? ?57 cache2
> + ? ?58 mem
> + ? ?59 ahbdma
> + ? ?60 apbdma
> + ? ?61 kbc
> + ? ?62 stat_mon
> + ? ?63 pmc
> + ? ?64 fuse
> + ? ?65 kfuse
> + ? ?66 sbc1
> + ? ?67 snor
> + ? ?68 spi
> + ? ?69 sbc2
> + ? ?70 xio
> + ? ?71 sbc3
> + ? ?72 dvc
> + ? ?73 dsi
> + ? ?74 cve
> + ? ?75 tvo
> + ? ?76 mipi
> + ? ?77 hdmi
> + ? ?78 csi
> + ? ?79 tvdac
> + ? ?80 i2c2
> + ? ?81 uart3
> + ? ?82 emc
> + ? ?83 usb2
> + ? ?84 usb3
> + ? ?85 mpe
> + ? ?86 vde
> + ? ?87 bsea
> + ? ?88 bsev
> + ? ?89 speedo
> + ? ?90 uart4
> + ? ?91 uart5
> + ? ?92 i2c3
> + ? ?93 sbc4
> + ? ?94 sdmmc3
> + ? ?95 pcie
> + ? ?96 owr
> + ? ?97 afi
> + ? ?98 csite
> + ? ?99 avpucq
> + ? ?100 ? ? ? ?la
> + ? ?101 ? ? ? ?irama
> + ? ?102 ? ? ? ?iramb
> + ? ?103 ? ? ? ?iramc
> + ? ?104 ? ? ? ?iramd
> + ? ?105 ? ? ? ?cram2
> + ? ?106 ? ? ? ?clk_d
> + ? ?107 ? ? ? ?sus
> + ? ?108 ? ? ? ?cdev1
> + ? ?109 ? ? ? ?cdev2
> +
> +Example:
> +
> +clocks {
> + ? ? ? #address-cells = <1>;
> + ? ? ? #size-cells = <0>;
> +
> + ? ? ? clk_32k: clock at 0 {
> + ? ? ? ? ? ? ? compatible = "fixed-clock";
> + ? ? ? ? ? ? ? reg = <0>;
> + ? ? ? ? ? ? ? #clock-cells = <0>;
> + ? ? ? ? ? ? ? clock-frequency = <32768>;
> + ? ? ? };
> +
> + ? ? ? osc: clock at 1 {
> + ? ? ? ? ? ? ? compatible = "fixed-clock";
> + ? ? ? ? ? ? ? reg = <1>;
> + ? ? ? ? ? ? ? #clock-cells = <0>;
> + ? ? ? ? ? ? ? clock-frequency = <12000000>;
> + ? ? ? };
> +};
> +
> +tegra_car: clock at 60006000 {
> + ? ? ? compatible = "nvidia,tegra20-car";
> + ? ? ? reg = <0x60006000 1000>;
> + ? ? ? clocks = <&clk_32k> <&osc>;
> + ? ? ? clock-names = "32k_in", "osc";
> + ? ? ? #clock-cells = <1>;
> +};
> +
> +usb at c5004000 {
> + ? ? ? ...
> + ? ? ? clocks = <&tegra_car 58>; /* usb2 */
> +};
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 3da7afd..8208660 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -4,6 +4,12 @@
> ? ? ? ?compatible = "nvidia,tegra20";
> ? ? ? ?interrupt-parent = <&intc>;
>
> + ? ? ? tegra_car: clock at 60006000 {
> + ? ? ? ? ? ? ? compatible = "nvidia,tegra20-car";
> + ? ? ? ? ? ? ? reg = <0x60006000 1000>;
> + ? ? ? ? ? ? ? #clock-cells = <1>;
> + ? ? ? };
> +
> ? ? ? ?intc: interrupt-controller at 50041000 {
> ? ? ? ? ? ? ? ?compatible = "arm,cortex-a9-gic";
> ? ? ? ? ? ? ? ?interrupt-controller;
> --
> 1.7.0.4
>

Regards,
Simon

RE: [PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Simon Glass wrote at Wednesday, January 25, 2012 1:14 PM:
> On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
> > add it to tegra20.dtsi, and configure it for the board in tegra-
> > seaboard.dts.
...
> > v2:
...
> > * Resolve FIXME re: multiple clocks with the same "reset ID"; give each
> >  unique clock an ID, and ignore the reset bits, since this is purely a
> >  clock binding. Code (e.g. U-Boot) that wants to use this to determine
> >  CAR reset bit numbers would need a table to convert from the clock IDs
> >  in this binding to the related reset bit number, if any. In general, I
> >  think that's true, and the U-Boot code that handles "peripheral IDs"
> >  should be reworked to handle "clocks", the peripheral clocks being a
> >  subset of all clocks.
> 
> The clock enable and reset enable bits use the same numbering. I think
> you have invented a third which is an arbitrary number which doesn't
> correspond to anything in hardware. That makes sense for pin mux
> function perhaps, since the indirection provides a useful concept of
> function number, but here I can't see the benefit.

I didn't do it because there was specific benefit, but simply because
we have no choice.

There are clocks that don't have reset bits or clock enable bits.

There are some clocks that are really different clocks (different mux
selection or divider control registers) yet share the same bit for reset
and clock enable.

Therefore it simply isn't true that there's a 1:1 mapping between clocks
and clock-enable/reset bits.

I deliberately made this updated binding have a different numbering
scheme to the clock-enable/reset bits to make this clear, so that no one
would accidentally confuse the two concepts.

> > * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
> >  etc.
> > * Separate tegra-seaboard.dts usage example into a separate patch.
> >
> > This patch semantically relies on Grant Likely's common clock binding patch
> > series. Once that's finalized, this patch could be checked into the kernel
> > provided there are no relevant changes to Grant's patches. I believe that
> > Simon Glass wants to start using this within U-Boot ASAP though.
> 
> As I may have said I am really not keen on the idea of having a table
> just to use it in U-Boot.

I don't see any alternative given the way the HW is designed.

We could ignore the way the HW works and assume that clock ID == clock-
enable/reset bits is true for many clocks. However, it's not true for
all, so I think that'd be too error-prone.

Equally, I know that you will need a table sometime in U-Boot to map
from clock ID to clock-enable/reset bit and many other per-clock
parameters if you're going to be initializing stuff in U-Boot from DT
rather than hard-coding it, so I think you may as well just add it now.

> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
...
> > +  The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
> > +  registers. Later, subsequent IDs will be assigned to all other clocks the
> > +  CAR controls; mainly the PLLs.
> 
> Are you sure? The ordering doesn't seem to fit with U-Boot's enum
> periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file
> follows along with the hardware.

No, that paragraph is wrong. I simply forgot to remove it.

-- 
nvpublic

[U-Boot] [PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Simon Glass wrote at Wednesday, January 25, 2012 1:14 PM:
> On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren@nvidia.com> wrote:
> > Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
> > add it to tegra20.dtsi, and configure it for the board in tegra-
> > seaboard.dts.
...
> > v2:
...
> > * Resolve FIXME re: multiple clocks with the same "reset ID"; give each
> > ?unique clock an ID, and ignore the reset bits, since this is purely a
> > ?clock binding. Code (e.g. U-Boot) that wants to use this to determine
> > ?CAR reset bit numbers would need a table to convert from the clock IDs
> > ?in this binding to the related reset bit number, if any. In general, I
> > ?think that's true, and the U-Boot code that handles "peripheral IDs"
> > ?should be reworked to handle "clocks", the peripheral clocks being a
> > ?subset of all clocks.
> 
> The clock enable and reset enable bits use the same numbering. I think
> you have invented a third which is an arbitrary number which doesn't
> correspond to anything in hardware. That makes sense for pin mux
> function perhaps, since the indirection provides a useful concept of
> function number, but here I can't see the benefit.

I didn't do it because there was specific benefit, but simply because
we have no choice.

There are clocks that don't have reset bits or clock enable bits.

There are some clocks that are really different clocks (different mux
selection or divider control registers) yet share the same bit for reset
and clock enable.

Therefore it simply isn't true that there's a 1:1 mapping between clocks
and clock-enable/reset bits.

I deliberately made this updated binding have a different numbering
scheme to the clock-enable/reset bits to make this clear, so that no one
would accidentally confuse the two concepts.

> > * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
> > ?etc.
> > * Separate tegra-seaboard.dts usage example into a separate patch.
> >
> > This patch semantically relies on Grant Likely's common clock binding patch
> > series. Once that's finalized, this patch could be checked into the kernel
> > provided there are no relevant changes to Grant's patches. I believe that
> > Simon Glass wants to start using this within U-Boot ASAP though.
> 
> As I may have said I am really not keen on the idea of having a table
> just to use it in U-Boot.

I don't see any alternative given the way the HW is designed.

We could ignore the way the HW works and assume that clock ID == clock-
enable/reset bits is true for many clocks. However, it's not true for
all, so I think that'd be too error-prone.

Equally, I know that you will need a table sometime in U-Boot to map
from clock ID to clock-enable/reset bit and many other per-clock
parameters if you're going to be initializing stuff in U-Boot from DT
rather than hard-coding it, so I think you may as well just add it now.

> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
...
> > + ?The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
> > + ?registers. Later, subsequent IDs will be assigned to all other clocks the
> > + ?CAR controls; mainly the PLLs.
> 
> Are you sure? The ordering doesn't seem to fit with U-Boot's enum
> periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file
> follows along with the hardware.

No, that paragraph is wrong. I simply forgot to remove it.

-- 
nvpublic

Re: [PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Simon Glass]

Hi Stephen,

On Wed, Jan 25, 2012 at 12:31 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Simon Glass wrote at Wednesday, January 25, 2012 1:14 PM:
>> On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
>> > add it to tegra20.dtsi, and configure it for the board in tegra-
>> > seaboard.dts.
> ...
>> > v2:
> ...
>> > * Resolve FIXME re: multiple clocks with the same "reset ID"; give each
>> >  unique clock an ID, and ignore the reset bits, since this is purely a
>> >  clock binding. Code (e.g. U-Boot) that wants to use this to determine
>> >  CAR reset bit numbers would need a table to convert from the clock IDs
>> >  in this binding to the related reset bit number, if any. In general, I
>> >  think that's true, and the U-Boot code that handles "peripheral IDs"
>> >  should be reworked to handle "clocks", the peripheral clocks being a
>> >  subset of all clocks.
>>
>> The clock enable and reset enable bits use the same numbering. I think
>> you have invented a third which is an arbitrary number which doesn't
>> correspond to anything in hardware. That makes sense for pin mux
>> function perhaps, since the indirection provides a useful concept of
>> function number, but here I can't see the benefit.
>
> I didn't do it because there was specific benefit, but simply because
> we have no choice.

I was quite happy with your original proposal which made them line up
where they could, and used other numbers where they couldn't.

>
> There are clocks that don't have reset bits or clock enable bits.
>
> There are some clocks that are really different clocks (different mux
> selection or divider control registers) yet share the same bit for reset
> and clock enable.
>
> Therefore it simply isn't true that there's a 1:1 mapping between clocks
> and clock-enable/reset bits.
>
> I deliberately made this updated binding have a different numbering
> scheme to the clock-enable/reset bits to make this clear, so that no one
> would accidentally confuse the two concepts.

I think it makes sense to line them up where you can (all but two
cases out of about 60 I think).

>
>> > * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
>> >  etc.
>> > * Separate tegra-seaboard.dts usage example into a separate patch.
>> >
>> > This patch semantically relies on Grant Likely's common clock binding patch
>> > series. Once that's finalized, this patch could be checked into the kernel
>> > provided there are no relevant changes to Grant's patches. I believe that
>> > Simon Glass wants to start using this within U-Boot ASAP though.
>>
>> As I may have said I am really not keen on the idea of having a table
>> just to use it in U-Boot.
>
> I don't see any alternative given the way the HW is designed.

You had the alternative yourself the first time around. There clearly
is an alternative.

>
> We could ignore the way the HW works and assume that clock ID == clock-
> enable/reset bits is true for many clocks. However, it's not true for
> all, so I think that'd be too error-prone.
>
> Equally, I know that you will need a table sometime in U-Boot to map
> from clock ID to clock-enable/reset bit and many other per-clock
> parameters if you're going to be initializing stuff in U-Boot from DT
> rather than hard-coding it, so I think you may as well just add it now.

I am happy that there is now a concept of a peripheral number and we
don't have to refer to everything with strings. I don't mind if for
hardware reasons this peripheral number doesn't always correspond to
everything we point it at (clock, reset, pinmux, clock source). But I
am uncomfortable with it corresponding to nothing because it might be
'error-prone'. This seems to be introducing a layer of indirection
which is not needed at all in U-Boot, for example.

I prefer a clock number which corresponds to the clock enable/reset
bit positions in all cases it can (all but two) and a different number
where it can't. At least this saves one table. Alternatively perhaps
these bit numbers should be specified in the device tree also? I was
rather hoping that the device tree would take us away from having lots
of tables in the C code.

>
>> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> ...
>> > +  The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
>> > +  registers. Later, subsequent IDs will be assigned to all other clocks the
>> > +  CAR controls; mainly the PLLs.
>>
>> Are you sure? The ordering doesn't seem to fit with U-Boot's enum
>> periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file
>> follows along with the hardware.
>
> No, that paragraph is wrong. I simply forgot to remove it.

Well I vote for a return :-)

>
> --
> nvpublic
>

Regards,
Simon

[U-Boot] [PATCH 1/2] ARM: tegra: Define Tegra20 CAR binding

[Simon Glass]

Hi Stephen,

On Wed, Jan 25, 2012 at 12:31 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Simon Glass wrote at Wednesday, January 25, 2012 1:14 PM:
>> On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> > Document a binding for the Tegra20 CAR (Clock And Reset) Controller,
>> > add it to tegra20.dtsi, and configure it for the board in tegra-
>> > seaboard.dts.
> ...
>> > v2:
> ...
>> > * Resolve FIXME re: multiple clocks with the same "reset ID"; give each
>> > ?unique clock an ID, and ignore the reset bits, since this is purely a
>> > ?clock binding. Code (e.g. U-Boot) that wants to use this to determine
>> > ?CAR reset bit numbers would need a table to convert from the clock IDs
>> > ?in this binding to the related reset bit number, if any. In general, I
>> > ?think that's true, and the U-Boot code that handles "peripheral IDs"
>> > ?should be reworked to handle "clocks", the peripheral clocks being a
>> > ?subset of all clocks.
>>
>> The clock enable and reset enable bits use the same numbering. I think
>> you have invented a third which is an arbitrary number which doesn't
>> correspond to anything in hardware. That makes sense for pin mux
>> function perhaps, since the indirection provides a useful concept of
>> function number, but here I can't see the benefit.
>
> I didn't do it because there was specific benefit, but simply because
> we have no choice.

I was quite happy with your original proposal which made them line up
where they could, and used other numbers where they couldn't.

>
> There are clocks that don't have reset bits or clock enable bits.
>
> There are some clocks that are really different clocks (different mux
> selection or divider control registers) yet share the same bit for reset
> and clock enable.
>
> Therefore it simply isn't true that there's a 1:1 mapping between clocks
> and clock-enable/reset bits.
>
> I deliberately made this updated binding have a different numbering
> scheme to the clock-enable/reset bits to make this clear, so that no one
> would accidentally confuse the two concepts.

I think it makes sense to line them up where you can (all but two
cases out of about 60 I think).

>
>> > * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs,
>> > ?etc.
>> > * Separate tegra-seaboard.dts usage example into a separate patch.
>> >
>> > This patch semantically relies on Grant Likely's common clock binding patch
>> > series. Once that's finalized, this patch could be checked into the kernel
>> > provided there are no relevant changes to Grant's patches. I believe that
>> > Simon Glass wants to start using this within U-Boot ASAP though.
>>
>> As I may have said I am really not keen on the idea of having a table
>> just to use it in U-Boot.
>
> I don't see any alternative given the way the HW is designed.

You had the alternative yourself the first time around. There clearly
is an alternative.

>
> We could ignore the way the HW works and assume that clock ID == clock-
> enable/reset bits is true for many clocks. However, it's not true for
> all, so I think that'd be too error-prone.
>
> Equally, I know that you will need a table sometime in U-Boot to map
> from clock ID to clock-enable/reset bit and many other per-clock
> parameters if you're going to be initializing stuff in U-Boot from DT
> rather than hard-coding it, so I think you may as well just add it now.

I am happy that there is now a concept of a peripheral number and we
don't have to refer to everything with strings. I don't mind if for
hardware reasons this peripheral number doesn't always correspond to
everything we point it at (clock, reset, pinmux, clock source). But I
am uncomfortable with it corresponding to nothing because it might be
'error-prone'. This seems to be introducing a layer of indirection
which is not needed at all in U-Boot, for example.

I prefer a clock number which corresponds to the clock enable/reset
bit positions in all cases it can (all but two) and a different number
where it can't. At least this saves one table. Alternatively perhaps
these bit numbers should be specified in the device tree also? I was
rather hoping that the device tree would take us away from having lots
of tables in the C code.

>
>> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> ...
>> > + ?The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB
>> > + ?registers. Later, subsequent IDs will be assigned to all other clocks the
>> > + ?CAR controls; mainly the PLLs.
>>
>> Are you sure? The ordering doesn't seem to fit with U-Boot's enum
>> periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file
>> follows along with the hardware.
>
> No, that paragraph is wrong. I simply forgot to remove it.

Well I vote for a return :-)

>
> --
> nvpublic
>

Regards,
Simon