diff mbox series

arm64: dts: rockchip: rk3399: Add xin32k clk

Message ID 20181116051719.23376-1-dbasehore@chromium.org (mailing list archive)
State New, archived
Headers show
Series arm64: dts: rockchip: rk3399: Add xin32k clk | expand

Commit Message

Derek Basehore Nov. 16, 2018, 5:17 a.m. UTC
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(+)

Comments

Doug Anderson Nov. 16, 2018, 4 p.m. UTC | #1
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
Derek Basehore Nov. 16, 2018, 5:39 p.m. UTC | #2
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
Heiko Stübner Nov. 16, 2018, 6:23 p.m. UTC | #3
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
Doug Anderson Nov. 16, 2018, 6:23 p.m. UTC | #4
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
Heiko Stübner Nov. 19, 2018, 9:41 a.m. UTC | #5
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
Derek Basehore Nov. 20, 2018, 2:05 a.m. UTC | #6
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
>
>
diff mbox series

Patch

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>;