[PATCH] ARM: tegra: Define Tegra20 CAR binding

[PATCH] 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>
---
All, Simon Glass is attempting to upstream Tegra USB support in U-Boot.
The driver is only instantiated from device tree, and needs to configure
clocks for the USB module. Hence, he proposed a clock binding for Tegra.
I've read through Grant's proposed common clock bindings and reworked
Simon's proposal a little, ending up with the patch below for the kernel.
I'd appreciate any comments you have.

Grant, can you confirm that it's reasonable to start making use of your
proposed common clock bindings in U-Boot, even though they're only an RFC?

Grant, there are a couple of FIXMEs in the binding documentation below.
Can you give any insight on these?

A comment on the shared enable issue:

When enabling/disabling clocks, Tegra requires you to reset the HW module
that the clock is routed to. Both reset and clock enable registers are
part of the CAR. U-Boot currently has an API to do the reset based on a
"peripheral ID", which simply means a bit number in a set of 3 "reset"
registers. The bits in those registers share the same numbering as the
"clock enable" registers. Hence, to make life easy for U-Boot, I've
defined the clock IDs in this binding to be equal to these bit numbers.
However, this breaks down where there isn't a 1:1 mapping between reset
and clock enable bits, and clocks. For example, there's a single reset
and clock enable bit for the SPDIF controller, yet this applies to two
clocks; spdif_in and spdif_out. Hence, this simplification for U-Boot
breaks down. I think the correct solution is to fix U-Boot not to
require this simplification, renumber the clocks in the CAR binding in
a completely arbitrary fashion, and require U-Boot to contain a mapping
table between CAR's DT clock IDs and any other information related to
those clocks. Do you see any alternative? We really need the two SPDIF
clocks to be different clock IDs in the binding, since each has a
completely independant mux for the clock source/parent, and the two
clocks obviously can't share the same clock ID (well, hmm, perhaps they
could if we used 2 cells for the specifier, 1 for the bit number, and
one more to differentiate clocks that use the same bit...). I'm still
inclined to simply push back on U-Boot to do it right.

 .../bindings/clock/nvidia,tegra20-car.txt          |  156 ++++++++++++++++++++
 arch/arm/boot/dts/tegra-seaboard.dts               |   18 +++
 arch/arm/boot/dts/tegra20.dtsi                     |    7 +
 3 files changed, 181 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..71cfdd2
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
@@ -0,0 +1,156 @@
+* 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-names : Should contain a list of strings, with values "32k_in",
+  and "osc".
+- #clock-cells : Should be 1.
+  In clock consumers, this cell represents the clock ID exposed by the CAR.
+  For a list of valid values, see the clock-output-names property.
+
+Optional properties :
+- clock-output-names : Should contain a list of strings defining the clocks
+  that the CAR provides. The list of names and clock IDs is below. The IDs
+  may be used in clock specifiers. The names should be listed in this clock-
+  output-names property, sorted in ID order, if this property is present.
+
+  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.
+
+    0   "cpu"
+    1   "unassigned"
+    2   "unassigned"    FIXME: Is it OK to have 2 identical (unused) names?
+    3   "ac97"
+    4   "rtc"
+    5   "tmr"
+    6   "uart1"
+    7   "uart2"
+    8   "gpio"
+    9   "sdmmc2"
+    10  "spdif"         FIXME: One enable bit controls two clocks!!!
+    11  "i2s1"
+    12  "i2c1"
+    13  "ndflash"
+    14  "sdmmc1"
+    15  "sdmmc4"
+    16  "twc"
+    17  "pwm"
+    18  "i2s2"
+    19  "epp"
+    20  "vi"            FIXME: One enable bit controls two clocks!!!
+    21  "2d"
+    22  "usbd"
+    23  "isp"
+    24  "3d"
+    25  "ide"
+    26  "disp2"
+    27  "disp1"
+    28  "host1x"
+    29  "vcp"
+    30  "unassigned"
+    31  "cache2"
+
+    32  "mem"
+    33  "ahbdma"
+    34  "apbdma"
+    35  "unassigned"
+    36  "kbc"
+    37  "stat_mon"
+    38  "pmc"
+    39  "fuse"
+    40  "kfuse"
+    41  "sbc1"
+    42  "snor"
+    43  "spi"
+    44  "sbc2"
+    45  "xio"
+    46  "sbc3"
+    47  "dvc_i2c"
+    48  "dsi"
+    49  "tvo"
+    50  "mipi"
+    51  "hdmi"
+    52  "csi"
+    53  "tvdac"
+    54  "i2c2"
+    55  "uart3"
+    56  "unassigned"
+    57  "emc"
+    58  "usb2"
+    59  "usb3"
+    60  "mpe"
+    61  "vde"
+    62  "bsea"
+    63  "bsev"
+
+    64  "speedo"
+    65  "uart4"
+    66  "uart5"
+    67  "i2c3"
+    68  "sbc4"
+    69  "sdmmc3"
+    70  "pcie"
+    71  "owr"
+    72  "afi"
+    73  "csite"
+    74  "unassigned"
+    75  "avpucq"
+    76  "unassigned"
+    77  "unassigned"
+    78  "unassigned"
+    79  "unassigned"
+    80  "unassigned"
+    81  "unassigned"
+    82  "unassigned"
+    83  "unassigned"
+    84  "irama"
+    85  "iramb"
+    86  "iramc"
+    87  "iramd"
+    88  "cram2"
+    89  "audio_2x"
+    90  "clk_d"
+    91  "unassigned"
+    92  "sus"
+    93  "cdev1"
+    94  "cdev2"
+    95  "unassigned"
+
+Example:
+
+clocks {
+	clk_32k: oscillator@0 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <32768>;
+	};
+
+	osc: oscillator@1 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <12000000>;
+	};
+};
+
+tegar_car: clock@60006000 {
+	compatible = "tegra,periphclk";
+	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/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
index b55a02e..f9347fe 100644
--- a/arch/arm/boot/dts/tegra-seaboard.dts
+++ b/arch/arm/boot/dts/tegra-seaboard.dts
@@ -11,6 +11,24 @@
 		reg = < 0x00000000 0x40000000 >;
 	};
 
