[[PATCH,v2] 1/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

Commit Message

Taniya Das April 8, 2018, 10:32 a.m. UTC

From: Amit Nischal <[e-mail]>

Add the RPMh clock driver to control the RPMh managed clock resources on
some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by: David Collins <[e-mail]>
Signed-off-by: Amit Nischal <[e-mail]>
Signed-off-by: Taniya Das <[e-mail]>
---
 drivers/clk/qcom/Kconfig              |   9 +
 drivers/clk/qcom/Makefile             |   1 +
 drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h |  25 +++
 4 files changed, 429 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c
 create mode 100644 include/dt-bindings/clock/qcom,rpmh.h

Comments

Bjorn Andersson April 10, 2018, 5:39 p.m. UTC | #1

On Sun 08 Apr 03:32 PDT 2018, Taniya Das wrote:

> From: Amit Nischal <[e-mail]>
> 
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
> 
> Signed-off-by: David Collins <[e-mail]>
> Signed-off-by: Amit Nischal <[e-mail]>
> Signed-off-by: Taniya Das <[e-mail]>
> ---
>  drivers/clk/qcom/Kconfig              |   9 +
>  drivers/clk/qcom/Makefile             |   1 +
>  drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
>  include/dt-bindings/clock/qcom,rpmh.h |  25 +++
>  4 files changed, 429 insertions(+)
>  create mode 100644 drivers/clk/qcom/clk-rpmh.c
>  create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index fbf4532..3697a6a 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
>  	  i2c, USB, SD/eMMC, etc. Select this for the root clock
>  	  of ipq8074.
>  
> +config MSM_CLK_RPMH

QCOM_CLK_RPMH

> +	tristate "RPMh Clock Driver"
> +	depends on COMMON_CLK_QCOM && QCOM_RPMH
> +	help
> +	 RPMh manages shared resources on some Qualcomm Technologies, Inc.
> +	 SoCs. It accepts requests from other hardware subsystems via RSC.
> +	 Say Y if you want to support the clocks exposed by RPMh on
> +	 platforms such as sdm845.
> +
>  config MSM_GCC_8660
>  	tristate "MSM8660 Global Clock Controller"
>  	depends on COMMON_CLK_QCOM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 230332c..b7da05b 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
>  obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
>  obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
>  obj-$(CONFIG_MDM_LCC_9615) += lcc-mdm9615.o
> +obj-$(CONFIG_MSM_CLK_RPMH) += clk-rpmh.o
>  obj-$(CONFIG_MSM_GCC_8660) += gcc-msm8660.o
>  obj-$(CONFIG_MSM_GCC_8916) += gcc-msm8916.o
>  obj-$(CONFIG_MSM_GCC_8960) += gcc-msm8960.o
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 0000000..763401f
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,394 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

Unused

> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +
> +#include "common.h"
> +#include "clk-regmap.h"

Unused

> +
> +#define CLK_RPMH_ARC_EN_OFFSET 0
> +#define CLK_RPMH_VRM_EN_OFFSET 4
> +#define CLK_RPMH_VRM_OFF_VAL 0
> +#define CLK_RPMH_VRM_ON_VAL 1

Use a bool (true/false) rather than lugging around these two defines.

> +#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
> +					 BIT(RPMH_ACTIVE_ONLY_STATE))
> +#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
> +				      BIT(RPMH_ACTIVE_ONLY_STATE) | \
> +				      BIT(RPMH_SLEEP_STATE))
> +struct clk_rpmh {
> +	struct clk_hw hw;
> +	const char *res_name;
> +	u32 res_addr;
> +	u32 res_en_offset;
> +	u32 res_on_val;
> +	u32 res_off_val;

res_off_val is 0 for all clocks.

> +	u32 state;
> +	u32 aggr_state;
> +	u32 last_sent_aggr_state;

Through the use of some local variables you shouldn't have to lug around
aggr_state and last_sent_aggr_state.

> +	u32 valid_state_mask;
> +	struct rpmh_client *rpmh_client;
> +	unsigned long rate;
> +	struct clk_rpmh *peer;
> +};
> +
> +struct rpmh_cc {
> +	struct clk_onecell_data data;
> +	struct clk *clks[];
> +};
> +
> +struct clk_rpmh_desc {
> +	struct clk_hw **clks;
> +	size_t num_clks;
> +};
> +
> +static DEFINE_MUTEX(rpmh_clk_lock);
> +
> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
> +			  _res_en_offset, _res_on, _res_off, _rate, \
> +			  _state_mask, _state_on_mask)			      \
> +	static struct clk_rpmh _platform##_##_name_active;		      \
> +	static struct clk_rpmh _platform##_##_name = {			      \
> +		.res_name = _res_name,					      \
> +		.res_en_offset = _res_en_offset,			      \
> +		.res_on_val = _res_on,					      \
> +		.res_off_val = _res_off,				      \
> +		.rate = _rate,						      \
> +		.peer = &_platform##_##_name_active,			      \
> +		.valid_state_mask = _state_mask,			      \

This is always CLK_RPMH_APPS_RSC_STATE_MASK

> +		.hw.init = &(struct clk_init_data){			      \
> +			.ops = &clk_rpmh_ops,			              \
> +			.name = #_name,					      \
> +		},							      \
> +	};								      \
> +	static struct clk_rpmh _platform##_##_name_active = {		      \
> +		.res_name = _res_name,					      \
> +		.res_en_offset = _res_en_offset,			      \
> +		.res_on_val = _res_on,					      \
> +		.res_off_val = _res_off,				      \
> +		.rate = _rate,						      \
> +		.peer = &_platform##_##_name,				      \
> +		.valid_state_mask = _state_on_mask,			      \

and this is always CLK_RPMH_APPS_RSC_AO_STATE_MASK. If you just hard
code them here (until there's a need for them to be anything else) the
clock list below becomes tidier.

> +		.hw.init = &(struct clk_init_data){			      \
> +			.ops = &clk_rpmh_ops,				      \
> +			.name = #_name_active,				      \
> +		},							      \
> +	}
> +
> +#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
> +			    _res_on, _res_off, _rate, _state_mask, \
> +			    _state_on_mask)				\
> +	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
> +			  CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off, \
> +			  _rate, _state_mask, _state_on_mask)
> +
> +#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name,	\
> +			    _rate, _state_mask, _state_on_mask) \
> +	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
> +			  CLK_RPMH_VRM_EN_OFFSET, CLK_RPMH_VRM_ON_VAL,	\
> +			  CLK_RPMH_VRM_OFF_VAL, _rate, _state_mask, \
> +			  _state_on_mask)
> +
> +static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
> +{
> +	return container_of(_hw, struct clk_rpmh, hw);
> +}
> +
> +static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
> +{
> +	return ((c->last_sent_aggr_state & BIT(state))
> +		!= (c->aggr_state & BIT(state)));
> +}
> +
> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
> +{
> +	struct tcs_cmd cmd = { 0 };
> +	int ret = 0;
> +
> +	cmd.addr = c->res_addr + c->res_en_offset;
> +
> +	if (has_state_changed(c, RPMH_SLEEP_STATE)) {
> +		cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE))
> +				? c->res_on_val : c->res_off_val;

