Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1217846 > unrolled thread

Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks Stephen Boyd <sboyd@codeaurora.org> - 2015-09-02 22:40 +0200
    Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks Georgi Djakov <georgi.djakov@linaro.org> - 2015-09-03 17:50 +0200
      Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks Bjorn Andersson <bjorn.andersson@sonymobile.com> - 2015-09-03 19:30 +0200
      Re: [PATCH v2 1/2] clk: qcom: Add support for RPM Clocks Stephen Boyd <sboyd@codeaurora.org> - 2015-09-03 19:40 +0200

#1217846 — 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 000000000000..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>

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, &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)

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 set.
	 */

> +
> +	return r->rate;
> +}
> +
> +int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
> +{
> +	int ret;
> +	struct clk_smd_rpm_req req = {
> +		.key = QCOM_RPM_SMD_KEY_ENABLE,
> +		.nbytes = sizeof(u32),
> +		.value = 1,
> +	};
> +
> +	ret = qcom_rpm_smd_write(rpm, QCOM_SMD_RPM_ACTIVE_STATE,
> +				 QCOM_SMD_RPM_MISC_CLK,
> +				 QCOM_RPM_SCALING_ENABLE_ID, &req, sizeof(req));
> +	if (ret < 0) {
> +		if (ret != -EPROBE_DEFER)

Does this API return EPROBE_DEFER?

> +			pr_err("RPM clock scaling (active set) not enabled!\n");
> +		return ret;
> +	}
> +
> +	pr_debug("%s: RPM clock scaling is enabled\n", __func__);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(clk_smd_rpm_enable_scaling);
> diff --git a/drivers/clk/qcom/clk-smd-rpm.h b/drivers/clk/qcom/clk-smd-rpm.h
> new file mode 100644
> index 000000000000..7fd67c0e31b5
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-smd-rpm.h
> @@ -0,0 +1,139 @@
> +/*
> + * 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.
> + */
> +
> +#ifndef __QCOM_CLK_SMD_RPM_H__
> +#define __QCOM_CLK_SMD_RPM_H__
> +
> +#include <linux/clk-provider.h>
> +#include <linux/soc/qcom/smd-rpm.h>

Drop this include and forward declare struct qcom_smd_rpm?

> +
> +#define QCOM_RPM_KEY_SOFTWARE_ENABLE			0x6e657773
> +#define QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY	0x62636370
> +#define QCOM_RPM_SMD_KEY_RATE				0x007a484b
> +#define QCOM_RPM_SMD_KEY_ENABLE				0x62616e45
> +#define QCOM_RPM_SMD_KEY_STATE				0x54415453
> +#define QCOM_RPM_SCALING_ENABLE_ID			0x2
> +
> +struct clk_smd_rpm {
> +	const int rpm_res_type;
> +	const int rpm_key;
> +	const int rpm_clk_id;
> +	const int rpm_status_id;
> +	const bool active_only;
> +	bool enabled;
> +	bool branch;
> +	struct clk_smd_rpm *peer;
> +	struct clk_hw hw;
> +	unsigned long rate;
> +	struct qcom_smd_rpm *rpm;
> +};
> +
> +struct clk_smd_rpm_req {
> +	u32 key;
> +	u32 nbytes;
> +	u32 value;

Should all be __le32.

> +};
> +
> +extern const struct clk_ops clk_smd_rpm_ops;
> +extern const struct clk_ops clk_smd_rpm_branch_ops;
> +int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm);
> +
> +#define to_clk_smd_rpm(_hw) container_of(_hw, struct clk_smd_rpm, hw)

Can we move this to the C file? We shouldn't need to use this
outside of the file that implements the ops.


-- 
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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1218381

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 000000000000..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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218456

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 000000000000..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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218462

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 000000000000..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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web