+	clocks {
+		clk_32k: oscillator@0 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32768>;
+		};
+
+		osc: oscillator@1 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <12000000>;
+		};
+	};
+
+	clock@60006000 {
+		clocks = <&clk_32k> <&osc>;
+	};
+
 	i2c@7000c000 {
 		clock-frequency = <400000>;
 	};
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 3da7afd..be4abd2 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -4,6 +4,13 @@
 	compatible = "nvidia,tegra20";
 	interrupt-parent = <&intc>;
 
+	tegar_car: clock@60006000 {
+		compatible = "tegra,periphclk";
+		reg = <0x60006000 1000>;
+		clock-names = "32k_in", "osc";
+		#clock-cells = <1>;
+	};
+
 	intc: interrupt-controller@50041000 {
 		compatible = "arm,cortex-a9-gic";
 		interrupt-controller;
-- 
1.7.0.4

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Olof Johansson]

Hi,

On Wed, Jan 18, 2012 at 05:16:52PM -0700, Stephen Warren wrote:

>  .../bindings/clock/nvidia,tegra20-car.txt          |  156 ++++++++++++++++++++
>  arch/arm/boot/dts/tegra-seaboard.dts               |   18 +++
>  arch/arm/boot/dts/tegra20.dtsi                     |    7 +
>  3 files changed, 181 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..71cfdd2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> @@ -0,0 +1,156 @@
> +* 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-names : Should contain a list of strings, with values "32k_in",
> +  and "osc".

Hmm. I'd prefer to just ditch the notion of "clock-names" in the cases
where it isn't strictly necessary. Just because some vendors don't want
to define an order between their clocks doesn't mean it's a good idea
for everybody to use that model. In this case, just declaring that the
two clocks refs have to be to those two clocks in that order should
be sufficient.

> +- #clock-cells : Should be 1.
> +  In clock consumers, this cell represents the clock ID exposed by the CAR.
> +  For a list of valid values, see the clock-output-names property.
> +
> +Optional properties :
> +- clock-output-names : Should contain a list of strings defining the clocks
> +  that the CAR provides. The list of names and clock IDs is below. The IDs
> +  may be used in clock specifiers. The names should be listed in this clock-
> +  output-names property, sorted in ID order, if this property is present.
> +
> +  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.

Aren't these names hardcoded per SoC? If so, they can be derived from the
compatible field + output number without having a table in the device tree.

If anything, you might want to enumerate the allowed/expected values, but
actually putting them in a property seems overkill.

[...]

> +Example:
> +
> +clocks {
> +	clk_32k: oscillator@0 {

If it has a unit addres (@<x>) it needs a reg property with the same base
address.

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <32768>;
> +	};
> +
> +	osc: oscillator@1 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <12000000>;
> +	};
> +};
> +
> +tegar_car: clock@60006000 {

typo? tegra_car?

> +	compatible = "tegra,periphclk";
> +	reg = <0x60006000 1000>;
> +	clocks = <&clk_32k> <&osc>;
> +	clock-names = "32k_in", "osc";
> +	#clock-cells = <1>;
> +};
> +
> +usb@c5004000 {
> +	...
> +	clocks = <&tegra_car 58>; /* usb2 */
> +};

RE: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Olof Johansson wrote at Wednesday, January 18, 2012 10:32 PM:
> On Wed, Jan 18, 2012 at 05:16:52PM -0700, Stephen Warren wrote:
> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> > +* 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-names : Should contain a list of strings, with values "32k_in",
> > +  and "osc".
> 
> Hmm. I'd prefer to just ditch the notion of "clock-names" in the cases
> where it isn't strictly necessary. Just because some vendors don't want
> to define an order between their clocks doesn't mean it's a good idea
> for everybody to use that model. In this case, just declaring that the
> two clocks refs have to be to those two clocks in that order should
> be sufficient.

OK, that seems reasonable. I'm happy using of_clk_get() rather than
of_clk_get_by_name(). I guess that means we should just avoid any
discussion of clock-output-names too.

> > +- #clock-cells : Should be 1.
> > +  In clock consumers, this cell represents the clock ID exposed by the CAR.
> > +  For a list of valid values, see the clock-output-names property.
> > +
> > +Optional properties :
> > +- clock-output-names : Should contain a list of strings defining the clocks
> > +  that the CAR provides. The list of names and clock IDs is below. The IDs
> > +  may be used in clock specifiers. The names should be listed in this clock-
> > +  output-names property, sorted in ID order, if this property is present.
> > +
> > +  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.
> 
> Aren't these names hardcoded per SoC? If so, they can be derived from the
> compatible field + output number without having a table in the device tree.
>
> If anything, you might want to enumerate the allowed/expected values, but
> actually putting them in a property seems overkill.

Yes, the set of clocks is hard-coded per SoC, and the clock is uniquely
identified by compatible+id.

clock-output-names doesn't actually serve any functional purpose in
Grant's common clock bindings patches; it's more of a documentation
thing. As such, yes, I can remove the suggestion to actually put the
table into the device tree, and rework the binding to simply define
what the mapping is independently.

...
> > +Example:
> > +
> > +clocks {
> > +	clk_32k: oscillator@0 {
> 
> If it has a unit addres (@<x>) it needs a reg property with the same base
> address.

I thought everything needed a unit address period? Is the rule more like
the unit address is optional, but if present must match reg, so I can
simply remove @0 and @1 here? I guess that makes a lot more sense.

> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <32768>;
> > +	};
> > +
> > +	osc: oscillator@1 {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <12000000>;
> > +	};
> > +};

-- 
nvpublic

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Olof Johansson]

