diff mbox series

[1/5] bindings: clock: qcom: Add "qcom,adsp-skip-pll" property

Message ID 20240208062836.19767-2-quic_tdas@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add updates for clock controllers to support QCM6490 | expand

Commit Message

Taniya Das Feb. 8, 2024, 6:28 a.m. UTC
When remoteproc is used to boot the LPASS the ADSP PLL should not be
configured from the high level OS. Thus add support for property to
avoid configuring the LPASS PLL.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
---
 .../devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml   | 5 +++++
 1 file changed, 5 insertions(+)

--
2.17.1

Comments

Dmitry Baryshkov Feb. 8, 2024, 6:59 a.m. UTC | #1
On Thu, 8 Feb 2024 at 08:29, Taniya Das <quic_tdas@quicinc.com> wrote:
>
> When remoteproc is used to boot the LPASS the ADSP PLL should not be
> configured from the high level OS.

Why?

> Thus add support for property to
> avoid configuring the LPASS PLL.
>
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> ---
>  .../devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml   | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
> index deee5423d66e..358eb4a1cffd 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
> @@ -49,6 +49,11 @@ properties:
>        peripheral loader.
>      type: boolean
>
> +  qcom,adsp-skip-pll:
> +    description:
> +      Indicates if the LPASS PLL configuration is to be skipped.
> +    type: boolean

This property describes OS behaviour rather than the hardware. Such
things are generally not acceptable.

> +
>  required:
>    - compatible
>    - reg
> --
> 2.17.1
>
>
Krzysztof Kozlowski Feb. 8, 2024, 8:15 a.m. UTC | #2
On 08/02/2024 07:28, Taniya Das wrote:
> When remoteproc is used to boot the LPASS the ADSP PLL should not be
> configured from the high level OS. Thus add support for property to
> avoid configuring the LPASS PLL.
> 
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>

You nicely bypassed all my filters.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

Anyway, I don't understand point of this commit. Aren't you now
duplicating qcom,adsp-pil-mode?

Best regards,
Krzysztof
Taniya Das March 6, 2024, 6:53 a.m. UTC | #3
Thanks for your review Krzysztof.

On 2/8/2024 1:45 PM, Krzysztof Kozlowski wrote:
> On 08/02/2024 07:28, Taniya Das wrote:
>> When remoteproc is used to boot the LPASS the ADSP PLL should not be
>> configured from the high level OS. Thus add support for property to
>> avoid configuring the LPASS PLL.
>>
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> 
> You nicely bypassed all my filters.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
> 

My bad, I will update the commit subject.

> Anyway, I don't understand point of this commit. Aren't you now
> duplicating qcom,adsp-pil-mode?

No, the expectation with pil-mode was still certain level of 
configuration from HLOS LPASS clock drivers. But on the QCM6490 boards 
these clocks need to be completely reserved except the resets.

> 
> Best regards,
> Krzysztof
>
Taniya Das March 6, 2024, 6:54 a.m. UTC | #4
Thanks for your review, I will take care of the comments in my next 
patch series.

On 2/8/2024 12:29 PM, Dmitry Baryshkov wrote:
> On Thu, 8 Feb 2024 at 08:29, Taniya Das <quic_tdas@quicinc.com> wrote:
>>
>> When remoteproc is used to boot the LPASS the ADSP PLL should not be
>> configured from the high level OS.
> 
> Why?
> 
>> Thus add support for property to
>> avoid configuring the LPASS PLL.
>>
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>> ---
>>   .../devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml   | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
>> index deee5423d66e..358eb4a1cffd 100644
>> --- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
>> +++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
>> @@ -49,6 +49,11 @@ properties:
>>         peripheral loader.
>>       type: boolean
>>
>> +  qcom,adsp-skip-pll:
>> +    description:
>> +      Indicates if the LPASS PLL configuration is to be skipped.
>> +    type: boolean
> 
> This property describes OS behaviour rather than the hardware. Such
> things are generally not acceptable.
> 
>> +
>>   required:
>>     - compatible
>>     - reg
>> --
>> 2.17.1
>>
>>
> 
>
Krzysztof Kozlowski March 7, 2024, 7:52 a.m. UTC | #5
On 06/03/2024 07:53, Taniya Das wrote:
> Thanks for your review Krzysztof.
> 
> On 2/8/2024 1:45 PM, Krzysztof Kozlowski wrote:
>> On 08/02/2024 07:28, Taniya Das wrote:
>>> When remoteproc is used to boot the LPASS the ADSP PLL should not be
>>> configured from the high level OS. Thus add support for property to
>>> avoid configuring the LPASS PLL.
>>>
>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>
>> You nicely bypassed all my filters.
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
>>
> 
> My bad, I will update the commit subject.
> 
>> Anyway, I don't understand point of this commit. Aren't you now
>> duplicating qcom,adsp-pil-mode?
> 
> No, the expectation with pil-mode was still certain level of 
> configuration from HLOS LPASS clock drivers. But on the QCM6490 boards 
> these clocks need to be completely reserved except the resets.

Sounds like qcom,adsp-pil-mode or qcom,controlled-remotely or one of
others. Stop inventing 10 properties describing similar case, one for
each binding.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
index deee5423d66e..358eb4a1cffd 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
@@ -49,6 +49,11 @@  properties:
       peripheral loader.
     type: boolean

+  qcom,adsp-skip-pll:
+    description:
+      Indicates if the LPASS PLL configuration is to be skipped.
+    type: boolean
+
 required:
   - compatible
   - reg