diff mbox series

[02/13] dt-bindings: clock: google,gs101-clock: add PERIC0 clock management unit

Message ID 20231214105243.3707730-3-tudor.ambarus@linaro.org (mailing list archive)
State Changes Requested, archived
Headers show
Series GS101 Oriole: CMU_PERIC0 support and USI updates | expand

Commit Message

Tudor Ambarus Dec. 14, 2023, 10:52 a.m. UTC
Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
clock management unit.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 .../bindings/clock/google,gs101-clock.yaml    | 25 +++++-
 include/dt-bindings/clock/google,gs101.h      | 86 +++++++++++++++++++
 2 files changed, 109 insertions(+), 2 deletions(-)

Comments

Sam Protsenko Dec. 14, 2023, 3:12 p.m. UTC | #1
On Thu, Dec 14, 2023 at 4:52 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
> clock management unit.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  .../bindings/clock/google,gs101-clock.yaml    | 25 +++++-
>  include/dt-bindings/clock/google,gs101.h      | 86 +++++++++++++++++++
>  2 files changed, 109 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> index 3eebc03a309b..ba54c13c55bc 100644
> --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> @@ -30,14 +30,15 @@ properties:
>        - google,gs101-cmu-top
>        - google,gs101-cmu-apm
>        - google,gs101-cmu-misc
> +      - google,gs101-cmu-peric0
>
>    clocks:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 3
>
>    clock-names:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 3
>
>    "#clock-cells":
>      const: 1
> @@ -88,6 +89,26 @@ allOf:
>              - const: dout_cmu_misc_bus
>              - const: dout_cmu_misc_sss
>
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: google,gs101-cmu-peric0
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (24.576 MHz)
> +            - description: Connectivity Peripheral 0 bus clock (from CMU_TOP)
> +            - description: Connectivity Peripheral 0 IP clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: dout_cmu_peric0_bus
> +            - const: dout_cmu_peric0_ip
> +
>  additionalProperties: false
>
>  examples:
> diff --git a/include/dt-bindings/clock/google,gs101.h b/include/dt-bindings/clock/google,gs101.h
> index 21adec22387c..7d7a896416a7 100644
> --- a/include/dt-bindings/clock/google,gs101.h
> +++ b/include/dt-bindings/clock/google,gs101.h
> @@ -389,4 +389,90 @@
>  #define CLK_GOUT_MISC_WDT_CLUSTER1_PCLK                        73
>  #define CLK_GOUT_MISC_XIU_D_MISC_ACLK                  74
>
> +/* CMU_PERIC0 */

This comments looks off here. Other than than, LGTM:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

> +/* CMU_PERIC0 MUX */
> +#define CLK_MOUT_PERIC0_BUS_USER                       1
> +#define CLK_MOUT_PERIC0_I3C_USER                       2
> +#define CLK_MOUT_PERIC0_USI0_UART_USER                 3
> +#define CLK_MOUT_PERIC0_USI14_USI_USER                 4
> +#define CLK_MOUT_PERIC0_USI1_USI_USER                  5
> +#define CLK_MOUT_PERIC0_USI2_USI_USER                  6
> +#define CLK_MOUT_PERIC0_USI3_USI_USER                  7
> +#define CLK_MOUT_PERIC0_USI4_USI_USER                  8
> +#define CLK_MOUT_PERIC0_USI5_USI_USER                  9
> +#define CLK_MOUT_PERIC0_USI6_USI_USER                  10
> +#define CLK_MOUT_PERIC0_USI7_USI_USER                  11
> +#define CLK_MOUT_PERIC0_USI8_USI_USER                  12
> +
> +/* CMU_PERIC0 Dividers */
> +#define CLK_DOUT_PERIC0_I3C                            13
> +#define CLK_DOUT_PERIC0_USI0_UART                      14
> +#define CLK_DOUT_PERIC0_USI14_USI                      15
> +#define CLK_DOUT_PERIC0_USI1_USI                       16
> +#define CLK_DOUT_PERIC0_USI2_USI                       17
> +#define CLK_DOUT_PERIC0_USI3_USI                       18
> +#define CLK_DOUT_PERIC0_USI4_USI                       19
> +#define CLK_DOUT_PERIC0_USI5_USI                       20
> +#define CLK_DOUT_PERIC0_USI6_USI                       21
> +#define CLK_DOUT_PERIC0_USI7_USI                       22
> +#define CLK_DOUT_PERIC0_USI8_USI                       23
> +
> +/* CMU_PERIC0 Gates */
> +#define CLK_GOUT_PERIC0_IP                             24
> +#define CLK_GOUT_PERIC0_PERIC0_CMU_PERIC0_PCLK         25
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_OSCCLK_CLK          26
> +#define CLK_GOUT_PERIC0_D_TZPC_PERIC0_PCLK             27
> +#define CLK_GOUT_PERIC0_GPC_PERIC0_PCLK                        28
> +#define CLK_GOUT_PERIC0_GPIO_PERIC0_PCLK               29
> +#define CLK_GOUT_PERIC0_LHM_AXI_P_PERIC0_I_CLK         30
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_0            31
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_1            32
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_10           33
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_11           34
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_12           35
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_13           36
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_14           37
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_15           38
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_2            39
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_3            40
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_4            41
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_5            42
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_6            43
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7            44
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_8            45
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_9            46
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_0             47
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_1             48
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_10            49
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_11            50
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_12            51
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_13            52
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_14            53
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_15            54
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_2             55
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_3             56
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_4             57
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_5             58
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_6             59
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7             60
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_8             61
> +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_9             62
> +#define CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_0            63
> +#define CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_2            64
> +#define CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_0             65
> +#define CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_2             66
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_BUSP_CLK            67
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_I3C_CLK             68
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI0_UART_CLK       69
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI14_USI_CLK       70
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI1_USI_CLK                71
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI2_USI_CLK                72
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI3_USI_CLK                73
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI4_USI_CLK                74
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI5_USI_CLK                75
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI6_USI_CLK                76
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI7_USI_CLK                77
> +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK                78
> +#define CLK_GOUT_PERIC0_SYSREG_PERIC0_PCLK             79
> +
>  #endif /* _DT_BINDINGS_CLOCK_GOOGLE_GS101_H */
> --
> 2.43.0.472.g3155946c3a-goog
>
Rob Herring (Arm) Dec. 20, 2023, 3:07 p.m. UTC | #2
On Thu, Dec 14, 2023 at 10:52:32AM +0000, Tudor Ambarus wrote:
> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
> clock management unit.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  .../bindings/clock/google,gs101-clock.yaml    | 25 +++++-
>  include/dt-bindings/clock/google,gs101.h      | 86 +++++++++++++++++++
>  2 files changed, 109 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> index 3eebc03a309b..ba54c13c55bc 100644
> --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> @@ -30,14 +30,15 @@ properties:
>        - google,gs101-cmu-top
>        - google,gs101-cmu-apm
>        - google,gs101-cmu-misc
> +      - google,gs101-cmu-peric0
>  
>    clocks:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 3
>  
>    clock-names:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 3
>  
>    "#clock-cells":
>      const: 1
> @@ -88,6 +89,26 @@ allOf:
>              - const: dout_cmu_misc_bus
>              - const: dout_cmu_misc_sss
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: google,gs101-cmu-peric0
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (24.576 MHz)
> +            - description: Connectivity Peripheral 0 bus clock (from CMU_TOP)
> +            - description: Connectivity Peripheral 0 IP clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: dout_cmu_peric0_bus
> +            - const: dout_cmu_peric0_ip

'bus' and 'ip' are sufficient because naming is local to the module. The 
same is true on 'dout_cmu_misc_bus'. As that has not made a release, 
please fix all of them.

