[v11,3/5] dt-bindings: clock: meson: add A1 PLL and Peripherals clkcs bindings

Commit Message

Dmitry Rokosov March 21, 2023, 7:30 p.m. UTC

Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
clock drivers.
Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
bindings and include them to MAINTAINERS.

Signed-off-by: Jian Hu <[e-mail]>
Signed-off-by: Dmitry Rokosov <[e-mail]>
---
 .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
 .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
 MAINTAINERS                                   |   1 +
 include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
 .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
 5 files changed, 267 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
 create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h
 create mode 100644 include/dt-bindings/clock/amlogic,a1-pll-clkc.h

Comments

Jerome Brunet March 27, 2023, 9:51 a.m. UTC | #1

On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:

> Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
> clock drivers.
> Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
> bindings and include them to MAINTAINERS.
>
> Signed-off-by: Jian Hu <[e-mail]>
> Signed-off-by: Dmitry Rokosov <[e-mail]>
> ---
>  .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
>  MAINTAINERS                                   |   1 +
>  include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
>  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
>  5 files changed, 267 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml

There is two drivers (and 2 independent patches). There should be 2
bindings patches as well.

>  create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h
>  create mode 100644 include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> new file mode 100644
> index 000000000000..cb6d8f4eb959
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/amlogic,a1-clkc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic Meson A/C serials Peripheral Clock Control Unit
> +
> +maintainers:
> +  - Neil Armstrong <[e-mail]>
> +  - Jerome Brunet <[e-mail]>
> +  - Jian Hu <[e-mail]>
> +  - Dmitry Rokosov <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: amlogic,a1-clkc
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: input fixed pll div2
> +      - description: input fixed pll div3
> +      - description: input fixed pll div5
> +      - description: input fixed pll div7
> +      - description: input hifi pll
> +      - description: input oscillator (usually at 24MHz)
> +
> +  clock-names:
> +    items:
> +      - const: fclk_div2
> +      - const: fclk_div3
> +      - const: fclk_div5
> +      - const: fclk_div7
> +      - const: hifi_pll
> +      - const: xtal
> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/amlogic,a1-pll-clkc.h>
> +    apb {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        clock-controller@800 {
> +            compatible = "amlogic,a1-clkc";
> +            reg = <0 0x800 0 0x104>;
> +            #clock-cells = <1>;
> +            clocks = <&clkc_pll CLKID_FCLK_DIV2>,
> +                     <&clkc_pll CLKID_FCLK_DIV3>,
> +                     <&clkc_pll CLKID_FCLK_DIV5>,
> +                     <&clkc_pll CLKID_FCLK_DIV7>,
> +                     <&clkc_pll CLKID_HIFI_PLL>,
> +                     <&xtal>;
> +            clock-names = "fclk_div2", "fclk_div3",
> +                          "fclk_div5", "fclk_div7",
> +                          "hifi_pll", "xtal";
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> new file mode 100644
> index 000000000000..77a13b1f9d5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic Meson A/C serials PLL Clock Control Unit
> +
> +maintainers:
> +  - Neil Armstrong <[e-mail]>
> +  - Jerome Brunet <[e-mail]>
> +  - Jian Hu <[e-mail]>
> +  - Dmitry Rokosov <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: amlogic,a1-pll-clkc
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: input fixpll_in
> +      - description: input hifipll_in
> +
> +  clock-names:
> +    items:
> +      - const: fixpll_in
> +      - const: hifipll_in
> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/amlogic,a1-clkc.h>
> +    apb {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        clock-controller@7c80 {
> +            compatible = "amlogic,a1-pll-clkc";
> +            reg = <0 0x7c80 0 0x18c>;
> +            #clock-cells = <1>;
> +            clocks = <&clkc_periphs CLKID_FIXPLL_IN>,
> +                     <&clkc_periphs CLKID_HIFIPLL_IN>;
> +            clock-names = "fixpll_in", "hifipll_in";
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 39ff1a717625..8438bc9bd636 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1895,6 +1895,7 @@ L:	[e-mail]
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/clock/amlogic*
>  F:	drivers/clk/meson/
> +F:	include/dt-bindings/clock/a1*
>  F:	include/dt-bindings/clock/gxbb*
>  F:	include/dt-bindings/clock/meson*
>  
> diff --git a/include/dt-bindings/clock/amlogic,a1-clkc.h b/include/dt-bindings/clock/amlogic,a1-clkc.h
> new file mode 100644
> index 000000000000..95131779982c
> --- /dev/null
> +++ b/include/dt-bindings/clock/amlogic,a1-clkc.h
> @@ -0,0 +1,113 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + * Author: Jian Hu <[e-mail]>
> + *
> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <[e-mail]>
> + */
> +
> +#ifndef __A1_CLKC_H
> +#define __A1_CLKC_H
> +
> +#define CLKID_FIXPLL_IN		0
> +#define CLKID_USB_PHY_IN	1
> +#define CLKID_USB_CTRL_IN	2
> +#define CLKID_HIFIPLL_IN	3
> +#define CLKID_SYSPLL_IN		4
> +#define CLKID_DDS_IN		5
> +#define CLKID_SYS		6
> +#define CLKID_CLKTREE		7
> +#define CLKID_RESET_CTRL	8
> +#define CLKID_ANALOG_CTRL	9
> +#define CLKID_PWR_CTRL		10
> +#define CLKID_PAD_CTRL		11
> +#define CLKID_SYS_CTRL		12
> +#define CLKID_TEMP_SENSOR	13
> +#define CLKID_AM2AXI_DIV	14
> +#define CLKID_SPICC_B		15
> +#define CLKID_SPICC_A		16
> +#define CLKID_MSR		17
> +#define CLKID_AUDIO		18
> +#define CLKID_JTAG_CTRL		19
> +#define CLKID_SARADC_EN		20
> +#define CLKID_PWM_EF		21
> +#define CLKID_PWM_CD		22
> +#define CLKID_PWM_AB		23
> +#define CLKID_CEC		24
> +#define CLKID_I2C_S		25
> +#define CLKID_IR_CTRL		26
> +#define CLKID_I2C_M_D		27
> +#define CLKID_I2C_M_C		28
> +#define CLKID_I2C_M_B		29
> +#define CLKID_I2C_M_A		30
> +#define CLKID_ACODEC		31
> +#define CLKID_OTP		32
> +#define CLKID_SD_EMMC_A		33
> +#define CLKID_USB_PHY		34
> +#define CLKID_USB_CTRL		35
> +#define CLKID_SYS_DSPB		36
> +#define CLKID_SYS_DSPA		37
> +#define CLKID_DMA		38
> +#define CLKID_IRQ_CTRL		39
> +#define CLKID_NIC		40
> +#define CLKID_GIC		41
> +#define CLKID_UART_C		42
> +#define CLKID_UART_B		43
> +#define CLKID_UART_A		44
> +#define CLKID_SYS_PSRAM		45
> +#define CLKID_RSA		46
> +#define CLKID_CORESIGHT		47
> +#define CLKID_AM2AXI_VAD	48
> +#define CLKID_AUDIO_VAD		49
> +#define CLKID_AXI_DMC		50
> +#define CLKID_AXI_PSRAM		51
> +#define CLKID_RAMB		52
> +#define CLKID_RAMA		53
> +#define CLKID_AXI_SPIFC		54
> +#define CLKID_AXI_NIC		55
> +#define CLKID_AXI_DMA		56
> +#define CLKID_CPU_CTRL		57
> +#define CLKID_ROM		58
> +#define CLKID_PROC_I2C		59
> +#define CLKID_DSPA_EN		60
> +#define CLKID_DSPA_EN_NIC	61
> +#define CLKID_DSPB_EN		62
> +#define CLKID_DSPB_EN_NIC	63
> +#define CLKID_RTC		64
> +#define CLKID_CECA_32K		65
> +#define CLKID_CECB_32K		66
> +#define CLKID_24M		67
> +#define CLKID_12M		68
> +#define CLKID_FCLK_DIV2_DIVN	69
> +#define CLKID_GEN		70
> +#define CLKID_SARADC		71
> +#define CLKID_PWM_A		72
> +#define CLKID_PWM_B		73
> +#define CLKID_PWM_C		74
> +#define CLKID_PWM_D		75
> +#define CLKID_PWM_E		76
> +#define CLKID_PWM_F		77
> +#define CLKID_SPICC		78
> +#define CLKID_TS		79
> +#define CLKID_SPIFC		80
> +#define CLKID_USB_BUS		81
> +#define CLKID_SD_EMMC		82
> +#define CLKID_PSRAM		83
> +#define CLKID_DMC		84
> +#define CLKID_GEN_SEL		85
> +#define CLKID_PWM_A_SEL		86
> +#define CLKID_PWM_B_SEL		87
> +#define CLKID_PWM_C_SEL		88
> +#define CLKID_PWM_D_SEL		89
> +#define CLKID_PWM_E_SEL		90
> +#define CLKID_PWM_F_SEL		91
> +#define CLKID_DSPA_A_SEL	92
> +#define CLKID_DSPA_B_SEL	93
> +#define CLKID_DSPB_A_SEL	94
> +#define CLKID_DSPB_B_SEL	95
> +#define CLKID_CECA_32K_SEL	96
> +#define CLKID_CECB_32K_SEL	97
> +#define NR_CLKS			98
> +
> +#endif /* __A1_CLKC_H */
> diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> new file mode 100644
> index 000000000000..740fe8c4db5d
> --- /dev/null
> +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + * Author: Jian Hu <[e-mail]>
> + *
> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <[e-mail]>
> + */
> +
> +#ifndef __A1_PLL_CLKC_H
> +#define __A1_PLL_CLKC_H
> +
> +#define CLKID_FIXED_PLL		0
> +#define CLKID_FCLK_DIV2		1
> +#define CLKID_FCLK_DIV3		2
> +#define CLKID_FCLK_DIV5		3
> +#define CLKID_FCLK_DIV7		4
> +#define CLKID_HIFI_PLL		5
> +#define NR_PLL_CLKS		6
> +
> +#endif /* __A1_PLL_CLKC_H */

Dmitry Rokosov March 27, 2023, 10:51 a.m. UTC | #2

On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
> 
> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
> 
> > Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
> > clock drivers.
> > Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
> > bindings and include them to MAINTAINERS.
> >
> > Signed-off-by: Jian Hu <[e-mail]>
> > Signed-off-by: Dmitry Rokosov <[e-mail]>
> > ---
> >  .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
> >  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
> >  MAINTAINERS                                   |   1 +
> >  include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
> >  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
> >  5 files changed, 267 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> >  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> 
> There is two drivers (and 2 independent patches). There should be 2
> bindings patches as well.
> 

Before, in previous versions I had two versions, but it wasn't bisectable
approach.
a1-clkc schema depends on a1-pll-clkc headers and vice versa.
It means dt schemas checkers will show us failure if we split them into two
patchsets.
I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
look like production schema and it requires one more patchset above the
series with proper CLKID definitons usage and proper header including.

BTW, there is an example of Rob's test bot failure found in the previous
v10 patch series due to chicken or the egg problem.
https://lore.kernel.org/all/[e-mail]/

Please advise what's the best practice to resolve that..

> >  create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h
> >  create mode 100644 include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> >
> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> > new file mode 100644
> > index 000000000000..cb6d8f4eb959
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/amlogic,a1-clkc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Amlogic Meson A/C serials Peripheral Clock Control Unit
> > +
> > +maintainers:
> > +  - Neil Armstrong <[e-mail]>
> > +  - Jerome Brunet <[e-mail]>
> > +  - Jian Hu <[e-mail]>
> > +  - Dmitry Rokosov <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    const: amlogic,a1-clkc
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: input fixed pll div2
> > +      - description: input fixed pll div3
> > +      - description: input fixed pll div5
> > +      - description: input fixed pll div7
> > +      - description: input hifi pll
> > +      - description: input oscillator (usually at 24MHz)
> > +
> > +  clock-names:
> > +    items:
> > +      - const: fclk_div2
> > +      - const: fclk_div3
> > +      - const: fclk_div5
> > +      - const: fclk_div7
> > +      - const: hifi_pll
> > +      - const: xtal
> > +
> > +required:
> > +  - compatible
> > +  - '#clock-cells'
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/amlogic,a1-pll-clkc.h>
> > +    apb {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        clock-controller@800 {
> > +            compatible = "amlogic,a1-clkc";
> > +            reg = <0 0x800 0 0x104>;
> > +            #clock-cells = <1>;
> > +            clocks = <&clkc_pll CLKID_FCLK_DIV2>,
> > +                     <&clkc_pll CLKID_FCLK_DIV3>,
> > +                     <&clkc_pll CLKID_FCLK_DIV5>,
> > +                     <&clkc_pll CLKID_FCLK_DIV7>,
> > +                     <&clkc_pll CLKID_HIFI_PLL>,
> > +                     <&xtal>;
> > +            clock-names = "fclk_div2", "fclk_div3",
> > +                          "fclk_div5", "fclk_div7",
> > +                          "hifi_pll", "xtal";
> > +        };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > new file mode 100644
> > index 000000000000..77a13b1f9d5a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Amlogic Meson A/C serials PLL Clock Control Unit
> > +
> > +maintainers:
> > +  - Neil Armstrong <[e-mail]>
> > +  - Jerome Brunet <[e-mail]>
> > +  - Jian Hu <[e-mail]>
> > +  - Dmitry Rokosov <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    const: amlogic,a1-pll-clkc
> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: input fixpll_in
> > +      - description: input hifipll_in
> > +
> > +  clock-names:
> > +    items:
> > +      - const: fixpll_in
> > +      - const: hifipll_in
> > +
> > +required:
> > +  - compatible
> > +  - '#clock-cells'
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/amlogic,a1-clkc.h>
> > +    apb {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        clock-controller@7c80 {
> > +            compatible = "amlogic,a1-pll-clkc";
> > +            reg = <0 0x7c80 0 0x18c>;
> > +            #clock-cells = <1>;
> > +            clocks = <&clkc_periphs CLKID_FIXPLL_IN>,
> > +                     <&clkc_periphs CLKID_HIFIPLL_IN>;
> > +            clock-names = "fixpll_in", "hifipll_in";
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 39ff1a717625..8438bc9bd636 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1895,6 +1895,7 @@ L:	[e-mail]
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/clock/amlogic*
> >  F:	drivers/clk/meson/
> > +F:	include/dt-bindings/clock/a1*
> >  F:	include/dt-bindings/clock/gxbb*
> >  F:	include/dt-bindings/clock/meson*
> >  
> > diff --git a/include/dt-bindings/clock/amlogic,a1-clkc.h b/include/dt-bindings/clock/amlogic,a1-clkc.h
> > new file mode 100644
> > index 000000000000..95131779982c
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/amlogic,a1-clkc.h
> > @@ -0,0 +1,113 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> > +/*
> > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> > + * Author: Jian Hu <[e-mail]>
> > + *
> > + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> > + * Author: Dmitry Rokosov <[e-mail]>
> > + */
> > +
> > +#ifndef __A1_CLKC_H
> > +#define __A1_CLKC_H
> > +
> > +#define CLKID_FIXPLL_IN		0
> > +#define CLKID_USB_PHY_IN	1
> > +#define CLKID_USB_CTRL_IN	2
> > +#define CLKID_HIFIPLL_IN	3
> > +#define CLKID_SYSPLL_IN		4
> > +#define CLKID_DDS_IN		5
> > +#define CLKID_SYS		6
> > +#define CLKID_CLKTREE		7
> > +#define CLKID_RESET_CTRL	8
> > +#define CLKID_ANALOG_CTRL	9
> > +#define CLKID_PWR_CTRL		10
> > +#define CLKID_PAD_CTRL		11
> > +#define CLKID_SYS_CTRL		12
> > +#define CLKID_TEMP_SENSOR	13
> > +#define CLKID_AM2AXI_DIV	14
> > +#define CLKID_SPICC_B		15
> > +#define CLKID_SPICC_A		16
> > +#define CLKID_MSR		17
> > +#define CLKID_AUDIO		18
> > +#define CLKID_JTAG_CTRL		19
> > +#define CLKID_SARADC_EN		20
> > +#define CLKID_PWM_EF		21
> > +#define CLKID_PWM_CD		22
> > +#define CLKID_PWM_AB		23
> > +#define CLKID_CEC		24
> > +#define CLKID_I2C_S		25
> > +#define CLKID_IR_CTRL		26
> > +#define CLKID_I2C_M_D		27
> > +#define CLKID_I2C_M_C		28
> > +#define CLKID_I2C_M_B		29
> > +#define CLKID_I2C_M_A		30
> > +#define CLKID_ACODEC		31
> > +#define CLKID_OTP		32
> > +#define CLKID_SD_EMMC_A		33
> > +#define CLKID_USB_PHY		34
> > +#define CLKID_USB_CTRL		35
> > +#define CLKID_SYS_DSPB		36
> > +#define CLKID_SYS_DSPA		37
> > +#define CLKID_DMA		38
> > +#define CLKID_IRQ_CTRL		39
> > +#define CLKID_NIC		40
> > +#define CLKID_GIC		41
> > +#define CLKID_UART_C		42
> > +#define CLKID_UART_B		43
> > +#define CLKID_UART_A		44
> > +#define CLKID_SYS_PSRAM		45
> > +#define CLKID_RSA		46
> > +#define CLKID_CORESIGHT		47
> > +#define CLKID_AM2AXI_VAD	48
> > +#define CLKID_AUDIO_VAD		49
> > +#define CLKID_AXI_DMC		50
> > +#define CLKID_AXI_PSRAM		51
> > +#define CLKID_RAMB		52
> > +#define CLKID_RAMA		53
> > +#define CLKID_AXI_SPIFC		54
> > +#define CLKID_AXI_NIC		55
> > +#define CLKID_AXI_DMA		56
> > +#define CLKID_CPU_CTRL		57
> > +#define CLKID_ROM		58
> > +#define CLKID_PROC_I2C		59
> > +#define CLKID_DSPA_EN		60
> > +#define CLKID_DSPA_EN_NIC	61
> > +#define CLKID_DSPB_EN		62
> > +#define CLKID_DSPB_EN_NIC	63
> > +#define CLKID_RTC		64
> > +#define CLKID_CECA_32K		65
> > +#define CLKID_CECB_32K		66
> > +#define CLKID_24M		67
> > +#define CLKID_12M		68
> > +#define CLKID_FCLK_DIV2_DIVN	69
> > +#define CLKID_GEN		70
> > +#define CLKID_SARADC		71
> > +#define CLKID_PWM_A		72
> > +#define CLKID_PWM_B		73
> > +#define CLKID_PWM_C		74
> > +#define CLKID_PWM_D		75
> > +#define CLKID_PWM_E		76
> > +#define CLKID_PWM_F		77
> > +#define CLKID_SPICC		78
> > +#define CLKID_TS		79
> > +#define CLKID_SPIFC		80
> > +#define CLKID_USB_BUS		81
> > +#define CLKID_SD_EMMC		82
> > +#define CLKID_PSRAM		83
> > +#define CLKID_DMC		84
> > +#define CLKID_GEN_SEL		85
> > +#define CLKID_PWM_A_SEL		86
> > +#define CLKID_PWM_B_SEL		87
> > +#define CLKID_PWM_C_SEL		88
> > +#define CLKID_PWM_D_SEL		89
> > +#define CLKID_PWM_E_SEL		90
> > +#define CLKID_PWM_F_SEL		91
> > +#define CLKID_DSPA_A_SEL	92
> > +#define CLKID_DSPA_B_SEL	93
> > +#define CLKID_DSPB_A_SEL	94
> > +#define CLKID_DSPB_B_SEL	95
> > +#define CLKID_CECA_32K_SEL	96
> > +#define CLKID_CECB_32K_SEL	97
> > +#define NR_CLKS			98
> > +
> > +#endif /* __A1_CLKC_H */
> > diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> > new file mode 100644
> > index 000000000000..740fe8c4db5d
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> > +/*
> > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> > + * Author: Jian Hu <[e-mail]>
> > + *
> > + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> > + * Author: Dmitry Rokosov <[e-mail]>
> > + */
> > +
> > +#ifndef __A1_PLL_CLKC_H
> > +#define __A1_PLL_CLKC_H
> > +
> > +#define CLKID_FIXED_PLL		0
> > +#define CLKID_FCLK_DIV2		1
> > +#define CLKID_FCLK_DIV3		2
> > +#define CLKID_FCLK_DIV5		3
> > +#define CLKID_FCLK_DIV7		4
> > +#define CLKID_HIFI_PLL		5
> > +#define NR_PLL_CLKS		6
> > +
> > +#endif /* __A1_PLL_CLKC_H */
>

Jerome Brunet March 27, 2023, 11:39 a.m. UTC | #3

On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:

> On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
>> 
>> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
>> 
>> > Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
>> > clock drivers.
>> > Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
>> > bindings and include them to MAINTAINERS.
>> >
>> > Signed-off-by: Jian Hu <[e-mail]>
>> > Signed-off-by: Dmitry Rokosov <[e-mail]>
>> > ---
>> >  .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
>> >  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
>> >  MAINTAINERS                                   |   1 +
>> >  include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
>> >  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
>> >  5 files changed, 267 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>> >  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> 
>> There is two drivers (and 2 independent patches). There should be 2
>> bindings patches as well.
>> 
>
> Before, in previous versions I had two versions, but it wasn't bisectable
> approach.

You are confusing bisectable and Rob's robot. Splitting patches is more
that likely to help bisect (and patches backport) - not the other way around.

> a1-clkc schema depends on a1-pll-clkc headers and vice versa.
> It means dt schemas checkers will show us failure if we split them into two
> patchsets.

Only because you are patches are not upstream yet ...

> I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
> look like production schema and it requires one more patchset above the
> series with proper CLKID definitons usage and proper header including.
>
> BTW, there is an example of Rob's test bot failure found in the previous
> v10 patch series due to chicken or the egg problem.
> https://lore.kernel.org/all/[e-mail]/
>
> Please advise what's the best practice to resolve that..

Don't use the header in your example would solve the problem and
still be correct DT wise.

The examples are just examples, they are not required to actually
matches a real HW, as far as I know.

>
>> >  create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h
>> >  create mode 100644 include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>> >
>> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>> > new file mode 100644
>> > index 000000000000..cb6d8f4eb959
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>> > @@ -0,0 +1,73 @@
>> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> > +%YAML 1.2
>> > +---
>> > +$id: http://devicetree.org/schemas/clock/amlogic,a1-clkc.yaml#
>> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> > +
>> > +title: Amlogic Meson A/C serials Peripheral Clock Control Unit
>> > +
>> > +maintainers:
>> > +  - Neil Armstrong <[e-mail]>
>> > +  - Jerome Brunet <[e-mail]>
>> > +  - Jian Hu <[e-mail]>
>> > +  - Dmitry Rokosov <[e-mail]>
>> > +
>> > +properties:
>> > +  compatible:
>> > +    const: amlogic,a1-clkc
>> > +
>> > +  '#clock-cells':
>> > +    const: 1
>> > +
>> > +  reg:
>> > +    maxItems: 1
>> > +
>> > +  clocks:
>> > +    items:
>> > +      - description: input fixed pll div2
>> > +      - description: input fixed pll div3
>> > +      - description: input fixed pll div5
>> > +      - description: input fixed pll div7
>> > +      - description: input hifi pll
>> > +      - description: input oscillator (usually at 24MHz)
>> > +
>> > +  clock-names:
>> > +    items:
>> > +      - const: fclk_div2
>> > +      - const: fclk_div3
>> > +      - const: fclk_div5
>> > +      - const: fclk_div7
>> > +      - const: hifi_pll
>> > +      - const: xtal
>> > +
>> > +required:
>> > +  - compatible
>> > +  - '#clock-cells'
>> > +  - reg
>> > +  - clocks
>> > +  - clock-names
>> > +
>> > +additionalProperties: false
>> > +
>> > +examples:
>> > +  - |
>> > +    #include <dt-bindings/clock/amlogic,a1-pll-clkc.h>
>> > +    apb {
>> > +        #address-cells = <2>;
>> > +        #size-cells = <2>;
>> > +
>> > +        clock-controller@800 {
>> > +            compatible = "amlogic,a1-clkc";
>> > +            reg = <0 0x800 0 0x104>;
>> > +            #clock-cells = <1>;
>> > +            clocks = <&clkc_pll CLKID_FCLK_DIV2>,
>> > +                     <&clkc_pll CLKID_FCLK_DIV3>,
>> > +                     <&clkc_pll CLKID_FCLK_DIV5>,
>> > +                     <&clkc_pll CLKID_FCLK_DIV7>,
>> > +                     <&clkc_pll CLKID_HIFI_PLL>,
>> > +                     <&xtal>;
>> > +            clock-names = "fclk_div2", "fclk_div3",
>> > +                          "fclk_div5", "fclk_div7",
>> > +                          "hifi_pll", "xtal";
>> > +        };
>> > +    };
>> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> > new file mode 100644
>> > index 000000000000..77a13b1f9d5a
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> > @@ -0,0 +1,59 @@
>> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> > +%YAML 1.2
>> > +---
>> > +$id: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
>> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> > +
>> > +title: Amlogic Meson A/C serials PLL Clock Control Unit
>> > +
>> > +maintainers:
>> > +  - Neil Armstrong <[e-mail]>
>> > +  - Jerome Brunet <[e-mail]>
>> > +  - Jian Hu <[e-mail]>
>> > +  - Dmitry Rokosov <[e-mail]>
>> > +
>> > +properties:
>> > +  compatible:
>> > +    const: amlogic,a1-pll-clkc
>> > +
>> > +  '#clock-cells':
>> > +    const: 1
>> > +
>> > +  reg:
>> > +    maxItems: 1
>> > +
>> > +  clocks:
>> > +    items:
>> > +      - description: input fixpll_in
>> > +      - description: input hifipll_in
>> > +
>> > +  clock-names:
>> > +    items:
>> > +      - const: fixpll_in
>> > +      - const: hifipll_in
>> > +
>> > +required:
>> > +  - compatible
>> > +  - '#clock-cells'
>> > +  - reg
>> > +  - clocks
>> > +  - clock-names
>> > +
>> > +additionalProperties: false
>> > +
>> > +examples:
>> > +  - |
>> > +    #include <dt-bindings/clock/amlogic,a1-clkc.h>
>> > +    apb {
>> > +        #address-cells = <2>;
>> > +        #size-cells = <2>;
>> > +
>> > +        clock-controller@7c80 {
>> > +            compatible = "amlogic,a1-pll-clkc";
>> > +            reg = <0 0x7c80 0 0x18c>;
>> > +            #clock-cells = <1>;
>> > +            clocks = <&clkc_periphs CLKID_FIXPLL_IN>,
>> > +                     <&clkc_periphs CLKID_HIFIPLL_IN>;
>> > +            clock-names = "fixpll_in", "hifipll_in";
>> > +        };
>> > +    };
>> > diff --git a/MAINTAINERS b/MAINTAINERS
>> > index 39ff1a717625..8438bc9bd636 100644
>> > --- a/MAINTAINERS
>> > +++ b/MAINTAINERS
>> > @@ -1895,6 +1895,7 @@ L:	[e-mail]
>> >  S:	Maintained
>> >  F:	Documentation/devicetree/bindings/clock/amlogic*
>> >  F:	drivers/clk/meson/
>> > +F:	include/dt-bindings/clock/a1*
>> >  F:	include/dt-bindings/clock/gxbb*
>> >  F:	include/dt-bindings/clock/meson*
>> >  
>> > diff --git a/include/dt-bindings/clock/amlogic,a1-clkc.h b/include/dt-bindings/clock/amlogic,a1-clkc.h
>> > new file mode 100644
>> > index 000000000000..95131779982c
>> > --- /dev/null
>> > +++ b/include/dt-bindings/clock/amlogic,a1-clkc.h
>> > @@ -0,0 +1,113 @@
>> > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> > +/*
>> > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> > + * Author: Jian Hu <[e-mail]>
>> > + *
>> > + * Copyright (c) 2023, SberDevices. All Rights Reserved.
>> > + * Author: Dmitry Rokosov <[e-mail]>
>> > + */
>> > +
>> > +#ifndef __A1_CLKC_H
>> > +#define __A1_CLKC_H
>> > +
>> > +#define CLKID_FIXPLL_IN		0
>> > +#define CLKID_USB_PHY_IN	1
>> > +#define CLKID_USB_CTRL_IN	2
>> > +#define CLKID_HIFIPLL_IN	3
>> > +#define CLKID_SYSPLL_IN		4
>> > +#define CLKID_DDS_IN		5
>> > +#define CLKID_SYS		6
>> > +#define CLKID_CLKTREE		7
>> > +#define CLKID_RESET_CTRL	8
>> > +#define CLKID_ANALOG_CTRL	9
>> > +#define CLKID_PWR_CTRL		10
>> > +#define CLKID_PAD_CTRL		11
>> > +#define CLKID_SYS_CTRL		12
>> > +#define CLKID_TEMP_SENSOR	13
>> > +#define CLKID_AM2AXI_DIV	14
>> > +#define CLKID_SPICC_B		15
>> > +#define CLKID_SPICC_A		16
>> > +#define CLKID_MSR		17
>> > +#define CLKID_AUDIO		18
>> > +#define CLKID_JTAG_CTRL		19
>> > +#define CLKID_SARADC_EN		20
>> > +#define CLKID_PWM_EF		21
>> > +#define CLKID_PWM_CD		22
>> > +#define CLKID_PWM_AB		23
>> > +#define CLKID_CEC		24
>> > +#define CLKID_I2C_S		25
>> > +#define CLKID_IR_CTRL		26
>> > +#define CLKID_I2C_M_D		27
>> > +#define CLKID_I2C_M_C		28
>> > +#define CLKID_I2C_M_B		29
>> > +#define CLKID_I2C_M_A		30
>> > +#define CLKID_ACODEC		31
>> > +#define CLKID_OTP		32
>> > +#define CLKID_SD_EMMC_A		33
>> > +#define CLKID_USB_PHY		34
>> > +#define CLKID_USB_CTRL		35
>> > +#define CLKID_SYS_DSPB		36
>> > +#define CLKID_SYS_DSPA		37
>> > +#define CLKID_DMA		38
>> > +#define CLKID_IRQ_CTRL		39
>> > +#define CLKID_NIC		40
>> > +#define CLKID_GIC		41
>> > +#define CLKID_UART_C		42
>> > +#define CLKID_UART_B		43
>> > +#define CLKID_UART_A		44
>> > +#define CLKID_SYS_PSRAM		45
>> > +#define CLKID_RSA		46
>> > +#define CLKID_CORESIGHT		47
>> > +#define CLKID_AM2AXI_VAD	48
>> > +#define CLKID_AUDIO_VAD		49
>> > +#define CLKID_AXI_DMC		50
>> > +#define CLKID_AXI_PSRAM		51
>> > +#define CLKID_RAMB		52
>> > +#define CLKID_RAMA		53
>> > +#define CLKID_AXI_SPIFC		54
>> > +#define CLKID_AXI_NIC		55
>> > +#define CLKID_AXI_DMA		56
>> > +#define CLKID_CPU_CTRL		57
>> > +#define CLKID_ROM		58
>> > +#define CLKID_PROC_I2C		59
>> > +#define CLKID_DSPA_EN		60
>> > +#define CLKID_DSPA_EN_NIC	61
>> > +#define CLKID_DSPB_EN		62
>> > +#define CLKID_DSPB_EN_NIC	63
>> > +#define CLKID_RTC		64
>> > +#define CLKID_CECA_32K		65
>> > +#define CLKID_CECB_32K		66
>> > +#define CLKID_24M		67
>> > +#define CLKID_12M		68
>> > +#define CLKID_FCLK_DIV2_DIVN	69
>> > +#define CLKID_GEN		70
>> > +#define CLKID_SARADC		71
>> > +#define CLKID_PWM_A		72
>> > +#define CLKID_PWM_B		73
>> > +#define CLKID_PWM_C		74
>> > +#define CLKID_PWM_D		75
>> > +#define CLKID_PWM_E		76
>> > +#define CLKID_PWM_F		77
>> > +#define CLKID_SPICC		78
>> > +#define CLKID_TS		79
>> > +#define CLKID_SPIFC		80
>> > +#define CLKID_USB_BUS		81
>> > +#define CLKID_SD_EMMC		82
>> > +#define CLKID_PSRAM		83
>> > +#define CLKID_DMC		84
>> > +#define CLKID_GEN_SEL		85
>> > +#define CLKID_PWM_A_SEL		86
>> > +#define CLKID_PWM_B_SEL		87
>> > +#define CLKID_PWM_C_SEL		88
>> > +#define CLKID_PWM_D_SEL		89
>> > +#define CLKID_PWM_E_SEL		90
>> > +#define CLKID_PWM_F_SEL		91
>> > +#define CLKID_DSPA_A_SEL	92
>> > +#define CLKID_DSPA_B_SEL	93
>> > +#define CLKID_DSPB_A_SEL	94
>> > +#define CLKID_DSPB_B_SEL	95
>> > +#define CLKID_CECA_32K_SEL	96
>> > +#define CLKID_CECB_32K_SEL	97
>> > +#define NR_CLKS			98
>> > +
>> > +#endif /* __A1_CLKC_H */
>> > diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>> > new file mode 100644
>> > index 000000000000..740fe8c4db5d
>> > --- /dev/null
>> > +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>> > @@ -0,0 +1,21 @@
>> > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> > +/*
>> > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> > + * Author: Jian Hu <[e-mail]>
>> > + *
>> > + * Copyright (c) 2023, SberDevices. All Rights Reserved.
>> > + * Author: Dmitry Rokosov <[e-mail]>
>> > + */
>> > +
>> > +#ifndef __A1_PLL_CLKC_H
>> > +#define __A1_PLL_CLKC_H
>> > +
>> > +#define CLKID_FIXED_PLL		0
>> > +#define CLKID_FCLK_DIV2		1
>> > +#define CLKID_FCLK_DIV3		2
>> > +#define CLKID_FCLK_DIV5		3
>> > +#define CLKID_FCLK_DIV7		4
>> > +#define CLKID_HIFI_PLL		5
>> > +#define NR_PLL_CLKS		6
>> > +
>> > +#endif /* __A1_PLL_CLKC_H */
>>

Neil Armstrong March 27, 2023, 12:03 p.m. UTC | #4

On 27/03/2023 13:39, Jerome Brunet wrote:
> 
> On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:
> 
>> On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
>>>
>>> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
>>>
>>>> Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
>>>> clock drivers.
>>>> Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
>>>> bindings and include them to MAINTAINERS.
>>>>
>>>> Signed-off-by: Jian Hu <[e-mail]>
>>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
>>>> ---
>>>>   .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
>>>>   .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
>>>>   MAINTAINERS                                   |   1 +
>>>>   include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
>>>>   .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
>>>>   5 files changed, 267 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>>>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>
>>> There is two drivers (and 2 independent patches). There should be 2
>>> bindings patches as well.
>>>
>>
>> Before, in previous versions I had two versions, but it wasn't bisectable
>> approach.
> 
> You are confusing bisectable and Rob's robot. Splitting patches is more
> that likely to help bisect (and patches backport) - not the other way around.
> 
>> a1-clkc schema depends on a1-pll-clkc headers and vice versa.
>> It means dt schemas checkers will show us failure if we split them into two
>> patchsets.
> 
> Only because you are patches are not upstream yet ...
> 
>> I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
>> look like production schema and it requires one more patchset above the
>> series with proper CLKID definitons usage and proper header including.
>>
>> BTW, there is an example of Rob's test bot failure found in the previous
>> v10 patch series due to chicken or the egg problem.
>> https://lore.kernel.org/all/[e-mail]/
>>
>> Please advise what's the best practice to resolve that..
> 
> Don't use the header in your example would solve the problem and
> still be correct DT wise.
> 
> The examples are just examples, they are not required to actually
> matches a real HW, as far as I know.

Exact, you can use fake lables instead of defined:

<&clkc_pll CLKID_FCLK_DIV2>,

=>
remove "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"

<&clkc_pll_fclk_div2>,

is perfectly ok and will permit have 2 separate patches.

The dependency is only if you have a common yaml file for
both bindings files, but this is not the case here.

Neil

> 
>>
>>>>   create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h
>>>>   create mode 100644 include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>>>> new file mode 100644
>>>> index 000000000000..cb6d8f4eb959
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>>>> @@ -0,0 +1,73 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/clock/amlogic,a1-clkc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Amlogic Meson A/C serials Peripheral Clock Control Unit
>>>> +
>>>> +maintainers:
>>>> +  - Neil Armstrong <[e-mail]>
>>>> +  - Jerome Brunet <[e-mail]>
>>>> +  - Jian Hu <[e-mail]>
>>>> +  - Dmitry Rokosov <[e-mail]>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: amlogic,a1-clkc
>>>> +
>>>> +  '#clock-cells':
>>>> +    const: 1
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    items:
>>>> +      - description: input fixed pll div2
>>>> +      - description: input fixed pll div3
>>>> +      - description: input fixed pll div5
>>>> +      - description: input fixed pll div7
>>>> +      - description: input hifi pll
>>>> +      - description: input oscillator (usually at 24MHz)
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: fclk_div2
>>>> +      - const: fclk_div3
>>>> +      - const: fclk_div5
>>>> +      - const: fclk_div7
>>>> +      - const: hifi_pll
>>>> +      - const: xtal
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - '#clock-cells'
>>>> +  - reg
>>>> +  - clocks
>>>> +  - clock-names
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/clock/amlogic,a1-pll-clkc.h>
>>>> +    apb {
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +
>>>> +        clock-controller@800 {
>>>> +            compatible = "amlogic,a1-clkc";
>>>> +            reg = <0 0x800 0 0x104>;
>>>> +            #clock-cells = <1>;
>>>> +            clocks = <&clkc_pll CLKID_FCLK_DIV2>,
>>>> +                     <&clkc_pll CLKID_FCLK_DIV3>,
>>>> +                     <&clkc_pll CLKID_FCLK_DIV5>,
>>>> +                     <&clkc_pll CLKID_FCLK_DIV7>,
>>>> +                     <&clkc_pll CLKID_HIFI_PLL>,
>>>> +                     <&xtal>;
>>>> +            clock-names = "fclk_div2", "fclk_div3",
>>>> +                          "fclk_div5", "fclk_div7",
>>>> +                          "hifi_pll", "xtal";
>>>> +        };
>>>> +    };
>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>> new file mode 100644
>>>> index 000000000000..77a13b1f9d5a
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>> @@ -0,0 +1,59 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Amlogic Meson A/C serials PLL Clock Control Unit
>>>> +
>>>> +maintainers:
>>>> +  - Neil Armstrong <[e-mail]>
>>>> +  - Jerome Brunet <[e-mail]>
>>>> +  - Jian Hu <[e-mail]>
>>>> +  - Dmitry Rokosov <[e-mail]>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: amlogic,a1-pll-clkc
>>>> +
>>>> +  '#clock-cells':
>>>> +    const: 1
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    items:
>>>> +      - description: input fixpll_in
>>>> +      - description: input hifipll_in
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: fixpll_in
>>>> +      - const: hifipll_in
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - '#clock-cells'
>>>> +  - reg
>>>> +  - clocks
>>>> +  - clock-names
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/clock/amlogic,a1-clkc.h>
>>>> +    apb {
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +
>>>> +        clock-controller@7c80 {
>>>> +            compatible = "amlogic,a1-pll-clkc";
>>>> +            reg = <0 0x7c80 0 0x18c>;
>>>> +            #clock-cells = <1>;
>>>> +            clocks = <&clkc_periphs CLKID_FIXPLL_IN>,
>>>> +                     <&clkc_periphs CLKID_HIFIPLL_IN>;
>>>> +            clock-names = "fixpll_in", "hifipll_in";
>>>> +        };
>>>> +    };
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 39ff1a717625..8438bc9bd636 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -1895,6 +1895,7 @@ L:	[e-mail]
>>>>   S:	Maintained
>>>>   F:	Documentation/devicetree/bindings/clock/amlogic*
>>>>   F:	drivers/clk/meson/
>>>> +F:	include/dt-bindings/clock/a1*
>>>>   F:	include/dt-bindings/clock/gxbb*
>>>>   F:	include/dt-bindings/clock/meson*
>>>>   
>>>> diff --git a/include/dt-bindings/clock/amlogic,a1-clkc.h b/include/dt-bindings/clock/amlogic,a1-clkc.h
>>>> new file mode 100644
>>>> index 000000000000..95131779982c
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/clock/amlogic,a1-clkc.h
>>>> @@ -0,0 +1,113 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>>>> +/*
>>>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>>>> + * Author: Jian Hu <[e-mail]>
>>>> + *
>>>> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
>>>> + * Author: Dmitry Rokosov <[e-mail]>
>>>> + */
>>>> +
>>>> +#ifndef __A1_CLKC_H
>>>> +#define __A1_CLKC_H
>>>> +
>>>> +#define CLKID_FIXPLL_IN		0
>>>> +#define CLKID_USB_PHY_IN	1
>>>> +#define CLKID_USB_CTRL_IN	2
>>>> +#define CLKID_HIFIPLL_IN	3
>>>> +#define CLKID_SYSPLL_IN		4
>>>> +#define CLKID_DDS_IN		5
>>>> +#define CLKID_SYS		6
>>>> +#define CLKID_CLKTREE		7
>>>> +#define CLKID_RESET_CTRL	8
>>>> +#define CLKID_ANALOG_CTRL	9
>>>> +#define CLKID_PWR_CTRL		10
>>>> +#define CLKID_PAD_CTRL		11
>>>> +#define CLKID_SYS_CTRL		12
>>>> +#define CLKID_TEMP_SENSOR	13
>>>> +#define CLKID_AM2AXI_DIV	14
>>>> +#define CLKID_SPICC_B		15
>>>> +#define CLKID_SPICC_A		16
>>>> +#define CLKID_MSR		17
>>>> +#define CLKID_AUDIO		18
>>>> +#define CLKID_JTAG_CTRL		19
>>>> +#define CLKID_SARADC_EN		20
>>>> +#define CLKID_PWM_EF		21
>>>> +#define CLKID_PWM_CD		22
>>>> +#define CLKID_PWM_AB		23
>>>> +#define CLKID_CEC		24
>>>> +#define CLKID_I2C_S		25
>>>> +#define CLKID_IR_CTRL		26
>>>> +#define CLKID_I2C_M_D		27
>>>> +#define CLKID_I2C_M_C		28
>>>> +#define CLKID_I2C_M_B		29
>>>> +#define CLKID_I2C_M_A		30
>>>> +#define CLKID_ACODEC		31
>>>> +#define CLKID_OTP		32
>>>> +#define CLKID_SD_EMMC_A		33
>>>> +#define CLKID_USB_PHY		34
>>>> +#define CLKID_USB_CTRL		35
>>>> +#define CLKID_SYS_DSPB		36
>>>> +#define CLKID_SYS_DSPA		37
>>>> +#define CLKID_DMA		38
>>>> +#define CLKID_IRQ_CTRL		39
>>>> +#define CLKID_NIC		40
>>>> +#define CLKID_GIC		41
>>>> +#define CLKID_UART_C		42
>>>> +#define CLKID_UART_B		43
>>>> +#define CLKID_UART_A		44
>>>> +#define CLKID_SYS_PSRAM		45
>>>> +#define CLKID_RSA		46
>>>> +#define CLKID_CORESIGHT		47
>>>> +#define CLKID_AM2AXI_VAD	48
>>>> +#define CLKID_AUDIO_VAD		49
>>>> +#define CLKID_AXI_DMC		50
>>>> +#define CLKID_AXI_PSRAM		51
>>>> +#define CLKID_RAMB		52
>>>> +#define CLKID_RAMA		53
>>>> +#define CLKID_AXI_SPIFC		54
>>>> +#define CLKID_AXI_NIC		55
>>>> +#define CLKID_AXI_DMA		56
>>>> +#define CLKID_CPU_CTRL		57
>>>> +#define CLKID_ROM		58
>>>> +#define CLKID_PROC_I2C		59
>>>> +#define CLKID_DSPA_EN		60
>>>> +#define CLKID_DSPA_EN_NIC	61
>>>> +#define CLKID_DSPB_EN		62
>>>> +#define CLKID_DSPB_EN_NIC	63
>>>> +#define CLKID_RTC		64
>>>> +#define CLKID_CECA_32K		65
>>>> +#define CLKID_CECB_32K		66
>>>> +#define CLKID_24M		67
>>>> +#define CLKID_12M		68
>>>> +#define CLKID_FCLK_DIV2_DIVN	69
>>>> +#define CLKID_GEN		70
>>>> +#define CLKID_SARADC		71
>>>> +#define CLKID_PWM_A		72
>>>> +#define CLKID_PWM_B		73
>>>> +#define CLKID_PWM_C		74
>>>> +#define CLKID_PWM_D		75
>>>> +#define CLKID_PWM_E		76
>>>> +#define CLKID_PWM_F		77
>>>> +#define CLKID_SPICC		78
>>>> +#define CLKID_TS		79
>>>> +#define CLKID_SPIFC		80
>>>> +#define CLKID_USB_BUS		81
>>>> +#define CLKID_SD_EMMC		82
>>>> +#define CLKID_PSRAM		83
>>>> +#define CLKID_DMC		84
>>>> +#define CLKID_GEN_SEL		85
>>>> +#define CLKID_PWM_A_SEL		86
>>>> +#define CLKID_PWM_B_SEL		87
>>>> +#define CLKID_PWM_C_SEL		88
>>>> +#define CLKID_PWM_D_SEL		89
>>>> +#define CLKID_PWM_E_SEL		90
>>>> +#define CLKID_PWM_F_SEL		91
>>>> +#define CLKID_DSPA_A_SEL	92
>>>> +#define CLKID_DSPA_B_SEL	93
>>>> +#define CLKID_DSPB_A_SEL	94
>>>> +#define CLKID_DSPB_B_SEL	95
>>>> +#define CLKID_CECA_32K_SEL	96
>>>> +#define CLKID_CECB_32K_SEL	97
>>>> +#define NR_CLKS			98
>>>> +
>>>> +#endif /* __A1_CLKC_H */
>>>> diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>>>> new file mode 100644
>>>> index 000000000000..740fe8c4db5d
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>>>> @@ -0,0 +1,21 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>>>> +/*
>>>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>>>> + * Author: Jian Hu <[e-mail]>
>>>> + *
>>>> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
>>>> + * Author: Dmitry Rokosov <[e-mail]>
>>>> + */
>>>> +
>>>> +#ifndef __A1_PLL_CLKC_H
>>>> +#define __A1_PLL_CLKC_H
>>>> +
>>>> +#define CLKID_FIXED_PLL		0
>>>> +#define CLKID_FCLK_DIV2		1
>>>> +#define CLKID_FCLK_DIV3		2
>>>> +#define CLKID_FCLK_DIV5		3
>>>> +#define CLKID_FCLK_DIV7		4
>>>> +#define CLKID_HIFI_PLL		5
>>>> +#define NR_PLL_CLKS		6
>>>> +
>>>> +#endif /* __A1_PLL_CLKC_H */
>>>
>

Dmitry Rokosov March 27, 2023, 1:19 p.m. UTC | #5

On Mon, Mar 27, 2023 at 02:03:25PM +0200, [e-mail] wrote:
> On 27/03/2023 13:39, Jerome Brunet wrote:
> > 
> > On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:
> > 
> > > On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
> > > > 
> > > > On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
> > > > 
> > > > > Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
> > > > > clock drivers.
> > > > > Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
> > > > > bindings and include them to MAINTAINERS.
> > > > > 
> > > > > Signed-off-by: Jian Hu <[e-mail]>
> > > > > Signed-off-by: Dmitry Rokosov <[e-mail]>
> > > > > ---
> > > > >   .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
> > > > >   .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
> > > > >   MAINTAINERS                                   |   1 +
> > > > >   include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
> > > > >   .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
> > > > >   5 files changed, 267 insertions(+)
> > > > >   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> > > > >   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > > > 
> > > > There is two drivers (and 2 independent patches). There should be 2
> > > > bindings patches as well.
> > > > 
> > > 
> > > Before, in previous versions I had two versions, but it wasn't bisectable
> > > approach.
> > 
> > You are confusing bisectable and Rob's robot. Splitting patches is more
> > that likely to help bisect (and patches backport) - not the other way around.
> > 
> > > a1-clkc schema depends on a1-pll-clkc headers and vice versa.
> > > It means dt schemas checkers will show us failure if we split them into two
> > > patchsets.
> > 
> > Only because you are patches are not upstream yet ...
> > 
> > > I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
> > > look like production schema and it requires one more patchset above the
> > > series with proper CLKID definitons usage and proper header including.
> > > 
> > > BTW, there is an example of Rob's test bot failure found in the previous
> > > v10 patch series due to chicken or the egg problem.
> > > https://lore.kernel.org/all/[e-mail]/
> > > 
> > > Please advise what's the best practice to resolve that..
> > 
> > Don't use the header in your example would solve the problem and
> > still be correct DT wise.
> > 
> > The examples are just examples, they are not required to actually
> > matches a real HW, as far as I know.
> 
> Exact, you can use fake lables instead of defined:
> 
> <&clkc_pll CLKID_FCLK_DIV2>,
> 
> =>
> remove "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
> 
> <&clkc_pll_fclk_div2>,
> 
> is perfectly ok and will permit have 2 separate patches.
> 
> The dependency is only if you have a common yaml file for
> both bindings files, but this is not the case here.

Simple removal of "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
header doesn't work, dt_binding_check make rule is failed:

Error: Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts:28.37-38 syntax error
FATAL ERROR: Unable to parse input tree

It happens, because 'dt_binding_check' generates simple dts example and
tries to compile it:

cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
===

/dts-v1/;
/plugin/; // silence any missing phandle references


/{
    compatible = "foo";
    model = "foo";
    #address-cells = <1>;
    #size-cells = <1>;



    example-0 {
        #address-cells = <1>;
        #size-cells = <1>;

        

        apb {
            #address-cells = <2>;
            #size-cells = <2>;
        
            clock-controller@800 {
                compatible = "amlogic,a1-clkc";
                reg = <0 0x800 0 0x104>;
                #clock-cells = <1>;
                clocks = <&clkc_pll CLKID_FCLK_DIV2>,
                         <&clkc_pll CLKID_FCLK_DIV3>,
                         <&clkc_pll CLKID_FCLK_DIV5>,
                         <&clkc_pll CLKID_FCLK_DIV7>,
                         <&clkc_pll CLKID_HIFI_PLL>,
                         <&xtal>;
                clock-names = "fclk_div2", "fclk_div3",
                              "fclk_div5", "fclk_div7",
                              "hifi_pll", "xtal";
            };
        };

    };
};
===

As you can see, header is required.

But looks like, dt binding checker is happy with the fake references hack :)
Below there is generated example dts:

cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
===

/dts-v1/;
/plugin/; // silence any missing phandle references


/{
    compatible = "foo";
    model = "foo";
    #address-cells = <1>;
    #size-cells = <1>;



    example-0 {
        #address-cells = <1>;
        #size-cells = <1>;

        

        apb {
            #address-cells = <2>;
            #size-cells = <2>;
        
            clock-controller@800 {
                compatible = "amlogic,a1-clkc";
                reg = <0 0x800 0 0x104>;
                #clock-cells = <1>;
                clocks = <&clkc_pll_fclk_div2>,
                         <&clkc_pll_fclk_div3>,
                         <&clkc_pll_fclk_div5>,
                         <&clkc_pll_fclk_div7>,
                         <&clkc_pll_hifi_pll>,
                         <&xtal>;
                clock-names = "fclk_div2", "fclk_div3",
                              "fclk_div5", "fclk_div7",
                              "hifi_pll", "xtal";
            };
        };

    };
};
===

Yep, we are able to cheat dt checkers, but we don't help dt developers
with such example.
May be, it's better to prepare two patches in such hierarchy:

1) A1 PLL clkc bindings with fake references without clkc headers
2) A1 clkc bindings with real CLKID bindings + A1 PLL clkc bindings fix
with real CLKID A1 clkc bindings + header.

The such approach resolves DT checkers failures and split DT bindings
into two patchsets.

[...]

Dmitry Rokosov March 27, 2023, 1:22 p.m. UTC | #6

Don't know why, but my client deleted Jerome from To:. Sorry, added him
back.

On Mon, Mar 27, 2023 at 02:03:25PM +0200, [e-mail] wrote:
> On 27/03/2023 13:39, Jerome Brunet wrote:
> > 
> > On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:
> > 
> > > On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
> > > > 
> > > > On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
> > > > 
> > > > > Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
> > > > > clock drivers.
> > > > > Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
> > > > > bindings and include them to MAINTAINERS.
> > > > > 
> > > > > Signed-off-by: Jian Hu <[e-mail]>
> > > > > Signed-off-by: Dmitry Rokosov <[e-mail]>
> > > > > ---
> > > > >   .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
> > > > >   .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
> > > > >   MAINTAINERS                                   |   1 +
> > > > >   include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
> > > > >   .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
> > > > >   5 files changed, 267 insertions(+)
> > > > >   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> > > > >   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > > > 
> > > > There is two drivers (and 2 independent patches). There should be 2
> > > > bindings patches as well.
> > > > 
> > > 
> > > Before, in previous versions I had two versions, but it wasn't bisectable
> > > approach.
> > 
> > You are confusing bisectable and Rob's robot. Splitting patches is more
> > that likely to help bisect (and patches backport) - not the other way around.
> > 
> > > a1-clkc schema depends on a1-pll-clkc headers and vice versa.
> > > It means dt schemas checkers will show us failure if we split them into two
> > > patchsets.
> > 
> > Only because you are patches are not upstream yet ...
> > 
> > > I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
> > > look like production schema and it requires one more patchset above the
> > > series with proper CLKID definitons usage and proper header including.
> > > 
> > > BTW, there is an example of Rob's test bot failure found in the previous
> > > v10 patch series due to chicken or the egg problem.
> > > https://lore.kernel.org/all/[e-mail]/
> > > 
> > > Please advise what's the best practice to resolve that..
> > 
> > Don't use the header in your example would solve the problem and
> > still be correct DT wise.
> > 
> > The examples are just examples, they are not required to actually
> > matches a real HW, as far as I know.
> 
> Exact, you can use fake lables instead of defined:
> 
> <&clkc_pll CLKID_FCLK_DIV2>,
> 
> =>
> remove "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
> 
> <&clkc_pll_fclk_div2>,
> 
> is perfectly ok and will permit have 2 separate patches.
> 
> The dependency is only if you have a common yaml file for
> both bindings files, but this is not the case here.

Simple removal of "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
header doesn't work, dt_binding_check make rule is failed:

Error: Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts:28.37-38 syntax error
FATAL ERROR: Unable to parse input tree

It happens, because 'dt_binding_check' generates simple dts example and
tries to compile it:

cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
===

/dts-v1/;
/plugin/; // silence any missing phandle references


/{
    compatible = "foo";
    model = "foo";
    #address-cells = <1>;
    #size-cells = <1>;



    example-0 {
        #address-cells = <1>;
        #size-cells = <1>;

        

        apb {
            #address-cells = <2>;
            #size-cells = <2>;
        
            clock-controller@800 {
                compatible = "amlogic,a1-clkc";
                reg = <0 0x800 0 0x104>;
                #clock-cells = <1>;
                clocks = <&clkc_pll CLKID_FCLK_DIV2>,
                         <&clkc_pll CLKID_FCLK_DIV3>,
                         <&clkc_pll CLKID_FCLK_DIV5>,
                         <&clkc_pll CLKID_FCLK_DIV7>,
                         <&clkc_pll CLKID_HIFI_PLL>,
                         <&xtal>;
                clock-names = "fclk_div2", "fclk_div3",
                              "fclk_div5", "fclk_div7",
                              "hifi_pll", "xtal";
            };
        };

    };
};
===

As you can see, header is required.

But looks like, dt binding checker is happy with the fake references hack :)
Below there is generated example dts:

cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
===

/dts-v1/;
/plugin/; // silence any missing phandle references


/{
    compatible = "foo";
    model = "foo";
    #address-cells = <1>;
    #size-cells = <1>;



    example-0 {
        #address-cells = <1>;
        #size-cells = <1>;

        

        apb {
            #address-cells = <2>;
            #size-cells = <2>;
        
            clock-controller@800 {
                compatible = "amlogic,a1-clkc";
                reg = <0 0x800 0 0x104>;
                #clock-cells = <1>;
                clocks = <&clkc_pll_fclk_div2>,
                         <&clkc_pll_fclk_div3>,
                         <&clkc_pll_fclk_div5>,
                         <&clkc_pll_fclk_div7>,
                         <&clkc_pll_hifi_pll>,
                         <&xtal>;
                clock-names = "fclk_div2", "fclk_div3",
                              "fclk_div5", "fclk_div7",
                              "hifi_pll", "xtal";
            };
        };

    };
};
===

Yep, we are able to cheat dt checkers, but we don't help dt developers
with such example.
May be, it's better to prepare two patches in such hierarchy:

1) A1 PLL clkc bindings with fake references without clkc headers
2) A1 clkc bindings with real CLKID bindings + A1 PLL clkc bindings fix
with real CLKID A1 clkc bindings + header.

The such approach resolves DT checkers failures and split DT bindings
into two patchsets.

[...]

Krzysztof Kozlowski March 27, 2023, 1:41 p.m. UTC | #7

On 27/03/2023 13:39, Jerome Brunet wrote:
> 
> On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:
> 
>> On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
>>>
>>> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
>>>
>>>> Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
>>>> clock drivers.
>>>> Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
>>>> bindings and include them to MAINTAINERS.
>>>>
>>>> Signed-off-by: Jian Hu <[e-mail]>
>>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
>>>> ---
>>>>  .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
>>>>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
>>>>  MAINTAINERS                                   |   1 +
>>>>  include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
>>>>  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
>>>>  5 files changed, 267 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>
>>> There is two drivers (and 2 independent patches). There should be 2
>>> bindings patches as well.
>>>
>>
>> Before, in previous versions I had two versions, but it wasn't bisectable
>> approach.
> 
> You are confusing bisectable and Rob's robot. Splitting patches is more
> that likely to help bisect (and patches backport) - not the other way around.

No, he did not confuse. Splitting patches makes the series
non-bisectable which was visible in the past.

What's more, there is no reason to have bindings patches split just
because you split drivers. Bindings are independent of drivers - we
write them for hardware description.

> 
>> a1-clkc schema depends on a1-pll-clkc headers and vice versa.
>> It means dt schemas checkers will show us failure if we split them into two
>> patchsets.
> 
> Only because you are patches are not upstream yet ...
> 
>> I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
>> look like production schema and it requires one more patchset above the
>> series with proper CLKID definitons usage and proper header including.
>>
>> BTW, there is an example of Rob's test bot failure found in the previous
>> v10 patch series due to chicken or the egg problem.
>> https://lore.kernel.org/all/[e-mail]/
>>
>> Please advise what's the best practice to resolve that..
> 
> Don't use the header in your example would solve the problem and
> still be correct DT wise.
> 
> The examples are just examples, they are not required to actually
> matches a real HW, as far as I know.

Yes, that would work... or just keep them here.


Best regards,
Krzysztof

Dmitry Rokosov March 27, 2023, 1:46 p.m. UTC | #8

On Mon, Mar 27, 2023 at 03:41:27PM +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 13:39, Jerome Brunet wrote:
> > 
> > On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:
> > 
> >> On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
> >>>
> >>> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
> >>>
> >>>> Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
> >>>> clock drivers.
> >>>> Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
> >>>> bindings and include them to MAINTAINERS.
> >>>>
> >>>> Signed-off-by: Jian Hu <[e-mail]>
> >>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
> >>>> ---
> >>>>  .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
> >>>>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
> >>>>  MAINTAINERS                                   |   1 +
> >>>>  include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
> >>>>  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
> >>>>  5 files changed, 267 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> >>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> >>>
> >>> There is two drivers (and 2 independent patches). There should be 2
> >>> bindings patches as well.
> >>>
> >>
> >> Before, in previous versions I had two versions, but it wasn't bisectable
> >> approach.
> > 
> > You are confusing bisectable and Rob's robot. Splitting patches is more
> > that likely to help bisect (and patches backport) - not the other way around.
> 
> No, he did not confuse. Splitting patches makes the series
> non-bisectable which was visible in the past.
> 
> What's more, there is no reason to have bindings patches split just
> because you split drivers. Bindings are independent of drivers - we
> write them for hardware description.
> 
> > 
> >> a1-clkc schema depends on a1-pll-clkc headers and vice versa.
> >> It means dt schemas checkers will show us failure if we split them into two
> >> patchsets.
> > 
> > Only because you are patches are not upstream yet ...
> > 
> >> I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
> >> look like production schema and it requires one more patchset above the
> >> series with proper CLKID definitons usage and proper header including.
> >>
> >> BTW, there is an example of Rob's test bot failure found in the previous
> >> v10 patch series due to chicken or the egg problem.
> >> https://lore.kernel.org/all/[e-mail]/
> >>
> >> Please advise what's the best practice to resolve that..
> > 
> > Don't use the header in your example would solve the problem and
> > still be correct DT wise.
> > 
> > The examples are just examples, they are not required to actually
> > matches a real HW, as far as I know.
> 
> Yes, that would work... or just keep them here.

I've mentioned it in another reply, by anyway..

Yep, we are able to cheat dt checkers, but we don't help dt developers
with such example. From my point of view, it's more clear for DT
developer to see direct CLKID points instead of "fake references".

May be, it's better to prepare two patches in such hierarchy:

1) A1 PLL clkc bindings with fake references without clkc headers
2) A1 clkc bindings with real CLKID bindings + A1 PLL clkc bindings fix
with real CLKID A1 clkc bindings + header.