As these values are reused several times in this function you could by
using some local variables get this down to the much more readable:

		cmd.data = state & BIT(RPMH_SLEEP_STATE) ? on_val : off_val;

And as off_val is 0 for all your clocks you can even drop the latter.

> +		ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
> +				       &cmd, 1);
> +		if (ret) {
> +			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n",
> +			       __func__, c->res_name, RPMH_SLEEP_STATE, ret);

Please drop the __func__, the error string is unique in this driver; and
rather than passing a constant to a %d actually write your error message
in a human readable form, like: "set sleep state of %s failed: %d\n".

And please do carry the struct device, so that you can use dev_err()
instead.

> +			return ret;
> +		}
> +	}
> +
> +	if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
> +		cmd.data = (c->aggr_state & BIT(RPMH_WAKE_ONLY_STATE))
> +				? c->res_on_val : c->res_off_val;
> +		ret = rpmh_write_async(c->rpmh_client,
> +				       RPMH_WAKE_ONLY_STATE, &cmd, 1);
> +		if (ret) {
> +			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n"

You're missing a , for this to compile.

> +			       __func__, c->res_name, RPMH_WAKE_ONLY_STATE,
> +				 ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
> +		cmd.data = (c->aggr_state & BIT(RPMH_ACTIVE_ONLY_STATE))
> +				? c->res_on_val : c->res_off_val;
> +		ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
> +				 &cmd, 1);
> +		if (ret) {
> +			pr_err("%s: rpmh_write(%s, state=%d) failed (%d)\n",
> +			       __func__, c->res_name, RPMH_ACTIVE_ONLY_STATE,
> +				 ret);
> +			return ret;
> +		}
> +	}

But, RPMH_SLEEP_STATE, RPMH_WAKE_ONLY_STATE and RPMH_ACTIVE_ONLY_STATE
are values in an enum, so you could roll these up in a for loop and
reduce the duplication.

> +
> +	c->last_sent_aggr_state = c->aggr_state;
> +	c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
> +
> +	return 0;
> +}
> +
> +static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
> +						bool enable)
> +{
> +	/* Update state and aggregate state values based on enable value. */
> +	c->state = enable ? c->valid_state_mask : 0;
> +	c->aggr_state = c->state | c->peer->state;
> +	c->peer->aggr_state = c->aggr_state;
> +
> +	ret = clk_rpmh_send_aggregate_command(c);

"ret" doesn't exist.

> +	if (ret && enable)
> +		c->state = 0;
> +	else if (ret) {

If you have any part of your conditional wrapped in braces do wrap all
the others too.

> +		c->state = c->valid_state_mask;
> +		WARN(1, "clk: %s failed to disable\n", c->res_name);
> +	}
> +
> +	return ret;
> +}
> +
> +static int clk_rpmh_prepare(struct clk_hw *hw)
> +{
> +	struct clk_rpmh *c = to_clk_rpmh(hw);
> +	int ret = 0;
> +
> +	mutex_lock(&rpmh_clk_lock);
> +
> +	if (c->state)
> +		goto out;
> +
> +	ret = clk_rpmh_aggregate_state_send_command(c, true);
> +out:
> +	mutex_unlock(&rpmh_clk_lock);
> +	return ret;
> +};
> +
> +static void clk_rpmh_unprepare(struct clk_hw *hw)
> +{
> +	struct clk_rpmh *c = to_clk_rpmh(hw);
> +
> +	mutex_lock(&rpmh_clk_lock);
> +
> +	if (!c->state)
> +		goto out;
> +
> +	clk_rpmh_aggregate_state_send_command(c, false);
> +out:
> +	mutex_unlock(&rpmh_clk_lock);
> +	return;
> +};
> +
> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
> +					  unsigned long parent_rate)
> +{
> +	struct clk_rpmh *r = to_clk_rpmh(hw);
> +
> +	/*
> +	 * RPMh clocks have a fixed rate. Return static rate set
> +	 * at init time.
> +	 */
> +	return r->rate;
> +}
> +
> +static const struct clk_ops clk_rpmh_ops = {
> +	.prepare	= clk_rpmh_prepare,
> +	.unprepare	= clk_rpmh_unprepare,
> +	.recalc_rate	= clk_rpmh_recalc_rate,
> +};
> +
> +/* Resource name must match resource id present in cmd-db. */
> +DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0,
> +		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
> +DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2",
> +		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
> +DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3",
> +		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1",
> +		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2",
> +		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3",
> +		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
> +
> +static struct clk_hw *sdm845_rpmh_clocks[] = {
> +	[RPMH_CXO_CLK]		= &sdm845_bi_tcxo.hw,
> +	[RPMH_CXO_CLK_A]	= &sdm845_bi_tcxo_ao.hw,

Registering these fails with EEXIST because the gcc driver also register
them.

> +	[RPMH_LN_BB_CLK2]	= &sdm845_ln_bb_clk2.hw,
> +	[RPMH_LN_BB_CLK2_A]	= &sdm845_ln_bb_clk2_ao.hw,
> +	[RPMH_LN_BB_CLK3]	= &sdm845_ln_bb_clk3.hw,
> +	[RPMH_LN_BB_CLK3_A]	= &sdm845_ln_bb_clk3_ao.hw,
> +	[RPMH_RF_CLK1]		= &sdm845_rf_clk1.hw,
> +	[RPMH_RF_CLK1_A]	= &sdm845_rf_clk1_ao.hw,
> +	[RPMH_RF_CLK2]		= &sdm845_rf_clk2.hw,
> +	[RPMH_RF_CLK2_A]	= &sdm845_rf_clk2_ao.hw,
> +	[RPMH_RF_CLK3]		= &sdm845_rf_clk3.hw,
> +	[RPMH_RF_CLK3_A]	= &sdm845_rf_clk3_ao.hw,
> +};
> +
> +static const struct clk_rpmh_desc clk_rpmh_sdm845 = {
> +	.clks = sdm845_rpmh_clocks,
> +	.num_clks = ARRAY_SIZE(sdm845_rpmh_clocks),
> +};
> +
> +static const struct of_device_id clk_rpmh_match_table[] = {
> +	{ .compatible = "qcom,rpmh-clk-sdm845", .data = &clk_rpmh_sdm845},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);

Please move the match_table just prior to the definition of your
platform_driver.

