Re: [PATCH v8 0/8] The great interconnecification fixation

[Konrad Dybcio]

On 7.04.2023 22:14, Konrad Dybcio wrote:
> Hi!
Bump!

Konrad
> 
> v7 -> v8:
> - Rebase (dropping has_bus_pd, picked patches from v7)
> 
> - Clean up the QoS-setting functions [3/8]
> 
> - Only set the QoS registers once [4/8] - Georgi talked to some
>   Qualcomm folks and we concluded that it's "good enough" as they
>   should:tm: persist until a full reboot on "almost all" platforms
> 
> - Move the intf clock enabling/disabling to correspond with /\ [5/8]
> 
> - DO NOT switch to associating an interface clock with a given node
>   (instead of a provider), as it makes little sense with the changes
>   with [4/8] and the new iteration of [5/8]
> 
> v7: https://lore.kernel.org/r/20230228-topic-qos-v7-0-815606092fff@xxxxxxxxxx
> 
> v6 -> v7 changelog:
> - Rebase on Johan's recent patches
> 
> Link to v6: https://lore.kernel.org/r/20230228-topic-qos-v6-0-3c37a349656f@xxxxxxxxxx
> 
> v5 -> v6 changelog:
> - Completely rewrite the commit message of [1/9], I realized that there
>   was actually no issue with the present upstream setups and the only
>   drivers suffering from ghost votes were.. my own OOT drivers..
>   As a consequence of that, all fixes tags were dropped and the patch
>   has been kept, since it was deemed useful for newer SoCs that don't
>   distinguish ap_owned nodes.
> 
> - Change the number of allowed bus_clocks from (0-2 in the previous
>   revision, 0-inf in the current upstream state) to {0, 2}. Scaling is
>   only possible with a pair of wake-sleep clocks, but some providers
>   don't do scaling at all (see 8996 A0NoC, 660 GNoC). Drop the cheeky
>   -1 / 0 / >0 checks from the previous revision. [7/9]
> 
> - bus_clocks are now forced to be named "bus", "bus_a", as there is no
>   need for variance here - we don't do scaling on non-SMD RPM bus clocks.
>   [7/9]
> 
> - The interface clocks are now only turned on when the associated bus
>   is running at a non-zero frequency [6/9] instead of being always on
>   and leaking power
> 
> Tested on MSM8996 Kagura, SM6375 PDX225 (OOT), MSM8998 Maple (OOT)
> 
> Link to v5: https://lore.kernel.org/linux-arm-msm/20230217-topic-icc-fixes-v5-v5-0-c9a550f9fdb9@xxxxxxxxxx/
> 
> v4 -> v5 changelog:
> - Previously the "Always set QoS params on QNoC" contained part of what
>   should have been included in "make QoS INVALID default".. (very bad)
>   Fix it!
> 
> - Drop negative offset and keep_alive, they will be resubmitted with new
>   icc driver submissions
> 
> - use b4 this time.. hopefully the series gets to everybody now
> 
> Link to v4: https://lore.kernel.org/linux-arm-msm/20230214143720.2416762-1-konrad.dybcio@xxxxxxxxxx/
> 
> 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@xxxxxxxxxx/
> 
> 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@xxxxxxxxxx/T/#m056692bea71d4c272968d5e07afbd9eb07a88123
> [2] https://lore.kernel.org/linux-arm-msm/20230110132202.956619-1-konrad.dybcio@xxxxxxxxxx/
> 
> 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)
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> ---
> Konrad Dybcio (8):
>       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: Drop unused parameters
>       interconnect: qcom: rpm: Set QoS registers only once
>       interconnect: qcom: rpm: Handle interface clocks
>       interconnect: qcom: icc-rpm: Enforce 2 or 0 bus clocks
>       interconnect: qcom: rpm: Don't use clk_get_optional for bus clocks anymore
>       interconnect: qcom: msm8996: Promote to core_initcall
> 
>  drivers/interconnect/qcom/icc-rpm.c | 110 +++++++++++++++++++-----------------
>  drivers/interconnect/qcom/icc-rpm.h |  22 ++++++--
>  drivers/interconnect/qcom/msm8996.c |  35 +++++++-----
>  drivers/interconnect/qcom/sdm660.c  |  17 +++---
>  4 files changed, 102 insertions(+), 82 deletions(-)
> ---
> base-commit: e134c93f788fb93fd6a3ec3af9af850a2048c7e6
> change-id: 20230228-topic-qos-5435cac88d89
> 
> Best regards,