mbox series

[v6,00/10] Add power domain driver for corners on msm8996/sdm845

Message ID 20181211094938.25086-1-rnayak@codeaurora.org
Headers show
Series Add power domain driver for corners on msm8996/sdm845 | expand

Message

Rajendra Nayak Dec. 11, 2018, 9:49 a.m. UTC
Changes in v6:
* OPP binding updates for qcom,level reviewed by Rob
* DT bindings for rpmpd and rpmhpd updated to specify the
OPP tables as child nodes of the power-controller itself
* Removed some module specific remains from the drivers,
now that they can only be built-in
* Added a simple_opp_to_performance_state() helper

Changes in v5:
* First 6 patches are unchanged
* Patch 7/8 adds the DT node for rpmh power-controller on sdm845 and the
corresponding OPP tables for it to describe the performance states
* Patch 8/8 adds a parent/child relationship across mx/cx and mx_ao/cx_ao
as needed on sdm845 platform. This patch is dependent on the series from
Viresh [1] which adds support to propogate performance states across the
power domain hierarchy which is still being reviewed

Changes in v4:
* Included the patch to add qcom-opp bindings (dropped accidentally in v3)
* merged the patches to add bindings for rpm and rpmh, added consumer binding example
* Made the drivers built in, removed .remove
* Added better description in changelog for PATCH 6/6
* Updated rpmhpd_aggregate_corner() based on Davids feedback
* rpmhpd_set_performance_state() returns max corner, in cases where its called
with an INT_MAX
* Dropped the patch to max vote on all corners at init, the patch did not
work anyway, and it shouldn't be needed now

Changes in v3:
* Bindings split into seperate patches
* Bindings updated to remove duplicate OPP table phandles
* DT headers defining macros for Power domain indexes and OPP levels
* Optimisations to use rpmh_write_async() whereever applicable
* Fixed up handling of ACTIVE_ONLY/WAKE_ONLY/SLEEP voting for RPMh
* Fixed the vlvl to hlvl conversions in set_performance
* Other minor fixes based on review of v2
* TODO: This series does not handle the case where all VDD_MX votes
should be higher than VDD_CX from APPs, as pointed out
by David Collins in v2. This needs support at genpd to propogate performance
state up the parents, if we model these as Parent/Child to handle the
interdependency.

Changes in v2:
* added a power domain driver for sdm845 which supports communicating to RPMh
* dropped the changes to sdhc driver to move over to using OPP
as there is active discussion on using OPP as the interface vs
handling all of it in clock drivers
* Other minor binding updates based on review of v1

With performance state support for genpd/OPP merged, this is an effort
to model a power domain driver to communicate corner/level
values for qualcomm platforms to RPM (Remote Power Manager) and RPMh.

[1] https://lkml.org/lkml/2018/11/26/333

Rajendra Nayak (10):
  dt-bindings: opp: Introduce qcom-opp bindings
  dt-bindings: power: Add qcom rpm power domain driver bindings
  soc: qcom: rpmpd: Add a Power domain driver to model corners
  soc: qcom: rpmpd: Add support for get/set performance state
  arm64: dts: msm8996: Add rpmpd device node
  soc: qcom: rpmhpd: Add RPMh power domain driver
  arm64: dts: sdm845: Add rpmh powercontroller node
  PM / Domains: Add a simple_opp_to_performance_state() helper
  soc: qcom: rpmpd: Use simple_opp_to_performance_state() helper
  soc: qcom: rpmhpd: Mark mx as a parent for cx

 .../devicetree/bindings/opp/qcom-opp.txt      |  25 ++
 .../devicetree/bindings/power/qcom,rpmpd.txt  | 146 ++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |  34 ++
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  51 +++
 drivers/base/power/domain.c                   |  17 +
 drivers/soc/qcom/Kconfig                      |  18 +
 drivers/soc/qcom/Makefile                     |   2 +
 drivers/soc/qcom/rpmhpd.c                     | 419 ++++++++++++++++++
 drivers/soc/qcom/rpmpd.c                      | 317 +++++++++++++
 include/dt-bindings/power/qcom-rpmpd.h        |  39 ++
 include/linux/pm_domain.h                     |   9 +
 11 files changed, 1077 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/opp/qcom-opp.txt
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
 create mode 100644 drivers/soc/qcom/rpmhpd.c
 create mode 100644 drivers/soc/qcom/rpmpd.c
 create mode 100644 include/dt-bindings/power/qcom-rpmpd.h

