[OpenWrt-Devel,2/4] ipq40xx: fix sleep clock
diff mbox series

Message ID 20190514134220.3626-2-be.dissent@gmail.com
State Changes Requested
Delegated to: Petr Štetiar
Headers show
Series
  • [OpenWrt-Devel,1/4] ipq40xx: directly define voltage per opp
Related show

Commit Message

Павел May 14, 2019, 1:42 p.m. UTC
It seems like sleep_clk was copied from ipq806x.
Fix ipq40xx sleep_clk to the value QSDK defines.

Hope someone with datasheet could clarify the correct
value.

Also rename the sleep clock node like the GCC driver 
awaits it to be..

Signed-off-by: Pavel Kubelun <be.dissent@gmail.com>
---
 .../patches-4.14/089-ipq40xx-fix-sleep-clock.patch | 29 ++++++++++++++++++++++
 .../patches-4.19/085-ipq40xx-fix-sleep-clock.patch | 29 ++++++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100644 target/linux/ipq40xx/patches-4.14/089-ipq40xx-fix-sleep-clock.patch
 create mode 100644 target/linux/ipq40xx/patches-4.19/085-ipq40xx-fix-sleep-clock.patch

Comments

Petr Štetiar May 15, 2019, 3:55 p.m. UTC | #1
Pavel Kubelun <be.dissent@gmail.com> [2019-05-14 16:42:18]:

Hi,

> It seems like sleep_clk was copied from ipq806x.
> Fix ipq40xx sleep_clk to the value QSDK defines.
> 
> Hope someone with datasheet could clarify the correct
> value.

what problem does this exactly fixes? Is there any particular reason why this
shouldn't be sent upstream and then backported to OpenWrt?

-- ynezz
Павел May 15, 2019, 5:16 p.m. UTC | #2
ср, 15 мая 2019 г., 18:55 Petr Štetiar <ynezz@true.cz>:

> Pavel Kubelun <be.dissent@gmail.com> [2019-05-14 16:42:18]:
>
> Hi,
>
> > It seems like sleep_clk was copied from ipq806x.
> > Fix ipq40xx sleep_clk to the value QSDK defines.
> >
> > Hope someone with datasheet could clarify the correct
> > value.
>
> what problem does this exactly fixes? Is there any particular reason why
> this
> shouldn't be sent upstream and then backported to OpenWrt?
>

There are no reasons why it shouldn't be sent upstream along with other
patches. I hope to find someone with datasheet beforehand to verify the
correct sleep clock rate.

Besides upstreaming a patch takes time while the next openwrt release
should be out soon I suppose.

This patch also contains a fix (node name) that allows the GCC driver to
correctly initialize the clock and its children (USB sleep clock in fact)


-- ynezz
>
Sven Eckelmann May 16, 2019, 10:05 a.m. UTC | #3
On Wednesday, 15 May 2019 19:16:51 CEST Павел wrote:
[...]
> > Is there any particular reason why
> > this
> > shouldn't be sent upstream and then backported to OpenWrt?
> >
> 
> There are no reasons why it shouldn't be sent upstream along with other
> patches. I hope to find someone with datasheet beforehand to verify the
> correct sleep clock rate.

But you will most likely find the persons with the datasheet when you try to 
upstream it via 

* Andy Gross <agross@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT)
* David Brown <david.brown@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT)
* linux-arm-msm@vger.kernel.org (open list:ARM/QUALCOMM SUPPORT)

And maybe some of these guys also know how to find the ipq40xx clock 
controller reference or hardware reference. Because I was only able to verify 
for IPQ8072 that it had a 32.768 KHz sleep clock. But the 
"IPQ4018/IPQ4028/IPQ4019/IPQ4029 Watchdog" document states that the watchdog 
runs on a 32 KHz sleep clock. And according to the device tree, the clock you 
modified here is connected to the watchdog.

And for the device tree bindings:

* devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
* Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
* Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)

> Besides upstreaming a patch takes time while the next openwrt release
> should be out soon I suppose.

Good reason to try to upstream it at the same time to OpenWrt and upstream :)
At least then we could get some feedback from upstream before OpenWrt ships 
something which potentially has negative effects.

Kind regards,
	Sven
Павел May 16, 2019, 11:18 a.m. UTC | #4
чт, 16 мая 2019 г., 13:05 Sven Eckelmann <sven@narfation.org>:

> On Wednesday, 15 May 2019 19:16:51 CEST Павел wrote:
> [...]
> > > Is there any particular reason why
> > > this
> > > shouldn't be sent upstream and then backported to OpenWrt?
> > >
> >
> > There are no reasons why it shouldn't be sent upstream along with other
> > patches. I hope to find someone with datasheet beforehand to verify the
> > correct sleep clock rate.
>
> But you will most likely find the persons with the datasheet when you try
> to
> upstream it via
>
> * Andy Gross <agross@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT)
> * David Brown <david.brown@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT)
> * linux-arm-msm@vger.kernel.org (open list:ARM/QUALCOMM SUPPORT)
>
> And maybe some of these guys also know how to find the ipq40xx clock
> controller reference or hardware reference. Because I was only able to
> verify
> for IPQ8072 that it had a 32.768 KHz sleep clock. But the
>

