Message-ID: <9841fd7d-861c-4b5c-51cf-7b8da53a06c9@linaro.org>
Date:   Tue, 14 Feb 2023 17:34:30 +0100
From:   Neil Armstrong <neil.armstrong@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-arm-msm@...r.kernel.org, andersson@...nel.org,
        agross@...nel.org
Cc:     marijn.suijten@...ainline.org, Georgi Djakov <djakov@...nel.org>,
        linux-pm@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 00/12] The great interconnecification fixation

On 14/02/2023 17:27, Konrad Dybcio wrote:
> +CC Georgi, linux-pm, linux-kernel (thanks git send-email for not including these)

Time to switch to b4 prep ;-)

> 
> On 14.02.2023 15:37, Konrad Dybcio wrote:
>> Hi!
>>
>> v3 -> v4 changelog:
>> - Drop "Always set QoS params on QNoC", it only causes issues.. this
>>    can be investigated another day, as it's not necessary for operation
>>
>> - Drop "Add a way to always set QoS registers", same as /\
>>
>> - Add a way (and use it) to have no bus_clocks (the ones we set rate on),
>>    as at least msm8996 has a bus (A0NoC) that doesn't have any and does
>>    all the scaling through RPM requests
>>
>> - Promote 8996 icc to core_initcall
>>
>> - Introduce keep_alive (see patch [11/12]) (important!, will be used by at least 6375)
>>
>> - Allow negative QoS offsets in preparation for introducing 8998 icc [12/12]
>>
>> Link to v3: https://lore.kernel.org/linux-arm-msm/20230116132152.405535-1-konrad.dybcio@linaro.org/
>>
>> v2 -> v3 changelog:
>> - Drop "Don't set QoS params before non-zero bw is requested"
>>
>> - Rebase on next
>>
>> - [1/9] ("..make QoS INVALID default.."): remove unused define for
>>    MODE_INVALID_VAL
>>
>> - Pick up tags
>>
>> v1 -> v2 changelog:
>> - reorder "make QoS INVALID default", makes more sense to have it
>>    before "Always set QoS params on QNoC"
>>
>> - Limit ap_owned-independent QoS setting to QNoC only
>>
>> - Add new patches for handling the 8996-and-friends clocks situation
>>    and optional BIMC regardless-of-ap_owned QoS programming
>>
>>
>> [1] https://lore.kernel.org/linux-arm-msm/14e06574-f95e-8960-0243-8c95a1c294e9@linaro.org/T/#m056692bea71d4c272968d5e07afbd9eb07a88123
>> [2] https://lore.kernel.org/linux-arm-msm/20230110132202.956619-1-konrad.dybcio@linaro.org/
>>
>> This series grew quite a bit bigger than the previous [1] attempt, so
>> I decided to also add a cover letter.
>>
>> Link to v2: [2]
>>
>> It addresses a few things that were not quite right:
>>
>> - Setting QoS params before a "real" (non-zero) bandwidth request
>>    makes little sense (since there's no data supposed to flow through
>>    the bus, why would the QoS matter) and (at least newer) downstream
>>    prevents that from happening. Do the same in Patch 1.
>>
>> - QNoC type buses expect to always have their QoS registers set as long
>>    as there's a non-INVALID QoS mode set; ap_owned is not really a thing
>>    on these anymore, Patch 3 handles that.
>>
>> - The recent MSM8996 boot fix was done quickly and not quite properly,
>>    leading to possibly setting the aggregate bus rate on "normal"
>>    hardware interface clocks; this series handles that by limiting the
>>    number of bus_clocks to 2 (which is the maximum that makes sense,
>>    anyway) and handling the rest as "intf_clocks", which are required
>>    to access the   hardware at the other end. Patches 5-8 take care of
>>    that and Patch 10 reverts the _optional moniker in clk_get_ to make
>>    sure we always have the bus scaling clocks, as they're well, kind
>>    of important ;)
>>
>> - Similarly to QNoC, BIMC on "newer" (which can be loosely approximated
>>    by "new enough" == "has only BIMC and QNoC hosts") SoCs expects to
>>    always receive QoS programming, whereas BIMC on "older" SoCs cries
>>    like a wild boar and crashes the platform when trying to do so
>>    unconditionally. Patch 9 adds a way to take care of that for newer
>>    SoCs (like SM6375)
>>
>> - QoS mode INVALID was assumed by developers before to be the default
>>    ("I didn't specify any QoS settings, so the driver can't assume I
>>    did.. right? right!?" - wrong, partial struct initialization led to
>>    0 being set and 0 corresponded to QoS mode FIXED). Make it so, as
>>    that's the logical choice. This allows the "Always set QoS params
>>    on QNoC" patch to work without setting tons of what-should-
>>    -obviously-be-the-default values everywhere, as well as fixes older
>>    drivers that set ap_owned = true but left the QoS mode field unset.
>>    Patch 2 cleans that up.
>>
>> - Some nodes are physically connected over more than one channel
>>    (usually DDR or other high-throughput paths). Patch 4 allows that
>>    to be reflected in calculations. This will be required for at least
>>    MSM8998 and SM6375 (which will be submitted soon after this lands)
>>
>>
>> Konrad Dybcio (12):
>>    interconnect: qcom: rpm: make QoS INVALID default, separate out driver
>>      data
>>    interconnect: qcom: rpm: Add support for specifying channel num
>>    interconnect: qcom: Sort kerneldoc entries
>>    interconnect: qcom: rpm: Rename icc desc clocks to bus_blocks
>>    interconnect: qcom: rpm: Rename icc provider num_clocks to
>>      num_bus_clocks
>>    interconnect: qcom: rpm: Handle interface clocks
>>    interconnect: qcom: icc-rpm: Allow negative num_bus_clocks
>>    interconnect: qcom: msm8996: Specify no bus clock scaling on A0NoC
>>    interconnect: qcom: rpm: Don't use clk_get_optional for bus clocks
>>      anymore
>>    interconnect: qcom: msm8996: Promote to core_initcall
>>    interconnect: qcom: icc-rpm: Introduce keep_alive
>>    interconnect: qcom: icc-rpm: Allow negative QoS offset
>>
>>   drivers/interconnect/qcom/icc-rpm.c | 101 ++++++++++++++++++++--------
>>   drivers/interconnect/qcom/icc-rpm.h |  41 +++++++----
>>   drivers/interconnect/qcom/msm8996.c |  35 ++++++----
>>   drivers/interconnect/qcom/sdm660.c  |  16 ++---
>>   4 files changed, 126 insertions(+), 67 deletions(-)
>>

Powered by blists - more mailing lists