The such approach resolves DT checkers failures and split DT bindings
into two patchsets. Also bisectability isn't broken.

Neil Armstrong March 27, 2023, 1:55 p.m. UTC | #9

On 27/03/2023 15:19, Dmitry Rokosov wrote:
> On Mon, Mar 27, 2023 at 02:03:25PM +0200, [e-mail] wrote:
>> On 27/03/2023 13:39, Jerome Brunet wrote:
>>>
>>> On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:
>>>
>>>> On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
>>>>>
>>>>> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
>>>>>
>>>>>> Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
>>>>>> clock drivers.
>>>>>> Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
>>>>>> bindings and include them to MAINTAINERS.
>>>>>>
>>>>>> Signed-off-by: Jian Hu <[e-mail]>
>>>>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
>>>>>> ---
>>>>>>    .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
>>>>>>    .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
>>>>>>    MAINTAINERS                                   |   1 +
>>>>>>    include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
>>>>>>    .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
>>>>>>    5 files changed, 267 insertions(+)
>>>>>>    create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>>>>>>    create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>>>
>>>>> There is two drivers (and 2 independent patches). There should be 2
>>>>> bindings patches as well.
>>>>>
>>>>
>>>> Before, in previous versions I had two versions, but it wasn't bisectable
>>>> approach.
>>>
>>> You are confusing bisectable and Rob's robot. Splitting patches is more
>>> that likely to help bisect (and patches backport) - not the other way around.
>>>
>>>> a1-clkc schema depends on a1-pll-clkc headers and vice versa.
>>>> It means dt schemas checkers will show us failure if we split them into two
>>>> patchsets.
>>>
>>> Only because you are patches are not upstream yet ...
>>>
>>>> I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
>>>> look like production schema and it requires one more patchset above the
>>>> series with proper CLKID definitons usage and proper header including.
>>>>
>>>> BTW, there is an example of Rob's test bot failure found in the previous
>>>> v10 patch series due to chicken or the egg problem.
>>>> https://lore.kernel.org/all/[e-mail]/
>>>>
>>>> Please advise what's the best practice to resolve that..
>>>
>>> Don't use the header in your example would solve the problem and
>>> still be correct DT wise.
>>>
>>> The examples are just examples, they are not required to actually
>>> matches a real HW, as far as I know.
>>
>> Exact, you can use fake lables instead of defined:
>>
>> <&clkc_pll CLKID_FCLK_DIV2>,
>>
>> =>
>> remove "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
>>
>> <&clkc_pll_fclk_div2>,
>>
>> is perfectly ok and will permit have 2 separate patches.
>>
>> The dependency is only if you have a common yaml file for
>> both bindings files, but this is not the case here.
> 
> Simple removal of "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
> header doesn't work, dt_binding_check make rule is failed:

