[v3,07/11] dt-bindings: clock: Add StarFive JH7110 system clock and reset generator

Commit Message

Hal Feng Dec. 20, 2022, 12:50 a.m. UTC

From: Emil Renner Berthing <[e-mail]>

Add bindings for the system clock and reset generator (SYSCRG) on the
JH7110 RISC-V SoC by StarFive Ltd.

Signed-off-by: Emil Renner Berthing <[e-mail]>
Signed-off-by: Hal Feng <[e-mail]>
---
 .../clock/starfive,jh7110-syscrg.yaml         |  80 +++++++
 MAINTAINERS                                   |   8 +-
 .../dt-bindings/clock/starfive,jh7110-crg.h   | 207 ++++++++++++++++++
 .../dt-bindings/reset/starfive,jh7110-crg.h   | 142 ++++++++++++
 4 files changed, 434 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
 create mode 100644 include/dt-bindings/clock/starfive,jh7110-crg.h
 create mode 100644 include/dt-bindings/reset/starfive,jh7110-crg.h

Comments

Rob Herring (Arm) Dec. 20, 2022, 8:12 p.m. UTC | #1

On Tue, 20 Dec 2022 08:50:50 +0800, Hal Feng wrote:
> From: Emil Renner Berthing <[e-mail]>
> 
> Add bindings for the system clock and reset generator (SYSCRG) on the
> JH7110 RISC-V SoC by StarFive Ltd.
> 
> Signed-off-by: Emil Renner Berthing <[e-mail]>
> Signed-off-by: Hal Feng <[e-mail]>
> ---
>  .../clock/starfive,jh7110-syscrg.yaml         |  80 +++++++
>  MAINTAINERS                                   |   8 +-
>  .../dt-bindings/clock/starfive,jh7110-crg.h   | 207 ++++++++++++++++++
>  .../dt-bindings/reset/starfive,jh7110-crg.h   | 142 ++++++++++++
>  4 files changed, 434 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
>  create mode 100644 include/dt-bindings/clock/starfive,jh7110-crg.h
>  create mode 100644 include/dt-bindings/reset/starfive,jh7110-crg.h
> 

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

Conor Dooley Dec. 20, 2022, 11:14 p.m. UTC | #2

On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> From: Emil Renner Berthing <[e-mail]>
> 
> Add bindings for the system clock and reset generator (SYSCRG) on the
> JH7110 RISC-V SoC by StarFive Ltd.
> 
> Signed-off-by: Emil Renner Berthing <[e-mail]>
> Signed-off-by: Hal Feng <[e-mail]>
> ---
>  .../clock/starfive,jh7110-syscrg.yaml         |  80 +++++++
>  MAINTAINERS                                   |   8 +-
>  .../dt-bindings/clock/starfive,jh7110-crg.h   | 207 ++++++++++++++++++
>  .../dt-bindings/reset/starfive,jh7110-crg.h   | 142 ++++++++++++
>  4 files changed, 434 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
>  create mode 100644 include/dt-bindings/clock/starfive,jh7110-crg.h
>  create mode 100644 include/dt-bindings/reset/starfive,jh7110-crg.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> new file mode 100644
> index 000000000000..ec81504dcb27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/starfive,jh7110-syscrg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 System Clock and Reset Generator
> +
> +maintainers:
> +  - Emil Renner Berthing <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-syscrg
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Main Oscillator (24 MHz)
> +      - description: GMAC1 RMII reference
> +      - description: GMAC1 RGMII RX
> +      - description: External I2S TX bit clock
> +      - description: External I2S TX left/right channel clock
> +      - description: External I2S RX bit clock
> +      - description: External I2S RX left/right channel clock
> +      - description: External TDM clock
> +      - description: External audio master clock

So, from peeking at the clock driver & the dt - it looks like a bunch of
these are not actually required?
I'd have ploughed through this, but having read Krzysztof's comments on
the DTS I'm not sure that this binding is correct.
https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc

I *think* the DT is correct - the fixed clocks are all inputs from clock
sources on the board and as such they are empty in soc.dtsi and are
populated in board.dts?

However, are they all actually required? In the driver I see:
	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
		    JH7110_SYSCLK_GMAC1_RMII_RTX),
That macro is:
#define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
	.name = _name,								\
	.flags = 0,								\
	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
	.parents = { __VA_ARGS__ },						\
}

AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
Does that mean you need to populate only one of GMAC1 RMII reference
and GMAC1 RMGII RX and the other is optional?

What have I missed?

> +
> +  clock-names:
> +    items:
> +      - const: osc
> +      - const: gmac1_rmii_refin
> +      - const: gmac1_rgmii_rxin
> +      - const: i2stx_bclk_ext
> +      - const: i2stx_lrck_ext
> +      - const: i2srx_bclk_ext
> +      - const: i2srx_lrck_ext
> +      - const: tdm_ext
> +      - const: mclk_ext

If all clocks are in fact required though, isn't this kinda pointless to
have since we already know that the order is fixed from the "clocks"
property?
Krzk/Rob?

> +
> +  '#clock-cells':
> +    const: 1
> +    description:
> +      See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices.
> +
> +  '#reset-cells':
> +    const: 1
> +    description:
> +      See <dt-bindings/reset/starfive,jh7110-crg.h> for valid indices.
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clock-controller@13020000 {
> +        compatible = "starfive,jh7110-syscrg";
> +        reg = <0x13020000 0x10000>;
> +        clocks = <&osc>, <&gmac1_rmii_refin>,
> +                 <&gmac1_rgmii_rxin>,
> +                 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
> +                 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> +                 <&tdm_ext>, <&mclk_ext>;
> +        clock-names = "osc", "gmac1_rmii_refin",
> +                      "gmac1_rgmii_rxin",
> +                      "i2stx_bclk_ext", "i2stx_lrck_ext",
> +                      "i2srx_bclk_ext", "i2srx_lrck_ext",
> +                      "tdm_ext", "mclk_ext";
> +        #clock-cells = <1>;
> +        #reset-cells = <1>;
> +    };

Also, whatever happened to GMAC0? It has fixed-clocks defined in the DTS
but doesn't appear in the binding or driver?

Thanks,
Conor.

Conor Dooley Dec. 20, 2022, 11:16 p.m. UTC | #3

On Tue, Dec 20, 2022 at 11:14:44PM +0000, Conor Dooley wrote:
> On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> > +    clock-controller@13020000 {
> > +        compatible = "starfive,jh7110-syscrg";
> > +        reg = <0x13020000 0x10000>;
> > +        clocks = <&osc>, <&gmac1_rmii_refin>,
> > +                 <&gmac1_rgmii_rxin>,
> > +                 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
> > +                 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> > +                 <&tdm_ext>, <&mclk_ext>;
> > +        clock-names = "osc", "gmac1_rmii_refin",
> > +                      "gmac1_rgmii_rxin",
> > +                      "i2stx_bclk_ext", "i2stx_lrck_ext",
> > +                      "i2srx_bclk_ext", "i2srx_lrck_ext",
> > +                      "tdm_ext", "mclk_ext";
> > +        #clock-cells = <1>;
> > +        #reset-cells = <1>;
> > +    };
> 
> Also, whatever happened to GMAC0? It has fixed-clocks defined in the DTS
> but doesn't appear in the binding or driver?

Ah, I should have looked at the next patch...

D'oh.

Hal Feng Dec. 25, 2022, 4:26 p.m. UTC | #4

On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
> On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> > From: Emil Renner Berthing <[e-mail]>
> > 
> > Add bindings for the system clock and reset generator (SYSCRG) on the
> > JH7110 RISC-V SoC by StarFive Ltd.
> > 
> > Signed-off-by: Emil Renner Berthing <[e-mail]>
> > Signed-off-by: Hal Feng <[e-mail]>
> > ---
> >  .../clock/starfive,jh7110-syscrg.yaml         |  80 +++++++
> >  MAINTAINERS                                   |   8 +-
> >  .../dt-bindings/clock/starfive,jh7110-crg.h   | 207 ++++++++++++++++++
> >  .../dt-bindings/reset/starfive,jh7110-crg.h   | 142 ++++++++++++
> >  4 files changed, 434 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> >  create mode 100644 include/dt-bindings/clock/starfive,jh7110-crg.h
> >  create mode 100644 include/dt-bindings/reset/starfive,jh7110-crg.h
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> > new file mode 100644
> > index 000000000000..ec81504dcb27
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/starfive,jh7110-syscrg.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: StarFive JH7110 System Clock and Reset Generator
> > +
> > +maintainers:
> > +  - Emil Renner Berthing <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    const: starfive,jh7110-syscrg
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: Main Oscillator (24 MHz)
> > +      - description: GMAC1 RMII reference
> > +      - description: GMAC1 RGMII RX
> > +      - description: External I2S TX bit clock
> > +      - description: External I2S TX left/right channel clock
> > +      - description: External I2S RX bit clock
> > +      - description: External I2S RX left/right channel clock
> > +      - description: External TDM clock
> > +      - description: External audio master clock
> 
> So, from peeking at the clock driver & the dt - it looks like a bunch of
> these are not actually required?

These clocks are used as root clocks or optional parent clocks in clock tree.
Some of them are optional, but they are required if we want to describe the
complete clock tree of JH7110 SoC.

> I'd have ploughed through this, but having read Krzysztof's comments on
> the DTS I'm not sure that this binding is correct.
> https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
> 
> I *think* the DT is correct - the fixed clocks are all inputs from clock
> sources on the board and as such they are empty in soc.dtsi and are
> populated in board.dts?

Yes, the fixed clocks are all clock sources on the board and input to the SoC.

> 
> However, are they all actually required? In the driver I see:
> 	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
> 		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
> 		    JH7110_SYSCLK_GMAC1_RMII_RTX),
> That macro is:
> #define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
> 	.name = _name,								\
> 	.flags = 0,								\
> 	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
> 	.parents = { __VA_ARGS__ },						\
> }
> 
> AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
> Does that mean you need to populate only one of GMAC1 RMII reference
> and GMAC1 RMGII RX and the other is optional?

Yes, actually only one of them is chosen as the root clock
source of the clock "gmac1_rx".

> 
> What have I missed?
> 
> > +
> > +  clock-names:
> > +    items:
> > +      - const: osc
> > +      - const: gmac1_rmii_refin
> > +      - const: gmac1_rgmii_rxin
> > +      - const: i2stx_bclk_ext
> > +      - const: i2stx_lrck_ext
> > +      - const: i2srx_bclk_ext
> > +      - const: i2srx_lrck_ext
> > +      - const: tdm_ext
> > +      - const: mclk_ext
> 
> If all clocks are in fact required though, isn't this kinda pointless to
> have since we already know that the order is fixed from the "clocks"
> property?
> Krzk/Rob?

The clock-names are used to easily identify these clocks in the clock driver.

Best regards,
Hal

Conor Dooley Dec. 27, 2022, 8:15 p.m. UTC | #5

On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
> On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
> > On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> > > From: Emil Renner Berthing <[e-mail]>
> > > 
> > > Add bindings for the system clock and reset generator (SYSCRG) on the
> > > JH7110 RISC-V SoC by StarFive Ltd.
> > > 
> > > Signed-off-by: Emil Renner Berthing <[e-mail]>
> > > Signed-off-by: Hal Feng <[e-mail]>

> > > +  clocks:
> > > +    items:
> > > +      - description: Main Oscillator (24 MHz)
> > > +      - description: GMAC1 RMII reference
> > > +      - description: GMAC1 RGMII RX
> > > +      - description: External I2S TX bit clock
> > > +      - description: External I2S TX left/right channel clock
> > > +      - description: External I2S RX bit clock
> > > +      - description: External I2S RX left/right channel clock
> > > +      - description: External TDM clock
> > > +      - description: External audio master clock
> > 
> > So, from peeking at the clock driver & the dt - it looks like a bunch of
> > these are not actually required?
> 
> These clocks are used as root clocks or optional parent clocks in clock tree.
> Some of them are optional, but they are required if we want to describe the
> complete clock tree of JH7110 SoC.

Perhaps I have a misunderstand of what required means. To me, required
means "you must provide this clock for the SoC to operate in all
configurations".
Optional therefore would be for things that are needed only for some
configurations and may be omitted if not required.

From your comment below, boards with a JH7110 may choose not to populate
both external clock inputs to a mux. In that case, "dummy" clocks should
not have to be provided in the DT of such boards to satisfy this binding
which seems wrong to me..

It would seem to me that you need to set minItems < maxItems here to
account for that & you do in fact need clock-names.

> 
> > I'd have ploughed through this, but having read Krzysztof's comments on
> > the DTS I'm not sure that this binding is correct.
> > https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
> > 
> > I *think* the DT is correct - the fixed clocks are all inputs from clock
> > sources on the board and as such they are empty in soc.dtsi and are
> > populated in board.dts?
> 
> Yes, the fixed clocks are all clock sources on the board and input to the SoC.
> 
> > 
> > However, are they all actually required? In the driver I see:
> > 	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
> > 		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
> > 		    JH7110_SYSCLK_GMAC1_RMII_RTX),
> > That macro is:
> > #define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
> > 	.name = _name,								\
> > 	.flags = 0,								\
> > 	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
> > 	.parents = { __VA_ARGS__ },						\
> > }
> > 
> > AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
> > Does that mean you need to populate only one of GMAC1 RMII reference
> > and GMAC1 RMGII RX and the other is optional?
> 
> Yes, actually only one of them is chosen as the root clock
> source of the clock "gmac1_rx".
> 
> > 
> > What have I missed?
> > 
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: osc
> > > +      - const: gmac1_rmii_refin
> > > +      - const: gmac1_rgmii_rxin
> > > +      - const: i2stx_bclk_ext
> > > +      - const: i2stx_lrck_ext
> > > +      - const: i2srx_bclk_ext
> > > +      - const: i2srx_lrck_ext
> > > +      - const: tdm_ext
> > > +      - const: mclk_ext
> > 
> > If all clocks are in fact required though, isn't this kinda pointless to
> > have since we already know that the order is fixed from the "clocks"
> > property?
> > Krzk/Rob?
> 
> The clock-names are used to easily identify these clocks in the clock driver.

*IF* all clocks were in fact required, which they aren't, you could rely
on the order alone in the driver as it is enforced by the binding.

Thanks,
Conor.

Hal Feng Feb. 16, 2023, 2:42 p.m. UTC | #6