Hi,

On Thu, Jan 19, 2012 at 9:17 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Olof Johansson wrote at Wednesday, January 18, 2012 10:32 PM:
>> On Wed, Jan 18, 2012 at 05:16:52PM -0700, Stephen Warren wrote:
>> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
>> > +* 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-names : Should contain a list of strings, with values "32k_in",
>> > +  and "osc".
>>
>> Hmm. I'd prefer to just ditch the notion of "clock-names" in the cases
>> where it isn't strictly necessary. Just because some vendors don't want
>> to define an order between their clocks doesn't mean it's a good idea
>> for everybody to use that model. In this case, just declaring that the
>> two clocks refs have to be to those two clocks in that order should
>> be sufficient.
>
> OK, that seems reasonable. I'm happy using of_clk_get() rather than
> of_clk_get_by_name(). I guess that means we should just avoid any
> discussion of clock-output-names too.

Sounds good to me. Let's make sure Grant is OK with it too though.

>> > +- #clock-cells : Should be 1.
>> > +  In clock consumers, this cell represents the clock ID exposed by the CAR.
>> > +  For a list of valid values, see the clock-output-names property.
>> > +
>> > +Optional properties :
>> > +- clock-output-names : Should contain a list of strings defining the clocks
>> > +  that the CAR provides. The list of names and clock IDs is below. The IDs
>> > +  may be used in clock specifiers. The names should be listed in this clock-
>> > +  output-names property, sorted in ID order, if this property is present.
>> > +
>> > +  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.
>>
>> Aren't these names hardcoded per SoC? If so, they can be derived from the
>> compatible field + output number without having a table in the device tree.
>>
>> If anything, you might want to enumerate the allowed/expected values, but
>> actually putting them in a property seems overkill.
>
> Yes, the set of clocks is hard-coded per SoC, and the clock is uniquely
> identified by compatible+id.
>
> clock-output-names doesn't actually serve any functional purpose in
> Grant's common clock bindings patches; it's more of a documentation
> thing. As such, yes, I can remove the suggestion to actually put the
> table into the device tree, and rework the binding to simply define
> what the mapping is independently.

Again, sounds good to me.

>> > +Example:
>> > +
>> > +clocks {
>> > +   clk_32k: oscillator@0 {
>>
>> If it has a unit addres (@<x>) it needs a reg property with the same base
>> address.
>
> I thought everything needed a unit address period? Is the rule more like
> the unit address is optional, but if present must match reg, so I can
> simply remove @0 and @1 here? I guess that makes a lot more sense.

Nope, you only need a unit address to make a node unique, i.e. if you
have more than one with the same name. It's not something that's been
followed very well on ARM dts files, I have even seen review comments
asking for explicit unit IDs when none are needed.

So you can't remove @0 and @1 here, since there are two oscillator subnodes.

I'm not 100% sure if you have to have the unit address represented as
"reg" or not, but it should probably be represented by _something_ in
the properties of the node. Mitch? Segher? :)


>> > +           compatible = "fixed-clock";
>> > +           #clock-cells = <0>;
>> > +           clock-frequency = <32768>;
>> > +   };
>> > +
>> > +   osc: oscillator@1 {
>> > +           compatible = "fixed-clock";
>> > +           #clock-cells = <0>;
>> > +           clock-frequency = <12000000>;
>> > +   };
>> > +};


-Olof

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Mitch Bradley]

On 1/20/2012 9:32 PM, Olof Johansson wrote:
> Hi,
>
> On Thu, Jan 19, 2012 at 9:17 AM, Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>  wrote:
>> Olof Johansson wrote at Wednesday, January 18, 2012 10:32 PM:
>>> On Wed, Jan 18, 2012 at 05:16:52PM -0700, Stephen Warren wrote:
>>>> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
>>>> +* 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-names : Should contain a list of strings, with values "32k_in",
>>>> +  and "osc".
>>>
>>> Hmm. I'd prefer to just ditch the notion of "clock-names" in the cases
>>> where it isn't strictly necessary. Just because some vendors don't want
>>> to define an order between their clocks doesn't mean it's a good idea
>>> for everybody to use that model. In this case, just declaring that the
>>> two clocks refs have to be to those two clocks in that order should
>>> be sufficient.
>>
>> OK, that seems reasonable. I'm happy using of_clk_get() rather than
>> of_clk_get_by_name(). I guess that means we should just avoid any
>> discussion of clock-output-names too.
>
> Sounds good to me. Let's make sure Grant is OK with it too though.
>
>>>> +- #clock-cells : Should be 1.
>>>> +  In clock consumers, this cell represents the clock ID exposed by the CAR.
>>>> +  For a list of valid values, see the clock-output-names property.
>>>> +
>>>> +Optional properties :
>>>> +- clock-output-names : Should contain a list of strings defining the clocks
>>>> +  that the CAR provides. The list of names and clock IDs is below. The IDs
>>>> +  may be used in clock specifiers. The names should be listed in this clock-
>>>> +  output-names property, sorted in ID order, if this property is present.
>>>> +
>>>> +  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.
>>>
>>> Aren't these names hardcoded per SoC? If so, they can be derived from the
>>> compatible field + output number without having a table in the device tree.
>>>
>>> If anything, you might want to enumerate the allowed/expected values, but
>>> actually putting them in a property seems overkill.
>>
>> Yes, the set of clocks is hard-coded per SoC, and the clock is uniquely
>> identified by compatible+id.
>>
>> clock-output-names doesn't actually serve any functional purpose in
>> Grant's common clock bindings patches; it's more of a documentation
>> thing. As such, yes, I can remove the suggestion to actually put the
>> table into the device tree, and rework the binding to simply define
>> what the mapping is independently.
>
> Again, sounds good to me.
>
>>>> +Example:
>>>> +
>>>> +clocks {
>>>> +   clk_32k: oscillator@0 {
>>>
>>> If it has a unit addres (@<x>) it needs a reg property with the same base
>>> address.
>>
>> I thought everything needed a unit address period? Is the rule more like
>> the unit address is optional, but if present must match reg, so I can
>> simply remove @0 and @1 here? I guess that makes a lot more sense.
>
> Nope, you only need a unit address to make a node unique, i.e. if you
> have more than one with the same name. It's not something that's been
> followed very well on ARM dts files, I have even seen review comments
> asking for explicit unit IDs when none are needed.
>
> So you can't remove @0 and @1 here, since there are two oscillator subnodes.
>
> I'm not 100% sure if you have to have the unit address represented as
> "reg" or not, but it should probably be represented by _something_ in
> the properties of the node. Mitch? Segher? :)

unit-address is, by definition, the first address component of reg

>
>
>>>> +           compatible = "fixed-clock";
>>>> +           #clock-cells =<0>;
>>>> +           clock-frequency =<32768>;
>>>> +   };
>>>> +
>>>> +   osc: oscillator@1 {
>>>> +           compatible = "fixed-clock";
>>>> +           #clock-cells =<0>;
>>>> +           clock-frequency =<12000000>;
>>>> +   };
>>>> +};
>
>
> -Olof
>
>