I never wrote you to only remove the include, adding fake labels phandles was the logical next step.

Neil

> 
> Error: Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts:28.37-38 syntax error
> FATAL ERROR: Unable to parse input tree
> 
> It happens, because 'dt_binding_check' generates simple dts example and
> tries to compile it:
> 
> cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
> ===
> 
> /dts-v1/;
> /plugin/; // silence any missing phandle references
> 
> 
> /{
>      compatible = "foo";
>      model = "foo";
>      #address-cells = <1>;
>      #size-cells = <1>;
> 
> 
> 
>      example-0 {
>          #address-cells = <1>;
>          #size-cells = <1>;
> 
>          
> 
>          apb {
>              #address-cells = <2>;
>              #size-cells = <2>;
>          
>              clock-controller@800 {
>                  compatible = "amlogic,a1-clkc";
>                  reg = <0 0x800 0 0x104>;
>                  #clock-cells = <1>;
>                  clocks = <&clkc_pll CLKID_FCLK_DIV2>,
>                           <&clkc_pll CLKID_FCLK_DIV3>,
>                           <&clkc_pll CLKID_FCLK_DIV5>,
>                           <&clkc_pll CLKID_FCLK_DIV7>,
>                           <&clkc_pll CLKID_HIFI_PLL>,
>                           <&xtal>;
>                  clock-names = "fclk_div2", "fclk_div3",
>                                "fclk_div5", "fclk_div7",
>                                "hifi_pll", "xtal";
>              };
>          };
> 
>      };
> };
> ===
> 
> As you can see, header is required.
> 
> But looks like, dt binding checker is happy with the fake references hack :)
> Below there is generated example dts:
> 
> cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
> ===
> 
> /dts-v1/;
> /plugin/; // silence any missing phandle references
> 
> 
> /{
>      compatible = "foo";
>      model = "foo";
>      #address-cells = <1>;
>      #size-cells = <1>;
> 
> 
> 
>      example-0 {
>          #address-cells = <1>;
>          #size-cells = <1>;
> 
>          
> 
>          apb {
>              #address-cells = <2>;
>              #size-cells = <2>;
>          
>              clock-controller@800 {
>                  compatible = "amlogic,a1-clkc";
>                  reg = <0 0x800 0 0x104>;
>                  #clock-cells = <1>;
>                  clocks = <&clkc_pll_fclk_div2>,
>                           <&clkc_pll_fclk_div3>,
>                           <&clkc_pll_fclk_div5>,
>                           <&clkc_pll_fclk_div7>,
>                           <&clkc_pll_hifi_pll>,
>                           <&xtal>;
>                  clock-names = "fclk_div2", "fclk_div3",
>                                "fclk_div5", "fclk_div7",
>                                "hifi_pll", "xtal";
>              };
>          };
> 
>      };
> };
> ===
> 
> Yep, we are able to cheat dt checkers, but we don't help dt developers
> with such example.
> May be, it's better to prepare two patches in such hierarchy:
> 
> 1) A1 PLL clkc bindings with fake references without clkc headers
> 2) A1 clkc bindings with real CLKID bindings + A1 PLL clkc bindings fix
> with real CLKID A1 clkc bindings + header.
> 
> The such approach resolves DT checkers failures and split DT bindings
> into two patchsets.
> 
> [...]
>