Comments

Ulf Hansson Dec. 11, 2018, 10:22 a.m. UTC | #1
On Tue, 11 Dec 2018 at 10:50, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
> Specify the active + sleep and active-only MX power domains as
> the parents of the corresponding CX power domains. This will ensure that
> performance state requests on CX automatically generate equivalent requests
> on MX power domains.
>
> This is used to enforce a requirement that exists for various
> hardware blocks on SDM845 that MX performance state >= CX performance
> state for a given operating frequency.

I assume that also means the MX power domain must not be power off as
long as the CX power domain is powered on?

Just to make sure there are no conflicting hierarchical constraints
between idle management and performance state management!?

Kind regards
Uffe

>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> This patch is dependent on the series from
> Viresh [1] which adds support to propogate performance states across the
> power domain hierarchy which is still being reviewed.
>
> [1] https://lkml.org/lkml/2018/11/26/333
>
>  drivers/soc/qcom/rpmhpd.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> index 1ce86f0cc9fa..af7b07d49e4f 100644
> --- a/drivers/soc/qcom/rpmhpd.c
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -102,12 +102,14 @@ static struct rpmhpd sdm845_cx_ao;
>  static struct rpmhpd sdm845_cx = {
>         .pd = { .name = "cx", },
>         .peer = &sdm845_cx_ao,
> +       .parent = &sdm845_mx.pd,
>         .res_name = "cx.lvl",
>  };
>
>  static struct rpmhpd sdm845_cx_ao = {
>         .pd = { .name = "cx_ao", },
>         .peer = &sdm845_cx,
> +       .parent = &sdm845_mx_ao.pd,
>         .res_name = "cx.lvl",
>  };
>
> @@ -389,6 +391,15 @@ static int rpmhpd_probe(struct platform_device *pdev)
>                 data->domains[i] = &rpmhpds[i]->pd;
>         }
>
> +       /* Add subdomains */
> +       for (i = 0; i < num_pds; i++) {
> +               if (!rpmhpds[i])
> +                       continue;
> +               if (rpmhpds[i]->parent)
> +                       pm_genpd_add_subdomain(rpmhpds[i]->parent,
> +                                              &rpmhpds[i]->pd);
> +       }
> +
>         return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>  }
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
Rajendra Nayak Dec. 11, 2018, 10:33 a.m. UTC | #2
On 12/11/2018 3:52 PM, Ulf Hansson wrote:
> On Tue, 11 Dec 2018 at 10:50, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>> Specify the active + sleep and active-only MX power domains as
>> the parents of the corresponding CX power domains. This will ensure that
>> performance state requests on CX automatically generate equivalent requests
>> on MX power domains.
>>
>> This is used to enforce a requirement that exists for various
>> hardware blocks on SDM845 that MX performance state >= CX performance
>> state for a given operating frequency.
> 
> I assume that also means the MX power domain must not be power off as
> long as the CX power domain is powered on?

So with rpmh, there's really no separate on/off control, we just put
it in the lowest perf state at off.