Rob
Tudor Ambarus Dec. 21, 2023, 7:20 a.m. UTC | #3
On 12/20/23 15:07, Rob Herring wrote:
> On Thu, Dec 14, 2023 at 10:52:32AM +0000, Tudor Ambarus wrote:
>> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
>> clock management unit.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  .../bindings/clock/google,gs101-clock.yaml    | 25 +++++-
>>  include/dt-bindings/clock/google,gs101.h      | 86 +++++++++++++++++++
>>  2 files changed, 109 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>> index 3eebc03a309b..ba54c13c55bc 100644
>> --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>> +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>> @@ -30,14 +30,15 @@ properties:
>>        - google,gs101-cmu-top
>>        - google,gs101-cmu-apm
>>        - google,gs101-cmu-misc
>> +      - google,gs101-cmu-peric0
>>  
>>    clocks:
>>      minItems: 1
>> -    maxItems: 2
>> +    maxItems: 3
>>  
>>    clock-names:
>>      minItems: 1
>> -    maxItems: 2
>> +    maxItems: 3
>>  
>>    "#clock-cells":
>>      const: 1
>> @@ -88,6 +89,26 @@ allOf:
>>              - const: dout_cmu_misc_bus
>>              - const: dout_cmu_misc_sss
>>  
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: google,gs101-cmu-peric0
>> +
>> +    then:
>> +      properties:
>> +        clocks:
>> +          items:
>> +            - description: External reference clock (24.576 MHz)
>> +            - description: Connectivity Peripheral 0 bus clock (from CMU_TOP)
>> +            - description: Connectivity Peripheral 0 IP clock (from CMU_TOP)
>> +
>> +        clock-names:
>> +          items:
>> +            - const: oscclk
>> +            - const: dout_cmu_peric0_bus
>> +            - const: dout_cmu_peric0_ip
> 
> 'bus' and 'ip' are sufficient because naming is local to the module. The 
> same is true on 'dout_cmu_misc_bus'. As that has not made a release, 
> please fix all of them.
>

Ok, will fix them shortly. Thanks, Rob!
Peter Griffin Dec. 22, 2023, 5:56 p.m. UTC | #4
Hi Tudor,

On Thu, 21 Dec 2023 at 07:20, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 12/20/23 15:07, Rob Herring wrote:
> > On Thu, Dec 14, 2023 at 10:52:32AM +0000, Tudor Ambarus wrote:
> >> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
> >> clock management unit.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >> ---
> >>  .../bindings/clock/google,gs101-clock.yaml    | 25 +++++-
> >>  include/dt-bindings/clock/google,gs101.h      | 86 +++++++++++++++++++
> >>  2 files changed, 109 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> >> index 3eebc03a309b..ba54c13c55bc 100644
> >> --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> >> +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
> >> @@ -30,14 +30,15 @@ properties:
> >>        - google,gs101-cmu-top
> >>        - google,gs101-cmu-apm
> >>        - google,gs101-cmu-misc
> >> +      - google,gs101-cmu-peric0
> >>
> >>    clocks:
> >>      minItems: 1
> >> -    maxItems: 2
> >> +    maxItems: 3
> >>
> >>    clock-names:
> >>      minItems: 1
> >> -    maxItems: 2
> >> +    maxItems: 3
> >>
> >>    "#clock-cells":
> >>      const: 1
> >> @@ -88,6 +89,26 @@ allOf:
> >>              - const: dout_cmu_misc_bus
> >>              - const: dout_cmu_misc_sss
> >>
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            const: google,gs101-cmu-peric0
> >> +
> >> +    then:
> >> +      properties:
> >> +        clocks:
> >> +          items:
> >> +            - description: External reference clock (24.576 MHz)
> >> +            - description: Connectivity Peripheral 0 bus clock (from CMU_TOP)
> >> +            - description: Connectivity Peripheral 0 IP clock (from CMU_TOP)
> >> +
> >> +        clock-names:
> >> +          items:
> >> +            - const: oscclk
> >> +            - const: dout_cmu_peric0_bus
> >> +            - const: dout_cmu_peric0_ip
> >
> > 'bus' and 'ip' are sufficient because naming is local to the module. The
> > same is true on 'dout_cmu_misc_bus'. As that has not made a release,
> > please fix all of them.
> >
>
> Ok, will fix them shortly. Thanks, Rob!

With Robs review comments addressed feel free to add my:

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
Tudor Ambarus Dec. 27, 2023, 12:38 p.m. UTC | #5
Hi, Rob,

