Re: [PATCH v5 01/10] interconnect: qcom: rpm: make QoS INVALID default, separate out driver data

[Bryan O'Donoghue]

On 17/02/2023 10:46, Konrad Dybcio wrote:

I find this commit log difficult to understand.

Could you reduce it down ?

> Currently, NOC_QOS_MODE_FIXED is defined as 0x0, which makes it the
> "default" option (as that's what uninitialized members of partially
> initialized structs are set to), which should really have been
> NOC_QOS_MODE_INVALID, and that's what people (wrongly) assumed was
> the case when .qos.qos_mode was not defined and what really makes
> the most sense..

"Currently NOC_QOS_MODE_FIXED is defined as 0x0 which makes it the 
default option. The default option however should be NOC_QOS_MODE_INVALID"

> That resulted in {port 0, prio 0, areq_prio 0, urg_fwd = false, rpm-voted}
> QoS being always voted for, because the code flow assumed "hey, it's fixed
> QoS, so let's just roll with whatever parameters are set" [again, set by
> partial struct initialization, as these fields were left unfilled by the
> developers]. That is of course incorrect, and on many of these platforms
> port 0 is MAS_APPS_PROC, which 9/10 times is supposed to be handled by
> the ap_owned path, not to mention the rest of the parameters may differ.
> Arguably, the APPS node is the most important one, next to EBI0..

This paragraph in particular is difficult to decipher, at least for me 
with my native Dublinese


> The modes are defined as preprocessor constants. They are not used
> anywhere outside the driver or sent to any remote processor outside
> qcom_icc_set_noc_qos(), which is easily worked around.
> Separate the type specified in driver data from the value sent to msmbus.
> Make the former an enum for better mainainability.
>
> This is an implicit fix for every SMD RPM ICC driver that didn't
> explicitly specify NOC_QOS_MODE_INVALID on non-AP_owned nodes that
> don't have QoS settings.

It would be nice to reduce the commit log down to say three paragraphs 
of no more than three sentences each.

---
bod