> 
> Just to make sure there are no conflicting hierarchical constraints
> between idle management and performance state management!?
> 
> Kind regards
> Uffe
> 
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>> This patch is dependent on the series from
>> Viresh [1] which adds support to propogate performance states across the
>> power domain hierarchy which is still being reviewed.
>>
>> [1] https://lkml.org/lkml/2018/11/26/333
>>
>>   drivers/soc/qcom/rpmhpd.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> index 1ce86f0cc9fa..af7b07d49e4f 100644
>> --- a/drivers/soc/qcom/rpmhpd.c
>> +++ b/drivers/soc/qcom/rpmhpd.c
>> @@ -102,12 +102,14 @@ static struct rpmhpd sdm845_cx_ao;
>>   static struct rpmhpd sdm845_cx = {
>>          .pd = { .name = "cx", },
>>          .peer = &sdm845_cx_ao,
>> +       .parent = &sdm845_mx.pd,
>>          .res_name = "cx.lvl",
>>   };
>>
>>   static struct rpmhpd sdm845_cx_ao = {
>>          .pd = { .name = "cx_ao", },
>>          .peer = &sdm845_cx,
>> +       .parent = &sdm845_mx_ao.pd,
>>          .res_name = "cx.lvl",
>>   };
>>
>> @@ -389,6 +391,15 @@ static int rpmhpd_probe(struct platform_device *pdev)
>>                  data->domains[i] = &rpmhpds[i]->pd;
>>          }
>>
>> +       /* Add subdomains */
>> +       for (i = 0; i < num_pds; i++) {
>> +               if (!rpmhpds[i])
>> +                       continue;
>> +               if (rpmhpds[i]->parent)
>> +                       pm_genpd_add_subdomain(rpmhpds[i]->parent,
>> +                                              &rpmhpds[i]->pd);
>> +       }
>> +
>>          return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>>   }
>>
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
Stephen Boyd Dec. 11, 2018, 9:33 p.m. UTC | #3
Quoting Rajendra Nayak (2018-12-11 01:49:36)
> Now that we have atleast 2 genpd providers, both using a simple
> routine to read a performance state value from device tree and
> return it, in order to implement the .opp_to_performance_state
> callback, add a simple_opp_to_performance_state() helper to do
> it, so it can be resued across all such genpd providers which
> just need to read the value from DT.
> 
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Stephen Boyd Dec. 11, 2018, 9:34 p.m. UTC | #4
Quoting Rajendra Nayak (2018-12-11 01:49:37)
> Get rid of the duplicate code across rpmpd and rpmhpd to read the
> performance state value from Device tree and use the
> simple_opp_to_performance_state() helper instead.
> 
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Stephen Boyd Dec. 11, 2018, 9:50 p.m. UTC | #5
Quoting Rajendra Nayak (2018-12-11 02:33:23)
> 
> 
> On 12/11/2018 3:52 PM, Ulf Hansson wrote:
> > On Tue, 11 Dec 2018 at 10:50, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> >>
> >> Specify the active + sleep and active-only MX power domains as
> >> the parents of the corresponding CX power domains. This will ensure that
> >> performance state requests on CX automatically generate equivalent requests
> >> on MX power domains.
> >>
> >> This is used to enforce a requirement that exists for various
> >> hardware blocks on SDM845 that MX performance state >= CX performance
> >> state for a given operating frequency.
> > 
> > I assume that also means the MX power domain must not be power off as
> > long as the CX power domain is powered on?
> 
> So with rpmh, there's really no separate on/off control, we just put
> it in the lowest perf state at off.

I think in theory the answer is MX can't be off if CX is on, but in
reality, MX and CX are never turned off, just set to something really
low and even then the constraint applies for MX >= CX. Is that right?

> 
> > 
> > Just to make sure there are no conflicting hierarchical constraints
> > between idle management and performance state management!?
> > 

I'm not sure what idle states mean to the CX and MX domains. Would it be
some sort of idle state governor attached at genpd creation time that
would adjust the main SoC power rails when all devices attached are
idle? Maybe I don't understand how idle states are different from
performance states.

My understanding is that devices using these domains would almost always
expect their clk frequency and clk on/off state to decide what the
performance state is, unless they need to ignore clk state because they
aren't managing clks and bump up the voltage directly when the device is
active. Either way, devices are actively managing the voltage they need
these voltage domains to operate at by using the genpd performance
states APIs.
Stephen Boyd Dec. 11, 2018, 9:51 p.m. UTC | #6
Quoting Rajendra Nayak (2018-12-11 01:49:28)
> Changes in v6:
> * OPP binding updates for qcom,level reviewed by Rob
> * DT bindings for rpmpd and rpmhpd updated to specify the
> OPP tables as child nodes of the power-controller itself
> * Removed some module specific remains from the drivers,
> now that they can only be built-in
> * Added a simple_opp_to_performance_state() helper

