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

[Hal Feng]

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@xxxxxxxx>
>> >> >> > > 
>> >> >> > > 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@xxxxxxxx>
>> >> >> > > Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
>> >> > 
>> >> >> > > +  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@xxxxxxxxxxxxxxxx/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