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?
Cheers,
Biju
