mbox series

[v3,00/23] Restructure RPM SMD ICC

Message ID 20230526-topic-smd_icc-v3-0-5fb7d39b874f@linaro.org
Headers show
Series Restructure RPM SMD ICC | expand

Message

Konrad Dybcio June 12, 2023, 6:24 p.m. UTC 
This series reshuffles things around, moving the management of SMD RPM
bus clocks to the interconnect framework where they belong. This helps
us solve a couple of issues:

1. We can work towards unused clk cleanup of RPMCC without worrying
   about it killing some NoC bus, resulting in the SoC dying.
   Deasserting actually unused RPM clocks (among other things) will
   let us achieve "true SoC-wide power collapse states", also known as
   VDD_LOW and VDD_MIN.

2. We no longer have to keep tons of quirky bus clock ifs in the icc
   driver. You either have a RPM clock and call "rpm set rate" or you
   have a single non-RPM clock (like AHB_CLK_SRC) or you don't have any.

3. There's less overhead - instead of going through layers and layers of
   the CCF, ratesetting comes down to calling max() and sending a single
   RPM message. ICC is very very dynamic so that's a big plus.

The clocks still need to be vaguely described in the clk-smd-rpm driver,
as it gives them an initial kickoff, before actually telling RPM to
enable DVFS scaling.  After RPM receives that command, all clocks that
have not been assigned a rate are considered unused and are shut down
in hardware, leading to the same issue as described in point 1.

We can consider marking them __initconst in the future, but this series
is very fat even without that..

Apart from that, it squashes a couple of bugs that really need fixing..

--- MERGING STRATEGY ---
If Stephen and Georgi agree, it would be best to take all of this through
the qcom tree, as it touches on heavily intertwined components and
introduces compile-time dependencies between icc and clk drivers.

Tested on SM6375 (OOT), MSM8998 (OOT), MSM8996.

MSM8974 conversion to common code and modernization will be handled separately.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Changes in v3:
- Use devm_clk_get_optional and only get() the clock once
- Drop unnecessary NULL-checks for qp->bus_clk
- Handle ARM32 CCF limitations, add an explicit comment about them
- Use Stephan's alternative rpmcc readiness check
- Fix one more wrong usage of QCOM_ICC_NUM_BUCKETS in icc-rpm.h
- Introduce new dt-bindings for icc rpm tags
- Mention the rpm tags situation in the commit message of
  "Fix bucket number"
- Pick up tags
- Link to v2: https://lore.kernel.org/r/20230526-topic-smd_icc-v2-0-e5934b07d813@linaro.org

Changes in v2:
- Sort entries properly in "Add missing headers in icc-rpm.h"
- Fix the check for no clocks on a given provider
- Replace "Divide clk rate by src node bus width" with a proper fix
- Add "Set correct bandwidth through RPM bw req"
- Split "Add QCOM_SMD_RPM_STATE_NUM" into 2 logical changes
- Move "Separate out interconnect bus clocks" a bit later in the series
- Link to v1: https://lore.kernel.org/r/20230526-topic-smd_icc-v1-0-1bf8e6663c4e@linaro.org

---
Konrad Dybcio (22):
      dt-bindings: interconnect: Add Qcom RPM ICC bindings
      soc: qcom: smd-rpm: Add QCOM_SMD_RPM_STATE_NUM
      soc: qcom: smd-rpm: Use tabs for defines
      clk: qcom: smd-rpm: Move some RPM resources to the common header
      interconnect: qcom: icc-rpm: Introduce keep_alive
      interconnect: qcom: icc-rpm: Allow negative QoS offset
      interconnect: qcom: Fold smd-rpm.h into icc-rpm.h
      interconnect: qcom: smd-rpm: Add rpmcc handling skeleton code
      interconnect: qcom: Add missing headers in icc-rpm.h
      interconnect: qcom: Define RPM bus clocks
      interconnect: qcom: sdm660: Hook up RPM bus clk definitions
      interconnect: qcom: msm8996: Hook up RPM bus clk definitions
      interconnect: qcom: qcs404: Hook up RPM bus clk definitions
      interconnect: qcom: msm8939: Hook up RPM bus clk definitions
      interconnect: qcom: msm8916: Hook up RPM bus clk definitions
      interconnect: qcom: qcm2290: Hook up RPM bus clk definitions
      interconnect: qcom: icc-rpm: Control bus rpmcc from icc
      clk: qcom: smd-rpm: Separate out interconnect bus clocks
      interconnect: qcom: icc-rpm: Fix bucket number
      interconnect: qcom: icc-rpm: Set bandwidth on both contexts
      interconnect: qcom: icc-rpm: Set correct bandwidth through RPM bw req
      interconnect: qcom: icc-rpm: Fix bandwidth calculations

