Re: [PATCH v4 1/4] interconnect: qcom: icc-rpmh: Add QoS configuration support

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 5/28/24 16:52, Odelu Kukatla wrote:
> 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.

Sorry for the late answer, but ack, let's go forward with this

Konrad