On 12/21/23 07:20, Tudor Ambarus wrote:
> 
> 
> On 12/20/23 15:07, Rob Herring wrote:
>> On Thu, Dec 14, 2023 at 10:52:32AM +0000, Tudor Ambarus wrote:
>>> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
>>> clock management unit.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>> ---
>>>  .../bindings/clock/google,gs101-clock.yaml    | 25 +++++-
>>>  include/dt-bindings/clock/google,gs101.h      | 86 +++++++++++++++++++
>>>  2 files changed, 109 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>> index 3eebc03a309b..ba54c13c55bc 100644
>>> --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>> @@ -30,14 +30,15 @@ properties:
>>>        - google,gs101-cmu-top
>>>        - google,gs101-cmu-apm
>>>        - google,gs101-cmu-misc
>>> +      - google,gs101-cmu-peric0
>>>  
>>>    clocks:
>>>      minItems: 1
>>> -    maxItems: 2
>>> +    maxItems: 3
>>>  
>>>    clock-names:
>>>      minItems: 1
>>> -    maxItems: 2
>>> +    maxItems: 3
>>>  
>>>    "#clock-cells":
>>>      const: 1
>>> @@ -88,6 +89,26 @@ allOf:
>>>              - const: dout_cmu_misc_bus
>>>              - const: dout_cmu_misc_sss
>>>  
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: google,gs101-cmu-peric0
>>> +
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          items:
>>> +            - description: External reference clock (24.576 MHz)
>>> +            - description: Connectivity Peripheral 0 bus clock (from CMU_TOP)
>>> +            - description: Connectivity Peripheral 0 IP clock (from CMU_TOP)
>>> +
>>> +        clock-names:
>>> +          items:
>>> +            - const: oscclk
>>> +            - const: dout_cmu_peric0_bus
>>> +            - const: dout_cmu_peric0_ip
>>
>> 'bus' and 'ip' are sufficient because naming is local to the module. The 
>> same is true on 'dout_cmu_misc_bus'. As that has not made a release, 
>> please fix all of them.
>>
> 
> Ok, will fix them shortly. Thanks, Rob!

I tried your suggestion at
https://lore.kernel.org/linux-arm-kernel/c6cc6e74-6c3d-439b-8dc1-bc50a88a3d8f@linaro.org/

and we noticed that we'd have to update the clock driver as well.
These CMUs set the DT clock-name of the parent clock in the driver in
struct samsung_cmu_info::clk_name[]. The driver then tries to enable the
parent clock based on the clock-name in exynos_arm64_register_cmu().

In order to enable the parent clock of the CMU the following would be
needed in the driver:

diff --git a/drivers/clk/samsung/clk-gs101.c
b/drivers/clk/samsung/clk-gs101.c
index 68a27b78b00b..e91836ea3a98 100644
--- a/drivers/clk/samsung/clk-gs101.c
+++ b/drivers/clk/samsung/clk-gs101.c
@@ -2476,7 +2476,7 @@ static const struct samsung_cmu_info misc_cmu_info
__initconst = {
        .nr_clk_ids             = CLKS_NR_MISC,
        .clk_regs               = misc_clk_regs,
        .nr_clk_regs            = ARRAY_SIZE(misc_clk_regs),
-       .clk_name               = "dout_cmu_misc_bus",
+       .clk_name               = "bus",
 };

I think I lean towards keeping the name as it was because it's clearer
what are the clock dependencies in the driver. "dout_cmu_misc_bus" is
the clock name used when defining the clock:
DIV(CLK_DOUT_CMU_MISC_BUS, "dout_cmu_misc_bus", "gout_cmu_misc_bus",
    CLK_CON_DIV_CLKCMU_MISC_BUS, 0, 4),
The other exynos clock drivers are using too the driver's clock names
for the clock-names device tree property. For consistency I'd keep it
the same.

If you have a stronger opinion than mine, please tell and I'll happily
update the driver.