> +
> +static int clk_rpmh_probe(struct platform_device *pdev)
> +{
> +	struct clk **clks;
> +	struct clk *clk;
> +	struct rpmh_cc *rcc;
> +	struct clk_onecell_data *data;
> +	int ret;
> +	size_t num_clks, i;
> +	struct clk_hw **hw_clks;
> +	struct clk_rpmh *rpmh_clk;
> +	const struct clk_rpmh_desc *desc;
> +	struct property *prop;

prop is unused.

> +	struct rpmh_client *rpmh_client = NULL;

Don't goto err until you have acquired rpmh_client and you don't need to
initialize this variable.

> +
> +	desc = of_device_get_match_data(&pdev->dev);
> +	if (!desc) {
> +		ret = -EINVAL;
> +		goto err;
> +	}

This won't happen, no need to check for it.

> +
> +	ret = cmd_db_ready();
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER) {
> +			dev_err(&pdev->dev, "Command DB not available (%d)\n",
> +				ret);
> +			goto err;

goto err? There's nothing to clean up at this point.

> +		}
> +		return ret;
> +	}
> +
> +	rpmh_client = rpmh_get_client(pdev);
> +	if (IS_ERR(rpmh_client)) {
> +		ret = PTR_ERR(rpmh_client);
> +		if (ret != -EPROBE_DEFER)

You're getting a handle to your parent, it's not going to return
-EPROBE_DEFER.

> +			dev_err(&pdev->dev, "failed to request RPMh client, ret=%d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	hw_clks = desc->clks;
> +	num_clks = desc->num_clks;
> +
> +	rcc = devm_kzalloc(&pdev->dev, sizeof(*rcc) + sizeof(*clks) * num_clks,
> +			   GFP_KERNEL);
> +	if (!rcc) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	clks = rcc->clks;
> +	data = &rcc->data;
> +	data->clks = clks;
> +	data->clk_num = num_clks;
> +
> +	for (i = 0; i < num_clks; i++) {
> +		rpmh_clk = to_clk_rpmh(hw_clks[i]);
> +		rpmh_clk->res_addr = cmd_db_read_addr(rpmh_clk->res_name);
> +		if (!rpmh_clk->res_addr) {
> +			dev_err(&pdev->dev, "missing RPMh resource address for %s\n",
> +				rpmh_clk->res_name);
> +			ret = -ENODEV;
> +			goto err;
> +		}
> +
> +		rpmh_clk->rpmh_client = rpmh_client;
> +
> +		clk = devm_clk_register(&pdev->dev, hw_clks[i]);
> +		if (IS_ERR(clk)) {

Please add:

dev_err(&pdev->dev, "failed to register %s\n", hw_clks[i]->init->name);

> +			ret = PTR_ERR(clk);
> +			goto err;
> +		}
> +
> +		clks[i] = clk;
> +	}
> +
> +	ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
> +				  data);
> +	if (ret)

Please add:

dev_err(&pdev->dev, "failed to add clock provider\n");

> +		goto err;
> +
> +	dev_info(&pdev->dev, "Registered RPMh clocks\n");

Please don't spam the kernel log.

> +	return ret;

ret can't be anything but 0 here, so let's make it easer to read by
writing 0 here.

> +
> +err:
> +	if (rpmh_client)
> +		rpmh_release(rpmh_client);

This is annoying (but not your fault).

> +
> +	dev_err(&pdev->dev, "Error registering RPMh Clock driver (%d)\n", ret);

Testing the driver I got this error message, it didn't help! Had to add
the two dev_err above, just drop this one.

> +	return ret;
> +}
> +
> +static struct platform_driver clk_rpmh_driver = {
> +	.probe		= clk_rpmh_probe,

Your driver is tristate and works just fine as a kernel module, so you
need a remove function to call rpmh_release(rpmh_client) - or we need to
get rid of the need to call that.

> +	.driver		= {
> +		.name	= "clk-rpmh",
> +		.of_match_table = clk_rpmh_match_table,
> +	},
> +};
> +
> +static int __init clk_rpmh_init(void)
> +{
> +	return platform_driver_register(&clk_rpmh_driver);
> +}
> +subsys_initcall(clk_rpmh_init);
> +
> +static void __exit clk_rpmh_exit(void)
> +{
> +	platform_driver_unregister(&clk_rpmh_driver);
> +}
> +module_exit(clk_rpmh_exit);
> +
> +MODULE_DESCRIPTION("QTI RPMh Clock Driver");

We use "Qualcomm" or "QCOM" in all existing driver, can you please use
that here too?

> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:clk-rpmh");

Nothing is going to attempt to autoload a driver based on the alias
platform:clk-rpmh, so just drop this.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring April 13, 2018, 4:37 p.m. UTC | #2

On Sun, Apr 08, 2018 at 04:02:12PM +0530, Taniya Das wrote:
> From: Amit Nischal <[e-mail]>
> 
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
> 
> Signed-off-by: David Collins <[e-mail]>
> Signed-off-by: Amit Nischal <[e-mail]>
> Signed-off-by: Taniya Das <[e-mail]>
> ---
>  drivers/clk/qcom/Kconfig              |   9 +
>  drivers/clk/qcom/Makefile             |   1 +
>  drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++

>  include/dt-bindings/clock/qcom,rpmh.h |  25 +++

This goes with the binding patch which should come first in the series.

>  4 files changed, 429 insertions(+)
>  create mode 100644 drivers/clk/qcom/clk-rpmh.c
>  create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Taniya Das April 14, 2018, 2:02 a.m. UTC | #3

Hello Bjorn,

Thank you for the review comments.

On 4/10/2018 11:09 PM, Bjorn Andersson wrote:
> On Sun 08 Apr 03:32 PDT 2018, Taniya Das wrote:
> 
>> From: Amit Nischal <[e-mail]>
>>
>> Add the RPMh clock driver to control the RPMh managed clock resources on
>> some of the Qualcomm Technologies, Inc. SoCs.
>>
>> Signed-off-by: David Collins <[e-mail]>
>> Signed-off-by: Amit Nischal <[e-mail]>
>> Signed-off-by: Taniya Das <[e-mail]>
>> ---
>>   drivers/clk/qcom/Kconfig              |   9 +
>>   drivers/clk/qcom/Makefile             |   1 +
>>   drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
>>   include/dt-bindings/clock/qcom,rpmh.h |  25 +++
>>   4 files changed, 429 insertions(+)
>>   create mode 100644 drivers/clk/qcom/clk-rpmh.c
>>   create mode 100644 include/dt-bindings/clock/qcom,rpmh.h
>>
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index fbf4532..3697a6a 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
>>   	  i2c, USB, SD/eMMC, etc. Select this for the root clock
>>   	  of ipq8074.
>>   
>> +config MSM_CLK_RPMH
> 
> QCOM_CLK_RPMH
> 

Would update it to use "QCOM_CLK_RPMH".