RE: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Olof Johansson wrote at Saturday, January 21, 2012 12:32 AM:
> On Thu, Jan 19, 2012 at 9:17 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Olof Johansson wrote at Wednesday, January 18, 2012 10:32 PM:
> >> On Wed, Jan 18, 2012 at 05:16:52PM -0700, Stephen Warren wrote:
> >> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> >> > +* 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-names : Should contain a list of strings, with values "32k_in",
> >> > +  and "osc".
...
> >> > +Example:
> >> > +
> >> > +clocks {
> >> > +   clk_32k: oscillator@0 {
> >>
> >> If it has a unit addres (@<x>) it needs a reg property with the same base
> >> address.
> >
> > I thought everything needed a unit address period? Is the rule more like
> > the unit address is optional, but if present must match reg, so I can
> > simply remove @0 and @1 here? I guess that makes a lot more sense.
> 
> Nope, you only need a unit address to make a node unique, i.e. if you
> have more than one with the same name. It's not something that's been
> followed very well on ARM dts files, I have even seen review comments
> asking for explicit unit IDs when none are needed.
> 
> So you can't remove @0 and @1 here, since there are two oscillator subnodes.

If I keep the unit address, then I need to add a reg property per Mitch's
response. But, then I need #address-cells and #size-cells in the clock
node too. Yuck, since this isn't an addressable bus.

Instead, is it acceptable to simply rename the nodes to e.g.:

clk_32k: clock {...};
osc: crystal {...};

In the long term, I believe that osc/crystal is going to continue to
be a top-level object, but clk_32k is actually an output from the PMU
chip, and hence there won't be any naming conflict here anyway...

-- 
nvpublic

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Mitch Bradley]

On 1/23/2012 6:18 AM, Stephen Warren wrote:
> Olof Johansson wrote at Saturday, January 21, 2012 12:32 AM:
>> On Thu, Jan 19, 2012 at 9:17 AM, Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>  wrote:
>>> Olof Johansson wrote at Wednesday, January 18, 2012 10:32 PM:
>>>> On Wed, Jan 18, 2012 at 05:16:52PM -0700, Stephen Warren wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
>>>>> +* 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-names : Should contain a list of strings, with values "32k_in",
>>>>> +  and "osc".
> ...
>>>>> +Example:
>>>>> +
>>>>> +clocks {
>>>>> +   clk_32k: oscillator@0 {
>>>>
>>>> If it has a unit addres (@<x>) it needs a reg property with the same base
>>>> address.
>>>
>>> I thought everything needed a unit address period? Is the rule more like
>>> the unit address is optional, but if present must match reg, so I can
>>> simply remove @0 and @1 here? I guess that makes a lot more sense.
>>
>> Nope, you only need a unit address to make a node unique, i.e. if you
>> have more than one with the same name. It's not something that's been
>> followed very well on ARM dts files, I have even seen review comments
>> asking for explicit unit IDs when none are needed.
>>
>> So you can't remove @0 and @1 here, since there are two oscillator subnodes.
>
> If I keep the unit address, then I need to add a reg property per Mitch's
> response. But, then I need #address-cells and #size-cells in the clock
> node too. Yuck, since this isn't an addressable bus.
>
> Instead, is it acceptable to simply rename the nodes to e.g.:
>
> clk_32k: clock {...};
> osc: crystal {...};


One consideration:  These nodes are children of a parent, so that parent 
is implicitly a "bus node", even though there is no physical hardware 
bus.  The path resolution rules say that, in the absence of a 
"#address-cells" property in a bus node, the default value is 2.  So any 
code that implements that rule will think these nodes are missing a 
"reg" property, thus triggering the "wildcard node" rule, which says 
that you can match the node with any unit address.

Whether or not that would cause problems in practice is hard to say.  I 
think that my Open Firmware implementation would handle it okay, but I 
can't speak to the Linux device tree code.

I'm not sure why you thing "yuck" about synthesizing an address space 
for the subnodes.  It doesn't seem that bad to me.

>
> In the long term, I believe that osc/crystal is going to continue to
> be a top-level object, but clk_32k is actually an output from the PMU
> chip, and hence there won't be any naming conflict here anyway...
>

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Grant Likely]

On Fri, Jan 20, 2012 at 11:32:04PM -0800, Olof Johansson wrote:
> Hi,
> 
> On Thu, Jan 19, 2012 at 9:17 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Olof Johansson wrote at Wednesday, January 18, 2012 10:32 PM:
> >> On Wed, Jan 18, 2012 at 05:16:52PM -0700, Stephen Warren wrote:
> >> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> >> > +* 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-names : Should contain a list of strings, with values "32k_in",
> >> > +  and "osc".
> >>
> >> Hmm. I'd prefer to just ditch the notion of "clock-names" in the cases
> >> where it isn't strictly necessary. Just because some vendors don't want
> >> to define an order between their clocks doesn't mean it's a good idea
> >> for everybody to use that model. In this case, just declaring that the
> >> two clocks refs have to be to those two clocks in that order should
> >> be sufficient.
> >
> > OK, that seems reasonable. I'm happy using of_clk_get() rather than
> > of_clk_get_by_name(). I guess that means we should just avoid any
> > discussion of clock-output-names too.
> 
> Sounds good to me. Let's make sure Grant is OK with it too though.

Yes, I agree.

g.

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Simon Glass]

Hi Stephen,

On Wed, Jan 18, 2012 at 4:16 PM, 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>
> ---
> All, Simon Glass is attempting to upstream Tegra USB support in U-Boot.
> The driver is only instantiated from device tree, and needs to configure
> clocks for the USB module. Hence, he proposed a clock binding for Tegra.
> I've read through Grant's proposed common clock bindings and reworked
> Simon's proposal a little, ending up with the patch below for the kernel.
> I'd appreciate any comments you have.
>
> Grant, can you confirm that it's reasonable to start making use of your
> proposed common clock bindings in U-Boot, even though they're only an RFC?
>
> Grant, there are a couple of FIXMEs in the binding documentation below.
> Can you give any insight on these?
>
> A comment on the shared enable issue:
>
> When enabling/disabling clocks, Tegra requires you to reset the HW module
> that the clock is routed to. Both reset and clock enable registers are
> part of the CAR. U-Boot currently has an API to do the reset based on a
> "peripheral ID", which simply means a bit number in a set of 3 "reset"
> registers. The bits in those registers share the same numbering as the
> "clock enable" registers. Hence, to make life easy for U-Boot, I've
> defined the clock IDs in this binding to be equal to these bit numbers.
> However, this breaks down where there isn't a 1:1 mapping between reset
> and clock enable bits, and clocks. For example, there's a single reset
> and clock enable bit for the SPDIF controller, yet this applies to two
> clocks; spdif_in and spdif_out. Hence, this simplification for U-Boot
> breaks down. I think the correct solution is to fix U-Boot not to
> require this simplification, renumber the clocks in the CAR binding in
> a completely arbitrary fashion, and require U-Boot to contain a mapping
> table between CAR's DT clock IDs and any other information related to
> those clocks. Do you see any alternative? We really need the two SPDIF
> clocks to be different clock IDs in the binding, since each has a
> completely independant mux for the clock source/parent, and the two
> clocks obviously can't share the same clock ID (well, hmm, perhaps they
> could if we used 2 cells for the specifier, 1 for the bit number, and
> one more to differentiate clocks that use the same bit...). I'm still
> inclined to simply push back on U-Boot to do it right.

