Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks
Hi Stephen,
On 09/02/2015 11:31 PM, Stephen Boyd wrote:
> On 08/03, Georgi Djakov wrote:
>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
>> new file mode 100644
>> index ..e564673ec3a5
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
[..]
>> +static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate,
>> +unsigned long parent_rate)
>> +{
>> +struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
>> +int ret = 0;
>> +
>> +if (r->enabled) {
>> +u32 value;
>> +struct clk_smd_rpm *peer = r->peer;
>> +
>> +/* Take peer clock's rate into account only if it's enabled. */
>> +if (peer->enabled)
>
> This peer stuff almost doesn't even matter because we're only
> sending active set requests. Why can't this code be updated to
> send both active and sleep set requests? The sleep set stuff
> won't be cached, etc., but I don't see a problem in doing both.
> Otherwise we should drop all the peer stuff until we introduce
> active only clocks.
Initially I tried sending both active and sleep sets, but as they are
not cached like in downstream (yet) i got hangs during boot. Disabling
caching in downstream kernel also caused the same hangs, so i left
this out for now. Will try debugging it further.
Will fix the rest according to your comments. Thank you!
BR,
Georgi
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks
On Thu 03 Sep 08:40 PDT 2015, Georgi Djakov wrote:
> Hi Stephen,
>
> On 09/02/2015 11:31 PM, Stephen Boyd wrote:
> > On 08/03, Georgi Djakov wrote:
> >> diff --git a/drivers/clk/qcom/clk-smd-rpm.c
> >> b/drivers/clk/qcom/clk-smd-rpm.c
> >> new file mode 100644
> >> index ..e564673ec3a5
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>
> [..]
>
> >> +static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate,
> >> + unsigned long parent_rate)
> >> +{
> >> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> >> + int ret = 0;
> >> +
> >> + if (r->enabled) {
> >> + u32 value;
> >> + struct clk_smd_rpm *peer = r->peer;
> >> +
> >> + /* Take peer clock's rate into account only if it's enabled. */
> >> + if (peer->enabled)
> >
> > This peer stuff almost doesn't even matter because we're only
> > sending active set requests. Why can't this code be updated to
> > send both active and sleep set requests? The sleep set stuff
> > won't be cached, etc., but I don't see a problem in doing both.
> > Otherwise we should drop all the peer stuff until we introduce
> > active only clocks.
>
> Initially I tried sending both active and sleep sets, but as they are
> not cached like in downstream (yet) i got hangs during boot. Disabling
> caching in downstream kernel also caused the same hangs, so i left
> this out for now. Will try debugging it further.
>
This sounds odd, although I presume the downstream code is rarely/never
tested with the caching disabled.
Can you please retry this with [1] applied (should be in -next), the RPM
fifo is tiny, so I would not be surprised if this could be your problem.
[1] https://lkml.org/lkml/2015/8/24/756
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks
On 09/03, Georgi Djakov wrote:
> Hi Stephen,
>
> On 09/02/2015 11:31 PM, Stephen Boyd wrote:
> > On 08/03, Georgi Djakov wrote:
> >> diff --git a/drivers/clk/qcom/clk-smd-rpm.c
> >> b/drivers/clk/qcom/clk-smd-rpm.c
> >> new file mode 100644
> >> index ..e564673ec3a5
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>
> [..]
>
> >> +static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate,
> >> + unsigned long parent_rate)
> >> +{
> >> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> >> + int ret = 0;
> >> +
> >> + if (r->enabled) {
> >> + u32 value;
> >> + struct clk_smd_rpm *peer = r->peer;
> >> +
> >> + /* Take peer clock's rate into account only if it's enabled. */
> >> + if (peer->enabled)
> >
> > This peer stuff almost doesn't even matter because we're only
> > sending active set requests. Why can't this code be updated to
> > send both active and sleep set requests? The sleep set stuff
> > won't be cached, etc., but I don't see a problem in doing both.
> > Otherwise we should drop all the peer stuff until we introduce
> > active only clocks.
>
> Initially I tried sending both active and sleep sets, but as they are
> not cached like in downstream (yet) i got hangs during boot. Disabling
> caching in downstream kernel also caused the same hangs, so i left
> this out for now. Will try debugging it further.
Ok. That's mildly concerning.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks
On 08/03, Georgi Djakov wrote:
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> new file mode 100644
> index ..e564673ec3a5
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -0,0 +1,165 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
Is this include used?
> +
> +#include "clk-smd-rpm.h"
> +
> +static int clk_smd_rpm_set_rate_active(struct clk_smd_rpm *r,
> +unsigned long value)
>y +{
> + struct clk_smd_rpm_req req = {
> + .key = QCOM_RPM_SMD_KEY_RATE,
> + .nbytes = sizeof(u32),
> + .value = DIV_ROUND_UP(value, 1000), /* RPM expects KHz */
s/KHz/kHz/
> + };
> +
> + return qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
> + r->rpm_res_type, r->rpm_clk_id, ,
> + sizeof(req));
> +}
> +
> +static int clk_smd_rpm_prepare(struct clk_hw *hw)
> +{
> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> + struct clk_smd_rpm *peer = r->peer;
> + u32 value;
> + int ret = 0;
> +
> + /* Don't send requests to the RPM if the rate has not been set. */
> + if (!r->rate)
> + goto out;
> +
> + /* Take peer clock's rate into account only if it's enabled. */
> + if (peer->enabled)
We need some sort of lock here. Please make an internal mutex
like the downstream code to protect accesses from one RPM clock
to another RPM clock.
> + value = max(r->rate, peer->rate);
> + else
> + value = r->rate;
> +
> + if (r->branch)
> + value = !!value;
> +
> + ret = clk_smd_rpm_set_rate_active(r, value);
> + if (ret)
> + goto out;
> +
> +out:
> + if (!ret)
> + r->enabled = true;
> +
> + return ret;
> +}
> +
> +static void clk_smd_rpm_unprepare(struct clk_hw *hw)
> +{
> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> +
> + if (r->rate) {
> + struct clk_smd_rpm *peer = r->peer;
> + unsigned long peer_rate;
> + u32 value;
Why not unsigned long?
> + int ret;
> +
> + /* Take peer clock's rate into account only if it's enabled. */
> + peer_rate = peer->enabled ? peer->rate : 0;
> + value = r->branch ? !!peer_rate : peer_rate;
> + ret = clk_smd_rpm_set_rate_active(r, value);
> + if (ret)
> + return;
> + }
> + r->enabled = false;
> +}
> +
> +static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> + int ret = 0;
> +
> + if (r->enabled) {
> + u32 value;
> + struct clk_smd_rpm *peer = r->peer;
> +
> + /* Take peer clock's rate into account only if it's enabled. */
> + if (peer->enabled)
This peer stuff almost doesn't even matter because we're only
sending active set requests. Why can't this code be updated to
send both active and sleep set requests? The sleep set stuff
won't be cached, etc., but I don't see a problem in doing both.
Otherwise we should drop all the peer stuff until we introduce
active only clocks.
> + value = max(rate, peer->rate);
> + else
> + value = rate;
> +
> + ret = clk_smd_rpm_set_rate_active(r, value);
> + if (ret)
> + goto out;
> + }
> + r->rate = rate;
> +out:
> + return ret;
> +}
> +
> +static long clk_smd_rpm_round_rate(struct clk_hw *hw, unsigned long rate,
> +unsigned long *parent_rate)
> +{
Please add a comment here like
/*
* RPM handles rate rounding and we don't have a way to
* know what the rate will be, so just return whatever
* rate is requested.
*/
> + return rate;
> +}
> +
> +static unsigned long clk_smd_rpm_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
And something similar here...
/*
* RPM handles rate rounding and we don't have a way to
* know what the rate will be, so just return whatever
* rate was
[PATCH v2 1/2] clk: qcom: Add support for RPM Clocks
This adds initial support for clocks controlled by the Resource
Power Manager (RPM) processor found on some Qualcomm SoCs.
The RPM is a dedicated hardware engine for managing the shared
SoC resources in order to keep the lowest power profile. It
communicates with other hardware subsystems via shared memory
and accepts clock requests, aggregates the requests and turns
the clocks on/off or scales them on demand.
This driver is based on the codeaurora.org driver:
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/clk/qcom/clock-rpm.c
Signed-off-by: Georgi Djakov georgi.dja...@linaro.org
---
drivers/clk/qcom/Kconfig |3 +
drivers/clk/qcom/Makefile |1 +
drivers/clk/qcom/clk-smd-rpm.c | 165
drivers/clk/qcom/clk-smd-rpm.h | 139 +
4 files changed, 308 insertions(+)
create mode 100644 drivers/clk/qcom/clk-smd-rpm.c
create mode 100644 drivers/clk/qcom/clk-smd-rpm.h
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 59d16668bdf5..e347b97aa9c7 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -5,6 +5,9 @@ config COMMON_CLK_QCOM
select REGMAP_MMIO
select RESET_CONTROLLER
+config QCOM_CLK_SMD_RPM
+ bool
+
config APQ_GCC_8084
tristate APQ8084 Global Clock Controller
depends on COMMON_CLK_QCOM
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 50b337a24a87..33adf1d97da3 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -9,6 +9,7 @@ clk-qcom-y += clk-branch.o
clk-qcom-y += clk-regmap-divider.o
clk-qcom-y += clk-regmap-mux.o
clk-qcom-y += reset.o
+clk-qcom-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
obj-$(CONFIG_APQ_GCC_8084) += gcc-apq8084.o
obj-$(CONFIG_APQ_MMCC_8084) += mmcc-apq8084.o
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
new file mode 100644
index ..e564673ec3a5
--- /dev/null
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -0,0 +1,165 @@
+/*
+ * Copyright (c) 2015, Linaro Limited
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/clk-provider.h
+#include linux/err.h
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+
+#include clk-smd-rpm.h
+
+static int clk_smd_rpm_set_rate_active(struct clk_smd_rpm *r,
+ unsigned long value)
+{
+ struct clk_smd_rpm_req req = {
+ .key = QCOM_RPM_SMD_KEY_RATE,
+ .nbytes = sizeof(u32),
+ .value = DIV_ROUND_UP(value, 1000), /* RPM expects KHz */
+ };
+
+ return qcom_rpm_smd_write(r-rpm, QCOM_SMD_RPM_ACTIVE_STATE,
+ r-rpm_res_type, r-rpm_clk_id, req,
+ sizeof(req));
+}
+
+static int clk_smd_rpm_prepare(struct clk_hw *hw)
+{
+ struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+ struct clk_smd_rpm *peer = r-peer;
+ u32 value;
+ int ret = 0;
+
+ /* Don't send requests to the RPM if the rate has not been set. */
+ if (!r-rate)
+ goto out;
+
+ /* Take peer clock's rate into account only if it's enabled. */
+ if (peer-enabled)
+ value = max(r-rate, peer-rate);
+ else
+ value = r-rate;
+
+ if (r-branch)
+ value = !!value;
+
+ ret = clk_smd_rpm_set_rate_active(r, value);
+ if (ret)
+ goto out;
+
+out:
+ if (!ret)
+ r-enabled = true;
+
+ return ret;
+}
+
+static void clk_smd_rpm_unprepare(struct clk_hw *hw)
+{
+ struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+
+ if (r-rate) {
+ struct clk_smd_rpm *peer = r-peer;
+ unsigned long peer_rate;
+ u32 value;
+ int ret;
+
+ /* Take peer clock's rate into account only if it's enabled. */
+ peer_rate = peer-enabled ? peer-rate : 0;
+ value = r-branch ? !!peer_rate : peer_rate;
+ ret = clk_smd_rpm_set_rate_active(r, value);
+ if (ret)
+ return;
+ }
+ r-enabled = false;
+}
+
+static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+ int ret = 0;
+
+ if (r-enabled) {
+ u32 value;
+