>> +	tristate "RPMh Clock Driver"
>> +	depends on COMMON_CLK_QCOM && QCOM_RPMH
>> +	help
>> +	 RPMh manages shared resources on some Qualcomm Technologies, Inc.
>> +	 SoCs. It accepts requests from other hardware subsystems via RSC.
>> +	 Say Y if you want to support the clocks exposed by RPMh on
>> +	 platforms such as sdm845.
>> +
>>   config MSM_GCC_8660
>>   	tristate "MSM8660 Global Clock Controller"
>>   	depends on COMMON_CLK_QCOM
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index 230332c..b7da05b 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
>>   obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
>>   obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
>>   obj-$(CONFIG_MDM_LCC_9615) += lcc-mdm9615.o
>> +obj-$(CONFIG_MSM_CLK_RPMH) += clk-rpmh.o
>>   obj-$(CONFIG_MSM_GCC_8660) += gcc-msm8660.o
>>   obj-$(CONFIG_MSM_GCC_8916) += gcc-msm8916.o
>>   obj-$(CONFIG_MSM_GCC_8960) += gcc-msm8960.o
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> new file mode 100644
>> index 0000000..763401f
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -0,0 +1,394 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
> 
> Unused
> 
>> +#include <soc/qcom/cmd-db.h>
>> +#include <soc/qcom/rpmh.h>
>> +
>> +#include <dt-bindings/clock/qcom,rpmh.h>
>> +
>> +#include "common.h"
>> +#include "clk-regmap.h"
> 
> Unused
> 

I would remove the unused header includes.

>> +
>> +#define CLK_RPMH_ARC_EN_OFFSET 0
>> +#define CLK_RPMH_VRM_EN_OFFSET 4
>> +#define CLK_RPMH_VRM_OFF_VAL 0
>> +#define CLK_RPMH_VRM_ON_VAL 1
> 
> Use a bool (true/false) rather than lugging around these two defines.

I would remove these in the next patch.

> 
>> +#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
>> +					 BIT(RPMH_ACTIVE_ONLY_STATE))
>> +#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
>> +				      BIT(RPMH_ACTIVE_ONLY_STATE) | \
>> +				      BIT(RPMH_SLEEP_STATE))
>> +struct clk_rpmh {
>> +	struct clk_hw hw;
>> +	const char *res_name;
>> +	u32 res_addr;
>> +	u32 res_en_offset;
>> +	u32 res_on_val;
>> +	u32 res_off_val;
> 
> res_off_val is 0 for all clocks.
> 

They could change if required, so is the reason for keeping it.

>> +	u32 state;
>> +	u32 aggr_state;
>> +	u32 last_sent_aggr_state;
> 
> Through the use of some local variables you shouldn't have to lug around
> aggr_state and last_sent_aggr_state.
> 

We need to check if the last state and the current state requested, 
has_state_changed().

>> +	u32 valid_state_mask;
>> +	struct rpmh_client *rpmh_client;
>> +	unsigned long rate;
>> +	struct clk_rpmh *peer;
>> +};
>> +
>> +struct rpmh_cc {
>> +	struct clk_onecell_data data;
>> +	struct clk *clks[];
>> +};
>> +
>> +struct clk_rpmh_desc {
>> +	struct clk_hw **clks;
>> +	size_t num_clks;
>> +};
>> +
>> +static DEFINE_MUTEX(rpmh_clk_lock);
>> +
>> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
>> +			  _res_en_offset, _res_on, _res_off, _rate, \
>> +			  _state_mask, _state_on_mask)			      \
>> +	static struct clk_rpmh _platform##_##_name_active;		      \
>> +	static struct clk_rpmh _platform##_##_name = {			      \
>> +		.res_name = _res_name,					      \
>> +		.res_en_offset = _res_en_offset,			      \
>> +		.res_on_val = _res_on,					      \
>> +		.res_off_val = _res_off,				      \
>> +		.rate = _rate,						      \
>> +		.peer = &_platform##_##_name_active,			      \
>> +		.valid_state_mask = _state_mask,			      \
> 
> This is always CLK_RPMH_APPS_RSC_STATE_MASK
> 
>> +		.hw.init = &(struct clk_init_data){			      \
>> +			.ops = &clk_rpmh_ops,			              \
>> +			.name = #_name,					      \
>> +		},							      \
>> +	};								      \
>> +	static struct clk_rpmh _platform##_##_name_active = {		      \
>> +		.res_name = _res_name,					      \
>> +		.res_en_offset = _res_en_offset,			      \
>> +		.res_on_val = _res_on,					      \
>> +		.res_off_val = _res_off,				      \
>> +		.rate = _rate,						      \
>> +		.peer = &_platform##_##_name,				      \
>> +		.valid_state_mask = _state_on_mask,			      \
> 
> and this is always CLK_RPMH_APPS_RSC_AO_STATE_MASK. If you just hard
> code them here (until there's a need for them to be anything else) the
> clock list below becomes tidier.
> 

As of now the state_mask is CLK_RPMH_APPS_RSC_STATE_MASK and 
state_on_mask is CLK_RPMH_APPS_RSC_AO_STATE_MASK. I would update the 
logic if the state masks changes.

>> +		.hw.init = &(struct clk_init_data){			      \
>> +			.ops = &clk_rpmh_ops,				      \
>> +			.name = #_name_active,				      \
>> +		},							      \
>> +	}
>> +
>> +#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
>> +			    _res_on, _res_off, _rate, _state_mask, \
>> +			    _state_on_mask)				\
>> +	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
>> +			  CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off, \
>> +			  _rate, _state_mask, _state_on_mask)
>> +
>> +#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name,	\
>> +			    _rate, _state_mask, _state_on_mask) \
>> +	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
>> +			  CLK_RPMH_VRM_EN_OFFSET, CLK_RPMH_VRM_ON_VAL,	\
>> +			  CLK_RPMH_VRM_OFF_VAL, _rate, _state_mask, \
>> +			  _state_on_mask)
>> +
>> +static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
>> +{
>> +	return container_of(_hw, struct clk_rpmh, hw);
>> +}
>> +
>> +static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
>> +{
>> +	return ((c->last_sent_aggr_state & BIT(state))
>> +		!= (c->aggr_state & BIT(state)));
>> +}
>> +
>> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
>> +{
>> +	struct tcs_cmd cmd = { 0 };
>> +	int ret = 0;
>> +
>> +	cmd.addr = c->res_addr + c->res_en_offset;
>> +
>> +	if (has_state_changed(c, RPMH_SLEEP_STATE)) {
>> +		cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE))
>> +				? c->res_on_val : c->res_off_val;
> 
> As these values are reused several times in this function you could by
> using some local variables get this down to the much more readable:
> 
> 		cmd.data = state & BIT(RPMH_SLEEP_STATE) ? on_val : off_val;
> 
> And as off_val is 0 for all your clocks you can even drop the latter.
> 
>> +		ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
>> +				       &cmd, 1);
>> +		if (ret) {
>> +			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n",
>> +			       __func__, c->res_name, RPMH_SLEEP_STATE, ret);
> 
> Please drop the __func__, the error string is unique in this driver; and
> rather than passing a constant to a %d actually write your error message
> in a human readable form, like: "set sleep state of %s failed: %d\n".
> 
> And please do carry the struct device, so that you can use dev_err()
> instead.
> 

Thanks, I would take care of the above comments and also loop thorugh 
the enum to avoid duplication in the next patch.

