| 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 |
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
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
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
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
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
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 --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>;
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(+)