[PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[Derek Basehore]

This adds the xin32k clock to the RK3399 CPU. Even though it's not
directly used, muxes will end up traversing the entire clk tree on
calls to determine_rate if it doesn't exist.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 99e7f65c1779..3d09472978f8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -191,6 +191,13 @@
 		#clock-cells = <0>;
 	};
 
+	ap_rtc_clk: ap-rtc-clk {
+		compatible = "fixed-clock";
+		clock-frequency = <32768>;
+		clock-output-names = "xin32k";
+		#clock-cells = <0>;
+	};
+
 	amba {
 		compatible = "simple-bus";
 		#address-cells = <2>;
-- 
2.19.1.1215.g8438c0b245-goog

Re: [PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[Doug Anderson]

Hi,

On Thu, Nov 15, 2018 at 9:17 PM Derek Basehore <dbasehore@chromium.org> wrote:
>
> This adds the xin32k clock to the RK3399 CPU. Even though it's not
> directly used, muxes will end up traversing the entire clk tree on
> calls to determine_rate if it doesn't exist.
>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)

nit: I would have expected ${SUBJECT} to have v2 in it somewhere.


> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 99e7f65c1779..3d09472978f8 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi

Aww crud.  I was at the airport yesterday and so I didn't notice that
you were touching rk3399, not rk3399-gru.  This belongs in the gru
device tree file, not in the top level rk3399.  As you have written
this it will break rk3399 boards that have an rk808 on them, AKA:

arch/arm64/boot/dts/rockchip/rk3399-ficus.dts:
arch/arm64/boot/dts/rockchip/rk3399-firefly.dts:
arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi:
arch/arm64/boot/dts/rockchip/rk3399-sapphire.dtsi:

-Doug

Re: [PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[dbasehore .]

On Fri, Nov 16, 2018 at 8:01 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Nov 15, 2018 at 9:17 PM Derek Basehore <dbasehore@chromium.org> wrote:
> >
> > This adds the xin32k clock to the RK3399 CPU. Even though it's not
> > directly used, muxes will end up traversing the entire clk tree on
> > calls to determine_rate if it doesn't exist.
> >
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
> >  1 file changed, 7 insertions(+)
>
> nit: I would have expected ${SUBJECT} to have v2 in it somewhere.
>
>
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > index 99e7f65c1779..3d09472978f8 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>
> Aww crud.  I was at the airport yesterday and so I didn't notice that
> you were touching rk3399, not rk3399-gru.  This belongs in the gru
> device tree file, not in the top level rk3399.  As you have written
> this it will break rk3399 boards that have an rk808 on them, AKA:

Should this be moved to the rk3399.dtsi file? The RK3399 assumes that
this clk exists (same as the 24MHz clk which is in rk3399.dtsi). While
it can function without it defined, it really shouldn't. We can just
assign the existing labels in the dts files you pointed out.

>
> arch/arm64/boot/dts/rockchip/rk3399-ficus.dts:
> arch/arm64/boot/dts/rockchip/rk3399-firefly.dts:
> arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi:
> arch/arm64/boot/dts/rockchip/rk3399-sapphire.dtsi:
>
> -Doug

Re: [PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[Heiko Stübner]

Hi Derek,

Am Freitag, 16. November 2018, 18:39:09 CET schrieb dbasehore .:
> On Fri, Nov 16, 2018 at 8:01 AM Doug Anderson <dianders@chromium.org> wrote:
> > Hi,
> > 
> > On Thu, Nov 15, 2018 at 9:17 PM Derek Basehore <dbasehore@chromium.org> wrote:
> > > This adds the xin32k clock to the RK3399 CPU. Even though it's not
> > > directly used, muxes will end up traversing the entire clk tree on
> > > calls to determine_rate if it doesn't exist.
> > > 
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > > 
> > >  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > 
> > nit: I would have expected ${SUBJECT} to have v2 in it somewhere.
> > 
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index
> > > 99e7f65c1779..3d09472978f8 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > 
> > Aww crud.  I was at the airport yesterday and so I didn't notice that
> > you were touching rk3399, not rk3399-gru.  This belongs in the gru
> > device tree file, not in the top level rk3399.  As you have written
> 
> > this it will break rk3399 boards that have an rk808 on them, AKA:
> Should this be moved to the rk3399.dtsi file? The RK3399 assumes that
> this clk exists (same as the 24MHz clk which is in rk3399.dtsi). While
> it can function without it defined, it really shouldn't. We can just
> assign the existing labels in the dts files you pointed out.

Right now this patch puts your clock into the rk3399.dtsi, which is the
wrong place. On most Rockchip systems, the xin32k clock is created
by the pmic (rk808 in a lot of cases) and the clock-tree gets amended
once the pmic has probed. See for example [0].

While I don't know where your xin32k really comes from (cros-ec maybe)
it is definitly a board-specific source for the clock and should thus
live in the rk3399-gru.dtsi.

And we really expect each board to actually make sure its xin32k is
properly defined as for example on the rk808 and act8846 pmics it could
also very well be turned off at boot and also does support multiple rates,
thus needing proper clock handling.


Heiko

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-rock960.dtsi#n159

Re: [PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[Doug Anderson]

Hi,

On Fri, Nov 16, 2018 at 9:39 AM dbasehore . <dbasehore@chromium.org> wrote:
>
> On Fri, Nov 16, 2018 at 8:01 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Thu, Nov 15, 2018 at 9:17 PM Derek Basehore <dbasehore@chromium.org> wrote:
> > >
> > > This adds the xin32k clock to the RK3399 CPU. Even though it's not
> > > directly used, muxes will end up traversing the entire clk tree on
> > > calls to determine_rate if it doesn't exist.
> > >
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> >
> > nit: I would have expected ${SUBJECT} to have v2 in it somewhere.
> >
> >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > index 99e7f65c1779..3d09472978f8 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> >
> > Aww crud.  I was at the airport yesterday and so I didn't notice that
> > you were touching rk3399, not rk3399-gru.  This belongs in the gru
> > device tree file, not in the top level rk3399.  As you have written
> > this it will break rk3399 boards that have an rk808 on them, AKA:
>
> Should this be moved to the rk3399.dtsi file? The RK3399 assumes that
> this clk exists (same as the 24MHz clk which is in rk3399.dtsi). While
> it can function without it defined, it really shouldn't. We can just
> assign the existing labels in the dts files you pointed out.

No, it should be in the board files.  Each board may produce the 32k
clock through a different component.  On gru-based devices we produce
the 32k clock through a silego part.  On some other ones we produce it
from rk808.

Technically you could say that we don't _truly_ need to model this
clock and we could have just inserted a dummy/fixed 32k clock in the
clk-rk3399.c file.  ...but we did model it so that means we should
probably model it semi-properly.

If a given board forgets to provide a 32k clock then that's a bug for
them like it was for us.

-Doug

Re: [PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[Heiko Stübner]

Am Freitag, 16. November 2018, 19:23:59 CET schrieb Doug Anderson:
> Hi,
> 
> On Fri, Nov 16, 2018 at 9:39 AM dbasehore . <dbasehore@chromium.org> wrote:
> > On Fri, Nov 16, 2018 at 8:01 AM Doug Anderson <dianders@chromium.org> 
wrote:
> > > Hi,
> > > 
> > > On Thu, Nov 15, 2018 at 9:17 PM Derek Basehore <dbasehore@chromium.org> 
wrote:
> > > > This adds the xin32k clock to the RK3399 CPU. Even though it's not
> > > > directly used, muxes will end up traversing the entire clk tree on
> > > > calls to determine_rate if it doesn't exist.
> > > > 
> > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > > ---
> > > > 
> > > >  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > 
> > > nit: I would have expected ${SUBJECT} to have v2 in it somewhere.
> > > 
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > > b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index
> > > > 99e7f65c1779..3d09472978f8 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > 
> > > Aww crud.  I was at the airport yesterday and so I didn't notice that
> > > you were touching rk3399, not rk3399-gru.  This belongs in the gru
> > > device tree file, not in the top level rk3399.  As you have written
> > 
> > > this it will break rk3399 boards that have an rk808 on them, AKA:
> > Should this be moved to the rk3399.dtsi file? The RK3399 assumes that
> > this clk exists (same as the 24MHz clk which is in rk3399.dtsi). While
> > it can function without it defined, it really shouldn't. We can just
> > assign the existing labels in the dts files you pointed out.
> 
> No, it should be in the board files.  Each board may produce the 32k
> clock through a different component.  On gru-based devices we produce
> the 32k clock through a silego part.

That would also be a great part of the commit message, like
"...on Gru boards the 32kHz clock gets produced by a Silego oscillator"
or so when you move it over to rk3399-gru.dtsi .


> Technically you could say that we don't _truly_ need to model this
> clock and we could have just inserted a dummy/fixed 32k clock in the
> clk-rk3399.c file.  ...but we did model it so that means we should
> probably model it semi-properly.
>
> If a given board forgets to provide a 32k clock then that's a bug for
> them like it was for us.

Yep and as I said in my other mail, on these pmic generated clocks
the clock generation often even is configurable (rate, on/off), so it
should really be a real clock not some hack ;-) .


Heiko

Re: [PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[dbasehore .]

On Mon, Nov 19, 2018 at 1:41 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Freitag, 16. November 2018, 19:23:59 CET schrieb Doug Anderson:
> > Hi,
> >
> > On Fri, Nov 16, 2018 at 9:39 AM dbasehore . <dbasehore@chromium.org> wrote:
> > > On Fri, Nov 16, 2018 at 8:01 AM Doug Anderson <dianders@chromium.org>
> wrote:
> > > > Hi,
> > > >
> > > > On Thu, Nov 15, 2018 at 9:17 PM Derek Basehore <dbasehore@chromium.org>
> wrote:
> > > > > This adds the xin32k clock to the RK3399 CPU. Even though it's not
> > > > > directly used, muxes will end up traversing the entire clk tree on
> > > > > calls to determine_rate if it doesn't exist.
> > > > >
> > > > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > > > ---
> > > > >
> > > > >  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > >
> > > > nit: I would have expected ${SUBJECT} to have v2 in it somewhere.
> > > >
> > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > > > b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index
> > > > > 99e7f65c1779..3d09472978f8 100644
> > > > > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > > > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > > >
> > > > Aww crud.  I was at the airport yesterday and so I didn't notice that
> > > > you were touching rk3399, not rk3399-gru.  This belongs in the gru
> > > > device tree file, not in the top level rk3399.  As you have written
> > >
> > > > this it will break rk3399 boards that have an rk808 on them, AKA:
> > > Should this be moved to the rk3399.dtsi file? The RK3399 assumes that
> > > this clk exists (same as the 24MHz clk which is in rk3399.dtsi). While
> > > it can function without it defined, it really shouldn't. We can just
> > > assign the existing labels in the dts files you pointed out.
> >
> > No, it should be in the board files.  Each board may produce the 32k
> > clock through a different component.  On gru-based devices we produce
> > the 32k clock through a silego part.
>
> That would also be a great part of the commit message, like
> "...on Gru boards the 32kHz clock gets produced by a Silego oscillator"
> or so when you move it over to rk3399-gru.dtsi .
>
>
> > Technically you could say that we don't _truly_ need to model this
> > clock and we could have just inserted a dummy/fixed 32k clock in the
> > clk-rk3399.c file.  ...but we did model it so that means we should
> > probably model it semi-properly.
> >
> > If a given board forgets to provide a 32k clock then that's a bug for
> > them like it was for us.
>
> Yep and as I said in my other mail, on these pmic generated clocks
> the clock generation often even is configurable (rate, on/off), so it
> should really be a real clock not some hack ;-) .
>

Ok, I'll make that change in about a week after I get back from
vacation in about a week.

>
> Heiko
>
>

Re: [PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[dbasehore .]

On Thu, Nov 15, 2018 at 9:03 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Nov 15, 2018 at 4:42 PM Derek Basehore <dbasehore@chromium.org> wrote:
> >
> > This adds the xin32k clock to the RK3399 CPU. Even though it's not
> > directly used, muxes will end up traversing the entire clk tree on
> > calls to determine_rate if it doesn't exist.
> >
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > index 99e7f65c1779..6a32293982d0 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > @@ -191,6 +191,13 @@
> >                 #clock-cells = <0>;
> >         };
> >
> > +       xin32k: xin32k {
>
> nit: xin32k is the name of the clock that rk3399 consumes.  It seems a
> little weird to name this node with that name.  Can you call this:
>
> ap_rtc_clk: ap-rtc-clk
>
> ...after the gru schematic?  You wouldn't change the
> clock-output-names, just the node name / label.
>
>
> > +               compatible = "fixed-clock";
> > +               clock-frequency = <32000>;
>
> I checked the datasheet for the 32K clock and it shows that this is a
> 32768 Hz clock, not a 32000 Hz one.  I also checked the rk808 clock
> driver (which is supposed to be compatible with rk3399) and it
> produces a 32768 clock.

Ok, sending out an updated patch that addresses these concerns.

Re: [PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[Doug Anderson]

Hi,

On Thu, Nov 15, 2018 at 4:42 PM Derek Basehore <dbasehore@chromium.org> wrote:
>
> This adds the xin32k clock to the RK3399 CPU. Even though it's not
> directly used, muxes will end up traversing the entire clk tree on
> calls to determine_rate if it doesn't exist.
>
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 99e7f65c1779..6a32293982d0 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -191,6 +191,13 @@
>                 #clock-cells = <0>;
>         };
>
> +       xin32k: xin32k {

nit: xin32k is the name of the clock that rk3399 consumes.  It seems a
little weird to name this node with that name.  Can you call this:

ap_rtc_clk: ap-rtc-clk

...after the gru schematic?  You wouldn't change the
clock-output-names, just the node name / label.


> +               compatible = "fixed-clock";
> +               clock-frequency = <32000>;

I checked the datasheet for the 32K clock and it shows that this is a
32768 Hz clock, not a 32000 Hz one.  I also checked the rk808 clock
driver (which is supposed to be compatible with rk3399) and it
produces a 32768 clock.

[PATCH] arm64: dts: rockchip: rk3399: Add xin32k clk

[Derek Basehore]

This adds the xin32k clock to the RK3399 CPU. Even though it's not
directly used, muxes will end up traversing the entire clk tree on
calls to determine_rate if it doesn't exist.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 99e7f65c1779..6a32293982d0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -191,6 +191,13 @@
 		#clock-cells = <0>;
 	};
 
+	xin32k: xin32k {
+		compatible = "fixed-clock";
+		clock-frequency = <32000>;
+		clock-output-names = "xin32k";
+		#clock-cells = <0>;
+	};
+
 	amba {
 		compatible = "simple-bus";
 		#address-cells = <2>;
-- 
2.19.1.1215.g8438c0b245-goog