>> +			return ret;
>> +		}
>> +	}
>> +
>> +	if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
>> +		cmd.data = (c->aggr_state & BIT(RPMH_WAKE_ONLY_STATE))
>> +				? c->res_on_val : c->res_off_val;
>> +		ret = rpmh_write_async(c->rpmh_client,
>> +				       RPMH_WAKE_ONLY_STATE, &cmd, 1);
>> +		if (ret) {
>> +			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n"
> 
> You're missing a , for this to compile.
> 

Thanks, would fix in the next patch.

>> +			       __func__, c->res_name, RPMH_WAKE_ONLY_STATE,
>> +				 ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
>> +		cmd.data = (c->aggr_state & BIT(RPMH_ACTIVE_ONLY_STATE))
>> +				? c->res_on_val : c->res_off_val;
>> +		ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
>> +				 &cmd, 1);
>> +		if (ret) {
>> +			pr_err("%s: rpmh_write(%s, state=%d) failed (%d)\n",
>> +			       __func__, c->res_name, RPMH_ACTIVE_ONLY_STATE,
>> +				 ret);
>> +			return ret;
>> +		}
>> +	}
> 
> But, RPMH_SLEEP_STATE, RPMH_WAKE_ONLY_STATE and RPMH_ACTIVE_ONLY_STATE
> are values in an enum, so you could roll these up in a for loop and
> reduce the duplication.
> 
>> +
>> +	c->last_sent_aggr_state = c->aggr_state;
>> +	c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
>> +
>> +	return 0;
>> +}
>> +
>> +static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
>> +						bool enable)
>> +{
>> +	/* Update state and aggregate state values based on enable value. */
>> +	c->state = enable ? c->valid_state_mask : 0;
>> +	c->aggr_state = c->state | c->peer->state;
>> +	c->peer->aggr_state = c->aggr_state;
>> +
>> +	ret = clk_rpmh_send_aggregate_command(c);
> 
> "ret" doesn't exist.
> 

Thanks, I would add the missing variable.

>> +	if (ret && enable)
>> +		c->state = 0;
>> +	else if (ret) {
> 
> If you have any part of your conditional wrapped in braces do wrap all
> the others too.
> 

Thanks, would take care in the next patch.

>> +		c->state = c->valid_state_mask;
>> +		WARN(1, "clk: %s failed to disable\n", c->res_name);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int clk_rpmh_prepare(struct clk_hw *hw)
>> +{
>> +	struct clk_rpmh *c = to_clk_rpmh(hw);
>> +	int ret = 0;
>> +
>> +	mutex_lock(&rpmh_clk_lock);
>> +
>> +	if (c->state)
>> +		goto out;
>> +
>> +	ret = clk_rpmh_aggregate_state_send_command(c, true);
>> +out:
>> +	mutex_unlock(&rpmh_clk_lock);
>> +	return ret;
>> +};
>> +
>> +static void clk_rpmh_unprepare(struct clk_hw *hw)
>> +{
>> +	struct clk_rpmh *c = to_clk_rpmh(hw);
>> +
>> +	mutex_lock(&rpmh_clk_lock);
>> +
>> +	if (!c->state)
>> +		goto out;
>> +
>> +	clk_rpmh_aggregate_state_send_command(c, false);
>> +out:
>> +	mutex_unlock(&rpmh_clk_lock);
>> +	return;
>> +};
>> +
>> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
>> +					  unsigned long parent_rate)
>> +{
>> +	struct clk_rpmh *r = to_clk_rpmh(hw);
>> +
>> +	/*
>> +	 * RPMh clocks have a fixed rate. Return static rate set
>> +	 * at init time.
>> +	 */
>> +	return r->rate;
>> +}
>> +
>> +static const struct clk_ops clk_rpmh_ops = {
>> +	.prepare	= clk_rpmh_prepare,
>> +	.unprepare	= clk_rpmh_unprepare,
>> +	.recalc_rate	= clk_rpmh_recalc_rate,
>> +};
>> +
>> +/* Resource name must match resource id present in cmd-db. */
>> +DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0,
>> +		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
>> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
>> +DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2",
>> +		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
>> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
>> +DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3",
>> +		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
>> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
>> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1",
>> +		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
>> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
>> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2",
>> +		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
>> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
>> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3",
>> +		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
>> +		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
>> +
>> +static struct clk_hw *sdm845_rpmh_clocks[] = {
>> +	[RPMH_CXO_CLK]		= &sdm845_bi_tcxo.hw,
>> +	[RPMH_CXO_CLK_A]	= &sdm845_bi_tcxo_ao.hw,
> 
> Registering these fails with EEXIST because the gcc driver also register
> them.
> 

It was added till the time RPMh driver was not ready. We would submit 
removing the change from GCC driver.

>> +	[RPMH_LN_BB_CLK2]	= &sdm845_ln_bb_clk2.hw,
>> +	[RPMH_LN_BB_CLK2_A]	= &sdm845_ln_bb_clk2_ao.hw,
>> +	[RPMH_LN_BB_CLK3]	= &sdm845_ln_bb_clk3.hw,
>> +	[RPMH_LN_BB_CLK3_A]	= &sdm845_ln_bb_clk3_ao.hw,
>> +	[RPMH_RF_CLK1]		= &sdm845_rf_clk1.hw,
>> +	[RPMH_RF_CLK1_A]	= &sdm845_rf_clk1_ao.hw,
>> +	[RPMH_RF_CLK2]		= &sdm845_rf_clk2.hw,
>> +	[RPMH_RF_CLK2_A]	= &sdm845_rf_clk2_ao.hw,
>> +	[RPMH_RF_CLK3]		= &sdm845_rf_clk3.hw,
>> +	[RPMH_RF_CLK3_A]	= &sdm845_rf_clk3_ao.hw,
>> +};
>> +
>> +static const struct clk_rpmh_desc clk_rpmh_sdm845 = {
>> +	.clks = sdm845_rpmh_clocks,
>> +	.num_clks = ARRAY_SIZE(sdm845_rpmh_clocks),
>> +};
>> +
>> +static const struct of_device_id clk_rpmh_match_table[] = {
>> +	{ .compatible = "qcom,rpmh-clk-sdm845", .data = &clk_rpmh_sdm845},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);
> 
> Please move the match_table just prior to the definition of your
> platform_driver.
> 

Would update this in the next patch.

>> +
>> +static int clk_rpmh_probe(struct platform_device *pdev)
>> +{
>> +	struct clk **clks;
>> +	struct clk *clk;
>> +	struct rpmh_cc *rcc;
>> +	struct clk_onecell_data *data;
>> +	int ret;
>> +	size_t num_clks, i;
>> +	struct clk_hw **hw_clks;
>> +	struct clk_rpmh *rpmh_clk;
>> +	const struct clk_rpmh_desc *desc;
>> +	struct property *prop;
> 
> prop is unused.
> 

Thanks, would remove the unused variable.

>> +	struct rpmh_client *rpmh_client = NULL;
> 
> Don't goto err until you have acquired rpmh_client and you don't need to
> initialize this variable.
> 

Would remove the variable initialization in the next patch.

>> +
>> +	desc = of_device_get_match_data(&pdev->dev);
>> +	if (!desc) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
> 
> This won't happen, no need to check for it.
> 

Would remove this check too in the next patch.

>> +
>> +	ret = cmd_db_ready();
>> +	if (ret) {
>> +		if (ret != -EPROBE_DEFER) {
>> +			dev_err(&pdev->dev, "Command DB not available (%d)\n",
>> +				ret);
>> +			goto err;
> 
> goto err? There's nothing to clean up at this point.
> 

I would fix this in the next patch.

>> +		}
>> +		return ret;
>> +	}
>> +
>> +	rpmh_client = rpmh_get_client(pdev);
>> +	if (IS_ERR(rpmh_client)) {
>> +		ret = PTR_ERR(rpmh_client);
>> +		if (ret != -EPROBE_DEFER)
> 
> You're getting a handle to your parent, it's not going to return
> -EPROBE_DEFER.
>

I would fix the error path in the next patch.

>> +			dev_err(&pdev->dev, "failed to request RPMh client, ret=%d\n",
>> +				ret);
>> +		return ret;
>> +	}
>> +
>> +	hw_clks = desc->clks;
>> +	num_clks = desc->num_clks;
>> +
>> +	rcc = devm_kzalloc(&pdev->dev, sizeof(*rcc) + sizeof(*clks) * num_clks,
>> +			   GFP_KERNEL);
>> +	if (!rcc) {
>> +		ret = -ENOMEM;
>> +		goto err;
>> +	}
>> +
>> +	clks = rcc->clks;
>> +	data = &rcc->data;
>> +	data->clks = clks;
>> +	data->clk_num = num_clks;
>> +
>> +	for (i = 0; i < num_clks; i++) {
>> +		rpmh_clk = to_clk_rpmh(hw_clks[i]);
>> +		rpmh_clk->res_addr = cmd_db_read_addr(rpmh_clk->res_name);
>> +		if (!rpmh_clk->res_addr) {
>> +			dev_err(&pdev->dev, "missing RPMh resource address for %s\n",
>> +				rpmh_clk->res_name);
>> +			ret = -ENODEV;
>> +			goto err;
>> +		}
>> +
>> +		rpmh_clk->rpmh_client = rpmh_client;
>> +
>> +		clk = devm_clk_register(&pdev->dev, hw_clks[i]);
>> +		if (IS_ERR(clk)) {
> 
> Please add:
> 
> dev_err(&pdev->dev, "failed to register %s\n", hw_clks[i]->init->name);
> 
>> +			ret = PTR_ERR(clk);
>> +			goto err;
>> +		}
>> +
>> +		clks[i] = clk;
>> +	}
>> +
>> +	ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
>> +				  data);
>> +	if (ret)
> 
> Please add:
> 
> dev_err(&pdev->dev, "failed to add clock provider\n");
> 
>> +		goto err;
>> +
>> +	dev_info(&pdev->dev, "Registered RPMh clocks\n");
> 
> Please don't spam the kernel log.
> 
>> +	return ret;
> 
> ret can't be anything but 0 here, so let's make it easer to read by
> writing 0 here.
> 
>> +
>> +err:
>> +	if (rpmh_client)
>> +		rpmh_release(rpmh_client);
> 
> This is annoying (but not your fault).
> 
>> +
>> +	dev_err(&pdev->dev, "Error registering RPMh Clock driver (%d)\n", ret);
> 
> Testing the driver I got this error message, it didn't help! Had to add
> the two dev_err above, just drop this one.
> 

I would take care of the above comments in the next patch.

>> +	return ret;
>> +}
>> +
>> +static struct platform_driver clk_rpmh_driver = {
>> +	.probe		= clk_rpmh_probe,
> 
> Your driver is tristate and works just fine as a kernel module, so you
> need a remove function to call rpmh_release(rpmh_client) - or we need to
> get rid of the need to call that.
> 

Yes, I would add the remove and do the rpmh_client and of_del_provider 
clean ups.

>> +	.driver		= {
>> +		.name	= "clk-rpmh",
>> +		.of_match_table = clk_rpmh_match_table,
>> +	},
>> +};
>> +
>> +static int __init clk_rpmh_init(void)
>> +{
>> +	return platform_driver_register(&clk_rpmh_driver);
>> +}
>> +subsys_initcall(clk_rpmh_init);
>> +
>> +static void __exit clk_rpmh_exit(void)
>> +{
>> +	platform_driver_unregister(&clk_rpmh_driver);
>> +}
>> +module_exit(clk_rpmh_exit);
>> +
>> +MODULE_DESCRIPTION("QTI RPMh Clock Driver");
> 
> We use "Qualcomm" or "QCOM" in all existing driver, can you please use
> that here too?
> 

