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

[Odelu Kukatla]

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@xxxxxxxxxxx>
>>>>> ---
>>
>> [...]
>>
>>>>> @@ -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@xxxxxxxxxx/
> 
> 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