On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
> On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
>> On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
>> > On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
>> > > From: Emil Renner Berthing <[e-mail]>
>> > > 
>> > > Add bindings for the system clock and reset generator (SYSCRG) on the
>> > > JH7110 RISC-V SoC by StarFive Ltd.
>> > > 
>> > > Signed-off-by: Emil Renner Berthing <[e-mail]>
>> > > Signed-off-by: Hal Feng <[e-mail]>
> 
>> > > +  clocks:
>> > > +    items:
>> > > +      - description: Main Oscillator (24 MHz)
>> > > +      - description: GMAC1 RMII reference
>> > > +      - description: GMAC1 RGMII RX
>> > > +      - description: External I2S TX bit clock
>> > > +      - description: External I2S TX left/right channel clock
>> > > +      - description: External I2S RX bit clock
>> > > +      - description: External I2S RX left/right channel clock
>> > > +      - description: External TDM clock
>> > > +      - description: External audio master clock
>> > 
>> > So, from peeking at the clock driver & the dt - it looks like a bunch of
>> > these are not actually required?
>> 
>> These clocks are used as root clocks or optional parent clocks in clock tree.
>> Some of them are optional, but they are required if we want to describe the
>> complete clock tree of JH7110 SoC.
> 
> Perhaps I have a misunderstand of what required means. To me, required
> means "you must provide this clock for the SoC to operate in all
> configurations".
> Optional therefore would be for things that are needed only for some
> configurations and may be omitted if not required.
> 
> From your comment below, boards with a JH7110 may choose not to populate
> both external clock inputs to a mux. In that case, "dummy" clocks should
> not have to be provided in the DT of such boards to satisfy this binding
> which seems wrong to me..

Please see the picture of these external clocks in clock tree.

# mount -t debugfs none /mnt
# cat /mnt/clk/clk_summary
                                 enable  prepare  protect                                duty  hardware
   clock                          count    count    count        rate   accuracy phase  cycle    enable
-------------------------------------------------------------------------------------------------------
 *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
 *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
 *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
 *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
 *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
 *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
 *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
    gmac1_rx                          0        0        0   125000000          0     0  50000         Y
       gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
 *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
    gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
       gmac1_tx                       0        0        0    50000000          0     0  50000         N
          gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
 *osc*                                  4        4        0    24000000          0     0  50000         Y
    apb_func                          0        0        0    24000000          0     0  50000         Y
 ...

The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
actually used as the parent of other clocks. The "dummy" clocks
you said are all internal clocks.

For the audio related clocks (mclk_ext/tdm_ext/i2srx_lrck_ext/
i2srx_bclk_ext/i2stx_lrck_ext/i2stx_bclk_ext), they will be used
as the parent clocks in audio related drivers. Note that some
clocks need to select different clocks as parent according to
requirement.
So all these external clocks are required.

> 
> It would seem to me that you need to set minItems < maxItems here to
> account for that & you do in fact need clock-names.
> 
>> 
>> > I'd have ploughed through this, but having read Krzysztof's comments on
>> > the DTS I'm not sure that this binding is correct.
>> > https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
>> > 
>> > I *think* the DT is correct - the fixed clocks are all inputs from clock
>> > sources on the board and as such they are empty in soc.dtsi and are
>> > populated in board.dts?
>> 
>> Yes, the fixed clocks are all clock sources on the board and input to the SoC.
>> 
>> > 
>> > However, are they all actually required? In the driver I see:
>> > 	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
>> > 		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
>> > 		    JH7110_SYSCLK_GMAC1_RMII_RTX),
>> > That macro is:
>> > #define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
>> > 	.name = _name,								\
>> > 	.flags = 0,								\
>> > 	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
>> > 	.parents = { __VA_ARGS__ },						\
>> > }
>> > 
>> > AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
>> > Does that mean you need to populate only one of GMAC1 RMII reference
>> > and GMAC1 RMGII RX and the other is optional?
>> 
>> Yes, actually only one of them is chosen as the root clock
>> source of the clock "gmac1_rx".
>> 
>> > 
>> > What have I missed?
>> > 
>> > > +
>> > > +  clock-names:
>> > > +    items:
>> > > +      - const: osc
>> > > +      - const: gmac1_rmii_refin
>> > > +      - const: gmac1_rgmii_rxin
>> > > +      - const: i2stx_bclk_ext
>> > > +      - const: i2stx_lrck_ext
>> > > +      - const: i2srx_bclk_ext
>> > > +      - const: i2srx_lrck_ext
>> > > +      - const: tdm_ext
>> > > +      - const: mclk_ext
>> > 
>> > If all clocks are in fact required though, isn't this kinda pointless to
>> > have since we already know that the order is fixed from the "clocks"
>> > property?
>> > Krzk/Rob?
>> 
>> The clock-names are used to easily identify these clocks in the clock driver.
> 
> *IF* all clocks were in fact required, which they aren't, you could rely
> on the order alone in the driver as it is enforced by the binding.

OK, I'll remove "clock-names" property in the bindings and device tree.
Instead, will use index to get these clocks in drivers.

Best regards,
Hal

Conor Dooley Feb. 16, 2023, 6:20 p.m. UTC | #7

Hey Hal!

On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
> On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
> > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
> >> On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
> >> > On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> >> > > From: Emil Renner Berthing <[e-mail]>
> >> > > 
> >> > > Add bindings for the system clock and reset generator (SYSCRG) on the
> >> > > JH7110 RISC-V SoC by StarFive Ltd.
> >> > > 
> >> > > Signed-off-by: Emil Renner Berthing <[e-mail]>
> >> > > Signed-off-by: Hal Feng <[e-mail]>
> > 
> >> > > +  clocks:
> >> > > +    items:
> >> > > +      - description: Main Oscillator (24 MHz)
> >> > > +      - description: GMAC1 RMII reference
> >> > > +      - description: GMAC1 RGMII RX
> >> > > +      - description: External I2S TX bit clock
> >> > > +      - description: External I2S TX left/right channel clock
> >> > > +      - description: External I2S RX bit clock
> >> > > +      - description: External I2S RX left/right channel clock
> >> > > +      - description: External TDM clock
> >> > > +      - description: External audio master clock
> >> > 
> >> > So, from peeking at the clock driver & the dt - it looks like a bunch of
> >> > these are not actually required?
> >> 
> >> These clocks are used as root clocks or optional parent clocks in clock tree.
> >> Some of them are optional, but they are required if we want to describe the
> >> complete clock tree of JH7110 SoC.
> > 
> > Perhaps I have a misunderstand of what required means. To me, required
> > means "you must provide this clock for the SoC to operate in all
> > configurations".
> > Optional therefore would be for things that are needed only for some
> > configurations and may be omitted if not required.
> > 
> > From your comment below, boards with a JH7110 may choose not to populate
> > both external clock inputs to a mux. In that case, "dummy" clocks should
> > not have to be provided in the DT of such boards to satisfy this binding
> > which seems wrong to me..
> 
> Please see the picture of these external clocks in clock tree.
> 
> # mount -t debugfs none /mnt
> # cat /mnt/clk/clk_summary
>                                  enable  prepare  protect                                duty  hardware
>    clock                          count    count    count        rate   accuracy phase  cycle    enable
> -------------------------------------------------------------------------------------------------------
>  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
>  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
>  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
>     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
>        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
>  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
>     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
>        gmac1_tx                       0        0        0    50000000          0     0  50000         N
>           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
>  *osc*                                  4        4        0    24000000          0     0  50000         Y
>     apb_func                          0        0        0    24000000          0     0  50000         Y
>  ...
> 
> The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
> actually used as the parent of other clocks.

> The "dummy" clocks
> you said are all internal clocks.

No, what I meant by "dummy" clocks is that if you make clocks "required"
in the binding that are not needed by the hardware for operation a
customer of yours might have to add "dummy" clocks to their devicetree
to pass dtbs_check.

> For the audio related clocks (mclk_ext/tdm_ext/i2srx_lrck_ext/
> i2srx_bclk_ext/i2stx_lrck_ext/i2stx_bclk_ext), they will be used
> as the parent clocks in audio related drivers. Note that some
> clocks need to select different clocks as parent according to
> requirement.
> So all these external clocks are required.
> 
> > 
> > It would seem to me that you need to set minItems < maxItems here to
> > account for that & you do in fact need clock-names.
> > 
> >> 
> >> > I'd have ploughed through this, but having read Krzysztof's comments on
> >> > the DTS I'm not sure that this binding is correct.
> >> > https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
> >> > 
> >> > I *think* the DT is correct - the fixed clocks are all inputs from clock
> >> > sources on the board and as such they are empty in soc.dtsi and are
> >> > populated in board.dts?
> >> 
> >> Yes, the fixed clocks are all clock sources on the board and input to the SoC.
> >> 
> >> > 
> >> > However, are they all actually required? In the driver I see:
> >> > 	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
> >> > 		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
> >> > 		    JH7110_SYSCLK_GMAC1_RMII_RTX),
> >> > That macro is:
> >> > #define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
> >> > 	.name = _name,								\
> >> > 	.flags = 0,								\
> >> > 	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
> >> > 	.parents = { __VA_ARGS__ },						\
> >> > }

> >> > AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
> >> > Does that mean you need to populate only one of GMAC1 RMII reference
> >> > and GMAC1 RMGII RX and the other is optional?

> >> Yes, actually only one of them is chosen as the root clock
> >> source of the clock "gmac1_rx".
|  *gmac1_rgmii_rxin*   
|     gmac1_rx          
|        gmac1_rx_inv   
|  *gmac1_rmii_refin*   
|     gmac1_rmii_rtx    
|        gmac1_tx       
|           gmac1_tx_inv
|
| description: GMAC1 RMII reference
| description: GMAC1 RGMII RX


So you're telling me that you can either:
- Provide GMAC1 RMII reference and GMAC1 RGMII RX & then use different
  clocks for gmac1_rx and gmac1_tx
- Provide only GMAC1 RMII reference & use it for both gmac1_tx *and*
  gmac1_rx

Is that correct?

> >> > 
> >> > > +
> >> > > +  clock-names:
> >> > > +    items:
> >> > > +      - const: osc
> >> > > +      - const: gmac1_rmii_refin
> >> > > +      - const: gmac1_rgmii_rxin
> >> > > +      - const: i2stx_bclk_ext
> >> > > +      - const: i2stx_lrck_ext
> >> > > +      - const: i2srx_bclk_ext
> >> > > +      - const: i2srx_lrck_ext
> >> > > +      - const: tdm_ext
> >> > > +      - const: mclk_ext
> >> > 
> >> > If all clocks are in fact required though, isn't this kinda pointless to
> >> > have since we already know that the order is fixed from the "clocks"
> >> > property?
> >> > Krzk/Rob?
> >> 
> >> The clock-names are used to easily identify these clocks in the clock driver.
> > 
> > *IF* all clocks were in fact required, which they aren't, you could rely
> > on the order alone in the driver as it is enforced by the binding.
> 
> OK, I'll remove "clock-names" property in the bindings and device tree.
> Instead, will use index to get these clocks in drivers.

Hang on until you answer my question above before deleting this from the
dt-binding & driver ;)

Hal Feng Feb. 17, 2023, 2:27 a.m. UTC | #8

On Thu, 16 Feb 2023 18:20:34 +0000, Conor Dooley wrote:
> Hey Hal!
> 
> On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
>> On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
>> > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
>> >> On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
>> >> > On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
>> >> > > From: Emil Renner Berthing <[e-mail]>
>> >> > > 
>> >> > > Add bindings for the system clock and reset generator (SYSCRG) on the
>> >> > > JH7110 RISC-V SoC by StarFive Ltd.
>> >> > > 
>> >> > > Signed-off-by: Emil Renner Berthing <[e-mail]>
>> >> > > Signed-off-by: Hal Feng <[e-mail]>
>> > 
>> >> > > +  clocks:
>> >> > > +    items:
>> >> > > +      - description: Main Oscillator (24 MHz)
>> >> > > +      - description: GMAC1 RMII reference
>> >> > > +      - description: GMAC1 RGMII RX
>> >> > > +      - description: External I2S TX bit clock
>> >> > > +      - description: External I2S TX left/right channel clock
>> >> > > +      - description: External I2S RX bit clock
>> >> > > +      - description: External I2S RX left/right channel clock
>> >> > > +      - description: External TDM clock
>> >> > > +      - description: External audio master clock
>> >> > 
>> >> > So, from peeking at the clock driver & the dt - it looks like a bunch of
>> >> > these are not actually required?
>> >> 
>> >> These clocks are used as root clocks or optional parent clocks in clock tree.
>> >> Some of them are optional, but they are required if we want to describe the
>> >> complete clock tree of JH7110 SoC.
>> > 
>> > Perhaps I have a misunderstand of what required means. To me, required
>> > means "you must provide this clock for the SoC to operate in all
>> > configurations".
>> > Optional therefore would be for things that are needed only for some
>> > configurations and may be omitted if not required.
>> > 
>> > From your comment below, boards with a JH7110 may choose not to populate
>> > both external clock inputs to a mux. In that case, "dummy" clocks should
>> > not have to be provided in the DT of such boards to satisfy this binding
>> > which seems wrong to me..
>> 
>> Please see the picture of these external clocks in clock tree.
>> 
>> # mount -t debugfs none /mnt
>> # cat /mnt/clk/clk_summary
>>                                  enable  prepare  protect                                duty  hardware
>>    clock                          count    count    count        rate   accuracy phase  cycle    enable
>> -------------------------------------------------------------------------------------------------------
>>  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
>>  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
>>  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>>  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>>  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>>  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>>  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
>>     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
>>        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
>>  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
>>     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
>>        gmac1_tx                       0        0        0    50000000          0     0  50000         N
>>           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
>>  *osc*                                  4        4        0    24000000          0     0  50000         Y
>>     apb_func                          0        0        0    24000000          0     0  50000         Y
>>  ...
>> 
>> The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
>> actually used as the parent of other clocks.
> 
>> The "dummy" clocks
>> you said are all internal clocks.
> 
> No, what I meant by "dummy" clocks is that if you make clocks "required"
> in the binding that are not needed by the hardware for operation a
> customer of yours might have to add "dummy" clocks to their devicetree
> to pass dtbs_check.
> 
>> For the audio related clocks (mclk_ext/tdm_ext/i2srx_lrck_ext/
>> i2srx_bclk_ext/i2stx_lrck_ext/i2stx_bclk_ext), they will be used
>> as the parent clocks in audio related drivers. Note that some
>> clocks need to select different clocks as parent according to
>> requirement.
>> So all these external clocks are required.
>> 
>> > 
>> > It would seem to me that you need to set minItems < maxItems here to
>> > account for that & you do in fact need clock-names.
>> > 
>> >> 
>> >> > I'd have ploughed through this, but having read Krzysztof's comments on
>> >> > the DTS I'm not sure that this binding is correct.
>> >> > https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
>> >> > 
>> >> > I *think* the DT is correct - the fixed clocks are all inputs from clock
>> >> > sources on the board and as such they are empty in soc.dtsi and are
>> >> > populated in board.dts?
>> >> 
>> >> Yes, the fixed clocks are all clock sources on the board and input to the SoC.
>> >> 
>> >> > 
>> >> > However, are they all actually required? In the driver I see:
>> >> > 	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
>> >> > 		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
>> >> > 		    JH7110_SYSCLK_GMAC1_RMII_RTX),
>> >> > That macro is:
>> >> > #define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
>> >> > 	.name = _name,								\
>> >> > 	.flags = 0,								\
>> >> > 	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
>> >> > 	.parents = { __VA_ARGS__ },						\
>> >> > }
> 
>> >> > AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
>> >> > Does that mean you need to populate only one of GMAC1 RMII reference
>> >> > and GMAC1 RMGII RX and the other is optional?
> 
>> >> Yes, actually only one of them is chosen as the root clock
>> >> source of the clock "gmac1_rx".
> |  *gmac1_rgmii_rxin*   
> |     gmac1_rx          
> |        gmac1_rx_inv   
> |  *gmac1_rmii_refin*   
> |     gmac1_rmii_rtx    
> |        gmac1_tx       
> |           gmac1_tx_inv
> |
> | description: GMAC1 RMII reference
> | description: GMAC1 RGMII RX
> 
> 
> So you're telling me that you can either:
> - Provide GMAC1 RMII reference and GMAC1 RGMII RX & then use different
>   clocks for gmac1_rx and gmac1_tx
> - Provide only GMAC1 RMII reference & use it for both gmac1_tx *and*
>   gmac1_rx
> 
> Is that correct?

