by Mike Tipton

[permalink] [raw]

Subject: Re: [PATCH 4/4] interconnect: qcom: icc-rpm: Remodel how QoS settings are stored

Hi Konrad,

On Tue, Mar 26, 2024 at 08:42:35PM +0100, Konrad Dybcio wrote:
> Currently, the QoS settings are stored in the node data, even though
> they're a property of the bus/provider instead. Moreover, they are only
> needed during the probe step, so they can be easily moved into struct
> qcom_icc_desc.

The QoS settings *are* fundamentally a property of the node. The nodes
are 1:1 with the NOC ports. And the QoS settings tune the priority of
the data coming out of those ports. So, logically speaking, the QoS data
does belong in the node structs along with the rest of the data for that
node and port.

Only a subset of NOC ports support configurable QoS, but for those ports
that do it's a property of the port itself. Those settings impact just
that specific port and nothing else.

The current method of directly embedding the qcom_icc_qos_data struct
into qcom_icc_node isn't optimal, since that data is irrelevant for
ports that don't support it. So, the size could be optimized by
converting qcom_icc_node::qos into a pointer instead. But I don't think
we should separate the QoS settings from node struct entirely. It makes
it very difficult to understand which QoS settings are impacting which
port.

For example...

>
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index 788131400cd1..96c8ea8edd7a 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -43,11 +43,7 @@ static struct qcom_icc_node mas_pcie_0 = {
> .buswidth = 8,
> .mas_rpm_id = 65,
> .slv_rpm_id = -1,
> - .qos.ap_owned = true,
> - .qos.qos_mode = NOC_QOS_MODE_FIXED,
> - .qos.areq_prio = 1,
> - .qos.prio_level = 1,
> - .qos.qos_port = 0,
> + .ap_owned = true,
> .num_links = ARRAY_SIZE(mas_a0noc_common_links),
> .links = mas_a0noc_common_links
> };

[...]

>
> +static const struct qcom_icc_qos_data a0noc_qos_data[] = {
> + {
> + .qos_port = 0,
> + .qos_mode = NOC_QOS_MODE_FIXED,
> + .areq_prio = 1,
> + .prio_level = 1,
> + .urg_fwd_en = false,
> + .limit_commands = false,
> + }, {

How can I tell that these a0noc_qos_data[0] settings are for the
mas_pcie_0 port? It's not possible from the code anymore. *We* could
figure it out internally by looking at the NOC SWI to determine the
qos_port index. But this should be obvious from the code itself.

> + .qos_port = 1,
> + .qos_mode = NOC_QOS_MODE_FIXED,
> + .areq_prio = 1,
> + .prio_level = 1,
> + .urg_fwd_en = false,
> + .limit_commands = false,
> + }, {
> + .qos_port = 2,
> + .qos_mode = NOC_QOS_MODE_FIXED,
> + .areq_prio = 1,
> + .prio_level = 1,
> + .urg_fwd_en = false,
> + .limit_commands = false,
> + },
> +};
> +

2024-05-08 01:57:49

by Mike Tipton

[permalink] [raw]

Subject: Re: [PATCH 3/4] interconnect: qcom: icc-rpm: Make simple functions return void

On Tue, Mar 26, 2024 at 08:42:34PM +0100, Konrad Dybcio wrote:
> Register accesses can't just randomly fail. Change the return type of
> functions that only do that to void.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/interconnect/qcom/icc-rpm.c | 110 +++++++++++++++---------------------
> 1 file changed, 47 insertions(+), 63 deletions(-)
>

[...]

> @@ -115,48 +111,42 @@ static int qcom_icc_set_bimc_qos(struct icc_node *src)
> * only if we are NOT in Bypass Mode.
> */
> if (mode != NOC_QOS_MODE_BYPASS) {
> - for (i = 3; i >= 0; i--) {
> - rc = qcom_icc_bimc_set_qos_health(qp,
> - &qn->qos, i);
> - if (rc)
> - return rc;
> - }
> + qcom_icc_bimc_set_qos_health(qp, &qn->qos, 3);
> + qcom_icc_bimc_set_qos_health(qp, &qn->qos, 2);
> + qcom_icc_bimc_set_qos_health(qp, &qn->qos, 1);
> + qcom_icc_bimc_set_qos_health(qp, &qn->qos, 0);

Not sure I see the point of unrolling this loop. With the error checking
removed, the loop could be reduced to just two lines, which is shorter
than the unrolled version and isn't really any more complicated:

for (i = 3; i >= 0; i--)
qcom_icc_bimc_set_qos_health(qp, &qn->qos, i);

Unrolling the loop is also slightly out-of-scope for this patch that
claims to just change the return types.

Otherwise, patch looks good to me.