| Message ID | 20230526-topic-smd_icc-v6-0-263283111e66@linaro.org |
|---|---|
| Headers | show |
| Series | Restructure RPM SMD ICC | expand |
Quoting Konrad Dybcio (2023-06-14 11:04:37) > The interconnect bus clocks are now handled within the ICC framework. > They still however need to get a kickstart *before* we call > clk_smd_rpm_enable_scaling(), or RPM will assume that they should all > be running at 0 kHz and the system will inevitably die. > > Separate them out to ensure such a kickstart can still take place. > > As a happy accident, the file got smaller: > > Total: Before=41951, After=41555, chg -0.94% > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/clk/qcom/clk-smd-rpm.c | 278 +++++++++++++++++------------------------ > 1 file changed, 115 insertions(+), 163 deletions(-) > > diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c > index 6e7f0438e8b8..0d1d97659d59 100644 > --- a/drivers/clk/qcom/clk-smd-rpm.c > +++ b/drivers/clk/qcom/clk-smd-rpm.c > @@ -498,13 +506,69 @@ DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk1, 11, 19200000); > DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk2, 12, 19200000); > DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk3, 13, 19200000); > > +static struct clk_smd_rpm *bimc_pcnoc_icc_clks[] = { Can these be const arrays? > + &clk_smd_rpm_bimc_clk, > + &clk_smd_rpm_bus_0_pcnoc_clk, > +}; > + [...] > @@ -1332,6 +1275,15 @@ static int rpm_smd_clk_probe(struct platform_device *pdev) > goto err; > } > > + for (i = 0; i < desc->num_icc_clks; i++) { > + if (!desc->icc_clks[i]) > + continue; > + > + ret = clk_smd_rpm_handoff(desc->icc_clks[i]); This API can probably take a const struct clk_smd_rpm pointer as well.
Quoting Konrad Dybcio (2023-06-14 11:04:19) > 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. Why can't we move the enable of DVFS scaling call to the interconnect driver as well? We want the clk driver to not reference the interconnect resources at all.
On 15.06.2023 02:48, Stephen Boyd wrote: > Quoting Konrad Dybcio (2023-06-14 11:04:37) >> The interconnect bus clocks are now handled within the ICC framework. >> They still however need to get a kickstart *before* we call >> clk_smd_rpm_enable_scaling(), or RPM will assume that they should all >> be running at 0 kHz and the system will inevitably die. >> >> Separate them out to ensure such a kickstart can still take place. >> >> As a happy accident, the file got smaller: >> >> Total: Before=41951, After=41555, chg -0.94% >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> drivers/clk/qcom/clk-smd-rpm.c | 278 +++++++++++++++++------------------------ >> 1 file changed, 115 insertions(+), 163 deletions(-) >> >> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c >> index 6e7f0438e8b8..0d1d97659d59 100644 >> --- a/drivers/clk/qcom/clk-smd-rpm.c >> +++ b/drivers/clk/qcom/clk-smd-rpm.c >> @@ -498,13 +506,69 @@ DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk1, 11, 19200000); >> DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk2, 12, 19200000); >> DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk3, 13, 19200000); >> >> +static struct clk_smd_rpm *bimc_pcnoc_icc_clks[] = { > > Can these be const arrays? Sorta, look below. > >> + &clk_smd_rpm_bimc_clk, >> + &clk_smd_rpm_bus_0_pcnoc_clk, >> +}; >> + > [...] >> @@ -1332,6 +1275,15 @@ static int rpm_smd_clk_probe(struct platform_device *pdev) >> goto err; >> } >> >> + for (i = 0; i < desc->num_icc_clks; i++) { >> + if (!desc->icc_clks[i]) >> + continue; >> + >> + ret = clk_smd_rpm_handoff(desc->icc_clks[i]); > > This API can probably take a const struct clk_smd_rpm pointer as well. It would be great, but ideally we want to set r->enabled in that func for !bus clocks (for unused clk cleanup). I could probably introduce a separate function for the icc clocks that takes const. Konrad
On 15.06.2023 02:49, Stephen Boyd wrote: > Quoting Konrad Dybcio (2023-06-14 11:04:19) >> 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. > > Why can't we move the enable of DVFS scaling call to the interconnect > driver as well? We want the clk driver to not reference the interconnect > resources at all. That would result in no rpmcc ratesetting on platforms without a functional interconnect driver. The DVFS call concerns both bus and !bus clocks. Konrad
Quoting Konrad Dybcio (2023-06-15 00:52:07) > On 15.06.2023 02:49, Stephen Boyd wrote: > > Quoting Konrad Dybcio (2023-06-14 11:04:19) > >> 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. > > > > Why can't we move the enable of DVFS scaling call to the interconnect > > driver as well? We want the clk driver to not reference the interconnect > > resources at all. > That would result in no rpmcc ratesetting on platforms without a functional > interconnect driver. The DVFS call concerns both bus and !bus clocks. > That's the intent. Probe the interconnect driver to get bus clk rate setting. What are the !bus clocks managed by RPM?
On 15.06.2023 19:35, Stephen Boyd wrote: > Quoting Konrad Dybcio (2023-06-15 00:52:07) >> On 15.06.2023 02:49, Stephen Boyd wrote: >>> Quoting Konrad Dybcio (2023-06-14 11:04:19) >>>> 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. >>> >>> Why can't we move the enable of DVFS scaling call to the interconnect >>> driver as well? We want the clk driver to not reference the interconnect >>> resources at all. >> That would result in no rpmcc ratesetting on platforms without a functional >> interconnect driver. The DVFS call concerns both bus and !bus clocks. >> > > That's the intent. Probe the interconnect driver to get bus clk rate > setting. > > What are the !bus clocks managed by RPM? Depending on the platform, that includes IPA, GPU, OCMEM, RF.. everything that's not been separated out in patch 18. Konrad
Quoting Konrad Dybcio (2023-06-15 00:49:59) > On 15.06.2023 02:48, Stephen Boyd wrote: > > Quoting Konrad Dybcio (2023-06-14 11:04:37) > >> @@ -1332,6 +1275,15 @@ static int rpm_smd_clk_probe(struct platform_device *pdev) > >> goto err; > >> } > >> > >> + for (i = 0; i < desc->num_icc_clks; i++) { > >> + if (!desc->icc_clks[i]) > >> + continue; > >> + > >> + ret = clk_smd_rpm_handoff(desc->icc_clks[i]); > > > > This API can probably take a const struct clk_smd_rpm pointer as well. > It would be great, but ideally we want to set r->enabled in that func > for !bus clocks (for unused clk cleanup). I could probably introduce a > separate function for the icc clocks that takes const. > Sure. Or have this take const and in the clk driver set r->enabled after calling it. That's future code though, so just make it take const?
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 v6: - Fix argument naming in "Add rpmcc handling skeleton code" - Fix missing clk.h and reorder patch "Add missing headers in icc-rpm.h", drop Dmitry's rb - Pick up tags - Link to v5: https://lore.kernel.org/r/20230526-topic-smd_icc-v5-0-eeaa09d0082e@linaro.org Changes in v5: - Pass RPM context id to qcom_icc_rpm_set_bus_rate() - Fix min_t call cutting off bits 32-63 in set() path - Pick up tags - Link to v4: https://lore.kernel.org/r/20230526-topic-smd_icc-v4-0-5ba82b6fbba2@linaro.org Changes in v4: - Only set clk rate on a context if necessary - Mention qcom,icc.h is not the correct header in "Control bus rpmcc form icc" - Fix the bindings (BIT vs 1<<) - Fix one more wrong use of qcom,icc.h in "Fix bucket numer" and uninclude it - Drop "Allow negative QoS offset" (will be handled separately) - Export icc clocks descriptions to unbreak =m builds - Pick up tags - Link to v3: https://lore.kernel.org/r/20230526-topic-smd_icc-v3-0-5fb7d39b874f@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 (21): 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: Add missing headers in icc-rpm.h interconnect: qcom: Fold smd-rpm.h into icc-rpm.h interconnect: qcom: smd-rpm: Add rpmcc handling skeleton code 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 | 77 ++++++ drivers/interconnect/qcom/icc-rpm.c | 220 +++++++++-------- drivers/interconnect/qcom/icc-rpm.h | 56 ++++- drivers/interconnect/qcom/msm8916.c | 5 +- drivers/interconnect/qcom/msm8939.c | 6 +- drivers/interconnect/qcom/msm8974.c | 2 +- drivers/interconnect/qcom/msm8996.c | 10 +- drivers/interconnect/qcom/qcm2290.c | 8 +- drivers/interconnect/qcom/qcs404.c | 5 +- drivers/interconnect/qcom/sdm660.c | 8 +- drivers/interconnect/qcom/smd-rpm.c | 23 +- 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, 455 insertions(+), 344 deletions(-) --- base-commit: b16049b21162bb649cdd8519642a35972b7910fe change-id: 20230526-topic-smd_icc-b8213948a5ed Best regards,