Jerome Brunet March 27, 2023, 1:59 p.m. UTC | #10

On Mon 27 Mar 2023 at 15:41, Krzysztof Kozlowski <[e-mail]> wrote:

> On 27/03/2023 13:39, Jerome Brunet wrote:
>> 
>> On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:
>> 
>>> On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
>>>>
>>>> On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
>>>>
>>>>> Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
>>>>> clock drivers.
>>>>> Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
>>>>> bindings and include them to MAINTAINERS.
>>>>>
>>>>> Signed-off-by: Jian Hu <[e-mail]>
>>>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
>>>>> ---
>>>>>  .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
>>>>>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
>>>>>  MAINTAINERS                                   |   1 +
>>>>>  include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
>>>>>  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
>>>>>  5 files changed, 267 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>>
>>>> There is two drivers (and 2 independent patches). There should be 2
>>>> bindings patches as well.
>>>>
>>>
>>> Before, in previous versions I had two versions, but it wasn't bisectable
>>> approach.
>> 
>> You are confusing bisectable and Rob's robot. Splitting patches is more
>> that likely to help bisect (and patches backport) - not the other way around.
>
> No, he did not confuse. Splitting patches makes the series
> non-bisectable which was visible in the past.
>
> What's more, there is no reason to have bindings patches split just
> because you split drivers. Bindings are independent of drivers - we
> write them for hardware description.