Yes, it is.

Best regards,
Hal

> 
>> >> > 
>> >> > > +
>> >> > > +  clock-names:
>> >> > > +    items:
>> >> > > +      - const: osc
>> >> > > +      - const: gmac1_rmii_refin
>> >> > > +      - const: gmac1_rgmii_rxin
>> >> > > +      - const: i2stx_bclk_ext
>> >> > > +      - const: i2stx_lrck_ext
>> >> > > +      - const: i2srx_bclk_ext
>> >> > > +      - const: i2srx_lrck_ext
>> >> > > +      - const: tdm_ext
>> >> > > +      - const: mclk_ext
>> >> > 
>> >> > If all clocks are in fact required though, isn't this kinda pointless to
>> >> > have since we already know that the order is fixed from the "clocks"
>> >> > property?
>> >> > Krzk/Rob?
>> >> 
>> >> The clock-names are used to easily identify these clocks in the clock driver.
>> > 
>> > *IF* all clocks were in fact required, which they aren't, you could rely
>> > on the order alone in the driver as it is enforced by the binding.
>> 
>> OK, I'll remove "clock-names" property in the bindings and device tree.
>> Instead, will use index to get these clocks in drivers.
> 
> Hang on until you answer my question above before deleting this from the
> dt-binding & driver ;)
>

Conor Dooley Feb. 17, 2023, 7:51 a.m. UTC | #9

On Fri, Feb 17, 2023 at 10:27:27AM +0800, Hal Feng wrote:
> On Thu, 16 Feb 2023 18:20:34 +0000, Conor Dooley wrote:
> > Hey Hal!
> > 
> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
> >> On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
> >> > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
> >> >> On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
> >> >> > On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> >> >> > > From: Emil Renner Berthing <[e-mail]>
> >> >> > > 
> >> >> > > Add bindings for the system clock and reset generator (SYSCRG) on the
> >> >> > > JH7110 RISC-V SoC by StarFive Ltd.
> >> >> > > 
> >> >> > > Signed-off-by: Emil Renner Berthing <[e-mail]>
> >> >> > > Signed-off-by: Hal Feng <[e-mail]>
> >> > 
> >> >> > > +  clocks:
> >> >> > > +    items:
> >> >> > > +      - description: Main Oscillator (24 MHz)
> >> >> > > +      - description: GMAC1 RMII reference
> >> >> > > +      - description: GMAC1 RGMII RX
> >> >> > > +      - description: External I2S TX bit clock
> >> >> > > +      - description: External I2S TX left/right channel clock
> >> >> > > +      - description: External I2S RX bit clock
> >> >> > > +      - description: External I2S RX left/right channel clock
> >> >> > > +      - description: External TDM clock
> >> >> > > +      - description: External audio master clock
> >> >> > 
> >> >> > So, from peeking at the clock driver & the dt - it looks like a bunch of
> >> >> > these are not actually required?
> >> >> 
> >> >> These clocks are used as root clocks or optional parent clocks in clock tree.
> >> >> Some of them are optional, but they are required if we want to describe the
> >> >> complete clock tree of JH7110 SoC.
> >> > 
> >> > Perhaps I have a misunderstand of what required means. To me, required
> >> > means "you must provide this clock for the SoC to operate in all
> >> > configurations".
> >> > Optional therefore would be for things that are needed only for some
> >> > configurations and may be omitted if not required.
> >> > 
> >> > From your comment below, boards with a JH7110 may choose not to populate
> >> > both external clock inputs to a mux. In that case, "dummy" clocks should
> >> > not have to be provided in the DT of such boards to satisfy this binding
> >> > which seems wrong to me..
> >> 
> >> Please see the picture of these external clocks in clock tree.
> >> 
> >> # mount -t debugfs none /mnt
> >> # cat /mnt/clk/clk_summary
> >>                                  enable  prepare  protect                                duty  hardware
> >>    clock                          count    count    count        rate   accuracy phase  cycle    enable
> >> -------------------------------------------------------------------------------------------------------
> >>  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
> >>  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
> >>  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> >>  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> >>  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> >>  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> >>  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
> >>     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
> >>        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
> >>  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
> >>     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
> >>        gmac1_tx                       0        0        0    50000000          0     0  50000         N
> >>           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
> >>  *osc*                                  4        4        0    24000000          0     0  50000         Y
> >>     apb_func                          0        0        0    24000000          0     0  50000         Y
> >>  ...
> >> 
> >> The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
> >> actually used as the parent of other clocks.
> > 
> >> The "dummy" clocks
> >> you said are all internal clocks.
> > 
> > No, what I meant by "dummy" clocks is that if you make clocks "required"
> > in the binding that are not needed by the hardware for operation a
> > customer of yours might have to add "dummy" clocks to their devicetree
> > to pass dtbs_check.
> > 
> >> For the audio related clocks (mclk_ext/tdm_ext/i2srx_lrck_ext/
> >> i2srx_bclk_ext/i2stx_lrck_ext/i2stx_bclk_ext), they will be used
> >> as the parent clocks in audio related drivers. Note that some
> >> clocks need to select different clocks as parent according to
> >> requirement.
> >> So all these external clocks are required.
> >> 
> >> > 
> >> > It would seem to me that you need to set minItems < maxItems here to
> >> > account for that & you do in fact need clock-names.
> >> > 
> >> >> 
> >> >> > I'd have ploughed through this, but having read Krzysztof's comments on
> >> >> > the DTS I'm not sure that this binding is correct.
> >> >> > https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
> >> >> > 
> >> >> > I *think* the DT is correct - the fixed clocks are all inputs from clock
> >> >> > sources on the board and as such they are empty in soc.dtsi and are
> >> >> > populated in board.dts?
> >> >> 
> >> >> Yes, the fixed clocks are all clock sources on the board and input to the SoC.
> >> >> 
> >> >> > 
> >> >> > However, are they all actually required? In the driver I see:
> >> >> > 	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
> >> >> > 		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
> >> >> > 		    JH7110_SYSCLK_GMAC1_RMII_RTX),
> >> >> > That macro is:
> >> >> > #define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
> >> >> > 	.name = _name,								\
> >> >> > 	.flags = 0,								\
> >> >> > 	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
> >> >> > 	.parents = { __VA_ARGS__ },						\
> >> >> > }
> > 
> >> >> > AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
> >> >> > Does that mean you need to populate only one of GMAC1 RMII reference
> >> >> > and GMAC1 RMGII RX and the other is optional?
> > 
> >> >> Yes, actually only one of them is chosen as the root clock
> >> >> source of the clock "gmac1_rx".
> > |  *gmac1_rgmii_rxin*   
> > |     gmac1_rx          
> > |        gmac1_rx_inv   
> > |  *gmac1_rmii_refin*   
> > |     gmac1_rmii_rtx    
> > |        gmac1_tx       
> > |           gmac1_tx_inv
> > |
> > | description: GMAC1 RMII reference
> > | description: GMAC1 RGMII RX
> > 
> > 
> > So you're telling me that you can either:
> > - Provide GMAC1 RMII reference and GMAC1 RGMII RX & then use different
> >   clocks for gmac1_rx and gmac1_tx
> > - Provide only GMAC1 RMII reference & use it for both gmac1_tx *and*
> >   gmac1_rx
> > 
> > Is that correct?
> 
> Yes, it is.

Which would then make GMAC1 RGMII RX optional, rather than required?

Hal Feng Feb. 17, 2023, 12:20 p.m. UTC | #10

On Fri, 17 Feb 2023 07:51:24 +0000, Conor Dooley wrote:
> On Fri, Feb 17, 2023 at 10:27:27AM +0800, Hal Feng wrote:
>> On Thu, 16 Feb 2023 18:20:34 +0000, Conor Dooley wrote:
>> > Hey Hal!
>> > 
>> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
>> >> On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
>> >> > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
>> >> >> On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
>> >> >> > On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
>> >> >> > > From: Emil Renner Berthing <[e-mail]>
>> >> >> > > 
>> >> >> > > Add bindings for the system clock and reset generator (SYSCRG) on the
>> >> >> > > JH7110 RISC-V SoC by StarFive Ltd.
>> >> >> > > 
>> >> >> > > Signed-off-by: Emil Renner Berthing <[e-mail]>
>> >> >> > > Signed-off-by: Hal Feng <[e-mail]>
>> >> > 
>> >> >> > > +  clocks:
>> >> >> > > +    items:
>> >> >> > > +      - description: Main Oscillator (24 MHz)
>> >> >> > > +      - description: GMAC1 RMII reference
>> >> >> > > +      - description: GMAC1 RGMII RX
>> >> >> > > +      - description: External I2S TX bit clock
>> >> >> > > +      - description: External I2S TX left/right channel clock
>> >> >> > > +      - description: External I2S RX bit clock
>> >> >> > > +      - description: External I2S RX left/right channel clock
>> >> >> > > +      - description: External TDM clock
>> >> >> > > +      - description: External audio master clock
>> >> >> > 
>> >> >> > So, from peeking at the clock driver & the dt - it looks like a bunch of
>> >> >> > these are not actually required?
>> >> >> 
>> >> >> These clocks are used as root clocks or optional parent clocks in clock tree.
>> >> >> Some of them are optional, but they are required if we want to describe the
>> >> >> complete clock tree of JH7110 SoC.
>> >> > 
>> >> > Perhaps I have a misunderstand of what required means. To me, required
>> >> > means "you must provide this clock for the SoC to operate in all
>> >> > configurations".
>> >> > Optional therefore would be for things that are needed only for some
>> >> > configurations and may be omitted if not required.
>> >> > 
>> >> > From your comment below, boards with a JH7110 may choose not to populate
>> >> > both external clock inputs to a mux. In that case, "dummy" clocks should
>> >> > not have to be provided in the DT of such boards to satisfy this binding
>> >> > which seems wrong to me..
>> >> 
>> >> Please see the picture of these external clocks in clock tree.
>> >> 
>> >> # mount -t debugfs none /mnt
>> >> # cat /mnt/clk/clk_summary
>> >>                                  enable  prepare  protect                                duty  hardware
>> >>    clock                          count    count    count        rate   accuracy phase  cycle    enable
>> >> -------------------------------------------------------------------------------------------------------
>> >>  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
>> >>  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
>> >>  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>> >>  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>> >>  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>> >>  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>> >>  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
>> >>     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
>> >>        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
>> >>  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
>> >>     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
>> >>        gmac1_tx                       0        0        0    50000000          0     0  50000         N
>> >>           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
>> >>  *osc*                                  4        4        0    24000000          0     0  50000         Y
>> >>     apb_func                          0        0        0    24000000          0     0  50000         Y
>> >>  ...
>> >> 
>> >> The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
>> >> actually used as the parent of other clocks.
>> > 
>> >> The "dummy" clocks
>> >> you said are all internal clocks.
>> > 
>> > No, what I meant by "dummy" clocks is that if you make clocks "required"
>> > in the binding that are not needed by the hardware for operation a
>> > customer of yours might have to add "dummy" clocks to their devicetree
>> > to pass dtbs_check.
>> > 
>> >> For the audio related clocks (mclk_ext/tdm_ext/i2srx_lrck_ext/
>> >> i2srx_bclk_ext/i2stx_lrck_ext/i2stx_bclk_ext), they will be used
>> >> as the parent clocks in audio related drivers. Note that some
>> >> clocks need to select different clocks as parent according to
>> >> requirement.
>> >> So all these external clocks are required.
>> >> 
>> >> > 
>> >> > It would seem to me that you need to set minItems < maxItems here to
>> >> > account for that & you do in fact need clock-names.
>> >> > 
>> >> >> 
>> >> >> > I'd have ploughed through this, but having read Krzysztof's comments on
>> >> >> > the DTS I'm not sure that this binding is correct.
>> >> >> > https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
>> >> >> > 
>> >> >> > I *think* the DT is correct - the fixed clocks are all inputs from clock
>> >> >> > sources on the board and as such they are empty in soc.dtsi and are
>> >> >> > populated in board.dts?
>> >> >> 
>> >> >> Yes, the fixed clocks are all clock sources on the board and input to the SoC.
>> >> >> 
>> >> >> > 
>> >> >> > However, are they all actually required? In the driver I see:
>> >> >> > 	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
>> >> >> > 		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
>> >> >> > 		    JH7110_SYSCLK_GMAC1_RMII_RTX),
>> >> >> > That macro is:
>> >> >> > #define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
>> >> >> > 	.name = _name,								\
>> >> >> > 	.flags = 0,								\
>> >> >> > 	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
>> >> >> > 	.parents = { __VA_ARGS__ },						\
>> >> >> > }
>> > 
>> >> >> > AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
>> >> >> > Does that mean you need to populate only one of GMAC1 RMII reference
>> >> >> > and GMAC1 RMGII RX and the other is optional?
>> > 
>> >> >> Yes, actually only one of them is chosen as the root clock
>> >> >> source of the clock "gmac1_rx".
>> > |  *gmac1_rgmii_rxin*   
>> > |     gmac1_rx          
>> > |        gmac1_rx_inv   
>> > |  *gmac1_rmii_refin*   
>> > |     gmac1_rmii_rtx    
>> > |        gmac1_tx       
>> > |           gmac1_tx_inv
>> > |
>> > | description: GMAC1 RMII reference
>> > | description: GMAC1 RGMII RX
>> > 
>> > 
>> > So you're telling me that you can either:
>> > - Provide GMAC1 RMII reference and GMAC1 RGMII RX & then use different
>> >   clocks for gmac1_rx and gmac1_tx
>> > - Provide only GMAC1 RMII reference & use it for both gmac1_tx *and*
>> >   gmac1_rx
>> > 
>> > Is that correct?
>> 
>> Yes, it is.
> 
> Which would then make GMAC1 RGMII RX optional, rather than required?

