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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/20/2012 9:32 PM, Olof Johansson wrote:
Hi,

On Thu, Jan 19, 2012 at 9:17 AM, Stephen Warren<swarren@xxxxxxxxxx>  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


--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux