Re: [PATCH v3 0/9] RPMPD for QCS404 and MSM8998

Re: [PATCH v3 0/9] RPMPD for QCS404 and MSM8998

[Marc Gonzalez]

On 27/03/2019 13:38, Sibi Sankar wrote:

> Re-worked the macros of the rpmpd driver. Add power domains support
> for QCS404 and MSM8998.
> 
> V3:
> * always send level updates to vfc and vfl in set_performance state
> * fixup commit messages [Rajendra]
> * fixup s-o-b ordering
> 
> V2:
> * Add rpmpd support for msm8998
> * fixup corner/vfc with vlfl/vfl
> 
> Bjorn Andersson (4):
>   soc: qcom: rpmpd: Modify corner defining macros
>   dt-bindings: power: Add rpm power domain bindings for qcs404
>   soc: qcom: rpmpd: Add QCS404 power-domains
>   arm64: dts: qcom: qcs404: Add rpmpd node
> 
> Sibi Sankar (5):
>   soc: qcom: rpmpd: fixup rpmpd set performance state
>   soc: qcom: rpmpd: Add support to set rpmpd state to max
>   dt-bindings: power: Add rpm power domain bindings for msm8998
>   soc: qcom: rpmpd: Add MSM8998 power-domains
>   arm64: dts: qcom: msm8998: Add rpmpd node
> 
>  .../devicetree/bindings/power/qcom,rpmpd.txt  |   2 +
>  arch/arm64/boot/dts/qcom/msm8998.dtsi         |  51 +++++++
>  arch/arm64/boot/dts/qcom/qcs404.dtsi          |  55 +++++++
>  drivers/soc/qcom/rpmpd.c                      | 135 ++++++++++++++----
>  include/dt-bindings/power/qcom-rpmpd.h        |  34 +++++
>  5 files changed, 252 insertions(+), 25 deletions(-)

Hello Sibi,

Please note that you sent this series to linux-arm-msm-owner@vger.kernel.org
instead of linux-arm-msm@vger.kernel.org (the admin instead of the list).

This seems to be the case for other patches as well.

Regards.

Re: [PATCH v3 0/9] RPMPD for QCS404 and MSM8998

[Marc Gonzalez]

On 27/03/2019 13:38, Sibi Sankar wrote:

> Re-worked the macros of the rpmpd driver. Add power domains support
> for QCS404 and MSM8998.
> 
> V3:
> * always send level updates to vfc and vfl in set_performance state
> * fixup commit messages [Rajendra]
> * fixup s-o-b ordering
> 
> V2:
> * Add rpmpd support for msm8998
> * fixup corner/vfc with vlfl/vfl
> 
> Bjorn Andersson (4):
>   soc: qcom: rpmpd: Modify corner defining macros
>   dt-bindings: power: Add rpm power domain bindings for qcs404
>   soc: qcom: rpmpd: Add QCS404 power-domains
>   arm64: dts: qcom: qcs404: Add rpmpd node
> 
> Sibi Sankar (5):
>   soc: qcom: rpmpd: fixup rpmpd set performance state
>   soc: qcom: rpmpd: Add support to set rpmpd state to max
>   dt-bindings: power: Add rpm power domain bindings for msm8998
>   soc: qcom: rpmpd: Add MSM8998 power-domains
>   arm64: dts: qcom: msm8998: Add rpmpd node
> 
>  .../devicetree/bindings/power/qcom,rpmpd.txt  |   2 +
>  arch/arm64/boot/dts/qcom/msm8998.dtsi         |  51 +++++++
>  arch/arm64/boot/dts/qcom/qcs404.dtsi          |  55 +++++++
>  drivers/soc/qcom/rpmpd.c                      | 135 ++++++++++++++----
>  include/dt-bindings/power/qcom-rpmpd.h        |  34 +++++
>  5 files changed, 252 insertions(+), 25 deletions(-)

Hello Sibi,

Please note that you sent this series to linux-arm-msm-owner@vger.kernel.org
instead of linux-arm-msm@vger.kernel.org (the admin instead of the list).

This seems to be the case for other patches as well.

Regards.

Re: [PATCH v3 1/9] soc: qcom: rpmpd: fixup rpmpd set performance state

[Marc Gonzalez]

On 27/03/2019 13:38, Sibi Sankar wrote:

> Remoteproc q6v5-mss does set_performace_state with INT_MAX on

Remoteproc q6v5-mss calls set_performance_state()

> rpmpd. This is currently ignored since it is greater than the
> max supported state. Fixup rpmpd state to max if the required
> state is greater than all the supported states.
> 
> Fixes: 075d3db8d10d ("Add support for the .set_performace_state() and
> .opp_to_performance_state()")
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>

AFAIU, the Fixes tag should be on a single line, and quote the commit subject.
(And no blank line between Fixes and Signed-off-by tags.)

Fixes: 075d3db8d10d ("soc: qcom: rpmpd: Add support for get/set performance state")

> ---
>  drivers/soc/qcom/rpmpd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> index 005326050c23..235d01870dd8 100644
> --- a/drivers/soc/qcom/rpmpd.c
> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -226,7 +226,7 @@ static int rpmpd_set_performance(struct generic_pm_domain *domain,
>  	struct rpmpd *pd = domain_to_rpmpd(domain);
>  
>  	if (state > MAX_RPMPD_STATE)
> -		goto out;
> +		state = MAX_RPMPD_STATE;
>  
>  	mutex_lock(&rpmpd_lock);
>  

With these two nits taken care of:

Reviewed-by: Marc Gonzalez <marc.w.gonzalez@free.fr>

Regards.

Re: [PATCH v3 1/9] soc: qcom: rpmpd: fixup rpmpd set performance state

[Marc Gonzalez]

On 27/03/2019 13:38, Sibi Sankar wrote:

> Remoteproc q6v5-mss does set_performace_state with INT_MAX on

Remoteproc q6v5-mss calls set_performance_state()

> rpmpd. This is currently ignored since it is greater than the
> max supported state. Fixup rpmpd state to max if the required
> state is greater than all the supported states.
> 
> Fixes: 075d3db8d10d ("Add support for the .set_performace_state() and
> .opp_to_performance_state()")
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>

AFAIU, the Fixes tag should be on a single line, and quote the commit subject.
(And no blank line between Fixes and Signed-off-by tags.)

Fixes: 075d3db8d10d ("soc: qcom: rpmpd: Add support for get/set performance state")

> ---
>  drivers/soc/qcom/rpmpd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> index 005326050c23..235d01870dd8 100644
> --- a/drivers/soc/qcom/rpmpd.c
> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -226,7 +226,7 @@ static int rpmpd_set_performance(struct generic_pm_domain *domain,
>  	struct rpmpd *pd = domain_to_rpmpd(domain);
>  
>  	if (state > MAX_RPMPD_STATE)
> -		goto out;
> +		state = MAX_RPMPD_STATE;
>  
>  	mutex_lock(&rpmpd_lock);
>  

With these two nits taken care of:

Reviewed-by: Marc Gonzalez <marc.w.gonzalez@free.fr>

Regards.

Re: [PATCH v3 8/9] soc: qcom: rpmpd: Add MSM8998 power-domains

[Marc Gonzalez]

On 27/03/2019 13:38, Sibi Sankar wrote:

> Add the shared cx/mx and sensor sub-system's cx and mx
> power-domains found on MSM8998.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/soc/qcom/rpmpd.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> index 238a9e02e890..706a3f63038e 100644
> --- a/drivers/soc/qcom/rpmpd.c
> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -19,9 +19,12 @@
>  /* Resource types */
>  #define RPMPD_SMPA 0x61706d73 /* smpa */
>  #define RPMPD_LDOA 0x616f646c /* ldoa */
> +#define RPMPD_RWCX 0x78637772 /* rwcx */
>  #define RPMPD_RWMX 0x786d7772 /* rwmx */
>  #define RPMPD_RWLC 0x636c7772 /* rwlc */
>  #define RPMPD_RWLM 0x6d6c7772 /* rwlm */
> +#define RPMPD_RWSC 0x63737772 /* rwsc */
> +#define RPMPD_RWSM 0x6d737772 /* rwsm */

I do not see any value in the comments. Maybe remove them?

I will take a closer look at patches 7-9 on Monday.

Regards.

Re: [PATCH v3 8/9] soc: qcom: rpmpd: Add MSM8998 power-domains

[Marc Gonzalez]

On 27/03/2019 13:38, Sibi Sankar wrote:

> Add the shared cx/mx and sensor sub-system's cx and mx
> power-domains found on MSM8998.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/soc/qcom/rpmpd.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> index 238a9e02e890..706a3f63038e 100644
> --- a/drivers/soc/qcom/rpmpd.c
> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -19,9 +19,12 @@
>  /* Resource types */
>  #define RPMPD_SMPA 0x61706d73 /* smpa */
>  #define RPMPD_LDOA 0x616f646c /* ldoa */
> +#define RPMPD_RWCX 0x78637772 /* rwcx */
>  #define RPMPD_RWMX 0x786d7772 /* rwmx */
>  #define RPMPD_RWLC 0x636c7772 /* rwlc */
>  #define RPMPD_RWLM 0x6d6c7772 /* rwlm */
> +#define RPMPD_RWSC 0x63737772 /* rwsc */
> +#define RPMPD_RWSM 0x6d737772 /* rwsm */

I do not see any value in the comments. Maybe remove them?

I will take a closer look at patches 7-9 on Monday.

Regards.

Re: [PATCH v3 8/9] soc: qcom: rpmpd: Add MSM8998 power-domains

[Sibi Sankar]

On 2019-04-05 20:38, Marc Gonzalez wrote:
> On 27/03/2019 13:38, Sibi Sankar wrote:
> 
>> Add the shared cx/mx and sensor sub-system's cx and mx
>> power-domains found on MSM8998.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/soc/qcom/rpmpd.c | 36 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>> 
>> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
>> index 238a9e02e890..706a3f63038e 100644
>> --- a/drivers/soc/qcom/rpmpd.c
>> +++ b/drivers/soc/qcom/rpmpd.c
>> @@ -19,9 +19,12 @@
>>  /* Resource types */
>>  #define RPMPD_SMPA 0x61706d73 /* smpa */
>>  #define RPMPD_LDOA 0x616f646c /* ldoa */
>> +#define RPMPD_RWCX 0x78637772 /* rwcx */
>>  #define RPMPD_RWMX 0x786d7772 /* rwmx */
>>  #define RPMPD_RWLC 0x636c7772 /* rwlc */
>>  #define RPMPD_RWLM 0x6d6c7772 /* rwlm */
>> +#define RPMPD_RWSC 0x63737772 /* rwsc */
>> +#define RPMPD_RWSM 0x6d737772 /* rwsm */
> 
> I do not see any value in the comments. Maybe remove them?

comments were included to add value
though, however I guess the comments
were definitely not clear enough.
The magic values for the resources
are calculated as follows:

ascii to hex in reverse order
eg: smpa -> 0x61706d73

0x61 0x70 0x6d 0x73
   a    p    m    s

> 
> I will take a closer look at patches 7-9 on Monday.

Thanks for taking time to review
this series.

> 
> Regards.

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

Re: [PATCH v3 8/9] soc: qcom: rpmpd: Add MSM8998 power-domains

[Sibi Sankar]

On 2019-04-05 20:38, Marc Gonzalez wrote:
> On 27/03/2019 13:38, Sibi Sankar wrote:
> 
>> Add the shared cx/mx and sensor sub-system's cx and mx
>> power-domains found on MSM8998.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/soc/qcom/rpmpd.c | 36 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>> 
>> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
>> index 238a9e02e890..706a3f63038e 100644
>> --- a/drivers/soc/qcom/rpmpd.c
>> +++ b/drivers/soc/qcom/rpmpd.c
>> @@ -19,9 +19,12 @@
>>  /* Resource types */
>>  #define RPMPD_SMPA 0x61706d73 /* smpa */
>>  #define RPMPD_LDOA 0x616f646c /* ldoa */
>> +#define RPMPD_RWCX 0x78637772 /* rwcx */
>>  #define RPMPD_RWMX 0x786d7772 /* rwmx */
>>  #define RPMPD_RWLC 0x636c7772 /* rwlc */
>>  #define RPMPD_RWLM 0x6d6c7772 /* rwlm */
>> +#define RPMPD_RWSC 0x63737772 /* rwsc */
>> +#define RPMPD_RWSM 0x6d737772 /* rwsm */
> 
> I do not see any value in the comments. Maybe remove them?

comments were included to add value
though, however I guess the comments
were definitely not clear enough.
The magic values for the resources
are calculated as follows:

ascii to hex in reverse order
eg: smpa -> 0x61706d73

0x61 0x70 0x6d 0x73
   a    p    m    s

> 
> I will take a closer look at patches 7-9 on Monday.

Thanks for taking time to review
this series.

> 
> Regards.

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

Re: [PATCH v3 8/9] soc: qcom: rpmpd: Add MSM8998 power-domains

[Marc Gonzalez]

On 08/04/2019 10:30, Sibi Sankar wrote:
> On 2019-04-05 20:38, Marc Gonzalez wrote:
>> On 27/03/2019 13:38, Sibi Sankar wrote:
>>
>>> Add the shared cx/mx and sensor sub-system's cx and mx
>>> power-domains found on MSM8998.
>>>
>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>> ---
>>>  drivers/soc/qcom/rpmpd.c | 36 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
>>> index 238a9e02e890..706a3f63038e 100644
>>> --- a/drivers/soc/qcom/rpmpd.c
>>> +++ b/drivers/soc/qcom/rpmpd.c
>>> @@ -19,9 +19,12 @@
>>>  /* Resource types */
>>>  #define RPMPD_SMPA 0x61706d73 /* smpa */
>>>  #define RPMPD_LDOA 0x616f646c /* ldoa */
>>> +#define RPMPD_RWCX 0x78637772 /* rwcx */
>>>  #define RPMPD_RWMX 0x786d7772 /* rwmx */
>>>  #define RPMPD_RWLC 0x636c7772 /* rwlc */
>>>  #define RPMPD_RWLM 0x6d6c7772 /* rwlm */
>>> +#define RPMPD_RWSC 0x63737772 /* rwsc */
>>> +#define RPMPD_RWSM 0x6d737772 /* rwsm */
>>
>> I do not see any value in the comments. Maybe remove them?
> 
> comments were included to add value
> though, however I guess the comments
> were definitely not clear enough.
> The magic values for the resources
> are calculated as follows:
> 
> ascii to hex in reverse order
> eg: smpa -> 0x61706d73
> 
> 0x61 0x70 0x6d 0x73
>    a    p    m    s

Ah... I see now.

I agree that explaining *why* e.g. RPMPD_SMPA is defined as 0x61706d73
is worthwhile indeed.

What I meant is that adding /* smpa */ to a macro named RPMPD_SMPA does
not really bring any new information ;-)

How about prefixing the whole block with a small blurb, for example

/* The value of RPMPD_X is X encoded as a little-endian, lower-case, ASCII string */

Regards.

Re: [PATCH v3 8/9] soc: qcom: rpmpd: Add MSM8998 power-domains

[Marc Gonzalez]

On 08/04/2019 10:30, Sibi Sankar wrote:
> On 2019-04-05 20:38, Marc Gonzalez wrote:
>> On 27/03/2019 13:38, Sibi Sankar wrote:
>>
>>> Add the shared cx/mx and sensor sub-system's cx and mx
>>> power-domains found on MSM8998.
>>>
>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>> ---
>>>  drivers/soc/qcom/rpmpd.c | 36 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
>>> index 238a9e02e890..706a3f63038e 100644
>>> --- a/drivers/soc/qcom/rpmpd.c
>>> +++ b/drivers/soc/qcom/rpmpd.c
>>> @@ -19,9 +19,12 @@
>>>  /* Resource types */
>>>  #define RPMPD_SMPA 0x61706d73 /* smpa */
>>>  #define RPMPD_LDOA 0x616f646c /* ldoa */
>>> +#define RPMPD_RWCX 0x78637772 /* rwcx */
>>>  #define RPMPD_RWMX 0x786d7772 /* rwmx */
>>>  #define RPMPD_RWLC 0x636c7772 /* rwlc */
>>>  #define RPMPD_RWLM 0x6d6c7772 /* rwlm */
>>> +#define RPMPD_RWSC 0x63737772 /* rwsc */
>>> +#define RPMPD_RWSM 0x6d737772 /* rwsm */
>>
>> I do not see any value in the comments. Maybe remove them?
> 
> comments were included to add value
> though, however I guess the comments
> were definitely not clear enough.
> The magic values for the resources
> are calculated as follows:
> 
> ascii to hex in reverse order
> eg: smpa -> 0x61706d73
> 
> 0x61 0x70 0x6d 0x73
>    a    p    m    s

Ah... I see now.

I agree that explaining *why* e.g. RPMPD_SMPA is defined as 0x61706d73
is worthwhile indeed.

What I meant is that adding /* smpa */ to a macro named RPMPD_SMPA does
not really bring any new information ;-)

How about prefixing the whole block with a small blurb, for example

/* The value of RPMPD_X is X encoded as a little-endian, lower-case, ASCII string */

Regards.

Re: [PATCH v3 8/9] soc: qcom: rpmpd: Add MSM8998 power-domains

[Sibi Sankar]

Hey Marc,
Thanks for the review!

On 2019-04-08 14:24, Marc Gonzalez wrote:
> On 08/04/2019 10:30, Sibi Sankar wrote:
>> On 2019-04-05 20:38, Marc Gonzalez wrote:
>>> On 27/03/2019 13:38, Sibi Sankar wrote:
>>> 
>>>> Add the shared cx/mx and sensor sub-system's cx and mx
>>>> power-domains found on MSM8998.
>>>> 
>>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>>> ---
>>>>  drivers/soc/qcom/rpmpd.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 36 insertions(+)
>>>> 
>>>> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
>>>> index 238a9e02e890..706a3f63038e 100644
>>>> --- a/drivers/soc/qcom/rpmpd.c
>>>> +++ b/drivers/soc/qcom/rpmpd.c
>>>> @@ -19,9 +19,12 @@
>>>>  /* Resource types */
>>>>  #define RPMPD_SMPA 0x61706d73 /* smpa */
>>>>  #define RPMPD_LDOA 0x616f646c /* ldoa */
>>>> +#define RPMPD_RWCX 0x78637772 /* rwcx */
>>>>  #define RPMPD_RWMX 0x786d7772 /* rwmx */
>>>>  #define RPMPD_RWLC 0x636c7772 /* rwlc */
>>>>  #define RPMPD_RWLM 0x6d6c7772 /* rwlm */
>>>> +#define RPMPD_RWSC 0x63737772 /* rwsc */
>>>> +#define RPMPD_RWSM 0x6d737772 /* rwsm */
>>> 
>>> I do not see any value in the comments. Maybe remove them?
>> 
>> comments were included to add value
>> though, however I guess the comments
>> were definitely not clear enough.
>> The magic values for the resources
>> are calculated as follows:
>> 
>> ascii to hex in reverse order
>> eg: smpa -> 0x61706d73
>> 
>> 0x61 0x70 0x6d 0x73
>>    a    p    m    s
> 
> Ah... I see now.
> 
> I agree that explaining *why* e.g. RPMPD_SMPA is defined as 0x61706d73
> is worthwhile indeed.
> 
> What I meant is that adding /* smpa */ to a macro named RPMPD_SMPA does
> not really bring any new information ;-)

yeah I got that

> 
> How about prefixing the whole block with a small blurb, for example
> 
> /* The value of RPMPD_X is X encoded as a little-endian, lower-case,
> ASCII string */

sure will add this ^^ instead in the next
re-spin

> 
> Regards.

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

Re: [PATCH v3 8/9] soc: qcom: rpmpd: Add MSM8998 power-domains

[Sibi Sankar]

Hey Marc,
Thanks for the review!

On 2019-04-08 14:24, Marc Gonzalez wrote:
> On 08/04/2019 10:30, Sibi Sankar wrote:
>> On 2019-04-05 20:38, Marc Gonzalez wrote:
>>> On 27/03/2019 13:38, Sibi Sankar wrote:
>>> 
>>>> Add the shared cx/mx and sensor sub-system's cx and mx
>>>> power-domains found on MSM8998.
>>>> 
>>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>>> ---
>>>>  drivers/soc/qcom/rpmpd.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 36 insertions(+)
>>>> 
>>>> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
>>>> index 238a9e02e890..706a3f63038e 100644
>>>> --- a/drivers/soc/qcom/rpmpd.c
>>>> +++ b/drivers/soc/qcom/rpmpd.c
>>>> @@ -19,9 +19,12 @@
>>>>  /* Resource types */
>>>>  #define RPMPD_SMPA 0x61706d73 /* smpa */
>>>>  #define RPMPD_LDOA 0x616f646c /* ldoa */
>>>> +#define RPMPD_RWCX 0x78637772 /* rwcx */
>>>>  #define RPMPD_RWMX 0x786d7772 /* rwmx */
>>>>  #define RPMPD_RWLC 0x636c7772 /* rwlc */
>>>>  #define RPMPD_RWLM 0x6d6c7772 /* rwlm */
>>>> +#define RPMPD_RWSC 0x63737772 /* rwsc */
>>>> +#define RPMPD_RWSM 0x6d737772 /* rwsm */
>>> 
>>> I do not see any value in the comments. Maybe remove them?
>> 
>> comments were included to add value
>> though, however I guess the comments
>> were definitely not clear enough.
>> The magic values for the resources
>> are calculated as follows:
>> 
>> ascii to hex in reverse order
>> eg: smpa -> 0x61706d73
>> 
>> 0x61 0x70 0x6d 0x73
>>    a    p    m    s
> 
> Ah... I see now.
> 
> I agree that explaining *why* e.g. RPMPD_SMPA is defined as 0x61706d73
> is worthwhile indeed.
> 
> What I meant is that adding /* smpa */ to a macro named RPMPD_SMPA does
> not really bring any new information ;-)

yeah I got that

> 
> How about prefixing the whole block with a small blurb, for example
> 
> /* The value of RPMPD_X is X encoded as a little-endian, lower-case,
> ASCII string */

sure will add this ^^ instead in the next
re-spin

> 
> Regards.

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

Re: [PATCH v3 6/9] arm64: dts: qcom: qcs404: Add rpmpd node

[Sibi Sankar]

On 2019-03-27 18:08, Sibi Sankar wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Add the rpmpd node on the qcs404 and define the available levels.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> [sibis: fixup available levels]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/qcs404.dtsi | 55 ++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index e8fd26633d57..a7d46647c416 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -4,6 +4,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-qcs404.h>
>  #include <dt-bindings/clock/qcom,rpmcc.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>
> 
>  / {
>  	interrupt-parent = <&intc>;
> @@ -230,6 +231,60 @@
>  				compatible = "qcom,rpmcc-qcs404";
>  				#clock-cells = <1>;
>  			};
> +
> +			rpmpd: power-controller {
> +				compatible = "qcom,qcs404-rpmpd";
> +				#power-domain-cells = <1>;
> +				operating-points-v2 = <&rpmpd_opp_table>;
> +
> +				rpmpd_opp_table: opp-table {
> +					compatible = "operating-points-v2";
> +
> +					rpmpd_opp_ret: opp1 {
> +						opp-level = <RPM_SMD_LEVEL_RETENTION>;
> +					};
> +
> +					rpmpd_opp_ret_plus: opp2 {
> +						opp-level = <RPM_SMD_LEVEL_RETENTION_PLUS>;
> +					};
> +
> +					rpmpd_opp_min_svs: opp3 {
> +						opp-level = <RPM_SMD_LEVEL_MIN_SVS>;
> +					};
> +
> +					rpmpd_opp_low_svs: opp4 {
> +						opp-level = <RPM_SMD_LEVEL_LOW_SVS>;
> +					};
> +

typo in opp5 - opp11 rpmhpd/rpmpd
will fix it in the next re-spin

> +					rpmhpd_opp_svs: opp5 {
> +						opp-level = <RPM_SMD_LEVEL_SVS>;
> +					};
> +
> +					rpmhpd_opp_svs_plus: opp6 {
> +						opp-level = <RPM_SMD_LEVEL_SVS_PLUS>;
> +					};
> +
> +					rpmhpd_opp_nom: opp7 {
> +						opp-level = <RPM_SMD_LEVEL_NOM>;
> +					};
> +
> +					rpmhpd_opp_nom_plus: opp8 {
> +						opp-level = <RPM_SMD_LEVEL_NOM_PLUS>;
> +					};
> +
> +					rpmhpd_opp_turbo: opp9 {
> +						opp-level = <RPM_SMD_LEVEL_TURBO>;
> +					};
> +
> +					rpmhpd_opp_turbo_no_cpr: opp10 {
> +						opp-level = <RPM_SMD_LEVEL_TURBO_NO_CPR>;
> +					};
> +
> +					rpmhpd_opp_turbo_plus: opp11 {
> +						opp-level = <RPM_SMD_LEVEL_BINNING>;
> +					};
> +				};
> +			};
>  		};
>  	};

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

Re: [PATCH v3 6/9] arm64: dts: qcom: qcs404: Add rpmpd node

[Sibi Sankar]

On 2019-03-27 18:08, Sibi Sankar wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Add the rpmpd node on the qcs404 and define the available levels.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> [sibis: fixup available levels]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/qcs404.dtsi | 55 ++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index e8fd26633d57..a7d46647c416 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -4,6 +4,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-qcs404.h>
>  #include <dt-bindings/clock/qcom,rpmcc.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>
> 
>  / {
>  	interrupt-parent = <&intc>;
> @@ -230,6 +231,60 @@
>  				compatible = "qcom,rpmcc-qcs404";
>  				#clock-cells = <1>;
>  			};
> +
> +			rpmpd: power-controller {
> +				compatible = "qcom,qcs404-rpmpd";
> +				#power-domain-cells = <1>;
> +				operating-points-v2 = <&rpmpd_opp_table>;
> +
> +				rpmpd_opp_table: opp-table {
> +					compatible = "operating-points-v2";
> +
> +					rpmpd_opp_ret: opp1 {
> +						opp-level = <RPM_SMD_LEVEL_RETENTION>;
> +					};
> +
> +					rpmpd_opp_ret_plus: opp2 {
> +						opp-level = <RPM_SMD_LEVEL_RETENTION_PLUS>;
> +					};
> +
> +					rpmpd_opp_min_svs: opp3 {
> +						opp-level = <RPM_SMD_LEVEL_MIN_SVS>;
> +					};
> +
> +					rpmpd_opp_low_svs: opp4 {
> +						opp-level = <RPM_SMD_LEVEL_LOW_SVS>;
> +					};
> +

typo in opp5 - opp11 rpmhpd/rpmpd
will fix it in the next re-spin

> +					rpmhpd_opp_svs: opp5 {
> +						opp-level = <RPM_SMD_LEVEL_SVS>;
> +					};
> +
> +					rpmhpd_opp_svs_plus: opp6 {
> +						opp-level = <RPM_SMD_LEVEL_SVS_PLUS>;
> +					};
> +
> +					rpmhpd_opp_nom: opp7 {
> +						opp-level = <RPM_SMD_LEVEL_NOM>;
> +					};
> +
> +					rpmhpd_opp_nom_plus: opp8 {
> +						opp-level = <RPM_SMD_LEVEL_NOM_PLUS>;
> +					};
> +
> +					rpmhpd_opp_turbo: opp9 {
> +						opp-level = <RPM_SMD_LEVEL_TURBO>;
> +					};
> +
> +					rpmhpd_opp_turbo_no_cpr: opp10 {
> +						opp-level = <RPM_SMD_LEVEL_TURBO_NO_CPR>;
> +					};
> +
> +					rpmhpd_opp_turbo_plus: opp11 {
> +						opp-level = <RPM_SMD_LEVEL_BINNING>;
> +					};
> +				};
> +			};
>  		};
>  	};

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

Re: [PATCH v3 9/9] arm64: dts: qcom: msm8998: Add rpmpd node

[Sibi Sankar]

On 2019-03-27 18:08, Sibi Sankar wrote:
> Add the rpmpd node on the msm8998 and define the available levels.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8998.dtsi | 51 +++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 3fd0769fe648..b7e3e0646b9b 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -4,6 +4,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-msm8998.h>
>  #include <dt-bindings/clock/qcom,rpmcc.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>
>  #include <dt-bindings/gpio/gpio.h>
> 
>  / {
> @@ -272,6 +273,56 @@
>  				compatible = "qcom,rpmcc-msm8998", "qcom,rpmcc";
>  				#clock-cells = <1>;
>  			};
> +
> +			rpmpd: power-controller {
> +				compatible = "qcom,msm8998-rpmpd";
> +				#power-domain-cells = <1>;
> +				operating-points-v2 = <&rpmpd_opp_table>;
> +
> +				rpmpd_opp_table: opp-table {
> +					compatible = "operating-points-v2";
> +
> +					rpmpd_opp_ret: opp1 {
> +						opp-level = <RPM_SMD_LEVEL_RETENTION>;
> +					};
> +
> +					rpmpd_opp_ret_plus: opp2 {
> +						opp-level = <RPM_SMD_LEVEL_RETENTION_PLUS>;
> +					};
> +
> +					rpmpd_opp_min_svs: opp3 {
> +						opp-level = <RPM_SMD_LEVEL_MIN_SVS>;
> +					};
> +
> +					rpmpd_opp_low_svs: opp4 {
> +						opp-level = <RPM_SMD_LEVEL_LOW_SVS>;
> +					};
> +


typo in opp5 - opp10 rpmhpd/rpmpd
will fix it in the next re-spin

> +					rpmhpd_opp_svs: opp5 {
> +						opp-level = <RPM_SMD_LEVEL_SVS>;
> +					};
> +
> +					rpmhpd_opp_svs_plus: opp6 {
> +						opp-level = <RPM_SMD_LEVEL_SVS_PLUS>;
> +					};
> +
> +					rpmhpd_opp_nom: opp7 {
> +						opp-level = <RPM_SMD_LEVEL_NOM>;
> +					};
> +
> +					rpmhpd_opp_nom_plus: opp8 {
> +						opp-level = <RPM_SMD_LEVEL_NOM_PLUS>;
> +					};
> +
> +					rpmhpd_opp_turbo: opp9 {
> +						opp-level = <RPM_SMD_LEVEL_TURBO>;
> +					};
> +
> +					rpmhpd_opp_turbo_plus: opp10 {
> +						opp-level = <RPM_SMD_LEVEL_BINNING>;
> +					};
> +				};
> +			};
>  		};
>  	};

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

