[1/4] interconnect: qcom: icc-rpmh: Add QoS config support

[1/4] interconnect: qcom: icc-rpmh: Add QoS config support

[Odelu Kukatla]

Introduce support to initialize QoS settings for QNOC platforms.

Change-Id: I068d49cbcfec5d34c01e5adc930eec72d306ed89
Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
---
 drivers/interconnect/qcom/icc-rpmh.c | 158 +++++++++++++++++++++++++++
 drivers/interconnect/qcom/icc-rpmh.h |  33 ++++++
 2 files changed, 191 insertions(+)

diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index c1aa265c1f4e..49334065ccfa 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
+#include <linux/clk.h>
 #include <linux/interconnect.h>
 #include <linux/interconnect-provider.h>
 #include <linux/module.h>
@@ -14,6 +16,37 @@
 #include "icc-common.h"
 #include "icc-rpmh.h"
 
+/* QNOC QoS */
+#define QOSGEN_MAINCTL_LO(p, qp)	(0x8 + (p->offsets[qp]))
+#define QOS_SLV_URG_MSG_EN_SHFT		3
+#define QOS_DFLT_PRIO_MASK		0x7
+#define QOS_DFLT_PRIO_SHFT		4
+#define QOS_DISABLE_SHIFT		24
+
+/**
+ * qcom_icc_set_qos - initialize static QoS configurations
+ * @node: qcom icc node to operate on
+ */
+static void qcom_icc_set_qos(struct qcom_icc_node *node)
+{
+	struct qcom_icc_qosbox *qos = node->qosbox;
+	int port;
+
+	for (port = 0; port < qos->num_ports; port++) {
+		regmap_update_bits(node->regmap, QOSGEN_MAINCTL_LO(qos, port),
+				   BIT(QOS_DISABLE_SHIFT),
+				   qos->prio_fwd_disable << QOS_DISABLE_SHIFT);
+
+		regmap_update_bits(node->regmap, QOSGEN_MAINCTL_LO(qos, port),
+				   QOS_DFLT_PRIO_MASK << QOS_DFLT_PRIO_SHFT,
+				   qos->prio << QOS_DFLT_PRIO_SHFT);
+
+		regmap_update_bits(node->regmap, QOSGEN_MAINCTL_LO(qos, port),
+				   BIT(QOS_SLV_URG_MSG_EN_SHFT),
+				   qos->urg_fwd << QOS_SLV_URG_MSG_EN_SHFT);
+	}
+}
+
 /**
  * qcom_icc_pre_aggregate - cleans up stale values from prior icc_set
  * @node: icc node to operate on
@@ -159,6 +192,113 @@ int qcom_icc_bcm_init(struct qcom_icc_bcm *bcm, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(qcom_icc_bcm_init);
 
+static bool bcm_needs_qos_proxy(struct qcom_icc_bcm *bcm)
+{
+	int i;
+
+	for (i = 0; i < bcm->num_nodes; i++)
+		if (bcm->nodes[i]->qosbox)
+			return true;
+
+	return false;
+}
+
+static int enable_qos_deps(struct qcom_icc_provider *qp)
+{
+	struct qcom_icc_bcm *bcm;
+	bool keepalive;
+	int ret, i;
+
+	for (i = 0; i < qp->num_bcms; i++) {
+		bcm = qp->bcms[i];
+		if (bcm_needs_qos_proxy(bcm)) {
+			keepalive = bcm->keepalive;
+			bcm->keepalive = true;
+
+			qcom_icc_bcm_voter_add(qp->voter, bcm);
+			ret = qcom_icc_bcm_voter_commit(qp->voter);
+
+			bcm->keepalive = keepalive;
+
+			if (ret) {
+				dev_err(qp->dev, "failed to vote BW to %s for QoS\n",
+					bcm->name);
+				return ret;
+			}
+		}
+	}
+
+	ret = clk_bulk_prepare_enable(qp->num_clks, qp->clks);
+	if (ret) {
+		dev_err(qp->dev, "failed to enable clocks for QoS\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void disable_qos_deps(struct qcom_icc_provider *qp)
+{
+	struct qcom_icc_bcm *bcm;
+	int i;
+
+	clk_bulk_disable_unprepare(qp->num_clks, qp->clks);
+
+	for (i = 0; i < qp->num_bcms; i++) {
+		bcm = qp->bcms[i];
+		if (bcm_needs_qos_proxy(bcm)) {
+			qcom_icc_bcm_voter_add(qp->voter, bcm);
+			qcom_icc_bcm_voter_commit(qp->voter);
+		}
+	}
+}
+
+int qcom_icc_rpmh_configure_qos(struct qcom_icc_provider *qp)
+{
+	struct qcom_icc_node *qnode;
+	size_t i;
+	int ret;
+
+	ret = enable_qos_deps(qp);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < qp->num_nodes; i++) {
+		qnode = qp->nodes[i];
+		if (!qnode)
+			continue;
+
+		if (qnode->qosbox)
+			qcom_icc_set_qos(qnode);
+	}
+
+	disable_qos_deps(qp);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_icc_rpmh_configure_qos);
+
+static struct regmap *qcom_icc_rpmh_map(struct platform_device *pdev,
+					const struct qcom_icc_desc *desc)
+{
+	void __iomem *base;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+
+	if (!desc->config)
+		return NULL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return NULL;
+
+	base = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(base))
+		return ERR_CAST(base);
+
+	return devm_regmap_init_mmio(dev, base, desc->config);
+}
+
 int qcom_icc_rpmh_probe(struct platform_device *pdev)
 {
 	const struct qcom_icc_desc *desc;
@@ -199,12 +339,22 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
 
 	qp->dev = dev;
 	qp->bcms = desc->bcms;
+	qp->nodes = desc->nodes;
 	qp->num_bcms = desc->num_bcms;
+	qp->num_nodes = desc->num_nodes;
 
 	qp->voter = of_bcm_voter_get(qp->dev, NULL);
 	if (IS_ERR(qp->voter))
 		return PTR_ERR(qp->voter);
 
+	qp->regmap = qcom_icc_rpmh_map(pdev, desc);
+	if (IS_ERR(qp->regmap))
+		return PTR_ERR(qp->regmap);
+
+	qp->num_clks = devm_clk_bulk_get_all(qp->dev, &qp->clks);
+	if (qp->num_clks < 0)
+		return qp->num_clks;
+
 	for (i = 0; i < qp->num_bcms; i++)
 		qcom_icc_bcm_init(qp->bcms[i], dev);
 
@@ -213,6 +363,8 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
 		if (!qn)
 			continue;
 
+		qn->regmap = dev_get_regmap(qp->dev, NULL);
+
 		node = icc_node_create(qn->id);
 		if (IS_ERR(node)) {
 			ret = PTR_ERR(node);
@@ -229,6 +381,10 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
 		data->nodes[i] = node;
 	}
 
+	ret = qcom_icc_rpmh_configure_qos(qp);
+	if (ret)
+		goto err_remove_nodes;
+
 	ret = icc_provider_register(provider);
 	if (ret)
 		goto err_remove_nodes;
@@ -247,6 +403,7 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
 err_deregister_provider:
 	icc_provider_deregister(provider);
 err_remove_nodes:
+	clk_bulk_put_all(qp->num_clks, qp->clks);
 	icc_nodes_remove(provider);
 
 	return ret;
@@ -258,6 +415,7 @@ void qcom_icc_rpmh_remove(struct platform_device *pdev)
 	struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
 
 	icc_provider_deregister(&qp->provider);
+	clk_bulk_put_all(qp->num_clks, qp->clks);
 	icc_nodes_remove(&qp->provider);
 }
 EXPORT_SYMBOL_GPL(qcom_icc_rpmh_remove);
diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h
index 2de29460e808..8fb674ff4637 100644
--- a/drivers/interconnect/qcom/icc-rpmh.h
+++ b/drivers/interconnect/qcom/icc-rpmh.h
@@ -7,6 +7,7 @@
 #define __DRIVERS_INTERCONNECT_QCOM_ICC_RPMH_H__
 
 #include <dt-bindings/interconnect/qcom,icc.h>
+#include <linux/regmap.h>
 
 #define to_qcom_provider(_provider) \
 	container_of(_provider, struct qcom_icc_provider, provider)
@@ -18,6 +19,11 @@
  * @bcms: list of bcms that maps to the provider
  * @num_bcms: number of @bcms
  * @voter: bcm voter targeted by this provider
+ * @nodes: list of interconnect nodes that maps to the provider
+ * @num_nodes: number of @nodes
+ * @regmap: used for QOS registers access
+ * @clks : clks required for register access
+ * @num_clks: number of @clks
  */
 struct qcom_icc_provider {
 	struct icc_provider provider;
@@ -25,6 +31,11 @@ struct qcom_icc_provider {
 	struct qcom_icc_bcm * const *bcms;
 	size_t num_bcms;
 	struct bcm_voter *voter;
+	struct qcom_icc_node * const *nodes;
+	size_t num_nodes;
+	struct regmap *regmap;
+	struct clk_bulk_data *clks;
+	int num_clks;
 };
 
 /**
@@ -41,6 +52,23 @@ struct bcm_db {
 	u8 reserved;
 };
 
+/**
+ * struct qcom_icc_qosbox - Qualcomm Technologies, Inc specific QoS config
+ * @prio: priority value assigned to requests on the node
+ * @urg_fwd: if set, master priority is used for requests.
+ * @prio_fwd_disable: if set, master priority is ignored and NOCs default priority is used.
+ * @num_ports: number of @ports
+ * @offsets: qos register offsets
+ */
+
+struct qcom_icc_qosbox {
+	u32 prio;
+	u32 urg_fwd;
+	bool prio_fwd_disable;
+	u32 num_ports;
+	u32 offsets[];
+};
+
 #define MAX_LINKS		128
 #define MAX_BCMS		64
 #define MAX_BCM_PER_NODE	3
@@ -58,6 +86,8 @@ struct bcm_db {
  * @max_peak: current max aggregate value of all peak bw requests
  * @bcms: list of bcms associated with this logical node
  * @num_bcms: num of @bcms
+ * @regmap: used for QOS registers access
+ * @qosbox: qos config data associated with node
  */
 struct qcom_icc_node {
 	const char *name;
@@ -70,6 +100,8 @@ struct qcom_icc_node {
 	u64 max_peak[QCOM_ICC_NUM_BUCKETS];
 	struct qcom_icc_bcm *bcms[MAX_BCM_PER_NODE];
 	size_t num_bcms;
+	struct regmap *regmap;
+	struct qcom_icc_qosbox *qosbox;
 };
 
 /**
@@ -114,6 +146,7 @@ struct qcom_icc_fabric {
 };
 
 struct qcom_icc_desc {
+	const struct regmap_config *config;
 	struct qcom_icc_node * const *nodes;
 	size_t num_nodes;
 	struct qcom_icc_bcm * const *bcms;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [1/4] interconnect: qcom: icc-rpmh: Add QoS config support

[Konrad Dybcio]

On 22.01.2024 15:30, Odelu Kukatla wrote:
> Introduce support to initialize QoS settings for QNOC platforms.

You should describe why this is useful.

For reference, disabling QoS programming on sm8350 on an android
kernel & userspace yields an inconsistent 1-2% difference in
benchmarks like geekbench or antutu, but perhaps it's useful for
not clogging up the NoCs when there's a lot of multimedia-dram
traffic etc.?

> 
> Change-Id: I068d49cbcfec5d34c01e5adc930eec72d306ed89

This tag has no place upstream

> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
> ---
>  drivers/interconnect/qcom/icc-rpmh.c | 158 +++++++++++++++++++++++++++
>  drivers/interconnect/qcom/icc-rpmh.h |  33 ++++++
>  2 files changed, 191 insertions(+)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
> index c1aa265c1f4e..49334065ccfa 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/interconnect.h>
>  #include <linux/interconnect-provider.h>
>  #include <linux/module.h>
> @@ -14,6 +16,37 @@
>  #include "icc-common.h"
>  #include "icc-rpmh.h"
>  
> +/* QNOC QoS */
> +#define QOSGEN_MAINCTL_LO(p, qp)	(0x8 + (p->offsets[qp]))
> +#define QOS_SLV_URG_MSG_EN_SHFT		3
> +#define QOS_DFLT_PRIO_MASK		0x7
> +#define QOS_DFLT_PRIO_SHFT		4
> +#define QOS_DISABLE_SHIFT		24

mask + shift -> GENMASK(), then use FIELD_PREP/GET in the callers

These are already defined in icc-rpm.c.. Perhaps they can be factored out
to icc-qnoc.h or something?

[...]

> +
> +static int enable_qos_deps(struct qcom_icc_provider *qp)

Can we perhaps integrate this into .sync_state?

Currently, !synced_state holds all paths (and by extension, all BCMs)
at their max values, so they're definitely enabled, and it conviniently
is also supposed to only fire once.

> +{
> +	struct qcom_icc_bcm *bcm;
> +	bool keepalive;
> +	int ret, i;
> +
> +	for (i = 0; i < qp->num_bcms; i++) {
> +		bcm = qp->bcms[i];
> +		if (bcm_needs_qos_proxy(bcm)) {
> +			keepalive = bcm->keepalive;
> +			bcm->keepalive = true;
> +
> +			qcom_icc_bcm_voter_add(qp->voter, bcm);
> +			ret = qcom_icc_bcm_voter_commit(qp->voter);
> +
> +			bcm->keepalive = keepalive;
> +
> +			if (ret) {
> +				dev_err(qp->dev, "failed to vote BW to %s for QoS\n",
> +					bcm->name);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	ret = clk_bulk_prepare_enable(qp->num_clks, qp->clks);
> +	if (ret) {
> +		dev_err(qp->dev, "failed to enable clocks for QoS\n");
> +		return ret;
> +	}

if (ret)
	dev_err(qp->dev...

return ret;

> +
> +	return 0;
> +}
> +
> +static void disable_qos_deps(struct qcom_icc_provider *qp)
> +{
> +	struct qcom_icc_bcm *bcm;
> +	int i;
> +
> +	clk_bulk_disable_unprepare(qp->num_clks, qp->clks);
> +
> +	for (i = 0; i < qp->num_bcms; i++) {
> +		bcm = qp->bcms[i];
> +		if (bcm_needs_qos_proxy(bcm)) {
> +			qcom_icc_bcm_voter_add(qp->voter, bcm);
> +			qcom_icc_bcm_voter_commit(qp->voter);
> +		}
> +	}
> +}
> +
> +int qcom_icc_rpmh_configure_qos(struct qcom_icc_provider *qp)
> +{
> +	struct qcom_icc_node *qnode;
> +	size_t i;
> +	int ret;
> +
> +	ret = enable_qos_deps(qp);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < qp->num_nodes; i++) {
> +		qnode = qp->nodes[i];
> +		if (!qnode)
> +			continue;
> +
> +		if (qnode->qosbox)
> +			qcom_icc_set_qos(qnode);
> +	}
> +
> +	disable_qos_deps(qp);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_icc_rpmh_configure_qos);

This is simply copypasted from downstream [1].. not necessary at all,
in this patch this func is exclusively called from within this file.

> +
> +static struct regmap *qcom_icc_rpmh_map(struct platform_device *pdev,
> +					const struct qcom_icc_desc *desc)
> +{
> +	void __iomem *base;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;

Reverse-Christmas-tree throughout the code, please

> +
> +	if (!desc->config)
> +		return NULL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return NULL;
> +
> +	base = devm_ioremap(dev, res->start, resource_size(res));
> +	if (IS_ERR(base))
> +		return ERR_CAST(base);
> +
> +	return devm_regmap_init_mmio(dev, base, desc->config);
> +}


This is devm_platform_get_and_ioremap_resource + devm_regmap_init_mmio

please inline this in the probe func

[...]

>  
> @@ -213,6 +363,8 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>  		if (!qn)
>  			continue;
>  
> +		qn->regmap = dev_get_regmap(qp->dev, NULL);

Why would all nodes need a regmap reference? there's to_qcom_provider()

> +
>  		node = icc_node_create(qn->id);
>  		if (IS_ERR(node)) {
>  			ret = PTR_ERR(node);
> @@ -229,6 +381,10 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>  		data->nodes[i] = node;
>  	}
>  
> +	ret = qcom_icc_rpmh_configure_qos(qp);
> +	if (ret)
> +		goto err_remove_nodes;
> +
>  	ret = icc_provider_register(provider);
>  	if (ret)
>  		goto err_remove_nodes;
> @@ -247,6 +403,7 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>  err_deregister_provider:
>  	icc_provider_deregister(provider);
>  err_remove_nodes:
> +	clk_bulk_put_all(qp->num_clks, qp->clks);

Use devm_clk_bulk_get_all instead

[...]

> + * @nodes: list of interconnect nodes that maps to the provider
> + * @num_nodes: number of @nodes
> + * @regmap: used for QOS registers access

QoS, 'register'

> + * @clks : clks required for register access
> + * @num_clks: number of @clks
>   */
>  struct qcom_icc_provider {
>  	struct icc_provider provider;
> @@ -25,6 +31,11 @@ struct qcom_icc_provider {
>  	struct qcom_icc_bcm * const *bcms;
>  	size_t num_bcms;
>  	struct bcm_voter *voter;
> +	struct qcom_icc_node * const *nodes;
> +	size_t num_nodes;
> +	struct regmap *regmap;
> +	struct clk_bulk_data *clks;
> +	int num_clks;
>  };
>  
>  /**
> @@ -41,6 +52,23 @@ struct bcm_db {
>  	u8 reserved;
>  };
>  
> +/**
> + * struct qcom_icc_qosbox - Qualcomm Technologies, Inc specific QoS config

qosbox -> qos

plus I'm not sure if the full company name adds value to a driver in
drivers/interconnect/qcom..

> + * @prio: priority value assigned to requests on the node
> + * @urg_fwd: if set, master priority is used for requests.

"master priority" meaning "this req goes before anyone else", or "use the
icc provider [master]'s priority value"?

> + * @prio_fwd_disable: if set, master priority is ignored and NOCs default priority is used.

NoC's

This sounds like !(prio || urg_fwd)? Surely it must do something more useful?


> + * @num_ports: number of @ports
> + * @offsets: qos register offsets
> + */
> +
> +struct qcom_icc_qosbox {
> +	u32 prio;
> +	u32 urg_fwd;
> +	bool prio_fwd_disable;
> +	u32 num_ports;
> +	u32 offsets[];

u32 offsets __counted_by(num_ports)

Also, it would probably be more clear if you renamed it to "port_offsets"

> +};
> +
>  #define MAX_LINKS		128
>  #define MAX_BCMS		64
>  #define MAX_BCM_PER_NODE	3
> @@ -58,6 +86,8 @@ struct bcm_db {
>   * @max_peak: current max aggregate value of all peak bw requests
>   * @bcms: list of bcms associated with this logical node
>   * @num_bcms: num of @bcms
> + * @regmap: used for QOS registers access
> + * @qosbox: qos config data associated with node
>   */
>  struct qcom_icc_node {
>  	const char *name;
> @@ -70,6 +100,8 @@ struct qcom_icc_node {
>  	u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>  	struct qcom_icc_bcm *bcms[MAX_BCM_PER_NODE];
>  	size_t num_bcms;
> +	struct regmap *regmap;

Remove

> +	struct qcom_icc_qosbox *qosbox;

Why would this be a pointer and not a const member of the struct?

It seems totally counter-intuitive to reuse QoS settings for more than
one node, given their offsets are unique.

Konrad

[1] https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/kernel.lnx.5.15.r26-rel/drivers/interconnect/qcom/icc-rpmh.c?ref_type=heads#L329-354

Re: [1/4] interconnect: qcom: icc-rpmh: Add QoS config support

[Odelu Kukatla]

On 2/22/2024 6:00 PM, Konrad Dybcio wrote:
> 
> 
> On 2/21/24 18:57, Odelu Kukatla wrote:
>>
>>
>> On 2/5/2024 5:51 PM, Konrad Dybcio wrote:
>>> On 5.02.2024 12:57, Odelu Kukatla (QUIC) wrote:
>>>> -----Original Message-----
>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> Sent: Monday, January 22, 2024 8:55 PM
>>>> To: Odelu Kukatla (QUIC) <quic_okukatla@quicinc.com>; georgi.djakov@linaro.org; Bjorn Andersson <andersson@kernel.org>; Georgi Djakov <djakov@kernel.org>; linux-arm-msm@vger.kernel.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [1/4] interconnect: qcom: icc-rpmh: Add QoS config support
> 
> Why is this email taken private?
> 
> [...]
> 
I am adding previous list back for now, will add "to list" from "./scripts/get_maintainer.pl" in V3.
>>>
>>
>> Yeah, you are right. !synced_state holds all BCMs enabled. But clock voting is still required.
>> But qcom_icc_rpmh_configure_qos() can be invoked from out side of qcom_icc_rpmh_probe() in QuickBoot mode where *_probe does not happen.
>> so irrespective of !synced_state votes, better to keep it separate from sync_state() and place votes on required BCMs during QoS time and remove them after that.
> 
> What on earth is quickboot mode?
> Grepping for it on linux-next, there's precisely 0 results.
> 
> If it's some downstream sw invention, it has no right to affect
> the code submitted upstream - "if it's not on the list, it doesn't
> exist".
> 

Quickboot is not there yet in upstream. I will clean the code related to BCM voting and move the required clock voting to qcom_icc_rpmh_configure_qos().

>>
>>>>> + * @prio: priority value assigned to requests on the node
>>>>> + * @urg_fwd: if set, master priority is used for requests.
>>>>
>>>> "master priority" meaning "this req goes before anyone else", or "use the icc provider [master]'s priority value"?
>>>>
>>>>> + * @prio_fwd_disable: if set, master priority is ignored and NOCs default priority is used.
>>>>
>>>> NoC's
>>>>
>>>> This sounds like !(prio || urg_fwd)? Surely it must do something more useful?
>>>
>>> This remains unanswered
>>>
>>> Konrad
>>
>> @prio_fwd_disable:
>> 1 -> FIXED MODE; upstream tnx-inband-priority is ignored; value @pio is used for transaction.
>> 0 -> BYPASS MODE; max of (tnx-inband-priority, @prio).
> 
> So, this could be basically called "force_priority_val"
> 
> 
>> @urg_fwd:
>> 1 -> Urgency promotion messages issued by the connected master IP remain unchanged as they transit the NIU and the QNoC QoS generator.
>> 0 -> IP-generated messages are discarded, and messages generated by QoS generators are used instead.
> 
> So the interconnect endpoints can effectively modify the priority if
> urg_fwd is enabled.. Interesting.. "ignore_ep_urgency_req"?
> 

Names were chosen closer to bit fields in QoS registers. I will keep the names same and update the documentation with clear explanation.

> Both of these should also be better documented, the kerneldoc you
> submitted explains nothing unless this what I believe to be excerpt
> from the documentation is present to support it.
> 
> Konrad

Thanks,
Odelu

Re: [1/4] interconnect: qcom: icc-rpmh: Add QoS config support

[Konrad Dybcio]

On 22.01.2024 15:30, Odelu Kukatla wrote:
> Introduce support to initialize QoS settings for QNOC platforms.
> 
> Change-Id: I068d49cbcfec5d34c01e5adc930eec72d306ed89
> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
> ---

[...]


> +
> +struct qcom_icc_qosbox {
> +	u32 prio;
> +	u32 urg_fwd;

Also, why is this field not a bool?

Everything in here could be const, btw

> +	bool prio_fwd_disable;
> +	u32 num_ports;
> +	u32 offsets[];
> +};
> +
>  #define MAX_LINKS		128
>  #define MAX_BCMS		64
>  #define MAX_BCM_PER_NODE	3
> @@ -58,6 +86,8 @@ struct bcm_db {
>   * @max_peak: current max aggregate value of all peak bw requests
>   * @bcms: list of bcms associated with this logical node
>   * @num_bcms: num of @bcms
> + * @regmap: used for QOS registers access
> + * @qosbox: qos config data associated with node
>   */
>  struct qcom_icc_node {
>  	const char *name;
> @@ -70,6 +100,8 @@ struct qcom_icc_node {
>  	u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>  	struct qcom_icc_bcm *bcms[MAX_BCM_PER_NODE];
>  	size_t num_bcms;
> +	struct regmap *regmap;
> +	struct qcom_icc_qosbox *qosbox;

this member here as well

Konrad

Re: [1/4] interconnect: qcom: icc-rpmh: Add QoS config support

[Jeff Johnson]

On 1/22/2024 6:30 AM, Odelu Kukatla wrote:
> Introduce support to initialize QoS settings for QNOC platforms.
> 
> Change-Id: I068d49cbcfec5d34c01e5adc930eec72d306ed89
> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
> ---
>  drivers/interconnect/qcom/icc-rpmh.c | 158 +++++++++++++++++++++++++++
>  drivers/interconnect/qcom/icc-rpmh.h |  33 ++++++
>  2 files changed, 191 insertions(+)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
> index c1aa265c1f4e..49334065ccfa 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.

Should all of your copyright changes include 2024?

[2/4] interconnect: qcom: sc7280: enable qos programming

[Odelu Kukatla]

Enable QoS for the master ports with predefined values
for priority and urgency.

Change-Id: I1c4515402bcd6df8eed814be096aa5e1fc16cef6
Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
---
 drivers/interconnect/qcom/sc7280.c | 250 +++++++++++++++++++++++++++++
 1 file changed, 250 insertions(+)

diff --git a/drivers/interconnect/qcom/sc7280.c b/drivers/interconnect/qcom/sc7280.c
index 7d33694368e8..719844c34894 100644
--- a/drivers/interconnect/qcom/sc7280.c
+++ b/drivers/interconnect/qcom/sc7280.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
  *
  */
 
@@ -16,29 +17,59 @@
 #include "icc-rpmh.h"
 #include "sc7280.h"
 
+static const struct regmap_config icc_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+};
+
+static struct qcom_icc_qosbox qhm_qspi_qos = {
+	.num_ports = 1,
+	.offsets = { 0x7000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qhm_qspi = {
 	.name = "qhm_qspi",
 	.id = SC7280_MASTER_QSPI_0,
 	.channels = 1,
 	.buswidth = 4,
+	.qosbox = &qhm_qspi_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox qhm_qup0_qos = {
+	.num_ports = 1,
+	.offsets = { 0x11000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qhm_qup0 = {
 	.name = "qhm_qup0",
 	.id = SC7280_MASTER_QUP_0,
 	.channels = 1,
 	.buswidth = 4,
+	.qosbox = &qhm_qup0_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox qhm_qup1_qos = {
+	.num_ports = 1,
+	.offsets = { 0x8000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qhm_qup1 = {
 	.name = "qhm_qup1",
 	.id = SC7280_MASTER_QUP_1,
 	.channels = 1,
 	.buswidth = 4,
+	.qosbox = &qhm_qup1_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
@@ -52,38 +83,70 @@ static struct qcom_icc_node qnm_a1noc_cfg = {
 	.links = { SC7280_SLAVE_SERVICE_A1NOC },
 };
 
+static struct qcom_icc_qosbox xm_sdc1_qos = {
+	.num_ports = 1,
+	.offsets = { 0xc000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node xm_sdc1 = {
 	.name = "xm_sdc1",
 	.id = SC7280_MASTER_SDCC_1,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &xm_sdc1_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox xm_sdc2_qos = {
+	.num_ports = 1,
+	.offsets = { 0xe000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node xm_sdc2 = {
 	.name = "xm_sdc2",
 	.id = SC7280_MASTER_SDCC_2,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &xm_sdc2_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox xm_sdc4_qos = {
+	.num_ports = 1,
+	.offsets = { 0x9000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node xm_sdc4 = {
 	.name = "xm_sdc4",
 	.id = SC7280_MASTER_SDCC_4,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &xm_sdc4_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox xm_ufs_mem_qos = {
+	.num_ports = 1,
+	.offsets = { 0xa000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node xm_ufs_mem = {
 	.name = "xm_ufs_mem",
 	.id = SC7280_MASTER_UFS_MEM,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &xm_ufs_mem_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
@@ -97,20 +160,36 @@ static struct qcom_icc_node xm_usb2 = {
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox xm_usb3_0_qos = {
+	.num_ports = 1,
+	.offsets = { 0xb000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node xm_usb3_0 = {
 	.name = "xm_usb3_0",
 	.id = SC7280_MASTER_USB3_0,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &xm_usb3_0_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A1NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox qhm_qdss_bam_qos = {
+	.num_ports = 1,
+	.offsets = { 0x18000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qhm_qdss_bam = {
 	.name = "qhm_qdss_bam",
 	.id = SC7280_MASTER_QDSS_BAM,
 	.channels = 1,
 	.buswidth = 4,
+	.qosbox = &qhm_qdss_bam_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A2NOC_SNOC },
 };
@@ -124,29 +203,53 @@ static struct qcom_icc_node qnm_a2noc_cfg = {
 	.links = { SC7280_SLAVE_SERVICE_A2NOC },
 };
 
+static struct qcom_icc_qosbox qnm_cnoc_datapath_qos = {
+	.num_ports = 1,
+	.offsets = { 0x1c000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qnm_cnoc_datapath = {
 	.name = "qnm_cnoc_datapath",
 	.id = SC7280_MASTER_CNOC_A2NOC,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &qnm_cnoc_datapath_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A2NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox qxm_crypto_qos = {
+	.num_ports = 1,
+	.offsets = { 0x1d000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qxm_crypto = {
 	.name = "qxm_crypto",
 	.id = SC7280_MASTER_CRYPTO,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &qxm_crypto_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A2NOC_SNOC },
 };
 
+static struct qcom_icc_qosbox qxm_ipa_qos = {
+	.num_ports = 1,
+	.offsets = { 0x10000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qxm_ipa = {
 	.name = "qxm_ipa",
 	.id = SC7280_MASTER_IPA,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &qxm_ipa_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A2NOC_SNOC },
 };
@@ -168,11 +271,19 @@ static struct qcom_icc_node xm_pcie3_1 = {
 	.links = { SC7280_SLAVE_ANOC_PCIE_GEM_NOC },
 };
 
+static struct qcom_icc_qosbox xm_qdss_etr_qos = {
+	.num_ports = 1,
+	.offsets = { 0x15000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node xm_qdss_etr = {
 	.name = "xm_qdss_etr",
 	.id = SC7280_MASTER_QDSS_ETR,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &xm_qdss_etr_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_A2NOC_SNOC },
 };
@@ -300,20 +411,36 @@ static struct qcom_icc_node qnm_cnoc_dc_noc = {
 	.links = { SC7280_SLAVE_LLCC_CFG, SC7280_SLAVE_GEM_NOC_CFG },
 };
 
+static struct qcom_icc_qosbox alm_gpu_tcu_qos = {
+	.num_ports = 1,
+	.offsets = { 0xd7000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node alm_gpu_tcu = {
 	.name = "alm_gpu_tcu",
 	.id = SC7280_MASTER_GPU_TCU,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &alm_gpu_tcu_qos,
 	.num_links = 2,
 	.links = { SC7280_SLAVE_GEM_NOC_CNOC, SC7280_SLAVE_LLCC },
 };
 
+static struct qcom_icc_qosbox alm_sys_tcu_qos = {
+	.num_ports = 1,
+	.offsets = { 0xd6000 },
+	.prio = 6,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node alm_sys_tcu = {
 	.name = "alm_sys_tcu",
 	.id = SC7280_MASTER_SYS_TCU,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &alm_sys_tcu_qos,
 	.num_links = 2,
 	.links = { SC7280_SLAVE_GEM_NOC_CNOC, SC7280_SLAVE_LLCC },
 };
@@ -328,11 +455,19 @@ static struct qcom_icc_node chm_apps = {
 		   SC7280_SLAVE_MEM_NOC_PCIE_SNOC },
 };
 
+static struct qcom_icc_qosbox qnm_cmpnoc_qos = {
+	.num_ports = 2,
+	.offsets = { 0x21000, 0x61000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qnm_cmpnoc = {
 	.name = "qnm_cmpnoc",
 	.id = SC7280_MASTER_COMPUTE_NOC,
 	.channels = 2,
 	.buswidth = 32,
+	.qosbox = &qnm_cmpnoc_qos,
 	.num_links = 2,
 	.links = { SC7280_SLAVE_GEM_NOC_CNOC, SC7280_SLAVE_LLCC },
 };
@@ -348,29 +483,53 @@ static struct qcom_icc_node qnm_gemnoc_cfg = {
 		   SC7280_SLAVE_SERVICE_GEM_NOC },
 };
 
+static struct qcom_icc_qosbox qnm_gpu_qos = {
+	.num_ports = 2,
+	.offsets = { 0x22000, 0x62000 },
+	.prio = 0,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qnm_gpu = {
 	.name = "qnm_gpu",
 	.id = SC7280_MASTER_GFX3D,
 	.channels = 2,
 	.buswidth = 32,
+	.qosbox = &qnm_gpu_qos,
 	.num_links = 2,
 	.links = { SC7280_SLAVE_GEM_NOC_CNOC, SC7280_SLAVE_LLCC },
 };
 
+static struct qcom_icc_qosbox qnm_mnoc_hf_qos = {
+	.num_ports = 2,
+	.offsets = { 0x23000, 0x63000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qnm_mnoc_hf = {
 	.name = "qnm_mnoc_hf",
 	.id = SC7280_MASTER_MNOC_HF_MEM_NOC,
 	.channels = 2,
 	.buswidth = 32,
+	.qosbox = &qnm_mnoc_hf_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_LLCC },
 };
 
+static struct qcom_icc_qosbox qnm_mnoc_sf_qos = {
+	.num_ports = 1,
+	.offsets = { 0xcf000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qnm_mnoc_sf = {
 	.name = "qnm_mnoc_sf",
 	.id = SC7280_MASTER_MNOC_SF_MEM_NOC,
 	.channels = 1,
 	.buswidth = 32,
+	.qosbox = &qnm_mnoc_sf_qos,
 	.num_links = 2,
 	.links = { SC7280_SLAVE_GEM_NOC_CNOC, SC7280_SLAVE_LLCC },
 };
@@ -384,20 +543,36 @@ static struct qcom_icc_node qnm_pcie = {
 	.links = { SC7280_SLAVE_GEM_NOC_CNOC, SC7280_SLAVE_LLCC },
 };
 
+static struct qcom_icc_qosbox qnm_snoc_gc_qos = {
+	.num_ports = 1,
+	.offsets = { 0xd3000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qnm_snoc_gc = {
 	.name = "qnm_snoc_gc",
 	.id = SC7280_MASTER_SNOC_GC_MEM_NOC,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &qnm_snoc_gc_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_LLCC },
 };
 
+static struct qcom_icc_qosbox qnm_snoc_sf_qos = {
+	.num_ports = 1,
+	.offsets = { 0xd4000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qnm_snoc_sf = {
 	.name = "qnm_snoc_sf",
 	.id = SC7280_MASTER_SNOC_SF_MEM_NOC,
 	.channels = 1,
 	.buswidth = 16,
+	.qosbox = &qnm_snoc_sf_qos,
 	.num_links = 3,
 	.links = { SC7280_SLAVE_GEM_NOC_CNOC, SC7280_SLAVE_LLCC,
 		   SC7280_SLAVE_MEM_NOC_PCIE_SNOC },
@@ -432,56 +607,104 @@ static struct qcom_icc_node qnm_mnoc_cfg = {
 	.links = { SC7280_SLAVE_SERVICE_MNOC },
 };
 
+static struct qcom_icc_qosbox qnm_video0_qos = {
+	.num_ports = 1,
+	.offsets = { 0x14000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qnm_video0 = {
 	.name = "qnm_video0",
 	.id = SC7280_MASTER_VIDEO_P0,
 	.channels = 1,
 	.buswidth = 32,
+	.qosbox = &qnm_video0_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_MNOC_SF_MEM_NOC },
 };
 
+static struct qcom_icc_qosbox qnm_video_cpu_qos = {
+	.num_ports = 1,
+	.offsets = { 0x15000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qnm_video_cpu = {
 	.name = "qnm_video_cpu",
 	.id = SC7280_MASTER_VIDEO_PROC,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &qnm_video_cpu_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_MNOC_SF_MEM_NOC },
 };
 
+static struct qcom_icc_qosbox qxm_camnoc_hf_qos = {
+	.num_ports = 2,
+	.offsets = { 0x10000, 0x10180 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qxm_camnoc_hf = {
 	.name = "qxm_camnoc_hf",
 	.id = SC7280_MASTER_CAMNOC_HF,
 	.channels = 2,
 	.buswidth = 32,
+	.qosbox = &qxm_camnoc_hf_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_MNOC_HF_MEM_NOC },
 };
 
+static struct qcom_icc_qosbox qxm_camnoc_icp_qos = {
+	.num_ports = 1,
+	.offsets = { 0x11000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qxm_camnoc_icp = {
 	.name = "qxm_camnoc_icp",
 	.id = SC7280_MASTER_CAMNOC_ICP,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &qxm_camnoc_icp_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_MNOC_SF_MEM_NOC },
 };
 
+static struct qcom_icc_qosbox qxm_camnoc_sf_qos = {
+	.num_ports = 1,
+	.offsets = { 0x12000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qxm_camnoc_sf = {
 	.name = "qxm_camnoc_sf",
 	.id = SC7280_MASTER_CAMNOC_SF,
 	.channels = 1,
 	.buswidth = 32,
+	.qosbox = &qxm_camnoc_sf_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_MNOC_SF_MEM_NOC },
 };
 
+static struct qcom_icc_qosbox qxm_mdp0_qos = {
+	.num_ports = 1,
+	.offsets = { 0x16000 },
+	.prio = 0,
+	.urg_fwd = 1,
+};
+
 static struct qcom_icc_node qxm_mdp0 = {
 	.name = "qxm_mdp0",
 	.id = SC7280_MASTER_MDP0,
 	.channels = 1,
 	.buswidth = 32,
+	.qosbox = &qxm_mdp0_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_MNOC_HF_MEM_NOC },
 };
@@ -531,20 +754,36 @@ static struct qcom_icc_node qnm_snoc_cfg = {
 	.links = { SC7280_SLAVE_SERVICE_SNOC },
 };
 
+static struct qcom_icc_qosbox qxm_pimem_qos = {
+	.num_ports = 1,
+	.offsets = { 0x8000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node qxm_pimem = {
 	.name = "qxm_pimem",
 	.id = SC7280_MASTER_PIMEM,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &qxm_pimem_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_SNOC_GEM_NOC_GC },
 };
 
+static struct qcom_icc_qosbox xm_gic_qos = {
+	.num_ports = 1,
+	.offsets = { 0xa000 },
+	.prio = 2,
+	.urg_fwd = 0,
+};
+
 static struct qcom_icc_node xm_gic = {
 	.name = "xm_gic",
 	.id = SC7280_MASTER_GIC,
 	.channels = 1,
 	.buswidth = 8,
+	.qosbox = &xm_gic_qos,
 	.num_links = 1,
 	.links = { SC7280_SLAVE_SNOC_GEM_NOC_GC },
 };
@@ -1503,6 +1742,7 @@ static struct qcom_icc_node * const aggre1_noc_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_aggre1_noc = {
+	.config = &icc_regmap_config,
 	.nodes = aggre1_noc_nodes,
 	.num_nodes = ARRAY_SIZE(aggre1_noc_nodes),
 	.bcms = aggre1_noc_bcms,
@@ -1525,6 +1765,7 @@ static struct qcom_icc_node * const aggre2_noc_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_aggre2_noc = {
+	.config = &icc_regmap_config,
 	.nodes = aggre2_noc_nodes,
 	.num_nodes = ARRAY_SIZE(aggre2_noc_nodes),
 	.bcms = aggre2_noc_bcms,
@@ -1606,6 +1847,7 @@ static struct qcom_icc_node * const cnoc2_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_cnoc2 = {
+	.config = &icc_regmap_config,
 	.nodes = cnoc2_nodes,
 	.num_nodes = ARRAY_SIZE(cnoc2_nodes),
 	.bcms = cnoc2_bcms,
@@ -1638,6 +1880,7 @@ static struct qcom_icc_node * const cnoc3_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_cnoc3 = {
+	.config = &icc_regmap_config,
 	.nodes = cnoc3_nodes,
 	.num_nodes = ARRAY_SIZE(cnoc3_nodes),
 	.bcms = cnoc3_bcms,
@@ -1654,6 +1897,7 @@ static struct qcom_icc_node * const dc_noc_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_dc_noc = {
+	.config = &icc_regmap_config,
 	.nodes = dc_noc_nodes,
 	.num_nodes = ARRAY_SIZE(dc_noc_nodes),
 	.bcms = dc_noc_bcms,
@@ -1690,6 +1934,7 @@ static struct qcom_icc_node * const gem_noc_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_gem_noc = {
+	.config = &icc_regmap_config,
 	.nodes = gem_noc_nodes,
 	.num_nodes = ARRAY_SIZE(gem_noc_nodes),
 	.bcms = gem_noc_bcms,
@@ -1710,6 +1955,7 @@ static struct qcom_icc_node * const lpass_ag_noc_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_lpass_ag_noc = {
+	.config = &icc_regmap_config,
 	.nodes = lpass_ag_noc_nodes,
 	.num_nodes = ARRAY_SIZE(lpass_ag_noc_nodes),
 	.bcms = lpass_ag_noc_bcms,
@@ -1727,6 +1973,7 @@ static struct qcom_icc_node * const mc_virt_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_mc_virt = {
+	.config = &icc_regmap_config,
 	.nodes = mc_virt_nodes,
 	.num_nodes = ARRAY_SIZE(mc_virt_nodes),
 	.bcms = mc_virt_bcms,
@@ -1754,6 +2001,7 @@ static struct qcom_icc_node * const mmss_noc_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_mmss_noc = {
+	.config = &icc_regmap_config,
 	.nodes = mmss_noc_nodes,
 	.num_nodes = ARRAY_SIZE(mmss_noc_nodes),
 	.bcms = mmss_noc_bcms,
@@ -1773,6 +2021,7 @@ static struct qcom_icc_node * const nsp_noc_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_nsp_noc = {
+	.config = &icc_regmap_config,
 	.nodes = nsp_noc_nodes,
 	.num_nodes = ARRAY_SIZE(nsp_noc_nodes),
 	.bcms = nsp_noc_bcms,
@@ -1798,6 +2047,7 @@ static struct qcom_icc_node * const system_noc_nodes[] = {
 };
 
 static const struct qcom_icc_desc sc7280_system_noc = {
+	.config = &icc_regmap_config,
 	.nodes = system_noc_nodes,
 	.num_nodes = ARRAY_SIZE(system_noc_nodes),
 	.bcms = system_noc_bcms,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [2/4] interconnect: qcom: sc7280: enable qos programming

[Konrad Dybcio]

On 22.01.2024 15:30, Odelu Kukatla wrote:
> Enable QoS for the master ports with predefined values
> for priority and urgency.
> 
> Change-Id: I1c4515402bcd6df8eed814be096aa5e1fc16cef6
> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
> ---
>  drivers/interconnect/qcom/sc7280.c | 250 +++++++++++++++++++++++++++++
>  1 file changed, 250 insertions(+)
> 
> diff --git a/drivers/interconnect/qcom/sc7280.c b/drivers/interconnect/qcom/sc7280.c
> index 7d33694368e8..719844c34894 100644
> --- a/drivers/interconnect/qcom/sc7280.c
> +++ b/drivers/interconnect/qcom/sc7280.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>   *
>   */
>  
> @@ -16,29 +17,59 @@
>  #include "icc-rpmh.h"
>  #include "sc7280.h"
>  
> +static const struct regmap_config icc_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,

If you don't bother having .max_register, perhaps it could be defined
in a common file?

Also, do you really need locking between each access? If not, add
.fast_io = true

Konrad

[3/4] dt-bindings: interconnect: Add clock property to enable QOS on SC7280

[Odelu Kukatla]

Added clock property to enable clocks required for accessing
qos registers.

Change-Id: Ie0478cc7ae9742742e2389cfa37ee57bab1aa320
Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
---
 .../interconnect/qcom,sc7280-rpmh.yaml        | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
index b135597d9489..758a6e924037 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
@@ -53,10 +53,50 @@ allOf:
       required:
         - reg
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc7280-aggre1-noc
+    then:
+      properties:
+        clocks:
+          items:
+            - description: aggre UFS PHY AXI clock
+            - description: aggre USB3 PRIM AXI clock
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc7280-aggre2-noc
+    then:
+      properties:
+        clocks:
+          items:
+            - description: RPMH CC IPA clock
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc7280-aggre1-noc
+              - qcom,sc7280-aggre2-noc
+    then:
+      required:
+        - clocks
+    else:
+      properties:
+        clocks: false
+
 unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
     interconnect {
         compatible = "qcom,sc7280-clk-virt";
         #interconnect-cells = <2>;
@@ -69,3 +109,12 @@ examples:
         #interconnect-cells = <2>;
         qcom,bcm-voters = <&apps_bcm_voter>;
     };
+
+    interconnect@16e0000 {
+        reg = <0x016e0000 0x1c080>;
+        compatible = "qcom,sc7280-aggre1-noc";
+        #interconnect-cells = <2>;
+        qcom,bcm-voters = <&apps_bcm_voter>;
+        clocks = <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
+                 <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>;
+    };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [3/4] dt-bindings: interconnect: Add clock property to enable QOS on SC7280

[Krzysztof Kozlowski]

On 22/01/2024 15:30, Odelu Kukatla wrote:
> Added clock property to enable clocks required for accessing
> qos registers.

Please use standard email subjects, so with the PATCH keyword in the
title. `git format-patch` helps here to create proper versioned patches.
Another useful tool is b4. Skipping the PATCH keyword makes filtering of
emails more difficult thus making the review process less convenient.

> 
> Change-Id: Ie0478cc7ae9742742e2389cfa37ee57bab1aa320

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Best regards,
Krzysztof

[4/4] arm64: dts: qcom: sc7280: Add clocks for QOS configuration

[Odelu Kukatla]

Add clock handles for required clocks to be enabled for
configuring QoS on sc7280.

Change-Id: I58991300ff1d8d2865763d4e79ee81c03586249e
Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 83b5b76ba179..73acf1bd0f97 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2099,6 +2099,8 @@
 			reg = <0 0x016e0000 0 0x1c080>;
 			#interconnect-cells = <2>;
 			qcom,bcm-voters = <&apps_bcm_voter>;
+			clocks = <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
+				<&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>;
 		};
 
 		aggre2_noc: interconnect@1700000 {
@@ -2106,6 +2108,7 @@
 			compatible = "qcom,sc7280-aggre2-noc";
 			#interconnect-cells = <2>;
 			qcom,bcm-voters = <&apps_bcm_voter>;
+			clocks = <&rpmhcc RPMH_IPA_CLK>;
 		};
 
 		mmss_noc: interconnect@1740000 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [4/4] arm64: dts: qcom: sc7280: Add clocks for QOS configuration

[Dmitry Baryshkov]

On Mon, 22 Jan 2024 at 16:39, Odelu Kukatla <quic_okukatla@quicinc.com> wrote:
>
> Add clock handles for required clocks to be enabled for
> configuring QoS on sc7280.
>
> Change-Id: I58991300ff1d8d2865763d4e79ee81c03586249e
> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 83b5b76ba179..73acf1bd0f97 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2099,6 +2099,8 @@
>                         reg = <0 0x016e0000 0 0x1c080>;
>                         #interconnect-cells = <2>;
>                         qcom,bcm-voters = <&apps_bcm_voter>;
> +                       clocks = <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
> +                               <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>;
>                 };
>
>                 aggre2_noc: interconnect@1700000 {
> @@ -2106,6 +2108,7 @@
>                         compatible = "qcom,sc7280-aggre2-noc";
>                         #interconnect-cells = <2>;
>                         qcom,bcm-voters = <&apps_bcm_voter>;
> +                       clocks = <&rpmhcc RPMH_IPA_CLK>;

Is there any reason to write QoS for the IPA before the IPA starts
poking around? The same question applies to aggre1 NoC.

>                 };
>
>                 mmss_noc: interconnect@1740000 {
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>


-- 
With best wishes
Dmitry

Re: [4/4] arm64: dts: qcom: sc7280: Add clocks for QOS configuration

[Konrad Dybcio]

On 1/22/24 18:43, Dmitry Baryshkov wrote:
> On Mon, 22 Jan 2024 at 16:39, Odelu Kukatla <quic_okukatla@quicinc.com> wrote:
>>
>> Add clock handles for required clocks to be enabled for
>> configuring QoS on sc7280.
>>
>> Change-Id: I58991300ff1d8d2865763d4e79ee81c03586249e
>> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 83b5b76ba179..73acf1bd0f97 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -2099,6 +2099,8 @@
>>                          reg = <0 0x016e0000 0 0x1c080>;
>>                          #interconnect-cells = <2>;
>>                          qcom,bcm-voters = <&apps_bcm_voter>;
>> +                       clocks = <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
>> +                               <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>;
>>                  };
>>
>>                  aggre2_noc: interconnect@1700000 {
>> @@ -2106,6 +2108,7 @@
>>                          compatible = "qcom,sc7280-aggre2-noc";
>>                          #interconnect-cells = <2>;
>>                          qcom,bcm-voters = <&apps_bcm_voter>;
>> +                       clocks = <&rpmhcc RPMH_IPA_CLK>;
> 
> Is there any reason to write QoS for the IPA before the IPA starts
> poking around? The same question applies to aggre1 NoC.

Yes, as the NIUs require a clock source which may or may not be
the peripheral's clock

Konrad