Thanks,
ta
Krzysztof Kozlowski Dec. 28, 2023, 7:30 a.m. UTC | #6
On 27/12/2023 13:38, Tudor Ambarus wrote:
> Hi, Rob,
> 
> On 12/21/23 07:20, Tudor Ambarus wrote:
>>
>>
>> On 12/20/23 15:07, Rob Herring wrote:
>>> On Thu, Dec 14, 2023 at 10:52:32AM +0000, Tudor Ambarus wrote:
>>>> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
>>>> clock management unit.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>> ---
>>>>  .../bindings/clock/google,gs101-clock.yaml    | 25 +++++-
>>>>  include/dt-bindings/clock/google,gs101.h      | 86 +++++++++++++++++++
>>>>  2 files changed, 109 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>>> index 3eebc03a309b..ba54c13c55bc 100644
>>>> --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>>> +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>>> @@ -30,14 +30,15 @@ properties:
>>>>        - google,gs101-cmu-top
>>>>        - google,gs101-cmu-apm
>>>>        - google,gs101-cmu-misc
>>>> +      - google,gs101-cmu-peric0
>>>>  
>>>>    clocks:
>>>>      minItems: 1
>>>> -    maxItems: 2
>>>> +    maxItems: 3
>>>>  
>>>>    clock-names:
>>>>      minItems: 1
>>>> -    maxItems: 2
>>>> +    maxItems: 3
>>>>  
>>>>    "#clock-cells":
>>>>      const: 1
>>>> @@ -88,6 +89,26 @@ allOf:
>>>>              - const: dout_cmu_misc_bus
>>>>              - const: dout_cmu_misc_sss
>>>>  
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: google,gs101-cmu-peric0
>>>> +
>>>> +    then:
>>>> +      properties:
>>>> +        clocks:
>>>> +          items:
>>>> +            - description: External reference clock (24.576 MHz)
>>>> +            - description: Connectivity Peripheral 0 bus clock (from CMU_TOP)
>>>> +            - description: Connectivity Peripheral 0 IP clock (from CMU_TOP)
>>>> +
>>>> +        clock-names:
>>>> +          items:
>>>> +            - const: oscclk
>>>> +            - const: dout_cmu_peric0_bus
>>>> +            - const: dout_cmu_peric0_ip
>>>
>>> 'bus' and 'ip' are sufficient because naming is local to the module. The 
>>> same is true on 'dout_cmu_misc_bus'. As that has not made a release, 
>>> please fix all of them.
>>>
>>
>> Ok, will fix them shortly. Thanks, Rob!
> 
> I tried your suggestion at
> https://lore.kernel.org/linux-arm-kernel/c6cc6e74-6c3d-439b-8dc1-bc50a88a3d8f@linaro.org/
> 
> and we noticed that we'd have to update the clock driver as well.
> These CMUs set the DT clock-name of the parent clock in the driver in
> struct samsung_cmu_info::clk_name[]. The driver then tries to enable the
> parent clock based on the clock-name in exynos_arm64_register_cmu().
> 
> In order to enable the parent clock of the CMU the following would be
> needed in the driver:
> 
> diff --git a/drivers/clk/samsung/clk-gs101.c
> b/drivers/clk/samsung/clk-gs101.c
> index 68a27b78b00b..e91836ea3a98 100644
> --- a/drivers/clk/samsung/clk-gs101.c
> +++ b/drivers/clk/samsung/clk-gs101.c
> @@ -2476,7 +2476,7 @@ static const struct samsung_cmu_info misc_cmu_info
> __initconst = {
>         .nr_clk_ids             = CLKS_NR_MISC,
>         .clk_regs               = misc_clk_regs,
>         .nr_clk_regs            = ARRAY_SIZE(misc_clk_regs),
> -       .clk_name               = "dout_cmu_misc_bus",
> +       .clk_name               = "bus",

Yes, obviously, the names are used...

The entire point was that a week ago Rob said:
"As that has not made a release,  please fix all of them."
but if you keep waiting, like 8 days for this simple patch, his
statement is not true anymore.

The only point was to send a fix *the next day*, so I would apply it and
send further. You kind of solved that problem by waiting entire week for
a simple driver and DTS change.

Best regards,
Krzysztof
Tudor Ambarus Dec. 28, 2023, 7:49 a.m. UTC | #7
On 12/28/23 07:30, Krzysztof Kozlowski wrote:
> On 27/12/2023 13:38, Tudor Ambarus wrote:
>> Hi, Rob,
>>
>> On 12/21/23 07:20, Tudor Ambarus wrote:
>>>
>>>
>>> On 12/20/23 15:07, Rob Herring wrote:
>>>> On Thu, Dec 14, 2023 at 10:52:32AM +0000, Tudor Ambarus wrote:
>>>>> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0)
>>>>> clock management unit.
>>>>>
>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>>> ---
>>>>>  .../bindings/clock/google,gs101-clock.yaml    | 25 +++++-
>>>>>  include/dt-bindings/clock/google,gs101.h      | 86 +++++++++++++++++++
>>>>>  2 files changed, 109 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>>>> index 3eebc03a309b..ba54c13c55bc 100644
>>>>> --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>>>> +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
>>>>> @@ -30,14 +30,15 @@ properties:
>>>>>        - google,gs101-cmu-top
>>>>>        - google,gs101-cmu-apm
>>>>>        - google,gs101-cmu-misc
>>>>> +      - google,gs101-cmu-peric0
>>>>>  
>>>>>    clocks:
>>>>>      minItems: 1
>>>>> -    maxItems: 2
>>>>> +    maxItems: 3
>>>>>  
>>>>>    clock-names:
>>>>>      minItems: 1
>>>>> -    maxItems: 2
>>>>> +    maxItems: 3
>>>>>  
>>>>>    "#clock-cells":
>>>>>      const: 1
>>>>> @@ -88,6 +89,26 @@ allOf:
>>>>>              - const: dout_cmu_misc_bus
>>>>>              - const: dout_cmu_misc_sss
>>>>>  
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            const: google,gs101-cmu-peric0
>>>>> +
>>>>> +    then:
>>>>> +      properties:
>>>>> +        clocks:
>>>>> +          items:
>>>>> +            - description: External reference clock (24.576 MHz)
>>>>> +            - description: Connectivity Peripheral 0 bus clock (from CMU_TOP)
>>>>> +            - description: Connectivity Peripheral 0 IP clock (from CMU_TOP)
>>>>> +
>>>>> +        clock-names:
>>>>> +          items:
>>>>> +            - const: oscclk
>>>>> +            - const: dout_cmu_peric0_bus
>>>>> +            - const: dout_cmu_peric0_ip
>>>>
>>>> 'bus' and 'ip' are sufficient because naming is local to the module. The 
>>>> same is true on 'dout_cmu_misc_bus'. As that has not made a release, 
>>>> please fix all of them.
>>>>
>>>
>>> Ok, will fix them shortly. Thanks, Rob!
>>
>> I tried your suggestion at
>> https://lore.kernel.org/linux-arm-kernel/c6cc6e74-6c3d-439b-8dc1-bc50a88a3d8f@linaro.org/
>>
>> and we noticed that we'd have to update the clock driver as well.
>> These CMUs set the DT clock-name of the parent clock in the driver in
>> struct samsung_cmu_info::clk_name[]. The driver then tries to enable the
>> parent clock based on the clock-name in exynos_arm64_register_cmu().
>>
>> In order to enable the parent clock of the CMU the following would be
>> needed in the driver:
>>
>> diff --git a/drivers/clk/samsung/clk-gs101.c
>> b/drivers/clk/samsung/clk-gs101.c
>> index 68a27b78b00b..e91836ea3a98 100644
>> --- a/drivers/clk/samsung/clk-gs101.c
>> +++ b/drivers/clk/samsung/clk-gs101.c
>> @@ -2476,7 +2476,7 @@ static const struct samsung_cmu_info misc_cmu_info
>> __initconst = {
>>         .nr_clk_ids             = CLKS_NR_MISC,
>>         .clk_regs               = misc_clk_regs,
>>         .nr_clk_regs            = ARRAY_SIZE(misc_clk_regs),
>> -       .clk_name               = "dout_cmu_misc_bus",
>> +       .clk_name               = "bus",
> 
> Yes, obviously, the names are used...
> 
> The entire point was that a week ago Rob said:
> "As that has not made a release,  please fix all of them."
> but if you keep waiting, like 8 days for this simple patch, his
> statement is not true anymore.
> 

I saw the problem at the end of Thursday, reported it, and after that I
entered vacation.

> The only point was to send a fix *the next day*, so I would apply it and
> send further. You kind of solved that problem by waiting entire week for
> a simple driver and DTS change.
> 
Why can't we queue the name fix as a patch for v6.8-rc1? Of course if
you consider that the change is worth it. As I said, I lean towards not
changing it.

Thanks,
ta
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
index 3eebc03a309b..ba54c13c55bc 100644
--- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
+++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
@@ -30,14 +30,15 @@  properties:
       - google,gs101-cmu-top
       - google,gs101-cmu-apm
       - google,gs101-cmu-misc
+      - google,gs101-cmu-peric0
 
   clocks:
     minItems: 1
-    maxItems: 2
+    maxItems: 3
 
   clock-names:
     minItems: 1
-    maxItems: 2
+    maxItems: 3
 
   "#clock-cells":
     const: 1
@@ -88,6 +89,26 @@  allOf:
             - const: dout_cmu_misc_bus
             - const: dout_cmu_misc_sss
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: google,gs101-cmu-peric0
+
+    then:
+      properties:
+        clocks:
+          items:
+            - description: External reference clock (24.576 MHz)
+            - description: Connectivity Peripheral 0 bus clock (from CMU_TOP)
+            - description: Connectivity Peripheral 0 IP clock (from CMU_TOP)
+
+        clock-names:
+          items:
+            - const: oscclk
+            - const: dout_cmu_peric0_bus
+            - const: dout_cmu_peric0_ip
+
 additionalProperties: false
 
 examples:
diff --git a/include/dt-bindings/clock/google,gs101.h b/include/dt-bindings/clock/google,gs101.h
index 21adec22387c..7d7a896416a7 100644
--- a/include/dt-bindings/clock/google,gs101.h
+++ b/include/dt-bindings/clock/google,gs101.h
@@ -389,4 +389,90 @@ 
 #define CLK_GOUT_MISC_WDT_CLUSTER1_PCLK			73
 #define CLK_GOUT_MISC_XIU_D_MISC_ACLK			74
 
+/* CMU_PERIC0 */
+/* CMU_PERIC0 MUX */
+#define CLK_MOUT_PERIC0_BUS_USER			1
+#define CLK_MOUT_PERIC0_I3C_USER			2
+#define CLK_MOUT_PERIC0_USI0_UART_USER			3
+#define CLK_MOUT_PERIC0_USI14_USI_USER			4
+#define CLK_MOUT_PERIC0_USI1_USI_USER			5
+#define CLK_MOUT_PERIC0_USI2_USI_USER			6
+#define CLK_MOUT_PERIC0_USI3_USI_USER			7
+#define CLK_MOUT_PERIC0_USI4_USI_USER			8
+#define CLK_MOUT_PERIC0_USI5_USI_USER			9
+#define CLK_MOUT_PERIC0_USI6_USI_USER			10
+#define CLK_MOUT_PERIC0_USI7_USI_USER			11
+#define CLK_MOUT_PERIC0_USI8_USI_USER			12
+
+/* CMU_PERIC0 Dividers */
+#define CLK_DOUT_PERIC0_I3C				13
+#define CLK_DOUT_PERIC0_USI0_UART			14
+#define CLK_DOUT_PERIC0_USI14_USI			15
+#define CLK_DOUT_PERIC0_USI1_USI			16
+#define CLK_DOUT_PERIC0_USI2_USI			17
+#define CLK_DOUT_PERIC0_USI3_USI			18
+#define CLK_DOUT_PERIC0_USI4_USI			19
+#define CLK_DOUT_PERIC0_USI5_USI			20
+#define CLK_DOUT_PERIC0_USI6_USI			21
+#define CLK_DOUT_PERIC0_USI7_USI			22
+#define CLK_DOUT_PERIC0_USI8_USI			23
+
+/* CMU_PERIC0 Gates */
+#define CLK_GOUT_PERIC0_IP				24
+#define CLK_GOUT_PERIC0_PERIC0_CMU_PERIC0_PCLK		25
+#define CLK_GOUT_PERIC0_CLK_PERIC0_OSCCLK_CLK		26
+#define CLK_GOUT_PERIC0_D_TZPC_PERIC0_PCLK		27
+#define CLK_GOUT_PERIC0_GPC_PERIC0_PCLK			28
+#define CLK_GOUT_PERIC0_GPIO_PERIC0_PCLK		29
+#define CLK_GOUT_PERIC0_LHM_AXI_P_PERIC0_I_CLK		30
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_0		31
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_1		32
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_10		33
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_11		34
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_12		35
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_13		36
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_14		37
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_15		38
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_2		39
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_3		40
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_4		41
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_5		42
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_6		43
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7		44
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_8		45
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_9		46
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_0		47
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_1		48
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_10		49
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_11		50
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_12		51
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_13		52
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_14		53
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_15		54
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_2		55
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_3		56
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_4		57
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_5		58
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_6		59
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7		60
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_8		61
+#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_9		62
+#define CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_0		63
+#define CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_2		64
+#define CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_0		65
+#define CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_2		66
+#define CLK_GOUT_PERIC0_CLK_PERIC0_BUSP_CLK		67
+#define CLK_GOUT_PERIC0_CLK_PERIC0_I3C_CLK		68
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI0_UART_CLK	69
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI14_USI_CLK	70
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI1_USI_CLK		71
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI2_USI_CLK		72
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI3_USI_CLK		73
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI4_USI_CLK		74
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI5_USI_CLK		75
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI6_USI_CLK		76
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI7_USI_CLK		77
+#define CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK		78
+#define CLK_GOUT_PERIC0_SYSREG_PERIC0_PCLK		79
+
 #endif /* _DT_BINDINGS_CLOCK_GOOGLE_GS101_H */