Re: [PATCH v3 9/9] arm64: dts: qcom: msm8998: Add rpmpd node

[Sibi Sankar]

On 2019-03-27 18:08, Sibi Sankar wrote:
> Add the rpmpd node on the msm8998 and define the available levels.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8998.dtsi | 51 +++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 3fd0769fe648..b7e3e0646b9b 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -4,6 +4,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/qcom,gcc-msm8998.h>
>  #include <dt-bindings/clock/qcom,rpmcc.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>
>  #include <dt-bindings/gpio/gpio.h>
> 
>  / {
> @@ -272,6 +273,56 @@
>  				compatible = "qcom,rpmcc-msm8998", "qcom,rpmcc";
>  				#clock-cells = <1>;
>  			};
> +
> +			rpmpd: power-controller {
> +				compatible = "qcom,msm8998-rpmpd";
> +				#power-domain-cells = <1>;
> +				operating-points-v2 = <&rpmpd_opp_table>;
> +
> +				rpmpd_opp_table: opp-table {
> +					compatible = "operating-points-v2";
> +
> +					rpmpd_opp_ret: opp1 {
> +						opp-level = <RPM_SMD_LEVEL_RETENTION>;
> +					};
> +
> +					rpmpd_opp_ret_plus: opp2 {
> +						opp-level = <RPM_SMD_LEVEL_RETENTION_PLUS>;
> +					};
> +
> +					rpmpd_opp_min_svs: opp3 {
> +						opp-level = <RPM_SMD_LEVEL_MIN_SVS>;
> +					};
> +
> +					rpmpd_opp_low_svs: opp4 {
> +						opp-level = <RPM_SMD_LEVEL_LOW_SVS>;
> +					};
> +


typo in opp5 - opp10 rpmhpd/rpmpd
will fix it in the next re-spin

> +					rpmhpd_opp_svs: opp5 {
> +						opp-level = <RPM_SMD_LEVEL_SVS>;
> +					};
> +
> +					rpmhpd_opp_svs_plus: opp6 {
> +						opp-level = <RPM_SMD_LEVEL_SVS_PLUS>;
> +					};
> +
> +					rpmhpd_opp_nom: opp7 {
> +						opp-level = <RPM_SMD_LEVEL_NOM>;
> +					};
> +
> +					rpmhpd_opp_nom_plus: opp8 {
> +						opp-level = <RPM_SMD_LEVEL_NOM_PLUS>;
> +					};
> +
> +					rpmhpd_opp_turbo: opp9 {
> +						opp-level = <RPM_SMD_LEVEL_TURBO>;
> +					};
> +
> +					rpmhpd_opp_turbo_plus: opp10 {
> +						opp-level = <RPM_SMD_LEVEL_BINNING>;
> +					};
> +				};
> +			};
>  		};
>  	};

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