Stephan Gerhold (1):
      soc: qcom: smd-rpm: Move icc_smd_rpm registration to clk-smd-rpm

 drivers/clk/qcom/clk-smd-rpm.c                  | 312 +++++++++++-------------
 drivers/interconnect/qcom/Makefile              |   2 +-
 drivers/interconnect/qcom/icc-rpm-clocks.c      |  66 +++++
 drivers/interconnect/qcom/icc-rpm.c             | 210 ++++++++--------
 drivers/interconnect/qcom/icc-rpm.h             |  59 ++++-
 drivers/interconnect/qcom/msm8916.c             |   4 +-
 drivers/interconnect/qcom/msm8939.c             |   5 +-
 drivers/interconnect/qcom/msm8974.c             |   2 +-
 drivers/interconnect/qcom/msm8996.c             |   9 +-
 drivers/interconnect/qcom/qcm2290.c             |   7 +-
 drivers/interconnect/qcom/qcs404.c              |   4 +-
 drivers/interconnect/qcom/sdm660.c              |   7 +-
 drivers/interconnect/qcom/smd-rpm.c             |  35 ++-
 drivers/interconnect/qcom/smd-rpm.h             |  15 --
 drivers/soc/qcom/smd-rpm.c                      |  17 +-
 include/dt-bindings/interconnect/qcom,rpm-icc.h |  13 +
 include/linux/soc/qcom/smd-rpm.h                |  20 +-
 17 files changed, 449 insertions(+), 338 deletions(-)
---
base-commit: 53ab6975c12d1ad86c599a8927e8c698b144d669
change-id: 20230526-topic-smd_icc-b8213948a5ed

Best regards,

Comments

Konrad Dybcio June 13, 2023, 9:06 a.m. UTC | #1 
On 12.06.2023 22:57, Stephan Gerhold wrote:
> On Mon, Jun 12, 2023 at 08:24:37PM +0200, Konrad Dybcio wrote:
>> SMD RPM only provides two buckets, one each for the active-only and
>> active-sleep RPM contexts. Use the correct constant to allocate and
>> operate on them.
>>
>> This will make the qcom,icc.h header no longer work with this driver,
>> mostly because.. it was never meant to! The commit that introduced
>> bucket support to SMD RPM was trying to shove a square into a round
>> hole and it did not work out very well. That said, there are no
>> active users of SMD RPM ICC + qcom,icc.h, so that doesn't hurt.
>>
>> Fixes: dcbce7b0a79c ("interconnect: qcom: icc-rpm: Support multiple buckets")
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/interconnect/qcom/icc-rpm.c | 14 +++++++-------
>>  drivers/interconnect/qcom/icc-rpm.h |  4 ++--
>>  2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index 5ffcf5ca8914..54a9999fe55d 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -249,7 +249,7 @@ static void qcom_icc_pre_bw_aggregate(struct icc_node *node)
>>  	size_t i;
>>  
>>  	qn = node->data;
>> -	for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>> +	for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) {
>>  		qn->sum_avg[i] = 0;
>>  		qn->max_peak[i] = 0;
>>  	}
>> @@ -275,7 +275,7 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>>  	if (!tag)
>>  		tag = QCOM_ICC_TAG_ALWAYS;
> 
> We should replace this with the RPM variant. Also, can you check which
> header/file still includes qcom,icc.h? Don't think we should have it
> included at all for RPM so that referencing the wrong things cannot
> happen.
Nice catch

Konrad
> 
> Thanks,
> Stephan
Stephan Gerhold June 13, 2023, 11:11 a.m. UTC | #2 
On Mon, Jun 12, 2023 at 08:24:18PM +0200, Konrad Dybcio wrote:
> The SMD RPM interconnect driver requires different icc tags to the
> RPMh driver. Add bindings to reflect that.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  include/dt-bindings/interconnect/qcom,rpm-icc.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/dt-bindings/interconnect/qcom,rpm-icc.h b/include/dt-bindings/interconnect/qcom,rpm-icc.h
> new file mode 100644
> index 000000000000..ef61006c9c58
> --- /dev/null
> +++ b/include/dt-bindings/interconnect/qcom,rpm-icc.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_RPM_ICC_H
> +#define __DT_BINDINGS_INTERCONNECT_QCOM_RPM_ICC_H
> +
> +#define RPM_ACTIVE_TAG		BIT(0)
> +#define RPM_SLEEP_TAG		BIT(1)

BIT(n) doesn't work in dt-bindings (syntax error), can you change this
to (1 << 0) / (1 << 1)?

Thanks!
Konrad Dybcio June 13, 2023, 11:20 a.m. UTC | #3 
On 13.06.2023 13:11, Stephan Gerhold wrote:
> On Mon, Jun 12, 2023 at 08:24:18PM +0200, Konrad Dybcio wrote:
>> The SMD RPM interconnect driver requires different icc tags to the
>> RPMh driver. Add bindings to reflect that.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  include/dt-bindings/interconnect/qcom,rpm-icc.h | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/include/dt-bindings/interconnect/qcom,rpm-icc.h b/include/dt-bindings/interconnect/qcom,rpm-icc.h
>> new file mode 100644
>> index 000000000000..ef61006c9c58
>> --- /dev/null
>> +++ b/include/dt-bindings/interconnect/qcom,rpm-icc.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>> +/*
>> + * Copyright (c) 2023, Linaro Limited
>> + */
>> +
>> +#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_RPM_ICC_H
>> +#define __DT_BINDINGS_INTERCONNECT_QCOM_RPM_ICC_H
>> +
>> +#define RPM_ACTIVE_TAG		BIT(0)
>> +#define RPM_SLEEP_TAG		BIT(1)
> 
> BIT(n) doesn't work in dt-bindings (syntax error), can you change this
> to (1 << 0) / (1 << 1)?
Just including the binding did not show it.. You're right..

Konrad
> 
> Thanks!
>