Would update it to use "QCOM".

>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:clk-rpmh");
> 
> Nothing is going to attempt to autoload a driver based on the alias
> platform:clk-rpmh, so just drop this.
> 

Would drop this in the next patch.

> Regards,
> Bjorn
>

Taniya Das April 14, 2018, 2:02 a.m. UTC | #4

Hello Rob,

Thank you for the review comments.

On 4/13/2018 10:07 PM, Rob Herring wrote:
> On Sun, Apr 08, 2018 at 04:02:12PM +0530, Taniya Das wrote:
>> From: Amit Nischal <[e-mail]>
>>
>> Add the RPMh clock driver to control the RPMh managed clock resources on
>> some of the Qualcomm Technologies, Inc. SoCs.
>>
>> Signed-off-by: David Collins <[e-mail]>
>> Signed-off-by: Amit Nischal <[e-mail]>
>> Signed-off-by: Taniya Das <[e-mail]>
>> ---
>>   drivers/clk/qcom/Kconfig              |   9 +
>>   drivers/clk/qcom/Makefile             |   1 +
>>   drivers/clk/qcom/clk-rpmh.c           | 394 ++++++++++++++++++++++++++++++++++
> 
>>   include/dt-bindings/clock/qcom,rpmh.h |  25 +++
> 
> This goes with the binding patch which should come first in the series.
> 

I would make the change in the next patch series.

>>   4 files changed, 429 insertions(+)
>>   create mode 100644 drivers/clk/qcom/clk-rpmh.c
>>   create mode 100644 include/dt-bindings/clock/qcom,rpmh.h

Patch

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..3697a6a 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -112,6 +112,15 @@  config IPQ_GCC_8074
 	  i2c, USB, SD/eMMC, etc. Select this for the root clock
 	  of ipq8074.
 
+config MSM_CLK_RPMH
+	tristate "RPMh Clock Driver"
+	depends on COMMON_CLK_QCOM && QCOM_RPMH
+	help
+	 RPMh manages shared resources on some Qualcomm Technologies, Inc.
+	 SoCs. It accepts requests from other hardware subsystems via RSC.
+	 Say Y if you want to support the clocks exposed by RPMh on
+	 platforms such as sdm845.
+
 config MSM_GCC_8660
 	tristate "MSM8660 Global Clock Controller"
 	depends on COMMON_CLK_QCOM
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 230332c..b7da05b 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -23,6 +23,7 @@  obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
 obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
 obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
 obj-$(CONFIG_MDM_LCC_9615) += lcc-mdm9615.o
