From: swarren@wwwdotorg•org (Stephen Warren)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 01/31] ARM: tegra: add missing clock documentation to DT bindings
Date: Wed, 04 Dec 2013 10:34:17 -0700 [thread overview]
Message-ID: <529F6799.1070609@wwwdotorg.org> (raw)
In-Reply-To: <20131204084811.GF19943@ulmo.nvidia.com>
On 12/04/2013 01:48 AM, Thierry Reding wrote:
> On Tue, Dec 03, 2013 at 11:31:00AM -0700, Stephen Warren wrote:
>> On 12/02/2013 01:52 AM, Thierry Reding wrote:
>>> On Sun, Dec 01, 2013 at 12:05:44PM -0700, Stephen Warren
>>> wrote:
>>>> On 11/29/2013 04:49 AM, Thierry Reding wrote:
>>>>> On Fri, Nov 15, 2013 at 01:53:56PM -0700, Stephen Warren
>>>>> wrote: [...]
>>>>>> @@ -60,6 +81,12 @@ of the following host1x client
>>>>>> modules: - compatible: "nvidia,tegra<chip>-dc" - reg:
>>>>>> Physical base address and length of the controller's
>>>>>> registers. - interrupts: The interrupt outputs from the
>>>>>> controller. + - clocks : Must contain an entry for each
>>>>>> entry in clock-names. + See
>>>>>> ../clocks/clock-bindings.txt for details. + -
>>>>>> clock-names : Must include the following entries: + -
>>>>>> disp1 or disp2 (depending on the controller instance)
>>>>>
>>>>> I'm not sure if this makes sense. The name could be the
>>>>> same independent of which controller uses it. If it isn't
>>>>> then the driver would need additional code to find out
>>>>> which instance it is and construct a dynamic string.
>>>>>
>>>>> Any objection to just make this entry "disp", or "dc"?
>>>>
>>>> This patch simply documents the binding that the various
>>>> drivers already require and/or whatever is already in the DT
>>>> files if there are any clocks the drivers don't currently
>>>> use. I did consider fixing up all the current usage to
>>>> actually be sane, but that would require even more driver
>>>> changes (in addition to those required for the reset
>>>> framework patches).
>>>
>>> Okay, I understand. I still think we should change the usage
>>> for this particular use-case subsequently. In retrospect the
>>> entry in clock-names wasn't thought out very well. It seems
>>> like the reason for using disp1 and disp2 respectively was so
>>> that it would match the system-wide clock name, rather than
>>> the clock's label within the display controller's context.
>>>
>>> Just to clarify what I mean, if we stick to the above, then
>>> we'll need to add code to the driver along the lines of:
>>>
>>> char clock_name[6];
>>>
>>> if (regs->start == 0x54200000) index = 1; else index = 2;
>>>
>>> sprintf(clock_name, "disp%u", index);
>>>
>>> clk = devm_clk_get(&pdev->dev, clock_name);
>>>
>>> rather than the much more simple and elegant:
>>>
>>> clk = devm_clk_get(&pdev->dev, "disp");
>>>
>>> The whole purpose of the clock consumer ID is to be generic
>>> and as such independent of the specific IP block or instance
>>> thereof.
>>
>> I think if the code needs this clock, I'd be tempted to do the
>> following:
>>
>> clk = clk_get(dev, "disp1"); if (IS_ERR(clk) && PTR_ERR(clk) !=
>> -EPROBE_DEFERRED) clk = clk_get(dev, "disp2"); if (IS_ERR(clk))
>> return PTR_ERR(clk);
>>
>> That avoids having to hard-code IP block base addresses and
>> construct clock names at run-time.
>
> I think perhaps we're getting our wires crossed. What I've been
> trying to say is that I think the binding should define the first
> clock to be named simply "disp".
>
> The reason why I think "disp" is a better choice than "disp1" and
> "disp2" is that it merely encodes the purpose of the clock for the
> display controller, and doesn't contain knowledge about the
> particular instance of the display controller. That's analogous to
> I2C or SPI nodes where the clock isn't named "i2c1", "i2c2", ...
> or "spi1", "spi2", ... but simply "i2c" or "spi" respectively.
>
> I know that existing DTS files use "disp1" and "disp2",
> respectively, but I think that was a wrong choice back at the time
> and therefore I suggest that we change it while we still can (DTS
> files are changing in 3.14 anyway because of the reset and DMA
> binding updates).
>
> Is that any clearer than what I was saying before?
No, because I know what you meant before:-)
The thing I was missing is that the existing disp1/disp2 naming is
/only/ something that's in the DT. The driver (and hence the exiting
as-yet-undocumented ABI) looks up this clock as "index 0" not as "name
disp1". Hence, we /can/ change the documented name without affecting
the ABI at all, and not affecting the ABI is something I was trying to
avoid in this patch.
So, how about I fold the following into this patch:
> diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> index 42fe3a498e71..ab45c02aa658 100644
> --- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> +++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> @@ -111,7 +111,7 @@ of the following host1x client modules:
> - clocks: Must contain an entry for each entry in clock-names.
> See ../clocks/clock-bindings.txt for details.
> - clock-names: Must include the following entries:
> - - disp1 or disp2 (depending on the controller instance)
> + - dc
> This MUST be the first entry.
> - parent
> - resets: Must contain an entry for each entry in reset-names.
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 01c20c7dbb51..648c494e927f 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -89,7 +89,7 @@
> interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&tegra_car TEGRA20_CLK_DISP1>,
> <&tegra_car TEGRA20_CLK_PLL_P>;
> - clock-names = "disp1", "parent";
> + clock-names = "dc", "parent";
> resets = <&tegra_car 27>;
> reset-names = "dc";
>
> @@ -104,7 +104,7 @@
> interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&tegra_car TEGRA20_CLK_DISP2>,
> <&tegra_car TEGRA20_CLK_PLL_P>;
> - clock-names = "disp2", "parent";
> + clock-names = "dc", "parent";
> resets = <&tegra_car 26>;
> reset-names = "dc";
>
> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> index 58a3dca24c49..829eb4b5091d 100644
> --- a/arch/arm/boot/dts/tegra30.dtsi
> +++ b/arch/arm/boot/dts/tegra30.dtsi
> @@ -165,7 +165,7 @@
> interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&tegra_car TEGRA30_CLK_DISP1>,
> <&tegra_car TEGRA30_CLK_PLL_P>;
> - clock-names = "disp1", "parent";
> + clock-names = "dc", "parent";
> resets = <&tegra_car 27>;
> reset-names = "dc";
>
> @@ -180,7 +180,7 @@
> interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&tegra_car TEGRA30_CLK_DISP2>,
> <&tegra_car TEGRA30_CLK_PLL_P>;
> - clock-names = "disp2", "parent";
> + clock-names = "dc", "parent";
> resets = <&tegra_car 26>;
> reset-names = "dc";
>
next prev parent reply other threads:[~2013-12-04 17:34 UTC|newest]
Thread overview: 176+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-15 20:53 [PATCH 00/31] ARM: tegra: use common reset and DMA bindings Stephen Warren
2013-11-15 20:53 ` [PATCH 01/31] ARM: tegra: add missing clock documentation to DT bindings Stephen Warren
2013-11-16 22:00 ` Marc Dietrich
2013-11-18 17:36 ` Stephen Warren
2013-11-29 11:49 ` Thierry Reding
2013-12-01 19:05 ` Stephen Warren
2013-12-02 8:52 ` Thierry Reding
2013-12-03 18:31 ` Stephen Warren
2013-12-04 8:48 ` Thierry Reding
2013-12-04 17:34 ` Stephen Warren [this message]
2013-12-04 19:27 ` Thierry Reding
2013-12-03 18:36 ` Stephen Warren
2013-12-04 8:49 ` Thierry Reding
2013-11-15 20:53 ` [PATCH 02/31] ARM: tegra: document reset properties in " Stephen Warren
2013-11-29 12:23 ` Thierry Reding
2013-12-01 19:06 ` Stephen Warren
2013-12-02 9:08 ` Thierry Reding
2013-12-03 18:48 ` Stephen Warren
2013-12-04 8:56 ` Thierry Reding
2013-11-15 20:53 ` [PATCH 03/31] ARM: tegra: document use of standard DMA " Stephen Warren
2013-11-29 12:29 ` Thierry Reding
2013-12-01 19:09 ` Stephen Warren
2013-12-02 9:05 ` Thierry Reding
2013-12-03 18:52 ` Stephen Warren
2013-12-04 8:56 ` Thierry Reding
2013-11-15 20:53 ` [PATCH 04/31] ARM: tegra: update DT files to add reset properties Stephen Warren
2013-11-29 13:00 ` Thierry Reding
2013-12-01 19:15 ` Stephen Warren
2013-12-02 9:01 ` Thierry Reding
2013-12-03 18:59 ` Stephen Warren
2013-11-15 20:54 ` [PATCH 05/31] ARM: tegra: update DT files to add DMA properties Stephen Warren
2013-11-29 13:08 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 06/31] ARM: tegra: select the reset framework Stephen Warren
2013-11-29 13:10 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 07/31] clk: tegra: implement a reset driver Stephen Warren
2013-11-29 13:26 ` Thierry Reding
2013-12-03 19:07 ` Stephen Warren
2013-11-15 20:54 ` [PATCH 08/31] pci: tegra: use reset framework Stephen Warren
2013-11-15 21:16 ` Bjorn Helgaas
2013-11-29 13:29 ` Thierry Reding
2013-11-29 13:33 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 09/31] drm/tegra: " Stephen Warren
2013-11-29 13:42 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 10/31] ARM: tegra: pass reset to tegra_powergate_sequence_power_up() Stephen Warren
2013-11-15 21:17 ` Bjorn Helgaas
2013-11-29 13:45 ` Thierry Reding
2013-11-29 13:46 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 11/31] dma: add channel request API that supports deferred probe Stephen Warren
2013-11-15 21:01 ` Dan Williams
2013-11-15 21:05 ` Dan Williams
2013-11-18 9:18 ` Shevchenko, Andriy
2013-11-18 17:42 ` Stephen Warren
2013-11-19 12:00 ` Andy Shevchenko
2013-11-19 17:15 ` Stephen Warren
2013-11-19 23:37 ` Dan Williams
2013-11-20 0:09 ` Stephen Warren
2013-11-20 0:38 ` Dan Williams
2013-11-20 18:24 ` Stephen Warren
2013-11-20 19:15 ` Dan Williams
2013-11-20 19:22 ` Stephen Warren
2013-11-20 20:23 ` Williams, Dan J
2013-11-20 21:24 ` Stephen Warren
2013-11-21 3:22 ` Dan Williams
2013-11-21 9:13 ` Andy Shevchenko
2013-11-21 18:22 ` Stephen Warren
2013-11-22 6:54 ` Dan Williams
2013-11-22 17:34 ` Stephen Warren
2013-11-22 18:04 ` Dan Williams
2013-11-22 18:10 ` Stephen Warren
2013-11-22 19:49 ` Dan Williams
2013-11-22 19:53 ` Stephen Warren
2013-11-22 20:46 ` Dan Williams
2013-11-22 21:50 ` Stephen Warren
2013-11-22 23:13 ` Dan Williams
2013-11-22 23:45 ` Stephen Warren
2013-11-23 0:40 ` Russell King - ARM Linux
2013-11-23 0:34 ` Russell King - ARM Linux
2013-11-25 17:26 ` Stephen Warren
2013-11-25 17:45 ` Dan Williams
2013-11-25 18:00 ` Russell King - ARM Linux
2013-11-25 18:07 ` Stephen Warren
2013-11-25 18:42 ` Dan Williams
2013-11-25 19:00 ` Stephen Warren
2013-11-25 19:28 ` Dan Williams
2013-11-25 19:30 ` Stephen Warren
2013-11-25 19:45 ` Dan Williams
2013-11-25 19:47 ` Stephen Warren
2013-11-25 19:09 ` Russell King - ARM Linux
2013-11-25 17:53 ` Russell King - ARM Linux
2013-11-25 17:57 ` Stephen Warren
2013-11-25 20:28 ` Gerhard Sittig
2013-11-25 20:52 ` Russell King - ARM Linux
2013-11-28 21:20 ` NULL clock items (was: [PATCH 11/31] dma: add channel request API that supports deferred probe) Gerhard Sittig
2013-11-22 23:45 ` [PATCH 11/31] dma: add channel request API that supports deferred probe Dan Williams
2013-11-23 0:17 ` Stephen Warren
2013-11-23 0:37 ` Dan Williams
2013-11-15 23:08 ` Stephen Warren
2013-11-22 23:50 ` Dan Williams
2013-11-23 0:05 ` Stephen Warren
2013-11-15 20:54 ` [PATCH 12/31] dma: tegra: use reset framework Stephen Warren
2013-11-25 22:11 ` Stephen Warren
2013-11-29 13:47 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 13/31] dma: tegra: register as an OF DMA controller Stephen Warren
2013-11-20 15:28 ` Arnd Bergmann
2013-11-20 18:22 ` Stephen Warren
2013-11-15 20:54 ` [PATCH 14/31] ASoC: dmaengine: support deferred probe for DMA channels Stephen Warren
2013-11-16 9:29 ` Mark Brown
2013-11-16 10:49 ` [alsa-devel] " Lars-Peter Clausen
2013-11-18 17:59 ` Stephen Warren
2013-11-15 20:54 ` [PATCH 15/31] ASoC: dmaengine: add custom DMA config to snd_dmaengine_pcm_config Stephen Warren
2013-11-16 9:44 ` Mark Brown
2013-11-18 18:45 ` Stephen Warren
2013-11-19 9:35 ` Mark Brown
2013-11-16 10:43 ` [alsa-devel] " Lars-Peter Clausen
2013-11-15 20:54 ` [PATCH 16/31] ASoC: tegra: use reset framework Stephen Warren
2013-11-16 9:55 ` Mark Brown
2013-11-18 17:21 ` Stephen Warren
2013-11-18 18:37 ` Mark Brown
2013-11-25 21:56 ` Stephen Warren
2013-11-26 13:14 ` Mark Brown
2013-11-26 16:31 ` Stephen Warren
2013-11-26 18:37 ` Mark Brown
2013-11-26 18:45 ` Stephen Warren
2013-11-15 20:54 ` [PATCH 17/31] ASoC: tegra: call pm_runtime APIs around register accesses Stephen Warren
2013-11-16 10:02 ` Mark Brown
2013-11-18 17:25 ` Stephen Warren
2013-11-18 18:39 ` Mark Brown
2013-11-18 22:38 ` Stephen Warren
2013-11-19 9:53 ` Mark Brown
2013-11-15 20:54 ` [PATCH 18/31] ASoC: tegra: allocate AHUB FIFO during probe() not startup() Stephen Warren
2013-11-16 10:04 ` Mark Brown
2013-11-29 14:40 ` Thierry Reding
2013-12-03 19:55 ` Stephen Warren
2013-12-04 9:00 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 19/31] ASoC: tegra: convert to standard DMA DT bindings Stephen Warren
2013-11-16 10:05 ` Mark Brown
2013-11-15 20:54 ` [PATCH 20/31] i2c: tegra: use reset framework Stephen Warren
2013-11-15 22:20 ` Wolfram Sang
2013-11-29 14:46 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 21/31] staging: nvec: " Stephen Warren
2013-11-16 22:33 ` Marc Dietrich
2013-11-19 23:23 ` Greg Kroah-Hartman
2013-11-29 14:47 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 22/31] spi: tegra: " Stephen Warren
2013-11-16 10:07 ` Mark Brown
2013-11-29 14:48 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 23/31] spi: tegra: convert to standard DMA DT bindings Stephen Warren
2013-11-16 10:14 ` Mark Brown
2013-11-18 17:30 ` Stephen Warren
2013-11-18 18:41 ` Mark Brown
2013-11-15 20:54 ` [PATCH 24/31] serial: tegra: use reset framework Stephen Warren
2013-11-19 23:24 ` Greg Kroah-Hartman
2013-11-29 14:49 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 25/31] serial: tegra: convert to standard DMA DT bindings Stephen Warren
2013-11-19 23:23 ` Greg Kroah-Hartman
2013-11-15 20:54 ` [PATCH 26/31] Input: tegra-kbc - use reset framework Stephen Warren
2013-11-19 21:17 ` Dmitry Torokhov
2013-11-29 14:50 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 27/31] USB: EHCI: tegra: " Stephen Warren
2013-11-16 18:12 ` Alan Stern
2013-11-19 23:24 ` Greg Kroah-Hartman
2013-11-29 14:51 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 28/31] ARM: tegra: remove legacy clock entries from DT Stephen Warren
2013-11-29 14:53 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 29/31] ARM: tegra: remove legacy DMA " Stephen Warren
2013-11-29 14:53 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 30/31] clk: tegra: remove legacy reset APIs Stephen Warren
2013-11-29 14:55 ` Thierry Reding
2013-11-15 20:54 ` [PATCH 31/31] clk: tegra: remove bogus PCIE_XCLK Stephen Warren
2013-11-29 14:56 ` Thierry Reding
2013-11-18 8:24 ` [PATCH 00/31] ARM: tegra: use common reset and DMA bindings Terje Bergström
2013-11-20 15:37 ` Arnd Bergmann
2013-11-20 16:45 ` Stephen Warren
2013-11-20 17:03 ` Arnd Bergmann
2013-11-20 17:23 ` Stephen Warren
2013-12-12 0:11 ` Stephen Warren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=529F6799.1070609@wwwdotorg.org \
--to=swarren@wwwdotorg$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox