1. Patchew
  2. linux
  3. [PATCH 0/3] add some node for amlogic c3
  4. View patch

[PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter

Xianwei Zhao via B4 Relay posted 3 patches 1 year, 4 months ago
There is a newer version of this series
[PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Xianwei Zhao via B4 Relay 1 year, 4 months ago 
From: Xianwei Zhao <xianwei.zhao@amlogic.com>

Add C3 PLL controller input clock parameters "fix".

Fixes: 0e6be855a96d ("dt-bindings: clock: add Amlogic C3 PLL clock controller")
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
index 43de3c6fc1cf..700865cc9792 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
+++ b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
@@ -24,11 +24,13 @@ properties:
     items:
       - description: input top pll
       - description: input mclk pll
+      - description: input fix pll
 
   clock-names:
     items:
       - const: top
       - const: mclk
+      - const: fix
 
   "#clock-cells":
     const: 1
@@ -52,8 +54,9 @@ examples:
             compatible = "amlogic,c3-pll-clkc";
             reg = <0x0 0x8000 0x0 0x1a4>;
             clocks = <&scmi_clk 2>,
-                     <&scmi_clk 5>;
-            clock-names = "top", "mclk";
+                     <&scmi_clk 5>,
+                     <&scmi_clk 12>;
+            clock-names = "top", "mclk", "fix";
             #clock-cells = <1>;
         };
     };

-- 
2.37.1
Re: [PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Krzysztof Kozlowski 1 year, 4 months ago 
On 06/08/2024 12:27, Xianwei Zhao via B4 Relay wrote:
> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
> 
> Add C3 PLL controller input clock parameters "fix".

What is "parameters" here? Why you are adding it? Is it missing?
Something is not working?

> 
> Fixes: 0e6be855a96d ("dt-bindings: clock: add Amlogic C3 PLL clock controller")

Why? What bug are you fixing?

> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
> ---
>  Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
> index 43de3c6fc1cf..700865cc9792 100644
> --- a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
> +++ b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
> @@ -24,11 +24,13 @@ properties:
>      items:
>        - description: input top pll
>        - description: input mclk pll
> +      - description: input fix pll
>  
>    clock-names:
>      items:
>        - const: top
>        - const: mclk
> +      - const: fix

and that's not an ABI break because?

Best regards,
Krzysztof
Re: [PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Xianwei Zhao 1 year, 4 months ago 
Hi Krzysztof,
     Thanks for your review.

On 2024/8/6 21:10, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
> 
> On 06/08/2024 12:27, Xianwei Zhao via B4 Relay wrote:
>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>
>> Add C3 PLL controller input clock parameters "fix".
> 
> What is "parameters" here? Why you are adding it? Is it missing?
> Something is not working?
> 
Yes. The previous submission was lost.

>>
>> Fixes: 0e6be855a96d ("dt-bindings: clock: add Amlogic C3 PLL clock controller")
> 
> Why? What bug are you fixing?

The input clock of PLL clock controller need the clock whose fw_name is 
called "fix".
> 
>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> ---
>>   Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>> index 43de3c6fc1cf..700865cc9792 100644
>> --- a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>> @@ -24,11 +24,13 @@ properties:
>>       items:
>>         - description: input top pll
>>         - description: input mclk pll
>> +      - description: input fix pll
>>
>>     clock-names:
>>       items:
>>         - const: top
>>         - const: mclk
>> +      - const: fix
> 
> and that's not an ABI break because?
This is "fixed" clock.
I will modify "fix" to "fixed",in next version.
> 
> Best regards,
> Krzysztof
>
Re: [PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Krzysztof Kozlowski 1 year, 4 months ago 
On 07/08/2024 03:55, Xianwei Zhao wrote:
> Hi Krzysztof,
>      Thanks for your review.
> 
> On 2024/8/6 21:10, Krzysztof Kozlowski wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On 06/08/2024 12:27, Xianwei Zhao via B4 Relay wrote:
>>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>
>>> Add C3 PLL controller input clock parameters "fix".
>>
>> What is "parameters" here? Why you are adding it? Is it missing?
>> Something is not working?
>>
> Yes. The previous submission was lost.

What submission is lost?

> 
>>>
>>> Fixes: 0e6be855a96d ("dt-bindings: clock: add Amlogic C3 PLL clock controller")
>>
>> Why? What bug are you fixing?
> 
> The input clock of PLL clock controller need the clock whose fw_name is 
> called "fix".

Then explain this in commit msg.

>>
>>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>> ---
>>>   Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>> index 43de3c6fc1cf..700865cc9792 100644
>>> --- a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>> @@ -24,11 +24,13 @@ properties:
>>>       items:
>>>         - description: input top pll
>>>         - description: input mclk pll
>>> +      - description: input fix pll
>>>
>>>     clock-names:
>>>       items:
>>>         - const: top
>>>         - const: mclk
>>> +      - const: fix
>>
>> and that's not an ABI break because?
> This is "fixed" clock.
> I will modify "fix" to "fixed",in next version.

With "fixed" it is still ABI break, right?

Best regards,
Krzysztof
Re: [PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Jerome Brunet 1 year, 4 months ago 
On Wed 07 Aug 2024 at 07:44, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/08/2024 03:55, Xianwei Zhao wrote:
>> Hi Krzysztof,
>>      Thanks for your review.
>> 
>> On 2024/8/6 21:10, Krzysztof Kozlowski wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On 06/08/2024 12:27, Xianwei Zhao via B4 Relay wrote:
>>>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>>
>>>> Add C3 PLL controller input clock parameters "fix".
>>>
>>> What is "parameters" here? Why you are adding it? Is it missing?
>>> Something is not working?
>>>
>> Yes. The previous submission was lost.
>
> What submission is lost?
>

He means it was just forgotten in the submission and noboby picked up on
it at the time. He is not not trying to sneak anything new. That input
is used everywhere in the driver and nothing would work without it.

It is just an honest mistake that he is trying to correct

Indeed the description could certainly detail this a bit more

>> 
>>>>
>>>> Fixes: 0e6be855a96d ("dt-bindings: clock: add Amlogic C3 PLL clock controller")
>>>
>>> Why? What bug are you fixing?
>> 
>> The input clock of PLL clock controller need the clock whose fw_name is 
>> called "fix".
>
> Then explain this in commit msg.
>
>>>
>>>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml | 7 +++++--
>>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>> index 43de3c6fc1cf..700865cc9792 100644
>>>> --- a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>> @@ -24,11 +24,13 @@ properties:
>>>>       items:
>>>>         - description: input top pll
>>>>         - description: input mclk pll
>>>> +      - description: input fix pll
>>>>
>>>>     clock-names:
>>>>       items:
>>>>         - const: top
>>>>         - const: mclk
>>>> +      - const: fix
>>>
>>> and that's not an ABI break because?
>> This is "fixed" clock.
>> I will modify "fix" to "fixed",in next version.

No keep the original name. No reason to change it and make more of a mess.

>
> With "fixed" it is still ABI break, right?

It is an ABI break but on a new and immature platform.
Noboby could really use that platform at this stage, so nothing is going
to break on anyone really.

>
> Best regards,
> Krzysztof

-- 
Jerome
Re: [PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Xianwei Zhao 1 year, 4 months ago 
Hi Jerome,
     Thanks for your reply.

On 2024/8/7 15:56, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> On Wed 07 Aug 2024 at 07:44, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 07/08/2024 03:55, Xianwei Zhao wrote:
>>> Hi Krzysztof,
>>>       Thanks for your review.
>>>
>>> On 2024/8/6 21:10, Krzysztof Kozlowski wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On 06/08/2024 12:27, Xianwei Zhao via B4 Relay wrote:
>>>>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>>>
>>>>> Add C3 PLL controller input clock parameters "fix".
>>>>
>>>> What is "parameters" here? Why you are adding it? Is it missing?
>>>> Something is not working?
>>>>
>>> Yes. The previous submission was lost.
>>
>> What submission is lost?
>>
> 
> He means it was just forgotten in the submission and noboby picked up on
> it at the time. He is not not trying to sneak anything new. That input
> is used everywhere in the driver and nothing would work without it.
> 
> It is just an honest mistake that he is trying to correct
> 
> Indeed the description could certainly detail this a bit more
> 
>>>
>>>>>
>>>>> Fixes: 0e6be855a96d ("dt-bindings: clock: add Amlogic C3 PLL clock controller")
>>>>
>>>> Why? What bug are you fixing?
>>>
>>> The input clock of PLL clock controller need the clock whose fw_name is
>>> called "fix".
>>
>> Then explain this in commit msg.
>>
>>>>
>>>>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml | 7 +++++--
>>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>>> index 43de3c6fc1cf..700865cc9792 100644
>>>>> --- a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>>> @@ -24,11 +24,13 @@ properties:
>>>>>        items:
>>>>>          - description: input top pll
>>>>>          - description: input mclk pll
>>>>> +      - description: input fix pll
>>>>>
>>>>>      clock-names:
>>>>>        items:
>>>>>          - const: top
>>>>>          - const: mclk
>>>>> +      - const: fix
>>>>
>>>> and that's not an ABI break because?
>>> This is "fixed" clock.
>>> I will modify "fix" to "fixed",in next version.
> 
> No keep the original name. No reason to change it and make more of a mess.
> 

OK. Will keep original name.

>>
>> With "fixed" it is still ABI break, right?
> 
> It is an ABI break but on a new and immature platform.
> Noboby could really use that platform at this stage, so nothing is going
> to break on anyone really.
> 
>>
>> Best regards,
>> Krzysztof
> 
> --
> Jerome
Re: [PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Krzysztof Kozlowski 1 year, 4 months ago 
On 07/08/2024 09:56, Jerome Brunet wrote:
>>>> and that's not an ABI break because?
>>> This is "fixed" clock.
>>> I will modify "fix" to "fixed",in next version.
> 
> No keep the original name. No reason to change it and make more of a mess.
> 
>>
>> With "fixed" it is still ABI break, right?
> 
> It is an ABI break but on a new and immature platform.
> Noboby could really use that platform at this stage, so nothing is going
> to break on anyone really.

Sure, this could be also used in commit msg saving us from this entire
discussion.

Best regards,
Krzysztof
Re: [PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Xianwei Zhao 1 year, 4 months ago 
Hi Krzysztof,
    Thank you for your reply.

On 2024/8/7 13:44, Krzysztof Kozlowski wrote:
> [ EXTERNAL EMAIL ]
> 
> On 07/08/2024 03:55, Xianwei Zhao wrote:
>> Hi Krzysztof,
>>       Thanks for your review.
>>
>> On 2024/8/6 21:10, Krzysztof Kozlowski wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On 06/08/2024 12:27, Xianwei Zhao via B4 Relay wrote:
>>>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>>
>>>> Add C3 PLL controller input clock parameters "fix".
>>>
>>> What is "parameters" here? Why you are adding it? Is it missing?
>>> Something is not working?
>>>
>> Yes. The previous submission was lost.
> 
> What submission is lost?
> 
>>
>>>>
>>>> Fixes: 0e6be855a96d ("dt-bindings: clock: add Amlogic C3 PLL clock controller")
>>>
>>> Why? What bug are you fixing?
>>
>> The input clock of PLL clock controller need the clock whose fw_name is
>> called "fix".
> 
> Then explain this in commit msg.
> 
Will add this in commit msg.
>>>
>>>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>> index 43de3c6fc1cf..700865cc9792 100644
>>>> --- a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>> @@ -24,11 +24,13 @@ properties:
>>>>        items:
>>>>          - description: input top pll
>>>>          - description: input mclk pll
>>>> +      - description: input fix pll
>>>>
>>>>      clock-names:
>>>>        items:
>>>>          - const: top
>>>>          - const: mclk
>>>> +      - const: fix
>>>
>>> and that's not an ABI break because?
>> This is "fixed" clock.
>> I will modify "fix" to "fixed",in next version.
> 
> With "fixed" it is still ABI break, right?
> No. The clock named "fixed" was initially implemented in the PLL clock 
controller driver, but some registers needed secure zone access,
so we put it in secure zone(BL31) implemented and access it through the 
SCMI. but the PLL clock driver need uses this clock, so the "fixed" 
clock is input as an input source,
We changed the driver and forgot to change the binding, so we added it here.
> Best regards,
> Krzysztof
>
Re: [PATCH 1/3] dt-bindings: clock: fix C3 PLL input parameter
Posted by Krzysztof Kozlowski 1 year, 4 months ago 
On 07/08/2024 08:40, Xianwei Zhao wrote:
> Hi Krzysztof,
>     Thank you for your reply.
> 
> On 2024/8/7 13:44, Krzysztof Kozlowski wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On 07/08/2024 03:55, Xianwei Zhao wrote:
>>> Hi Krzysztof,
>>>       Thanks for your review.
>>>
>>> On 2024/8/6 21:10, Krzysztof Kozlowski wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On 06/08/2024 12:27, Xianwei Zhao via B4 Relay wrote:
>>>>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>>>
>>>>> Add C3 PLL controller input clock parameters "fix".
>>>>
>>>> What is "parameters" here? Why you are adding it? Is it missing?
>>>> Something is not working?
>>>>
>>> Yes. The previous submission was lost.
>>
>> What submission is lost?
>>
>>>
>>>>>
>>>>> Fixes: 0e6be855a96d ("dt-bindings: clock: add Amlogic C3 PLL clock controller")
>>>>
>>>> Why? What bug are you fixing?
>>>
>>> The input clock of PLL clock controller need the clock whose fw_name is
>>> called "fix".
>>
>> Then explain this in commit msg.
>>
> Will add this in commit msg.
>>>>
>>>>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml | 7 +++++--
>>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>>> index 43de3c6fc1cf..700865cc9792 100644
>>>>> --- a/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,c3-pll-clkc.yaml
>>>>> @@ -24,11 +24,13 @@ properties:
>>>>>        items:
>>>>>          - description: input top pll
>>>>>          - description: input mclk pll
>>>>> +      - description: input fix pll
>>>>>
>>>>>      clock-names:
>>>>>        items:
>>>>>          - const: top
>>>>>          - const: mclk
>>>>> +      - const: fix
>>>>
>>>> and that's not an ABI break because?
>>> This is "fixed" clock.
>>> I will modify "fix" to "fixed",in next version.
>>
>> With "fixed" it is still ABI break, right?
>> No. The clock named "fixed" was initially implemented in the PLL clock 
> controller driver, but some registers needed secure zone access,
> so we put it in secure zone(BL31) implemented and access it through the 
> SCMI. but the PLL clock driver need uses this clock, so the "fixed" 
> clock is input as an input source,
> We changed the driver and forgot to change the binding, so we added it here.

Fix your quoting or your email program - I don't understand what you are
saying here.

Anyway, provide justification in the commit msg.

Best regards,
Krzysztof

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.