Patches should do one thing, my comment is a simple application of that.

There no reason to have a single patch provide the bindings for 2
independent pieces of HW, which those components are. If a dependency
has been set, it is one that should not be there.

They do provide inputs to one another, yes but remain independent pieces of
HW. They have a different address space and as a consequences, different
drivers

If we were being strict, it should even be seperate series.

>
>> 
>>> a1-clkc schema depends on a1-pll-clkc headers and vice versa.
>>> It means dt schemas checkers will show us failure if we split them into two
>>> patchsets.
>> 
>> Only because you are patches are not upstream yet ...
>> 
>>> I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
>>> look like production schema and it requires one more patchset above the
>>> series with proper CLKID definitons usage and proper header including.
>>>
>>> BTW, there is an example of Rob's test bot failure found in the previous
>>> v10 patch series due to chicken or the egg problem.
>>> https://lore.kernel.org/all/[e-mail]/
>>>
>>> Please advise what's the best practice to resolve that..
>> 
>> Don't use the header in your example would solve the problem and
>> still be correct DT wise.
>> 
>> The examples are just examples, they are not required to actually
>> matches a real HW, as far as I know.
>
> Yes, that would work... or just keep them here.
>
>
> Best regards,
> Krzysztof
>
>
> _______________________________________________
> linux-amlogic mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Dmitry Rokosov March 27, 2023, 2:02 p.m. UTC | #11

