Hi Stephen,
> -----Original Message-----
> From: Biju Das
> Sent: 25 March 2025 12:23
> Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for coupled clock
>
> Hi Stephen,
>
> > -----Original Message-----
> > From: Stephen Boyd <sboyd@xxxxxxxxxx>
> > Sent: 24 March 2025 23:48
> > Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for
> > coupled clock
> >
> > Quoting Biju Das (2025-03-21 07:21:24)
> > > > -----Original Message-----
> > > > From: Stephen Boyd <sboyd@xxxxxxxxxx>
> > > > > > > >
> > > > > > > > The parent clock rate of spi and spix2 are different. If
> > > > > > > > we use an intermediate parent clk, What clk rate the parent will use??
> > > > > > >
> > > > > > > Alright, got it. Does the consumer care about the difference
> > > > > > > between the two clks for the gating
> > > > > > part?
> > > > > >
> > > > > > Although gating bit is same, for some reason their monitor bit
> > > > > > is different. So, to confirm clk on status we need to check
> > > > > > respective monitor bits. Parallelly, I will check with
> > > > > > hardware team, does it need to monitor both these
> > > > bits??
> > > > >
> > > > > According to hardware team, the spix2 clock is twice the
> > > > > frequency of the spi clock, and the clock ON/OFF period
> > > > > displayed for each bit in the monitor register varies
> > > > slightly due to the difference in frequency.
> > > > >
> > > > > So to check the status after changing the clock ON/OFF register
> > > > > setting, please check the two monitor register bits together
> > > > >
> > > >
> > > > That answers the hardware side of the question. Why does software
> > > > need to care that they're two different things vs. one clk?
> > >
> > > From software point, Consumer driver bother only about spi_clk.
> > >
> > > So, treating as one clk(spi_clk) should be OK and we should drop
> > > handling spi_x2 module clk in the clk driver instead treat this as
> > > an internal clock (".spi_clk_x2")??
> > >
> > > Then we should update the binding to have only 3 module clks instead
> > > of 4 by dropping the spi_x2 module clk.
> >
> > I don't see why the binding has to be updated. Can't we return a NULL
> > clk pointer when the driver calls clk_get() on the specifier for the
> > spi_x2 clk?
>
> Same specifier is used for both clocks. So, we cannot distinguish from DT entries.
>
> > Then nothing will happen for that clk. I guess we may need to return
> > the rate of the spi clk multiplied by 2 or something, but that is far
> > simpler to implement than arbitrating the hardware with custom logic and meets the same result.
>
>
> Maybe, during registering, treat spix2 as an ignore clock and store its pointer(eg: sibling).
>
> During get(), return the stored pointer(by introducing new type CPG_MOD_COUPLED =2 in DT) for spix2.
> This needs binding modification.
>
> Now we have clk and clk->sibling for spi and spix2. With this we get proper rate.
>
> [ 2.927484] rzv2h-cpg 10420000.clock-controller: clock (1, 161) is spi_clk_spi at 20000000 Hz
> [ 2.945706] rzv2h-cpg 10420000.clock-controller: clock (2, 161) is spi_clk_spix2 at 40000000 Hz
>
> Enable/disable calls ignore for spix2. But it returns proper rate for both spi and spix2.
>
> Is this approach, ok?
>
Another option is handle this as core clock, currently core clock does not gate in this driver.
With this, clock specifier can distinguish both the clocks.
<&cpg CPG_MOD 0xa1>,
+ <&cpg CPG_CORE R9A09G047_SPI_CLK_SPIX2>;
Cheers,
Biju
