Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1167591 > unrolled thread

[PATCH] arm64: dts: mt8173: add clock_null

Back to article view | Back to linux.kernel

Contents

  [PATCH] arm64: dts: mt8173: add clock_null Eddie Huang <eddie.huang@mediatek.com> - 2015-06-18 07:30 +0200
    Re: [PATCH] arm64: dts: mt8173: add clock_null Heiko Stübner <heiko@sntech.de> - 2015-06-22 15:00 +0200
      Re: [PATCH] arm64: dts: mt8173: add clock_null James Liao <jamesjj.liao@mediatek.com> - 2015-06-24 10:00 +0200
        Re: [PATCH] arm64: dts: mt8173: add clock_null James Liao <jamesjj.liao@mediatek.com> - 2015-07-02 04:10 +0200

#1167591 — [PATCH] arm64: dts: mt8173: add clock_null

Add clk_null, which represents clocks that can not / need not
controlled by software.
There are many clocks' parent set to clk_null.

Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
Base on 4.1-rc1

Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 924fdb6..4798f44 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -81,6 +81,12 @@
 		cpu_on	      = <0x84000003>;
 	};
 
+	clk_null: clk_null {
+		compatible = "fixed-clock";
+		clock-frequency = <0>;
+		#clock-cells = <0>;
+	};
+
 	uart_clk: dummy26m {
 		compatible = "fixed-clock";
 		clock-frequency = <26000000>;
-- 
1.8.1.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1169924

Hi James,

Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
> On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
> > Am Donnerstag, 18. Juni 2015, 18:15:03 schrieb Heiko Stuebner:
> > > Am Donnerstag, 18. Juni 2015, 13:29:11 schrieb Eddie Huang:
> > > > Add clk_null, which represents clocks that can not / need not
> > > > controlled by software.
> > > > There are many clocks' parent set to clk_null.
> > > 
> > > Devicetree is supposed to describe hardware, and ideally not what
> > > software
> > > does with it. If the clock simply cannot be controlled by software, it
> > > will
> > > still have a rate and I think it should probably be modelled - similarly
> > > we
> > > sometimes have fixed regulators that also are not software controllable.
> > > 
> > > 
> > > While it might be ok to define dummy clocks as a temporary stopgap,
> > > these
> > > should definitly be marked as such. This clk_null at least sounds like
> > > there is no plan to replace this with a real solution at some point.
> > > 
> > > And of course a bit of context would be cool, to know which type of
> > > clocks
> > > this actually replaces.
> > 
> > After looking a bit more into this, I'm feel that the clk_null approach is
> > wrong.
> > 
> > For one, even if the clk_null stuff would be ok, the binding doc of the
> > clock controller does not describe the need of this specific clock at
> > all.
> > 
> > 
> > The other more important point, looking at clk-mt8173 I see at least these
> > clocks being set as children of "clk_null":
> > 
> > static const struct mtk_fixed_factor root_clk_alias[] __initconst = {
> > 
> > 	FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
> > 	FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
> > 	FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
> > 	FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),
> > 
> > };
> > 
> > 
> > These look more like they are fed from some external source to the clock
> > controller? I did ask Matthias but he also couldn't find anything
> > describing where these clocks actually come from.
> 
> Some clocks such as clkph_mck_o, we don't really care where they come
> from and what frequencies are. We model these clocks just because they
> or their derived clocks can be the source of topckgen muxes. Is there a
> better way to model "don't care" clocks?

There are two different concepts at work here. You might not care in your 
kernel driver implementation _at the moment_ where the clocks come from; but 
the devicetree is supposed to model how the hardware is structured and not 
contain implementation specific details.

So the clock tree should be modeled according to how the hardware is layed out 
not how you want to use the clocks at the moment :-) .

It would it any case be good, if you could describe where these clocks come 
from in the hardware, so we can find the best solution on how to model those.


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1171156

Hi Heiko,

On Mon, 2015-06-22 at 14:53 +0200, Heiko Stübner wrote:
> Hi James,
> 
> Am Montag, 22. Juni 2015, 11:38:37 schrieb James Liao:
> > On Fri, 2015-06-19 at 13:36 +0200, Heiko Stuebner wrote:
> > 
> > Some clocks such as clkph_mck_o, we don't really care where they come
> > from and what frequencies are. We model these clocks just because they
> > or their derived clocks can be the source of topckgen muxes. Is there a
> > better way to model "don't care" clocks?
> 
> There are two different concepts at work here. You might not care in your 
> kernel driver implementation _at the moment_ where the clocks come from; but 
> the devicetree is supposed to model how the hardware is structured and not 
> contain implementation specific details.

If we model "don't care" clocks inside the driver (i.e. create clk_null
in clock driver), then we don't need to model the dummy clock in device.
Is it an acceptable way?

> So the clock tree should be modeled according to how the hardware is layed out 
> not how you want to use the clocks at the moment :-) .
> 
> It would it any case be good, if you could describe where these clocks come 
> from in the hardware, so we can find the best solution on how to model those.

In fact, I don't know where these clocks come from at all, especially
when they come from outside of SoC. Besides, some clocks don't need to
model in CCF, but they can be the source of clocks that controlled by
CCF.

I don't think ALL clocks on a SoC need to be handled in CCF, so there
should be some clocks don't have a "real" or "correct" parent. In
current implementation I use a dummy clock (clk_null) to be the unreal
parent.

Do you think we should model as more clocks as we can in CCF even we
don't need them? If no, how do we handle those clocks which are not
handled in CCF but can be parent of CCF clocks?


Best regards,

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1175661

Hi Sascha,

On Wed, 2015-07-01 at 08:49 +0200, Sascha Hauer wrote:
> On Tue, Jun 30, 2015 at 05:07:09PM +0800, James Liao wrote:
> > There are 4 clocks which are derived from clk_null directly in current
> > topckgen implementation:
> > 
> > 	clkph_mck_o, dpi_ck, usb_syspll_125m, hdmitx_dig_cts
> > 
> > Our designer mentioned 2 things about external clocks:
> > 
> > 1. These 4 clocks come from analog macro, not from PLL, nor from
> > external clocks directly.
> 
> Ok, this means there actually are clocks. We can't control these clock and
> they have some known or unknown rate. This makes them fixed clocks. Just
> specify them in the device tree and you are done. Give them reasonable
> names and the rate if you know it, 0 otherwise.
> 
> The problem is not that you use fixed clocks for non software
> controllable clocks of unknwown rates, but that you try to use a single
> clock for all of them and name it 'dummy' or 'null'. Name it
> 
> dpi_ck {
> 	compatible = "fixed-clock";
> 	rate = <0>; /* unknown, generated by some Analog block */
> };
> 
> Technically it's the same, but it sounds much more professional and like
> you know what you are doing ;)

These clocks are SoC internal clocks. Is it suitable to specify them in
the device tree?

According to your and Heiko's suggestion, it looks the best way should
be specifying these clocks in the driver code without a dummy parent.
i.e.:

Original code:

FACTOR(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk_null", 1, 1),
FACTOR(CLK_TOP_DPI, "dpi_ck", "clk_null", 1, 1),
FACTOR(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk_null", 1, 1),
FACTOR(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", "clk_null", 1, 1),

New code:

FIXED(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", UNKNOWN_RATE),
FIXED(CLK_TOP_DPI, "dpi_ck", UNKNOWN_RATE),
FIXED(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", UNKNOWN_RATE),
FIXED(CLK_TOP_HDMITX_DIG_CTS, "hdmitx_dig_cts", UNKNOWN_RATE),

Then we don't need to specify a dummy clock such as clk_null in device
tree.


Best regards,

James

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web