Is the idea that it can be squashed into the patches that use it by
reordering the series? But otherwise left at the end of the series to
make things simpler to merge without it?
Stephen Boyd Dec. 11, 2018, 9:52 p.m. UTC | #7
Quoting Rajendra Nayak (2018-12-11 01:49:31)
> The Power domains for corners just pass the performance state set by the
> consumers to the RPM (Remote Power manager) which then takes care
> of setting the appropriate voltage on the corresponding rails to
> meet the performance needs.
> 
> We add all power domain data needed on msm8996 here. This driver can easily
> be extended by adding data for other qualcomm SoCs as well.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Stephen Boyd Dec. 11, 2018, 9:53 p.m. UTC | #8
Quoting Rajendra Nayak (2018-12-11 01:49:33)
> Add rpmpd device node and its OPP table
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Stephen Boyd Dec. 11, 2018, 10:01 p.m. UTC | #9
Quoting Rajendra Nayak (2018-12-11 01:49:34)
> The RPMh power domain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
> 
> With RPMh we use 2 different numbering space for corners, one used
> by the clients to express their performance needs, and another used
> to communicate to RPMh hardware.
> 
> The clients express their performance requirements using a sparse
> numbering space which are mapped to meaningful levels like RET, SVS,
> NOMINAL, TURBO etc which then get mapped to another number space
> between 0 and 15 which is communicated to RPMh. The sparse number space,
> also referred to as vlvl is mapped to the continuous number space of 0
> to 15, also referred to as hlvl, using command DB.
> 
> Some power domain clients could request a performance state only while
> the CPU is active, while some others could request for a certain
> performance state all the time regardless of the state of the CPU.
> We handle this by internally aggregating the votes from both type of
> clients and then send the aggregated votes to RPMh.
> 
> There are also 3 different types of Votes that are comunicated to RPMh

Why capitalize vote?

> for every resource.
> 1. ACTIVE_ONLY: This specifies the requirement for the resource when the
> CPU is active
> 2. SLEEP: This specifies the requirement for the resource when the CPU
> is going to sleep
> 3. WAKE_ONLY: This specifies the requirement for the resource when the
> CPU is coming out of sleep to active state

Can you tab these in?

> 
> We add data for all power domains on sdm845 SoC as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Just minor nitpicks ahead and a warning.

> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index f1b25fdcf2ad..dd6ca92985ee 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -22,3 +22,4 @@ obj-$(CONFIG_QCOM_APR) += apr.o
>  obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
>  obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o

Put this before RPMPD? At least it would be semi-sorted then.

> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> new file mode 100644
> index 000000000000..f993a86be48c
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -0,0 +1,417 @@
[..]
> +
> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
> +{
> +       u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];
> +       int i, j, len, ret;
> +
> +       len = cmd_db_read_aux_data_len(rpmhpd->res_name);
> +       if (len <= 0)
> +               return len;
> +       else if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
> +               return -EINVAL;
> +
> +       ret = cmd_db_read_aux_data(rpmhpd->res_name, buf, len);
> +       if (ret < 0)
> +               return ret;

I've changed cmd_db_read_aux_data() and that change is winding through
the arm-soc tree. This will break in the compilation phase in
linux-next.

> +
> +       rpmhpd->level_count = len / RPMH_ARC_LEVEL_SIZE;
> +
> +       for (i = 0; i < rpmhpd->level_count; i++) {
> +               rpmhpd->level[i] = 0;
> +               for (j = 0; j < RPMH_ARC_LEVEL_SIZE; j++)
> +                       rpmhpd->level[i] |=
> +                               buf[i * RPMH_ARC_LEVEL_SIZE + j] << (8 * j);
> +
> +               /*
> +                * The AUX data may be zero padded.  These 0 valued entries at
> +                * the end of the map must be ignored.
> +                */
> +               if (i > 0 && rpmhpd->level[i] == 0) {
> +                       rpmhpd->level_count = i;
> +                       break;
> +               }
> +               pr_debug("%s: ARC hlvl=%2d --> vlvl=%4u\n", rpmhpd->res_name, i,
> +                        rpmhpd->level[i]);
> +       }
> +
> +       return 0;
> +}
> +
> +static int rpmhpd_probe(struct platform_device *pdev)
> +{
> +       int i, ret;
> +       size_t num_pds;
> +       struct device *dev = &pdev->dev;
> +       struct genpd_onecell_data *data;
> +       struct rpmhpd **rpmhpds;
> +       const struct rpmhpd_desc *desc;
> +
> +       desc = of_device_get_match_data(dev);
> +       if (!desc)
> +               return -EINVAL;
> +
> +       rpmhpds = desc->rpmhpds;
> +       num_pds = desc->num_pds;
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->domains = devm_kcalloc(dev, num_pds, sizeof(*data->domains),
> +                                    GFP_KERNEL);
> +       if (!data->domains)
> +               return -ENOMEM;
> +
> +       data->num_domains = num_pds;
> +
> +       ret = cmd_db_ready();

Does this matter? I thought we forced cmd_db_ready() in the rpmh driver
probe, and that was the parent of all rpmh devices so we should be fine
to not need to check it again here?

> +       if (ret) {
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(dev, "Command DB unavailable, ret=%d\n", ret);
> +               return ret;
> +       }
> +
Stephen Boyd Dec. 11, 2018, 10:01 p.m. UTC | #10
Quoting Rajendra Nayak (2018-12-11 01:49:35)
> Add the DT node for the rpmhpd powercontroller.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Rajendra Nayak Dec. 12, 2018, 4:13 a.m. UTC | #11
[]...

>>>> This is used to enforce a requirement that exists for various
>>>> hardware blocks on SDM845 that MX performance state >= CX performance
>>>> state for a given operating frequency.
>>>
>>> I assume that also means the MX power domain must not be power off as
>>> long as the CX power domain is powered on?
>>
>> So with rpmh, there's really no separate on/off control, we just put
>> it in the lowest perf state at off.
> 
> I think in theory the answer is MX can't be off if CX is on, but in
> reality, MX and CX are never turned off, just set to something really
> low and even then the constraint applies for MX >= CX. Is that right?

to some extent, let me clarify this a bit more. rpmh is the central entity
controlling these rails and takes a final call on when it can or cannot
be 'really' off and when its turned on, at what level it should be set to.

CPU is just one of the entities 'voting' on them. Now legacy platforms which
had rpm (like msm8996) did have separate control to send a
on/off vote and a performance level vote. With rpmh, its just that there
is no separate message that you send to rpmh for OFF, its just mapped to
the lowest perf level. So think of it as the lowest perf level is mapped to
a '0' which is same as OFF.

So when you say 'MX can't be off if CX is on', yes MX can't be at the lowest
level '0', while CX is at something higher.

>>> Just to make sure there are no conflicting hierarchical constraints
>>> between idle management and performance state management!?
>>>
> 
> I'm not sure what idle states mean to the CX and MX domains. Would it be
> some sort of idle state governor attached at genpd creation time that
> would adjust the main SoC power rails when all devices attached are
> idle? Maybe I don't understand how idle states are different from
> performance states.
> My understanding is that devices using these domains would almost always
> expect their clk frequency and clk on/off state to decide what the
> performance state is, unless they need to ignore clk state because they
> aren't managing clks and bump up the voltage directly when the device is
> active. Either way, devices are actively managing the voltage they need
> these voltage domains to operate at by using the genpd performance
> states APIs.

I am not quite sure whats the point that you are trying to make here,
but this is what I would expect the users of these genpds to do,
regardless of if they have a clk dependency or not.
When the device is active, vote for a performance state they need
then request for the genpd to be on. When they are idle, request for the
genpd to be turned off.
Rajendra Nayak Dec. 12, 2018, 4:17 a.m. UTC | #12
On 12/12/2018 3:21 AM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2018-12-11 01:49:28)
>> Changes in v6:
>> * OPP binding updates for qcom,level reviewed by Rob
>> * DT bindings for rpmpd and rpmhpd updated to specify the
>> OPP tables as child nodes of the power-controller itself
>> * Removed some module specific remains from the drivers,
>> now that they can only be built-in
>> * Added a simple_opp_to_performance_state() helper
> 
> Is the idea that it can be squashed into the patches that use it by
> reordering the series? But otherwise left at the end of the series to
> make things simpler to merge without it?

Yes, I left them at the end so its easier to merge. The PM domains change
would need to go via Ulf/Viresh while the SoC drivers using them would
go via Andy. So now its only 09/10 that's dependent on 08/10 and not the
entire series.
Rajendra Nayak Dec. 12, 2018, 5:04 a.m. UTC | #13
On 12/12/2018 3:31 AM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2018-12-11 01:49:34)
>> The RPMh power domain driver aggregates the corner votes from various
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> With RPMh we use 2 different numbering space for corners, one used
>> by the clients to express their performance needs, and another used
>> to communicate to RPMh hardware.
>>
>> The clients express their performance requirements using a sparse
>> numbering space which are mapped to meaningful levels like RET, SVS,
>> NOMINAL, TURBO etc which then get mapped to another number space
>> between 0 and 15 which is communicated to RPMh. The sparse number space,
>> also referred to as vlvl is mapped to the continuous number space of 0
>> to 15, also referred to as hlvl, using command DB.
>>
>> Some power domain clients could request a performance state only while
>> the CPU is active, while some others could request for a certain
>> performance state all the time regardless of the state of the CPU.
>> We handle this by internally aggregating the votes from both type of
>> clients and then send the aggregated votes to RPMh.
>>
>> There are also 3 different types of Votes that are comunicated to RPMh
> 
> Why capitalize vote?

will fix,

> 
>> for every resource.
>> 1. ACTIVE_ONLY: This specifies the requirement for the resource when the
>> CPU is active
>> 2. SLEEP: This specifies the requirement for the resource when the CPU
>> is going to sleep
>> 3. WAKE_ONLY: This specifies the requirement for the resource when the
>> CPU is coming out of sleep to active state
> 
> Can you tab these in?

sure, will do

> 
>>
>> We add data for all power domains on sdm845 SoC as part of the patch.
>> The driver can be extended to support other SoCs which support RPMh
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Just minor nitpicks ahead and a warning.
> 
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index f1b25fdcf2ad..dd6ca92985ee 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -22,3 +22,4 @@ obj-$(CONFIG_QCOM_APR) += apr.o
>>   obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
>>   obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
>>   obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>> +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> 
> Put this before RPMPD? At least it would be semi-sorted then.

okay, will do

> 
>> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
>> new file mode 100644
>> index 000000000000..f993a86be48c
>> --- /dev/null
>> +++ b/drivers/soc/qcom/rpmhpd.c
>> @@ -0,0 +1,417 @@
> [..]
>> +
>> +static int rpmhpd_update_level_mapping(struct rpmhpd *rpmhpd)
>> +{
>> +       u8 buf[RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE];
>> +       int i, j, len, ret;
>> +
>> +       len = cmd_db_read_aux_data_len(rpmhpd->res_name);
>> +       if (len <= 0)
>> +               return len;
>> +       else if (len > RPMH_ARC_MAX_LEVELS * RPMH_ARC_LEVEL_SIZE)
>> +               return -EINVAL;
>> +
>> +       ret = cmd_db_read_aux_data(rpmhpd->res_name, buf, len);
>> +       if (ret < 0)
>> +               return ret;
> 
> I've changed cmd_db_read_aux_data() and that change is winding through
> the arm-soc tree. This will break in the compilation phase in
> linux-next.

will resend these based on Andy's for-next

> 
>> +
>> +       rpmhpd->level_count = len / RPMH_ARC_LEVEL_SIZE;
>> +
>> +       for (i = 0; i < rpmhpd->level_count; i++) {
>> +               rpmhpd->level[i] = 0;
>> +               for (j = 0; j < RPMH_ARC_LEVEL_SIZE; j++)
>> +                       rpmhpd->level[i] |=
>> +                               buf[i * RPMH_ARC_LEVEL_SIZE + j] << (8 * j);
>> +
>> +               /*
>> +                * The AUX data may be zero padded.  These 0 valued entries at
>> +                * the end of the map must be ignored.
>> +                */
>> +               if (i > 0 && rpmhpd->level[i] == 0) {
>> +                       rpmhpd->level_count = i;
>> +                       break;
>> +               }
>> +               pr_debug("%s: ARC hlvl=%2d --> vlvl=%4u\n", rpmhpd->res_name, i,
>> +                        rpmhpd->level[i]);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int rpmhpd_probe(struct platform_device *pdev)
>> +{
>> +       int i, ret;
>> +       size_t num_pds;
>> +       struct device *dev = &pdev->dev;
>> +       struct genpd_onecell_data *data;
>> +       struct rpmhpd **rpmhpds;
>> +       const struct rpmhpd_desc *desc;
>> +
>> +       desc = of_device_get_match_data(dev);
>> +       if (!desc)
>> +               return -EINVAL;
>> +
>> +       rpmhpds = desc->rpmhpds;
>> +       num_pds = desc->num_pds;
>> +
>> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +       if (!data)
>> +               return -ENOMEM;
>> +
>> +       data->domains = devm_kcalloc(dev, num_pds, sizeof(*data->domains),
>> +                                    GFP_KERNEL);
>> +       if (!data->domains)
>> +               return -ENOMEM;
>> +
>> +       data->num_domains = num_pds;
>> +
>> +       ret = cmd_db_ready();
> 
> Does this matter? I thought we forced cmd_db_ready() in the rpmh driver
> probe, and that was the parent of all rpmh devices so we should be fine
> to not need to check it again here?

sure, will remove.

thanks for the review.

> 
>> +       if (ret) {
>> +               if (ret != -EPROBE_DEFER)
>> +                       dev_err(dev, "Command DB unavailable, ret=%d\n", ret);
>> +               return ret;
>> +       }
>> +
Stephen Boyd Dec. 12, 2018, 6:32 p.m. UTC | #14
Quoting Rajendra Nayak (2018-12-11 20:13:13)
> 
> >>> Just to make sure there are no conflicting hierarchical constraints
> >>> between idle management and performance state management!?
> >>>
> > 
> > I'm not sure what idle states mean to the CX and MX domains. Would it be
> > some sort of idle state governor attached at genpd creation time that
> > would adjust the main SoC power rails when all devices attached are
> > idle? Maybe I don't understand how idle states are different from
> > performance states.
> > My understanding is that devices using these domains would almost always
> > expect their clk frequency and clk on/off state to decide what the
> > performance state is, unless they need to ignore clk state because they
> > aren't managing clks and bump up the voltage directly when the device is
> > active. Either way, devices are actively managing the voltage they need
> > these voltage domains to operate at by using the genpd performance
> > states APIs.
> 
> I am not quite sure whats the point that you are trying to make here,
> but this is what I would expect the users of these genpds to do,
> regardless of if they have a clk dependency or not.
> When the device is active, vote for a performance state they need
> then request for the genpd to be on. When they are idle, request for the
> genpd to be turned off.
>   

I believe Ulf is asking because he's proposing to make genpd idle states
and genpd performance states orthogonal to each other. And to also make
performance states unaffected by the on/off state of the genpd.
Ulf Hansson Dec. 13, 2018, 4:02 p.m. UTC | #15
On Wed, 12 Dec 2018 at 19:32, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rajendra Nayak (2018-12-11 20:13:13)
> >
> > >>> Just to make sure there are no conflicting hierarchical constraints
> > >>> between idle management and performance state management!?
> > >>>
> > >
> > > I'm not sure what idle states mean to the CX and MX domains. Would it be
> > > some sort of idle state governor attached at genpd creation time that
> > > would adjust the main SoC power rails when all devices attached are
> > > idle? Maybe I don't understand how idle states are different from
> > > performance states.
> > > My understanding is that devices using these domains would almost always
> > > expect their clk frequency and clk on/off state to decide what the
> > > performance state is, unless they need to ignore clk state because they
> > > aren't managing clks and bump up the voltage directly when the device is
> > > active. Either way, devices are actively managing the voltage they need
> > > these voltage domains to operate at by using the genpd performance
> > > states APIs.
> >
> > I am not quite sure whats the point that you are trying to make here,
> > but this is what I would expect the users of these genpds to do,
> > regardless of if they have a clk dependency or not.
> > When the device is active, vote for a performance state they need
> > then request for the genpd to be on. When they are idle, request for the
> > genpd to be turned off.
> >
>
> I believe Ulf is asking because he's proposing to make genpd idle states
> and genpd performance states orthogonal to each other. And to also make
> performance states unaffected by the on/off state of the genpd.

Yes, that's one of the reasons.

Anyway, I appreciate both of yours descriptive feedback, no further
worries from my side!

Kind regards
Uffe