If thinking in this way, I must say yes, it is optional. But actually
GMAC1 RGMII RX feeds gmac1_rx by default. 
For a mux, it usually works if you populate only one input to it.
Does it mean all the other inputs are optional? And how can we define
which input is required?

Best regards,
Hal

Conor Dooley Feb. 17, 2023, 1:32 p.m. UTC | #11

Hal, Rob, Krzysztof,

On Fri, Feb 17, 2023 at 08:20:14PM +0800, Hal Feng wrote:
> On Fri, 17 Feb 2023 07:51:24 +0000, Conor Dooley wrote:
> > On Fri, Feb 17, 2023 at 10:27:27AM +0800, Hal Feng wrote:
> >> On Thu, 16 Feb 2023 18:20:34 +0000, Conor Dooley wrote:
> >> > Hey Hal!
> >> > 
> >> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
> >> >> On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
> >> >> > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
> >> >> >> On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
> >> >> >> > On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> >> >> >> > > From: Emil Renner Berthing <[e-mail]>
> >> >> >> > > 
> >> >> >> > > Add bindings for the system clock and reset generator (SYSCRG) on the
> >> >> >> > > JH7110 RISC-V SoC by StarFive Ltd.
> >> >> >> > > 
> >> >> >> > > Signed-off-by: Emil Renner Berthing <[e-mail]>
> >> >> >> > > Signed-off-by: Hal Feng <[e-mail]>
> >> >> > 
> >> >> >> > > +  clocks:
> >> >> >> > > +    items:
> >> >> >> > > +      - description: Main Oscillator (24 MHz)
> >> >> >> > > +      - description: GMAC1 RMII reference
> >> >> >> > > +      - description: GMAC1 RGMII RX
> >> >> >> > > +      - description: External I2S TX bit clock
> >> >> >> > > +      - description: External I2S TX left/right channel clock
> >> >> >> > > +      - description: External I2S RX bit clock
> >> >> >> > > +      - description: External I2S RX left/right channel clock
> >> >> >> > > +      - description: External TDM clock
> >> >> >> > > +      - description: External audio master clock
> >> >> >> > 
> >> >> >> > So, from peeking at the clock driver & the dt - it looks like a bunch of
> >> >> >> > these are not actually required?
> >> >> >> 
> >> >> >> These clocks are used as root clocks or optional parent clocks in clock tree.
> >> >> >> Some of them are optional, but they are required if we want to describe the
> >> >> >> complete clock tree of JH7110 SoC.
> >> >> > 
> >> >> > Perhaps I have a misunderstand of what required means. To me, required
> >> >> > means "you must provide this clock for the SoC to operate in all
> >> >> > configurations".
> >> >> > Optional therefore would be for things that are needed only for some
> >> >> > configurations and may be omitted if not required.
> >> >> > 
> >> >> > From your comment below, boards with a JH7110 may choose not to populate
> >> >> > both external clock inputs to a mux. In that case, "dummy" clocks should
> >> >> > not have to be provided in the DT of such boards to satisfy this binding
> >> >> > which seems wrong to me..
> >> >> 
> >> >> Please see the picture of these external clocks in clock tree.
> >> >> 
> >> >> # mount -t debugfs none /mnt
> >> >> # cat /mnt/clk/clk_summary
> >> >>                                  enable  prepare  protect                                duty  hardware
> >> >>    clock                          count    count    count        rate   accuracy phase  cycle    enable
> >> >> -------------------------------------------------------------------------------------------------------
> >> >>  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
> >> >>  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
> >> >>  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> >> >>  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> >> >>  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> >> >>  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> >> >>  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
> >> >>     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
> >> >>        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
> >> >>  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
> >> >>     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
> >> >>        gmac1_tx                       0        0        0    50000000          0     0  50000         N
> >> >>           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
> >> >>  *osc*                                  4        4        0    24000000          0     0  50000         Y
> >> >>     apb_func                          0        0        0    24000000          0     0  50000         Y
> >> >>  ...
> >> >> 
> >> >> The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
> >> >> actually used as the parent of other clocks.
> >> > 
> >> >> The "dummy" clocks
> >> >> you said are all internal clocks.
> >> > 
> >> > No, what I meant by "dummy" clocks is that if you make clocks "required"
> >> > in the binding that are not needed by the hardware for operation a
> >> > customer of yours might have to add "dummy" clocks to their devicetree
> >> > to pass dtbs_check.
> >> > 
> >> >> For the audio related clocks (mclk_ext/tdm_ext/i2srx_lrck_ext/
> >> >> i2srx_bclk_ext/i2stx_lrck_ext/i2stx_bclk_ext), they will be used
> >> >> as the parent clocks in audio related drivers. Note that some
> >> >> clocks need to select different clocks as parent according to
> >> >> requirement.
> >> >> So all these external clocks are required.
> >> >> 
> >> >> > 
> >> >> > It would seem to me that you need to set minItems < maxItems here to
> >> >> > account for that & you do in fact need clock-names.
> >> >> > 
> >> >> >> 
> >> >> >> > I'd have ploughed through this, but having read Krzysztof's comments on
> >> >> >> > the DTS I'm not sure that this binding is correct.
> >> >> >> > https://lore.kernel.org/linux-riscv/[e-mail]/T/#mdf67621a2344dce801aa8015d4963593a2c28bcc
> >> >> >> > 
> >> >> >> > I *think* the DT is correct - the fixed clocks are all inputs from clock
> >> >> >> > sources on the board and as such they are empty in soc.dtsi and are
> >> >> >> > populated in board.dts?
> >> >> >> 
> >> >> >> Yes, the fixed clocks are all clock sources on the board and input to the SoC.
> >> >> >> 
> >> >> >> > 
> >> >> >> > However, are they all actually required? In the driver I see:
> >> >> >> > 	JH71X0__MUX(JH7110_SYSCLK_GMAC1_RX, "gmac1_rx", 2,
> >> >> >> > 		    JH7110_SYSCLK_GMAC1_RGMII_RXIN,
> >> >> >> > 		    JH7110_SYSCLK_GMAC1_RMII_RTX),
> >> >> >> > That macro is:
> >> >> >> > #define JH71X0__MUX(_idx, _name, _nparents, ...) [_idx] = {			\
> >> >> >> > 	.name = _name,								\
> >> >> >> > 	.flags = 0,								\
> >> >> >> > 	.max = ((_nparents) - 1) << JH71X0_CLK_MUX_SHIFT,			\
> >> >> >> > 	.parents = { __VA_ARGS__ },						\
> >> >> >> > }
> >> > 
> >> >> >> > AFAICT, RMII reference feeds RMII_RTX & RGMII RX *is* RGMII_RXIN?
> >> >> >> > Does that mean you need to populate only one of GMAC1 RMII reference
> >> >> >> > and GMAC1 RMGII RX and the other is optional?
> >> > 
> >> >> >> Yes, actually only one of them is chosen as the root clock
> >> >> >> source of the clock "gmac1_rx".
> >> > |  *gmac1_rgmii_rxin*   
> >> > |     gmac1_rx          
> >> > |        gmac1_rx_inv   
> >> > |  *gmac1_rmii_refin*   
> >> > |     gmac1_rmii_rtx    
> >> > |        gmac1_tx       
> >> > |           gmac1_tx_inv
> >> > |
> >> > | description: GMAC1 RMII reference
> >> > | description: GMAC1 RGMII RX
> >> > 
> >> > 
> >> > So you're telling me that you can either:
> >> > - Provide GMAC1 RMII reference and GMAC1 RGMII RX & then use different
> >> >   clocks for gmac1_rx and gmac1_tx
> >> > - Provide only GMAC1 RMII reference & use it for both gmac1_tx *and*
> >> >   gmac1_rx
> >> > 
> >> > Is that correct?
> >> 
> >> Yes, it is.
> > 
> > Which would then make GMAC1 RGMII RX optional, rather than required?
> 
> If thinking in this way, I must say yes, it is optional. But actually
> GMAC1 RGMII RX feeds gmac1_rx by default. 
> For a mux, it usually works if you populate only one input to it.
> Does it mean all the other inputs are optional? And how can we define
> which input is required?

I'm not sure, that is a question for Krzysztof and/or Rob.

Krzysztof Kozlowski Feb. 17, 2023, 3:47 p.m. UTC | #12

On 17/02/2023 14:32, Conor Dooley wrote:
>>>> Yes, it is.
>>>
>>> Which would then make GMAC1 RGMII RX optional, rather than required?
>>
>> If thinking in this way, I must say yes, it is optional. But actually
>> GMAC1 RGMII RX feeds gmac1_rx by default. 
>> For a mux, it usually works if you populate only one input to it.
>> Does it mean all the other inputs are optional? And how can we define
>> which input is required?
> 
> I'm not sure, that is a question for Krzysztof and/or Rob.

That's a long thread, please summarize what you ask. Otherwise I have no
clue what is the question.

Does the mux works correctly if clock input is not connected? I mean,
are you now talking about real hardware or some simplification from SW
point of view?

Best regards,
Krzysztof

Conor Dooley Feb. 17, 2023, 4:27 p.m. UTC | #13

On Fri, Feb 17, 2023 at 04:47:48PM +0100, Krzysztof Kozlowski wrote:
> On 17/02/2023 14:32, Conor Dooley wrote:
> >>>> Yes, it is.
> >>>
> >>> Which would then make GMAC1 RGMII RX optional, rather than required?
> >>
> >> If thinking in this way, I must say yes, it is optional. But actually
> >> GMAC1 RGMII RX feeds gmac1_rx by default. 
> >> For a mux, it usually works if you populate only one input to it.
> >> Does it mean all the other inputs are optional? And how can we define
> >> which input is required?
> > 
> > I'm not sure, that is a question for Krzysztof and/or Rob.
> 
> That's a long thread, please summarize what you ask. Otherwise I have no
> clue what is the question.

Sorry. I tried to preserve the context of the conversation the last time
I cropped it so that things would be contained on one email.

For me at least, I am wondering how you convey that out of a list of
clock inputs (for example a, b, c, d) that two of the clocks are inputs
to a mux and it is only required to provide one of the two (say b & c).

> Does the mux works correctly if clock input is not connected? I mean,
> are you now talking about real hardware or some simplification from SW
> point of view?

I'm coming at this from an angle of "is a StarFive customer going to show
up with a devicetree containing dummy fixed-clocks to satisfy dtbs_check
because they opted to only populate one input to the mux".
I don't really care about implications for the driver, just about
whether the hardware allows for inputs to the mux to be left
un-populated.

Cheers,
Conor.

Krzysztof Kozlowski Feb. 18, 2023, 10:20 a.m. UTC | #14

On 17/02/2023 17:27, Conor Dooley wrote:
> On Fri, Feb 17, 2023 at 04:47:48PM +0100, Krzysztof Kozlowski wrote:
>> On 17/02/2023 14:32, Conor Dooley wrote:
>>>>>> Yes, it is.
>>>>>
>>>>> Which would then make GMAC1 RGMII RX optional, rather than required?
>>>>
>>>> If thinking in this way, I must say yes, it is optional. But actually
>>>> GMAC1 RGMII RX feeds gmac1_rx by default. 
>>>> For a mux, it usually works if you populate only one input to it.
>>>> Does it mean all the other inputs are optional? And how can we define
>>>> which input is required?
>>>
>>> I'm not sure, that is a question for Krzysztof and/or Rob.
>>
>> That's a long thread, please summarize what you ask. Otherwise I have no
>> clue what is the question.
> 
> Sorry. I tried to preserve the context of the conversation the last time
> I cropped it so that things would be contained on one email.
> 
> For me at least, I am wondering how you convey that out of a list of
> clock inputs (for example a, b, c, d) that two of the clocks are inputs
> to a mux and it is only required to provide one of the two (say b & c).
> 
>> Does the mux works correctly if clock input is not connected? I mean,
>> are you now talking about real hardware or some simplification from SW
>> point of view?
> 
> I'm coming at this from an angle of "is a StarFive customer going to show
> up with a devicetree containing dummy fixed-clocks to satisfy dtbs_check
> because they opted to only populate one input to the mux".
> I don't really care about implications for the driver, just about
> whether the hardware allows for inputs to the mux to be left
> un-populated.

Whether hardware allows - not a question to me. BTW, this is rather
question coming from me...

Best regards,
Krzysztof

Conor Dooley Feb. 18, 2023, 11:17 a.m. UTC | #15

Hey Krzysztof,

On Sat, Feb 18, 2023 at 11:20:30AM +0100, Krzysztof Kozlowski wrote:
> On 17/02/2023 17:27, Conor Dooley wrote:
> > On Fri, Feb 17, 2023 at 04:47:48PM +0100, Krzysztof Kozlowski wrote:
> >> On 17/02/2023 14:32, Conor Dooley wrote:
> >>>>>> Yes, it is.
> >>>>>
> >>>>> Which would then make GMAC1 RGMII RX optional, rather than required?
> >>>>
> >>>> If thinking in this way, I must say yes, it is optional. But actually
> >>>> GMAC1 RGMII RX feeds gmac1_rx by default. 
> >>>> For a mux, it usually works if you populate only one input to it.
> >>>> Does it mean all the other inputs are optional? And how can we define
> >>>> which input is required?
> >>>
> >>> I'm not sure, that is a question for Krzysztof and/or Rob.
> >>
> >> That's a long thread, please summarize what you ask. Otherwise I have no
> >> clue what is the question.
> > 
> > Sorry. I tried to preserve the context of the conversation the last time
> > I cropped it so that things would be contained on one email.
> > 
> > For me at least, I am wondering how you convey that out of a list of
> > clock inputs (for example a, b, c, d) that two of the clocks are inputs
> > to a mux and it is only required to provide one of the two (say b & c).

You skipped this part which was what I was trying to ask you about.
Do you know how to convey this situation, or is it even possible to
express those rules?

> >> Does the mux works correctly if clock input is not connected? I mean,
> >> are you now talking about real hardware or some simplification from SW
> >> point of view?
> > 
> > I'm coming at this from an angle of "is a StarFive customer going to show
> > up with a devicetree containing dummy fixed-clocks to satisfy dtbs_check
> > because they opted to only populate one input to the mux".
> > I don't really care about implications for the driver, just about
> > whether the hardware allows for inputs to the mux to be left
> > un-populated.
> 
> Whether hardware allows - not a question to me.

> BTW, this is rather question coming from me...

I don't understand what you mean by this, sorry.

Thanks,
Conor.

Krzysztof Kozlowski Feb. 18, 2023, 2:55 p.m. UTC | #16

