Re: [PATCH v5 3/8] soc: qcom: rpmpd: Add a Power domain driver to model corners

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Overall looks good to me, just some nitpicks around modules and
includes.

Quoting Rajendra Nayak (2018-12-03 21:21:14)
> The Power domains for corners just pass the performance state set by the
> consumers to the RPM (Remote Power manager) which then takes care
> of setting the appropriate voltage on the corresponding rails to
> meet the performance needs.
> 
> We add all Power domain data needed on msm8996 here. This driver can easily
> be extended by adding data for other qualcomm SoCs as well.
> 

Why is "Power" capitalized in power domain?

> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index f25b54cd6cf8..f1b25fdcf2ad 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>  obj-$(CONFIG_QCOM_APR) += apr.o
>  obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
>  obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
> +obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> new file mode 100644
> index 000000000000..a0b9f122d793
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. */
> +
> +#include <linux/err.h>
> +#include <linux/export.h>

And what is exported?

> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

Is it a module? It's only bool so I don't think so.

> +#include <linux/mutex.h>
> +#include <linux/pm_domain.h>
> +#include <linux/mfd/qcom_rpm.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/qcom/smd-rpm.h>
> +
> +#include <dt-bindings/mfd/qcom-rpm.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>
> +
> +#define domain_to_rpmpd(domain) container_of(domain, struct rpmpd, pd)
> +
> +/* Resource types */
> +#define RPMPD_SMPA 0x61706d73
> +#define RPMPD_LDOA 0x616f646c
> +
> +/* Operation Keys */
> +#define KEY_CORNER             0x6e726f63 /* corn */
> +#define KEY_ENABLE             0x6e657773 /* swen */
> +#define KEY_FLOOR_CORNER       0x636676   /* vfc */
> +
> +#define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id)                \
> +       static struct rpmpd _platform##_##_active;                      \
> +       static struct rpmpd _platform##_##_name = {                     \
> +               .pd = { .name = #_name, },                              \
> +               .peer = &_platform##_##_active,                         \
> +               .res_type = RPMPD_SMPA,                                 \
> +               .res_id = r_id,                                         \
> +               .key = KEY_CORNER,                                      \
> +       };                                                              \
> +       static struct rpmpd _platform##_##_active = {                   \
> +               .pd = { .name = #_active, },                            \
> +               .peer = &_platform##_##_name,                           \
> +               .active_only = true,                                    \
> +               .res_type = RPMPD_SMPA,                                 \
> +               .res_id = r_id,                                         \
> +               .key = KEY_CORNER,                                      \
> +       }
> +
> +#define DEFINE_RPMPD_CORN_LDOA(_platform, _name, r_id)                 \
> +       static struct rpmpd _platform##_##_name = {                     \
> +               .pd = { .name = #_name, },                              \
> +               .res_type = RPMPD_LDOA,                                 \
> +               .res_id = r_id,                                         \
> +               .key = KEY_CORNER,                                      \
> +       }
> +
> +#define DEFINE_RPMPD_VFC(_platform, _name, r_id, r_type)               \
> +       static struct rpmpd _platform##_##_name = {                     \
> +               .pd = { .name = #_name, },                              \
> +               .res_type = r_type,                                     \
> +               .res_id = r_id,                                         \
> +               .key = KEY_FLOOR_CORNER,                                \
> +       }
> +
> +#define DEFINE_RPMPD_VFC_SMPA(_platform, _name, r_id)                  \
> +       DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_SMPA)
> +
> +#define DEFINE_RPMPD_VFC_LDOA(_platform, _name, r_id)                  \
> +       DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_LDOA)
> +
> +struct rpmpd_req {
> +       __le32 key;
> +       __le32 nbytes;
> +       __le32 value;
> +};
> +
> +struct rpmpd {
> +       struct generic_pm_domain pd;
> +       struct rpmpd *peer;
> +       const bool active_only;
> +       unsigned int corner;
> +       bool enabled;
> +       const char *res_name;
> +       const int res_type;
> +       const int res_id;
> +       struct qcom_smd_rpm *rpm;
> +       __le32 key;
> +};
> +
> +struct rpmpd_desc {
> +       struct rpmpd **rpmpds;
> +       size_t num_pds;
> +};
> +
> +static DEFINE_MUTEX(rpmpd_lock);
> +
> +/* msm8996 RPM Power domains */
> +DEFINE_RPMPD_CORN_SMPA(msm8996, vddcx, vddcx_ao, 1);
> +DEFINE_RPMPD_CORN_SMPA(msm8996, vddmx, vddmx_ao, 2);
> +DEFINE_RPMPD_CORN_LDOA(msm8996, vddsscx, 26);

Mmm.. CORN...

> +
> +DEFINE_RPMPD_VFC_SMPA(msm8996, vddcx_vfc, 1);
> +DEFINE_RPMPD_VFC_LDOA(msm8996, vddsscx_vfc, 26);
> +
> +static struct rpmpd *msm8996_rpmpds[] = {
> +       [MSM8996_VDDCX] =       &msm8996_vddcx,
> +       [MSM8996_VDDCX_AO] =    &msm8996_vddcx_ao,
> +       [MSM8996_VDDCX_VFC] =   &msm8996_vddcx_vfc,
> +       [MSM8996_VDDMX] =       &msm8996_vddmx,
> +       [MSM8996_VDDMX_AO] =    &msm8996_vddmx_ao,
> +       [MSM8996_VDDSSCX] =     &msm8996_vddsscx,
> +       [MSM8996_VDDSSCX_VFC] = &msm8996_vddsscx_vfc,
> +};
> +
[...]
> +       }
> +
> +       return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
> +}
> +
> +static struct platform_driver rpmpd_driver = {
> +       .driver = {
> +               .name = "qcom-rpmpd",
> +               .of_match_table = rpmpd_match_table,

suppress bind attributes here?

> +       },
> +       .probe = rpmpd_probe,

Because there isn't a remove function to tear down the genpds.

> +};
> +
> +static int __init rpmpd_init(void)
> +{
> +       return platform_driver_register(&rpmpd_driver);
> +}
> +core_initcall(rpmpd_init);
> +
> +static void __exit rpmpd_exit(void)
> +{
> +       platform_driver_unregister(&rpmpd_driver);
> +}
> +module_exit(rpmpd_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPM Power Domain Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-rpmpd");

Is this MODULE_ALIAS required? I thought this wasn't useful because it's
auto-generated or something like that. Also, this is bool so these can
all go away unless it becomes tristate.





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux