Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Lina Iyer]

On Fri, May 25 2018 at 19:08 -0600, David Collins wrote:

Hello Rajendra,

On 05/25/2018 03:01 AM, Rajendra Nayak wrote:

+
+   to_active_sleep(pd, pd->corner, _corner, _sleep_corner);
+
+   if (peer && peer->enabled)
+   to_active_sleep(peer, peer->corner, _corner,
+   _sleep_corner);
+
+   active_corner = max(this_corner, peer_corner);
+
+   ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, active_corner);
+   if (ret)
+   return ret;
+
+   sleep_corner = max(this_sleep_corner, peer_sleep_corner);
+
+   return rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, sleep_corner);
+}


This aggregation function as well as the rpmhpd_send_corner() calls below
are not sufficient for RPMh.  There are 3 states that must all be used:
RPMH_ACTIVE_ONLY_STATE, RPMH_WAKE_ONLY_STATE, and RPMH_SLEEP_STATE.  The
naming is somewhat confusing as rpmhpd is defining a different concept of
active-only.

For power domains without active-only or peers, only
RPMH_ACTIVE_ONLY_STATE should be used.  This instructs RPMh to issue the
request immediately.

For power domains with active-only, requests will need to be made for all
three.  active_corner would be sent for both RPMH_ACTIVE_ONLY_STATE (so
that the request takes effect immediately) and RPMH_WAKE_ONLY_STATE (so
that the request is inserted into the wake TCS).  sleep_corner would be
sent for RPMH_SLEEP_STATE (so that the request is inserted into the sleep
TCS).

You can see how this is handled in the RPMh clock driver in patch [3].

You may be able to get away with using only RPMH_SLEEP_STATE and
RPMH_ACTIVE_ONLY_STATE assuming that you issue the RPMH_SLEEP_STATE
request first due to the rpmh driver caching behavior added in the
cache_rpm_request() function in [4].  However, could you please confirm
with Lina that this usage will continue to work in the future?  I'm not
sure what guarantees are made at the rpmh API level.


We expect to cache all active values into wake if there was a sleep
value already defined. Expect to continue this behavior in the future
as well. But it would be safer for you to send sleep and wake votes in
addition to active votes. It shouldn't add too much of an overhead.

-- Lina

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Lina Iyer]

On Fri, May 25 2018 at 19:08 -0600, David Collins wrote:

Hello Rajendra,

On 05/25/2018 03:01 AM, Rajendra Nayak wrote:

+
+   to_active_sleep(pd, pd->corner, _corner, _sleep_corner);
+
+   if (peer && peer->enabled)
+   to_active_sleep(peer, peer->corner, _corner,
+   _sleep_corner);
+
+   active_corner = max(this_corner, peer_corner);
+
+   ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, active_corner);
+   if (ret)
+   return ret;
+
+   sleep_corner = max(this_sleep_corner, peer_sleep_corner);
+
+   return rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, sleep_corner);
+}


This aggregation function as well as the rpmhpd_send_corner() calls below
are not sufficient for RPMh.  There are 3 states that must all be used:
RPMH_ACTIVE_ONLY_STATE, RPMH_WAKE_ONLY_STATE, and RPMH_SLEEP_STATE.  The
naming is somewhat confusing as rpmhpd is defining a different concept of
active-only.

For power domains without active-only or peers, only
RPMH_ACTIVE_ONLY_STATE should be used.  This instructs RPMh to issue the
request immediately.

For power domains with active-only, requests will need to be made for all
three.  active_corner would be sent for both RPMH_ACTIVE_ONLY_STATE (so
that the request takes effect immediately) and RPMH_WAKE_ONLY_STATE (so
that the request is inserted into the wake TCS).  sleep_corner would be
sent for RPMH_SLEEP_STATE (so that the request is inserted into the sleep
TCS).

You can see how this is handled in the RPMh clock driver in patch [3].

You may be able to get away with using only RPMH_SLEEP_STATE and
RPMH_ACTIVE_ONLY_STATE assuming that you issue the RPMH_SLEEP_STATE
request first due to the rpmh driver caching behavior added in the
cache_rpm_request() function in [4].  However, could you please confirm
with Lina that this usage will continue to work in the future?  I'm not
sure what guarantees are made at the rpmh API level.


We expect to cache all active values into wake if there was a sleep
value already defined. Expect to continue this behavior in the future
as well. But it would be safer for you to send sleep and wake votes in
addition to active votes. It shouldn't add too much of an overhead.

-- Lina

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 06/01/2018 01:48 AM, Rajendra Nayak wrote:
> On 05/26/2018 06:38 AM, David Collins wrote:
>>
>>> +   [1] = _mx,
>>> +   [2] = _mx_ao,
>>> +   [3] = _cx,
>>> +   [4] = _cx_ao,
>>> +   [5] = _lmx,
>>> +   [6] = _lcx,
>>> +   [7] = _gfx,
>>> +   [8] = _mss,
>>> +};
>>> +
>>> +static const struct rpmhpd_desc sdm845_desc = {
>>> +   .rpmhpds = sdm845_rpmhpds,
>>> +   .num_pds = ARRAY_SIZE(sdm845_rpmhpds),
>>> +};
>>> +
>>> +static const struct of_device_id rpmhpd_match_table[] = {
>>> +   { .compatible = "qcom,sdm845-rpmhpd", .data = _desc },
>>> +   { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
>>> +
>>> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state, unsigned int 
>>> corner)
>>> +{
>>> +   struct tcs_cmd cmd = {
>>> +   .addr = pd->addr,
>>> +   .data = corner,
>>> +   };
>>> +
>>> +   return rpmh_write(pd->dev, state, , 1);
>> This can be optimized by calling rpmh_write_async() whenever the corner
>> being sent is smaller than the last value sent.  That way, no time is
>> wasted waiting for an ACK when decreasing voltage.  Would you mind adding
>> the necessary check and previous request caching for this?
> 
> is it safe to assume all sleep votes can be sent as async always? and only
> active votes could be sync/async depending on the last value sent?

Yes, rpmh_write_async() can always be used for RPMH_SLEEP_STATE and
RPMH_WAKE_ONLY_STATE.  The rpmh_write() vs rpmh_write_async() distinction
only matters for RPMH_ACTIVE_ONLY_STATE.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 06/01/2018 01:48 AM, Rajendra Nayak wrote:
> On 05/26/2018 06:38 AM, David Collins wrote:
>>
>>> +   [1] = _mx,
>>> +   [2] = _mx_ao,
>>> +   [3] = _cx,
>>> +   [4] = _cx_ao,
>>> +   [5] = _lmx,
>>> +   [6] = _lcx,
>>> +   [7] = _gfx,
>>> +   [8] = _mss,
>>> +};
>>> +
>>> +static const struct rpmhpd_desc sdm845_desc = {
>>> +   .rpmhpds = sdm845_rpmhpds,
>>> +   .num_pds = ARRAY_SIZE(sdm845_rpmhpds),
>>> +};
>>> +
>>> +static const struct of_device_id rpmhpd_match_table[] = {
>>> +   { .compatible = "qcom,sdm845-rpmhpd", .data = _desc },
>>> +   { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
>>> +
>>> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state, unsigned int 
>>> corner)
>>> +{
>>> +   struct tcs_cmd cmd = {
>>> +   .addr = pd->addr,
>>> +   .data = corner,
>>> +   };
>>> +
>>> +   return rpmh_write(pd->dev, state, , 1);
>> This can be optimized by calling rpmh_write_async() whenever the corner
>> being sent is smaller than the last value sent.  That way, no time is
>> wasted waiting for an ACK when decreasing voltage.  Would you mind adding
>> the necessary check and previous request caching for this?
> 
> is it safe to assume all sleep votes can be sent as async always? and only
> active votes could be sync/async depending on the last value sent?

Yes, rpmh_write_async() can always be used for RPMH_SLEEP_STATE and
RPMH_WAKE_ONLY_STATE.  The rpmh_write() vs rpmh_write_async() distinction
only matters for RPMH_ACTIVE_ONLY_STATE.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

Hi David,

On 05/26/2018 06:38 AM, David Collins wrote:
> 
>> +[1] = _mx,
>> +[2] = _mx_ao,
>> +[3] = _cx,
>> +[4] = _cx_ao,
>> +[5] = _lmx,
>> +[6] = _lcx,
>> +[7] = _gfx,
>> +[8] = _mss,
>> +};
>> +
>> +static const struct rpmhpd_desc sdm845_desc = {
>> +.rpmhpds = sdm845_rpmhpds,
>> +.num_pds = ARRAY_SIZE(sdm845_rpmhpds),
>> +};
>> +
>> +static const struct of_device_id rpmhpd_match_table[] = {
>> +{ .compatible = "qcom,sdm845-rpmhpd", .data = _desc },
>> +{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
>> +
>> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state, unsigned int 
>> corner)
>> +{
>> +struct tcs_cmd cmd = {
>> +.addr = pd->addr,
>> +.data = corner,
>> +};
>> +
>> +return rpmh_write(pd->dev, state, , 1);
> This can be optimized by calling rpmh_write_async() whenever the corner
> being sent is smaller than the last value sent.  That way, no time is
> wasted waiting for an ACK when decreasing voltage.  Would you mind adding
> the necessary check and previous request caching for this?

is it safe to assume all sleep votes can be sent as async always? and only
active votes could be sync/async depending on the last value sent?

regards,
Rajendra

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

Hi David,

On 05/26/2018 06:38 AM, David Collins wrote:
> 
>> +[1] = _mx,
>> +[2] = _mx_ao,
>> +[3] = _cx,
>> +[4] = _cx_ao,
>> +[5] = _lmx,
>> +[6] = _lcx,
>> +[7] = _gfx,
>> +[8] = _mss,
>> +};
>> +
>> +static const struct rpmhpd_desc sdm845_desc = {
>> +.rpmhpds = sdm845_rpmhpds,
>> +.num_pds = ARRAY_SIZE(sdm845_rpmhpds),
>> +};
>> +
>> +static const struct of_device_id rpmhpd_match_table[] = {
>> +{ .compatible = "qcom,sdm845-rpmhpd", .data = _desc },
>> +{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, rpmhpd_match_table);
>> +
>> +static int rpmhpd_send_corner(struct rpmhpd *pd, int state, unsigned int 
>> corner)
>> +{
>> +struct tcs_cmd cmd = {
>> +.addr = pd->addr,
>> +.data = corner,
>> +};
>> +
>> +return rpmh_write(pd->dev, state, , 1);
> This can be optimized by calling rpmh_write_async() whenever the corner
> being sent is smaller than the last value sent.  That way, no time is
> wasted waiting for an ACK when decreasing voltage.  Would you mind adding
> the necessary check and previous request caching for this?

is it safe to assume all sleep votes can be sent as async always? and only
active votes could be sync/async depending on the last value sent?

regards,
Rajendra

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

On 05/31/2018 09:01 AM, Rob Herring wrote:
> On Fri, May 25, 2018 at 03:31:20PM +0530, Rajendra Nayak wrote:
>> The RPMh powerdomain driver aggregates the corner votes from various
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all powerdomains on sdm845 as part of the patch.
>> The driver can be extended to support other SoCs which support RPMh
>>
>> Signed-off-by: Rajendra Nayak 
>> ---
>>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
>>  drivers/soc/qcom/Kconfig  |   9 +
>>  drivers/soc/qcom/Makefile |   1 +
>>  drivers/soc/qcom/rpmhpd.c | 360 ++
>>  4 files changed, 435 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>>
>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt 
>> b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> new file mode 100644
>> index ..c1fa986c12ee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> @@ -0,0 +1,65 @@
>> +Qualcomm RPMh Powerdomains
>> +
>> +* For RPMh powerdomains, we communicate a performance state to RPMh
>> +which then translates it into a corresponding voltage on a rail
>> +
>> +Required Properties:
>> + - compatible: Should be one of the following
>> +* qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
>> + - power-domain-cells: number of cells in power domain specifier
>> +must be 1
>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
>> +Refer to Documentation/devicetree/bindings/power/power_domain.txt
>> +and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
>> +
>> +Example:
>> +
>> +rpmhpd: power-controller {
>> +compatible = "qcom,sdm845-rpmhpd";
>> +#power-domain-cells = <1>;
>> +operating-points-v2 = <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>;
>> +};
>> +
>> +rpmhpd_opp_table: opp-table {
>> +compatible = "operating-points-v2-qcom-level", 
>> "operating-points-v2";
>> +
>> +rpmhpd_opp1: opp@1 {
>> +qcom-corner = <16>;
> 
> Is it corner or level?

It should be level, I will fix it up when I respin. thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

On 05/31/2018 09:01 AM, Rob Herring wrote:
> On Fri, May 25, 2018 at 03:31:20PM +0530, Rajendra Nayak wrote:
>> The RPMh powerdomain driver aggregates the corner votes from various
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all powerdomains on sdm845 as part of the patch.
>> The driver can be extended to support other SoCs which support RPMh
>>
>> Signed-off-by: Rajendra Nayak 
>> ---
>>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
>>  drivers/soc/qcom/Kconfig  |   9 +
>>  drivers/soc/qcom/Makefile |   1 +
>>  drivers/soc/qcom/rpmhpd.c | 360 ++
>>  4 files changed, 435 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
>>
>> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt 
>> b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> new file mode 100644
>> index ..c1fa986c12ee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>> @@ -0,0 +1,65 @@
>> +Qualcomm RPMh Powerdomains
>> +
>> +* For RPMh powerdomains, we communicate a performance state to RPMh
>> +which then translates it into a corresponding voltage on a rail
>> +
>> +Required Properties:
>> + - compatible: Should be one of the following
>> +* qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
>> + - power-domain-cells: number of cells in power domain specifier
>> +must be 1
>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
>> +Refer to Documentation/devicetree/bindings/power/power_domain.txt
>> +and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
>> +
>> +Example:
>> +
>> +rpmhpd: power-controller {
>> +compatible = "qcom,sdm845-rpmhpd";
>> +#power-domain-cells = <1>;
>> +operating-points-v2 = <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>;
>> +};
>> +
>> +rpmhpd_opp_table: opp-table {
>> +compatible = "operating-points-v2-qcom-level", 
>> "operating-points-v2";
>> +
>> +rpmhpd_opp1: opp@1 {
>> +qcom-corner = <16>;
> 
> Is it corner or level?

It should be level, I will fix it up when I respin. thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rob Herring]

On Fri, May 25, 2018 at 03:31:20PM +0530, Rajendra Nayak wrote:
> The RPMh powerdomain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all powerdomains on sdm845 as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
> 
> Signed-off-by: Rajendra Nayak 
> ---
>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
>  drivers/soc/qcom/Kconfig  |   9 +
>  drivers/soc/qcom/Makefile |   1 +
>  drivers/soc/qcom/rpmhpd.c | 360 ++
>  4 files changed, 435 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
> 
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt 
> b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> new file mode 100644
> index ..c1fa986c12ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> @@ -0,0 +1,65 @@
> +Qualcomm RPMh Powerdomains
> +
> +* For RPMh powerdomains, we communicate a performance state to RPMh
> +which then translates it into a corresponding voltage on a rail
> +
> +Required Properties:
> + - compatible: Should be one of the following
> + * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
> + - power-domain-cells: number of cells in power domain specifier
> + must be 1
> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> + Refer to Documentation/devicetree/bindings/power/power_domain.txt
> + and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> +
> +Example:
> +
> + rpmhpd: power-controller {
> + compatible = "qcom,sdm845-rpmhpd";
> + #power-domain-cells = <1>;
> + operating-points-v2 = <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>;
> + };
> +
> + rpmhpd_opp_table: opp-table {
> + compatible = "operating-points-v2-qcom-level", 
> "operating-points-v2";
> +
> + rpmhpd_opp1: opp@1 {
> + qcom-corner = <16>;

Is it corner or level?

> + };
> +
> + rpmhpd_opp2: opp@2 {
> + qcom-corner = <48>;
> + };
> +
> + rpmhpd_opp3: opp@3 {
> + qcom-corner = <64>;
> + };
> +
> + rpmhpd_opp4: opp@4 {
> + qcom-corner = <128>;
> + };
> +
> + rpmhpd_opp5: opp@5 {
> + qcom-corner = <192>;
> + };
> +
> + rpmhpd_opp6: opp@6 {
> + qcom-corner = <256>;
> + };
> +
> + rpmhpd_opp7: opp@7 {
> + qcom-corner = <320>;
> + };
> +
> + rpmhpd_opp8: opp@8 {
> + qcom-corner = <416>;
> + };
> + };

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rob Herring]

On Fri, May 25, 2018 at 03:31:20PM +0530, Rajendra Nayak wrote:
> The RPMh powerdomain driver aggregates the corner votes from various
> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all powerdomains on sdm845 as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
> 
> Signed-off-by: Rajendra Nayak 
> ---
>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
>  drivers/soc/qcom/Kconfig  |   9 +
>  drivers/soc/qcom/Makefile |   1 +
>  drivers/soc/qcom/rpmhpd.c | 360 ++
>  4 files changed, 435 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
> 
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt 
> b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> new file mode 100644
> index ..c1fa986c12ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> @@ -0,0 +1,65 @@
> +Qualcomm RPMh Powerdomains
> +
> +* For RPMh powerdomains, we communicate a performance state to RPMh
> +which then translates it into a corresponding voltage on a rail
> +
> +Required Properties:
> + - compatible: Should be one of the following
> + * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
> + - power-domain-cells: number of cells in power domain specifier
> + must be 1
> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> + Refer to Documentation/devicetree/bindings/power/power_domain.txt
> + and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> +
> +Example:
> +
> + rpmhpd: power-controller {
> + compatible = "qcom,sdm845-rpmhpd";
> + #power-domain-cells = <1>;
> + operating-points-v2 = <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>;
> + };
> +
> + rpmhpd_opp_table: opp-table {
> + compatible = "operating-points-v2-qcom-level", 
> "operating-points-v2";
> +
> + rpmhpd_opp1: opp@1 {
> + qcom-corner = <16>;

Is it corner or level?

> + };
> +
> + rpmhpd_opp2: opp@2 {
> + qcom-corner = <48>;
> + };
> +
> + rpmhpd_opp3: opp@3 {
> + qcom-corner = <64>;
> + };
> +
> + rpmhpd_opp4: opp@4 {
> + qcom-corner = <128>;
> + };
> +
> + rpmhpd_opp5: opp@5 {
> + qcom-corner = <192>;
> + };
> +
> + rpmhpd_opp6: opp@6 {
> + qcom-corner = <256>;
> + };
> +
> + rpmhpd_opp7: opp@7 {
> + qcom-corner = <320>;
> + };
> +
> + rpmhpd_opp8: opp@8 {
> + qcom-corner = <416>;
> + };
> + };

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

On 05/30/2018 03:14 PM, Viresh Kumar wrote:
> On 30-05-18, 14:25, Rajendra Nayak wrote:
>> []...
>>
> +Required Properties:
> + - compatible: Should be one of the following
> + * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
> + - power-domain-cells: number of cells in power domain specifier
> + must be 1
> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> + Refer to Documentation/devicetree/bindings/power/power_domain.txt
> + and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> +
> +Example:
> +
> + rpmhpd: power-controller {
> + compatible = "qcom,sdm845-rpmhpd";
> + #power-domain-cells = <1>;
> + operating-points-v2 = <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>;

 Can this be changed to simply:
   operating-points-v2 = <_opp_table>;

 The opp binding documentation [1] states that this should be ok:

 If only one phandle is available, then the same OPP table will be used
 for all power domains provided by the power domain provider.
>>>
>>> thanks, I mentioned this to Viresh but didn't realize he fixed it up.
>>> Will remove the redundant entries.
>>
>> Looks like the kernel implementation does not handle this yet, and I get
>> an error adding the OPP tables for the powerdomains if I just specify
>> a single OPP table phandle.
>>
>> Viresh, is this expected with the latest patches in linux-next?
>>
>> It would be good if I can specify just one phandle instead of coping
>> the same phandle n times. 
> 
> Please try this untested hunk:

yes, seems to work.

> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 6d15f05bfc28..7af0ddec936b 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -554,11 +554,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
>  int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
>  {
> struct device_node *opp_np;
> -   int ret;
> +   int ret, count;
>  
> +again:
> opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
> -   if (!opp_np)
> +   if (!opp_np) {
> +   /*
> +* If only one phandle is present, then the same OPP table
> +* applies for all index requests.
> +*/
> +   count = of_count_phandle_with_args(dev->of_node,
> +  "operating-points-v2", 
> NULL);
> +   if (count == 1 && index) {
> +   index = 0;
> +   goto again;
> +   }
> +
> return -ENODEV;
> +   }
>  
> ret = _of_add_opp_table_v2(dev, opp_np);
> of_node_put(opp_np);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

On 05/30/2018 03:14 PM, Viresh Kumar wrote:
> On 30-05-18, 14:25, Rajendra Nayak wrote:
>> []...
>>
> +Required Properties:
> + - compatible: Should be one of the following
> + * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
> + - power-domain-cells: number of cells in power domain specifier
> + must be 1
> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> + Refer to Documentation/devicetree/bindings/power/power_domain.txt
> + and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> +
> +Example:
> +
> + rpmhpd: power-controller {
> + compatible = "qcom,sdm845-rpmhpd";
> + #power-domain-cells = <1>;
> + operating-points-v2 = <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>;

 Can this be changed to simply:
   operating-points-v2 = <_opp_table>;

 The opp binding documentation [1] states that this should be ok:

 If only one phandle is available, then the same OPP table will be used
 for all power domains provided by the power domain provider.
>>>
>>> thanks, I mentioned this to Viresh but didn't realize he fixed it up.
>>> Will remove the redundant entries.
>>
>> Looks like the kernel implementation does not handle this yet, and I get
>> an error adding the OPP tables for the powerdomains if I just specify
>> a single OPP table phandle.
>>
>> Viresh, is this expected with the latest patches in linux-next?
>>
>> It would be good if I can specify just one phandle instead of coping
>> the same phandle n times. 
> 
> Please try this untested hunk:

yes, seems to work.

> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 6d15f05bfc28..7af0ddec936b 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -554,11 +554,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
>  int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
>  {
> struct device_node *opp_np;
> -   int ret;
> +   int ret, count;
>  
> +again:
> opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
> -   if (!opp_np)
> +   if (!opp_np) {
> +   /*
> +* If only one phandle is present, then the same OPP table
> +* applies for all index requests.
> +*/
> +   count = of_count_phandle_with_args(dev->of_node,
> +  "operating-points-v2", 
> NULL);
> +   if (count == 1 && index) {
> +   index = 0;
> +   goto again;
> +   }
> +
> return -ENODEV;
> +   }
>  
> ret = _of_add_opp_table_v2(dev, opp_np);
> of_node_put(opp_np);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Viresh Kumar]

On 30-05-18, 14:25, Rajendra Nayak wrote:
> []...
> 
> >>> +Required Properties:
> >>> + - compatible: Should be one of the following
> >>> + * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
> >>> + - power-domain-cells: number of cells in power domain specifier
> >>> + must be 1
> >>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> >>> + Refer to Documentation/devicetree/bindings/power/power_domain.txt
> >>> + and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> >>> +
> >>> +Example:
> >>> +
> >>> + rpmhpd: power-controller {
> >>> + compatible = "qcom,sdm845-rpmhpd";
> >>> + #power-domain-cells = <1>;
> >>> + operating-points-v2 = <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>;
> >>
> >> Can this be changed to simply:
> >>   operating-points-v2 = <_opp_table>;
> >>
> >> The opp binding documentation [1] states that this should be ok:
> >>
> >> If only one phandle is available, then the same OPP table will be used
> >> for all power domains provided by the power domain provider.
> > 
> > thanks, I mentioned this to Viresh but didn't realize he fixed it up.
> > Will remove the redundant entries.
> 
> Looks like the kernel implementation does not handle this yet, and I get
> an error adding the OPP tables for the powerdomains if I just specify
> a single OPP table phandle.
> 
> Viresh, is this expected with the latest patches in linux-next?
> 
> It would be good if I can specify just one phandle instead of coping
> the same phandle n times. 

Please try this untested hunk:

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 6d15f05bfc28..7af0ddec936b 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -554,11 +554,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
 int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
 {
struct device_node *opp_np;
-   int ret;
+   int ret, count;
 
+again:
opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
-   if (!opp_np)
+   if (!opp_np) {
+   /*
+* If only one phandle is present, then the same OPP table
+* applies for all index requests.
+*/
+   count = of_count_phandle_with_args(dev->of_node,
+  "operating-points-v2", NULL);
+   if (count == 1 && index) {
+   index = 0;
+   goto again;
+   }
+
return -ENODEV;
+   }
 
ret = _of_add_opp_table_v2(dev, opp_np);
of_node_put(opp_np);

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Viresh Kumar]

On 30-05-18, 14:25, Rajendra Nayak wrote:
> []...
> 
> >>> +Required Properties:
> >>> + - compatible: Should be one of the following
> >>> + * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
> >>> + - power-domain-cells: number of cells in power domain specifier
> >>> + must be 1
> >>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> >>> + Refer to Documentation/devicetree/bindings/power/power_domain.txt
> >>> + and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> >>> +
> >>> +Example:
> >>> +
> >>> + rpmhpd: power-controller {
> >>> + compatible = "qcom,sdm845-rpmhpd";
> >>> + #power-domain-cells = <1>;
> >>> + operating-points-v2 = <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>,
> >>> +   <_opp_table>;
> >>
> >> Can this be changed to simply:
> >>   operating-points-v2 = <_opp_table>;
> >>
> >> The opp binding documentation [1] states that this should be ok:
> >>
> >> If only one phandle is available, then the same OPP table will be used
> >> for all power domains provided by the power domain provider.
> > 
> > thanks, I mentioned this to Viresh but didn't realize he fixed it up.
> > Will remove the redundant entries.
> 
> Looks like the kernel implementation does not handle this yet, and I get
> an error adding the OPP tables for the powerdomains if I just specify
> a single OPP table phandle.
> 
> Viresh, is this expected with the latest patches in linux-next?
> 
> It would be good if I can specify just one phandle instead of coping
> the same phandle n times. 

Please try this untested hunk:

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 6d15f05bfc28..7af0ddec936b 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -554,11 +554,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_add_table);
 int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
 {
struct device_node *opp_np;
-   int ret;
+   int ret, count;
 
+again:
opp_np = _opp_of_get_opp_desc_node(dev->of_node, index);
-   if (!opp_np)
+   if (!opp_np) {
+   /*
+* If only one phandle is present, then the same OPP table
+* applies for all index requests.
+*/
+   count = of_count_phandle_with_args(dev->of_node,
+  "operating-points-v2", NULL);
+   if (count == 1 && index) {
+   index = 0;
+   goto again;
+   }
+
return -ENODEV;
+   }
 
ret = _of_add_opp_table_v2(dev, opp_np);
of_node_put(opp_np);

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

[]...

>>> +Required Properties:
>>> + - compatible: Should be one of the following
>>> +   * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
>>> + - power-domain-cells: number of cells in power domain specifier
>>> +   must be 1
>>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
>>> +   Refer to Documentation/devicetree/bindings/power/power_domain.txt
>>> +   and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
>>> +
>>> +Example:
>>> +
>>> +   rpmhpd: power-controller {
>>> +   compatible = "qcom,sdm845-rpmhpd";
>>> +   #power-domain-cells = <1>;
>>> +   operating-points-v2 = <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>;
>>
>> Can this be changed to simply:
>>   operating-points-v2 = <_opp_table>;
>>
>> The opp binding documentation [1] states that this should be ok:
>>
>> If only one phandle is available, then the same OPP table will be used
>> for all power domains provided by the power domain provider.
> 
> thanks, I mentioned this to Viresh but didn't realize he fixed it up.
> Will remove the redundant entries.

Looks like the kernel implementation does not handle this yet, and I get
an error adding the OPP tables for the powerdomains if I just specify
a single OPP table phandle.

Viresh, is this expected with the latest patches in linux-next?

It would be good if I can specify just one phandle instead of coping
the same phandle n times. 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

[]...

>>> +Required Properties:
>>> + - compatible: Should be one of the following
>>> +   * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
>>> + - power-domain-cells: number of cells in power domain specifier
>>> +   must be 1
>>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
>>> +   Refer to Documentation/devicetree/bindings/power/power_domain.txt
>>> +   and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
>>> +
>>> +Example:
>>> +
>>> +   rpmhpd: power-controller {
>>> +   compatible = "qcom,sdm845-rpmhpd";
>>> +   #power-domain-cells = <1>;
>>> +   operating-points-v2 = <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>,
>>> + <_opp_table>;
>>
>> Can this be changed to simply:
>>   operating-points-v2 = <_opp_table>;
>>
>> The opp binding documentation [1] states that this should be ok:
>>
>> If only one phandle is available, then the same OPP table will be used
>> for all power domains provided by the power domain provider.
> 
> thanks, I mentioned this to Viresh but didn't realize he fixed it up.
> Will remove the redundant entries.

Looks like the kernel implementation does not handle this yet, and I get
an error adding the OPP tables for the powerdomains if I just specify
a single OPP table phandle.

Viresh, is this expected with the latest patches in linux-next?

It would be good if I can specify just one phandle instead of coping
the same phandle n times. 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 05/29/2018 03:19 AM, Rajendra Nayak wrote:
> On 05/26/2018 06:38 AM, David Collins wrote:
>> On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
>>> The RPMh powerdomain driver aggregates the corner votes from various
...
>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>> index a7a405178967..1faed239701d 100644
>>> --- a/drivers/soc/qcom/Kconfig
>>> +++ b/drivers/soc/qcom/Kconfig
>>> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>>>  
>>>   Say y here if you intend to boot the modem remoteproc.
>>>  
>>> +config QCOM_RPMHPD
>>> +   tristate "Qualcomm RPMh Powerdomain driver"
>>
>> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> All other config options in qcom/Kconfig use 'Qualcomm XYZ feature'
> for the comment. Maybe I will leave it that way for consistency?

I don't have a strong opinion about it.  I just want the legal folks to be
happy.  I'm fine with whatever achieves that goal.


>>> +
>>> +struct rpmhpd_desc {
>>> +   struct rpmhpd **rpmhpds;
>>> +   size_t num_pds;
>>> +};
>>
>> This struct could be removed and the per-platform arrays could instead be
>> NULL terminated.
> 
> Yes, but I would prefer it this way unless you have strong objections.
> Just makes it easier to do the allocations at probe for genpd_onecell_data 
> structures.

I'm fine if you keep it as-is.  I mentioned the alternative because
Stephen had requested the same modification on my qcom-rpmh-regulator
driver patch [1].  Other reviewers may care about this point.


>> Is there an API to determine the currently operating performance state of
>> a given power domain?  Is this information accessible from userspace?  We
>> will definitely need this for general debugging.
> 
> A quick look shows me its not. I agree its a necessary feature for debug.
> I will add a patch to expose it via debugfs

Thanks


>>> +static int rpmhpd_probe(struct platform_device *pdev)
>>> +{
>>> +   int i, ret;
>>> +   size_t num;
>>> +   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 = desc->num_pds;
>>> +
>>> +   data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
>>> +   if (!data)
>>> +   return -ENOMEM;
>>> +
>>> +   data->domains = devm_kcalloc(>dev, num, sizeof(*data->domains),
>>> +GFP_KERNEL);
>>> +   data->num_domains = num;
>>> +
>>> +   ret = cmd_db_ready();
>>> +   if (ret) {
>>> +   if (ret != -EPROBE_DEFER)
>>> +   dev_err(>dev, "Command DB unavailable, ret=%d\n",
>>> +   ret);
>>> +   return ret;
>>> +   }
>>> +
>>> +   for (i = 0; i < num; i++) {
>>> +   if (!rpmhpds[i])
>>> +   continue;
>>
>> Why is this check needed?
> 
> Just to check/ignore if there are any holes.
> maybe I should atleast throw a warning instead of silently ignoring it?

A warning message might be a good idea if this condition should ever be
reached but also doesn't necessarily imply that probing must be ceased.
It looks like of_genpd_add_provider_onecell() ignores the NULL initialized
data->domains[i] values so it should be safe to leave the holes in and not
decrement num_domains accordingly.

Take care,
David

[1]: https://lkml.org/lkml/2018/3/21/681

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 05/29/2018 03:19 AM, Rajendra Nayak wrote:
> On 05/26/2018 06:38 AM, David Collins wrote:
>> On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
>>> The RPMh powerdomain driver aggregates the corner votes from various
...
>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>> index a7a405178967..1faed239701d 100644
>>> --- a/drivers/soc/qcom/Kconfig
>>> +++ b/drivers/soc/qcom/Kconfig
>>> @@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
>>>  
>>>   Say y here if you intend to boot the modem remoteproc.
>>>  
>>> +config QCOM_RPMHPD
>>> +   tristate "Qualcomm RPMh Powerdomain driver"
>>
>> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> All other config options in qcom/Kconfig use 'Qualcomm XYZ feature'
> for the comment. Maybe I will leave it that way for consistency?

I don't have a strong opinion about it.  I just want the legal folks to be
happy.  I'm fine with whatever achieves that goal.


>>> +
>>> +struct rpmhpd_desc {
>>> +   struct rpmhpd **rpmhpds;
>>> +   size_t num_pds;
>>> +};
>>
>> This struct could be removed and the per-platform arrays could instead be
>> NULL terminated.
> 
> Yes, but I would prefer it this way unless you have strong objections.
> Just makes it easier to do the allocations at probe for genpd_onecell_data 
> structures.

I'm fine if you keep it as-is.  I mentioned the alternative because
Stephen had requested the same modification on my qcom-rpmh-regulator
driver patch [1].  Other reviewers may care about this point.


>> Is there an API to determine the currently operating performance state of
>> a given power domain?  Is this information accessible from userspace?  We
>> will definitely need this for general debugging.
> 
> A quick look shows me its not. I agree its a necessary feature for debug.
> I will add a patch to expose it via debugfs

Thanks


>>> +static int rpmhpd_probe(struct platform_device *pdev)
>>> +{
>>> +   int i, ret;
>>> +   size_t num;
>>> +   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 = desc->num_pds;
>>> +
>>> +   data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
>>> +   if (!data)
>>> +   return -ENOMEM;
>>> +
>>> +   data->domains = devm_kcalloc(>dev, num, sizeof(*data->domains),
>>> +GFP_KERNEL);
>>> +   data->num_domains = num;
>>> +
>>> +   ret = cmd_db_ready();
>>> +   if (ret) {
>>> +   if (ret != -EPROBE_DEFER)
>>> +   dev_err(>dev, "Command DB unavailable, ret=%d\n",
>>> +   ret);
>>> +   return ret;
>>> +   }
>>> +
>>> +   for (i = 0; i < num; i++) {
>>> +   if (!rpmhpds[i])
>>> +   continue;
>>
>> Why is this check needed?
> 
> Just to check/ignore if there are any holes.
> maybe I should atleast throw a warning instead of silently ignoring it?

A warning message might be a good idea if this condition should ever be
reached but also doesn't necessarily imply that probing must be ceased.
It looks like of_genpd_add_provider_onecell() ignores the NULL initialized
data->domains[i] values so it should be safe to leave the holes in and not
decrement num_domains accordingly.

Take care,
David

[1]: https://lkml.org/lkml/2018/3/21/681

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

Hi David,

On 05/26/2018 06:38 AM, David Collins wrote:
> Hello Rajendra,
> 
> On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
>> The RPMh powerdomain driver aggregates the corner votes from various
> 
> s/powerdomain/power domain/
> 
> This applies to all instances in this patch.  "Power domain" seems to be
> the preferred spelling in the kernel.

thanks, will change in all applicable places.

> 
> 
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all powerdomains on sdm845 as part of the patch.
>> The driver can be extended to support other SoCs which support RPMh
>>
>> Signed-off-by: Rajendra Nayak 
>> ---
>>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
>>  drivers/soc/qcom/Kconfig  |   9 +
>>  drivers/soc/qcom/Makefile |   1 +
>>  drivers/soc/qcom/rpmhpd.c | 360 ++
>>  4 files changed, 435 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
> ...
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> 
> I think that this binding documentation should be in a patch separate from
> the driver.

yes, will split it when I repost

> 
> 
>> @@ -0,0 +1,65 @@
>> +Qualcomm RPMh Powerdomains
> 
> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> 
>> +
>> +* For RPMh powerdomains, we communicate a performance state to RPMh
> 
> Does this line need to start with '*'?

No, will remove it

> 
> 
>> +which then translates it into a corresponding voltage on a rail
>> +
>> +Required Properties:
>> + - compatible: Should be one of the following
>> +* qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
>> + - power-domain-cells: number of cells in power domain specifier
>> +must be 1
>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
>> +Refer to Documentation/devicetree/bindings/power/power_domain.txt
>> +and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
>> +
>> +Example:
>> +
>> +rpmhpd: power-controller {
>> +compatible = "qcom,sdm845-rpmhpd";
>> +#power-domain-cells = <1>;
>> +operating-points-v2 = <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>;
> 
> Can this be changed to simply:
>   operating-points-v2 = <_opp_table>;
> 
> The opp binding documentation [1] states that this should be ok:
> 
> If only one phandle is available, then the same OPP table will be used
> for all power domains provided by the power domain provider.

thanks, I mentioned this to Viresh but didn't realize he fixed it up.
Will remove the redundant entries.

> 
> 
>> +};
>> +
>> +rpmhpd_opp_table: opp-table {
>> +compatible = "operating-points-v2-qcom-level", 
>> "operating-points-v2";
>> +
>> +rpmhpd_opp1: opp@1 {
> 
> Is there any significance to the 1 through 8 values in these OPP table
> nodes?  If not, then could this be changed to something like:
> 
> rpmhpd_retention: opp@16 {
> ...
> rpmhpd_turbo_l1: opp@416 {
> 
> 
>> +qcom-corner = <16>;
> 
> s/qcom-corner/qcom,level/g

Thanks, I'll fix these up.
> 
> 
>> +};
>> +
>> +rpmhpd_opp2: opp@2 {
>> +qcom-corner = <48>;
>> +};
>> +
>> +rpmhpd_opp3: opp@3 {
>> +qcom-corner = <64>;
>> +};
>> +
>> +rpmhpd_opp4: opp@4 {
>> +qcom-corner = <128>;
>> +};
>> +
>> +rpmhpd_opp5: opp@5 {
>> +qcom-corner = <192>;
>> +};
>> +
>> +rpmhpd_opp6: opp@6 {
>> +qcom-corner = <256>;
>> +};
>> +
>> +rpmhpd_opp7: opp@7 {
>> +qcom-corner = <320>;
>> +};
> 
> Can you please add 336 and 384 to your example?  384 at least should be
> present as it corresponds to the Turbo level which all supplies support.

will do.

> 
> 
>> +rpmhpd_opp8: opp@8 {
>> +qcom-corner = <416>;
>> +};
>> +};
> 
> How are consumers of these power domains supposed to identify which domain
> within <> to use (e.g. VDD_CX vs VDD_MX)?  If the answer is a plain
> integer index, then could you please add per-platform #define constants in
> a DT header file which explicitly define the meaning for each index?
> 
> How do consumers of these power domains identify which level they want to
> set for a specific power domain (e.g. Nominal vs Turbo)?
> 
> Would it be 

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

Hi David,

On 05/26/2018 06:38 AM, David Collins wrote:
> Hello Rajendra,
> 
> On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
>> The RPMh powerdomain driver aggregates the corner votes from various
> 
> s/powerdomain/power domain/
> 
> This applies to all instances in this patch.  "Power domain" seems to be
> the preferred spelling in the kernel.

thanks, will change in all applicable places.

> 
> 
>> consumers for the ARC resources and communicates it to RPMh.
>>
>> We also add data for all powerdomains on sdm845 as part of the patch.
>> The driver can be extended to support other SoCs which support RPMh
>>
>> Signed-off-by: Rajendra Nayak 
>> ---
>>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
>>  drivers/soc/qcom/Kconfig  |   9 +
>>  drivers/soc/qcom/Makefile |   1 +
>>  drivers/soc/qcom/rpmhpd.c | 360 ++
>>  4 files changed, 435 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>>  create mode 100644 drivers/soc/qcom/rpmhpd.c
> ...
>> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
> 
> I think that this binding documentation should be in a patch separate from
> the driver.

yes, will split it when I repost

> 
> 
>> @@ -0,0 +1,65 @@
>> +Qualcomm RPMh Powerdomains
> 
> s/Qualcomm/Qualcomm Technologies, Inc./
> 
> 
>> +
>> +* For RPMh powerdomains, we communicate a performance state to RPMh
> 
> Does this line need to start with '*'?

No, will remove it

> 
> 
>> +which then translates it into a corresponding voltage on a rail
>> +
>> +Required Properties:
>> + - compatible: Should be one of the following
>> +* qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
>> + - power-domain-cells: number of cells in power domain specifier
>> +must be 1
>> + - operating-points-v2: Phandle to the OPP table for the power-domain.
>> +Refer to Documentation/devicetree/bindings/power/power_domain.txt
>> +and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
>> +
>> +Example:
>> +
>> +rpmhpd: power-controller {
>> +compatible = "qcom,sdm845-rpmhpd";
>> +#power-domain-cells = <1>;
>> +operating-points-v2 = <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>,
>> +  <_opp_table>;
> 
> Can this be changed to simply:
>   operating-points-v2 = <_opp_table>;
> 
> The opp binding documentation [1] states that this should be ok:
> 
> If only one phandle is available, then the same OPP table will be used
> for all power domains provided by the power domain provider.

thanks, I mentioned this to Viresh but didn't realize he fixed it up.
Will remove the redundant entries.

> 
> 
>> +};
>> +
>> +rpmhpd_opp_table: opp-table {
>> +compatible = "operating-points-v2-qcom-level", 
>> "operating-points-v2";
>> +
>> +rpmhpd_opp1: opp@1 {
> 
> Is there any significance to the 1 through 8 values in these OPP table
> nodes?  If not, then could this be changed to something like:
> 
> rpmhpd_retention: opp@16 {
> ...
> rpmhpd_turbo_l1: opp@416 {
> 
> 
>> +qcom-corner = <16>;
> 
> s/qcom-corner/qcom,level/g

Thanks, I'll fix these up.
> 
> 
>> +};
>> +
>> +rpmhpd_opp2: opp@2 {
>> +qcom-corner = <48>;
>> +};
>> +
>> +rpmhpd_opp3: opp@3 {
>> +qcom-corner = <64>;
>> +};
>> +
>> +rpmhpd_opp4: opp@4 {
>> +qcom-corner = <128>;
>> +};
>> +
>> +rpmhpd_opp5: opp@5 {
>> +qcom-corner = <192>;
>> +};
>> +
>> +rpmhpd_opp6: opp@6 {
>> +qcom-corner = <256>;
>> +};
>> +
>> +rpmhpd_opp7: opp@7 {
>> +qcom-corner = <320>;
>> +};
> 
> Can you please add 336 and 384 to your example?  384 at least should be
> present as it corresponds to the Turbo level which all supplies support.

will do.

> 
> 
>> +rpmhpd_opp8: opp@8 {
>> +qcom-corner = <416>;
>> +};
>> +};
> 
> How are consumers of these power domains supposed to identify which domain
> within <> to use (e.g. VDD_CX vs VDD_MX)?  If the answer is a plain
> integer index, then could you please add per-platform #define constants in
> a DT header file which explicitly define the meaning for each index?
> 
> How do consumers of these power domains identify which level they want to
> set for a specific power domain (e.g. Nominal vs Turbo)?
> 
> Would it be 

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
> The RPMh powerdomain driver aggregates the corner votes from various

s/powerdomain/power domain/

This applies to all instances in this patch.  "Power domain" seems to be
the preferred spelling in the kernel.


> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all powerdomains on sdm845 as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
> 
> Signed-off-by: Rajendra Nayak 
> ---
>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
>  drivers/soc/qcom/Kconfig  |   9 +
>  drivers/soc/qcom/Makefile |   1 +
>  drivers/soc/qcom/rpmhpd.c | 360 ++
>  4 files changed, 435 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
...
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt

I think that this binding documentation should be in a patch separate from
the driver.


> @@ -0,0 +1,65 @@
> +Qualcomm RPMh Powerdomains

s/Qualcomm/Qualcomm Technologies, Inc./


> +
> +* For RPMh powerdomains, we communicate a performance state to RPMh

Does this line need to start with '*'?


> +which then translates it into a corresponding voltage on a rail
> +
> +Required Properties:
> + - compatible: Should be one of the following
> + * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
> + - power-domain-cells: number of cells in power domain specifier
> + must be 1
> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> + Refer to Documentation/devicetree/bindings/power/power_domain.txt
> + and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> +
> +Example:
> +
> + rpmhpd: power-controller {
> + compatible = "qcom,sdm845-rpmhpd";
> + #power-domain-cells = <1>;
> + operating-points-v2 = <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>;

Can this be changed to simply:
  operating-points-v2 = <_opp_table>;

The opp binding documentation [1] states that this should be ok:

If only one phandle is available, then the same OPP table will be used
for all power domains provided by the power domain provider.


> + };
> +
> + rpmhpd_opp_table: opp-table {
> + compatible = "operating-points-v2-qcom-level", 
> "operating-points-v2";
> +
> + rpmhpd_opp1: opp@1 {

Is there any significance to the 1 through 8 values in these OPP table
nodes?  If not, then could this be changed to something like:

rpmhpd_retention: opp@16 {
...
rpmhpd_turbo_l1: opp@416 {


> + qcom-corner = <16>;

s/qcom-corner/qcom,level/g


> + };
> +
> + rpmhpd_opp2: opp@2 {
> + qcom-corner = <48>;
> + };
> +
> + rpmhpd_opp3: opp@3 {
> + qcom-corner = <64>;
> + };
> +
> + rpmhpd_opp4: opp@4 {
> + qcom-corner = <128>;
> + };
> +
> + rpmhpd_opp5: opp@5 {
> + qcom-corner = <192>;
> + };
> +
> + rpmhpd_opp6: opp@6 {
> + qcom-corner = <256>;
> + };
> +
> + rpmhpd_opp7: opp@7 {
> + qcom-corner = <320>;
> + };

Can you please add 336 and 384 to your example?  384 at least should be
present as it corresponds to the Turbo level which all supplies support.


> + rpmhpd_opp8: opp@8 {
> + qcom-corner = <416>;
> + };
> + };

How are consumers of these power domains supposed to identify which domain
within <> to use (e.g. VDD_CX vs VDD_MX)?  If the answer is a plain
integer index, then could you please add per-platform #define constants in
a DT header file which explicitly define the meaning for each index?

How do consumers of these power domains identify which level they want to
set for a specific power domain (e.g. Nominal vs Turbo)?

Would it be helpful to provide a DT header file with #define constants for
the cross-platform sparse level mapping?  This is done in [2] for the
downstream rpmh-regulator driver that handles ARC managed regulators.


Would it be ok to add some consumer DT nodes in this binding file example
so that it is clear how consumers interact with the rpmhpd?


> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a7a405178967..1faed239701d 

Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[David Collins]

Hello Rajendra,

On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
> The RPMh powerdomain driver aggregates the corner votes from various

s/powerdomain/power domain/

This applies to all instances in this patch.  "Power domain" seems to be
the preferred spelling in the kernel.


> consumers for the ARC resources and communicates it to RPMh.
> 
> We also add data for all powerdomains on sdm845 as part of the patch.
> The driver can be extended to support other SoCs which support RPMh
> 
> Signed-off-by: Rajendra Nayak 
> ---
>  .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
>  drivers/soc/qcom/Kconfig  |   9 +
>  drivers/soc/qcom/Makefile |   1 +
>  drivers/soc/qcom/rpmhpd.c | 360 ++
>  4 files changed, 435 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
>  create mode 100644 drivers/soc/qcom/rpmhpd.c
...
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt

I think that this binding documentation should be in a patch separate from
the driver.


> @@ -0,0 +1,65 @@
> +Qualcomm RPMh Powerdomains

s/Qualcomm/Qualcomm Technologies, Inc./


> +
> +* For RPMh powerdomains, we communicate a performance state to RPMh

Does this line need to start with '*'?


> +which then translates it into a corresponding voltage on a rail
> +
> +Required Properties:
> + - compatible: Should be one of the following
> + * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
> + - power-domain-cells: number of cells in power domain specifier
> + must be 1
> + - operating-points-v2: Phandle to the OPP table for the power-domain.
> + Refer to Documentation/devicetree/bindings/power/power_domain.txt
> + and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
> +
> +Example:
> +
> + rpmhpd: power-controller {
> + compatible = "qcom,sdm845-rpmhpd";
> + #power-domain-cells = <1>;
> + operating-points-v2 = <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>,
> +   <_opp_table>;

Can this be changed to simply:
  operating-points-v2 = <_opp_table>;

The opp binding documentation [1] states that this should be ok:

If only one phandle is available, then the same OPP table will be used
for all power domains provided by the power domain provider.


> + };
> +
> + rpmhpd_opp_table: opp-table {
> + compatible = "operating-points-v2-qcom-level", 
> "operating-points-v2";
> +
> + rpmhpd_opp1: opp@1 {

Is there any significance to the 1 through 8 values in these OPP table
nodes?  If not, then could this be changed to something like:

rpmhpd_retention: opp@16 {
...
rpmhpd_turbo_l1: opp@416 {


> + qcom-corner = <16>;

s/qcom-corner/qcom,level/g


> + };
> +
> + rpmhpd_opp2: opp@2 {
> + qcom-corner = <48>;
> + };
> +
> + rpmhpd_opp3: opp@3 {
> + qcom-corner = <64>;
> + };
> +
> + rpmhpd_opp4: opp@4 {
> + qcom-corner = <128>;
> + };
> +
> + rpmhpd_opp5: opp@5 {
> + qcom-corner = <192>;
> + };
> +
> + rpmhpd_opp6: opp@6 {
> + qcom-corner = <256>;
> + };
> +
> + rpmhpd_opp7: opp@7 {
> + qcom-corner = <320>;
> + };

Can you please add 336 and 384 to your example?  384 at least should be
present as it corresponds to the Turbo level which all supplies support.


> + rpmhpd_opp8: opp@8 {
> + qcom-corner = <416>;
> + };
> + };

How are consumers of these power domains supposed to identify which domain
within <> to use (e.g. VDD_CX vs VDD_MX)?  If the answer is a plain
integer index, then could you please add per-platform #define constants in
a DT header file which explicitly define the meaning for each index?

How do consumers of these power domains identify which level they want to
set for a specific power domain (e.g. Nominal vs Turbo)?

Would it be helpful to provide a DT header file with #define constants for
the cross-platform sparse level mapping?  This is done in [2] for the
downstream rpmh-regulator driver that handles ARC managed regulators.


Would it be ok to add some consumer DT nodes in this binding file example
so that it is clear how consumers interact with the rpmhpd?


> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a7a405178967..1faed239701d 100644
> --- 

[PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

The RPMh powerdomain driver aggregates the corner votes from various
consumers for the ARC resources and communicates it to RPMh.

We also add data for all powerdomains on sdm845 as part of the patch.
The driver can be extended to support other SoCs which support RPMh

Signed-off-by: Rajendra Nayak 
---
 .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
 drivers/soc/qcom/Kconfig  |   9 +
 drivers/soc/qcom/Makefile |   1 +
 drivers/soc/qcom/rpmhpd.c | 360 ++
 4 files changed, 435 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
 create mode 100644 drivers/soc/qcom/rpmhpd.c

diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt 
b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
new file mode 100644
index ..c1fa986c12ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
@@ -0,0 +1,65 @@
+Qualcomm RPMh Powerdomains
+
+* For RPMh powerdomains, we communicate a performance state to RPMh
+which then translates it into a corresponding voltage on a rail
+
+Required Properties:
+ - compatible: Should be one of the following
+   * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
+ - power-domain-cells: number of cells in power domain specifier
+   must be 1
+ - operating-points-v2: Phandle to the OPP table for the power-domain.
+   Refer to Documentation/devicetree/bindings/power/power_domain.txt
+   and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
+
+Example:
+
+   rpmhpd: power-controller {
+   compatible = "qcom,sdm845-rpmhpd";
+   #power-domain-cells = <1>;
+   operating-points-v2 = <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>;
+   };
+
+   rpmhpd_opp_table: opp-table {
+   compatible = "operating-points-v2-qcom-level", 
"operating-points-v2";
+
+   rpmhpd_opp1: opp@1 {
+   qcom-corner = <16>;
+   };
+
+   rpmhpd_opp2: opp@2 {
+   qcom-corner = <48>;
+   };
+
+   rpmhpd_opp3: opp@3 {
+   qcom-corner = <64>;
+   };
+
+   rpmhpd_opp4: opp@4 {
+   qcom-corner = <128>;
+   };
+
+   rpmhpd_opp5: opp@5 {
+   qcom-corner = <192>;
+   };
+
+   rpmhpd_opp6: opp@6 {
+   qcom-corner = <256>;
+   };
+
+   rpmhpd_opp7: opp@7 {
+   qcom-corner = <320>;
+   };
+
+   rpmhpd_opp8: opp@8 {
+   qcom-corner = <416>;
+   };
+   };
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a7a405178967..1faed239701d 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
 
  Say y here if you intend to boot the modem remoteproc.
 
+config QCOM_RPMHPD
+   tristate "Qualcomm RPMh Powerdomain driver"
+   depends on QCOM_RPMH && QCOM_COMMAND_DB
+   help
+ QCOM RPMh powerdomain driver to support powerdomain with
+ performance states. The driver communicates a performance state
+ value to RPMh which then translates it into corresponding voltage
+ for the voltage rail.
+
 config QCOM_RPMPD
tristate "Qualcomm RPM Powerdomain driver"
depends on MFD_QCOM_RPM && QCOM_SMD_RPM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 9550c170de93..499513f63bef 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)   += smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
+obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
new file mode 100644
index ..a79f094ad326
--- /dev/null
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -0,0 +1,360 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
+
+#define DEFINE_RPMHPD_AO(_platform, _name, _active)\
+   static struct 

[PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Rajendra Nayak]

The RPMh powerdomain driver aggregates the corner votes from various
consumers for the ARC resources and communicates it to RPMh.

We also add data for all powerdomains on sdm845 as part of the patch.
The driver can be extended to support other SoCs which support RPMh

Signed-off-by: Rajendra Nayak 
---
 .../devicetree/bindings/power/qcom,rpmhpd.txt |  65 
 drivers/soc/qcom/Kconfig  |   9 +
 drivers/soc/qcom/Makefile |   1 +
 drivers/soc/qcom/rpmhpd.c | 360 ++
 4 files changed, 435 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
 create mode 100644 drivers/soc/qcom/rpmhpd.c

diff --git a/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt 
b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
new file mode 100644
index ..c1fa986c12ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,rpmhpd.txt
@@ -0,0 +1,65 @@
+Qualcomm RPMh Powerdomains
+
+* For RPMh powerdomains, we communicate a performance state to RPMh
+which then translates it into a corresponding voltage on a rail
+
+Required Properties:
+ - compatible: Should be one of the following
+   * qcom,sdm845-rpmhpd: RPMh powerdomain for the sdm845 family of SoC
+ - power-domain-cells: number of cells in power domain specifier
+   must be 1
+ - operating-points-v2: Phandle to the OPP table for the power-domain.
+   Refer to Documentation/devicetree/bindings/power/power_domain.txt
+   and Documentation/devicetree/bindings/opp/qcom-opp.txt for more details
+
+Example:
+
+   rpmhpd: power-controller {
+   compatible = "qcom,sdm845-rpmhpd";
+   #power-domain-cells = <1>;
+   operating-points-v2 = <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>,
+ <_opp_table>;
+   };
+
+   rpmhpd_opp_table: opp-table {
+   compatible = "operating-points-v2-qcom-level", 
"operating-points-v2";
+
+   rpmhpd_opp1: opp@1 {
+   qcom-corner = <16>;
+   };
+
+   rpmhpd_opp2: opp@2 {
+   qcom-corner = <48>;
+   };
+
+   rpmhpd_opp3: opp@3 {
+   qcom-corner = <64>;
+   };
+
+   rpmhpd_opp4: opp@4 {
+   qcom-corner = <128>;
+   };
+
+   rpmhpd_opp5: opp@5 {
+   qcom-corner = <192>;
+   };
+
+   rpmhpd_opp6: opp@6 {
+   qcom-corner = <256>;
+   };
+
+   rpmhpd_opp7: opp@7 {
+   qcom-corner = <320>;
+   };
+
+   rpmhpd_opp8: opp@8 {
+   qcom-corner = <416>;
+   };
+   };
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a7a405178967..1faed239701d 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -74,6 +74,15 @@ config QCOM_RMTFS_MEM
 
  Say y here if you intend to boot the modem remoteproc.
 
+config QCOM_RPMHPD
+   tristate "Qualcomm RPMh Powerdomain driver"
+   depends on QCOM_RPMH && QCOM_COMMAND_DB
+   help
+ QCOM RPMh powerdomain driver to support powerdomain with
+ performance states. The driver communicates a performance state
+ value to RPMh which then translates it into corresponding voltage
+ for the voltage rail.
+
 config QCOM_RPMPD
tristate "Qualcomm RPM Powerdomain driver"
depends on MFD_QCOM_RPM && QCOM_SMD_RPM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 9550c170de93..499513f63bef 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_QCOM_SMSM)   += smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
+obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
new file mode 100644
index ..a79f094ad326
--- /dev/null
+++ b/drivers/soc/qcom/rpmhpd.c
@@ -0,0 +1,360 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define domain_to_rpmhpd(domain) container_of(domain, struct rpmhpd, pd)
+
+#define DEFINE_RPMHPD_AO(_platform, _name, _active)\
+   static struct rpmhpd