From: Tomasz Figa <tomasz.figa@gmail.com> To: Humberto Naves <hsnaves@gmail.com> Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, devicetree@vger.kernel.org, Kukjin Kim <kgene.kim@samsung.com>, Tomasz Figa <t.figa@samsung.com>, Thomas Abraham <ta.omasab@gmail.com>, Andreas Farber <afaerber@suse.de>, Randy Dunlap <rdunlap@infradead.org>, Ian Campbell <ijc+devicetree@hellion.org.uk> Subject: Re: [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks Date: Thu, 31 Jul 2014 15:37:39 +0200 [thread overview] Message-ID: <53DA46A3.9080808@gmail.com> (raw) In-Reply-To: <CAEe5pB_7e_5YkpcuRvsaUg6vKfdu6C+XhwRYyY1uUGptxM5zkg@mail.gmail.com> On 31.07.2014 15:23, Humberto Naves wrote: > Hi Tomasz, > > I perfectly see your point. > However my question was why you did you decide to postpone > Sylwester's? Was there any specific reason? > I suppose it would break all the dtb compatibility, but besides that, > was there any other reason? We discussed this in private (we work in the same office) and I pointed out certain issues with Sylwester's proposed implementation and we agreed that one more revision of the patch is needed, but as it happens, higher priority tasks showed up and this one got lost in action. The first version of the patch [1] changed the original behavior breaking DT ABI compatibility and relied on improper assumption that those clocks are always in "fixed-rate-clocks" node. The thing is that no code should rely on DT node naming. Second version [2] was much better in this aspect, but it had some minor implementation issues - custom clk_ops used instead of generic mux clock and chipid block being constantly mapped and unmapped in every call to __fin_pll_mux_get_parent(). I should have reviewed them both on the mailing lists, but at that time there was no major activity related to Exynos4 outside of our office, so it was more convenient to just talk together directly. [1] http://www.spinics.net/lists/arm-kernel/msg333211.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/258490.html Anyway, I don't think this is all relevant to Exynos5410, which just uses the generic fixed rate binding and has the thing done right from the start. Best regards, Tomasz > > Best, > Humberto > > On Thu, Jul 31, 2014 at 2:53 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> Hi Humberto, >> >> You can find my comments inline. >> >> On 31.07.2014 13:22, Humberto Silva Naves wrote: >>> This implements the fixed rate clocks generated either inside or >>> outside the SoC. It also adds a dt-binding constant for the >>> sclk_hdmiphy clock, which shall be later used by other drivers, >>> such as the DRM. >>> >>> Since the external fixed rate clock fin_pll is now registered by >>> the clk-exynos5410 file, the bindings with the device tree file have >>> changed. It is no longer needed to define fin_pll as a fixed clock, >>> such as in: >>> >>> fin_pll: xxti { >>> compatible = "fixed-clock"; >>> clock-frequency = <24000000>; >>> clock-output-names = "fin_pll"; >>> #clock-cells = <0>; >>> }; >>> >>> The above lines should be replaced by the following lines: >>> >>> fixed-rate-clocks { >>> oscclk { >>> compatible = "samsung,exynos5410-oscclk"; >>> clock-frequency = <24000000>; >>> }; >>> }; >>> >>> This new form of binding was properly documented in the relevant >>> documentation file. >> >> In general this is backwards. This Exynos-specific clock binding was >> invented before generic fixed rate clock binding showed up and so few >> drivers still use it to maintain DT ABI compatibility. However new >> drivers are required to use the new generic binding and so does the one >> for Exynos5410. >> >> Best regards, >> Tomasz
WARNING: multiple messages have this Message-ID (diff)
From: tomasz.figa@gmail.com (Tomasz Figa) To: linux-arm-kernel@lists.infradead.org Subject: [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks Date: Thu, 31 Jul 2014 15:37:39 +0200 [thread overview] Message-ID: <53DA46A3.9080808@gmail.com> (raw) In-Reply-To: <CAEe5pB_7e_5YkpcuRvsaUg6vKfdu6C+XhwRYyY1uUGptxM5zkg@mail.gmail.com> On 31.07.2014 15:23, Humberto Naves wrote: > Hi Tomasz, > > I perfectly see your point. > However my question was why you did you decide to postpone > Sylwester's? Was there any specific reason? > I suppose it would break all the dtb compatibility, but besides that, > was there any other reason? We discussed this in private (we work in the same office) and I pointed out certain issues with Sylwester's proposed implementation and we agreed that one more revision of the patch is needed, but as it happens, higher priority tasks showed up and this one got lost in action. The first version of the patch [1] changed the original behavior breaking DT ABI compatibility and relied on improper assumption that those clocks are always in "fixed-rate-clocks" node. The thing is that no code should rely on DT node naming. Second version [2] was much better in this aspect, but it had some minor implementation issues - custom clk_ops used instead of generic mux clock and chipid block being constantly mapped and unmapped in every call to __fin_pll_mux_get_parent(). I should have reviewed them both on the mailing lists, but at that time there was no major activity related to Exynos4 outside of our office, so it was more convenient to just talk together directly. [1] http://www.spinics.net/lists/arm-kernel/msg333211.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/258490.html Anyway, I don't think this is all relevant to Exynos5410, which just uses the generic fixed rate binding and has the thing done right from the start. Best regards, Tomasz > > Best, > Humberto > > On Thu, Jul 31, 2014 at 2:53 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> Hi Humberto, >> >> You can find my comments inline. >> >> On 31.07.2014 13:22, Humberto Silva Naves wrote: >>> This implements the fixed rate clocks generated either inside or >>> outside the SoC. It also adds a dt-binding constant for the >>> sclk_hdmiphy clock, which shall be later used by other drivers, >>> such as the DRM. >>> >>> Since the external fixed rate clock fin_pll is now registered by >>> the clk-exynos5410 file, the bindings with the device tree file have >>> changed. It is no longer needed to define fin_pll as a fixed clock, >>> such as in: >>> >>> fin_pll: xxti { >>> compatible = "fixed-clock"; >>> clock-frequency = <24000000>; >>> clock-output-names = "fin_pll"; >>> #clock-cells = <0>; >>> }; >>> >>> The above lines should be replaced by the following lines: >>> >>> fixed-rate-clocks { >>> oscclk { >>> compatible = "samsung,exynos5410-oscclk"; >>> clock-frequency = <24000000>; >>> }; >>> }; >>> >>> This new form of binding was properly documented in the relevant >>> documentation file. >> >> In general this is backwards. This Exynos-specific clock binding was >> invented before generic fixed rate clock binding showed up and so few >> drivers still use it to maintain DT ABI compatibility. However new >> drivers are required to use the new generic binding and so does the one >> for Exynos5410. >> >> Best regards, >> Tomasz
next prev parent reply other threads:[~2014-07-31 13:37 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-07-31 11:22 [PATCHv2 0/5] clk: samsung: exynos5410: Implementation of the PLL clocks Humberto Silva Naves 2014-07-31 11:22 ` Humberto Silva Naves 2014-07-31 11:22 ` [PATCHv2 1/5] clk: samsung: exynos5410: Add NULL pointer checks in clock init Humberto Silva Naves 2014-07-31 11:22 ` Humberto Silva Naves 2014-07-31 11:22 ` Humberto Silva Naves 2014-07-31 12:34 ` Tomasz Figa 2014-07-31 12:34 ` Tomasz Figa 2014-07-31 13:13 ` Humberto Naves 2014-07-31 13:13 ` Humberto Naves 2014-07-31 13:13 ` Humberto Naves 2014-07-31 13:20 ` Tomasz Figa 2014-07-31 13:20 ` Tomasz Figa 2014-07-31 11:22 ` [PATCHv2 2/5] clk: samsung: exynos5410: Organize register offset constants Humberto Silva Naves 2014-07-31 11:22 ` Humberto Silva Naves 2014-07-31 11:22 ` Humberto Silva Naves 2014-07-31 12:49 ` Tomasz Figa 2014-07-31 12:49 ` Tomasz Figa 2014-07-31 11:22 ` [PATCHv2 3/5] clk: samsung: exynos5410: Add suspend/resume handling Humberto Silva Naves 2014-07-31 11:22 ` Humberto Silva Naves 2014-07-31 13:09 ` Tomasz Figa 2014-07-31 13:09 ` Tomasz Figa 2014-07-31 11:22 ` [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks Humberto Silva Naves 2014-07-31 11:22 ` Humberto Silva Naves 2014-07-31 11:45 ` Sylwester Nawrocki 2014-07-31 11:45 ` Sylwester Nawrocki 2014-07-31 11:45 ` Sylwester Nawrocki 2014-07-31 21:01 ` Humberto Naves 2014-07-31 21:01 ` Humberto Naves 2014-07-31 21:08 ` Tomasz Figa 2014-07-31 21:08 ` Tomasz Figa 2014-07-31 12:53 ` Tomasz Figa 2014-07-31 12:53 ` Tomasz Figa 2014-07-31 13:23 ` Humberto Naves 2014-07-31 13:23 ` Humberto Naves 2014-07-31 13:37 ` Tomasz Figa [this message] 2014-07-31 13:37 ` Tomasz Figa 2014-07-31 11:22 ` [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL Humberto Silva Naves 2014-07-31 11:22 ` Humberto Silva Naves 2014-07-31 13:07 ` Tomasz Figa 2014-07-31 13:07 ` Tomasz Figa 2014-07-31 13:37 ` Humberto Naves 2014-07-31 13:37 ` Humberto Naves 2014-07-31 15:19 ` Tomasz Figa 2014-07-31 15:19 ` Tomasz Figa 2014-07-31 21:19 ` Humberto Naves 2014-07-31 21:19 ` Humberto Naves 2014-07-31 22:17 ` Tomasz Figa 2014-07-31 22:17 ` Tomasz Figa 2014-07-31 22:51 ` Mike Turquette 2014-07-31 22:51 ` Mike Turquette
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=53DA46A3.9080808@gmail.com \ --to=tomasz.figa@gmail.com \ --cc=afaerber@suse.de \ --cc=devicetree@vger.kernel.org \ --cc=hsnaves@gmail.com \ --cc=ijc+devicetree@hellion.org.uk \ --cc=kgene.kim@samsung.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=rdunlap@infradead.org \ --cc=t.figa@samsung.com \ --cc=ta.omasab@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.