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

  From: Emil Renner Berthing <kernel@esmil.dk>

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 <kernel@esmil.dk>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../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
  

On Tue, 20 Dec 2022 08:50:50 +0800, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@esmil.dk>
> 
> 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 <kernel@esmil.dk>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../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 <robh@kernel.org>
  

On Tue, Dec 20, 2022 at 08:50:50AM +0800, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@esmil.dk>
> 
> 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 <kernel@esmil.dk>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../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 <kernel@esmil.dk>
> +
> +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/20221220011247.35560-1-hal.feng@starfivetech.com/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.
  

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.
  

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 <kernel@esmil.dk>
> > 
> > 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 <kernel@esmil.dk>
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  .../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 <kernel@esmil.dk>
> > +
> > +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/20221220011247.35560-1-hal.feng@starfivetech.com/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
  

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 <kernel@esmil.dk>
> > > 
> > > 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 <kernel@esmil.dk>
> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>

> > > +  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/20221220011247.35560-1-hal.feng@starfivetech.com/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.
  

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 <kernel@esmil.dk>
>> > > 
>> > > 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 <kernel@esmil.dk>
>> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> 
>> > > +  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/20221220011247.35560-1-hal.feng@starfivetech.com/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
  

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 <kernel@esmil.dk>
> >> > > 
> >> > > 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 <kernel@esmil.dk>
> >> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > 
> >> > > +  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/20221220011247.35560-1-hal.feng@starfivetech.com/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 ;)
  

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 <kernel@esmil.dk>
>> >> > > 
>> >> > > 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 <kernel@esmil.dk>
>> >> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> > 
>> >> > > +  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/20221220011247.35560-1-hal.feng@starfivetech.com/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 ;)
>
  

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 <kernel@esmil.dk>
> >> >> > > 
> >> >> > > 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 <kernel@esmil.dk>
> >> >> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >> > 
> >> >> > > +  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/20221220011247.35560-1-hal.feng@starfivetech.com/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?
  

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 <kernel@esmil.dk>
>> >> >> > > 
>> >> >> > > 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 <kernel@esmil.dk>
>> >> >> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> >> > 
>> >> >> > > +  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/20221220011247.35560-1-hal.feng@starfivetech.com/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
  

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 <kernel@esmil.dk>
> >> >> >> > > 
> >> >> >> > > 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 <kernel@esmil.dk>
> >> >> >> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >> >> > 
> >> >> >> > > +  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/20221220011247.35560-1-hal.feng@starfivetech.com/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.
  

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
  

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.
  

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
  

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.
  

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
  

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.
  

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 <kernel@esmil.dk>
> > >> > > 
> > >> > > 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 <kernel@esmil.dk>
> > >> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > > 
> > >> > > +  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.
  

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/20230221024645.127922-1-hal.feng@starfivetech.com/
  

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/20230221024645.127922-1-hal.feng@starfivetech.com/
  

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.
  

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
  

On 23 February 2023 03:03:04 GMT, Hal Feng <hal.feng@starfivetech.com> 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
  

On Thu, 23 Feb 2023 06:18:01 +0000, Conor Dooley wrote:
> On 23 February 2023 03:03:04 GMT, Hal Feng <hal.feng@starfivetech.com> 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.