Hello Rajendra,
On 06/11/2018 09:40 PM, Rajendra Nayak wrote:
> The RPMh Power domain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
>
> We also add data for all power domains on sdm845 SoC as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
>
> Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/Kconfig | 9 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/rpmhpd.c | 427 ++++++++++++++++++++++++
> include/dt-bindings/power/qcom-rpmhpd.h | 31 ++
> 4 files changed, 468 insertions(+)
> create mode 100644 drivers/soc/qcom/rpmhpd.c
> create mode 100644 include/dt-bindings/power/qcom-rpmhpd.h
This DT header file should be included in a DT binding patch that is
separate from the driver patch.
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 5c54931a7b99..7cb7eba2b997 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>
> Say y here if you intend to boot the modem remoteproc.
>
> +config QCOM_RPMHPD
> + tristate "Qualcomm RPMh Power domain driver"
> + depends on QCOM_RPMH && QCOM_COMMAND_DB
> + help
> + QCOM RPMh Power domain driver to support power-domains with
> + performance states. The driver communicates a performance state
> + value to RPMh which then translates it into corresponding voltage
> + for the voltage rail.
> +
> config QCOM_RPMPD
> tristate "Qualcomm RPM Power domain driver"
> depends on MFD_QCOM_RPM && QCOM_SMD_RPM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 9550c170de93..499513f63bef 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM) += smsm.o
> obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> obj-$(CONFIG_QCOM_APR) += apr.o
> obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> new file mode 100644
> index 000000000000..7083ec1590ff
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -0,0 +1,427 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.*/
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <soc/qcom/cmd-db.h>
> +#include <soc/qcom/rpmh.h>
> +#include <dt-bindings/power/qcom-rpmhpd.h>
> +
> +#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
> +
> +#define DEFINE_RPMHPD_AO(_platform, _name, _active) \
> + static struct rpmhpd _platform##_##_active; \
> + static struct rpmhpd _platform##_##_name = { \
> + .pd = { .name = #_name, }, \
> + .peer = &_platform##_##_active, \
> + .res_name = #_name".lvl", \
> + .valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) | \
> + BIT(RPMH_WAKE_ONLY_STATE) | \
> + BIT(RPMH_SLEEP_STATE)), \
> + }; \
> + static struct rpmhpd _platform##_##_active = { \
> + .pd = { .name = #_active, }, \
> + .peer = &_platform##_##_name, \
> + .active_only = true, \
> + .res_name = #_name".lvl", \
> + .valid_state_mask = (BIT(RPMH_ACTIVE_ONLY_STATE) | \
> + BIT(RPMH_WAKE_ONLY_STATE) | \
> + BIT(RPMH_SLEEP_STATE)), \> + }
> +
> +#define DEFINE_RPMHPD(_platform, _name) \
> + static struct rpmhpd _platform##_##_name = { \
> + .pd = { .name = #_name, }, \
> + .res_name = #_name".lvl", \
> + .valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE), \
> + }
> +
> +/*
> + * This is the number of bytes used for each command DB aux data entry of an
> + * ARC resource.
> + */
> +#define RPMH_ARC_LEVEL_SIZE 2
> +#define RPMH_ARC_MAX_LEVELS 16
> +
Would you mind adding a kernel-doc comment for here for struct rpmhpd? I
think that would make the code clearer. It would be good to mention the
numbering spaces for 'corner' and 'level' elements as well as the usage of
'peer' and 'active_only' elements.
> +struct rpmhpd {
> + struct device *dev;
> + struct generic_pm_domain pd;
> + struct rpmhpd *peer;
> + const bool active_only;
> + unsigned int corner;
> + unsigned int active_corner> + u32 level[RPMH_ARC_MAX_LEVELS];
> + int level_count;
> + bool enabled;
> + const char *res_name;
> + u32 addr;
> + u8 valid_state_mask;
> +};
> +
> +struct rpmhpd_desc {
> + struct rpmhpd **rpmhpds;
> + size_t num_pds;
> +};
> +
> +static DEFINE_MUTEX(rpmhpd_lock);
> +
> +/* sdm845 RPMh Power domains */
> +DEFINE_RPMHPD(sdm845, ebi);
> +DEFINE_RPMHPD_AO(sdm845, mx, mx_ao);
> +DEFINE_RPMHPD_AO(sdm845, cx, cx_ao);
> +DEFINE_RPMHPD(sdm845, lmx);
> +DEFINE_RPMHPD(sdm845, lcx);
> +DEFINE_RPMHPD(sdm845, gfx);
> +DEFINE_RPMHPD(sdm845, mss);
> +
> +static struct rpmhpd *sdm845_rpmhpds[] = {
> + [SDM845_EBI] = &sdm845_ebi,
> + [SDM845_MX] = &sdm845_mx,
> + [SDM845_MX_AO] = &sdm845_mx_ao,
> + [SDM845_CX] = &sdm845_cx,
> + [SDM845_CX_AO] = &sdm845_cx_ao,
> + [SDM845_LMX] = &sdm845_lmx,
> + [SDM845_LCX] = &sdm845_lcx,
> + [SDM845_GFX] = &sdm845_gfx,
> + [SDM845_MSS] = &sdm845_mss,
> +};
> +
> +static const struct rpmhpd_desc sdm845_desc = {
> + .rpmhpds = sdm845_rpmhpds,
> + .num_pds = ARRAY_SIZE(sdm845_rpmhpds),
> +};
> +
> +static const struct of_device_id rpmhpd_match_table[] = {
> + { .compatible = "qcom,sdm845-rpmhpd", .data = &sdm845_desc },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
> +
> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state,
> + unsigned int corner, bool sync)
> +{
> + struct tcs_cmd cmd = {
> + .addr = pd->addr,
> + .data = corner,
> + };
> +
> + if (sync)
> + return rpmh_write(pd->dev, state, &cmd, 1);
> + else
> + return rpmh_write_async(pd->dev, state, &cmd, 1);
> +}
> +
> +static int rpmhpd_send_corner_sync(struct rpmhpd *pd, int state,
> + unsigned int corner)
> +{
> + return rpmhpd_send_corner(pd, state, corner, true);
> +}
> +
> +static int rpmhpd_send_corner_async(struct rpmhpd *pd, int state,
> + unsigned int corner)
> +{
> + return rpmhpd_send_corner(pd, state, corner, false);
> +};
I'm not sure about the need for rpmhpd_send_corner_sync() and
rpmhpd_send_corner_async(). They are adding lines that aren't strictly
needed since rpmhpd_send_corner() could be called directly instead. Doing
that could actually save some more lines in rpmhpd_aggregate_corner()
below as 'active_corner > pd->active_corner' could be passed as the 'sync'
argument so that the if statement isn't needed. However, I also see the
utility in not having a magic bool in the calls below. Let's see if other
reviewers have a preference about it one way or the other.
> +static void to_active_sleep(struct rpmhpd *pd, unsigned int corner,
> + unsigned int *active, unsigned int *sleep)
> +{
> + *active = corner;
> +
> + if (pd->active_only)
> + *sleep = 0;
> + else
> + *sleep = *active;
> +}
> +
> +/*
> + * This function is used to aggregate the votes across the active only
> + * resources and its peers. The aggregated votes are send to RPMh as
> + * ACTIVE_ONLY votes (which take effect immediately), as WAKE_ONLY votes
> + * (applied by RPMh on system wakeup) and as SLEEP votes (applied by RPMh
> + * on system sleep).
> + * We send ACTIVE_ONLY votes for resources without any peers. For others,
> + * which have an active only peer, all 3 Votes are sent.
> + */
> +static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner)
> +{
> + int ret = -EINVAL;
> + struct rpmhpd *peer = pd->peer;
> + unsigned int active_corner, sleep_corner;
> + unsigned int this_active_corner = 0, this_sleep_corner = 0;
> + unsigned int peer_active_corner = 0, peer_sleep_corner = 0;
> +
> + to_active_sleep(pd, corner, &this_active_corner, &this_sleep_corner);
> +
> + if (peer && peer->enabled)
> + to_active_sleep(peer, peer->corner, &peer_active_corner,
> + &peer_sleep_corner);
> +
> + active_corner = max(this_active_corner, peer_active_corner);
> +
> + if (pd->valid_state_mask & BIT(RPMH_ACTIVE_ONLY_STATE)) {
This condition will always be true, so this check can be removed.
> + /*
> + * Wait for an ack only when we are increasing the
> + * perf state of the power domain
> + */
> + if (active_corner > pd->active_corner)
> + ret = rpmhpd_send_corner_sync(pd,
> + RPMH_ACTIVE_ONLY_STATE,
> + active_corner);
> + else
> + ret = rpmhpd_send_corner_async(pd,
> + RPMH_ACTIVE_ONLY_STATE,
> + active_corner);
> + if (ret)
> + return ret;
> + pd->active_corner = active_corner;
> + if (peer)
> + peer->active_corner = active_corner;
> + }
> +
> + if (pd->valid_state_mask & BIT(RPMH_WAKE_ONLY_STATE)) {
This check and the one below could be changed to simply:
if (pd->peer) {
That way, the valid_state_mask element can be removed from struct rpmhpd
and the two if blocks can be consolidated together. I think that
valid_state_mask is making the code more confusing at this point than it
is at verbosely describing the aggregation semantics.
> + ret = rpmhpd_send_corner_async(pd, RPMH_WAKE_ONLY_STATE,
> + active_corner);
> + if (ret)
> + return ret;
> + }
> +
> + sleep_corner = max(this_sleep_corner, peer_sleep_corner);
> +
> + if (pd->valid_state_mask & BIT(RPMH_SLEEP_STATE))
> + ret = rpmhpd_send_corner_async(pd, RPMH_SLEEP_STATE,
> + sleep_corner);
> +
> + return ret;
> +}
> +
> +static int rpmhpd_power_on(struct generic_pm_domain *domain)
> +{
> + struct rpmhpd *pd = domain_to_rpmhpd(domain);
> + int ret = 0;
> +
> + mutex_lock(&rpmhpd_lock);
> +
> + pd->enabled = true;
It would probably be better to remove this line and add the following
after the rpmhpd_aggregate_corner() call:
if (!ret)
pd->enabled = true;
Only the peer 'enabled' value is checked in rpmhpd_aggregate_corner() so
architecturally, it doesn't matter if the value is configured before or
after the call.
> +
> + if (pd->corner)
> + ret = rpmhpd_aggregate_corner(pd, pd->corner);
> +
> + mutex_unlock(&rpmhpd_lock);
> +
> + return ret;
> +}
> +
> +static int rpmhpd_power_off(struct generic_pm_domain *domain)
> +{
> + struct rpmhpd *pd = domain_to_rpmhpd(domain);
> + int ret = 0;
> +
> + mutex_lock(&rpmhpd_lock);
> +
> + if (pd->level[0] == 0)
> + ret = rpmhpd_aggregate_corner(pd, 0);
I'm not sure that we want to have the 'pd->level[0] == 0' check,
especially when considering aggregation with the peer pd. I understand
its intention to try to keep enable state and level setting orthogonal.
However, as it stands now, the final request sent to hardware would differ
depending upon the order of calls. Consider the following example.
Initial state:
pd->level[0] == 0
pd->corner = 5, pd->enabled = true, pd->active_only = false
pd->peer->corner = 7, pd->peer->enabled = true, pd->peer->active_only = true
Outstanding requests:
RPMH_ACTIVE_ONLY_STATE = 7, RPMH_WAKE_ONLY_STATE = 7, RPMH_SLEEP_STATE = 5
Case A:
1. set pd->corner = 6
--> new value request: RPMH_SLEEP_STATE = 6
--> duplicate value requests: RPMH_ACTIVE_ONLY_STATE = 7,
RPMH_WAKE_ONLY_STATE = 7
2. power_off pd->peer
--> no requests
Final state:
RPMH_ACTIVE_ONLY_STATE = 7
RPMH_WAKE_ONLY_STATE = 7
RPMH_SLEEP_STATE = 6
Case B:
1. power_off pd->peer
--> no requests
2. set pd->corner = 6
--> new value requests: RPMH_ACTIVE_ONLY_STATE = 6,
RPMH_WAKE_ONLY_STATE = 6, RPMH_SLEEP_STATE = 6
Final state:
RPMH_ACTIVE_ONLY_STATE = 6
RPMH_WAKE_ONLY_STATE = 6
RPMH_SLEEP_STATE = 6
Without the check, Linux would vote for the lowest supported level when
power_off is called. This seems semantically reasonable given that the
consumer is ok with the power domain going fully off and that would be the
closest that we can get.
> +
> + if (!ret)
> + pd->enabled = false;
> +
> + mutex_unlock(&rpmhpd_lock);
> +
> + return ret;
> +}
> +
> +static int rpmhpd_set_performance(struct generic_pm_domain *domain,
> + unsigned int state)
The code might be a bit more readable if 'state' is changed to 'level'.
Also, is there a particular reason that this is named
rpmhpd_set_performance() instead of rpmhpd_set_performance_state()?
> +{
> + struct rpmhpd *pd = domain_to_rpmhpd(domain);
> + int ret = 0, i;
> +
> + mutex_lock(&rpmhpd_lock);
> +
> + for (i = 0; i < pd->level_count; i++)
> + if (state <= pd->level[i])
> + break;
> +
> + if (i == pd->level_count) {
> + ret = -EINVAL;
> + dev_err(pd->dev, "invalid state=%u for domain %s",
> + state, pd->pd.name);
> + goto out;
One level of indentation should be removed from this line.
> + }
> +
> + pd->corner = i;
> +
> + if (!pd->enabled)
> + goto out;
> +
> + ret = rpmhpd_aggregate_corner(pd, i);
Would it be worthwhile to roll back the pd->corner value in the case of an
error?
> +out:
> + mutex_unlock(&rpmhpd_lock);
> +
> + return ret;
> +}
> +
> +static unsigned int rpmhpd_get_performance(struct generic_pm_domain *genpd,
> + struct dev_pm_opp *opp)
Is there a particular reason that this is named rpmhpd_get_performance()
instead of rpmhpd_get_performance_state()?
> +{
> + struct device_node *np;
> + unsigned int corner = 0;
Please change 'corner' to 'level' for consistency. In this driver "level"
values are in the vlvl RPMH_REGULATOR_LEVEL_* numbering space and "corner"
values are in the hlvl 0-15 numbering space.
> +
> + np = dev_pm_opp_get_of_node(opp);
> + if (of_property_read_u32(np, "qcom,level", &corner)) {
> + pr_err("%s: missing 'qcom,level' property\n", __func__);
> + return 0;
> + }
> +
> + of_node_put(np);
> +
> + return corner;
> +}
> +
> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
> +{
> + int i, j, len, ret;
> + u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];
Minor: It might look better to list buf[] first.
> +
> + len = cmd_db_read_aux_data_len(rpmhpd->res_name);
> + if (len <= 0)
> + return len;
> +
> + if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
> + return -EINVAL;
'else if' could be used here.
> +
> + ret = cmd_db_read_aux_data(rpmhpd->res_name, buf, len);
> + if (ret < 0)
> + return ret;
> +
> + rpmhpd->level_count = len / RPMH_ARC_LEVEL_SIZE;
> +
> + for (i = 0; i < rpmhpd->level_count; i++) {
> + rpmhpd->level[i] = 0;
> + for (j = 0; j < RPMH_ARC_LEVEL_SIZE; j++)
> + rpmhpd->level[i] |=
> + buf[i * RPMH_ARC_LEVEL_SIZE + j] << (8 * j);
> +
> + /*
> + * The AUX data may be zero padded. These 0 valued entries at
> + * the end of the map must be ignored.
> + */
> + if (i > 0 && rpmhpd->level[i] == 0) {
> + rpmhpd->level_count = i;
> + break;
> + }
> + pr_dbg("%s: ARC hlvl=%2d --> vlvl=%4u\n", rpmhpd->res_name, i,
> + rpmhpd->level[i]);
> + }
> +
> + return 0;
> +}
> +
> +static int rpmhpd_probe(struct platform_device *pdev)
> +{
> + int i, ret;
> + size_t num;
> + struct genpd_onecell_data *data;
> + struct rpmhpd **rpmhpds;
> + const struct rpmhpd_desc *desc;
> +
> + desc = of_device_get_match_data(&pdev->dev);
> + if (!desc)
> + return -EINVAL;
> +
> + rpmhpds = desc->rpmhpds;
> + num = desc->num_pds;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
> + GFP_KERNEL);
> + data->num_domains = num;
> +
> + ret = cmd_db_ready();
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Command DB unavailable, ret=%d\n",
> + ret);
> + return ret;
> + }
> +
> + for (i = 0; i < num; i++) {
> + if (!rpmhpds[i]) {
> + dev_warn(&pdev->dev, "rpmhpds[] with empty entry at index=%d\n",
> + i);
Minor: This could be simplified to:
dev_warn(&pdev->dev, "rpmhpds[%d] is empty\n", i);
> + continue;
> + }
> +
> + rpmhpds[i]->dev = &pdev->dev;
> + rpmhpds[i]->addr = cmd_db_read_addr(rpmhpds[i]->res_name);
> + if (!rpmhpds[i]->addr) {
> + dev_err(&pdev->dev, "Could not find RPMh address for resource %s\n",
> + rpmhpds[i]->res_name);
> + return -ENODEV;
> + }
> +
> + ret = cmd_db_read_slave_id(rpmhpds[i]->res_name);
> + if (ret != CMD_DB_HW_ARC) {
> + dev_err(&pdev->dev, "RPMh slave ID mismatch\n");
> + return -EINVAL;
> + }
> +
> + ret = rpmhpd_update_level_mapping(rpmhpds[i]);
> + if (ret)
> + return ret;
> +
> + rpmhpds[i]->pd.power_off = rpmhpd_power_off;
> + rpmhpds[i]->pd.power_on = rpmhpd_power_on;
> + rpmhpds[i]->pd.set_performance_state = rpmhpd_set_performance;
> + rpmhpds[i]->pd.opp_to_performance_state = rpmhpd_get_performance;
> + pm_genpd_init(&rpmhpds[i]->pd, NULL, true);
> +
> + data->domains[i] = &rpmhpds[i]->pd;
> + }
> +
> + return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> +}
> +
> +static int rpmhpd_remove(struct platform_device *pdev)
> +{
> + of_genpd_del_provider(pdev->dev.of_node);
> + return 0;
> +}
> +
> +static struct platform_driver rpmhpd_driver = {
> + .driver = {
> + .name = "qcom-rpmhpd",
> + .of_match_table = rpmhpd_match_table,
> + },
> + .probe = rpmhpd_probe,
> + .remove = rpmhpd_remove,
> +};
> +
> +static int __init rpmhpd_init(void)
> +{
> + return platform_driver_register(&rpmhpd_driver);
> +}
> +core_initcall(rpmhpd_init);
> +
> +static void __exit rpmhpd_exit(void)
> +{
> + platform_driver_unregister(&rpmhpd_driver);
> +}
> +module_exit(rpmhpd_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Power Domain Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-rpmhpd");
> diff --git a/include/dt-bindings/power/qcom-rpmhpd.h b/include/dt-bindings/power/qcom-rpmhpd.h
> new file mode 100644
> index 000000000000..b01ae2452603
> --- /dev/null
> +++ b/include/dt-bindings/power/qcom-rpmhpd.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
> +
> +#ifndef _DT_BINDINGS_POWER_QCOM_RPMHPD_H
> +#define _DT_BINDINGS_POWER_QCOM_RPMHPD_H
> +
> +/* SDM845 Power Domain Indexes */
> +#define SDM845_EBI 0
> +#define SDM845_MX 1
> +#define SDM845_MX_AO 2
> +#define SDM845_CX 3
> +#define SDM845_CX_AO 4
> +#define SDM845_LMX 5
> +#define SDM845_LCX 6
> +#define SDM845_GFX 7
> +#define SDM845_MSS 8
> +
> +/* SDM845 Power Domain performance levels */
> +#define RPMH_REGULATOR_LEVEL_OFF 0
Do you really want to specify 0 as a performance level? This would allow
an OFF request to be sent by setting the performance state and without
disabling the power domain. I would suggest removing it.
It will also lead to rpmhpd_get_performance() returning 0 in a non-error case.
> +#define RPMH_REGULATOR_LEVEL_RETENTION 16
> +#define RPMH_REGULATOR_LEVEL_MIN_SVS 48
> +#define RPMH_REGULATOR_LEVEL_LOW_SVS 64
> +#define RPMH_REGULATOR_LEVEL_SVS 128
> +#define RPMH_REGULATOR_LEVEL_SVS_L1 192
> +#define RPMH_REGULATOR_LEVEL_NOM 256
> +#define RPMH_REGULATOR_LEVEL_NOM_L1 320
> +#define RPMH_REGULATOR_LEVEL_NOM_L2 336
> +#define RPMH_REGULATOR_LEVEL_TURBO 384
> +#define RPMH_REGULATOR_LEVEL_TURBO_L1 416
> +
> +#endif
Thanks,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