On Mon, Mar 27, 2023 at 03:55:46PM +0200, [e-mail] wrote:
> On 27/03/2023 15:19, Dmitry Rokosov wrote:
> > On Mon, Mar 27, 2023 at 02:03:25PM +0200, [e-mail] wrote:
> > > On 27/03/2023 13:39, Jerome Brunet wrote:
> > > > 
> > > > On Mon 27 Mar 2023 at 13:51, Dmitry Rokosov <[e-mail]> wrote:
> > > > 
> > > > > On Mon, Mar 27, 2023 at 11:51:21AM +0200, Jerome Brunet wrote:
> > > > > > 
> > > > > > On Tue 21 Mar 2023 at 22:30, Dmitry Rokosov <[e-mail]> wrote:
> > > > > > 
> > > > > > > Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
> > > > > > > clock drivers.
> > > > > > > Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
> > > > > > > bindings and include them to MAINTAINERS.
> > > > > > > 
> > > > > > > Signed-off-by: Jian Hu <[e-mail]>
> > > > > > > Signed-off-by: Dmitry Rokosov <[e-mail]>
> > > > > > > ---
> > > > > > >    .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
> > > > > > >    .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
> > > > > > >    MAINTAINERS                                   |   1 +
> > > > > > >    include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
> > > > > > >    .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
> > > > > > >    5 files changed, 267 insertions(+)
> > > > > > >    create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
> > > > > > >    create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > > > > > 
> > > > > > There is two drivers (and 2 independent patches). There should be 2
> > > > > > bindings patches as well.
> > > > > > 
> > > > > 
> > > > > Before, in previous versions I had two versions, but it wasn't bisectable
> > > > > approach.
> > > > 
> > > > You are confusing bisectable and Rob's robot. Splitting patches is more
> > > > that likely to help bisect (and patches backport) - not the other way around.
> > > > 
> > > > > a1-clkc schema depends on a1-pll-clkc headers and vice versa.
> > > > > It means dt schemas checkers will show us failure if we split them into two
> > > > > patchsets.
> > > > 
> > > > Only because you are patches are not upstream yet ...
> > > > 
> > > > > I know, that we can use raw digits instead of CLKID names, but IMO it doesn't
> > > > > look like production schema and it requires one more patchset above the
> > > > > series with proper CLKID definitons usage and proper header including.
> > > > > 
> > > > > BTW, there is an example of Rob's test bot failure found in the previous
> > > > > v10 patch series due to chicken or the egg problem.
> > > > > https://lore.kernel.org/all/[e-mail]/
> > > > > 
> > > > > Please advise what's the best practice to resolve that..
> > > > 
> > > > Don't use the header in your example would solve the problem and
> > > > still be correct DT wise.
> > > > 
> > > > The examples are just examples, they are not required to actually
> > > > matches a real HW, as far as I know.
> > > 
> > > Exact, you can use fake lables instead of defined:
> > > 
> > > <&clkc_pll CLKID_FCLK_DIV2>,
> > > 
> > > =>
> > > remove "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
> > > 
> > > <&clkc_pll_fclk_div2>,
> > > 
> > > is perfectly ok and will permit have 2 separate patches.
> > > 
> > > The dependency is only if you have a common yaml file for
> > > both bindings files, but this is not the case here.
> > 
> > Simple removal of "#include <dt-bindings/clock/amlogic,a1-pll-clkc.h>"
> > header doesn't work, dt_binding_check make rule is failed:
> 
> I never wrote you to only remove the include, adding fake labels phandles was the logical next step.