Are SPDIF and VI the only examples of this? If so, perhaps just having
a special extra clock ID for those two and mapping in a special way
would be more efficient. So two IDs would map to one clock/reset bit
but different clocks.

Also I note that one is an input and one is an output clock. So we
could name them this way, and use the separate ID for the input clocks
perhaps.

If you do add an arbitrary mapping, what would it be? It might as well
follow along with the registers so far as it can, since the device
tree should describe the hardware. Then the mapping becomes a few
lines of code in the driver instead of YAT (yet another table).

Poor U-Boot doesn't even use the SPDIF or vi controllers.

>
>  .../bindings/clock/nvidia,tegra20-car.txt          |  156 ++++++++++++++++++++
>  arch/arm/boot/dts/tegra-seaboard.dts               |   18 +++
>  arch/arm/boot/dts/tegra20.dtsi                     |    7 +
>  3 files changed, 181 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..71cfdd2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt
> @@ -0,0 +1,156 @@
> +* 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-names : Should contain a list of strings, with values "32k_in",
> +  and "osc".
> +- #clock-cells : Should be 1.
> +  In clock consumers, this cell represents the clock ID exposed by the CAR.
> +  For a list of valid values, see the clock-output-names property.
> +
> +Optional properties :
> +- clock-output-names : Should contain a list of strings defining the clocks
> +  that the CAR provides. The list of names and clock IDs is below. The IDs
> +  may be used in clock specifiers. The names should be listed in this clock-
> +  output-names property, sorted in ID order, if this property is present.
> +
> +  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.

I hope that at some point these would have symbolic names so that we
don't need to use open-coded integers in the device tree cells.

> +
> +    0   "cpu"
> +    1   "unassigned"
> +    2   "unassigned"    FIXME: Is it OK to have 2 identical (unused) names?

Or in fact just empty so save space?