If you are completely sure about that, then I guess that they have
(un)intentionally messed with the clock in QSDK, because they state that
ipq807x has the same 32000 khz crystal.
https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/tree/arch/arm64/boot/dts/qcom/qcom-ipq807x-soc.dtsi?h=eggplant#n2055

Furthermore, it has been upstreamed...

So I'm confused actually what path to choose now. Probably it depends on
your level of confidence that ipq8072 definitely has a 32.768 khz rate - it
will mean that qsdk is not trustworthy on this matter.


"IPQ4018/IPQ4028/IPQ4019/IPQ4029 Watchdog" document states that the
> watchdog
> runs on a 32 KHz sleep clock. And according to the device tree, the clock
> you
> modified here is connected to the watchdog.
>
> And for the device tree bindings:
>
> * devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS)
> * Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND
> FLATTENED DEVICE TREE BINDINGS)
> * Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND
> FLATTENED DEVICE TREE BINDINGS)
>
> > Besides upstreaming a patch takes time while the next openwrt release
> > should be out soon I suppose.
>
> Good reason to try to upstream it at the same time to OpenWrt and upstream
> :)
> At least then we could get some feedback from upstream before OpenWrt
> ships
> something which potentially has negative effects.
>
> Kind regards,
>         Sven
Sven Eckelmann May 16, 2019, 12:22 p.m. UTC | #5
On Tuesday, 14 May 2019 15:42:18 CEST Pavel Kubelun wrote:
> +--- a/arch/arm/boot/dts/qcom-ipq4019.dtsi
> ++++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi
> +@@ -141,9 +141,9 @@
> +       };
> + 
> +       clocks {
> +-              sleep_clk: sleep_clk {
> ++              sleep_clk: gcc_sleep_clk_src {
> +                       compatible = "fixed-clock";
> +-                      clock-frequency = <32768>;
> ++                      clock-frequency = <32000>;
> +                       #clock-cells = <0>;
> +               };

On Thursday, 16 May 2019 13:18:14 CEST Павел wrote:
[...]
> > And maybe some of these guys also know how to find the ipq40xx clock
> > controller reference or hardware reference. Because I was only able to
> > verify
> > for IPQ8072 that it had a 32.768 KHz sleep clock. But the
> >
> 
> If you are completely sure about that, then I guess that they have
> (un)intentionally messed with the clock in QSDK, because they state that
> ipq807x has the same 32000 khz crystal.
> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/tree/arch/arm64/boot/dts/qcom/qcom-ipq807x-soc.dtsi?h=eggplant#n2055

Confidence is the wrong word. I can only state that this is written in 
80-YA727-13 Rev. D (IPQ8072.AP.HK07). Same for other devices like 
IPQ8078 AP.HK02, IPQ8074 AP.HK01, ...

But I found in the same document that they call it the "32 KHz sleep clock in" 
in one section and and in another table "32.768 KHz sleep clock input to the 
IPQ8072" (next to the name "...32K..."). So it is now to the reader to find 
out what they meant here in which reference document. So maybe they also meant 
32.768 KHz when in the IPQ4019 Watchdog document when they wrote 32 Khz sleep 
clock... who knows.

My gut feeling (sorry, not an HW guy) tell me that they are just using a 
32.768 KHz clock (from a standard 32.768 KHz oscillator) in all these products 
and just shortened it to 32K at some point in the document. And now Gopinath 
Sekar wrote 32000 instead of 32768. But I absolutely don't know what actually 
is there in HW.

Kind regards,
	Sven

[1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/commit/?id=d92ec59973484acc86dd24b67f10f8911b4b4b7d
Павел May 17, 2019, 8:16 a.m. UTC | #6
чт, 16 мая 2019 г., 15:22 Sven Eckelmann <sven@narfation.org>:

> On Tuesday, 14 May 2019 15:42:18 CEST Pavel Kubelun wrote:
> > +--- a/arch/arm/boot/dts/qcom-ipq4019.dtsi
> > ++++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi
> > +@@ -141,9 +141,9 @@
> > +       };
> > +
> > +       clocks {
> > +-              sleep_clk: sleep_clk {
> > ++              sleep_clk: gcc_sleep_clk_src {
> > +                       compatible = "fixed-clock";
> > +-                      clock-frequency = <32768>;
> > ++                      clock-frequency = <32000>;
> > +                       #clock-cells = <0>;
> > +               };
>
> On Thursday, 16 May 2019 13:18:14 CEST Павел wrote:
> [...]
> > > And maybe some of these guys also know how to find the ipq40xx clock
> > > controller reference or hardware reference. Because I was only able to
> > > verify
> > > for IPQ8072 that it had a 32.768 KHz sleep clock. But the
> > >
> >
> > If you are completely sure about that, then I guess that they have
> > (un)intentionally messed with the clock in QSDK, because they state that
> > ipq807x has the same 32000 khz crystal.
> >
> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/tree/arch/arm64/boot/dts/qcom/qcom-ipq807x-soc.dtsi?h=eggplant#n2055
>
> Confidence is the wrong word. I can only state that this is written in
> 80-YA727-13 Rev. D (IPQ8072.AP.HK07). Same for other devices like
> IPQ8078 AP.HK02, IPQ8074 AP.HK01, ...
>
> But I found in the same document that they call it the "32 KHz sleep clock
> in"
> in one section and and in another table "32.768 KHz sleep clock input to
> the
> IPQ8072" (next to the name "...32K..."). So it is now to the reader to
> find
> out what they meant here in which reference document. So maybe they also
> meant
> 32.768 KHz when in the IPQ4019 Watchdog document when they wrote 32 Khz
> sleep
> clock... who knows.
>

Okay, here what I've found in oem firmware (Zyxel nbg6617)

There are 2 clocks related to timer in oem firmware:
1. gcc_sleep_clk_src - 32.000
 ^---USB3 sleep clock
 ^---USB2 sleep clock
2. wifi_rtc_clk_src - 32.768
 ^---wcnssN sleep clock
 ^---wcnssN sleep clock

In upstream we don't have wifi_rtc clk and all mentioned childs are
connected to sleep clk.

So sleep clock at 32000 rate looks like some kind of a workaround and not
to break wifi they introduced a separate clock for that, the one that
really represents the hardware.

I'll drop the sleep clock rate change in the next version of patchset since
we don't need that kind of workaround for now.


> My gut feeling (sorry, not an HW guy) tell me that they are just using a
> 32.768 KHz clock (from a standard 32.768 KHz oscillator) in all these
> products
> and just shortened it to 32K at some point in the document. And now
> Gopinath
> Sekar wrote 32000 instead of 32768. But I absolutely don't know what
> actually
> is there in HW.
>
> Kind regards,
>         Sven
>
> [1]
> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/commit/?id=d92ec59973484acc86dd24b67f10f8911b4b4b7d
Павел July 8, 2019, 10:59 a.m. UTC | #7
Hey,
I believe this patch can be merged as is. According to Sricharan R. [1]:
"It is [sleep clk] derived from a 48M wifi refclk

48M wifi ref clk -> [/2 divider] -> [/750 divider] -> sleep_clk (32000)"


[1] https://patchwork.kernel.org/comment/22721613/

пт, 17 мая 2019 г., 11:16 Павел <be.dissent@gmail.com>:

>
>
> чт, 16 мая 2019 г., 15:22 Sven Eckelmann <sven@narfation.org>:
>
>> On Tuesday, 14 May 2019 15:42:18 CEST Pavel Kubelun wrote:
>> > +--- a/arch/arm/boot/dts/qcom-ipq4019.dtsi
>> > ++++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi
>> > +@@ -141,9 +141,9 @@
>> > +       };
>> > +
>> > +       clocks {
>> > +-              sleep_clk: sleep_clk {
>> > ++              sleep_clk: gcc_sleep_clk_src {
>> > +                       compatible = "fixed-clock";
>> > +-                      clock-frequency = <32768>;
>> > ++                      clock-frequency = <32000>;
>> > +                       #clock-cells = <0>;
>> > +               };
>>
>> On Thursday, 16 May 2019 13:18:14 CEST Павел wrote:
>> [...]
>> > > And maybe some of these guys also know how to find the ipq40xx clock
>> > > controller reference or hardware reference. Because I was only able to
>> > > verify
>> > > for IPQ8072 that it had a 32.768 KHz sleep clock. But the
>> > >
>> >
>> > If you are completely sure about that, then I guess that they have
>> > (un)intentionally messed with the clock in QSDK, because they state that
>> > ipq807x has the same 32000 khz crystal.
>> >
>> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/tree/arch/arm64/boot/dts/qcom/qcom-ipq807x-soc.dtsi?h=eggplant#n2055
>>
>> Confidence is the wrong word. I can only state that this is written in
>> 80-YA727-13 Rev. D (IPQ8072.AP.HK07). Same for other devices like
>> IPQ8078 AP.HK02, IPQ8074 AP.HK01, ...
>>
>> But I found in the same document that they call it the "32 KHz sleep
>> clock in"
>> in one section and and in another table "32.768 KHz sleep clock input to
>> the
>> IPQ8072" (next to the name "...32K..."). So it is now to the reader to
>> find
>> out what they meant here in which reference document. So maybe they also
>> meant
>> 32.768 KHz when in the IPQ4019 Watchdog document when they wrote 32 Khz
>> sleep
>> clock... who knows.
>>
>
> Okay, here what I've found in oem firmware (Zyxel nbg6617)
>
> There are 2 clocks related to timer in oem firmware:
> 1. gcc_sleep_clk_src - 32.000
>  ^---USB3 sleep clock
>  ^---USB2 sleep clock
> 2. wifi_rtc_clk_src - 32.768
>  ^---wcnssN sleep clock
>  ^---wcnssN sleep clock
>
> In upstream we don't have wifi_rtc clk and all mentioned childs are
> connected to sleep clk.
>
> So sleep clock at 32000 rate looks like some kind of a workaround and not
> to break wifi they introduced a separate clock for that, the one that
> really represents the hardware.
>
> I'll drop the sleep clock rate change in the next version of patchset
> since we don't need that kind of workaround for now.
>
>
>> My gut feeling (sorry, not an HW guy) tell me that they are just using a
>> 32.768 KHz clock (from a standard 32.768 KHz oscillator) in all these
>> products
>> and just shortened it to 32K at some point in the document. And now
>> Gopinath
>> Sekar wrote 32000 instead of 32768. But I absolutely don't know what
>> actually
>> is there in HW.
>>
>> Kind regards,
>>         Sven
>>
>> [1]
>> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/commit/?id=d92ec59973484acc86dd24b67f10f8911b4b4b7d
>
>

Patch
diff mbox series

diff --git a/target/linux/ipq40xx/patches-4.14/089-ipq40xx-fix-sleep-clock.patch b/target/linux/ipq40xx/patches-4.14/089-ipq40xx-fix-sleep-clock.patch
new file mode 100644
index 0000000000..9e40f7c17f
--- /dev/null
+++ b/target/linux/ipq40xx/patches-4.14/089-ipq40xx-fix-sleep-clock.patch
@@ -0,0 +1,29 @@ 
+From 4d44bb1031a68d7d5b604d3b340c059f41ca62af Mon Sep 17 00:00:00 2001
+From: Pavel Kubelun <be.dissent@gmail.com>
+Date: Mon, 6 May 2019 20:55:16 +0300
+Subject: [PATCH] ipq40xx: fix sleep clock
+
+It seems like sleep_clk was copied from ipq806x.
+Fix ipq40xx sleep_clk to the value QSDK defines.
+
+Also rename the sleep clock node like the GCC driver awaits.
+
+Signed-off-by: Pavel Kubelun <be.dissent@gmail.com>
+---
+ arch/arm/boot/dts/qcom-ipq4019.dtsi | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+--- a/arch/arm/boot/dts/qcom-ipq4019.dtsi
++++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi
+@@ -141,9 +141,9 @@
+ 	};
+ 
+ 	clocks {
+-		sleep_clk: sleep_clk {
++		sleep_clk: gcc_sleep_clk_src {
+ 			compatible = "fixed-clock";
+-			clock-frequency = <32768>;
++			clock-frequency = <32000>;
+ 			#clock-cells = <0>;
+ 		};
+ 
diff --git a/target/linux/ipq40xx/patches-4.19/085-ipq40xx-fix-sleep-clock.patch b/target/linux/ipq40xx/patches-4.19/085-ipq40xx-fix-sleep-clock.patch
new file mode 100644
index 0000000000..e7d8bb71d9
--- /dev/null
+++ b/target/linux/ipq40xx/patches-4.19/085-ipq40xx-fix-sleep-clock.patch
@@ -0,0 +1,29 @@ 
+From 4d44bb1031a68d7d5b604d3b340c059f41ca62af Mon Sep 17 00:00:00 2001
+From: Pavel Kubelun <be.dissent@gmail.com>
+Date: Mon, 6 May 2019 20:55:16 +0300
+Subject: [PATCH] ipq40xx: fix sleep clock
+
+It seems like sleep_clk was copied from ipq806x.
+Fix ipq40xx sleep_clk to the value QSDK defines.
+
+Also rename the sleep clock node like the GCC driver awaits.
+
+Signed-off-by: Pavel Kubelun <be.dissent@gmail.com>
+---
+ arch/arm/boot/dts/qcom-ipq4019.dtsi | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+--- a/arch/arm/boot/dts/qcom-ipq4019.dtsi
++++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi
+@@ -145,9 +145,9 @@
+ 	};
+ 
+ 	clocks {
+-		sleep_clk: sleep_clk {
++		sleep_clk: gcc_sleep_clk_src {
+ 			compatible = "fixed-clock";
+-			clock-frequency = <32768>;
++			clock-frequency = <32000>;
+ 			#clock-cells = <0>;
+ 		};
+