On 18/02/2023 12:17, Conor Dooley wrote:
> Hey Krzysztof,
> 
> On Sat, Feb 18, 2023 at 11:20:30AM +0100, Krzysztof Kozlowski wrote:
>> On 17/02/2023 17:27, Conor Dooley wrote:
>>> On Fri, Feb 17, 2023 at 04:47:48PM +0100, Krzysztof Kozlowski wrote:
>>>> On 17/02/2023 14:32, Conor Dooley wrote:
>>>>>>>> Yes, it is.
>>>>>>>
>>>>>>> Which would then make GMAC1 RGMII RX optional, rather than required?
>>>>>>
>>>>>> If thinking in this way, I must say yes, it is optional. But actually
>>>>>> GMAC1 RGMII RX feeds gmac1_rx by default. 
>>>>>> For a mux, it usually works if you populate only one input to it.
>>>>>> Does it mean all the other inputs are optional? And how can we define
>>>>>> which input is required?
>>>>>
>>>>> I'm not sure, that is a question for Krzysztof and/or Rob.
>>>>
>>>> That's a long thread, please summarize what you ask. Otherwise I have no
>>>> clue what is the question.
>>>
>>> Sorry. I tried to preserve the context of the conversation the last time
>>> I cropped it so that things would be contained on one email.
>>>
>>> For me at least, I am wondering how you convey that out of a list of
>>> clock inputs (for example a, b, c, d) that two of the clocks are inputs
>>> to a mux and it is only required to provide one of the two (say b & c).
> 
> You skipped this part which was what I was trying to ask you about.

Yeah, I skipped a lot because there was one big thread with a question:
what do you think? Sorry, I will not dig 8 emails thread to figure out
which question is to me and which is not...

> Do you know how to convey this situation, or is it even possible to
> express those rules?

oneOf:
 - clock-names:
     minItems: 3
     items:
       - a
       - b
       - c
       - d
 - clock-names:
     items:
       - a
       - b
       - d

or maybe:
 - clock-names:
     minItems: 3
     items:
       - a
       - b
       - enum: [c, d]
       - d


> 
>>>> Does the mux works correctly if clock input is not connected? I mean,
>>>> are you now talking about real hardware or some simplification from SW
>>>> point of view?
>>>
>>> I'm coming at this from an angle of "is a StarFive customer going to show
>>> up with a devicetree containing dummy fixed-clocks to satisfy dtbs_check
>>> because they opted to only populate one input to the mux".
>>> I don't really care about implications for the driver, just about
>>> whether the hardware allows for inputs to the mux to be left
>>> un-populated.
>>
>> Whether hardware allows - not a question to me.
> 
>> BTW, this is rather question coming from me...
> 
> I don't understand what you mean by this, sorry.

You said to a letter addressed to me "whether the hardware allows for
...". Why would you ask me about hardware I know nothing about? That was
my question - I am asking - whether hardware allows it or not. Then
write bindings depending on that.

Best regards,
Krzysztof

Conor Dooley Feb. 18, 2023, 3:08 p.m. UTC | #17

On Sat, Feb 18, 2023 at 03:55:25PM +0100, Krzysztof Kozlowski wrote:
> On 18/02/2023 12:17, Conor Dooley wrote:
> > On Sat, Feb 18, 2023 at 11:20:30AM +0100, Krzysztof Kozlowski wrote:

> >>>> That's a long thread, please summarize what you ask. Otherwise I have no
> >>>> clue what is the question.
> >>>
> >>> Sorry. I tried to preserve the context of the conversation the last time
> >>> I cropped it so that things would be contained on one email.
> >>>
> >>> For me at least, I am wondering how you convey that out of a list of
> >>> clock inputs (for example a, b, c, d) that two of the clocks are inputs
> >>> to a mux and it is only required to provide one of the two (say b & c).
> > 
> > You skipped this part which was what I was trying to ask you about.
> 
> Yeah, I skipped a lot because there was one big thread with a question:
> what do you think? Sorry, I will not dig 8 emails thread to figure out
> which question is to me and which is not...

This was in the cut-down message & a fake scenario to avoid you needing
to understand the full thread.
I kept the context originally so that you would not have to dig out the
thread & provided the fake scenario this time for this very reason.

> > Do you know how to convey this situation, or is it even possible to
> > express those rules?
> 
> oneOf:
>  - clock-names:
>      minItems: 3
>      items:
>        - a
>        - b
>        - c
>        - d
>  - clock-names:
>      items:
>        - a
>        - b
>        - d
> 
> or maybe:
>  - clock-names:
>      minItems: 3
>      items:
>        - a
>        - b
>        - enum: [c, d]
>        - d

Thanks for the suggestions. Without actually going and playing with it,
the first of those two looks like it may be the right fit for this
situation, depending on what Hal says the hardware can do.

> >>>> Does the mux works correctly if clock input is not connected? I mean,
> >>>> are you now talking about real hardware or some simplification from SW
> >>>> point of view?
> >>>
> >>> I'm coming at this from an angle of "is a StarFive customer going to show
> >>> up with a devicetree containing dummy fixed-clocks to satisfy dtbs_check
> >>> because they opted to only populate one input to the mux".
> >>> I don't really care about implications for the driver, just about
> >>> whether the hardware allows for inputs to the mux to be left
> >>> un-populated.
> >>
> >> Whether hardware allows - not a question to me.
> > 
> >> BTW, this is rather question coming from me...
> > 
> > I don't understand what you mean by this, sorry.
> 
> You said to a letter addressed to me "whether the hardware allows for
> ...". Why would you ask me about hardware I know nothing about? That was
> my question - I am asking - whether hardware allows it or not. Then
> write bindings depending on that.

There was no question here, instead it was an answer to this question of
yours:
> I mean,
> are you now talking about real hardware or some simplification from SW
> point of view?

In which I was saying I cared about the "real hardware". For obvious
reasons, I wouldn't ask you to explain whether the hardware is capable
of it or not!
Perhaps your original question here was misunderstood.

Thanks for the suggestions,
Conor.

Stephen Boyd Feb. 21, 2023, 10:17 p.m. UTC | #18

Quoting Conor Dooley (2023-02-16 10:20:34)
> Hey Hal!
> 
> On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
> > On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
> > > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
> > >> On Tue, 20 Dec 2022 23:14:39 +0000, Conor Dooley wrote:
> > >> > On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> > >> > > From: Emil Renner Berthing <[e-mail]>
> > >> > > 
> > >> > > Add bindings for the system clock and reset generator (SYSCRG) on the
> > >> > > JH7110 RISC-V SoC by StarFive Ltd.
> > >> > > 
> > >> > > Signed-off-by: Emil Renner Berthing <[e-mail]>
> > >> > > Signed-off-by: Hal Feng <[e-mail]>
> > > 
> > >> > > +  clocks:
> > >> > > +    items:
> > >> > > +      - description: Main Oscillator (24 MHz)
> > >> > > +      - description: GMAC1 RMII reference
> > >> > > +      - description: GMAC1 RGMII RX
> > >> > > +      - description: External I2S TX bit clock
> > >> > > +      - description: External I2S TX left/right channel clock
> > >> > > +      - description: External I2S RX bit clock
> > >> > > +      - description: External I2S RX left/right channel clock
> > >> > > +      - description: External TDM clock
> > >> > > +      - description: External audio master clock
> > >> > 
> > >> > So, from peeking at the clock driver & the dt - it looks like a bunch of
> > >> > these are not actually required?
> > >> 
> > >> These clocks are used as root clocks or optional parent clocks in clock tree.
> > >> Some of them are optional, but they are required if we want to describe the
> > >> complete clock tree of JH7110 SoC.
> > > 
> > > Perhaps I have a misunderstand of what required means. To me, required
> > > means "you must provide this clock for the SoC to operate in all
> > > configurations".
> > > Optional therefore would be for things that are needed only for some
> > > configurations and may be omitted if not required.
> > > 
> > > From your comment below, boards with a JH7110 may choose not to populate
> > > both external clock inputs to a mux. In that case, "dummy" clocks should
> > > not have to be provided in the DT of such boards to satisfy this binding
> > > which seems wrong to me..

I agree. We don't want there to be "dummy" clks in DT. It should never
be required.

> > 
> > Please see the picture of these external clocks in clock tree.
> > 
> > # mount -t debugfs none /mnt
> > # cat /mnt/clk/clk_summary
> >                                  enable  prepare  protect                                duty  hardware
> >    clock                          count    count    count        rate   accuracy phase  cycle    enable
> > -------------------------------------------------------------------------------------------------------
> >  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
> >  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
> >  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> >  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> >  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> >  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> >  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
> >     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
> >        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
> >  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
> >     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
> >        gmac1_tx                       0        0        0    50000000          0     0  50000         N
> >           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
> >  *osc*                                  4        4        0    24000000          0     0  50000         Y
> >     apb_func                          0        0        0    24000000          0     0  50000         Y
> >  ...
> > 
> > The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
> > actually used as the parent of other clocks.
> 
> > The "dummy" clocks
> > you said are all internal clocks.
> 
> No, what I meant by "dummy" clocks is that if you make clocks "required"
> in the binding that are not needed by the hardware for operation a
> customer of yours might have to add "dummy" clocks to their devicetree
> to pass dtbs_check.

They can set the phandle specifier to '<0>' to fill in the required
property when there isn't anything there. If this is inside an SoC, it
is always connected because silicon can't change after it is made
(unless this is an FPGA). Therefore, any and all input clocks should be
listed as required. If the clk controller has inputs that are
pads/balls/pins on the SoC then they can be optional if a valid design
can leave those pins not connected.

Conor Dooley Feb. 21, 2023, 11:39 p.m. UTC | #19

On Tue, Feb 21, 2023 at 02:17:17PM -0800, Stephen Boyd wrote:
> Quoting Conor Dooley (2023-02-16 10:20:34)
> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
> > > On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
> > > > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
> > > Please see the picture of these external clocks in clock tree.
> > > 
> > > # mount -t debugfs none /mnt
> > > # cat /mnt/clk/clk_summary
> > >                                  enable  prepare  protect                                duty  hardware
> > >    clock                          count    count    count        rate   accuracy phase  cycle    enable
> > > -------------------------------------------------------------------------------------------------------
> > >  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
> > >  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
> > >  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> > >  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> > >  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> > >  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> > >  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
> > >     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
> > >        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
> > >  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
> > >     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
> > >        gmac1_tx                       0        0        0    50000000          0     0  50000         N
> > >           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
> > >  *osc*                                  4        4        0    24000000          0     0  50000         Y
> > >     apb_func                          0        0        0    24000000          0     0  50000         Y
> > >  ...
> > > 
> > > The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
> > > actually used as the parent of other clocks.
> > 
> > > The "dummy" clocks
> > > you said are all internal clocks.
> > 
> > No, what I meant by "dummy" clocks is that if you make clocks "required"
> > in the binding that are not needed by the hardware for operation a
> > customer of yours might have to add "dummy" clocks to their devicetree
> > to pass dtbs_check.
> 
> They can set the phandle specifier to '<0>' to fill in the required
> property when there isn't anything there. If this is inside an SoC, it
> is always connected because silicon can't change after it is made
> (unless this is an FPGA). Therefore, any and all input clocks should be
> listed as required.

> If the clk controller has inputs that are
> pads/balls/pins on the SoC then they can be optional if a valid design
> can leave those pins not connected.

From the discussion on the dts patches, where the clocks have been put
(intentionally) into board.dts, I've been under the impression that we
are in this situation.
Up to Hal to tell us if the hardware is capable of having those inputs
left unfilled!

FWIW, there's a v4 [1] of this series - but the question has yet to be
resolved.

Thanks,
Conor.

1 - https://lore.kernel.org/all/[e-mail]/

Hal Feng Feb. 22, 2023, 1:27 p.m. UTC | #20

On Tue, 21 Feb 2023 23:39:32 +0000, Conor Dooley wrote:
> On Tue, Feb 21, 2023 at 02:17:17PM -0800, Stephen Boyd wrote:
>> Quoting Conor Dooley (2023-02-16 10:20:34)
>> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
>> > > On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
>> > > > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
>> > > Please see the picture of these external clocks in clock tree.
>> > > 
>> > > # mount -t debugfs none /mnt
>> > > # cat /mnt/clk/clk_summary
>> > >                                  enable  prepare  protect                                duty  hardware
>> > >    clock                          count    count    count        rate   accuracy phase  cycle    enable
>> > > -------------------------------------------------------------------------------------------------------
>> > >  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
>> > >  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
>> > >  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>> > >  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>> > >  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>> > >  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>> > >  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
>> > >     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
>> > >        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
>> > >  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
>> > >     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
>> > >        gmac1_tx                       0        0        0    50000000          0     0  50000         N
>> > >           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
>> > >  *osc*                                  4        4        0    24000000          0     0  50000         Y
>> > >     apb_func                          0        0        0    24000000          0     0  50000         Y
>> > >  ...
>> > > 
>> > > The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
>> > > actually used as the parent of other clocks.
>> > 
>> > > The "dummy" clocks
>> > > you said are all internal clocks.
>> > 
>> > No, what I meant by "dummy" clocks is that if you make clocks "required"
>> > in the binding that are not needed by the hardware for operation a
>> > customer of yours might have to add "dummy" clocks to their devicetree
>> > to pass dtbs_check.
>> 
>> They can set the phandle specifier to '<0>' to fill in the required
>> property when there isn't anything there. If this is inside an SoC, it
>> is always connected because silicon can't change after it is made
>> (unless this is an FPGA). Therefore, any and all input clocks should be
>> listed as required.
> 
>> If the clk controller has inputs that are
>> pads/balls/pins on the SoC then they can be optional if a valid design
>> can leave those pins not connected.
> 
> From the discussion on the dts patches, where the clocks have been put
> (intentionally) into board.dts, I've been under the impression that we
> are in this situation.

For the system (sys) clock controller, we are in this situation.
For the always-on (aon) clock controller, we are not, because some input
clocks are inside the SoC.

> Up to Hal to tell us if the hardware is capable of having those inputs
> left unfilled!

The situation is different for v1.2A and v1.3B boards.

For the v1.2A board,
gmac1 only requires "gmac1_rmii_refin", which support 100MHz
gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz

For the v1.3B board,
gmac1 only requires "gmac1_rgmii_rxin", which support 1000MHz
gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz

So we should set the "required" property depending on different
boards.

Best regards,
Hal

> 
> FWIW, there's a v4 [1] of this series - but the question has yet to be
> resolved.
> 
> Thanks,
> Conor.
> 
> 1 - https://lore.kernel.org/all/[e-mail]/

Conor Dooley Feb. 22, 2023, 4:26 p.m. UTC | #21