> +    3   "ac97"
> +    4   "rtc"
> +    5   "tmr"
> +    6   "uart1"
> +    7   "uart2"
> +    8   "gpio"
> +    9   "sdmmc2"
> +    10  "spdif"         FIXME: One enable bit controls two clocks!!!
> +    11  "i2s1"
> +    12  "i2c1"
> +    13  "ndflash"
> +    14  "sdmmc1"
> +    15  "sdmmc4"
> +    16  "twc"
> +    17  "pwm"
> +    18  "i2s2"
> +    19  "epp"
> +    20  "vi"            FIXME: One enable bit controls two clocks!!!
> +    21  "2d"
> +    22  "usbd"
> +    23  "isp"
> +    24  "3d"
> +    25  "ide"
> +    26  "disp2"
> +    27  "disp1"
> +    28  "host1x"
> +    29  "vcp"
> +    30  "unassigned"
> +    31  "cache2"
> +
> +    32  "mem"
> +    33  "ahbdma"
> +    34  "apbdma"
> +    35  "unassigned"
> +    36  "kbc"
> +    37  "stat_mon"
> +    38  "pmc"
> +    39  "fuse"
> +    40  "kfuse"
> +    41  "sbc1"
> +    42  "snor"
> +    43  "spi"
> +    44  "sbc2"
> +    45  "xio"
> +    46  "sbc3"
> +    47  "dvc_i2c"
> +    48  "dsi"
> +    49  "tvo"
> +    50  "mipi"
> +    51  "hdmi"
> +    52  "csi"
> +    53  "tvdac"
> +    54  "i2c2"
> +    55  "uart3"
> +    56  "unassigned"
> +    57  "emc"
> +    58  "usb2"
> +    59  "usb3"
> +    60  "mpe"
> +    61  "vde"
> +    62  "bsea"
> +    63  "bsev"
> +
> +    64  "speedo"
> +    65  "uart4"
> +    66  "uart5"
> +    67  "i2c3"
> +    68  "sbc4"
> +    69  "sdmmc3"
> +    70  "pcie"
> +    71  "owr"
> +    72  "afi"
> +    73  "csite"
> +    74  "unassigned"
> +    75  "avpucq"
> +    76  "unassigned"
> +    77  "unassigned"
> +    78  "unassigned"
> +    79  "unassigned"
> +    80  "unassigned"
> +    81  "unassigned"
> +    82  "unassigned"
> +    83  "unassigned"
> +    84  "irama"
> +    85  "iramb"
> +    86  "iramc"
> +    87  "iramd"
> +    88  "cram2"
> +    89  "audio_2x"
> +    90  "clk_d"
> +    91  "unassigned"
> +    92  "sus"
> +    93  "cdev1"
> +    94  "cdev2"
> +    95  "unassigned"
> +
> +Example:
> +
> +clocks {
> +       clk_32k: oscillator@0 {
> +               compatible = "fixed-clock";
> +               #clock-cells = <0>;
> +               clock-frequency = <32768>;
> +       };
> +
> +       osc: oscillator@1 {
> +               compatible = "fixed-clock";
> +               #clock-cells = <0>;
> +               clock-frequency = <12000000>;
> +       };
> +};
> +
> +tegar_car: clock@60006000 {
> +       compatible = "tegra,periphclk";
> +       reg = <0x60006000 1000>;

0x1000 ?

> +       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/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
> index b55a02e..f9347fe 100644
> --- a/arch/arm/boot/dts/tegra-seaboard.dts
> +++ b/arch/arm/boot/dts/tegra-seaboard.dts
> @@ -11,6 +11,24 @@
>                reg = < 0x00000000 0x40000000 >;
>        };
>
> +       clocks {
> +               clk_32k: oscillator@0 {
> +                       compatible = "fixed-clock";
> +                       #clock-cells = <0>;
> +                       clock-frequency = <32768>;
> +               };
> +
> +               osc: oscillator@1 {
> +                       compatible = "fixed-clock";
> +                       #clock-cells = <0>;
> +                       clock-frequency = <12000000>;
> +               };

Is there a reason why these two nodes are in the tegra20.dtsi include
file? Is it because some boards don't have one of these clocks? Or
because the frequency is not know? If the former, perhaps used status
= "disabled" and if the latter perhaps define only the frequency in
this file? Just trying to reduce boilerplate and complexity for board
bring-up engineers.

Also the oscillator frequency seems to be detected at run-time. What
happens if it doesn't match the fdt? Should be not detect it at
run-time?

> +       };
> +
> +       clock@60006000 {
> +               clocks = <&clk_32k> <&osc>;

Why is the clocks property here, but the clock-names property in the
include file?

> +       };
> +
>        i2c@7000c000 {
>                clock-frequency = <400000>;
>        };
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 3da7afd..be4abd2 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -4,6 +4,13 @@
>        compatible = "nvidia,tegra20";
>        interrupt-parent = <&intc>;
>
> +       tegar_car: clock@60006000 {

Is car a standard name? If not tegra_clk_rst would be an alternative.

> +               compatible = "tegra,periphclk";
> +               reg = <0x60006000 1000>;

0x1000 ?

> +               clock-names = "32k_in", "osc";
> +               #clock-cells = <1>;
> +       };
> +
>        intc: interrupt-controller@50041000 {
>                compatible = "arm,cortex-a9-gic";
>                interrupt-controller;
> --
> 1.7.0.4
>

Regards,
Simon

RE: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Simon Glass wrote at Sunday, January 22, 2012 11:03 AM:
> On Wed, Jan 18, 2012 at 4:16 PM, 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.
...
> > A comment on the shared enable issue:
> >
> > When enabling/disabling clocks, Tegra requires you to reset the HW module
> > that the clock is routed to. Both reset and clock enable registers are
> > part of the CAR. U-Boot currently has an API to do the reset based on a
> > "peripheral ID", which simply means a bit number in a set of 3 "reset"
> > registers. The bits in those registers share the same numbering as the
> > "clock enable" registers. Hence, to make life easy for U-Boot, I've
> > defined the clock IDs in this binding to be equal to these bit numbers.
> > However, this breaks down where there isn't a 1:1 mapping between reset
> > and clock enable bits, and clocks. For example, there's a single reset
> > and clock enable bit for the SPDIF controller, yet this applies to two
> > clocks; spdif_in and spdif_out. Hence, this simplification for U-Boot
> > breaks down. I think the correct solution is to fix U-Boot not to
> > require this simplification, renumber the clocks in the CAR binding in
> > a completely arbitrary fashion, and require U-Boot to contain a mapping
> > table between CAR's DT clock IDs and any other information related to
> > those clocks. Do you see any alternative? We really need the two SPDIF
> > clocks to be different clock IDs in the binding, since each has a
> > completely independant mux for the clock source/parent, and the two
> > clocks obviously can't share the same clock ID (well, hmm, perhaps they
> > could if we used 2 cells for the specifier, 1 for the bit number, and
> > one more to differentiate clocks that use the same bit...). I'm still
> > inclined to simply push back on U-Boot to do it right.
> 
> Are SPDIF and VI the only examples of this?