Ah, sorry, I've confused. Anyway, the fake labels are working perfectly.
DT checkers are silent.

> > 
> > Error: Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts:28.37-38 syntax error
> > FATAL ERROR: Unable to parse input tree
> > 
> > It happens, because 'dt_binding_check' generates simple dts example and
> > tries to compile it:
> > 
> > cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
> > ===
> > 
> > /dts-v1/;
> > /plugin/; // silence any missing phandle references
> > 
> > 
> > /{
> >      compatible = "foo";
> >      model = "foo";
> >      #address-cells = <1>;
> >      #size-cells = <1>;
> > 
> > 
> > 
> >      example-0 {
> >          #address-cells = <1>;
> >          #size-cells = <1>;
> > 
> > 
> >          apb {
> >              #address-cells = <2>;
> >              #size-cells = <2>;
> >              clock-controller@800 {
> >                  compatible = "amlogic,a1-clkc";
> >                  reg = <0 0x800 0 0x104>;
> >                  #clock-cells = <1>;
> >                  clocks = <&clkc_pll CLKID_FCLK_DIV2>,
> >                           <&clkc_pll CLKID_FCLK_DIV3>,
> >                           <&clkc_pll CLKID_FCLK_DIV5>,
> >                           <&clkc_pll CLKID_FCLK_DIV7>,
> >                           <&clkc_pll CLKID_HIFI_PLL>,
> >                           <&xtal>;
> >                  clock-names = "fclk_div2", "fclk_div3",
> >                                "fclk_div5", "fclk_div7",
> >                                "hifi_pll", "xtal";
> >              };
> >          };
> > 
> >      };
> > };
> > ===
> > 
> > As you can see, header is required.
> > 
> > But looks like, dt binding checker is happy with the fake references hack :)
> > Below there is generated example dts:
> > 
> > cat Documentation/devicetree/bindings/clock/amlogic,a1-clkc.example.dts
> > ===
> > 
> > /dts-v1/;
> > /plugin/; // silence any missing phandle references
> > 
> > 
> > /{
> >      compatible = "foo";
> >      model = "foo";
> >      #address-cells = <1>;
> >      #size-cells = <1>;
> > 
> > 
> > 
> >      example-0 {
> >          #address-cells = <1>;
> >          #size-cells = <1>;
> > 
> > 
> >          apb {
> >              #address-cells = <2>;
> >              #size-cells = <2>;
> >              clock-controller@800 {
> >                  compatible = "amlogic,a1-clkc";
> >                  reg = <0 0x800 0 0x104>;
> >                  #clock-cells = <1>;
> >                  clocks = <&clkc_pll_fclk_div2>,
> >                           <&clkc_pll_fclk_div3>,
> >                           <&clkc_pll_fclk_div5>,
> >                           <&clkc_pll_fclk_div7>,
> >                           <&clkc_pll_hifi_pll>,
> >                           <&xtal>;
> >                  clock-names = "fclk_div2", "fclk_div3",
> >                                "fclk_div5", "fclk_div7",
> >                                "hifi_pll", "xtal";
> >              };
> >          };
> > 
> >      };
> > };
> > ===
> > 
> > Yep, we are able to cheat dt checkers, but we don't help dt developers
> > with such example.
> > May be, it's better to prepare two patches in such hierarchy:
> > 
> > 1) A1 PLL clkc bindings with fake references without clkc headers
> > 2) A1 clkc bindings with real CLKID bindings + A1 PLL clkc bindings fix
> > with real CLKID A1 clkc bindings + header.
> > 
> > The such approach resolves DT checkers failures and split DT bindings
> > into two patchsets.