On Wed, Feb 22, 2023 at 09:27:37PM +0800, Hal Feng wrote:
> On Tue, 21 Feb 2023 23:39:32 +0000, Conor Dooley wrote:
> > On Tue, Feb 21, 2023 at 02:17:17PM -0800, Stephen Boyd wrote:
> >> Quoting Conor Dooley (2023-02-16 10:20:34)
> >> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
> >> > > On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
> >> > > > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
> >> > > Please see the picture of these external clocks in clock tree.
> >> > > 
> >> > > # mount -t debugfs none /mnt
> >> > > # cat /mnt/clk/clk_summary
> >> > >                                  enable  prepare  protect                                duty  hardware
> >> > >    clock                          count    count    count        rate   accuracy phase  cycle    enable
> >> > > -------------------------------------------------------------------------------------------------------
> >> > >  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
> >> > >  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
> >> > >  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> >> > >  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> >> > >  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
> >> > >  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
> >> > >  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
> >> > >     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
> >> > >        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
> >> > >  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
> >> > >     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
> >> > >        gmac1_tx                       0        0        0    50000000          0     0  50000         N
> >> > >           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
> >> > >  *osc*                                  4        4        0    24000000          0     0  50000         Y
> >> > >     apb_func                          0        0        0    24000000          0     0  50000         Y
> >> > >  ...
> >> > > 
> >> > > The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
> >> > > actually used as the parent of other clocks.
> >> > 
> >> > > The "dummy" clocks
> >> > > you said are all internal clocks.
> >> > 
> >> > No, what I meant by "dummy" clocks is that if you make clocks "required"
> >> > in the binding that are not needed by the hardware for operation a
> >> > customer of yours might have to add "dummy" clocks to their devicetree
> >> > to pass dtbs_check.
> >> 
> >> They can set the phandle specifier to '<0>' to fill in the required
> >> property when there isn't anything there. If this is inside an SoC, it
> >> is always connected because silicon can't change after it is made
> >> (unless this is an FPGA). Therefore, any and all input clocks should be
> >> listed as required.
> > 
> >> If the clk controller has inputs that are
> >> pads/balls/pins on the SoC then they can be optional if a valid design
> >> can leave those pins not connected.
> > 
> > From the discussion on the dts patches, where the clocks have been put
> > (intentionally) into board.dts, I've been under the impression that we
> > are in this situation.
> 
> For the system (sys) clock controller, we are in this situation.
> For the always-on (aon) clock controller, we are not, because some input
> clocks are inside the SoC.
> 
> > Up to Hal to tell us if the hardware is capable of having those inputs
> > left unfilled!
> 
> The situation is different for v1.2A and v1.3B boards.
> 
> For the v1.2A board,
> gmac1 only requires "gmac1_rmii_refin", which support 100MHz
> gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz
> 
> For the v1.3B board,
> gmac1 only requires "gmac1_rgmii_rxin", which support 1000MHz
> gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz
> 
> So we should set the "required" property depending on different
> boards.

These were Krzk's suggestions:
oneOf:
 - clock-names:
     minItems: 3
     items:
       - a
       - b
       - c
       - d
 - clock-names:
     items:
       - a
       - b
       - d

or maybe:
 - clock-names:
     minItems: 3
     items:
       - a
       - b
       - enum: [c, d]
       - d

Might be making a mess here, but I think that becomes:
  clock-names:
    oneOf:
      - items:
          - const: osc
          - enum:
              - gmac1_rmii_refin
              - gmac1_rgmii_rxin
          - const: i2stx_bclk_ext
          - const: i2stx_lrck_ext
          - const: i2srx_bclk_ext
          - const: i2srx_lrck_ext
          - const: tdm_ext
          - const: mclk_ext

      - items:
          - const: osc
          - const: gmac1_rmii_refin
          - const: gmac1_rgmii_rxin
          - const: i2stx_bclk_ext
          - const: i2stx_lrck_ext
          - const: i2srx_bclk_ext
          - const: i2srx_lrck_ext
          - const: tdm_ext
          - const: mclk_ext

Cheers,
Conor.

Hal Feng Feb. 23, 2023, 3:03 a.m. UTC | #22

On Wed, 22 Feb 2023 16:26:46 +0000, Conor Dooley wrote:
> On Wed, Feb 22, 2023 at 09:27:37PM +0800, Hal Feng wrote:
>> On Tue, 21 Feb 2023 23:39:32 +0000, Conor Dooley wrote:
>> > On Tue, Feb 21, 2023 at 02:17:17PM -0800, Stephen Boyd wrote:
>> >> Quoting Conor Dooley (2023-02-16 10:20:34)
>> >> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
>> >> > > On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
>> >> > > > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
>> >> > > Please see the picture of these external clocks in clock tree.
>> >> > > 
>> >> > > # mount -t debugfs none /mnt
>> >> > > # cat /mnt/clk/clk_summary
>> >> > >                                  enable  prepare  protect                                duty  hardware
>> >> > >    clock                          count    count    count        rate   accuracy phase  cycle    enable
>> >> > > -------------------------------------------------------------------------------------------------------
>> >> > >  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
>> >> > >  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
>> >> > >  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>> >> > >  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>> >> > >  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>> >> > >  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>> >> > >  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
>> >> > >     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
>> >> > >        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
>> >> > >  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
>> >> > >     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
>> >> > >        gmac1_tx                       0        0        0    50000000          0     0  50000         N
>> >> > >           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
>> >> > >  *osc*                                  4        4        0    24000000          0     0  50000         Y
>> >> > >     apb_func                          0        0        0    24000000          0     0  50000         Y
>> >> > >  ...
>> >> > > 
>> >> > > The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
>> >> > > actually used as the parent of other clocks.
>> >> > 
>> >> > > The "dummy" clocks
>> >> > > you said are all internal clocks.
>> >> > 
>> >> > No, what I meant by "dummy" clocks is that if you make clocks "required"
>> >> > in the binding that are not needed by the hardware for operation a
>> >> > customer of yours might have to add "dummy" clocks to their devicetree
>> >> > to pass dtbs_check.
>> >> 
>> >> They can set the phandle specifier to '<0>' to fill in the required
>> >> property when there isn't anything there. If this is inside an SoC, it
>> >> is always connected because silicon can't change after it is made
>> >> (unless this is an FPGA). Therefore, any and all input clocks should be
>> >> listed as required.
>> > 
>> >> If the clk controller has inputs that are
>> >> pads/balls/pins on the SoC then they can be optional if a valid design
>> >> can leave those pins not connected.
>> > 
>> > From the discussion on the dts patches, where the clocks have been put
>> > (intentionally) into board.dts, I've been under the impression that we
>> > are in this situation.
>> 
>> For the system (sys) clock controller, we are in this situation.
>> For the always-on (aon) clock controller, we are not, because some input
>> clocks are inside the SoC.
>> 
>> > Up to Hal to tell us if the hardware is capable of having those inputs
>> > left unfilled!
>> 
>> The situation is different for v1.2A and v1.3B boards.
>> 
>> For the v1.2A board,
>> gmac1 only requires "gmac1_rmii_refin", which support 100MHz
>> gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz
>> 
>> For the v1.3B board,
>> gmac1 only requires "gmac1_rgmii_rxin", which support 1000MHz
>> gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz
>> 
>> So we should set the "required" property depending on different
>> boards.
> 
> These were Krzk's suggestions:
> oneOf:
>  - clock-names:
>      minItems: 3
>      items:
>        - a
>        - b
>        - c
>        - d
>  - clock-names:
>      items:
>        - a
>        - b
>        - d
> 
> or maybe:
>  - clock-names:
>      minItems: 3
>      items:
>        - a
>        - b
>        - enum: [c, d]
>        - d
> 
> Might be making a mess here, but I think that becomes:
>   clock-names:
>     oneOf:
>       - items:
>           - const: osc
>           - enum:
>               - gmac1_rmii_refin
>               - gmac1_rgmii_rxin
>           - const: i2stx_bclk_ext
>           - const: i2stx_lrck_ext
>           - const: i2srx_bclk_ext
>           - const: i2srx_lrck_ext
>           - const: tdm_ext
>           - const: mclk_ext
> 
>       - items:
>           - const: osc
>           - const: gmac1_rmii_refin
>           - const: gmac1_rgmii_rxin
>           - const: i2stx_bclk_ext
>           - const: i2stx_lrck_ext
>           - const: i2srx_bclk_ext
>           - const: i2srx_lrck_ext
>           - const: tdm_ext
>           - const: mclk_ext

Will modify it and improve the description of clock items for
pointing out which clock is required on different boards.
Thank you all for your helpful suggestions.

Best regards,
Hal

Conor Dooley Feb. 23, 2023, 6:18 a.m. UTC | #23

On 23 February 2023 03:03:04 GMT, Hal Feng <[e-mail]> wrote:
>On Wed, 22 Feb 2023 16:26:46 +0000, Conor Dooley wrote:
>> On Wed, Feb 22, 2023 at 09:27:37PM +0800, Hal Feng wrote:
>>> On Tue, 21 Feb 2023 23:39:32 +0000, Conor Dooley wrote:
>>> > On Tue, Feb 21, 2023 at 02:17:17PM -0800, Stephen Boyd wrote:
>>> >> Quoting Conor Dooley (2023-02-16 10:20:34)
>>> >> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
>>> >> > > On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
>>> >> > > > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
>>> >> > > Please see the picture of these external clocks in clock tree.
>>> >> > > 
>>> >> > > # mount -t debugfs none /mnt
>>> >> > > # cat /mnt/clk/clk_summary
>>> >> > >                                  enable  prepare  protect                                duty  hardware
>>> >> > >    clock                          count    count    count        rate   accuracy phase  cycle    enable
>>> >> > > -------------------------------------------------------------------------------------------------------
>>> >> > >  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
>>> >> > >  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
>>> >> > >  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>>> >> > >  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>>> >> > >  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>>> >> > >  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>>> >> > >  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
>>> >> > >     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
>>> >> > >        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
>>> >> > >  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
>>> >> > >     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
>>> >> > >        gmac1_tx                       0        0        0    50000000          0     0  50000         N
>>> >> > >           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
>>> >> > >  *osc*                                  4        4        0    24000000          0     0  50000         Y
>>> >> > >     apb_func                          0        0        0    24000000          0     0  50000         Y
>>> >> > >  ...
>>> >> > > 
>>> >> > > The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
>>> >> > > actually used as the parent of other clocks.
>>> >> > 
>>> >> > > The "dummy" clocks
>>> >> > > you said are all internal clocks.
>>> >> > 
>>> >> > No, what I meant by "dummy" clocks is that if you make clocks "required"
>>> >> > in the binding that are not needed by the hardware for operation a
>>> >> > customer of yours might have to add "dummy" clocks to their devicetree
>>> >> > to pass dtbs_check.
>>> >> 
>>> >> They can set the phandle specifier to '<0>' to fill in the required
>>> >> property when there isn't anything there. If this is inside an SoC, it
>>> >> is always connected because silicon can't change after it is made
>>> >> (unless this is an FPGA). Therefore, any and all input clocks should be
>>> >> listed as required.
>>> > 
>>> >> If the clk controller has inputs that are
>>> >> pads/balls/pins on the SoC then they can be optional if a valid design
>>> >> can leave those pins not connected.
>>> > 
>>> > From the discussion on the dts patches, where the clocks have been put
>>> > (intentionally) into board.dts, I've been under the impression that we
>>> > are in this situation.
>>> 
>>> For the system (sys) clock controller, we are in this situation.
>>> For the always-on (aon) clock controller, we are not, because some input
>>> clocks are inside the SoC.
>>> 
>>> > Up to Hal to tell us if the hardware is capable of having those inputs
>>> > left unfilled!
>>> 
>>> The situation is different for v1.2A and v1.3B boards.
>>> 
>>> For the v1.2A board,
>>> gmac1 only requires "gmac1_rmii_refin", which support 100MHz
>>> gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz
>>> 
>>> For the v1.3B board,
>>> gmac1 only requires "gmac1_rgmii_rxin", which support 1000MHz
>>> gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz
>>> 
>>> So we should set the "required" property depending on different
>>> boards.
>> 
>> These were Krzk's suggestions:
>> oneOf:
>>  - clock-names:
>>      minItems: 3
>>      items:
>>        - a
>>        - b
>>        - c
>>        - d
>>  - clock-names:
>>      items:
>>        - a
>>        - b
>>        - d
>> 
>> or maybe:
>>  - clock-names:
>>      minItems: 3
>>      items:
>>        - a
>>        - b
>>        - enum: [c, d]
>>        - d
>> 
>> Might be making a mess here, but I think that becomes:
>>   clock-names:
>>     oneOf:
>>       - items:
>>           - const: osc
>>           - enum:
>>               - gmac1_rmii_refin
>>               - gmac1_rgmii_rxin
>>           - const: i2stx_bclk_ext
>>           - const: i2stx_lrck_ext
>>           - const: i2srx_bclk_ext
>>           - const: i2srx_lrck_ext
>>           - const: tdm_ext
>>           - const: mclk_ext
>> 
>>       - items:
>>           - const: osc
>>           - const: gmac1_rmii_refin
>>           - const: gmac1_rgmii_rxin
>>           - const: i2stx_bclk_ext
>>           - const: i2stx_lrck_ext
>>           - const: i2srx_bclk_ext
>>           - const: i2srx_lrck_ext
>>           - const: tdm_ext
>>           - const: mclk_ext
>
>Will modify it and improve the description of clock items for
>pointing out which clock is required on different boards.

I don't think you need to mention the boards in it.

>Thank you all for your helpful suggestions.
>
>Best regards,
>Hal

Hal Feng Feb. 23, 2023, 9:52 a.m. UTC | #24