I think so. I should double-check to be sure though before committing
to a final binding.

> If so, perhaps just having
> a special extra clock ID for those two and mapping in a special way
> would be more efficient. So two IDs would map to one clock/reset bit
> but different clocks.

I thought about that initially, but it doesn't make semantic sense;
there isn't a 1:1 mapping between clock ID and reset bit, so I don't
think we should pretend there is for some clocks, and special-case
others.

> Also I note that one is an input and one is an output clock. So we
> could name them this way, and use the separate ID for the input clocks
> perhaps.

I don't think that solves the problem; HW modules and drivers use both
clocks, so special-casing the input clocks doesn't make the problem
less relevant.

> If you do add an arbitrary mapping, what would it be? It might as well
> follow along with the registers so far as it can, since the device
> tree should describe the hardware. Then the mapping becomes a few
> lines of code in the driver instead of YAT (yet another table).

The mapping would be that the clock IDs are unrelated to the bit numbers
in the CAR's reset/clock-enable registers.

In practice, this means that to find the reset bit number, you need a
table indexed by the .dts's clock number, with the bit number being
retrieved from that table.

So instead of bit = of_get_u32(1), we'd have bit = table[of_get_u32(1)].

In practice, I believe we'll need such a table anyway, since each clock
has many more parameters than just the reset bit ID; some clocks have
no reset bit at all, some clocks have a mux but some don't, the inputs
to the mux are different for each clock, some have a divider but some
don't, where there's a divider or mux, you need to know the register
address to configure them, each clock has a different max rate, etc.

Having thought a little more about this, I'm definitely leaning towards
this way; making the clock ID and register bit completely unrelated just
to make it really obvious that you can't use the clock ID as a register
bit.

-- 
nvpublic

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Peter De Schrijver]

What about the peripheral resets which are also handled by CAR? Peripheral
clock nodes also offer assert and deassert methods for the reset signal
associated with them. Those methods are used when powergating domains for
example. Should we model this in the same binding?

Thanks,

Peter.

RE: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Peter De Schrijver wrote at Tuesday, January 24, 2012 2:53 AM:
> What about the peripheral resets which are also handled by CAR? Peripheral
> clock nodes also offer assert and deassert methods for the reset signal
> associated with them. Those methods are used when powergating domains for
> example. Should we model this in the same binding?

In most cases, I think the resets are handled purely within clock enable
and disable, so there's no need to explicitly manage them or represent
them in the device tree. Do you agree here?

I recall that you mentioned some power-management code might need to
manage reset separately though. Can you explain why a module would be
reset separately from disabling the clocks though?

If we do need this, I think we can just add a property to the node of
each device that needs such explicit management. Something like:

tegra_car: clock@60006000 {
    compatible = "nvidia,tegra20-car";
    ....
};

foo@70007000 {
    compatible = "nvidia,tegra20-foo";
    regs = <...>;
    nvidia,car-reset-id = <&tegra_car 30>;
};

And the driver for "foo" can ignore cell 0, and extract the cell 1 and
pass the value to tegra_periph_reset_assert().

I'd expect to put the CAR phandle into the property in case we ever get
a more generalized reset subsystem, and need more general code to
interpret the property. I did the same for the Tegra APB DMA controller
client binding where clients need to know the "DMA select" value.

Does that all seem reasonable?

-- 
nvpublic

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Colin Cross]

On Tue, Jan 24, 2012 at 2:08 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Peter De Schrijver wrote at Tuesday, January 24, 2012 2:53 AM:
>> What about the peripheral resets which are also handled by CAR? Peripheral
>> clock nodes also offer assert and deassert methods for the reset signal
>> associated with them. Those methods are used when powergating domains for
>> example. Should we model this in the same binding?
>
> In most cases, I think the resets are handled purely within clock enable
> and disable, so there's no need to explicitly manage them or represent
> them in the device tree. Do you agree here?

clk_enable could force a deasserted reset, but clk_disable cannot
imply asserting reset.  The clocks need to be turned off for power
management without resetting the registers.

> I recall that you mentioned some power-management code might need to
> manage reset separately though. Can you explain why a module would be
> reset separately from disabling the clocks though?

The TRM lists a very specific sequence of clocks, resets, clamps, and
power for power domain gating.  Other than that, I think the only use
for directly calling reset that ended up in the Android Tegra2 tree
was for error recovery on HW blocks that could get locked up, probably
I2C.

> If we do need this, I think we can just add a property to the node of
> each device that needs such explicit management. Something like:
>
> tegra_car: clock@60006000 {
>    compatible = "nvidia,tegra20-car";
>    ....
> };
>
> foo@70007000 {
>    compatible = "nvidia,tegra20-foo";
>    regs = <...>;
>    nvidia,car-reset-id = <&tegra_car 30>;
> };
>
> And the driver for "foo" can ignore cell 0, and extract the cell 1 and
> pass the value to tegra_periph_reset_assert().
>
> I'd expect to put the CAR phandle into the property in case we ever get
> a more generalized reset subsystem, and need more general code to
> interpret the property. I did the same for the Tegra APB DMA controller
> client binding where clients need to know the "DMA select" value.
>
> Does that all seem reasonable?
>
> --
> nvpublic
>

RE: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Stephen Warren]

Colin Cross wrote at Tuesday, January 24, 2012 3:33 PM:
> On Tue, Jan 24, 2012 at 2:08 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Peter De Schrijver wrote at Tuesday, January 24, 2012 2:53 AM:
> >> What about the peripheral resets which are also handled by CAR? Peripheral
> >> clock nodes also offer assert and deassert methods for the reset signal
> >> associated with them. Those methods are used when powergating domains for
> >> example. Should we model this in the same binding?
> >
> > In most cases, I think the resets are handled purely within clock enable
> > and disable, so there's no need to explicitly manage them or represent
> > them in the device tree. Do you agree here?
> 
> clk_enable could force a deasserted reset, but clk_disable cannot
> imply asserting reset.  The clocks need to be turned off for power
> management without resetting the registers.

