Re: [PATCH v3 6/7] soc: qcom: Add RPMh Power domain driver

Matthias Kaehlcke
2018-06-12T19:42:31Z

Hi,

On Tue, Jun 12, 2018 at 10:10:51AM +0530, 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@codeaurora.org>
> ---
> 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
>
> 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), \
> + }

Perhaps describe briefly the concept of an AO (active-only) and a peer
domain. It is not necessarily evident why an AO domain would have
WAKE_ONLY and SLEEP_ONLY as valid states, while the non-AO domain has
ACTIVE_ONLY as it's only state.

> +/*
> + * This function is used to aggregate the votes across the active only
> + * resources and its peers. The aggregated votes are send to RPMh as

s/send/sent/

> + * 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.

s/Votes/votes/

> +static int rpmhpd_probe(struct platform_device *pdev)
> +{
> + int i, ret;
> + size_t num;

nit: naming this num_pds would slightly improve readability (e.g. make
it evident that the for loop iterates over the PDs).

> + 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);

Check for NULL?

Thanks

Matthias