On Thu, 23 Feb 2023 06:18:01 +0000, Conor Dooley wrote:
> On 23 February 2023 03:03:04 GMT, Hal Feng <[e-mail]> wrote:
>>On Wed, 22 Feb 2023 16:26:46 +0000, Conor Dooley wrote:
>>> On Wed, Feb 22, 2023 at 09:27:37PM +0800, Hal Feng wrote:
>>>> On Tue, 21 Feb 2023 23:39:32 +0000, Conor Dooley wrote:
>>>> > On Tue, Feb 21, 2023 at 02:17:17PM -0800, Stephen Boyd wrote:
>>>> >> Quoting Conor Dooley (2023-02-16 10:20:34)
>>>> >> > On Thu, Feb 16, 2023 at 10:42:20PM +0800, Hal Feng wrote:
>>>> >> > > On Tue, 27 Dec 2022 20:15:20 +0000, Conor Dooley wrote:
>>>> >> > > > On Mon, Dec 26, 2022 at 12:26:32AM +0800, Hal Feng wrote:
>>>> >> > > Please see the picture of these external clocks in clock tree.
>>>> >> > > 
>>>> >> > > # mount -t debugfs none /mnt
>>>> >> > > # cat /mnt/clk/clk_summary
>>>> >> > >                                  enable  prepare  protect                                duty  hardware
>>>> >> > >    clock                          count    count    count        rate   accuracy phase  cycle    enable
>>>> >> > > -------------------------------------------------------------------------------------------------------
>>>> >> > >  *mclk_ext*                             0        0        0    12288000          0     0  50000         Y
>>>> >> > >  *tdm_ext*                              0        0        0    49152000          0     0  50000         Y
>>>> >> > >  *i2srx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>>>> >> > >  *i2srx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>>>> >> > >  *i2stx_lrck_ext*                       0        0        0      192000          0     0  50000         Y
>>>> >> > >  *i2stx_bclk_ext*                       0        0        0    12288000          0     0  50000         Y
>>>> >> > >  *gmac1_rgmii_rxin*                     0        0        0   125000000          0     0  50000         Y
>>>> >> > >     gmac1_rx                          0        0        0   125000000          0     0  50000         Y
>>>> >> > >        gmac1_rx_inv                   0        0        0   125000000          0   180  50000         Y
>>>> >> > >  *gmac1_rmii_refin*                     0        0        0    50000000          0     0  50000         Y
>>>> >> > >     gmac1_rmii_rtx                    0        0        0    50000000          0     0  50000         Y
>>>> >> > >        gmac1_tx                       0        0        0    50000000          0     0  50000         N
>>>> >> > >           gmac1_tx_inv                0        0        0    50000000          0   180  50000         Y
>>>> >> > >  *osc*                                  4        4        0    24000000          0     0  50000         Y
>>>> >> > >     apb_func                          0        0        0    24000000          0     0  50000         Y
>>>> >> > >  ...
>>>> >> > > 
>>>> >> > > The clock "gmac1_rgmii_rxin" and the clock "gmac1_rmii_refin" are
>>>> >> > > actually used as the parent of other clocks.
>>>> >> > 
>>>> >> > > The "dummy" clocks
>>>> >> > > you said are all internal clocks.
>>>> >> > 
>>>> >> > No, what I meant by "dummy" clocks is that if you make clocks "required"
>>>> >> > in the binding that are not needed by the hardware for operation a
>>>> >> > customer of yours might have to add "dummy" clocks to their devicetree
>>>> >> > to pass dtbs_check.
>>>> >> 
>>>> >> They can set the phandle specifier to '<0>' to fill in the required
>>>> >> property when there isn't anything there. If this is inside an SoC, it
>>>> >> is always connected because silicon can't change after it is made
>>>> >> (unless this is an FPGA). Therefore, any and all input clocks should be
>>>> >> listed as required.
>>>> > 
>>>> >> If the clk controller has inputs that are
>>>> >> pads/balls/pins on the SoC then they can be optional if a valid design
>>>> >> can leave those pins not connected.
>>>> > 
>>>> > From the discussion on the dts patches, where the clocks have been put
>>>> > (intentionally) into board.dts, I've been under the impression that we
>>>> > are in this situation.
>>>> 
>>>> For the system (sys) clock controller, we are in this situation.
>>>> For the always-on (aon) clock controller, we are not, because some input
>>>> clocks are inside the SoC.
>>>> 
>>>> > Up to Hal to tell us if the hardware is capable of having those inputs
>>>> > left unfilled!
>>>> 
>>>> The situation is different for v1.2A and v1.3B boards.
>>>> 
>>>> For the v1.2A board,
>>>> gmac1 only requires "gmac1_rmii_refin", which support 100MHz
>>>> gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz
>>>> 
>>>> For the v1.3B board,
>>>> gmac1 only requires "gmac1_rgmii_rxin", which support 1000MHz
>>>> gmac0 only requires "gmac0_rgmii_rxin", which support 1000MHz
>>>> 
>>>> So we should set the "required" property depending on different
>>>> boards.
>>> 
>>> These were Krzk's suggestions:
>>> oneOf:
>>>  - clock-names:
>>>      minItems: 3
>>>      items:
>>>        - a
>>>        - b
>>>        - c
>>>        - d
>>>  - clock-names:
>>>      items:
>>>        - a
>>>        - b
>>>        - d
>>> 
>>> or maybe:
>>>  - clock-names:
>>>      minItems: 3
>>>      items:
>>>        - a
>>>        - b
>>>        - enum: [c, d]
>>>        - d
>>> 
>>> Might be making a mess here, but I think that becomes:
>>>   clock-names:
>>>     oneOf:
>>>       - items:
>>>           - const: osc
>>>           - enum:
>>>               - gmac1_rmii_refin
>>>               - gmac1_rgmii_rxin
>>>           - const: i2stx_bclk_ext
>>>           - const: i2stx_lrck_ext
>>>           - const: i2srx_bclk_ext
>>>           - const: i2srx_lrck_ext
>>>           - const: tdm_ext
>>>           - const: mclk_ext
>>> 
>>>       - items:
>>>           - const: osc
>>>           - const: gmac1_rmii_refin
>>>           - const: gmac1_rgmii_rxin
>>>           - const: i2stx_bclk_ext
>>>           - const: i2stx_lrck_ext
>>>           - const: i2srx_bclk_ext
>>>           - const: i2srx_lrck_ext
>>>           - const: tdm_ext
>>>           - const: mclk_ext
>>
>>Will modify it and improve the description of clock items for
>>pointing out which clock is required on different boards.
> 
> I don't think you need to mention the boards in it.

Got it. Thanks.

Best regards,
Hal

> 
>>Thank you all for your helpful suggestions.

Patch

diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
new file mode 100644
index 000000000000..ec81504dcb27
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-syscrg.yaml
@@ -0,0 +1,80 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/starfive,jh7110-syscrg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 System Clock and Reset Generator
+
+maintainers:
+  - Emil Renner Berthing <[e-mail]>
+
+properties:
+  compatible:
+    const: starfive,jh7110-syscrg
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Main Oscillator (24 MHz)
+      - description: GMAC1 RMII reference
+      - description: GMAC1 RGMII RX
+      - description: External I2S TX bit clock
+      - description: External I2S TX left/right channel clock
+      - description: External I2S RX bit clock
+      - description: External I2S RX left/right channel clock
+      - description: External TDM clock
+      - description: External audio master clock
+
+  clock-names:
+    items:
+      - const: osc
+      - const: gmac1_rmii_refin
+      - const: gmac1_rgmii_rxin
+      - const: i2stx_bclk_ext
+      - const: i2stx_lrck_ext
+      - const: i2srx_bclk_ext
+      - const: i2srx_lrck_ext
+      - const: tdm_ext
+      - const: mclk_ext
+
+  '#clock-cells':
+    const: 1
+    description:
+      See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices.
+
+  '#reset-cells':
+    const: 1
+    description:
+      See <dt-bindings/reset/starfive,jh7110-crg.h> for valid indices.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+  - '#reset-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-controller@13020000 {
+        compatible = "starfive,jh7110-syscrg";
+        reg = <0x13020000 0x10000>;
+        clocks = <&osc>, <&gmac1_rmii_refin>,
+                 <&gmac1_rgmii_rxin>,
+                 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
+                 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
+                 <&tdm_ext>, <&mclk_ext>;
+        clock-names = "osc", "gmac1_rmii_refin",
+                      "gmac1_rgmii_rxin",
+                      "i2stx_bclk_ext", "i2stx_lrck_ext",
+                      "i2srx_bclk_ext", "i2srx_lrck_ext",
+                      "tdm_ext", "mclk_ext";
+        #clock-cells = <1>;
+        #reset-cells = <1>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index a2ce424b6986..7916f2fb7619 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19634,10 +19634,11 @@  F:	arch/riscv/boot/dts/starfive/
 
 STARFIVE JH71X0 CLOCK DRIVERS
 M:	Emil Renner Berthing <[e-mail]>
+M:	Hal Feng <[e-mail]>
 S:	Maintained
-F:	Documentation/devicetree/bindings/clock/starfive,jh7100-*.yaml
+F:	Documentation/devicetree/bindings/clock/starfive,jh71*.yaml
 F:	drivers/clk/starfive/clk-starfive-jh71*
-F:	include/dt-bindings/clock/starfive-jh7100*.h
+F:	include/dt-bindings/clock/starfive?jh71*.h
 
 STARFIVE JH7100 PINCTRL DRIVER
 M:	Emil Renner Berthing <[e-mail]>
@@ -19649,10 +19650,11 @@  F:	include/dt-bindings/pinctrl/pinctrl-starfive-jh7100.h
 
 STARFIVE JH71X0 RESET CONTROLLER DRIVERS
 M:	Emil Renner Berthing <[e-mail]>
+M:	Hal Feng <[e-mail]>
 S:	Maintained
 F:	Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
 F:	drivers/reset/starfive/reset-starfive-jh71*