+obj-$(CONFIG_MSM_CLK_RPMH) += clk-rpmh.o
 obj-$(CONFIG_MSM_GCC_8660) += gcc-msm8660.o
 obj-$(CONFIG_MSM_GCC_8916) += gcc-msm8916.o
 obj-$(CONFIG_MSM_GCC_8960) += gcc-msm8960.o
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 0000000..763401f
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,394 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <soc/qcom/cmd-db.h>
+#include <soc/qcom/rpmh.h>
+
+#include <dt-bindings/clock/qcom,rpmh.h>
+
+#include "common.h"
+#include "clk-regmap.h"
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+#define CLK_RPMH_VRM_OFF_VAL 0
+#define CLK_RPMH_VRM_ON_VAL 1
+#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+					 BIT(RPMH_ACTIVE_ONLY_STATE))
+#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+				      BIT(RPMH_ACTIVE_ONLY_STATE) | \
+				      BIT(RPMH_SLEEP_STATE))
+struct clk_rpmh {
+	struct clk_hw hw;
+	const char *res_name;
+	u32 res_addr;
+	u32 res_en_offset;
+	u32 res_on_val;
+	u32 res_off_val;
+	u32 state;
+	u32 aggr_state;
+	u32 last_sent_aggr_state;
+	u32 valid_state_mask;
+	struct rpmh_client *rpmh_client;
+	unsigned long rate;
+	struct clk_rpmh *peer;
+};
+
+struct rpmh_cc {
+	struct clk_onecell_data data;
+	struct clk *clks[];
+};
+
+struct clk_rpmh_desc {
+	struct clk_hw **clks;
+	size_t num_clks;
+};
+
+static DEFINE_MUTEX(rpmh_clk_lock);
+
+#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \
+			  _res_en_offset, _res_on, _res_off, _rate, \
+			  _state_mask, _state_on_mask)			      \
+	static struct clk_rpmh _platform##_##_name_active;		      \
+	static struct clk_rpmh _platform##_##_name = {			      \
+		.res_name = _res_name,					      \
+		.res_en_offset = _res_en_offset,			      \
+		.res_on_val = _res_on,					      \
+		.res_off_val = _res_off,				      \
+		.rate = _rate,						      \
+		.peer = &_platform##_##_name_active,			      \
+		.valid_state_mask = _state_mask,			      \
+		.hw.init = &(struct clk_init_data){			      \
+			.ops = &clk_rpmh_ops,			              \
+			.name = #_name,					      \
+		},							      \
+	};								      \
+	static struct clk_rpmh _platform##_##_name_active = {		      \
+		.res_name = _res_name,					      \
+		.res_en_offset = _res_en_offset,			      \
+		.res_on_val = _res_on,					      \
+		.res_off_val = _res_off,				      \
+		.rate = _rate,						      \
+		.peer = &_platform##_##_name,				      \
+		.valid_state_mask = _state_on_mask,			      \
+		.hw.init = &(struct clk_init_data){			      \
+			.ops = &clk_rpmh_ops,				      \
+			.name = #_name_active,				      \
+		},							      \
+	}
+
+#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \
+			    _res_on, _res_off, _rate, _state_mask, \
+			    _state_on_mask)				\
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off, \
+			  _rate, _state_mask, _state_on_mask)
+
+#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name,	\
+			    _rate, _state_mask, _state_on_mask) \
+	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
+			  CLK_RPMH_VRM_EN_OFFSET, CLK_RPMH_VRM_ON_VAL,	\
+			  CLK_RPMH_VRM_OFF_VAL, _rate, _state_mask, \
+			  _state_on_mask)
+
+static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
+{
+	return container_of(_hw, struct clk_rpmh, hw);
+}
+
+static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
+{
+	return ((c->last_sent_aggr_state & BIT(state))
+		!= (c->aggr_state & BIT(state)));
+}
+
+static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
+{
+	struct tcs_cmd cmd = { 0 };
+	int ret = 0;
+
+	cmd.addr = c->res_addr + c->res_en_offset;
+
+	if (has_state_changed(c, RPMH_SLEEP_STATE)) {
+		cmd.data = (c->aggr_state & BIT(RPMH_SLEEP_STATE))
+				? c->res_on_val : c->res_off_val;
+		ret = rpmh_write_async(c->rpmh_client, RPMH_SLEEP_STATE,
+				       &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n",
+			       __func__, c->res_name, RPMH_SLEEP_STATE, ret);
+			return ret;
+		}
+	}
+
+	if (has_state_changed(c, RPMH_WAKE_ONLY_STATE)) {
+		cmd.data = (c->aggr_state & BIT(RPMH_WAKE_ONLY_STATE))
+				? c->res_on_val : c->res_off_val;
+		ret = rpmh_write_async(c->rpmh_client,
+				       RPMH_WAKE_ONLY_STATE, &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write_async(%s, state=%d) failed (%d)\n"
+			       __func__, c->res_name, RPMH_WAKE_ONLY_STATE,
+				 ret);
+			return ret;
+		}
+	}
+
+	if (has_state_changed(c, RPMH_ACTIVE_ONLY_STATE)) {
+		cmd.data = (c->aggr_state & BIT(RPMH_ACTIVE_ONLY_STATE))
+				? c->res_on_val : c->res_off_val;
+		ret = rpmh_write(c->rpmh_client, RPMH_ACTIVE_ONLY_STATE,
+				 &cmd, 1);
+		if (ret) {
+			pr_err("%s: rpmh_write(%s, state=%d) failed (%d)\n",
+			       __func__, c->res_name, RPMH_ACTIVE_ONLY_STATE,
+				 ret);
+			return ret;
+		}
+	}
+
+	c->last_sent_aggr_state = c->aggr_state;
+	c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
+
+	return 0;
+}
+
+static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c,
+						bool enable)
+{
+	/* Update state and aggregate state values based on enable value. */
+	c->state = enable ? c->valid_state_mask : 0;
+	c->aggr_state = c->state | c->peer->state;
+	c->peer->aggr_state = c->aggr_state;
+
+	ret = clk_rpmh_send_aggregate_command(c);
+	if (ret && enable)
+		c->state = 0;
+	else if (ret) {
+		c->state = c->valid_state_mask;
+		WARN(1, "clk: %s failed to disable\n", c->res_name);
+	}
+
+	return ret;
+}
+
+static int clk_rpmh_prepare(struct clk_hw *hw)
+{
+	struct clk_rpmh *c = to_clk_rpmh(hw);
+	int ret = 0;
+
+	mutex_lock(&rpmh_clk_lock);
+
+	if (c->state)
+		goto out;
+
+	ret = clk_rpmh_aggregate_state_send_command(c, true);
+out:
+	mutex_unlock(&rpmh_clk_lock);
+	return ret;
+};
+
+static void clk_rpmh_unprepare(struct clk_hw *hw)
+{
+	struct clk_rpmh *c = to_clk_rpmh(hw);
+
+	mutex_lock(&rpmh_clk_lock);
+
+	if (!c->state)
+		goto out;
+
+	clk_rpmh_aggregate_state_send_command(c, false);
+out:
+	mutex_unlock(&rpmh_clk_lock);
+	return;
+};
+
+static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct clk_rpmh *r = to_clk_rpmh(hw);
+
+	/*
+	 * RPMh clocks have a fixed rate. Return static rate set
+	 * at init time.
+	 */
+	return r->rate;
+}
+
+static const struct clk_ops clk_rpmh_ops = {
+	.prepare	= clk_rpmh_prepare,
+	.unprepare	= clk_rpmh_unprepare,
+	.recalc_rate	= clk_rpmh_recalc_rate,
+};
+
+/* Resource name must match resource id present in cmd-db. */
+DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0,
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2",
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3",
+		    19200000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3",
+		    38400000, CLK_RPMH_APPS_RSC_STATE_MASK,
+		    CLK_RPMH_APPS_RSC_AO_STATE_MASK);
+
+static struct clk_hw *sdm845_rpmh_clocks[] = {
+	[RPMH_CXO_CLK]		= &sdm845_bi_tcxo.hw,
+	[RPMH_CXO_CLK_A]	= &sdm845_bi_tcxo_ao.hw,
+	[RPMH_LN_BB_CLK2]	= &sdm845_ln_bb_clk2.hw,
+	[RPMH_LN_BB_CLK2_A]	= &sdm845_ln_bb_clk2_ao.hw,
+	[RPMH_LN_BB_CLK3]	= &sdm845_ln_bb_clk3.hw,
+	[RPMH_LN_BB_CLK3_A]	= &sdm845_ln_bb_clk3_ao.hw,
+	[RPMH_RF_CLK1]		= &sdm845_rf_clk1.hw,
+	[RPMH_RF_CLK1_A]	= &sdm845_rf_clk1_ao.hw,
+	[RPMH_RF_CLK2]		= &sdm845_rf_clk2.hw,
+	[RPMH_RF_CLK2_A]	= &sdm845_rf_clk2_ao.hw,
+	[RPMH_RF_CLK3]		= &sdm845_rf_clk3.hw,
+	[RPMH_RF_CLK3_A]	= &sdm845_rf_clk3_ao.hw,
+};
+
+static const struct clk_rpmh_desc clk_rpmh_sdm845 = {
+	.clks = sdm845_rpmh_clocks,
+	.num_clks = ARRAY_SIZE(sdm845_rpmh_clocks),
+};
+
+static const struct of_device_id clk_rpmh_match_table[] = {
+	{ .compatible = "qcom,rpmh-clk-sdm845", .data = &clk_rpmh_sdm845},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clk_rpmh_match_table);
+
+static int clk_rpmh_probe(struct platform_device *pdev)
+{
+	struct clk **clks;
+	struct clk *clk;
+	struct rpmh_cc *rcc;
+	struct clk_onecell_data *data;
+	int ret;
+	size_t num_clks, i;
+	struct clk_hw **hw_clks;
+	struct clk_rpmh *rpmh_clk;
+	const struct clk_rpmh_desc *desc;
+	struct property *prop;
+	struct rpmh_client *rpmh_client = NULL;
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	ret = cmd_db_ready();
+	if (ret) {
+		if (ret != -EPROBE_DEFER) {
+			dev_err(&pdev->dev, "Command DB not available (%d)\n",
+				ret);
+			goto err;
+		}
+		return ret;
+	}
+
+	rpmh_client = rpmh_get_client(pdev);
+	if (IS_ERR(rpmh_client)) {
+		ret = PTR_ERR(rpmh_client);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to request RPMh client, ret=%d\n",
+				ret);
+		return ret;
+	}
+
+	hw_clks = desc->clks;
+	num_clks = desc->num_clks;
+
+	rcc = devm_kzalloc(&pdev->dev, sizeof(*rcc) + sizeof(*clks) * num_clks,
+			   GFP_KERNEL);
+	if (!rcc) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	clks = rcc->clks;
+	data = &rcc->data;
+	data->clks = clks;
+	data->clk_num = num_clks;
+
+	for (i = 0; i < num_clks; i++) {
+		rpmh_clk = to_clk_rpmh(hw_clks[i]);
+		rpmh_clk->res_addr = cmd_db_read_addr(rpmh_clk->res_name);
+		if (!rpmh_clk->res_addr) {
+			dev_err(&pdev->dev, "missing RPMh resource address for %s\n",
+				rpmh_clk->res_name);
+			ret = -ENODEV;
+			goto err;
+		}
+
+		rpmh_clk->rpmh_client = rpmh_client;
+
+		clk = devm_clk_register(&pdev->dev, hw_clks[i]);
+		if (IS_ERR(clk)) {
+			ret = PTR_ERR(clk);
+			goto err;
+		}
+
+		clks[i] = clk;
+	}
+
+	ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
+				  data);
+	if (ret)
+		goto err;
+
+	dev_info(&pdev->dev, "Registered RPMh clocks\n");
+	return ret;
+
+err:
+	if (rpmh_client)
+		rpmh_release(rpmh_client);
+
+	dev_err(&pdev->dev, "Error registering RPMh Clock driver (%d)\n", ret);
+	return ret;
+}
+
+static struct platform_driver clk_rpmh_driver = {
+	.probe		= clk_rpmh_probe,
+	.driver		= {
+		.name	= "clk-rpmh",
+		.of_match_table = clk_rpmh_match_table,
+	},
+};
+
+static int __init clk_rpmh_init(void)
+{
+	return platform_driver_register(&clk_rpmh_driver);
+}
+subsys_initcall(clk_rpmh_init);
+
+static void __exit clk_rpmh_exit(void)
+{
+	platform_driver_unregister(&clk_rpmh_driver);
+}
+module_exit(clk_rpmh_exit);
+
+MODULE_DESCRIPTION("QTI RPMh Clock Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:clk-rpmh");
diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h
new file mode 100644
index 0000000..34fbf3c
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,rpmh.h
@@ -0,0 +1,25 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_MSM_RPMH_H
+#define _DT_BINDINGS_CLK_MSM_RPMH_H
+
+/* RPMh controlled clocks */
+#define RPMH_CXO_CLK				0
+#define RPMH_CXO_CLK_A				1
+#define RPMH_LN_BB_CLK2				2
+#define RPMH_LN_BB_CLK2_A			3
+#define RPMH_LN_BB_CLK3				4
+#define RPMH_LN_BB_CLK3_A			5
+#define RPMH_RF_CLK1				6
+#define RPMH_RF_CLK1_A				7
+#define RPMH_RF_CLK2				8
+#define RPMH_RF_CLK2_A				9
+#define RPMH_RF_CLK3				10
+#define RPMH_RF_CLK3_A				11
+#define RPMH_RF_CLK4				12
+#define RPMH_RF_CLK4_A				13
+
+#endif