Yes, I just spoke to someone off-list, and he said the same thing. He
went on to say that therefore even the reset removal with clk_enable
was questionable, and that drivers should explicitly manage reset
removal themselves. Does that seem a reasonable stance? It'd certainly
take away the somewhat asymmetric nature of reset removal just magically
working when you first enable the clocks. We'd presumably then want a
common reset infra-structure and binding to match that change?

(I know that'd make Simon happy, since then we'd be able to represent
the reset IDs directly everywhere, unrelated to the clock IDs, and
hence he could just use the reset IDs directly in the U-Boot code he's
writing and ignore the non 1:1 mapping between clock and reset IDs)

> > I recall that you mentioned some power-management code might need to
> > manage reset separately though. Can you explain why a module would be
> > reset separately from disabling the clocks though?
> 
> The TRM lists a very specific sequence of clocks, resets, clamps, and
> power for power domain gating.  Other than that, I think the only use
> for directly calling reset that ended up in the Android Tegra2 tree
> was for error recovery on HW blocks that could get locked up, probably
> I2C.

OK, those reasons make sense.

-- 
nvpublic

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Colin Cross]

On Tue, Jan 24, 2012 at 2:43 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Colin Cross wrote at Tuesday, January 24, 2012 3:33 PM:
>> On Tue, Jan 24, 2012 at 2:08 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > Peter De Schrijver wrote at Tuesday, January 24, 2012 2:53 AM:
>> >> What about the peripheral resets which are also handled by CAR? Peripheral
>> >> clock nodes also offer assert and deassert methods for the reset signal
>> >> associated with them. Those methods are used when powergating domains for
>> >> example. Should we model this in the same binding?
>> >
>> > In most cases, I think the resets are handled purely within clock enable
>> > and disable, so there's no need to explicitly manage them or represent
>> > them in the device tree. Do you agree here?
>>
>> clk_enable could force a deasserted reset, but clk_disable cannot
>> imply asserting reset.  The clocks need to be turned off for power
>> management without resetting the registers.
>
> Yes, I just spoke to someone off-list, and he said the same thing. He
> went on to say that therefore even the reset removal with clk_enable
> was questionable, and that drivers should explicitly manage reset
> removal themselves. Does that seem a reasonable stance? It'd certainly
> take away the somewhat asymmetric nature of reset removal just magically
> working when you first enable the clocks. We'd presumably then want a
> common reset infra-structure and binding to match that change?

In 99% of drivers reset is irrelevant, as long as the block is out of
reset by the time the driver starts writing to registers.  clk_enable
is a decent place to ensure that - it always has to be done before
writing to the registers, and it maps nicely to the reset bits on
Tegra.  It would be nice if those driver didn't need to deassert reset
explicitly, especially if that requires a Tegra-specific API.

> (I know that'd make Simon happy, since then we'd be able to represent
> the reset IDs directly everywhere, unrelated to the clock IDs, and
> hence he could just use the reset IDs directly in the U-Boot code he's
> writing and ignore the non 1:1 mapping between clock and reset IDs)
>
>> > I recall that you mentioned some power-management code might need to
>> > manage reset separately though. Can you explain why a module would be
>> > reset separately from disabling the clocks though?
>>
>> The TRM lists a very specific sequence of clocks, resets, clamps, and
>> power for power domain gating.  Other than that, I think the only use
>> for directly calling reset that ended up in the Android Tegra2 tree
>> was for error recovery on HW blocks that could get locked up, probably
>> I2C.
>
> OK, those reasons make sense.
>
> --
> nvpublic
>

Re: [PATCH] ARM: tegra: Define Tegra20 CAR binding

[Peter De Schrijver]

On Tue, Jan 24, 2012 at 11:59:40PM +0100, Colin Cross wrote:
> On Tue, Jan 24, 2012 at 2:43 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Colin Cross wrote at Tuesday, January 24, 2012 3:33 PM:
> >> On Tue, Jan 24, 2012 at 2:08 PM, Stephen Warren <swarren-DDmLM1+adcphl2p70BpVqQ@public.gmane.orgm> wrote:
> >> > Peter De Schrijver wrote at Tuesday, January 24, 2012 2:53 AM:
> >> >> What about the peripheral resets which are also handled by CAR? Peripheral
> >> >> clock nodes also offer assert and deassert methods for the reset signal
> >> >> associated with them. Those methods are used when powergating domains for
> >> >> example. Should we model this in the same binding?
> >> >
> >> > In most cases, I think the resets are handled purely within clock enable
> >> > and disable, so there's no need to explicitly manage them or represent
> >> > them in the device tree. Do you agree here?
> >>
> >> clk_enable could force a deasserted reset, but clk_disable cannot
> >> imply asserting reset.  The clocks need to be turned off for power
> >> management without resetting the registers.
> >
> > Yes, I just spoke to someone off-list, and he said the same thing. He
> > went on to say that therefore even the reset removal with clk_enable
> > was questionable, and that drivers should explicitly manage reset
> > removal themselves. Does that seem a reasonable stance? It'd certainly
> > take away the somewhat asymmetric nature of reset removal just magically
> > working when you first enable the clocks. We'd presumably then want a
> > common reset infra-structure and binding to match that change?
> 
> In 99% of drivers reset is irrelevant, as long as the block is out of
> reset by the time the driver starts writing to registers.  clk_enable
> is a decent place to ensure that - it always has to be done before
> writing to the registers, and it maps nicely to the reset bits on
> Tegra.  It would be nice if those driver didn't need to deassert reset
> explicitly, especially if that requires a Tegra-specific API.

I think this could be solved by moving the drivers to runtime PM and handle the
clock and reset bits in tegra platform device specific code. iirc that's the
way it's handled in OMAP?

Cheers,

Peter.