-F:	include/dt-bindings/reset/starfive-jh7100.h
+F:	include/dt-bindings/reset/starfive?jh71*.h
 
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <[e-mail]>
diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h
new file mode 100644
index 000000000000..cda199084bcf
--- /dev/null
+++ b/include/dt-bindings/clock/starfive,jh7110-crg.h
@@ -0,0 +1,207 @@ 
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Copyright 2022 Emil Renner Berthing <[e-mail]>
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
+#define __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
+
+/* SYSCRG clocks */
+#define JH7110_SYSCLK_CPU_ROOT			0
+#define JH7110_SYSCLK_CPU_CORE			1
+#define JH7110_SYSCLK_CPU_BUS			2
+#define JH7110_SYSCLK_GPU_ROOT			3
+#define JH7110_SYSCLK_PERH_ROOT			4
+#define JH7110_SYSCLK_BUS_ROOT			5
+#define JH7110_SYSCLK_NOCSTG_BUS		6
+#define JH7110_SYSCLK_AXI_CFG0			7
+#define JH7110_SYSCLK_STG_AXIAHB		8
+#define JH7110_SYSCLK_AHB0			9
+#define JH7110_SYSCLK_AHB1			10
+#define JH7110_SYSCLK_APB_BUS			11
+#define JH7110_SYSCLK_APB0			12
+#define JH7110_SYSCLK_PLL0_DIV2			13
+#define JH7110_SYSCLK_PLL1_DIV2			14
+#define JH7110_SYSCLK_PLL2_DIV2			15
+#define JH7110_SYSCLK_AUDIO_ROOT		16
+#define JH7110_SYSCLK_MCLK_INNER		17
+#define JH7110_SYSCLK_MCLK			18
+#define JH7110_SYSCLK_MCLK_OUT			19
+#define JH7110_SYSCLK_ISP_2X			20
+#define JH7110_SYSCLK_ISP_AXI			21
+#define JH7110_SYSCLK_GCLK0			22
+#define JH7110_SYSCLK_GCLK1			23
+#define JH7110_SYSCLK_GCLK2			24
+#define JH7110_SYSCLK_CORE			25
+#define JH7110_SYSCLK_CORE1			26
+#define JH7110_SYSCLK_CORE2			27
+#define JH7110_SYSCLK_CORE3			28
+#define JH7110_SYSCLK_CORE4			29
+#define JH7110_SYSCLK_DEBUG			30
+#define JH7110_SYSCLK_RTC_TOGGLE		31
+#define JH7110_SYSCLK_TRACE0			32
+#define JH7110_SYSCLK_TRACE1			33
+#define JH7110_SYSCLK_TRACE2			34
+#define JH7110_SYSCLK_TRACE3			35
+#define JH7110_SYSCLK_TRACE4			36
+#define JH7110_SYSCLK_TRACE_COM			37
+#define JH7110_SYSCLK_NOC_BUS_CPU_AXI		38
+#define JH7110_SYSCLK_NOC_BUS_AXICFG0_AXI	39
+#define JH7110_SYSCLK_OSC_DIV2			40
+#define JH7110_SYSCLK_PLL1_DIV4			41
+#define JH7110_SYSCLK_PLL1_DIV8			42
+#define JH7110_SYSCLK_DDR_BUS			43
+#define JH7110_SYSCLK_DDR_AXI			44
+#define JH7110_SYSCLK_GPU_CORE			45
+#define JH7110_SYSCLK_GPU_CORE_CLK		46
+#define JH7110_SYSCLK_GPU_SYS_CLK		47
+#define JH7110_SYSCLK_GPU_APB			48
+#define JH7110_SYSCLK_GPU_RTC_TOGGLE		49
+#define JH7110_SYSCLK_NOC_BUS_GPU_AXI		50
+#define JH7110_SYSCLK_ISP_TOP_CORE		51
+#define JH7110_SYSCLK_ISP_TOP_AXI		52
+#define JH7110_SYSCLK_NOC_BUS_ISP_AXI		53
+#define JH7110_SYSCLK_HIFI4_CORE		54
+#define JH7110_SYSCLK_HIFI4_AXI			55
+#define JH7110_SYSCLK_AXI_CFG1_MAIN		56
+#define JH7110_SYSCLK_AXI_CFG1_AHB		57
+#define JH7110_SYSCLK_VOUT_SRC			58
+#define JH7110_SYSCLK_VOUT_AXI			59
+#define JH7110_SYSCLK_NOC_BUS_DISP_AXI		60
+#define JH7110_SYSCLK_VOUT_TOP_AHB		61
+#define JH7110_SYSCLK_VOUT_TOP_AXI		62
+#define JH7110_SYSCLK_VOUT_TOP_HDMITX0_MCLK	63
+#define JH7110_SYSCLK_VOUT_TOP_MIPIPHY_REF	64
+#define JH7110_SYSCLK_JPEGC_AXI			65
+#define JH7110_SYSCLK_CODAJ12_AXI		66
+#define JH7110_SYSCLK_CODAJ12_CORE		67
+#define JH7110_SYSCLK_CODAJ12_APB		68
+#define JH7110_SYSCLK_VDEC_AXI			69
+#define JH7110_SYSCLK_WAVE511_AXI		70
+#define JH7110_SYSCLK_WAVE511_BPU		71
+#define JH7110_SYSCLK_WAVE511_VCE		72
+#define JH7110_SYSCLK_WAVE511_APB		73
+#define JH7110_SYSCLK_VDEC_JPG			74
+#define JH7110_SYSCLK_VDEC_MAIN			75
+#define JH7110_SYSCLK_NOC_BUS_VDEC_AXI		76
+#define JH7110_SYSCLK_VENC_AXI			77
+#define JH7110_SYSCLK_WAVE420L_AXI		78
+#define JH7110_SYSCLK_WAVE420L_BPU		79
+#define JH7110_SYSCLK_WAVE420L_VCE		80
+#define JH7110_SYSCLK_WAVE420L_APB		81
+#define JH7110_SYSCLK_NOC_BUS_VENC_AXI		82
+#define JH7110_SYSCLK_AXI_CFG0_MAIN_DIV		83
+#define JH7110_SYSCLK_AXI_CFG0_MAIN		84
+#define JH7110_SYSCLK_AXI_CFG0_HIFI4		85
+#define JH7110_SYSCLK_AXIMEM2_AXI		86
+#define JH7110_SYSCLK_QSPI_AHB			87
+#define JH7110_SYSCLK_QSPI_APB			88
+#define JH7110_SYSCLK_QSPI_REF_SRC		89
+#define JH7110_SYSCLK_QSPI_REF			90
+#define JH7110_SYSCLK_SDIO0_AHB			91
+#define JH7110_SYSCLK_SDIO1_AHB			92
+#define JH7110_SYSCLK_SDIO0_SDCARD		93
+#define JH7110_SYSCLK_SDIO1_SDCARD		94
+#define JH7110_SYSCLK_USB_125M			95
+#define JH7110_SYSCLK_NOC_BUS_STG_AXI		96
+#define JH7110_SYSCLK_GMAC1_AHB			97
+#define JH7110_SYSCLK_GMAC1_AXI			98
+#define JH7110_SYSCLK_GMAC_SRC			99
+#define JH7110_SYSCLK_GMAC1_GTXCLK		100
+#define JH7110_SYSCLK_GMAC1_RMII_RTX		101
+#define JH7110_SYSCLK_GMAC1_PTP			102
+#define JH7110_SYSCLK_GMAC1_RX			103
+#define JH7110_SYSCLK_GMAC1_RX_INV		104
+#define JH7110_SYSCLK_GMAC1_TX			105
+#define JH7110_SYSCLK_GMAC1_TX_INV		106
+#define JH7110_SYSCLK_GMAC1_GTXC		107
+#define JH7110_SYSCLK_GMAC0_GTXCLK		108
+#define JH7110_SYSCLK_GMAC0_PTP			109
+#define JH7110_SYSCLK_GMAC_PHY			110
+#define JH7110_SYSCLK_GMAC0_GTXC		111
+#define JH7110_SYSCLK_IOMUX_APB			112
+#define JH7110_SYSCLK_MAILBOX_APB		113
+#define JH7110_SYSCLK_INT_CTRL_APB		114
+#define JH7110_SYSCLK_CAN0_APB			115
+#define JH7110_SYSCLK_CAN0_TIMER		116
+#define JH7110_SYSCLK_CAN0_CAN			117
+#define JH7110_SYSCLK_CAN1_APB			118
+#define JH7110_SYSCLK_CAN1_TIMER		119
+#define JH7110_SYSCLK_CAN1_CAN			120
+#define JH7110_SYSCLK_PWM_APB			121
+#define JH7110_SYSCLK_WDT_APB			122
+#define JH7110_SYSCLK_WDT_CORE			123
+#define JH7110_SYSCLK_TIMER_APB			124
+#define JH7110_SYSCLK_TIMER0			125
+#define JH7110_SYSCLK_TIMER1			126
+#define JH7110_SYSCLK_TIMER2			127
+#define JH7110_SYSCLK_TIMER3			128
+#define JH7110_SYSCLK_TEMP_APB			129
+#define JH7110_SYSCLK_TEMP_CORE			130
+#define JH7110_SYSCLK_SPI0_APB			131
+#define JH7110_SYSCLK_SPI1_APB			132
+#define JH7110_SYSCLK_SPI2_APB			133
+#define JH7110_SYSCLK_SPI3_APB			134
+#define JH7110_SYSCLK_SPI4_APB			135
+#define JH7110_SYSCLK_SPI5_APB			136
+#define JH7110_SYSCLK_SPI6_APB			137
+#define JH7110_SYSCLK_I2C0_APB			138
+#define JH7110_SYSCLK_I2C1_APB			139
+#define JH7110_SYSCLK_I2C2_APB			140
+#define JH7110_SYSCLK_I2C3_APB			141
+#define JH7110_SYSCLK_I2C4_APB			142
+#define JH7110_SYSCLK_I2C5_APB			143
+#define JH7110_SYSCLK_I2C6_APB			144
+#define JH7110_SYSCLK_UART0_APB			145
+#define JH7110_SYSCLK_UART0_CORE		146
+#define JH7110_SYSCLK_UART1_APB			147
+#define JH7110_SYSCLK_UART1_CORE		148
+#define JH7110_SYSCLK_UART2_APB			149
+#define JH7110_SYSCLK_UART2_CORE		150
+#define JH7110_SYSCLK_UART3_APB			151
+#define JH7110_SYSCLK_UART3_CORE		152
+#define JH7110_SYSCLK_UART4_APB			153
+#define JH7110_SYSCLK_UART4_CORE		154
+#define JH7110_SYSCLK_UART5_APB			155
+#define JH7110_SYSCLK_UART5_CORE		156
+#define JH7110_SYSCLK_PWMDAC_APB		157
+#define JH7110_SYSCLK_PWMDAC_CORE		158
+#define JH7110_SYSCLK_SPDIF_APB			159
+#define JH7110_SYSCLK_SPDIF_CORE		160
+#define JH7110_SYSCLK_I2STX0_APB		161
+#define JH7110_SYSCLK_I2STX0_BCLK_MST		162
+#define JH7110_SYSCLK_I2STX0_BCLK_MST_INV	163
+#define JH7110_SYSCLK_I2STX0_LRCK_MST		164
+#define JH7110_SYSCLK_I2STX0_BCLK		165
+#define JH7110_SYSCLK_I2STX0_BCLK_INV		166
+#define JH7110_SYSCLK_I2STX0_LRCK		167
+#define JH7110_SYSCLK_I2STX1_APB		168
+#define JH7110_SYSCLK_I2STX1_BCLK_MST		169
+#define JH7110_SYSCLK_I2STX1_BCLK_MST_INV	170
+#define JH7110_SYSCLK_I2STX1_LRCK_MST		171
+#define JH7110_SYSCLK_I2STX1_BCLK		172
+#define JH7110_SYSCLK_I2STX1_BCLK_INV		173
+#define JH7110_SYSCLK_I2STX1_LRCK		174
+#define JH7110_SYSCLK_I2SRX_APB			175
+#define JH7110_SYSCLK_I2SRX_BCLK_MST		176
+#define JH7110_SYSCLK_I2SRX_BCLK_MST_INV	177
+#define JH7110_SYSCLK_I2SRX_LRCK_MST		178
+#define JH7110_SYSCLK_I2SRX_BCLK		179
+#define JH7110_SYSCLK_I2SRX_BCLK_INV		180
+#define JH7110_SYSCLK_I2SRX_LRCK		181
+#define JH7110_SYSCLK_PDM_DMIC			182
+#define JH7110_SYSCLK_PDM_APB			183
+#define JH7110_SYSCLK_TDM_AHB			184
+#define JH7110_SYSCLK_TDM_APB			185
+#define JH7110_SYSCLK_TDM_INTERNAL		186
+#define JH7110_SYSCLK_TDM_TDM			187
+#define JH7110_SYSCLK_TDM_TDM_INV		188
+#define JH7110_SYSCLK_JTAG_CERTIFICATION_TRNG	189
+
+#define JH7110_SYSCLK_PLL0_OUT			190
+#define JH7110_SYSCLK_PLL1_OUT			191
+#define JH7110_SYSCLK_PLL2_OUT			192
+
+#define JH7110_SYSCLK_END			193
+
+#endif /* __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__ */
diff --git a/include/dt-bindings/reset/starfive,jh7110-crg.h b/include/dt-bindings/reset/starfive,jh7110-crg.h
new file mode 100644
index 000000000000..b88216a4fe40
--- /dev/null
+++ b/include/dt-bindings/reset/starfive,jh7110-crg.h
@@ -0,0 +1,142 @@ 
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Copyright (C) 2022 Emil Renner Berthing <[e-mail]>
+ */
+
+#ifndef __DT_BINDINGS_RESET_STARFIVE_JH7110_CRG_H__
+#define __DT_BINDINGS_RESET_STARFIVE_JH7110_CRG_H__
+
+/* SYSCRG resets */
+#define JH7110_SYSRST_JTAG_APB			0
+#define JH7110_SYSRST_SYSCON_APB		1
+#define JH7110_SYSRST_IOMUX_APB			2
+#define JH7110_SYSRST_BUS			3
+#define JH7110_SYSRST_DEBUG			4
+#define JH7110_SYSRST_CORE0			5
+#define JH7110_SYSRST_CORE1			6
+#define JH7110_SYSRST_CORE2			7
+#define JH7110_SYSRST_CORE3			8
+#define JH7110_SYSRST_CORE4			9
+#define JH7110_SYSRST_CORE0_ST			10
+#define JH7110_SYSRST_CORE1_ST			11
+#define JH7110_SYSRST_CORE2_ST			12
+#define JH7110_SYSRST_CORE3_ST			13
+#define JH7110_SYSRST_CORE4_ST			14
+#define JH7110_SYSRST_TRACE0			15
+#define JH7110_SYSRST_TRACE1			16
+#define JH7110_SYSRST_TRACE2			17
+#define JH7110_SYSRST_TRACE3			18
+#define JH7110_SYSRST_TRACE4			19
+#define JH7110_SYSRST_TRACE_COM			20
+#define JH7110_SYSRST_GPU_APB			21
+#define JH7110_SYSRST_GPU_DOMA			22
+#define JH7110_SYSRST_NOC_BUS_APB		23
+#define JH7110_SYSRST_NOC_BUS_AXICFG0_AXI	24
+#define JH7110_SYSRST_NOC_BUS_CPU_AXI		25
+#define JH7110_SYSRST_NOC_BUS_DISP_AXI		26
+#define JH7110_SYSRST_NOC_BUS_GPU_AXI		27
+#define JH7110_SYSRST_NOC_BUS_ISP_AXI		28
+#define JH7110_SYSRST_NOC_BUS_DDRC		29
+#define JH7110_SYSRST_NOC_BUS_STG_AXI		30
+#define JH7110_SYSRST_NOC_BUS_VDEC_AXI		31
+
+#define JH7110_SYSRST_NOC_BUS_VENC_AXI		32
+#define JH7110_SYSRST_AXI_CFG1_AHB		33
+#define JH7110_SYSRST_AXI_CFG1_MAIN		34
+#define JH7110_SYSRST_AXI_CFG0_MAIN		35
+#define JH7110_SYSRST_AXI_CFG0_MAIN_DIV		36
+#define JH7110_SYSRST_AXI_CFG0_HIFI4		37
+#define JH7110_SYSRST_DDR_AXI			38
+#define JH7110_SYSRST_DDR_OSC			39
+#define JH7110_SYSRST_DDR_APB			40
+#define JH7110_SYSRST_ISP_TOP			41
+#define JH7110_SYSRST_ISP_TOP_AXI		42
+#define JH7110_SYSRST_VOUT_TOP_SRC		43
+#define JH7110_SYSRST_CODAJ12_AXI		44
+#define JH7110_SYSRST_CODAJ12_CORE		45
+#define JH7110_SYSRST_CODAJ12_APB		46
+#define JH7110_SYSRST_WAVE511_AXI		47
+#define JH7110_SYSRST_WAVE511_BPU		48
+#define JH7110_SYSRST_WAVE511_VCE		49
+#define JH7110_SYSRST_WAVE511_APB		50
+#define JH7110_SYSRST_VDEC_JPG			51
+#define JH7110_SYSRST_VDEC_MAIN			52
+#define JH7110_SYSRST_AXIMEM0_AXI		53
+#define JH7110_SYSRST_WAVE420L_AXI		54
+#define JH7110_SYSRST_WAVE420L_BPU		55
+#define JH7110_SYSRST_WAVE420L_VCE		56
+#define JH7110_SYSRST_WAVE420L_APB		57
+#define JH7110_SYSRST_AXIMEM1_AXI		58
+#define JH7110_SYSRST_AXIMEM2_AXI		59
+#define JH7110_SYSRST_INTMEM			60
+#define JH7110_SYSRST_QSPI_AHB			61
+#define JH7110_SYSRST_QSPI_APB			62
+#define JH7110_SYSRST_QSPI_REF			63
+
+#define JH7110_SYSRST_SDIO0_AHB			64
+#define JH7110_SYSRST_SDIO1_AHB			65
+#define JH7110_SYSRST_GMAC1_AXI			66
+#define JH7110_SYSRST_GMAC1_AHB			67
+#define JH7110_SYSRST_MAILBOX_APB		68
+#define JH7110_SYSRST_SPI0_APB			69
+#define JH7110_SYSRST_SPI1_APB			70
+#define JH7110_SYSRST_SPI2_APB			71
+#define JH7110_SYSRST_SPI3_APB			72
+#define JH7110_SYSRST_SPI4_APB			73
+#define JH7110_SYSRST_SPI5_APB			74
+#define JH7110_SYSRST_SPI6_APB			75
+#define JH7110_SYSRST_I2C0_APB			76
+#define JH7110_SYSRST_I2C1_APB			77
+#define JH7110_SYSRST_I2C2_APB			78
+#define JH7110_SYSRST_I2C3_APB			79
+#define JH7110_SYSRST_I2C4_APB			80
+#define JH7110_SYSRST_I2C5_APB			81
+#define JH7110_SYSRST_I2C6_APB			82
+#define JH7110_SYSRST_UART0_APB			83
+#define JH7110_SYSRST_UART0_CORE		84
+#define JH7110_SYSRST_UART1_APB			85
+#define JH7110_SYSRST_UART1_CORE		86
+#define JH7110_SYSRST_UART2_APB			87
+#define JH7110_SYSRST_UART2_CORE		88
+#define JH7110_SYSRST_UART3_APB			89
+#define JH7110_SYSRST_UART3_CORE		90
+#define JH7110_SYSRST_UART4_APB			91
+#define JH7110_SYSRST_UART4_CORE		92
+#define JH7110_SYSRST_UART5_APB			93
+#define JH7110_SYSRST_UART5_CORE		94
+#define JH7110_SYSRST_SPDIF_APB			95
+
+#define JH7110_SYSRST_PWMDAC_APB		96
+#define JH7110_SYSRST_PDM_DMIC			97
+#define JH7110_SYSRST_PDM_APB			98
+#define JH7110_SYSRST_I2SRX_APB			99
+#define JH7110_SYSRST_I2SRX_BCLK		100
+#define JH7110_SYSRST_I2STX0_APB		101
+#define JH7110_SYSRST_I2STX0_BCLK		102
+#define JH7110_SYSRST_I2STX1_APB		103
+#define JH7110_SYSRST_I2STX1_BCLK		104
+#define JH7110_SYSRST_TDM_AHB			105
+#define JH7110_SYSRST_TDM_CORE			106
+#define JH7110_SYSRST_TDM_APB			107
+#define JH7110_SYSRST_PWM_APB			108
+#define JH7110_SYSRST_WDT_APB			109
+#define JH7110_SYSRST_WDT_CORE			110
+#define JH7110_SYSRST_CAN0_APB			111
+#define JH7110_SYSRST_CAN0_CORE			112
+#define JH7110_SYSRST_CAN0_TIMER		113
+#define JH7110_SYSRST_CAN1_APB			114
+#define JH7110_SYSRST_CAN1_CORE			115
+#define JH7110_SYSRST_CAN1_TIMER		116
+#define JH7110_SYSRST_TIMER_APB			117
+#define JH7110_SYSRST_TIMER0			118
+#define JH7110_SYSRST_TIMER1			119
+#define JH7110_SYSRST_TIMER2			120
+#define JH7110_SYSRST_TIMER3			121
+#define JH7110_SYSRST_INT_CTRL_APB		122
+#define JH7110_SYSRST_TEMP_APB			123
+#define JH7110_SYSRST_TEMP_CORE			124
+#define JH7110_SYSRST_JTAG_CERTIFICATION	125
+
+#define JH7110_SYSRST_END			126
+
+#endif /* __DT_BINDINGS_RESET_STARFIVE_JH7110_CRG_H__ */