| Message ID | 20131219123719.3226.44864.stgit@tamien (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/19/2013 05:49 AM, Paul Walmsley wrote: > Add basic DT bindings for the DFLL IP block for the NVIDIA Tegra114 SoC. > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt > +- clocks : Must contain an array of two-cell arrays, one per clock. > + DFLL source clocks. At minimum this should include the > + reference clock source and the IP block's main clock > + source. Also it should contain the DFLL's I2C controller > + clock source. The format is <&clock-provider-phandle > + clock-id>. Entries in "clocks" aren't two cells, they're a phandle plus as many cells as the node referenced by the phandle specifies. > + > +- clock-names : Must contain an array of strings, one per 'clocks' > + two-cell array. The position in the array of these clock-names defines the set of entries in clocks, not the other way around. > + strings must correspond to the position in the 'clocks' > + array (see above). The DFLL driver currently requires > + the "soc", "ref", and "i2c" clock names to be populated. The standard wording used by all the Tegra clock client bindings is now: - clocks : Must contain an entry for each entry in clock-names. See clock-bindings.txt for details. - clock-names : Must include the following entries: - soc - ref - i2c For consistency, it'd be nice to adopt the same style here. > +Optional properties: > + > +- status : device availability -- managed by the DT integration code, not > + the DFLL driver. Should be set to "disabled" in the SoC > + DTS file. That's such a core property that it's not worth documenting in every single binding. > + Blank line at EOF.
(Resending, and revising - the original wasn't fully baked in several regards..) Hello Stephen, thanks for your review. On Thu, 19 Dec 2013, Stephen Warren wrote: > On 12/19/2013 05:49 AM, Paul Walmsley wrote: >> Add basic DT bindings for the DFLL IP block for the NVIDIA Tegra114 SoC. > >> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt > >> +- clocks : Must contain an array of two-cell arrays, one per clock. >> + DFLL source clocks. At minimum this should include the >> + reference clock source and the IP block's main clock >> + source. Also it should contain the DFLL's I2C controller >> + clock source. The format is <&clock-provider-phandle >> + clock-id>. > > Entries in "clocks" aren't two cells, they're a phandle plus as many > cells as the node referenced by the phandle specifies. It's worth noting that the clock binding documentation itself refers to pairs: ---- clocks: List of phandle and clock specifier pairs, one pair for each clock input to the device. Note: if the clock provider specifies '0' for #clock-cells, then only the phandle portion of the pair will appear. ---- https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n50 But given the ambiguity of that documentation, I basically agree, so have changed it to: - clocks : Must contain an array of clock specifiers, one per clock. DFLL source clocks. At minimum this should include the reference clock source and the IP block's main clock source. Also it should contain the DFLL's I2C controller clock source. The format is <&clock-provider-phandle clock-id>. >> + >> +- clock-names : Must contain an array of strings, one per 'clocks' >> + two-cell array. The position in the array of these > > clock-names defines the set of entries in clocks, not the other way around. Hmm. Referring to the DT clock binding documentation, it lists the 'clock-names' property as optional, and the 'clocks' property as mandatory: ----- ==Clock consumers== Required properties: clocks: List of phandle and clock specifier pairs, one pair for each clock input to the device. Note: if the clock provider specifies '0' for #clock-cells, then only the phandle portion of the pair will appear. Optional properties: clock-names: List of clock input name strings sorted in the same order as the clocks property. Consumers drivers will use clock-names to match clock input names with clocks specifiers. ----- https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n47 Considering that of_clk_get() doesn't require names, the situation appears to be the way that the original patch described it (modulo the part about "two cells.") >> + strings must correspond to the position in the 'clocks' >> + array (see above). The DFLL driver currently requires >> + the "soc", "ref", and "i2c" clock names to be populated. > > The standard wording used by all the Tegra clock client bindings is now: > > - clocks : Must contain an entry for each entry in clock-names. > See clock-bindings.txt for details. > - clock-names : Must include the following entries: > - soc > - ref > - i2c > > For consistency, it'd be nice to adopt the same style here. I've altered the 'clock-names' format along the lines of what you've suggested, but have not changed the 'clocks' format, per the above discussion: ----- - clocks : Must contain an array of clock specifiers, one per clock. DFLL source clocks. At minimum this should include the reference clock source and the IP block's main clock source. Also it should contain the DFLL's I2C controller clock source. The format is <&clock-provider-phandle clock-id>. - clock-names : Must contain an array of strings, one per 'clocks' cell. The position in the array of these strings must correspond to the position in the 'clocks' array (see above). - soc - ref - i2c ----- >> +Optional properties: >> + >> +- status : device availability -- managed by the DT integration code, not >> + the DFLL driver. Should be set to "disabled" in the SoC >> + DTS file. > > That's such a core property that it's not worth documenting in every > single binding. That's fine. Removed. > >> + > > Blank line at EOF. Hehe. Removed. - Paul
On 01/13/2014 10:27 PM, Paul Walmsley wrote:
> The format is <&clock-provider-phandle clock-id>.
Ah, one other change: the above string has been dropped. Pesky examples ;-)
- Paul
On 01/13/2014 11:03 PM, Paul Walmsley wrote: > On 12/19/2013 04:05 PM, Stephen Warren wrote: >> On 12/19/2013 05:49 AM, Paul Walmsley wrote: >>> Add basic DT bindings for the DFLL IP block for the NVIDIA Tegra114 SoC. >>> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt >>> +- clock-names : Must contain an array of strings, one per 'clocks' >>> + two-cell array. The position in the array of these >> clock-names defines the set of entries in clocks, not the other way around. > > Hmm. If one refers to the DT clock binding documentation, it lists the > 'clock-names' property as optional, and the 'clocks' property as mandatory: > > ----- > > |==Clock consumers== > > Required properties: > clocks: List of phandle and clock specifier pairs, one pair > for each clock input to the device. Note: if the > clock provider specifies '0' for #clock-cells, then > only the phandle portion of the pair will appear. > > Optional properties: > clock-names: List of clock input name strings sorted in the same > order as the clocks property. Consumers drivers > will use clock-names to match clock input names > with clocks specifiers. > > ----- > | > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n47 > > Considering that of_clk_get() doesn't require names, it certainly > appears to be the way that the original patch described it (modulo the > bit about 'cells'; which has been fixed.) If the binding defines that clock must exist at certain indices in the clock property, and clock-names isn't used, then the clocks property defines which clocks must exist. However, if the binding is defining clock names, then the clock-names property is the master that defines which clocks exist. In this case, the index of entries in the clocks property is undefined by the binding; it must simply match the clock-names property, which can be arbitrarily ordered by the user. Since this is a new binding, we should use the clock-names style to ensure easy future backwards-compatible changes.
On Mon, Jan 13, 2014 at 22:27 -0800, Paul Walmsley wrote: > > On Thu, 19 Dec 2013, Stephen Warren wrote: > > >On 12/19/2013 05:49 AM, Paul Walmsley wrote: > >>Add basic DT bindings for the DFLL IP block for the NVIDIA Tegra114 SoC. > > > >>diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt > > > >>+- clocks : Must contain an array of two-cell arrays, one per clock. > >>+ DFLL source clocks. At minimum this should include the > >>+ reference clock source and the IP block's main clock > >>+ source. Also it should contain the DFLL's I2C controller > >>+ clock source. The format is <&clock-provider-phandle > >>+ clock-id>. > > > >Entries in "clocks" aren't two cells, they're a phandle plus as many > >cells as the node referenced by the phandle specifies. > > It's worth noting that the clock binding documentation itself refers > to pairs: > > ---- > > clocks: List of phandle and clock specifier pairs, one pair > for each clock input to the device. Note: if the > clock provider specifies '0' for #clock-cells, then > only the phandle portion of the pair will appear. > > ---- > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n50 > > But given the ambiguity of that documentation, I basically agree, so > have changed it to: Please note that there neither is an ambiguity nor a conflict here, and that you actually acknowledge what Stephen said: Clocks get referenced by a pair which consists of a phandle for the clock provider and a clock specifier. The clock specifier is made of as many cells as the clock provider's #clock-cells property defines (including none, or any other number than 1). This is exactly what Stephen said: A "clocks" item does not need to have two cells. The pair of phandle and clock specifier don't necessarily translate into two cells, instead the number of cells depends on the clock provider. Also note that the phandle is specifically _not_ part of the clock specifier. virtually yours Gerhard Sittig
On 01/15/2014 11:50 AM, Gerhard Sittig wrote: > On Mon, Jan 13, 2014 at 22:27 -0800, Paul Walmsley wrote: >> On Thu, 19 Dec 2013, Stephen Warren wrote: >> >>> On 12/19/2013 05:49 AM, Paul Walmsley wrote: >>>> Add basic DT bindings for the DFLL IP block for the NVIDIA Tegra114 SoC. >>>> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt >>>> +- clocks : Must contain an array of two-cell arrays, one per clock. >>>> + DFLL source clocks. At minimum this should include the >>>> + reference clock source and the IP block's main clock >>>> + source. Also it should contain the DFLL's I2C controller >>>> + clock source. The format is <&clock-provider-phandle >>>> + clock-id>. >>> Entries in "clocks" aren't two cells, they're a phandle plus as many >>> cells as the node referenced by the phandle specifies. >> It's worth noting that the clock binding documentation itself refers >> to pairs: >> >> ---- >> >> clocks: List of phandle and clock specifier pairs, one pair >> for each clock input to the device. Note: if the >> clock provider specifies '0' for #clock-cells, then >> only the phandle portion of the pair will appear. >> >> ---- >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/clock-bindings.txt#n50 >> >> But given the ambiguity of that documentation, I basically agree, so >> have changed it to: > Please note that there neither is an ambiguity nor a conflict > here, and that you actually acknowledge what Stephen said: I do not agree that the Documentation is unambiguous. It is not correct to refer to a "pair" without a second item as a "pair." > This is exactly what Stephen said: A "clocks" item does not need > to have two cells. The pair of phandle and clock specifier don't > necessarily translate into two cells, instead the number of cells > depends on the clock provider. I do agree with this, and have updated the documentation accordingly. - Paul
diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt new file mode 100644 index 000000000000..b868bf97bc3d --- /dev/null +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt @@ -0,0 +1,43 @@ +NVIDIA Tegra114 DFLL FCPU clocksource data in the SoC DTS file: + +Required properties: + +- compatible : "nvidia,tegra114-dfll-fcpu" + +- reg : Must contain the starting physical address and length for the + DFLL's MMIO register space, including the DFLL-to-I2C + controller interface and the DFLL's I2C controller. + +- clocks : Must contain an array of two-cell arrays, one per clock. + DFLL source clocks. At minimum this should include the + reference clock source and the IP block's main clock + source. Also it should contain the DFLL's I2C controller + clock source. The format is <&clock-provider-phandle + clock-id>. + +- clock-names : Must contain an array of strings, one per 'clocks' + two-cell array. The position in the array of these + strings must correspond to the position in the 'clocks' + array (see above). The DFLL driver currently requires + the "soc", "ref", and "i2c" clock names to be populated. + + +Optional properties: + +- status : device availability -- managed by the DT integration code, not + the DFLL driver. Should be set to "disabled" in the SoC + DTS file. + + +Example: + +dfll@70110000 { + compatible = "nvidia,tegra114-dfll-fcpu"; + reg = <0x70110000 0x400>; + clocks = <&tegra_car TEGRA114_CLK_DFLL_SOC>, + <&tegra_car TEGRA114_CLK_DFLL_REF>, + <&tegra_car TEGRA114_CLK_I2C5>; + clock-names = "soc", "ref", "i2c"; + status = "disabled"; +}; + diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi index ae855ec60bbd..1cd59d79e67c 100644 --- a/arch/arm/boot/dts/tegra114.dtsi +++ b/arch/arm/boot/dts/tegra114.dtsi @@ -480,6 +480,16 @@ }; }; + dfll@70110000 { + compatible = "nvidia,tegra114-dfll-fcpu"; + reg = <0x70110000 0x400>; + clocks = <&tegra_car TEGRA114_CLK_DFLL_SOC>, + <&tegra_car TEGRA114_CLK_DFLL_REF>, + <&tegra_car TEGRA114_CLK_I2C5>; + clock-names = "soc", "ref", "i2c"; + status = "disabled"; + }; + sdhci@78000000 { compatible = "nvidia,tegra114-sdhci", "nvidia,tegra30-sdhci"; reg = <0x78000000 0x200>;
Add basic DT bindings for the DFLL IP block for the NVIDIA Tegra114 SoC. Signed-off-by: Paul Walmsley <pwalmsley@nvidia.com> Cc: Matthew Longnecker <mlongnecker@nvidia.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> Cc: Kumar Gala <galak@codeaurora.org> --- .../bindings/clock/nvidia,tegra114-dfll.txt | 43 ++++++++++++++++++++ arch/arm/boot/dts/tegra114.dtsi | 10 +++++ 2 files changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra114-dfll.txt