Hi Sibi, Thank you for refreshing this patchset! On 2/9/20 20:34, Sibi Sankar wrote: > From: David Dai <daidavid1@xxxxxxxxxxxxxx> > > Add bcm voter driver and add support for RPMh specific interconnect > providers which implements the set and aggregate functionalities that > translates bandwidth requests into RPMh messages. These modules provide > a common set of functionalities for all Qualcomm RPMh based interconnect > providers and should help reduce code duplication when adding new > providers. > > Signed-off-by: David Dai <daidavid1@xxxxxxxxxxxxxx> > Signed-off-by: Odelu Kukatla <okukatla@xxxxxxxxxxxxxx> > Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx> > --- > drivers/interconnect/qcom/Kconfig | 13 +- > drivers/interconnect/qcom/Makefile | 4 + > drivers/interconnect/qcom/bcm-voter.c | 366 ++++++++++++++++++++++++++ > drivers/interconnect/qcom/bcm-voter.h | 27 ++ > drivers/interconnect/qcom/icc-rpmh.c | 147 +++++++++++ > drivers/interconnect/qcom/icc-rpmh.h | 149 +++++++++++ > 6 files changed, 705 insertions(+), 1 deletion(-) > create mode 100644 drivers/interconnect/qcom/bcm-voter.c > create mode 100644 drivers/interconnect/qcom/bcm-voter.h > create mode 100644 drivers/interconnect/qcom/icc-rpmh.c > create mode 100644 drivers/interconnect/qcom/icc-rpmh.h > > diff --git a/drivers/interconnect/qcom/Kconfig b/drivers/interconnect/qcom/Kconfig > index 76938ece1658e..08cfd676b4299 100644 > --- a/drivers/interconnect/qcom/Kconfig > +++ b/drivers/interconnect/qcom/Kconfig > @@ -5,6 +5,11 @@ config INTERCONNECT_QCOM > help > Support for Qualcomm's Network-on-Chip interconnect hardware. > > +config INTERCONNECT_QCOM_BCM_VOTER > + tristate > + depends on INTERCONNECT_QCOM || COMPILE_TEST > + depends on QCOM_RPMH && OF This is not user-selectable, but has a "depends on" lines... > + > config INTERCONNECT_QCOM_MSM8916 > tristate "Qualcomm MSM8916 interconnect driver" > depends on INTERCONNECT_QCOM > @@ -32,10 +37,16 @@ config INTERCONNECT_QCOM_QCS404 > This is a driver for the Qualcomm Network-on-Chip on qcs404-based > platforms. > > +config INTERCONNECT_QCOM_RPMH > + tristate > + depends on INTERCONNECT_QCOM || COMPILE_TEST > + depends on QCOM_COMMAND_DB > + > config INTERCONNECT_QCOM_SDM845 > tristate "Qualcomm SDM845 interconnect driver" > depends on INTERCONNECT_QCOM > - depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST > + select INTERCONNECT_QCOM_RPMH > + select INTERCONNECT_QCOM_BCM_VOTER ...and these selects will force a symbol to a value without checking its dependencies. So i was wondering whether this will cause any problems. > help > This is a driver for the Qualcomm Network-on-Chip on sdm845-based > platforms. > diff --git a/drivers/interconnect/qcom/Makefile b/drivers/interconnect/qcom/Makefile > index e8271575e3d8a..d591bb56273b1 100644 [..] > +static const struct of_device_id bcm_voter_of_match[] = { > + { .compatible = "qcom,bcm-voter" }, > + { } > +}; > + > +static struct platform_driver qcom_icc_bcm_voter_driver = { > + .probe = qcom_icc_bcm_voter_probe, > + .driver = { > + .name = "sdm845_bcm_voter", Nit: This is not just about sdm845 anymore, so maybe qcom_bcm_voter. > + .of_match_table = bcm_voter_of_match, > + }, > +}; > +module_platform_driver(qcom_icc_bcm_voter_driver); > + > +MODULE_AUTHOR("David Dai <daidavid1@xxxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Qualcomm BCM Voter interconnect driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/interconnect/qcom/bcm-voter.h b/drivers/interconnect/qcom/bcm-voter.h > new file mode 100644 > index 0000000000000..3436673e9f104 > --- /dev/null [.] > diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c > new file mode 100644 > index 0000000000000..e9443b50e0b48 > --- /dev/null > +++ b/drivers/interconnect/qcom/icc-rpmh.c > @@ -0,0 +1,147 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2020, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/interconnect.h> > +#include <linux/interconnect-provider.h> > + > +#include "bcm-voter.h" > +#include "icc-rpmh.h" > + > +/** > + * qcom_icc_pre_aggregate - cleans up stale values from prior icc_set > + * @node: icc node to operate on > + */ > +void qcom_icc_pre_aggregate(struct icc_node *node) > +{ > + size_t i; > + struct qcom_icc_node *qn; > + > + qn = node->data; > + > + for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) { > + qn->sum_avg[i] = 0; > + qn->max_peak[i] = 0; > + } > +} > +EXPORT_SYMBOL_GPL(qcom_icc_pre_aggregate); > + > +/** > + * qcom_icc_aggregate - aggregate bw for buckets indicated by tag > + * @node: node to aggregate > + * @tag: tag to indicate which buckets to aggregate > + * @avg_bw: new bw to sum aggregate > + * @peak_bw: new bw to max aggregate > + * @agg_avg: existing aggregate avg bw val > + * @agg_peak: existing aggregate peak bw val > + */ > +int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw, > + u32 peak_bw, u32 *agg_avg, u32 *agg_peak) > +{ > + size_t i; > + struct qcom_icc_node *qn; > + struct qcom_icc_provider *qp; > + > + qn = node->data; > + qp = to_qcom_provider(node->provider); > + > + if (!tag) > + tag = QCOM_ICC_TAG_ALWAYS; > + > + for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) { > + if (tag & BIT(i)) { > + qn->sum_avg[i] += avg_bw; > + qn->max_peak[i] = max_t(u32, qn->max_peak[i], peak_bw); > + } > + } > + > + *agg_avg += avg_bw; > + *agg_peak = max_t(u32, *agg_peak, peak_bw); > + > + for (i = 0; i < qn->num_bcms; i++) > + qcom_icc_bcm_voter_add(qp->voter, qn->bcms[i]); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(qcom_icc_aggregate); > + > +/** > + * qcom_icc_set - set the constraints based on path > + * @src: source node for the path to set constraints on > + * @dst: destination node for the path to set constraints on > + * > + * Return: 0 on success, or an error code otherwise > + */ > +int qcom_icc_set(struct icc_node *src, struct icc_node *dst) > +{ > + struct qcom_icc_provider *qp; > + struct icc_node *node; > + > + if (!src) > + node = dst; > + else > + node = src; > + > + qp = to_qcom_provider(node->provider); > + > + qcom_icc_bcm_voter_commit(qp->voter); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(qcom_icc_set); > + > +/** > + * qcom_icc_bcm_init - populates bcm aux data and connect qnodes > + * @bcm: bcm to be initialized > + * @dev: associated provider device > + * > + * Return: 0 on success, or an error code otherwise > + */ > +int qcom_icc_bcm_init(struct qcom_icc_bcm *bcm, struct device *dev) > +{ > + struct qcom_icc_node *qn; > + const struct bcm_db *data; > + size_t data_count; > + int i; > + > + /* BCM is already initialised*/ > + if (bcm->addr) > + return 0; > + > + bcm->addr = cmd_db_read_addr(bcm->name); > + if (!bcm->addr) { > + dev_err(dev, "%s could not find RPMh address\n", > + bcm->name); > + return -EINVAL; > + } > + > + data = cmd_db_read_aux_data(bcm->name, &data_count); > + if (IS_ERR(data)) { > + dev_err(dev, "%s command db read error (%ld)\n", > + bcm->name, PTR_ERR(data)); > + return PTR_ERR(data); > + } > + if (!data_count) { > + dev_err(dev, "%s command db missing or partial aux data\n", > + bcm->name); > + return -EINVAL; > + } > + > + bcm->aux_data.unit = le32_to_cpu(data->unit); > + bcm->aux_data.width = le16_to_cpu(data->width); > + bcm->aux_data.vcd = data->vcd; > + bcm->aux_data.reserved = data->reserved; > + INIT_LIST_HEAD(&bcm->list); > + INIT_LIST_HEAD(&bcm->ws_list); > + > + /* Link Qnodes to their respective BCMs */ > + for (i = 0; i < bcm->num_nodes; i++) { > + qn = bcm->nodes[i]; > + qn->bcms[qn->num_bcms] = bcm; > + qn->num_bcms++; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(qcom_icc_bcm_init); When i tried building this as a module i see the following warning: WARNING: modpost: missing MODULE_LICENSE() in drivers/interconnect/qcom/icc-rpmh.o So we may want to include module.h and add MODULE_LICENSE("GPL v2") as specified in the SPDX tag. Thanks, Georgi