Received-SPF: pass (google.com: domain of linux-kernel+bounces-192574-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <434c6cfa-cede-4e62-a785-35a81ae0d30d@quicinc.com>
Date: Tue, 28 May 2024 20:22:22 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 1/4] interconnect: qcom: icc-rpmh: Add QoS
 configuration support
To: Mike Tipton <quic_mdtipton@quicinc.com>,
        Konrad Dybcio
	<konrad.dybcio@linaro.org>
CC: Bjorn Andersson <andersson@kernel.org>, Georgi Djakov <djakov@kernel.org>,
        Rob Herring <robh@kernel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        "Kees Cook" <keescook@chromium.org>,
        <cros-qcom-dts-watchers@chromium.org>,
        "Gustavo A . R . Silva" <gustavoars@kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-pm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-hardening@vger.kernel.org>, <quic_rlaggysh@quicinc.com>
References: <20240325181628.9407-1-quic_okukatla@quicinc.com>
 <20240325181628.9407-2-quic_okukatla@quicinc.com>
 <d59896bb-a559-4013-a615-37bb43278b2e@linaro.org>
 <91f59477-1799-4db6-bcc2-3f0c5225d1c8@quicinc.com>
 <0a58e05a-7bf5-459a-b202-66d88c095b45@linaro.org>
 <20240508023716.GD25316@hu-mdtipton-lv.qualcomm.com>
Content-Language: en-US
From: Odelu Kukatla <quic_okukatla@quicinc.com>
In-Reply-To: <20240508023716.GD25316@hu-mdtipton-lv.qualcomm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

Hi Konrad,

On 5/8/2024 8:07 AM, Mike Tipton wrote:
> On Sat, Apr 13, 2024 at 09:31:47PM +0200, Konrad Dybcio wrote:
>> On 3.04.2024 10:45 AM, Odelu Kukatla wrote:
>>>
>>>
>>> On 3/27/2024 2:26 AM, Konrad Dybcio wrote:
>>>> On 25.03.2024 7:16 PM, Odelu Kukatla wrote:
>>>>> It adds QoS support for QNOC device and includes support for
>>>>> configuring priority, priority forward disable, urgency forwarding.
>>>>> This helps in priortizing the traffic originating from different
>>>>> interconnect masters at NoC(Network On Chip).
>>>>>
>>>>> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
>>>>> ---
>>
>> [...]
>>
>>>>> @@ -70,6 +102,7 @@ struct qcom_icc_node {
>>>>>  	u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>>>>>  	struct qcom_icc_bcm *bcms[MAX_BCM_PER_NODE];
>>>>>  	size_t num_bcms;
>>>>> +	const struct qcom_icc_qosbox *qosbox;
>>>>
>>>> I believe I came up with a better approach for storing this.. see [1]
>>>>
>>>> Konrad
>>>>
>>>> [1] https://lore.kernel.org/linux-arm-msm/20240326-topic-rpm_icc_qos_cleanup-v1-4-357e736792be@linaro.org/
> 
> Note that I replied to this patch series as well. Similar comments here
> for how that approach would apply to icc-rpmh.
> 
>>>>
>>>
>>> I see in this series, QoS parameters are moved into struct qcom_icc_desc. 
>>> Even though we program QoS at Provider/Bus level, it is property of the node/master connected to a Bus/NoC.
>>
>> I don't see how it could be the case, we're obviously telling the controller which
>> endpoints have priority over others, not telling nodes whether the data they
>> transfer can omit the queue.
> 
> The QoS settings tune the priority of data coming out of a specific port
> on the NOC. The nodes are 1:1 with the ports. Yes, this does tell the
> NOC which ports have priority over others. But that's done by
> configuring each port's priority in their own port-specific QoS
> registers.
> 
>>
>>> It will be easier later to know which master's QoS we are programming if we add in node data.
>>> Readability point of view,  it might be good to keep QoS parameters in node data.  
>>
>> I don't agree here either, with the current approach we've made countless mistakes
>> when converting the downstream data (I have already submitted some fixes with more
>> in flight), as there's tons of jumping around the code to find what goes where.
> 
> I don't follow why keeping the port's own QoS settings in that port's
> struct results in more jumping around. It should do the opposite, in
> fact. If someone wants to know the QoS settings applied to the qhm_qup0
> port, then they should be able to look directly in the qhm_qup0 struct.
> Otherwise, if it's placed elsewhere then they'd have to jump elsewhere
> to find what that logical qhm_qup0-related data is set to.
> 
> If it *was* placed elsewhere, then we'd still need some logical way to
> map between that separate location and the node it's associated with.
> Which is a problem with your patch for cleaning up the icc-rpm QoS. In
> its current form, it's impossible to identify which QoS settings apply
> to which logical node (without detailed knowledge of the NOC register
> layout).
> 
> Keeping this data with the node struct reduces the need for extra layers
> of mapping between the QoS settings and the node struct. It keeps all
> the port-related information all together in one place.
> 
> I did like your earlier suggestion of using a compound literal to
> initialize the .qosbox pointers, such that we don't need a separate
> top-level variable defined for them. They're only ever referenced by a
> single node, so there's no need for them to be separate variables.
> 
> But I don't see the logic in totally separating the QoS data from the
> port it's associated with.
> 
>>
I will update the patch as per your suggestion of keeping .qosbox initialization inside *qcom_icc_node* structure.
I will post next version with this update and addressing other comments from v4.

Thanks,
Odelu

>> Konrad