What do you think about patchsets form listed above? Fake labels as a
first step and real CLKIDs as a second. I'm embarrassed only that second
patch fixes the previous in the one patch series...

Rob Herring March 27, 2023, 3:28 p.m. UTC | #12

On Tue, 21 Mar 2023 22:30:12 +0300, Dmitry Rokosov wrote:
> Add the documentation for Amlogic A1 PLL and Amlogic A1 Peripherals
> clock drivers.
> Introduce Amlogic A1 PLL and Amlogic A1 Peripherals device tree
> bindings and include them to MAINTAINERS.
> 
> Signed-off-by: Jian Hu <[e-mail]>
> Signed-off-by: Dmitry Rokosov <[e-mail]>
> ---
>  .../bindings/clock/amlogic,a1-clkc.yaml       |  73 +++++++++++
>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |  59 +++++++++
>  MAINTAINERS                                   |   1 +
>  include/dt-bindings/clock/amlogic,a1-clkc.h   | 113 ++++++++++++++++++
>  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |  21 ++++
>  5 files changed, 267 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>  create mode 100644 include/dt-bindings/clock/amlogic,a1-clkc.h
>  create mode 100644 include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> 

Reviewed-by: Rob Herring <[e-mail]>

Patch

diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
new file mode 100644
index 000000000000..cb6d8f4eb959
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
@@ -0,0 +1,73 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,a1-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Meson A/C serials Peripheral Clock Control Unit
+
+maintainers:
+  - Neil Armstrong <[e-mail]>
+  - Jerome Brunet <[e-mail]>
+  - Jian Hu <[e-mail]>
+  - Dmitry Rokosov <[e-mail]>
+
+properties:
+  compatible:
+    const: amlogic,a1-clkc
+
+  '#clock-cells':
+    const: 1
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: input fixed pll div2
+      - description: input fixed pll div3
+      - description: input fixed pll div5
+      - description: input fixed pll div7
+      - description: input hifi pll
+      - description: input oscillator (usually at 24MHz)
+
+  clock-names:
+    items:
+      - const: fclk_div2
+      - const: fclk_div3
+      - const: fclk_div5
+      - const: fclk_div7
+      - const: hifi_pll
+      - const: xtal
+
+required:
+  - compatible
+  - '#clock-cells'
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/amlogic,a1-pll-clkc.h>
+    apb {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@800 {
+            compatible = "amlogic,a1-clkc";
+            reg = <0 0x800 0 0x104>;
+            #clock-cells = <1>;
+            clocks = <&clkc_pll CLKID_FCLK_DIV2>,
+                     <&clkc_pll CLKID_FCLK_DIV3>,
+                     <&clkc_pll CLKID_FCLK_DIV5>,
+                     <&clkc_pll CLKID_FCLK_DIV7>,
+                     <&clkc_pll CLKID_HIFI_PLL>,
+                     <&xtal>;
+            clock-names = "fclk_div2", "fclk_div3",
+                          "fclk_div5", "fclk_div7",
+                          "hifi_pll", "xtal";
+        };
+    };
diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
new file mode 100644
index 000000000000..77a13b1f9d5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
@@ -0,0 +1,59 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Meson A/C serials PLL Clock Control Unit
+
+maintainers:
+  - Neil Armstrong <[e-mail]>
+  - Jerome Brunet <[e-mail]>
+  - Jian Hu <[e-mail]>
+  - Dmitry Rokosov <[e-mail]>
+
+properties:
+  compatible:
+    const: amlogic,a1-pll-clkc
+
+  '#clock-cells':
+    const: 1
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: input fixpll_in
+      - description: input hifipll_in
+
+  clock-names:
+    items:
+      - const: fixpll_in
+      - const: hifipll_in
+
+required:
+  - compatible
+  - '#clock-cells'
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/amlogic,a1-clkc.h>
+    apb {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        clock-controller@7c80 {
+            compatible = "amlogic,a1-pll-clkc";
+            reg = <0 0x7c80 0 0x18c>;
+            #clock-cells = <1>;
+            clocks = <&clkc_periphs CLKID_FIXPLL_IN>,
+                     <&clkc_periphs CLKID_HIFIPLL_IN>;
+            clock-names = "fixpll_in", "hifipll_in";
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 39ff1a717625..8438bc9bd636 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1895,6 +1895,7 @@  L:	[e-mail]
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/amlogic*
 F:	drivers/clk/meson/
+F:	include/dt-bindings/clock/a1*
 F:	include/dt-bindings/clock/gxbb*
 F:	include/dt-bindings/clock/meson*
 
diff --git a/include/dt-bindings/clock/amlogic,a1-clkc.h b/include/dt-bindings/clock/amlogic,a1-clkc.h
new file mode 100644
index 000000000000..95131779982c
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,a1-clkc.h
@@ -0,0 +1,113 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ * Author: Jian Hu <[e-mail]>
+ *
+ * Copyright (c) 2023, SberDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <[e-mail]>
+ */
+
+#ifndef __A1_CLKC_H
+#define __A1_CLKC_H
+
+#define CLKID_FIXPLL_IN		0
+#define CLKID_USB_PHY_IN	1
+#define CLKID_USB_CTRL_IN	2
+#define CLKID_HIFIPLL_IN	3
+#define CLKID_SYSPLL_IN		4
+#define CLKID_DDS_IN		5
+#define CLKID_SYS		6
+#define CLKID_CLKTREE		7
+#define CLKID_RESET_CTRL	8
+#define CLKID_ANALOG_CTRL	9
+#define CLKID_PWR_CTRL		10
+#define CLKID_PAD_CTRL		11
+#define CLKID_SYS_CTRL		12
+#define CLKID_TEMP_SENSOR	13
+#define CLKID_AM2AXI_DIV	14
+#define CLKID_SPICC_B		15
+#define CLKID_SPICC_A		16
+#define CLKID_MSR		17
+#define CLKID_AUDIO		18
+#define CLKID_JTAG_CTRL		19
+#define CLKID_SARADC_EN		20
+#define CLKID_PWM_EF		21
+#define CLKID_PWM_CD		22
+#define CLKID_PWM_AB		23
+#define CLKID_CEC		24
+#define CLKID_I2C_S		25
+#define CLKID_IR_CTRL		26
+#define CLKID_I2C_M_D		27
+#define CLKID_I2C_M_C		28
+#define CLKID_I2C_M_B		29
+#define CLKID_I2C_M_A		30
+#define CLKID_ACODEC		31
+#define CLKID_OTP		32
+#define CLKID_SD_EMMC_A		33
+#define CLKID_USB_PHY		34
+#define CLKID_USB_CTRL		35
+#define CLKID_SYS_DSPB		36
+#define CLKID_SYS_DSPA		37
+#define CLKID_DMA		38
+#define CLKID_IRQ_CTRL		39
+#define CLKID_NIC		40
+#define CLKID_GIC		41
+#define CLKID_UART_C		42
+#define CLKID_UART_B		43
+#define CLKID_UART_A		44
+#define CLKID_SYS_PSRAM		45
+#define CLKID_RSA		46
+#define CLKID_CORESIGHT		47
+#define CLKID_AM2AXI_VAD	48
+#define CLKID_AUDIO_VAD		49
+#define CLKID_AXI_DMC		50
+#define CLKID_AXI_PSRAM		51
+#define CLKID_RAMB		52
+#define CLKID_RAMA		53
+#define CLKID_AXI_SPIFC		54
+#define CLKID_AXI_NIC		55
+#define CLKID_AXI_DMA		56
+#define CLKID_CPU_CTRL		57
+#define CLKID_ROM		58
+#define CLKID_PROC_I2C		59
+#define CLKID_DSPA_EN		60
+#define CLKID_DSPA_EN_NIC	61
+#define CLKID_DSPB_EN		62
+#define CLKID_DSPB_EN_NIC	63
+#define CLKID_RTC		64
+#define CLKID_CECA_32K		65
+#define CLKID_CECB_32K		66
+#define CLKID_24M		67
+#define CLKID_12M		68
+#define CLKID_FCLK_DIV2_DIVN	69
+#define CLKID_GEN		70
+#define CLKID_SARADC		71
+#define CLKID_PWM_A		72
+#define CLKID_PWM_B		73
+#define CLKID_PWM_C		74
+#define CLKID_PWM_D		75
+#define CLKID_PWM_E		76
+#define CLKID_PWM_F		77
+#define CLKID_SPICC		78
+#define CLKID_TS		79
+#define CLKID_SPIFC		80
+#define CLKID_USB_BUS		81
+#define CLKID_SD_EMMC		82
+#define CLKID_PSRAM		83
+#define CLKID_DMC		84
+#define CLKID_GEN_SEL		85
+#define CLKID_PWM_A_SEL		86
+#define CLKID_PWM_B_SEL		87
+#define CLKID_PWM_C_SEL		88
+#define CLKID_PWM_D_SEL		89
+#define CLKID_PWM_E_SEL		90
+#define CLKID_PWM_F_SEL		91
+#define CLKID_DSPA_A_SEL	92
+#define CLKID_DSPA_B_SEL	93
+#define CLKID_DSPB_A_SEL	94
+#define CLKID_DSPB_B_SEL	95
+#define CLKID_CECA_32K_SEL	96
+#define CLKID_CECB_32K_SEL	97
+#define NR_CLKS			98
+
+#endif /* __A1_CLKC_H */
diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
new file mode 100644
index 000000000000..740fe8c4db5d
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
@@ -0,0 +1,21 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ * Author: Jian Hu <[e-mail]>
+ *
+ * Copyright (c) 2023, SberDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <[e-mail]>
+ */
+
+#ifndef __A1_PLL_CLKC_H
+#define __A1_PLL_CLKC_H
+
+#define CLKID_FIXED_PLL		0
+#define CLKID_FCLK_DIV2		1
+#define CLKID_FCLK_DIV3		2
+#define CLKID_FCLK_DIV5		3
+#define CLKID_FCLK_DIV7		4
+#define CLKID_HIFI_PLL		5
+#define NR_PLL_CLKS		6
+
+#endif /* __A1_PLL_CLKC_H */