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

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

On 5/8/24 03:45, Mike Tipton wrote:
> 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.

I think that would make more sense indeed

[...]

>>   
>> +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.

Right

Konrad